Re: [PATCH v7 08/15] merge-one-file: rewrite in C

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