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

From: Heiko Voigt <hvoigt@hvoigt.net>
Date: Nov 27, 2012, 18:31 UTC
Message-ID: <20121127183125.GA4185@book.hvoigt.net>
In-Reply-To: <cover.1353962698.git.wking@tremily.us>

Hi,

On Mon, Nov 26, 2012 at 04:00:15PM -0500, W. Trevor King wrote:

Show 16 quoted lines

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

Show 6 quoted lines

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

Show 13 quoted lines

> 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