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.
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
