Hi Johannes,
Le 22/03/2021 à 23:20, Johannes Schindelin a écrit :
> Hi Alban,
>
> On Wed, 17 Mar 2021, Alban Gruin wrote:
>
>> @@ -69,10 +69,13 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix)
>>
>> if (skip_prefix(pgm, "--use=", &use_internal)) {
>> if (!strcmp(use_internal, "merge-one-file"))
>> - pgm = "git-merge-one-file";
>> + merge_action = merge_one_file_func;
>> else
>> die(_("git merge-index: unknown internal program %s"), use_internal);
>> - }
>> +
>> + repo_hold_locked_index(r, &lock, LOCK_DIE_ON_ERROR);
>> + } else
>> + merge_action = merge_one_file_spawn;
>
> I would have a slight preference to keep the default initializer, because
> that makes it easer to reason about. But if you _want_ to keep this patch
> as-is, I won't object.
>
Yeah, not sure why I did this. I'll change this.
> It is a bit sad that the conversion cannot be done more incrementally, as
> there is a lot to unpack in the many different cases that are handled. It
> looks correct, though.
>
> Just one thing:
>
>>
>> for (; i < argc; i++) {
>> const char *arg = argv[i];
>> diff --git a/builtin/merge-one-file.c b/builtin/merge-one-file.c
>> new file mode 100644
>> index 0000000000..ad99c6dbd4
>> --- /dev/null
>> +++ b/builtin/merge-one-file.c
>> @@ -0,0 +1,94 @@
>> +/*
>> + * Builtin "git merge-one-file"
>> + *
>> + * Copyright (c) 2020 Alban Gruin
>> + *
>> + * Based on git-merge-one-file.sh, written by Linus Torvalds.
>> + *
>> + * This is the git per-file merge utility, called with
>> + *
>> + * argv[1] - original file object name (or empty)
>> + * argv[2] - file in branch1 object name (or empty)
>> + * argv[3] - file in branch2 object name (or empty)
>> + * argv[4] - pathname in repository
>> + * argv[5] - original file mode (or empty)
>> + * argv[6] - file in branch1 mode (or empty)
>> + * argv[7] - file in branch2 mode (or empty)
>> + *
>> + * Handle some trivial cases. The _really_ trivial cases have been
>> + * handled already by git read-tree, but that one doesn't do any merges
>> + * that might change the tree layout.
>> + */
>> +
>> +#include "cache.h"
>> +#include "builtin.h"
>> +#include "lockfile.h"
>> +#include "merge-strategies.h"
>> +
>> +static const char builtin_merge_one_file_usage[] =
>> + "git merge-one-file <orig blob> <our blob> <their blob> <path> "
>> + "<orig mode> <our mode> <their mode>\n\n"
>> + "Blob ids and modes should be empty for missing files.";
>> +
>> +static int read_mode(const char *name, const char *arg, unsigned int *mode)
>> +{
>> + char *last;
>> + int ret = 0;
>> +
>> + *mode = strtol(arg, &last, 8);
>> +
>> + if (*last)
>> + ret = error(_("invalid '%s' mode: expected nothing, got '%c'"), name, *last);
>> + else if (!(S_ISREG(*mode) || S_ISDIR(*mode) || S_ISLNK(*mode)))
>> + ret = error(_("invalid '%s' mode: %o"), name, *mode);
>> +
>> + return ret;
>> +}
>> +
>> +int cmd_merge_one_file(int argc, const char **argv, const char *prefix)
>> +{
>> + struct object_id orig_blob, our_blob, their_blob,
>> + *p_orig_blob = NULL, *p_our_blob = NULL, *p_their_blob = NULL;
>> + unsigned int orig_mode = 0, our_mode = 0, their_mode = 0, ret = 0;
>> + struct lock_file lock = LOCK_INIT;
>> + struct repository *r = the_repository;
>> +
>> + if (argc != 8)
>> + usage(builtin_merge_one_file_usage);
>> +
>> + if (repo_read_index(r) < 0)
>> + die("invalid index");
>> +
>> + repo_hold_locked_index(r, &lock, LOCK_DIE_ON_ERROR);
>> +
>> + if (!get_oid_hex(argv[1], &orig_blob)) {
>> + p_orig_blob = &orig_blob;
>> + ret = read_mode("orig", argv[5], &orig_mode);
>> + } else if (!*argv[1] && *argv[5])
>> + ret = error(_("no 'orig' object id given, but a mode was still given."));
>
> Here, it looks as if the case of an empty `argv[1]` is not handled
> _explicitly_, but we rely on `get_oid_hex()` to return non-zero, and then
> we rely on the second arm _also_ not re-assigning `orig_blob`.
>
> I wonder whether this could be checked, and whether it would make sense to
> fold this, along with most of these 5 lines, into the `read_mode()` helper
> function (DRYing up the code even further).
>
Do you mean rewriting the first condition to read like this:
if (*argv[1] && !get_oid_hex(argv[1], &orig_blob)) {
?
In which case yes, I can do that.
BTW the two lasts calls to read_mode() should be like
err |= read_mode(…);
Cheers,
Alban
> As for the rest of the patch, it is totally possible that I missed a bug,
> but it looks correct to me, and the added regression tests give me a good
> feeling about the patch, too.
>
> Thanks,
> Dscho
>
