Hi Johannes,

Le 22/03/2021 à 23:20, Johannes Schindelin a écrit :

Hi Alban,

On Wed, 17 Mar 2021, Alban Gruin wrote:

quoted

@@ -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:

quoted

 	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