* [PATCH] recursive submodules: detach HEAD from new state @ 2017-07-24 17:36 Stefan Beller 2017-07-24 18:03 ` Jonathan Nieder 0 siblings, 1 reply; 13+ messages in thread From: Stefan Beller @ 2017-07-24 17:36 UTC (permalink / raw) To: git; +Cc: gitster, jrnieder, Stefan Beller When a submodule is on a branch and in its superproject you run a recursive checkout, the branch of the submodule is updated to what the superproject checks out. This is very unexpected in the current model of Git as e.g. 'submodule update' always detaches the submodule HEAD. Despite having plans to have submodule HEADS not detached in the future, the current behavior is really bad as it doesn't match user expectations and it is not checking for loss of commits (only to be recovered via the reflog). Detach the HEAD unconditionally in the submodule when updating it. Signed-off-by: Stefan Beller <sbeller@google.com> --- This is a resend of [1], which did not receive any attention. I improved the commit message laying out the current state of affairs, arguing that any future plan should not weigh in as much as the current possible data loss. [1] https://public-inbox.org/git/20170630003851.17288-1-sbeller@google.com/ Thanks, Stefan submodule.c | 3 ++- t/lib-submodule-update.sh | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/submodule.c b/submodule.c index ef83c2a9ee..4b7c0a4c82 100644 --- a/submodule.c +++ b/submodule.c @@ -1653,7 +1653,8 @@ int submodule_move_head(const char *path, cp.dir = path; prepare_submodule_repo_env(&cp.env_array); - argv_array_pushl(&cp.args, "update-ref", "HEAD", new, NULL); + argv_array_pushl(&cp.args, "update-ref", "HEAD", + "--no-deref", new, NULL); if (run_command(&cp)) { ret = -1; diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index 2d26f86800..fc406b95d7 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -848,6 +848,23 @@ test_submodule_switch_recursing_with_args () { test_submodule_content sub1 origin/add_sub1 ) ' + test_expect_success "$command: submodule branch is not changed, detach HEAD instead" ' + prolog && + reset_work_tree_to_interested add_sub1 && + ( + cd submodule_update && + git -C sub1 checkout -b keep_branch && + git -C sub1 rev-parse HEAD >expect && + git branch -t check-keep origin/modify_sub1 && + $command check-keep && + test_superproject_content origin/modify_sub1 && + test_submodule_content sub1 origin/modify_sub1 && + git -C sub1 rev-parse keep_branch >actual && + test_cmp expect actual && + test_must_fail git -C sub1 symbolic-ref HEAD + ) + ' + # Replacing a tracked file with a submodule produces a checked out submodule test_expect_success "$command: replace tracked file with submodule checks out submodule" ' prolog && -- 2.14.0.rc0.3.g6c2e499285 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] recursive submodules: detach HEAD from new state 2017-07-24 17:36 [PATCH] recursive submodules: detach HEAD from new state Stefan Beller @ 2017-07-24 18:03 ` Jonathan Nieder 2017-07-24 19:07 ` Stefan Beller 2017-07-24 21:33 ` Junio C Hamano 0 siblings, 2 replies; 13+ messages in thread From: Jonathan Nieder @ 2017-07-24 18:03 UTC (permalink / raw) To: Stefan Beller; +Cc: git, gitster Hi, Stefan Beller wrote: > When a submodule is on a branch and in its superproject you run a > recursive checkout, the branch of the submodule is updated to what the > superproject checks out. This is very unexpected in the current model of > Git as e.g. 'submodule update' always detaches the submodule HEAD. > > Despite having plans to have submodule HEADS not detached in the future, > the current behavior is really bad as it doesn't match user expectations > and it is not checking for loss of commits (only to be recovered via the > reflog). I think the corrected behavior doesn't match user expectations, either. Could this patch include some documentation to help users know what to expect? > Detach the HEAD unconditionally in the submodule when updating it. > > Signed-off-by: Stefan Beller <sbeller@google.com> > --- > This is a resend of [1], which did not receive any attention. Yikes. Yes, this bug looks problematic. Thanks for working on it. > I improved the commit message laying out the current state of affairs, > arguing that any future plan should not weigh in as much as the current > possible data loss. Can you elaborate on what you mean about data loss? At first glance it would seem to me that detaching HEAD could lead to data loss since there isn't a branch to keep track of the user's work. Are you saying the current behavior of updating whatever branch HEAD is on (which, don't get me wrong, is a wrong behavior that needs fixing) bypassed the reflog? Thanks, Jonathan > [1] https://public-inbox.org/git/20170630003851.17288-1-sbeller@google.com/ [...] > --- a/submodule.c > +++ b/submodule.c > @@ -1653,7 +1653,8 @@ int submodule_move_head(const char *path, > cp.dir = path; > > prepare_submodule_repo_env(&cp.env_array); > - argv_array_pushl(&cp.args, "update-ref", "HEAD", new, NULL); > + argv_array_pushl(&cp.args, "update-ref", "HEAD", > + "--no-deref", new, NULL); > > if (run_command(&cp)) { > ret = -1; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] recursive submodules: detach HEAD from new state 2017-07-24 18:03 ` Jonathan Nieder @ 2017-07-24 19:07 ` Stefan Beller 2017-07-24 20:57 ` Junio C Hamano 2017-07-24 21:33 ` Junio C Hamano 1 sibling, 1 reply; 13+ messages in thread From: Stefan Beller @ 2017-07-24 19:07 UTC (permalink / raw) To: Jonathan Nieder; +Cc: git@vger.kernel.org, Junio C Hamano On Mon, Jul 24, 2017 at 11:03 AM, Jonathan Nieder <jrnieder@gmail.com> wrote: > Hi, > > Stefan Beller wrote: > >> When a submodule is on a branch and in its superproject you run a >> recursive checkout, the branch of the submodule is updated to what the >> superproject checks out. This is very unexpected in the current model of >> Git as e.g. 'submodule update' always detaches the submodule HEAD. >> >> Despite having plans to have submodule HEADS not detached in the future, >> the current behavior is really bad as it doesn't match user expectations >> and it is not checking for loss of commits (only to be recovered via the >> reflog). > > I think the corrected behavior doesn't match user expectations, > either. Well, what is the user expectation? > > Could this patch include some documentation to help users know what to > expect? Sure, once we figured out what is reasonable. > >> Detach the HEAD unconditionally in the submodule when updating it. >> >> Signed-off-by: Stefan Beller <sbeller@google.com> >> --- >> This is a resend of [1], which did not receive any attention. > > Yikes. Yes, this bug looks problematic. Thanks for working on it. > >> I improved the commit message laying out the current state of affairs, >> arguing that any future plan should not weigh in as much as the current >> possible data loss. > > Can you elaborate on what you mean about data loss? Assume we have a submodule 'sub' inside the superproject 'super', then git -C super/sub checkout <my-unrelated-branch> git -C super checkout <some-tree-ish> modifies my-unrelated-branch in the submodule, which is not related to the superproject in any way. This patch would detach from that branch and have the HEAD contain the desired sha1. To think that further we'd still have potential data loss: git -C super/sub checkout <my-unrelated-branch> git -C super checkout <some-tree-ish> # fine so far as sub is in detached HEAD, but: ... hack hack hack ... in 'sub' git -C super/sub commit -m "work" git -C super checkout <other-tree-ish> # subs work is only to be recovered via reflog! However this matches the current behavior of "submodule update" which also tips, that are not reachable from any ref. > At first glance > it would seem to me that detaching HEAD could lead to data loss since > there isn't a branch to keep track of the user's work. yes, but that is the same with "submodule update", which is what people may have in mind? > Are you saying > the current behavior of updating whatever branch HEAD is on (which, > don't get me wrong, is a wrong behavior that needs fixing) bypassed > the reflog? No, I am not saying that. I am saying that updating an unrelated branch (which is dragged into the affair just because HEAD points at it) is very subtle thing, as any commits on that branch can be considered safe (it is on a branch, right?) but the detached HEAD is the default unsafe mode we currently have. Thanks, Stefan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] recursive submodules: detach HEAD from new state 2017-07-24 19:07 ` Stefan Beller @ 2017-07-24 20:57 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2017-07-24 20:57 UTC (permalink / raw) To: Stefan Beller; +Cc: Jonathan Nieder, git@vger.kernel.org Stefan Beller <sbeller@google.com> writes: >> Are you saying >> the current behavior of updating whatever branch HEAD is on (which, >> don't get me wrong, is a wrong behavior that needs fixing) bypassed >> the reflog? > > No, I am not saying that. > I am saying that updating an unrelated branch (which is dragged into > the affair just because HEAD points at it) is very subtle thing, as any > commits on that branch can be considered safe (it is on a branch, right?) > but the detached HEAD is the default unsafe mode we currently have. Then it is not a data loss as you claimed in the proposed log message, but is something else, no? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] recursive submodules: detach HEAD from new state 2017-07-24 18:03 ` Jonathan Nieder 2017-07-24 19:07 ` Stefan Beller @ 2017-07-24 21:33 ` Junio C Hamano 2017-07-24 22:23 ` Junio C Hamano 1 sibling, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2017-07-24 21:33 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Stefan Beller, git Jonathan Nieder <jrnieder@gmail.com> writes: > Yikes. Yes, this bug looks problematic. Thanks for working on it. > >> I improved the commit message laying out the current state of affairs, >> arguing that any future plan should not weigh in as much as the current >> possible data loss. > > Can you elaborate on what you mean about data loss? At first glance > it would seem to me that detaching HEAD could lead to data loss since > there isn't a branch to keep track of the user's work. Are you saying > the current behavior of updating whatever branch HEAD is on (which, > don't get me wrong, is a wrong behavior that needs fixing) bypassed > the reflog? Also, while I do agree with you that the problem exists, it is unclear why this patch is a solution and not a hack that sweeps a problem under the rug. It is unclear why this "silently detach HEAD without telling the user" is a better solution than erroring out, for example [*1*]. [Footnote] *1* For example, I would imagine that the problem can also be "fixed" by detecting that the HEAD is on a branch, and noticing that it will force rewinding of that branch if we did update-ref in this codepath, and signal the failure to the caller of submodule_move_head() without making the damage larger. And tell the user what is going on, and perhaps suggest to detach HEAD to avoid clobbering their branch. > > Thanks, Jonathan > >> [1] https://public-inbox.org/git/20170630003851.17288-1-sbeller@google.com/ > [...] >> --- a/submodule.c >> +++ b/submodule.c >> @@ -1653,7 +1653,8 @@ int submodule_move_head(const char *path, >> cp.dir = path; >> >> prepare_submodule_repo_env(&cp.env_array); >> - argv_array_pushl(&cp.args, "update-ref", "HEAD", new, NULL); >> + argv_array_pushl(&cp.args, "update-ref", "HEAD", >> + "--no-deref", new, NULL); >> >> if (run_command(&cp)) { >> ret = -1; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] recursive submodules: detach HEAD from new state 2017-07-24 21:33 ` Junio C Hamano @ 2017-07-24 22:23 ` Junio C Hamano 2017-07-25 22:27 ` Stefan Beller 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2017-07-24 22:23 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Stefan Beller, git Junio C Hamano <gitster@pobox.com> writes: > Also, while I do agree with you that the problem exists, it is > unclear why this patch is a solution and not a hack that sweeps a > problem under the rug. > > It is unclear why this "silently detach HEAD without telling the > user" is a better solution than erroring out, for example [*1*]. Just to avoid possible confusion; I am not claiming that it would be more (or less for that matter) sensible to error out than silently detaching HEAD, because I am not giving the reason to substantiate the claim and I do not have a strong opinion to favour which one (or another potential solution, if any). I am just saying that the patch that proposes a solution should be backed with an explanation why it is a good idea, especially when there are obvious alternatives that are not so clearly inferior. Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] recursive submodules: detach HEAD from new state 2017-07-24 22:23 ` Junio C Hamano @ 2017-07-25 22:27 ` Stefan Beller 2017-07-26 19:36 ` Junio C Hamano 2017-11-21 22:34 ` Jonathan Nieder 0 siblings, 2 replies; 13+ messages in thread From: Stefan Beller @ 2017-07-25 22:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jonathan Nieder, git@vger.kernel.org On Mon, Jul 24, 2017 at 3:23 PM, Junio C Hamano <gitster@pobox.com> wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> Also, while I do agree with you that the problem exists, it is >> unclear why this patch is a solution and not a hack that sweeps a >> problem under the rug. >> >> It is unclear why this "silently detach HEAD without telling the >> user" is a better solution than erroring out, for example [*1*]. > > Just to avoid possible confusion; I am not claiming that it would be > more (or less for that matter) sensible to error out than silently > detaching HEAD, because I am not giving the reason to substantiate > the claim and I do not have a strong opinion to favour which one (or > another potential solution, if any). > > I am just saying that the patch that proposes a solution should be > backed with an explanation why it is a good idea, especially when > there are obvious alternatives that are not so clearly inferior. > > Thanks. So I took a step back and wrote about different proposals where we want to go long term. See below. This will help us figuring out how to approach this bug correctly. ------ RFC: A new type of symbolic refs A symbolic ref can currently only point at a ref or another symbolic ref. This proposal show cases different scenarios on how this could change in the future. A: HEAD pointing at the superprojects index =========================================== Introduce a new symbolic ref that points at the superprojects index of the gitlink. The format is "repo:" <superprojects gitdir> '\0' <gitlink-path> '\0' Ref read operations ------------------- e.g. git log HEAD Just like existing symrefs, the content of the ref will be read and followed. On reading "repo:", the sha1 will be obtained equivalent to: git -C <superproject> ls-files -s <gitlink-path> | awk '{ print $2}' In case of error (superproject not found, gitlink path does not exist), the ref is broken and Ref write operations driven by the submodule, affecting symrefs --------------------------------------------------------------- e.g. git checkout <other branch> (in the submodule) In this scenario only the HEAD is optionally attached to the superproject, so we can rewrite the HEAD to be anything else, such as a branch just fine. Once the HEAD is not pointing at the superproject any more, we'll leave the submodule alone in operations driven by the superproject. Ref write operations driven by the submodule, affecting target ref ------------------------------------------------------------------ e.g. git commit, reset --hard, update-ref (in the submodule) The HEAD stays the same, pointing at the superproject. The gitlink is changed to the target sha1, using git -C <superproject> update-index --add \ --cacheinfo 160000,$SHA1,<gitlink-path> This will affect the superprojects index, such that then a commit in the superproject is needed. Ref write operations driven by the superproject, changing the gitlink --------------------------------------------------------------------- e.g. git checkout <tree-ish>, git reset --hard (in the superproject) This will change the gitlink in the superprojects index, such that the HEAD in the submodule changes, which would trigger an update of the submodules working tree. Consistency considerations (gc) ------------------------------- e.g. git gc --aggressive --prune=now The repacking logic is already aware of a detached HEAD, such that using this new symref mechanism would not generate problems as long as we keep the HEAD attached to the superproject. However when commits/objects are created while the HEAD is attached to the superproject and then HEAD switches to a local branch, there are problems with the created objects as they seem unreachable now. This problem is not new as a superproject may record submodule objects that are not reachable from any of the submodule branches. Such objects fall prey to overzealous packing in the submodule. This proposal however exposes this problem a lot more, as the submodule has fewer needs for branches. B: HEAD pointing at a superprojects branch ========================================== Instead of pointing at the index of the superproject, we also encode a branch name: repo:" <superprojects gitdir> '\0' <gitlink-path> '\0' branch '\0' Ref read operations ------------------- e.g. git log HEAD This is similar to the case of pointing at the index, except that the reading operation reads from the tip of the branch: git -C <superproject> ls-tree <superproject branch> -- \ <gitlink-path> | awk '{ print $3}' Ref write operations driven by the submodule, affecting symrefs --------------------------------------------------------------- e.g. git checkout <other branch> (in the submodule) HEAD will be pointed at the local target branch, dropping the affliation to the superproject. Ref write operations driven by the submodule, affecting target ref ------------------------------------------------------------------ e.g. git commit, reset --hard, update-ref (in the submodule) As we're pointing at the superprojects branch, this would have to create a dummy(?) commit in the superproject, that just changes the submodule pointer in the superprojects branch, such that the operation of storing a new sha1 for the submodule is equivalent to git -C <superproject> update-index --add \ --cacheinfo 160000,$SHA1,<gitlink-path> git -C <superproject> commit -m "Update submodule" This behavior in the superproject is similar to Gerrits subscription model where superprojects are updated from the submodule. Each operation in the submodule triggers a local superproject commit. Ref write operations driven by the superproject, changing the gitlink --------------------------------------------------------------------- e.g. git merge, git pull (in the superproject) This will change the gitlink in the superprojects index, such that the HEAD in the submodule changes, which would trigger an update of the submodules working tree. This would require a good merge strategy for submodules, i.e. on merge the submodule would create a merge commit that is recorded in the superprojects merge commit. Consistency considerations (gc) ------------------------------- e.g. git gc --aggressive --prune=now The repacking problem comes with a solution unlike the previous proposal. This is because any relevant commit in the submodule is recorded in the superproject via a commit in a branch. Then even non-fast-forward histories in the submodule can all be kept by walking the superproject and looking at all gitlink entries of the submodule. C: All branches are symbolic references to the superproject =========================================================== Instead of having just HEAD pointed at a superproject, all(!) branches in the submodule point at the superprojects branch of the same name. Symbolic refs that resolve to a local sha1 are not allowed, any symbolic ref ends up pointing at the superproject eventually. e.g. HEAD points at a submodule branch, which in turn points at the superproject branch of the same name. Ref read operations ------------------- e.g. git log HEAD is read, which may be either (a) locally detached or (b) pointing at a superproject branch. Resolve as in B. Ref write operations driven by the submodule, affecting symrefs --------------------------------------------------------------- e.g. git checkout <other branch> (in the submodule) As there is no other local branch, HEAD would point at the other submodule branch, which then points at another branch in the superproject. Ref write operations driven by the submodule, affecting target ref ------------------------------------------------------------------ e.g. git commit, reset --hard, update-ref (in the submodule) same as B. Ref write operations driven by the superproject, changing the gitlink --------------------------------------------------------------------- e.g. git merge, git pull (in the superproject) same as B. Consistency considerations (gc) ------------------------------- e.g. git gc --aggressive --prune=now As the superproject contains all knowledge, the gc starts with a walk of all superproject branches, destilling the recorded gitlink entries and then starts walking in the submodule from all the recorded gitlinks to create a pack. gc and repacking would either be forbidden in the submodule or deflected to the superproject. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] recursive submodules: detach HEAD from new state 2017-07-25 22:27 ` Stefan Beller @ 2017-07-26 19:36 ` Junio C Hamano 2017-11-21 22:34 ` Jonathan Nieder 1 sibling, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2017-07-26 19:36 UTC (permalink / raw) To: Stefan Beller; +Cc: Jonathan Nieder, git@vger.kernel.org Stefan Beller <sbeller@google.com> writes: > So I took a step back and wrote about different proposals where > we want to go long term. See below. This will help us > figuring out how to approach this bug correctly. Thanks for writing this. > RFC: A new type of symbolic refs > > A symbolic ref can currently only point at a ref or another symbolic ref. > This proposal show cases different scenarios on how this could change in > the future. > > > > A: HEAD pointing at the superprojects index > =========================================== > > Introduce a new symbolic ref that points at the superprojects index of > the gitlink. The format is > > "repo:" <superprojects gitdir> '\0' <gitlink-path> '\0' > > Ref read operations > ------------------- > e.g. git log HEAD > > Just like existing symrefs, the content of the ref will be read and followed. > On reading "repo:", the sha1 will be obtained equivalent to: > > git -C <superproject> ls-files -s <gitlink-path> | awk '{ print $2}' > > In case of error > (superproject not found, gitlink path does not exist), the ref is broken and > > Ref write operations driven by the submodule, affecting symrefs > --------------------------------------------------------------- > e.g. git checkout <other branch> (in the submodule) > > In this scenario only the HEAD is optionally attached to the superproject, > so we can rewrite the HEAD to be anything else, such as a branch just fine. > Once the HEAD is not pointing at the superproject any more, we'll leave the > submodule alone in operations driven by the superproject. That explains what the proposed code _does_. It does not explain why the chosen behaviour is a sensible one. This illustrates the point I have trouble with when trying to judge all of these discrete update proposals to submodules. They only say "This feature does this in this case, does that in that case,..." but lack "this is meant to be used when you want to implement the workflow that goes like this, and fits as a building block at this point for that workflow. Other elements needed to support that workflow well are ...". No proposal gives a big picture and explain how these small bits fit together. For example, I would understand better if this write-up of yours were not organized with the "proposal X added A that behaves this way and added B that behaves that way" as its major axis, but instead was written with the workflow that is meant to be realized as its major axis, e.g. A project may want to use submodules as if it is just part of superproject. In such a project, checking out branch X at the superproject level, working on files in both superproject and submodules, and then committing recursively and pushing the results out recursively at the superproject level, all would want to affect the same branch X at all levels in the upstream. may be one possible workflow you want to support. As one ingredient to support such structure, the HEAD in the submodule that points at an index entry in the superproject may be very useful. After a recursive checkout at the superproject level, the HEAD of the submodule ought to be what came from and recorded in the tree in the superproject, and after a commit in the submodule, the HEAD moves to the new commit and the entry in the superproject's index also gets updated which would have a nice property that "commit" in submodule acts almost like "add" in superproject. A recursive "git diff" would show that submodule is clean after such a commit, recursive "push" would know which branch to push out, etc. And when operating in such a mode, it would make most sense if "git checkout" of a different branch Y in a submodule repository is either forbidden, or should behave as if the submodule directory were an ordinary directory of the superproject (i.e. causing recursive checkout of the branch Y at the superproject level). BUT. Because none of the proposals paint a big picture (e.g. the big picture the above hypothetical example gives is that the core concept of this particular workflow being supported is that everything recursively stays on the branch with the same name), we cannot judge if it is sensible for "a new style symref" to be updated/demoted to a normal branch pointer when "git checkout" happens. It is not sensible in such a hypothetical workflow, but it may be very sensible in another workflow. Without stating what big-picture goal is being achieved, it is impossible to see if a proposal to add/change an individual component that is to be used as a building block makes sense. Historically, we can get away without giving choices of "supported workflows", allowing the user to pick one, and explaining how things fit together, primarily because the operations that can recurse were primarily read-only e.g. status, grep, etc., and the supported model was "the user can be on whatever branch or detached in each submodule that may or may not be consistent with what happens in the superproject; it is up to the user to hang themselves with the long rope". When allowing potentially destructive operations like checkout to go recursive [*1*], depending on how the entire tree of repositories is meant to be managed, sensible mode of operation would be different, but without defining what various ways "how the entire tree of repositories is meant to be managed" are supported, we cannot teach such operations to go recursive in a sensible way. [Footnote] *1* Some readers may wonder "checkout is destructive???", and in the context of this discussion, it is. Recursive checkout done at the superproject level that rewinds the branch currently checked out in a submodule is destructive by potentially losing history, and a recursive checkout that checks out a different branch in a submodule can be destructive by changing where the next "git push" in a submodule would go, depending on how the entire tree of repositories is meant to be managed. In some workflows, always detaching HEAD to the commit that is bound to the superproject may be _the_ sensible way to recursively check out a branch. In some other workflows, detecting that the submodule is on a branch that is not the branch the superproject is checking out and erroring out may be more sensible way. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] recursive submodules: detach HEAD from new state 2017-07-25 22:27 ` Stefan Beller 2017-07-26 19:36 ` Junio C Hamano @ 2017-11-21 22:34 ` Jonathan Nieder 2017-11-21 22:45 ` Stefan Beller 2017-11-22 0:54 ` Junio C Hamano 1 sibling, 2 replies; 13+ messages in thread From: Jonathan Nieder @ 2017-11-21 22:34 UTC (permalink / raw) To: Stefan Beller; +Cc: Junio C Hamano, git@vger.kernel.org Stefan Beller wrote: >> Junio C Hamano <gitster@pobox.com> writes: >>> Also, while I do agree with you that the problem exists, it is >>> unclear why this patch is a solution and not a hack that sweeps a >>> problem under the rug. >>> >>> It is unclear why this "silently detach HEAD without telling the >>> user" is a better solution than erroring out, for example [*1*]. [...] > So I took a step back and wrote about different proposals where > we want to go long term. See below. This will help us > figuring out how to approach this bug correctly. Stefan, do you know what thread I should look at to find the current state of this patch? I've had it applied locally for a long time. The thread I am replying to appears to be where the patch comes from but I have some memories of more recent discussion that I'm not finding. More context: https://public-inbox.org/git/xmqqshd8i3ze.fsf@gitster.mtv.corp.google.com/ says * sb/submodule-recursive-checkout-detach-head (2017-07-28) 2 commits - Documentation/checkout: clarify submodule HEADs to be detached - recursive submodules: detach HEAD from new state "git checkout --recursive" may overwrite and rewind the history of the branch that happens to be checked out in submodule repositories, which might not be desirable. Detach the HEAD but still allow the recursive checkout to succeed in such a case. Expecting a reroll. Thanks, Jonathan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] recursive submodules: detach HEAD from new state 2017-11-21 22:34 ` Jonathan Nieder @ 2017-11-21 22:45 ` Stefan Beller 2017-11-21 22:47 ` Jonathan Nieder 2017-11-22 0:54 ` Junio C Hamano 1 sibling, 1 reply; 13+ messages in thread From: Stefan Beller @ 2017-11-21 22:45 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Junio C Hamano, git@vger.kernel.org On Tue, Nov 21, 2017 at 2:34 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: > Stefan Beller wrote: >>> Junio C Hamano <gitster@pobox.com> writes: > >>>> Also, while I do agree with you that the problem exists, it is >>>> unclear why this patch is a solution and not a hack that sweeps a >>>> problem under the rug. >>>> >>>> It is unclear why this "silently detach HEAD without telling the >>>> user" is a better solution than erroring out, for example [*1*]. > [...] >> So I took a step back and wrote about different proposals where >> we want to go long term. See below. This will help us >> figuring out how to approach this bug correctly. > > Stefan, do you know what thread I should look at to find the current > state of this patch? I've had it applied locally for a long time. It was "Undecided" for some time, then Junio kicked it back to pu, expecting a reroll[1]. The "send out a plan" that was referenced is found in [2] describing 6 plans for the future of submodules. The approach in [3] which is different on the implementation level, but very similar on the UX level sounds best currently. I'll coordinate with JTan to come up with patches for that. [1] https://public-inbox.org/git/CAGZ79kYUZv0g+3OEMrbT26A7mSLJzeS-yf5Knr-CnARHqVB=aQ@mail.gmail.com/ [2] https://public-inbox.org/git/20171109001007.11894-1-sbeller@google.com/ [3] https://public-inbox.org/git/20171108172945.33c42a0e91b4ac494217b788@google.com/ > The thread I am replying to appears to be where the patch comes from > but I have some memories of more recent discussion that I'm not > finding. > > More context: > https://public-inbox.org/git/xmqqshd8i3ze.fsf@gitster.mtv.corp.google.com/ > says > > * sb/submodule-recursive-checkout-detach-head (2017-07-28) 2 commits > - Documentation/checkout: clarify submodule HEADs to be detached > - recursive submodules: detach HEAD from new state > > "git checkout --recursive" may overwrite and rewind the history of > the branch that happens to be checked out in submodule > repositories, which might not be desirable. Detach the HEAD but > still allow the recursive checkout to succeed in such a case. > > Expecting a reroll. > > Thanks, > Jonathan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] recursive submodules: detach HEAD from new state 2017-11-21 22:45 ` Stefan Beller @ 2017-11-21 22:47 ` Jonathan Nieder 2017-11-21 23:00 ` Stefan Beller 0 siblings, 1 reply; 13+ messages in thread From: Jonathan Nieder @ 2017-11-21 22:47 UTC (permalink / raw) To: Stefan Beller; +Cc: Junio C Hamano, git@vger.kernel.org Hi, Stefan Beller wrote: > On Tue, Nov 21, 2017 at 2:34 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: >> Stefan, do you know what thread I should look at to find the current >> state of this patch? I've had it applied locally for a long time. > > It was "Undecided" for some time, then Junio kicked it back to pu, expecting a > reroll[1]. The "send out a plan" that was referenced is found in [2] > describing 6 plans for the future of submodules. The approach in [3] > which is different on the implementation level, but very similar on > the UX level sounds best currently. I'll coordinate with JTan to > come up with patches for that. > > [1] https://public-inbox.org/git/CAGZ79kYUZv0g+3OEMrbT26A7mSLJzeS-yf5Knr-CnARHqVB=aQ@mail.gmail.com/ > [2] https://public-inbox.org/git/20171109001007.11894-1-sbeller@google.com/ > [3] https://public-inbox.org/git/20171108172945.33c42a0e91b4ac494217b788@google.com/ Thanks. That thread appears to be about a long-term plan; what is the short-term plan? E.g. is there any additional documentation that should be added to the patch that detaches? Or should it go in as-is? Thanks, Jonathan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] recursive submodules: detach HEAD from new state 2017-11-21 22:47 ` Jonathan Nieder @ 2017-11-21 23:00 ` Stefan Beller 0 siblings, 0 replies; 13+ messages in thread From: Stefan Beller @ 2017-11-21 23:00 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Junio C Hamano, git@vger.kernel.org On Tue, Nov 21, 2017 at 2:47 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: > Hi, > > Stefan Beller wrote: >> On Tue, Nov 21, 2017 at 2:34 PM, Jonathan Nieder <jrnieder@gmail.com> wrote: > >>> Stefan, do you know what thread I should look at to find the current >>> state of this patch? I've had it applied locally for a long time. >> >> It was "Undecided" for some time, then Junio kicked it back to pu, expecting a >> reroll[1]. The "send out a plan" that was referenced is found in [2] >> describing 6 plans for the future of submodules. The approach in [3] >> which is different on the implementation level, but very similar on >> the UX level sounds best currently. I'll coordinate with JTan to >> come up with patches for that. >> >> [1] https://public-inbox.org/git/CAGZ79kYUZv0g+3OEMrbT26A7mSLJzeS-yf5Knr-CnARHqVB=aQ@mail.gmail.com/ >> [2] https://public-inbox.org/git/20171109001007.11894-1-sbeller@google.com/ >> [3] https://public-inbox.org/git/20171108172945.33c42a0e91b4ac494217b788@google.com/ > > Thanks. That thread appears to be about a long-term plan; what is the > short-term plan? > > E.g. is there any additional documentation that should be added to the > patch that detaches? The second patch in that series has a tiny bit of information, see "Documentation/checkout: clarify submodule HEADs to be detached". I would think that this is sufficient for the short term to get into a safe state. > Or should it go in as-is? That is what I would prefer and then we'll build on top of this once we figured out the direction of the long term solution. Thanks, Stefan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] recursive submodules: detach HEAD from new state 2017-11-21 22:34 ` Jonathan Nieder 2017-11-21 22:45 ` Stefan Beller @ 2017-11-22 0:54 ` Junio C Hamano 1 sibling, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2017-11-22 0:54 UTC (permalink / raw) To: Jonathan Nieder; +Cc: Stefan Beller, git@vger.kernel.org Jonathan Nieder <jrnieder@gmail.com> writes: > The thread I am replying to appears to be where the patch comes from > but I have some memories of more recent discussion that I'm not > finding. > > More context: > https://public-inbox.org/git/xmqqshd8i3ze.fsf@gitster.mtv.corp.google.com/ > says > > * sb/submodule-recursive-checkout-detach-head (2017-07-28) 2 commits > - Documentation/checkout: clarify submodule HEADs to be detached > - recursive submodules: detach HEAD from new state > > "git checkout --recursive" may overwrite and rewind the history of > the branch that happens to be checked out in submodule > repositories, which might not be desirable. Detach the HEAD but > still allow the recursive checkout to succeed in such a case. > > Expecting a reroll. Sorry, I should have done my usual "cf. <message-id>" to help me recalling the discussion that lead to the marking there. We kicked it back to 'pu' after the discussion that had <xmqq375ppqma.fsf@gitster.mtv.corp.google.com>, and the "send out a plan" sort-of happened with <20171109001007.11894-1-sbeller@google.com> which seemed to have converged to a conclusion in the subthread with <CAGZ79kZAvMKQUjbqWZkhy39sE5e9k1DmkiA42ywiw2NgY1+Xig@mail.gmail.com> where a preferred way would be to detach and opportunistically reattach to keep the illusion of submodule being on a branch as much as possible (correct me if I am misunderstanding the consensus). I had a suspicion that such a random re-attachment may lead to an unpredictable behaviour from end-user's point of view that is confusing, but there wasn't a concrete patch to do so, so that was why I was waiting for a reroll so that people can take a look at it and see how well it fares. The responses in this thread seems to indicate that now we are punting on that re-attachment plan and want to give this "always detach" to the end users, which I think is a lot more predictable thing to do. After such a recursive checkout from the top-level, any work done in the submodule from that state and is referenced from the top-level (recorded presumably with a recursive commit) is not referenced by anything other than the reflog of HEAD in the submodule repository, and I suspect many users are not used to and are comfortable with working on a detached HEAD for extended period of time, so this new behaviour might deserve a stronger warning to help them, but I am OK with the stance "We'll learn as we go---let's merge it as-is and see what happens". Thanks for prodding. One less topic that is stale but has to be carried around in 'pu' is always a good thing ;-) ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-11-22 0:54 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-07-24 17:36 [PATCH] recursive submodules: detach HEAD from new state Stefan Beller 2017-07-24 18:03 ` Jonathan Nieder 2017-07-24 19:07 ` Stefan Beller 2017-07-24 20:57 ` Junio C Hamano 2017-07-24 21:33 ` Junio C Hamano 2017-07-24 22:23 ` Junio C Hamano 2017-07-25 22:27 ` Stefan Beller 2017-07-26 19:36 ` Junio C Hamano 2017-11-21 22:34 ` Jonathan Nieder 2017-11-21 22:45 ` Stefan Beller 2017-11-21 22:47 ` Jonathan Nieder 2017-11-21 23:00 ` Stefan Beller 2017-11-22 0:54 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).