Re: [RFC/PATCH] git-submodule: add support for --rebase.

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Wed, 8 Apr 2009, Peter Hutterer wrote:

> On Tue, Apr 07, 2009 at 02:38:37PM +0200, Johannes Schindelin wrote:
>
> > Peter wrote originally:
> >
> > > diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> > > index 3b8df44..117ad3d 100644
> > > --- a/Documentation/git-submodule.txt
> > > +++ b/Documentation/git-submodule.txt
> > > @@ -177,6 +178,12 @@ OPTIONS
> > >  	This option is only valid for the update command.
> > >  	Don't fetch new objects from the remote site.
> > >  
> > > +--rebase::
> > > +	This option is only valid for the update command.
> > 
> > This is unnecessary, it was mentioned in the synopsis.
> 
> Correct, but in the same file the options --cached, --no-fetch, and 
> --summary-limit do state that they are only valid for the 
> update/status/summary commands, respectively. For consistency, we should 
> either add this sentence to --rebase, or remove them from the others. I 
> personally prefer having it there, just to make it more obvious.

Fair enough!

> > > +	Forward-port local commits to the index of the containing repository.
> > 
> > This is a bit misleading/unclear.  I'd rather have it read like this:
> > 
> > 	Instead of detaching the HEAD to the revision committed in the 
> > 	superproject, rebase the current branch onto that revision.
> 
> Hehe. I had something like this before and then decided to copy/paste 
> the line from the git-rebase man page :)
> 
> Changed to: "Rebase the current branch onto the index of the containing 
> repository instead of detaching the HEAD." (I vaguely remember the rule 
> that sentences are easier to understand if you have "blah instead of 
> foo" rather than "instead of foo, blah")

> The phrase "index of the containing repository" is commonly used in this 
> man page, so I think it's best to stick with it. That better now?

Hmm.

You can really rebase only onto a commit.  And the index is not a commit, 
so I do not like the wording (not even in the rebase man page).

But let's see what other reviewers say... :-)

> > > +	If a a merge failure prevents this process, you will have to resolve
> > > +	these failures with linkgit:git-rebase[1].
> > > +
> > >  <path>...::
> > >  	Paths to submodule(s). When specified this will restrict the command
> > >  	to only operate on the submodules found at the specified paths.
> > > diff --git a/git-submodule.sh b/git-submodule.sh
> > > index 7c2e060..6180bf4 100755
> > > --- a/git-submodule.sh
> > > +++ b/git-submodule.sh
> > 
> > You might want to error out when --rebase was passed with another 
> > command than "update".
> 
> cmd_init(), cmd_summary(), etc. have catch-all rules for unknown options 
> to display the usage, so this is covered. (e.g. line 439, 
> git-submodule.sh)

Oh, okay!

Other than the wording, I am completely happy with this patch.

Ciao,
Dscho