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

[Martin von Zweigbergk <martin.von.zweigbergk@gmail.com>]

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?

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


/Martin