Re: Re: Possible regression in master? (submodules without a "master" branch)

From: Heiko Voigt <hvoigt@hvoigt.net>
Date: Mar 27, 2014, 20:27 UTC
Message-ID: <20140327202702.GA3984@sandbox-ub>
In-Reply-To: <xmqqwqffctyg.fsf@gitster.dls.corp.google.com>

On Thu, Mar 27, 2014 at 12:39:03PM -0700, Junio C Hamano wrote:

Show 36 quoted lines

> "W. Trevor King" <wking@tremily.us> writes:
> 
> > On Thu, Mar 27, 2014 at 06:31:27PM +0100, Jens Lehmann wrote:
> >> Am 27.03.2014 18:16, schrieb Junio C Hamano:
> >> > Johan Herland <johan@herland.net> writes:
> >> > 
> >> >> I just found a failure to checkout a project with submodules where
> >> >> there is no explicit submodule branch configuration, and the
> >> >> submodules happen to not have a "master" branch:
> >> >>
> >> >>   git clone git://gitorious.org/qt/qt5.git qt5
> >> >>   cd qt5
> >> >>   git submodule init qtbase
> >> >>   git submodule update
> >> >>
> >> >> In current master, the last command fails with the following output:
> >> > 
> >> > ... and with a bug-free system, what does it do instead?  Just clone
> >> > 'qtbase' and make a detached-head checkout at the commit recorded in
> >> > the superproject's tree, or something else?
> >> 
> >> After reverting 23d25e48f5ead73 on current master it clones 'qtbase'
> >> nicely with a detached HEAD.
> >
> > Fixing this for initial update clone is pretty easy, we just need to
> > unset start_point before calling module_clone if
> > submodule.<name>.branch is not set. 
> 
> There is this bit for "update" in git-submodule.txt:
> 
>   For updates that clone missing submodules, checkout-mode updates
>   will create submodules with detached HEADs; all other modes will
>   create submodules with a local branch named after
>   submodule.<path>.branch.
> 
>   [side note] Isn't that a typo of submodule.<name>.branch?

Yep, thats is a typo. Trevor will you fix that as well? Or how should be do that? Since its just such a small change.

Show 5 quoted lines

> So the proposed change is to make the part before semicolon true?
> If we are not newly cloning (because we already have it), if the
> submodule.<name>.branch is not set *OR* refers to a branch that does
> not even exist, shouldn't we either (1) abort as an error, or (2) do
> the same and detach?

I would expect "(1) abort as an error" since the user is not getting what he would expect.

Show 16 quoted lines

> > However, that's just going to
> > push remote branch ambiguity problems back to the --remote update
> > functionality.  What should happen when submodule.<name>.branch is not
> > set and you run a --remote update, which has used:
> >
> >     git rev-parse "${remote_name}/${branch}"
> >
> > since the submodule.<name>.branch setting was introduced in 06b1abb
> > (submodule update: add --remote for submodule's upstream changes,
> > 2012-12-19)?
> 
> Isn't --remote about following one specific branch the user who
> issues that command has in mind?  If you as the end user did not
> give any indication which branch you meant, e.g. by leaving the
> submodule.<name>.branch empty, shouldn't that be diagnosed as an
> error?

Well to simplify things there was this fallback to origin/master (similar to the master branch we create on init) since that is a branch which many projects have. E.g. for the users that share one central server and just directly commit, push and pull to/from master. They would have an easy way to start working in a submodule, by simply saying --remote and then committing to master. At least that is what I imagine.

Show 8 quoted lines

> > gitmodules(5) is pretty clear that 'submodule.<name>.branch' defaults
> > to master (and not upstream's HEAD), do we want to adjust this at the
> > same time?
> 
> That may be likely.  If the value set to a configuration variable
> causes an established behaviour of a program change a lot, silently
> defaulting that variable to something many people are expected to
> have (e.g. 'master') would likely to cause a usability regression.

IMO this branch configuration should completely ignored in the default, non --remote, usage. Since we simply checkout a specific SHA1 in this case, that should be possible.

Cheers Heiko