threads / discuss / 27335

git cherry-pick --strategy=resolve segfaults if picking a root commit

Subject: git cherry-pick --strategy=resolve segfaults if picking a root commit

## tl;dr

12 messages between May 12, 2011 and May 16, 2011.

replies: 11people: 4as markdown or json

Sebastian Schuberth· May 12, 2011, 10:15 UTC · lore
Hi,

I just noticed that (under both Linux and Windows) Git 1.7.5 segfaults for me if cherry-picking a root commit from one repo into another one with "--strategy=resolve". It does not segfault if "--strategy=resolve" is omitted. Here's what I did:

# Create a repository with only a root commit. mkdir repo1 cd repo1 git init

echo "test1" > test1 git add test1 git commit -m "test1"

# Create another repository with only a root commit. mkdir ../repo2 cd ../repo2 git init

echo "test2" > test2 git add test2 git commit -m "test2"

git remote add repo1 ../repo1 git fetch repo1

# Cherry-pick with "--strategy=resolve" -> segfaults. git cherry-pick --strategy=resolve repo1/master

Is this already a known issue?
-- 
Sebastian Schuberth
Jeff King· May 12, 2011, 10:45 UTC · re: Sebastian Schuberth · lore

Re: git cherry-pick --strategy=resolve segfaults if picking a root commit

On Thu, May 12, 2011 at 12:15:24PM +0200, Sebastian Schuberth wrote:
Show 6 quoted lines
> I just noticed that (under both Linux and Windows) Git 1.7.5
> segfaults for me if cherry-picking a root commit from one repo into
> another one with "--strategy=resolve". It does not segfault if
> "--strategy=resolve" is omitted. Here's what I did:
> [...]
> Is this already a known issue?

No, I think you found a new one (I doubt many people use alternate strategies for cherry-picking, and probably even less frequently on root commits).

Here's a fix, but see below.
---
 builtin/revert.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index f697e66..2eebb58 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -497,7 +497,8 @@ static int do_pick_commit(void)
 
 		write_message(&msgbuf, defmsg);
 
-		commit_list_insert(base, &common);
+		if (base)
+			commit_list_insert(base, &common);
 		commit_list_insert(next, &remotes);
 		res = try_merge_command(strategy, xopts_nr, xopts, common,
 					sha1_to_hex(head), remotes);

The base commit comes from the "parent" pointer in a cherry, which is
going to be NULL in this case. In a revert, however, the parent pointer
ends up in "next". However, you can't get a segfault with "git revert
--strategy=resolve $some_root_commit", because earlier we explicitly
disallow reverting a root commit.

So this patch makes it stop segfaulting, but I'm not sure that it is
doing anything useful. Merge-resolve seems to just barf, whereas
merge-recursive properly handles this case (it cherry-picks the
difference between the empty tree and the root commit's tree).

So probably we should:

  1. Pass the empty tree along to merge-resolve. This will take a little
     bit of refactoring, but more importantly, it means we will be
     passing a tree-ish and not a commit-ish to a merge strategy. Is
     that OK?

  2. Consider lifting the restriction on reverting root commits. If we
     can cherry-pick it, we can revert it, so I suspect this would
     already work with merge-recursive, but I didn't try. I don't care
     too much either way, though; I doubt it's something people would do
     a lot. It just seems like an unnecessary restriction.

-Peff
Jeff King· May 12, 2011, 11:08 UTC · re: Jeff King · lore

Re: git cherry-pick --strategy=resolve segfaults if picking a root commit

On Thu, May 12, 2011 at 06:45:58AM -0400, Jeff King wrote:
Show 12 quoted lines
> So probably we should:
> 
>   1. Pass the empty tree along to merge-resolve. This will take a little
>      bit of refactoring, but more importantly, it means we will be
>      passing a tree-ish and not a commit-ish to a merge strategy. Is
>      that OK?
> 
>   2. Consider lifting the restriction on reverting root commits. If we
>      can cherry-pick it, we can revert it, so I suspect this would
>      already work with merge-recursive, but I didn't try. I don't care
>      too much either way, though; I doubt it's something people would do
>      a lot. It just seems like an unnecessary restriction.

This turned out to be quite easy. git-merge-resolve handles the tree-ish argument just fine. But it's possible other merge helpers might not be so happy. I dunno.

The series is:
  [1/3]: cherry-pick: handle root commits with external strategies
  [2/3]: revert: allow reverting a root commit
  [3/3]: t3503: test cherry picking and reverting root commits
-Peff
Jeff King· May 12, 2011, 11:09 UTC · re: Jeff King · lore

[PATCH 1/3] cherry-pick: handle root commits with external strategies

The merge-recursive strategy already handles root commits; it cherry-picks the difference between the empty tree and the root commit's tree.

However, for external strategies, we dereference NULL and segfault while building the argument list. Instead, let's handle this by passing the empty tree sha1 to the merge script.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/merge.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 9661c8f..5098bf6 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -599,6 +599,14 @@ static void write_tree_trivial(unsigned char *sha1)
 		die(_("git write-tree failed to write a tree"));
 }
 
