Re: [PATCH 31/31] rebase -i: remove unnecessary state rebase-root

Hi,

On Wed, Dec 29, 2010 at 11:31 PM, Martin von Zweigbergk
[off-list ref] wrote:

On Tue, 28 Dec 2010, Thomas Rast wrote:

quoted

Martin von Zweigbergk wrote:

quoted

@@ -168,11 +168,6 @@ pick_one () {

    output git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"
    test -d "$REWRITTEN" &&
            pick_one_preserving_merges "$@" && return
-   if test -n "$rebase_root"
-   then
-           output git cherry-pick "$@"
-           return
-   fi
    output git cherry-pick $ff "$@"
 }

[...]

quoted

While factoring out the state writing code a few patches back, I went
through each of the pieces of state that was written. I was a bit
hesitant to include this patch since I'm not quite sure why the code
was introduced, but I thought I would include it anyway to hear what
you have to say.

There used to be bug when using --ff when rebasing a root commit. This
was fixed in 6355e50 (builtin/revert.c: don't dereference a NULL
pointer, 2010-09-27). Could this have been the reason for the check?
Thomas, do you remember?

I think this just ended up being such a strange test because of the
following hunk in 8e75abf (rebase -i: use new --ff cherry-pick option,
2010-03-06):

@@ -232,16 +232,7 @@ pick_one () {

                output git cherry-pick "$@"
                return
        fi
-       parent_sha1=$(git rev-parse --verify $sha1^) ||
-               die "Could not get the parent of $sha1"
-       current_sha1=$(git rev-parse --verify HEAD)
-       if test -z "$no_ff" && test "$current_sha1" = "$parent_sha1"
-       then
-               output git reset --hard $sha1
-               output warn Fast-forward to $(git rev-parse --short $sha1)
-       else
-               output git cherry-pick "$@"
-       fi
+       output git cherry-pick $ff "$@"
 }
--

Yes, I saw that one as well, so it would probably have made more sense
to ask Christian instead (the author of 8e75abf). So do you remember,
Christian?

Yeah, I must say that I did not try to understand why there was a
special case for $rebase_root above the code I was changing.
Perhaps I should have, and I would probably have sent a patch like yours...

Anyway, thanks for your input, Thomas. That makes me feel a little
less worried about this patch.

Yeah I think it's a good one.

Thanks,
Christian.