Re: Re: Possible regression in master? (submodules without a "master" branch)
On Fri, Mar 28, 2014 at 12:21:23AM +0100, Johan Herland wrote: > On Thu, Mar 27, 2014 at 9:27 PM, Heiko Voigt wrote: > > On Thu, Mar 27, 2014 at 12:39:03PM -0700, Junio C Hamano wrote: > >> 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..branch. > >> > >> … > >> 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..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. Branch-attachment is mostly a function of submodule..update, not a function of submodule..branch. We could certainly interpret a missing submodule..branch as: * Use the subproject's HEAD for the initial clone (clear start_point in cmd_update if submodule."$name".branch is not set). * Don't change the branch name on subsequent local updates (what we already do). * Do $something if the user tries a --remote update. I just don't know what that $something should be. > FWIW, here is the behaviour I would expect from "git submodule > update": > > - In checkout-mode, if submodule..branch is not set, we > should _always_ detach. Whether or not the submodule is already > cloned does not matter. Agreed, checkout-mode should *always* detach the submodule's HEAD. > - In rebase/merge-mode, if submodule..branch is not set, we > should _always_ abort with an error. Why? Can't we mimic clone and use the remote's HEAD (for --remote updates)? That seems more intuitive to me. For local updates, we're just integrating the gitlinked commit with the submodule's HEAD, and you don't need submodule..branch for that at all. > - If submodule..branch is set - but the branch it refers to > does not exist - we should _always_ abort with an error. The current > checkout/rebase/merge-mode does not matter. Sounds good to me, and should match the current functionality. > In other words, submodule..branch is _necessary_ in > rebase/merge mode, but _optional_ in checkout-mode (its absence > indicating that we should detach). The main trigger for “we should detach” is the update mode (checkout-mode detaches, all others integrate with the submodule's HEAD (without changing submodule branches). You only need submodule..branch for determining which *remote* commit you're trying to integrate (or clone from). HEAD, master, and “die screaming” all sound like reasonable defaults in that case. Deciding between them is a policy/UI decision, not a technical decision. > >> > gitmodules(5) is pretty clear that 'submodule..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. > > Yes. Checkout-mode with no submodule..branch configured should > always detach. Except for the initial clone (where it's easy to fix), submodule..branch *is* ignored in non --remote updates. Cheers, Trevor -- This email may be signed or encrypted with GnuPG (http://www.gnupg.org). For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy signature.asc Description: OpenPGP digital signature
Re: Re: Possible regression in master? (submodules without a "master" branch)
(Thanks to all of you for picking this up and more or less resolving it while I was away from email for a few hours...) On Thu, Mar 27, 2014 at 9:27 PM, Heiko Voigt wrote: > On Thu, Mar 27, 2014 at 12:39:03PM -0700, Junio C Hamano wrote: >> "W. Trevor King" 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 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. ...which is exactly the behaviour I (and the Qt project - I assume) expected. >> > Fixing this for initial update clone is pretty easy, we just need to >> > unset start_point before calling module_clone if >> > submodule..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..branch. >> >> [side note] Isn't that a typo of submodule..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. > >> 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..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. FWIW, here is the behaviour I would expect from "git submodule update": - In checkout-mode, if submodule..branch is not set, we should _always_ detach. Whether or not the submodule is already cloned does not matter. - In rebase/merge-mode, if submodule..branch is not set, we should _always_ abort with an error. - If submodule..branch is set - but the branch it refers to does not exist - we should _always_ abort with an error. The current checkout/rebase/merge-mode does not matter. In other words, submodule..branch is _necessary_ in rebase/merge mode, but _optional_ in checkout-mode (its absence indicating that we should detach). >> > However, that's just going to >> > push remote branch ambiguity problems back to the --remote update >> > functionality. What should happen when submodule..branch is not >> > set and you run a --remote update, which has used: >> > >> > git rev-parse "${remote_name}/${branch}" >> > >> > since the submodule..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..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. I think the analogy to "the master branch we create on init" is false. A better analogy is running "git pull" or "git pull -rebase" in a branch where branch..merge has not yet been set. And this currently fails with "Please specify which branch you want to merge with." So I would be inclined to agree with Junio here: We should error out. > 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. If there are compelling arguments for providing a default fallback (and I'm not sure the above argument is enough), I say we should rather follow clone's lead, and use the submodule's upstream's HEAD, instead of blindly assuming "origin/master" to be present. I expect in most cases where "origin/master" happens to be the Right Answer, using the submodule's upstream's HEAD will yield the same result. >> > gitmodules(5) is pretty clear that 'submodule..branch' defaults >> > to master (and not upstream's HEAD), do we want to adjust this at the >>
Re: Re: Possible regression in master? (submodules without a "master" branch)
On Thu, Mar 27, 2014 at 12:39:03PM -0700, Junio C Hamano wrote: > "W. Trevor King" 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 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..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..branch. > > [side note] Isn't that a typo of submodule..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. > 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..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. > > However, that's just going to > > push remote branch ambiguity problems back to the --remote update > > functionality. What should happen when submodule..branch is not > > set and you run a --remote update, which has used: > > > > git rev-parse "${remote_name}/${branch}" > > > > since the submodule..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..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. > > gitmodules(5) is pretty clear that 'submodule..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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html