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

[Heiko Voigt <hvoigt@hvoigt.net>]

Hi,

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?

Doing the above does not assist you much in that does it?

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?

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

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

Maybe we need an else block here and possibly extract the current code
inside the if statement into a function. E.g. that the final code looks
something like this:

	if test "$subsha1" != "$sha1"
	then
		handle_on_demand_fetch_update ...
	else
		handle_tracked_branch_update ...
	fi

Not sure about the function names though. If we decide to go that route:
The extraction into a function should go in an extra preparation patch
which does not change any functionality.

I will reply to the patches for further comments.

Cheers Heiko