Re: [PATCH v2 1/3] rebase: learn to rebase root commit

[Junio C Hamano <gitster@pobox.com>]

Thomas Rast <trast@student.ethz.ch> writes:

> Teach git-rebase a new option --root, which instructs it to rebase the
> entire history leading up to <branch>.
>
> The main use-case is with git-svn: suppose you start hacking (perhaps
> offline) on a new project, but later notice you want to commit this
> work to SVN.  You will have to rebase the entire history, including
> the root commit, on a (possibly empty) commit coming from git-svn, to
> establish a history connection.  This previously had to be done by
> cherry-picking the root commit manually.

I like what this series tries to do.  Using the --root option is probably
a more natural way to do what people often do with the "add graft and
filter-branch the whole history once" procedure.

But it somewhat feels sad if the "main" use-case for this is to start your
project in git and then migrate away by feeding your history to subversion
;-).

> @@ -344,17 +348,23 @@ case "$diff" in
>  	;;
>  esac
>  
> +if test -z "$rebase_root"; then
>  # The upstream head must be given.  Make sure it is valid.
> -upstream_name="$1"
> -upstream=`git rev-parse --verify "${upstream_name}^0"` ||
> -    die "invalid upstream $upstream_name"
> +	upstream_name="$1"
> +	shift
> +	upstream=`git rev-parse --verify "${upstream_name}^0"` ||
> +	die "invalid upstream $upstream_name"
> +fi
> +
> +test ! -z "$rebase_root" -a -z "$newbase" &&
> +	die "--root must be used with --onto"

This is much easier to read if it were:

	if test -z "$rebase_root"
        then
        	... do the upstream_name/upstream thing, such as
                upstream_name="$1"
                ...
	else
        	... do the rebase_root thing, including
                unset upstream
                unset upstream_name
                test -z "$newbase" && die "--root without --onto"
                ...
	fi

(Mental note.  You shifted positional parameters and the remainders need
to be adjusted, which you seem to have done).

>  # Make sure the branch to rebase onto is valid.
>  onto_name=${newbase-"$upstream_name"}
>  onto=$(git rev-parse --verify "${onto_name}^0") || exit
>  
>  # If a hook exists, give it a chance to interrupt
> -run_pre_rebase_hook ${1+"$@"}
> +run_pre_rebase_hook ${upstream_name+"$upstream_name"} "$@"

I do not think ${upstream_name+"$upstream_name"} is a good check to begin
with, because presense of it does not necessarily mean the command was
invoked without --root; it could have come from the environment of the
invoker (hence the suggestion to unset the variable explicitly).

And I do not think this is a good way to extend the calling convention of
the hook, either.  pre-rebase-hook used to always take upstream and
optionally the explicit branch name.  When --root is given, your code will
give the hook a single parameter which is the explicit branch name
(i.e. "we will switch to this branch and rebase it, are you Ok with it?"),
but the hook will mistakenly think you are feeding the fork-point commit.

Because an old pre-rebase-hook cannot verify --root case correctly anyway
and needs to be updated, how about doing 'run_pre_rebase_hook --root "$@"'
when --root was given?

> @@ -393,7 +403,8 @@ case "$#" in
>  esac
>  orig_head=$branch
>  
> -# Now we are rebasing commits $upstream..$branch on top of $onto
> +# Now we are rebasing commits $upstream..$branch (or simply $branch
> +# with --root) on top of $onto

"or simply everything leading to $branch if --root is given"?

>  # Check if we are already based on $onto with linear history,
>  # but this should be done only when upstream and onto are the same.
> @@ -429,10 +440,18 @@ then
>  	exit 0
>  fi
>  
> +if test ! -z "$rebase_root"; then
> +	revisions="$orig_head"
> +	fp_flag="--root"
> +else
> +	revisions="$upstream..$orig_head"
> +	fp_flag="--ignore-if-in-upstream"
> +fi

Hmph, the reason why --ignore-if-in-upstream is irrelevant when --root is
given is because...?

> diff --git a/t/t3412-rebase-root.sh b/t/t3412-rebase-root.sh
> new file mode 100755
> index 0000000..63ec5e6
> --- /dev/null
> +++ b/t/t3412-rebase-root.sh
> @@ -0,0 +1,52 @@
> +#!/bin/sh
> +
> +test_description='git rebase --root
> ...
> +test_done

Including tests for the pre-rebase-hook would be nice.