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

From: Thomas Rast <trast@student.ethz.ch>
Date: Jan 2, 2009, 22:20 UTC
Message-ID: <200901022320.14055.trast@student.ethz.ch>
In-Reply-To: <7v4p0iivwh.fsf@gitster.siamese.dyndns.org>

Junio C Hamano wrote:

Show 6 quoted lines

> Thomas Rast <trast@student.ethz.ch> writes:
> > The main use-case is with git-svn: [...]
> 
> 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.

Their uses are somewhat disjoint, as grafting only works if the grafted parent has an empty tree. Otherwise the grafted child will appear to revert the entire history leading to it. Rebase OTOH changes the committers.

> 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
> ;-).

You can remove that paragraph if you don't want to "support" SVN in git.git ;-)

Show 12 quoted lines

> >  # 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).

Good catch, thanks.

I'm still not sure what ${1+"$@"} was about by the way. The most authoritative reference I can find is

  http://www.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_05_02

which says

  If there are no positional parameters, the expansion of '@' shall
  generate zero fields, even when '@' is double-quoted.

('man bash' agrees.)

Show 10 quoted lines

> 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?

True. I noticed that I had to fix the positionals, but forgot about the hook afterwards. v3 implements this as you suggested.

Show 10 quoted lines

> > +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...?

Well, originally because format-patch didn't like the argument. Thanks for prodding however, $onto..$head makes sort of makes sense here and I discovered that even $onto...$head works, which is used in 'rebase -i'.

However, it's still a change of semantics: With --root we now ignore patches that are already contained in $onto, as opposed to patches that were already in $upstream in normal operation. It seems sensible to do it this way, and perhaps even normal rebase should do it the same way... if only format-patch would support it.

Thanks for the review! v3 upcoming.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch