From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 26 Jun 2020 14:33:31 GMT
Subject: Re: [RFC PATCH v1 02/17] merge-one-file: rewrite in C
Message-ID: <1e1dfe42-ab62-103a-e213-cfa42f5d9792@gmail.com>
In-Reply-To: <32ce88bd-14ce-0cd4-b938-285a95302cae@gmail.com>

On 25/06/2020 19:17, Phillip Wood wrote:
> On 25/06/2020 16:16, Phillip Wood wrote:
>> Hi Alban
>>
>> I think this series is a great idea
>>
>> On 25/06/2020 13:19, Alban Gruin wrote:
>>> This rewrites `git merge-one-file' from shell to C.  This port is very
>>> straightforward: it keeps using external processes to edit the index,
>>> for instance.  Errors are also displayed with fprintf() instead of
>>> error().  Both of these will be addressed in the next few commits,
>>> leading to its libification so its main function can be used from other
>>> commands directly.
>>>
>>> This also fixes a bug present in the original script: instead of
>>> checking if a _regular_ file exists when a file exists in the branch to
>>> merge, but not in our branch, the rewritten version checks if a file of
>>> any kind (ie. a directory, ...) exists.  This fixes the tests t6035.14,
>>> where the branch to merge had a new file, `a/b', but our branch had a
>>> directory there; it should have failed because a directory exists, but
>>> it did not because there was no regular file called `a/b'.  This test is
>>> now marked as successful.
>>> [...]
>>> +static int merge_one_file(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 &&
>>> +        ((our_blob && oideq(orig_blob, our_blob)) ||
>>> +         (their_blob && oideq(orig_blob, their_blob))))
>>> +        return merge_one_file_deleted(orig_blob, our_blob,
>>> their_blob, path,
>>> +                          orig_mode, our_mode, their_mode);
>>
>> It would be nice to preserve the comments from the script as I find they
>> help a lot in understanding which case each piece of code is handling.
>> The code above appears to be handling deletions but does not appear to
>> check that one side is actually missing. Shouldn't it be something like
>>
>> if (orig_blob &&
>>      ((!their_blob && (our_blob && oideq(orig_blob, our_blob))) ||
>>       (!our_blob && (their_blob && oideq(orig_blob, their_blob))))
>>
>> Maybe this could do with a test case
> 
> The reason your version works is that if only one side has changed
> read-tree will have done the merge itself so this only gets called if
> one side has been deleted. However the original script printed an error
> if someone accidentally called when the content had changed in only one
> side and there were no mode changes. I think we want to keep that behavior.

Actually I think the original probably handles this case by calling 'git 
merge-file'

Best Wishes

Phillip

> In the future we could probably update this to also handle the cases
> that read-tree normally takes care of rather than erroring out but I
> don't think it is a high priority.
> 
> Best Wishes
> 
> Phillip
>