-
Notifications
You must be signed in to change notification settings - Fork 141
unpack-trees: fix '--recurse-submodules' when switching from no submodules to nested submodules #555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
unpack-trees: fix '--recurse-submodules' when switching from no submodules to nested submodules #555
Conversation
The known failure mode KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED was removed from lib-submodule-update.sh in 218c883 (submodule: properly recurse for read-tree and checkout, 2017-05-02) but at that time this change was not ported over to topic sb/reset-recurse-submodules, such that when this topic was merged in 5f074ca (Merge branch 'sb/reset-recurse-submodules', 2017-05-29), t7112-reset-submodules.sh kept a mention of this removed failure mode. Remove it now, as it does not mean anything anymore. Signed-off-by: Philippe Blain <[email protected]>
The commands in the unpack_trees machinery (checkout, reset, read-tree) were fixed in 218c883 (submodule: properly recurse for read-tree and checkout, 2017-05-02) to correctly update nested submodules when called with the `--recurse-submodules` flag. However, a comment in t/lib-submodule-update.sh mentions that this use case still doesn't work. Remove this outdated comment. Signed-off-by: Philippe Blain <[email protected]>
The test "$command: submodule branch is not changed, detach HEAD instead" is in the "Appearing submodule" section of test_submodule_recursing_with_args_common(), but this test updates a submodule; it does not test a transition from a state with no submodule to a state with a submodule. As such, for consistency, move it to the "Modified submodule" section of the same function. While at it, add a comment describing the test. Signed-off-by: Philippe Blain <[email protected]>
The function verify_clean_submodule() learned to verify if a submodule working tree is clean in a7bc845 (unpack-trees: check if we can perform the operation for submodules, 2017-03-14), but the commented description above it was not updated to reflect that, such that this description has been outdated since then. Since Git has now learned to optionnally recursively check out submodules during a superproject checkout, remove this outdated description. Signed-off-by: Philippe Blain <[email protected]>
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 <file>..." 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/[email protected]/ Reported-by: Philippe Blain <[email protected]> Reported-by: Damien Robert <[email protected]> Signed-off-by: Philippe Blain <[email protected]>
The previous commit fixed a bug with the (no submodule) -> (nested submodules) transition for commands in the unpack-trees machinery. Let's add a test for the reverse transition (going from nested submodules to no submodule), as it is not being tested currently. While at it, uniformize the capitalization in the list of tests. Signed-off-by: Philippe Blain <[email protected]>
/submit |
Submitted as [email protected] |
@@ -5,7 +5,6 @@ test_description='reset can handle submodules' | |||
. ./test-lib.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Philippe Blain via GitGitGadget" <[email protected]> writes:
> From: Philippe Blain <[email protected]>
>
> The known failure mode KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED was
> removed from lib-submodule-update.sh in 218c883783 (submodule: properly
> recurse for read-tree and checkout, 2017-05-02) but at that time this
> change was not ported over to topic sb/reset-recurse-submodules, such
> that when this topic was merged in 5f074ca7e8 (Merge branch
> 'sb/reset-recurse-submodules', 2017-05-29), t7112-reset-submodules.sh
> kept a mention of this removed failure mode.
>
> Remove it now, as it does not mean anything anymore.
>
> Signed-off-by: Philippe Blain <[email protected]>
> ---
> t/t7112-reset-submodule.sh | 1 -
> 1 file changed, 1 deletion(-)
Thanks for cleaning up.
>
> diff --git a/t/t7112-reset-submodule.sh b/t/t7112-reset-submodule.sh
> index a1cb9ff858e..67346424a53 100755
> --- a/t/t7112-reset-submodule.sh
> +++ b/t/t7112-reset-submodule.sh
> @@ -5,7 +5,6 @@ test_description='reset can handle submodules'
> . ./test-lib.sh
> . "$TEST_DIRECTORY"/lib-submodule-update.sh
>
> -KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1
> KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1
> KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1
@@ -626,6 +626,7 @@ test_submodule_forced_switch () { | |||
# New test cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Philippe Blain via GitGitGadget" <[email protected]> writes:
> From: Philippe Blain <[email protected]>
>
> 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 <file>..." 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.
Greate to see a detailed write-up. I'll read the surrounding
codepath again later before commenting further.
Thanks.
>
> This bug was first reported in [1].
>
> [1]
> https://lore.kernel.org/git/[email protected]/
>
> Reported-by: Philippe Blain <[email protected]>
> Reported-by: Damien Robert <[email protected]>
> Signed-off-by: Philippe Blain <[email protected]>
> ---
> 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 417da3602ae..ab30b2da24f 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 37eca3ede8b..fc6ba19486d 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -2064,7 +2064,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);
> @@ -2093,7 +2093,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);
This branch is now known as |
This patch series was integrated into pu via git@2f45558. |
This patch series was integrated into pu via git@52e9d91. |
This patch series was integrated into pu via git@9f08090. |
This patch series was integrated into pu via git@e86365d. |
This patch series was integrated into pu via git@498d17c. |
This patch series was integrated into pu via git@db6dd3c. |
This patch series was integrated into pu via git@c4b644e. |
This patch series was integrated into pu via git@bb2e381. |
This patch series was integrated into pu via git@9079fc3. |
This patch series was integrated into pu via git@57d1016. |
This patch series was integrated into pu via git@f692894. |
This patch series was integrated into pu via git@da5062f. |
This patch series was integrated into pu via git@850a371. |
This patch series was integrated into pu via git@8c326ec. |
This patch series was integrated into pu via git@c57e463. |
This patch series was integrated into pu via git@f249f61. |
This patch series was integrated into pu via git@384d1fb. |
This patch series was integrated into next via git@b46922d. |
This patch series was integrated into pu via git@f02bedb. |
This patch series was integrated into pu via git@e4908f0. |
This patch series was integrated into pu via git@6237a7d. |
This patch series was integrated into pu via git@f70743c. |
This patch series was integrated into pu via git@b34a27b. |
This patch series was integrated into pu via git@8746a12. |
This patch series was integrated into pu via git@06d7a77. |
This patch series was integrated into pu via git@8c8f232. |
This patch series was integrated into pu via git@fe87060. |
This patch series was integrated into next via git@fe87060. |
This patch series was integrated into master via git@fe87060. |
Closed via fe87060. |
Currently, using
git checkout --recurse-submodules
(or thesubmodule.recurse
config) to switch from a commit with no submodules to a commit with initialized nested submodules fails because a child git process tries tocd
to a yet non-existing nested submodule directory.reset
andread-tree
are also affected in the same way since they also use the unpack-trees machinery.The 5th commit in this series fixes this bug. The first four are clean-up patches in tests and in unpack-trees that mostly remove outdated comments/dead code. The 6th commit adds a test for the reverse transition (nested submodules -> no submodules) as it was not being tested.
The commit message of the 5th commit is quite long, as I tried to describe clearly what is the cause of the bug.
CC: Jonathan Tan [email protected], Stefan Beller [email protected], Damien Robert [email protected]