Re: [PATCH 7/9] merge-recursive: handle return values indicating errors

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

Hi Junio,

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

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/merge-recursive.c b/merge-recursive.c
> > index 6ab7dfc..bb075e3 100644
> > --- a/merge-recursive.c
> > +++ b/merge-recursive.c
> > @@ -266,8 +266,10 @@ struct tree *write_tree_from_memory(struct merge_options *o)
> >  		active_cache_tree = cache_tree();
> >  
> >  	if (!cache_tree_fully_valid(active_cache_tree) &&
> > -	    cache_tree_update(&the_index, 0) < 0)
> > -		die(_("error building trees"));
> > +	    cache_tree_update(&the_index, 0) < 0) {
> > +		error(_("error building trees"));
> > +		return NULL;
> > +	}
> 
> This actually is a BUG(), isn't it?  We have already verified that
> the cache is merged, so cache_tree_update() ought to be able to come
> up with the whole-tree hash.

I am not so sure. What if the disk is full?

> > @@ -548,19 +550,17 @@ static int update_stages(const char *path, const struct diff_filespec *o,
> >  	 */
> >  	int clear = 1;
> >  	int options = ADD_CACHE_OK_TO_ADD | ADD_CACHE_SKIP_DFCHECK;
> > +	int ret = 0;
> > +
> >  	if (clear)
> > -		if (remove_file_from_cache(path))
> > -			return -1;
> > -	if (o)
> > -		if (add_cacheinfo(o->mode, o->sha1, path, 1, 0, options))
> > -			return -1;
> > -	if (a)
> > -		if (add_cacheinfo(a->mode, a->sha1, path, 2, 0, options))
> > -			return -1;
> > -	if (b)
> > -		if (add_cacheinfo(b->mode, b->sha1, path, 3, 0, options))
> > -			return -1;
> > -	return 0;
> > +		ret = remove_file_from_cache(path);
> > +	if (!ret && o)
> > +		ret = add_cacheinfo(o->mode, o->sha1, path, 1, 0, options);
> > +	if (!ret && a)
> > +		ret = add_cacheinfo(a->mode, a->sha1, path, 2, 0, options);
> > +	if (!ret && b)
> > +		ret = add_cacheinfo(b->mode, b->sha1, path, 3, 0, options);
> > +	return ret;
> >  }
> 
> Aren't the preimage and the postimage doing the same thing?  The
> only two differences I spot are (1) it is clear in the original that
> the returned value is -1 in the error case, even if the error
> convention of remove_file_from_cache() and add_cacheinfo() were "0
> is good, others are bad"; and (2) the control flow is easier to
> follow in the original.

Ah crap. This is a late left-over from the time when I thought I had to
introduce a "really fatal error" condition, i.e. return -128. I thought
that there might be errors that need to be handled differently, and
therefore it was essential that the return value -128 would not be
converted into a -1.

Will revert this hunk.