+static const char *merge_argument(struct commit *commit)
+{
+	if (commit)
+		return sha1_to_hex(commit->object.sha1);
+	else
+		return EMPTY_TREE_SHA1_HEX;
+}
+
 int try_merge_command(const char *strategy, size_t xopts_nr,
 		      const char **xopts, struct commit_list *common,
 		      const char *head_arg, struct commit_list *remotes)
@@ -619,11 +627,11 @@ int try_merge_command(const char *strategy, size_t xopts_nr,
 		args[i++] = s;
 	}
 	for (j = common; j; j = j->next)
-		args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));
+		args[i++] = xstrdup(merge_argument(j->item));
 	args[i++] = "--";
 	args[i++] = head_arg;
 	for (j = remotes; j; j = j->next)
-		args[i++] = xstrdup(sha1_to_hex(j->item->object.sha1));
+		args[i++] = xstrdup(merge_argument(j->item));
 	args[i] = NULL;
 	ret = run_command_v_opt(args, RUN_GIT_CMD);
 	strbuf_release(&buf);

-- 
1.7.5.1.12.ga7abed
Sebastian Schuberth· May 12, 2011, 14:50 UTC · re: Jeff King · lore

Re: [PATCH 1/3] cherry-pick: handle root commits with external strategies

On Thu, May 12, 2011 at 13:09, Jeff King <peff@peff.net> wrote:
Show 10 quoted lines
> The merge-recursive strategy already handles root commits;
> it cherry-picks the difference between the empty tree and
> the root commit's tree.
>
> However, for external strategies, we dereference NULL and
> segfault while building the argument list. Instead, let's
> handle this by passing the empty tree sha1 to the merge
> script.
>
> Signed-off-by: Jeff King <peff@peff.net>
Works perfectly for me, thanks a lot for coming up with a patch incredibly fast!
-- 
Sebastian Schuberth
Jeff King· May 12, 2011, 11:09 UTC · re: Jeff King · lore

[PATCH 2/3] revert: allow reverting a root commit

Although it is probably an uncommon operation, there is no reason to disallow it, as it works just fine. It is the reverse of a cherry-pick of a root commit, which is already allowed.

