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

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

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

On Wed, Nov 28, 2012 at 12:28:58AM +0100, Heiko Voigt wrote:
> On Tue, Nov 27, 2012 at 02:01:05PM -0500, W. Trevor King wrote:
> > On Tue, Nov 27, 2012 at 07:31:25PM +0100, Heiko Voigt wrote:
> > 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.
> 
> But IMO thats the functionality which should be implemented in submodule
> update and not left to the user.

Then you might need submodule.<name>.local-branch,
submodule.<name>.remote-repository, and submodule.<name>.remote-branch
to configure

  $ git checkout submodule.<name>.local-branch
  $ git pull submodule.<name>.remote-repository submodule.<name>.remote-branch

and this would ignore the $sha1 stored in the gitlink (which all of
the other update commands use).  This ignoring-the-$sha1 bit made me
think that a built-in pull wasn't a good fit for 'submodule update'.
Maybe if it went into a new 'submodule pull'?  Then users have a clear
distinction:

* 'update' to push superproject $sha1 changes into the submodules
* 'pull' to push upstream-branch changes into the submodules

> > > 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?
> 
> Well the main difference is that gerrit is automatically updating the
> superproject AFAIK. I would like it if we could implement the same
> workflow support in the submodule script. It seems to me that this is
> already proven to be useful workflow.

Ah, sorry, I meant the configuring which remote branch you were
pulling from happens at submodule initialization (via .git/modules/…)
for both your workflow and my v4.

You're right that having a builtin pull is different from my v4.

> https://github.com/hvoigt/git/commits/hv/floating_submodules_draft

I looked over this before, but maybe not thoroughly enough ;).

> > > 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.
> 
> Could you point me to that mail? I cannot seem to find it in my archive.

Hmm.  It seems like Phil's initial response was (accidentally?) off
list.  The relevant portion was:

On Mon, Oct 22, 2012 at 06:03:53PM -0400, Phil Hord wrote:
> Some projects now use the 'branch' config value to record the tracking
> branch for the submodule.  Some ascribe different meaning to the
> configuration if the value is given vs. undefined.  For example, see
> the Gerrit submodule-subscription mechanism.  This change will cause
> those workflows to behave differently than they do now.
>
> I do like the idea, but I wish it had a different name for the
> recording.  Maybe --record-branch=${BRANCH} as an extra switch so the
> action is explicitly requested.

As I said, I'm happy to go back to --branch if opinions have changed.

On Wed, Nov 28, 2012 at 12:28:58AM +0100, Heiko Voigt wrote:
> On Tue, Nov 27, 2012 at 02:01:05PM -0500, W. Trevor King wrote:
> > On Tue, Nov 27, 2012 at 07:31:25PM +0100, Heiko Voigt wrote:
> > > > 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.
> 
> See my draft or the diff below for an illustration of the splitup.
> 
> [snip diff]

This looks fine, but my current --branch implementation (which doesn't
pull) is only a thin branch-checkout layer on top of the standard
`update` functionality.  I'm still unsure if built-in pulls are worth
the configuration trouble.  I'll sleep on it.  Maybe I'll feel better
about them tomorrow ;).

Cheers,
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 --]