[PATCH 0/2] Avoid run_command() for recursive in builtin-merge

STALE3706d

13 messages, 4 authors, 2016-06-15 · open the first message on its own page

[PATCH 0/2] Avoid run_command() for recursive in builtin-merge

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:07

Hi,

The idea (by Dscho) is simple: builtin-merge-recursive already has a
merge_recursive() function that is _almost_ ready to be called from
builtins without fork+exec, so make use of it.

And yes, of course this is not for 1.6.0, I just wanted to send this out
before the "suggested 'pencils down' date" of GSoC.

Miklos Vajna (2):
  merge-recursive: prepare merge_recursive() to be called from builtins
  builtin-merge: avoid run_command_v_opt() for recursive

 builtin-merge-recursive.c |   19 +++++++---
 builtin-merge.c           |   82 +++++++++++++++++++++++++++++++--------------
 2 files changed, 70 insertions(+), 31 deletions(-)

[PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:07

When other builtins call merge_recursive(), they would have to handle
the GIT_MERGE_VERBOSITY environment variable, causing a code
duplication. Same story for the git_config() call. It's better to do it
when merge_recursive() is called the first time.

Signed-off-by: Miklos Vajna <redacted>
---
 builtin-merge-recursive.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c
index 43e55bf..09aa830 100644
--- a/builtin-merge-recursive.c
+++ b/builtin-merge-recursive.c

@@ -1218,6 +1218,8 @@ static struct commit_list *reverse_commit_list(struct commit_list *list)
 	return next;
 }
 
+static int merge_config(const char *var, const char *value, void *cb);
+
 /*
  * Merge the commits h1 and h2, return the resulting virtual
  * commit object and a flag indicating the cleanness of the merge.

@@ -1233,6 +1235,17 @@ int merge_recursive(struct commit *h1,
 	struct commit *merged_common_ancestors;
 	struct tree *mrtree = mrtree;
 	int clean;
+	static int initial = 1;
+
+	if (initial) {
+		git_config(merge_config, NULL);
+		if (getenv("GIT_MERGE_VERBOSITY"))
+			verbosity = strtol(getenv("GIT_MERGE_VERBOSITY"), NULL, 10);
+		if (verbosity >= 5)
+			buffer_output = 0;
+
+		initial = 0;
+	}
 
 	if (show(4)) {
 		output(4, "Merging:");

@@ -1369,10 +1382,6 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 			subtree_merge = 1;
 	}
 
-	git_config(merge_config, NULL);
-	if (getenv("GIT_MERGE_VERBOSITY"))
-		verbosity = strtol(getenv("GIT_MERGE_VERBOSITY"), NULL, 10);
-
 	if (argc < 4)
 		die("Usage: %s <base>... -- <head> <remote> ...\n", argv[0]);

@@ -1384,8 +1393,6 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 	}
 	if (argc - i != 3) /* "--" "<head>" "<remote>" */
 		die("Not handling anything other than two heads merge.");
-	if (verbosity >= 5)
-		buffer_output = 0;
 
 	branch1 = argv[++i];
 	branch2 = argv[++i];

-- 
1.6.0.rc0.14.g95f8.dirty

[PATCH 2/2] builtin-merge: avoid run_command_v_opt() for recursive

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:07

The try_merge_strategy() function always ran the strategy in a separate
process, though this is not always necessary. The recursive strategy can
be called without a fork(). This patch adds a check, and calls recursive
in the same process without wasting resources.

Signed-off-by: Johannes Schindelin <redacted>
Signed-off-by: Miklos Vajna <redacted>
---

Adding the signoff of Dscho as well, as he wrote the last hunk.

 builtin-merge.c |   82 ++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 57 insertions(+), 25 deletions(-)

diff --git a/builtin-merge.c b/builtin-merge.c
index dde0c7e..2789a78 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c

@@ -22,6 +22,7 @@
 #include "log-tree.h"
 #include "color.h"
 #include "rerere.h"
