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

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

[Sebastian Schuberth]

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

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

[Jeff King]

On Thu, May 12, 2011 at 12:15:24PM +0200, Sebastian Schuberth wrote:

> 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

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

[Jeff King]

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

-Peff

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

[Jeff King]

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

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

[Sebastian Schuberth]

On Thu, May 12, 2011 at 13:09, Jeff King <peff@peff.net> wrote:

> 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

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

[Jeff King]

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

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

[Sebastian Schuberth]

On Thu, May 12, 2011 at 13:09, Jeff King <peff@peff.net> wrote:

> 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

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

[Jeff King]

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

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

[Johannes Sixt]

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)"
 
 '
 

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

[Jeff King]

On Thu, May 12, 2011 at 02:20:23PM +0200, Johannes Sixt wrote:

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

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

[Jeff King]

On Thu, May 12, 2011 at 07:08:55AM -0400, Jeff King wrote:

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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