Re: Possible regression in master? (submodules without a "master" branch)
- From
Jens Lehmann <jens.lehmann@web.de>
- Date
- Mar 27, 2014, 22:55 UTC
- Message-ID
- <5334AC59.7010605@web.de>
- In-Reply-To
- <xmqq8urvebok.fsf@gitster.dls.corp.google.com>
Am 27.03.2014 19:30, schrieb Junio C Hamano:
Show 46 quoted lines
> 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.
Show 17 quoted lines
> - 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)