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

[Thomas Rast <trast@student.ethz.ch>]

[-- Attachment #1: Type: text/plain, Size: 3524 bytes --]

Junio C Hamano wrote:
> 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 ;-)

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

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

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


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]