{"thread":{"id":"49973","subject":"[PATCH 0/4]","startedAt":"2018-12-07T23:54:40Z","lastAt":"2018-12-28T20:12:20Z","messageCount":21,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"364774","messageId":"20181207235425.128568-1-sbeller@google.com","threadId":"49973","inReplyTo":null,"subject":"[PATCH 0/4]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-07T23:54:21Z","receivedAt":"2018-12-07T23:54:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"A couple days before the 2.19 release we had a bug report about\nbroken submodules[1] and reverted[2] the commits leading up to them.\n\nThe behavior of said bug fixed itself by taking a different approach[3],\nspecifically by a weaker enforcement of having `core.worktree` set in a\nsubmodule [4].\n\nThe revert [2] was overly broad as we neared the release, such that we wanted\nto rather keep the known buggy behavior of always having `core.worktree` set,\nrather than figuring out how to fix the new bug of having 'git submodule update'\nnot working in old style repository setups.\n\nThis series re-introduces those reverted patches, with no changes in code,\nbut with drastically changed commit messages, as those focus on why it is safe\nto re-introduce them instead of explaining the desire for the change.\n\n[1] https://public-inbox.org/git/2659750.rG6xLiZASK@twilight\n[2] f178c13fda (Revert \"Merge branch 'sb/submodule-core-worktree'\", 2018-09-07)\n[3] 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17)\n[4] 74d4731da1 (submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13)\n\nStefan Beller (4):\n submodule update: add regression test with old style setups\n submodule: unset core.worktree if no working tree is present\n submodule--helper: fix BUG message in ensure_core_worktree\n submodule deinit: unset core.worktree\n\n builtin/submodule--helper.c | 4 +++-\n submodule.c | 14 ++++++++++++++\n submodule.h | 2 ++\n t/lib-submodule-update.sh | 5 +++--\n t/t7400-submodule-basic.sh | 5 +++++\n t/t7412-submodule-absorbgitdirs.sh | 7 ++++++-\n 6 files changed, 33 insertions(+), 4 deletions(-)\n\n-- \n2.20.0.rc2.403.gdbc3b29805-goog\n\n"},{"id":"364775","messageId":"20181207235425.128568-2-sbeller@google.com","threadId":"49973","inReplyTo":"20181207235425.128568-1-sbeller@google.com","subject":"[PATCH 1/4] submodule update: add regression test with old style setups","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-07T23:54:22Z","receivedAt":"2018-12-07T23:54:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"As f178c13fda (Revert \"Merge branch 'sb/submodule-core-worktree'\",\n2018-09-07) was produced shortly before a release, nobody asked for\na regression test to be included. Add a regression test that makes sure\nthat the invocation of `git submodule update` on old setups doesn't\nproduce errors as pointed out in f178c13fda.\n\nThe place to add such a regression test may look odd in t7412, but\nthat is the best place as there we setup old style submodule setups\nexplicitly.\n\nSigned-off-by: Stefan Beller \n---\n t/t7412-submodule-absorbgitdirs.sh | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\nindex ce74c12da2..1cfa150768 100755\n--- a/t/t7412-submodule-absorbgitdirs.sh\n+++ b/t/t7412-submodule-absorbgitdirs.sh\n@@ -75,7 +75,12 @@ test_expect_success 're-setup nested submodule' '\n \tGIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \\\n \t\tcore.worktree \"../../../nested\" &&\n \t# make sure this re-setup is correct\n-\tgit status --ignore-submodules=none\n+\tgit status --ignore-submodules=none &&\n+\n+\t# also make sure this old setup does not regress\n+\tgit submodule update --init --recursive >out 2>err &&\n+\ttest_must_be_empty out &&\n+\ttest_must_be_empty err\n '\n \n test_expect_success 'absorb the git dir in a nested submodule' '\n-- \n2.20.0.rc2.403.gdbc3b29805-goog\n\n"},{"id":"364776","messageId":"20181207235425.128568-3-sbeller@google.com","threadId":"49973","inReplyTo":"20181207235425.128568-1-sbeller@google.com","subject":"[PATCH 2/4] submodule: unset core.worktree if no working tree is present","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-07T23:54:23Z","receivedAt":"2018-12-07T23:54:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This reintroduces 4fa4f90ccd (submodule: unset core.worktree if no working\ntree is present, 2018-06-12), which was reverted as part of f178c13fda\n(Revert \"Merge branch 'sb/submodule-core-worktree'\", 2018-09-07).\n\n4fa4f90ccd was reverted as its followup commit was faulty, but without\nthe accompanying change of the followup, we'd have an incomplete workflow\nof setting `core.worktree` again, when it is needed such as checking out\na revision that contains a submodule.\n\nSo re-introduce that commit as-is, focusing on fixing up the followup\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \nSigned-off-by: Stefan Beller \n---\n submodule.c | 14 ++++++++++++++\n submodule.h | 2 ++\n t/lib-submodule-update.sh | 3 ++-\n 3 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 6415cc5580..d393e947e6 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1561,6 +1561,18 @@ int bad_to_remove_submodule(const char *path, unsigned flags)\n \treturn ret;\n }\n \n+void submodule_unset_core_worktree(const struct submodule *sub)\n+{\n+\tchar *config_path = xstrfmt(\"%s/modules/%s/config\",\n+\t\t\t\t get_git_common_dir(), sub->name);\n+\n+\tif (git_config_set_in_file_gently(config_path, \"core.worktree\", NULL))\n+\t\twarning(_(\"Could not unset core.worktree setting in submodule '%s'\"),\n+\t\t\t sub->path);\n+\n+\tfree(config_path);\n+}\n+\n static const char *get_super_prefix_or_empty(void)\n {\n \tconst char *s = get_super_prefix();\n@@ -1726,6 +1738,8 @@ int submodule_move_head(const char *path,\n \n \t\t\tif (is_empty_dir(path))\n \t\t\t\trmdir_or_warn(path);\n+\n+\t\t\tsubmodule_unset_core_worktree(sub);\n \t\t}\n \t}\n out:\ndiff --git a/submodule.h b/submodule.h\nindex a680214c01..9e18e9b807 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -131,6 +131,8 @@ int submodule_move_head(const char *path,\n \t\t\tconst char *new_head,\n \t\t\tunsigned flags);\n \n+void submodule_unset_core_worktree(const struct submodule *sub);\n+\n /*\n * Prepare the \"env_array\" parameter of a \"struct child_process\" for executing\n * a submodule by clearing any repo-specific environment variables, but\ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex 016391723c..51d4555549 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -709,7 +709,8 @@ test_submodule_recursing_with_args_common() {\n \t\t\tgit branch -t remove_sub1 origin/remove_sub1 &&\n \t\t\t$command remove_sub1 &&\n \t\t\ttest_superproject_content origin/remove_sub1 &&\n-\t\t\t! test -e sub1\n+\t\t\t! test -e sub1 &&\n+\t\t\ttest_must_fail git config -f .git/modules/sub1/config core.worktree\n \t\t)\n \t'\n \t# ... absorbing a .git directory along the way.\n-- \n2.20.0.rc2.403.gdbc3b29805-goog\n\n"},{"id":"364777","messageId":"20181207235425.128568-4-sbeller@google.com","threadId":"49973","inReplyTo":"20181207235425.128568-1-sbeller@google.com","subject":"[PATCH 3/4] submodule--helper: fix BUG message in ensure_core_worktree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-07T23:54:24Z","receivedAt":"2018-12-07T23:54:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Shortly after f178c13fda (Revert \"Merge branch\n'sb/submodule-core-worktree'\", 2018-09-07), we had another series\nthat implemented partially the same, ensuring that core.worktree was\nset in a checked out submodule, namely 74d4731da1 (submodule--helper:\nreplace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13)\n\nAs the series 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c',\n2018-09-17) has different goals than the reverted series 7e25437d35\n(Merge branch 'sb/submodule-core-worktree', 2018-07-18), I'd wanted to\nreplay the series on top of it to reach the goal of having `core.worktree`\ncorrectly set when the submodules worktree is present, and unset when the\nworktree is not present.\n\nThe replay resulted in a strange merge conflict highlighting that\nthe BUG message was not changed in 74d4731da1 (submodule--helper:\nreplace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13).\n\nFix the error message.\n\nSigned-off-by: Stefan Beller \n---\n builtin/submodule--helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex d38113a31a..31ac30cf2f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2045,7 +2045,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)\n \tstruct repository subrepo;\n \n \tif (argc != 2)\n-\t\tBUG(\"submodule--helper connect-gitdir-workingtree \");\n+\t\tBUG(\"submodule--helper ensure-core-worktree \");\n \n \tpath = argv[1];\n \n-- \n2.20.0.rc2.403.gdbc3b29805-goog\n\n"},{"id":"364778","messageId":"20181207235425.128568-5-sbeller@google.com","threadId":"49973","inReplyTo":"20181207235425.128568-1-sbeller@google.com","subject":"[PATCH 4/4] submodule deinit: unset core.worktree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-07T23:54:25Z","receivedAt":"2018-12-07T23:54:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This re-introduces 984cd77ddb (submodule deinit: unset core.worktree,\n2018-06-18), which was reverted as part of f178c13fda (Revert \"Merge\nbranch 'sb/submodule-core-worktree'\", 2018-09-07)\n\nThe whole series was reverted as the offending commit e98317508c\n(submodule: ensure core.worktree is set after update, 2018-06-18)\nwas relied on by other commits such as 984cd77ddb.\n\nKeep the offending commit reverted, but its functionality came back via\n4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17), such\nthat we can reintroduce 984cd77ddb now.\n\nSigned-off-by: Stefan Beller \n---\n builtin/submodule--helper.c | 2 ++\n t/lib-submodule-update.sh | 2 +-\n t/t7400-submodule-basic.sh | 5 +++++\n 3 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 31ac30cf2f..672b74db89 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1131,6 +1131,8 @@ static void deinit_submodule(const char *path, const char *prefix,\n \t\tif (!(flags & OPT_QUIET))\n \t\t\tprintf(format, displaypath);\n \n+\t\tsubmodule_unset_core_worktree(sub);\n+\n \t\tstrbuf_release(&sb_rm);\n \t}\n \ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex 51d4555549..5b56b23166 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -235,7 +235,7 @@ reset_work_tree_to_interested () {\n \tthen\n \t\tmkdir -p submodule_update/.git/modules/sub1/modules &&\n \t\tcp -r submodule_update_repo/.git/modules/sub1/modules/sub2 submodule_update/.git/modules/sub1/modules/sub2\n-\t\tGIT_WORK_TREE=. git -C submodule_update/.git/modules/sub1/modules/sub2 config --unset core.worktree\n+\t\t# core.worktree is unset for sub2 as it is not checked out\n \tfi &&\n \t# indicate we are interested in the submodule:\n \tgit -C submodule_update config submodule.sub1.url \"bogus\" &&\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 76a7cb0af7..aba2d4d6ee 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -984,6 +984,11 @@ test_expect_success 'submodule deinit should remove the whole submodule section\n \trmdir init\n '\n \n+test_expect_success 'submodule deinit should unset core.worktree' '\n+\ttest_path_is_file .git/modules/example/config &&\n+\ttest_must_fail git config -f .git/modules/example/config core.worktree\n+'\n+\n test_expect_success 'submodule deinit from subdirectory' '\n \tgit submodule update --init &&\n \tgit config submodule.example.foo bar &&\n-- \n2.20.0.rc2.403.gdbc3b29805-goog\n\n"},{"id":"364789","messageId":"xmqqefas8ss4.fsf@gitster-ct.c.googlers.com","threadId":"49973","inReplyTo":"20181207235425.128568-1-sbeller@google.com","subject":"Re: [PATCH 0/4]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-08T05:57:31Z","receivedAt":"2018-12-08T05:57:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> A couple days before the 2.19 release we had a bug report about\n> broken submodules[1] and reverted[2] the commits leading up to them.\n>\n> The behavior of said bug fixed itself by taking a different approach[3],\n> specifically by a weaker enforcement of having `core.worktree` set in a\n> submodule [4].\n>\n> The revert [2] was overly broad as we neared the release, such that we wanted\n> to rather keep the known buggy behavior of always having `core.worktree` set,\n> rather than figuring out how to fix the new bug of having 'git submodule update'\n> not working in old style repository setups.\n>\n> This series re-introduces those reverted patches, with no changes in code,\n> but with drastically changed commit messages, as those focus on why it is safe\n> to re-introduce them instead of explaining the desire for the change.\n\nThe above was a bit too cryptic for me to grok, so let me try\nrephrasing to see if I got them all correctly.\n\n - three-patch series leading to 984cd77ddb were meant to fix some\n bug, but the series itself was buggy and caused problems; we got\n rid of them\n\n - the problem 984cd77ddb wanted to fix was fixed differently\n without reintroducing the problem three-patch series introduced.\n That fix is already with us since 4d6d6ef1fc.\n\n - now these three changes that were problematic in the past is\n resent without any update (other than that it has one preparatory\n patch to add tests).\n\nIs that what is going on? Obviously I am not getting \"the other\"\nbenefit we wanted to gain out of these three patches (because the\nabove description fails to explain what that is), other than to fix\nthe issue that was fixed by 4d6d6ef1fc.\n\nSorry for being puzzled...\n\n> [1] https://public-inbox.org/git/2659750.rG6xLiZASK@twilight\n> [2] f178c13fda (Revert \"Merge branch 'sb/submodule-core-worktree'\", 2018-09-07)\n> [3] 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17)\n> [4] 74d4731da1 (submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13)\n>\n> Stefan Beller (4):\n> submodule update: add regression test with old style setups\n> submodule: unset core.worktree if no working tree is present\n> submodule--helper: fix BUG message in ensure_core_worktree\n> submodule deinit: unset core.worktree\n>\n> builtin/submodule--helper.c | 4 +++-\n> submodule.c | 14 ++++++++++++++\n> submodule.h | 2 ++\n> t/lib-submodule-update.sh | 5 +++--\n> t/t7400-submodule-basic.sh | 5 +++++\n> t/t7412-submodule-absorbgitdirs.sh | 7 ++++++-\n> 6 files changed, 33 insertions(+), 4 deletions(-)\n"},{"id":"364793","messageId":"xmqqwook7c1k.fsf@gitster-ct.c.googlers.com","threadId":"49973","inReplyTo":"20181207235425.128568-3-sbeller@google.com","subject":"Re: [PATCH 2/4] submodule: unset core.worktree if no working tree is present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-08T06:44:23Z","receivedAt":"2018-12-08T06:44:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This reintroduces 4fa4f90ccd (submodule: unset core.worktree if no working\n> tree is present, 2018-06-12), which was reverted as part of f178c13fda\n> (Revert \"Merge branch 'sb/submodule-core-worktree'\", 2018-09-07).\n>\n> 4fa4f90ccd was reverted as its followup commit was faulty, but without\n> the accompanying change of the followup, we'd have an incomplete workflow\n> of setting `core.worktree` again, when it is needed such as checking out\n> a revision that contains a submodule.\n>\n> So re-introduce that commit as-is, focusing on fixing up the followup\n\nI was hoping to hear (given what 0/4 claimed) a clearer explanation\nof what this change wants to achieve, but that is lacking.\n\nNo need to grumble about an earlier work was that turned out to be\ninappropriate for the codebase back then. Repeatedly saying \"this\nis needed\" without giving further explaining why it is so, or\nanything like that, would help readers.\n\nJust pretend that the ealier commits and their reversion never\nhappened, and further pretend that we are doing the best thing that\nshould happen to our codebase. How would we explain this change,\nwhat the problem it tries to solve and what the solution looks like\nin the larger picture?\n\n\tWhen removing a working tree of a submodule (e.g. we may be\n\tswitching back to an earlier commit in the superproject that\n\tdid not have the submodule in question yet), we failed to\n\tunset core.worktree of the submodule's repository. That\n\tcaused this and that issues, exhibited by a few new tests\n\tthis commit adds.\n\n\tMake sure that core.worktree gets unset so that a leftover\n\tsetting won't cause these issues.\n\nor something like that? I am just guessing by looking at the old\ncommit's text, as the above two paragraphs and one sentence does not\nsay much.\n\n> diff --git a/submodule.c b/submodule.c\n> index 6415cc5580..d393e947e6 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1561,6 +1561,18 @@ int bad_to_remove_submodule(const char *path, unsigned flags)\n> \treturn ret;\n> }\n> \n> +void submodule_unset_core_worktree(const struct submodule *sub)\n> +{\n> +\tchar *config_path = xstrfmt(\"%s/modules/%s/config\",\n> +\t\t\t\t get_git_common_dir(), sub->name);\n> +\n> +\tif (git_config_set_in_file_gently(config_path, \"core.worktree\", NULL))\n> +\t\twarning(_(\"Could not unset core.worktree setting in submodule '%s'\"),\n> +\t\t\t sub->path);\n> +\n> +\tfree(config_path);\n> +}\n> +\n> static const char *get_super_prefix_or_empty(void)\n> {\n> \tconst char *s = get_super_prefix();\n> @@ -1726,6 +1738,8 @@ int submodule_move_head(const char *path,\n> \n> \t\t\tif (is_empty_dir(path))\n> \t\t\t\trmdir_or_warn(path);\n> +\n> +\t\t\tsubmodule_unset_core_worktree(sub);\n> \t\t}\n> \t}\n> out:\n> diff --git a/submodule.h b/submodule.h\n> index a680214c01..9e18e9b807 100644\n> --- a/submodule.h\n> +++ b/submodule.h\n> @@ -131,6 +131,8 @@ int submodule_move_head(const char *path,\n> \t\t\tconst char *new_head,\n> \t\t\tunsigned flags);\n> \n> +void submodule_unset_core_worktree(const struct submodule *sub);\n> +\n> /*\n> * Prepare the \"env_array\" parameter of a \"struct child_process\" for executing\n> * a submodule by clearing any repo-specific environment variables, but\n> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\n> index 016391723c..51d4555549 100755\n> --- a/t/lib-submodule-update.sh\n> +++ b/t/lib-submodule-update.sh\n> @@ -709,7 +709,8 @@ test_submodule_recursing_with_args_common() {\n> \t\t\tgit branch -t remove_sub1 origin/remove_sub1 &&\n> \t\t\t$command remove_sub1 &&\n> \t\t\ttest_superproject_content origin/remove_sub1 &&\n> -\t\t\t! test -e sub1\n> +\t\t\t! test -e sub1 &&\n> +\t\t\ttest_must_fail git config -f .git/modules/sub1/config core.worktree\n> \t\t)\n> \t'\n> \t# ... absorbing a .git directory along the way.\n"},{"id":"364795","messageId":"xmqqsgz87bj9.fsf@gitster-ct.c.googlers.com","threadId":"49973","inReplyTo":"20181207235425.128568-4-sbeller@google.com","subject":"Re: [PATCH 3/4] submodule--helper: fix BUG message in ensure_core_worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-08T06:55:22Z","receivedAt":"2018-12-08T06:55:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Shortly after f178c13fda (Revert \"Merge branch\n> 'sb/submodule-core-worktree'\", 2018-09-07), we had another series\n> that implemented partially the same, ensuring that core.worktree was\n> set in a checked out submodule, namely 74d4731da1 (submodule--helper:\n> replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13)\n>\n> As the series 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c',\n> 2018-09-17) has different goals than the reverted series 7e25437d35\n> (Merge branch 'sb/submodule-core-worktree', 2018-07-18), I'd wanted to\n> replay the series on top of it to reach the goal of having `core.worktree`\n> correctly set when the submodules worktree is present, and unset when the\n> worktree is not present.\n>\n> The replay resulted in a strange merge conflict highlighting that\n> the BUG message was not changed in 74d4731da1 (submodule--helper:\n> replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13).\n>\n> Fix the error message.\n>\n> Signed-off-by: Stefan Beller \n> ---\n\nUnlike the step 2/4 I commented on, this does explain what this\nwants to do and why, at least when looked from sideways. Is the\nabove saying the same as the following two-liner?\n\n\tAn ealier mistake while rebasing to produce 74d4731da1\n\tfailed to update this BUG message. Fix this.\n\n\n\n> builtin/submodule--helper.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index d38113a31a..31ac30cf2f 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2045,7 +2045,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)\n> \tstruct repository subrepo;\n> \n> \tif (argc != 2)\n> -\t\tBUG(\"submodule--helper connect-gitdir-workingtree \");\n> +\t\tBUG(\"submodule--helper ensure-core-worktree \");\n> \n> \tpath = argv[1];\n"},{"id":"364796","messageId":"xmqqo99w7b4z.fsf@gitster-ct.c.googlers.com","threadId":"49973","inReplyTo":"20181207235425.128568-5-sbeller@google.com","subject":"Re: [PATCH 4/4] submodule deinit: unset core.worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-08T07:03:56Z","receivedAt":"2018-12-08T07:04:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This re-introduces 984cd77ddb (submodule deinit: unset core.worktree,\n> 2018-06-18), which was reverted as part of f178c13fda (Revert \"Merge\n> branch 'sb/submodule-core-worktree'\", 2018-09-07)\n>\n> The whole series was reverted as the offending commit e98317508c\n> (submodule: ensure core.worktree is set after update, 2018-06-18)\n> was relied on by other commits such as 984cd77ddb.\n>\n> Keep the offending commit reverted, but its functionality came back via\n> 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17), such\n> that we can reintroduce 984cd77ddb now.\n\nNone of the above three explains the most important thing directly,\nso readers fail to grasp what the main theme of the three-patch\nseries is, without looking at the commits that were reverted\nalready.\n\nIs the theme of the overall series to make sure core.worktree is set\nto point at the working tree when submodule's working tree is\ninstantiated, and unset it when it is not?\n\n2/4 was also explained (in the original) that it wants to unset and\ndid so when \"move_head\" gets called. This one does the unset when a\nsubmodule is deinited. Are these the only two cases a submodule\nloses its working tree? If so, the log message for this step should\ndeclare victory by ending with something like\n\n\t... as we covered the only other case in which a submodule\n\tloses its working tree in the earlier step (i.e. switching\n\tbranches of top-level project to move to a commit that did\n\tnot have the submodule), this makes the code always maintain\n\tcore.worktree correctly unset when there is no working tree\n\tfor a submodule.\n\nThanks. I think I agree with what the series wants to do (if I read\nwhat it wants to do correctly, that is).\n\n> Signed-off-by: Stefan Beller \n> ---\n> builtin/submodule--helper.c | 2 ++\n> t/lib-submodule-update.sh | 2 +-\n> t/t7400-submodule-basic.sh | 5 +++++\n> 3 files changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 31ac30cf2f..672b74db89 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1131,6 +1131,8 @@ static void deinit_submodule(const char *path, const char *prefix,\n> \t\tif (!(flags & OPT_QUIET))\n> \t\t\tprintf(format, displaypath);\n> \n> +\t\tsubmodule_unset_core_worktree(sub);\n> +\n> \t\tstrbuf_release(&sb_rm);\n> \t}\n> \n> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\n> index 51d4555549..5b56b23166 100755\n> --- a/t/lib-submodule-update.sh\n> +++ b/t/lib-submodule-update.sh\n> @@ -235,7 +235,7 @@ reset_work_tree_to_interested () {\n> \tthen\n> \t\tmkdir -p submodule_update/.git/modules/sub1/modules &&\n> \t\tcp -r submodule_update_repo/.git/modules/sub1/modules/sub2 submodule_update/.git/modules/sub1/modules/sub2\n> -\t\tGIT_WORK_TREE=. git -C submodule_update/.git/modules/sub1/modules/sub2 config --unset core.worktree\n> +\t\t# core.worktree is unset for sub2 as it is not checked out\n> \tfi &&\n> \t# indicate we are interested in the submodule:\n> \tgit -C submodule_update config submodule.sub1.url \"bogus\" &&\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 76a7cb0af7..aba2d4d6ee 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -984,6 +984,11 @@ test_expect_success 'submodule deinit should remove the whole submodule section\n> \trmdir init\n> '\n> \n> +test_expect_success 'submodule deinit should unset core.worktree' '\n> +\ttest_path_is_file .git/modules/example/config &&\n> +\ttest_must_fail git config -f .git/modules/example/config core.worktree\n> +'\n> +\n> test_expect_success 'submodule deinit from subdirectory' '\n> \tgit submodule update --init &&\n> \tgit config submodule.example.foo bar &&\n"},{"id":"364823","messageId":"xmqqk1kj7e4u.fsf@gitster-ct.c.googlers.com","threadId":"49973","inReplyTo":"20181207235425.128568-2-sbeller@google.com","subject":"Re: [PATCH 1/4] submodule update: add regression test with old style setups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-09T00:11:29Z","receivedAt":"2018-12-09T00:11:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> As f178c13fda (Revert \"Merge branch 'sb/submodule-core-worktree'\",\n> 2018-09-07) was produced shortly before a release, nobody asked for\n> a regression test to be included. Add a regression test that makes sure\n> that the invocation of `git submodule update` on old setups doesn't\n> produce errors as pointed out in f178c13fda.\n>\n> The place to add such a regression test may look odd in t7412, but\n> that is the best place as there we setup old style submodule setups\n> explicitly.\n\nVery good first step. Thanks.\n\n>\n> Signed-off-by: Stefan Beller \n> ---\n> t/t7412-submodule-absorbgitdirs.sh | 7 ++++++-\n> 1 file changed, 6 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\n> index ce74c12da2..1cfa150768 100755\n> --- a/t/t7412-submodule-absorbgitdirs.sh\n> +++ b/t/t7412-submodule-absorbgitdirs.sh\n> @@ -75,7 +75,12 @@ test_expect_success 're-setup nested submodule' '\n> \tGIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \\\n> \t\tcore.worktree \"../../../nested\" &&\n> \t# make sure this re-setup is correct\n> -\tgit status --ignore-submodules=none\n> +\tgit status --ignore-submodules=none &&\n> +\n> +\t# also make sure this old setup does not regress\n> +\tgit submodule update --init --recursive >out 2>err &&\n> +\ttest_must_be_empty out &&\n> +\ttest_must_be_empty err\n> '\n> \n> test_expect_success 'absorb the git dir in a nested submodule' '\n"},{"id":"365207","messageId":"CAGZ79kbPQx4Z0FHioQWxUYoJOKU0TxZXgxEDPFE7XQCMxtRqaw@mail.gmail.com","threadId":"49973","inReplyTo":"xmqqefas8ss4.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/4]","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-12T22:35:10Z","receivedAt":"2018-12-12T22:35:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 7, 2018 at 9:57 PM Junio C Hamano wrote:\n>\n> Stefan Beller writes:\n>\n> > A couple days before the 2.19 release we had a bug report about\n> > broken submodules[1] and reverted[2] the commits leading up to them.\n> >\n> > The behavior of said bug fixed itself by taking a different approach[3],\n> > specifically by a weaker enforcement of having `core.worktree` set in a\n> > submodule [4].\n> >\n> > The revert [2] was overly broad as we neared the release, such that we wanted\n> > to rather keep the known buggy behavior of always having `core.worktree` set,\n> > rather than figuring out how to fix the new bug of having 'git submodule update'\n> > not working in old style repository setups.\n> >\n> > This series re-introduces those reverted patches, with no changes in code,\n> > but with drastically changed commit messages, as those focus on why it is safe\n> > to re-introduce them instead of explaining the desire for the change.\n>\n> The above was a bit too cryptic for me to grok, so let me try\n> rephrasing to see if I got them all correctly.\n>\n> - three-patch series leading to 984cd77ddb were meant to fix some\n> bug, but the series itself was buggy and caused problems; we got\n> rid of them\n\nyes.\n\n> - the problem 984cd77ddb wanted to fix was fixed differently\n\ne98317508c02*\n\n> without reintroducing the problem three-patch series introduced.\n> That fix is already with us since 4d6d6ef1fc.\n\nyes.\n\n> - now these three changes that were problematic in the past is\n> resent without any update (other than that it has one preparatory\n> patch to add tests).\n\nOne of the three changes was problematic, (e98317508c02),\nthe other two are good (in company of the third).\n\nBut those two were not good on their own, which is why we\nreverted all three at once.\n\nNow that we have a different approach for the third,\nwe could re-introduce the two.\n(4fa4f90ccd8, 984cd77ddbf0)\n\nWe do that, but with precaution (an extra test);\nadditional careful reading found a typo, hence\nwe have \"a third\" patch, but that is totally different\nthan what above was referred to \"one of three\".\n\n\n> Is that what is going on? Obviously I am not getting \"the other\"\n> benefit we wanted to gain out of these three patches (because the\n> above description fails to explain what that is), other than to fix\n> the issue that was fixed by 4d6d6ef1fc.\n\nThe other benefit refers to\n7e25437d35 (Merge branch 'sb/submodule-core-worktree', 2018-07-18)\nwhich was reverted as a whole.\nIt's goal was to handle core.worktree appropriately.\n\n(Instead of having it there all the time, only have it when\na working tree is present)\n\n> Sorry for being puzzled...\n\nThis means I need to revamp the commit messages and\ncover letter altogether.\n\nStefan\n"},{"id":"365208","messageId":"CAGZ79kb0Vqk8Gtao6OdKx7gJi6pCEpLzcqQsk=uqCLfePZrmVw@mail.gmail.com","threadId":"49973","inReplyTo":"xmqqsgz87bj9.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/4] submodule--helper: fix BUG message in ensure_core_worktree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-12T22:46:50Z","receivedAt":"2018-12-12T22:47:05Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Unlike the step 2/4 I commented on, this does explain what this\n> wants to do and why, at least when looked from sideways. Is the\n> above saying the same as the following two-liner?\n>\n> An ealier mistake while rebasing to produce 74d4731da1\n> failed to update this BUG message. Fix this.\n\nI am not sure if it was rebasing, which was executed mistakenly.\nSo maybe just saying \"74d4731da1 contains a faulty BUG\nmessage. Fix it.\" would do.\n\nThe intent of the longer message was to shed light in how I found\nthe BUG (ie. I did not see the BUG message, which would ask me\nto actually fix a bug, but found it via code inspection), which I\nthought was valuable information, too.\n"},{"id":"365219","messageId":"xmqq5zvygltp.fsf@gitster-ct.c.googlers.com","threadId":"49973","inReplyTo":"CAGZ79kb0Vqk8Gtao6OdKx7gJi6pCEpLzcqQsk=uqCLfePZrmVw@mail.gmail.com","subject":"Re: [PATCH 3/4] submodule--helper: fix BUG message in ensure_core_worktree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-13T03:14:10Z","receivedAt":"2018-12-13T03:14:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> Unlike the step 2/4 I commented on, this does explain what this\n>> wants to do and why, at least when looked from sideways. Is the\n>> above saying the same as the following two-liner?\n>>\n>> An ealier mistake while rebasing to produce 74d4731da1\n>> failed to update this BUG message. Fix this.\n>\n> I am not sure if it was rebasing, which was executed mistakenly.\n> So maybe just saying \"74d4731da1 contains a faulty BUG\n> message. Fix it.\" would do.\n>\n> The intent of the longer message was to shed light in how I found\n> the BUG (ie. I did not see the BUG message, which would ask me\n> to actually fix a bug, but found it via code inspection), which I\n> thought was valuable information, too.\n\nI guess that it could be stated in a way to make it valuable, but in\nthe presented text, I somehow found it was making the more important\npart of the description (i.e. \"this patch fixes a mistake made by\n74d4731da1\") buried and harder to grok.\n\nThanks.\n"},{"id":"365220","messageId":"xmqq1s6mglry.fsf@gitster-ct.c.googlers.com","threadId":"49973","inReplyTo":"CAGZ79kbPQx4Z0FHioQWxUYoJOKU0TxZXgxEDPFE7XQCMxtRqaw@mail.gmail.com","subject":"Re: [PATCH 0/4]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-13T03:15:13Z","receivedAt":"2018-12-13T03:15:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n>> - now these three changes that were problematic in the past is\n>> resent without any update (other than that it has one preparatory\n>> patch to add tests).\n>\n> One of the three changes was problematic, (e98317508c02),\n> the other two are good (in company of the third).\n\nAh, that is what I failed to read.\n\n> This means I need to revamp the commit messages and\n> cover letter altogether.\n\nI guess that would help future readers. Thanks.\n"},{"id":"365391","messageId":"20181214235945.41191-1-sbeller@google.com","threadId":"49973","inReplyTo":"xmqqefas8ss4.fsf@gitster-ct.c.googlers.com","subject":"[PATCH 0/4] submodules: unset core.worktree when no working tree present","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-14T23:59:41Z","receivedAt":"2018-12-15T00:00:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v2:\nI reworded the commit messages to explain the patches from the ground up\ninstead of only linking to the old commits, that got reverted.\n\n> Just pretend that the ealier commits and their reversion never\n> happened, and further pretend that we are doing the best thing that\n> should happen to our codebase.\n\nI disagree with that first stance (I can freely admit those commits happened),\nbut agree on the second point, so I explained why the code is the best\nfor the code base now. So I kept those pointers in there, too, to make it\neasier for future code archeologists. \n\nv1:\n\nA couple days before the 2.19 release we had a bug report about\nbroken submodules[1] and reverted[2] the commits leading up to them.\n\nThe behavior of said bug fixed itself by taking a different approach[3],\nspecifically by a weaker enforcement of having `core.worktree` set in a\nsubmodule [4].\n\nThe revert [2] was overly broad as we neared the release, such that we wanted\nto rather keep the known buggy behavior of always having `core.worktree` set,\nrather than figuring out how to fix the new bug of having 'git submodule update'\nnot working in old style repository setups.\n\nThis series re-introduces those reverted patches, with no changes in code,\nbut with drastically changed commit messages, as those focus on why it is safe\nto re-introduce them instead of explaining the desire for the change.\n\n[1] https://public-inbox.org/git/2659750.rG6xLiZASK@twilight\n[2] f178c13fda (Revert \"Merge branch 'sb/submodule-core-worktree'\", 2018-09-07)\n[3] 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17)\n[4] 74d4731da1 (submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13)\nStefan Beller (4):\n submodule update: add regression test with old style setups\n submodule: unset core.worktree if no working tree is present\n submodule--helper: fix BUG message in ensure_core_worktree\n submodule deinit: unset core.worktree\n\n builtin/submodule--helper.c | 4 +++-\n submodule.c | 14 ++++++++++++++\n submodule.h | 2 ++\n t/lib-submodule-update.sh | 5 +++--\n t/t7400-submodule-basic.sh | 5 +++++\n t/t7412-submodule-absorbgitdirs.sh | 7 ++++++-\n 6 files changed, 33 insertions(+), 4 deletions(-)\n\n-- \n2.20.0.405.gbc1bbc6f85-goog\n\n"},{"id":"365392","messageId":"20181214235945.41191-2-sbeller@google.com","threadId":"49973","inReplyTo":"20181214235945.41191-1-sbeller@google.com","subject":"[PATCH 1/4] submodule update: add regression test with old style setups","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-14T23:59:42Z","receivedAt":"2018-12-15T00:00:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"As f178c13fda (Revert \"Merge branch 'sb/submodule-core-worktree'\",\n2018-09-07) was produced shortly before a release, nobody asked for\na regression test to be included. Add a regression test that makes sure\nthat the invocation of `git submodule update` on old setups doesn't\nproduce errors as pointed out in f178c13fda.\n\nThe place to add such a regression test may look odd in t7412, but\nthat is the best place as there we setup old style submodule setups\nexplicitly.\n\nSigned-off-by: Stefan Beller \n---\n t/t7412-submodule-absorbgitdirs.sh | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\nindex ce74c12da2..1cfa150768 100755\n--- a/t/t7412-submodule-absorbgitdirs.sh\n+++ b/t/t7412-submodule-absorbgitdirs.sh\n@@ -75,7 +75,12 @@ test_expect_success 're-setup nested submodule' '\n \tGIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \\\n \t\tcore.worktree \"../../../nested\" &&\n \t# make sure this re-setup is correct\n-\tgit status --ignore-submodules=none\n+\tgit status --ignore-submodules=none &&\n+\n+\t# also make sure this old setup does not regress\n+\tgit submodule update --init --recursive >out 2>err &&\n+\ttest_must_be_empty out &&\n+\ttest_must_be_empty err\n '\n \n test_expect_success 'absorb the git dir in a nested submodule' '\n-- \n2.20.0.405.gbc1bbc6f85-goog\n\n"},{"id":"365393","messageId":"20181214235945.41191-3-sbeller@google.com","threadId":"49973","inReplyTo":"20181214235945.41191-1-sbeller@google.com","subject":"[PATCH 2/4] submodule: unset core.worktree if no working tree is present","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-14T23:59:43Z","receivedAt":"2018-12-15T00:00:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When a submodules work tree is removed, we should unset its core.worktree\nsetting as the worktree is no longer present. This is not just in line\nwith the conceptual view of submodules, but it fixes an inconvenience\nfor looking at submodules that are not checked out:\n\n git clone --recurse-submodules git://github.com/git/git && cd git &&\n git checkout --recurse-submodules v2.13.0\n git -C .git/modules/sha1collisiondetection log\n fatal: cannot chdir to '../../../sha1collisiondetection': \\\n No such file or directory\n\nWith this patch applied, the final call to git log works instead of dying\nin its setup, as the checkout will unset the core.worktree setting such\nthat following log will be run in a bare repository.\n\nThis patch covers all commands that are in the unpack machinery, i.e.\ncheckout, read-tree, reset. A follow up patch will address\n\"git submodule deinit\", which will also make use of the new function\nsubmodule_unset_core_worktree(), which is why we expose it in this patch.\n\nThis patch was authored as 4fa4f90ccd (submodule: unset core.worktree if\nno working tree is present, 2018-06-12), which was reverted as part of\nf178c13fda (Revert \"Merge branch 'sb/submodule-core-worktree'\",\n2018-09-07). The revert was needed as the nearby commit e98317508c\n(submodule: ensure core.worktree is set after update, 2018-06-18) is\nfaulty and at the time of 7e25437d35 (Merge branch\n'sb/submodule-core-worktree', 2018-07-18) we could not revert the faulty\ncommit only, as they were depending on each other: If core.worktree is\nunset, we have to have ways to ensure that it is set again once\nthe working tree reappears again.\n\nNow that 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17),\nspecifically 74d4731da1 (submodule--helper: replace\nconnect-gitdir-workingtree by ensure-core-worktree, 2018-08-13) is\npresent, we already check and ensure core.worktree is set when\npopulating a new work tree, such that we can re-introduce the commits\nthat unset core.worktree when removing the worktree.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \nSigned-off-by: Stefan Beller \n---\n submodule.c | 14 ++++++++++++++\n submodule.h | 2 ++\n t/lib-submodule-update.sh | 3 ++-\n 3 files changed, 18 insertions(+), 1 deletion(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 6415cc5580..d393e947e6 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1561,6 +1561,18 @@ int bad_to_remove_submodule(const char *path, unsigned flags)\n \treturn ret;\n }\n \n+void submodule_unset_core_worktree(const struct submodule *sub)\n+{\n+\tchar *config_path = xstrfmt(\"%s/modules/%s/config\",\n+\t\t\t\t get_git_common_dir(), sub->name);\n+\n+\tif (git_config_set_in_file_gently(config_path, \"core.worktree\", NULL))\n+\t\twarning(_(\"Could not unset core.worktree setting in submodule '%s'\"),\n+\t\t\t sub->path);\n+\n+\tfree(config_path);\n+}\n+\n static const char *get_super_prefix_or_empty(void)\n {\n \tconst char *s = get_super_prefix();\n@@ -1726,6 +1738,8 @@ int submodule_move_head(const char *path,\n \n \t\t\tif (is_empty_dir(path))\n \t\t\t\trmdir_or_warn(path);\n+\n+\t\t\tsubmodule_unset_core_worktree(sub);\n \t\t}\n \t}\n out:\ndiff --git a/submodule.h b/submodule.h\nindex a680214c01..9e18e9b807 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -131,6 +131,8 @@ int submodule_move_head(const char *path,\n \t\t\tconst char *new_head,\n \t\t\tunsigned flags);\n \n+void submodule_unset_core_worktree(const struct submodule *sub);\n+\n /*\n * Prepare the \"env_array\" parameter of a \"struct child_process\" for executing\n * a submodule by clearing any repo-specific environment variables, but\ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex 016391723c..51d4555549 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -709,7 +709,8 @@ test_submodule_recursing_with_args_common() {\n \t\t\tgit branch -t remove_sub1 origin/remove_sub1 &&\n \t\t\t$command remove_sub1 &&\n \t\t\ttest_superproject_content origin/remove_sub1 &&\n-\t\t\t! test -e sub1\n+\t\t\t! test -e sub1 &&\n+\t\t\ttest_must_fail git config -f .git/modules/sub1/config core.worktree\n \t\t)\n \t'\n \t# ... absorbing a .git directory along the way.\n-- \n2.20.0.405.gbc1bbc6f85-goog\n\n"},{"id":"365395","messageId":"20181214235945.41191-5-sbeller@google.com","threadId":"49973","inReplyTo":"20181214235945.41191-1-sbeller@google.com","subject":"[PATCH 4/4] submodule deinit: unset core.worktree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-14T23:59:45Z","receivedAt":"2018-12-15T00:00:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When a submodule is deinit'd, the working tree is gone, so the setting of\ncore.worktree is bogus. Unset it. As we covered the only other case in\nwhich a submodule loses its working tree in the earlier step\n(i.e. switching branches of top-level project to move to a commit that did\nnot have the submodule), this makes the code always maintain\ncore.worktree correctly unset when there is no working tree\nfor a submodule.\n\nThis re-introduces 984cd77ddb (submodule deinit: unset core.worktree,\n2018-06-18), which was reverted as part of f178c13fda (Revert \"Merge\nbranch 'sb/submodule-core-worktree'\", 2018-09-07)\n\nThe whole series was reverted as the offending commit e98317508c\n(submodule: ensure core.worktree is set after update, 2018-06-18)\nwas relied on by other commits such as 984cd77ddb.\n\nKeep the offending commit reverted, but its functionality came back via\n4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17), such\nthat we can reintroduce 984cd77ddb now.\n\nSigned-off-by: Stefan Beller \n---\n builtin/submodule--helper.c | 2 ++\n t/lib-submodule-update.sh | 2 +-\n t/t7400-submodule-basic.sh | 5 +++++\n 3 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 31ac30cf2f..672b74db89 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1131,6 +1131,8 @@ static void deinit_submodule(const char *path, const char *prefix,\n \t\tif (!(flags & OPT_QUIET))\n \t\t\tprintf(format, displaypath);\n \n+\t\tsubmodule_unset_core_worktree(sub);\n+\n \t\tstrbuf_release(&sb_rm);\n \t}\n \ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex 51d4555549..5b56b23166 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -235,7 +235,7 @@ reset_work_tree_to_interested () {\n \tthen\n \t\tmkdir -p submodule_update/.git/modules/sub1/modules &&\n \t\tcp -r submodule_update_repo/.git/modules/sub1/modules/sub2 submodule_update/.git/modules/sub1/modules/sub2\n-\t\tGIT_WORK_TREE=. git -C submodule_update/.git/modules/sub1/modules/sub2 config --unset core.worktree\n+\t\t# core.worktree is unset for sub2 as it is not checked out\n \tfi &&\n \t# indicate we are interested in the submodule:\n \tgit -C submodule_update config submodule.sub1.url \"bogus\" &&\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 76a7cb0af7..aba2d4d6ee 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -984,6 +984,11 @@ test_expect_success 'submodule deinit should remove the whole submodule section\n \trmdir init\n '\n \n+test_expect_success 'submodule deinit should unset core.worktree' '\n+\ttest_path_is_file .git/modules/example/config &&\n+\ttest_must_fail git config -f .git/modules/example/config core.worktree\n+'\n+\n test_expect_success 'submodule deinit from subdirectory' '\n \tgit submodule update --init &&\n \tgit config submodule.example.foo bar &&\n-- \n2.20.0.405.gbc1bbc6f85-goog\n\n"},{"id":"365394","messageId":"20181214235945.41191-4-sbeller@google.com","threadId":"49973","inReplyTo":"20181214235945.41191-1-sbeller@google.com","subject":"[PATCH 3/4] submodule--helper: fix BUG message in ensure_core_worktree","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-12-14T23:59:44Z","receivedAt":"2018-12-15T00:00:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"74d4731da1 (submodule--helper: replace connect-gitdir-workingtree by\nensure-core-worktree, 2018-08-13) missed to update the BUG message.\nFix it.\n\nSigned-off-by: Stefan Beller \n---\n builtin/submodule--helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex d38113a31a..31ac30cf2f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2045,7 +2045,7 @@ static int ensure_core_worktree(int argc, const char **argv, const char *prefix)\n \tstruct repository subrepo;\n \n \tif (argc != 2)\n-\t\tBUG(\"submodule--helper connect-gitdir-workingtree \");\n+\t\tBUG(\"submodule--helper ensure-core-worktree \");\n \n \tpath = argv[1];\n \n-- \n2.20.0.405.gbc1bbc6f85-goog\n\n"},{"id":"365919","messageId":"xmqqefa1o1gi.fsf@gitster-ct.c.googlers.com","threadId":"49973","inReplyTo":"20181214235945.41191-2-sbeller@google.com","subject":"Re: [PATCH 1/4] submodule update: add regression test with old style setups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-26T18:21:25Z","receivedAt":"2018-12-28T20:12:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> The place to add such a regression test may look odd in t7412, but\n> that is the best place as there we setup old style submodule setups\n> explicitly.\n\nMakes sense; thanks.\n\n>\n> Signed-off-by: Stefan Beller \n> ---\n> t/t7412-submodule-absorbgitdirs.sh | 7 ++++++-\n> 1 file changed, 6 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\n> index ce74c12da2..1cfa150768 100755\n> --- a/t/t7412-submodule-absorbgitdirs.sh\n> +++ b/t/t7412-submodule-absorbgitdirs.sh\n> @@ -75,7 +75,12 @@ test_expect_success 're-setup nested submodule' '\n> \tGIT_WORK_TREE=../../../nested git -C sub1/.git/modules/nested config \\\n> \t\tcore.worktree \"../../../nested\" &&\n> \t# make sure this re-setup is correct\n> -\tgit status --ignore-submodules=none\n> +\tgit status --ignore-submodules=none &&\n> +\n> +\t# also make sure this old setup does not regress\n> +\tgit submodule update --init --recursive >out 2>err &&\n> +\ttest_must_be_empty out &&\n> +\ttest_must_be_empty err\n> '\n> \n> test_expect_success 'absorb the git dir in a nested submodule' '\n"},{"id":"365920","messageId":"xmqq8t09o1gg.fsf@gitster-ct.c.googlers.com","threadId":"49973","inReplyTo":"20181214235945.41191-3-sbeller@google.com","subject":"Re: [PATCH 2/4] submodule: unset core.worktree if no working tree is present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-26T18:27:11Z","receivedAt":"2018-12-28T20:12:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> 2018-09-07). The revert was needed as the nearby commit e98317508c\n> (submodule: ensure core.worktree is set after update, 2018-06-18) is\n> faulty and at the time of 7e25437d35 (Merge branch\n> 'sb/submodule-core-worktree', 2018-07-18) we could not revert the faulty\n> commit only, as they were depending on each other: If core.worktree is\n> unset, we have to have ways to ensure that it is set again once\n> the working tree reappears again.\n>\n> Now that 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17),\n> specifically 74d4731da1 (submodule--helper: replace\n> connect-gitdir-workingtree by ensure-core-worktree, 2018-08-13) is\n> present, we already check and ensure core.worktree is set when\n> populating a new work tree, such that we can re-introduce the commits\n> that unset core.worktree when removing the worktree.\n\nCleanly explained. Will queue. Thanks.\n\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 14 ++++++++++++++\n> submodule.h | 2 ++\n> t/lib-submodule-update.sh | 3 ++-\n> 3 files changed, 18 insertions(+), 1 deletion(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 6415cc5580..d393e947e6 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1561,6 +1561,18 @@ int bad_to_remove_submodule(const char *path, unsigned flags)\n> \treturn ret;\n> }\n> \n> +void submodule_unset_core_worktree(const struct submodule *sub)\n> +{\n> +\tchar *config_path = xstrfmt(\"%s/modules/%s/config\",\n> +\t\t\t\t get_git_common_dir(), sub->name);\n> +\n> +\tif (git_config_set_in_file_gently(config_path, \"core.worktree\", NULL))\n> +\t\twarning(_(\"Could not unset core.worktree setting in submodule '%s'\"),\n> +\t\t\t sub->path);\n> +\n> +\tfree(config_path);\n> +}\n> +\n> static const char *get_super_prefix_or_empty(void)\n> {\n> \tconst char *s = get_super_prefix();\n> @@ -1726,6 +1738,8 @@ int submodule_move_head(const char *path,\n> \n> \t\t\tif (is_empty_dir(path))\n> \t\t\t\trmdir_or_warn(path);\n> +\n> +\t\t\tsubmodule_unset_core_worktree(sub);\n> \t\t}\n> \t}\n> out:\n> diff --git a/submodule.h b/submodule.h\n> index a680214c01..9e18e9b807 100644\n> --- a/submodule.h\n> +++ b/submodule.h\n> @@ -131,6 +131,8 @@ int submodule_move_head(const char *path,\n> \t\t\tconst char *new_head,\n> \t\t\tunsigned flags);\n> \n> +void submodule_unset_core_worktree(const struct submodule *sub);\n> +\n> /*\n> * Prepare the \"env_array\" parameter of a \"struct child_process\" for executing\n> * a submodule by clearing any repo-specific environment variables, but\n> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\n> index 016391723c..51d4555549 100755\n> --- a/t/lib-submodule-update.sh\n> +++ b/t/lib-submodule-update.sh\n> @@ -709,7 +709,8 @@ test_submodule_recursing_with_args_common() {\n> \t\t\tgit branch -t remove_sub1 origin/remove_sub1 &&\n> \t\t\t$command remove_sub1 &&\n> \t\t\ttest_superproject_content origin/remove_sub1 &&\n> -\t\t\t! test -e sub1\n> +\t\t\t! test -e sub1 &&\n> +\t\t\ttest_must_fail git config -f .git/modules/sub1/config core.worktree\n> \t\t)\n> \t'\n> \t# ... absorbing a .git directory along the way.\n"}]}