Re: [PATCH v1 5/8] sequencer: don't die in print_commit_summary()
On 10/11/17 18:05, Junio C Hamano wrote: > Phillip Wood writes: > >> On 07/11/17 15:13, Junio C Hamano wrote: >> ... >>> Another possibility perhaps is that the function is safe to reuse >>> already even without this patch, of course ;-). >>> >> Hmm, maybe it is. Looking at pick_commits() and do_pick_commit() if the >> sequencer dies in print_commit_summary() (which can only happen when >> cherry-picking or reverting) then neither the todo list or the abort >> safety file are updated to reflect the commit that was just made. >> >> As I understand it print_commit_summary() dies because: (i) it cannot >> resolve HEAD either because some other process is updating it (which is >> bad news in the middle of a cherry-pick); (ii) because something went >> wrong HEAD is corrupt; or (iii) log_tree_commit() cannot read some >> objects. In all those cases dying will leave the sequencer in a sane >> state for aborting - 'git cherry-pick --abort' will rewind HEAD to the >> last successful commit before there was a problem with HEAD or the >> object database. If the user somehow fixes the problem and runs 'git >> cherry-pick --continue' then the sequencer will try and pick the same >> commit again which may or may not be what the user wants depending on >> what caused print_commit_summary() to die. > > The above is all good analysis---thanks for your diligence. Perhaps > some if not all of it can go to the log message? > Thanks, that's a good idea. I see the above as a reason to drop this commit as returning an error will try and update the abort safety file which we don't want to do if there is a problem with HEAD so I'll add it to the previous commit to explain why it is okay to die.
Re: [PATCH v1 5/8] sequencer: don't die in print_commit_summary()
Phillip Wood writes: > On 07/11/17 15:13, Junio C Hamano wrote: > ... >> Another possibility perhaps is that the function is safe to reuse >> already even without this patch, of course ;-). >> > Hmm, maybe it is. Looking at pick_commits() and do_pick_commit() if the > sequencer dies in print_commit_summary() (which can only happen when > cherry-picking or reverting) then neither the todo list or the abort > safety file are updated to reflect the commit that was just made. > > As I understand it print_commit_summary() dies because: (i) it cannot > resolve HEAD either because some other process is updating it (which is > bad news in the middle of a cherry-pick); (ii) because something went > wrong HEAD is corrupt; or (iii) log_tree_commit() cannot read some > objects. In all those cases dying will leave the sequencer in a sane > state for aborting - 'git cherry-pick --abort' will rewind HEAD to the > last successful commit before there was a problem with HEAD or the > object database. If the user somehow fixes the problem and runs 'git > cherry-pick --continue' then the sequencer will try and pick the same > commit again which may or may not be what the user wants depending on > what caused print_commit_summary() to die. The above is all good analysis---thanks for your diligence. Perhaps some if not all of it can go to the log message?
Re: [PATCH v1 5/8] sequencer: don't die in print_commit_summary()
On 07/11/17 15:13, Junio C Hamano wrote: > Junio C Hamano writes: > >> And this step is going in the right direction, but I am not sure if >> this made the function safe enough to be called repeatedly from the >> rebase machinery and we are ready to unleash this to the end users >> and tell them it is safe to use it. > > Another possibility perhaps is that the function is safe to reuse > already even without this patch, of course ;-). > Hmm, maybe it is. Looking at pick_commits() and do_pick_commit() if the sequencer dies in print_commit_summary() (which can only happen when cherry-picking or reverting) then neither the todo list or the abort safety file are updated to reflect the commit that was just made. As I understand it print_commit_summary() dies because: (i) it cannot resolve HEAD either because some other process is updating it (which is bad news in the middle of a cherry-pick); (ii) because something went wrong HEAD is corrupt; or (iii) log_tree_commit() cannot read some objects. In all those cases dying will leave the sequencer in a sane state for aborting - 'git cherry-pick --abort' will rewind HEAD to the last successful commit before there was a problem with HEAD or the object database. If the user somehow fixes the problem and runs 'git cherry-pick --continue' then the sequencer will try and pick the same commit again which may or may not be what the user wants depending on what caused print_commit_summary() to die.
Re: [PATCH v1 5/8] sequencer: don't die in print_commit_summary()
Junio C Hamano writes: > And this step is going in the right direction, but I am not sure if > this made the function safe enough to be called repeatedly from the > rebase machinery and we are ready to unleash this to the end users > and tell them it is safe to use it. Another possibility perhaps is that the function is safe to reuse already even without this patch, of course ;-).
Re: [PATCH v1 5/8] sequencer: don't die in print_commit_summary()
Hi Junio, On Tue, 7 Nov 2017, Junio C Hamano wrote: > Phillip Wood writes: > > > From: Phillip Wood > > > > Return an error rather than dying so that the sequencer can exit > > cleanly once it starts committing without forking 'git commit' > > > > Signed-off-by: Phillip Wood > > --- > >... > > @@ -948,7 +951,9 @@ void print_commit_summary(const char *prefix, const > > struct object_id *oid, > > log_tree_commit(&rev, commit); > > } > > Is that call to log_tree_commit() a big elephant in the room? Or maybe not so big an elephant, given that we *already* use it in sequencer.c, in the `make_patch()` function. I did spend a substantial amount of time to try to libify all of those calls back when I worked on the rebase helper. Also, bisect.c (not builtin/bisect.c) seems to use that `log_tree_commit()` function, so it better be libified. Granted, those two callers are *pretty* close to the end of the process, `bisect_next_all()` is exit(10)-ing right after calling the caller of `log_tree_commit()` (*shudder* what were we thinking, allowing this completely unelegant and inconvenient pattern of exit()ing from libgit.a?), and `make_patch()` is what the sequencer uses to fake the patch that it never tried to apply (just to be consistent with non-interactive rebase) just before erroring out. And I am quite embarrassed for not having spotted the `maybe_flush_or_die()` call in `log_tree_commit()`. However, from a not-quite-as-quick-as-I-wanted look, it would appear that this call is the only hard `exit()` code path in `log_tree_commit()`, and it should be easily fixed. Please also note that this is our mess, not Phillip's, as we let these die()/exit() calls creep into libgit.a. It would be no fair to ask Phillip to clean it up for us. *We* let this slide for over a decade: 06f59e9f5da (Don't fflush(stdout) when it's not helpful, 2007-06-29). > It definitely is a good thing to eventually make a direct in-process > call into the commit machinery, and we should aim for that endgame. > > And this step is going in the right direction, but I am not sure if > this made the function safe enough to be called repeatedly from the > rebase machinery and we are ready to unleash this to the end users > and tell them it is safe to use it. Well, holding it up won't fix it faster, it will just delay the fix. I mean, this is Git, right? This is the same Git where we knowingly introduced a BUG() call into released versions via b1ef400eece (setup_git_env: avoid blind fall-back to ".git", 2016-10-20) that were *prone* to hit end users' use cases that we did not think about, just so we could fix those code paths. Sure, we tried to do our best to avoid having end users see those BUG reports, by investigating the code paths that eventually call `setup_git_env()`. We can just as easily do the same here: investigate as well as is reasonable the code paths that eventually call `die()` or `exit()` from `log_tree_commit()`, and then flesh out the remaining problems in production. Ciao, Dscho
Re: [PATCH v1 5/8] sequencer: don't die in print_commit_summary()
Phillip Wood writes: > From: Phillip Wood > > Return an error rather than dying so that the sequencer can exit > cleanly once it starts committing without forking 'git commit' > > Signed-off-by: Phillip Wood > --- >... > @@ -948,7 +951,9 @@ void print_commit_summary(const char *prefix, const > struct object_id *oid, > log_tree_commit(&rev, commit); > } Is that call to log_tree_commit() a big elephant in the room? All of the die() we see above you are making into error() are rather unlikely conditions (e.g. you created a commit, and try to look it up immediately after that, and you somehow fail to find it); log_tree_commit() makes tons more object accesses, any of which would be equally likely to fail and die. It definitely is a good thing to eventually make a direct in-process call into the commit machinery, and we should aim for that endgame. And this step is going in the right direction, but I am not sure if this made the function safe enough to be called repeatedly from the rebase machinery and we are ready to unleash this to the end users and tell them it is safe to use it.