+#include "merge-recursive.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)

@@ -511,28 +512,55 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
 	struct commit_list *j;
 	struct strbuf buf;
 
-	args = xmalloc((4 + commit_list_count(common) +
-			commit_list_count(remoteheads)) * sizeof(char *));
-	strbuf_init(&buf, 0);
-	strbuf_addf(&buf, "merge-%s", strategy);
-	args[i++] = buf.buf;
-	for (j = common; j; j = j->next)
-		args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));
-	args[i++] = "--";
-	args[i++] = head_arg;
-	for (j = remoteheads; j; j = j->next)
-		args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));
-	args[i] = NULL;
-	ret = run_command_v_opt(args, RUN_GIT_CMD);
-	strbuf_release(&buf);
-	i = 1;
-	for (j = common; j; j = j->next)
-		free((void *)args[i++]);
-	i += 2;
-	for (j = remoteheads; j; j = j->next)
-		free((void *)args[i++]);
-	free(args);
-	return -ret;
+	if (!strcmp(strategy, "recursive")) {
+		int clean;
+		struct commit *result;
+		struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
+		int index_fd;
+		struct commit_list *reversed = NULL;
+
+		if (remoteheads->next) {
+			error("Not handling anything other than two heads merge.");
+			return 2;
+		}
+
+		for (j = common; j; j = j->next)
+			commit_list_insert(j->item, &reversed);
+
+		index_fd = hold_locked_index(lock, 1);
+		clean = merge_recursive(lookup_commit(head),
+				remoteheads->item, head_arg,
+				(const char *)remoteheads->item->util,
+				reversed, &result);
+		if (active_cache_changed &&
+				(write_cache(index_fd, active_cache, active_nr) ||
+				 commit_locked_index(lock)))
+			die ("unable to write %s", get_index_file());
+		return clean ? 0 : 1;
+	} else {
+		args = xmalloc((4 + commit_list_count(common) +
+					commit_list_count(remoteheads)) * sizeof(char *));
+		strbuf_init(&buf, 0);
+		strbuf_addf(&buf, "merge-%s", strategy);
+		args[i++] = buf.buf;
+		for (j = common; j; j = j->next)
+			args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));
+		args[i++] = "--";
+		args[i++] = head_arg;
+		for (j = remoteheads; j; j = j->next)
+			args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));
+		args[i] = NULL;
+		ret = run_command_v_opt(args, RUN_GIT_CMD);
+		strbuf_release(&buf);
+		i = 1;
+		for (j = common; j; j = j->next)
+			free((void *)args[i++]);
+		i += 2;
+		for (j = remoteheads; j; j = j->next)
+			free((void *)args[i++]);
+		free(args);
+		return -ret;
+	}
 }
 
 static void count_diff_files(struct diff_queue_struct *q,

@@ -670,7 +698,9 @@ static int finish_automerge(struct commit_list *common,
 	struct strbuf buf = STRBUF_INIT;
 	unsigned char result_commit[20];
 
-	free_commit_list(common);
+	if (strcmp(wt_strategy, "recursive"))
+		/* recursive already freed it */
+		free_commit_list(common);
 	if (allow_fast_forward) {
 		parents = remoteheads;
 		commit_list_insert(lookup_commit(head), &parents);

@@ -873,12 +903,14 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 
 	for (i = 0; i < argc; i++) {
 		struct object *o;
+		struct commit *commit;
 
 		o = peel_to_type(argv[i], 0, NULL, OBJ_COMMIT);
 		if (!o)
 			die("%s - not something we can merge", argv[i]);
-		remotes = &commit_list_insert(lookup_commit(o->sha1),
-			remotes)->next;
+		commit = lookup_commit(o->sha1);
+		commit->util = (void *)argv[i];
+		remotes = &commit_list_insert(commit, remotes)->next;
 
 		strbuf_addf(&buf, "GITHEAD_%s", sha1_to_hex(o->sha1));
 		setenv(buf.buf, argv[i], 1);

-- 
1.6.0.rc0.14.g95f8.dirty

[PATCH] builtin-revert.c: Make use of merge_recursive()

From: Stephan Beyer <hidden>
Date: 2016-06-15 22:45:08

Cherry-pick and revert always ran the merging in a separate process.
This patch makes cherry-pick/revert call merge_recursive() instead
of running git-merge-recursive.

Signed-off-by: Stephan Beyer <redacted>
---
	Hi,
	I wonder if this patch fits in line.

 builtin-merge-recursive.c |    2 +-
 builtin-revert.c          |   41 ++++++++++++++++++++++-------------------
 merge-recursive.h         |    1 +
 3 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c
index 09aa830..d8bd21f 100644
--- a/builtin-merge-recursive.c
+++ b/builtin-merge-recursive.c

@@ -1327,7 +1327,7 @@ static const char *better_branch_name(const char *branch)
 	return name ? name : branch;
 }
 
-static struct commit *get_ref(const char *ref)
+struct commit *get_ref(const char *ref)
 {
 	unsigned char sha1[20];
 	struct object *object;

diff --git a/builtin-revert.c b/builtin-revert.c
index 27881e9..c54cf8a 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c

@@ -11,6 +11,7 @@
 #include "cache-tree.h"
 #include "diff.h"
 #include "revision.h"
+#include "merge-recursive.h"
 
 /*
  * This implements the builtins revert and cherry-pick.

@@ -200,18 +201,14 @@ static void set_author_ident_env(const char *message)
 			sha1_to_hex(commit->object.sha1));
 }
 
-static int merge_recursive(const char *base_sha1,
+static int merge_recursive_helper(const char *base_sha1,
 		const char *head_sha1, const char *head_name,
 		const char *next_sha1, const char *next_name)
 {
-	char buffer[256];
-	const char *argv[6];
-	int i = 0;
-
-	sprintf(buffer, "GITHEAD_%s", head_sha1);
-	setenv(buffer, head_name, 1);
-	sprintf(buffer, "GITHEAD_%s", next_sha1);
-	setenv(buffer, next_name, 1);
+	int clean, index_fd;
+	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
+	struct commit *result, *h1, *h2;
+	struct commit_list *ca = NULL;
 
 	/*
 	 * This three way merge is an interesting one.  We are at

@@ -219,15 +216,21 @@ static int merge_recursive(const char *base_sha1,
 	 * and $prev on top of us (when reverting), or the change between
 	 * $prev and $commit on top of us (when cherry-picking or replaying).
 	 */
-	argv[i++] = "merge-recursive";
-	if (base_sha1)
-		argv[i++] = base_sha1;
-	argv[i++] = "--";
-	argv[i++] = head_sha1;
-	argv[i++] = next_sha1;
-	argv[i++] = NULL;
-
-	return run_command_v_opt(argv, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD);
+	if (base_sha1) {
+		struct commit *base = get_ref(base_sha1);
+		commit_list_insert(base, &ca);
+	}
+	h1 = get_ref(head_sha1);
+	h2 = get_ref(next_sha1);
+
+	index_fd = hold_locked_index(lock, 1);
+	clean = merge_recursive(h1, h2, head_name, next_name, ca, &result);
+	if (active_cache_changed &&
+			(write_cache(index_fd, active_cache, active_nr) ||
+			 commit_locked_index(lock)))
+		die("Unable to write index.");
+
+	return clean ? 0 : 1;
 }
 
 static char *help_msg(const unsigned char *sha1)

@@ -373,7 +376,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		}
 	}
 
-	if (merge_recursive(base == NULL ?
+	if (merge_recursive_helper(base == NULL ?
 				NULL : sha1_to_hex(base->object.sha1),
 				sha1_to_hex(head), "HEAD",
 				sha1_to_hex(next->object.sha1), oneline) ||

diff --git a/merge-recursive.h b/merge-recursive.h
index f37630a..40f329b 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h

@@ -1,6 +1,7 @@
 #ifndef MERGE_RECURSIVE_H
 #define MERGE_RECURSIVE_H
 
+struct commit *get_ref(const char *ref);
 int merge_recursive(struct commit *h1,
 		    struct commit *h2,
 		    const char *branch1,

-- 
1.6.0.rc2.264.g563b6

Re: [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins

From: Stephan Beyer <hidden>
Date: 2016-06-15 22:45:08

Hi,

Miklos Vajna wrote:

When other builtins call merge_recursive(), they would have to handle
the GIT_MERGE_VERBOSITY environment variable, causing a code
duplication. Same story for the git_config() call. It's better to do it
when merge_recursive() is called the first time.

Hmm, I have the long-run vision that we have a nice libgit some day,
with merge_recursive() being part of it.  And I'm a little unsure if
libified functions should rely on environment variables.
So I'm wondering if the verbosity should be set in the caller functions
of merge_recursive(), i.e. that cmd_merge_recursive() and cmd_merge()
(or another part of builtin-merge.c) does the
	getenv("GIT_MERGE_VERBOSITY")
stuff and a verbosity value could be a new argument to merge_recursive().
Then other merge_recursive() users don't need to
	setenv("GIT_MERGE_VERBOSITY", "3", 1)
or something, they just pass 3 as the verbosity value.

Just a thought,
  Stephan

PS: Your patch looks fine to me.

-- 
Stephan Beyer [off-list ref], PGP 0x6EDDD207FCC5040F

Re: [PATCH] builtin-revert.c: Make use of merge_recursive()

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:45:08

Hi,

On Mon, 11 Aug 2008, Stephan Beyer wrote:

quoted hunk

diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c
index 09aa830..d8bd21f 100644
--- a/builtin-merge-recursive.c
+++ b/builtin-merge-recursive.c

@@ -1327,7 +1327,7 @@ static const char *better_branch_name(const char *branch)
 	return name ? name : branch;
 }
 
-static struct commit *get_ref(const char *ref)
+struct commit *get_ref(const char *ref)

The name get_ref() is way too generic to be non-static.  But I have a 
hunch that peel_to_type() does a lot of what we want here, if not all of 
it.

quoted hunk

diff --git a/builtin-revert.c b/builtin-revert.c
index 27881e9..c54cf8a 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c

@@ -219,15 +216,21 @@ static int merge_recursive(const char *base_sha1,
 	 * and $prev on top of us (when reverting), or the change between
 	 * $prev and $commit on top of us (when cherry-picking or replaying).
 	 */
-	argv[i++] = "merge-recursive";
-	if (base_sha1)
-		argv[i++] = base_sha1;
-	argv[i++] = "--";
-	argv[i++] = head_sha1;
-	argv[i++] = next_sha1;
-	argv[i++] = NULL;
-
-	return run_command_v_opt(argv, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD);
+	if (base_sha1) {
+		struct commit *base = get_ref(base_sha1);
+		commit_list_insert(base, &ca);
+	}
+	h1 = get_ref(head_sha1);
+	h2 = get_ref(next_sha1);
+
+	index_fd = hold_locked_index(lock, 1);
+	clean = merge_recursive(h1, h2, head_name, next_name, ca, &result);

h1 and h2 are not expressive.  head_commit and next_commit would be.

Rest looks good to me -- even if I had to spend too much time (therefore 
being not really thorough in the end) verifying that merge_recursive() 
does not lock the index itself, and that GITHEAD_* definitions are not 
necessary anymore, since merge_recursive() takes the arguments directly; 
you might want to make it easier for this reviewer in the future, if you 
want this reviewer to review your patches, that is.

Ciao,
Dscho

Re: [PATCH 1/2] merge-recursive: prepare merge_recursive() to be called from builtins

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:08

On Mon, Aug 11, 2008 at 05:13:03PM +0200, Stephan Beyer [off-list ref] wrote:

Hmm, I have the long-run vision that we have a nice libgit some day,

That would be nice, but in that case I would start avoiding die() which
is an awful amount of work... (There were multiple threads on the list
previously.)

with merge_recursive() being part of it.  And I'm a little unsure if
libified functions should rely on environment variables.

Well, many libs do this. Random example: ld.so relies on LD_LIBRARY_PATH
as well.

Anyway sure, using function parameters instead of env vars is more
elegant.

Re: [PATCH 2/2] builtin-merge: avoid run_command_v_opt() for recursive

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:45:08

Miklos Vajna [off-list ref] writes:

The try_merge_strategy() function always ran the strategy in a separate
process, though this is not always necessary. The recursive strategy can
be called without a fork(). This patch adds a check, and calls recursive
in the same process without wasting resources.

Yes, it saves a fork, but is this really worth it in the bigger picture?

Doesn't the current code structure have benefit of allowing git-merge
itself do necessary clean-up action when merge-recursive calls any of the
die() it has in many places?

Re: [PATCH] builtin-revert.c: Make use of merge_recursive()

From: Stephan Beyer <hidden>
Date: 2016-06-15 22:45:08

Hi,

Johannes Schindelin wrote:

Hi,

On Mon, 11 Aug 2008, Stephan Beyer wrote:

quoted

diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c
index 09aa830..d8bd21f 100644
--- a/builtin-merge-recursive.c
+++ b/builtin-merge-recursive.c

@@ -1327,7 +1327,7 @@ static const char *better_branch_name(const char *branch)
 	return name ? name : branch;
 }
 
-static struct commit *get_ref(const char *ref)
+struct commit *get_ref(const char *ref)

The name get_ref() is way too generic to be non-static.

That's right.

But I have a hunch that peel_to_type() does a lot of what we want here,
if not all of it.

get_ref() has a big advantage over peel_to_type(): it can handle trees,
via "virtual commits" (make_virtual_commit()).
If you wonder where we need to handle trees on cherry-pick/revert:
With the -n (no commit) option you are allowed to have a dirty index.
So the recursive merge is not done using the HEAD commit but using the
uncommitted tree of the index.

Well, a good alternative could be to just make the really cool
make_virtual_commit() function non-static.
The name could be generic enough. Is it? :-)
Or perhaps: make_virtual_commit_from_tree().

Btw I also need get_ref() (or make_virtual_commit()) for threeway
fallback of the sequencer "patch -3" instruction ("git am -3"). ;)

quoted

+	h1 = get_ref(head_sha1);
+	h2 = get_ref(next_sha1);
+
+	index_fd = hold_locked_index(lock, 1);
+	clean = merge_recursive(h1, h2, head_name, next_name, ca, &result);

h1 and h2 are not expressive.  head_commit and next_commit would be.

This is also quite true.
Those names, also "ca", were taken from cmd_merge_recursive().
(This is no excuse, just an explanation.)

Rest looks good to me -- even if I had to spend too much time (therefore 
being not really thorough in the end) verifying that merge_recursive() 
does not lock the index itself,

I can't help here.  Miklos has the same change in patch v2/2 and I
wonder if you really expect that I don't test my patches, because
a double lock wouldn't have worked.

and that GITHEAD_* definitions are not necessary anymore, since merge_recursive()
takes the arguments directly;

Ok, I hoped that would've been clear by using head_name/next_name
directly in the merge_recursive() arguments, but nevertheless
thanks for your comment, ...because: using get_ref() the GITHEAD_*
definitions *are* still needed, because it takes the GITHEAD_*
name for the virtual commits...

Under this additional circumstance, I really tend to make
make_virtual_commit() non-static.

Kind regards,
  Stephan

-- 
Stephan Beyer [off-list ref], PGP 0x6EDDD207FCC5040F

Re: [PATCH 2/2] builtin-merge: avoid run_command_v_opt() for recursive

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:08

On Mon, Aug 11, 2008 at 11:47:07AM -0700, Junio C Hamano [off-list ref] wrote:

Yes, it saves a fork, but is this really worth it in the bigger picture?

Doesn't the current code structure have benefit of allowing git-merge
itself do necessary clean-up action when merge-recursive calls any of the
die() it has in many places?

As far as I see in most cases merge-recursive does not call die().

Cases when it does are like:

- broken snprintf
- cache_tree_fully_valid() or cache_tree_update() fails
- diff_setup_done() fails
- flush_buffer() fails
- read_sha1_file() fails
- ll_merge() fails

etc.

In short, I think there are two cases when a die() would be problematic
inside merge-recursive when calling it from builtin-merge:

- merge-recursive can't handle a merge, but an other strategy could do.
  This is the case when doing an octopus merge but in that case
  merge_recursive() is not called at all.

- merge-recursive results in conflicts, but an other strategy could
  handle the merge without conflicts. In this case die() isn't used
  either, so this will not be a problem.

So I don't think there is a case when a die() inside merge-recursive
would occur, but an other strategy would handle the merge properly.

Unless I missed something. ;-)

Re: [PATCH] builtin-revert.c: Make use of merge_recursive()

From: Miklos Vajna <hidden>
Date: 2016-06-15 22:45:08

On Mon, Aug 11, 2008 at 09:01:23PM +0200, Stephan Beyer [off-list ref] wrote:

Well, a good alternative could be to just make the really cool
make_virtual_commit() function non-static.
The name could be generic enough. Is it? :-)
Or perhaps: make_virtual_commit_from_tree().

Given that you can't make virtual commits from commits, tags or blobs, I
think the name "as is" generic enough.

[PATCH] builtin-revert: Make use of merge_recursive()

From: Stephan Beyer <hidden>
Date: 2016-06-15 22:45:08

Cherry-pick and revert always ran the merging in a separate process.
This patch makes cherry-pick/revert call merge_recursive() instead
of running git-merge-recursive.

To be able to cherry-pick/revert -n (without committing) on a dirty
index, make_virtual_commit() is needed and thus declared non-static.

Also the GITHEAD_* environment definitions are not needed anymore,
since the names are direct arguments to make_virtual_commit() and
merge_recursive().

Signed-off-by: Stephan Beyer <redacted>
---
	Hi,
	so I give it a new try.

 builtin-merge-recursive.c |    2 +-
 builtin-revert.c          |   56 +++++++++++++++++++++++++-------------------
 merge-recursive.h         |    2 +
 3 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/builtin-merge-recursive.c b/builtin-merge-recursive.c
index 09aa830..395bdf8 100644
--- a/builtin-merge-recursive.c
+++ b/builtin-merge-recursive.c

@@ -42,7 +42,7 @@ static struct tree *shift_tree_object(struct tree *one, struct tree *two)
  * - *(int *)commit->object.sha1 set to the virtual id.
  */
 
-static struct commit *make_virtual_commit(struct tree *tree, const char *comment)
+struct commit *make_virtual_commit(struct tree *tree, const char *comment)
 {
 	struct commit *commit = xcalloc(1, sizeof(struct commit));
 	static unsigned virtual_id = 1;

diff --git a/builtin-revert.c b/builtin-revert.c
index 27881e9..dcee181 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c

@@ -11,6 +11,7 @@
 #include "cache-tree.h"
 #include "diff.h"
 #include "revision.h"
+#include "merge-recursive.h"
 
 /*
  * This implements the builtins revert and cherry-pick.

@@ -200,18 +201,27 @@ static void set_author_ident_env(const char *message)
 			sha1_to_hex(commit->object.sha1));
 }
 
-static int merge_recursive(const char *base_sha1,
-		const char *head_sha1, const char *head_name,
-		const char *next_sha1, const char *next_name)
+static int merge_recursive_helper(const unsigned char *base_sha1,
+		const unsigned char *head_sha1, const char *head_name,
+		const unsigned char *next_sha1, const char *next_name)
 {
-	char buffer[256];
-	const char *argv[6];
-	int i = 0;
-
-	sprintf(buffer, "GITHEAD_%s", head_sha1);
-	setenv(buffer, head_name, 1);
-	sprintf(buffer, "GITHEAD_%s", next_sha1);
-	setenv(buffer, next_name, 1);
+	int clean, index_fd;
+	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
+	struct commit *result;
+	struct commit *head_commit, *next_commit;
+	struct object *head_object = parse_object(head_sha1);
+	struct commit_list *ca = NULL;
+
+	if (base_sha1) {
+		struct commit *base = lookup_commit_reference(base_sha1);
+		commit_list_insert(base, &ca);
+	}
+	if (head_object->type == OBJ_TREE)
+		head_commit = make_virtual_commit((struct tree *)head_object,
+						  head_name);
+	else
+		head_commit = (struct commit *)head_object;
+	next_commit = lookup_commit_reference(next_sha1);
 
 	/*
 	 * This three way merge is an interesting one.  We are at

@@ -219,15 +229,15 @@ static int merge_recursive(const char *base_sha1,
 	 * and $prev on top of us (when reverting), or the change between
 	 * $prev and $commit on top of us (when cherry-picking or replaying).
 	 */
-	argv[i++] = "merge-recursive";
-	if (base_sha1)
-		argv[i++] = base_sha1;
-	argv[i++] = "--";
-	argv[i++] = head_sha1;
-	argv[i++] = next_sha1;
-	argv[i++] = NULL;
-
-	return run_command_v_opt(argv, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD);
+	index_fd = hold_locked_index(lock, 1);
+	clean = merge_recursive(head_commit, next_commit,
+				head_name, next_name, ca, &result);
+	if (active_cache_changed &&
+			(write_cache(index_fd, active_cache, active_nr) ||
+			 commit_locked_index(lock)))
+		die("Unable to write index.");
+
+	return clean ? 0 : 1;
 }
 
 static char *help_msg(const unsigned char *sha1)

@@ -373,10 +383,8 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 		}
 	}
 
