From e84704f15ca793a377cb6ef24ab614b95775d008 Mon Sep 17 00:00:00 2001 From: Philippe Blain Date: Mon, 17 Feb 2020 04:53:05 +0000 Subject: [PATCH] unpack-trees: check for missing submodule directory in merged_entry Using `git checkout --recurse-submodules` to switch between a branch with no submodules and a branch with initialized nested submodules currently causes a fatal error: $ git checkout --recurse-submodules branch-with-nested-submodules fatal: exec '--super-prefix=submodule/nested/': cd to 'nested' failed: No such file or directory error: Submodule 'nested' could not be updated. error: Submodule 'submodule/nested' cannot checkout new HEAD. error: Submodule 'submodule' could not be updated. M submodule Switched to branch 'branch-with-nested-submodules' The checkout succeeds but the worktree and index of the first level submodule are left empty: $ cd submodule $ git -c status.submoduleSummary=1 status HEAD detached at b3ce885 Changes to be committed: (use "git restore --staged ..." to unstage) deleted: .gitmodules deleted: first.t deleted: nested fatal: not a git repository: 'nested/.git' Submodule changes to be committed: * nested 1e96f59...0000000: $ git ls-files -s $ # empty $ ls -A .git The reason for the fatal error during the checkout is that a child git process tries to cd into the yet unexisting nested submodule directory. The sequence is the following: 1. The main git process (the one running in the superproject) eventually reaches write_entry() in entry.c, which creates the first level submodule directory and then calls submodule_move_head() in submodule.c, which spawns `git read-tree` in the submodule directory. 2. The first child git process (the one in the submodule of the superproject) eventually calls check_submodule_move_head() at unpack_trees.c:2021, which calls submodule_move_head in dry-run mode, which spawns `git read-tree` in the nested submodule directory. 3. The second child git process tries to chdir() in the yet unexisting nested submodule directory in start_command() at run-command.c:829 and dies before exec'ing. The reason why check_submodule_move_head() is reached in the first child and not in the main process is that it is inside an if(submodule_from_ce()) construct, and submodule_from_ce() returns a valid struct submodule pointer, whereas it returns a null pointer in the main git process. The reason why submodule_from_ce() returns a null pointer in the main git process is because the call to cache_lookup_path() in config_from() (called from submodule_from_path() in submodule_from_ce()) returns a null pointer since the hashmap "for_path" in the submodule_cache of the_repository is not yet populated. It is not populated because both repo_get_oid(repo, GITMODULES_INDEX, &oid) and repo_get_oid(repo, GITMODULES_HEAD, &oid) in config_from_gitmodules() at submodule-config.c:639-640 return -1, as at this stage of the operation, neither the HEAD of the superproject nor its index contain any .gitmodules file. In contrast, in the first child the hashmap is populated because repo_get_oid(repo, GITMODULES_HEAD, &oid) returns 0 as the HEAD of the first level submodule, i.e. .git/modules/submodule/HEAD, points to a commit where .gitmodules is present and records 'nested' as a submodule. Fix this bug by checking that the submodule directory exists before calling check_submodule_move_head() in merged_entry() in the `if(!old)` branch, i.e. if going from a commit with no submodule to a commit with a submodule present. Also protect the other call to check_submodule_move_head() in merged_entry() the same way as it is safer, even though the `else if (!(old->ce_flags & CE_CONFLICTED))` branch of the code is not at play in the present bug. The other calls to check_submodule_move_head() in other functions in unpack_trees.c are all already protected by calls to lstat() somewhere in the program flow so we don't need additional protection for them. All commands in the unpack_trees machinery are affected, i.e. checkout, reset and read-tree when called with the --recurse-submodules flag. This bug was first reported in [1]. [1] https://lore.kernel.org/git/7437BB59-4605-48EC-B05E-E2BDB2D9DABC@gmail.com/ Reported-by: Philippe Blain Reported-by: Damien Robert Signed-off-by: Philippe Blain Signed-off-by: Junio C Hamano --- t/lib-submodule-update.sh | 14 ++++++++++++++ unpack-trees.c | 4 ++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index 417da3602a..ab30b2da24 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -626,6 +626,7 @@ test_submodule_forced_switch () { # New test cases # - Removing a submodule with a git directory absorbs the submodules # git directory first into the superproject. +# - Switching from no submodule to nested submodules # Internal function; use test_submodule_switch_recursing_with_args() or # test_submodule_forced_switch_recursing_with_args() instead. @@ -683,6 +684,19 @@ test_submodule_recursing_with_args_common() { test_submodule_content sub1 origin/replace_directory_with_sub1 ) ' + # Switching to a commit with nested submodules recursively checks them out + test_expect_success "$command: nested submodules are checked out" ' + prolog && + reset_work_tree_to_interested no_submodule && + ( + cd submodule_update && + git branch -t modify_sub1_recursively origin/modify_sub1_recursively && + $command modify_sub1_recursively && + test_superproject_content origin/modify_sub1_recursively && + test_submodule_content sub1 origin/modify_sub1_recursively && + test_submodule_content -C sub1 sub2 origin/modify_sub1_recursively + ) + ' ######################## Disappearing submodule ####################### # Removing a submodule removes its work tree ... diff --git a/unpack-trees.c b/unpack-trees.c index 4ff9077c87..eff499e8d7 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -2060,7 +2060,7 @@ static int merged_entry(const struct cache_entry *ce, } invalidate_ce_path(merge, o); - if (submodule_from_ce(ce)) { + if (submodule_from_ce(ce) && file_exists(ce->name)) { int ret = check_submodule_move_head(ce, NULL, oid_to_hex(&ce->oid), o); @@ -2089,7 +2089,7 @@ static int merged_entry(const struct cache_entry *ce, invalidate_ce_path(old, o); } - if (submodule_from_ce(ce)) { + if (submodule_from_ce(ce) && file_exists(ce->name)) { int ret = check_submodule_move_head(ce, oid_to_hex(&old->oid), oid_to_hex(&ce->oid), o);