{"thread":{"id":"46454","subject":"[PATCH] recursive submodules: detach HEAD from new state","startedAt":"2017-07-24T17:38:45Z","lastAt":"2017-11-22T00:54:23Z","messageCount":13,"participants":["Stefan Beller","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"324961","messageId":"20170724173601.19921-1-sbeller@google.com","threadId":"46454","inReplyTo":null,"subject":"[PATCH] recursive submodules: detach HEAD from new state","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-24T17:36:01Z","receivedAt":"2017-07-24T17:38:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When a submodule is on a branch and in its superproject you run a\nrecursive checkout, the branch of the submodule is updated to what the\nsuperproject checks out. This is very unexpected in the current model of\nGit as e.g. 'submodule update' always detaches the submodule HEAD.\n\nDespite having plans to have submodule HEADS not detached in the future,\nthe current behavior is really bad as it doesn't match user expectations\nand it is not checking for loss of commits (only to be recovered via the\nreflog).\n\nDetach the HEAD unconditionally in the submodule when updating it.\n\nSigned-off-by: Stefan Beller \n---\n\nThis is a resend of [1], which did not receive any attention.\nI improved the commit message laying out the current state of affairs,\narguing that any future plan should not weigh in as much as the current\npossible data loss.\n\n[1] https://public-inbox.org/git/20170630003851.17288-1-sbeller@google.com/\n\nThanks,\nStefan\n\n submodule.c | 3 ++-\n t/lib-submodule-update.sh | 17 +++++++++++++++++\n 2 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex ef83c2a9ee..4b7c0a4c82 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1653,7 +1653,8 @@ int submodule_move_head(const char *path,\n \t\t\tcp.dir = path;\n \n \t\t\tprepare_submodule_repo_env(&cp.env_array);\n-\t\t\targv_array_pushl(&cp.args, \"update-ref\", \"HEAD\", new, NULL);\n+\t\t\targv_array_pushl(&cp.args, \"update-ref\", \"HEAD\",\n+\t\t\t\t\t \"--no-deref\", new, NULL);\n \n \t\t\tif (run_command(&cp)) {\n \t\t\t\tret = -1;\ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex 2d26f86800..fc406b95d7 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -848,6 +848,23 @@ test_submodule_switch_recursing_with_args () {\n \t\t\ttest_submodule_content sub1 origin/add_sub1\n \t\t)\n \t'\n+\ttest_expect_success \"$command: submodule branch is not changed, detach HEAD instead\" '\n+\t\tprolog &&\n+\t\treset_work_tree_to_interested add_sub1 &&\n+\t\t(\n+\t\t\tcd submodule_update &&\n+\t\t\tgit -C sub1 checkout -b keep_branch &&\n+\t\t\tgit -C sub1 rev-parse HEAD >expect &&\n+\t\t\tgit branch -t check-keep origin/modify_sub1 &&\n+\t\t\t$command check-keep &&\n+\t\t\ttest_superproject_content origin/modify_sub1 &&\n+\t\t\ttest_submodule_content sub1 origin/modify_sub1 &&\n+\t\t\tgit -C sub1 rev-parse keep_branch >actual &&\n+\t\t\ttest_cmp expect actual &&\n+\t\t\ttest_must_fail git -C sub1 symbolic-ref HEAD\n+\t\t)\n+\t'\n+\n \t# Replacing a tracked file with a submodule produces a checked out submodule\n \ttest_expect_success \"$command: replace tracked file with submodule checks out submodule\" '\n \t\tprolog &&\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"324963","messageId":"20170724180312.GE13924@aiede.mtv.corp.google.com","threadId":"46454","inReplyTo":"20170724173601.19921-1-sbeller@google.com","subject":"Re: [PATCH] recursive submodules: detach HEAD from new state","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-07-24T18:03:12Z","receivedAt":"2017-07-24T18:03:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> When a submodule is on a branch and in its superproject you run a\n> recursive checkout, the branch of the submodule is updated to what the\n> superproject checks out. This is very unexpected in the current model of\n> Git as e.g. 'submodule update' always detaches the submodule HEAD.\n>\n> Despite having plans to have submodule HEADS not detached in the future,\n> the current behavior is really bad as it doesn't match user expectations\n> and it is not checking for loss of commits (only to be recovered via the\n> reflog).\n\nI think the corrected behavior doesn't match user expectations,\neither.\n\nCould this patch include some documentation to help users know what to\nexpect?\n\n> Detach the HEAD unconditionally in the submodule when updating it.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> This is a resend of [1], which did not receive any attention.\n\nYikes. Yes, this bug looks problematic. Thanks for working on it.\n\n> I improved the commit message laying out the current state of affairs,\n> arguing that any future plan should not weigh in as much as the current\n> possible data loss.\n\nCan you elaborate on what you mean about data loss? At first glance\nit would seem to me that detaching HEAD could lead to data loss since\nthere isn't a branch to keep track of the user's work. Are you saying\nthe current behavior of updating whatever branch HEAD is on (which,\ndon't get me wrong, is a wrong behavior that needs fixing) bypassed\nthe reflog?\n\nThanks,\nJonathan\n\n> [1] https://public-inbox.org/git/20170630003851.17288-1-sbeller@google.com/\n[...]\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1653,7 +1653,8 @@ int submodule_move_head(const char *path,\n> \t\t\tcp.dir = path;\n> \n> \t\t\tprepare_submodule_repo_env(&cp.env_array);\n> -\t\t\targv_array_pushl(&cp.args, \"update-ref\", \"HEAD\", new, NULL);\n> +\t\t\targv_array_pushl(&cp.args, \"update-ref\", \"HEAD\",\n> +\t\t\t\t\t \"--no-deref\", new, NULL);\n> \n> \t\t\tif (run_command(&cp)) {\n> \t\t\t\tret = -1;\n"},{"id":"324970","messageId":"CAGZ79kaYzHzSdDZuMpM6UFXLon0SME-nC=9_09PLhLEADfuscw@mail.gmail.com","threadId":"46454","inReplyTo":"20170724180312.GE13924@aiede.mtv.corp.google.com","subject":"Re: [PATCH] recursive submodules: detach HEAD from new state","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-24T19:07:48Z","receivedAt":"2017-07-24T19:07:54Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jul 24, 2017 at 11:03 AM, Jonathan Nieder wrote:\n> Hi,\n>\n> Stefan Beller wrote:\n>\n>> When a submodule is on a branch and in its superproject you run a\n>> recursive checkout, the branch of the submodule is updated to what the\n>> superproject checks out. This is very unexpected in the current model of\n>> Git as e.g. 'submodule update' always detaches the submodule HEAD.\n>>\n>> Despite having plans to have submodule HEADS not detached in the future,\n>> the current behavior is really bad as it doesn't match user expectations\n>> and it is not checking for loss of commits (only to be recovered via the\n>> reflog).\n>\n> I think the corrected behavior doesn't match user expectations,\n> either.\n\nWell, what is the user expectation?\n\n>\n> Could this patch include some documentation to help users know what to\n> expect?\n\nSure, once we figured out what is reasonable.\n\n>\n>> Detach the HEAD unconditionally in the submodule when updating it.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>> This is a resend of [1], which did not receive any attention.\n>\n> Yikes. Yes, this bug looks problematic. Thanks for working on it.\n>\n>> I improved the commit message laying out the current state of affairs,\n>> arguing that any future plan should not weigh in as much as the current\n>> possible data loss.\n>\n> Can you elaborate on what you mean about data loss?\n\nAssume we have a submodule 'sub' inside the superproject 'super',\nthen\n\n git -C super/sub checkout \n git -C super checkout \n\nmodifies my-unrelated-branch in the submodule, which is not related\nto the superproject in any way.\n\nThis patch would detach from that branch and have the HEAD contain\nthe desired sha1. To think that further we'd still have potential data loss:\n\n git -C super/sub checkout \n git -C super checkout \n # fine so far as sub is in detached HEAD, but:\n ... hack hack hack ... in 'sub'\n git -C super/sub commit -m \"work\"\n git -C super checkout \n # subs work is only to be recovered via reflog!\n\nHowever this matches the current behavior of\n\"submodule update\" which also tips, that are\nnot reachable from any ref.\n\n> At first glance\n> it would seem to me that detaching HEAD could lead to data loss since\n> there isn't a branch to keep track of the user's work.\n\nyes, but that is the same with \"submodule update\", which is what\npeople may have in mind?\n\n> Are you saying\n> the current behavior of updating whatever branch HEAD is on (which,\n> don't get me wrong, is a wrong behavior that needs fixing) bypassed\n> the reflog?\n\nNo, I am not saying that.\nI am saying that updating an unrelated branch (which is dragged into\nthe affair just because HEAD points at it) is very subtle thing, as any\ncommits on that branch can be considered safe (it is on a branch, right?)\nbut the detached HEAD is the default unsafe mode we currently have.\n\nThanks,\nStefan\n"},{"id":"324999","messageId":"xmqqinihd048.fsf@gitster.mtv.corp.google.com","threadId":"46454","inReplyTo":"CAGZ79kaYzHzSdDZuMpM6UFXLon0SME-nC=9_09PLhLEADfuscw@mail.gmail.com","subject":"Re: [PATCH] recursive submodules: detach HEAD from new state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-24T20:57:11Z","receivedAt":"2017-07-24T20:57:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> Are you saying\n>> the current behavior of updating whatever branch HEAD is on (which,\n>> don't get me wrong, is a wrong behavior that needs fixing) bypassed\n>> the reflog?\n>\n> No, I am not saying that.\n> I am saying that updating an unrelated branch (which is dragged into\n> the affair just because HEAD points at it) is very subtle thing, as any\n> commits on that branch can be considered safe (it is on a branch, right?)\n> but the detached HEAD is the default unsafe mode we currently have.\n\nThen it is not a data loss as you claimed in the proposed log\nmessage, but is something else, no?\n"},{"id":"325003","messageId":"xmqq8tjdcyf1.fsf@gitster.mtv.corp.google.com","threadId":"46454","inReplyTo":"20170724180312.GE13924@aiede.mtv.corp.google.com","subject":"Re: [PATCH] recursive submodules: detach HEAD from new state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-24T21:33:54Z","receivedAt":"2017-07-24T21:34:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Yikes. Yes, this bug looks problematic. Thanks for working on it.\n>\n>> I improved the commit message laying out the current state of affairs,\n>> arguing that any future plan should not weigh in as much as the current\n>> possible data loss.\n>\n> Can you elaborate on what you mean about data loss? At first glance\n> it would seem to me that detaching HEAD could lead to data loss since\n> there isn't a branch to keep track of the user's work. Are you saying\n> the current behavior of updating whatever branch HEAD is on (which,\n> don't get me wrong, is a wrong behavior that needs fixing) bypassed\n> the reflog?\n\nAlso, while I do agree with you that the problem exists, it is\nunclear why this patch is a solution and not a hack that sweeps a\nproblem under the rug. \n\nIt is unclear why this \"silently detach HEAD without telling the\nuser\" is a better solution than erroring out, for example [*1*].\n\n\n[Footnote]\n\n*1* For example, I would imagine that the problem can also be\n \"fixed\" by detecting that the HEAD is on a branch, and noticing\n that it will force rewinding of that branch if we did update-ref\n in this codepath, and signal the failure to the caller of\n submodule_move_head() without making the damage larger. And\n tell the user what is going on, and perhaps suggest to detach\n HEAD to avoid clobbering their branch.\n\n>\n> Thanks, Jonathan\n>\n>> [1] https://public-inbox.org/git/20170630003851.17288-1-sbeller@google.com/\n> [...]\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -1653,7 +1653,8 @@ int submodule_move_head(const char *path,\n>> \t\t\tcp.dir = path;\n>> \n>> \t\t\tprepare_submodule_repo_env(&cp.env_array);\n>> -\t\t\targv_array_pushl(&cp.args, \"update-ref\", \"HEAD\", new, NULL);\n>> +\t\t\targv_array_pushl(&cp.args, \"update-ref\", \"HEAD\",\n>> +\t\t\t\t\t \"--no-deref\", new, NULL);\n>> \n>> \t\t\tif (run_command(&cp)) {\n>> \t\t\t\tret = -1;\n"},{"id":"325012","messageId":"xmqqr2x5bhk7.fsf@gitster.mtv.corp.google.com","threadId":"46454","inReplyTo":"xmqq8tjdcyf1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] recursive submodules: detach HEAD from new state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-24T22:23:20Z","receivedAt":"2017-07-24T22:23:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Also, while I do agree with you that the problem exists, it is\n> unclear why this patch is a solution and not a hack that sweeps a\n> problem under the rug. \n>\n> It is unclear why this \"silently detach HEAD without telling the\n> user\" is a better solution than erroring out, for example [*1*].\n\nJust to avoid possible confusion; I am not claiming that it would be\nmore (or less for that matter) sensible to error out than silently\ndetaching HEAD, because I am not giving the reason to substantiate\nthe claim and I do not have a strong opinion to favour which one (or\nanother potential solution, if any). \n\nI am just saying that the patch that proposes a solution should be\nbacked with an explanation why it is a good idea, especially when\nthere are obvious alternatives that are not so clearly inferior.\n\nThanks.\n"},{"id":"325097","messageId":"CAGZ79kZdoktBRBuNxVk-zehZR3Z-egEPG81KQ9WqHTEtrm+5uw@mail.gmail.com","threadId":"46454","inReplyTo":"xmqqr2x5bhk7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] recursive submodules: detach HEAD from new state","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-25T22:27:07Z","receivedAt":"2017-07-25T22:27:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jul 24, 2017 at 3:23 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Also, while I do agree with you that the problem exists, it is\n>> unclear why this patch is a solution and not a hack that sweeps a\n>> problem under the rug.\n>>\n>> It is unclear why this \"silently detach HEAD without telling the\n>> user\" is a better solution than erroring out, for example [*1*].\n>\n> Just to avoid possible confusion; I am not claiming that it would be\n> more (or less for that matter) sensible to error out than silently\n> detaching HEAD, because I am not giving the reason to substantiate\n> the claim and I do not have a strong opinion to favour which one (or\n> another potential solution, if any).\n>\n> I am just saying that the patch that proposes a solution should be\n> backed with an explanation why it is a good idea, especially when\n> there are obvious alternatives that are not so clearly inferior.\n>\n> Thanks.\n\nSo I took a step back and wrote about different proposals where\nwe want to go long term. See below. This will help us\nfiguring out how to approach this bug correctly.\n------\n\n\n\nRFC: A new type of symbolic refs\n\nA symbolic ref can currently only point at a ref or another symbolic ref.\nThis proposal show cases different scenarios on how this could change in\nthe future.\n\n\n\nA: HEAD pointing at the superprojects index\n===========================================\n\nIntroduce a new symbolic ref that points at the superprojects index of\nthe gitlink. The format is\n\n \"repo:\" '\\0' '\\0'\n\nRef read operations\n-------------------\n e.g. git log HEAD\n\nJust like existing symrefs, the content of the ref will be read and followed.\nOn reading \"repo:\", the sha1 will be obtained equivalent to:\n\n git -C ls-files -s | awk '{ print $2}'\n\nIn case of error\n(superproject not found, gitlink path does not exist), the ref is broken and\n\nRef write operations driven by the submodule, affecting symrefs\n---------------------------------------------------------------\n e.g. git checkout (in the submodule)\n\nIn this scenario only the HEAD is optionally attached to the superproject,\nso we can rewrite the HEAD to be anything else, such as a branch just fine.\nOnce the HEAD is not pointing at the superproject any more, we'll leave the\nsubmodule alone in operations driven by the superproject.\n\nRef write operations driven by the submodule, affecting target ref\n------------------------------------------------------------------\n e.g. git commit, reset --hard, update-ref (in the submodule)\n\nThe HEAD stays the same, pointing at the superproject.\nThe gitlink is changed to the target sha1, using\n\n git -C update-index --add \\\n --cacheinfo 160000,$SHA1,\n\nThis will affect the superprojects index, such that then a commit in\nthe superproject is needed.\n\nRef write operations driven by the superproject, changing the gitlink\n---------------------------------------------------------------------\n e.g. git checkout , git reset --hard (in the superproject)\n\nThis will change the gitlink in the superprojects index, such that the HEAD\nin the submodule changes, which would trigger an update of the\nsubmodules working tree.\n\nConsistency considerations (gc)\n-------------------------------\n e.g. git gc --aggressive --prune=now\n\nThe repacking logic is already aware of a detached HEAD, such that\nusing this new symref mechanism would not generate problems as long as\nwe keep the HEAD attached to the superproject. However when commits/objects\nare created while the HEAD is attached to the superproject and then HEAD\nswitches to a local branch, there are problems with the created objects\nas they seem unreachable now.\n\nThis problem is not new as a superproject may record submodule objects\nthat are not reachable from any of the submodule branches. Such objects\nfall prey to overzealous packing in the submodule.\n\nThis proposal however exposes this problem a lot more, as the submodule\nhas fewer needs for branches.\n\n\n\n\nB: HEAD pointing at a superprojects branch\n==========================================\n\nInstead of pointing at the index of the superproject, we also\nencode a branch name:\n\n repo:\" '\\0' '\\0' branch '\\0'\n\nRef read operations\n-------------------\n e.g. git log HEAD\n\nThis is similar to the case of pointing at the index, except that the reading\noperation reads from the tip of the branch:\n\n git -C ls-tree -- \\\n | awk '{ print $3}'\n\nRef write operations driven by the submodule, affecting symrefs\n---------------------------------------------------------------\n e.g. git checkout (in the submodule)\n\nHEAD will be pointed at the local target branch, dropping the affliation to\nthe superproject.\n\nRef write operations driven by the submodule, affecting target ref\n------------------------------------------------------------------\n e.g. git commit, reset --hard, update-ref (in the submodule)\n\nAs we're pointing at the superprojects branch, this would have to create\na dummy(?) commit in the superproject, that just changes the submodule\npointer in the superprojects branch, such that the operation of storing\na new sha1 for the submodule is equivalent to\n\n git -C update-index --add \\\n --cacheinfo 160000,$SHA1,\n git -C commit -m \"Update submodule\"\n\nThis behavior in the superproject is similar to Gerrits subscription model\nwhere superprojects are updated from the submodule.\n\nEach operation in the submodule triggers a local superproject commit.\n\nRef write operations driven by the superproject, changing the gitlink\n---------------------------------------------------------------------\n e.g. git merge, git pull (in the superproject)\n\nThis will change the gitlink in the superprojects index, such that the HEAD\nin the submodule changes, which would trigger an update of the\nsubmodules working tree.\n\nThis would require a good merge strategy for submodules, i.e. on merge\nthe submodule would create a merge commit that is recorded in the\nsuperprojects merge commit.\n\nConsistency considerations (gc)\n-------------------------------\n e.g. git gc --aggressive --prune=now\n\nThe repacking problem comes with a solution unlike the previous proposal.\nThis is because any relevant commit in the submodule is recorded in the\nsuperproject via a commit in a branch. Then even non-fast-forward histories\nin the submodule can all be kept by walking the superproject and looking at\nall gitlink entries of the submodule.\n\n\n\nC: All branches are symbolic references to the superproject\n===========================================================\n\nInstead of having just HEAD pointed at a superproject, all(!) branches\nin the submodule point at the superprojects branch of the same name.\nSymbolic refs that resolve to a local sha1 are not allowed, any symbolic ref\nends up pointing at the superproject eventually.\ne.g. HEAD points at a submodule branch, which in turn points at\nthe superproject branch of the same name.\n\nRef read operations\n-------------------\n e.g. git log\n\nHEAD is read, which may be either (a) locally detached or (b) pointing at a\nsuperproject branch. Resolve as in B.\n\nRef write operations driven by the submodule, affecting symrefs\n---------------------------------------------------------------\n e.g. git checkout (in the submodule)\n\nAs there is no other local branch, HEAD would point at the other submodule\nbranch, which then points at another branch in the superproject.\n\nRef write operations driven by the submodule, affecting target ref\n------------------------------------------------------------------\n e.g. git commit, reset --hard, update-ref (in the submodule)\n\n same as B.\n\nRef write operations driven by the superproject, changing the gitlink\n---------------------------------------------------------------------\n e.g. git merge, git pull (in the superproject)\n\n same as B.\n\nConsistency considerations (gc)\n-------------------------------\n e.g. git gc --aggressive --prune=now\n\nAs the superproject contains all knowledge, the gc starts with a\nwalk of all superproject branches, destilling the recorded gitlink entries\nand then starts walking in the submodule from all the recorded gitlinks\nto create a pack.\n\ngc and repacking would either be forbidden in the submodule or deflected\nto the superproject.\n"},{"id":"325144","messageId":"xmqqy3rbq9c4.fsf@gitster.mtv.corp.google.com","threadId":"46454","inReplyTo":"CAGZ79kZdoktBRBuNxVk-zehZR3Z-egEPG81KQ9WqHTEtrm+5uw@mail.gmail.com","subject":"Re: [PATCH] recursive submodules: detach HEAD from new state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-26T19:36:27Z","receivedAt":"2017-07-26T19:36:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> So I took a step back and wrote about different proposals where\n> we want to go long term. See below. This will help us\n> figuring out how to approach this bug correctly.\n\nThanks for writing this.\n\n> RFC: A new type of symbolic refs\n>\n> A symbolic ref can currently only point at a ref or another symbolic ref.\n> This proposal show cases different scenarios on how this could change in\n> the future.\n>\n>\n>\n> A: HEAD pointing at the superprojects index\n> ===========================================\n>\n> Introduce a new symbolic ref that points at the superprojects index of\n> the gitlink. The format is\n>\n> \"repo:\" '\\0' '\\0'\n>\n> Ref read operations\n> -------------------\n> e.g. git log HEAD\n>\n> Just like existing symrefs, the content of the ref will be read and followed.\n> On reading \"repo:\", the sha1 will be obtained equivalent to:\n>\n> git -C ls-files -s | awk '{ print $2}'\n>\n> In case of error\n> (superproject not found, gitlink path does not exist), the ref is broken and\n>\n> Ref write operations driven by the submodule, affecting symrefs\n> ---------------------------------------------------------------\n> e.g. git checkout (in the submodule)\n>\n> In this scenario only the HEAD is optionally attached to the superproject,\n> so we can rewrite the HEAD to be anything else, such as a branch just fine.\n> Once the HEAD is not pointing at the superproject any more, we'll leave the\n> submodule alone in operations driven by the superproject.\n\nThat explains what the proposed code _does_. It does not explain\nwhy the chosen behaviour is a sensible one.\n\nThis illustrates the point I have trouble with when trying to judge\nall of these discrete update proposals to submodules. They only say\n\"This feature does this in this case, does that in that case,...\"\nbut lack \"this is meant to be used when you want to implement the\nworkflow that goes like this, and fits as a building block at this\npoint for that workflow. Other elements needed to support that\nworkflow well are ...\". No proposal gives a big picture and explain\nhow these small bits fit together.\n\nFor example, I would understand better if this write-up of yours\nwere not organized with the \"proposal X added A that behaves this\nway and added B that behaves that way\" as its major axis, but\ninstead was written with the workflow that is meant to be realized\nas its major axis, e.g.\n\n A project may want to use submodules as if it is just part of\n superproject. In such a project, checking out branch X at the\n superproject level, working on files in both superproject and\n submodules, and then committing recursively and pushing the\n results out recursively at the superproject level, all would\n want to affect the same branch X at all levels in the upstream.\n\nmay be one possible workflow you want to support. As one ingredient\nto support such structure, the HEAD in the submodule that points at\nan index entry in the superproject may be very useful. After a\nrecursive checkout at the superproject level, the HEAD of the\nsubmodule ought to be what came from and recorded in the tree in the\nsuperproject, and after a commit in the submodule, the HEAD moves to\nthe new commit and the entry in the superproject's index also gets\nupdated which would have a nice property that \"commit\" in submodule\nacts almost like \"add\" in superproject. A recursive \"git diff\" would\nshow that submodule is clean after such a commit, recursive \"push\"\nwould know which branch to push out, etc.\n\nAnd when operating in such a mode, it would make most sense if \"git\ncheckout\" of a different branch Y in a submodule repository is\neither forbidden, or should behave as if the submodule directory\nwere an ordinary directory of the superproject (i.e. causing\nrecursive checkout of the branch Y at the superproject level).\n\nBUT.\n\nBecause none of the proposals paint a big picture (e.g. the big\npicture the above hypothetical example gives is that the core\nconcept of this particular workflow being supported is that\neverything recursively stays on the branch with the same name), we\ncannot judge if it is sensible for \"a new style symref\" to be\nupdated/demoted to a normal branch pointer when \"git checkout\"\nhappens. It is not sensible in such a hypothetical workflow, but it\nmay be very sensible in another workflow. Without stating what\nbig-picture goal is being achieved, it is impossible to see if a\nproposal to add/change an individual component that is to be used as\na building block makes sense.\n\nHistorically, we can get away without giving choices of \"supported\nworkflows\", allowing the user to pick one, and explaining how things\nfit together, primarily because the operations that can recurse were\nprimarily read-only e.g. status, grep, etc., and the supported model\nwas \"the user can be on whatever branch or detached in each\nsubmodule that may or may not be consistent with what happens in the\nsuperproject; it is up to the user to hang themselves with the long\nrope\". When allowing potentially destructive operations like\ncheckout to go recursive [*1*], depending on how the entire tree of\nrepositories is meant to be managed, sensible mode of operation\nwould be different, but without defining what various ways \"how the\nentire tree of repositories is meant to be managed\" are supported,\nwe cannot teach such operations to go recursive in a sensible way.\n\n[Footnote]\n\n*1* Some readers may wonder \"checkout is destructive???\", and in the\n context of this discussion, it is. Recursive checkout done at\n the superproject level that rewinds the branch currently checked\n out in a submodule is destructive by potentially losing history,\n and a recursive checkout that checks out a different branch in a\n submodule can be destructive by changing where the next \"git push\"\n in a submodule would go, depending on how the entire tree of\n repositories is meant to be managed. In some workflows, always\n detaching HEAD to the commit that is bound to the superproject\n may be _the_ sensible way to recursively check out a branch. In\n some other workflows, detecting that the submodule is on a\n branch that is not the branch the superproject is checking out\n and erroring out may be more sensible way.\n"},{"id":"333179","messageId":"20171121223449.GI3429@aiede.mtv.corp.google.com","threadId":"46454","inReplyTo":"CAGZ79kZdoktBRBuNxVk-zehZR3Z-egEPG81KQ9WqHTEtrm+5uw@mail.gmail.com","subject":"Re: [PATCH] recursive submodules: detach HEAD from new state","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-21T22:34:49Z","receivedAt":"2017-11-21T22:35:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n>> Junio C Hamano writes:\n\n>>> Also, while I do agree with you that the problem exists, it is\n>>> unclear why this patch is a solution and not a hack that sweeps a\n>>> problem under the rug.\n>>>\n>>> It is unclear why this \"silently detach HEAD without telling the\n>>> user\" is a better solution than erroring out, for example [*1*].\n[...]\n> So I took a step back and wrote about different proposals where\n> we want to go long term. See below. This will help us\n> figuring out how to approach this bug correctly.\n\nStefan, do you know what thread I should look at to find the current\nstate of this patch? I've had it applied locally for a long time.\n\nThe thread I am replying to appears to be where the patch comes from\nbut I have some memories of more recent discussion that I'm not\nfinding.\n\nMore context:\nhttps://public-inbox.org/git/xmqqshd8i3ze.fsf@gitster.mtv.corp.google.com/\nsays\n\n * sb/submodule-recursive-checkout-detach-head (2017-07-28) 2 commits\n - Documentation/checkout: clarify submodule HEADs to be detached\n - recursive submodules: detach HEAD from new state\n\n \"git checkout --recursive\" may overwrite and rewind the history of\n the branch that happens to be checked out in submodule\n repositories, which might not be desirable. Detach the HEAD but\n still allow the recursive checkout to succeed in such a case.\n\n Expecting a reroll.\n\nThanks,\nJonathan\n"},{"id":"333180","messageId":"CAGZ79kZxD4r0J+uZCuBStkZq1mqPSTaOdkpyOmPXjdLLr6rkOQ@mail.gmail.com","threadId":"46454","inReplyTo":"20171121223449.GI3429@aiede.mtv.corp.google.com","subject":"Re: [PATCH] recursive submodules: detach HEAD from new state","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-21T22:45:30Z","receivedAt":"2017-11-21T22:45:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 21, 2017 at 2:34 PM, Jonathan Nieder wrote:\n> Stefan Beller wrote:\n>>> Junio C Hamano writes:\n>\n>>>> Also, while I do agree with you that the problem exists, it is\n>>>> unclear why this patch is a solution and not a hack that sweeps a\n>>>> problem under the rug.\n>>>>\n>>>> It is unclear why this \"silently detach HEAD without telling the\n>>>> user\" is a better solution than erroring out, for example [*1*].\n> [...]\n>> So I took a step back and wrote about different proposals where\n>> we want to go long term. See below. This will help us\n>> figuring out how to approach this bug correctly.\n>\n> Stefan, do you know what thread I should look at to find the current\n> state of this patch? I've had it applied locally for a long time.\n\nIt was \"Undecided\" for some time, then Junio kicked it back to pu, expecting a\nreroll[1]. The \"send out a plan\" that was referenced is found in [2]\ndescribing 6\nplans for the future of submodules. The approach in [3] which is\ndifferent on the\nimplementation level, but very similar on the UX level sounds best currently.\nI'll coordinate with JTan to come up with patches for that.\n\n[1] https://public-inbox.org/git/CAGZ79kYUZv0g+3OEMrbT26A7mSLJzeS-yf5Knr-CnARHqVB=aQ@mail.gmail.com/\n[2] https://public-inbox.org/git/20171109001007.11894-1-sbeller@google.com/\n[3] https://public-inbox.org/git/20171108172945.33c42a0e91b4ac494217b788@google.com/\n\n> The thread I am replying to appears to be where the patch comes from\n> but I have some memories of more recent discussion that I'm not\n> finding.\n>\n> More context:\n> https://public-inbox.org/git/xmqqshd8i3ze.fsf@gitster.mtv.corp.google.com/\n> says\n>\n> * sb/submodule-recursive-checkout-detach-head (2017-07-28) 2 commits\n> - Documentation/checkout: clarify submodule HEADs to be detached\n> - recursive submodules: detach HEAD from new state\n>\n> \"git checkout --recursive\" may overwrite and rewind the history of\n> the branch that happens to be checked out in submodule\n> repositories, which might not be desirable. Detach the HEAD but\n> still allow the recursive checkout to succeed in such a case.\n>\n> Expecting a reroll.\n>\n> Thanks,\n> Jonathan\n"},{"id":"333181","messageId":"20171121224709.GJ3429@aiede.mtv.corp.google.com","threadId":"46454","inReplyTo":"CAGZ79kZxD4r0J+uZCuBStkZq1mqPSTaOdkpyOmPXjdLLr6rkOQ@mail.gmail.com","subject":"Re: [PATCH] recursive submodules: detach HEAD from new state","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-21T22:47:09Z","receivedAt":"2017-11-21T22:47:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n> On Tue, Nov 21, 2017 at 2:34 PM, Jonathan Nieder wrote:\n\n>> Stefan, do you know what thread I should look at to find the current\n>> state of this patch? I've had it applied locally for a long time.\n>\n> It was \"Undecided\" for some time, then Junio kicked it back to pu, expecting a\n> reroll[1]. The \"send out a plan\" that was referenced is found in [2]\n> describing 6 plans for the future of submodules. The approach in [3]\n> which is different on the implementation level, but very similar on\n> the UX level sounds best currently. I'll coordinate with JTan to\n> come up with patches for that.\n>\n> [1] https://public-inbox.org/git/CAGZ79kYUZv0g+3OEMrbT26A7mSLJzeS-yf5Knr-CnARHqVB=aQ@mail.gmail.com/\n> [2] https://public-inbox.org/git/20171109001007.11894-1-sbeller@google.com/\n> [3] https://public-inbox.org/git/20171108172945.33c42a0e91b4ac494217b788@google.com/\n\nThanks. That thread appears to be about a long-term plan; what is the\nshort-term plan?\n\nE.g. is there any additional documentation that should be added to the\npatch that detaches?\n\nOr should it go in as-is?\n\nThanks,\nJonathan\n"},{"id":"333183","messageId":"CAGZ79kbNwV5dg2raUu4SZRWsocpXr7xtgDuyyQ+gVcPFQWfKjg@mail.gmail.com","threadId":"46454","inReplyTo":"20171121224709.GJ3429@aiede.mtv.corp.google.com","subject":"Re: [PATCH] recursive submodules: detach HEAD from new state","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-11-21T23:00:08Z","receivedAt":"2017-11-21T23:00:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 21, 2017 at 2:47 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> Stefan Beller wrote:\n>> On Tue, Nov 21, 2017 at 2:34 PM, Jonathan Nieder wrote:\n>\n>>> Stefan, do you know what thread I should look at to find the current\n>>> state of this patch? I've had it applied locally for a long time.\n>>\n>> It was \"Undecided\" for some time, then Junio kicked it back to pu, expecting a\n>> reroll[1]. The \"send out a plan\" that was referenced is found in [2]\n>> describing 6 plans for the future of submodules. The approach in [3]\n>> which is different on the implementation level, but very similar on\n>> the UX level sounds best currently. I'll coordinate with JTan to\n>> come up with patches for that.\n>>\n>> [1] https://public-inbox.org/git/CAGZ79kYUZv0g+3OEMrbT26A7mSLJzeS-yf5Knr-CnARHqVB=aQ@mail.gmail.com/\n>> [2] https://public-inbox.org/git/20171109001007.11894-1-sbeller@google.com/\n>> [3] https://public-inbox.org/git/20171108172945.33c42a0e91b4ac494217b788@google.com/\n>\n> Thanks. That thread appears to be about a long-term plan; what is the\n> short-term plan?\n>\n> E.g. is there any additional documentation that should be added to the\n> patch that detaches?\n\nThe second patch in that series has a tiny bit of information, see\n\"Documentation/checkout: clarify submodule HEADs to be detached\".\n\nI would think that this is sufficient for the short term to get into a\nsafe state.\n\n> Or should it go in as-is?\n\nThat is what I would prefer and then we'll build on top of this once we\nfigured out the direction of the long term solution.\n\nThanks,\nStefan\n"},{"id":"333205","messageId":"xmqqr2sryvyi.fsf@gitster.mtv.corp.google.com","threadId":"46454","inReplyTo":"20171121223449.GI3429@aiede.mtv.corp.google.com","subject":"Re: [PATCH] recursive submodules: detach HEAD from new state","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-22T00:54:13Z","receivedAt":"2017-11-22T00:54:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> The thread I am replying to appears to be where the patch comes from\n> but I have some memories of more recent discussion that I'm not\n> finding.\n>\n> More context:\n> https://public-inbox.org/git/xmqqshd8i3ze.fsf@gitster.mtv.corp.google.com/\n> says\n>\n> * sb/submodule-recursive-checkout-detach-head (2017-07-28) 2 commits\n> - Documentation/checkout: clarify submodule HEADs to be detached\n> - recursive submodules: detach HEAD from new state\n>\n> \"git checkout --recursive\" may overwrite and rewind the history of\n> the branch that happens to be checked out in submodule\n> repositories, which might not be desirable. Detach the HEAD but\n> still allow the recursive checkout to succeed in such a case.\n>\n> Expecting a reroll.\n\nSorry, I should have done my usual \"cf. \" to help me\nrecalling the discussion that lead to the marking there.\n\nWe kicked it back to 'pu' after the discussion that had\n, and the \"send out a\nplan\" sort-of happened with <20171109001007.11894-1-sbeller@google.com>\nwhich seemed to have converged to a conclusion in the subthread with\n\nwhere a preferred way would be to detach and opportunistically reattach\nto keep the illusion of submodule being on a branch as much as possible\n(correct me if I am misunderstanding the consensus). \n\nI had a suspicion that such a random re-attachment may lead to an\nunpredictable behaviour from end-user's point of view that is\nconfusing, but there wasn't a concrete patch to do so, so that was\nwhy I was waiting for a reroll so that people can take a look at it\nand see how well it fares.\n\nThe responses in this thread seems to indicate that now we are\npunting on that re-attachment plan and want to give this \"always\ndetach\" to the end users, which I think is a lot more predictable\nthing to do. After such a recursive checkout from the top-level,\nany work done in the submodule from that state and is referenced\nfrom the top-level (recorded presumably with a recursive commit) is\nnot referenced by anything other than the reflog of HEAD in the\nsubmodule repository, and I suspect many users are not used to and\nare comfortable with working on a detached HEAD for extended period\nof time, so this new behaviour might deserve a stronger warning to\nhelp them, but I am OK with the stance \"We'll learn as we go---let's\nmerge it as-is and see what happens\".\n\nThanks for prodding. One less topic that is stale but has to be\ncarried around in 'pu' is always a good thing ;-)\n\n\n\n"}]}