-	if (merge_recursive(base == NULL ?
-				NULL : sha1_to_hex(base->object.sha1),
-				sha1_to_hex(head), "HEAD",
-				sha1_to_hex(next->object.sha1), oneline) ||
+	if (merge_recursive_helper(base == NULL ? NULL : base->object.sha1,
+				   head, "HEAD", next->object.sha1, oneline) ||
 			write_cache_as_tree(head, 0, NULL)) {
 		add_to_msg("\nConflicts:\n\n");
 		read_cache();

diff --git a/merge-recursive.h b/merge-recursive.h
index f37630a..a9eead3 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h

@@ -1,6 +1,8 @@
 #ifndef MERGE_RECURSIVE_H
 #define MERGE_RECURSIVE_H
 
+extern struct commit *make_virtual_commit(struct tree *tree,
+					  const char *comment);
 int merge_recursive(struct commit *h1,
 		    struct commit *h2,
 		    const char *branch1,

-- 
1.6.0.rc2.267.g02e66a

Re: [PATCH] builtin-revert: Make use of merge_recursive()

From: Stephan Beyer <hidden>
Date: 2016-06-15 22:45:08

Sorry, I forgot to change this to [PATCH v2] or something.

And...

Stephan Beyer wrote:

quoted hunk

diff --git a/merge-recursive.h b/merge-recursive.h
index f37630a..a9eead3 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h

@@ -1,6 +1,8 @@
 #ifndef MERGE_RECURSIVE_H
 #define MERGE_RECURSIVE_H
 
+extern struct commit *make_virtual_commit(struct tree *tree,
+					  const char *comment);
 int merge_recursive(struct commit *h1,
 		    struct commit *h2,
 		    const char *branch1,

Is this a mistake that some forward declarations in header files are not
declared "extern"?

Regards,
  Stephan

-- 
Stephan Beyer [off-list ref], PGP 0x6EDDD207FCC5040F

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help