Re: [PATCH v6 04/13] merge-one-file: rewrite in C

> Alban Gruin <alban.gruin@gmail.com> writes:
> 
>> +int merge_three_way(struct repository *r,
>> +		    const struct object_id *orig_blob,
>> +		    const struct object_id *our_blob,
>> +		    const struct object_id *their_blob, const char *path,
>> +		    unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode)
>> +{
>> +	if (orig_blob &&
>> +	    ((!their_blob && our_blob && oideq(orig_blob, our_blob)) ||
>> +	     (!our_blob && their_blob && oideq(orig_blob, their_blob)))) {
>> +		/* Deleted in both or deleted in one and unchanged in the other. */
>> +		return merge_one_file_deleted(r->index, our_blob, their_blob, path,
>> +					      orig_mode, our_mode, their_mode);
> 
> When both ours and theirs deleted, by definition orig_blob cannot be
> NULL, so "orig_blob &&" part would be true, but the other side that
> requires either (!their && our) or (!our && their) is true cannot be
> satisfied.  So it seems that the comment does not match the behaviour.
> 
> You'd need "(!their_blob && !our_blob) ||" in the second part?
> 

> This shows lack of test coverage, I think A manual test seems to
> trigger the "unhandled case" error you added:
> 
> $ make
> $ ./git-merge-one-file $(git rev-parse :COPYING) "" "" \
> 	COPYING \
> 	100644 "" ""
> error: COPYING: Not handling case 536e55524db72bd2acf175208aef4f3dfc148d42 ->  ->
> 

>> +	} else if (!orig_blob && our_blob && !their_blob) {
>> +		/*
>> +		 * Added in one.  The other side did not add and we
>> +		 * added so there is nothing to be done, except making
>> +		 * the path merged.
>> +		 */
> 
> This is not the sole "Added in one" case.  The next elseif arm also
> is added in one.
> 
> What is notable in this elseif arm is that this is "added in ours",
> which allows us (and forces us) not to touch the working tree with
> extra "checkout".  So either remove "Added in one" from here for
> symmetry with the next elseif arm, or better yet say "Added in
> ours".
> 
>> +		return add_to_index_cacheinfo(r->index, our_mode, our_blob,
>> +					      path, 0, 1, 1, NULL);
> 
> All callers to add_to_index_cacheinfo() uses 0, 1, 1 for stage,
> allow_add and allow_replace, except for the original.  The new
> callers you added should not have to keep repeating 0, 1, 1 like
> this caller does (see below).
> 
>> +	} else if (!orig_blob && !our_blob && their_blob) {
>> +		struct cache_entry *ce;
>> +		printf(_("Adding %s\n"), path);
>> +
>> +		if (file_exists(path))
>> +			return error(_("untracked %s is overwritten by the merge."), path);
>> +
>> +		if (add_to_index_cacheinfo(r->index, their_mode, their_blob,
>> +					   path, 0, 1, 1, &ce))
>> +			return -1;
>> +		return checkout_from_index(r->index, path, ce);
> 
> "git grep -A4 -e add_to_index_cacheinfo" after applying all patches
> in the series shows us that the &ce parameter was added only to call
> checkout_from_index() using it.
> 
> I doubt add_to_index_cacheinfo() is the right interface for this
> series.  This caller (and all other callers in the series that calls
> add_to_index_cacheinfo(), followed by checkout_from_index()) rather
> wants to have a function (defined in <cache.h>):
> 
> 	extern int add_merge_result_to_index(struct index_state, *
> 			unsigned int mode,
>                         const struct object_id *oid,
> 			const char *path,
> 			int checkout);
> 
> with which the last 4 lines of the above hunk can just become
> 
> 		return add_merge_result_to_index(r->index,
> 			their_mode, their_blob, path, 1);
> 
> I would think.  The earlier caller to add_to_index_cacheinfo() for
> "ours is the result" can pass 0 to the checkout parameter so the
> helper won't make a call to checkout_from_index().
> 
> And the step to add that helper would be in this patch (it could be
> after the previous step and before this step, but it is probably
> easier to understand if the new helper is introduced with its
> callers).
> 
> If we were to do that, then I do not mind the repetition of 0, 1, 1
> too much.
>