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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

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

> 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).

> 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