Re: [PATCH 3/9] Prepare the builtins for a libified merge_recursive()

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 1, 2016, 10:14 UTC
Message-ID: <alpine.DEB.2.20.1607011123550.12947@virtualbox>
In-Reply-To: <xmqqziq33ju2.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Wed, 29 Jun 2016, Junio C Hamano wrote:

Show 6 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > A truly libified function does not die() just for fun.
> 
> The sentence is wasting bits.  After all, a helper function in
> run-once-and-exit program does not die() just for fun, either.

Given that I had a hard time when reviewing Christian's apply patches to drive home the message that it is not okay for library functions to call die() or exit(), I happen to disagree with your notion that this sentence is wasting bits.

This sentence does not so much target *you* personally as audience, but the occasional reader of the log who wonders: "Why don't we just call die()? We would not have to worry about passing back the return value through all those long call chains..."

Show 17 quoted lines

> > As such, the recursive merge will convert all die() calls to return -1
> > instead in the next commits, giving the caller a chance at least to
> > print some helpful message.
> >
> > Let's prepare the builtins for this fatal error condition, even if we do
> > not really do more than imitating the previous die()'s exit(128): this is
> > what callers of e.g. `git merge` have come to expect.
> 
> One thing missing is your design decision and justification.
> 
> For example, the above explanation hints that the original code in
> this hunk expected merge_trees() to die() with some useful message
> when there is an error condition, but merge_trees() is going to be
> improved not to die() itself and return -1 instead to signal an
> error, so that the caller can react more flexibly, and this is a
> step to prepare for the version of merge_trees() that no longer
> dies.

Okay, I will replace the "As such..." paragraph with a modified version of your paraphrased explanation.

Show 13 quoted lines

> > -			merge_trees(&o, new->commit->tree, work,
> > +			ret = merge_trees(&o, new->commit->tree, work,
> >  				old->commit->tree, &result);
> > +			if (ret < 0)
> > +				exit(128);
> 
> The postimage of the patch tells us that the caller is now
> responsible for exiting with status 128, but neither the proposed
> log message nor the above hunk tells us where the message the
> original code must have given to the end user from die() inside
> merge_trees().  The updated caller just exits, so a natural guess is
> that the calls to die() have been changed to fprintf(stderr) with
> the patch.

Even more natural is it to guess that the code will call error(), just like we do almost everywhere else.

But you are right, I do not have to leave the reader guessing. Better to err on the side of being slightly verbose than to be so concise that nobody understands what I mean.

Show 17 quoted lines

> But that does not mesh very well with the stated objective of the
> patch.  The callers want flexibility to do their own error handling,
> including giving their own message, so letting merge_trees() to
> still write the same message to the standard error stream would not
> work well for them.  A caller may want to do merge_trees() just to
> see if it succeeds, without wanting to give _any_ indication of that
> is happening to the user, because it has an alternate/fallback code
> if merge_trees() fails, for example (analogy: "am -3" first tries a
> straight patch application before fallking back to 3-way merge; it
> may not want to show the error from the first attempt).
> 
> The reader _can_ guess that this step ignores the error-message
> issue, and improving it later (or keep ignoring that issue) might be
> OK in the context of this patch series, but it is necessary to be
> upfront to the readers what the design choices were and which one of
> those choices the proposed patch adopted as its design for them to
> be able to evaluate the patch series correctly.

To be honest, I did not even think about the error message issue because my primary concern is to teach the sequencer to perform rebase -i's grunt work. And while we usually suppress the output of the commands in rebase -i, we do show them in case of errors.

It will make things more complex, unfortunately, even if it will be straight-forward: there is already a strbuf and a flag in struct merge_options to collect output. The merge_options are simply not passed through to all of the previously die()ing functions yet.

I won't have time to get this implemented this week, unfortunately. So please do not expect the next iteration of this patch series before next week.

I could imagine that you wanted even more fine-grained control, where we have a range of return values indicating different error conditions. However, I already spent two weeks' worth of work to get this far, and would like to defer that task to the developer who will actually need these fine-grained return values (if we ever will need them).

Ciao, Dscho

P.S.: For the future, would you mind deleting the quoted remainder of my patches when there are no further comments? I deleted a footer of 73 unnecessary lines in this mail. It's no big deal if this is too tedious, but it would make my life easier.