Hi Junio,
Thank you for your comments.
Le 22/12/2020 à 22:36, Junio C Hamano a écrit :
> Alban Gruin <alban.gruin@xxxxxxxxx> 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?
>
Yes, you're correct.
> 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 -> ->
>
Okay, I will add a test case for this.
>> + } 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.
>
Okay. Are we sure we want add_merge_result_to_index() inside
read-cache.c/cache.h?
Cheers,
Alban
