Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle git push failure gracefully #56

Open
jonathanstrong opened this issue Apr 17, 2020 · 1 comment
Open

handle git push failure gracefully #56

jonathanstrong opened this issue Apr 17, 2020 · 1 comment
Assignees
Labels
C-bug Category: Bug P-medium Priority: Medium

Comments

@jonathanstrong
Copy link

Recently I had manually updated my crate index repo (to change the README). When I went to publish a crate, it appeared to succeed, but I couldn't get the new version via cargo update. What happened is, when alexandrie server went to push the crate index changes, but the push was "rejected" because the repo needed to pull my README changes first:

[master 4a58405] Updating crate `markets#0.3.1`                                 
 1 file changed, 1 insertion(+)                                                 
To git.jstrong.dev:jstrong/crate-index.git                                      
 ! [rejected]        master -> master (fetch first)                             
error: failed to push some refs to '[email protected]:jstrong/crate-index.git'
hint: Updates were rejected because the remote contains work that you do        
hint: not have locally. This is usually caused by another repository pushing    
hint: to the same ref. You may want to first integrate the remote changes       
hint: (e.g., 'git pull ...') before pushing again.                              
hint: See the 'Note about fast-forwards' in 'git push --help' for details.      

^ that's the error I found logged by alexandrie when I ssh'd to the server to figure it out.

This is not a huge problem, I imagine manual crate index repo changes are probably not a frequent occurrence. However, it does seem like it would be an improvement to try a git pull if the push fails, and see if that fixes things. Perhaps there's some reason I'm not thinking of why doing so on failure would be a problem, but if not, it might help someone avoid a confusing situation where the crate version seemingly was uploaded successfully to the registry, but isn't in the index.

Another approach would be to confirm the crate index changes have been pushed before cargo publish is permitted to succeed, and alexandrie's state about the crate is changed.

@Hirevo Hirevo self-assigned this Apr 25, 2020
@Hirevo Hirevo added C-bug Category: Bug P-medium Priority: Medium labels Apr 25, 2020
@Hirevo
Copy link
Owner

Hirevo commented Apr 25, 2020

Oh, yes indeed, good catch !
We're using std::process::Command in the command-line index to run the various git commands and we don't check their exit status (since the Command::output method returned a Result, I thought a non-zero exit status would return an Err(_), but no, I have just not read the docs carefully enough).
After having made a simple test (recreating a push error locally), git returns an non-zero exit code of 1 when this happens, so it would be quite easy to detect these (thankfully we don't need to inspect the output text).
So, yes, I agree that we can probably just attempt a git pull if we encounter this, and if the git push following it doesn't work (aka. there is a merge conflict), we can just emit an error saying that the index is now in an inconsistent state (and cargo publish allows us to display a custom error message, so the user can know about it).
I'll be doing this change when I can (there is currently a fair bit of substantial changes being made/discussed about how the index is managed within Alexandrie).
Thanks for filing this issue !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Bug P-medium Priority: Medium
Projects
None yet
Development

No branches or pull requests

2 participants