Re: [PATCH v4 0/4] git-submodule add: Add --local-branch option

["W. Trevor King" <wking@tremily.us>]

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

On Tue, Nov 27, 2012 at 07:31:25PM +0100, Heiko Voigt wrote:
> On Mon, Nov 26, 2012 at 04:00:15PM -0500, W. Trevor King wrote:
> > From: "W. Trevor King" <wking@tremily.us>
> > 
> > On Fri, Nov 23, 2012 at 12:54:02PM -0500, W. Trevor King wrote:
> > > We could add
> > >
> > >   $ git submodule update --branch
> > >
> > > to checkout the gitlinked SHA1 as submodule.<name>.branch in each of
> > > the submodules, leaving the submodules on the .gitmodules-configured
> > > branch.  Effectively (for each submodule):
> > >
> > >   $ git branch -f $branch $sha1
> > >   $ git checkout $branch
> > 
> > I haven't gotten any feedback on this as an idea, but perhaps someone
> > will comment on it as a patch series ;).
> 
> I am not sure I understand you correctly. You are suggesting that the
> branch option as an alias for the registered SHA1 in the superproject?
> 
> I though the goal of your series was that you want to track submodules
> branch which come from the remote side?

That's what I'd initially thought, but when I went to implement
`update --pull`, I realized that

  $ git submodule foreach 'git checkout $(git config --file $toplevel/.gitmodules submodule.$name.branch) && …'

is using submodule.<name>.branch as the local branch name.  The remote
branch name was actually setup in .git/modules/<name>/config during
the initial "clone -b <branch> …".

The v4 series leaves the remote branch amigious, but it helps you
point the local branch at the right hash so that future calls to

  $ git submodule foreach 'git pull'

can use the branch's .git/modules/<name>/config settings.

> I would think more of some convention like:
> 
> 	$ git checkout -t origin/$branch
> 
> when first initialising the submodule with e.g.
> 
> 	$ git submodule update --init --branch
> 
> Then later calls of
> 
> 	$ git submodule update --branch
> 
> would have a branch configured to pull from. I imagine that results in
> a similar behavior gerrit is doing on the server side?

That sounds like it's doing pretty much the same thing.  Can you think
of a test that would distinguish it from my current v4 implementation?

> > Changes since v3:
> > 
> > * --record=??? is now --local-branch=???
> > * Dropped patches 2 ($submodule_ export) and 3 (motivating documentation)
> > * Added local git-config overrides of .gitmodules' submodule.<name>.branch
> > * Added `submodule update --branch`
> 
> I would prefer if we could squash all these commits together into one
> since it seems to me one logical step, using the new variable for update
> belongs together with its configuration on initialization.
> 
> How about reusing the -b|--branch option for add? Since we only change
> the behavior when submodule.$name.update is set to branch it seems
> reasonable to me. Opinions?

That was the approach I used in v1, but people were concerned that we
would be stomping on previously unclaimed config space.  Since noone
has pointed out other uses besides Gerrit's very similar case, I'm not
sure if that is still an issue.

> > Because you need to recurse through submodules for `update --branch`
> > even if "$subsha1" == "$sha1", I had to amend the conditional
> > controlling that block.  This broke one of the existing tests, which I
> > "fixed" in patch 4.  I think a proper fix would involve rewriting
> > 
> >   (clear_local_git_env; cd "$sm_path" &&
> >    ( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&
> >     test -z "$rev") || git-fetch)) ||
> >   die "$(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
> > 
> > but I'm not familiar enough with rev-list to want to dig into that
> > yet.  If feedback for the earlier three patches is positive, I'll work
> > up a clean fix and resubmit.
> 
> You probably need to separate your handling here. The comparison of the
> currently checked out sha1 and the recorded sha1 is an optimization
> which skips unnecessary fetching in case the submodules commits are
> already correct. This code snippet checks whether the to be checked out
> sha1 is already local and also skips the fetch if it is. We should not
> break that.

Agreed.  However, determining if the target $sha1 is local should have
nothing to do with the current checked out $subsha1.

Thanks for the feedback!
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]