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 " > >> + " \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; 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? Ciao, Dscho