Re: [PATCH v4 06/16] merge_recursive: abort properly upon errors

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

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> There are a couple of places where return values indicating errors
> are ignored. Let's teach them manners.

That is because the return value never indicated errors before this
series, isn't it?  A true error used to be expressed by dying, and
the return value indicating "cleanliness" of the merge were
deliberately ignored.

The world order changed by previous patches in this series and the
callers need to be updated to take the new kind of return values
into account.  That is not teaching them manners ;-)

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  merge-recursive.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index dc3182b..2d4cb80 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1949,8 +1949,9 @@ int merge_recursive(struct merge_options *o,
>  		saved_b2 = o->branch2;
>  		o->branch1 = "Temporary merge branch 1";
>  		o->branch2 = "Temporary merge branch 2";
> -		merge_recursive(o, merged_common_ancestors, iter->item,
> -				NULL, &merged_common_ancestors);
> +		if (merge_recursive(o, merged_common_ancestors, iter->item,
> +				    NULL, &merged_common_ancestors) < 0)
> +			return -1;
>  		o->branch1 = saved_b1;
>  		o->branch2 = saved_b2;
>  		o->call_depth--;

This hunk feels somewhat wrong as-is.

There is a comment before the pre-context explaining why cleanness
flag is ignored.  It needs to be updated.  We still do not care
about cleanliness, i.e. 0=clean, 1=merged with conflict, but we now
can get negative values so we need to reject and return early if
this call indicates an error.

Thee other two hunks make sense.

Thanks.

> @@ -1966,6 +1967,8 @@ int merge_recursive(struct merge_options *o,
>  	o->ancestor = "merged common ancestors";
>  	clean = merge_trees(o, h1->tree, h2->tree, merged_common_ancestors->tree,
>  			    &mrtree);
> +	if (clean < 0)
> +		return clean;
>  
>  	if (o->call_depth) {
>  		*result = make_virtual_commit(mrtree, "merged tree");
> @@ -2022,6 +2025,9 @@ int merge_recursive_generic(struct merge_options *o,
>  	hold_locked_index(lock, 1);
>  	clean = merge_recursive(o, head_commit, next_commit, ca,
>  			result);
> +	if (clean < 0)
> +		return clean;
> +
>  	if (active_cache_changed &&
>  	    write_locked_index(&the_index, lock, COMMIT_LOCK))
>  		return error(_("Unable to write index."));