Hi Johannes,
Le 24/03/2021 à 10:10, Johannes Schindelin a écrit :
> Hi Alban,
>
> On Tue, 23 Mar 2021, Alban Gruin wrote:
>
>> Le 22/03/2021 à 23:20, Johannes Schindelin a écrit :
>>>
>>> On Wed, 17 Mar 2021, Alban Gruin wrote:
>>>
>>>>
>>>> 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.
>
> Yes, that's what I meant. Or this instead:
>
> if (!*argv[1]) {
> if (*argv[5])
> ret = error(... mode was still given ...)
> } else if (!get_oid_hex(...)) {
> ...
> }
>
>> BTW the two lasts calls to read_mode() should be like
>>
>> err |= read_mode(…);
>
> While this is certainly shorter than
>
> if (read_mode(...))
> ret = -1;
>
So, I folded all of this into a single function that reads the mode,
convert the oid, and show an error if needed. Now, I have:
if (read_param("orig", argv[1], argv[5], &orig_blob,
&p_orig_blob, &orig_mode))
ret = -1;
if (read_param("our", …))
ret = -1;
if (read_param("their", …))
ret = -1;
if (ret)
return ret;
> I actually prefer the latter, for clarity (we do want `read_mode()` to be
> called, i.e. we cannot use `||=` here, but it is also not a bit-wise "or"
> operation, therefore `|=` strikes me as misleading). What do you think?
>
Yes, I think it's much clearer that way.
FIY, `||=' does not exist in C.
Cheers,
Alban
> Ciao,
> Dscho
>