> > @@ -736,7 +736,7 @@ static int make_room_for_path(struct merge_options *o, const char *path)
> >  	return error(msg, path, _(": perhaps a D/F conflict?"));
> >  }
> >  
> > -static void update_file_flags(struct merge_options *o,
> > +static int update_file_flags(struct merge_options *o,
> >  			      const unsigned char *sha,
> >  			      unsigned mode,
> >  			      const char *path,
> > @@ -777,8 +777,7 @@ static void update_file_flags(struct merge_options *o,
> >  
> >  		if (make_room_for_path(o, path) < 0) {
> >  			update_wd = 0;
> > -			free(buf);
> > -			goto update_index;
> > +			goto free_buf;
> >  		}
> >  		if (S_ISREG(mode) || (!has_symlinks && S_ISLNK(mode))) {
> >  			int fd;
> > @@ -801,20 +800,22 @@ static void update_file_flags(struct merge_options *o,
> >  		} else
> >  			die(_("do not know what to do with %06o %s '%s'"),
> >  			    mode, sha1_to_hex(sha), path);
> > +free_buf:
> >  		free(buf);
> 
> I somehow find the above change harder to follow than the original.

Yeah, you noticed when commenting on the next patch why I did it. I added
a comment to the commit message.

> >  	}
> >   update_index:
> >  	if (update_cache)
> >  		add_cacheinfo(mode, sha, path, 0, update_wd, ADD_CACHE_OK_TO_ADD);
> > +	return 0;
> >  }
> >  
> 
> This one is in line with the stated goal of the patch.
> 
> > @@ -1028,21 +1030,23 @@ static void handle_change_delete(struct merge_options *o,
> >  		 * correct; since there is no true "middle point" between
> >  		 * them, simply reuse the base version for virtual merge base.
> >  		 */
> > -		remove_file_from_cache(path);
> > -		update_file(o, 0, o_sha, o_mode, renamed ? renamed : path);
> > +		ret = remove_file_from_cache(path);
> > +		if (!ret)
> > +			ret = update_file(o, 0, o_sha, o_mode,
> > +					  renamed ? renamed : path);
> 
> As you noted in the log message, this does change the behaviour.  If
> remove returns non-zero for whatever reason, we still did update()
> in the original, but we no longer do.  Does this have negative
> effect to the overall codeflow?

Please note that remove_file_from_index() does not, in fact, anything but
0 for the moment. So the practical use of testing the return value is nil
with Git's current source code.

It is more about future-proofing than anything else.

As to this incantation specifically, if we ever fail in
remove_file_from_cache(), I would expect the code to fail fast. That is
how I implemented it in this patch.

> > @@ -1087,21 +1094,22 @@ static void conflict_rename_delete(struct merge_options *o,
> >  		b_mode = dest->mode;
> >  	}
> >  
> > -	handle_change_delete(o,
> > +	ret = handle_change_delete(o,
> >  			     o->call_depth ? orig->path : dest->path,
> >  			     orig->sha1, orig->mode,
> >  			     a_sha, a_mode,
> >  			     b_sha, b_mode,
> >  			     _("rename"), _("renamed"));
> > -
> > -	if (o->call_depth) {
> > -		remove_file_from_cache(dest->path);
> > -	} else {
> > -		update_stages(dest->path, NULL,
> > +	if (ret < 0)
> > +		return ret;
> > +	if (o->call_depth)
> > +		ret = remove_file_from_cache(dest->path);
> > +	else
> > +		ret = update_stages(dest->path, NULL,
> >  			      rename_branch == o->branch1 ? dest : NULL,
> >  			      rename_branch == o->branch1 ? NULL : dest);
> > -	}
> 
> Similarly, if handle_change_delete() returns non-zero, we no longer
> call remove() or update().  Is that a good behaviour change?

Please note that handle_change_delete() did not return any value before
this patch series. In those cases that now return -1, it previously
die()d.

My patch ensures that the latter functions are not called in the error
case. Just like before.

> > -static void handle_file(struct merge_options *o,
> > +static int handle_file(struct merge_options *o,
> >  			struct diff_filespec *rename,
> >  			int stage,
> >  			struct rename_conflict_info *ci)
> 
> Likewise.

Ditto.

> > -static void conflict_rename_rename_1to2(struct merge_options *o,
> > +static int conflict_rename_rename_1to2(struct merge_options *o,
> >  					struct rename_conflict_info *ci)
> >  {
> > ...
> > -		if (merge_file_one(o, one->path,
> > +		if ((ret = merge_file_one(o, one->path,
> >  				 one->sha1, one->mode,
> >  				 a->sha1, a->mode,
> >  				 b->sha1, b->mode,
> > -				 ci->branch1, ci->branch2, &mfi) < 0)
> > -			return;
> > +				 ci->branch1, ci->branch2, &mfi)))
> > +			return ret;
> 
> This does not change behaviour.
> 
> > @@ -1194,7 +1208,8 @@ static void conflict_rename_rename_1to2(struct merge_options *o,
> >  		 * pathname and then either rename the add-source file to that
> >  		 * unique path, or use that unique path instead of src here.
> >  		 */
> > -		update_file(o, 0, mfi.sha, mfi.mode, one->path);
> > +		if ((ret = update_file(o, 0, mfi.sha, mfi.mode, one->path)))
> > +			return ret;
> 
> But this does.

As stated above: it does not. In case of an error in update_file() (which
is now reported via a return value, in contrast to the previous behavior),
just like before, we skip the rest of the function.

The difference is that we now clean up our resources and return an error
instead of die()ing.

> > @@ -1205,22 +1220,31 @@ static void conflict_rename_rename_1to2(struct merge_options *o,
> >  		 * resolving the conflict at that path in its favor.
> >  		 */
> >  		add = filespec_from_entry(&other, ci->dst_entry1, 2 ^ 1);
> > -		if (add)
> > -			update_file(o, 0, add->sha1, add->mode, a->path);
> > +		if (add) {
> > +			if ((ret = update_file(o, 0, add->sha1, add->mode,
> > +					a->path)))
> > +				return ret;
> > +		}
> 
> So does this.

Same as above (i.e. same behavior as previously).

> >  		else
> >  			remove_file_from_cache(a->path);
> >  		add = filespec_from_entry(&other, ci->dst_entry2, 3 ^ 1);
> > -		if (add)
> > -			update_file(o, 0, add->sha1, add->mode, b->path);
> > +		if (add) {
> > +			if ((ret = update_file(o, 0, add->sha1, add->mode,
> > +					b->path)))
> > +				return ret;
> > +		}
> 
> And this.

Yep. Same behavior.

> >  	} else {
> > -		handle_file(o, a, 2, ci);
> > -		handle_file(o, b, 3, ci);
> > +		if ((ret = handle_file(o, a, 2, ci)) ||
> > +		    (ret = handle_file(o, b, 3, ci)))
> > +			return ret;
> 
> And this.

Yep. Same behavior.

Ciao,
Dscho