We do have to tweak one check on whether we have a merge commit, which assumed we had at least one parent.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/revert.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index f697e66..1f27c63 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -408,8 +408,6 @@ static int do_pick_commit(void)
 	discard_cache();
 
 	if (!commit->parents) {
-		if (action == REVERT)
-			die (_("Cannot revert a root commit"));
 		parent = NULL;
 	}
 	else if (commit->parents->next) {
@@ -467,7 +465,7 @@ static int do_pick_commit(void)
 		strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit ");
 		strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 
-		if (commit->parents->next) {
+		if (commit->parents && commit->parents->next) {
 			strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
 			strbuf_addstr(&msgbuf, sha1_to_hex(parent->object.sha1));
 		}

-- 
1.7.5.1.12.ga7abed
Sebastian Schuberth· May 12, 2011, 14:53 UTC · re: Jeff King · lore

Re: [PATCH 2/3] revert: allow reverting a root commit

On Thu, May 12, 2011 at 13:09, Jeff King <peff@peff.net> wrote:
Show 9 quoted lines
> Although it is probably an uncommon operation, there is no
> reason to disallow it, as it works just fine. It is the
> reverse of a cherry-pick of a root commit, which is already
> allowed.
>
> We do have to tweak one check on whether we have a merge
> commit, which assumed we had at least one parent.
>
> Signed-off-by: Jeff King <peff@peff.net>

I've quickly tested this one, too, and it also seems to work nicely. Great stuff!

-- 
Sebastian Schuberth
Jeff King· May 12, 2011, 11:10 UTC · re: Jeff King · lore

[PATCH 3/3] t3503: test cherry picking and reverting root commits

We already tested cherry-picking a root commit, but only with the internal merge-recursive strategy. Let's also test the recently-allowed reverting of a root commit, as well as testing with external strategies (which until recently triggered a segfault).

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t3503-cherry-pick-root.sh |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)
diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
index b0faa29..1f9ed67 100755
--- a/t/t3503-cherry-pick-root.sh
+++ b/t/t3503-cherry-pick-root.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='test cherry-picking a root commit'
+test_description='test cherry-picking (and reverting) a root commit'
 
 . ./test-lib.sh
 
@@ -27,4 +27,25 @@ test_expect_success 'cherry-pick a root commit' '
 
 '
 
+test_expect_success 'revert a root commit' '
+
+	git revert master &&
+	! test -f file1
+
+'
+
+test_expect_success 'cherry-pick a root commit with an external strategy' '
+
+	git cherry-pick --strategy=resolve master &&
+	test first = $(cat file1)
+
+'
+
+test_expect_success 'revert a root commit with an external strategy' '
+
+	git revert --strategy=resolve master &&
+	! test -f file1
+
+'
+
 test_done

-- 
1.7.5.1.12.ga7abed
Johannes Sixt· May 12, 2011, 12:20 UTC · re: Jeff King · lore

Re: [PATCH 3/3] t3503: test cherry picking and reverting root commits

Am 5/12/2011 13:10, schrieb Jeff King:
> +test_expect_success 'cherry-pick a root commit with an external strategy' '
> +
> +	git cherry-pick --strategy=resolve master &&
> +	test first = $(cat file1)

What if file1 does not exist? Then cat fails loudly. But this does not fail the entire command immediately; rather, the test command fails, but not because of a non-equality, but because of an invalid usage ("syntax error"). IOW, the test does the right thing, but for the wrong reason.

Yes, an earlier test gave a bad precedent, and the following fixup (to be squashed in) fixes it, too.

-- Hannes
diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
index 1f9ed67..3006452 100755
--- a/t/t3503-cherry-pick-root.sh
+++ b/t/t3503-cherry-pick-root.sh
@@ -23,7 +23,7 @@ test_expect_success setup '
 test_expect_success 'cherry-pick a root commit' '
 
 	git cherry-pick master &&
-	test first = $(cat file1)
+	test first = "$(cat file1)"
 
 '
 
@@ -37,7 +37,7 @@ test_expect_success 'revert a root commit' '
 test_expect_success 'cherry-pick a root commit with an external strategy' '
 
 	git cherry-pick --strategy=resolve master &&
-	test first = $(cat file1)
+	test first = "$(cat file1)"
 
 '
Jeff King· May 12, 2011, 12:37 UTC · re: Johannes Sixt · lore

Re: [PATCH 3/3] t3503: test cherry picking and reverting root commits

On Thu, May 12, 2011 at 02:20:23PM +0200, Johannes Sixt wrote:
Show 13 quoted lines
> Am 5/12/2011 13:10, schrieb Jeff King:
> > +test_expect_success 'cherry-pick a root commit with an external strategy' '
> > +
> > +	git cherry-pick --strategy=resolve master &&
> > +	test first = $(cat file1)
> 
> What if file1 does not exist? Then cat fails loudly. But this does not
> fail the entire command immediately; rather, the test command fails, but
> not because of a non-equality, but because of an invalid usage ("syntax
> error"). IOW, the test does the right thing, but for the wrong reason.
> 
> Yes, an earlier test gave a bad precedent, and the following fixup (to
> be squashed in) fixes it, too.

Yeah, sorry, I blindly copied the earlier test without thinking. If we are going to tweak, my preference is actually to use test_cmp. And while we're at it, I should be using test_path_is_missing.

So:
diff --git a/t/t3503-cherry-pick-root.sh b/t/t3503-cherry-pick-root.sh
index 1f9ed67..9aefe3a 100755
--- a/t/t3503-cherry-pick-root.sh
+++ b/t/t3503-cherry-pick-root.sh
@@ -23,28 +23,30 @@ test_expect_success setup '
 test_expect_success 'cherry-pick a root commit' '
 
 	git cherry-pick master &&
-	test first = $(cat file1)
+	echo first >expect &&
+	test_cmp expect file1
 
 '
 
 test_expect_success 'revert a root commit' '
 
 	git revert master &&
-	! test -f file1
+	test_path_is_missing file1
 
 '
 
 test_expect_success 'cherry-pick a root commit with an external strategy' '
 
 	git cherry-pick --strategy=resolve master &&
-	test first = $(cat file1)
+	echo first >expect &&
+	test_cmp expect file1
 
 '
 
 test_expect_success 'revert a root commit with an external strategy' '
 
 	git revert --strategy=resolve master &&
-	! test -f file1
+	test_path_is_missing file1
 
 '
Jeff King· May 16, 2011, 10:27 UTC · re: Jeff King · lore

Re: git cherry-pick --strategy=resolve segfaults if picking a root commit

On Thu, May 12, 2011 at 07:08:55AM -0400, Jeff King wrote:
Show 24 quoted lines
> On Thu, May 12, 2011 at 06:45:58AM -0400, Jeff King wrote:
> 
> > So probably we should:
> > 
> >   1. Pass the empty tree along to merge-resolve. This will take a little
> >      bit of refactoring, but more importantly, it means we will be
> >      passing a tree-ish and not a commit-ish to a merge strategy. Is
> >      that OK?
> > 
> >   2. Consider lifting the restriction on reverting root commits. If we
> >      can cherry-pick it, we can revert it, so I suspect this would
> >      already work with merge-recursive, but I didn't try. I don't care
> >      too much either way, though; I doubt it's something people would do
> >      a lot. It just seems like an unnecessary restriction.
> 
> This turned out to be quite easy. git-merge-resolve handles the tree-ish
> argument just fine. But it's possible other merge helpers might not be
> so happy. I dunno.
> 
> The series is:
> 
>   [1/3]: cherry-pick: handle root commits with external strategies
>   [2/3]: revert: allow reverting a root commit
>   [3/3]: t3503: test cherry picking and reverting root commits
Junio,

I seem to recall seeing an email from you saying that merge-helpers need to handle tree-ish arguments, so this is an OK direction to go. But now I can't seem to find it. Did I dream it?

If that is the case, then I think this series is worth picking up. So I thought I'd prod you on it (I'm happy to repost, too, if that's easier).

-Peff
Junio C Hamano· May 16, 2011, 15:01 UTC · re: Jeff King · lore

Re: git cherry-pick --strategy=resolve segfaults if picking a root commit

Jeff King <peff@peff.net> writes:
Show 13 quoted lines
> On Thu, May 12, 2011 at 07:08:55AM -0400, Jeff King wrote:
>> The series is:
>> 
>>   [1/3]: cherry-pick: handle root commits with external strategies
>>   [2/3]: revert: allow reverting a root commit
>>   [3/3]: t3503: test cherry picking and reverting root commits
>
> I seem to recall seeing an email from you saying that merge-helpers need
> to handle tree-ish arguments, so this is an OK direction to go. But now
> I can't seem to find it. Did I dream it?
>
> If that is the case, then I think this series is worth picking up. So I
> thought I'd prod you on it (I'm happy to repost, too, if that's easier).

No you did not dream it but I think it is worth picking up. It was in my "to look at" queue that was accidentally got lost.

Thanks for reminding.

← back to recent threads