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

[Jens Lehmann <Jens.Lehmann@web.de>]

Am 27.03.2014 19:30, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>> Am 27.03.2014 16:52, schrieb W. Trevor King:
>>> On Thu, Mar 27, 2014 at 03:21:49PM +0100, Johan Herland wrote:
>>>> 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:
>>>
>>> The docs say [1]:
>>>
>>>   A remote branch name for tracking updates in the upstream submodule.
>>>   If the option is not specified, it defaults to 'master'.
>>
>> But the "branch" setting isn't configured for Qt, the .gitmodules
>> file contains only this:
>>
>> [submodule "qtbase"]
>> 	path = qtbase
>> 	url = ../qtbase.git
>> ...
>>
>>> which is what we do now.  Working around that to default to the
>>> upstream submodule's HEAD is possible (you can just use --branch
>>> HEAD), but I think it's easier to just explicitly specify your
>>> preferred branch.
>>
>> That is *not* easier, as Johan did not have to do that before.
>>
>> I think your patch 23d25e48f5ead73c9ce233986f90791abec9f1e8 does
>> not do what the commit message promised:
>>
>>     With this change, folks cloning submodules for the first time via:
>>
>>       $ git submodule update ...
>>
>>     will get a local branch instead of a detached HEAD, unless they are
>>     using the default checkout-mode updates.
>>
>> And Qt uses the "default checkout-mode updates" and doesn't have
>> "branch" configured either. So we are facing a serious regression
>> here.
> 
> There are two potential issues (and a half) then:
> 
>  - When cloning with the "default checkout-mode updates", the new
>    feature to avoid detaching the HEAD should not kick in at all.

Yep.

>  - For a repository that does not have that "branch" thing
>    configured, the doc says that it will default to 'master'.
> 
>    I do not think this was brought up during the review, but is it a
>    sensible default if the project does not even have that branch?
> 
>    What are viable alternatives?
> 
>    - use 'master' and fail just the way Johan saw?
> 
>    - use any random branch that happens to be at the same commit as
>      what is being checked out?
> 
>    - use the branch "clone" for the submodule repository saw the
>      upstream was pointing at with its HEAD?
> 
>    - something else?

Good question. Me thinks that when a superproject doesn't have
'branch' configured and does set 'update' to something other than
'checkout' for a submodule it should better make sure 'master'
is a valid branch in there. Everything else sounds like a
misconfiguration on the superproject's part that warrants an
error. But I may be wrong here as I only use 'checkout' together
with a detached HEADs myself. Comments welcome.

>  - Johan's set-up was apparently not covered in the addition to t/
>    in 23d25e48 (submodule: explicit local branch creation in
>    module_clone, 2014-01-26)---otherwise we would have caught this
>    regression.  Are there other conditions that are not covered?

I suspect so. This is one of the reasons I started the submodule
testing framework I posted an RFC for a few days ago, as an attempt
to start a systematic approach to submodule testing. This is not
the first time a breakage was not caught by the tests, so we need
to do better here. (Note to self: test for the detached HEAD for
the checkout case in the framework too)