git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C
@ 2020-06-25 12:19 Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 01/17] t6027: modernise tests Alban Gruin
                   ` (16 more replies)
  0 siblings, 17 replies; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

In an effort to reduce the number of shell scripts part of git, I
propose this patch converting the two remaining merge strategies,
resolve and octopus, from shell to C.  This will enable slightly better
performance, better integration with git itself (no more forking to
perform these operations), better portability (Windows and shell scripts
don't mix well).

Three scripts are actually converted: first git-merge-one-file.sh, then
git-merge-resolve.sh, and finally git-merge-octopus.sh.  Not only they
are converted, but they also are modified to operate without forking,
and then libified so they can be used by git without spawning another
process.

The first patch is not important to make the whole series work, but I
made this patch while working on this series.

Patches 2-5 rewrite, clean, and libify `git merge-one-file', used by the
resolve and octopus strategies.

Patch 6 libifies `git merge-index', so the rewritten `git
merge-one-file' can be called without forking.

Patch 7-8-9 rewrite, clean, and libify `git merge-resolve'.

Patch 10 moves a function, better_branch_name(), that will prove itself
useful in the C version of `git merge-octopus', but that is not part of
libgit.a.

Patches 11-12-13 rewrite, clean, and libify `git merge-octopus'.

Patches 14-15-16-17 teach `git merge' and the sequencer to call the
strategies without forking.

This series keeps the commands `git merge-one-file', `git merge-resolve'
and `git merge-octopus', so any script depending on them should keep
working without any changes.

This series is based on c9c318d6bf (The fourth batch, 2020-06-22).  The
tip is tagged as "rewrite-and-cleanup-merge-strategies-v1" at
https://github.com/agrn/git.

Alban Gruin (17):
  t6027: modernise tests
  merge-one-file: rewrite in C
  merge-one-file: remove calls to external processes
  merge-one-file: use error() instead of fprintf(stderr, ...)
  merge-one-file: libify merge_one_file()
  merge-index: libify merge_one_path() and merge_all()
  merge-resolve: rewrite in C
  merge-resolve: remove calls to external processes
  merge-resolve: libify merge_resolve()
  merge-recursive: move better_branch_name() to merge.c
  merge-octopus: rewrite in C
  merge-octopus: remove calls to external processes
  merge-octopus: libify merge_octopus()
  merge: use the "resolve" strategy without forking
  merge: use the "octopus" strategy without forking
  sequencer: use the "resolve" strategy without forking
  sequencer: use the "octopus" merge strategy without forking

 Makefile                        |   7 +-
 builtin.h                       |   3 +
 builtin/merge-index.c           |  77 +----
 builtin/merge-octopus.c         |  65 ++++
 builtin/merge-one-file.c        |  74 ++++
 builtin/merge-recursive.c       |  16 +-
 builtin/merge-resolve.c         |  69 ++++
 builtin/merge.c                 |   9 +-
 cache.h                         |   2 +-
 git-merge-octopus.sh            | 112 -------
 git-merge-one-file.sh           | 167 ---------
 git-merge-resolve.sh            |  54 ---
 git.c                           |   3 +
 merge-strategies.c              | 577 ++++++++++++++++++++++++++++++++
 merge-strategies.h              |  44 +++
 merge.c                         |  12 +
 sequencer.c                     |  16 +-
 t/t6027-merge-binary.sh         |  27 +-
 t/t6035-merge-dir-to-symlink.sh |   2 +-
 19 files changed, 889 insertions(+), 447 deletions(-)
 create mode 100644 builtin/merge-octopus.c
 create mode 100644 builtin/merge-one-file.c
 create mode 100644 builtin/merge-resolve.c
 delete mode 100755 git-merge-octopus.sh
 delete mode 100755 git-merge-one-file.sh
 delete mode 100755 git-merge-resolve.sh
 create mode 100644 merge-strategies.c
 create mode 100644 merge-strategies.h

-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [RFC PATCH v1 01/17] t6027: modernise tests
  2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
@ 2020-06-25 12:19 ` Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 02/17] merge-one-file: rewrite in C Alban Gruin
                   ` (15 subsequent siblings)
  16 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

Some tests in t6027 uses a if/then/else to check if a command failed or
not, but we have the `test_must_fail' function to do it correctly for us
nowadays.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 t/t6027-merge-binary.sh | 27 ++++++---------------------
 1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/t/t6027-merge-binary.sh b/t/t6027-merge-binary.sh
index 4e6c7cb77e..071d3f7343 100755
--- a/t/t6027-merge-binary.sh
+++ b/t/t6027-merge-binary.sh
@@ -5,7 +5,6 @@ test_description='ask merge-recursive to merge binary files'
 . ./test-lib.sh
 
 test_expect_success setup '
-
 	cat "$TEST_DIRECTORY"/test-binary-1.png >m &&
 	git add m &&
 	git ls-files -s | sed -e "s/ 0	/ 1	/" >E1 &&
@@ -35,33 +34,19 @@ test_expect_success setup '
 '
 
 test_expect_success resolve '
-
 	rm -f a* m* &&
 	git reset --hard anchor &&
-
-	if git merge -s resolve master
-	then
-		echo Oops, should not have succeeded
-		false
-	else
-		git ls-files -s >current
-		test_cmp expect current
-	fi
+	test_must_fail git merge -s resolve master &&
+	git ls-files -s >current &&
+	test_cmp expect current
 '
 
 test_expect_success recursive '
-
 	rm -f a* m* &&
 	git reset --hard anchor &&
-
-	if git merge -s recursive master
-	then
-		echo Oops, should not have succeeded
-		false
-	else
-		git ls-files -s >current
-		test_cmp expect current
-	fi
+	test_must_fail git merge -s recursive master &&
+	git ls-files -s >current &&
+	test_cmp expect current
 '
 
 test_done
-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [RFC PATCH v1 02/17] merge-one-file: rewrite in C
  2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 01/17] t6027: modernise tests Alban Gruin
@ 2020-06-25 12:19 ` Alban Gruin
  2020-06-25 14:55   ` Chris Torek
  2020-06-25 15:16   ` Phillip Wood
  2020-06-25 12:19 ` [RFC PATCH v1 03/17] merge-one-file: remove calls to external processes Alban Gruin
                   ` (14 subsequent siblings)
  16 siblings, 2 replies; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

This rewrites `git merge-one-file' from shell to C.  This port is very
straightforward: it keeps using external processes to edit the index,
for instance.  Errors are also displayed with fprintf() instead of
error().  Both of these will be addressed in the next few commits,
leading to its libification so its main function can be used from other
commands directly.

This also fixes a bug present in the original script: instead of
checking if a _regular_ file exists when a file exists in the branch to
merge, but not in our branch, the rewritten version checks if a file of
any kind (ie. a directory, ...) exists.  This fixes the tests t6035.14,
where the branch to merge had a new file, `a/b', but our branch had a
directory there; it should have failed because a directory exists, but
it did not because there was no regular file called `a/b'.  This test is
now marked as successful.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 Makefile                        |   2 +-
 builtin.h                       |   1 +
 builtin/merge-one-file.c        | 275 ++++++++++++++++++++++++++++++++
 git-merge-one-file.sh           | 167 -------------------
 git.c                           |   1 +
 t/t6035-merge-dir-to-symlink.sh |   2 +-
 6 files changed, 279 insertions(+), 169 deletions(-)
 create mode 100644 builtin/merge-one-file.c
 delete mode 100755 git-merge-one-file.sh

diff --git a/Makefile b/Makefile
index 372139f1f2..19574f5133 100644
--- a/Makefile
+++ b/Makefile
@@ -596,7 +596,6 @@ SCRIPT_SH += git-bisect.sh
 SCRIPT_SH += git-difftool--helper.sh
 SCRIPT_SH += git-filter-branch.sh
 SCRIPT_SH += git-merge-octopus.sh
-SCRIPT_SH += git-merge-one-file.sh
 SCRIPT_SH += git-merge-resolve.sh
 SCRIPT_SH += git-mergetool.sh
 SCRIPT_SH += git-quiltimport.sh
@@ -1089,6 +1088,7 @@ BUILTIN_OBJS += builtin/mailsplit.o
 BUILTIN_OBJS += builtin/merge-base.o
 BUILTIN_OBJS += builtin/merge-file.o
 BUILTIN_OBJS += builtin/merge-index.o
+BUILTIN_OBJS += builtin/merge-one-file.o
 BUILTIN_OBJS += builtin/merge-ours.o
 BUILTIN_OBJS += builtin/merge-recursive.o
 BUILTIN_OBJS += builtin/merge-tree.o
diff --git a/builtin.h b/builtin.h
index a5ae15bfe5..9205d5ecdc 100644
--- a/builtin.h
+++ b/builtin.h
@@ -172,6 +172,7 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix);
 int cmd_merge_index(int argc, const char **argv, const char *prefix);
 int cmd_merge_ours(int argc, const char **argv, const char *prefix);
 int cmd_merge_file(int argc, const char **argv, const char *prefix);
+int cmd_merge_one_file(int argc, const char **argv, const char *prefix);
 int cmd_merge_recursive(int argc, const char **argv, const char *prefix);
 int cmd_merge_tree(int argc, const char **argv, const char *prefix);
 int cmd_mktag(int argc, const char **argv, const char *prefix);
diff --git a/builtin/merge-one-file.c b/builtin/merge-one-file.c
new file mode 100644
index 0000000000..4992a6cd30
--- /dev/null
+++ b/builtin/merge-one-file.c
@@ -0,0 +1,275 @@
+/*
+ * 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 script, called with
+ *
+ *   $1 - original file SHA1 (or empty)
+ *   $2 - file in branch1 SHA1 (or empty)
+ *   $3 - file in branch2 SHA1 (or empty)
+ *   $4 - pathname in repository
+ *   $5 - original file mode (or empty)
+ *   $6 - file in branch1 mode (or empty)
+ *   $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.
+ */
+
+#define USE_THE_INDEX_COMPATIBILITY_MACROS
+#include "cache.h"
+#include "builtin.h"
+#include "commit.h"
+#include "dir.h"
+#include "lockfile.h"
+#include "object-store.h"
+#include "run-command.h"
+#include "xdiff-interface.h"
+
+static int create_temp_file(const struct object_id *oid, struct strbuf *path)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf err = STRBUF_INIT;
+	int ret;
+
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "unpack-file", oid_to_hex(oid), NULL);
+	ret = pipe_command(&cp, NULL, 0, path, 0, &err, 0);
+	if (!ret && path->len > 0)
+		strbuf_trim_trailing_newline(path);
+
+	fprintf(stderr, "%.*s", (int) err.len, err.buf);
+	strbuf_release(&err);
+
+	return ret;
+}
+
+static int add_to_index_cacheinfo(unsigned int mode,
+				  const struct object_id *oid, const char *path)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "update-index", "--add", "--cacheinfo", NULL);
+	argv_array_pushf(&cp.args, "%o,%s,%s", mode, oid_to_hex(oid), path);
+	return run_command(&cp);
+}
+
+static int remove_from_index(const char *path)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "update-index", "--remove", "--", path, NULL);
+	return run_command(&cp);
+}
+
+static int checkout_from_index(const char *path)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "checkout-index", "-u", "-f", "--", path, NULL);
+	return run_command(&cp);
+}
+
+static int merge_one_file_deleted(const struct object_id *orig_blob,
+				  const struct object_id *our_blob,
+				  const struct object_id *their_blob, const char *path,
+				  unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode)
+{
+	if ((our_blob && orig_mode != our_mode) ||
+	    (their_blob && orig_mode != their_mode)) {
+		fprintf(stderr, "ERROR: File %s deleted on one branch but had its\n", path);
+		fprintf(stderr, "ERROR: permissions changed on the other.\n");
+		return 1;
+	}
+
+	if (our_blob) {
+		printf("Removing %s\n", path);
+
+		if (file_exists(path))
+			remove_path(path);
+	}
+
+	return remove_from_index(path);
+}
+
+static int do_merge_one_file(const struct object_id *orig_blob,
+			     const struct object_id *our_blob,
+			     const struct object_id *their_blob, const char *path,
+			     unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode)
+{
+	int ret, source, dest;
+	struct strbuf src1 = STRBUF_INIT, src2 = STRBUF_INIT, orig = STRBUF_INIT;
+	struct child_process cp_merge = CHILD_PROCESS_INIT,
+		cp_checkout = CHILD_PROCESS_INIT,
+		cp_update = CHILD_PROCESS_INIT;
+
+	if (our_mode == S_IFLNK || their_mode == S_IFLNK) {
+		fprintf(stderr, "ERROR: %s: Not merging symbolic link changes.\n", path);
+		return 1;
+	} else if (our_mode == S_IFGITLINK || their_mode == S_IFGITLINK) {
+		fprintf(stderr, "ERROR: %s: Not merging conflicting submodule changes.\n",
+			path);
+		return 1;
+	}
+
+	create_temp_file(our_blob, &src1);
+	create_temp_file(their_blob, &src2);
+
+	if (orig_blob) {
+		printf("Auto-merging %s\n", path);
+		create_temp_file(orig_blob, &orig);
+	} else {
+		printf("Added %s in both, but differently.\n", path);
+		create_temp_file(the_hash_algo->empty_blob, &orig);
+	}
+
+	cp_merge.git_cmd = 1;
+	argv_array_pushl(&cp_merge.args, "merge-file", src1.buf, orig.buf, src2.buf,
+			 NULL);
+	ret = run_command(&cp_merge);
+
+	if (ret != 0)
+		ret = 1;
+
+	cp_checkout.git_cmd = 1;
+	argv_array_pushl(&cp_checkout.args, "checkout-index", "-f", "--stage=2",
+			 "--", path, NULL);
+	if (run_command(&cp_checkout))
+		return 1;
+
+	source = open(src1.buf, O_RDONLY);
+	dest = open(path, O_WRONLY | O_TRUNC);
+
+	copy_fd(source, dest);
+
+	close(source);
+	close(dest);
+
+	unlink(orig.buf);
+	unlink(src1.buf);
+	unlink(src2.buf);
+
+	strbuf_release(&src1);
+	strbuf_release(&src2);
+	strbuf_release(&orig);
+
+	if (ret) {
+		fprintf(stderr, "ERROR: ");
+
+		if (!orig_blob) {
+			fprintf(stderr, "content conflict");
+			if (our_mode != their_mode)
+				fprintf(stderr, ", ");
+		}
+
+		if (our_mode != their_mode)
+			fprintf(stderr, "permissions conflict: %o->%o,%o",
+				orig_mode, our_mode, their_mode);
+
+		fprintf(stderr, " in %s\n", path);
+
+		return 1;
+	}
+
+	cp_update.git_cmd = 1;
+	argv_array_pushl(&cp_update.args, "update-index", "--", path, NULL);
+	return run_command(&cp_update);
+}
+
+static int merge_one_file(const struct object_id *orig_blob,
+			  const struct object_id *our_blob,
+			  const struct object_id *their_blob, const char *path,
+			  unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode)
+{
+	if (orig_blob &&
+	    ((our_blob && oideq(orig_blob, our_blob)) ||
+	     (their_blob && oideq(orig_blob, their_blob))))
+		return merge_one_file_deleted(orig_blob, our_blob, their_blob, path,
+					      orig_mode, our_mode, their_mode);
+	else if (!orig_blob && our_blob && !their_blob) {
+		return add_to_index_cacheinfo(our_mode, our_blob, path);
+	} else if (!orig_blob && !our_blob && their_blob) {
+		printf("Adding %s\n", path);
+
+		if (file_exists(path)) {
+			fprintf(stderr, "ERROR: untracked %s is overwritten by the merge.\n", path);
+			return 1;
+		}
+
+		if (add_to_index_cacheinfo(their_mode, their_blob, path))
+			return 1;
+		return checkout_from_index(path);
+	} else if (!orig_blob && our_blob && their_blob &&
+		   oideq(our_blob, their_blob)) {
+		if (our_mode != their_mode) {
+			fprintf(stderr, "ERROR: File %s added identically in both branches,", path);
+			fprintf(stderr, "ERROR: but permissions conflict %o->%o.\n",
+				our_mode, their_mode);
+			return 1;
+		}
+
+		printf("Adding %s\n", path);
+
+		if (add_to_index_cacheinfo(our_mode, our_blob, path))
+			return 1;
+		return checkout_from_index(path);
+	} else if (our_blob && their_blob)
+		return do_merge_one_file(orig_blob, our_blob, their_blob, path,
+					 orig_mode, our_mode, their_mode);
+	else {
+		char *orig_hex = "", *our_hex = "", *their_hex = "";
+
+		if (orig_blob)
+			orig_hex = oid_to_hex(orig_blob);
+		if (our_blob)
+			our_hex = oid_to_hex(our_blob);
+		if (their_blob)
+			their_hex = oid_to_hex(their_blob);
+
+		fprintf(stderr, "ERROR: %s: Not handling case %s -> %s -> %s\n",
+			path, orig_hex, our_hex, their_hex);
+		return 1;
+	}
+
+	return 0;
+}
+
+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.";
+
+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;
+
+	if (argc != 8)
+		usage(builtin_merge_one_file_usage);
+
+	if (!get_oid(argv[1], &orig_blob)) {
+		p_orig_blob = &orig_blob;
+		orig_mode = strtol(argv[5], NULL, 8);
+	}
+
+	if (!get_oid(argv[2], &our_blob)) {
+		p_our_blob = &our_blob;
+		our_mode = strtol(argv[6], NULL, 8);
+	}
+
+	if (!get_oid(argv[3], &their_blob)) {
+		p_their_blob = &their_blob;
+		their_mode = strtol(argv[7], NULL, 8);
+	}
+
+	return merge_one_file(p_orig_blob, p_our_blob, p_their_blob, argv[4],
+			      orig_mode, our_mode, their_mode);
+}
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
deleted file mode 100755
index f6d9852d2f..0000000000
--- a/git-merge-one-file.sh
+++ /dev/null
@@ -1,167 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) Linus Torvalds, 2005
-#
-# This is the git per-file merge script, called with
-#
-#   $1 - original file SHA1 (or empty)
-#   $2 - file in branch1 SHA1 (or empty)
-#   $3 - file in branch2 SHA1 (or empty)
-#   $4 - pathname in repository
-#   $5 - original file mode (or empty)
-#   $6 - file in branch1 mode (or empty)
-#   $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.
-
-USAGE='<orig blob> <our blob> <their blob> <path>'
-USAGE="$USAGE <orig mode> <our mode> <their mode>"
-LONG_USAGE="usage: git merge-one-file $USAGE
-
-Blob ids and modes should be empty for missing files."
-
-SUBDIRECTORY_OK=Yes
-. git-sh-setup
-cd_to_toplevel
-require_work_tree
-
-if test $# != 7
-then
-	echo "$LONG_USAGE"
-	exit 1
-fi
-
-case "${1:-.}${2:-.}${3:-.}" in
-#
-# Deleted in both or deleted in one and unchanged in the other
-#
-"$1.." | "$1.$1" | "$1$1.")
-	if { test -z "$6" && test "$5" != "$7"; } ||
-	   { test -z "$7" && test "$5" != "$6"; }
-	then
-		echo "ERROR: File $4 deleted on one branch but had its" >&2
-		echo "ERROR: permissions changed on the other." >&2
-		exit 1
-	fi
-
-	if test -n "$2"
-	then
-		echo "Removing $4"
-	else
-		# read-tree checked that index matches HEAD already,
-		# so we know we do not have this path tracked.
-		# there may be an unrelated working tree file here,
-		# which we should just leave unmolested.  Make sure
-		# we do not have it in the index, though.
-		exec git update-index --remove -- "$4"
-	fi
-	if test -f "$4"
-	then
-		rm -f -- "$4" &&
-		rmdir -p "$(expr "z$4" : 'z\(.*\)/')" 2>/dev/null || :
-	fi &&
-		exec git update-index --remove -- "$4"
-	;;
-
-#
-# Added in one.
-#
-".$2.")
-	# the other side did not add and we added so there is nothing
-	# to be done, except making the path merged.
-	exec git update-index --add --cacheinfo "$6" "$2" "$4"
-	;;
-"..$3")
-	echo "Adding $4"
-	if test -f "$4"
-	then
-		echo "ERROR: untracked $4 is overwritten by the merge." >&2
-		exit 1
-	fi
-	git update-index --add --cacheinfo "$7" "$3" "$4" &&
-		exec git checkout-index -u -f -- "$4"
-	;;
-
-#
-# Added in both, identically (check for same permissions).
-#
-".$3$2")
-	if test "$6" != "$7"
-	then
-		echo "ERROR: File $4 added identically in both branches," >&2
-		echo "ERROR: but permissions conflict $6->$7." >&2
-		exit 1
-	fi
-	echo "Adding $4"
-	git update-index --add --cacheinfo "$6" "$2" "$4" &&
-		exec git checkout-index -u -f -- "$4"
-	;;
-
-#
-# Modified in both, but differently.
-#
-"$1$2$3" | ".$2$3")
-
-	case ",$6,$7," in
-	*,120000,*)
-		echo "ERROR: $4: Not merging symbolic link changes." >&2
-		exit 1
-		;;
-	*,160000,*)
-		echo "ERROR: $4: Not merging conflicting submodule changes." >&2
-		exit 1
-		;;
-	esac
-
-	src1=$(git unpack-file $2)
-	src2=$(git unpack-file $3)
-	case "$1" in
-	'')
-		echo "Added $4 in both, but differently."
-		orig=$(git unpack-file $(git hash-object /dev/null))
-		;;
-	*)
-		echo "Auto-merging $4"
-		orig=$(git unpack-file $1)
-		;;
-	esac
-
-	git merge-file "$src1" "$orig" "$src2"
-	ret=$?
-	msg=
-	if test $ret != 0 || test -z "$1"
-	then
-		msg='content conflict'
-		ret=1
-	fi
-
-	# Create the working tree file, using "our tree" version from the
-	# index, and then store the result of the merge.
-	git checkout-index -f --stage=2 -- "$4" && cat "$src1" >"$4" || exit 1
-	rm -f -- "$orig" "$src1" "$src2"
-
-	if test "$6" != "$7"
-	then
-		if test -n "$msg"
-		then
-			msg="$msg, "
-		fi
-		msg="${msg}permissions conflict: $5->$6,$7"
-		ret=1
-	fi
-
-	if test $ret != 0
-	then
-		echo "ERROR: $msg in $4" >&2
-		exit 1
-	fi
-	exec git update-index -- "$4"
-	;;
-
-*)
-	echo "ERROR: $4: Not handling case $1 -> $2 -> $3" >&2
-	;;
-esac
-exit 1
diff --git a/git.c b/git.c
index a2d337eed7..058d91a2a5 100644
--- a/git.c
+++ b/git.c
@@ -532,6 +532,7 @@ static struct cmd_struct commands[] = {
 	{ "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
 	{ "merge-index", cmd_merge_index, RUN_SETUP | NO_PARSEOPT },
 	{ "merge-ours", cmd_merge_ours, RUN_SETUP | NO_PARSEOPT },
+	{ "merge-one-file", cmd_merge_one_file, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
diff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh
index 2eddcc7664..5fb74e39a0 100755
--- a/t/t6035-merge-dir-to-symlink.sh
+++ b/t/t6035-merge-dir-to-symlink.sh
@@ -94,7 +94,7 @@ test_expect_success SYMLINKS 'a/b was resolved as symlink' '
 	test -h a/b
 '
 
-test_expect_failure 'do not lose untracked in merge (resolve)' '
+test_expect_success 'do not lose untracked in merge (resolve)' '
 	git reset --hard &&
 	git checkout baseline^0 &&
 	>a/b/c/e &&
-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [RFC PATCH v1 03/17] merge-one-file: remove calls to external processes
  2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 01/17] t6027: modernise tests Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 02/17] merge-one-file: rewrite in C Alban Gruin
@ 2020-06-25 12:19 ` Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 04/17] merge-one-file: use error() instead of fprintf(stderr, ...) Alban Gruin
                   ` (13 subsequent siblings)
  16 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

To save precious cycles by avoiding reading and flushing the index
repeatedly, or write temporary files when an operation can be performed
in-memory, this removes call to external processes:

 - calls to `update-index --add --cacheinfo' are replaced by calls to
   add_cache_entry();

 - calls to `update-index --remove' are replaced by calls to
   remove_file_from_cache();

 - calls to `checkout-index -u -f' are replaced by calls to
   checkout_entry();

 - calls to `unpack-file' and `merge-files' are replaced by calls to
   read_mmblob() and xdl_merge(), respectively, to merge files
   in-memory;

 - calls to `checkout-index -f --stage=2' are replaced by calls to
   cache_file_exists();

 - calls to `update-index' are replaced by calls to add_file_to_cache().

To enable these changes, the index is read and written back in
cmd_merge_one_file().

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/merge-one-file.c | 160 +++++++++++++++++++--------------------
 1 file changed, 78 insertions(+), 82 deletions(-)

diff --git a/builtin/merge-one-file.c b/builtin/merge-one-file.c
index 4992a6cd30..d9ebd820cb 100644
--- a/builtin/merge-one-file.c
+++ b/builtin/merge-one-file.c
@@ -27,54 +27,48 @@
 #include "dir.h"
 #include "lockfile.h"
 #include "object-store.h"
-#include "run-command.h"
 #include "xdiff-interface.h"
 
-static int create_temp_file(const struct object_id *oid, struct strbuf *path)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-	struct strbuf err = STRBUF_INIT;
-	int ret;
-
-	cp.git_cmd = 1;
-	argv_array_pushl(&cp.args, "unpack-file", oid_to_hex(oid), NULL);
-	ret = pipe_command(&cp, NULL, 0, path, 0, &err, 0);
-	if (!ret && path->len > 0)
-		strbuf_trim_trailing_newline(path);
-
-	fprintf(stderr, "%.*s", (int) err.len, err.buf);
-	strbuf_release(&err);
-
-	return ret;
-}
-
 static int add_to_index_cacheinfo(unsigned int mode,
 				  const struct object_id *oid, const char *path)
 {
-	struct child_process cp = CHILD_PROCESS_INIT;
+	struct cache_entry *ce;
+	int len, option;
 
-	cp.git_cmd = 1;
-	argv_array_pushl(&cp.args, "update-index", "--add", "--cacheinfo", NULL);
-	argv_array_pushf(&cp.args, "%o,%s,%s", mode, oid_to_hex(oid), path);
-	return run_command(&cp);
-}
+	if (!verify_path(path, mode))
+		return error("Invalid path '%s'", path);
 
