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
