From: Christian Couder <christian.couder@gmail.com>
Date: Fri, 31 Dec 2010 05:41:46 GMT
Subject: Re: [PATCH 31/31] rebase -i: remove unnecessary state rebase-root
Message-ID: <AANLkTikeUg-9Qik=Xuxo-dM08dg9=aa-mP7aJn8Yknht@mail.gmail.com>
In-Reply-To: <alpine.DEB.1.10.1012292325220.7175@debian>

Hi,

On Wed, Dec 29, 2010 at 11:31 PM, Martin von Zweigbergk
<martin.von.zweigbergk@gmail.com> wrote:
> On Tue, 28 Dec 2010, Thomas Rast wrote:
>
>> Martin von Zweigbergk wrote:
>> > @@ -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 "$@"
>> >  }
>> [...]
>> > 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.