{"thread":{"id":"54563","subject":"[REGRESSION FIX 1/2] submodules: test for fetch of non-init subsub-repo","startedAt":"2020-11-03T14:24:13Z","lastAt":"2020-11-12T16:01:14Z","messageCount":14,"participants":["Peter Kaestle","Junio C Hamano","Johannes Schindelin","Peter Kästle","Philip Oakley","Philippe Blain"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"408999","messageId":"1604413399-63090-2-git-send-email-peter.kaestle@nokia.com","threadId":"54563","inReplyTo":"1604413399-63090-1-git-send-email-peter.kaestle@nokia.com","subject":"[REGRESSION FIX 1/2] submodules: test for fetch of non-init subsub-repo","fromName":"Peter Kaestle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-11-03T14:23:18Z","receivedAt":"2020-11-03T14:24:13Z","isPatch":false,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"This test case triggers a regression, which was introduced by\na62387b3fc9f5aeeb04a2db278121d33a9caafa7 in following setup:\nouter_repo/middle_repo/inner_repo and a change in the remote of\ninner_repo happens.  Then it's being fetched by a second clone of the\nouter repo, in which the middle is initialized, but the inner is not.\n\nThis causes is_empty_dir() in submodule.c:get_next_submodule() to\ncheck for a directory only existing in the actual worktree, while the\nis_empty_dir() being called from .git/modules.\n\nSigned-off-by: Peter Kaestle <peter.kaestle@nokia.com>\n---\n t/t5526-fetch-submodules.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 38 insertions(+)\n\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex dd8e423..9fbd481 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -719,4 +719,42 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup\n \t)\n '\n \n+add_commit_push()\n+{\n+\tdir=\"$1\"\n+\tmsg=\"$2\"\n+\tshift 2\n+\tgit -C \"$dir\" add \"$@\" &&\n+\tgit -C \"$dir\" commit -a -m \"$msg\" &&\n+\tgit -C \"$dir\" push\n+}\n+\n+test_expect_failure 'fetching a superproject containing an uninitialized sub/sub project' '\n+\t# does not depend on any previous test setups\n+\n+\tfor repo in outer middle inner\n+\tdo\n+\t\tgit init --bare $repo &&\n+\t\tgit clone $repo ${repo}_content &&\n+\t\techo $repo > ${repo}_content/file &&\n+\t\tadd_commit_push ${repo}_content \"initial\" file\n+\tdone &&\n+\n+\tgit clone outer A &&\n+\tgit -C A submodule add \"$pwd/middle\" &&\n+\tgit -C A/middle/ submodule add \"$pwd/inner\" &&\n+\tadd_commit_push A/middle/ \"adding inner sub\" .gitmodules inner &&\n+\tadd_commit_push A/ \"adding middle sub\" .gitmodules middle &&\n+\n+\tgit clone outer B &&\n+\tgit -C B/ submodule update --init middle &&\n+\n+\techo \"change on inner repo of A\" > A/middle/inner/file &&\n+\tadd_commit_push A/middle/inner \"change on inner\" file &&\n+\tadd_commit_push A/middle \"change on inner\" inner &&\n+\tadd_commit_push A \"change on inner\" middle &&\n+\n+\tgit -C B/ fetch\n+'\n+\n test_done\n-- \n2.6.2\n\n"},{"id":"409000","messageId":"1604413399-63090-3-git-send-email-peter.kaestle@nokia.com","threadId":"54563","inReplyTo":"1604413399-63090-1-git-send-email-peter.kaestle@nokia.com","subject":"[REGRESSION FIX 2/2] Revert \"submodule.c: fetch in submodules git directory instead of in worktree\"","fromName":"Peter Kaestle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-11-03T14:23:19Z","receivedAt":"2020-11-03T14:24:15Z","isPatch":false,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"This reverts commit a62387b3fc9f5aeeb04a2db278121d33a9caafa7 and sets\nthe \"fetching a superproject containing an uninitialized sub/sub\nproject\" testcase to expect success.\n\nSigned-off-by: Peter Kaestle <peter.kaestle@nokia.com>\n---\n submodule.c                 | 14 ++++----------\n t/t5526-fetch-submodules.sh |  2 +-\n 2 files changed, 5 insertions(+), 11 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b3bb59f..eef5204e 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -499,12 +499,6 @@ void prepare_submodule_repo_env(struct strvec *out)\n \t\t     DEFAULT_GIT_DIR_ENVIRONMENT);\n }\n \n-static void prepare_submodule_repo_env_in_gitdir(struct strvec *out)\n-{\n-\tprepare_submodule_repo_env_no_git_dir(out);\n-\tstrvec_pushf(out, \"%s=.\", GIT_DIR_ENVIRONMENT);\n-}\n-\n /*\n  * Initialize a repository struct for a submodule based on the provided 'path'.\n  *\n@@ -1455,8 +1449,8 @@ static int get_next_submodule(struct child_process *cp,\n \t\tif (task->repo) {\n \t\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n \t\t\tchild_process_init(cp);\n-\t\t\tcp->dir = task->repo->gitdir;\n-\t\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n+\t\t\tcp->dir = task->repo->worktree;\n+\t\t\tprepare_submodule_repo_env(&cp->env_array);\n \t\t\tcp->git_cmd = 1;\n \t\t\tif (!spf->quiet)\n \t\t\t\tstrbuf_addf(err, _(\"Fetching submodule %s%s\\n\"),\n@@ -1505,9 +1499,9 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t    spf->prefix, task->sub->path);\n \n \t\tchild_process_init(cp);\n-\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n+\t\tprepare_submodule_repo_env(&cp->env_array);\n \t\tcp->git_cmd = 1;\n-\t\tcp->dir = task->repo->gitdir;\n+\t\tcp->dir = task->repo->worktree;\n \n \t\tstrvec_init(&cp->args);\n \t\tstrvec_pushv(&cp->args, spf->args.v);\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 9fbd481..236e26a 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -729,7 +729,7 @@ add_commit_push()\n \tgit -C \"$dir\" push\n }\n \n-test_expect_failure 'fetching a superproject containing an uninitialized sub/sub project' '\n+test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '\n \t# does not depend on any previous test setups\n \n \tfor repo in outer middle inner\n-- \n2.6.2\n\n"},{"id":"409001","messageId":"1604413399-63090-1-git-send-email-peter.kaestle@nokia.com","threadId":"54563","inReplyTo":null,"subject":"[REGRESSION FIX 0/2] Handling regression introduced by a62387b","fromName":"Peter Kaestle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-11-03T14:23:17Z","receivedAt":"2020-11-03T14:24:15Z","isPatch":false,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"These two patches introduce a test case which triggers a regression\nintroduced by a62387b3fc9f5aeeb04a2db278121d33a9caafa7 and a revert of\nthe commit introducing the issue.\n\n\nThe test case reproduces following scenario:\n\nRepository setup:\nsuperproject/middle_repo/inner_repo\n\nPerson A and B have both a clone of it, while Person B is not working\nwith the inner_repo and thus does not have it initialized in his working\ncopy.\n\nNow person A does a change to the inner_repo and propagates it through\nthe middle_repo and the superproject.\nOnce person A pushed the changes, a \"git fetch\" on superproject level of\nperson B will return with error saying:\n\nCould not access submodule 'inner_repo'\nErrors during submodule fetch:\n        middle_repo\n\n\nThe revert was my quick approach to fix it.  However as I'm not fully\naware of what the idea was behind handling the submodules inside\n.git/modules instead of the worktree, I don't know whether this is the\nbest solution.  Maybe rethinking the whole get_next_submodule()\nalgorithm or simply fixing the is_empty_dir() to use the worktree path\nwill be a better solution.\n\nbest regards,\n--peter;\n\nPeter Kaestle (2):\n  submodules: test for fetch of non-init subsub-repo\n  Revert \"submodule.c: fetch in submodules git directory instead of in\n    worktree\"\n\n submodule.c                 | 14 ++++----------\n t/t5526-fetch-submodules.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 10 deletions(-)\n\n-- \n2.6.2\n\n"},{"id":"409375","messageId":"1604910829-49109-1-git-send-email-peter.kaestle@nokia.com","threadId":"54563","inReplyTo":"1604413399-63090-1-git-send-email-peter.kaestle@nokia.com","subject":"[RFC 0/2] Handling regression introduced by a62387b","fromName":"Peter Kaestle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-11-09T08:33:47Z","receivedAt":"2020-11-09T08:34:06Z","isPatch":false,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"These two patches introduce a test case which triggers a regression\nintroduced by a62387b3fc9f5aeeb04a2db278121d33a9caafa7 and a revert of\nthe commit introducing the issue.\n\n\nThe test case reproduces following scenario:\n\nRepository setup:\nsuperproject/middle_repo/inner_repo\n\nPerson A and B have both a clone of it, while Person B is not working\nwith the inner_repo and thus does not have it initialized in his working\ncopy.\n\nNow person A does a change to the inner_repo and propagates it through\nthe middle_repo and the superproject.\nOnce person A pushed the changes, a \"git fetch\" on superproject level of\nperson B will return with error saying:\n\nCould not access submodule 'inner_repo'\nErrors during submodule fetch:\n        middle_repo\n\n\nThe revert was my quick approach to fix it.  However as I'm not fully\naware of what the idea was behind handling the submodules inside\n.git/modules instead of the worktree, I don't know whether this is the\nbest solution.  Maybe rethinking the whole get_next_submodule()\nalgorithm or simply fixing the is_empty_dir() to use the worktree path\nwill be a better solution.\n\nbest regards,\n--peter;\n\nPeter Kaestle (2):\n  submodules: test for fetch of non-init subsub-repo\n  Revert \"submodule.c: fetch in submodules git directory instead of in\n    worktree\"\n\n submodule.c                 | 14 ++++----------\n t/t5526-fetch-submodules.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 10 deletions(-)\n\n-- \n2.6.2\n\n"},{"id":"409376","messageId":"1604910829-49109-3-git-send-email-peter.kaestle@nokia.com","threadId":"54563","inReplyTo":"1604910829-49109-1-git-send-email-peter.kaestle@nokia.com","subject":"[RFC 2/2] Revert \"submodule.c: fetch in submodules git directory instead of in worktree\"","fromName":"Peter Kaestle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-11-09T08:33:49Z","receivedAt":"2020-11-09T08:34:09Z","isPatch":false,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"This reverts commit a62387b3fc9f5aeeb04a2db278121d33a9caafa7 and sets\nthe \"fetching a superproject containing an uninitialized sub/sub\nproject\" testcase to expect success.\n\nSigned-off-by: Peter Kaestle <peter.kaestle@nokia.com>\n---\n submodule.c                 | 14 ++++----------\n t/t5526-fetch-submodules.sh |  2 +-\n 2 files changed, 5 insertions(+), 11 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b3bb59f..eef5204e 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -499,12 +499,6 @@ void prepare_submodule_repo_env(struct strvec *out)\n \t\t     DEFAULT_GIT_DIR_ENVIRONMENT);\n }\n \n-static void prepare_submodule_repo_env_in_gitdir(struct strvec *out)\n-{\n-\tprepare_submodule_repo_env_no_git_dir(out);\n-\tstrvec_pushf(out, \"%s=.\", GIT_DIR_ENVIRONMENT);\n-}\n-\n /*\n  * Initialize a repository struct for a submodule based on the provided 'path'.\n  *\n@@ -1455,8 +1449,8 @@ static int get_next_submodule(struct child_process *cp,\n \t\tif (task->repo) {\n \t\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n \t\t\tchild_process_init(cp);\n-\t\t\tcp->dir = task->repo->gitdir;\n-\t\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n+\t\t\tcp->dir = task->repo->worktree;\n+\t\t\tprepare_submodule_repo_env(&cp->env_array);\n \t\t\tcp->git_cmd = 1;\n \t\t\tif (!spf->quiet)\n \t\t\t\tstrbuf_addf(err, _(\"Fetching submodule %s%s\\n\"),\n@@ -1505,9 +1499,9 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t    spf->prefix, task->sub->path);\n \n \t\tchild_process_init(cp);\n-\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n+\t\tprepare_submodule_repo_env(&cp->env_array);\n \t\tcp->git_cmd = 1;\n-\t\tcp->dir = task->repo->gitdir;\n+\t\tcp->dir = task->repo->worktree;\n \n \t\tstrvec_init(&cp->args);\n \t\tstrvec_pushv(&cp->args, spf->args.v);\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 9fbd481..236e26a 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -729,7 +729,7 @@ add_commit_push()\n \tgit -C \"$dir\" push\n }\n \n-test_expect_failure 'fetching a superproject containing an uninitialized sub/sub project' '\n+test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '\n \t# does not depend on any previous test setups\n \n \tfor repo in outer middle inner\n-- \n2.6.2\n\n"},{"id":"409377","messageId":"1604910829-49109-2-git-send-email-peter.kaestle@nokia.com","threadId":"54563","inReplyTo":"1604910829-49109-1-git-send-email-peter.kaestle@nokia.com","subject":"[RFC 1/2] submodules: test for fetch of non-init subsub-repo","fromName":"Peter Kaestle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-11-09T08:33:48Z","receivedAt":"2020-11-09T08:34:11Z","isPatch":false,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"This test case triggers a regression, which was introduced by\na62387b3fc9f5aeeb04a2db278121d33a9caafa7 in following setup:\nouter_repo/middle_repo/inner_repo and a change in the remote of\ninner_repo happens.  Then it's being fetched by a second clone of the\nouter repo, in which the middle is initialized, but the inner is not.\n\nThis causes is_empty_dir() in submodule.c:get_next_submodule() to\ncheck for a directory only existing in the actual worktree, while the\nis_empty_dir() being called from .git/modules.\n\nSigned-off-by: Peter Kaestle <peter.kaestle@nokia.com>\n---\n t/t5526-fetch-submodules.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 38 insertions(+)\n\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex dd8e423..9fbd481 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -719,4 +719,42 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup\n \t)\n '\n \n+add_commit_push()\n+{\n+\tdir=\"$1\"\n+\tmsg=\"$2\"\n+\tshift 2\n+\tgit -C \"$dir\" add \"$@\" &&\n+\tgit -C \"$dir\" commit -a -m \"$msg\" &&\n+\tgit -C \"$dir\" push\n+}\n+\n+test_expect_failure 'fetching a superproject containing an uninitialized sub/sub project' '\n+\t# does not depend on any previous test setups\n+\n+\tfor repo in outer middle inner\n+\tdo\n+\t\tgit init --bare $repo &&\n+\t\tgit clone $repo ${repo}_content &&\n+\t\techo $repo > ${repo}_content/file &&\n+\t\tadd_commit_push ${repo}_content \"initial\" file\n+\tdone &&\n+\n+\tgit clone outer A &&\n+\tgit -C A submodule add \"$pwd/middle\" &&\n+\tgit -C A/middle/ submodule add \"$pwd/inner\" &&\n+\tadd_commit_push A/middle/ \"adding inner sub\" .gitmodules inner &&\n+\tadd_commit_push A/ \"adding middle sub\" .gitmodules middle &&\n+\n+\tgit clone outer B &&\n+\tgit -C B/ submodule update --init middle &&\n+\n+\techo \"change on inner repo of A\" > A/middle/inner/file &&\n+\tadd_commit_push A/middle/inner \"change on inner\" file &&\n+\tadd_commit_push A/middle \"change on inner\" inner &&\n+\tadd_commit_push A \"change on inner\" middle &&\n+\n+\tgit -C B/ fetch\n+'\n+\n test_done\n-- \n2.6.2\n\n"},{"id":"409401","messageId":"xmqqk0uuct94.fsf@gitster.c.googlers.com","threadId":"54563","inReplyTo":"1604910829-49109-2-git-send-email-peter.kaestle@nokia.com","subject":"Re: [RFC 1/2] submodules: test for fetch of non-init subsub-repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-09T17:52:39Z","receivedAt":"2020-11-09T17:52:49Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Kaestle <peter.kaestle@nokia.com> writes:\n\n> This test case triggers a regression, which was introduced by\n> a62387b3fc9f5aeeb04a2db278121d33a9caafa7 in following setup:\n\nMinor nit.  Please refer to a commit like so:\n\na62387b3 (submodule.c: fetch in submodules git directory instead of in worktree, 2018-11-28)\n\nThat is what \"git show -s --pretty=reference\" gives for the commit.\n\nIf you have older git, \"--format='%h (%s, %ad)' --date=short\" would\nwork.\n\nInstead of saying \"if you follow this complex thing, it breaks and\nit is a regression at there\", please describe it as a regular bugfix\nlog message.  Describe the set-up first, explain the operation you'd\nperform under the condition, and tell readers what your expected\noutcome is.  Then tell readers what actually happens, and how that\nis different from your expected outcome.  Additionally, tell readers\nthat it used to work before such and such commit broke it and what\nthe root cause of the breakage is.\n\nWhat commit the set-up was broken is also an interesting piece of\ninformation, but it is not as important in the overall picture.\n\nAlso, it probably is a better arrangement, after explaining how the\ncurrent system does not work in the log message, to have the code\nfix in the same patch and add test to ensure the bug will stay\nfixed, in a single patch.  That way, you do not have to start with\nexpect_failure and then flip the polarity to expect_success, which\nis a horrible style for reviewers to understand the code fix because\nthe second \"fix\" step does not actually show the effect of what got\nfixed in the patch (the test change shows the flip of the polarity\nof the test plus only a few context lines and does not show what\nbehaviour change the \"fix\" causes).\n\n\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index dd8e423..9fbd481 100755\n> --- a/t/t5526-fetch-submodules.sh\n> +++ b/t/t5526-fetch-submodules.sh\n> @@ -719,4 +719,42 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup\n>  \t)\n>  '\n>  \n> +add_commit_push()\n> +{\n\nStyle. \n\n    add_commit_push () {\n\ncf. Documentation/CodingGuidelines.\n\n> +\tdir=\"$1\"\n> +\tmsg=\"$2\"\n> +\tshift 2\n> +\tgit -C \"$dir\" add \"$@\" &&\n> +\tgit -C \"$dir\" commit -a -m \"$msg\" &&\n> +\tgit -C \"$dir\" push\n> +}\n> +\n> +test_expect_failure 'fetching a superproject containing an uninitialized sub/sub project' '\n> +\t# does not depend on any previous test setups\n> +\n> +\tfor repo in outer middle inner\n> +\tdo\n> +\t\tgit init --bare $repo &&\n> +\t\tgit clone $repo ${repo}_content &&\n> +\t\techo $repo > ${repo}_content/file &&\n\nStyle. \n\n    echo \"$repo\" >\"${repo}_content/file\" &&\n\ncf. Documentation/CodingGuidelines.\n\n> +\t\tadd_commit_push ${repo}_content \"initial\" file\n\nIf any of these iterations, except for the last one, fails in the\nloop, you do not notice the breakage and go on to the next\niteration.  You'd need \"|| return 1\" at the end, perhaps.\n\nSo far, you created three bare repositories called outer, middle and\ninner, and each of {outer,middle,inner}_content repositories is a\ncopy with a working tree of its counterpart.\n\n> +\tdone &&\n> +\n> +\tgit clone outer A &&\n> +\tgit -C A submodule add \"$pwd/middle\" &&\n> +\tgit -C A/middle/ submodule add \"$pwd/inner\" &&\n\nHmph.  Is it essential to name these directories with full pathname\nfor the problem to reproduce, or would the issue also appear if\nthese repositories refer to each other with relative pathnames?\nJust being curious---if it only breaks with one and succeeds with\nthe other, that deserves commenting here.\n\nSo far, you created A that is \"outer\", added \"middle\" as its\nsubmodule and then added \"inner\" as a submodule of \"middle\".\n\nAlthough it is not wrong per-se, it somehow feels a bit unnatural\nthat you didn't do all of the above in the working trees you created\nin the previous step---I would have expected that middle_content\nworking tree would be used to add \"inner\" as its submodule, for\nexample.\n\n\n> +\tadd_commit_push A/middle/ \"adding inner sub\" .gitmodules inner &&\n> +\tadd_commit_push A/ \"adding middle sub\" .gitmodules middle &&\n\nAnd then you conclude the addition of submodules by recording each\nof these two \"submodule add\" events in a commit and push it out.\n\n> +\tgit clone outer B &&\n> +\tgit -C B/ submodule update --init middle &&\n\nAnd then you clone the outer thing (which does not recursively\ninstantiate) from A, and instantiate the middle layer (which does\nnot recursively instantiate the bottom later, I presume?)\n\nI _think_ the state here should be minimally validated in this test.\nIf you expect 'outer' and 'middle' are instantiated, perhaps check\nits contents (e.g. do you have a thing called 'file'?  What does it\nhave in it?) and check the commit (e.g. does 'rev-parse HEAD' give\nyou the commit you expect?).  If you expect 'inner' is not\ninstantiated at this point, that should be vaildated as well.  If\nanything, that would explain what your expectations are better than\nany word in the proposed log message.\n\nIn any case, i presume that up to this point things work as expected\nwith or without the \"fix\" patch?  If so, the usual way we structure\nthese tests is to stop here and make that a single \"setup\" test.\nStart the whole sequence above like so, perhaps.\n\n    test_expect_success 'setup nested submodule fetch test' '\n\t\t...\n\n\nand close here.\n\nAnd then the \"interesting\" part of the test.  \n\n> +\techo \"change on inner repo of A\" > A/middle/inner/file &&\n\nStyle.\n\n> +\tadd_commit_push A/middle/inner \"change on inner\" file &&\n> +\tadd_commit_push A/middle \"change on inner\" inner &&\n> +\tadd_commit_push A \"change on inner\" middle &&\n\nSo you create a new commit in the bottom layer, propagate it up to\nthe middle layer, and to the outer layer.  Are these steps also what\nyou expect to succeed, or does the \"regression\" break any of these?\nIf these are still part of set-up that is expected to work, you\nprobably need to roll these up to the 'setup' step (with some\nvalidation to express what the tests are expecting). From your\ndescription, which did not say where exactly in this long sequence\nyou expect things to break, unfortunately no reader can tell, so\nI'll leave the restructuring up to you.\n\n> +\n> +\tgit -C B/ fetch\n\nAnd from B that was an original copy of A with only the top and\nmiddle layer instantiated, you run \"git fetch\".  Are you happy as\nlong as \"git fetch\" does not exit with non-zero status?  That is\nhard to believe---it may be a necessary condition for the command to\nexit with zero status, but you have other expectations, like what\ncommit the remote tracking branch refs/remotes/origin/HEAD ought to\nbe pointing at.  I think we should check that, too.\n\n> +'\n> +\n>  test_done\n"},{"id":"409491","messageId":"nycvar.QRO.7.76.6.2011101607250.18437@tvgsbejvaqbjf.bet","threadId":"54563","inReplyTo":"1604910829-49109-3-git-send-email-peter.kaestle@nokia.com","subject":"Re: [RFC 2/2] Revert \"submodule.c: fetch in submodules git directory instead of in worktree\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-11-10T15:08:17Z","receivedAt":"2020-11-10T15:08:29Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peter,\n\nOn Mon, 9 Nov 2020, Peter Kaestle wrote:\n\n> This reverts commit a62387b3fc9f5aeeb04a2db278121d33a9caafa7 and sets\n> the \"fetching a superproject containing an uninitialized sub/sub\n> project\" testcase to expect success.\n\nJust like the cover letter, this commit message might be technically\ncorrect, but leaves me (and probably every future reader) wondering _why_\nyou would want this change.\n\nMaybe this can be improved?\n\nCiao,\nDscho\n\n>\n> Signed-off-by: Peter Kaestle <peter.kaestle@nokia.com>\n> ---\n>  submodule.c                 | 14 ++++----------\n>  t/t5526-fetch-submodules.sh |  2 +-\n>  2 files changed, 5 insertions(+), 11 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index b3bb59f..eef5204e 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -499,12 +499,6 @@ void prepare_submodule_repo_env(struct strvec *out)\n>  \t\t     DEFAULT_GIT_DIR_ENVIRONMENT);\n>  }\n>\n> -static void prepare_submodule_repo_env_in_gitdir(struct strvec *out)\n> -{\n> -\tprepare_submodule_repo_env_no_git_dir(out);\n> -\tstrvec_pushf(out, \"%s=.\", GIT_DIR_ENVIRONMENT);\n> -}\n> -\n>  /*\n>   * Initialize a repository struct for a submodule based on the provided 'path'.\n>   *\n> @@ -1455,8 +1449,8 @@ static int get_next_submodule(struct child_process *cp,\n>  \t\tif (task->repo) {\n>  \t\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n>  \t\t\tchild_process_init(cp);\n> -\t\t\tcp->dir = task->repo->gitdir;\n> -\t\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n> +\t\t\tcp->dir = task->repo->worktree;\n> +\t\t\tprepare_submodule_repo_env(&cp->env_array);\n>  \t\t\tcp->git_cmd = 1;\n>  \t\t\tif (!spf->quiet)\n>  \t\t\t\tstrbuf_addf(err, _(\"Fetching submodule %s%s\\n\"),\n> @@ -1505,9 +1499,9 @@ static int get_next_submodule(struct child_process *cp,\n>  \t\t\t    spf->prefix, task->sub->path);\n>\n>  \t\tchild_process_init(cp);\n> -\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n> +\t\tprepare_submodule_repo_env(&cp->env_array);\n>  \t\tcp->git_cmd = 1;\n> -\t\tcp->dir = task->repo->gitdir;\n> +\t\tcp->dir = task->repo->worktree;\n>\n>  \t\tstrvec_init(&cp->args);\n>  \t\tstrvec_pushv(&cp->args, spf->args.v);\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index 9fbd481..236e26a 100755\n> --- a/t/t5526-fetch-submodules.sh\n> +++ b/t/t5526-fetch-submodules.sh\n> @@ -729,7 +729,7 @@ add_commit_push()\n>  \tgit -C \"$dir\" push\n>  }\n>\n> -test_expect_failure 'fetching a superproject containing an uninitialized sub/sub project' '\n> +test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '\n>  \t# does not depend on any previous test setups\n>\n>  \tfor repo in outer middle inner\n> --\n> 2.6.2\n>\n>\n"},{"id":"409620","messageId":"c4741382-5662-ac15-24da-218b28ae9dee@nokia.com","threadId":"54563","inReplyTo":"xmqqk0uuct94.fsf@gitster.c.googlers.com","subject":"Re: [RFC 1/2] submodules: test for fetch of non-init subsub-repo","fromName":"Peter Kästle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-11-11T12:45:46Z","receivedAt":"2020-11-11T12:46:52Z","isPatch":false,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"On 09.11.20 18:52, Junio C Hamano wrote:\n> Peter Kaestle <peter.kaestle@nokia.com> writes:\n> \n>> This test case triggers a regression, which was introduced by\n>> a62387b3fc9f5aeeb04a2db278121d33a9caafa7 in following setup:\n> \n> Minor nit.  Please refer to a commit like so:\n> \n> a62387b3 (submodule.c: fetch in submodules git directory instead of in worktree, 2018-11-28)\n> \n> That is what \"git show -s --pretty=reference\" gives for the commit.\n> \n> If you have older git, \"--format='%h (%s, %ad)' --date=short\" would\n> work.\n\nThanks for this hint, this is really useful.\n\n\n> Instead of saying \"if you follow this complex thing, it breaks and\n> it is a regression at there\", please describe it as a regular bugfix\n> log message.  Describe the set-up first, explain the operation you'd\n> perform under the condition, and tell readers what your expected\n> outcome is.  Then tell readers what actually happens, and how that\n> is different from your expected outcome.  Additionally, tell readers\n> that it used to work before such and such commit broke it and what\n> the root cause of the breakage is.\n\nhm... I did do this in the cover letter, maybe you missed it or I was \nnot able to express myself good enough there.\nAnyhow, I'll add it to the commit messages, which goes into the log.\n\nHere is my proposal for a new commit message of the test case:\n\n----8<----\nA regression has been introduced by 'a62387b (submodule.c: fetch in \nsubmodules git directory instead of in worktree, 2018-11-28)'.\n\nThe scenario in which it triggers is when one has a remote repository \nwith a subrepository inside a subrepository like this:\nsuperproject/middle_repo/inner_repo\n\nPerson A and B have both a clone of it, while Person B is not working\nwith the inner_repo and thus does not have it initialized in his working\ncopy.\n\nNow person A introduces a change to the inner_repo and propagates it \nthrough the middle_repo and the superproject.\nOnce person A pushed the changes and person B wants to fetch them using \n\"git fetch\" on superproject level, git will return with error saying:\n\nCould not access submodule 'inner_repo'\nErrors during submodule fetch:\n         middle_repo\n\nExpectation is that in this case the inner submodule will be recognized \nas uninitialized subrepository and skipped by the git fetch command.\n\nThis used to work correctly before 'a62387b (submodule.c: fetch in \nsubmodules git directory instead of in worktree, 2018-11-28)'.\n\nStarting with a62387b the code wants to evaluate \"is_empty_dir()\" inside \n.git/modules for a directory only existing in the worktree, delivering \nthen of course wrong return value.\n---->8----\n\n\nAbout the revert of the a62387b commit, which I proposed in the second \npatch, I'm not sure it's the right way.  The revert was simply my quick \napproach to fix it.  As I'm not fully aware of what the idea was behind \nhandling the submodules inside .git/modules instead of the worktree, I \ndon't know whether this is the best solution.  Maybe rethinking the \nwhole get_next_submodule() algorithm or simply fixing the is_empty_dir() \nto use the worktree path will be a better solution.\n--> We should discuss about this.\n\n\n> What commit the set-up was broken is also an interesting piece of\n> information, but it is not as important in the overall picture.\n> \n> Also, it probably is a better arrangement, after explaining how the\n> current system does not work in the log message, to have the code\n> fix in the same patch and add test to ensure the bug will stay\n> fixed, in a single patch.  That way, you do not have to start with\n> expect_failure and then flip the polarity to expect_success, which\n> is a horrible style for reviewers to understand the code fix because\n> the second \"fix\" step does not actually show the effect of what got\n> fixed in the patch (the test change shows the flip of the polarity\n> of the test plus only a few context lines and does not show what\n> behaviour change the \"fix\" causes).\n\nOk, will deliver the test and the fix proposal in a single patch.\n\n\n>> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n>> index dd8e423..9fbd481 100755\n>> --- a/t/t5526-fetch-submodules.sh\n>> +++ b/t/t5526-fetch-submodules.sh\n>> @@ -719,4 +719,42 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup\n>>   \t)\n>>   '\n>>   \n>> +add_commit_push()\n>> +{\n> \n> Style.\n> \n>      add_commit_push () {\n\nok.\n\n\n> cf. Documentation/CodingGuidelines.\n> \n>> +\tdir=\"$1\"\n>> +\tmsg=\"$2\"\n>> +\tshift 2\n>> +\tgit -C \"$dir\" add \"$@\" &&\n>> +\tgit -C \"$dir\" commit -a -m \"$msg\" &&\n>> +\tgit -C \"$dir\" push\n>> +}\n>> +\n>> +test_expect_failure 'fetching a superproject containing an uninitialized sub/sub project' '\n>> +\t# does not depend on any previous test setups\n>> +\n>> +\tfor repo in outer middle inner\n>> +\tdo\n>> +\t\tgit init --bare $repo &&\n>> +\t\tgit clone $repo ${repo}_content &&\n>> +\t\techo $repo > ${repo}_content/file &&\n> \n> Style.\n> \n>      echo \"$repo\" >\"${repo}_content/file\" &&\n\nok.\n\n\n> cf. Documentation/CodingGuidelines.\n> \n>> +\t\tadd_commit_push ${repo}_content \"initial\" file\n> \n> If any of these iterations, except for the last one, fails in the\n> loop, you do not notice the breakage and go on to the next\n> iteration.  You'd need \"|| return 1\" at the end, perhaps.\n\nyes, I definitely missed that.\n\n\n> So far, you created three bare repositories called outer, middle and\n> inner, and each of {outer,middle,inner}_content repositories is a\n> copy with a working tree of its counterpart.\n> \n>> +\tdone &&\n>> +\n>> +\tgit clone outer A &&\n>> +\tgit -C A submodule add \"$pwd/middle\" &&\n>> +\tgit -C A/middle/ submodule add \"$pwd/inner\" &&\n> \n> Hmph.  Is it essential to name these directories with full pathname\n> for the problem to reproduce, or would the issue also appear if\n> these repositories refer to each other with relative pathnames?\n> Just being curious---if it only breaks with one and succeeds with\n> the other, that deserves commenting here.\n\nHaven't tried that as the case was intended to simulate an environment, \nwhere one has remote repositories.  And with remote repositories, you \nhave an url, which is kind of absolute path.  When reading the failing \ncode, I doubt that it really matters.\n\n\n> So far, you created A that is \"outer\", added \"middle\" as its\n> submodule and then added \"inner\" as a submodule of \"middle\".\n> \n> Although it is not wrong per-se, it somehow feels a bit unnatural\n> that you didn't do all of the above in the working trees you created\n> in the previous step---I would have expected that middle_content\n> working tree would be used to add \"inner\" as its submodule, for\n> example.\n\nNot sure I got your concern, maybe it helps you to understand when I add \nthis scenario description which we want to mimic:\nThe \"bare\" repos outer, middle and inner are created by an administrator \non a remote server.  Person A is preparing the split of the sources for \nall the other users working in the environment by adding the submodules \nthe way which is specified by the software architecture we intend to \ndevelop in.\n\n\n>> +\tadd_commit_push A/middle/ \"adding inner sub\" .gitmodules inner &&\n>> +\tadd_commit_push A/ \"adding middle sub\" .gitmodules middle &&\n> \n> And then you conclude the addition of submodules by recording each\n> of these two \"submodule add\" events in a commit and push it out.\n> \n>> +\tgit clone outer B &&\n>> +\tgit -C B/ submodule update --init middle &&\n> \n> And then you clone the outer thing (which does not recursively\n> instantiate) from A, and instantiate the middle layer (which does\n> not recursively instantiate the bottom later, I presume?)\n\nYes, Person B is cloning into the outer layer without recursively going \ninto all the submodules, just initializing the ones, which he is \nexpected to work on.  In the tests scenario he's only working on the \nmiddle layer, but not on the inner one.\n\n\n> I _think_ the state here should be minimally validated in this test.\n\nOf course we could do so.  My intention was to keep it focused on the \none thing which we needed to test.  Namely the fetch of an outer repo \nwith an uninitialized sub-sub repo.\n\n\n> If you expect 'outer' and 'middle' are instantiated, perhaps check\n> its contents (e.g. do you have a thing called 'file'?  What does it\n> have in it?) and check the commit (e.g. does 'rev-parse HEAD' give\n> you the commit you expect?).  If you expect 'inner' is not\n> instantiated at this point, that should be vaildated as well.  If\n> anything, that would explain what your expectations are better than\n> any word in the proposed log message.\n> \n> In any case, i presume that up to this point things work as expected\n> with or without the \"fix\" patch?  If so, the usual way we structure\n> these tests is to stop here and make that a single \"setup\" test.\n> Start the whole sequence above like so, perhaps.\n> \n>      test_expect_success 'setup nested submodule fetch test' '\n> \t\t...\n\nOk, got it, will refactor.\n\n\n> And then the \"interesting\" part of the test.\n> \n>> +\techo \"change on inner repo of A\" > A/middle/inner/file &&\n> \n> Style.\n\nok.\n\n\n>> +\tadd_commit_push A/middle/inner \"change on inner\" file &&\n>> +\tadd_commit_push A/middle \"change on inner\" inner &&\n>> +\tadd_commit_push A \"change on inner\" middle &&\n> \n> So you create a new commit in the bottom layer, propagate it up to\n> the middle layer, and to the outer layer.  Are these steps also what\n> you expect to succeed, or does the \"regression\" break any of these?\n> If these are still part of set-up that is expected to work, you\n> probably need to roll these up to the 'setup' step (with some\n> validation to express what the tests are expecting). From your\n> description, which did not say where exactly in this long sequence\n> you expect things to break, unfortunately no reader can tell, so\n> I'll leave the restructuring up to you.\n\nYes those steps are also expected to succeed, it's just important that \nthe initial clone of B happens before those pushes.  For your proposed \nrestructuring this could also go into the setup step.  Leaving only one \nsingle command for the actual test to fail:\n\n\n>> +\n>> +\tgit -C B/ fetch\n> \n> And from B that was an original copy of A with only the top and\n> middle layer instantiated, you run \"git fetch\".  Are you happy as\n> long as \"git fetch\" does not exit with non-zero status?  That is\n> hard to believe---it may be a necessary condition for the command to\n> exit with zero status, but you have other expectations, like what\n> commit the remote tracking branch refs/remotes/origin/HEAD ought to\n> be pointing at.  I think we should check that, too.\n\nChecking for return code is the one thing which catches this regression, \nbut checking whether all the repositories are at the correct HEAD is \nanother thing which we probably want to have in for testing future \nchanges on the respective part of the code.  Will add it.\n\nThank you very much for all the comments, I learned a lot by processing \nthrough them.  I'll send a patch v2 soon.\n\n-- \nkind regards,\n--peter;\n"},{"id":"409655","messageId":"4339e01e-d312-69ee-c810-2fbc65606b0d@iee.email","threadId":"54563","inReplyTo":"c4741382-5662-ac15-24da-218b28ae9dee@nokia.com","subject":"Re: [RFC 1/2] submodules: test for fetch of non-init subsub-repo","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2020-11-11T17:22:43Z","receivedAt":"2020-11-11T17:22:48Z","isPatch":false,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"minor readability comment,\n\nOn 11/11/2020 12:45, Peter Kästle wrote:\n> Here is my proposal for a new commit message of the test case:\n>\n> ----8<----\n> A regression has been introduced by 'a62387b (submodule.c: fetch in\n> submodules git directory instead of in worktree, 2018-11-28)'.\n>\n> The scenario in which it triggers is when one has a remote repository\n> with a subrepository inside a subrepository like this:\n> superproject/middle_repo/inner_repo\n>\n> Person A and B have both a clone of it, while Person B is not working\n> with the inner_repo and thus does not have it initialized in his working\n> copy.\n>\n> Now person A introduces a change to the inner_repo and propagates it\n> through the middle_repo and the superproject.\n> Once person A pushed the changes and person B wants to fetch them\n> using \"git fetch\" on superproject level, git\n\nIt's not obviously obvious which person is doing this final 'git'\noperation (it isn't attached to a particular person). Not sure if moving\nthe comma, or saying \", B's 'git fetch' will..\" is the right choice. The\nfollowing sentences also feel as treating 'git' as person (trusted\nfriend;-).\n\n> will return with error saying:\n>\n> Could not access submodule 'inner_repo'\n> Errors during submodule fetch:\n>         middle_repo\n>\n> Expectation is that in this case the inner submodule will be\n> recognized as uninitialized subrepository and skipped by the git fetch\n> command.\n>\n> This used to work correctly before 'a62387b (submodule.c: fetch in\n> submodules git directory instead of in worktree, 2018-11-28)'.\n>\n> Starting with a62387b the code wants to evaluate \"is_empty_dir()\"\n> inside .git/modules for a directory only existing in the worktree,\n> delivering then of course wrong return value.\n> ---->8----\n--\nPhilip\n"},{"id":"409657","messageId":"37DD13D4-FBE4-4DB7-85F5-824E850BA9AE@gmail.com","threadId":"54563","inReplyTo":"xmqqk0uuct94.fsf@gitster.c.googlers.com","subject":"Re: [RFC 1/2] submodules: test for fetch of non-init subsub-repo","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-11-11T17:35:53Z","receivedAt":"2020-11-11T17:35:59Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio,\n\n> Le 9 nov. 2020 à 12:52, Junio C Hamano <gitster@pobox.com> a écrit :\n> \n> Peter Kaestle <peter.kaestle@nokia.com> writes:\n> \n>> This test case triggers a regression, which was introduced by\n>> a62387b3fc9f5aeeb04a2db278121d33a9caafa7 in following setup:\n> \n> Also, it probably is a better arrangement, after explaining how the\n> current system does not work in the log message, to have the code\n> fix in the same patch and add test to ensure the bug will stay\n> fixed, in a single patch.  That way, you do not have to start with\n> expect_failure and then flip the polarity to expect_success, which\n> is a horrible style for reviewers to understand the code fix because\n> the second \"fix\" step does not actually show the effect of what got\n> fixed in the patch (the test change shows the flip of the polarity\n> of the test plus only a few context lines and does not show what\n> behaviour change the \"fix\" causes).\n\nI had learned by browsing the list that this was the preferred way to\nsubmit patches for bug fixes and regressions for this project, but I had\nnot yet read a good justification as to *why* it was preferred. \n\nThanks for spelling it out; I think a quick paragraph about this somewhere\nin SubmittingPatches would be a good addition for new contributors.\n\nCheers,\n\nPhilippe.\n\n"},{"id":"409667","messageId":"48197610-9B5B-43A0-9748-AB71B061858A@gmail.com","threadId":"54563","inReplyTo":"1604910829-49109-2-git-send-email-peter.kaestle@nokia.com","subject":"Re: [RFC 1/2] submodules: test for fetch of non-init subsub-repo","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-11-11T19:24:46Z","receivedAt":"2020-11-11T19:24:51Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Peter,\n\n\n> Le 9 nov. 2020 à 03:33, Peter Kaestle <peter.kaestle@nokia.com> a écrit :\n> \n> --- snip ---\n> \n> +add_commit_push()\n> +{\n> +\tdir=\"$1\"\n> +\tmsg=\"$2\"\n> +\tshift 2\n> +\tgit -C \"$dir\" add \"$@\" &&\n> +\tgit -C \"$dir\" commit -a -m \"$msg\" &&\n> +\tgit -C \"$dir\" push\n> +}\n> +\n> +test_expect_failure 'fetching a superproject containing an uninitialized sub/sub project' '\n> +\t# does not depend on any previous test setups\n> +\n> +\tfor repo in outer middle inner\n> +\tdo\n> +\t\tgit init --bare $repo &&\n> +\t\tgit clone $repo ${repo}_content &&\n> +\t\techo $repo > ${repo}_content/file &&\n> +\t\tadd_commit_push ${repo}_content \"initial\" file\n> +\tdone &&\n> +\n> +\tgit clone outer A &&\n> +\tgit -C A submodule add \"$pwd/middle\" &&\n> +\tgit -C A/middle/ submodule add \"$pwd/inner\" &&\n> +\tadd_commit_push A/middle/ \"adding inner sub\" .gitmodules inner &&\n> +\tadd_commit_push A/ \"adding middle sub\" .gitmodules middle &&\n> +\n> +\tgit clone outer B &&\n> +\tgit -C B/ submodule update --init middle &&\n> +\n> +\techo \"change on inner repo of A\" > A/middle/inner/file &&\n> +\tadd_commit_push A/middle/inner \"change on inner\" file &&\n> +\tadd_commit_push A/middle \"change on inner\" inner &&\n> +\tadd_commit_push A \"change on inner\" middle &&\n\nIn addition to what Junio wrote, maybe your test could make better use of the \nfunctions provided by the test harness library. Take a look at t/README [1] for a \npartial list, and t/test-lib-functions.sh [2] for the full list. test_create_repo, in particular,\ncould be useful.\n\nCheers,\n\nPhilippe.\n\n\n[1] https://github.com/git/git/blob/master/t/README#L743\n[2] https://github.com/git/git/blob/master/t/test-lib-functions.sh\n\n"},{"id":"409668","messageId":"xmqqft5f4rts.fsf@gitster.c.googlers.com","threadId":"54563","inReplyTo":"37DD13D4-FBE4-4DB7-85F5-824E850BA9AE@gmail.com","subject":"Re: [RFC 1/2] submodules: test for fetch of non-init subsub-repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-11T19:27:27Z","receivedAt":"2020-11-11T19:27:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain <levraiphilippeblain@gmail.com> writes:\n\n> Thanks for spelling it out; I think a quick paragraph about this somewhere\n> in SubmittingPatches would be a good addition for new contributors.\n\nPatches welcome ;-)\n"},{"id":"409767","messageId":"1605196853-37359-1-git-send-email-peter.kaestle@nokia.com","threadId":"54563","inReplyTo":"4339e01e-d312-69ee-c810-2fbc65606b0d@iee.email","subject":"[RFCv2] submodules: fix of regression on fetching of non-init subsub-repo","fromName":"Peter Kaestle","fromEmail":"peter.kaestle@nokia.com","sentAt":"2020-11-12T16:00:53Z","receivedAt":"2020-11-12T16:01:14Z","isPatch":false,"sender":{"key":"peter.kaestle@nokia.com","avatar":null},"body":"A regression has been introduced by 'a62387b (submodule.c: fetch in\nsubmodules git directory instead of in worktree, 2018-11-28)'.\n\nThe scenario in which it triggers is when one has a remote repository\nwith a subrepository inside a subrepository like this:\nsuperproject/middle_repo/inner_repo\n\nPerson A and B have both a clone of it, while Person B is not working\nwith the inner_repo and thus does not have it initialized in his working\ncopy.\n\nNow person A introduces a change to the inner_repo and propagates it\nthrough the middle_repo and the superproject.\nOnce person A pushed the changes and person B wants to fetch them using\n\"git fetch\" on superproject level, B's git call will return with error\nsaying:\n\nCould not access submodule 'inner_repo'\nErrors during submodule fetch:\n         middle_repo\n\nExpectation is that in this case the inner submodule will be recognized\nas uninitialized subrepository and skipped by the git fetch command.\n\nThis used to work correctly before 'a62387b (submodule.c: fetch in\nsubmodules git directory instead of in worktree, 2018-11-28)'.\n\nStarting with a62387b the code wants to evaluate \"is_empty_dir()\" inside\n.git/modules for a directory only existing in the worktree, delivering\nthen of course wrong return value.\n\nThis patch reverts the changes of a62387b and introduces a regression\ntest.\n\nSigned-off-by: Peter Kaestle <peter.kaestle@nokia.com>\n---\n submodule.c                 | 14 +++-------\n t/t5526-fetch-submodules.sh | 63 +++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 67 insertions(+), 10 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b3bb59f..eef5204e 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -499,12 +499,6 @@ void prepare_submodule_repo_env(struct strvec *out)\n \t\t     DEFAULT_GIT_DIR_ENVIRONMENT);\n }\n \n-static void prepare_submodule_repo_env_in_gitdir(struct strvec *out)\n-{\n-\tprepare_submodule_repo_env_no_git_dir(out);\n-\tstrvec_pushf(out, \"%s=.\", GIT_DIR_ENVIRONMENT);\n-}\n-\n /*\n  * Initialize a repository struct for a submodule based on the provided 'path'.\n  *\n@@ -1455,8 +1449,8 @@ static int get_next_submodule(struct child_process *cp,\n \t\tif (task->repo) {\n \t\t\tstruct strbuf submodule_prefix = STRBUF_INIT;\n \t\t\tchild_process_init(cp);\n-\t\t\tcp->dir = task->repo->gitdir;\n-\t\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n+\t\t\tcp->dir = task->repo->worktree;\n+\t\t\tprepare_submodule_repo_env(&cp->env_array);\n \t\t\tcp->git_cmd = 1;\n \t\t\tif (!spf->quiet)\n \t\t\t\tstrbuf_addf(err, _(\"Fetching submodule %s%s\\n\"),\n@@ -1505,9 +1499,9 @@ static int get_next_submodule(struct child_process *cp,\n \t\t\t    spf->prefix, task->sub->path);\n \n \t\tchild_process_init(cp);\n-\t\tprepare_submodule_repo_env_in_gitdir(&cp->env_array);\n+\t\tprepare_submodule_repo_env(&cp->env_array);\n \t\tcp->git_cmd = 1;\n-\t\tcp->dir = task->repo->gitdir;\n+\t\tcp->dir = task->repo->worktree;\n \n \t\tstrvec_init(&cp->args);\n \t\tstrvec_pushv(&cp->args, spf->args.v);\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex dd8e423..a7f6f9f 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -719,4 +719,67 @@ test_expect_success 'fetch new submodule commit intermittently referenced by sup\n \t)\n '\n \n+add_commit_push () {\n+\tdir=\"$1\"\n+\tmsg=\"$2\"\n+\tshift 2\n+\tgit -C \"$dir\" add \"$@\" &&\n+\tgit -C \"$dir\" commit -a -m \"$msg\" &&\n+\tgit -C \"$dir\" push\n+}\n+\n+compare_refs_in_dir () {\n+\tfail= &&\n+\tif test \"x$1\" = 'x!'\n+\tthen\n+\t\tfail='!' &&\n+\t\tshift\n+\tfi &&\n+\tgit -C \"$1\" rev-parse --verify \"$2\" >expect &&\n+\tgit -C \"$3\" rev-parse --verify \"$4\" >actual &&\n+\teval $fail test_cmp expect actual\n+}\n+\n+\n+test_expect_success 'setup nested submodule fetch test' '\n+\t# does not depend on any previous test setups\n+\n+\tfor repo in outer middle inner\n+\tdo\n+\t\t(\n+\t\t\tgit init --bare $repo &&\n+\t\t\tgit clone $repo ${repo}_content &&\n+\t\t\techo \"$repo\" >\"${repo}_content/file\" &&\n+\t\t\tadd_commit_push ${repo}_content \"initial\" file\n+\t\t) || return 1\n+\tdone &&\n+\n+\tgit clone outer A &&\n+\tgit -C A submodule add \"$pwd/middle\" &&\n+\tgit -C A/middle/ submodule add \"$pwd/inner\" &&\n+\tadd_commit_push A/middle/ \"adding inner sub\" .gitmodules inner &&\n+\tadd_commit_push A/ \"adding middle sub\" .gitmodules middle &&\n+\n+\tgit clone outer B &&\n+\tgit -C B/ submodule update --init middle &&\n+\n+\tcompare_refs_in_dir A HEAD B HEAD &&\n+\tcompare_refs_in_dir A/middle HEAD B/middle HEAD &&\n+\ttest -f B/file &&\n+\ttest -f B/middle/file &&\n+\t! test -f B/middle/inner/file &&\n+\n+\techo \"change on inner repo of A\" >\"A/middle/inner/file\" &&\n+\tadd_commit_push A/middle/inner \"change on inner\" file &&\n+\tadd_commit_push A/middle \"change on inner\" inner &&\n+\tadd_commit_push A \"change on inner\" middle\n+'\n+\n+test_expect_success 'fetching a superproject containing an uninitialized sub/sub project' '\n+\t# depends on previous test for setup\n+\n+\tgit -C B/ fetch &&\n+\tcompare_refs_in_dir A origin/master B origin/master\n+'\n+\n test_done\n-- \n2.6.2\n\n"}]}