Re: [PATCH v1 5/8] sequencer: don't die in print_commit_summary()

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 7, 2017, 10:24 UTC
Message-ID: <alpine.DEB.2.21.1.1711071101050.6482@virtualbox>
In-Reply-To: <xmqq8tfig1rr.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Tue, 7 Nov 2017, Junio C Hamano wrote:

Show 15 quoted lines

> Phillip Wood <phillip.wood@talktalk.net> writes:
> 
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > 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 <phillip.wood@dunelm.org.uk>
> > ---
> >...
> > @@ -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).

Show 7 quoted lines

> 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