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

[Junio C Hamano <gitster@pobox.com>]

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.

So what's more interesting to know for the readers?

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

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

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.

One design alternative we've seen used in our code to help callers
who want no default messages is to pass struct strbuf &err down the
callchain and collect the default messages without emitting them
directly to the standard error stream when an error is diagnosed.  I
do not know if that pattern is applicable or should be applied to
this codepath.

> Note that the callers of the sequencer (revert and cherry-pick) already
> fail fast even for the return value -1; The only difference is that they
> now get a chance to say "<command> failed".
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/checkout.c | 4 +++-
>  builtin/merge.c    | 4 ++++
>  sequencer.c        | 4 ++++
>  3 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index c3486bd..14312f7 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -567,8 +567,10 @@ static int merge_working_tree(const struct checkout_opts *opts,
>  			o.ancestor = old->name;
>  			o.branch1 = new->name;
>  			o.branch2 = "local";
> -			merge_trees(&o, new->commit->tree, work,
> +			ret = merge_trees(&o, new->commit->tree, work,
>  				old->commit->tree, &result);
> +			if (ret < 0)
> +				exit(128);
>  			ret = reset_tree(new->commit->tree, opts, 0,
>  					 writeout_error);
>  			if (ret)
> diff --git a/builtin/merge.c b/builtin/merge.c
> index b555a1b..133b853 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -682,6 +682,8 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
>  		hold_locked_index(&lock, 1);
>  		clean = merge_recursive(&o, head,
>  				remoteheads->item, reversed, &result);
> +		if (clean < 0)
> +			exit(128);
>  		if (active_cache_changed &&
>  		    write_locked_index(&the_index, &lock, COMMIT_LOCK))
>  			die (_("unable to write %s"), get_index_file());
> @@ -1550,6 +1552,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		ret = try_merge_strategy(use_strategies[i]->name,
>  					 common, remoteheads,
>  					 head_commit, head_arg);
> +		if (ret < 0)
> +			exit(128);
>  		if (!option_commit && !ret) {
>  			merge_was_ok = 1;
>  			/*
> diff --git a/sequencer.c b/sequencer.c
> index c6362d6..13b794a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -293,6 +293,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>  	clean = merge_trees(&o,
>  			    head_tree,
>  			    next_tree, base_tree, &result);
> +	if (clean < 0)
> +		return clean;
>  
>  	if (active_cache_changed &&
>  	    write_locked_index(&the_index, &index_lock, COMMIT_LOCK))
> @@ -561,6 +563,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>  	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) {
>  		res = do_recursive_merge(base, next, base_label, next_label,
>  					 head, &msgbuf, opts);
> +		if (res < 0)
> +			return res;
>  		write_message(&msgbuf, git_path_merge_msg());
>  	} else {
>  		struct commit_list *common = NULL;