-static int remove_from_index(const char *path)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
+	len = strlen(path);
+	ce = make_empty_cache_entry(&the_index, len);
 
-	cp.git_cmd = 1;
-	argv_array_pushl(&cp.args, "update-index", "--remove", "--", path, NULL);
-	return run_command(&cp);
+	oidcpy(&ce->oid, oid);
+	memcpy(ce->name, path, len);
+	ce->ce_flags = create_ce_flags(0);
+	ce->ce_namelen = len;
+	ce->ce_mode = create_ce_mode(mode);
+	if (assume_unchanged)
+		ce->ce_flags |= CE_VALID;
+	option = ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE;
+	if (add_cache_entry(ce, option))
+		return error("%s: cannot add to the index", path);
+
+	return 0;
 }
 
 static int checkout_from_index(const char *path)
 {
-	struct child_process cp = CHILD_PROCESS_INIT;
+	struct checkout state;
+	struct cache_entry *ce;
 
-	cp.git_cmd = 1;
-	argv_array_pushl(&cp.args, "checkout-index", "-u", "-f", "--", path, NULL);
-	return run_command(&cp);
+	state.istate = &the_index;
+	state.force = 1;
+	state.base_dir = "";
+	state.base_dir_len = 0;
+
+	ce = cache_file_exists(path, strlen(path), 0);
+	if (checkout_entry(ce, &state, NULL, NULL) < 0)
+		return error("%s: cannot checkout file", path);
+	return 0;
 }
 
 static int merge_one_file_deleted(const struct object_id *orig_blob,
@@ -96,7 +90,9 @@ static int merge_one_file_deleted(const struct object_id *orig_blob,
 			remove_path(path);
 	}
 
-	return remove_from_index(path);
+	if (remove_file_from_cache(path))
+		return error("%s: cannot remove from the index", path);
+	return 0;
 }
 
 static int do_merge_one_file(const struct object_id *orig_blob,
@@ -104,61 +100,50 @@ static int do_merge_one_file(const struct object_id *orig_blob,
 			     const struct object_id *their_blob, const char *path,
 			     unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode)
 {
-	int ret, source, dest;
-	struct strbuf src1 = STRBUF_INIT, src2 = STRBUF_INIT, orig = STRBUF_INIT;
-	struct child_process cp_merge = CHILD_PROCESS_INIT,
-		cp_checkout = CHILD_PROCESS_INIT,
-		cp_update = CHILD_PROCESS_INIT;
+	int ret, i, dest;
+	mmbuffer_t result = {NULL, 0};
+	mmfile_t mmfs[3];
+	xmparam_t xmp = {{0}};
+	struct cache_entry *ce;
 
-	if (our_mode == S_IFLNK || their_mode == S_IFLNK) {
-		fprintf(stderr, "ERROR: %s: Not merging symbolic link changes.\n", path);
-		return 1;
-	} else if (our_mode == S_IFGITLINK || their_mode == S_IFGITLINK) {
-		fprintf(stderr, "ERROR: %s: Not merging conflicting submodule changes.\n",
-			path);
-		return 1;
-	}
+	if (our_mode == S_IFLNK || their_mode == S_IFLNK)
+		return error(_("%s: Not merging symbolic link changes."), path);
+	else if (our_mode == S_IFGITLINK || their_mode == S_IFGITLINK)
+		return error(_("%s: Not merging conflicting submodule changes."), path);
 
-	create_temp_file(our_blob, &src1);
-	create_temp_file(their_blob, &src2);
+	read_mmblob(mmfs + 0, our_blob);
+	read_mmblob(mmfs + 2, their_blob);
 
 	if (orig_blob) {
 		printf("Auto-merging %s\n", path);
-		create_temp_file(orig_blob, &orig);
+		read_mmblob(mmfs + 1, orig_blob);
 	} else {
 		printf("Added %s in both, but differently.\n", path);
-		create_temp_file(the_hash_algo->empty_blob, &orig);
+		read_mmblob(mmfs + 1, the_hash_algo->empty_blob);
 	}
 
-	cp_merge.git_cmd = 1;
-	argv_array_pushl(&cp_merge.args, "merge-file", src1.buf, orig.buf, src2.buf,
-			 NULL);
-	ret = run_command(&cp_merge);
+	xmp.level = XDL_MERGE_ZEALOUS_ALNUM;
+	xmp.style = 0;
+	xmp.favor = 0;
 
-	if (ret != 0)
+	ret = xdl_merge(mmfs + 1, mmfs + 0, mmfs + 2, &xmp, &result);
+
+	for (i = 0; i < 3; i++)
+		free(mmfs[i].ptr);
+
+	if (ret > 127)
 		ret = 1;
 
-	cp_checkout.git_cmd = 1;
-	argv_array_pushl(&cp_checkout.args, "checkout-index", "-f", "--stage=2",
-			 "--", path, NULL);
-	if (run_command(&cp_checkout))
-		return 1;
+	ce = cache_file_exists(path, strlen(path), 0);
+	if (!ce)
+		BUG("file is not present in the cache?");
 
-	source = open(src1.buf, O_RDONLY);
-	dest = open(path, O_WRONLY | O_TRUNC);
-
-	copy_fd(source, dest);
-
-	close(source);
+	unlink(path);
+	dest = open(path, O_WRONLY | O_CREAT, ce->ce_mode);
+	write_in_full(dest, result.ptr, result.size);
 	close(dest);
 
-	unlink(orig.buf);
-	unlink(src1.buf);
-	unlink(src2.buf);
-
-	strbuf_release(&src1);
-	strbuf_release(&src2);
-	strbuf_release(&orig);
+	free(result.ptr);
 
 	if (ret) {
 		fprintf(stderr, "ERROR: ");
@@ -178,9 +163,7 @@ static int do_merge_one_file(const struct object_id *orig_blob,
 		return 1;
 	}
 
-	cp_update.git_cmd = 1;
-	argv_array_pushl(&cp_update.args, "update-index", "--", path, NULL);
-	return run_command(&cp_update);
+	return add_file_to_cache(path, 0);
 }
 
 static int merge_one_file(const struct object_id *orig_blob,
@@ -250,11 +233,17 @@ 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;
+	unsigned int orig_mode = 0, our_mode = 0, their_mode = 0, ret;
+	struct lock_file lock = LOCK_INIT;
 
 	if (argc != 8)
 		usage(builtin_merge_one_file_usage);
 
+	if (read_cache() < 0)
+		die("invalid index");
+
+	hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
+
 	if (!get_oid(argv[1], &orig_blob)) {
 		p_orig_blob = &orig_blob;
 		orig_mode = strtol(argv[5], NULL, 8);
@@ -270,6 +259,13 @@ int cmd_merge_one_file(int argc, const char **argv, const char *prefix)
 		their_mode = strtol(argv[7], NULL, 8);
 	}
 
-	return merge_one_file(p_orig_blob, p_our_blob, p_their_blob, argv[4],
-			      orig_mode, our_mode, their_mode);
+	ret = merge_one_file(p_orig_blob, p_our_blob, p_their_blob, argv[4],
+			     orig_mode, our_mode, their_mode);
+
+	if (ret) {
+		rollback_lock_file(&lock);
+		return ret;
+	}
+
+	return write_locked_index(&the_index, &lock, COMMIT_LOCK);
 }
-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [RFC PATCH v1 04/17] merge-one-file: use error() instead of fprintf(stderr, ...)
  2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
                   ` (2 preceding siblings ...)
  2020-06-25 12:19 ` [RFC PATCH v1 03/17] merge-one-file: remove calls to external processes Alban Gruin
@ 2020-06-25 12:19 ` Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 05/17] merge-one-file: libify merge_one_file() Alban Gruin
                   ` (12 subsequent siblings)
  16 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

We have a handy helper function to display errors and return a value.
Use it instead of fprintf(stderr, ...).

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/merge-one-file.c | 43 +++++++++++++---------------------------
 1 file changed, 14 insertions(+), 29 deletions(-)

diff --git a/builtin/merge-one-file.c b/builtin/merge-one-file.c
index d9ebd820cb..d612885723 100644
--- a/builtin/merge-one-file.c
+++ b/builtin/merge-one-file.c
@@ -77,11 +77,9 @@ static int merge_one_file_deleted(const struct object_id *orig_blob,
 				  unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode)
 {
 	if ((our_blob && orig_mode != our_mode) ||
-	    (their_blob && orig_mode != their_mode)) {
-		fprintf(stderr, "ERROR: File %s deleted on one branch but had its\n", path);
-		fprintf(stderr, "ERROR: permissions changed on the other.\n");
-		return 1;
-	}
+	    (their_blob && orig_mode != their_mode))
+		return error(_("File %s deleted on one branch but had its "
+			       "permissions changed on the other."), path);
 
 	if (our_blob) {
 		printf("Removing %s\n", path);
@@ -146,19 +144,11 @@ static int do_merge_one_file(const struct object_id *orig_blob,
 	free(result.ptr);
 
 	if (ret) {
-		fprintf(stderr, "ERROR: ");
-
-		if (!orig_blob) {
-			fprintf(stderr, "content conflict");
-			if (our_mode != their_mode)
-				fprintf(stderr, ", ");
-		}
-
+		if (!orig_blob)
+			error(_("content conflict in %s"), path);
 		if (our_mode != their_mode)
-			fprintf(stderr, "permissions conflict: %o->%o,%o",
-				orig_mode, our_mode, their_mode);
-
-		fprintf(stderr, " in %s\n", path);
+			error(_("permission conflict: %o->%o,%o in %s"),
+			      orig_mode, our_mode, their_mode, path);
 
 		return 1;
 	}
@@ -181,22 +171,18 @@ static int merge_one_file(const struct object_id *orig_blob,
 	} else if (!orig_blob && !our_blob && their_blob) {
 		printf("Adding %s\n", path);
 
-		if (file_exists(path)) {
-			fprintf(stderr, "ERROR: untracked %s is overwritten by the merge.\n", path);
-			return 1;
-		}
+		if (file_exists(path))
+			return error(_("untracked %s is overwritten by the merge."), path);
 
 		if (add_to_index_cacheinfo(their_mode, their_blob, path))
 			return 1;
 		return checkout_from_index(path);
 	} else if (!orig_blob && our_blob && their_blob &&
 		   oideq(our_blob, their_blob)) {
-		if (our_mode != their_mode) {
-			fprintf(stderr, "ERROR: File %s added identically in both branches,", path);
-			fprintf(stderr, "ERROR: but permissions conflict %o->%o.\n",
-				our_mode, their_mode);
-			return 1;
-		}
+		if (our_mode != their_mode)
+			return error(_("File %s added identically in both branches, "
+				       "but permissions conflict %o->%o."),
+				     path, our_mode, their_mode);
 
 		printf("Adding %s\n", path);
 
@@ -216,9 +202,8 @@ static int merge_one_file(const struct object_id *orig_blob,
 		if (their_blob)
 			their_hex = oid_to_hex(their_blob);
 
-		fprintf(stderr, "ERROR: %s: Not handling case %s -> %s -> %s\n",
+		return error(_("%s: Not handling case %s -> %s -> %s"),
 			path, orig_hex, our_hex, their_hex);
-		return 1;
 	}
 
 	return 0;
-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [RFC PATCH v1 05/17] merge-one-file: libify merge_one_file()
  2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
                   ` (3 preceding siblings ...)
  2020-06-25 12:19 ` [RFC PATCH v1 04/17] merge-one-file: use error() instead of fprintf(stderr, ...) Alban Gruin
@ 2020-06-25 12:19 ` Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 06/17] merge-index: libify merge_one_path() and merge_all() Alban Gruin
                   ` (11 subsequent siblings)
  16 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

This moves merge_one_file() (and its helper functions) to a new file,
merge-strategies.c.  This will enable the resolve and octopus strategies
to directly call it instead of forking.  It is also renamed
merge_strategies_one_file().

This is not a faithful copy-and-paste; in the builtin versions,
merge_one_file() operated on `the_repository' and `the_index', something
we cannot allow a function part of libgit.a to do.  Hence, it now takes
a pointer to a repository as its first argument (and helper functions
takes a pointer to an `index_state').

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---

Notes:
    This patch is best viewed with `--color-moved'.

 Makefile                 |   1 +
 builtin/merge-one-file.c | 190 +-------------------------------------
 merge-strategies.c       | 191 +++++++++++++++++++++++++++++++++++++++
 merge-strategies.h       |  13 +++
 4 files changed, 209 insertions(+), 186 deletions(-)
 create mode 100644 merge-strategies.c
 create mode 100644 merge-strategies.h

diff --git a/Makefile b/Makefile
index 19574f5133..1ab4d160cb 100644
--- a/Makefile
+++ b/Makefile
@@ -911,6 +911,7 @@ LIB_OBJS += match-trees.o
 LIB_OBJS += mem-pool.o
 LIB_OBJS += merge-blobs.o
 LIB_OBJS += merge-recursive.o
+LIB_OBJS += merge-strategies.o
 LIB_OBJS += merge.o
 LIB_OBJS += mergesort.o
 LIB_OBJS += midx.o
diff --git a/builtin/merge-one-file.c b/builtin/merge-one-file.c
index d612885723..2f7a3e1db2 100644
--- a/builtin/merge-one-file.c
+++ b/builtin/merge-one-file.c
@@ -23,191 +23,8 @@
 #define USE_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
 #include "builtin.h"
-#include "commit.h"
-#include "dir.h"
 #include "lockfile.h"
-#include "object-store.h"
-#include "xdiff-interface.h"
-
-static int add_to_index_cacheinfo(unsigned int mode,
-				  const struct object_id *oid, const char *path)
-{
-	struct cache_entry *ce;
-	int len, option;
-
-	if (!verify_path(path, mode))
-		return error("Invalid path '%s'", path);
-
-	len = strlen(path);
-	ce = make_empty_cache_entry(&the_index, len);
-
-	oidcpy(&ce->oid, oid);
-	memcpy(ce->name, path, len);
-	ce->ce_flags = create_ce_flags(0);
-	ce->ce_namelen = len;
-	ce->ce_mode = create_ce_mode(mode);
-	if (assume_unchanged)
-		ce->ce_flags |= CE_VALID;
-	option = ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE;
-	if (add_cache_entry(ce, option))
-		return error("%s: cannot add to the index", path);
-
-	return 0;
-}
-
-static int checkout_from_index(const char *path)
-{
-	struct checkout state;
-	struct cache_entry *ce;
-
-	state.istate = &the_index;
-	state.force = 1;
-	state.base_dir = "";
-	state.base_dir_len = 0;
-
-	ce = cache_file_exists(path, strlen(path), 0);
-	if (checkout_entry(ce, &state, NULL, NULL) < 0)
-		return error("%s: cannot checkout file", path);
-	return 0;
-}
-
-static int merge_one_file_deleted(const struct object_id *orig_blob,
-				  const struct object_id *our_blob,
-				  const struct object_id *their_blob, const char *path,
-				  unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode)
-{
-	if ((our_blob && orig_mode != our_mode) ||
-	    (their_blob && orig_mode != their_mode))
-		return error(_("File %s deleted on one branch but had its "
-			       "permissions changed on the other."), path);
-
-	if (our_blob) {
-		printf("Removing %s\n", path);
-
-		if (file_exists(path))
-			remove_path(path);
-	}
-
-	if (remove_file_from_cache(path))
-		return error("%s: cannot remove from the index", path);
-	return 0;
-}
-
-static int do_merge_one_file(const struct object_id *orig_blob,
-			     const struct object_id *our_blob,
-			     const struct object_id *their_blob, const char *path,
-			     unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode)
-{
-	int ret, i, dest;
-	mmbuffer_t result = {NULL, 0};
-	mmfile_t mmfs[3];
-	xmparam_t xmp = {{0}};
-	struct cache_entry *ce;
-
-	if (our_mode == S_IFLNK || their_mode == S_IFLNK)
-		return error(_("%s: Not merging symbolic link changes."), path);
-	else if (our_mode == S_IFGITLINK || their_mode == S_IFGITLINK)
-		return error(_("%s: Not merging conflicting submodule changes."), path);
-
-	read_mmblob(mmfs + 0, our_blob);
-	read_mmblob(mmfs + 2, their_blob);
-
-	if (orig_blob) {
-		printf("Auto-merging %s\n", path);
-		read_mmblob(mmfs + 1, orig_blob);
-	} else {
-		printf("Added %s in both, but differently.\n", path);
-		read_mmblob(mmfs + 1, the_hash_algo->empty_blob);
-	}
-
-	xmp.level = XDL_MERGE_ZEALOUS_ALNUM;
-	xmp.style = 0;
-	xmp.favor = 0;
-
-	ret = xdl_merge(mmfs + 1, mmfs + 0, mmfs + 2, &xmp, &result);
-
-	for (i = 0; i < 3; i++)
-		free(mmfs[i].ptr);
-
-	if (ret > 127)
-		ret = 1;
-
-	ce = cache_file_exists(path, strlen(path), 0);
-	if (!ce)
-		BUG("file is not present in the cache?");
-
-	unlink(path);
-	dest = open(path, O_WRONLY | O_CREAT, ce->ce_mode);
-	write_in_full(dest, result.ptr, result.size);
-	close(dest);
-
-	free(result.ptr);
-
-	if (ret) {
-		if (!orig_blob)
-			error(_("content conflict in %s"), path);
-		if (our_mode != their_mode)
-			error(_("permission conflict: %o->%o,%o in %s"),
-			      orig_mode, our_mode, their_mode, path);
-
-		return 1;
-	}
-
-	return add_file_to_cache(path, 0);
-}
-
-static int merge_one_file(const struct object_id *orig_blob,
-			  const struct object_id *our_blob,
-			  const struct object_id *their_blob, const char *path,
-			  unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode)
-{
-	if (orig_blob &&
-	    ((our_blob && oideq(orig_blob, our_blob)) ||
-	     (their_blob && oideq(orig_blob, their_blob))))
-		return merge_one_file_deleted(orig_blob, our_blob, their_blob, path,
-					      orig_mode, our_mode, their_mode);
-	else if (!orig_blob && our_blob && !their_blob) {
-		return add_to_index_cacheinfo(our_mode, our_blob, path);
-	} else if (!orig_blob && !our_blob && their_blob) {
-		printf("Adding %s\n", path);
-
-		if (file_exists(path))
-			return error(_("untracked %s is overwritten by the merge."), path);
-
-		if (add_to_index_cacheinfo(their_mode, their_blob, path))
-			return 1;
-		return checkout_from_index(path);
-	} else if (!orig_blob && our_blob && their_blob &&
-		   oideq(our_blob, their_blob)) {
-		if (our_mode != their_mode)
-			return error(_("File %s added identically in both branches, "
-				       "but permissions conflict %o->%o."),
-				     path, our_mode, their_mode);
-
-		printf("Adding %s\n", path);
-
-		if (add_to_index_cacheinfo(our_mode, our_blob, path))
-			return 1;
-		return checkout_from_index(path);
-	} else if (our_blob && their_blob)
-		return do_merge_one_file(orig_blob, our_blob, their_blob, path,
-					 orig_mode, our_mode, their_mode);
-	else {
-		char *orig_hex = "", *our_hex = "", *their_hex = "";
-
-		if (orig_blob)
-			orig_hex = oid_to_hex(orig_blob);
-		if (our_blob)
-			our_hex = oid_to_hex(our_blob);
-		if (their_blob)
-			their_hex = oid_to_hex(their_blob);
-
-		return error(_("%s: Not handling case %s -> %s -> %s"),
-			path, orig_hex, our_hex, their_hex);
-	}
-
-	return 0;
-}
+#include "merge-strategies.h"
 
 static const char builtin_merge_one_file_usage[] =
 	"git merge-one-file <orig blob> <our blob> <their blob> <path> "
@@ -244,8 +61,9 @@ int cmd_merge_one_file(int argc, const char **argv, const char *prefix)
 		their_mode = strtol(argv[7], NULL, 8);
 	}
 
-	ret = merge_one_file(p_orig_blob, p_our_blob, p_their_blob, argv[4],
-			     orig_mode, our_mode, their_mode);
+	ret = merge_strategies_one_file(the_repository,
+					p_orig_blob, p_our_blob, p_their_blob, argv[4],
+					orig_mode, our_mode, their_mode);
 
 	if (ret) {
 		rollback_lock_file(&lock);
diff --git a/merge-strategies.c b/merge-strategies.c
new file mode 100644
index 0000000000..3a9fce9f22
--- /dev/null
+++ b/merge-strategies.c
@@ -0,0 +1,191 @@
+#include "cache.h"
+#include "dir.h"
+#include "merge-strategies.h"
+#include "xdiff-interface.h"
+
+static int add_to_index_cacheinfo(struct index_state *istate,
+				  unsigned int mode,
+				  const struct object_id *oid, const char *path)
+{
+	struct cache_entry *ce;
+	int len, option;
+
+	if (!verify_path(path, mode))
+		return error(_("Invalid path '%s'"), path);
+
+	len = strlen(path);
+	ce = make_empty_cache_entry(istate, len);
+
+	oidcpy(&ce->oid, oid);
+	memcpy(ce->name, path, len);
+	ce->ce_flags = create_ce_flags(0);
+	ce->ce_namelen = len;
+	ce->ce_mode = create_ce_mode(mode);
+	if (assume_unchanged)
+		ce->ce_flags |= CE_VALID;
+	option = ADD_CACHE_OK_TO_ADD | ADD_CACHE_OK_TO_REPLACE;
+	if (add_index_entry(istate, ce, option))
+		return error(_("%s: cannot add to the index"), path);
+
+	return 0;
+}
+
+static int checkout_from_index(struct index_state *istate, const char *path)
+{
+	struct checkout state = CHECKOUT_INIT;
+	struct cache_entry *ce;
+
+	state.istate = istate;
+	state.force = 1;
+	state.base_dir = "";
+	state.base_dir_len = 0;
+
+	ce = index_file_exists(istate, path, strlen(path), 0);
+	if (checkout_entry(ce, &state, NULL, NULL) < 0)
+		return error(_("%s: cannot checkout file"), path);
+	return 0;
+}
+
+static int merge_one_file_deleted(struct index_state *istate,
+				  const struct object_id *orig_blob,
+				  const struct object_id *our_blob,
+				  const struct object_id *their_blob, const char *path,
+				  unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode)
+{
+	if ((our_blob && orig_mode != our_mode) ||
+	    (their_blob && orig_mode != their_mode))
+		return error(_("File %s deleted on one branch but had its "
+			       "permissions changed on the other."), path);
+
+	if (our_blob) {
+		printf("Removing %s\n", path);
+
+		if (file_exists(path))
+			remove_path(path);
+	}
+
+	if (remove_file_from_index(istate, path))
+		return error("%s: cannot remove from the index", path);
+	return 0;
+}
+
+static int do_merge_one_file(struct index_state *istate,
+			     const struct object_id *orig_blob,
+			     const struct object_id *our_blob,
+			     const struct object_id *their_blob, const char *path,
+			     unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode)
+{
+	int ret, i, dest;
+	mmbuffer_t result = {NULL, 0};
+	mmfile_t mmfs[3];
+	xmparam_t xmp = {{0}};
+	struct cache_entry *ce;
+
+	if (our_mode == S_IFLNK || their_mode == S_IFLNK)
+		return error(_("%s: Not merging symbolic link changes."), path);
+	else if (our_mode == S_IFGITLINK || their_mode == S_IFGITLINK)
+		return error(_("%s: Not merging conflicting submodule changes."), path);
+
+	read_mmblob(mmfs + 0, our_blob);
+	read_mmblob(mmfs + 2, their_blob);
+
+	if (orig_blob) {
+		printf("Auto-merging %s\n", path);
+		read_mmblob(mmfs + 1, orig_blob);
+	} else {
+		printf("Added %s in both, but differently.\n", path);
+		read_mmblob(mmfs + 1, &null_oid);
+	}
+
+	xmp.level = XDL_MERGE_ZEALOUS_ALNUM;
+	xmp.style = 0;
+	xmp.favor = 0;
+
+	ret = xdl_merge(mmfs + 1, mmfs + 0, mmfs + 2, &xmp, &result);
+
+	for (i = 0; i < 3; i++)
+		free(mmfs[i].ptr);
+
+	if (ret > 127)
+		ret = 1;
+
+	ce = index_file_exists(istate, path, strlen(path), 0);
+	if (!ce)
+		BUG("file is not present in the cache?");
+
+	unlink(path);
+	dest = open(path, O_WRONLY | O_CREAT, ce->ce_mode);
+	write_in_full(dest, result.ptr, result.size);
+	close(dest);
+
+	free(result.ptr);
+
+	if (ret) {
+		if (!orig_blob)
+			error(_("content conflict in %s"), path);
+		if (our_mode != their_mode)
+			error(_("permission conflict: %o->%o,%o in %s"),
+			      orig_mode, our_mode, their_mode, path);
+
+		return 1;
+	}
+
+	return add_file_to_index(istate, path, 0);
+}
+
+int merge_strategies_one_file(struct repository *r,
+			      const struct object_id *orig_blob,
+			      const struct object_id *our_blob,
+			      const struct object_id *their_blob, const char *path,
+			      unsigned int orig_mode, unsigned int our_mode,
+			      unsigned int their_mode)
+{
+	if (orig_blob &&
+	    ((our_blob && oideq(orig_blob, our_blob)) ||
+	     (their_blob && oideq(orig_blob, their_blob))))
+		return merge_one_file_deleted(r->index,
+					      orig_blob, our_blob, their_blob, path,
+					      orig_mode, our_mode, their_mode);
+	else if (!orig_blob && our_blob && !their_blob) {
+		return add_to_index_cacheinfo(r->index, our_mode, our_blob, path);
+	} else if (!orig_blob && !our_blob && their_blob) {
+		printf("Adding %s\n", path);
+
+		if (file_exists(path))
+			return error(_("untracked %s is overwritten by the merge."), path);
+
+		if (add_to_index_cacheinfo(r->index, their_mode, their_blob, path))
+			return 1;
+		return checkout_from_index(r->index, path);
+	} else if (!orig_blob && our_blob && their_blob &&
+		   oideq(our_blob, their_blob)) {
+		if (our_mode != their_mode)
+			return error(_("File %s added identically in both branches, "
+				       "but permissions conflict %o->%o."),
+				     path, our_mode, their_mode);
+
+		printf("Adding %s\n", path);
+
+		if (add_to_index_cacheinfo(r->index, our_mode, our_blob, path))
+			return 1;
+		return checkout_from_index(r->index, path);
+	} else if (our_blob && their_blob)
+		return do_merge_one_file(r->index,
+					 orig_blob, our_blob, their_blob, path,
+					 orig_mode, our_mode, their_mode);
+	else {
+		char *orig_hex = "", *our_hex = "", *their_hex = "";
+
+		if (orig_blob)
+			orig_hex = oid_to_hex(orig_blob);
+		if (our_blob)
+			our_hex = oid_to_hex(our_blob);
+		if (their_blob)
+			their_hex = oid_to_hex(their_blob);
+
+		return error(_("%s: Not handling case %s -> %s -> %s"),
+			path, orig_hex, our_hex, their_hex);
+	}
+
+	return 0;
+}
diff --git a/merge-strategies.h b/merge-strategies.h
new file mode 100644
index 0000000000..b527d145c7
--- /dev/null
+++ b/merge-strategies.h
@@ -0,0 +1,13 @@
+#ifndef MERGE_STRATEGIES_H
+#define MERGE_STRATEGIES_H
+
+#include "object.h"
+
+int merge_strategies_one_file(struct repository *r,
+			      const struct object_id *orig_blob,
+			      const struct object_id *our_blob,
+			      const struct object_id *their_blob, const char *path,
+			      unsigned int orig_mode, unsigned int our_mode,
+			      unsigned int their_mode);
+
+#endif /* MERGE_STRATEGIES_H */
-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [RFC PATCH v1 06/17] merge-index: libify merge_one_path() and merge_all()
  2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
                   ` (4 preceding siblings ...)
  2020-06-25 12:19 ` [RFC PATCH v1 05/17] merge-one-file: libify merge_one_file() Alban Gruin
@ 2020-06-25 12:19 ` Alban Gruin
  2020-06-26 10:13   ` Phillip Wood
  2020-06-25 12:19 ` [RFC PATCH v1 07/17] merge-resolve: rewrite in C Alban Gruin
                   ` (10 subsequent siblings)
  16 siblings, 1 reply; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

The "resolve" and "octopus" merge strategies do not call directly `git
merge-one-file', they delegate the work to another git command, `git
merge-index', that will loop over files in the index and call the
specified command.  Unfortunately, these functions are not part of
libgit.a, which means that once rewritten, the strategies would still
have to invoke `merge-one-file' by spawning a new process first.

To avoid this, this moves merge_one_path(), merge_all(), and their
helpers to merge-strategies.c.  They also take a callback to dictate
what they should do for each file.  For now, only one launching a new
process is defined to preserve the behaviour of the builtin version.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---

Notes:
    This patch is best viewed with `--color-moved'.

 builtin/merge-index.c | 77 +++------------------------------
 merge-strategies.c    | 99 +++++++++++++++++++++++++++++++++++++++++++
 merge-strategies.h    | 17 ++++++++
 3 files changed, 123 insertions(+), 70 deletions(-)

diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index 38ea6ad6ca..6cb666cc78 100644
--- a/builtin/merge-index.c
+++ b/builtin/merge-index.c
@@ -1,74 +1,11 @@
 #define USE_THE_INDEX_COMPATIBILITY_MACROS
 #include "builtin.h"
-#include "run-command.h"
-
-static const char *pgm;
-static int one_shot, quiet;
-static int err;
-
-static int merge_entry(int pos, const char *path)
-{
-	int found;
-	const char *arguments[] = { pgm, "", "", "", path, "", "", "", NULL };
-	char hexbuf[4][GIT_MAX_HEXSZ + 1];
-	char ownbuf[4][60];
-
-	if (pos >= active_nr)
-		die("git merge-index: %s not in the cache", path);
-	found = 0;
-	do {
-		const struct cache_entry *ce = active_cache[pos];
-		int stage = ce_stage(ce);
-
-		if (strcmp(ce->name, path))
-			break;
-		found++;
-		oid_to_hex_r(hexbuf[stage], &ce->oid);
-		xsnprintf(ownbuf[stage], sizeof(ownbuf[stage]), "%o", ce->ce_mode);
-		arguments[stage] = hexbuf[stage];
-		arguments[stage + 4] = ownbuf[stage];
-	} while (++pos < active_nr);
-	if (!found)
-		die("git merge-index: %s not in the cache", path);
-
-	if (run_command_v_opt(arguments, 0)) {
-		if (one_shot)
-			err++;
-		else {
-			if (!quiet)
-				die("merge program failed");
-			exit(1);
-		}
-	}
-	return found;
-}
-
-static void merge_one_path(const char *path)
-{
-	int pos = cache_name_pos(path, strlen(path));
-
-	/*
-	 * If it already exists in the cache as stage0, it's
-	 * already merged and there is nothing to do.
-	 */
-	if (pos < 0)
-		merge_entry(-pos-1, path);
-}
-
-static void merge_all(void)
-{
-	int i;
-	for (i = 0; i < active_nr; i++) {
-		const struct cache_entry *ce = active_cache[i];
-		if (!ce_stage(ce))
-			continue;
-		i += merge_entry(i, ce->name)-1;
-	}
-}
+#include "merge-strategies.h"
 
 int cmd_merge_index(int argc, const char **argv, const char *prefix)
 {
-	int i, force_file = 0;
+	int i, force_file = 0, err = 0, one_shot = 0, quiet = 0;
+	const char *pgm;
 
 	/* Without this we cannot rely on waitpid() to tell
 	 * what happened to our children.
@@ -98,14 +35,14 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 			if (!strcmp(arg, "-a")) {
-				merge_all();
+				err |= merge_all(&the_index, one_shot, quiet,
+						 merge_program_cb, (void *)pgm);
 				continue;
 			}
 			die("git merge-index: unknown option %s", arg);
 		}
-		merge_one_path(arg);
+		err |= merge_one_path(&the_index, one_shot, quiet, arg,
+				      merge_program_cb, (void *)pgm);
 	}
-	if (err && !quiet)
-		die("merge program failed");
 	return err;
 }
diff --git a/merge-strategies.c b/merge-strategies.c
index 3a9fce9f22..f4c0b4acd6 100644
--- a/merge-strategies.c
+++ b/merge-strategies.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "dir.h"
 #include "merge-strategies.h"
+#include "run-command.h"
 #include "xdiff-interface.h"
 
 static int add_to_index_cacheinfo(struct index_state *istate,
@@ -189,3 +190,101 @@ int merge_strategies_one_file(struct repository *r,
 
 	return 0;
 }
+
+int merge_program_cb(const struct object_id *orig_blob,
+		     const struct object_id *our_blob,
+		     const struct object_id *their_blob, const char *path,
+		     unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode,
+		     void *data)
+{
+	char ownbuf[3][60] = {{0}};
+	const char *arguments[] = { (char *)data, "", "", "", path,
+				    ownbuf[0], ownbuf[1], ownbuf[2],
+				    NULL };
+
+	if (orig_blob)
+		arguments[1] = oid_to_hex(orig_blob);
+	if (our_blob)
+		arguments[2] = oid_to_hex(our_blob);
+	if (their_blob)
+		arguments[3] = oid_to_hex(their_blob);
+
+	xsnprintf(ownbuf[0], sizeof(ownbuf[0]), "%o", orig_mode);
+	xsnprintf(ownbuf[1], sizeof(ownbuf[1]), "%o", our_mode);
+	xsnprintf(ownbuf[2], sizeof(ownbuf[2]), "%o", their_mode);
+
+	return run_command_v_opt(arguments, 0);
+}
+
+static int merge_entry(struct index_state *istate, int quiet, int pos,
+		       const char *path, merge_cb cb, void *data)
+{
+	int found = 0;
+	const struct object_id *oids[3] = {NULL};
+	unsigned int modes[3] = {0};
+
+	do {
+		const struct cache_entry *ce = istate->cache[pos];
+		int stage = ce_stage(ce);
+
+		if (strcmp(ce->name, path))
+			break;
+		found++;
+		oids[stage - 1] = &ce->oid;
+		modes[stage - 1] = ce->ce_mode;
+	} while (++pos < istate->cache_nr);
+	if (!found)
+		return error(_("%s is not in the cache"), path);
+
+	if (cb(oids[0], oids[1], oids[2], path, modes[0], modes[1], modes[2], data)) {
+		if (!quiet)
+			error(_("Merge program failed"));
+		return -2;
+	}
+
+	return found;
+}
+
+int merge_one_path(struct index_state *istate, int oneshot, int quiet,
+		   const char *path, merge_cb cb, void *data)
+{
+	int pos = index_name_pos(istate, path, strlen(path)), ret;
+
+	/*
+	 * If it already exists in the cache as stage0, it's
+	 * already merged and there is nothing to do.
+	 */
+	if (pos < 0) {
+		ret = merge_entry(istate, quiet, -pos - 1, path, cb, data);
+		if (ret == -1)
+			return -1;
+		else if (ret == -2)
+			return 1;
+	}
+	return 0;
+}
+
+int merge_all(struct index_state *istate, int oneshot, int quiet,
+	      merge_cb cb, void *data)
+{
+	int err = 0, i, ret;
+	for (i = 0; i < istate->cache_nr; i++) {
+		const struct cache_entry *ce = istate->cache[i];
+		if (!ce_stage(ce))
+			continue;
+
+		ret = merge_entry(istate, quiet, i, ce->name, cb, data);
+		if (ret > 0)
+			i += ret - 1;
+		else if (ret == -1)
+			return -1;
+		else if (ret == -2) {
+			if (oneshot)
+				err++;
+			else
+				return 1;
+		}
+	}
+
+	return err;
+}
diff --git a/merge-strategies.h b/merge-strategies.h
index b527d145c7..cf78d7eaf4 100644
--- a/merge-strategies.h
+++ b/merge-strategies.h
@@ -10,4 +10,21 @@ int merge_strategies_one_file(struct repository *r,
 			      unsigned int orig_mode, unsigned int our_mode,
 			      unsigned int their_mode);
 
+typedef int (*merge_cb)(const struct object_id *orig_blob,
+			const struct object_id *our_blob,
+			const struct object_id *their_blob, const char *path,
+			unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode,
+			void *data);
+
+int merge_program_cb(const struct object_id *orig_blob,
+		     const struct object_id *our_blob,
+		     const struct object_id *their_blob, const char *path,
+		     unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode,
+		     void *data);
+
+int merge_one_path(struct index_state *istate, int oneshot, int quiet,
+		   const char *path, merge_cb cb, void *data);
+int merge_all(struct index_state *istate, int oneshot, int quiet,
+	      merge_cb cb, void *data);
+
 #endif /* MERGE_STRATEGIES_H */
-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [RFC PATCH v1 07/17] merge-resolve: rewrite in C
  2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
                   ` (5 preceding siblings ...)
  2020-06-25 12:19 ` [RFC PATCH v1 06/17] merge-index: libify merge_one_path() and merge_all() Alban Gruin
@ 2020-06-25 12:19 ` Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 08/17] merge-resolve: remove calls to external processes Alban Gruin
                   ` (9 subsequent siblings)
  16 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

This rewrites `git merge-resolve' from shell to C.  As for `git
merge-one-file', this port keeps using external processes for operations
on the index, or to call `git merge-one-file'.  This will be addressed
in the next two commits.

The parameters of merge_resolve() will be surprising at first glance:
why using a commit list for `bases' and `remote', where we could use an
oid array, and a pointer to an oid?  Because, in a later commit,
try_merge_strategy() will be able to call merge_resolve() directly, and
it already uses a commit list for `bases' (`common') and
`remote' (`remoteheads'), and a string for `head_arg'.  To reduce
frictions later, merge_resolve() takes the same types of parameters.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 Makefile                |   2 +-
 builtin.h               |   1 +
 builtin/merge-resolve.c | 112 ++++++++++++++++++++++++++++++++++++++++
 git-merge-resolve.sh    |  54 -------------------
 git.c                   |   1 +
 5 files changed, 115 insertions(+), 55 deletions(-)
 create mode 100644 builtin/merge-resolve.c
 delete mode 100755 git-merge-resolve.sh

diff --git a/Makefile b/Makefile
index 1ab4d160cb..ccea651ac8 100644
--- a/Makefile
+++ b/Makefile
@@ -596,7 +596,6 @@ SCRIPT_SH += git-bisect.sh
 SCRIPT_SH += git-difftool--helper.sh
 SCRIPT_SH += git-filter-branch.sh
 SCRIPT_SH += git-merge-octopus.sh
-SCRIPT_SH += git-merge-resolve.sh
 SCRIPT_SH += git-mergetool.sh
 SCRIPT_SH += git-quiltimport.sh
 SCRIPT_SH += git-request-pull.sh
@@ -1092,6 +1091,7 @@ BUILTIN_OBJS += builtin/merge-index.o
 BUILTIN_OBJS += builtin/merge-one-file.o
 BUILTIN_OBJS += builtin/merge-ours.o
 BUILTIN_OBJS += builtin/merge-recursive.o
+BUILTIN_OBJS += builtin/merge-resolve.o
 BUILTIN_OBJS += builtin/merge-tree.o
 BUILTIN_OBJS += builtin/merge.o
 BUILTIN_OBJS += builtin/mktag.o
diff --git a/builtin.h b/builtin.h
index 9205d5ecdc..6ea207c9fd 100644
--- a/builtin.h
+++ b/builtin.h
@@ -174,6 +174,7 @@ int cmd_merge_ours(int argc, const char **argv, const char *prefix);
 int cmd_merge_file(int argc, const char **argv, const char *prefix);
 int cmd_merge_one_file(int argc, const char **argv, const char *prefix);
 int cmd_merge_recursive(int argc, const char **argv, const char *prefix);
+int cmd_merge_resolve(int argc, const char **argv, const char *prefix);
 int cmd_merge_tree(int argc, const char **argv, const char *prefix);
 int cmd_mktag(int argc, const char **argv, const char *prefix);
 int cmd_mktree(int argc, const char **argv, const char *prefix);
diff --git a/builtin/merge-resolve.c b/builtin/merge-resolve.c
new file mode 100644
index 0000000000..c66fef7b7f
--- /dev/null
+++ b/builtin/merge-resolve.c
@@ -0,0 +1,112 @@
+/*
+ * Builtin "git merge-resolve"
+ *
+ * Copyright (c) 2020 Alban Gruin
+ *
+ * Based on git-merge-resolve.sh, written by Linus Torvalds and Junio C
+ * Hamano.
+ *
+ * Resolve two trees, using enhanced multi-base read-tree.
+ */
+
+#include "cache.h"
+#include "builtin.h"
+#include "run-command.h"
+
+static int merge_resolve(struct commit_list *bases, const char *head_arg,
+			 struct commit_list *remote)
+{
+	struct commit_list *j;
+	struct child_process cp_update = CHILD_PROCESS_INIT,
+		cp_read = CHILD_PROCESS_INIT,
+		cp_write = CHILD_PROCESS_INIT;
+
+	cp_update.git_cmd = 1;
+	argv_array_pushl(&cp_update.args, "update-index", "-q", "--refresh", NULL);
+	run_command(&cp_update);
+
+	cp_read.git_cmd = 1;
+	argv_array_pushl(&cp_read.args, "read-tree", "-u", "-m", "--aggressive", NULL);
+
+	for (j = bases; j && j->item; j = j->next)
+		argv_array_push(&cp_read.args, oid_to_hex(&j->item->object.oid));
+
+	if (head_arg)
+		argv_array_push(&cp_read.args, head_arg);
+	if (remote && remote->item)
+		argv_array_push(&cp_read.args, oid_to_hex(&remote->item->object.oid));
+
+	if (run_command(&cp_read))
+		return 2;
+
+	puts("Trying simple merge.");
+
+	cp_write.git_cmd = 1;
+	cp_write.no_stdout = 1;
+	cp_write.no_stderr = 1;
+	argv_array_push(&cp_write.args, "write-tree");
+	if (run_command(&cp_write)) {
+		struct child_process cp_merge = CHILD_PROCESS_INIT;
+
+		puts("Simple merge failed, trying Automatic merge.");
+
+		cp_merge.git_cmd = 1;
+		argv_array_pushl(&cp_merge.args, "merge-index", "-o",
+				 "git-merge-one-file", "-a", NULL);
+		if (run_command(&cp_merge))
+			return 1;
+	}
+
+	return 0;
+}
+
+static const char builtin_merge_resolve_usage[] =
+	"git merge-resolve <bases>... -- <head> <remote>";
+
+int cmd_merge_resolve(int argc, const char **argv, const char *prefix)
+{
+	int i, is_baseless = 1, sep_seen = 0;
+	const char *head = NULL;
+	struct commit_list *bases = NULL, *remote = NULL;
+	struct commit_list **next_base = &bases;
+
+	if (argc < 5)
+		usage(builtin_merge_resolve_usage);
+
+	/* The first parameters up to -- are merge bases; the rest are
+	 * heads. */
+	for (i = 1; i < argc; i++) {
+		if (strcmp(argv[i], "--") == 0)
+			sep_seen = 1;
+		else if (strcmp(argv[i], "-h") == 0)
+			usage(builtin_merge_resolve_usage);
+		else if (sep_seen && !head)
+			head = argv[i];
+		else if (remote) {
+			/* Give up if we are given two or more remotes.
+			 * Not handling octopus. */
+			return 2;
+		} else {
+			struct object_id oid;
+
+			get_oid(argv[i], &oid);
+			is_baseless &= sep_seen;
+
+			if (!oideq(&oid, the_hash_algo->empty_tree)) {
+				struct commit *commit;
+				commit = lookup_commit_or_die(&oid, argv[i]);
+
+				if (sep_seen)
+					commit_list_append(commit, &remote);
+				else
+					next_base = commit_list_append(commit, next_base);
+			}
+		}
+	}
+
+	/* Give up if this is a baseless merge. */
+	if (is_baseless)
+		return 2;
+
+	return merge_resolve(bases, head, remote);
+}
diff --git a/git-merge-resolve.sh b/git-merge-resolve.sh
deleted file mode 100755
index 343fe7bccd..0000000000
--- a/git-merge-resolve.sh
+++ /dev/null
@@ -1,54 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2005 Linus Torvalds
-# Copyright (c) 2005 Junio C Hamano
-#
-# Resolve two trees, using enhanced multi-base read-tree.
-
-# The first parameters up to -- are merge bases; the rest are heads.
-bases= head= remotes= sep_seen=
-for arg
-do
-	case ",$sep_seen,$head,$arg," in
-	*,--,)
-		sep_seen=yes
-		;;
-	,yes,,*)
-		head=$arg
-		;;
-	,yes,*)
-		remotes="$remotes$arg "
-		;;
-	*)
-		bases="$bases$arg "
-		;;
-	esac
-done
-
-# Give up if we are given two or more remotes -- not handling octopus.
-case "$remotes" in
-?*' '?*)
-	exit 2 ;;
-esac
-
-# Give up if this is a baseless merge.
-if test '' = "$bases"
-then
-	exit 2
-fi
-
-git update-index -q --refresh
-git read-tree -u -m --aggressive $bases $head $remotes || exit 2
-echo "Trying simple merge."
-if result_tree=$(git write-tree 2>/dev/null)
-then
-	exit 0
-else
-	echo "Simple merge failed, trying Automatic merge."
-	if git merge-index -o git-merge-one-file -a
-	then
-		exit 0
-	else
-		exit 1
-	fi
-fi
diff --git a/git.c b/git.c
index 058d91a2a5..2e92019493 100644
--- a/git.c
+++ b/git.c
@@ -536,6 +536,7 @@ static struct cmd_struct commands[] = {
 	{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-resolve", cmd_merge_resolve, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-tree", cmd_merge_tree, RUN_SETUP | NO_PARSEOPT },
 	{ "mktag", cmd_mktag, RUN_SETUP | NO_PARSEOPT },
-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [RFC PATCH v1 08/17] merge-resolve: remove calls to external processes
  2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
                   ` (6 preceding siblings ...)
  2020-06-25 12:19 ` [RFC PATCH v1 07/17] merge-resolve: rewrite in C Alban Gruin
@ 2020-06-25 12:19 ` Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 09/17] merge-resolve: libify merge_resolve() Alban Gruin
                   ` (8 subsequent siblings)
  16 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

This removes calls to external processes to avoid reading and writing
the index over and over again.

 - The call to `update-index -q --refresh' is replaced by a call to
   refresh_index().

 - The call to `read-tree' is replaced by a call to unpack_trees() (and
   all the setup needed).

 - The call to `write-tree' is replaced by a call to
   write_index_as_tree().

 - The call to `merge-index', needed to invoke `git merge-one-file', is
   replaced by a call to the new merge_all() function.  A callback
   function, merge_one_file_cb(), is added to allow it to call
   merge_one_file() without forking.

Here too, the index is read in cmd_merge_resolve(), but merge_resolve()
takes care of writing it back to the disk.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/merge-resolve.c | 103 ++++++++++++++++++++++++++++------------
 merge-strategies.c      |  11 +++++
 merge-strategies.h      |   6 +++
 3 files changed, 89 insertions(+), 31 deletions(-)

diff --git a/builtin/merge-resolve.c b/builtin/merge-resolve.c
index c66fef7b7f..2c364fcdb0 100644
--- a/builtin/merge-resolve.c
+++ b/builtin/merge-resolve.c
@@ -10,54 +10,91 @@
  */
 
 #include "cache.h"
+#include "cache-tree.h"
 #include "builtin.h"
-#include "run-command.h"
+#include "lockfile.h"
+#include "merge-strategies.h"
+#include "unpack-trees.h"
+
+static int add_tree(const struct object_id *oid, struct tree_desc *t)
+{
+	struct tree *tree;
+
+	tree = parse_tree_indirect(oid);
+	if (parse_tree(tree))
+		return -1;
+
+	init_tree_desc(t, tree->buffer, tree->size);
+	return 0;
+}
 
 static int merge_resolve(struct commit_list *bases, const char *head_arg,
 			 struct commit_list *remote)
 {
+	int i = 0;
+	struct lock_file lock = LOCK_INIT;
+	struct tree_desc t[MAX_UNPACK_TREES];
+	struct unpack_trees_options opts;
+	struct object_id head, oid;
 	struct commit_list *j;
-	struct child_process cp_update = CHILD_PROCESS_INIT,
-		cp_read = CHILD_PROCESS_INIT,
-		cp_write = CHILD_PROCESS_INIT;
-
-	cp_update.git_cmd = 1;
-	argv_array_pushl(&cp_update.args, "update-index", "-q", "--refresh", NULL);
-	run_command(&cp_update);
-
-	cp_read.git_cmd = 1;
-	argv_array_pushl(&cp_read.args, "read-tree", "-u", "-m", "--aggressive", NULL);
-
-	for (j = bases; j && j->item; j = j->next)
-		argv_array_push(&cp_read.args, oid_to_hex(&j->item->object.oid));
 
 	if (head_arg)
-		argv_array_push(&cp_read.args, head_arg);
-	if (remote && remote->item)
-		argv_array_push(&cp_read.args, oid_to_hex(&remote->item->object.oid));
+		get_oid(head_arg, &head);
 
-	if (run_command(&cp_read))
-		return 2;
+	repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
+	refresh_index(the_repository->index, 0, NULL, NULL, NULL);
+
+	memset(&opts, 0, sizeof(opts));
+	opts.head_idx = 1;
+	opts.src_index = the_repository->index;
+	opts.dst_index = the_repository->index;
+	opts.update = 1;
+	opts.merge = 1;
+	opts.aggressive = 1;
+
+	for (j = bases; j; j = j->next) {
+		if (add_tree(&j->item->object.oid, t + (i++)))
+			goto out;
+	}
+
+	if (head_arg && add_tree(&head, t + (i++)))
+		goto out;
+	if (remote && add_tree(&remote->item->object.oid, t + (i++)))
+		goto out;
+
+	if (i == 1)
+		opts.fn = oneway_merge;
+	else if (i == 2) {
+		opts.fn = twoway_merge;
+		opts.initial_checkout = is_index_unborn(the_repository->index);
+	} else if (i >= 3) {
+		opts.fn = threeway_merge;
+		opts.head_idx = i - 1;
+	}
+
+	if (unpack_trees(i, t, &opts))
+		goto out;
 
 	puts("Trying simple merge.");
+	write_locked_index(the_repository->index, &lock, COMMIT_LOCK);
 
-	cp_write.git_cmd = 1;
-	cp_write.no_stdout = 1;
-	cp_write.no_stderr = 1;
-	argv_array_push(&cp_write.args, "write-tree");
-	if (run_command(&cp_write)) {
-		struct child_process cp_merge = CHILD_PROCESS_INIT;
+	if (write_index_as_tree(&oid, the_repository->index,
+				the_repository->index_file, 0, NULL)) {
+		int ret;
 
-		puts("Simple merge failed, trying Automatic merge.");
+		repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
+		ret = merge_all(the_repository->index, 0, 0,
+				merge_one_file_cb, the_repository);
 
-		cp_merge.git_cmd = 1;
-		argv_array_pushl(&cp_merge.args, "merge-index", "-o",
-				 "git-merge-one-file", "-a", NULL);
-		if (run_command(&cp_merge))
-			return 1;
+		write_locked_index(the_repository->index, &lock, COMMIT_LOCK);
+		return !!ret;
 	}
 
 	return 0;
+
+ out:
+	rollback_lock_file(&lock);
+	return 2;
 }
 
 static const char builtin_merge_resolve_usage[] =
@@ -73,6 +110,10 @@ int cmd_merge_resolve(int argc, const char **argv, const char *prefix)
 	if (argc < 5)
 		usage(builtin_merge_resolve_usage);
 
+	setup_work_tree();
+	if (repo_read_index(the_repository) < 0)
+		die("invalid index");
+
 	/* The first parameters up to -- are merge bases; the rest are
 	 * heads. */
 	for (i = 1; i < argc; i++) {
diff --git a/merge-strategies.c b/merge-strategies.c
index f4c0b4acd6..39bfa1af7b 100644
--- a/merge-strategies.c
+++ b/merge-strategies.c
@@ -191,6 +191,17 @@ int merge_strategies_one_file(struct repository *r,
 	return 0;
 }
 
+int merge_one_file_cb(const struct object_id *orig_blob,
+		      const struct object_id *our_blob,
+		      const struct object_id *their_blob, const char *path,
+		      unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode,
+		      void *data)
+{
+	return merge_strategies_one_file((struct repository *)data,
+					 orig_blob, our_blob, their_blob, path,
+					 orig_mode, our_mode, their_mode);
+}
+
 int merge_program_cb(const struct object_id *orig_blob,
 		     const struct object_id *our_blob,
 		     const struct object_id *their_blob, const char *path,
diff --git a/merge-strategies.h b/merge-strategies.h
index cf78d7eaf4..40e175ca39 100644
--- a/merge-strategies.h
+++ b/merge-strategies.h
@@ -16,6 +16,12 @@ typedef int (*merge_cb)(const struct object_id *orig_blob,
 			unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode,
 			void *data);
 
+int merge_one_file_cb(const struct object_id *orig_blob,
+		      const struct object_id *our_blob,
+		      const struct object_id *their_blob, const char *path,
+		      unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode,
+		      void *data);
+
 int merge_program_cb(const struct object_id *orig_blob,
 		     const struct object_id *our_blob,
 		     const struct object_id *their_blob, const char *path,
-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [RFC PATCH v1 09/17] merge-resolve: libify merge_resolve()
  2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
                   ` (7 preceding siblings ...)
  2020-06-25 12:19 ` [RFC PATCH v1 08/17] merge-resolve: remove calls to external processes Alban Gruin
@ 2020-06-25 12:19 ` Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 10/17] merge-recursive: move better_branch_name() to merge.c Alban Gruin
                   ` (7 subsequent siblings)
  16 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

This moves merge_resolve() (and its helper functions) to
merge-strategies.c.  This will enable `git merge' and the sequencer to
directly call it instead of forking.

Here too, this is not a faithful copy-and-paste; the new
merge_resolve() (renamed merge_strategies_resolve()) takes a pointer to
the repository, instead of using `the_repository'.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---

Notes:
    This patch is best viewed with `--color-moved'.

 builtin/merge-resolve.c | 86 +----------------------------------------
 merge-strategies.c      | 85 ++++++++++++++++++++++++++++++++++++++++
 merge-strategies.h      |  5 +++
 3 files changed, 91 insertions(+), 85 deletions(-)

diff --git a/builtin/merge-resolve.c b/builtin/merge-resolve.c
index 2c364fcdb0..59f734473b 100644
--- a/builtin/merge-resolve.c
+++ b/builtin/merge-resolve.c
@@ -10,92 +10,8 @@
  */
 
 #include "cache.h"
-#include "cache-tree.h"
 #include "builtin.h"
-#include "lockfile.h"
 #include "merge-strategies.h"
-#include "unpack-trees.h"
-
-static int add_tree(const struct object_id *oid, struct tree_desc *t)
-{
-	struct tree *tree;
-
-	tree = parse_tree_indirect(oid);
-	if (parse_tree(tree))
-		return -1;
-
-	init_tree_desc(t, tree->buffer, tree->size);
-	return 0;
-}
-
-static int merge_resolve(struct commit_list *bases, const char *head_arg,
-			 struct commit_list *remote)
-{
-	int i = 0;
-	struct lock_file lock = LOCK_INIT;
-	struct tree_desc t[MAX_UNPACK_TREES];
-	struct unpack_trees_options opts;
-	struct object_id head, oid;
-	struct commit_list *j;
-
-	if (head_arg)
-		get_oid(head_arg, &head);
-
-	repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
-	refresh_index(the_repository->index, 0, NULL, NULL, NULL);
-
-	memset(&opts, 0, sizeof(opts));
-	opts.head_idx = 1;
-	opts.src_index = the_repository->index;
-	opts.dst_index = the_repository->index;
-	opts.update = 1;
-	opts.merge = 1;
-	opts.aggressive = 1;
-
-	for (j = bases; j; j = j->next) {
-		if (add_tree(&j->item->object.oid, t + (i++)))
-			goto out;
-	}
-
-	if (head_arg && add_tree(&head, t + (i++)))
-		goto out;
-	if (remote && add_tree(&remote->item->object.oid, t + (i++)))
-		goto out;
-
-	if (i == 1)
-		opts.fn = oneway_merge;
-	else if (i == 2) {
-		opts.fn = twoway_merge;
-		opts.initial_checkout = is_index_unborn(the_repository->index);
-	} else if (i >= 3) {
-		opts.fn = threeway_merge;
-		opts.head_idx = i - 1;
-	}
-
-	if (unpack_trees(i, t, &opts))
-		goto out;
-
-	puts("Trying simple merge.");
-	write_locked_index(the_repository->index, &lock, COMMIT_LOCK);
-
-	if (write_index_as_tree(&oid, the_repository->index,
-				the_repository->index_file, 0, NULL)) {
-		int ret;
-
-		repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
-		ret = merge_all(the_repository->index, 0, 0,
-				merge_one_file_cb, the_repository);
-
-		write_locked_index(the_repository->index, &lock, COMMIT_LOCK);
-		return !!ret;
-	}
-
-	return 0;
-
- out:
-	rollback_lock_file(&lock);
-	return 2;
-}
 
 static const char builtin_merge_resolve_usage[] =
 	"git merge-resolve <bases>... -- <head> <remote>";
@@ -149,5 +65,5 @@ int cmd_merge_resolve(int argc, const char **argv, const char *prefix)
 	if (is_baseless)
 		return 2;
 
-	return merge_resolve(bases, head, remote);
+	return merge_strategies_resolve(the_repository, bases, head, remote);
 }
diff --git a/merge-strategies.c b/merge-strategies.c
index 39bfa1af7b..a12c575590 100644
--- a/merge-strategies.c
+++ b/merge-strategies.c
@@ -1,7 +1,10 @@
 #include "cache.h"
+#include "cache-tree.h"
 #include "dir.h"
+#include "lockfile.h"
 #include "merge-strategies.h"
 #include "run-command.h"
+#include "unpack-trees.h"
 #include "xdiff-interface.h"
 
 static int add_to_index_cacheinfo(struct index_state *istate,
@@ -299,3 +302,85 @@ int merge_all(struct index_state *istate, int oneshot, int quiet,
 
 	return err;
 }
+
+static int add_tree(const struct object_id *oid, struct tree_desc *t)
+{
+	struct tree *tree;
+
+	tree = parse_tree_indirect(oid);
+	if (parse_tree(tree))
+		return -1;
+
+	init_tree_desc(t, tree->buffer, tree->size);
+	return 0;
+}
+
+int merge_strategies_resolve(struct repository *r,
+			     struct commit_list *bases, const char *head_arg,
+			     struct commit_list *remote)
+{
+	int i = 0;
+	struct lock_file lock = LOCK_INIT;
+	struct tree_desc t[MAX_UNPACK_TREES];
+	struct unpack_trees_options opts;
+	struct object_id head, oid;
+	struct commit_list *j;
+
+	if (head_arg)
+		get_oid(head_arg, &head);
+
+	repo_hold_locked_index(r, &lock, LOCK_DIE_ON_ERROR);
+	refresh_index(r->index, 0, NULL, NULL, NULL);
+
+	memset(&opts, 0, sizeof(opts));
+	opts.head_idx = 1;
+	opts.src_index = r->index;
+	opts.dst_index = r->index;
+	opts.update = 1;
+	opts.merge = 1;
+	opts.aggressive = 1;
+
+	for (j = bases; j && j->item; j = j->next) {
+		if (add_tree(&j->item->object.oid, t + (i++)))
+			goto out;
+	}
+
+	if (head_arg && add_tree(&head, t + (i++)))
+		goto out;
+	if (remote && add_tree(&remote->item->object.oid, t + (i++)))
+		goto out;
+
+	if (i == 1)
+		opts.fn = oneway_merge;
+	else if (i == 2) {
+		opts.fn = twoway_merge;
+		opts.initial_checkout = is_index_unborn(r->index);
+	} else if (i >= 3) {
+		opts.fn = threeway_merge;
+		opts.head_idx = i - 1;
+	}
+
+	if (unpack_trees(i, t, &opts))
+		goto out;
+
+	puts("Trying simple merge.");
+	write_locked_index(r->index, &lock, COMMIT_LOCK);
+
+	if (write_index_as_tree(&oid, r->index, r->index_file,
+				WRITE_TREE_SILENT, NULL)) {
+		int ret;
+
+		puts("Simple merge failed, trying Automatic merge.");
+		repo_hold_locked_index(r, &lock, LOCK_DIE_ON_ERROR);
+		ret = merge_all(r->index, 0, 0, merge_one_file_cb, r);
+
+		write_locked_index(r->index, &lock, COMMIT_LOCK);
+		return !!ret;
+	}
+
+	return 0;
+
+ out:
+	rollback_lock_file(&lock);
+	return 2;
+}
diff --git a/merge-strategies.h b/merge-strategies.h
index 40e175ca39..778f8ce9d6 100644
--- a/merge-strategies.h
+++ b/merge-strategies.h
@@ -1,6 +1,7 @@
 #ifndef MERGE_STRATEGIES_H
 #define MERGE_STRATEGIES_H
 
+#include "commit.h"
 #include "object.h"
 
 int merge_strategies_one_file(struct repository *r,
@@ -33,4 +34,8 @@ int merge_one_path(struct index_state *istate, int oneshot, int quiet,
 int merge_all(struct index_state *istate, int oneshot, int quiet,
 	      merge_cb cb, void *data);
 
+int merge_strategies_resolve(struct repository *r,
+			     struct commit_list *bases, const char *head_arg,
+			     struct commit_list *remote);
+
 #endif /* MERGE_STRATEGIES_H */
-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [RFC PATCH v1 10/17] merge-recursive: move better_branch_name() to merge.c
  2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
                   ` (8 preceding siblings ...)
  2020-06-25 12:19 ` [RFC PATCH v1 09/17] merge-resolve: libify merge_resolve() Alban Gruin
@ 2020-06-25 12:19 ` Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 11/17] merge-octopus: rewrite in C Alban Gruin
                   ` (6 subsequent siblings)
  16 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

get_better_branch_name() will be used by rebase-octopus once it is
rewritten in C, so instead of duplicating it, this moves this function
preventively inside an appropriate file in libgit.a.  This function is
also renamed to reflect its usage by merge strategies.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---

Notes:
    This patch is best viewed with `--color-moved'.

 builtin/merge-recursive.c | 16 ++--------------
 cache.h                   |  2 +-
 merge.c                   | 12 ++++++++++++
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index a4bfd8fc51..972243b5e9 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -8,18 +8,6 @@
 static const char builtin_merge_recursive_usage[] =
 	"git %s <base>... -- <head> <remote> ...";
 
-static char *better_branch_name(const char *branch)
-{
-	static char githead_env[8 + GIT_MAX_HEXSZ + 1];
-	char *name;
-
-	if (strlen(branch) != the_hash_algo->hexsz)
-		return xstrdup(branch);
-	xsnprintf(githead_env, sizeof(githead_env), "GITHEAD_%s", branch);
-	name = getenv(githead_env);
-	return xstrdup(name ? name : branch);
-}
-
 int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 {
 	const struct object_id *bases[21];
@@ -75,8 +63,8 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 	if (get_oid(o.branch2, &h2))
 		die(_("could not resolve ref '%s'"), o.branch2);
 
-	o.branch1 = better1 = better_branch_name(o.branch1);
-	o.branch2 = better2 = better_branch_name(o.branch2);
+	o.branch1 = better1 = merge_get_better_branch_name(o.branch1);
+	o.branch2 = better2 = merge_get_better_branch_name(o.branch2);
 
 	if (o.verbosity >= 3)
 		printf(_("Merging %s with %s\n"), o.branch1, o.branch2);
diff --git a/cache.h b/cache.h
index 0f0485ecfe..bbbd8e352d 100644
--- a/cache.h
+++ b/cache.h
@@ -1915,7 +1915,7 @@ int checkout_fast_forward(struct repository *r,
 			  const struct object_id *from,
 			  const struct object_id *to,
 			  int overwrite_ignore);
-
+char *merge_get_better_branch_name(const char *branch);
 
 int sane_execvp(const char *file, char *const argv[]);
 
diff --git a/merge.c b/merge.c
index aa36de2f64..5f3d05268f 100644
--- a/merge.c
+++ b/merge.c
@@ -108,3 +108,15 @@ int checkout_fast_forward(struct repository *r,
 		return error(_("unable to write new index file"));
 	return 0;
 }
+
+char *merge_get_better_branch_name(const char *branch)
+{
+	static char githead_env[8 + GIT_MAX_HEXSZ + 1];
+	char *name;
+
+	if (strlen(branch) != the_hash_algo->hexsz)
+		return xstrdup(branch);
+	xsnprintf(githead_env, sizeof(githead_env), "GITHEAD_%s", branch);
+	name = getenv(githead_env);
+	return xstrdup(name ? name : branch);
+}
-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [RFC PATCH v1 11/17] merge-octopus: rewrite in C
  2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
                   ` (9 preceding siblings ...)
  2020-06-25 12:19 ` [RFC PATCH v1 10/17] merge-recursive: move better_branch_name() to merge.c Alban Gruin
@ 2020-06-25 12:19 ` Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 12/17] merge-octopus: remove calls to external processes Alban Gruin
                   ` (5 subsequent siblings)
  16 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

This rewrites `git merge-octopus' from shell to C.  As for the two last
conversions, this port keeps using external processes for operations on
the index, or to call `git merge-one-file'.  This will be addressed in
the next two commits.

Here to, merge_octopus() takes two commit lists and a string to reduce
frictions when try_merge_strategies() will be modified to call it
directly.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 Makefile                |   2 +-
 builtin.h               |   1 +
 builtin/merge-octopus.c | 241 ++++++++++++++++++++++++++++++++++++++++
 git-merge-octopus.sh    | 112 -------------------
 git.c                   |   1 +
 5 files changed, 244 insertions(+), 113 deletions(-)
 create mode 100644 builtin/merge-octopus.c
 delete mode 100755 git-merge-octopus.sh

diff --git a/Makefile b/Makefile
index ccea651ac8..8f45a3ec03 100644
--- a/Makefile
+++ b/Makefile
@@ -595,7 +595,6 @@ unexport CDPATH
 SCRIPT_SH += git-bisect.sh
 SCRIPT_SH += git-difftool--helper.sh
 SCRIPT_SH += git-filter-branch.sh
-SCRIPT_SH += git-merge-octopus.sh
 SCRIPT_SH += git-mergetool.sh
 SCRIPT_SH += git-quiltimport.sh
 SCRIPT_SH += git-request-pull.sh
@@ -1088,6 +1087,7 @@ BUILTIN_OBJS += builtin/mailsplit.o
 BUILTIN_OBJS += builtin/merge-base.o
 BUILTIN_OBJS += builtin/merge-file.o
 BUILTIN_OBJS += builtin/merge-index.o
+BUILTIN_OBJS += builtin/merge-octopus.o
 BUILTIN_OBJS += builtin/merge-one-file.o
 BUILTIN_OBJS += builtin/merge-ours.o
 BUILTIN_OBJS += builtin/merge-recursive.o
diff --git a/builtin.h b/builtin.h
index 6ea207c9fd..5a587ab70c 100644
--- a/builtin.h
+++ b/builtin.h
@@ -170,6 +170,7 @@ int cmd_mailsplit(int argc, const char **argv, const char *prefix);
 int cmd_merge(int argc, const char **argv, const char *prefix);
 int cmd_merge_base(int argc, const char **argv, const char *prefix);
 int cmd_merge_index(int argc, const char **argv, const char *prefix);
+int cmd_merge_octopus(int argc, const char **argv, const char *prefix);
 int cmd_merge_ours(int argc, const char **argv, const char *prefix);
 int cmd_merge_file(int argc, const char **argv, const char *prefix);
 int cmd_merge_one_file(int argc, const char **argv, const char *prefix);
diff --git a/builtin/merge-octopus.c b/builtin/merge-octopus.c
new file mode 100644
index 0000000000..6216beaa2b
--- /dev/null
+++ b/builtin/merge-octopus.c
@@ -0,0 +1,241 @@
+/*
+ * Builtin "git merge-octopus"
+ *
+ * Copyright (c) 2020 Alban Gruin
+ *
+ * Based on git-merge-octopus.sh, written by Junio C Hamano.
+ *
+ * Resolve two or more trees.
+ */
+
+#include "cache.h"
+#include "builtin.h"
+#include "commit-reach.h"
+#include "lockfile.h"
+#include "run-command.h"
+#include "unpack-trees.h"
+
+static int write_tree(struct tree **reference_tree)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf read_tree = STRBUF_INIT, err = STRBUF_INIT;
+	struct object_id oid;
+	int ret;
+
+	cp.git_cmd = 1;
+	argv_array_push(&cp.args, "write-tree");
+	ret = pipe_command(&cp, NULL, 0, &read_tree, 0, &err, 0);
+	if (err.len > 0)
+		fputs(err.buf, stderr);
+
+	strbuf_trim_trailing_newline(&read_tree);
+	get_oid(read_tree.buf, &oid);
+
+	*reference_tree = lookup_tree(the_repository, &oid);
+
+	strbuf_release(&read_tree);
+	strbuf_release(&err);
+	child_process_clear(&cp);
+
+	return ret;
+}
+
+static int merge_octopus(struct commit_list *bases, const char *head_arg,
+			 struct commit_list *remotes)
+{
+	int non_ff_merge = 0, ret = 0, references = 1;
+	struct commit **reference_commit;
+	struct tree *reference_tree;
+	struct commit_list *j;
+	struct object_id head;
+
+	get_oid(head_arg, &head);
+	reference_commit = xcalloc(commit_list_count(remotes) + 1, sizeof(struct commit *));
+	reference_commit[0] = lookup_commit_reference(the_repository, &head);
+	reference_tree = get_commit_tree(reference_commit[0]);
+
+	for (j = remotes; j; j = j->next) {
+		struct commit *c = j->item;
+		struct object_id *oid = &c->object.oid;
+		struct commit_list *common, *k;
+		char *branch_name;
+		int can_ff = 1;
+
+		if (ret) {
+			puts(_("Automated merge did not work."));
+			puts(_("Should not be doing an octopus."));
+
+			ret = 2;
+			goto out;
+		}
+
+		branch_name = merge_get_better_branch_name(oid_to_hex(oid));
+		common = get_merge_bases_many(c, references, reference_commit);
+
+		if (!common)
+			die(_("Unable to find common commit with %s"), branch_name);
+
+		for (k = common; k && !oideq(&k->item->object.oid, oid); k = k->next);
+
+		if (k) {
+			printf(_("Already up to date with %s\n"), branch_name);
+			free(branch_name);
+			free_commit_list(common);
+			continue;
+		}
+
+		if (!non_ff_merge) {
+			int i;
+
+			for (i = 0, k = common; k && i < references && can_ff; k = k->next, i++) {
+				can_ff = oideq(&k->item->object.oid,
+					       &reference_commit[i]->object.oid);
+			}
+		}
+
+		if (!non_ff_merge && can_ff) {
+			struct child_process cp = CHILD_PROCESS_INIT;
+
+			printf(_("Fast-forwarding to: %s\n"), branch_name);
+
+			cp.git_cmd = 1;
+			argv_array_pushl(&cp.args, "read-tree", "-u", "-m", NULL);
+			argv_array_push(&cp.args, oid_to_hex(&head));
+			argv_array_push(&cp.args, oid_to_hex(oid));
+
+			ret = run_command(&cp);
+			if (ret) {
+				free(branch_name);
+				free_commit_list(common);
+				goto out;
+			}
+
+			child_process_clear(&cp);
+			references = 0;
+			write_tree(&reference_tree);
+		} else {
+			struct commit_list *l;
+			struct tree *next = NULL;
+			struct child_process cp = CHILD_PROCESS_INIT;
+
+			non_ff_merge = 1;
+			printf(_("Trying simple merge with %s\n"), branch_name);
+
+			cp.git_cmd = 1;
+			argv_array_pushl(&cp.args, "read-tree", "-u", "-m", "--aggressive", NULL);
+
+			for (l = common; l; l = l->next)
+				argv_array_push(&cp.args, oid_to_hex(&l->item->object.oid));
+
+			argv_array_push(&cp.args, oid_to_hex(&reference_tree->object.oid));
+			argv_array_push(&cp.args, oid_to_hex(oid));
+
+			if (run_command(&cp)) {
+				ret = 2;
+
+				free(branch_name);
+				free_commit_list(common);
+
+				goto out;
+			}
+
+			child_process_clear(&cp);
+
+			if (write_tree(&next)) {
+				struct child_process cp = CHILD_PROCESS_INIT;
+				puts(_("Simple merge did not work, trying automatic merge."));
+
+				cp.git_cmd = 1;
+				argv_array_pushl(&cp.args, "merge-index", "-o",
+						 "git-merge-one-file", "-a", NULL);
+				if (run_command(&cp))
+					ret = 1;
+
+				child_process_clear(&cp);
+				write_tree(&next);
+			}
+
+			reference_tree = next;
+		}
+
+		reference_commit[references++] = c;
+
+		free(branch_name);
+		free_commit_list(common);
+	}
+
+out:
+	free(reference_commit);
+	return ret;
+}
+
+static const char builtin_merge_octopus_usage[] =
+	"git merge-octopus [<bases>...] -- <head> <remote1> <remote2> [<remotes>...]";
+
+int cmd_merge_octopus(int argc, const char **argv, const char *prefix)
+{
+	int i, sep_seen = 0;
+	struct commit_list *bases = NULL, *remotes = NULL;
+	struct commit_list **next_base = &bases, **next_remote = &remotes;
+	const char *head_arg = NULL;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf files = STRBUF_INIT;
+
+	if (argc < 5)
+		usage(builtin_merge_octopus_usage);
+
+	/* The first parameters up to -- are merge bases; the rest are
+	 * heads. */
+	for (i = 1; i < argc; i++) {
+		if (strcmp(argv[i], "--") == 0)
+			sep_seen = 1;
+		else if (strcmp(argv[i], "-h") == 0)
+			usage(builtin_merge_octopus_usage);
+		else if (sep_seen && !head_arg)
+			head_arg = argv[i];
+		else {
+			struct object_id oid;
+
+			get_oid(argv[i], &oid);
+
+			if (!oideq(&oid, the_hash_algo->empty_tree)) {
+				struct commit *commit;
+				commit = lookup_commit_or_die(&oid, argv[i]);
+
+				if (sep_seen)
+					next_remote = commit_list_append(commit, next_remote);
+				else
+					next_base = commit_list_append(commit, next_base);
+			}
+		}
+	}
+
+	/* Reject if this is not an octopus -- resolve should be used
+	 * instead. */
+	if (commit_list_count(remotes) < 2)
+		return 2;
+
+	cp.git_cmd = 1;
+	argv_array_pushl(&cp.args, "diff-index", "--cached",
+			 "--name-only", "HEAD", "--", NULL);
+	pipe_command(&cp, NULL, 0, &files, 0, NULL, 0);
+	child_process_clear(&cp);
+
+	if (files.len > 0) {
+		struct strbuf **s, **b;
+
+		s = strbuf_split(&files, '\n');
+
+		fprintf(stderr, _("Error: Your local changes to the following "
+				  "files would be overwritten by merge\n"));
+
+		for (b = s; *b; b++)
+			fprintf(stderr, "    %.*s", (int)(*b)->len, (*b)->buf);
+
+		strbuf_list_free(s);
+		strbuf_release(&files);
+		return 2;
+	}
+
+	return merge_octopus(bases, head_arg, remotes);
+}
diff --git a/git-merge-octopus.sh b/git-merge-octopus.sh
deleted file mode 100755
index 7d19d37951..0000000000
--- a/git-merge-octopus.sh
+++ /dev/null
@@ -1,112 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2005 Junio C Hamano
-#
-# Resolve two or more trees.
-#
-
-. git-sh-setup
-
-LF='
-'
-
-# The first parameters up to -- are merge bases; the rest are heads.
-bases= head= remotes= sep_seen=
-for arg
-do
-	case ",$sep_seen,$head,$arg," in
-	*,--,)
-		sep_seen=yes
-		;;
-	,yes,,*)
-		head=$arg
-		;;
-	,yes,*)
-		remotes="$remotes$arg "
-		;;
-	*)
-		bases="$bases$arg "
-		;;
-	esac
-done
-
-# Reject if this is not an octopus -- resolve should be used instead.
-case "$remotes" in
-?*' '?*)
-	;;
-*)
-	exit 2 ;;
-esac
-
-# MRC is the current "merge reference commit"
-# MRT is the current "merge result tree"
-
-if ! git diff-index --quiet --cached HEAD --
-then
-    gettextln "Error: Your local changes to the following files would be overwritten by merge"
-    git diff-index --cached --name-only HEAD -- | sed -e 's/^/    /'
-    exit 2
-fi
-MRC=$(git rev-parse --verify -q $head)
-MRT=$(git write-tree)
-NON_FF_MERGE=0
-OCTOPUS_FAILURE=0
-for SHA1 in $remotes
-do
-	case "$OCTOPUS_FAILURE" in
-	1)
-		# We allow only last one to have a hand-resolvable
-		# conflicts.  Last round failed and we still had
-		# a head to merge.
-		gettextln "Automated merge did not work."
-		gettextln "Should not be doing an octopus."
-		exit 2
-	esac
-
-	eval pretty_name=\${GITHEAD_$SHA1:-$SHA1}
-	if test "$SHA1" = "$pretty_name"
-	then
-		SHA1_UP="$(echo "$SHA1" | tr a-z A-Z)"
-		eval pretty_name=\${GITHEAD_$SHA1_UP:-$pretty_name}
-	fi
-	common=$(git merge-base --all $SHA1 $MRC) ||
-		die "$(eval_gettext "Unable to find common commit with \$pretty_name")"
-
-	case "$LF$common$LF" in
-	*"$LF$SHA1$LF"*)
-		eval_gettextln "Already up to date with \$pretty_name"
-		continue
-		;;
-	esac
-
-	if test "$common,$NON_FF_MERGE" = "$MRC,0"
-	then
-		# The first head being merged was a fast-forward.
-		# Advance MRC to the head being merged, and use that
-		# tree as the intermediate result of the merge.
-		# We still need to count this as part of the parent set.
-
-		eval_gettextln "Fast-forwarding to: \$pretty_name"
-		git read-tree -u -m $head $SHA1 || exit
-		MRC=$SHA1 MRT=$(git write-tree)
-		continue
-	fi
-
-	NON_FF_MERGE=1
-
-	eval_gettextln "Trying simple merge with \$pretty_name"
-	git read-tree -u -m --aggressive  $common $MRT $SHA1 || exit 2
-	next=$(git write-tree 2>/dev/null)
-	if test $? -ne 0
-	then
-		gettextln "Simple merge did not work, trying automatic merge."
-		git merge-index -o git-merge-one-file -a ||
-		OCTOPUS_FAILURE=1
-		next=$(git write-tree 2>/dev/null)
-	fi
-
-	MRC="$MRC $SHA1"
-	MRT=$next
-done
-
-exit "$OCTOPUS_FAILURE"
diff --git a/git.c b/git.c
index 2e92019493..28634cf61f 100644
--- a/git.c
+++ b/git.c
@@ -531,6 +531,7 @@ static struct cmd_struct commands[] = {
 	{ "merge-base", cmd_merge_base, RUN_SETUP },
 	{ "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
 	{ "merge-index", cmd_merge_index, RUN_SETUP | NO_PARSEOPT },
+	{ "merge-octopus", cmd_merge_octopus, RUN_SETUP | NO_PARSEOPT },
 	{ "merge-ours", cmd_merge_ours, RUN_SETUP | NO_PARSEOPT },
 	{ "merge-one-file", cmd_merge_one_file, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [RFC PATCH v1 12/17] merge-octopus: remove calls to external processes
  2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
                   ` (10 preceding siblings ...)
  2020-06-25 12:19 ` [RFC PATCH v1 11/17] merge-octopus: rewrite in C Alban Gruin
@ 2020-06-25 12:19 ` Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 13/17] merge-octopus: libify merge_octopus() Alban Gruin
                   ` (4 subsequent siblings)
  16 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

This removes calls to external processes to avoid reading and writing
the index over and over again.

 - Calls to `read-tree -u -m (--aggressive)?' are replaced by calls to
   unpack_trees().

 - The call to `write-tree' is replaced by a call to
   write_index_as_tree().

 - The call to `diff-index ...' is replaced by a call to
   repo_index_has_changes(), and is moved from cmd_merge_octopus() to
   merge_octopus().

 - The call to `merge-index', needed to invoke `git merge-one-file', is
   replaced by a call to merge_all().

The index is read in cmd_merge_octopus(), and is wrote back by
merge_octopus().

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/merge-octopus.c | 155 ++++++++++++++++++++++------------------
 1 file changed, 86 insertions(+), 69 deletions(-)

diff --git a/builtin/merge-octopus.c b/builtin/merge-octopus.c
index 6216beaa2b..14310a4eb1 100644
--- a/builtin/merge-octopus.c
+++ b/builtin/merge-octopus.c
@@ -9,33 +9,70 @@
  */
 
 #include "cache.h"
+#include "cache-tree.h"
 #include "builtin.h"
 #include "commit-reach.h"
 #include "lockfile.h"
-#include "run-command.h"
+#include "merge-strategies.h"
 #include "unpack-trees.h"
 
+static int fast_forward(const struct object_id *oids, int nr, int aggressive)
+{
+	int i;
+	struct tree_desc t[MAX_UNPACK_TREES];
+	struct unpack_trees_options opts;
+	struct lock_file lock = LOCK_INIT;
+
+	repo_read_index_preload(the_repository, NULL, 0);
+	if (refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL))
+		return -1;
+
+	repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
+
+	memset(&opts, 0, sizeof(opts));
+	opts.head_idx = 1;
+	opts.src_index = the_repository->index;
+	opts.dst_index = the_repository->index;
+	opts.merge = 1;
+	opts.update = 1;
+	opts.aggressive = aggressive;
+
+	for (i = 0; i < nr; i++) {
+		struct tree *tree;
+		tree = parse_tree_indirect(oids + i);
+		if (parse_tree(tree))
+			return -1;
+		init_tree_desc(t + i, tree->buffer, tree->size);
+	}
+
+	if (nr == 1)
+		opts.fn = oneway_merge;
+	else if (nr == 2) {
+		opts.fn = twoway_merge;
+		opts.initial_checkout = is_index_unborn(the_repository->index);
+	} else if (nr >= 3) {
+		opts.fn = threeway_merge;
+		opts.head_idx = nr - 1;
+	}
+
+	if (unpack_trees(nr, t, &opts))
+		return -1;
+
+	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK))
+		return error(_("unable to write new index file"));
+
+	return 0;
+}
+
 static int write_tree(struct tree **reference_tree)
 {
-	struct child_process cp = CHILD_PROCESS_INIT;
-	struct strbuf read_tree = STRBUF_INIT, err = STRBUF_INIT;
 	struct object_id oid;
 	int ret;
 
-	cp.git_cmd = 1;
-	argv_array_push(&cp.args, "write-tree");
-	ret = pipe_command(&cp, NULL, 0, &read_tree, 0, &err, 0);
-	if (err.len > 0)
-		fputs(err.buf, stderr);
-
-	strbuf_trim_trailing_newline(&read_tree);
-	get_oid(read_tree.buf, &oid);
-
-	*reference_tree = lookup_tree(the_repository, &oid);
-
-	strbuf_release(&read_tree);
-	strbuf_release(&err);
-	child_process_clear(&cp);
+	ret = write_index_as_tree(&oid, the_repository->index,
+				  the_repository->index_file, 0, NULL);
+	if (!ret)
+		*reference_tree = lookup_tree(the_repository, &oid);
 
 	return ret;
 }
