{"thread":{"id":"52818","subject":"[PATCH 0/6] unpack-trees: fix '--recurse-submodules' when switching from no submodules to nested submodules","startedAt":"2020-02-17T04:53:11Z","lastAt":"2020-02-19T23:33:00Z","messageCount":9,"participants":["Philippe Blain via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"391888","messageId":"pull.555.git.1581915186.gitgitgadget@gmail.com","threadId":"52818","inReplyTo":null,"subject":"[PATCH 0/6] unpack-trees: fix '--recurse-submodules' when switching from no submodules to nested submodules","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-17T04:53:00Z","receivedAt":"2020-02-17T04:53:11Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Currently, using git checkout --recurse-submodules (or the submodule.recurse \nconfig) to switch from a commit with no submodules to a commit with\ninitialized nested submodules fails because a child git process tries to cd \nto a yet non-existing nested submodule directory. reset and read-tree are\nalso affected in the same way since they also use the unpack-trees\nmachinery.\n\nThe 5th commit in this series fixes this bug. The first four are clean-up\npatches in tests and in unpack-trees that mostly remove outdated\ncomments/dead code. The 6th commit adds a test for the reverse transition\n(nested submodules -> no submodules) as it was not being tested.\n\nThe commit message of the 5th commit is quite long, as I tried to describe\nclearly what is the cause of the bug.\n\nPhilippe Blain (6):\n t7112: remove mention of KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED\n t/lib-submodule-update: remove outdated test description\n t/lib-submodule-update: move a test to the right section\n unpack-trees: remove outdated description for verify_clean_submodule\n unpack-trees: check for missing submodule directory in merged_entry\n t/lib-submodule-update: add test removing nested submodules\n\n t/lib-submodule-update.sh | 68 +++++++++++++++++++++++++++-----------\n t/t7112-reset-submodule.sh | 1 -\n unpack-trees.c | 7 ++--\n 3 files changed, 51 insertions(+), 25 deletions(-)\n\n\nbase-commit: d8437c57fa0752716dde2d3747e7c22bf7ce2e41\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-555%2Fphil-blain%2Fcheckout-recurse-nested-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-555/phil-blain/checkout-recurse-nested-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/555\n-- \ngitgitgadget\n"},{"id":"391889","messageId":"9004c95a72847615ded3d50779b80b77004df736.1581915186.git.gitgitgadget@gmail.com","threadId":"52818","inReplyTo":"pull.555.git.1581915186.gitgitgadget@gmail.com","subject":"[PATCH 2/6] t/lib-submodule-update: remove outdated test description","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-17T04:53:02Z","receivedAt":"2020-02-17T04:53:14Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nThe commands in the unpack_trees machinery (checkout, reset, read-tree)\nwere fixed in 218c883783 (submodule: properly recurse for read-tree and\ncheckout, 2017-05-02) to correctly update nested submodules when called\nwith the `--recurse-submodules` flag.\n\nHowever, a comment in t/lib-submodule-update.sh mentions that this use\ncase still doesn't work.\n\nRemove this outdated comment.\n\nSigned-off-by: Philippe Blain \n---\n t/lib-submodule-update.sh | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex 1dd17fc03e1..5f9eb682f6a 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -908,7 +908,6 @@ test_submodule_switch_recursing_with_args () {\n \t\t)\n \t'\n \n-\t# recursing deeper than one level doesn't work yet.\n \ttest_expect_success \"$command: modified submodule updates submodule recursively\" '\n \t\tprolog &&\n \t\treset_work_tree_to_interested add_nested_sub &&\n-- \ngitgitgadget\n\n"},{"id":"391890","messageId":"bc7217f293a381226abdff89ba046814777a96a5.1581915186.git.gitgitgadget@gmail.com","threadId":"52818","inReplyTo":"pull.555.git.1581915186.gitgitgadget@gmail.com","subject":"[PATCH 4/6] unpack-trees: remove outdated description for verify_clean_submodule","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-17T04:53:04Z","receivedAt":"2020-02-17T04:53:14Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nThe function verify_clean_submodule() learned to verify if a submodule\nworking tree is clean in a7bc845a9a (unpack-trees: check if we can\nperform the operation for submodules, 2017-03-14), but the commented\ndescription above it was not updated to reflect that, such that this\ndescription has been outdated since then.\n\nSince Git has now learned to optionnally recursively check out\nsubmodules during a superproject checkout, remove this outdated\ndescription.\n\nSigned-off-by: Philippe Blain \n---\n unpack-trees.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex da4d6d4ec01..37eca3ede8b 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1815,9 +1815,6 @@ static void invalidate_ce_path(const struct cache_entry *ce,\n /*\n * Check that checking out ce->sha1 in subdir ce->name is not\n * going to overwrite any working files.\n- *\n- * Currently, git does not checkout subprojects during a superproject\n- * checkout, so it is not going to overwrite anything.\n */\n static int verify_clean_submodule(const char *old_sha1,\n \t\t\t\t const struct cache_entry *ce,\n-- \ngitgitgadget\n\n"},{"id":"391891","messageId":"d47740181bf02036a83f3573164c6c650ae75afb.1581915186.git.gitgitgadget@gmail.com","threadId":"52818","inReplyTo":"pull.555.git.1581915186.gitgitgadget@gmail.com","subject":"[PATCH 5/6] unpack-trees: check for missing submodule directory in merged_entry","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-17T04:53:05Z","receivedAt":"2020-02-17T04:53:14Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nUsing `git checkout --recurse-submodules` to switch between a\nbranch with no submodules and a branch with initialized nested\nsubmodules currently causes a fatal error:\n\n $ git checkout --recurse-submodules branch-with-nested-submodules\n fatal: exec '--super-prefix=submodule/nested/': cd to 'nested'\nfailed: No such file or directory\n error: Submodule 'nested' could not be updated.\n error: Submodule 'submodule/nested' cannot checkout new HEAD.\n error: Submodule 'submodule' could not be updated.\n M\tsubmodule\n Switched to branch 'branch-with-nested-submodules'\n\nThe checkout succeeds but the worktree and index of the first level\nsubmodule are left empty:\n\n $ cd submodule\n $ git -c status.submoduleSummary=1 status\n HEAD detached at b3ce885\n Changes to be committed:\n (use \"git restore --staged ...\" to unstage)\n deleted: .gitmodules\n deleted: first.t\n deleted: nested\n\n fatal: not a git repository: 'nested/.git'\n Submodule changes to be committed:\n\n * nested 1e96f59...0000000:\n\n $ git ls-files -s\n $ # empty\n $ ls -A\n .git\n\nThe reason for the fatal error during the checkout is that a child git\nprocess tries to cd into the yet unexisting nested submodule directory.\nThe sequence is the following:\n\n1. The main git process (the one running in the superproject) eventually\nreaches write_entry() in entry.c, which creates the first level\nsubmodule directory and then calls submodule_move_head() in submodule.c,\nwhich spawns `git read-tree` in the submodule directory.\n\n2. The first child git process (the one in the submodule of the\nsuperproject) eventually calls check_submodule_move_head() at\nunpack_trees.c:2021, which calls submodule_move_head in dry-run mode,\nwhich spawns `git read-tree` in the nested submodule directory.\n\n3. The second child git process tries to chdir() in the yet unexisting\nnested submodule directory in start_command() at run-command.c:829 and\ndies before exec'ing.\n\nThe reason why check_submodule_move_head() is reached in the first child\nand not in the main process is that it is inside an\nif(submodule_from_ce()) construct, and submodule_from_ce() returns a\nvalid struct submodule pointer, whereas it returns a null pointer in the\nmain git process.\n\nThe reason why submodule_from_ce() returns a null pointer in the main\ngit process is because the call to cache_lookup_path() in config_from()\n(called from submodule_from_path() in submodule_from_ce()) returns a\nnull pointer since the hashmap \"for_path\" in the submodule_cache of\nthe_repository is not yet populated. It is not populated because both\nrepo_get_oid(repo, GITMODULES_INDEX, &oid) and repo_get_oid(repo,\nGITMODULES_HEAD, &oid) in config_from_gitmodules() at\nsubmodule-config.c:639-640 return -1, as at this stage of the operation,\nneither the HEAD of the superproject nor its index contain any\n.gitmodules file.\n\nIn contrast, in the first child the hashmap is populated because\nrepo_get_oid(repo, GITMODULES_HEAD, &oid) returns 0 as the HEAD of the\nfirst level submodule, i.e. .git/modules/submodule/HEAD, points to a\ncommit where .gitmodules is present and records 'nested' as a submodule.\n\nFix this bug by checking that the submodule directory exists before\ncalling check_submodule_move_head() in merged_entry() in the `if(!old)`\nbranch, i.e. if going from a commit with no submodule to a commit with a\nsubmodule present.\n\nAlso protect the other call to check_submodule_move_head() in\nmerged_entry() the same way as it is safer, even though the `else if\n(!(old->ce_flags & CE_CONFLICTED))` branch of the code is not at play in\nthe present bug.\n\nThe other calls to check_submodule_move_head() in other functions in\nunpack_trees.c are all already protected by calls to lstat() somewhere\nin\nthe program flow so we don't need additional protection for them.\n\nAll commands in the unpack_trees machinery are affected, i.e. checkout,\nreset and read-tree when called with the --recurse-submodules flag.\n\nThis bug was first reported in [1].\n\n[1]\nhttps://lore.kernel.org/git/7437BB59-4605-48EC-B05E-E2BDB2D9DABC@gmail.com/\n\nReported-by: Philippe Blain \nReported-by: Damien Robert \nSigned-off-by: Philippe Blain \n---\n t/lib-submodule-update.sh | 14 ++++++++++++++\n unpack-trees.c | 4 ++--\n 2 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex 417da3602ae..ab30b2da24f 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -626,6 +626,7 @@ test_submodule_forced_switch () {\n # New test cases\n # - Removing a submodule with a git directory absorbs the submodules\n # git directory first into the superproject.\n+# - Switching from no submodule to nested submodules\n \n # Internal function; use test_submodule_switch_recursing_with_args() or\n # test_submodule_forced_switch_recursing_with_args() instead.\n@@ -683,6 +684,19 @@ test_submodule_recursing_with_args_common() {\n \t\t\ttest_submodule_content sub1 origin/replace_directory_with_sub1\n \t\t)\n \t'\n+\t# Switching to a commit with nested submodules recursively checks them out\n+\ttest_expect_success \"$command: nested submodules are checked out\" '\n+\t\tprolog &&\n+\t\treset_work_tree_to_interested no_submodule &&\n+\t\t(\n+\t\t\tcd submodule_update &&\n+\t\t\tgit branch -t modify_sub1_recursively origin/modify_sub1_recursively &&\n+\t\t\t$command modify_sub1_recursively &&\n+\t\t\ttest_superproject_content origin/modify_sub1_recursively &&\n+\t\t\ttest_submodule_content sub1 origin/modify_sub1_recursively &&\n+\t\t\ttest_submodule_content -C sub1 sub2 origin/modify_sub1_recursively\n+\t\t)\n+\t'\n \n \t######################## Disappearing submodule #######################\n \t# Removing a submodule removes its work tree ...\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 37eca3ede8b..fc6ba19486d 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -2064,7 +2064,7 @@ static int merged_entry(const struct cache_entry *ce,\n \t\t}\n \t\tinvalidate_ce_path(merge, o);\n \n-\t\tif (submodule_from_ce(ce)) {\n+\t\tif (submodule_from_ce(ce) && file_exists(ce->name)) {\n \t\t\tint ret = check_submodule_move_head(ce, NULL,\n \t\t\t\t\t\t\t oid_to_hex(&ce->oid),\n \t\t\t\t\t\t\t o);\n@@ -2093,7 +2093,7 @@ static int merged_entry(const struct cache_entry *ce,\n \t\t\tinvalidate_ce_path(old, o);\n \t\t}\n \n-\t\tif (submodule_from_ce(ce)) {\n+\t\tif (submodule_from_ce(ce) && file_exists(ce->name)) {\n \t\t\tint ret = check_submodule_move_head(ce, oid_to_hex(&old->oid),\n \t\t\t\t\t\t\t oid_to_hex(&ce->oid),\n \t\t\t\t\t\t\t o);\n-- \ngitgitgadget\n\n"},{"id":"391892","messageId":"5a271857a1176ede2ac9c894dcb436758e28a79f.1581915186.git.gitgitgadget@gmail.com","threadId":"52818","inReplyTo":"pull.555.git.1581915186.gitgitgadget@gmail.com","subject":"[PATCH 1/6] t7112: remove mention of KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-17T04:53:01Z","receivedAt":"2020-02-17T04:53:17Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nThe known failure mode KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED was\nremoved from lib-submodule-update.sh in 218c883783 (submodule: properly\nrecurse for read-tree and checkout, 2017-05-02) but at that time this\nchange was not ported over to topic sb/reset-recurse-submodules, such\nthat when this topic was merged in 5f074ca7e8 (Merge branch\n'sb/reset-recurse-submodules', 2017-05-29), t7112-reset-submodules.sh\nkept a mention of this removed failure mode.\n\nRemove it now, as it does not mean anything anymore.\n\nSigned-off-by: Philippe Blain \n---\n t/t7112-reset-submodule.sh | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/t/t7112-reset-submodule.sh b/t/t7112-reset-submodule.sh\nindex a1cb9ff858e..67346424a53 100755\n--- a/t/t7112-reset-submodule.sh\n+++ b/t/t7112-reset-submodule.sh\n@@ -5,7 +5,6 @@ test_description='reset can handle submodules'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-submodule-update.sh\n \n-KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1\n KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1\n KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1\n \n-- \ngitgitgadget\n\n"},{"id":"391893","messageId":"fa64ff03647e85d79414da6776309ab7488bc2be.1581915186.git.gitgitgadget@gmail.com","threadId":"52818","inReplyTo":"pull.555.git.1581915186.gitgitgadget@gmail.com","subject":"[PATCH 3/6] t/lib-submodule-update: move a test to the right section","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-17T04:53:03Z","receivedAt":"2020-02-17T04:53:17Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nThe test \"$command: submodule branch is not changed, detach HEAD\ninstead\" is in the \"Appearing submodule\" section of\ntest_submodule_recursing_with_args_common(), but this test updates a\nsubmodule; it does not test a transition from a state with no submodule\nto a state with a submodule.\n\nAs such, for consistency, move it to the \"Modified submodule\" section of\nthe same function. While at it, add a comment describing the test.\n\nSigned-off-by: Philippe Blain \n---\n t/lib-submodule-update.sh | 33 +++++++++++++++++----------------\n 1 file changed, 17 insertions(+), 16 deletions(-)\n\ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex 5f9eb682f6a..417da3602ae 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -658,22 +658,6 @@ test_submodule_recursing_with_args_common() {\n \t\t\ttest_submodule_content sub1 origin/add_sub1\n \t\t)\n \t'\n-\ttest_expect_success \"$command: submodule branch is not changed, detach HEAD instead\" '\n-\t\tprolog &&\n-\t\treset_work_tree_to_interested add_sub1 &&\n-\t\t(\n-\t\t\tcd submodule_update &&\n-\t\t\tgit -C sub1 checkout -b keep_branch &&\n-\t\t\tgit -C sub1 rev-parse HEAD >expect &&\n-\t\t\tgit branch -t modify_sub1 origin/modify_sub1 &&\n-\t\t\t$command modify_sub1 &&\n-\t\t\ttest_superproject_content origin/modify_sub1 &&\n-\t\t\ttest_submodule_content sub1 origin/modify_sub1 &&\n-\t\t\tgit -C sub1 rev-parse keep_branch >actual &&\n-\t\t\ttest_cmp expect actual &&\n-\t\t\ttest_must_fail git -C sub1 symbolic-ref HEAD\n-\t\t)\n-\t'\n \n \t# Replacing a tracked file with a submodule produces a checked out submodule\n \ttest_expect_success \"$command: replace tracked file with submodule checks out submodule\" '\n@@ -789,6 +773,23 @@ test_submodule_recursing_with_args_common() {\n \t\t\ttest_submodule_content sub1 origin/add_sub1\n \t\t)\n \t'\n+\t# Updating a submodule does not touch the currently checked out branch in the submodule\n+\ttest_expect_success \"$command: submodule branch is not changed, detach HEAD instead\" '\n+\t\tprolog &&\n+\t\treset_work_tree_to_interested add_sub1 &&\n+\t\t(\n+\t\t\tcd submodule_update &&\n+\t\t\tgit -C sub1 checkout -b keep_branch &&\n+\t\t\tgit -C sub1 rev-parse HEAD >expect &&\n+\t\t\tgit branch -t modify_sub1 origin/modify_sub1 &&\n+\t\t\t$command modify_sub1 &&\n+\t\t\ttest_superproject_content origin/modify_sub1 &&\n+\t\t\ttest_submodule_content sub1 origin/modify_sub1 &&\n+\t\t\tgit -C sub1 rev-parse keep_branch >actual &&\n+\t\t\ttest_cmp expect actual &&\n+\t\t\ttest_must_fail git -C sub1 symbolic-ref HEAD\n+\t\t)\n+\t'\n }\n \n # Declares and invokes several tests that, in various situations, checks that\n-- \ngitgitgadget\n\n"},{"id":"391894","messageId":"70b2a20b4ebb09ccb85b576c212075ab1ca75601.1581915186.git.gitgitgadget@gmail.com","threadId":"52818","inReplyTo":"pull.555.git.1581915186.gitgitgadget@gmail.com","subject":"[PATCH 6/6] t/lib-submodule-update: add test removing nested submodules","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-17T04:53:06Z","receivedAt":"2020-02-17T04:53:20Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nThe previous commit fixed a bug with the (no submodule) -> (nested\nsubmodules) transition for commands in the unpack-trees machinery.\n\nLet's add a test for the reverse transition (going from nested\nsubmodules to no submodule), as it is not being tested currently.\n\nWhile at it, uniformize the capitalization in the list of tests.\n\nSigned-off-by: Philippe Blain \n---\n t/lib-submodule-update.sh | 20 ++++++++++++++++++--\n 1 file changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex ab30b2da24f..64fc6487dd9 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -297,7 +297,7 @@ test_submodule_content () {\n # - Directory containing tracked files replaced by submodule\n # - Submodule replaced by tracked files in directory\n # - Submodule replaced by tracked file with the same name\n-# - tracked file replaced by submodule\n+# - Tracked file replaced by submodule\n #\n # The default is that submodule contents aren't changed until \"git submodule\n # update\" is run. And even then that command doesn't delete the work tree of\n@@ -621,12 +621,13 @@ test_submodule_forced_switch () {\n # - Directory containing tracked files replaced by submodule\n # - Submodule replaced by tracked files in directory\n # - Submodule replaced by tracked file with the same name\n-# - tracked file replaced by submodule\n+# - Tracked file replaced by submodule\n #\n # New test cases\n # - Removing a submodule with a git directory absorbs the submodules\n # git directory first into the superproject.\n # - Switching from no submodule to nested submodules\n+# - Switching from nested submodules to no submodule\n \n # Internal function; use test_submodule_switch_recursing_with_args() or\n # test_submodule_forced_switch_recursing_with_args() instead.\n@@ -760,6 +761,21 @@ test_submodule_recursing_with_args_common() {\n \t\t)\n \t'\n \n+\t# Switching to a commit without nested submodules removes their worktrees\n+\ttest_expect_success \"$command: worktrees of nested submodules are removed\" '\n+\t\tprolog &&\n+\t\treset_work_tree_to_interested add_nested_sub &&\n+\t\t(\n+\t\t\tcd submodule_update &&\n+\t\t\tgit branch -t no_submodule origin/no_submodule &&\n+\t\t\t$command no_submodule &&\n+\t\t\ttest_superproject_content origin/no_submodule &&\n+\t\t\t! test_path_is_dir sub1 &&\n+\t\t\ttest_must_fail git config -f .git/modules/sub1/config core.worktree &&\n+\t\t\ttest_must_fail git config -f .git/modules/sub1/modules/sub2/config core.worktree\n+\t\t)\n+\t'\n+\n \t########################## Modified submodule #########################\n \t# Updating a submodule sha1 updates the submodule's work tree\n \ttest_expect_success \"$command: modified submodule updates submodule work tree\" '\n-- \ngitgitgadget\n"},{"id":"392130","messageId":"xmqqh7zmcg66.fsf@gitster-ct.c.googlers.com","threadId":"52818","inReplyTo":"5a271857a1176ede2ac9c894dcb436758e28a79f.1581915186.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/6] t7112: remove mention of KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-19T23:26:25Z","receivedAt":"2020-02-19T23:26:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" writes:\n\n> From: Philippe Blain \n>\n> The known failure mode KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED was\n> removed from lib-submodule-update.sh in 218c883783 (submodule: properly\n> recurse for read-tree and checkout, 2017-05-02) but at that time this\n> change was not ported over to topic sb/reset-recurse-submodules, such\n> that when this topic was merged in 5f074ca7e8 (Merge branch\n> 'sb/reset-recurse-submodules', 2017-05-29), t7112-reset-submodules.sh\n> kept a mention of this removed failure mode.\n>\n> Remove it now, as it does not mean anything anymore.\n>\n> Signed-off-by: Philippe Blain \n> ---\n> t/t7112-reset-submodule.sh | 1 -\n> 1 file changed, 1 deletion(-)\n\nThanks for cleaning up.\n\n>\n> diff --git a/t/t7112-reset-submodule.sh b/t/t7112-reset-submodule.sh\n> index a1cb9ff858e..67346424a53 100755\n> --- a/t/t7112-reset-submodule.sh\n> +++ b/t/t7112-reset-submodule.sh\n> @@ -5,7 +5,6 @@ test_description='reset can handle submodules'\n> . ./test-lib.sh\n> . \"$TEST_DIRECTORY\"/lib-submodule-update.sh\n> \n> -KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1\n> KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1\n> KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1\n"},{"id":"392131","messageId":"xmqqd0aacfvf.fsf@gitster-ct.c.googlers.com","threadId":"52818","inReplyTo":"d47740181bf02036a83f3573164c6c650ae75afb.1581915186.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/6] unpack-trees: check for missing submodule directory in merged_entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-19T23:32:52Z","receivedAt":"2020-02-19T23:33:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" writes:\n\n> From: Philippe Blain \n>\n> Using `git checkout --recurse-submodules` to switch between a\n> branch with no submodules and a branch with initialized nested\n> submodules currently causes a fatal error:\n>\n> $ git checkout --recurse-submodules branch-with-nested-submodules\n> fatal: exec '--super-prefix=submodule/nested/': cd to 'nested'\n> failed: No such file or directory\n> error: Submodule 'nested' could not be updated.\n> error: Submodule 'submodule/nested' cannot checkout new HEAD.\n> error: Submodule 'submodule' could not be updated.\n> M\tsubmodule\n> Switched to branch 'branch-with-nested-submodules'\n>\n> The checkout succeeds but the worktree and index of the first level\n> submodule are left empty:\n>\n> $ cd submodule\n> $ git -c status.submoduleSummary=1 status\n> HEAD detached at b3ce885\n> Changes to be committed:\n> (use \"git restore --staged ...\" to unstage)\n> deleted: .gitmodules\n> deleted: first.t\n> deleted: nested\n>\n> fatal: not a git repository: 'nested/.git'\n> Submodule changes to be committed:\n>\n> * nested 1e96f59...0000000:\n>\n> $ git ls-files -s\n> $ # empty\n> $ ls -A\n> .git\n>\n> The reason for the fatal error during the checkout is that a child git\n> process tries to cd into the yet unexisting nested submodule directory.\n> The sequence is the following:\n>\n> 1. The main git process (the one running in the superproject) eventually\n> reaches write_entry() in entry.c, which creates the first level\n> submodule directory and then calls submodule_move_head() in submodule.c,\n> which spawns `git read-tree` in the submodule directory.\n>\n> 2. The first child git process (the one in the submodule of the\n> superproject) eventually calls check_submodule_move_head() at\n> unpack_trees.c:2021, which calls submodule_move_head in dry-run mode,\n> which spawns `git read-tree` in the nested submodule directory.\n>\n> 3. The second child git process tries to chdir() in the yet unexisting\n> nested submodule directory in start_command() at run-command.c:829 and\n> dies before exec'ing.\n>\n> The reason why check_submodule_move_head() is reached in the first child\n> and not in the main process is that it is inside an\n> if(submodule_from_ce()) construct, and submodule_from_ce() returns a\n> valid struct submodule pointer, whereas it returns a null pointer in the\n> main git process.\n>\n> The reason why submodule_from_ce() returns a null pointer in the main\n> git process is because the call to cache_lookup_path() in config_from()\n> (called from submodule_from_path() in submodule_from_ce()) returns a\n> null pointer since the hashmap \"for_path\" in the submodule_cache of\n> the_repository is not yet populated. It is not populated because both\n> repo_get_oid(repo, GITMODULES_INDEX, &oid) and repo_get_oid(repo,\n> GITMODULES_HEAD, &oid) in config_from_gitmodules() at\n> submodule-config.c:639-640 return -1, as at this stage of the operation,\n> neither the HEAD of the superproject nor its index contain any\n> .gitmodules file.\n>\n> In contrast, in the first child the hashmap is populated because\n> repo_get_oid(repo, GITMODULES_HEAD, &oid) returns 0 as the HEAD of the\n> first level submodule, i.e. .git/modules/submodule/HEAD, points to a\n> commit where .gitmodules is present and records 'nested' as a submodule.\n>\n> Fix this bug by checking that the submodule directory exists before\n> calling check_submodule_move_head() in merged_entry() in the `if(!old)`\n> branch, i.e. if going from a commit with no submodule to a commit with a\n> submodule present.\n>\n> Also protect the other call to check_submodule_move_head() in\n> merged_entry() the same way as it is safer, even though the `else if\n> (!(old->ce_flags & CE_CONFLICTED))` branch of the code is not at play in\n> the present bug.\n>\n> The other calls to check_submodule_move_head() in other functions in\n> unpack_trees.c are all already protected by calls to lstat() somewhere\n> in\n> the program flow so we don't need additional protection for them.\n>\n> All commands in the unpack_trees machinery are affected, i.e. checkout,\n> reset and read-tree when called with the --recurse-submodules flag.\n\nGreate to see a detailed write-up. I'll read the surrounding\ncodepath again later before commenting further.\n\nThanks.\n\n>\n> This bug was first reported in [1].\n>\n> [1]\n> https://lore.kernel.org/git/7437BB59-4605-48EC-B05E-E2BDB2D9DABC@gmail.com/\n>\n> Reported-by: Philippe Blain \n> Reported-by: Damien Robert \n> Signed-off-by: Philippe Blain \n> ---\n> t/lib-submodule-update.sh | 14 ++++++++++++++\n> unpack-trees.c | 4 ++--\n> 2 files changed, 16 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\n> index 417da3602ae..ab30b2da24f 100755\n> --- a/t/lib-submodule-update.sh\n> +++ b/t/lib-submodule-update.sh\n> @@ -626,6 +626,7 @@ test_submodule_forced_switch () {\n> # New test cases\n> # - Removing a submodule with a git directory absorbs the submodules\n> # git directory first into the superproject.\n> +# - Switching from no submodule to nested submodules\n> \n> # Internal function; use test_submodule_switch_recursing_with_args() or\n> # test_submodule_forced_switch_recursing_with_args() instead.\n> @@ -683,6 +684,19 @@ test_submodule_recursing_with_args_common() {\n> \t\t\ttest_submodule_content sub1 origin/replace_directory_with_sub1\n> \t\t)\n> \t'\n> +\t# Switching to a commit with nested submodules recursively checks them out\n> +\ttest_expect_success \"$command: nested submodules are checked out\" '\n> +\t\tprolog &&\n> +\t\treset_work_tree_to_interested no_submodule &&\n> +\t\t(\n> +\t\t\tcd submodule_update &&\n> +\t\t\tgit branch -t modify_sub1_recursively origin/modify_sub1_recursively &&\n> +\t\t\t$command modify_sub1_recursively &&\n> +\t\t\ttest_superproject_content origin/modify_sub1_recursively &&\n> +\t\t\ttest_submodule_content sub1 origin/modify_sub1_recursively &&\n> +\t\t\ttest_submodule_content -C sub1 sub2 origin/modify_sub1_recursively\n> +\t\t)\n> +\t'\n> \n> \t######################## Disappearing submodule #######################\n> \t# Removing a submodule removes its work tree ...\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 37eca3ede8b..fc6ba19486d 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -2064,7 +2064,7 @@ static int merged_entry(const struct cache_entry *ce,\n> \t\t}\n> \t\tinvalidate_ce_path(merge, o);\n> \n> -\t\tif (submodule_from_ce(ce)) {\n> +\t\tif (submodule_from_ce(ce) && file_exists(ce->name)) {\n> \t\t\tint ret = check_submodule_move_head(ce, NULL,\n> \t\t\t\t\t\t\t oid_to_hex(&ce->oid),\n> \t\t\t\t\t\t\t o);\n> @@ -2093,7 +2093,7 @@ static int merged_entry(const struct cache_entry *ce,\n> \t\t\tinvalidate_ce_path(old, o);\n> \t\t}\n> \n> -\t\tif (submodule_from_ce(ce)) {\n> +\t\tif (submodule_from_ce(ce) && file_exists(ce->name)) {\n> \t\t\tint ret = check_submodule_move_head(ce, oid_to_hex(&old->oid),\n> \t\t\t\t\t\t\t oid_to_hex(&ce->oid),\n> \t\t\t\t\t\t\t o);\n"}]}