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

[Jeff King <peff@peff.net>]

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