@@ -48,12 +85,23 @@ static int merge_octopus(struct commit_list *bases, const char *head_arg,
 	struct tree *reference_tree;
 	struct commit_list *j;
 	struct object_id head;
+	struct strbuf sb = STRBUF_INIT;
 
 	get_oid(head_arg, &head);
+
 	reference_commit = xcalloc(commit_list_count(remotes) + 1, sizeof(struct commit *));
 	reference_commit[0] = lookup_commit_reference(the_repository, &head);
 	reference_tree = get_commit_tree(reference_commit[0]);
 
+	if (repo_index_has_changes(the_repository, reference_tree, &sb)) {
+		error(_("Your local changes to the following files "
+			"would be overwritten by merge:\n  %s"),
+		      sb.buf);
+		strbuf_release(&sb);
+		ret = 2;
+		goto out;
+	}
+
 	for (j = remotes; j; j = j->next) {
 		struct commit *c = j->item;
 		struct object_id *oid = &c->object.oid;
@@ -94,43 +142,36 @@ static int merge_octopus(struct commit_list *bases, const char *head_arg,
 		}
 
 		if (!non_ff_merge && can_ff) {
-			struct child_process cp = CHILD_PROCESS_INIT;
-
+			struct object_id oids[2];
 			printf(_("Fast-forwarding to: %s\n"), branch_name);
 
-			cp.git_cmd = 1;
-			argv_array_pushl(&cp.args, "read-tree", "-u", "-m", NULL);
-			argv_array_push(&cp.args, oid_to_hex(&head));
-			argv_array_push(&cp.args, oid_to_hex(oid));
+			oidcpy(oids, &head);
+			oidcpy(oids + 1, oid);
 
-			ret = run_command(&cp);
+			ret = fast_forward(oids, 2, 0);
 			if (ret) {
 				free(branch_name);
 				free_commit_list(common);
 				goto out;
 			}
 
-			child_process_clear(&cp);
 			references = 0;
 			write_tree(&reference_tree);
 		} else {
-			struct commit_list *l;
+			int i = 0;
 			struct tree *next = NULL;
-			struct child_process cp = CHILD_PROCESS_INIT;
+			struct object_id oids[MAX_UNPACK_TREES];
 
 			non_ff_merge = 1;
 			printf(_("Trying simple merge with %s\n"), branch_name);
 
-			cp.git_cmd = 1;
-			argv_array_pushl(&cp.args, "read-tree", "-u", "-m", "--aggressive", NULL);
+			for (k = common; k; k = k->next)
+				oidcpy(oids + (i++), &k->item->object.oid);
 
-			for (l = common; l; l = l->next)
-				argv_array_push(&cp.args, oid_to_hex(&l->item->object.oid));
+			oidcpy(oids + (i++), &reference_tree->object.oid);
+			oidcpy(oids + (i++), oid);
 
-			argv_array_push(&cp.args, oid_to_hex(&reference_tree->object.oid));
-			argv_array_push(&cp.args, oid_to_hex(oid));
-
-			if (run_command(&cp)) {
+			if (fast_forward(oids, i, 1)) {
 				ret = 2;
 
 				free(branch_name);
@@ -139,19 +180,15 @@ static int merge_octopus(struct commit_list *bases, const char *head_arg,
 				goto out;
 			}
 
-			child_process_clear(&cp);
-
 			if (write_tree(&next)) {
-				struct child_process cp = CHILD_PROCESS_INIT;
+				struct lock_file lock = LOCK_INIT;
+
 				puts(_("Simple merge did not work, trying automatic merge."));
+				repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
+				ret = !!merge_all(the_repository->index, 0, 0,
+						  merge_one_file_cb, the_repository);
+				write_locked_index(the_repository->index, &lock, COMMIT_LOCK);
 
-				cp.git_cmd = 1;
-				argv_array_pushl(&cp.args, "merge-index", "-o",
-						 "git-merge-one-file", "-a", NULL);
-				if (run_command(&cp))
-					ret = 1;
-
-				child_process_clear(&cp);
 				write_tree(&next);
 			}
 
@@ -178,12 +215,14 @@ int cmd_merge_octopus(int argc, const char **argv, const char *prefix)
 	struct commit_list *bases = NULL, *remotes = NULL;
 	struct commit_list **next_base = &bases, **next_remote = &remotes;
 	const char *head_arg = NULL;
-	struct child_process cp = CHILD_PROCESS_INIT;
-	struct strbuf files = STRBUF_INIT;
 
 	if (argc < 5)
 		usage(builtin_merge_octopus_usage);
 
+	setup_work_tree();
+	if (repo_read_index(the_repository) < 0)
+		die("corrupted cache");
+
 	/* The first parameters up to -- are merge bases; the rest are
 	 * heads. */
 	for (i = 1; i < argc; i++) {
@@ -215,27 +254,5 @@ int cmd_merge_octopus(int argc, const char **argv, const char *prefix)
 	if (commit_list_count(remotes) < 2)
 		return 2;
 
-	cp.git_cmd = 1;
-	argv_array_pushl(&cp.args, "diff-index", "--cached",
-			 "--name-only", "HEAD", "--", NULL);
-	pipe_command(&cp, NULL, 0, &files, 0, NULL, 0);
-	child_process_clear(&cp);
-
-	if (files.len > 0) {
-		struct strbuf **s, **b;
-
-		s = strbuf_split(&files, '\n');
-
-		fprintf(stderr, _("Error: Your local changes to the following "
-				  "files would be overwritten by merge\n"));
-
-		for (b = s; *b; b++)
-			fprintf(stderr, "    %.*s", (int)(*b)->len, (*b)->buf);
-
-		strbuf_list_free(s);
-		strbuf_release(&files);
-		return 2;
-	}
-
 	return merge_octopus(bases, head_arg, remotes);
 }
-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [RFC PATCH v1 13/17] merge-octopus: libify merge_octopus()
  2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
                   ` (11 preceding siblings ...)
  2020-06-25 12:19 ` [RFC PATCH v1 12/17] merge-octopus: remove calls to external processes Alban Gruin
@ 2020-06-25 12:19 ` Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 14/17] merge: use the "resolve" strategy without forking Alban Gruin
                   ` (3 subsequent siblings)
  16 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

This moves merge_octopus() (and its helper functions) to
merge-strategies.c.  This will enable `git merge' and the sequencer to
directly call it instead of forking.

Once again, this is not a faithful copy-and-paste; the new
merge_octopus() (renamed merge_strategies_octopus()) takes a pointer to
the repository, instead of using `the_repository'.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---

Notes:
    This patch is best viewed with `--color-moved'.

 builtin/merge-octopus.c | 197 +---------------------------------------
 merge-strategies.c      | 191 ++++++++++++++++++++++++++++++++++++++
 merge-strategies.h      |   3 +
 3 files changed, 196 insertions(+), 195 deletions(-)

diff --git a/builtin/merge-octopus.c b/builtin/merge-octopus.c
index 14310a4eb1..37bbdf11cc 100644
--- a/builtin/merge-octopus.c
+++ b/builtin/merge-octopus.c
@@ -9,202 +9,9 @@
  */
 
 #include "cache.h"
-#include "cache-tree.h"
 #include "builtin.h"
-#include "commit-reach.h"
-#include "lockfile.h"
+#include "commit.h"
 #include "merge-strategies.h"
-#include "unpack-trees.h"
-
-static int fast_forward(const struct object_id *oids, int nr, int aggressive)
-{
-	int i;
-	struct tree_desc t[MAX_UNPACK_TREES];
-	struct unpack_trees_options opts;
-	struct lock_file lock = LOCK_INIT;
-
-	repo_read_index_preload(the_repository, NULL, 0);
-	if (refresh_index(the_repository->index, REFRESH_QUIET, NULL, NULL, NULL))
-		return -1;
-
-	repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
-
-	memset(&opts, 0, sizeof(opts));
-	opts.head_idx = 1;
-	opts.src_index = the_repository->index;
-	opts.dst_index = the_repository->index;
-	opts.merge = 1;
-	opts.update = 1;
-	opts.aggressive = aggressive;
-
-	for (i = 0; i < nr; i++) {
-		struct tree *tree;
-		tree = parse_tree_indirect(oids + i);
-		if (parse_tree(tree))
-			return -1;
-		init_tree_desc(t + i, tree->buffer, tree->size);
-	}
-
-	if (nr == 1)
-		opts.fn = oneway_merge;
-	else if (nr == 2) {
-		opts.fn = twoway_merge;
-		opts.initial_checkout = is_index_unborn(the_repository->index);
-	} else if (nr >= 3) {
-		opts.fn = threeway_merge;
-		opts.head_idx = nr - 1;
-	}
-
-	if (unpack_trees(nr, t, &opts))
-		return -1;
-
-	if (write_locked_index(the_repository->index, &lock, COMMIT_LOCK))
-		return error(_("unable to write new index file"));
-
-	return 0;
-}
-
-static int write_tree(struct tree **reference_tree)
-{
-	struct object_id oid;
-	int ret;
-
-	ret = write_index_as_tree(&oid, the_repository->index,
-				  the_repository->index_file, 0, NULL);
-	if (!ret)
-		*reference_tree = lookup_tree(the_repository, &oid);
-
-	return ret;
-}
-
-static int merge_octopus(struct commit_list *bases, const char *head_arg,
-			 struct commit_list *remotes)
-{
-	int non_ff_merge = 0, ret = 0, references = 1;
-	struct commit **reference_commit;
-	struct tree *reference_tree;
-	struct commit_list *j;
-	struct object_id head;
-	struct strbuf sb = STRBUF_INIT;
-
-	get_oid(head_arg, &head);
-
-	reference_commit = xcalloc(commit_list_count(remotes) + 1, sizeof(struct commit *));
-	reference_commit[0] = lookup_commit_reference(the_repository, &head);
-	reference_tree = get_commit_tree(reference_commit[0]);
-
-	if (repo_index_has_changes(the_repository, reference_tree, &sb)) {
-		error(_("Your local changes to the following files "
-			"would be overwritten by merge:\n  %s"),
-		      sb.buf);
-		strbuf_release(&sb);
-		ret = 2;
-		goto out;
-	}
-
-	for (j = remotes; j; j = j->next) {
-		struct commit *c = j->item;
-		struct object_id *oid = &c->object.oid;
-		struct commit_list *common, *k;
-		char *branch_name;
-		int can_ff = 1;
-
-		if (ret) {
-			puts(_("Automated merge did not work."));
-			puts(_("Should not be doing an octopus."));
-
-			ret = 2;
-			goto out;
-		}
-
-		branch_name = merge_get_better_branch_name(oid_to_hex(oid));
-		common = get_merge_bases_many(c, references, reference_commit);
-
-		if (!common)
-			die(_("Unable to find common commit with %s"), branch_name);
-
-		for (k = common; k && !oideq(&k->item->object.oid, oid); k = k->next);
-
-		if (k) {
-			printf(_("Already up to date with %s\n"), branch_name);
-			free(branch_name);
-			free_commit_list(common);
-			continue;
-		}
-
-		if (!non_ff_merge) {
-			int i;
-
-			for (i = 0, k = common; k && i < references && can_ff; k = k->next, i++) {
-				can_ff = oideq(&k->item->object.oid,
-					       &reference_commit[i]->object.oid);
-			}
-		}
-
-		if (!non_ff_merge && can_ff) {
-			struct object_id oids[2];
-			printf(_("Fast-forwarding to: %s\n"), branch_name);
-
-			oidcpy(oids, &head);
-			oidcpy(oids + 1, oid);
-
-			ret = fast_forward(oids, 2, 0);
-			if (ret) {
-				free(branch_name);
-				free_commit_list(common);
-				goto out;
-			}
-
-			references = 0;
-			write_tree(&reference_tree);
-		} else {
-			int i = 0;
-			struct tree *next = NULL;
-			struct object_id oids[MAX_UNPACK_TREES];
-
-			non_ff_merge = 1;
-			printf(_("Trying simple merge with %s\n"), branch_name);
-
-			for (k = common; k; k = k->next)
-				oidcpy(oids + (i++), &k->item->object.oid);
-
-			oidcpy(oids + (i++), &reference_tree->object.oid);
-			oidcpy(oids + (i++), oid);
-
-			if (fast_forward(oids, i, 1)) {
-				ret = 2;
-
-				free(branch_name);
-				free_commit_list(common);
-
-				goto out;
-			}
-
-			if (write_tree(&next)) {
-				struct lock_file lock = LOCK_INIT;
-
-				puts(_("Simple merge did not work, trying automatic merge."));
-				repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
-				ret = !!merge_all(the_repository->index, 0, 0,
-						  merge_one_file_cb, the_repository);
-				write_locked_index(the_repository->index, &lock, COMMIT_LOCK);
-
-				write_tree(&next);
-			}
-
-			reference_tree = next;
-		}
-
-		reference_commit[references++] = c;
-
-		free(branch_name);
-		free_commit_list(common);
-	}
-
-out:
-	free(reference_commit);
-	return ret;
-}
 
 static const char builtin_merge_octopus_usage[] =
 	"git merge-octopus [<bases>...] -- <head> <remote1> <remote2> [<remotes>...]";
@@ -254,5 +61,5 @@ int cmd_merge_octopus(int argc, const char **argv, const char *prefix)
 	if (commit_list_count(remotes) < 2)
 		return 2;
 
-	return merge_octopus(bases, head_arg, remotes);
+	return merge_strategies_octopus(the_repository, bases, head_arg, remotes);
 }
diff --git a/merge-strategies.c b/merge-strategies.c
index a12c575590..8395c4c787 100644
--- a/merge-strategies.c
+++ b/merge-strategies.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "cache-tree.h"
+#include "commit-reach.h"
 #include "dir.h"
 #include "lockfile.h"
 #include "merge-strategies.h"
@@ -384,3 +385,193 @@ int merge_strategies_resolve(struct repository *r,
 	rollback_lock_file(&lock);
 	return 2;
 }
+
+static int fast_forward(struct repository *r, const struct object_id *oids,
+			int nr, int aggressive)
+{
+	int i;
+	struct tree_desc t[MAX_UNPACK_TREES];
+	struct unpack_trees_options opts;
+	struct lock_file lock = LOCK_INIT;
+
+	repo_read_index_preload(r, NULL, 0);
+	if (refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL))
+		return -1;
+
+	repo_hold_locked_index(r, &lock, LOCK_DIE_ON_ERROR);
+
+	memset(&opts, 0, sizeof(opts));
+	opts.head_idx = 1;
+	opts.src_index = r->index;
+	opts.dst_index = r->index;
+	opts.merge = 1;
+	opts.update = 1;
+	opts.aggressive = aggressive;
+
+	for (i = 0; i < nr; i++) {
+		struct tree *tree;
+		tree = parse_tree_indirect(oids + i);
+		if (parse_tree(tree))
+			return -1;
+		init_tree_desc(t + i, tree->buffer, tree->size);
+	}
+
+	if (nr == 1)
+		opts.fn = oneway_merge;
+	else if (nr == 2) {
+		opts.fn = twoway_merge;
+		opts.initial_checkout = is_index_unborn(r->index);
+	} else if (nr >= 3) {
+		opts.fn = threeway_merge;
+		opts.head_idx = nr - 1;
+	}
+
+	if (unpack_trees(nr, t, &opts))
+		return -1;
+
+	if (write_locked_index(r->index, &lock, COMMIT_LOCK))
+		return error(_("unable to write new index file"));
+
+	return 0;
+}
+
+static int write_tree(struct repository *r, struct tree **reference_tree)
+{
+	struct object_id oid;
+	int ret;
+
+	ret = write_index_as_tree(&oid, r->index, r->index_file, 0, NULL);
+	if (!ret)
+		*reference_tree = lookup_tree(r, &oid);
+
+	return ret;
+}
+
+int merge_strategies_octopus(struct repository *r,
+			     struct commit_list *bases, const char *head_arg,
+			     struct commit_list *remotes)
+{
+	int non_ff_merge = 0, ret = 0, references = 1;
+	struct commit **reference_commit;
+	struct tree *reference_tree;
+	struct commit_list *j;
+	struct object_id head;
+	struct strbuf sb = STRBUF_INIT;
+
+	get_oid(head_arg, &head);
+
+	reference_commit = xcalloc(commit_list_count(remotes) + 1, sizeof(struct commit *));
+	reference_commit[0] = lookup_commit_reference(r, &head);
+	reference_tree = repo_get_commit_tree(r, reference_commit[0]);
+
+	if (repo_index_has_changes(r, reference_tree, &sb)) {
+		error(_("Your local changes to the following files "
+			"would be overwritten by merge:\n  %s"),
+		      sb.buf);
+		strbuf_release(&sb);
+		ret = 2;
+		goto out;
+	}
+
+	for (j = remotes; j && j->item; j = j->next) {
+		struct commit *c = j->item;
+		struct object_id *oid = &c->object.oid;
+		struct commit_list *common, *k;
+		char *branch_name;
+		int can_ff = 1;
+
+		if (ret) {
+			puts(_("Automated merge did not work."));
+			puts(_("Should not be doing an octopus."));
+
+			ret = 2;
+			goto out;
+		}
+
+		branch_name = merge_get_better_branch_name(oid_to_hex(oid));
+		common = get_merge_bases_many(c, references, reference_commit);
+
+		if (!common)
+			die(_("Unable to find common commit with %s"), branch_name);
+
+		for (k = common; k && !oideq(&k->item->object.oid, oid); k = k->next);
+
+		if (k) {
+			printf(_("Already up to date with %s\n"), branch_name);
+			free(branch_name);
+			free_commit_list(common);
+			continue;
+		}
+
+		if (!non_ff_merge) {
+			int i;
+
+			for (i = 0, k = common; k && i < references && can_ff; k = k->next, i++) {
+				can_ff = oideq(&k->item->object.oid,
+					       &reference_commit[i]->object.oid);
+			}
+		}
+
+		if (!non_ff_merge && can_ff) {
+			struct object_id oids[2];
+			printf(_("Fast-forwarding to: %s\n"), branch_name);
+
+			oidcpy(oids, &head);
+			oidcpy(oids + 1, oid);
+
+			ret = fast_forward(r, oids, 2, 0);
+			if (ret) {
+				free(branch_name);
+				free_commit_list(common);
+				goto out;
+			}
+
+			references = 0;
+			write_tree(r, &reference_tree);
+		} else {
+			int i = 0;
+			struct tree *next = NULL;
+			struct object_id oids[MAX_UNPACK_TREES];
+
+			non_ff_merge = 1;
+			printf(_("Trying simple merge with %s\n"), branch_name);
+
+			for (k = common; k; k = k->next)
+				oidcpy(oids + (i++), &k->item->object.oid);
+
+			oidcpy(oids + (i++), &reference_tree->object.oid);
+			oidcpy(oids + (i++), oid);
+
+			if (fast_forward(r, oids, i, 1)) {
+				ret = 2;
+
+				free(branch_name);
+				free_commit_list(common);
+
+				goto out;
+			}
+
+			if (write_tree(r, &next)) {
+				struct lock_file lock = LOCK_INIT;
+
+				puts(_("Simple merge did not work, trying automatic merge."));
+				repo_hold_locked_index(r, &lock, LOCK_DIE_ON_ERROR);
+				ret = !!merge_all(r->index, 0, 0, merge_one_file_cb, r);
+				write_locked_index(r->index, &lock, COMMIT_LOCK);
+
+				write_tree(r, &next);
+			}
+
+			reference_tree = next;
+		}
+
+		reference_commit[references++] = c;
+
+		free(branch_name);
+		free_commit_list(common);
+	}
+
+out:
+	free(reference_commit);
+	return ret;
+}
diff --git a/merge-strategies.h b/merge-strategies.h
index 778f8ce9d6..938411a04e 100644
--- a/merge-strategies.h
+++ b/merge-strategies.h
@@ -37,5 +37,8 @@ int merge_all(struct index_state *istate, int oneshot, int quiet,
 int merge_strategies_resolve(struct repository *r,
 			     struct commit_list *bases, const char *head_arg,
 			     struct commit_list *remote);
+int merge_strategies_octopus(struct repository *r,
+			     struct commit_list *bases, const char *head_arg,
+			     struct commit_list *remote);
 
 #endif /* MERGE_STRATEGIES_H */
-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [RFC PATCH v1 14/17] merge: use the "resolve" strategy without forking
  2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
                   ` (12 preceding siblings ...)
  2020-06-25 12:19 ` [RFC PATCH v1 13/17] merge-octopus: libify merge_octopus() Alban Gruin
@ 2020-06-25 12:19 ` Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 15/17] merge: use the "octopus" " Alban Gruin
                   ` (2 subsequent siblings)
  16 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

This teaches `git merge' to invoke the "resolve" strategy with a
function call instead of forking.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/merge.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 7da707bf55..d50b4ad6ad 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -41,6 +41,7 @@
 #include "commit-reach.h"
 #include "wt-status.h"
 #include "commit-graph.h"
+#include "merge-strategies.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -744,7 +745,10 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 				       COMMIT_LOCK | SKIP_IF_UNCHANGED))
 			die(_("unable to write %s"), get_index_file());
 		return clean ? 0 : 1;
-	} else {
+	} else if (!strcmp(strategy, "resolve"))
+		return merge_strategies_resolve(the_repository, common,
+						head_arg, remoteheads);
+	else {
 		return try_merge_command(the_repository,
 					 strategy, xopts_nr, xopts,
 					 common, head_arg, remoteheads);
-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [RFC PATCH v1 15/17] merge: use the "octopus" strategy without forking
  2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
                   ` (13 preceding siblings ...)
  2020-06-25 12:19 ` [RFC PATCH v1 14/17] merge: use the "resolve" strategy without forking Alban Gruin
@ 2020-06-25 12:19 ` Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 16/17] sequencer: use the "resolve" " Alban Gruin
  2020-06-25 12:19 ` [RFC PATCH v1 17/17] sequencer: use the "octopus" merge " Alban Gruin
  16 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

This teaches `git merge' to invoke the "octopus" strategy with a
function call instead of forking.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 builtin/merge.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/merge.c b/builtin/merge.c
index d50b4ad6ad..53f64ddb87 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -748,6 +748,9 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 	} else if (!strcmp(strategy, "resolve"))
 		return merge_strategies_resolve(the_repository, common,
 						head_arg, remoteheads);
+	else if (!strcmp(strategy, "octopus"))
+		return merge_strategies_octopus(the_repository, common,
+						head_arg, remoteheads);
 	else {
 		return try_merge_command(the_repository,
 					 strategy, xopts_nr, xopts,
-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [RFC PATCH v1 16/17] sequencer: use the "resolve" strategy without forking
  2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
                   ` (14 preceding siblings ...)
  2020-06-25 12:19 ` [RFC PATCH v1 15/17] merge: use the "octopus" " Alban Gruin
@ 2020-06-25 12:19 ` Alban Gruin
  2020-06-25 16:11   ` Phillip Wood
  2020-06-25 12:19 ` [RFC PATCH v1 17/17] sequencer: use the "octopus" merge " Alban Gruin
  16 siblings, 1 reply; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

This teaches the sequencer to invoke the "resolve" strategy with a
function call instead of forking.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index fd7701c88a..ea8dc58108 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -33,6 +33,7 @@
 #include "commit-reach.h"
 #include "rebase-interactive.h"
 #include "reset.h"
+#include "merge-strategies.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -1922,9 +1923,15 @@ static int do_pick_commit(struct repository *r,
 
 		commit_list_insert(base, &common);
 		commit_list_insert(next, &remotes);
-		res |= try_merge_command(r, opts->strategy,
-					 opts->xopts_nr, (const char **)opts->xopts,
-					common, oid_to_hex(&head), remotes);
+
+		if (!strcmp(opts->strategy, "resolve")) {
+			repo_read_index(r);
+			res |= merge_strategies_resolve(r, common, oid_to_hex(&head), remotes);
+		} else
+			res |= try_merge_command(r, opts->strategy,
+						 opts->xopts_nr, (const char **)opts->xopts,
+						 common, oid_to_hex(&head), remotes);
+
 		free_commit_list(common);
 		free_commit_list(remotes);
 	}
-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [RFC PATCH v1 17/17] sequencer: use the "octopus" merge strategy without forking
  2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
                   ` (15 preceding siblings ...)
  2020-06-25 12:19 ` [RFC PATCH v1 16/17] sequencer: use the "resolve" " Alban Gruin
@ 2020-06-25 12:19 ` Alban Gruin
  16 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2020-06-25 12:19 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Alban Gruin

This teaches the sequencer to invoke the "octopus" strategy with a
function call instead of forking.

Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
---
 sequencer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index ea8dc58108..f9fa995b4b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1927,6 +1927,9 @@ static int do_pick_commit(struct repository *r,
 		if (!strcmp(opts->strategy, "resolve")) {
 			repo_read_index(r);
 			res |= merge_strategies_resolve(r, common, oid_to_hex(&head), remotes);
+		} else if (!strcmp(opts->strategy, "octopus")) {
+			repo_read_index(r);
+			res |= merge_strategies_octopus(r, common, oid_to_hex(&head), remotes);
 		} else
 			res |= try_merge_command(r, opts->strategy,
 						 opts->xopts_nr, (const char **)opts->xopts,
-- 
2.27.0.139.gc9c318d6bf


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [RFC PATCH v1 02/17] merge-one-file: rewrite in C
  2020-06-25 12:19 ` [RFC PATCH v1 02/17] merge-one-file: rewrite in C Alban Gruin
@ 2020-06-25 14:55   ` Chris Torek
  2020-06-25 15:16   ` Phillip Wood
  1 sibling, 0 replies; 29+ messages in thread
From: Chris Torek @ 2020-06-25 14:55 UTC (permalink / raw)
  To: Alban Gruin; +Cc: git, Junio C Hamano

much snippage below, keeping just enough context to see which file,
function, etc:

On Thu, Jun 25, 2020 at 5:49 AM Alban Gruin <alban.gruin@gmail.com> wrote:
> diff --git a/builtin/merge-one-file.c b/builtin/merge-one-file.c
> new file mode 100644
> index 0000000000..4992a6cd30
> --- /dev/null
> +++ b/builtin/merge-one-file.c
> @@ -0,0 +1,275 @@

> +static int do_merge_one_file(const struct object_id *orig_blob,
> +                            const struct object_id *our_blob,
> +                            const struct object_id *their_blob, const char *path,
> +                            unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode)
> +{
> +       int ret, source, dest;

> +       source = open(src1.buf, O_RDONLY);
> +       dest = open(path, O_WRONLY | O_TRUNC);
> +
> +       copy_fd(source, dest);
> +
> +       close(source);
> +       close(dest);
> +
> +       unlink(orig.buf);
> +       unlink(src1.buf);
> +       unlink(src2.buf);

Some of this goes away in subsequent patches, but most of these calls
should be checked for error returns, especially the two `open`s in case
someone has messed with permissions.

Chris

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [RFC PATCH v1 02/17] merge-one-file: rewrite in C
  2020-06-25 12:19 ` [RFC PATCH v1 02/17] merge-one-file: rewrite in C Alban Gruin
  2020-06-25 14:55   ` Chris Torek
@ 2020-06-25 15:16   ` Phillip Wood
  2020-06-25 18:17     ` Phillip Wood
  2020-07-12 11:22     ` Alban Gruin
  1 sibling, 2 replies; 29+ messages in thread
From: Phillip Wood @ 2020-06-25 15:16 UTC (permalink / raw)
  To: Alban Gruin, git; +Cc: Junio C Hamano

Hi Alban

I think this series is a great idea

On 25/06/2020 13:19, Alban Gruin wrote:
> This rewrites `git merge-one-file' from shell to C.  This port is very
> straightforward: it keeps using external processes to edit the index,
> for instance.  Errors are also displayed with fprintf() instead of
> error().  Both of these will be addressed in the next few commits,
> leading to its libification so its main function can be used from other
> commands directly.
> 
> This also fixes a bug present in the original script: instead of
> checking if a _regular_ file exists when a file exists in the branch to
> merge, but not in our branch, the rewritten version checks if a file of
> any kind (ie. a directory, ...) exists.  This fixes the tests t6035.14,
> where the branch to merge had a new file, `a/b', but our branch had a
> directory there; it should have failed because a directory exists, but
> it did not because there was no regular file called `a/b'.  This test is
> now marked as successful.
> 
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>   Makefile                        |   2 +-
>   builtin.h                       |   1 +
>   builtin/merge-one-file.c        | 275 ++++++++++++++++++++++++++++++++
>   git-merge-one-file.sh           | 167 -------------------
>   git.c                           |   1 +
>   t/t6035-merge-dir-to-symlink.sh |   2 +-
>   6 files changed, 279 insertions(+), 169 deletions(-)
>   create mode 100644 builtin/merge-one-file.c
>   delete mode 100755 git-merge-one-file.sh
> 
> diff --git a/Makefile b/Makefile
> index 372139f1f2..19574f5133 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -596,7 +596,6 @@ SCRIPT_SH += git-bisect.sh
>   SCRIPT_SH += git-difftool--helper.sh
>   SCRIPT_SH += git-filter-branch.sh
>   SCRIPT_SH += git-merge-octopus.sh
> -SCRIPT_SH += git-merge-one-file.sh
>   SCRIPT_SH += git-merge-resolve.sh
>   SCRIPT_SH += git-mergetool.sh
>   SCRIPT_SH += git-quiltimport.sh
> @@ -1089,6 +1088,7 @@ BUILTIN_OBJS += builtin/mailsplit.o
>   BUILTIN_OBJS += builtin/merge-base.o
>   BUILTIN_OBJS += builtin/merge-file.o
>   BUILTIN_OBJS += builtin/merge-index.o
> +BUILTIN_OBJS += builtin/merge-one-file.o
>   BUILTIN_OBJS += builtin/merge-ours.o
>   BUILTIN_OBJS += builtin/merge-recursive.o
>   BUILTIN_OBJS += builtin/merge-tree.o
> diff --git a/builtin.h b/builtin.h
> index a5ae15bfe5..9205d5ecdc 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -172,6 +172,7 @@ int cmd_merge_base(int argc, const char **argv, const char *prefix);
>   int cmd_merge_index(int argc, const char **argv, const char *prefix);
>   int cmd_merge_ours(int argc, const char **argv, const char *prefix);
>   int cmd_merge_file(int argc, const char **argv, const char *prefix);
> +int cmd_merge_one_file(int argc, const char **argv, const char *prefix);
>   int cmd_merge_recursive(int argc, const char **argv, const char *prefix);
>   int cmd_merge_tree(int argc, const char **argv, const char *prefix);
>   int cmd_mktag(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/merge-one-file.c b/builtin/merge-one-file.c
> new file mode 100644
> index 0000000000..4992a6cd30
> --- /dev/null
> +++ b/builtin/merge-one-file.c
> @@ -0,0 +1,275 @@
> +/*
> + * 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 script, called with
> + *
> + *   $1 - original file SHA1 (or empty)
> + *   $2 - file in branch1 SHA1 (or empty)
> + *   $3 - file in branch2 SHA1 (or empty)
> + *   $4 - pathname in repository
> + *   $5 - original file mode (or empty)
> + *   $6 - file in branch1 mode (or empty)
> + *   $7 - file in branch2 mode (or empty)

nit pick - these are now argv[1] etc rather than $1 etc

> + *
> + * 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.
> + */
> +
> +#define USE_THE_INDEX_COMPATIBILITY_MACROS
> +#include "cache.h"
> +#include "builtin.h"
> +#include "commit.h"
> +#include "dir.h"
> +#include "lockfile.h"
> +#include "object-store.h"
> +#include "run-command.h"
> +#include "xdiff-interface.h"
> +
> +static int create_temp_file(const struct object_id *oid, struct strbuf *path)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf err = STRBUF_INIT;
> +	int ret;
> +
> +	cp.git_cmd = 1;
> +	argv_array_pushl(&cp.args, "unpack-file", oid_to_hex(oid), NULL);
> +	ret = pipe_command(&cp, NULL, 0, path, 0, &err, 0);
> +	if (!ret && path->len > 0)
> +		strbuf_trim_trailing_newline(path);
> +
> +	fprintf(stderr, "%.*s", (int) err.len, err.buf);
> +	strbuf_release(&err);
> +
> +	return ret;
> +}

I know others will disagree but personally I'm not a huge fan of 
rewriting shell functions in C that forks other builtins and then 
converting the C to use the internal apis, it seems a much better to 
just write the proper C version the first time. This is especially true 
for simple function such as the ones in this file. That way the reviewer 
gets a clear view of the final code from the patch, rather than having 
to piece it together from a series of additions and deletions.

> +
> +static int add_to_index_cacheinfo(unsigned int mode,
> +				  const struct object_id *oid, const char *path)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +
> +	cp.git_cmd = 1;
> +	argv_array_pushl(&cp.args, "update-index", "--add", "--cacheinfo", NULL);
> +	argv_array_pushf(&cp.args, "%o,%s,%s", mode, oid_to_hex(oid), path);
> +	return run_command(&cp);
> +}
> +
> +static int remove_from_index(const char *path)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +
> +	cp.git_cmd = 1;
> +	argv_array_pushl(&cp.args, "update-index", "--remove", "--", path, NULL);
> +	return run_command(&cp);
> +}
> +
> +static int checkout_from_index(const char *path)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +
> +	cp.git_cmd = 1;
> +	argv_array_pushl(&cp.args, "checkout-index", "-u", "-f", "--", path, NULL);
> +	return run_command(&cp);
> +}
> +
> +static int merge_one_file_deleted(const struct object_id *orig_blob,
> +				  const struct object_id *our_blob,
> +				  const struct object_id *their_blob, const char *path,
> +				  unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode)
> +{
> +	if ((our_blob && orig_mode != our_mode) ||
> +	    (their_blob && orig_mode != their_mode)) {
> +		fprintf(stderr, "ERROR: File %s deleted on one branch but had its\n", path);
> +		fprintf(stderr, "ERROR: permissions changed on the other.\n");
> +		return 1;
> +	}
> +
> +	if (our_blob) {
> +		printf("Removing %s\n", path);
> +
> +		if (file_exists(path))
> +			remove_path(path);
> +	}
> +
> +	return remove_from_index(path);
> +}
> +
> +static int do_merge_one_file(const struct object_id *orig_blob,
> +			     const struct object_id *our_blob,
> +			     const struct object_id *their_blob, const char *path,
> +			     unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode)
> +{
> +	int ret, source, dest;
> +	struct strbuf src1 = STRBUF_INIT, src2 = STRBUF_INIT, orig = STRBUF_INIT;
> +	struct child_process cp_merge = CHILD_PROCESS_INIT,
> +		cp_checkout = CHILD_PROCESS_INIT,
> +		cp_update = CHILD_PROCESS_INIT;
> +
> +	if (our_mode == S_IFLNK || their_mode == S_IFLNK) {
> +		fprintf(stderr, "ERROR: %s: Not merging symbolic link changes.\n", path);
> +		return 1;
> +	} else if (our_mode == S_IFGITLINK || their_mode == S_IFGITLINK) {
> +		fprintf(stderr, "ERROR: %s: Not merging conflicting submodule changes.\n",
> +			path);
> +		return 1;
> +	}
> +
> +	create_temp_file(our_blob, &src1);
> +	create_temp_file(their_blob, &src2);
> +
> +	if (orig_blob) {
> +		printf("Auto-merging %s\n", path);
> +		create_temp_file(orig_blob, &orig);
> +	} else {
> +		printf("Added %s in both, but differently.\n", path);
> +		create_temp_file(the_hash_algo->empty_blob, &orig);
> +	}
> +
> +	cp_merge.git_cmd = 1;
> +	argv_array_pushl(&cp_merge.args, "merge-file", src1.buf, orig.buf, src2.buf,
> +			 NULL);
> +	ret = run_command(&cp_merge);
> +
> +	if (ret != 0)
> +		ret = 1;
> +
> +	cp_checkout.git_cmd = 1;
> +	argv_array_pushl(&cp_checkout.args, "checkout-index", "-f", "--stage=2",
> +			 "--", path, NULL);
> +	if (run_command(&cp_checkout))
> +		return 1;
> +
> +	source = open(src1.buf, O_RDONLY);
> +	dest = open(path, O_WRONLY | O_TRUNC);
> +
> +	copy_fd(source, dest);
> +
> +	close(source);
> +	close(dest);
> +
> +	unlink(orig.buf);
> +	unlink(src1.buf);
> +	unlink(src2.buf);
> +
> +	strbuf_release(&src1);
> +	strbuf_release(&src2);
> +	strbuf_release(&orig);

The whole business of creating temporary files and forking seems like a 
lot of effort compared to calling ll_merge() which would also mean we 
respect any merge attributes

> +
> +	if (ret) {
> +		fprintf(stderr, "ERROR: ");
> +
> +		if (!orig_blob) {

I think the original does if (ret || !orig_blob) not &&
> +			fprintf(stderr, "content conflict");
> +			if (our_mode != their_mode)
> +				fprintf(stderr, ", ");

sentence lego, in any case the message below should be printed 
regardless of content conflicts. We should probably mark all these 
messages for translation as well.

> +		}
> +
> +		if (our_mode != their_mode)
> +			fprintf(stderr, "permissions conflict: %o->%o,%o",
> +				orig_mode, our_mode, their_mode);
> +
> +		fprintf(stderr, " in %s\n", path);
> +
> +		return 1;
> +	}
> +
> +	cp_update.git_cmd = 1;
> +	argv_array_pushl(&cp_update.args, "update-index", "--", path, NULL);
> +	return run_command(&cp_update);
> +}
> +
> +static int merge_one_file(const struct object_id *orig_blob,
> +			  const struct object_id *our_blob,
> +			  const struct object_id *their_blob, const char *path,
> +			  unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode)
> +{
> +	if (orig_blob &&
> +	    ((our_blob && oideq(orig_blob, our_blob)) ||
> +	     (their_blob && oideq(orig_blob, their_blob))))
> +		return merge_one_file_deleted(orig_blob, our_blob, their_blob, path,
> +					      orig_mode, our_mode, their_mode);

It would be nice to preserve the comments from the script as I find they 
help a lot in understanding which case each piece of code is handling. 
The code above appears to be handling deletions but does not appear to 
check that one side is actually missing. Shouldn't it be something like

if (orig_blob &&
     ((!their_blob && (our_blob && oideq(orig_blob, our_blob))) ||
      (!our_blob && (their_blob && oideq(orig_blob, their_blob))))

Maybe this could do with a test case

> +	else if (!orig_blob && our_blob && !their_blob) {
> +		return add_to_index_cacheinfo(our_mode, our_blob, path);
> +	} else if (!orig_blob && !our_blob && their_blob) {
> +		printf("Adding %s\n", path);
> +
> +		if (file_exists(path)) {
> +			fprintf(stderr, "ERROR: untracked %s is overwritten by the merge.\n", path);
> +			return 1;
> +		}
> +
> +		if (add_to_index_cacheinfo(their_mode, their_blob, path))
> +			return 1;
> +		return checkout_from_index(path);
> +	} else if (!orig_blob && our_blob && their_blob &&
> +		   oideq(our_blob, their_blob)) {
> +		if (our_mode != their_mode) {
> +			fprintf(stderr, "ERROR: File %s added identically in both branches,", path);
> +			fprintf(stderr, "ERROR: but permissions conflict %o->%o.\n",
> +				our_mode, their_mode);
> +			return 1;
> +		}
> +
> +		printf("Adding %s\n", path);
> +
> +		if (add_to_index_cacheinfo(our_mode, our_blob, path))
> +			return 1;
> +		return checkout_from_index(path);
> +	} else if (our_blob && their_blob)
> +		return do_merge_one_file(orig_blob, our_blob, their_blob, path,
> +					 orig_mode, our_mode, their_mode);
> +	else {
> +		char *orig_hex = "", *our_hex = "", *their_hex = "";
> +
> +		if (orig_blob)
> +			orig_hex = oid_to_hex(orig_blob);
> +		if (our_blob)
> +			our_hex = oid_to_hex(our_blob);
> +		if (their_blob)
> +			their_hex = oid_to_hex(their_blob);
> +
> +		fprintf(stderr, "ERROR: %s: Not handling case %s -> %s -> %s\n",
> +			path, orig_hex, our_hex, their_hex);
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +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.";
> +
> +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;
> +
> +	if (argc != 8)
> +		usage(builtin_merge_one_file_usage);
> +
> +	if (!get_oid(argv[1], &orig_blob)) {
> +		p_orig_blob = &orig_blob;
> +		orig_mode = strtol(argv[5], NULL, 8);

It would probably make sense to check that strtol() succeeds (and the 
mode is sensible), and also that get_oid() fails because argv[1] is 
empty, not because it is invalid.

Thanks for working on this
Best Wishes

Phillip


> +	}
> +
> +	if (!get_oid(argv[2], &our_blob)) {
> +		p_our_blob = &our_blob;
> +		our_mode = strtol(argv[6], NULL, 8);
> +	}
> +
> +	if (!get_oid(argv[3], &their_blob)) {
> +		p_their_blob = &their_blob;
> +		their_mode = strtol(argv[7], NULL, 8);
> +	}
> +
> +	return merge_one_file(p_orig_blob, p_our_blob, p_their_blob, argv[4],
> +			      orig_mode, our_mode, their_mode);
> +}
> diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
> deleted file mode 100755
> index f6d9852d2f..0000000000
> --- a/git-merge-one-file.sh
> +++ /dev/null
> @@ -1,167 +0,0 @@
> -#!/bin/sh
> -#
> -# Copyright (c) Linus Torvalds, 2005
> -#
> -# This is the git per-file merge script, called with
> -#
> -#   $1 - original file SHA1 (or empty)
> -#   $2 - file in branch1 SHA1 (or empty)
> -#   $3 - file in branch2 SHA1 (or empty)
> -#   $4 - pathname in repository
> -#   $5 - original file mode (or empty)
> -#   $6 - file in branch1 mode (or empty)
> -#   $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.
> -
> -USAGE='<orig blob> <our blob> <their blob> <path>'
> -USAGE="$USAGE <orig mode> <our mode> <their mode>"
> -LONG_USAGE="usage: git merge-one-file $USAGE
> -
> -Blob ids and modes should be empty for missing files."
> -
> -SUBDIRECTORY_OK=Yes
> -. git-sh-setup
> -cd_to_toplevel
> -require_work_tree
> -
> -if test $# != 7
> -then
> -	echo "$LONG_USAGE"
> -	exit 1
> -fi
> -
> -case "${1:-.}${2:-.}${3:-.}" in
> -#
> -# Deleted in both or deleted in one and unchanged in the other
> -#
> -"$1.." | "$1.$1" | "$1$1.")
> -	if { test -z "$6" && test "$5" != "$7"; } ||
> -	   { test -z "$7" && test "$5" != "$6"; }
> -	then
> -		echo "ERROR: File $4 deleted on one branch but had its" >&2
> -		echo "ERROR: permissions changed on the other." >&2
> -		exit 1
> -	fi
> -
> -	if test -n "$2"
> -	then
> -		echo "Removing $4"
> -	else
> -		# read-tree checked that index matches HEAD already,
> -		# so we know we do not have this path tracked.
> -		# there may be an unrelated working tree file here,
> -		# which we should just leave unmolested.  Make sure
> -		# we do not have it in the index, though.
> -		exec git update-index --remove -- "$4"
> -	fi
> -	if test -f "$4"
> -	then
> -		rm -f -- "$4" &&
> -		rmdir -p "$(expr "z$4" : 'z\(.*\)/')" 2>/dev/null || :
> -	fi &&
> -		exec git update-index --remove -- "$4"
> -	;;
> -
> -#
> -# Added in one.
> -#
> -".$2.")
> -	# the other side did not add and we added so there is nothing
> -	# to be done, except making the path merged.
> -	exec git update-index --add --cacheinfo "$6" "$2" "$4"
> -	;;
> -"..$3")
> -	echo "Adding $4"
> -	if test -f "$4"
> -	then
> -		echo "ERROR: untracked $4 is overwritten by the merge." >&2
> -		exit 1
> -	fi
> -	git update-index --add --cacheinfo "$7" "$3" "$4" &&
> -		exec git checkout-index -u -f -- "$4"
> -	;;
> -
> -#
> -# Added in both, identically (check for same permissions).
> -#
> -".$3$2")
> -	if test "$6" != "$7"
> -	then
> -		echo "ERROR: File $4 added identically in both branches," >&2
> -		echo "ERROR: but permissions conflict $6->$7." >&2
> -		exit 1
> -	fi
> -	echo "Adding $4"
> -	git update-index --add --cacheinfo "$6" "$2" "$4" &&
> -		exec git checkout-index -u -f -- "$4"
> -	;;
> -
> -#
> -# Modified in both, but differently.
> -#
> -"$1$2$3" | ".$2$3")
> -
> -	case ",$6,$7," in
> -	*,120000,*)
> -		echo "ERROR: $4: Not merging symbolic link changes." >&2
> -		exit 1
> -		;;
> -	*,160000,*)
> -		echo "ERROR: $4: Not merging conflicting submodule changes." >&2
> -		exit 1
> -		;;
> -	esac
> -
> -	src1=$(git unpack-file $2)
> -	src2=$(git unpack-file $3)
> -	case "$1" in
> -	'')
> -		echo "Added $4 in both, but differently."
> -		orig=$(git unpack-file $(git hash-object /dev/null))
> -		;;
> -	*)
> -		echo "Auto-merging $4"
> -		orig=$(git unpack-file $1)
> -		;;
> -	esac
> -
> -	git merge-file "$src1" "$orig" "$src2"
> -	ret=$?
> -	msg=
> -	if test $ret != 0 || test -z "$1"
> -	then
> -		msg='content conflict'
> -		ret=1
> -	fi
> -
> -	# Create the working tree file, using "our tree" version from the
> -	# index, and then store the result of the merge.
> -	git checkout-index -f --stage=2 -- "$4" && cat "$src1" >"$4" || exit 1
> -	rm -f -- "$orig" "$src1" "$src2"
> -
> -	if test "$6" != "$7"
> -	then
> -		if test -n "$msg"
> -		then
> -			msg="$msg, "
> -		fi
> -		msg="${msg}permissions conflict: $5->$6,$7"
> -		ret=1
> -	fi
> -
> -	if test $ret != 0
> -	then
> -		echo "ERROR: $msg in $4" >&2
> -		exit 1
> -	fi
> -	exec git update-index -- "$4"
> -	;;
> -
> -*)
> -	echo "ERROR: $4: Not handling case $1 -> $2 -> $3" >&2
> -	;;
> -esac
> -exit 1
> diff --git a/git.c b/git.c
> index a2d337eed7..058d91a2a5 100644
> --- a/git.c
> +++ b/git.c
> @@ -532,6 +532,7 @@ static struct cmd_struct commands[] = {
>   	{ "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
>   	{ "merge-index", cmd_merge_index, RUN_SETUP | NO_PARSEOPT },
>   	{ "merge-ours", cmd_merge_ours, RUN_SETUP | NO_PARSEOPT },
> +	{ "merge-one-file", cmd_merge_one_file, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
>   	{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
>   	{ "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
>   	{ "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
> diff --git a/t/t6035-merge-dir-to-symlink.sh b/t/t6035-merge-dir-to-symlink.sh
> index 2eddcc7664..5fb74e39a0 100755
> --- a/t/t6035-merge-dir-to-symlink.sh
> +++ b/t/t6035-merge-dir-to-symlink.sh
> @@ -94,7 +94,7 @@ test_expect_success SYMLINKS 'a/b was resolved as symlink' '
>   	test -h a/b
>   '
>   
> -test_expect_failure 'do not lose untracked in merge (resolve)' '
> +test_expect_success 'do not lose untracked in merge (resolve)' '
>   	git reset --hard &&
>   	git checkout baseline^0 &&
>   	>a/b/c/e &&
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [RFC PATCH v1 16/17] sequencer: use the "resolve" strategy without forking
  2020-06-25 12:19 ` [RFC PATCH v1 16/17] sequencer: use the "resolve" " Alban Gruin
@ 2020-06-25 16:11   ` Phillip Wood
  2020-07-12 11:27     ` Alban Gruin
  0 siblings, 1 reply; 29+ messages in thread
From: Phillip Wood @ 2020-06-25 16:11 UTC (permalink / raw)
  To: Alban Gruin, git; +Cc: Junio C Hamano

Hi Alban

On 25/06/2020 13:19, Alban Gruin wrote:
> This teaches the sequencer to invoke the "resolve" strategy with a
> function call instead of forking.

This is a good idea, however we should check the existing tests that use 
this strategy to see if they are doing so to test the 
try_merge_command() code path. I've got some patches in seen that use 
'--strategy=resolve' to exercise the "non merge-recursive" code path, so 
I'll update them to use a proper custom merge strategy.

Is it worth optimizing do_merge() to take advantage of resolve and 
octopus being builtin as well?

Best Wishes

Phil


> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
>   sequencer.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index fd7701c88a..ea8dc58108 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -33,6 +33,7 @@
>   #include "commit-reach.h"
>   #include "rebase-interactive.h"
>   #include "reset.h"
> +#include "merge-strategies.h"
>   
>   #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>   
> @@ -1922,9 +1923,15 @@ static int do_pick_commit(struct repository *r,
>   
>   		commit_list_insert(base, &common);
>   		commit_list_insert(next, &remotes);
> -		res |= try_merge_command(r, opts->strategy,
> -					 opts->xopts_nr, (const char **)opts->xopts,
> -					common, oid_to_hex(&head), remotes);
> +
> +		if (!strcmp(opts->strategy, "resolve")) {
> +			repo_read_index(r);
> +			res |= merge_strategies_resolve(r, common, oid_to_hex(&head), remotes);
> +		} else
> +			res |= try_merge_command(r, opts->strategy,
> +						 opts->xopts_nr, (const char **)opts->xopts,
> +						 common, oid_to_hex(&head), remotes);
> +
>   		free_commit_list(common);
>   		free_commit_list(remotes);
>   	}
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [RFC PATCH v1 02/17] merge-one-file: rewrite in C
  2020-06-25 15:16   ` Phillip Wood
@ 2020-06-25 18:17     ` Phillip Wood
  2020-06-26 14:33       ` Phillip Wood
  2020-07-12 11:22     ` Alban Gruin
  1 sibling, 1 reply; 29+ messages in thread
From: Phillip Wood @ 2020-06-25 18:17 UTC (permalink / raw)
  To: Alban Gruin, git; +Cc: Junio C Hamano

On 25/06/2020 16:16, Phillip Wood wrote:
> Hi Alban
> 
> I think this series is a great idea
> 
> On 25/06/2020 13:19, Alban Gruin wrote:
>> This rewrites `git merge-one-file' from shell to C.  This port is very
>> straightforward: it keeps using external processes to edit the index,
>> for instance.  Errors are also displayed with fprintf() instead of
>> error().  Both of these will be addressed in the next few commits,
>> leading to its libification so its main function can be used from other
>> commands directly.
>>
>> This also fixes a bug present in the original script: instead of
>> checking if a _regular_ file exists when a file exists in the branch to
>> merge, but not in our branch, the rewritten version checks if a file of
>> any kind (ie. a directory, ...) exists.  This fixes the tests t6035.14,
>> where the branch to merge had a new file, `a/b', but our branch had a
>> directory there; it should have failed because a directory exists, but
>> it did not because there was no regular file called `a/b'.  This test is
>> now marked as successful.
>> [...]
>> +static int merge_one_file(const struct object_id *orig_blob,
>> +              const struct object_id *our_blob,
>> +              const struct object_id *their_blob, const char *path,
>> +              unsigned int orig_mode, unsigned int our_mode, unsigned
>> int their_mode)
>> +{
>> +    if (orig_blob &&
>> +        ((our_blob && oideq(orig_blob, our_blob)) ||
>> +         (their_blob && oideq(orig_blob, their_blob))))
>> +        return merge_one_file_deleted(orig_blob, our_blob,
>> their_blob, path,
>> +                          orig_mode, our_mode, their_mode);
> 
> It would be nice to preserve the comments from the script as I find they
> help a lot in understanding which case each piece of code is handling.
> The code above appears to be handling deletions but does not appear to
> check that one side is actually missing. Shouldn't it be something like
> 
> if (orig_blob &&
>     ((!their_blob && (our_blob && oideq(orig_blob, our_blob))) ||
>      (!our_blob && (their_blob && oideq(orig_blob, their_blob))))
> 
> Maybe this could do with a test case

The reason your version works is that if only one side has changed
read-tree will have done the merge itself so this only gets called if
one side has been deleted. However the original script printed an error
if someone accidentally called when the content had changed in only one
side and there were no mode changes. I think we want to keep that behavior.

In the future we could probably update this to also handle the cases
that read-tree normally takes care of rather than erroring out but I
don't think it is a high priority.

Best Wishes

Phillip

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [RFC PATCH v1 06/17] merge-index: libify merge_one_path() and merge_all()
  2020-06-25 12:19 ` [RFC PATCH v1 06/17] merge-index: libify merge_one_path() and merge_all() Alban Gruin
@ 2020-06-26 10:13   ` Phillip Wood
  2020-06-26 14:32     ` Phillip Wood
  2020-07-12 11:36     ` Alban Gruin
  0 siblings, 2 replies; 29+ messages in thread
From: Phillip Wood @ 2020-06-26 10:13 UTC (permalink / raw)
  To: Alban Gruin, git; +Cc: Junio C Hamano

Hi Alban

On 25/06/2020 13:19, Alban Gruin wrote:
> The "resolve" and "octopus" merge strategies do not call directly `git
> merge-one-file', they delegate the work to another git command, `git
> merge-index', that will loop over files in the index and call the
> specified command.  Unfortunately, these functions are not part of
> libgit.a, which means that once rewritten, the strategies would still
> have to invoke `merge-one-file' by spawning a new process first.
> 
> To avoid this, this moves merge_one_path(), merge_all(), and their
> helpers to merge-strategies.c.  They also take a callback to dictate
> what they should do for each file.  For now, only one launching a new
> process is defined to preserve the behaviour of the builtin version.
> 
> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
> ---
> 
> Notes:
>     This patch is best viewed with `--color-moved'.
> 
>  builtin/merge-index.c | 77 +++------------------------------
>  merge-strategies.c    | 99 +++++++++++++++++++++++++++++++++++++++++++
>  merge-strategies.h    | 17 ++++++++
>  3 files changed, 123 insertions(+), 70 deletions(-)
> 
> diff --git a/builtin/merge-index.c b/builtin/merge-index.c
> index 38ea6ad6ca..6cb666cc78 100644
> --- a/builtin/merge-index.c
> +++ b/builtin/merge-index.c
> @@ -1,74 +1,11 @@
>  #define USE_THE_INDEX_COMPATIBILITY_MACROS
>  #include "builtin.h"
> -#include "run-command.h"
> -
> -static const char *pgm;
> -static int one_shot, quiet;
> -static int err;
> -
> -static int merge_entry(int pos, const char *path)
> -{
> -	int found;
> -	const char *arguments[] = { pgm, "", "", "", path, "", "", "", NULL };
> -	char hexbuf[4][GIT_MAX_HEXSZ + 1];
> -	char ownbuf[4][60];
> -
> -	if (pos >= active_nr)
> -		die("git merge-index: %s not in the cache", path);
> -	found = 0;
> -	do {
> -		const struct cache_entry *ce = active_cache[pos];
> -		int stage = ce_stage(ce);
> -
> -		if (strcmp(ce->name, path))
> -			break;
> -		found++;
> -		oid_to_hex_r(hexbuf[stage], &ce->oid);
> -		xsnprintf(ownbuf[stage], sizeof(ownbuf[stage]), "%o", ce->ce_mode);
> -		arguments[stage] = hexbuf[stage];
> -		arguments[stage + 4] = ownbuf[stage];
> -	} while (++pos < active_nr);
> -	if (!found)
> -		die("git merge-index: %s not in the cache", path);
> -
> -	if (run_command_v_opt(arguments, 0)) {
> -		if (one_shot)
> -			err++;
> -		else {
> -			if (!quiet)
> -				die("merge program failed");
> -			exit(1);
> -		}
> -	}
> -	return found;
> -}
> -
> -static void merge_one_path(const char *path)
> -{
> -	int pos = cache_name_pos(path, strlen(path));
> -
> -	/*
> -	 * If it already exists in the cache as stage0, it's
> -	 * already merged and there is nothing to do.
> -	 */
> -	if (pos < 0)
> -		merge_entry(-pos-1, path);
> -}
> -
> -static void merge_all(void)
> -{
> -	int i;
> -	for (i = 0; i < active_nr; i++) {
> -		const struct cache_entry *ce = active_cache[i];
> -		if (!ce_stage(ce))
> -			continue;
> -		i += merge_entry(i, ce->name)-1;
> -	}
> -}
> +#include "merge-strategies.h"
>  
>  int cmd_merge_index(int argc, const char **argv, const char *prefix)
>  {
> -	int i, force_file = 0;
> +	int i, force_file = 0, err = 0, one_shot = 0, quiet = 0;
> +	const char *pgm;
>  
>  	/* Without this we cannot rely on waitpid() to tell
>  	 * what happened to our children.
> @@ -98,14 +35,14 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix)
>  				continue;
>  			}
>  			if (!strcmp(arg, "-a")) {
> -				merge_all();
> +				err |= merge_all(&the_index, one_shot, quiet,
> +						 merge_program_cb, (void *)pgm);
>  				continue;
>  			}
>  			die("git merge-index: unknown option %s", arg);
>  		}
> -		merge_one_path(arg);
> +		err |= merge_one_path(&the_index, one_shot, quiet, arg,
> +				      merge_program_cb, (void *)pgm);
>  	}
> -	if (err && !quiet)
> -		die("merge program failed");
>  	return err;
>  }
> diff --git a/merge-strategies.c b/merge-strategies.c
> index 3a9fce9f22..f4c0b4acd6 100644
> --- a/merge-strategies.c
> +++ b/merge-strategies.c
> @@ -1,6 +1,7 @@
>  #include "cache.h"
>  #include "dir.h"
>  #include "merge-strategies.h"
> +#include "run-command.h"
>  #include "xdiff-interface.h"
>  
>  static int add_to_index_cacheinfo(struct index_state *istate,
> @@ -189,3 +190,101 @@ int merge_strategies_one_file(struct repository *r,
>  
>  	return 0;
>  }
> +
> +int merge_program_cb(const struct object_id *orig_blob,
> +		     const struct object_id *our_blob,
> +		     const struct object_id *their_blob, const char *path,
> +		     unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode,
> +		     void *data)

Using void* is slightly unfortunate but it's needed later.

It would be nice to check if the program to run is git-merge-one-file
and call the appropriate function instead in that case so all users of
merge-index get the benefit of it being builtin. That probably wants to
be done in cmd_merge_index() rather than here though.

> +{
> +	char ownbuf[3][60] = {{0}};

I know this is copied from above but it would be better to use
GIT_MAX_HEXSZ rather than 60

> +	const char *arguments[] = { (char *)data, "", "", "", path,
> +				    ownbuf[0], ownbuf[1], ownbuf[2],
> +				    NULL };
> +
> +	if (orig_blob)
> +		arguments[1] = oid_to_hex(orig_blob);
> +	if (our_blob)
> +		arguments[2] = oid_to_hex(our_blob);
> +	if (their_blob)
> +		arguments[3] = oid_to_hex(their_blob);
> +
> +	xsnprintf(ownbuf[0], sizeof(ownbuf[0]), "%o", orig_mode);
> +	xsnprintf(ownbuf[1], sizeof(ownbuf[1]), "%o", our_mode);
> +	xsnprintf(ownbuf[2], sizeof(ownbuf[2]), "%o", their_mode);

These are leaked. Also are you sure we want to fill out the mode if the
corresponding blob is missing - I guess it doesn't matter but it would
be good to check that - i think the original passed "". It also passed
"" rather than "0000..." for the blobs that were missing I think.

Best Wishes

Phillip

> +
> +	return run_command_v_opt(arguments, 0);
> +}
> +
> +static int merge_entry(struct index_state *istate, int quiet, int pos,
> +		       const char *path, merge_cb cb, void *data)
> +{
> +	int found = 0;
> +	const struct object_id *oids[3] = {NULL};
> +	unsigned int modes[3] = {0};
> +
> +	do {
> +		const struct cache_entry *ce = istate->cache[pos];
> +		int stage = ce_stage(ce);
> +
> +		if (strcmp(ce->name, path))
> +			break;
> +		found++;
> +		oids[stage - 1] = &ce->oid;
> +		modes[stage - 1] = ce->ce_mode;
> +	} while (++pos < istate->cache_nr);
> +	if (!found)
> +		return error(_("%s is not in the cache"), path);
> +
> +	if (cb(oids[0], oids[1], oids[2], path, modes[0], modes[1], modes[2], data)) {
> +		if (!quiet)
> +			error(_("Merge program failed"));
> +		return -2;
> +	}
> +
> +	return found;
> +}
> +
> +int merge_one_path(struct index_state *istate, int oneshot, int quiet,
> +		   const char *path, merge_cb cb, void *data)
> +{
> +	int pos = index_name_pos(istate, path, strlen(path)), ret;
> +
> +	/*
> +	 * If it already exists in the cache as stage0, it's
> +	 * already merged and there is nothing to do.
> +	 */
> +	if (pos < 0) {
> +		ret = merge_entry(istate, quiet, -pos - 1, path, cb, data);
> +		if (ret == -1)
> +			return -1;
> +		else if (ret == -2)
> +			return 1;
> +	}
> +	return 0;
> +}
> +
> +int merge_all(struct index_state *istate, int oneshot, int quiet,
> +	      merge_cb cb, void *data)
> +{
> +	int err = 0, i, ret;
> +	for (i = 0; i < istate->cache_nr; i++) {
> +		const struct cache_entry *ce = istate->cache[i];
> +		if (!ce_stage(ce))
> +			continue;
> +
> +		ret = merge_entry(istate, quiet, i, ce->name, cb, data);
> +		if (ret > 0)
> +			i += ret - 1;
> +		else if (ret == -1)
> +			return -1;
> +		else if (ret == -2) {
> +			if (oneshot)
> +				err++;
> +			else
> +				return 1;
> +		}
> +	}
> +
> +	return err;
> +}
> diff --git a/merge-strategies.h b/merge-strategies.h
> index b527d145c7..cf78d7eaf4 100644
> --- a/merge-strategies.h
> +++ b/merge-strategies.h
> @@ -10,4 +10,21 @@ int merge_strategies_one_file(struct repository *r,
>  			      unsigned int orig_mode, unsigned int our_mode,
>  			      unsigned int their_mode);
>  
> +typedef int (*merge_cb)(const struct object_id *orig_blob,
> +			const struct object_id *our_blob,
> +			const struct object_id *their_blob, const char *path,
> +			unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode,
> +			void *data);
> +
> +int merge_program_cb(const struct object_id *orig_blob,
> +		     const struct object_id *our_blob,
> +		     const struct object_id *their_blob, const char *path,
> +		     unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode,
> +		     void *data);
> +
> +int merge_one_path(struct index_state *istate, int oneshot, int quiet,
> +		   const char *path, merge_cb cb, void *data);
> +int merge_all(struct index_state *istate, int oneshot, int quiet,
> +	      merge_cb cb, void *data);
> +
>  #endif /* MERGE_STRATEGIES_H */
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [RFC PATCH v1 06/17] merge-index: libify merge_one_path() and merge_all()
  2020-06-26 10:13   ` Phillip Wood
@ 2020-06-26 14:32     ` Phillip Wood
  2020-07-12 11:36     ` Alban Gruin
  1 sibling, 0 replies; 29+ messages in thread
From: Phillip Wood @ 2020-06-26 14:32 UTC (permalink / raw)
  To: Alban Gruin, git; +Cc: Junio C Hamano

Hi Alban

On 26/06/2020 11:13, Phillip Wood wrote:
> Hi Alban
> 
> On 25/06/2020 13:19, Alban Gruin wrote:
>> The "resolve" and "octopus" merge strategies do not call directly `git
>> merge-one-file', they delegate the work to another git command, `git
>> merge-index', that will loop over files in the index and call the
>> specified command.  Unfortunately, these functions are not part of
>> libgit.a, which means that once rewritten, the strategies would still
>> have to invoke `merge-one-file' by spawning a new process first.
>>
>> To avoid this, this moves merge_one_path(), merge_all(), and their
>> helpers to merge-strategies.c.  They also take a callback to dictate
>> what they should do for each file.  For now, only one launching a new
>> process is defined to preserve the behaviour of the builtin version.
>>
>> Signed-off-by: Alban Gruin <alban.gruin@gmail.com>
>> ---
>>
>> Notes:
>>      This patch is best viewed with `--color-moved'.
>>
>>   builtin/merge-index.c | 77 +++------------------------------
>>   merge-strategies.c    | 99 +++++++++++++++++++++++++++++++++++++++++++
>>   merge-strategies.h    | 17 ++++++++
>>   3 files changed, 123 insertions(+), 70 deletions(-)
>>
>> diff --git a/builtin/merge-index.c b/builtin/merge-index.c
>> index 38ea6ad6ca..6cb666cc78 100644
>> --- a/builtin/merge-index.c
>> +++ b/builtin/merge-index.c
>> @@ -1,74 +1,11 @@
>>   #define USE_THE_INDEX_COMPATIBILITY_MACROS
>>   #include "builtin.h"
>> -#include "run-command.h"
>> -
>> -static const char *pgm;
>> -static int one_shot, quiet;
>> -static int err;
>> -
>> -static int merge_entry(int pos, const char *path)
>> -{
>> -	int found;
>> -	const char *arguments[] = { pgm, "", "", "", path, "", "", "", NULL };
>> -	char hexbuf[4][GIT_MAX_HEXSZ + 1];
>> -	char ownbuf[4][60];
>> -
>> -	if (pos >= active_nr)
>> -		die("git merge-index: %s not in the cache", path);
>> -	found = 0;
>> -	do {
>> -		const struct cache_entry *ce = active_cache[pos];
>> -		int stage = ce_stage(ce);
>> -
>> -		if (strcmp(ce->name, path))
>> -			break;
>> -		found++;
>> -		oid_to_hex_r(hexbuf[stage], &ce->oid);
>> -		xsnprintf(ownbuf[stage], sizeof(ownbuf[stage]), "%o", ce->ce_mode);
>> -		arguments[stage] = hexbuf[stage];
>> -		arguments[stage + 4] = ownbuf[stage];
>> -	} while (++pos < active_nr);
>> -	if (!found)
>> -		die("git merge-index: %s not in the cache", path);
>> -
>> -	if (run_command_v_opt(arguments, 0)) {
>> -		if (one_shot)
>> -			err++;
>> -		else {
>> -			if (!quiet)
>> -				die("merge program failed");
>> -			exit(1);
>> -		}
>> -	}
>> -	return found;
>> -}
>> -
>> -static void merge_one_path(const char *path)
>> -{
>> -	int pos = cache_name_pos(path, strlen(path));
>> -
>> -	/*
>> -	 * If it already exists in the cache as stage0, it's
>> -	 * already merged and there is nothing to do.
>> -	 */
>> -	if (pos < 0)
>> -		merge_entry(-pos-1, path);
>> -}
>> -
>> -static void merge_all(void)
>> -{
>> -	int i;
>> -	for (i = 0; i < active_nr; i++) {
>> -		const struct cache_entry *ce = active_cache[i];
>> -		if (!ce_stage(ce))
>> -			continue;
>> -		i += merge_entry(i, ce->name)-1;
>> -	}
>> -}
>> +#include "merge-strategies.h"
>>   
>>   int cmd_merge_index(int argc, const char **argv, const char *prefix)
>>   {
>> -	int i, force_file = 0;
>> +	int i, force_file = 0, err = 0, one_shot = 0, quiet = 0;
>> +	const char *pgm;
>>   
>>   	/* Without this we cannot rely on waitpid() to tell
>>   	 * what happened to our children.
>> @@ -98,14 +35,14 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix)
>>   				continue;
>>   			}
>>   			if (!strcmp(arg, "-a")) {
>> -				merge_all();
>> +				err |= merge_all(&the_index, one_shot, quiet,
>> +						 merge_program_cb, (void *)pgm);
>>   				continue;
>>   			}
>>   			die("git merge-index: unknown option %s", arg);
>>   		}
>> -		merge_one_path(arg);
>> +		err |= merge_one_path(&the_index, one_shot, quiet, arg,
>> +				      merge_program_cb, (void *)pgm);
>>   	}
>> -	if (err && !quiet)
>> -		die("merge program failed");
>>   	return err;
>>   }
>> diff --git a/merge-strategies.c b/merge-strategies.c
>> index 3a9fce9f22..f4c0b4acd6 100644
>> --- a/merge-strategies.c
>> +++ b/merge-strategies.c
>> @@ -1,6 +1,7 @@
>>   #include "cache.h"
>>   #include "dir.h"
>>   #include "merge-strategies.h"
>> +#include "run-command.h"
>>   #include "xdiff-interface.h"
>>   
>>   static int add_to_index_cacheinfo(struct index_state *istate,
>> @@ -189,3 +190,101 @@ int merge_strategies_one_file(struct repository *r,
>>   
>>   	return 0;
>>   }
>> +
>> +int merge_program_cb(const struct object_id *orig_blob,
>> +		     const struct object_id *our_blob,
>> +		     const struct object_id *their_blob, const char *path,
>> +		     unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode,
>> +		     void *data)
> 
> Using void* is slightly unfortunate but it's needed later.
> 
> It would be nice to check if the program to run is git-merge-one-file
> and call the appropriate function instead in that case so all users of
> merge-index get the benefit of it being builtin. That probably wants to
> be done in cmd_merge_index() rather than here though.
> 
>> +{
>> +	char ownbuf[3][60] = {{0}};
> 
> I know this is copied from above but it would be better to use
> GIT_MAX_HEXSZ rather than 60
> 
>> +	const char *arguments[] = { (char *)data, "", "", "", path,
>> +				    ownbuf[0], ownbuf[1], ownbuf[2],
>> +				    NULL };
>> +
>> +	if (orig_blob)
>> +		arguments[1] = oid_to_hex(orig_blob);
>> +	if (our_blob)
>> +		arguments[2] = oid_to_hex(our_blob);
>> +	if (their_blob)
>> +		arguments[3] = oid_to_hex(their_blob);
>> +
>> +	xsnprintf(ownbuf[0], sizeof(ownbuf[0]), "%o", orig_mode);
>> +	xsnprintf(ownbuf[1], sizeof(ownbuf[1]), "%o", our_mode);
>> +	xsnprintf(ownbuf[2], sizeof(ownbuf[2]), "%o", their_mode);

Sorry ignore all the comments below, they are nonsense

Best Wishes

Phillip

> These are leaked. Also are you sure we want to fill out the mode if the
> corresponding blob is missing - I guess it doesn't matter but it would
> be good to check that - i think the original passed "". It also passed
> "" rather than "0000..." for the blobs that were missing I think.
> 
> Best Wishes
> 
> Phillip
> 
>> +
>> +	return run_command_v_opt(arguments, 0);
>> +}
>> +
>> +static int merge_entry(struct index_state *istate, int quiet, int pos,
>> +		       const char *path, merge_cb cb, void *data)
>> +{
>> +	int found = 0;
>> +	const struct object_id *oids[3] = {NULL};
>> +	unsigned int modes[3] = {0};
>> +
>> +	do {
>> +		const struct cache_entry *ce = istate->cache[pos];
>> +		int stage = ce_stage(ce);
>> +
>> +		if (strcmp(ce->name, path))
>> +			break;
>> +		found++;
>> +		oids[stage - 1] = &ce->oid;
>> +		modes[stage - 1] = ce->ce_mode;
>> +	} while (++pos < istate->cache_nr);
>> +	if (!found)
>> +		return error(_("%s is not in the cache"), path);
>> +
>> +	if (cb(oids[0], oids[1], oids[2], path, modes[0], modes[1], modes[2], data)) {
>> +		if (!quiet)
>> +			error(_("Merge program failed"));
>> +		return -2;
>> +	}
>> +
>> +	return found;
>> +}
>> +
>> +int merge_one_path(struct index_state *istate, int oneshot, int quiet,
>> +		   const char *path, merge_cb cb, void *data)
>> +{
>> +	int pos = index_name_pos(istate, path, strlen(path)), ret;
>> +
>> +	/*
>> +	 * If it already exists in the cache as stage0, it's
>> +	 * already merged and there is nothing to do.
>> +	 */
>> +	if (pos < 0) {
>> +		ret = merge_entry(istate, quiet, -pos - 1, path, cb, data);
>> +		if (ret == -1)
>> +			return -1;
>> +		else if (ret == -2)
>> +			return 1;
>> +	}
>> +	return 0;
>> +}
>> +
>> +int merge_all(struct index_state *istate, int oneshot, int quiet,
>> +	      merge_cb cb, void *data)
>> +{
>> +	int err = 0, i, ret;
>> +	for (i = 0; i < istate->cache_nr; i++) {
>> +		const struct cache_entry *ce = istate->cache[i];
>> +		if (!ce_stage(ce))
>> +			continue;
>> +
>> +		ret = merge_entry(istate, quiet, i, ce->name, cb, data);
>> +		if (ret > 0)
>> +			i += ret - 1;
>> +		else if (ret == -1)
>> +			return -1;
>> +		else if (ret == -2) {
>> +			if (oneshot)
>> +				err++;
>> +			else
>> +				return 1;
>> +		}
>> +	}
>> +
>> +	return err;
>> +}
>> diff --git a/merge-strategies.h b/merge-strategies.h
>> index b527d145c7..cf78d7eaf4 100644
>> --- a/merge-strategies.h
>> +++ b/merge-strategies.h
>> @@ -10,4 +10,21 @@ int merge_strategies_one_file(struct repository *r,
>>   			      unsigned int orig_mode, unsigned int our_mode,
>>   			      unsigned int their_mode);
>>   
>> +typedef int (*merge_cb)(const struct object_id *orig_blob,
>> +			const struct object_id *our_blob,
>> +			const struct object_id *their_blob, const char *path,
>> +			unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode,
>> +			void *data);
>> +
>> +int merge_program_cb(const struct object_id *orig_blob,
>> +		     const struct object_id *our_blob,
>> +		     const struct object_id *their_blob, const char *path,
>> +		     unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode,
>> +		     void *data);
>> +
>> +int merge_one_path(struct index_state *istate, int oneshot, int quiet,
>> +		   const char *path, merge_cb cb, void *data);
>> +int merge_all(struct index_state *istate, int oneshot, int quiet,
>> +	      merge_cb cb, void *data);
>> +
>>   #endif /* MERGE_STRATEGIES_H */
>>
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [RFC PATCH v1 02/17] merge-one-file: rewrite in C
  2020-06-25 18:17     ` Phillip Wood
@ 2020-06-26 14:33       ` Phillip Wood
  0 siblings, 0 replies; 29+ messages in thread
From: Phillip Wood @ 2020-06-26 14:33 UTC (permalink / raw)
  To: Alban Gruin, git; +Cc: Junio C Hamano

On 25/06/2020 19:17, Phillip Wood wrote:
> On 25/06/2020 16:16, Phillip Wood wrote:
>> Hi Alban
>>
>> I think this series is a great idea
>>
>> On 25/06/2020 13:19, Alban Gruin wrote:
>>> This rewrites `git merge-one-file' from shell to C.  This port is very
>>> straightforward: it keeps using external processes to edit the index,
>>> for instance.  Errors are also displayed with fprintf() instead of
>>> error().  Both of these will be addressed in the next few commits,
>>> leading to its libification so its main function can be used from other
>>> commands directly.
>>>
>>> This also fixes a bug present in the original script: instead of
>>> checking if a _regular_ file exists when a file exists in the branch to
>>> merge, but not in our branch, the rewritten version checks if a file of
>>> any kind (ie. a directory, ...) exists.  This fixes the tests t6035.14,
>>> where the branch to merge had a new file, `a/b', but our branch had a
>>> directory there; it should have failed because a directory exists, but
>>> it did not because there was no regular file called `a/b'.  This test is
>>> now marked as successful.
>>> [...]
>>> +static int merge_one_file(const struct object_id *orig_blob,
>>> +              const struct object_id *our_blob,
>>> +              const struct object_id *their_blob, const char *path,
>>> +              unsigned int orig_mode, unsigned int our_mode, unsigned
>>> int their_mode)
>>> +{
>>> +    if (orig_blob &&
>>> +        ((our_blob && oideq(orig_blob, our_blob)) ||
>>> +         (their_blob && oideq(orig_blob, their_blob))))
>>> +        return merge_one_file_deleted(orig_blob, our_blob,
>>> their_blob, path,
>>> +                          orig_mode, our_mode, their_mode);
>>
>> It would be nice to preserve the comments from the script as I find they
>> help a lot in understanding which case each piece of code is handling.
>> The code above appears to be handling deletions but does not appear to
>> check that one side is actually missing. Shouldn't it be something like
>>
>> if (orig_blob &&
>>      ((!their_blob && (our_blob && oideq(orig_blob, our_blob))) ||
>>       (!our_blob && (their_blob && oideq(orig_blob, their_blob))))
>>
>> Maybe this could do with a test case
> 
> The reason your version works is that if only one side has changed
> read-tree will have done the merge itself so this only gets called if
> one side has been deleted. However the original script printed an error
> if someone accidentally called when the content had changed in only one
> side and there were no mode changes. I think we want to keep that behavior.

Actually I think the original probably handles this case by calling 'git 
merge-file'

Best Wishes

Phillip

> In the future we could probably update this to also handle the cases
> that read-tree normally takes care of rather than erroring out but I
> don't think it is a high priority.
> 
> Best Wishes
> 
> Phillip
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [RFC PATCH v1 02/17] merge-one-file: rewrite in C
  2020-06-25 15:16   ` Phillip Wood
  2020-06-25 18:17     ` Phillip Wood
@ 2020-07-12 11:22     ` Alban Gruin
  1 sibling, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2020-07-12 11:22 UTC (permalink / raw)
  To: phillip.wood, git; +Cc: Junio C Hamano

[-- Attachment #1: Type: text/plain, Size: 6408 bytes --]

Hi Phillip,

Phillip Wood (phillip.wood123@gmail.com) a écrit :

> Hi Alban
> 
> I think this series is a great idea
> 
> On 25/06/2020 13:19, Alban Gruin wrote:
> -%<-
> > diff --git a/builtin/merge-one-file.c b/builtin/merge-one-file.c
> > new file mode 100644
> > index 0000000000..4992a6cd30
> > --- /dev/null
> > +++ b/builtin/merge-one-file.c
> > @@ -0,0 +1,275 @@
> > +/*
> > + * 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 script, called with
> > + *
> > + *   $1 - original file SHA1 (or empty)
> > + *   $2 - file in branch1 SHA1 (or empty)
> > + *   $3 - file in branch2 SHA1 (or empty)
> > + *   $4 - pathname in repository
> > + *   $5 - original file mode (or empty)
> > + *   $6 - file in branch1 mode (or empty)
> > + *   $7 - file in branch2 mode (or empty)
> 
> nit pick - these are now argv[1] etc rather than $1 etc
> 

I'll change that, and replace "script" by "utility".

> > + *
> > + * 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.
> > + */
> > +
> > +#define USE_THE_INDEX_COMPATIBILITY_MACROS
> > +#include "cache.h"
> > +#include "builtin.h"
> > +#include "commit.h"
> > +#include "dir.h"
> > +#include "lockfile.h"
> > +#include "object-store.h"
> > +#include "run-command.h"
> > +#include "xdiff-interface.h"
> > +
> > +static int create_temp_file(const struct object_id *oid, struct strbuf
> > *path)
> > +{
> > +	struct child_process cp = CHILD_PROCESS_INIT;
> > +	struct strbuf err = STRBUF_INIT;
> > +	int ret;
> > +
> > +	cp.git_cmd = 1;
> > +	argv_array_pushl(&cp.args, "unpack-file", oid_to_hex(oid), NULL);
> > +	ret = pipe_command(&cp, NULL, 0, path, 0, &err, 0);
> > +	if (!ret && path->len > 0)
> > +		strbuf_trim_trailing_newline(path);
> > +
> > +	fprintf(stderr, "%.*s", (int) err.len, err.buf);
> > +	strbuf_release(&err);
> > +
> > +	return ret;
> > +}
> 
> I know others will disagree but personally I'm not a huge fan of rewriting
> shell functions in C that forks other builtins and then converting the C to
> use the internal apis, it seems a much better to just write the proper C
> version the first time. This is especially true for simple function such as
> the ones in this file. That way the reviewer gets a clear view of the final
> code from the patch, rather than having to piece it together from a series of
> additions and deletions.
> 

I understand -- I'll squash the "rewrite" and "use internal APIs" patches 
together as a last step for the v2, so I'd be able to get them back with 
all the changes made in the v2 if needed.

> -%<-
> > +static int do_merge_one_file(const struct object_id *orig_blob,
> > +			     const struct object_id *our_blob,
> > +			     const struct object_id *their_blob, const char
> > *path,
> > +			     unsigned int orig_mode, unsigned int our_mode,
> > unsigned int their_mode)
> > +{
> > +	int ret, source, dest;
> > +	struct strbuf src1 = STRBUF_INIT, src2 = STRBUF_INIT, orig =
> > STRBUF_INIT;
> > +	struct child_process cp_merge = CHILD_PROCESS_INIT,
> > +		cp_checkout = CHILD_PROCESS_INIT,
> > +		cp_update = CHILD_PROCESS_INIT;
> > +
> > +	if (our_mode == S_IFLNK || their_mode == S_IFLNK) {
> > +		fprintf(stderr, "ERROR: %s: Not merging symbolic link
> > changes.\n", path);
> > +		return 1;
> > +	} else if (our_mode == S_IFGITLINK || their_mode == S_IFGITLINK) {
> > +		fprintf(stderr, "ERROR: %s: Not merging conflicting submodule
> > changes.\n",
> > +			path);
> > +		return 1;
> > +	}
> > +
> > +	create_temp_file(our_blob, &src1);
> > +	create_temp_file(their_blob, &src2);
> > +
> > +	if (orig_blob) {
> > +		printf("Auto-merging %s\n", path);
> > +		create_temp_file(orig_blob, &orig);
> > +	} else {
> > +		printf("Added %s in both, but differently.\n", path);
> > +		create_temp_file(the_hash_algo->empty_blob, &orig);
> > +	}
> > +
> > +	cp_merge.git_cmd = 1;
> > +	argv_array_pushl(&cp_merge.args, "merge-file", src1.buf, orig.buf,
> > src2.buf,
> > +			 NULL);
> > +	ret = run_command(&cp_merge);
> > +
> > +	if (ret != 0)
> > +		ret = 1;
> > +
> > +	cp_checkout.git_cmd = 1;
> > +	argv_array_pushl(&cp_checkout.args, "checkout-index", "-f",
> > "--stage=2",
> > +			 "--", path, NULL);
> > +	if (run_command(&cp_checkout))
> > +		return 1;
> > +
> > +	source = open(src1.buf, O_RDONLY);
> > +	dest = open(path, O_WRONLY | O_TRUNC);
> > +
> > +	copy_fd(source, dest);
> > +
> > +	close(source);
> > +	close(dest);
> > +
> > +	unlink(orig.buf);
> > +	unlink(src1.buf);
> > +	unlink(src2.buf);
> > +
> > +	strbuf_release(&src1);
> > +	strbuf_release(&src2);
> > +	strbuf_release(&orig);
> 
> The whole business of creating temporary files and forking seems like a lot of
> effort compared to calling ll_merge() which would also mean we respect any
> merge attributes
> 
> > +
> > +	if (ret) {
> > +		fprintf(stderr, "ERROR: ");
> > +
> > +		if (!orig_blob) {
> 
> I think the original does if (ret || !orig_blob) not &&

Good catch.

> > +			fprintf(stderr, "content conflict");
> > +			if (our_mode != their_mode)
> > +				fprintf(stderr, ", ");
> 
> sentence lego, in any case the message below should be printed regardless of
> content conflicts. We should probably mark all these messages for translation
> as well.
> 

Yeah, I think I will replace them with two calls to `error()'.

> -%<-
> > +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;
> > +
> > +	if (argc != 8)
> > +		usage(builtin_merge_one_file_usage);
> > +
> > +	if (!get_oid(argv[1], &orig_blob)) {
> > +		p_orig_blob = &orig_blob;
> > +		orig_mode = strtol(argv[5], NULL, 8);
> 
> It would probably make sense to check that strtol() succeeds (and the mode is
> sensible), and also that get_oid() fails because argv[1] is empty, not because
> it is invalid.
> 

Checking that `orig_mode' and friends are lower than 0800, and that 
`*argv[1]' is not equal to '\0' should be enough, right?

> Thanks for working on this

As always, thank you for your reviews.

> Best Wishes
> 
> Phillip
> 
> 

Cheers,
Alban

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [RFC PATCH v1 16/17] sequencer: use the "resolve" strategy without forking
  2020-06-25 16:11   ` Phillip Wood
@ 2020-07-12 11:27     ` Alban Gruin
  0 siblings, 0 replies; 29+ messages in thread
From: Alban Gruin @ 2020-07-12 11:27 UTC (permalink / raw)
  To: phillip.wood, git; +Cc: Junio C Hamano

[-- Attachment #1: Type: text/plain, Size: 932 bytes --]

Hi Phillip,

Phillip Wood (phillip.wood123@gmail.com) a écrit :

> Hi Alban
> 
> On 25/06/2020 13:19, Alban Gruin wrote:
> > This teaches the sequencer to invoke the "resolve" strategy with a
> > function call instead of forking.
> 
> This is a good idea, however we should check the existing tests that use this
> strategy to see if they are doing so to test the try_merge_command() code
> path. I've got some patches in seen that use '--strategy=resolve' to exercise
> the "non merge-recursive" code path, so I'll update them to use a proper
> custom merge strategy.
> 
> Is it worth optimizing do_merge() to take advantage of resolve and octopus
> being builtin as well?
> 

Hmm, I see that do_merge() doesn't call directly the strategies, and 
delegates this work to git-merge.  If calling the new APIs does not imply 
to copy/paste too much code from merge.c, then my answer is yes.

> Best Wishes
> 
> Phil
> 

Cheers,
Alban

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [RFC PATCH v1 06/17] merge-index: libify merge_one_path() and merge_all()
  2020-06-26 10:13   ` Phillip Wood
  2020-06-26 14:32     ` Phillip Wood
@ 2020-07-12 11:36     ` Alban Gruin
  2020-07-12 18:02       ` Phillip Wood
  1 sibling, 1 reply; 29+ messages in thread
From: Alban Gruin @ 2020-07-12 11:36 UTC (permalink / raw)
  To: Phillip Wood, git; +Cc: Junio C Hamano

Hi Phillip,

Phillip Wood (phillip.wood123@gmail.com) a écrit :

> Hi Alban
> 
> On 25/06/2020 13:19, Alban Gruin wrote:
> -%<-
> > diff --git a/merge-strategies.c b/merge-strategies.c
> > index 3a9fce9f22..f4c0b4acd6 100644
> > --- a/merge-strategies.c
> > +++ b/merge-strategies.c
> > @@ -1,6 +1,7 @@
> >  #include "cache.h"
> >  #include "dir.h"
> >  #include "merge-strategies.h"
> > +#include "run-command.h"
> >  #include "xdiff-interface.h"
> >  
> >  static int add_to_index_cacheinfo(struct index_state *istate,
> > @@ -189,3 +190,101 @@ int merge_strategies_one_file(struct repository *r,
> >  
> >  	return 0;
> >  }
> > +
> > +int merge_program_cb(const struct object_id *orig_blob,
> > +		     const struct object_id *our_blob,
> > +		     const struct object_id *their_blob, const char *path,
> > +		     unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode,
> > +		     void *data)
> 
> Using void* is slightly unfortunate but it's needed later.
> 
> It would be nice to check if the program to run is git-merge-one-file
> and call the appropriate function instead in that case so all users of
> merge-index get the benefit of it being builtin. That probably wants to
> be done in cmd_merge_index() rather than here though.
> 

Dunno, I am not completely comfortable with changing a parameter that 
specifically describe a program, to a parameter that may be a program, 
except in one case where `merge-index' should lock the index, setup the 
worktree, and call a function instead.

Well, I say that, but implementing that behaviour is not that hard:

-- snip --
diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index 6cb666cc78..19fff9a113 100644
--- a/builtin/merge-index.c
+++ b/builtin/merge-index.c
@@ -1,11 +1,15 @@
 #define USE_THE_INDEX_COMPATIBILITY_MACROS
 #include "builtin.h"
+#include "lockfile.h"
 #include "merge-strategies.h"
 
 int cmd_merge_index(int argc, const char **argv, const char *prefix)
 {
 	int i, force_file = 0, err = 0, one_shot = 0, quiet = 0;
 	const char *pgm;
+	void *data;
+	merge_cb merge_action;
+	struct lock_file lock = LOCK_INIT;
 
 	/* Without this we cannot rely on waitpid() to tell
 	 * what happened to our children.
@@ -26,7 +30,19 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix)
 		quiet = 1;
 		i++;
 	}
+
 	pgm = argv[i++];
+	if (!strcmp(pgm, "git-merge-one-file")) {
+		merge_action = merge_one_file_cb;
+		data = (void *)the_repository;
+
+		setup_work_tree();
+		hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
+	} else {
+		merge_action = merge_program_cb;
+		data = (void *)pgm;
+	}
+
 	for (; i < argc; i++) {
 		const char *arg = argv[i];
 		if (!force_file && *arg == '-') {
@@ -36,13 +52,22 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix)
 			}
 			if (!strcmp(arg, "-a")) {
 				err |= merge_all(&the_index, one_shot, quiet,
-						 merge_program_cb, (void *)pgm);
+						 merge_action, data);
 				continue;
 			}
 			die("git merge-index: unknown option %s", arg);
 		}
 		err |= merge_one_path(&the_index, one_shot, quiet, arg,
-				      merge_program_cb, (void *)pgm);
+				      merge_action, data);
+	}
+
+	if (merge_action == merge_one_file_cb) {
+		if (err) {
+			rollback_lock_file(&lock);
+			return err;
+		}
+
+		return write_locked_index(&the_index, &lock, COMMIT_LOCK);
 	}
 	return err;
 }
-- snap --

> > +{
> > +	char ownbuf[3][60] = {{0}};
> 
> I know this is copied from above but it would be better to use
> GIT_MAX_HEXSZ rather than 60
> 

Cheers,
Alban


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [RFC PATCH v1 06/17] merge-index: libify merge_one_path() and merge_all()
  2020-07-12 11:36     ` Alban Gruin
@ 2020-07-12 18:02       ` Phillip Wood
  0 siblings, 0 replies; 29+ messages in thread
From: Phillip Wood @ 2020-07-12 18:02 UTC (permalink / raw)
  To: Alban Gruin, git; +Cc: Junio C Hamano

Hi Alban

On 12/07/2020 12:36, Alban Gruin wrote:
> Hi Phillip,
> 
> Phillip Wood (phillip.wood123@gmail.com) a écrit :
> 
>> Hi Alban
>>
>> On 25/06/2020 13:19, Alban Gruin wrote:
>> -%<-
>>> diff --git a/merge-strategies.c b/merge-strategies.c
>>> index 3a9fce9f22..f4c0b4acd6 100644
>>> --- a/merge-strategies.c
>>> +++ b/merge-strategies.c
>>> @@ -1,6 +1,7 @@
>>>  #include "cache.h"
>>>  #include "dir.h"
>>>  #include "merge-strategies.h"
>>> +#include "run-command.h"
>>>  #include "xdiff-interface.h"
>>>  
>>>  static int add_to_index_cacheinfo(struct index_state *istate,
>>> @@ -189,3 +190,101 @@ int merge_strategies_one_file(struct repository *r,
>>>  
>>>  	return 0;
>>>  }
>>> +
>>> +int merge_program_cb(const struct object_id *orig_blob,
>>> +		     const struct object_id *our_blob,
>>> +		     const struct object_id *their_blob, const char *path,
>>> +		     unsigned int orig_mode, unsigned int our_mode, unsigned int their_mode,
>>> +		     void *data)
>>
>> Using void* is slightly unfortunate but it's needed later.
>>
>> It would be nice to check if the program to run is git-merge-one-file
>> and call the appropriate function instead in that case so all users of
>> merge-index get the benefit of it being builtin. That probably wants to
>> be done in cmd_merge_index() rather than here though.
>>
> 
> Dunno, I am not completely comfortable with changing a parameter that 
> specifically describe a program, to a parameter that may be a program, 
> except in one case where `merge-index' should lock the index, setup the 
> worktree, and call a function instead.

There is some previous discussion about this at
https://lore.kernel.org/git/xmqqblv5kr9u.fsf@gitster-ct.c.googlers.com/

I'll try and have a proper look at your comments towards the end of the
week (or maybe the week after the way things are at the moment...)

Best Wishes

Phillip

> Well, I say that, but implementing that behaviour is not that hard:
> 
> -- snip --
> diff --git a/builtin/merge-index.c b/builtin/merge-index.c
> index 6cb666cc78..19fff9a113 100644
> --- a/builtin/merge-index.c
> +++ b/builtin/merge-index.c
> @@ -1,11 +1,15 @@
>  #define USE_THE_INDEX_COMPATIBILITY_MACROS
>  #include "builtin.h"
> +#include "lockfile.h"
>  #include "merge-strategies.h"
>  
>  int cmd_merge_index(int argc, const char **argv, const char *prefix)
>  {
>  	int i, force_file = 0, err = 0, one_shot = 0, quiet = 0;
>  	const char *pgm;
> +	void *data;
> +	merge_cb merge_action;
> +	struct lock_file lock = LOCK_INIT;
>  
>  	/* Without this we cannot rely on waitpid() to tell
>  	 * what happened to our children.
> @@ -26,7 +30,19 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix)
>  		quiet = 1;
>  		i++;
>  	}
> +
>  	pgm = argv[i++];
> +	if (!strcmp(pgm, "git-merge-one-file")) {
> +		merge_action = merge_one_file_cb;
> +		data = (void *)the_repository;
> +
> +		setup_work_tree();
> +		hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
> +	} else {
> +		merge_action = merge_program_cb;
> +		data = (void *)pgm;
> +	}
> +
>  	for (; i < argc; i++) {
>  		const char *arg = argv[i];
>  		if (!force_file && *arg == '-') {
> @@ -36,13 +52,22 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix)
>  			}
>  			if (!strcmp(arg, "-a")) {
>  				err |= merge_all(&the_index, one_shot, quiet,
> -						 merge_program_cb, (void *)pgm);
> +						 merge_action, data);
>  				continue;
>  			}
>  			die("git merge-index: unknown option %s", arg);
>  		}
>  		err |= merge_one_path(&the_index, one_shot, quiet, arg,
> -				      merge_program_cb, (void *)pgm);
> +				      merge_action, data);
> +	}
> +
> +	if (merge_action == merge_one_file_cb) {
> +		if (err) {
> +			rollback_lock_file(&lock);
> +			return err;
> +		}
> +
> +		return write_locked_index(&the_index, &lock, COMMIT_LOCK);
>  	}
>  	return err;
>  }
> -- snap --
> 
>>> +{
>>> +	char ownbuf[3][60] = {{0}};
>>
>> I know this is copied from above but it would be better to use
>> GIT_MAX_HEXSZ rather than 60
>>
> 
> Cheers,
> Alban
> 


^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, back to index

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-25 12:19 [RFC PATCH v1 00/17] Rewrite the remaining merge strategies from shell to C Alban Gruin
2020-06-25 12:19 ` [RFC PATCH v1 01/17] t6027: modernise tests Alban Gruin
2020-06-25 12:19 ` [RFC PATCH v1 02/17] merge-one-file: rewrite in C Alban Gruin
2020-06-25 14:55   ` Chris Torek
2020-06-25 15:16   ` Phillip Wood
2020-06-25 18:17     ` Phillip Wood
2020-06-26 14:33       ` Phillip Wood
2020-07-12 11:22     ` Alban Gruin
2020-06-25 12:19 ` [RFC PATCH v1 03/17] merge-one-file: remove calls to external processes Alban Gruin
2020-06-25 12:19 ` [RFC PATCH v1 04/17] merge-one-file: use error() instead of fprintf(stderr, ...) Alban Gruin
2020-06-25 12:19 ` [RFC PATCH v1 05/17] merge-one-file: libify merge_one_file() Alban Gruin
2020-06-25 12:19 ` [RFC PATCH v1 06/17] merge-index: libify merge_one_path() and merge_all() Alban Gruin
2020-06-26 10:13   ` Phillip Wood
2020-06-26 14:32     ` Phillip Wood
2020-07-12 11:36     ` Alban Gruin
2020-07-12 18:02       ` Phillip Wood
2020-06-25 12:19 ` [RFC PATCH v1 07/17] merge-resolve: rewrite in C Alban Gruin
2020-06-25 12:19 ` [RFC PATCH v1 08/17] merge-resolve: remove calls to external processes Alban Gruin
2020-06-25 12:19 ` [RFC PATCH v1 09/17] merge-resolve: libify merge_resolve() Alban Gruin
2020-06-25 12:19 ` [RFC PATCH v1 10/17] merge-recursive: move better_branch_name() to merge.c Alban Gruin
2020-06-25 12:19 ` [RFC PATCH v1 11/17] merge-octopus: rewrite in C Alban Gruin
2020-06-25 12:19 ` [RFC PATCH v1 12/17] merge-octopus: remove calls to external processes Alban Gruin
2020-06-25 12:19 ` [RFC PATCH v1 13/17] merge-octopus: libify merge_octopus() Alban Gruin
2020-06-25 12:19 ` [RFC PATCH v1 14/17] merge: use the "resolve" strategy without forking Alban Gruin
2020-06-25 12:19 ` [RFC PATCH v1 15/17] merge: use the "octopus" " Alban Gruin
2020-06-25 12:19 ` [RFC PATCH v1 16/17] sequencer: use the "resolve" " Alban Gruin
2020-06-25 16:11   ` Phillip Wood
2020-07-12 11:27     ` Alban Gruin
2020-06-25 12:19 ` [RFC PATCH v1 17/17] sequencer: use the "octopus" merge " Alban Gruin

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git