* [PATCH 1/6] t7112: remove mention of KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED
2020-02-17 4:53 [PATCH 0/6] unpack-trees: fix '--recurse-submodules' when switching from no submodules to nested submodules Philippe Blain via GitGitGadget
@ 2020-02-17 4:53 ` Philippe Blain via GitGitGadget
2020-02-19 23:26 ` Junio C Hamano
2020-02-17 4:53 ` [PATCH 2/6] t/lib-submodule-update: remove outdated test description Philippe Blain via GitGitGadget
` (4 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: Philippe Blain via GitGitGadget @ 2020-02-17 4:53 UTC (permalink / raw)
To: git
Cc: Jonathan Tan, Stefan Beller, Damien Robert, Philippe Blain,
Philippe Blain
From: Philippe Blain <levraiphilippeblain@gmail.com>
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 <levraiphilippeblain@gmail.com>
---
t/t7112-reset-submodule.sh | 1 -
1 file changed, 1 deletion(-)
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
--
gitgitgadget
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 1/6] t7112: remove mention of KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED
2020-02-17 4:53 ` [PATCH 1/6] t7112: remove mention of KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED Philippe Blain via GitGitGadget
@ 2020-02-19 23:26 ` Junio C Hamano
0 siblings, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2020-02-19 23:26 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget
Cc: git, Jonathan Tan, Stefan Beller, Damien Robert, Philippe Blain
"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> 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 <levraiphilippeblain@gmail.com>
> ---
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/6] t/lib-submodule-update: remove outdated test description
2020-02-17 4:53 [PATCH 0/6] unpack-trees: fix '--recurse-submodules' when switching from no submodules to nested submodules Philippe Blain via GitGitGadget
2020-02-17 4:53 ` [PATCH 1/6] t7112: remove mention of KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED Philippe Blain via GitGitGadget
@ 2020-02-17 4:53 ` Philippe Blain via GitGitGadget
2020-02-17 4:53 ` [PATCH 3/6] t/lib-submodule-update: move a test to the right section Philippe Blain via GitGitGadget
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Philippe Blain via GitGitGadget @ 2020-02-17 4:53 UTC (permalink / raw)
To: git
Cc: Jonathan Tan, Stefan Beller, Damien Robert, Philippe Blain,
Philippe Blain
From: Philippe Blain <levraiphilippeblain@gmail.com>
The commands in the unpack_trees machinery (checkout, reset, read-tree)
were fixed in 218c883783 (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 <levraiphilippeblain@gmail.com>
---
t/lib-submodule-update.sh | 1 -
1 file changed, 1 deletion(-)
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 1dd17fc03e1..5f9eb682f6a 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -908,7 +908,6 @@ test_submodule_switch_recursing_with_args () {
)
'
- # recursing deeper than one level doesn't work yet.
test_expect_success "$command: modified submodule updates submodule recursively" '
prolog &&
reset_work_tree_to_interested add_nested_sub &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 3/6] t/lib-submodule-update: move a test to the right section
2020-02-17 4:53 [PATCH 0/6] unpack-trees: fix '--recurse-submodules' when switching from no submodules to nested submodules Philippe Blain via GitGitGadget
2020-02-17 4:53 ` [PATCH 1/6] t7112: remove mention of KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED Philippe Blain via GitGitGadget
2020-02-17 4:53 ` [PATCH 2/6] t/lib-submodule-update: remove outdated test description Philippe Blain via GitGitGadget
@ 2020-02-17 4:53 ` Philippe Blain via GitGitGadget
2020-02-17 4:53 ` [PATCH 4/6] unpack-trees: remove outdated description for verify_clean_submodule Philippe Blain via GitGitGadget
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Philippe Blain via GitGitGadget @ 2020-02-17 4:53 UTC (permalink / raw)
To: git
Cc: Jonathan Tan, Stefan Beller, Damien Robert, Philippe Blain,
Philippe Blain
From: Philippe Blain <levraiphilippeblain@gmail.com>
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 <levraiphilippeblain@gmail.com>
---
t/lib-submodule-update.sh | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 5f9eb682f6a..417da3602ae 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -658,22 +658,6 @@ test_submodule_recursing_with_args_common() {
test_submodule_content sub1 origin/add_sub1
)
'
- test_expect_success "$command: submodule branch is not changed, detach HEAD instead" '
- prolog &&
- reset_work_tree_to_interested add_sub1 &&
- (
- cd submodule_update &&
- git -C sub1 checkout -b keep_branch &&
- git -C sub1 rev-parse HEAD >expect &&
- git branch -t modify_sub1 origin/modify_sub1 &&
- $command modify_sub1 &&
- test_superproject_content origin/modify_sub1 &&
- test_submodule_content sub1 origin/modify_sub1 &&
- git -C sub1 rev-parse keep_branch >actual &&
- test_cmp expect actual &&
- test_must_fail git -C sub1 symbolic-ref HEAD
- )
- '
# Replacing a tracked file with a submodule produces a checked out submodule
test_expect_success "$command: replace tracked file with submodule checks out submodule" '
@@ -789,6 +773,23 @@ test_submodule_recursing_with_args_common() {
test_submodule_content sub1 origin/add_sub1
)
'
+ # Updating a submodule does not touch the currently checked out branch in the submodule
+ test_expect_success "$command: submodule branch is not changed, detach HEAD instead" '
+ prolog &&
+ reset_work_tree_to_interested add_sub1 &&
+ (
+ cd submodule_update &&
+ git -C sub1 checkout -b keep_branch &&
+ git -C sub1 rev-parse HEAD >expect &&
+ git branch -t modify_sub1 origin/modify_sub1 &&
+ $command modify_sub1 &&
+ test_superproject_content origin/modify_sub1 &&
+ test_submodule_content sub1 origin/modify_sub1 &&
+ git -C sub1 rev-parse keep_branch >actual &&
+ test_cmp expect actual &&
+ test_must_fail git -C sub1 symbolic-ref HEAD
+ )
+ '
}
# Declares and invokes several tests that, in various situations, checks that
--
gitgitgadget
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 4/6] unpack-trees: remove outdated description for verify_clean_submodule
2020-02-17 4:53 [PATCH 0/6] unpack-trees: fix '--recurse-submodules' when switching from no submodules to nested submodules Philippe Blain via GitGitGadget
` (2 preceding siblings ...)
2020-02-17 4:53 ` [PATCH 3/6] t/lib-submodule-update: move a test to the right section Philippe Blain via GitGitGadget
@ 2020-02-17 4:53 ` Philippe Blain via GitGitGadget
2020-02-17 4:53 ` [PATCH 5/6] unpack-trees: check for missing submodule directory in merged_entry Philippe Blain via GitGitGadget
2020-02-17 4:53 ` [PATCH 6/6] t/lib-submodule-update: add test removing nested submodules Philippe Blain via GitGitGadget
5 siblings, 0 replies; 9+ messages in thread
From: Philippe Blain via GitGitGadget @ 2020-02-17 4:53 UTC (permalink / raw)
To: git
Cc: Jonathan Tan, Stefan Beller, Damien Robert, Philippe Blain,
Philippe Blain
From: Philippe Blain <levraiphilippeblain@gmail.com>
The function verify_clean_submodule() learned to verify if a submodule
working tree is clean in a7bc845a9a (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 <levraiphilippeblain@gmail.com>
---
unpack-trees.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/unpack-trees.c b/unpack-trees.c
index da4d6d4ec01..37eca3ede8b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1815,9 +1815,6 @@ static void invalidate_ce_path(const struct cache_entry *ce,
/*
* Check that checking out ce->sha1 in subdir ce->name is not
* going to overwrite any working files.
- *
- * Currently, git does not checkout subprojects during a superproject
- * checkout, so it is not going to overwrite anything.
*/
static int verify_clean_submodule(const char *old_sha1,
const struct cache_entry *ce,
--
gitgitgadget
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 5/6] unpack-trees: check for missing submodule directory in merged_entry
2020-02-17 4:53 [PATCH 0/6] unpack-trees: fix '--recurse-submodules' when switching from no submodules to nested submodules Philippe Blain via GitGitGadget
` (3 preceding siblings ...)
2020-02-17 4:53 ` [PATCH 4/6] unpack-trees: remove outdated description for verify_clean_submodule Philippe Blain via GitGitGadget
@ 2020-02-17 4:53 ` Philippe Blain via GitGitGadget
2020-02-19 23:32 ` Junio C Hamano
2020-02-17 4:53 ` [PATCH 6/6] t/lib-submodule-update: add test removing nested submodules Philippe Blain via GitGitGadget
5 siblings, 1 reply; 9+ messages in thread
From: Philippe Blain via GitGitGadget @ 2020-02-17 4:53 UTC (permalink / raw)
To: git
Cc: Jonathan Tan, Stefan Beller, Damien Robert, Philippe Blain,
Philippe Blain
From: Philippe Blain <levraiphilippeblain@gmail.com>
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/7437BB59-4605-48EC-B05E-E2BDB2D9DABC@gmail.com/
Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
Reported-by: Damien Robert <damien.olivier.robert@gmail.com>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
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);
--
gitgitgadget
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 5/6] unpack-trees: check for missing submodule directory in merged_entry
2020-02-17 4:53 ` [PATCH 5/6] unpack-trees: check for missing submodule directory in merged_entry Philippe Blain via GitGitGadget
@ 2020-02-19 23:32 ` Junio C Hamano
0 siblings, 0 replies; 9+ messages in thread
From: Junio C Hamano @ 2020-02-19 23:32 UTC (permalink / raw)
To: Philippe Blain via GitGitGadget
Cc: git, Jonathan Tan, Stefan Beller, Damien Robert, Philippe Blain
"Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> 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/7437BB59-4605-48EC-B05E-E2BDB2D9DABC@gmail.com/
>
> Reported-by: Philippe Blain <levraiphilippeblain@gmail.com>
> Reported-by: Damien Robert <damien.olivier.robert@gmail.com>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
> 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);
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 6/6] t/lib-submodule-update: add test removing nested submodules
2020-02-17 4:53 [PATCH 0/6] unpack-trees: fix '--recurse-submodules' when switching from no submodules to nested submodules Philippe Blain via GitGitGadget
` (4 preceding siblings ...)
2020-02-17 4:53 ` [PATCH 5/6] unpack-trees: check for missing submodule directory in merged_entry Philippe Blain via GitGitGadget
@ 2020-02-17 4:53 ` Philippe Blain via GitGitGadget
5 siblings, 0 replies; 9+ messages in thread
From: Philippe Blain via GitGitGadget @ 2020-02-17 4:53 UTC (permalink / raw)
To: git
Cc: Jonathan Tan, Stefan Beller, Damien Robert, Philippe Blain,
Philippe Blain
From: Philippe Blain <levraiphilippeblain@gmail.com>
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 <levraiphilippeblain@gmail.com>
---
t/lib-submodule-update.sh | 20 ++++++++++++++++++--
1 file changed, 18 insertions(+), 2 deletions(-)
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index ab30b2da24f..64fc6487dd9 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -297,7 +297,7 @@ test_submodule_content () {
# - Directory containing tracked files replaced by submodule
# - Submodule replaced by tracked files in directory
# - Submodule replaced by tracked file with the same name
-# - tracked file replaced by submodule
+# - Tracked file replaced by submodule
#
# The default is that submodule contents aren't changed until "git submodule
# update" is run. And even then that command doesn't delete the work tree of
@@ -621,12 +621,13 @@ test_submodule_forced_switch () {
# - Directory containing tracked files replaced by submodule
# - Submodule replaced by tracked files in directory
# - Submodule replaced by tracked file with the same name
-# - tracked file replaced by submodule
+# - Tracked file replaced by submodule
#
# 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
+# - Switching from nested submodules to no submodule
# Internal function; use test_submodule_switch_recursing_with_args() or
# test_submodule_forced_switch_recursing_with_args() instead.
@@ -760,6 +761,21 @@ test_submodule_recursing_with_args_common() {
)
'
+ # Switching to a commit without nested submodules removes their worktrees
+ test_expect_success "$command: worktrees of nested submodules are removed" '
+ prolog &&
+ reset_work_tree_to_interested add_nested_sub &&
+ (
+ cd submodule_update &&
+ git branch -t no_submodule origin/no_submodule &&
+ $command no_submodule &&
+ test_superproject_content origin/no_submodule &&
+ ! test_path_is_dir sub1 &&
+ test_must_fail git config -f .git/modules/sub1/config core.worktree &&
+ test_must_fail git config -f .git/modules/sub1/modules/sub2/config core.worktree
+ )
+ '
+
########################## Modified submodule #########################
# Updating a submodule sha1 updates the submodule's work tree
test_expect_success "$command: modified submodule updates submodule work tree" '
--
gitgitgadget
^ permalink raw reply related [flat|nested] 9+ messages in thread