* [RFC PATCH 0/6] add: block invalid submodules
@ 2023-02-13 18:21 Calvin Wan
2023-02-13 18:21 ` [RFC PATCH 1/6] leak fix: cache_put_path Calvin Wan
` (6 more replies)
0 siblings, 7 replies; 40+ messages in thread
From: Calvin Wan @ 2023-02-13 18:21 UTC (permalink / raw)
To: git; +Cc: Calvin Wan
I will be taking over a series that Josh was working on before he
went on parental leave. For this RFC, I was wondering whether this
approach to blocking users from eventually commiting invalid
submodules is feasible. Eg. should we block at commit time instead?
And if so, how would that approach look?
=== Cover letter ===
At $dayjob, we believe that we need to change the default behavior
around adding embedded repositories. Currently, we issue advice that
embedded repositories cannot be checked out when the top-level repo is
cloned, and that the user should probably use a submodule instead.
However, our experience is that many users ignore this advice, and
commit embedded repositories to shared projects. This causes toil for
repo administrators who must correct these mistakes.
The main point of this series is to change the warning issued in this
case into a fatal error. However, this breaks lots of test cases that
were implemented prior to the original creation of the warning, so the
bulk of the changes are test cleanups.
For reviewer convenience, I've tried to group the test changes with
similar structure into the same commits:
* Patch 1: Fixes a leak in submodule-config.c
* Patch 2: Move setup code into test_expect blocks
* Patch 3: Substitute `git submodule add` instead of `git add` for
simple cases (no other adjustments necessary)
* Patch 4: As in patch 2, but also adjust expected diff output
* Patch 5: As in patch 2, but also adjust expected status output
* Patch 6: Change the embedded repo warning to a fatal error
An open question:
* Currently we can bypass the embedded repo check with
`--no-warn-embedded-repo`. I've kept the name of the flag the same for
backwards compatibility. Should we instead rename the flag?
Calvin Wan (1):
leak fix: cache_put_path
Josh Steadmon (5):
t4041, t4060: modernize test style
tests: Use `git submodule add` instead of `git add`
tests: use `git submodule add` and fix expected diffs
tests: use `git submodule add` and fix expected status
add: reject nested repositories
Documentation/git-add.txt | 7 +-
builtin/add.c | 28 ++-
submodule-config.c | 4 +-
t/t0008-ignores.sh | 2 +-
t/t2013-checkout-submodule.sh | 4 +-
t/t2103-update-index-ignore-missing.sh | 2 +-
t/t2107-update-index-basic.sh | 2 +-
t/t3040-subprojects-basic.sh | 5 +-
t/t3050-subprojects-fetch.sh | 3 +-
t/t3404-rebase-interactive.sh | 3 +-
t/t3701-add-interactive.sh | 5 +-
t/t4010-diff-pathspec.sh | 2 +-
t/t4020-diff-external.sh | 2 +-
t/t4027-diff-submodule.sh | 17 +-
t/t4035-diff-quiet.sh | 1 +
t/t4041-diff-submodule-option.sh | 228 +++++++++++++++----
t/t4060-diff-submodule-option-diff-format.sh | 224 +++++++++++++-----
t/t5531-deep-submodule-push.sh | 4 +-
t/t6416-recursive-corner-cases.sh | 12 +-
t/t6430-merge-recursive.sh | 1 +
t/t6437-submodule-merge.sh | 12 +-
t/t7400-submodule-basic.sh | 4 +-
t/t7401-submodule-summary.sh | 4 +-
t/t7402-submodule-rebase.sh | 2 +-
t/t7412-submodule-absorbgitdirs.sh | 2 +-
t/t7414-submodule-mistakes.sh | 21 +-
t/t7450-bad-git-dotfiles.sh | 2 +-
t/t7506-status-submodule.sh | 15 +-
t/t7508-status.sh | 134 +++++++++--
29 files changed, 569 insertions(+), 183 deletions(-)
--
2.39.1.581.gbfd45094c4-goog
^ permalink raw reply [flat|nested] 40+ messages in thread* [RFC PATCH 1/6] leak fix: cache_put_path 2023-02-13 18:21 [RFC PATCH 0/6] add: block invalid submodules Calvin Wan @ 2023-02-13 18:21 ` Calvin Wan 2023-02-13 19:23 ` Junio C Hamano 2023-02-13 18:21 ` [RFC PATCH 2/6] t4041, t4060: modernize test style Calvin Wan ` (5 subsequent siblings) 6 siblings, 1 reply; 40+ messages in thread From: Calvin Wan @ 2023-02-13 18:21 UTC (permalink / raw) To: git; +Cc: Calvin Wan hashmap_put returns a pointer if the key was found and subsequently replaced. Free this pointer so it isn't leaked. Signed-off-by: Calvin Wan <calvinwan@google.com> --- submodule-config.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/submodule-config.c b/submodule-config.c index 4dc61b3a78..90cab34568 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -128,9 +128,11 @@ static void cache_put_path(struct submodule_cache *cache, unsigned int hash = hash_oid_string(&submodule->gitmodules_oid, submodule->path); struct submodule_entry *e = xmalloc(sizeof(*e)); + struct hashmap_entry *replaced; hashmap_entry_init(&e->ent, hash); e->config = submodule; - hashmap_put(&cache->for_path, &e->ent); + replaced = hashmap_put(&cache->for_path, &e->ent); + free(replaced); } static void cache_remove_path(struct submodule_cache *cache, -- 2.39.1.581.gbfd45094c4-goog ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [RFC PATCH 1/6] leak fix: cache_put_path 2023-02-13 18:21 ` [RFC PATCH 1/6] leak fix: cache_put_path Calvin Wan @ 2023-02-13 19:23 ` Junio C Hamano 2023-02-14 19:56 ` Calvin Wan 0 siblings, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2023-02-13 19:23 UTC (permalink / raw) To: Calvin Wan; +Cc: git Calvin Wan <calvinwan@google.com> writes: > hashmap_put returns a pointer if the key was found and subsequently > replaced. Free this pointer so it isn't leaked. > > Signed-off-by: Calvin Wan <calvinwan@google.com> > --- > submodule-config.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/submodule-config.c b/submodule-config.c > index 4dc61b3a78..90cab34568 100644 > --- a/submodule-config.c > +++ b/submodule-config.c > @@ -128,9 +128,11 @@ static void cache_put_path(struct submodule_cache *cache, > unsigned int hash = hash_oid_string(&submodule->gitmodules_oid, > submodule->path); > struct submodule_entry *e = xmalloc(sizeof(*e)); > + struct hashmap_entry *replaced; > hashmap_entry_init(&e->ent, hash); > e->config = submodule; > - hashmap_put(&cache->for_path, &e->ent); > + replaced = hashmap_put(&cache->for_path, &e->ent); > + free(replaced); > } Out of curiosity, I've checked all the grep hits from hashmap_put() in the codebase and this seems to be the only one. Everybody else either calls hashmap_put() only after hashmap_get() sees that there is no existing one, or unconditionally calls hashmap_put() and dies if an earlier registration is found. The callers of oidmap_put() in sequencer.c I didn't check. There might be similar leaks there, or they may be safe---I dunno. But all other callers of oidmap_put() also seem to be safe. Back to the patch itself. The only caller of this function does if (submodule->path) { cache_remove_path(me->cache, submodule); free(submodule->path); } submodule->path = xstrdup(value); cache_put_path(me->cache, submodule); It is curious how the same submodule->path is occupied by more than one submodule? Isn't that a configuration error we want to report to the user somehow (not necessarily error/die), instead of silently replacing with the "last one wins" precedence? Assuming that the "last one wins" is the sensible thing to do, the change proposed by this patch does seem reasonable way to plug the leak. Thanks. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH 1/6] leak fix: cache_put_path 2023-02-13 19:23 ` Junio C Hamano @ 2023-02-14 19:56 ` Calvin Wan 2023-02-14 21:08 ` Junio C Hamano 0 siblings, 1 reply; 40+ messages in thread From: Calvin Wan @ 2023-02-14 19:56 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Mon, Feb 13, 2023 at 11:23 AM Junio C Hamano <gitster@pobox.com> wrote: > > Calvin Wan <calvinwan@google.com> writes: > > > hashmap_put returns a pointer if the key was found and subsequently > > replaced. Free this pointer so it isn't leaked. > > > > Signed-off-by: Calvin Wan <calvinwan@google.com> > > --- > > submodule-config.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/submodule-config.c b/submodule-config.c > > index 4dc61b3a78..90cab34568 100644 > > --- a/submodule-config.c > > +++ b/submodule-config.c > > @@ -128,9 +128,11 @@ static void cache_put_path(struct submodule_cache *cache, > > unsigned int hash = hash_oid_string(&submodule->gitmodules_oid, > > submodule->path); > > struct submodule_entry *e = xmalloc(sizeof(*e)); > > + struct hashmap_entry *replaced; > > hashmap_entry_init(&e->ent, hash); > > e->config = submodule; > > - hashmap_put(&cache->for_path, &e->ent); > > + replaced = hashmap_put(&cache->for_path, &e->ent); > > + free(replaced); > > } > > Out of curiosity, I've checked all the grep hits from hashmap_put() > in the codebase and this seems to be the only one. Everybody else > either calls hashmap_put() only after hashmap_get() sees that there > is no existing one, or unconditionally calls hashmap_put() and dies > if an earlier registration is found. > > The callers of oidmap_put() in sequencer.c I didn't check. There > might be similar leaks there, or they may be safe---I dunno. But > all other callers of oidmap_put() also seem to be safe. > > Back to the patch itself. The only caller of this function does > > if (submodule->path) { > cache_remove_path(me->cache, submodule); > free(submodule->path); > } > submodule->path = xstrdup(value); > cache_put_path(me->cache, submodule); > > It is curious how the same submodule->path is occupied by more than > one submodule? Isn't that a configuration error we want to report > to the user somehow (not necessarily error/die), instead of silently > replacing with the "last one wins" precedence? > > Assuming that the "last one wins" is the sensible thing to do, the > change proposed by this patch does seem reasonable way to plug the > leak. Swapping this functionality to "first one wins" or erroring out breaks many tests that are setup improperly. If we continue with the "last one wins" precedence, then a warning and documentation should be added. We definitely should not swap it to "first one wins" -- one doesn't make sense than the other, but "last one wins" at least has precedence. If we choose to error out during config parsing when duplicated submodule paths are detected, then those respective tests will also need to be updated. I'm leaning towards leaving the functionality as is since a user would have to manually edit the .gitmodules file to get into the state and is protected from it with `git submodule add`. What do you think about adding a warning and possibly documentation? ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH 1/6] leak fix: cache_put_path 2023-02-14 19:56 ` Calvin Wan @ 2023-02-14 21:08 ` Junio C Hamano 2023-02-14 21:39 ` Calvin Wan 0 siblings, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2023-02-14 21:08 UTC (permalink / raw) To: Calvin Wan; +Cc: git Calvin Wan <calvinwan@google.com> writes: >> Assuming that the "last one wins" is the sensible thing to do, the >> change proposed by this patch does seem reasonable way to plug the >> leak. > > Swapping this functionality to "first one wins" or erroring out breaks many > tests that are setup improperly. If we continue with the "last one wins" > precedence, then a warning and documentation should be added. We > definitely should not swap it to "first one wins" -- one doesn't make sense > than the other, but "last one wins" at least has precedence. If we choose > to error out during config parsing when duplicated submodule paths are > detected, then those respective tests will also need to be updated. These tests expect the same submodule to be registered at different paths? Is that a set-up that is expected to happen commonly in real life? If so, yes, the current behaviour needs to be kept (with some documentation to explain why it makes sense and how it is usefully used). If not, these tests may need to be updated to test scenarios that are closer to the real life, I guess, plus an additional test that makes sure such a .gitmodules file is diagnosed as an error with the code to do that some time in the future. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH 1/6] leak fix: cache_put_path 2023-02-14 21:08 ` Junio C Hamano @ 2023-02-14 21:39 ` Calvin Wan 2023-02-14 21:59 ` Junio C Hamano 0 siblings, 1 reply; 40+ messages in thread From: Calvin Wan @ 2023-02-14 21:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Tue, Feb 14, 2023 at 1:08 PM Junio C Hamano <gitster@pobox.com> wrote: > > Calvin Wan <calvinwan@google.com> writes: > > >> Assuming that the "last one wins" is the sensible thing to do, the > >> change proposed by this patch does seem reasonable way to plug the > >> leak. > > > > Swapping this functionality to "first one wins" or erroring out breaks many > > tests that are setup improperly. If we continue with the "last one wins" > > precedence, then a warning and documentation should be added. We > > definitely should not swap it to "first one wins" -- one doesn't make sense > > than the other, but "last one wins" at least has precedence. If we choose > > to error out during config parsing when duplicated submodule paths are > > detected, then those respective tests will also need to be updated. > > These tests expect the same submodule to be registered at different > paths? Is that a set-up that is expected to happen commonly in real > life? If so, yes, the current behaviour needs to be kept (with some > documentation to explain why it makes sense and how it is usefully > used). If not, these tests may need to be updated to test scenarios > that are closer to the real life, I guess, plus an additional test > that makes sure such a .gitmodules file is diagnosed as an error > with the code to do that some time in the future. If you look at t4027-diff-submodule.sh:git diff HEAD with dirty submodule (work tree, refs match) [.gitmodules], it's two different submodules pointing to the same path (or more precisely a second submodule is manually added to .gitmodules with a path to the first submodule). This is a setup that is not expected to happen commonly in real life so updating this test makes sense to me. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH 1/6] leak fix: cache_put_path 2023-02-14 21:39 ` Calvin Wan @ 2023-02-14 21:59 ` Junio C Hamano 0 siblings, 0 replies; 40+ messages in thread From: Junio C Hamano @ 2023-02-14 21:59 UTC (permalink / raw) To: Calvin Wan; +Cc: git Calvin Wan <calvinwan@google.com> writes: > If you look at t4027-diff-submodule.sh:git diff HEAD with dirty submodule > (work tree, refs match) [.gitmodules], it's two different submodules > pointing to the same path (or more precisely a second submodule is > manually added to .gitmodules with a path to the first submodule). This > is a setup that is not expected to happen commonly in real life so > updating this test makes sense to me. Thanks for double checking. If it is a configuration that is an error, then we would want to notice it eventually (probably outside the scope of this series), and before that happens, I agree that modifying the test to avoid such an errnoneous scenario would be a very good idea---otherwise we'd be pretending that such a scenario is a supported configuration somehow and misleading developers and users. Thanks. ^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH 2/6] t4041, t4060: modernize test style 2023-02-13 18:21 [RFC PATCH 0/6] add: block invalid submodules Calvin Wan 2023-02-13 18:21 ` [RFC PATCH 1/6] leak fix: cache_put_path Calvin Wan @ 2023-02-13 18:21 ` Calvin Wan 2023-02-13 19:41 ` Junio C Hamano 2023-02-13 18:21 ` [RFC PATCH 3/6] tests: Use `git submodule add` instead of `git add` Calvin Wan ` (4 subsequent siblings) 6 siblings, 1 reply; 40+ messages in thread From: Calvin Wan @ 2023-02-13 18:21 UTC (permalink / raw) To: git; +Cc: Josh Steadmon, Calvin Wan From: Josh Steadmon <steadmon@google.com> In preparation for later changes, move setup code into test_expect blocks. Smaller sections are moved into existing blocks, while larger sections with a standalone purpose are given their own new blocks. This makes sure that later changes that may break the test setup are easier to diagnose, because errors will caught immediately rather than in later unrelated test_expect blocks. Signed-off-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> --- t/t4041-diff-submodule-option.sh | 75 +++++++-------- t/t4060-diff-submodule-option-diff-format.sh | 99 ++++++++++---------- 2 files changed, 87 insertions(+), 87 deletions(-) diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh index 0c1502d4b0..556682b18b 100755 --- a/t/t4041-diff-submodule-option.sh +++ b/t/t4041-diff-submodule-option.sh @@ -42,11 +42,12 @@ commit_file () { git commit "$@" -m "Commit $*" >/dev/null } -test_create_repo sm1 && -add_file . foo >/dev/null - -head1=$(add_file sm1 foo1 foo2) -fullhead1=$(cd sm1; git rev-parse --verify HEAD) +test_expect_success 'setup' ' + test_create_repo sm1 && + add_file . foo >/dev/null && + head1=$(add_file sm1 foo1 foo2) && + fullhead1=$(cd sm1 && git rev-parse --verify HEAD) +' test_expect_success 'added submodule' ' git add sm1 && @@ -99,10 +100,9 @@ test_expect_success 'diff.submodule does not affect plumbing' ' test_cmp expected actual ' -commit_file sm1 && -head2=$(add_file sm1 foo3) - test_expect_success 'modified submodule(forward)' ' + commit_file sm1 && + head2=$(add_file sm1 foo3) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head1..$head2: @@ -129,8 +129,8 @@ test_expect_success 'modified submodule(forward) --submodule' ' test_cmp expected actual ' -fullhead2=$(cd sm1; git rev-parse --verify HEAD) test_expect_success 'modified submodule(forward) --submodule=short' ' + fullhead2=$(cd sm1 && git rev-parse --verify HEAD) && git diff --submodule=short >actual && cat >expected <<-EOF && diff --git a/sm1 b/sm1 @@ -144,14 +144,14 @@ test_expect_success 'modified submodule(forward) --submodule=short' ' test_cmp expected actual ' -commit_file sm1 && -head3=$( - cd sm1 && - git reset --hard HEAD~2 >/dev/null && - git rev-parse --short --verify HEAD -) test_expect_success 'modified submodule(backward)' ' + commit_file sm1 && + head3=$( + cd sm1 && + git reset --hard HEAD~2 >/dev/null && + git rev-parse --short --verify HEAD + ) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head2..$head3 (rewind): @@ -161,8 +161,8 @@ test_expect_success 'modified submodule(backward)' ' test_cmp expected actual ' -head4=$(add_file sm1 foo4 foo5) test_expect_success 'modified submodule(backward and forward)' ' + head4=$(add_file sm1 foo4 foo5) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head2...$head4: @@ -174,13 +174,15 @@ test_expect_success 'modified submodule(backward and forward)' ' test_cmp expected actual ' -commit_file sm1 && -mv sm1 sm1-bak && -echo sm1 >sm1 && -head5=$(git hash-object sm1 | cut -c1-7) && -git add sm1 && -rm -f sm1 && -mv sm1-bak sm1 +test_expect_success 'setup - change sm1 to a blob' ' + commit_file sm1 && + mv sm1 sm1-bak && + echo sm1 >sm1 && + head5=$(git hash-object sm1 | cut -c1-7) && + git add sm1 && + rm -f sm1 && + mv sm1-bak sm1 +' test_expect_success 'typechanged submodule(submodule->blob), --cached' ' git diff --submodule=log --cached >actual && @@ -212,9 +214,9 @@ test_expect_success 'typechanged submodule(submodule->blob)' ' test_cmp expected actual ' -rm -rf sm1 && -git checkout-index sm1 test_expect_success 'typechanged submodule(submodule->blob)' ' + rm -rf sm1 && + git checkout-index sm1 && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head4...0000000 (submodule deleted) @@ -229,11 +231,11 @@ test_expect_success 'typechanged submodule(submodule->blob)' ' test_cmp expected actual ' -rm -f sm1 && -test_create_repo sm1 && -head6=$(add_file sm1 foo6 foo7) -fullhead6=$(cd sm1; git rev-parse --verify HEAD) test_expect_success 'nonexistent commit' ' + rm -f sm1 && + test_create_repo sm1 && + head6=$(add_file sm1 foo6 foo7) && + fullhead6=$(cd sm1 && git rev-parse --verify HEAD) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head4...$head6 (commits not present) @@ -241,8 +243,8 @@ test_expect_success 'nonexistent commit' ' test_cmp expected actual ' -commit_file test_expect_success 'typechanged submodule(blob->submodule)' ' + commit_file && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && diff --git a/sm1 b/sm1 @@ -257,8 +259,8 @@ test_expect_success 'typechanged submodule(blob->submodule)' ' test_cmp expected actual ' -commit_file sm1 && test_expect_success 'submodule is up to date' ' + commit_file sm1 && git diff-index -p --submodule=log HEAD >actual && test_must_be_empty actual ' @@ -327,9 +329,9 @@ test_expect_success 'submodule contains modified content' ' test_cmp expected actual ' -(cd sm1; git commit -mchange foo6 >/dev/null) && -head8=$(cd sm1; git rev-parse --short --verify HEAD) && test_expect_success 'submodule is modified' ' + (cd sm1 && git commit -mchange foo6 >/dev/null) && + head8=$(cd sm1 && git rev-parse --short --verify HEAD) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head6..$head8: @@ -422,8 +424,8 @@ test_expect_success 'modified submodule contains modified content' ' test_cmp expected actual ' -rm -rf sm1 test_expect_success 'deleted submodule' ' + rm -rf sm1 && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head6...0000000 (submodule deleted) @@ -454,8 +456,8 @@ test_expect_success 'path filter' ' test_cmp expected actual ' -commit_file sm2 test_expect_success 'given commit' ' + commit_file sm2 && git diff-index -p --submodule=log HEAD^ >actual && cat >expected <<-EOF && Submodule sm1 $head6...0000000 (submodule deleted) @@ -473,9 +475,8 @@ test_expect_success 'given commit --submodule' ' test_cmp expected actual ' -fullhead7=$(cd sm2; git rev-parse --verify HEAD) - test_expect_success 'given commit --submodule=short' ' + fullhead7=$(cd sm2 && git rev-parse --verify HEAD) && git diff-index -p --submodule=short HEAD^ >actual && cat >expected <<-EOF && diff --git a/sm1 b/sm1 diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh index 97c6424cd5..3cda8ffd14 100755 --- a/t/t4060-diff-submodule-option-diff-format.sh +++ b/t/t4060-diff-submodule-option-diff-format.sh @@ -138,10 +138,9 @@ test_expect_success 'diff.submodule does not affect plumbing' ' diff_cmp expected actual ' -commit_file sm1 && -head2=$(add_file sm1 foo3) - test_expect_success 'modified submodule(forward)' ' + commit_file sm1 && + head2=$(add_file sm1 foo3) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head1..$head2: @@ -180,8 +179,8 @@ test_expect_success 'modified submodule(forward) --submodule' ' diff_cmp expected actual ' -fullhead2=$(cd sm1; git rev-parse --verify HEAD) test_expect_success 'modified submodule(forward) --submodule=short' ' + fullhead2=$(cd sm1 && git rev-parse --verify HEAD) && git diff --submodule=short >actual && cat >expected <<-EOF && diff --git a/sm1 b/sm1 @@ -195,14 +194,13 @@ test_expect_success 'modified submodule(forward) --submodule=short' ' diff_cmp expected actual ' -commit_file sm1 && -head3=$( - cd sm1 && - git reset --hard HEAD~2 >/dev/null && - git rev-parse --short --verify HEAD -) - test_expect_success 'modified submodule(backward)' ' + commit_file sm1 && + head3=$( + cd sm1 && + git reset --hard HEAD~2 >/dev/null && + git rev-parse --short --verify HEAD + ) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head2..$head3 (rewind): @@ -224,8 +222,8 @@ test_expect_success 'modified submodule(backward)' ' diff_cmp expected actual ' -head4=$(add_file sm1 foo4 foo5) test_expect_success 'modified submodule(backward and forward)' ' + head4=$(add_file sm1 foo4 foo5) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head2...$head4: @@ -261,13 +259,15 @@ test_expect_success 'modified submodule(backward and forward)' ' diff_cmp expected actual ' -commit_file sm1 && -mv sm1 sm1-bak && -echo sm1 >sm1 && -head5=$(git hash-object sm1 | cut -c1-7) && -git add sm1 && -rm -f sm1 && -mv sm1-bak sm1 +test_expect_success 'setup - change sm1 to a blob' ' + commit_file sm1 && + mv sm1 sm1-bak && + echo sm1 >sm1 && + head5=$(git hash-object sm1 | cut -c1-7) && + git add sm1 && + rm -f sm1 && + mv sm1-bak sm1 +' test_expect_success 'typechanged submodule(submodule->blob), --cached' ' git diff --submodule=diff --cached >actual && @@ -341,9 +341,9 @@ test_expect_success 'typechanged submodule(submodule->blob)' ' diff_cmp expected actual ' -rm -rf sm1 && -git checkout-index sm1 test_expect_success 'typechanged submodule(submodule->blob)' ' + rm -rf sm1 && + git checkout-index sm1 && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head4...0000000 (submodule deleted) @@ -358,10 +358,10 @@ test_expect_success 'typechanged submodule(submodule->blob)' ' diff_cmp expected actual ' -rm -f sm1 && -test_create_repo sm1 && -head6=$(add_file sm1 foo6 foo7) test_expect_success 'nonexistent commit' ' + rm -f sm1 && + test_create_repo sm1 && + head6=$(add_file sm1 foo6 foo7) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head4...$head6 (commits not present) @@ -369,8 +369,8 @@ test_expect_success 'nonexistent commit' ' diff_cmp expected actual ' -commit_file test_expect_success 'typechanged submodule(blob->submodule)' ' + commit_file && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && diff --git a/sm1 b/sm1 @@ -399,8 +399,8 @@ test_expect_success 'typechanged submodule(blob->submodule)' ' diff_cmp expected actual ' -commit_file sm1 && test_expect_success 'submodule is up to date' ' + commit_file sm1 && head7=$(git -C sm1 rev-parse --short --verify HEAD) && git diff-index -p --submodule=diff HEAD >actual && test_must_be_empty actual @@ -492,9 +492,9 @@ test_expect_success 'submodule contains modified content' ' diff_cmp expected actual ' -(cd sm1; git commit -mchange foo6 >/dev/null) && -head8=$(cd sm1; git rev-parse --short --verify HEAD) && test_expect_success 'submodule is modified' ' + (cd sm1 && git commit -mchange foo6 >/dev/null) && + head8=$(cd sm1 && git rev-parse --short --verify HEAD) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head7..$head8: @@ -643,8 +643,8 @@ test_expect_success 'modified submodule contains modified content' ' diff_cmp expected actual ' -rm -rf sm1 test_expect_success 'deleted submodule' ' + rm -rf sm1 && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head7...0000000 (submodule deleted) @@ -703,13 +703,15 @@ test_expect_success 'path filter' ' diff_cmp expected actual ' -cat >.gitmodules <<-EOF -[submodule "sm2"] - path = sm2 - url = bogus_url -EOF -git add .gitmodules -commit_file sm2 .gitmodules +test_expect_success 'setup - construct .gitmodules' ' + cat >.gitmodules <<-EOF && + [submodule "sm2"] + path = sm2 + url = bogus_url + EOF + git add .gitmodules && + commit_file sm2 .gitmodules +' test_expect_success 'given commit' ' git diff-index -p --submodule=diff HEAD^ >actual && @@ -779,9 +781,8 @@ test_expect_success 'diff --submodule=diff with .git file' ' diff_cmp expected actual ' -mv sm2 sm2-bak - test_expect_success 'deleted submodule with .git file' ' + mv sm2 sm2-bak && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head7...0000000 (submodule deleted) @@ -804,9 +805,8 @@ test_expect_success 'deleted submodule with .git file' ' diff_cmp expected actual ' -echo submodule-to-blob>sm2 - test_expect_success 'typechanged(submodule->blob) submodule with .git file' ' + echo submodule-to-blob>sm2 && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head7...0000000 (submodule deleted) @@ -836,10 +836,9 @@ test_expect_success 'typechanged(submodule->blob) submodule with .git file' ' diff_cmp expected actual ' -rm sm2 -mv sm2-bak sm2 - test_expect_success 'setup nested submodule' ' + rm sm2 && + mv sm2-bak sm2 && git -c protocol.file.allow=always -C sm2 submodule add ../sm2 nested && git -C sm2 commit -a -m "nested sub" && head10=$(git -C sm2 rev-parse --short --verify HEAD) @@ -910,13 +909,11 @@ test_expect_success 'diff --submodule=diff recurses into nested submodules' ' diff_cmp expected actual ' -(cd sm2; commit_file nested) -commit_file sm2 -head12=$(cd sm2; git rev-parse --short --verify HEAD) - -mv sm2 sm2-bak - test_expect_success 'diff --submodule=diff recurses into deleted nested submodules' ' + (cd sm2 && commit_file nested) && + commit_file sm2 && + head12=$(cd sm2 && git rev-parse --short --verify HEAD) && + mv sm2 sm2-bak && cat >expected <<-EOF && Submodule sm1 $head7...0000000 (submodule deleted) Submodule sm2 $head12...0000000 (submodule deleted) @@ -971,6 +968,8 @@ test_expect_success 'diff --submodule=diff recurses into deleted nested submodul diff_cmp expected actual ' -mv sm2-bak sm2 +test_expect_success 'submodule cleanup' ' + mv sm2-bak sm2 +' test_done -- 2.39.1.581.gbfd45094c4-goog ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [RFC PATCH 2/6] t4041, t4060: modernize test style 2023-02-13 18:21 ` [RFC PATCH 2/6] t4041, t4060: modernize test style Calvin Wan @ 2023-02-13 19:41 ` Junio C Hamano 2023-02-14 20:22 ` Calvin Wan 0 siblings, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2023-02-13 19:41 UTC (permalink / raw) To: Calvin Wan; +Cc: git, Josh Steadmon Calvin Wan <calvinwan@google.com> writes: > From: Josh Steadmon <steadmon@google.com> > > In preparation for later changes, move setup code into test_expect > blocks. Smaller sections are moved into existing blocks, while larger > sections with a standalone purpose are given their own new blocks. Nice. > -test_create_repo sm1 && > -add_file . foo >/dev/null > - > -head1=$(add_file sm1 foo1 foo2) > -fullhead1=$(cd sm1; git rev-parse --verify HEAD) > +test_expect_success 'setup' ' > + test_create_repo sm1 && > + add_file . foo >/dev/null && Now this is inside test_expect_success, redirection to /dev/null is unnecessary. > + head1=$(add_file sm1 foo1 foo2) && > + fullhead1=$(cd sm1 && git rev-parse --verify HEAD) > +' Or "fullhead1=$(git -C sm1 rev-parse ...)". Both of the above can be ignored if we are trying to be a strict rewrite of the original, but moving code inside test_expect_success block is a large enough change that there may not be much point in avoiding such an obvious modernization "while at it". > -rm sm2 > -mv sm2-bak sm2 > - > test_expect_success 'setup nested submodule' ' > + rm sm2 && > + mv sm2-bak sm2 && To me, this looks more like something test_when_finished in the test that wanted not to have sm2 (i.e. "deleted submodule with .git file") should have done as part of its own clean-up. There certainly can be two schools of thought when it comes to how to arrange the precondition of subsequent tests. More modern tests tend to clean after themselves by reverting the damage they made to the environment inside test_when_finished in themselves. This one, as the posted patch does, goes to the other extreme and forces the subsequent test to undo the damage done by the previous ones. The latter approach has two major downsides. It would not work if the tester wants to skip an earlier step, or if an earlier step failed to cause the expected damage this step wants to undo. The correctness of "what we should see as sm2 here must be in sm2-bak because we know an earlier step should have moved it there" can easily be broken. It also makes it harder to update the earlier step to cause different damage to the environment---the "undoing the damage done by the previous step(s)" done as early parts of this step also needs to be updated. Whichever approach we pick to use in each script, it would be better to stick to one philosophy, and if we can make each step revert the damage it caused when it is done, that would be nice. > -mv sm2-bak sm2 > +test_expect_success 'submodule cleanup' ' > + mv sm2-bak sm2 > +' > > test_done Likewise. Thanks. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH 2/6] t4041, t4060: modernize test style 2023-02-13 19:41 ` Junio C Hamano @ 2023-02-14 20:22 ` Calvin Wan 0 siblings, 0 replies; 40+ messages in thread From: Calvin Wan @ 2023-02-14 20:22 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Josh Steadmon > > -test_create_repo sm1 && > > -add_file . foo >/dev/null > > - > > -head1=$(add_file sm1 foo1 foo2) > > -fullhead1=$(cd sm1; git rev-parse --verify HEAD) > > +test_expect_success 'setup' ' > > + test_create_repo sm1 && > > + add_file . foo >/dev/null && > > Now this is inside test_expect_success, redirection to /dev/null is > unnecessary. ack. > > > + head1=$(add_file sm1 foo1 foo2) && > > + fullhead1=$(cd sm1 && git rev-parse --verify HEAD) > > +' > > Or "fullhead1=$(git -C sm1 rev-parse ...)". > > Both of the above can be ignored if we are trying to be a strict > rewrite of the original, but moving code inside test_expect_success > block is a large enough change that there may not be much point in > avoiding such an obvious modernization "while at it". I agree, will fix. > > > -rm sm2 > > -mv sm2-bak sm2 > > - > > test_expect_success 'setup nested submodule' ' > > + rm sm2 && > > + mv sm2-bak sm2 && > > To me, this looks more like something test_when_finished in the test > that wanted not to have sm2 (i.e. "deleted submodule with .git file") > should have done as part of its own clean-up. > > There certainly can be two schools of thought when it comes to how to > arrange the precondition of subsequent tests. More modern tests tend > to clean after themselves by reverting the damage they made to the > environment inside test_when_finished in themselves. This one, as > the posted patch does, goes to the other extreme and forces the > subsequent test to undo the damage done by the previous ones. > > The latter approach has two major downsides. It would not work if > the tester wants to skip an earlier step, or if an earlier step > failed to cause the expected damage this step wants to undo. The > correctness of "what we should see as sm2 here must be in sm2-bak > because we know an earlier step should have moved it there" can > easily be broken. It also makes it harder to update the earlier > step to cause different damage to the environment---the "undoing the > damage done by the previous step(s)" done as early parts of this > step also needs to be updated. > > Whichever approach we pick to use in each script, it would be better > to stick to one philosophy, and if we can make each step revert the > damage it caused when it is done, that would be nice. > > > -mv sm2-bak sm2 > > +test_expect_success 'submodule cleanup' ' > > + mv sm2-bak sm2 > > +' > > > > test_done > > Likewise. ack. Swapping to test_when_finished ^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH 3/6] tests: Use `git submodule add` instead of `git add` 2023-02-13 18:21 [RFC PATCH 0/6] add: block invalid submodules Calvin Wan 2023-02-13 18:21 ` [RFC PATCH 1/6] leak fix: cache_put_path Calvin Wan 2023-02-13 18:21 ` [RFC PATCH 2/6] t4041, t4060: modernize test style Calvin Wan @ 2023-02-13 18:21 ` Calvin Wan 2023-02-13 18:21 ` [RFC PATCH 4/6] tests: use `git submodule add` and fix expected diffs Calvin Wan ` (3 subsequent siblings) 6 siblings, 0 replies; 40+ messages in thread From: Calvin Wan @ 2023-02-13 18:21 UTC (permalink / raw) To: git; +Cc: Josh Steadmon, Calvin Wan From: Josh Steadmon <steadmon@google.com> Since 532139940c (add: warn when adding an embedded repository, 2017-06-14), we have recognized that adding embedded repositories is a subpar user experience compared to submodules. Embedded repositories lack a .gitmodules entry, which means clones of the top-level repo will be unable to obtain the nested repo. We issue advice about this situation in the case where a user adds an embedded repository. However, many tests predate this advice, and directly add embedded repositories instead of using submodules. This commit cleans up such test cases where minimal other changes are required (e.g., committing later changes to .gitmodules or changing the counts of committed files in the worktree). Future commits will handle tests requiring more complicated adjustments. These changes will enable us to switch the default behavior of git-add from warning about embedded repositories to rejecting them outright. See later commits for a further discussion of that topic. Signed-off-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> --- t/t0008-ignores.sh | 2 +- t/t2013-checkout-submodule.sh | 4 +++- t/t2103-update-index-ignore-missing.sh | 2 +- t/t2107-update-index-basic.sh | 2 +- t/t3040-subprojects-basic.sh | 2 +- t/t3050-subprojects-fetch.sh | 3 ++- t/t3404-rebase-interactive.sh | 3 ++- t/t3701-add-interactive.sh | 5 +++-- t/t4010-diff-pathspec.sh | 2 +- t/t4020-diff-external.sh | 2 +- t/t4035-diff-quiet.sh | 1 + t/t5531-deep-submodule-push.sh | 4 ++-- t/t6416-recursive-corner-cases.sh | 12 ++++++------ t/t6430-merge-recursive.sh | 1 + t/t6437-submodule-merge.sh | 12 ++++++------ t/t7401-submodule-summary.sh | 4 ++-- t/t7402-submodule-rebase.sh | 2 +- 17 files changed, 35 insertions(+), 28 deletions(-) diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh index c70d11bc91..ea58d415ba 100755 --- a/t/t0008-ignores.sh +++ b/t/t0008-ignores.sh @@ -191,7 +191,7 @@ test_expect_success 'setup' ' git add a && git commit -m"commit in submodule" ) && - git add a/submodule && + git submodule add ./a/submodule ./a/submodule && cat <<-\EOF >.gitignore && one ignored-* diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh index b2bdd1fcb4..dd2858648b 100755 --- a/t/t2013-checkout-submodule.sh +++ b/t/t2013-checkout-submodule.sh @@ -10,7 +10,7 @@ test_expect_success 'setup' ' (cd submodule && git init && test_commit first) && - git add submodule && + git submodule add ./submodule && test_tick && git commit -m superproject && (cd submodule && @@ -51,6 +51,7 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .gitm git config diff.ignoreSubmodules none && git config -f .gitmodules submodule.submodule.path submodule && git config -f .gitmodules submodule.submodule.ignore untracked && + git commit -m "Update patterns in .gitmodules" .gitmodules && git checkout HEAD >actual 2>&1 && test_must_be_empty actual ' @@ -59,6 +60,7 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .git/ git config -f .gitmodules submodule.submodule.ignore none && git config submodule.submodule.path submodule && git config submodule.submodule.ignore all && + git commit -m "Update patterns in .gitmodules" .gitmodules && git checkout HEAD >actual 2>&1 && test_must_be_empty actual ' diff --git a/t/t2103-update-index-ignore-missing.sh b/t/t2103-update-index-ignore-missing.sh index e9451cd567..f43a57350c 100755 --- a/t/t2103-update-index-ignore-missing.sh +++ b/t/t2103-update-index-ignore-missing.sh @@ -36,7 +36,7 @@ test_expect_success basics ' git add file && git commit -m "sub initial" ) && - git add xyzzy && + git submodule add ./xyzzy && test_tick && git commit -m initial && diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh index 07e6de84e6..465b41ccdc 100755 --- a/t/t2107-update-index-basic.sh +++ b/t/t2107-update-index-basic.sh @@ -49,7 +49,7 @@ test_expect_success '--cacheinfo does not accept blob null sha1' ' test_expect_success '--cacheinfo does not accept gitlink null sha1' ' git init submodule && (cd submodule && test_commit foo) && - git add submodule && + git submodule add ./submodule && git rev-parse :submodule >expect && test_must_fail git update-index --cacheinfo 160000 $ZERO_OID submodule && git rev-parse :submodule >actual && diff --git a/t/t3040-subprojects-basic.sh b/t/t3040-subprojects-basic.sh index bd65dfcffc..61da7e3b94 100755 --- a/t/t3040-subprojects-basic.sh +++ b/t/t3040-subprojects-basic.sh @@ -69,7 +69,7 @@ test_expect_success 'check if clone works' ' test_expect_success 'removing and adding subproject' ' git update-index --force-remove -- sub2 && mv sub2 sub3 && - git add sub3 && + git submodule add ./sub3 && git commit -q -m "renaming a subproject" && test_expect_code 1 git diff -M --name-status --exit-code HEAD^ HEAD ' diff --git a/t/t3050-subprojects-fetch.sh b/t/t3050-subprojects-fetch.sh index f1f09abdd9..9a692274b9 100755 --- a/t/t3050-subprojects-fetch.sh +++ b/t/t3050-subprojects-fetch.sh @@ -14,7 +14,8 @@ test_expect_success setup ' git commit -m "subproject commit #1" ) && >mainfile && - git add sub mainfile && + git add mainfile && + git submodule add ./sub && test_tick && git commit -m "superproject commit #1" ' diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 462cefd25d..1d0574216b 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -675,7 +675,8 @@ test_expect_success 'submodule rebase setup' ' git add elif && git commit -m "submodule initial" ) && echo 1 >file1 && - git add file1 sub && + git add file1 && + git submodule add ./sub && test_tick && git commit -m "One" && echo 2 >file1 && diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 5841f280fb..715c4fcc62 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -913,8 +913,9 @@ test_expect_success 'setup different kinds of dirty submodules' ' ) && cp -R dirty-head dirty-otherwise && cp -R dirty-head dirty-both-ways && - git add dirty-head && - git add dirty-otherwise dirty-both-ways && + git submodule add ./dirty-head && + git submodule add ./dirty-otherwise && + git submodule add ./dirty-both-ways && git commit -m initial && cd dirty-head && diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh index 9d9650eba7..844258c418 100755 --- a/t/t4010-diff-pathspec.sh +++ b/t/t4010-diff-pathspec.sh @@ -117,7 +117,7 @@ test_expect_success 'setup submodules' ' test_tick && git init submod && ( cd submod && test_commit first ) && - git add submod && + git submodule add ./submod && git commit -m first && ( cd submod && test_commit second ) && git add submod && diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh index c1ac09ecc7..ca2a23a78f 100755 --- a/t/t4020-diff-external.sh +++ b/t/t4020-diff-external.sh @@ -253,7 +253,7 @@ test_expect_success 'clean up crlf leftovers' ' test_expect_success 'submodule diff' ' git init sub && ( cd sub && test_commit sub1 ) && - git add sub && + git submodule add ./sub && test_tick && git commit -m "add submodule" && ( cd sub && test_commit sub2 ) && diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh index 76f8034c60..23ef222006 100755 --- a/t/t4035-diff-quiet.sh +++ b/t/t4035-diff-quiet.sh @@ -66,6 +66,7 @@ test_expect_success 'git diff-index --cached HEAD^' ' test_expect_success 'git diff-index --cached HEAD^' ' echo text >>b && echo 3 >c && + git submodule add ./test-outside/repo ./test-outside/repo && git add . && test_expect_code 1 git diff-index --quiet --cached HEAD^ >cnt && test_line_count = 0 cnt diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh index 302e4cbdba..f8ef70b5a2 100755 --- a/t/t5531-deep-submodule-push.sh +++ b/t/t5531-deep-submodule-push.sh @@ -28,7 +28,7 @@ test_expect_success setup ' git add junk && git commit -m "Initial junk" ) && - git add gar/bage && + git submodule add ./gar/bage ./gar/bage && git commit -m "Initial superproject" ) ' @@ -367,7 +367,7 @@ test_expect_success 'push succeeds if submodule has no remote and is on the firs git add junk && git commit -m "initial" ) && - git add b && + git submodule add ./b && git commit -m "added submodule" && git push --recurse-submodules=check origin main ) diff --git a/t/t6416-recursive-corner-cases.sh b/t/t6416-recursive-corner-cases.sh index 17b54d625d..b366dd77e5 100755 --- a/t/t6416-recursive-corner-cases.sh +++ b/t/t6416-recursive-corner-cases.sh @@ -1270,7 +1270,7 @@ test_expect_success 'setup submodule modify/modify' ' ) && git -C submod reset --hard A && - git add submod && + git submodule add ./submod && git commit -m A && git tag A && @@ -1303,7 +1303,7 @@ test_expect_merge_algorithm failure success 'check submodule modify/modify' ' test_must_fail git merge -s recursive E^0 && git ls-files -s >out && - test_line_count = 3 out && + test_line_count = 4 out && git ls-files -u >out && test_line_count = 3 out && git ls-files -o >out && @@ -1364,12 +1364,12 @@ test_expect_success 'setup submodule add/add' ' git checkout -b B A && git -C submod reset --hard B && - git add submod && + git submodule add ./submod && git commit -m B && git checkout -b C A && git -C submod reset --hard C && - git add submod && + git submodule add ./submod && git commit -m C && git checkout -q B^0 && @@ -1391,7 +1391,7 @@ test_expect_merge_algorithm failure success 'check submodule add/add' ' test_must_fail git merge -s recursive E^0 && git ls-files -s >out && - test_line_count = 3 out && + test_line_count = 4 out && git ls-files -u >out && test_line_count = 2 out && git ls-files -o >out && @@ -1439,7 +1439,7 @@ test_expect_success 'setup conflicting entry types (submodule vs symlink)' ' git checkout -b B A && git -C path reset --hard B && - git add path && + git submodule add ./path && git commit -m B && git checkout -b C A && diff --git a/t/t6430-merge-recursive.sh b/t/t6430-merge-recursive.sh index 07067bb347..8962db8faa 100755 --- a/t/t6430-merge-recursive.sh +++ b/t/t6430-merge-recursive.sh @@ -677,6 +677,7 @@ test_expect_success 'merging with triple rename across D/F conflict' ' echo content3 >sub2/file3 && mkdir simple && echo base >simple/bar && + git -c protocol.file.allow=always submodule add ./sym && git add -A && test_tick && git commit -m base && diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh index c9a86f2e94..7f6e89f541 100755 --- a/t/t6437-submodule-merge.sh +++ b/t/t6437-submodule-merge.sh @@ -30,7 +30,7 @@ test_expect_success setup ' git add file && test_tick && git commit -m sub-root) && - git add sub && + git submodule add ./sub && test_tick && git commit -m root && @@ -85,7 +85,7 @@ test_expect_success 'setup for merge search' ' git branch sub-a) && git commit --allow-empty -m init && git branch init && - git add sub && + git submodule add ./sub && git commit -m "a" && git branch a && @@ -132,7 +132,7 @@ test_expect_success 'finish setup for merge-search' ' git checkout -b g init && (cd sub && git checkout -b sub-g sub-c) && - git add sub && + git submodule add ./sub && git commit -a -m "g") ' @@ -296,7 +296,7 @@ test_expect_success 'setup for recursive merge with submodule' ' git checkout -b sub-cb sub-c && git merge sub-b && git checkout main) && - git add sub && + git submodule add ./sub && git commit -m a && git checkout -b top-b main && (cd sub && git checkout sub-b) && @@ -520,7 +520,7 @@ test_expect_success 'setup for null merge base' ' git commit --allow-empty -m init && git branch init && git checkout -b a init && - git add sub && + git submodule add ./sub && git commit -m "a" && git switch main && (cd sub && @@ -532,7 +532,7 @@ test_expect_success 'setup for null merge base' ' test_expect_success 'merging should fail with no merge base' ' (cd no-merge-base && git checkout -b b init && - git add sub && + git submodule add ./sub && git commit -m "b" && test_must_fail git merge a >actual && if test "$GIT_TEST_MERGE_ALGORITHM" = ort diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh index 542b3331a7..73fd09edb6 100755 --- a/t/t7401-submodule-summary.sh +++ b/t/t7401-submodule-summary.sh @@ -45,7 +45,7 @@ add_file . foo >/dev/null head1=$(add_file sm1 foo1 foo2) test_expect_success 'added submodule' " - git add sm1 && + git submodule add ./sm1 && git submodule summary >actual && cat >expected <<-EOF && * sm1 0000000...$head1 (2): @@ -253,7 +253,7 @@ test_expect_success 'deleted submodule' " test_expect_success 'create second submodule' ' test_create_repo sm2 && head7=$(add_file sm2 foo8 foo9) && - git add sm2 + git submodule add ./sm2 ' test_expect_success 'multiple submodules' " diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh index ebeca12a71..abc2092741 100755 --- a/t/t7402-submodule-rebase.sh +++ b/t/t7402-submodule-rebase.sh @@ -14,7 +14,7 @@ test_expect_success setup ' test_tick && git commit -m initial && git clone . submodule && - git add submodule && + git submodule add ./submodule && test_tick && git commit -m submodule && echo second line >> file && -- 2.39.1.581.gbfd45094c4-goog ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC PATCH 4/6] tests: use `git submodule add` and fix expected diffs 2023-02-13 18:21 [RFC PATCH 0/6] add: block invalid submodules Calvin Wan ` (2 preceding siblings ...) 2023-02-13 18:21 ` [RFC PATCH 3/6] tests: Use `git submodule add` instead of `git add` Calvin Wan @ 2023-02-13 18:21 ` Calvin Wan 2023-02-13 23:07 ` Junio C Hamano 2023-02-13 18:21 ` [RFC PATCH 5/6] tests: use `git submodule add` and fix expected status Calvin Wan ` (2 subsequent siblings) 6 siblings, 1 reply; 40+ messages in thread From: Calvin Wan @ 2023-02-13 18:21 UTC (permalink / raw) To: git; +Cc: Josh Steadmon, Calvin Wan From: Josh Steadmon <steadmon@google.com> This commit continues the previous work of updating the test suite to use `git submodule add` to create submodules instead of using `git add` to include embedded repositories. Specifically, in this commit we update test cases where expected diffs must change due to the presence of a .gitmodules file. Signed-off-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> --- t/t3040-subprojects-basic.sh | 3 +- t/t4041-diff-submodule-option.sh | 153 ++++++++++++++++++- t/t4060-diff-submodule-option-diff-format.sh | 125 +++++++++++++-- t/t7506-status-submodule.sh | 15 +- 4 files changed, 278 insertions(+), 18 deletions(-) diff --git a/t/t3040-subprojects-basic.sh b/t/t3040-subprojects-basic.sh index 61da7e3b94..a0f14db3d2 100755 --- a/t/t3040-subprojects-basic.sh +++ b/t/t3040-subprojects-basic.sh @@ -19,11 +19,12 @@ test_expect_success 'setup: create subprojects' ' ( cd sub2 && git init && : >Makefile && git add * && git commit -q -m "subproject 2" ) && git update-index --add sub1 && - git add sub2 && + git submodule add ./sub2 && git commit -q -m "subprojects added" && GIT_PRINT_SHA1_ELLIPSIS="yes" git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current && git branch save HEAD && cat >expected <<-\EOF && + :000000 100644 00000... A .gitmodules :000000 160000 00000... A sub1 :000000 160000 00000... A sub2 EOF diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh index 556682b18b..6c666d2b7a 100755 --- a/t/t4041-diff-submodule-option.sh +++ b/t/t4041-diff-submodule-option.sh @@ -50,9 +50,19 @@ test_expect_success 'setup' ' ' test_expect_success 'added submodule' ' - git add sm1 && + git submodule add ./sm1 && + gitmodules_hash1=$(git rev-parse --short $(git hash-object .gitmodules)) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 0000000...$head1 (new submodule) EOF test_cmp expected actual @@ -63,6 +73,15 @@ test_expect_success 'added submodule, set diff.submodule' ' git add sm1 && git diff --cached >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 0000000...$head1 (new submodule) EOF git config --unset diff.submodule && @@ -74,6 +93,15 @@ test_expect_success '--submodule=short overrides diff.submodule' ' git add sm1 && git diff --submodule=short --cached >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 diff --git a/sm1 b/sm1 new file mode 160000 index 0000000..$head1 @@ -89,6 +117,15 @@ test_expect_success 'diff.submodule does not affect plumbing' ' test_config diff.submodule log && git diff-index -p HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 diff --git a/sm1 b/sm1 new file mode 160000 index 0000000..$head1 @@ -105,6 +142,15 @@ test_expect_success 'modified submodule(forward)' ' head2=$(add_file sm1 foo3) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head1..$head2: > Add foo3 ($added foo3) EOF @@ -154,6 +200,15 @@ test_expect_success 'modified submodule(backward)' ' ) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head2..$head3 (rewind): < Add foo3 ($added foo3) < Add foo2 ($added foo2) @@ -165,6 +220,15 @@ test_expect_success 'modified submodule(backward and forward)' ' head4=$(add_file sm1 foo4 foo5) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head2...$head4: > Add foo5 ($added foo5) > Add foo4 ($added foo4) @@ -187,6 +251,15 @@ test_expect_success 'setup - change sm1 to a blob' ' test_expect_success 'typechanged submodule(submodule->blob), --cached' ' git diff --submodule=log --cached >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head4...0000000 (submodule deleted) diff --git a/sm1 b/sm1 new file mode 100644 @@ -219,6 +292,15 @@ test_expect_success 'typechanged submodule(submodule->blob)' ' git checkout-index sm1 && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head4...0000000 (submodule deleted) diff --git a/sm1 b/sm1 new file mode 100644 @@ -238,6 +320,15 @@ test_expect_success 'nonexistent commit' ' fullhead6=$(cd sm1 && git rev-parse --verify HEAD) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head4...$head6 (commits not present) EOF test_cmp expected actual @@ -436,12 +527,24 @@ test_expect_success 'deleted submodule' ' test_expect_success 'create second submodule' ' test_create_repo sm2 && head7=$(add_file sm2 foo8 foo9) && - git add sm2 + git submodule add ./sm2 && + gitmodules_hash2=$(git rev-parse --short $(git hash-object .gitmodules)) ' test_expect_success 'multiple submodules' ' git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + index $gitmodules_hash1..$gitmodules_hash2 100644 + --- a/.gitmodules + +++ b/.gitmodules + @@ -1,3 +1,6 @@ + [submodule "sm1"] + path = sm1 + url = ./sm1 + +[submodule "sm2"] + + path = sm2 + + url = ./sm2 Submodule sm1 $head6...0000000 (submodule deleted) Submodule sm2 0000000...$head7 (new submodule) EOF @@ -460,6 +563,17 @@ test_expect_success 'given commit' ' commit_file sm2 && git diff-index -p --submodule=log HEAD^ >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + index $gitmodules_hash1..$gitmodules_hash2 100644 + --- a/.gitmodules + +++ b/.gitmodules + @@ -1,3 +1,6 @@ + [submodule "sm1"] + path = sm1 + url = ./sm1 + +[submodule "sm2"] + + path = sm2 + + url = ./sm2 Submodule sm1 $head6...0000000 (submodule deleted) Submodule sm2 0000000...$head7 (new submodule) EOF @@ -469,6 +583,17 @@ test_expect_success 'given commit' ' test_expect_success 'given commit --submodule' ' git diff-index -p --submodule HEAD^ >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + index $gitmodules_hash1..$gitmodules_hash2 100644 + --- a/.gitmodules + +++ b/.gitmodules + @@ -1,3 +1,6 @@ + [submodule "sm1"] + path = sm1 + url = ./sm1 + +[submodule "sm2"] + + path = sm2 + + url = ./sm2 Submodule sm1 $head6...0000000 (submodule deleted) Submodule sm2 0000000...$head7 (new submodule) EOF @@ -479,6 +604,17 @@ test_expect_success 'given commit --submodule=short' ' fullhead7=$(cd sm2 && git rev-parse --verify HEAD) && git diff-index -p --submodule=short HEAD^ >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + index $gitmodules_hash1..$gitmodules_hash2 100644 + --- a/.gitmodules + +++ b/.gitmodules + @@ -1,3 +1,6 @@ + [submodule "sm1"] + path = sm1 + url = ./sm1 + +[submodule "sm2"] + + path = sm2 + + url = ./sm2 diff --git a/sm1 b/sm1 deleted file mode 160000 index $head6..0000000 @@ -507,6 +643,17 @@ test_expect_success 'setup .git file for sm2' ' test_expect_success 'diff --submodule with .git file' ' git diff --submodule HEAD^ >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + index $gitmodules_hash1..$gitmodules_hash2 100644 + --- a/.gitmodules + +++ b/.gitmodules + @@ -1,3 +1,6 @@ + [submodule "sm1"] + path = sm1 + url = ./sm1 + +[submodule "sm2"] + + path = sm2 + + url = ./sm2 Submodule sm1 $head6...0000000 (submodule deleted) Submodule sm2 0000000...$head7 (new submodule) EOF @@ -525,7 +672,7 @@ test_expect_success 'diff --submodule with objects referenced by alternates' ' (cd super && git clone -s ../sub_alt sub && git init && - git add sub && + git submodule add ./sub && git commit -m "sub a" ) && (cd sub_alt && diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh index 3cda8ffd14..6bee3ac628 100755 --- a/t/t4060-diff-submodule-option-diff-format.sh +++ b/t/t4060-diff-submodule-option-diff-format.sh @@ -61,9 +61,18 @@ test_expect_success 'setup repository' ' ' test_expect_success 'added submodule' ' - git add sm1 && + git submodule add ./sm1 && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 1234567..89abcde + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 0000000...$head1 (new submodule) diff --git a/sm1/foo1 b/sm1/foo1 new file mode 100644 @@ -88,6 +97,15 @@ test_expect_success 'added submodule, set diff.submodule' ' git add sm1 && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 1234567..89abcde + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 0000000...$head1 (new submodule) diff --git a/sm1/foo1 b/sm1/foo1 new file mode 100644 @@ -112,6 +130,15 @@ test_expect_success '--submodule=short overrides diff.submodule' ' git add sm1 && git diff --submodule=short --cached >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 1234567..89abcde + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 diff --git a/sm1 b/sm1 new file mode 160000 index 0000000..$head1 @@ -127,6 +154,15 @@ test_expect_success 'diff.submodule does not affect plumbing' ' test_config diff.submodule log && git diff-index -p HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 1234567..89abcde + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 diff --git a/sm1 b/sm1 new file mode 160000 index 0000000..$head1 @@ -143,6 +179,15 @@ test_expect_success 'modified submodule(forward)' ' head2=$(add_file sm1 foo3) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..2334463 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head1..$head2: diff --git a/sm1/foo3 b/sm1/foo3 new file mode 100644 @@ -203,6 +248,15 @@ test_expect_success 'modified submodule(backward)' ' ) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..2334463 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head2..$head3 (rewind): diff --git a/sm1/foo2 b/sm1/foo2 deleted file mode 100644 @@ -226,6 +280,15 @@ test_expect_success 'modified submodule(backward and forward)' ' head4=$(add_file sm1 foo4 foo5) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..2334463 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head2...$head4: diff --git a/sm1/foo2 b/sm1/foo2 deleted file mode 100644 @@ -272,6 +335,15 @@ test_expect_success 'setup - change sm1 to a blob' ' test_expect_success 'typechanged submodule(submodule->blob), --cached' ' git diff --submodule=diff --cached >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..2334463 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head4...0000000 (submodule deleted) diff --git a/sm1/foo1 b/sm1/foo1 deleted file mode 100644 @@ -346,6 +418,15 @@ test_expect_success 'typechanged submodule(submodule->blob)' ' git checkout-index sm1 && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..2334463 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head4...0000000 (submodule deleted) diff --git a/sm1 b/sm1 new file mode 100644 @@ -364,6 +445,15 @@ test_expect_success 'nonexistent commit' ' head6=$(add_file sm1 foo6 foo7) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..2334463 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head4...$head6 (commits not present) EOF diff_cmp expected actual @@ -655,12 +745,23 @@ test_expect_success 'deleted submodule' ' test_expect_success 'create second submodule' ' test_create_repo sm2 && head9=$(add_file sm2 foo8 foo9) && - git add sm2 + git submodule add ./sm2 ' test_expect_success 'multiple submodules' ' git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + index 2334463..8809a9f 100644 + --- a/.gitmodules + +++ b/.gitmodules + @@ -1,3 +1,6 @@ + [submodule "sm1"] + path = sm1 + url = ./sm1 + +[submodule "sm2"] + + path = sm2 + + url = ./sm2 Submodule sm1 $head7...0000000 (submodule deleted) Submodule sm2 0000000...$head9 (new submodule) diff --git a/sm2/foo8 b/sm2/foo8 @@ -717,11 +818,13 @@ test_expect_success 'given commit' ' git diff-index -p --submodule=diff HEAD^ >actual && cat >expected <<-EOF && diff --git a/.gitmodules b/.gitmodules - new file mode 100644 - index 1234567..89abcde - --- /dev/null + index 1234567..89abcde 100644 + --- a/.gitmodules +++ b/.gitmodules - @@ -0,0 +1,3 @@ + @@ -1,3 +1,3 @@ + -[submodule "sm1"] + - path = sm1 + - url = ./sm1 +[submodule "sm2"] +path = sm2 +url = bogus_url @@ -753,11 +856,13 @@ test_expect_success 'diff --submodule=diff with .git file' ' git diff --submodule=diff HEAD^ >actual && cat >expected <<-EOF && diff --git a/.gitmodules b/.gitmodules - new file mode 100644 - index 1234567..89abcde - --- /dev/null + index 1234567..89abcde 100644 + --- a/.gitmodules +++ b/.gitmodules - @@ -0,0 +1,3 @@ + @@ -1,3 +1,3 @@ + -[submodule "sm1"] + - path = sm1 + - url = ./sm1 +[submodule "sm2"] +path = sm2 +url = bogus_url diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh index d050091345..9dbd9ff266 100755 --- a/t/t7506-status-submodule.sh +++ b/t/t7506-status-submodule.sh @@ -31,7 +31,8 @@ sanitize_diff () { test_expect_success 'setup' ' test_create_repo_with_commit sub && echo output > .gitignore && - git add sub .gitignore && + git submodule add ./sub && + git add .gitignore && git commit -m "Add submodule sub" ' @@ -243,7 +244,7 @@ test_expect_success 'status -a clean (empty submodule dir)' ' ' cat >status_expect <<\EOF -AA .gitmodules +UU .gitmodules A sub1 EOF @@ -277,7 +278,10 @@ cat >diff_expect <<\EOF diff --cc .gitmodules --- a/.gitmodules +++ b/.gitmodules -@@@ -1,3 -1,3 +1,9 @@@ +@@@ -1,6 -1,6 +1,12 @@@ + [submodule "sub"] + path = sub + url = ./sub ++<<<<<<< HEAD +[submodule "sub2"] + path = sub2 @@ -293,7 +297,10 @@ cat >diff_submodule_expect <<\EOF diff --cc .gitmodules --- a/.gitmodules +++ b/.gitmodules -@@@ -1,3 -1,3 +1,9 @@@ +@@@ -1,6 -1,6 +1,12 @@@ + [submodule "sub"] + path = sub + url = ./sub ++<<<<<<< HEAD +[submodule "sub2"] + path = sub2 -- 2.39.1.581.gbfd45094c4-goog ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [RFC PATCH 4/6] tests: use `git submodule add` and fix expected diffs 2023-02-13 18:21 ` [RFC PATCH 4/6] tests: use `git submodule add` and fix expected diffs Calvin Wan @ 2023-02-13 23:07 ` Junio C Hamano 2023-02-13 23:19 ` Junio C Hamano 0 siblings, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2023-02-13 23:07 UTC (permalink / raw) To: Calvin Wan; +Cc: git, Josh Steadmon Calvin Wan <calvinwan@google.com> writes: > From: Josh Steadmon <steadmon@google.com> > > This commit continues the previous work of updating the test suite to > use `git submodule add` to create submodules instead of using `git add` > to include embedded repositories. Specifically, in this commit we update > test cases where expected diffs must change due to the presence of a > .gitmodules file. Good that these "same" tasks are split into two steps, ones that don't notice the additional presence of .gitmodules (i.e. the previous step) and the result that do. Strictly speaking, gitlink support in the index and trees does not have to be used to record submodules, and changing all "git add" to "git submodule add" is somewhat a sad change that closes the door to those folks who want to use the mechanism for implementing "better submodules" that do not depend on a separate .gitmodules files, for example. To leave the door open, "git add" may need to learn an overriding mechanism allow adding a gitlink to the index without anything else. And then this step could use "git add -f" (or whategver the overriding mechanism) instead of "git submodule add", without changing the expected diff output. Would such a tweak make this step better? I dunno. > t/t3040-subprojects-basic.sh | 3 +- > t/t4041-diff-submodule-option.sh | 153 ++++++++++++++++++- > t/t4060-diff-submodule-option-diff-format.sh | 125 +++++++++++++-- > t/t7506-status-submodule.sh | 15 +- > 4 files changed, 278 insertions(+), 18 deletions(-) ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH 4/6] tests: use `git submodule add` and fix expected diffs 2023-02-13 23:07 ` Junio C Hamano @ 2023-02-13 23:19 ` Junio C Hamano 0 siblings, 0 replies; 40+ messages in thread From: Junio C Hamano @ 2023-02-13 23:19 UTC (permalink / raw) To: Calvin Wan; +Cc: git, Josh Steadmon Junio C Hamano <gitster@pobox.com> writes: > Calvin Wan <calvinwan@google.com> writes: > >> From: Josh Steadmon <steadmon@google.com> >> >> This commit continues the previous work of updating the test suite to >> use `git submodule add` to create submodules instead of using `git add` >> to include embedded repositories. Specifically, in this commit we update >> test cases where expected diffs must change due to the presence of a >> .gitmodules file. > > Good that these "same" tasks are split into two steps, ones that > don't notice the additional presence of .gitmodules (i.e. the > previous step) and the result that do. > > Strictly speaking, gitlink support in the index and trees does not > have to be used to record submodules, and changing all "git add" to > "git submodule add" is somewhat a sad change that closes the door to > those folks who want to use the mechanism for implementing "better > submodules" that do not depend on a separate .gitmodules files, for > example. To leave the door open, "git add" may need to learn an > overriding mechanism allow adding a gitlink to the index without > anything else. And then this step could use "git add -f" (or > whategver the overriding mechanism) instead of "git submodule add", > without changing the expected diff output. > > Would such a tweak make this step better? I dunno. The other side of the coin is that the tests that were touched by the previous step were not testing half of what "submodule add" is doing. Not all the "let's add submodule here" test may not have to verify the resulting .gitmodules file (e.g. ones that want to see the low-level diff machinery works correctly by taking output from "git diff --cached --raw" would be primarily interested in what happens to the index and not to .gitmodules at all), but some of them should. A rough rule of thumb might be that the tests that are about "submodules" should care what ".gitmodules" has, while the tests tare are about "gitlink" should not. It might even make sense to use "add -f" (or whatever override to allow adding a gitlink only to the index without touching .gitmodules and other things), not "submodule add", for the latter class of tests. Thanks. ^ permalink raw reply [flat|nested] 40+ messages in thread
* [RFC PATCH 5/6] tests: use `git submodule add` and fix expected status 2023-02-13 18:21 [RFC PATCH 0/6] add: block invalid submodules Calvin Wan ` (3 preceding siblings ...) 2023-02-13 18:21 ` [RFC PATCH 4/6] tests: use `git submodule add` and fix expected diffs Calvin Wan @ 2023-02-13 18:21 ` Calvin Wan 2023-02-13 18:21 ` [RFC PATCH 6/6] add: reject nested repositories Calvin Wan 2023-02-28 18:52 ` [PATCH v2 0/6] add: block invalid submodules Calvin Wan 6 siblings, 0 replies; 40+ messages in thread From: Calvin Wan @ 2023-02-13 18:21 UTC (permalink / raw) To: git; +Cc: Josh Steadmon, Calvin Wan From: Josh Steadmon <steadmon@google.com> This commit continues the previous work of updating the test suite to use `git submodule add` to create submodules instead of using `git add` to include embedded repositories. In this commit, we update test cases where the expected status output must change due to the presence of a .gitmodules file. We use the pre-existing expected output as a template for cases where .gitmodules has been modified but not committed. Signed-off-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> --- t/t4027-diff-submodule.sh | 17 +++-- t/t7508-status.sh | 134 ++++++++++++++++++++++++++++++++------ 2 files changed, 128 insertions(+), 23 deletions(-) diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh index 40164ae07d..2ee9f18b38 100755 --- a/t/t4027-diff-submodule.sh +++ b/t/t4027-diff-submodule.sh @@ -18,7 +18,8 @@ test_expect_success setup ' test_tick && echo frotz >nitfol && - git add nitfol sub && + git add nitfol && + git submodule add ./sub && git commit -m superproject && ( @@ -122,25 +123,30 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)' ' test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.gitmodules]' ' + git branch pristine-gitmodules && git config diff.ignoreSubmodules dirty && git diff HEAD >actual && test_must_be_empty actual && git config --add -f .gitmodules submodule.subname.ignore none && git config --add -f .gitmodules submodule.subname.path sub && + git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual && sed -e "1,/^@@/d" actual >actual.body && expect_from_to >expect.body $subprev $subprev-dirty && test_cmp expect.body actual.body && git config -f .gitmodules submodule.subname.ignore all && git config -f .gitmodules submodule.subname.path sub && + git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual2 && test_must_be_empty actual2 && git config -f .gitmodules submodule.subname.ignore untracked && + git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual3 && sed -e "1,/^@@/d" actual3 >actual3.body && expect_from_to >expect.body $subprev $subprev-dirty && test_cmp expect.body actual3.body && git config -f .gitmodules submodule.subname.ignore dirty && + git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual4 && test_must_be_empty actual4 && git config submodule.subname.ignore none && @@ -152,7 +158,7 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) git config --remove-section submodule.subname && git config --remove-section -f .gitmodules submodule.subname && git config --unset diff.ignoreSubmodules && - rm .gitmodules + git reset --hard pristine-gitmodules ' test_expect_success 'git diff HEAD with dirty submodule (index, refs match)' ' @@ -190,12 +196,15 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)' test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.gitmodules]' ' git config --add -f .gitmodules submodule.subname.ignore all && git config --add -f .gitmodules submodule.subname.path sub && + git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual2 && test_must_be_empty actual2 && git config -f .gitmodules submodule.subname.ignore untracked && + git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual3 && test_must_be_empty actual3 && git config -f .gitmodules submodule.subname.ignore dirty && + git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual4 && test_must_be_empty actual4 && git config submodule.subname.ignore none && @@ -206,7 +215,7 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) test_cmp expect.body actual.body && git config --remove-section submodule.subname && git config --remove-section -f .gitmodules submodule.subname && - rm .gitmodules + git reset --hard pristine-gitmodules ' test_expect_success 'git diff between submodule commits' ' @@ -243,7 +252,7 @@ test_expect_success 'git diff between submodule commits [.gitmodules]' ' expect_from_to >expect.body $subtip $subprev && git config --remove-section submodule.subname && git config --remove-section -f .gitmodules submodule.subname && - rm .gitmodules + git reset --hard pristine-gitmodules ' test_expect_success 'git diff (empty submodule dir)' ' diff --git a/t/t7508-status.sh b/t/t7508-status.sh index 2b7ef6c41a..5808339997 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh @@ -892,7 +892,7 @@ test_expect_success 'setup status submodule summary' ' git add foo && git commit -m "Add foo" ) && - git add sm + git submodule add ./sm ' test_expect_success 'status submodule summary is disabled by default' ' @@ -904,6 +904,7 @@ and have 1 and 2 different commits each, respectively. Changes to be committed: (use "git restore --staged <file>..." to unstage) + new file: .gitmodules new file: dir2/added new file: sm @@ -931,6 +932,7 @@ test_expect_success 'status --untracked-files=all does not show submodule' ' ' cat >expect <<EOF +A .gitmodules M dir1/modified A dir2/added A sm @@ -961,6 +963,7 @@ and have 1 and 2 different commits each, respectively. Changes to be committed: (use "git restore --staged <file>..." to unstage) + new file: .gitmodules new file: dir2/added new file: sm @@ -998,6 +1001,7 @@ test_expect_success 'commit with submodule summary ignores status.displayComment ' cat >expect <<EOF +A .gitmodules M dir1/modified A dir2/added A sm @@ -1068,6 +1072,7 @@ and have 2 and 2 different commits each, respectively. Changes to be committed: (use "git restore --source=HEAD^1 --staged <file>..." to unstage) + new file: .gitmodules new file: dir2/added new file: sm @@ -1134,7 +1139,6 @@ Submodule changes to be committed: Untracked files: (use "git add <file>..." to include in what will be committed) - .gitmodules dir1/untracked dir2/modified dir2/untracked @@ -1152,8 +1156,37 @@ test_expect_success '.gitmodules ignore=untracked suppresses submodules with unt test_cmp expect output && git config --add -f .gitmodules submodule.subname.ignore untracked && git config --add -f .gitmodules submodule.subname.path sm && + cat > expect-modified-gitmodules << EOF && +On branch main +Your branch and '\''upstream'\'' have diverged, +and have 2 and 2 different commits each, respectively. + (use "git pull" to merge the remote branch into yours) + +Changes to be committed: + (use "git restore --staged <file>..." to unstage) + modified: sm + +Changes not staged for commit: + (use "git add <file>..." to update what will be committed) + (use "git restore <file>..." to discard changes in working directory) + modified: .gitmodules + modified: dir1/modified + +Submodule changes to be committed: + +* sm $head...$new_head (1): + > Add bar + +Untracked files: + (use "git add <file>..." to include in what will be committed) + dir1/untracked + dir2/modified + dir2/untracked + untracked + +EOF git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config -f .gitmodules --remove-section submodule.subname ' @@ -1163,7 +1196,7 @@ test_expect_success '.git/config ignore=untracked suppresses submodules with unt git config --add submodule.subname.ignore untracked && git config --add submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config --remove-section submodule.subname && git config --remove-section -f .gitmodules submodule.subname ' @@ -1180,7 +1213,7 @@ test_expect_success '.gitmodules ignore=dirty suppresses submodules with untrack git config --add -f .gitmodules submodule.subname.ignore dirty && git config --add -f .gitmodules submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config -f .gitmodules --remove-section submodule.subname ' @@ -1190,7 +1223,7 @@ test_expect_success '.git/config ignore=dirty suppresses submodules with untrack git config --add submodule.subname.ignore dirty && git config --add submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config --remove-section submodule.subname && git config -f .gitmodules --remove-section submodule.subname ' @@ -1205,7 +1238,7 @@ test_expect_success '.gitmodules ignore=dirty suppresses submodules with modifie git config --add -f .gitmodules submodule.subname.ignore dirty && git config --add -f .gitmodules submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config -f .gitmodules --remove-section submodule.subname ' @@ -1215,7 +1248,7 @@ test_expect_success '.git/config ignore=dirty suppresses submodules with modifie git config --add submodule.subname.ignore dirty && git config --add submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config --remove-section submodule.subname && git config -f .gitmodules --remove-section submodule.subname ' @@ -1245,7 +1278,6 @@ Submodule changes to be committed: Untracked files: (use "git add <file>..." to include in what will be committed) - .gitmodules dir1/untracked dir2/modified dir2/untracked @@ -1259,8 +1291,39 @@ EOF test_expect_success ".gitmodules ignore=untracked doesn't suppress submodules with modified content" ' git config --add -f .gitmodules submodule.subname.ignore untracked && git config --add -f .gitmodules submodule.subname.path sm && + cat > expect-modified-gitmodules << EOF && +On branch main +Your branch and '\''upstream'\'' have diverged, +and have 2 and 2 different commits each, respectively. + (use "git pull" to merge the remote branch into yours) + +Changes to be committed: + (use "git restore --staged <file>..." to unstage) + modified: sm + +Changes not staged for commit: + (use "git add <file>..." to update what will be committed) + (use "git restore <file>..." to discard changes in working directory) + (commit or discard the untracked or modified content in submodules) + modified: .gitmodules + modified: dir1/modified + modified: sm (modified content) + +Submodule changes to be committed: + +* sm $head...$new_head (1): + > Add bar + +Untracked files: + (use "git add <file>..." to include in what will be committed) + dir1/untracked + dir2/modified + dir2/untracked + untracked + +EOF git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config -f .gitmodules --remove-section submodule.subname ' @@ -1270,7 +1333,7 @@ test_expect_success ".git/config ignore=untracked doesn't suppress submodules wi git config --add submodule.subname.ignore untracked && git config --add submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config --remove-section submodule.subname && git config -f .gitmodules --remove-section submodule.subname ' @@ -1306,7 +1369,6 @@ Submodules changed but not updated: Untracked files: (use "git add <file>..." to include in what will be committed) - .gitmodules dir1/untracked dir2/modified dir2/untracked @@ -1318,10 +1380,45 @@ EOF ' test_expect_success ".gitmodules ignore=untracked doesn't suppress submodule summary" ' + cat > expect-modified-gitmodules << EOF && +On branch main +Your branch and '\''upstream'\'' have diverged, +and have 2 and 2 different commits each, respectively. + (use "git pull" to merge the remote branch into yours) + +Changes to be committed: + (use "git restore --staged <file>..." to unstage) + modified: sm + +Changes not staged for commit: + (use "git add <file>..." to update what will be committed) + (use "git restore <file>..." to discard changes in working directory) + modified: .gitmodules + modified: dir1/modified + modified: sm (new commits) + +Submodule changes to be committed: + +* sm $head...$new_head (1): + > Add bar + +Submodules changed but not updated: + +* sm $new_head...$head2 (1): + > 2nd commit + +Untracked files: + (use "git add <file>..." to include in what will be committed) + dir1/untracked + dir2/modified + dir2/untracked + untracked + +EOF git config --add -f .gitmodules submodule.subname.ignore untracked && git config --add -f .gitmodules submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config -f .gitmodules --remove-section submodule.subname ' @@ -1331,7 +1428,7 @@ test_expect_success ".git/config ignore=untracked doesn't suppress submodule sum git config --add submodule.subname.ignore untracked && git config --add submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config --remove-section submodule.subname && git config -f .gitmodules --remove-section submodule.subname ' @@ -1344,7 +1441,7 @@ test_expect_success ".gitmodules ignore=dirty doesn't suppress submodule summary git config --add -f .gitmodules submodule.subname.ignore dirty && git config --add -f .gitmodules submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config -f .gitmodules --remove-section submodule.subname ' @@ -1354,7 +1451,7 @@ test_expect_success ".git/config ignore=dirty doesn't suppress submodule summary git config --add submodule.subname.ignore dirty && git config --add submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config --remove-section submodule.subname && git config -f .gitmodules --remove-section submodule.subname ' @@ -1387,7 +1484,6 @@ cat > expect << EOF ; ; Untracked files: ; (use "git add <file>..." to include in what will be committed) -; .gitmodules ; dir1/untracked ; dir2/modified ; dir2/untracked @@ -1420,7 +1516,6 @@ Changes not staged for commit: Untracked files: (use "git add <file>..." to include in what will be committed) - .gitmodules dir1/untracked dir2/modified dir2/untracked @@ -1446,11 +1541,11 @@ Changes to be committed: Changes not staged for commit: (use "git add <file>..." to update what will be committed) (use "git restore <file>..." to discard changes in working directory) + modified: .gitmodules modified: dir1/modified Untracked files: (use "git add <file>..." to include in what will be committed) - .gitmodules dir1/untracked dir2/modified dir2/untracked @@ -1566,6 +1661,7 @@ Changes to be committed: Changes not staged for commit: (use "git add <file>..." to update what will be committed) (use "git restore <file>..." to discard changes in working directory) + modified: .gitmodules modified: dir1/modified Untracked files not listed (use -u option to show untracked files) -- 2.39.1.581.gbfd45094c4-goog ^ permalink raw reply related [flat|nested] 40+ messages in thread
* [RFC PATCH 6/6] add: reject nested repositories 2023-02-13 18:21 [RFC PATCH 0/6] add: block invalid submodules Calvin Wan ` (4 preceding siblings ...) 2023-02-13 18:21 ` [RFC PATCH 5/6] tests: use `git submodule add` and fix expected status Calvin Wan @ 2023-02-13 18:21 ` Calvin Wan 2023-02-13 20:42 ` Jeff King 2023-02-28 18:52 ` [PATCH v2 0/6] add: block invalid submodules Calvin Wan 6 siblings, 1 reply; 40+ messages in thread From: Calvin Wan @ 2023-02-13 18:21 UTC (permalink / raw) To: git; +Cc: Josh Steadmon, Calvin Wan From: Josh Steadmon <steadmon@google.com> As noted in 532139940c (add: warn when adding an embedded repository, 2017-06-14), adding embedded repositories results in subpar experience compared to submodules, due to the lack of a corresponding .gitmodules entry, which means later clones of the top-level repository cannot locate the embedded repo. We expect that this situation is usually unintentional, which is why 532139940c added a warning message and advice when users attempt to add an embedded repo. At $dayjob, we have found that even this advice is insufficient to stop users from committing unclonable embedded repos in shared projects. This causes toil for the owners of the top-level project repository as they must clean up the resulting gitlinks. Additionally, these mistakes are often made by partners outside of $dayjob, which means that a simple organization-wide change to the default Git config would be insufficient to prevent these mistakes. Due to this experience, we believe that Git's default behavior should be changed to disallow adding embedded repositories. This commit changes the existing warning into a fatal error while preserving the `--no-warn-embedded-repo` flag as a way to bypass this check. Signed-off-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> --- Documentation/git-add.txt | 7 ++++--- builtin/add.c | 28 ++++++++++++++++++++-------- t/t7400-submodule-basic.sh | 4 ++-- t/t7412-submodule-absorbgitdirs.sh | 2 +- t/t7414-submodule-mistakes.sh | 21 ++++++++++----------- t/t7450-bad-git-dotfiles.sh | 2 +- 6 files changed, 38 insertions(+), 26 deletions(-) diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index a030d33c6e..b7fb95b061 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -177,10 +177,11 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files. tree or not. --no-warn-embedded-repo:: - By default, `git add` will warn when adding an embedded + By default, `git add` will error out when adding an embedded repository to the index without using `git submodule add` to - create an entry in `.gitmodules`. This option will suppress the - warning (e.g., if you are manually performing operations on + create an entry in `.gitmodules`. This option will allow the + embedded repository to be added and suppress the error. + (e.g., if you are manually performing operations on submodules). --renormalize:: diff --git a/builtin/add.c b/builtin/add.c index 76277df326..795d9251b9 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -421,36 +421,45 @@ static const char embedded_advice[] = N_( "\n" " git rm --cached %s\n" "\n" -"See \"git help submodule\" for more information." +"See \"git help submodule\" for more information.\n" +"\n" +"If you cannot use submodules, you may bypass this check with:\n" +"\n" +" git add --no-warn-embedded-repo %s\n" ); -static void check_embedded_repo(const char *path) +static int check_embedded_repo(const char *path) { + int ret = 0; struct strbuf name = STRBUF_INIT; static int adviced_on_embedded_repo = 0; if (!warn_on_embedded_repo) - return; + goto cleanup; if (!ends_with(path, "/")) - return; + goto cleanup; + + ret = 1; /* Drop trailing slash for aesthetics */ strbuf_addstr(&name, path); strbuf_strip_suffix(&name, "/"); - warning(_("adding embedded git repository: %s"), name.buf); + error(_("cannot add embedded git repository: %s"), name.buf); if (!adviced_on_embedded_repo && advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) { - advise(embedded_advice, name.buf, name.buf); + advise(embedded_advice, name.buf, name.buf, name.buf); adviced_on_embedded_repo = 1; } +cleanup: strbuf_release(&name); + return ret; } static int add_files(struct dir_struct *dir, int flags) { - int i, exit_status = 0; + int i, exit_status = 0, embedded_repo = 0; struct string_list matched_sparse_paths = STRING_LIST_INIT_NODUP; if (dir->ignored_nr) { @@ -476,10 +485,13 @@ static int add_files(struct dir_struct *dir, int flags) die(_("adding files failed")); exit_status = 1; } else { - check_embedded_repo(dir->entries[i]->name); + embedded_repo |= check_embedded_repo(dir->entries[i]->name); } } + if (embedded_repo) + die(_("refusing to add embedded git repositories")); + if (matched_sparse_paths.nr) { advise_on_updating_sparse_paths(&matched_sparse_paths); exit_status = 1; diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index eae6a46ef3..e0bcecba6e 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -118,7 +118,7 @@ test_expect_success 'setup - repository in init subdirectory' ' test_expect_success 'setup - commit with gitlink' ' echo a >a && echo z >z && - git add a init z && + git add --no-warn-embedded-repo a init z && git commit -m "super commit 1" ' @@ -771,7 +771,7 @@ test_expect_success 'set up for relative path tests' ' git init && test_commit foo ) && - git add sub && + git add --no-warn-embedded-repo sub && git config -f .gitmodules submodule.sub.path sub && git config -f .gitmodules submodule.sub.url ../subrepo && cp .git/config pristine-.git-config && diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh index 2859695c6d..365953b558 100755 --- a/t/t7412-submodule-absorbgitdirs.sh +++ b/t/t7412-submodule-absorbgitdirs.sh @@ -100,7 +100,7 @@ test_expect_success 'absorb the git dir in a nested submodule' ' test_expect_success 'setup a gitlink with missing .gitmodules entry' ' git init sub2 && test_commit -C sub2 first && - git add sub2 && + git add --no-warn-embedded-repo sub2 && git commit -m superproject ' diff --git a/t/t7414-submodule-mistakes.sh b/t/t7414-submodule-mistakes.sh index 101afff30f..dbdcbe909d 100755 --- a/t/t7414-submodule-mistakes.sh +++ b/t/t7414-submodule-mistakes.sh @@ -10,31 +10,30 @@ test_expect_success 'create embedded repository' ' test_commit -C embed one ' -test_expect_success 'git-add on embedded repository warns' ' - test_when_finished "git rm --cached -f embed" && - git add embed 2>stderr && - test_i18ngrep warning stderr +test_expect_success 'git-add on embedded repository dies' ' + test_must_fail git add embed 2>stderr && + test_i18ngrep fatal stderr ' -test_expect_success '--no-warn-embedded-repo suppresses warning' ' +test_expect_success '--no-warn-embedded-repo suppresses error message' ' test_when_finished "git rm --cached -f embed" && git add --no-warn-embedded-repo embed 2>stderr && - test_i18ngrep ! warning stderr + test_i18ngrep ! fatal stderr ' -test_expect_success 'no warning when updating entry' ' +test_expect_success 'no error message when updating entry' ' test_when_finished "git rm --cached -f embed" && - git add embed && + git add --no-warn-embedded-repo embed && git -C embed commit --allow-empty -m two && git add embed 2>stderr && - test_i18ngrep ! warning stderr + test_i18ngrep ! fatal stderr ' -test_expect_success 'submodule add does not warn' ' +test_expect_success 'submodule add does not issue error message' ' test_when_finished "git rm -rf submodule .gitmodules" && git -c protocol.file.allow=always \ submodule add ./embed submodule 2>stderr && - test_i18ngrep ! warning stderr + test_i18ngrep ! fatal stderr ' test_done diff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh index ba1f569bcb..6fbcf36ae9 100755 --- a/t/t7450-bad-git-dotfiles.sh +++ b/t/t7450-bad-git-dotfiles.sh @@ -307,7 +307,7 @@ test_expect_success 'git dirs of sibling submodules must not be nested' ' EOF git clone . thing1 && git clone . thing2 && - git add .gitmodules thing1 thing2 && + git add --no-warn-embedded-repo .gitmodules thing1 thing2 && test_tick && git commit -m nested ) && -- 2.39.1.581.gbfd45094c4-goog ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [RFC PATCH 6/6] add: reject nested repositories 2023-02-13 18:21 ` [RFC PATCH 6/6] add: reject nested repositories Calvin Wan @ 2023-02-13 20:42 ` Jeff King 2023-02-14 2:17 ` Junio C Hamano 0 siblings, 1 reply; 40+ messages in thread From: Jeff King @ 2023-02-13 20:42 UTC (permalink / raw) To: Calvin Wan; +Cc: git, Josh Steadmon On Mon, Feb 13, 2023 at 06:21:34PM +0000, Calvin Wan wrote: > As noted in 532139940c (add: warn when adding an embedded repository, > 2017-06-14), adding embedded repositories results in subpar experience > compared to submodules, due to the lack of a corresponding .gitmodules > entry, which means later clones of the top-level repository cannot > locate the embedded repo. We expect that this situation is usually > unintentional, which is why 532139940c added a warning message and > advice when users attempt to add an embedded repo. As the author of 532139940c, this escalation to an error seems like a reasonable step to me. The patch looks pretty reasonable to me from a cursory read, but here a few small comments: > diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt > index a030d33c6e..b7fb95b061 100644 > --- a/Documentation/git-add.txt > +++ b/Documentation/git-add.txt > @@ -177,10 +177,11 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files. > tree or not. > > --no-warn-embedded-repo:: > - By default, `git add` will warn when adding an embedded > + By default, `git add` will error out when adding an embedded The option name here is rather unfortunate, since it's no longer a warning. But keeping it as-is for historical compatibility may be the best option. In retrospect, I wish I'd called it --allow-embedded-repos or something. > diff --git a/builtin/add.c b/builtin/add.c > index 76277df326..795d9251b9 100644 > --- a/builtin/add.c > +++ b/builtin/add.c > @@ -421,36 +421,45 @@ static const char embedded_advice[] = N_( > "\n" > " git rm --cached %s\n" > "\n" > -"See \"git help submodule\" for more information." > +"See \"git help submodule\" for more information.\n" > +"\n" > +"If you cannot use submodules, you may bypass this check with:\n" > +"\n" > +" git add --no-warn-embedded-repo %s\n" > ); I was a little surprised by this hunk, but I guess if we are going to block the user's operation from completing, we might want to tell them how to get around it. But it seems odd to me that the instructions to "git rm --cached" the submodule remain. If this situation is now an error and not a warning, there is nothing to roll back from the index, since we will have bailed before writing it. If we are going to start recommending --no-warn-embedded-repo here, would we want to promote it from being OPT_HIDDEN_BOOL()? We do document it in the manpage, but just omit it from the "-h" output, since it should be rarely used. Maybe it is OK to stay that way; you don't need it until you run into this situation, at which point the advice hopefully has guided you in the right direction. > -static void check_embedded_repo(const char *path) > +static int check_embedded_repo(const char *path) > { > + int ret = 0; > struct strbuf name = STRBUF_INIT; > static int adviced_on_embedded_repo = 0; > > if (!warn_on_embedded_repo) > - return; > + goto cleanup; > if (!ends_with(path, "/")) > - return; > + goto cleanup; > + > + ret = 1; I wondered about these "goto cleanup" calls here, since there is nothing to clean up yet. But you are just piggy-backing on the "return ret", rather than a separate "return 0" here. And I was surprised by returning at all, since the point is to make this a hard error. But it looks like the intent is to report an error for every such case, and then a final die, like: $ git add . error: cannot add embedded git repository: foo advice: ... error: cannot add embedded git repository: bar fatal: refusing to add embedded git repositories OK. I doubt anybody cares much either way (it is more convenient if you have a lot of cases, at the expense of making the error more verbose when there is only one case), so this is fine. > /* Drop trailing slash for aesthetics */ > strbuf_addstr(&name, path); > strbuf_strip_suffix(&name, "/"); > > - warning(_("adding embedded git repository: %s"), name.buf); > + error(_("cannot add embedded git repository: %s"), name.buf); > if (!adviced_on_embedded_repo && > advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) { > - advise(embedded_advice, name.buf, name.buf); > + advise(embedded_advice, name.buf, name.buf, name.buf); This triple name.buf that must match the earlier string is horrible, of course, but nothing new in your patch. If you drop the "rm --cached" part from the advice, it can remain as a horrible double-mention of name.buf. :) > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > index eae6a46ef3..e0bcecba6e 100755 > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh > @@ -118,7 +118,7 @@ test_expect_success 'setup - repository in init subdirectory' ' > test_expect_success 'setup - commit with gitlink' ' > echo a >a && > echo z >z && > - git add a init z && > + git add --no-warn-embedded-repo a init z && > git commit -m "super commit 1" > ' > > @@ -771,7 +771,7 @@ test_expect_success 'set up for relative path tests' ' > git init && > test_commit foo > ) && > - git add sub && > + git add --no-warn-embedded-repo sub && > git config -f .gitmodules submodule.sub.path sub && > git config -f .gitmodules submodule.sub.url ../subrepo && > cp .git/config pristine-.git-config && OK, these are cases that are warning now (because they are trying to do something clever with the setup), but which will be blocked. Arguably they should have --no-warn-embedded-repo already, but nobody cared so far that their stderr was a little noisy. I do wonder if there are any users who might do clever things like this themselves, but they'd already have been nagged by the warning (and hopefully discovered --no-warn-embedded-repo on their own). -Peff ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH 6/6] add: reject nested repositories 2023-02-13 20:42 ` Jeff King @ 2023-02-14 2:17 ` Junio C Hamano 2023-02-14 16:07 ` Jeff King 0 siblings, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2023-02-14 2:17 UTC (permalink / raw) To: Jeff King; +Cc: Calvin Wan, git, Josh Steadmon Jeff King <peff@peff.net> writes: >> " git rm --cached %s\n" >> "\n" >> -"See \"git help submodule\" for more information." >> +"See \"git help submodule\" for more information.\n" >> +"\n" >> +"If you cannot use submodules, you may bypass this check with:\n" >> +"\n" >> +" git add --no-warn-embedded-repo %s\n" >> ); > > I was a little surprised by this hunk, but I guess if we are going to > block the user's operation from completing, we might want to tell them > how to get around it. But it seems odd to me that the instructions to > "git rm --cached" the submodule remain. If this situation is now an > error and not a warning, there is nothing to roll back from the index, > since we will have bailed before writing it. > > If we are going to start recommending --no-warn-embedded-repo here, > would we want to promote it from being OPT_HIDDEN_BOOL()? We do document > it in the manpage, but just omit it from the "-h" output, since it > should be rarely used. Maybe it is OK to stay that way; you don't need > it until you run into this situation, at which point the advice > hopefully has guided you in the right direction. If we are keeping the escape hatch, it would make sense to actually use that escape hatch to protect existing "git add" with that, instead of turning them into "git submodule add" and then adjust the tests for the consequences (i.e. "submodule add" does more than what "git add [--no-warn-embedded-repo]" would), at least for these tests in [3,4,5/6]. Also I do not think it is too late for a more natural UI, e.g. "--allow-embedded-repo=[yes/no/warn]", to deprecate the "--[no-]warn-*" option. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH 6/6] add: reject nested repositories 2023-02-14 2:17 ` Junio C Hamano @ 2023-02-14 16:07 ` Jeff King 2023-02-14 16:32 ` Junio C Hamano 0 siblings, 1 reply; 40+ messages in thread From: Jeff King @ 2023-02-14 16:07 UTC (permalink / raw) To: Junio C Hamano; +Cc: Calvin Wan, git, Josh Steadmon On Mon, Feb 13, 2023 at 06:17:53PM -0800, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > >> " git rm --cached %s\n" > >> "\n" > >> -"See \"git help submodule\" for more information." > >> +"See \"git help submodule\" for more information.\n" > >> +"\n" > >> +"If you cannot use submodules, you may bypass this check with:\n" > >> +"\n" > >> +" git add --no-warn-embedded-repo %s\n" > >> ); > > > > I was a little surprised by this hunk, but I guess if we are going to > > block the user's operation from completing, we might want to tell them > > how to get around it. But it seems odd to me that the instructions to > > "git rm --cached" the submodule remain. If this situation is now an > > error and not a warning, there is nothing to roll back from the index, > > since we will have bailed before writing it. > > > > If we are going to start recommending --no-warn-embedded-repo here, > > would we want to promote it from being OPT_HIDDEN_BOOL()? We do document > > it in the manpage, but just omit it from the "-h" output, since it > > should be rarely used. Maybe it is OK to stay that way; you don't need > > it until you run into this situation, at which point the advice > > hopefully has guided you in the right direction. > > If we are keeping the escape hatch, it would make sense to actually > use that escape hatch to protect existing "git add" with that, > instead of turning them into "git submodule add" and then adjust the > tests for the consequences (i.e. "submodule add" does more than what > "git add [--no-warn-embedded-repo]" would), at least for these tests > in [3,4,5/6]. Good point. I did not really look at the test modifications, but anywhere that is triggering the current warning is arguably a good spot to be using --no-warn-embedded-repo already. It is simply that the test did not bother to look at their noisy stderr. And such a modification is obviously correct, as there are no further implications for the test. > Also I do not think it is too late for a more natural UI, e.g. > "--allow-embedded-repo=[yes/no/warn]", to deprecate the > "--[no-]warn-*" option. True. We have to keep the existing form for backwards compatibility, but we can certainly add a new one. I kind of doubt that --allow-embedded-repo=warn is useful, though. If a caller knows what it is doing is OK, then it would say "yes". And otherwise, you'd want "no". There is no situation where a caller is unsure. -Peff ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH 6/6] add: reject nested repositories 2023-02-14 16:07 ` Jeff King @ 2023-02-14 16:32 ` Junio C Hamano 2023-02-14 21:45 ` Calvin Wan 0 siblings, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2023-02-14 16:32 UTC (permalink / raw) To: Jeff King; +Cc: Calvin Wan, git, Josh Steadmon Jeff King <peff@peff.net> writes: >> If we are keeping the escape hatch, it would make sense to actually >> use that escape hatch to protect existing "git add" with that, >> instead of turning them into "git submodule add" and then adjust the >> tests for the consequences (i.e. "submodule add" does more than what >> "git add [--no-warn-embedded-repo]" would), at least for these tests >> in [3,4,5/6]. > > Good point. I did not really look at the test modifications, but > anywhere that is triggering the current warning is arguably a good spot > to be using --no-warn-embedded-repo already. It is simply that the test > did not bother to look at their noisy stderr. And such a modification is > obviously correct, as there are no further implications for the test. I did not mean that no "git add" that create a gitlink in existing tests should be made into "git submodule add". The ones that clearly wanted to set up tests to see what happens in a top-level with a subproject may become more realistic tests by switching to "git submodule add" and updating the expected "git diff HEAD" output to include a newly created .gitmodules file. But some of the tests are merely to see what happens with an index with a gitlink in it, and "add --no-warn" would be more appropriate for them. >> Also I do not think it is too late for a more natural UI, e.g. >> "--allow-embedded-repo=[yes/no/warn]", to deprecate the >> "--[no-]warn-*" option. > > True. We have to keep the existing form for backwards compatibility, but > we can certainly add a new one. > > I kind of doubt that --allow-embedded-repo=warn is useful, though. If a > caller knows what it is doing is OK, then it would say "yes". And > otherwise, you'd want "no". There is no situation where a caller is > unsure. Yeah, if the default becomes "no", then there isn't much point, other than just for completeness, to have "warn" as a choice. Thanks. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [RFC PATCH 6/6] add: reject nested repositories 2023-02-14 16:32 ` Junio C Hamano @ 2023-02-14 21:45 ` Calvin Wan 0 siblings, 0 replies; 40+ messages in thread From: Calvin Wan @ 2023-02-14 21:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, git, Josh Steadmon On Tue, Feb 14, 2023 at 8:32 AM Junio C Hamano <gitster@pobox.com> wrote: > > Jeff King <peff@peff.net> writes: > > >> If we are keeping the escape hatch, it would make sense to actually > >> use that escape hatch to protect existing "git add" with that, > >> instead of turning them into "git submodule add" and then adjust the > >> tests for the consequences (i.e. "submodule add" does more than what > >> "git add [--no-warn-embedded-repo]" would), at least for these tests > >> in [3,4,5/6]. > > > > Good point. I did not really look at the test modifications, but > > anywhere that is triggering the current warning is arguably a good spot > > to be using --no-warn-embedded-repo already. It is simply that the test > > did not bother to look at their noisy stderr. And such a modification is > > obviously correct, as there are no further implications for the test. > > I did not mean that no "git add" that create a gitlink in existing > tests should be made into "git submodule add". The ones that > clearly wanted to set up tests to see what happens in a top-level > with a subproject may become more realistic tests by switching to > "git submodule add" and updating the expected "git diff HEAD" output > to include a newly created .gitmodules file. But some of the tests > are merely to see what happens with an index with a gitlink in it, > and "add --no-warn" would be more appropriate for them. I'll take another pass into the modified tests from previous patches and pick out ones that are not specifically submodule related tests. > >> Also I do not think it is too late for a more natural UI, e.g. > >> "--allow-embedded-repo=[yes/no/warn]", to deprecate the > >> "--[no-]warn-*" option. > > > > True. We have to keep the existing form for backwards compatibility, but > > we can certainly add a new one. > > > > I kind of doubt that --allow-embedded-repo=warn is useful, though. If a > > caller knows what it is doing is OK, then it would say "yes". And > > otherwise, you'd want "no". There is no situation where a caller is > > unsure. > > Yeah, if the default becomes "no", then there isn't much point, > other than just for completeness, to have "warn" as a choice. I don't see a point for "warn" as well. The default "no" case should carry over part of the deprecated warning from before. ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 0/6] add: block invalid submodules 2023-02-13 18:21 [RFC PATCH 0/6] add: block invalid submodules Calvin Wan ` (5 preceding siblings ...) 2023-02-13 18:21 ` [RFC PATCH 6/6] add: reject nested repositories Calvin Wan @ 2023-02-28 18:52 ` Calvin Wan 2023-02-28 18:56 ` [PATCH v2 1/6] t4041, t4060: modernize test style Calvin Wan ` (5 more replies) 6 siblings, 6 replies; 40+ messages in thread From: Calvin Wan @ 2023-02-28 18:52 UTC (permalink / raw) To: git; +Cc: Calvin Wan, steadmon, peff, gitster A couple of significant changes were made since the RFC. From the feedback gathered it seems like the idea of this series is worth pursuing -- thanks Peff and Junio for the reviews! In the initial RFC, we continued using `--no-warn-embedded-repo` to allow the user to add an embedded repo, but the naming of that option was not ideal. This reroll adds `--allow-embedded-repo` and deprecates `--no-warn-embedded-repo`. While I initially claimed that there were incorrectly setup .gitmodules tests with duplicate paths, it turns out that was caused by patch 4/6 of this series. I added a new patch (see 5/6) that fixes the issue. Subsequently, this means that the memory leak does not occur any more (maybe I should still include that patch -- it's an edge case that comes from a manually misconfigured .gitmodules). Lastly I cleaned up the advice message and some of the tests -- replacing `git add` with `git submodule add` only in submodule specific cases and some style cleanups. Calvin Wan (1): tests: remove duplicate .gitmodules path Josh Steadmon (5): t4041, t4060: modernize test style tests: Use `git submodule add` instead of `git add` tests: use `git submodule add` and fix expected diffs tests: use `git submodule add` and fix expected status add: reject nested repositories Documentation/git-add.txt | 15 +- builtin/add.c | 37 ++- builtin/submodule--helper.c | 4 +- t/t0008-ignores.sh | 2 +- t/t2013-checkout-submodule.sh | 4 +- t/t2103-update-index-ignore-missing.sh | 2 +- t/t2107-update-index-basic.sh | 2 +- t/t3040-subprojects-basic.sh | 5 +- t/t3050-subprojects-fetch.sh | 3 +- t/t3404-rebase-interactive.sh | 3 +- t/t3701-add-interactive.sh | 5 +- t/t4010-diff-pathspec.sh | 2 +- t/t4020-diff-external.sh | 2 +- t/t4027-diff-submodule.sh | 58 ++--- t/t4035-diff-quiet.sh | 2 +- t/t4041-diff-submodule-option.sh | 232 +++++++++++++---- t/t4060-diff-submodule-option-diff-format.sh | 226 ++++++++++++----- t/t5531-deep-submodule-push.sh | 4 +- t/t6416-recursive-corner-cases.sh | 12 +- t/t6430-merge-recursive.sh | 2 +- t/t6437-submodule-merge.sh | 12 +- t/t7400-submodule-basic.sh | 4 +- t/t7401-submodule-summary.sh | 4 +- t/t7402-submodule-rebase.sh | 2 +- t/t7412-submodule-absorbgitdirs.sh | 2 +- t/t7414-submodule-mistakes.sh | 29 ++- t/t7450-bad-git-dotfiles.sh | 2 +- t/t7506-status-submodule.sh | 15 +- t/t7508-status.sh | 248 ++++++++++++------- 29 files changed, 649 insertions(+), 291 deletions(-) Range-diff against v1: 1: 34eacaabbd < -: ---------- leak fix: cache_put_path 2: 2da941fc59 ! 1: 3efc25bf0e t4041, t4060: modernize test style @@ Commit message easier to diagnose, because errors will caught immediately rather than in later unrelated test_expect blocks. + While at it, have tests clean up after themselves with + test_when_finished and fix other minor style issues. + Signed-off-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> @@ t/t4041-diff-submodule-option.sh: commit_file () { -fullhead1=$(cd sm1; git rev-parse --verify HEAD) +test_expect_success 'setup' ' + test_create_repo sm1 && -+ add_file . foo >/dev/null && ++ add_file . foo && + head1=$(add_file sm1 foo1 foo2) && -+ fullhead1=$(cd sm1 && git rev-parse --verify HEAD) ++ fullhead1=$(git -C sm1 rev-parse --verify HEAD) +' test_expect_success 'added submodule' ' @@ t/t4041-diff-submodule-option.sh: test_expect_success 'modified submodule(forwar -fullhead2=$(cd sm1; git rev-parse --verify HEAD) test_expect_success 'modified submodule(forward) --submodule=short' ' -+ fullhead2=$(cd sm1 && git rev-parse --verify HEAD) && ++ fullhead2=$(git -C sm1 rev-parse --verify HEAD) && git diff --submodule=short >actual && cat >expected <<-EOF && diff --git a/sm1 b/sm1 @@ t/t4041-diff-submodule-option.sh: test_expect_success 'modified submodule(backwa test_expect_success 'typechanged submodule(submodule->blob), --cached' ' git diff --submodule=log --cached >actual && +@@ t/t4041-diff-submodule-option.sh: test_expect_success 'typechanged submodule(submodule->blob), --cached' ' + ' + + test_expect_success 'typechanged submodule(submodule->blob)' ' ++ test_when_finished rm -rf sm1 && + git diff --submodule=log >actual && + cat >expected <<-EOF && + diff --git a/sm1 b/sm1 @@ t/t4041-diff-submodule-option.sh: test_expect_success 'typechanged submodule(submodule->blob)' ' test_cmp expected actual ' @@ t/t4041-diff-submodule-option.sh: test_expect_success 'typechanged submodule(sub -rm -rf sm1 && -git checkout-index sm1 test_expect_success 'typechanged submodule(submodule->blob)' ' -+ rm -rf sm1 && ++ test_when_finished rm -f sm1 && + git checkout-index sm1 && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && @@ t/t4041-diff-submodule-option.sh: test_expect_success 'typechanged submodule(sub -head6=$(add_file sm1 foo6 foo7) -fullhead6=$(cd sm1; git rev-parse --verify HEAD) test_expect_success 'nonexistent commit' ' -+ rm -f sm1 && + test_create_repo sm1 && + head6=$(add_file sm1 foo6 foo7) && -+ fullhead6=$(cd sm1 && git rev-parse --verify HEAD) && ++ fullhead6=$(git -C sm1 rev-parse --verify HEAD) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head4...$head6 (commits not present) @@ t/t4041-diff-submodule-option.sh: test_expect_success 'typechanged submodule(blo git diff-index -p --submodule=log HEAD >actual && test_must_be_empty actual ' +@@ t/t4041-diff-submodule-option.sh: test_expect_success 'submodule contains untracked and modified content (dirty ig + ' + + test_expect_success 'submodule contains untracked and modified content (all ignored)' ' ++ test_when_finished rm -f sm1/new-file && + echo new > sm1/foo6 && + git diff-index -p --ignore-submodules --submodule=log HEAD >actual && + test_must_be_empty actual + ' + + test_expect_success 'submodule contains modified content' ' +- rm -f sm1/new-file && + git diff-index -p --submodule=log HEAD >actual && + cat >expected <<-EOF && + Submodule sm1 contains modified content @@ t/t4041-diff-submodule-option.sh: test_expect_success 'submodule contains modified content' ' test_cmp expected actual ' @@ t/t4041-diff-submodule-option.sh: test_expect_success 'submodule contains modifi git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head6..$head8: +@@ t/t4041-diff-submodule-option.sh: test_expect_success 'modified submodule contains untracked and modified content + ' + + test_expect_success 'modified submodule contains untracked and modified content (all ignored)' ' ++ test_when_finished rm -f sm1/new-file && + echo modification >> sm1/foo6 && + git diff-index -p --ignore-submodules --submodule=log HEAD >actual && + test_must_be_empty actual + ' + + test_expect_success 'modified submodule contains modified content' ' +- rm -f sm1/new-file && ++ test_when_finished rm -rf sm1 && + git diff-index -p --submodule=log HEAD >actual && + cat >expected <<-EOF && + Submodule sm1 contains modified content @@ t/t4041-diff-submodule-option.sh: test_expect_success 'modified submodule contains modified content' ' test_cmp expected actual ' -rm -rf sm1 test_expect_success 'deleted submodule' ' -+ rm -rf sm1 && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && - Submodule sm1 $head6...0000000 (submodule deleted) @@ t/t4041-diff-submodule-option.sh: test_expect_success 'path filter' ' test_cmp expected actual ' @@ t/t4041-diff-submodule-option.sh: test_expect_success 'given commit --submodule' -fullhead7=$(cd sm2; git rev-parse --verify HEAD) - test_expect_success 'given commit --submodule=short' ' -+ fullhead7=$(cd sm2 && git rev-parse --verify HEAD) && ++ fullhead7=$(git -C sm2 rev-parse --verify HEAD) && git diff-index -p --submodule=short HEAD^ >actual && cat >expected <<-EOF && diff --git a/sm1 b/sm1 @@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'modified subm -fullhead2=$(cd sm1; git rev-parse --verify HEAD) test_expect_success 'modified submodule(forward) --submodule=short' ' -+ fullhead2=$(cd sm1 && git rev-parse --verify HEAD) && ++ fullhead2=$(git -C sm1 rev-parse --verify HEAD) && git diff --submodule=short >actual && cat >expected <<-EOF && diff --git a/sm1 b/sm1 @@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'modified subm test_expect_success 'modified submodule(backward)' ' + commit_file sm1 && + head3=$( -+ cd sm1 && -+ git reset --hard HEAD~2 >/dev/null && -+ git rev-parse --short --verify HEAD ++ git -C sm1 reset --hard HEAD~2 >/dev/null && ++ git -C sm1 rev-parse --short --verify HEAD + ) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && @@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'modified subm test_expect_success 'typechanged submodule(submodule->blob), --cached' ' git diff --submodule=diff --cached >actual && +@@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'typechanged submodule(submodule->blob), --cached' ' + ' + + test_expect_success 'typechanged submodule(submodule->blob)' ' ++ test_when_finished rm -rf sm1 && + git diff --submodule=diff >actual && + cat >expected <<-EOF && + diff --git a/sm1 b/sm1 @@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'typechanged submodule(submodule->blob)' ' diff_cmp expected actual ' @@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'typechanged s -rm -rf sm1 && -git checkout-index sm1 test_expect_success 'typechanged submodule(submodule->blob)' ' -+ rm -rf sm1 && ++ test_when_finished rm -f sm1 && + git checkout-index sm1 && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && @@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'typechanged s -test_create_repo sm1 && -head6=$(add_file sm1 foo6 foo7) test_expect_success 'nonexistent commit' ' -+ rm -f sm1 && + test_create_repo sm1 && + head6=$(add_file sm1 foo6 foo7) && git diff-index -p --submodule=diff HEAD >actual && @@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'typechanged s head7=$(git -C sm1 rev-parse --short --verify HEAD) && git diff-index -p --submodule=diff HEAD >actual && test_must_be_empty actual +@@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'submodule contains untracked and modified content (dirty ig + ' + + test_expect_success 'submodule contains untracked and modified content (all ignored)' ' ++ test_when_finished rm -f sm1/new-file && + echo new > sm1/foo6 && + git diff-index -p --ignore-submodules --submodule=diff HEAD >actual && + test_must_be_empty actual + ' + + test_expect_success 'submodule contains modified content' ' +- rm -f sm1/new-file && + git diff-index -p --submodule=diff HEAD >actual && + cat >expected <<-EOF && + Submodule sm1 contains modified content @@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'submodule contains modified content' ' diff_cmp expected actual ' @@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'submodule con -(cd sm1; git commit -mchange foo6 >/dev/null) && -head8=$(cd sm1; git rev-parse --short --verify HEAD) && test_expect_success 'submodule is modified' ' -+ (cd sm1 && git commit -mchange foo6 >/dev/null) && -+ head8=$(cd sm1 && git rev-parse --short --verify HEAD) && ++ (git -C sm1 commit -mchange foo6 >/dev/null) && ++ head8=$(git -C sm1 rev-parse --short --verify HEAD) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head7..$head8: +@@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'modified submodule contains untracked and modified content + ' + + test_expect_success 'modified submodule contains untracked and modified content (all ignored)' ' ++ test_when_finished rm -f sm1/new-file && + echo modification >> sm1/foo6 && + git diff-index -p --ignore-submodules --submodule=diff HEAD >actual && + test_must_be_empty actual +@@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'modified submodule contains untracked and modified content + + # NOT OK + test_expect_success 'modified submodule contains modified content' ' +- rm -f sm1/new-file && ++ test_when_finished rm -rf sm1 && + git diff-index -p --submodule=diff HEAD >actual && + cat >expected <<-EOF && + Submodule sm1 contains modified content @@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'modified submodule contains modified content' ' diff_cmp expected actual ' -rm -rf sm1 test_expect_success 'deleted submodule' ' -+ rm -rf sm1 && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && - Submodule sm1 $head7...0000000 (submodule deleted) @@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'path filter' ' diff_cmp expected actual ' @@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'deleted submo -echo submodule-to-blob>sm2 - test_expect_success 'typechanged(submodule->blob) submodule with .git file' ' ++ test_when_finished "rm sm2 && mv sm2-bak sm2" && + echo submodule-to-blob>sm2 && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && @@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'typechanged(s -mv sm2-bak sm2 - test_expect_success 'setup nested submodule' ' -+ rm sm2 && -+ mv sm2-bak sm2 && git -c protocol.file.allow=always -C sm2 submodule add ../sm2 nested && git -C sm2 commit -a -m "nested sub" && - head10=$(git -C sm2 rev-parse --short --verify HEAD) @@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'diff --submodule=diff recurses into nested submodules' ' diff_cmp expected actual ' @@ t/t4060-diff-submodule-option-diff-format.sh: test_expect_success 'diff --submod test_expect_success 'diff --submodule=diff recurses into deleted nested submodules' ' + (cd sm2 && commit_file nested) && + commit_file sm2 && -+ head12=$(cd sm2 && git rev-parse --short --verify HEAD) && ++ head12=$(git -C sm2 rev-parse --short --verify HEAD) && + mv sm2 sm2-bak && cat >expected <<-EOF && Submodule sm1 $head7...0000000 (submodule deleted) 3: e1c7c2f63e ! 2: a438f4072d tests: Use `git submodule add` instead of `git add` @@ Commit message Signed-off-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> - ## t/t0008-ignores.sh ## -@@ t/t0008-ignores.sh: test_expect_success 'setup' ' - git add a && - git commit -m"commit in submodule" - ) && -- git add a/submodule && -+ git submodule add ./a/submodule ./a/submodule && - cat <<-\EOF >.gitignore && - one - ignored-* - ## t/t2013-checkout-submodule.sh ## @@ t/t2013-checkout-submodule.sh: test_expect_success 'setup' ' (cd submodule && @@ t/t2103-update-index-ignore-missing.sh: test_expect_success basics ' git commit -m "sub initial" ) && - git add xyzzy && -+ git submodule add ./xyzzy && ++ git add ./xyzzy && test_tick && git commit -m initial && @@ t/t4020-diff-external.sh: test_expect_success 'clean up crlf leftovers' ' git commit -m "add submodule" && ( cd sub && test_commit sub2 ) && - ## t/t4035-diff-quiet.sh ## -@@ t/t4035-diff-quiet.sh: test_expect_success 'git diff-index --cached HEAD^' ' - test_expect_success 'git diff-index --cached HEAD^' ' - echo text >>b && - echo 3 >c && -+ git submodule add ./test-outside/repo ./test-outside/repo && - git add . && - test_expect_code 1 git diff-index --quiet --cached HEAD^ >cnt && - test_line_count = 0 cnt - ## t/t5531-deep-submodule-push.sh ## @@ t/t5531-deep-submodule-push.sh: test_expect_success setup ' git add junk && @@ t/t6416-recursive-corner-cases.sh: test_expect_success 'setup conflicting entry git checkout -b C A && - ## t/t6430-merge-recursive.sh ## -@@ t/t6430-merge-recursive.sh: test_expect_success 'merging with triple rename across D/F conflict' ' - echo content3 >sub2/file3 && - mkdir simple && - echo base >simple/bar && -+ git -c protocol.file.allow=always submodule add ./sym && - git add -A && - test_tick && - git commit -m base && - ## t/t6437-submodule-merge.sh ## @@ t/t6437-submodule-merge.sh: test_expect_success setup ' git add file && 4: a7630f5a4e ! 3: 336e095b38 tests: use `git submodule add` and fix expected diffs @@ t/t4041-diff-submodule-option.sh: test_expect_success 'typechanged submodule(sub diff --git a/sm1 b/sm1 new file mode 100644 @@ t/t4041-diff-submodule-option.sh: test_expect_success 'nonexistent commit' ' - fullhead6=$(cd sm1 && git rev-parse --verify HEAD) && + fullhead6=$(git -C sm1 rev-parse --verify HEAD) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules @@ t/t4041-diff-submodule-option.sh: test_expect_success 'given commit' ' Submodule sm2 0000000...$head7 (new submodule) EOF @@ t/t4041-diff-submodule-option.sh: test_expect_success 'given commit --submodule=short' ' - fullhead7=$(cd sm2 && git rev-parse --verify HEAD) && + fullhead7=$(git -C sm2 rev-parse --verify HEAD) && git diff-index -p --submodule=short HEAD^ >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules 5: 7182e1fb2f = 4: 40a4647ae3 tests: use `git submodule add` and fix expected status -: ---------- > 5: caa2f228a0 tests: remove duplicate .gitmodules path 6: 6d2ef61fb4 ! 6: 28cb291edd add: reject nested repositories @@ Commit message Due to this experience, we believe that Git's default behavior should be changed to disallow adding embedded repositories. This commit changes - the existing warning into a fatal error while preserving the - `--no-warn-embedded-repo` flag as a way to bypass this check. + the existing warning into a fatal error, rewrites the advice given, and + deprecates `--no-warn-embedded-repo` in favor of `--allow-embedded-repo` + to bypass the fatal error. Signed-off-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> ## Documentation/git-add.txt ## @@ Documentation/git-add.txt: for "git add --no-all <pathspec>...", i.e. ignored removed files. + be ignored, no matter if they are already present in the work tree or not. - --no-warn-embedded-repo:: +---no-warn-embedded-repo:: - By default, `git add` will warn when adding an embedded ++--allow-embedded-repo:: + By default, `git add` will error out when adding an embedded repository to the index without using `git submodule add` to - create an entry in `.gitmodules`. This option will suppress the - warning (e.g., if you are manually performing operations on +- submodules). + create an entry in `.gitmodules`. This option will allow the -+ embedded repository to be added and suppress the error. -+ (e.g., if you are manually performing operations on - submodules). ++ embedded repository to be added. (e.g., if you are manually ++ performing operations on submodules). ++ ++--no-warn-embedded-repo:: ++ This option is deprecated in favor of '--add-embedded-repo'. ++ Passing this option still suppresses advice but does not bypass ++ the error. --renormalize:: + Apply the "clean" process freshly to all tracked files to ## builtin/add.c ## -@@ builtin/add.c: static const char embedded_advice[] = N_( +@@ builtin/add.c: N_("The following paths are ignored by one of your .gitignore files:\n"); + static int verbose, show_only, ignored_too, refresh_only; + static int ignore_add_errors, intent_to_add, ignore_missing; + static int warn_on_embedded_repo = 1; ++static int allow_embedded_repo = 0; + + #define ADDREMOVE_DEFAULT 1 + static int addremove = ADDREMOVE_DEFAULT; +@@ builtin/add.c: static struct option builtin_add_options[] = { + N_("override the executable bit of the listed files")), + OPT_HIDDEN_BOOL(0, "warn-embedded-repo", &warn_on_embedded_repo, + N_("warn when adding an embedded repository")), ++ OPT_HIDDEN_BOOL(0, "allow-embedded-repo", &allow_embedded_repo, ++ N_("allow adding an embedded repository")), + OPT_PATHSPEC_FROM_FILE(&pathspec_from_file), + OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul), + OPT_END(), +@@ builtin/add.c: static int add_config(const char *var, const char *value, void *cb) + } + + static const char embedded_advice[] = N_( +-"You've added another git repository inside your current repository.\n" ++"You attempted to add another git repository inside your current repository.\n" + "Clones of the outer repository will not contain the contents of\n" + "the embedded repository and will not know how to obtain it.\n" + "If you meant to add a submodule, use:\n" "\n" - " git rm --cached %s\n" + " git submodule add <url> %s\n" "\n" --"See \"git help submodule\" for more information." +-"If you added this path by mistake, you can remove it from the\n" +-"index with:\n" +"See \"git help submodule\" for more information.\n" -+"\n" + "\n" +-" git rm --cached %s\n" +"If you cannot use submodules, you may bypass this check with:\n" -+"\n" -+" git add --no-warn-embedded-repo %s\n" + "\n" +-"See \"git help submodule\" for more information." ++" git add --allow-embedded-repo %s\n" ); -static void check_embedded_repo(const char *path) @@ builtin/add.c: static const char embedded_advice[] = N_( struct strbuf name = STRBUF_INIT; static int adviced_on_embedded_repo = 0; - if (!warn_on_embedded_repo) +- if (!warn_on_embedded_repo) - return; ++ if (allow_embedded_repo) + goto cleanup; if (!ends_with(path, "/")) - return; @@ builtin/add.c: static const char embedded_advice[] = N_( - warning(_("adding embedded git repository: %s"), name.buf); + error(_("cannot add embedded git repository: %s"), name.buf); if (!adviced_on_embedded_repo && - advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) { -- advise(embedded_advice, name.buf, name.buf); -+ advise(embedded_advice, name.buf, name.buf, name.buf); +- advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) { ++ warn_on_embedded_repo && ++ advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) { + advise(embedded_advice, name.buf, name.buf); adviced_on_embedded_repo = 1; } @@ builtin/add.c: static int add_files(struct dir_struct *dir, int flags) advise_on_updating_sparse_paths(&matched_sparse_paths); exit_status = 1; + ## builtin/submodule--helper.c ## +@@ builtin/submodule--helper.c: static void configure_added_submodule(struct add_data *add_data) + + add_submod.git_cmd = 1; + strvec_pushl(&add_submod.args, "add", +- "--no-warn-embedded-repo", NULL); ++ "--allow-embedded-repo", NULL); + if (add_data->force) + strvec_push(&add_submod.args, "--force"); + strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL); +@@ builtin/submodule--helper.c: static int module_add(int argc, const char **argv, const char *prefix) + cp.git_cmd = 1; + cp.no_stdout = 1; + strvec_pushl(&cp.args, "add", "--dry-run", "--ignore-missing", +- "--no-warn-embedded-repo", add_data.sm_path, NULL); ++ "--allow-embedded-repo", add_data.sm_path, NULL); + if ((ret = pipe_command(&cp, NULL, 0, NULL, 0, &sb, 0))) { + strbuf_complete_line(&sb); + fputs(sb.buf, stderr); + + ## t/t0008-ignores.sh ## +@@ t/t0008-ignores.sh: test_expect_success 'setup' ' + git add a && + git commit -m"commit in submodule" + ) && +- git add a/submodule && ++ git add --allow-embedded-repo a/submodule && + cat <<-\EOF >.gitignore && + one + ignored-* + + ## t/t2103-update-index-ignore-missing.sh ## +@@ t/t2103-update-index-ignore-missing.sh: test_expect_success basics ' + git add file && + git commit -m "sub initial" + ) && +- git add ./xyzzy && ++ git add --allow-embedded-repo ./xyzzy && + + test_tick && + git commit -m initial && + + ## t/t4035-diff-quiet.sh ## +@@ t/t4035-diff-quiet.sh: test_expect_success 'git diff-index --cached HEAD^' ' + test_expect_success 'git diff-index --cached HEAD^' ' + echo text >>b && + echo 3 >c && +- git add . && ++ git add --allow-embedded-repo . && + test_expect_code 1 git diff-index --quiet --cached HEAD^ >cnt && + test_line_count = 0 cnt + ' + + ## t/t6430-merge-recursive.sh ## +@@ t/t6430-merge-recursive.sh: test_expect_success 'merging with triple rename across D/F conflict' ' + echo content3 >sub2/file3 && + mkdir simple && + echo base >simple/bar && +- git add -A && ++ git add -A --allow-embedded-repo && + test_tick && + git commit -m base && + + ## t/t7400-submodule-basic.sh ## @@ t/t7400-submodule-basic.sh: test_expect_success 'setup - repository in init subdirectory' ' test_expect_success 'setup - commit with gitlink' ' echo a >a && echo z >z && - git add a init z && -+ git add --no-warn-embedded-repo a init z && ++ git add --allow-embedded-repo a init z && git commit -m "super commit 1" ' @@ t/t7400-submodule-basic.sh: test_expect_success 'set up for relative path tests' test_commit foo ) && - git add sub && -+ git add --no-warn-embedded-repo sub && ++ git add --allow-embedded-repo sub && git config -f .gitmodules submodule.sub.path sub && git config -f .gitmodules submodule.sub.url ../subrepo && cp .git/config pristine-.git-config && @@ t/t7412-submodule-absorbgitdirs.sh: test_expect_success 'absorb the git dir in a git init sub2 && test_commit -C sub2 first && - git add sub2 && -+ git add --no-warn-embedded-repo sub2 && ++ git add --allow-embedded-repo sub2 && git commit -m superproject ' @@ t/t7414-submodule-mistakes.sh: test_expect_success 'create embedded repository' ' -test_expect_success '--no-warn-embedded-repo suppresses warning' ' -+test_expect_success '--no-warn-embedded-repo suppresses error message' ' ++test_expect_success '--allow-embedded-repo adds embedded repository and suppresses error message' ' test_when_finished "git rm --cached -f embed" && - git add --no-warn-embedded-repo embed 2>stderr && +- git add --no-warn-embedded-repo embed 2>stderr && - test_i18ngrep ! warning stderr ++ git add --allow-embedded-repo embed 2>stderr && + test_i18ngrep ! fatal stderr ++' ++ ++test_expect_success '--no-warn-embedded-repo dies and suppresses advice' ' ++ test_must_fail git add --no-warn-embedded-repo embed 2>stderr && ++ test_i18ngrep ! hint stderr && ++ test_i18ngrep fatal stderr ' -test_expect_success 'no warning when updating entry' ' +test_expect_success 'no error message when updating entry' ' test_when_finished "git rm --cached -f embed" && - git add embed && -+ git add --no-warn-embedded-repo embed && ++ git add --allow-embedded-repo embed && git -C embed commit --allow-empty -m two && git add embed 2>stderr && - test_i18ngrep ! warning stderr @@ t/t7414-submodule-mistakes.sh: test_expect_success 'create embedded repository' ' -test_expect_success 'submodule add does not warn' ' -+test_expect_success 'submodule add does not issue error message' ' ++test_expect_success 'submodule add neither fails nor issues error message' ' test_when_finished "git rm -rf submodule .gitmodules" && git -c protocol.file.allow=always \ submodule add ./embed submodule 2>stderr && @@ t/t7450-bad-git-dotfiles.sh: test_expect_success 'git dirs of sibling submodules git clone . thing1 && git clone . thing2 && - git add .gitmodules thing1 thing2 && -+ git add --no-warn-embedded-repo .gitmodules thing1 thing2 && ++ git add --allow-embedded-repo .gitmodules thing1 thing2 && test_tick && git commit -m nested ) && -- 2.39.2.722.g9855ee24e9-goog ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 1/6] t4041, t4060: modernize test style 2023-02-28 18:52 ` [PATCH v2 0/6] add: block invalid submodules Calvin Wan @ 2023-02-28 18:56 ` Calvin Wan 2023-03-06 19:32 ` Glen Choo 2023-02-28 18:56 ` [PATCH v2 2/6] tests: Use `git submodule add` instead of `git add` Calvin Wan ` (4 subsequent siblings) 5 siblings, 1 reply; 40+ messages in thread From: Calvin Wan @ 2023-02-28 18:56 UTC (permalink / raw) To: git; +Cc: Josh Steadmon, peff, gitster, Calvin Wan From: Josh Steadmon <steadmon@google.com> In preparation for later changes, move setup code into test_expect blocks. Smaller sections are moved into existing blocks, while larger sections with a standalone purpose are given their own new blocks. This makes sure that later changes that may break the test setup are easier to diagnose, because errors will caught immediately rather than in later unrelated test_expect blocks. While at it, have tests clean up after themselves with test_when_finished and fix other minor style issues. Signed-off-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> --- t/t4041-diff-submodule-option.sh | 79 ++++++++------- t/t4060-diff-submodule-option-diff-format.sh | 101 +++++++++---------- 2 files changed, 89 insertions(+), 91 deletions(-) diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh index 0c1502d4b0..2aa12243bd 100755 --- a/t/t4041-diff-submodule-option.sh +++ b/t/t4041-diff-submodule-option.sh @@ -42,11 +42,12 @@ commit_file () { git commit "$@" -m "Commit $*" >/dev/null } -test_create_repo sm1 && -add_file . foo >/dev/null - -head1=$(add_file sm1 foo1 foo2) -fullhead1=$(cd sm1; git rev-parse --verify HEAD) +test_expect_success 'setup' ' + test_create_repo sm1 && + add_file . foo && + head1=$(add_file sm1 foo1 foo2) && + fullhead1=$(git -C sm1 rev-parse --verify HEAD) +' test_expect_success 'added submodule' ' git add sm1 && @@ -99,10 +100,9 @@ test_expect_success 'diff.submodule does not affect plumbing' ' test_cmp expected actual ' -commit_file sm1 && -head2=$(add_file sm1 foo3) - test_expect_success 'modified submodule(forward)' ' + commit_file sm1 && + head2=$(add_file sm1 foo3) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head1..$head2: @@ -129,8 +129,8 @@ test_expect_success 'modified submodule(forward) --submodule' ' test_cmp expected actual ' -fullhead2=$(cd sm1; git rev-parse --verify HEAD) test_expect_success 'modified submodule(forward) --submodule=short' ' + fullhead2=$(git -C sm1 rev-parse --verify HEAD) && git diff --submodule=short >actual && cat >expected <<-EOF && diff --git a/sm1 b/sm1 @@ -144,14 +144,14 @@ test_expect_success 'modified submodule(forward) --submodule=short' ' test_cmp expected actual ' -commit_file sm1 && -head3=$( - cd sm1 && - git reset --hard HEAD~2 >/dev/null && - git rev-parse --short --verify HEAD -) test_expect_success 'modified submodule(backward)' ' + commit_file sm1 && + head3=$( + cd sm1 && + git reset --hard HEAD~2 >/dev/null && + git rev-parse --short --verify HEAD + ) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head2..$head3 (rewind): @@ -161,8 +161,8 @@ test_expect_success 'modified submodule(backward)' ' test_cmp expected actual ' -head4=$(add_file sm1 foo4 foo5) test_expect_success 'modified submodule(backward and forward)' ' + head4=$(add_file sm1 foo4 foo5) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head2...$head4: @@ -174,13 +174,15 @@ test_expect_success 'modified submodule(backward and forward)' ' test_cmp expected actual ' -commit_file sm1 && -mv sm1 sm1-bak && -echo sm1 >sm1 && -head5=$(git hash-object sm1 | cut -c1-7) && -git add sm1 && -rm -f sm1 && -mv sm1-bak sm1 +test_expect_success 'setup - change sm1 to a blob' ' + commit_file sm1 && + mv sm1 sm1-bak && + echo sm1 >sm1 && + head5=$(git hash-object sm1 | cut -c1-7) && + git add sm1 && + rm -f sm1 && + mv sm1-bak sm1 +' test_expect_success 'typechanged submodule(submodule->blob), --cached' ' git diff --submodule=log --cached >actual && @@ -198,6 +200,7 @@ test_expect_success 'typechanged submodule(submodule->blob), --cached' ' ' test_expect_success 'typechanged submodule(submodule->blob)' ' + test_when_finished rm -rf sm1 && git diff --submodule=log >actual && cat >expected <<-EOF && diff --git a/sm1 b/sm1 @@ -212,9 +215,9 @@ test_expect_success 'typechanged submodule(submodule->blob)' ' test_cmp expected actual ' -rm -rf sm1 && -git checkout-index sm1 test_expect_success 'typechanged submodule(submodule->blob)' ' + test_when_finished rm -f sm1 && + git checkout-index sm1 && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head4...0000000 (submodule deleted) @@ -229,11 +232,10 @@ test_expect_success 'typechanged submodule(submodule->blob)' ' test_cmp expected actual ' -rm -f sm1 && -test_create_repo sm1 && -head6=$(add_file sm1 foo6 foo7) -fullhead6=$(cd sm1; git rev-parse --verify HEAD) test_expect_success 'nonexistent commit' ' + test_create_repo sm1 && + head6=$(add_file sm1 foo6 foo7) && + fullhead6=$(git -C sm1 rev-parse --verify HEAD) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head4...$head6 (commits not present) @@ -241,8 +243,8 @@ test_expect_success 'nonexistent commit' ' test_cmp expected actual ' -commit_file test_expect_success 'typechanged submodule(blob->submodule)' ' + commit_file && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && diff --git a/sm1 b/sm1 @@ -257,8 +259,8 @@ test_expect_success 'typechanged submodule(blob->submodule)' ' test_cmp expected actual ' -commit_file sm1 && test_expect_success 'submodule is up to date' ' + commit_file sm1 && git diff-index -p --submodule=log HEAD >actual && test_must_be_empty actual ' @@ -313,13 +315,13 @@ test_expect_success 'submodule contains untracked and modified content (dirty ig ' test_expect_success 'submodule contains untracked and modified content (all ignored)' ' + test_when_finished rm -f sm1/new-file && echo new > sm1/foo6 && git diff-index -p --ignore-submodules --submodule=log HEAD >actual && test_must_be_empty actual ' test_expect_success 'submodule contains modified content' ' - rm -f sm1/new-file && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 contains modified content @@ -327,9 +329,9 @@ test_expect_success 'submodule contains modified content' ' test_cmp expected actual ' -(cd sm1; git commit -mchange foo6 >/dev/null) && -head8=$(cd sm1; git rev-parse --short --verify HEAD) && test_expect_success 'submodule is modified' ' + (cd sm1 && git commit -mchange foo6 >/dev/null) && + head8=$(cd sm1 && git rev-parse --short --verify HEAD) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head6..$head8: @@ -406,13 +408,14 @@ test_expect_success 'modified submodule contains untracked and modified content ' test_expect_success 'modified submodule contains untracked and modified content (all ignored)' ' + test_when_finished rm -f sm1/new-file && echo modification >> sm1/foo6 && git diff-index -p --ignore-submodules --submodule=log HEAD >actual && test_must_be_empty actual ' test_expect_success 'modified submodule contains modified content' ' - rm -f sm1/new-file && + test_when_finished rm -rf sm1 && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && Submodule sm1 contains modified content @@ -422,7 +425,6 @@ test_expect_success 'modified submodule contains modified content' ' test_cmp expected actual ' -rm -rf sm1 test_expect_success 'deleted submodule' ' git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && @@ -454,8 +456,8 @@ test_expect_success 'path filter' ' test_cmp expected actual ' -commit_file sm2 test_expect_success 'given commit' ' + commit_file sm2 && git diff-index -p --submodule=log HEAD^ >actual && cat >expected <<-EOF && Submodule sm1 $head6...0000000 (submodule deleted) @@ -473,9 +475,8 @@ test_expect_success 'given commit --submodule' ' test_cmp expected actual ' -fullhead7=$(cd sm2; git rev-parse --verify HEAD) - test_expect_success 'given commit --submodule=short' ' + fullhead7=$(git -C sm2 rev-parse --verify HEAD) && git diff-index -p --submodule=short HEAD^ >actual && cat >expected <<-EOF && diff --git a/sm1 b/sm1 diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh index 97c6424cd5..a760ed5eb6 100755 --- a/t/t4060-diff-submodule-option-diff-format.sh +++ b/t/t4060-diff-submodule-option-diff-format.sh @@ -138,10 +138,9 @@ test_expect_success 'diff.submodule does not affect plumbing' ' diff_cmp expected actual ' -commit_file sm1 && -head2=$(add_file sm1 foo3) - test_expect_success 'modified submodule(forward)' ' + commit_file sm1 && + head2=$(add_file sm1 foo3) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head1..$head2: @@ -180,8 +179,8 @@ test_expect_success 'modified submodule(forward) --submodule' ' diff_cmp expected actual ' -fullhead2=$(cd sm1; git rev-parse --verify HEAD) test_expect_success 'modified submodule(forward) --submodule=short' ' + fullhead2=$(git -C sm1 rev-parse --verify HEAD) && git diff --submodule=short >actual && cat >expected <<-EOF && diff --git a/sm1 b/sm1 @@ -195,14 +194,12 @@ test_expect_success 'modified submodule(forward) --submodule=short' ' diff_cmp expected actual ' -commit_file sm1 && -head3=$( - cd sm1 && - git reset --hard HEAD~2 >/dev/null && - git rev-parse --short --verify HEAD -) - test_expect_success 'modified submodule(backward)' ' + commit_file sm1 && + head3=$( + git -C sm1 reset --hard HEAD~2 >/dev/null && + git -C sm1 rev-parse --short --verify HEAD + ) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head2..$head3 (rewind): @@ -224,8 +221,8 @@ test_expect_success 'modified submodule(backward)' ' diff_cmp expected actual ' -head4=$(add_file sm1 foo4 foo5) test_expect_success 'modified submodule(backward and forward)' ' + head4=$(add_file sm1 foo4 foo5) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head2...$head4: @@ -261,13 +258,15 @@ test_expect_success 'modified submodule(backward and forward)' ' diff_cmp expected actual ' -commit_file sm1 && -mv sm1 sm1-bak && -echo sm1 >sm1 && -head5=$(git hash-object sm1 | cut -c1-7) && -git add sm1 && -rm -f sm1 && -mv sm1-bak sm1 +test_expect_success 'setup - change sm1 to a blob' ' + commit_file sm1 && + mv sm1 sm1-bak && + echo sm1 >sm1 && + head5=$(git hash-object sm1 | cut -c1-7) && + git add sm1 && + rm -f sm1 && + mv sm1-bak sm1 +' test_expect_success 'typechanged submodule(submodule->blob), --cached' ' git diff --submodule=diff --cached >actual && @@ -306,6 +305,7 @@ test_expect_success 'typechanged submodule(submodule->blob), --cached' ' ' test_expect_success 'typechanged submodule(submodule->blob)' ' + test_when_finished rm -rf sm1 && git diff --submodule=diff >actual && cat >expected <<-EOF && diff --git a/sm1 b/sm1 @@ -341,9 +341,9 @@ test_expect_success 'typechanged submodule(submodule->blob)' ' diff_cmp expected actual ' -rm -rf sm1 && -git checkout-index sm1 test_expect_success 'typechanged submodule(submodule->blob)' ' + test_when_finished rm -f sm1 && + git checkout-index sm1 && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head4...0000000 (submodule deleted) @@ -358,10 +358,9 @@ test_expect_success 'typechanged submodule(submodule->blob)' ' diff_cmp expected actual ' -rm -f sm1 && -test_create_repo sm1 && -head6=$(add_file sm1 foo6 foo7) test_expect_success 'nonexistent commit' ' + test_create_repo sm1 && + head6=$(add_file sm1 foo6 foo7) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head4...$head6 (commits not present) @@ -369,8 +368,8 @@ test_expect_success 'nonexistent commit' ' diff_cmp expected actual ' -commit_file test_expect_success 'typechanged submodule(blob->submodule)' ' + commit_file && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && diff --git a/sm1 b/sm1 @@ -399,8 +398,8 @@ test_expect_success 'typechanged submodule(blob->submodule)' ' diff_cmp expected actual ' -commit_file sm1 && test_expect_success 'submodule is up to date' ' + commit_file sm1 && head7=$(git -C sm1 rev-parse --short --verify HEAD) && git diff-index -p --submodule=diff HEAD >actual && test_must_be_empty actual @@ -471,13 +470,13 @@ test_expect_success 'submodule contains untracked and modified content (dirty ig ' test_expect_success 'submodule contains untracked and modified content (all ignored)' ' + test_when_finished rm -f sm1/new-file && echo new > sm1/foo6 && git diff-index -p --ignore-submodules --submodule=diff HEAD >actual && test_must_be_empty actual ' test_expect_success 'submodule contains modified content' ' - rm -f sm1/new-file && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 contains modified content @@ -492,9 +491,9 @@ test_expect_success 'submodule contains modified content' ' diff_cmp expected actual ' -(cd sm1; git commit -mchange foo6 >/dev/null) && -head8=$(cd sm1; git rev-parse --short --verify HEAD) && test_expect_success 'submodule is modified' ' + (git -C sm1 commit -mchange foo6 >/dev/null) && + head8=$(git -C sm1 rev-parse --short --verify HEAD) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head7..$head8: @@ -616,6 +615,7 @@ test_expect_success 'modified submodule contains untracked and modified content ' test_expect_success 'modified submodule contains untracked and modified content (all ignored)' ' + test_when_finished rm -f sm1/new-file && echo modification >> sm1/foo6 && git diff-index -p --ignore-submodules --submodule=diff HEAD >actual && test_must_be_empty actual @@ -623,7 +623,7 @@ test_expect_success 'modified submodule contains untracked and modified content # NOT OK test_expect_success 'modified submodule contains modified content' ' - rm -f sm1/new-file && + test_when_finished rm -rf sm1 && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 contains modified content @@ -643,7 +643,6 @@ test_expect_success 'modified submodule contains modified content' ' diff_cmp expected actual ' -rm -rf sm1 test_expect_success 'deleted submodule' ' git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && @@ -703,13 +702,15 @@ test_expect_success 'path filter' ' diff_cmp expected actual ' -cat >.gitmodules <<-EOF -[submodule "sm2"] - path = sm2 - url = bogus_url -EOF -git add .gitmodules -commit_file sm2 .gitmodules +test_expect_success 'setup - construct .gitmodules' ' + cat >.gitmodules <<-EOF && + [submodule "sm2"] + path = sm2 + url = bogus_url + EOF + git add .gitmodules && + commit_file sm2 .gitmodules +' test_expect_success 'given commit' ' git diff-index -p --submodule=diff HEAD^ >actual && @@ -779,9 +780,8 @@ test_expect_success 'diff --submodule=diff with .git file' ' diff_cmp expected actual ' -mv sm2 sm2-bak - test_expect_success 'deleted submodule with .git file' ' + mv sm2 sm2-bak && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head7...0000000 (submodule deleted) @@ -804,9 +804,9 @@ test_expect_success 'deleted submodule with .git file' ' diff_cmp expected actual ' -echo submodule-to-blob>sm2 - test_expect_success 'typechanged(submodule->blob) submodule with .git file' ' + test_when_finished "rm sm2 && mv sm2-bak sm2" && + echo submodule-to-blob>sm2 && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && Submodule sm1 $head7...0000000 (submodule deleted) @@ -836,9 +836,6 @@ test_expect_success 'typechanged(submodule->blob) submodule with .git file' ' diff_cmp expected actual ' -rm sm2 -mv sm2-bak sm2 - test_expect_success 'setup nested submodule' ' git -c protocol.file.allow=always -C sm2 submodule add ../sm2 nested && git -C sm2 commit -a -m "nested sub" && @@ -910,13 +907,11 @@ test_expect_success 'diff --submodule=diff recurses into nested submodules' ' diff_cmp expected actual ' -(cd sm2; commit_file nested) -commit_file sm2 -head12=$(cd sm2; git rev-parse --short --verify HEAD) - -mv sm2 sm2-bak - test_expect_success 'diff --submodule=diff recurses into deleted nested submodules' ' + (cd sm2 && commit_file nested) && + commit_file sm2 && + head12=$(git -C sm2 rev-parse --short --verify HEAD) && + mv sm2 sm2-bak && cat >expected <<-EOF && Submodule sm1 $head7...0000000 (submodule deleted) Submodule sm2 $head12...0000000 (submodule deleted) @@ -971,6 +966,8 @@ test_expect_success 'diff --submodule=diff recurses into deleted nested submodul diff_cmp expected actual ' -mv sm2-bak sm2 +test_expect_success 'submodule cleanup' ' + mv sm2-bak sm2 +' test_done -- 2.39.2.722.g9855ee24e9-goog ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/6] t4041, t4060: modernize test style 2023-02-28 18:56 ` [PATCH v2 1/6] t4041, t4060: modernize test style Calvin Wan @ 2023-03-06 19:32 ` Glen Choo 2023-03-06 20:40 ` Calvin Wan 0 siblings, 1 reply; 40+ messages in thread From: Glen Choo @ 2023-03-06 19:32 UTC (permalink / raw) To: Calvin Wan, git; +Cc: Josh Steadmon, peff, gitster, Calvin Wan Calvin Wan <calvinwan@google.com> writes: > In preparation for later changes, move setup code into test_expect > blocks. Smaller sections are moved into existing blocks, while larger > sections with a standalone purpose are given their own new blocks. The changes where we moved lines outside of blocks into blocks without changing them look good to me. > While at it, have tests clean up after themselves with > test_when_finished I believe this came about as part of the discussion in https://lore.kernel.org/git/xmqqedqtbbf4.fsf@gitster.g I think it's good to have tests clean up after themselves, but I'm not sure if that's what we're doing in all of these cases, see below. I'm leaving the diff header in place, since the two files have very confusingly similar tests. > diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh > test_expect_success 'typechanged submodule(submodule->blob)' ' > + test_when_finished rm -rf sm1 && > git diff --submodule=log >actual && > cat >expected <<-EOF && > diff --git a/sm1 b/sm1 This hunk and the next... > @@ -212,9 +215,9 @@ test_expect_success 'typechanged submodule(submodule->blob)' ' > test_cmp expected actual > ' > > -rm -rf sm1 && > -git checkout-index sm1 > test_expect_success 'typechanged submodule(submodule->blob)' ' > + test_when_finished rm -f sm1 && > + git checkout-index sm1 && > git diff-index -p --submodule=log HEAD >actual && > cat >expected <<-EOF && > Submodule sm1 $head4...0000000 (submodule deleted) were changed so that the "rm -rf" happens in the clean up phase of the earlier test (test 14) instead of set up phase of the later test (test 15). But, the "rm -rf" actually results in a _different_ state from before 14, so it isn't actually cleaning up, it really is preparation for 15's git checkout-index. You can observe this by running ./t4041-diff-submodule-option.sh --run=1-13,15 which fails as expected. On the other hand, it passes if we move the "rm -rf" into test 15. Nearly all of the other test_when_finished here have the same problem, where they 'clean up' state that wasn't changed in the same test body. I believe they will show similar dependency issues, though I didn't go through and test them all. > @@ -643,7 +643,6 @@ test_expect_success 'modified submodule contains modified content' ' > diff_cmp expected actual > ' > > -rm -rf sm1 > test_expect_success 'deleted submodule' ' > git diff-index -p --submodule=diff HEAD >actual && > cat >expected <<-EOF && This one is fairly obvious, since the test says 'deleted submodule', but we no longer delete the submodule in the setup. > @@ -779,9 +780,8 @@ test_expect_success 'diff --submodule=diff with .git file' ' > diff_cmp expected actual > ' > > -mv sm2 sm2-bak > - > test_expect_success 'deleted submodule with .git file' ' > + mv sm2 sm2-bak && > git diff-index -p --submodule=diff HEAD >actual && > cat >expected <<-EOF && > Submodule sm1 $head7...0000000 (submodule deleted) > @@ -804,9 +804,9 @@ test_expect_success 'deleted submodule with .git file' ' > diff_cmp expected actual > ' > > -echo submodule-to-blob>sm2 > - > test_expect_success 'typechanged(submodule->blob) submodule with .git file' ' > + test_when_finished "rm sm2 && mv sm2-bak sm2" && > + echo submodule-to-blob>sm2 && > git diff-index -p --submodule=diff HEAD >actual && > cat >expected <<-EOF && > Submodule sm1 $head7...0000000 (submodule deleted) > @@ -836,9 +836,6 @@ test_expect_success 'typechanged(submodule->blob) submodule with .git file' ' > diff_cmp expected actual > ' > > -rm sm2 > -mv sm2-bak sm2 This is the original case that Junio flagged, which I think is an almost correct use of test_when_finished, since we do get back to an earlier state before this string of tests, but not to the state before the actual test with the test_when_finished. If we want to use test_when_finished here (which I think we do), we should add another test_when_finished to remove the dependency between the two tests. like so: test_expect_success 'deleted submodule with .git file' ' + test_when_finished "mv sm2-bak sm2" && mv sm2 sm2-bak && git diff-index -p --submodule=diff HEAD >actual && ... test_expect_success 'typechanged(submodule->blob) submodule with .git file' ' test_when_finished "rm sm2 && mv sm2-bak sm2" && + mv sm2 sm2-bak && Currently, they're still dependent because one creates sm2-bak and the other moves it back, but if we have each test restore sm2, there will be no more dependency. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 1/6] t4041, t4060: modernize test style 2023-03-06 19:32 ` Glen Choo @ 2023-03-06 20:40 ` Calvin Wan 0 siblings, 0 replies; 40+ messages in thread From: Calvin Wan @ 2023-03-06 20:40 UTC (permalink / raw) To: Glen Choo; +Cc: git, Josh Steadmon, peff, gitster On Mon, Mar 6, 2023 at 11:32 AM Glen Choo <chooglen@google.com> wrote: > > Calvin Wan <calvinwan@google.com> writes: > > > In preparation for later changes, move setup code into test_expect > > blocks. Smaller sections are moved into existing blocks, while larger > > sections with a standalone purpose are given their own new blocks. > > The changes where we moved lines outside of blocks into blocks without > changing them look good to me. > > > While at it, have tests clean up after themselves with > > test_when_finished > > I believe this came about as part of the discussion in > > https://lore.kernel.org/git/xmqqedqtbbf4.fsf@gitster.g > > I think it's good to have tests clean up after themselves, but I'm not > sure if that's what we're doing in all of these cases, see below. > > I'm leaving the diff header in place, since the two files have very > confusingly similar tests. > > > diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh > > test_expect_success 'typechanged submodule(submodule->blob)' ' > > + test_when_finished rm -rf sm1 && > > git diff --submodule=log >actual && > > cat >expected <<-EOF && > > diff --git a/sm1 b/sm1 > > This hunk and the next... > > > @@ -212,9 +215,9 @@ test_expect_success 'typechanged submodule(submodule->blob)' ' > > test_cmp expected actual > > ' > > > > -rm -rf sm1 && > > -git checkout-index sm1 > > test_expect_success 'typechanged submodule(submodule->blob)' ' > > + test_when_finished rm -f sm1 && > > + git checkout-index sm1 && > > git diff-index -p --submodule=log HEAD >actual && > > cat >expected <<-EOF && > > Submodule sm1 $head4...0000000 (submodule deleted) > > were changed so that the "rm -rf" happens in the clean up phase of the > earlier test (test 14) instead of set up phase of the later test (test > 15). But, the "rm -rf" actually results in a _different_ state from > before 14, so it isn't actually cleaning up, it really is preparation > for 15's git checkout-index. > > You can observe this by running > > ./t4041-diff-submodule-option.sh --run=1-13,15 > > which fails as expected. On the other hand, it passes if we move the "rm > -rf" into test 15. > > Nearly all of the other test_when_finished here have the same problem, > where they 'clean up' state that wasn't changed in the same test body. I > believe they will show similar dependency issues, though I didn't go > through and test them all. Good catch. I'll go thru the rest of them and remove the dependency issues. > > > @@ -643,7 +643,6 @@ test_expect_success 'modified submodule contains modified content' ' > > diff_cmp expected actual > > ' > > > > -rm -rf sm1 > > test_expect_success 'deleted submodule' ' > > git diff-index -p --submodule=diff HEAD >actual && > > cat >expected <<-EOF && > > This one is fairly obvious, since the test says 'deleted submodule', but > we no longer delete the submodule in the setup. > > > @@ -779,9 +780,8 @@ test_expect_success 'diff --submodule=diff with .git file' ' > > diff_cmp expected actual > > ' > > > > -mv sm2 sm2-bak > > - > > test_expect_success 'deleted submodule with .git file' ' > > + mv sm2 sm2-bak && > > git diff-index -p --submodule=diff HEAD >actual && > > cat >expected <<-EOF && > > Submodule sm1 $head7...0000000 (submodule deleted) > > @@ -804,9 +804,9 @@ test_expect_success 'deleted submodule with .git file' ' > > diff_cmp expected actual > > ' > > > > -echo submodule-to-blob>sm2 > > - > > test_expect_success 'typechanged(submodule->blob) submodule with .git file' ' > > + test_when_finished "rm sm2 && mv sm2-bak sm2" && > > + echo submodule-to-blob>sm2 && > > git diff-index -p --submodule=diff HEAD >actual && > > cat >expected <<-EOF && > > Submodule sm1 $head7...0000000 (submodule deleted) > > @@ -836,9 +836,6 @@ test_expect_success 'typechanged(submodule->blob) submodule with .git file' ' > > diff_cmp expected actual > > ' > > > > -rm sm2 > > -mv sm2-bak sm2 > > This is the original case that Junio flagged, which I think is an almost > correct use of test_when_finished, since we do get back to an earlier > state before this string of tests, but not to the state before the > actual test with the test_when_finished. > > If we want to use test_when_finished here (which I think we do), we > should add another test_when_finished to remove the dependency between > the two tests. like so: > > test_expect_success 'deleted submodule with .git file' ' > + test_when_finished "mv sm2-bak sm2" && > mv sm2 sm2-bak && > git diff-index -p --submodule=diff HEAD >actual && > > ... > > test_expect_success 'typechanged(submodule->blob) submodule with .git file' ' > test_when_finished "rm sm2 && mv sm2-bak sm2" && > + mv sm2 sm2-bak && > > Currently, they're still dependent because one creates sm2-bak and the > other moves it back, but if we have each test restore sm2, there will be > no more dependency. > That all makes sense. Thanks for the recommendations ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 2/6] tests: Use `git submodule add` instead of `git add` 2023-02-28 18:52 ` [PATCH v2 0/6] add: block invalid submodules Calvin Wan 2023-02-28 18:56 ` [PATCH v2 1/6] t4041, t4060: modernize test style Calvin Wan @ 2023-02-28 18:56 ` Calvin Wan 2023-02-28 23:30 ` Junio C Hamano 2023-03-06 21:26 ` Glen Choo 2023-02-28 18:56 ` [PATCH v2 3/6] tests: use `git submodule add` and fix expected diffs Calvin Wan ` (3 subsequent siblings) 5 siblings, 2 replies; 40+ messages in thread From: Calvin Wan @ 2023-02-28 18:56 UTC (permalink / raw) To: git; +Cc: Josh Steadmon, peff, gitster, Calvin Wan From: Josh Steadmon <steadmon@google.com> Since 532139940c (add: warn when adding an embedded repository, 2017-06-14), we have recognized that adding embedded repositories is a subpar user experience compared to submodules. Embedded repositories lack a .gitmodules entry, which means clones of the top-level repo will be unable to obtain the nested repo. We issue advice about this situation in the case where a user adds an embedded repository. However, many tests predate this advice, and directly add embedded repositories instead of using submodules. This commit cleans up such test cases where minimal other changes are required (e.g., committing later changes to .gitmodules or changing the counts of committed files in the worktree). Future commits will handle tests requiring more complicated adjustments. These changes will enable us to switch the default behavior of git-add from warning about embedded repositories to rejecting them outright. See later commits for a further discussion of that topic. Signed-off-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> --- t/t2013-checkout-submodule.sh | 4 +++- t/t2103-update-index-ignore-missing.sh | 2 +- t/t2107-update-index-basic.sh | 2 +- t/t3040-subprojects-basic.sh | 2 +- t/t3050-subprojects-fetch.sh | 3 ++- t/t3404-rebase-interactive.sh | 3 ++- t/t3701-add-interactive.sh | 5 +++-- t/t4010-diff-pathspec.sh | 2 +- t/t4020-diff-external.sh | 2 +- t/t5531-deep-submodule-push.sh | 4 ++-- t/t6416-recursive-corner-cases.sh | 12 ++++++------ t/t6437-submodule-merge.sh | 12 ++++++------ t/t7401-submodule-summary.sh | 4 ++-- t/t7402-submodule-rebase.sh | 2 +- 14 files changed, 32 insertions(+), 27 deletions(-) diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh index b2bdd1fcb4..dd2858648b 100755 --- a/t/t2013-checkout-submodule.sh +++ b/t/t2013-checkout-submodule.sh @@ -10,7 +10,7 @@ test_expect_success 'setup' ' (cd submodule && git init && test_commit first) && - git add submodule && + git submodule add ./submodule && test_tick && git commit -m superproject && (cd submodule && @@ -51,6 +51,7 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .gitm git config diff.ignoreSubmodules none && git config -f .gitmodules submodule.submodule.path submodule && git config -f .gitmodules submodule.submodule.ignore untracked && + git commit -m "Update patterns in .gitmodules" .gitmodules && git checkout HEAD >actual 2>&1 && test_must_be_empty actual ' @@ -59,6 +60,7 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .git/ git config -f .gitmodules submodule.submodule.ignore none && git config submodule.submodule.path submodule && git config submodule.submodule.ignore all && + git commit -m "Update patterns in .gitmodules" .gitmodules && git checkout HEAD >actual 2>&1 && test_must_be_empty actual ' diff --git a/t/t2103-update-index-ignore-missing.sh b/t/t2103-update-index-ignore-missing.sh index e9451cd567..11bc136f6e 100755 --- a/t/t2103-update-index-ignore-missing.sh +++ b/t/t2103-update-index-ignore-missing.sh @@ -36,7 +36,7 @@ test_expect_success basics ' git add file && git commit -m "sub initial" ) && - git add xyzzy && + git add ./xyzzy && test_tick && git commit -m initial && diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh index 07e6de84e6..465b41ccdc 100755 --- a/t/t2107-update-index-basic.sh +++ b/t/t2107-update-index-basic.sh @@ -49,7 +49,7 @@ test_expect_success '--cacheinfo does not accept blob null sha1' ' test_expect_success '--cacheinfo does not accept gitlink null sha1' ' git init submodule && (cd submodule && test_commit foo) && - git add submodule && + git submodule add ./submodule && git rev-parse :submodule >expect && test_must_fail git update-index --cacheinfo 160000 $ZERO_OID submodule && git rev-parse :submodule >actual && diff --git a/t/t3040-subprojects-basic.sh b/t/t3040-subprojects-basic.sh index bd65dfcffc..61da7e3b94 100755 --- a/t/t3040-subprojects-basic.sh +++ b/t/t3040-subprojects-basic.sh @@ -69,7 +69,7 @@ test_expect_success 'check if clone works' ' test_expect_success 'removing and adding subproject' ' git update-index --force-remove -- sub2 && mv sub2 sub3 && - git add sub3 && + git submodule add ./sub3 && git commit -q -m "renaming a subproject" && test_expect_code 1 git diff -M --name-status --exit-code HEAD^ HEAD ' diff --git a/t/t3050-subprojects-fetch.sh b/t/t3050-subprojects-fetch.sh index f1f09abdd9..9a692274b9 100755 --- a/t/t3050-subprojects-fetch.sh +++ b/t/t3050-subprojects-fetch.sh @@ -14,7 +14,8 @@ test_expect_success setup ' git commit -m "subproject commit #1" ) && >mainfile && - git add sub mainfile && + git add mainfile && + git submodule add ./sub && test_tick && git commit -m "superproject commit #1" ' diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 462cefd25d..1d0574216b 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -675,7 +675,8 @@ test_expect_success 'submodule rebase setup' ' git add elif && git commit -m "submodule initial" ) && echo 1 >file1 && - git add file1 sub && + git add file1 && + git submodule add ./sub && test_tick && git commit -m "One" && echo 2 >file1 && diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 5841f280fb..715c4fcc62 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -913,8 +913,9 @@ test_expect_success 'setup different kinds of dirty submodules' ' ) && cp -R dirty-head dirty-otherwise && cp -R dirty-head dirty-both-ways && - git add dirty-head && - git add dirty-otherwise dirty-both-ways && + git submodule add ./dirty-head && + git submodule add ./dirty-otherwise && + git submodule add ./dirty-both-ways && git commit -m initial && cd dirty-head && diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh index 9d9650eba7..844258c418 100755 --- a/t/t4010-diff-pathspec.sh +++ b/t/t4010-diff-pathspec.sh @@ -117,7 +117,7 @@ test_expect_success 'setup submodules' ' test_tick && git init submod && ( cd submod && test_commit first ) && - git add submod && + git submodule add ./submod && git commit -m first && ( cd submod && test_commit second ) && git add submod && diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh index c1ac09ecc7..ca2a23a78f 100755 --- a/t/t4020-diff-external.sh +++ b/t/t4020-diff-external.sh @@ -253,7 +253,7 @@ test_expect_success 'clean up crlf leftovers' ' test_expect_success 'submodule diff' ' git init sub && ( cd sub && test_commit sub1 ) && - git add sub && + git submodule add ./sub && test_tick && git commit -m "add submodule" && ( cd sub && test_commit sub2 ) && diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh index 302e4cbdba..f8ef70b5a2 100755 --- a/t/t5531-deep-submodule-push.sh +++ b/t/t5531-deep-submodule-push.sh @@ -28,7 +28,7 @@ test_expect_success setup ' git add junk && git commit -m "Initial junk" ) && - git add gar/bage && + git submodule add ./gar/bage ./gar/bage && git commit -m "Initial superproject" ) ' @@ -367,7 +367,7 @@ test_expect_success 'push succeeds if submodule has no remote and is on the firs git add junk && git commit -m "initial" ) && - git add b && + git submodule add ./b && git commit -m "added submodule" && git push --recurse-submodules=check origin main ) diff --git a/t/t6416-recursive-corner-cases.sh b/t/t6416-recursive-corner-cases.sh index 17b54d625d..b366dd77e5 100755 --- a/t/t6416-recursive-corner-cases.sh +++ b/t/t6416-recursive-corner-cases.sh @@ -1270,7 +1270,7 @@ test_expect_success 'setup submodule modify/modify' ' ) && git -C submod reset --hard A && - git add submod && + git submodule add ./submod && git commit -m A && git tag A && @@ -1303,7 +1303,7 @@ test_expect_merge_algorithm failure success 'check submodule modify/modify' ' test_must_fail git merge -s recursive E^0 && git ls-files -s >out && - test_line_count = 3 out && + test_line_count = 4 out && git ls-files -u >out && test_line_count = 3 out && git ls-files -o >out && @@ -1364,12 +1364,12 @@ test_expect_success 'setup submodule add/add' ' git checkout -b B A && git -C submod reset --hard B && - git add submod && + git submodule add ./submod && git commit -m B && git checkout -b C A && git -C submod reset --hard C && - git add submod && + git submodule add ./submod && git commit -m C && git checkout -q B^0 && @@ -1391,7 +1391,7 @@ test_expect_merge_algorithm failure success 'check submodule add/add' ' test_must_fail git merge -s recursive E^0 && git ls-files -s >out && - test_line_count = 3 out && + test_line_count = 4 out && git ls-files -u >out && test_line_count = 2 out && git ls-files -o >out && @@ -1439,7 +1439,7 @@ test_expect_success 'setup conflicting entry types (submodule vs symlink)' ' git checkout -b B A && git -C path reset --hard B && - git add path && + git submodule add ./path && git commit -m B && git checkout -b C A && diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh index c9a86f2e94..7f6e89f541 100755 --- a/t/t6437-submodule-merge.sh +++ b/t/t6437-submodule-merge.sh @@ -30,7 +30,7 @@ test_expect_success setup ' git add file && test_tick && git commit -m sub-root) && - git add sub && + git submodule add ./sub && test_tick && git commit -m root && @@ -85,7 +85,7 @@ test_expect_success 'setup for merge search' ' git branch sub-a) && git commit --allow-empty -m init && git branch init && - git add sub && + git submodule add ./sub && git commit -m "a" && git branch a && @@ -132,7 +132,7 @@ test_expect_success 'finish setup for merge-search' ' git checkout -b g init && (cd sub && git checkout -b sub-g sub-c) && - git add sub && + git submodule add ./sub && git commit -a -m "g") ' @@ -296,7 +296,7 @@ test_expect_success 'setup for recursive merge with submodule' ' git checkout -b sub-cb sub-c && git merge sub-b && git checkout main) && - git add sub && + git submodule add ./sub && git commit -m a && git checkout -b top-b main && (cd sub && git checkout sub-b) && @@ -520,7 +520,7 @@ test_expect_success 'setup for null merge base' ' git commit --allow-empty -m init && git branch init && git checkout -b a init && - git add sub && + git submodule add ./sub && git commit -m "a" && git switch main && (cd sub && @@ -532,7 +532,7 @@ test_expect_success 'setup for null merge base' ' test_expect_success 'merging should fail with no merge base' ' (cd no-merge-base && git checkout -b b init && - git add sub && + git submodule add ./sub && git commit -m "b" && test_must_fail git merge a >actual && if test "$GIT_TEST_MERGE_ALGORITHM" = ort diff --git a/t/t7401-submodule-summary.sh b/t/t7401-submodule-summary.sh index 542b3331a7..73fd09edb6 100755 --- a/t/t7401-submodule-summary.sh +++ b/t/t7401-submodule-summary.sh @@ -45,7 +45,7 @@ add_file . foo >/dev/null head1=$(add_file sm1 foo1 foo2) test_expect_success 'added submodule' " - git add sm1 && + git submodule add ./sm1 && git submodule summary >actual && cat >expected <<-EOF && * sm1 0000000...$head1 (2): @@ -253,7 +253,7 @@ test_expect_success 'deleted submodule' " test_expect_success 'create second submodule' ' test_create_repo sm2 && head7=$(add_file sm2 foo8 foo9) && - git add sm2 + git submodule add ./sm2 ' test_expect_success 'multiple submodules' " diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh index ebeca12a71..abc2092741 100755 --- a/t/t7402-submodule-rebase.sh +++ b/t/t7402-submodule-rebase.sh @@ -14,7 +14,7 @@ test_expect_success setup ' test_tick && git commit -m initial && git clone . submodule && - git add submodule && + git submodule add ./submodule && test_tick && git commit -m submodule && echo second line >> file && -- 2.39.2.722.g9855ee24e9-goog ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 2/6] tests: Use `git submodule add` instead of `git add` 2023-02-28 18:56 ` [PATCH v2 2/6] tests: Use `git submodule add` instead of `git add` Calvin Wan @ 2023-02-28 23:30 ` Junio C Hamano 2023-03-03 0:16 ` Calvin Wan 2023-03-06 21:26 ` Glen Choo 1 sibling, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2023-02-28 23:30 UTC (permalink / raw) To: Calvin Wan; +Cc: git, Josh Steadmon, peff Calvin Wan <calvinwan@google.com> writes: > diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh > index b2bdd1fcb4..dd2858648b 100755 > --- a/t/t2013-checkout-submodule.sh > +++ b/t/t2013-checkout-submodule.sh > @@ -10,7 +10,7 @@ test_expect_success 'setup' ' > (cd submodule && > git init && > test_commit first) && > - git add submodule && > + git submodule add ./submodule && The change from "submodule" to "./submodule" was not explained in the proposed log message. I think this is necessary for "git submodule add" to function as expected, but if that is why we are making this change, perhaps we should mention it? > @@ -51,6 +51,7 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .gitm > git config diff.ignoreSubmodules none && > git config -f .gitmodules submodule.submodule.path submodule && > git config -f .gitmodules submodule.submodule.ignore untracked && > + git commit -m "Update patterns in .gitmodules" .gitmodules && What does "patterns" refer here (another one in the next hunk)? > diff --git a/t/t2103-update-index-ignore-missing.sh b/t/t2103-update-index-ignore-missing.sh > index e9451cd567..11bc136f6e 100755 > --- a/t/t2103-update-index-ignore-missing.sh > +++ b/t/t2103-update-index-ignore-missing.sh > @@ -36,7 +36,7 @@ test_expect_success basics ' > git add file && > git commit -m "sub initial" > ) && > - git add xyzzy && > + git add ./xyzzy && Is this supposed to have become "git submodule add ./xyzzy"? Or "git add xyzzy" will trigger "don't add gitlink" warning but you can write "git add ./xyzzy" as a way to work around the warning? Or is this an incomplete change that wasn't spotted during proofreading? > diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh > index 302e4cbdba..f8ef70b5a2 100755 > --- a/t/t5531-deep-submodule-push.sh > +++ b/t/t5531-deep-submodule-push.sh > @@ -28,7 +28,7 @@ test_expect_success setup ' > git add junk && > git commit -m "Initial junk" > ) && > - git add gar/bage && > + git submodule add ./gar/bage ./gar/bage && Why does this one (and only this one) look different? Everybody else changed "git add A" to "git submodule add ./A", it seems? > diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh > ... I think I saw a code section that was touched in the previous patch that hand-crafted .gitmodules file to make the gitlink it adds into a submodule. It is unexpected and puzzling that there is no removal of that "cat >.gitmodules" from t4060. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 2/6] tests: Use `git submodule add` instead of `git add` 2023-02-28 23:30 ` Junio C Hamano @ 2023-03-03 0:16 ` Calvin Wan 0 siblings, 0 replies; 40+ messages in thread From: Calvin Wan @ 2023-03-03 0:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Josh Steadmon, peff On Tue, Feb 28, 2023 at 3:30 PM Junio C Hamano <gitster@pobox.com> wrote: > > Calvin Wan <calvinwan@google.com> writes: > > > diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh > > index b2bdd1fcb4..dd2858648b 100755 > > --- a/t/t2013-checkout-submodule.sh > > +++ b/t/t2013-checkout-submodule.sh > > @@ -10,7 +10,7 @@ test_expect_success 'setup' ' > > (cd submodule && > > git init && > > test_commit first) && > > - git add submodule && > > + git submodule add ./submodule && > > The change from "submodule" to "./submodule" was not explained in > the proposed log message. I think this is necessary for "git > submodule add" to function as expected, but if that is why we are > making this change, perhaps we should mention it? ack. > > > @@ -51,6 +51,7 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .gitm > > git config diff.ignoreSubmodules none && > > git config -f .gitmodules submodule.submodule.path submodule && > > git config -f .gitmodules submodule.submodule.ignore untracked && > > + git commit -m "Update patterns in .gitmodules" .gitmodules && > > What does "patterns" refer here (another one in the next hunk)? "patterns" seems unnecessary here. "Update .gitmodules" is more than sufficient. > > > diff --git a/t/t2103-update-index-ignore-missing.sh b/t/t2103-update-index-ignore-missing.sh > > index e9451cd567..11bc136f6e 100755 > > --- a/t/t2103-update-index-ignore-missing.sh > > +++ b/t/t2103-update-index-ignore-missing.sh > > @@ -36,7 +36,7 @@ test_expect_success basics ' > > git add file && > > git commit -m "sub initial" > > ) && > > - git add xyzzy && > > + git add ./xyzzy && > > Is this supposed to have become "git submodule add ./xyzzy"? Or > "git add xyzzy" will trigger "don't add gitlink" warning but you can > write "git add ./xyzzy" as a way to work around the warning? > > Or is this an incomplete change that wasn't spotted during > proofreading? The latter. Missed this one when I was reverting the change from v1. > > > diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh > > index 302e4cbdba..f8ef70b5a2 100755 > > --- a/t/t5531-deep-submodule-push.sh > > +++ b/t/t5531-deep-submodule-push.sh > > @@ -28,7 +28,7 @@ test_expect_success setup ' > > git add junk && > > git commit -m "Initial junk" > > ) && > > - git add gar/bage && > > + git submodule add ./gar/bage ./gar/bage && > > Why does this one (and only this one) look different? Everybody > else changed "git add A" to "git submodule add ./A", it seems? The second ./gar/bage is to define the submodule path. Without it, submodule add attempts to add it to ./bage instead of ./gar/bage. This is unique only to this test since the submodule is 2 folders deep. > > > diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh > > ... > > I think I saw a code section that was touched in the previous patch > that hand-crafted .gitmodules file to make the gitlink it adds into > a submodule. It is unexpected and puzzling that there is no removal > of that "cat >.gitmodules" from t4060. > I have gone ahead and removed the "cat >.gitmodules" from t4060, and fixed the subsequent tests that were affected by that ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 2/6] tests: Use `git submodule add` instead of `git add` 2023-02-28 18:56 ` [PATCH v2 2/6] tests: Use `git submodule add` instead of `git add` Calvin Wan 2023-02-28 23:30 ` Junio C Hamano @ 2023-03-06 21:26 ` Glen Choo 1 sibling, 0 replies; 40+ messages in thread From: Glen Choo @ 2023-03-06 21:26 UTC (permalink / raw) To: Calvin Wan, git; +Cc: Josh Steadmon, peff, gitster, Calvin Wan Calvin Wan <calvinwan@google.com> writes: > This commit cleans up such > test cases where minimal other changes are required (e.g., committing > later changes to .gitmodules or changing the counts of committed files > in the worktree). Okay, though perhaps not detailed enough to explain why these changes were needed in some circumstances. It might be helpful to explain the exact differences that you are adjusting for, i.e.: - 'git submodule add' requires a './'-prefixed path - 'git submodule add' changes and tracks .gitmodules > diff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh > index b2bdd1fcb4..dd2858648b 100755 > --- a/t/t2013-checkout-submodule.sh > +++ b/t/t2013-checkout-submodule.sh > @@ -10,7 +10,7 @@ test_expect_success 'setup' ' > (cd submodule && > git init && > test_commit first) && > - git add submodule && > + git submodule add ./submodule && > test_tick && > git commit -m superproject && > (cd submodule && Junio mentioned that the change from submodule to ./submodule was surprising, but this seemed quite clear to me. Maybe that's just a sign that I've worked on submodules for too long. > @@ -51,6 +51,7 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .gitm > git config diff.ignoreSubmodules none && > git config -f .gitmodules submodule.submodule.path submodule && > git config -f .gitmodules submodule.submodule.ignore untracked && > + git commit -m "Update patterns in .gitmodules" .gitmodules && > git checkout HEAD >actual 2>&1 && > test_must_be_empty actual > ' > @@ -59,6 +60,7 @@ test_expect_success '"checkout <submodule>" honors submodule.*.ignore from .git/ > git config -f .gitmodules submodule.submodule.ignore none && > git config submodule.submodule.path submodule && > git config submodule.submodule.ignore all && > + git commit -m "Update patterns in .gitmodules" .gitmodules && > git checkout HEAD >actual 2>&1 && > test_must_be_empty actual I puzzled a lot over why these changes were necessary. The answer is that .gitmodules was formerly untracked, so it didn't show up in the list of files during checkout. But, now that we use 'git submodule add', we do track .gitmodules, so we instead 'fix' the test by committing the .gitmodules. This one is unobvious enough that I think it's worth calling out specifically in the commit message. ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 3/6] tests: use `git submodule add` and fix expected diffs 2023-02-28 18:52 ` [PATCH v2 0/6] add: block invalid submodules Calvin Wan 2023-02-28 18:56 ` [PATCH v2 1/6] t4041, t4060: modernize test style Calvin Wan 2023-02-28 18:56 ` [PATCH v2 2/6] tests: Use `git submodule add` instead of `git add` Calvin Wan @ 2023-02-28 18:56 ` Calvin Wan 2023-03-06 23:34 ` Glen Choo 2023-02-28 18:56 ` [PATCH v2 4/6] tests: use `git submodule add` and fix expected status Calvin Wan ` (2 subsequent siblings) 5 siblings, 1 reply; 40+ messages in thread From: Calvin Wan @ 2023-02-28 18:56 UTC (permalink / raw) To: git; +Cc: Josh Steadmon, peff, gitster, Calvin Wan From: Josh Steadmon <steadmon@google.com> This commit continues the previous work of updating the test suite to use `git submodule add` to create submodules instead of using `git add` to include embedded repositories. Specifically, in this commit we update test cases where expected diffs must change due to the presence of a .gitmodules file. Signed-off-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> --- t/t3040-subprojects-basic.sh | 3 +- t/t4041-diff-submodule-option.sh | 153 ++++++++++++++++++- t/t4060-diff-submodule-option-diff-format.sh | 125 +++++++++++++-- t/t7506-status-submodule.sh | 15 +- 4 files changed, 278 insertions(+), 18 deletions(-) diff --git a/t/t3040-subprojects-basic.sh b/t/t3040-subprojects-basic.sh index 61da7e3b94..a0f14db3d2 100755 --- a/t/t3040-subprojects-basic.sh +++ b/t/t3040-subprojects-basic.sh @@ -19,11 +19,12 @@ test_expect_success 'setup: create subprojects' ' ( cd sub2 && git init && : >Makefile && git add * && git commit -q -m "subproject 2" ) && git update-index --add sub1 && - git add sub2 && + git submodule add ./sub2 && git commit -q -m "subprojects added" && GIT_PRINT_SHA1_ELLIPSIS="yes" git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current && git branch save HEAD && cat >expected <<-\EOF && + :000000 100644 00000... A .gitmodules :000000 160000 00000... A sub1 :000000 160000 00000... A sub2 EOF diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh index 2aa12243bd..f5074071a4 100755 --- a/t/t4041-diff-submodule-option.sh +++ b/t/t4041-diff-submodule-option.sh @@ -50,9 +50,19 @@ test_expect_success 'setup' ' ' test_expect_success 'added submodule' ' - git add sm1 && + git submodule add ./sm1 && + gitmodules_hash1=$(git rev-parse --short $(git hash-object .gitmodules)) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 0000000...$head1 (new submodule) EOF test_cmp expected actual @@ -63,6 +73,15 @@ test_expect_success 'added submodule, set diff.submodule' ' git add sm1 && git diff --cached >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 0000000...$head1 (new submodule) EOF git config --unset diff.submodule && @@ -74,6 +93,15 @@ test_expect_success '--submodule=short overrides diff.submodule' ' git add sm1 && git diff --submodule=short --cached >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 diff --git a/sm1 b/sm1 new file mode 160000 index 0000000..$head1 @@ -89,6 +117,15 @@ test_expect_success 'diff.submodule does not affect plumbing' ' test_config diff.submodule log && git diff-index -p HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 diff --git a/sm1 b/sm1 new file mode 160000 index 0000000..$head1 @@ -105,6 +142,15 @@ test_expect_success 'modified submodule(forward)' ' head2=$(add_file sm1 foo3) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head1..$head2: > Add foo3 ($added foo3) EOF @@ -154,6 +200,15 @@ test_expect_success 'modified submodule(backward)' ' ) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head2..$head3 (rewind): < Add foo3 ($added foo3) < Add foo2 ($added foo2) @@ -165,6 +220,15 @@ test_expect_success 'modified submodule(backward and forward)' ' head4=$(add_file sm1 foo4 foo5) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head2...$head4: > Add foo5 ($added foo5) > Add foo4 ($added foo4) @@ -187,6 +251,15 @@ test_expect_success 'setup - change sm1 to a blob' ' test_expect_success 'typechanged submodule(submodule->blob), --cached' ' git diff --submodule=log --cached >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head4...0000000 (submodule deleted) diff --git a/sm1 b/sm1 new file mode 100644 @@ -220,6 +293,15 @@ test_expect_success 'typechanged submodule(submodule->blob)' ' git checkout-index sm1 && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head4...0000000 (submodule deleted) diff --git a/sm1 b/sm1 new file mode 100644 @@ -238,6 +320,15 @@ test_expect_success 'nonexistent commit' ' fullhead6=$(git -C sm1 rev-parse --verify HEAD) && git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..$gitmodules_hash1 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head4...$head6 (commits not present) EOF test_cmp expected actual @@ -436,12 +527,24 @@ test_expect_success 'deleted submodule' ' test_expect_success 'create second submodule' ' test_create_repo sm2 && head7=$(add_file sm2 foo8 foo9) && - git add sm2 + git submodule add ./sm2 && + gitmodules_hash2=$(git rev-parse --short $(git hash-object .gitmodules)) ' test_expect_success 'multiple submodules' ' git diff-index -p --submodule=log HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + index $gitmodules_hash1..$gitmodules_hash2 100644 + --- a/.gitmodules + +++ b/.gitmodules + @@ -1,3 +1,6 @@ + [submodule "sm1"] + path = sm1 + url = ./sm1 + +[submodule "sm2"] + + path = sm2 + + url = ./sm2 Submodule sm1 $head6...0000000 (submodule deleted) Submodule sm2 0000000...$head7 (new submodule) EOF @@ -460,6 +563,17 @@ test_expect_success 'given commit' ' commit_file sm2 && git diff-index -p --submodule=log HEAD^ >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + index $gitmodules_hash1..$gitmodules_hash2 100644 + --- a/.gitmodules + +++ b/.gitmodules + @@ -1,3 +1,6 @@ + [submodule "sm1"] + path = sm1 + url = ./sm1 + +[submodule "sm2"] + + path = sm2 + + url = ./sm2 Submodule sm1 $head6...0000000 (submodule deleted) Submodule sm2 0000000...$head7 (new submodule) EOF @@ -469,6 +583,17 @@ test_expect_success 'given commit' ' test_expect_success 'given commit --submodule' ' git diff-index -p --submodule HEAD^ >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + index $gitmodules_hash1..$gitmodules_hash2 100644 + --- a/.gitmodules + +++ b/.gitmodules + @@ -1,3 +1,6 @@ + [submodule "sm1"] + path = sm1 + url = ./sm1 + +[submodule "sm2"] + + path = sm2 + + url = ./sm2 Submodule sm1 $head6...0000000 (submodule deleted) Submodule sm2 0000000...$head7 (new submodule) EOF @@ -479,6 +604,17 @@ test_expect_success 'given commit --submodule=short' ' fullhead7=$(git -C sm2 rev-parse --verify HEAD) && git diff-index -p --submodule=short HEAD^ >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + index $gitmodules_hash1..$gitmodules_hash2 100644 + --- a/.gitmodules + +++ b/.gitmodules + @@ -1,3 +1,6 @@ + [submodule "sm1"] + path = sm1 + url = ./sm1 + +[submodule "sm2"] + + path = sm2 + + url = ./sm2 diff --git a/sm1 b/sm1 deleted file mode 160000 index $head6..0000000 @@ -507,6 +643,17 @@ test_expect_success 'setup .git file for sm2' ' test_expect_success 'diff --submodule with .git file' ' git diff --submodule HEAD^ >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + index $gitmodules_hash1..$gitmodules_hash2 100644 + --- a/.gitmodules + +++ b/.gitmodules + @@ -1,3 +1,6 @@ + [submodule "sm1"] + path = sm1 + url = ./sm1 + +[submodule "sm2"] + + path = sm2 + + url = ./sm2 Submodule sm1 $head6...0000000 (submodule deleted) Submodule sm2 0000000...$head7 (new submodule) EOF @@ -525,7 +672,7 @@ test_expect_success 'diff --submodule with objects referenced by alternates' ' (cd super && git clone -s ../sub_alt sub && git init && - git add sub && + git submodule add ./sub && git commit -m "sub a" ) && (cd sub_alt && diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh index a760ed5eb6..89c5641c6b 100755 --- a/t/t4060-diff-submodule-option-diff-format.sh +++ b/t/t4060-diff-submodule-option-diff-format.sh @@ -61,9 +61,18 @@ test_expect_success 'setup repository' ' ' test_expect_success 'added submodule' ' - git add sm1 && + git submodule add ./sm1 && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 1234567..89abcde + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 0000000...$head1 (new submodule) diff --git a/sm1/foo1 b/sm1/foo1 new file mode 100644 @@ -88,6 +97,15 @@ test_expect_success 'added submodule, set diff.submodule' ' git add sm1 && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 1234567..89abcde + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 0000000...$head1 (new submodule) diff --git a/sm1/foo1 b/sm1/foo1 new file mode 100644 @@ -112,6 +130,15 @@ test_expect_success '--submodule=short overrides diff.submodule' ' git add sm1 && git diff --submodule=short --cached >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 1234567..89abcde + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 diff --git a/sm1 b/sm1 new file mode 160000 index 0000000..$head1 @@ -127,6 +154,15 @@ test_expect_success 'diff.submodule does not affect plumbing' ' test_config diff.submodule log && git diff-index -p HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 1234567..89abcde + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 diff --git a/sm1 b/sm1 new file mode 160000 index 0000000..$head1 @@ -143,6 +179,15 @@ test_expect_success 'modified submodule(forward)' ' head2=$(add_file sm1 foo3) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..2334463 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head1..$head2: diff --git a/sm1/foo3 b/sm1/foo3 new file mode 100644 @@ -202,6 +247,15 @@ test_expect_success 'modified submodule(backward)' ' ) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..2334463 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head2..$head3 (rewind): diff --git a/sm1/foo2 b/sm1/foo2 deleted file mode 100644 @@ -225,6 +279,15 @@ test_expect_success 'modified submodule(backward and forward)' ' head4=$(add_file sm1 foo4 foo5) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..2334463 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head2...$head4: diff --git a/sm1/foo2 b/sm1/foo2 deleted file mode 100644 @@ -271,6 +334,15 @@ test_expect_success 'setup - change sm1 to a blob' ' test_expect_success 'typechanged submodule(submodule->blob), --cached' ' git diff --submodule=diff --cached >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..2334463 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head4...0000000 (submodule deleted) diff --git a/sm1/foo1 b/sm1/foo1 deleted file mode 100644 @@ -346,6 +418,15 @@ test_expect_success 'typechanged submodule(submodule->blob)' ' git checkout-index sm1 && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..2334463 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head4...0000000 (submodule deleted) diff --git a/sm1 b/sm1 new file mode 100644 @@ -363,6 +444,15 @@ test_expect_success 'nonexistent commit' ' head6=$(add_file sm1 foo6 foo7) && git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + new file mode 100644 + index 0000000..2334463 + --- /dev/null + +++ b/.gitmodules + @@ -0,0 +1,3 @@ + +[submodule "sm1"] + + path = sm1 + + url = ./sm1 Submodule sm1 $head4...$head6 (commits not present) EOF diff_cmp expected actual @@ -654,12 +744,23 @@ test_expect_success 'deleted submodule' ' test_expect_success 'create second submodule' ' test_create_repo sm2 && head9=$(add_file sm2 foo8 foo9) && - git add sm2 + git submodule add ./sm2 ' test_expect_success 'multiple submodules' ' git diff-index -p --submodule=diff HEAD >actual && cat >expected <<-EOF && + diff --git a/.gitmodules b/.gitmodules + index 2334463..8809a9f 100644 + --- a/.gitmodules + +++ b/.gitmodules + @@ -1,3 +1,6 @@ + [submodule "sm1"] + path = sm1 + url = ./sm1 + +[submodule "sm2"] + + path = sm2 + + url = ./sm2 Submodule sm1 $head7...0000000 (submodule deleted) Submodule sm2 0000000...$head9 (new submodule) diff --git a/sm2/foo8 b/sm2/foo8 @@ -716,11 +817,13 @@ test_expect_success 'given commit' ' git diff-index -p --submodule=diff HEAD^ >actual && cat >expected <<-EOF && diff --git a/.gitmodules b/.gitmodules - new file mode 100644 - index 1234567..89abcde - --- /dev/null + index 1234567..89abcde 100644 + --- a/.gitmodules +++ b/.gitmodules - @@ -0,0 +1,3 @@ + @@ -1,3 +1,3 @@ + -[submodule "sm1"] + - path = sm1 + - url = ./sm1 +[submodule "sm2"] +path = sm2 +url = bogus_url @@ -752,11 +855,13 @@ test_expect_success 'diff --submodule=diff with .git file' ' git diff --submodule=diff HEAD^ >actual && cat >expected <<-EOF && diff --git a/.gitmodules b/.gitmodules - new file mode 100644 - index 1234567..89abcde - --- /dev/null + index 1234567..89abcde 100644 + --- a/.gitmodules +++ b/.gitmodules - @@ -0,0 +1,3 @@ + @@ -1,3 +1,3 @@ + -[submodule "sm1"] + - path = sm1 + - url = ./sm1 +[submodule "sm2"] +path = sm2 +url = bogus_url diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh index d050091345..9dbd9ff266 100755 --- a/t/t7506-status-submodule.sh +++ b/t/t7506-status-submodule.sh @@ -31,7 +31,8 @@ sanitize_diff () { test_expect_success 'setup' ' test_create_repo_with_commit sub && echo output > .gitignore && - git add sub .gitignore && + git submodule add ./sub && + git add .gitignore && git commit -m "Add submodule sub" ' @@ -243,7 +244,7 @@ test_expect_success 'status -a clean (empty submodule dir)' ' ' cat >status_expect <<\EOF -AA .gitmodules +UU .gitmodules A sub1 EOF @@ -277,7 +278,10 @@ cat >diff_expect <<\EOF diff --cc .gitmodules --- a/.gitmodules +++ b/.gitmodules -@@@ -1,3 -1,3 +1,9 @@@ +@@@ -1,6 -1,6 +1,12 @@@ + [submodule "sub"] + path = sub + url = ./sub ++<<<<<<< HEAD +[submodule "sub2"] + path = sub2 @@ -293,7 +297,10 @@ cat >diff_submodule_expect <<\EOF diff --cc .gitmodules --- a/.gitmodules +++ b/.gitmodules -@@@ -1,3 -1,3 +1,9 @@@ +@@@ -1,6 -1,6 +1,12 @@@ + [submodule "sub"] + path = sub + url = ./sub ++<<<<<<< HEAD +[submodule "sub2"] + path = sub2 -- 2.39.2.722.g9855ee24e9-goog ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 3/6] tests: use `git submodule add` and fix expected diffs 2023-02-28 18:56 ` [PATCH v2 3/6] tests: use `git submodule add` and fix expected diffs Calvin Wan @ 2023-03-06 23:34 ` Glen Choo 2023-03-06 23:57 ` Junio C Hamano 0 siblings, 1 reply; 40+ messages in thread From: Glen Choo @ 2023-03-06 23:34 UTC (permalink / raw) To: Calvin Wan, git; +Cc: Josh Steadmon, peff, gitster, Calvin Wan Calvin Wan <calvinwan@google.com> writes: > This commit continues the previous work of updating the test suite to > use `git submodule add` to create submodules instead of using `git add` > to include embedded repositories. Specifically, in this commit we update > test cases where expected diffs must change due to the presence of a > .gitmodules file. Adjusting the diff makes sense when the .gitmodules file is relevant to the diff being tested. > diff --git a/t/t3040-subprojects-basic.sh b/t/t3040-subprojects-basic.sh > index 61da7e3b94..a0f14db3d2 100755 > --- a/t/t3040-subprojects-basic.sh > +++ b/t/t3040-subprojects-basic.sh > @@ -19,11 +19,12 @@ test_expect_success 'setup: create subprojects' ' > ( cd sub2 && git init && : >Makefile && git add * && > git commit -q -m "subproject 2" ) && > git update-index --add sub1 && > - git add sub2 && > + git submodule add ./sub2 && > git commit -q -m "subprojects added" && > GIT_PRINT_SHA1_ELLIPSIS="yes" git diff-tree --abbrev=5 HEAD^ HEAD |cut -d" " -f-3,5- >current && > git branch save HEAD && > cat >expected <<-\EOF && > + :000000 100644 00000... A .gitmodules > :000000 160000 00000... A sub1 > :000000 160000 00000... A sub2 > EOF e.g. this change makes sense > diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh > index 2aa12243bd..f5074071a4 100755 > --- a/t/t4041-diff-submodule-option.sh > +++ b/t/t4041-diff-submodule-option.sh > @@ -50,9 +50,19 @@ test_expect_success 'setup' ' > ' > > test_expect_success 'added submodule' ' > - git add sm1 && > + git submodule add ./sm1 && > + gitmodules_hash1=$(git rev-parse --short $(git hash-object .gitmodules)) && > git diff-index -p --submodule=log HEAD >actual && > cat >expected <<-EOF && > + diff --git a/.gitmodules b/.gitmodules > + new file mode 100644 > + index 0000000..$gitmodules_hash1 > + --- /dev/null > + +++ b/.gitmodules > + @@ -0,0 +1,3 @@ > + +[submodule "sm1"] > + + path = sm1 > + + url = ./sm1 > Submodule sm1 0000000...$head1 (new submodule) > EOF > test_cmp expected actual But in this file and the next (t4041 and t4060), we are checking submodule diffing behavior, so wouldn't it make sense to ignore non-submodule changes in the diff? E.g. we could have ignored .gitmodules during the diff like so test_expect_success 'added submodule' ' git submodule add ./sm1 && gitmodules_hash1=$(git rev-parse --short $(git hash-object .gitmodules)) && - git diff-index -p --submodule=log HEAD >actual && + git diff-index -p --submodule=log HEAD -- :!.gitmodules >actual && and then we wouldn't have to adjust the diff. That would be my preferred approach, since it keeps the irrelevant details out of the test. To play devil's advocate, there's a small integration test-style benefit to testing both a regular file diff and a submodule diff together. I haven't checked if these are the only files that are testing this, but even if not, checking .gitmodules repeatedly seems like a suboptimal way to do this. > @@ -243,7 +244,7 @@ test_expect_success 'status -a clean (empty submodule dir)' ' > ' > > cat >status_expect <<\EOF > -AA .gitmodules > +UU .gitmodules > A sub1 > EOF _Maybe_ this is worth modernizing too as a 'while we're at it' kind of change, though it's far less important than the earlier patch (where the setup actually touches the Git repo and submodules). Idk. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 3/6] tests: use `git submodule add` and fix expected diffs 2023-03-06 23:34 ` Glen Choo @ 2023-03-06 23:57 ` Junio C Hamano 0 siblings, 0 replies; 40+ messages in thread From: Junio C Hamano @ 2023-03-06 23:57 UTC (permalink / raw) To: Glen Choo; +Cc: Calvin Wan, git, Josh Steadmon, peff Glen Choo <chooglen@google.com> writes: > E.g. we could have ignored .gitmodules during the diff like so > > test_expect_success 'added submodule' ' > git submodule add ./sm1 && > gitmodules_hash1=$(git rev-parse --short $(git hash-object .gitmodules)) && > - git diff-index -p --submodule=log HEAD >actual && > + git diff-index -p --submodule=log HEAD -- :!.gitmodules >actual && > > and then we wouldn't have to adjust the diff. That would be my preferred > approach, since it keeps the irrelevant details out of the test. Yes, it certainly does make the diff simpler to read, but the reasoning cuts both ways. The end-goal of the topic is to propose that updating .gitmodules is an integral part of "adding a submodule" and to make it impossible to just add gitlink without a command line option using "git add", so from that point of view, readers would expect that the diff after "adding a submodule" to have both new gitlink and also a modified (or if this were the first submodule, added) ".gitmodules" file. ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 4/6] tests: use `git submodule add` and fix expected status 2023-02-28 18:52 ` [PATCH v2 0/6] add: block invalid submodules Calvin Wan ` (2 preceding siblings ...) 2023-02-28 18:56 ` [PATCH v2 3/6] tests: use `git submodule add` and fix expected diffs Calvin Wan @ 2023-02-28 18:56 ` Calvin Wan 2023-03-07 0:15 ` Glen Choo 2023-02-28 18:56 ` [PATCH v2 5/6] tests: remove duplicate .gitmodules path Calvin Wan 2023-02-28 18:56 ` [PATCH v2 6/6] add: reject nested repositories Calvin Wan 5 siblings, 1 reply; 40+ messages in thread From: Calvin Wan @ 2023-02-28 18:56 UTC (permalink / raw) To: git; +Cc: Josh Steadmon, peff, gitster, Calvin Wan From: Josh Steadmon <steadmon@google.com> This commit continues the previous work of updating the test suite to use `git submodule add` to create submodules instead of using `git add` to include embedded repositories. In this commit, we update test cases where the expected status output must change due to the presence of a .gitmodules file. We use the pre-existing expected output as a template for cases where .gitmodules has been modified but not committed. Signed-off-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> --- t/t4027-diff-submodule.sh | 17 +++-- t/t7508-status.sh | 134 ++++++++++++++++++++++++++++++++------ 2 files changed, 128 insertions(+), 23 deletions(-) diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh index 40164ae07d..2ee9f18b38 100755 --- a/t/t4027-diff-submodule.sh +++ b/t/t4027-diff-submodule.sh @@ -18,7 +18,8 @@ test_expect_success setup ' test_tick && echo frotz >nitfol && - git add nitfol sub && + git add nitfol && + git submodule add ./sub && git commit -m superproject && ( @@ -122,25 +123,30 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)' ' test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.gitmodules]' ' + git branch pristine-gitmodules && git config diff.ignoreSubmodules dirty && git diff HEAD >actual && test_must_be_empty actual && git config --add -f .gitmodules submodule.subname.ignore none && git config --add -f .gitmodules submodule.subname.path sub && + git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual && sed -e "1,/^@@/d" actual >actual.body && expect_from_to >expect.body $subprev $subprev-dirty && test_cmp expect.body actual.body && git config -f .gitmodules submodule.subname.ignore all && git config -f .gitmodules submodule.subname.path sub && + git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual2 && test_must_be_empty actual2 && git config -f .gitmodules submodule.subname.ignore untracked && + git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual3 && sed -e "1,/^@@/d" actual3 >actual3.body && expect_from_to >expect.body $subprev $subprev-dirty && test_cmp expect.body actual3.body && git config -f .gitmodules submodule.subname.ignore dirty && + git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual4 && test_must_be_empty actual4 && git config submodule.subname.ignore none && @@ -152,7 +158,7 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) git config --remove-section submodule.subname && git config --remove-section -f .gitmodules submodule.subname && git config --unset diff.ignoreSubmodules && - rm .gitmodules + git reset --hard pristine-gitmodules ' test_expect_success 'git diff HEAD with dirty submodule (index, refs match)' ' @@ -190,12 +196,15 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)' test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.gitmodules]' ' git config --add -f .gitmodules submodule.subname.ignore all && git config --add -f .gitmodules submodule.subname.path sub && + git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual2 && test_must_be_empty actual2 && git config -f .gitmodules submodule.subname.ignore untracked && + git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual3 && test_must_be_empty actual3 && git config -f .gitmodules submodule.subname.ignore dirty && + git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual4 && test_must_be_empty actual4 && git config submodule.subname.ignore none && @@ -206,7 +215,7 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) test_cmp expect.body actual.body && git config --remove-section submodule.subname && git config --remove-section -f .gitmodules submodule.subname && - rm .gitmodules + git reset --hard pristine-gitmodules ' test_expect_success 'git diff between submodule commits' ' @@ -243,7 +252,7 @@ test_expect_success 'git diff between submodule commits [.gitmodules]' ' expect_from_to >expect.body $subtip $subprev && git config --remove-section submodule.subname && git config --remove-section -f .gitmodules submodule.subname && - rm .gitmodules + git reset --hard pristine-gitmodules ' test_expect_success 'git diff (empty submodule dir)' ' diff --git a/t/t7508-status.sh b/t/t7508-status.sh index 2b7ef6c41a..5808339997 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh @@ -892,7 +892,7 @@ test_expect_success 'setup status submodule summary' ' git add foo && git commit -m "Add foo" ) && - git add sm + git submodule add ./sm ' test_expect_success 'status submodule summary is disabled by default' ' @@ -904,6 +904,7 @@ and have 1 and 2 different commits each, respectively. Changes to be committed: (use "git restore --staged <file>..." to unstage) + new file: .gitmodules new file: dir2/added new file: sm @@ -931,6 +932,7 @@ test_expect_success 'status --untracked-files=all does not show submodule' ' ' cat >expect <<EOF +A .gitmodules M dir1/modified A dir2/added A sm @@ -961,6 +963,7 @@ and have 1 and 2 different commits each, respectively. Changes to be committed: (use "git restore --staged <file>..." to unstage) + new file: .gitmodules new file: dir2/added new file: sm @@ -998,6 +1001,7 @@ test_expect_success 'commit with submodule summary ignores status.displayComment ' cat >expect <<EOF +A .gitmodules M dir1/modified A dir2/added A sm @@ -1068,6 +1072,7 @@ and have 2 and 2 different commits each, respectively. Changes to be committed: (use "git restore --source=HEAD^1 --staged <file>..." to unstage) + new file: .gitmodules new file: dir2/added new file: sm @@ -1134,7 +1139,6 @@ Submodule changes to be committed: Untracked files: (use "git add <file>..." to include in what will be committed) - .gitmodules dir1/untracked dir2/modified dir2/untracked @@ -1152,8 +1156,37 @@ test_expect_success '.gitmodules ignore=untracked suppresses submodules with unt test_cmp expect output && git config --add -f .gitmodules submodule.subname.ignore untracked && git config --add -f .gitmodules submodule.subname.path sm && + cat > expect-modified-gitmodules << EOF && +On branch main +Your branch and '\''upstream'\'' have diverged, +and have 2 and 2 different commits each, respectively. + (use "git pull" to merge the remote branch into yours) + +Changes to be committed: + (use "git restore --staged <file>..." to unstage) + modified: sm + +Changes not staged for commit: + (use "git add <file>..." to update what will be committed) + (use "git restore <file>..." to discard changes in working directory) + modified: .gitmodules + modified: dir1/modified + +Submodule changes to be committed: + +* sm $head...$new_head (1): + > Add bar + +Untracked files: + (use "git add <file>..." to include in what will be committed) + dir1/untracked + dir2/modified + dir2/untracked + untracked + +EOF git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config -f .gitmodules --remove-section submodule.subname ' @@ -1163,7 +1196,7 @@ test_expect_success '.git/config ignore=untracked suppresses submodules with unt git config --add submodule.subname.ignore untracked && git config --add submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config --remove-section submodule.subname && git config --remove-section -f .gitmodules submodule.subname ' @@ -1180,7 +1213,7 @@ test_expect_success '.gitmodules ignore=dirty suppresses submodules with untrack git config --add -f .gitmodules submodule.subname.ignore dirty && git config --add -f .gitmodules submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config -f .gitmodules --remove-section submodule.subname ' @@ -1190,7 +1223,7 @@ test_expect_success '.git/config ignore=dirty suppresses submodules with untrack git config --add submodule.subname.ignore dirty && git config --add submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config --remove-section submodule.subname && git config -f .gitmodules --remove-section submodule.subname ' @@ -1205,7 +1238,7 @@ test_expect_success '.gitmodules ignore=dirty suppresses submodules with modifie git config --add -f .gitmodules submodule.subname.ignore dirty && git config --add -f .gitmodules submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config -f .gitmodules --remove-section submodule.subname ' @@ -1215,7 +1248,7 @@ test_expect_success '.git/config ignore=dirty suppresses submodules with modifie git config --add submodule.subname.ignore dirty && git config --add submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config --remove-section submodule.subname && git config -f .gitmodules --remove-section submodule.subname ' @@ -1245,7 +1278,6 @@ Submodule changes to be committed: Untracked files: (use "git add <file>..." to include in what will be committed) - .gitmodules dir1/untracked dir2/modified dir2/untracked @@ -1259,8 +1291,39 @@ EOF test_expect_success ".gitmodules ignore=untracked doesn't suppress submodules with modified content" ' git config --add -f .gitmodules submodule.subname.ignore untracked && git config --add -f .gitmodules submodule.subname.path sm && + cat > expect-modified-gitmodules << EOF && +On branch main +Your branch and '\''upstream'\'' have diverged, +and have 2 and 2 different commits each, respectively. + (use "git pull" to merge the remote branch into yours) + +Changes to be committed: + (use "git restore --staged <file>..." to unstage) + modified: sm + +Changes not staged for commit: + (use "git add <file>..." to update what will be committed) + (use "git restore <file>..." to discard changes in working directory) + (commit or discard the untracked or modified content in submodules) + modified: .gitmodules + modified: dir1/modified + modified: sm (modified content) + +Submodule changes to be committed: + +* sm $head...$new_head (1): + > Add bar + +Untracked files: + (use "git add <file>..." to include in what will be committed) + dir1/untracked + dir2/modified + dir2/untracked + untracked + +EOF git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config -f .gitmodules --remove-section submodule.subname ' @@ -1270,7 +1333,7 @@ test_expect_success ".git/config ignore=untracked doesn't suppress submodules wi git config --add submodule.subname.ignore untracked && git config --add submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config --remove-section submodule.subname && git config -f .gitmodules --remove-section submodule.subname ' @@ -1306,7 +1369,6 @@ Submodules changed but not updated: Untracked files: (use "git add <file>..." to include in what will be committed) - .gitmodules dir1/untracked dir2/modified dir2/untracked @@ -1318,10 +1380,45 @@ EOF ' test_expect_success ".gitmodules ignore=untracked doesn't suppress submodule summary" ' + cat > expect-modified-gitmodules << EOF && +On branch main +Your branch and '\''upstream'\'' have diverged, +and have 2 and 2 different commits each, respectively. + (use "git pull" to merge the remote branch into yours) + +Changes to be committed: + (use "git restore --staged <file>..." to unstage) + modified: sm + +Changes not staged for commit: + (use "git add <file>..." to update what will be committed) + (use "git restore <file>..." to discard changes in working directory) + modified: .gitmodules + modified: dir1/modified + modified: sm (new commits) + +Submodule changes to be committed: + +* sm $head...$new_head (1): + > Add bar + +Submodules changed but not updated: + +* sm $new_head...$head2 (1): + > 2nd commit + +Untracked files: + (use "git add <file>..." to include in what will be committed) + dir1/untracked + dir2/modified + dir2/untracked + untracked + +EOF git config --add -f .gitmodules submodule.subname.ignore untracked && git config --add -f .gitmodules submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config -f .gitmodules --remove-section submodule.subname ' @@ -1331,7 +1428,7 @@ test_expect_success ".git/config ignore=untracked doesn't suppress submodule sum git config --add submodule.subname.ignore untracked && git config --add submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config --remove-section submodule.subname && git config -f .gitmodules --remove-section submodule.subname ' @@ -1344,7 +1441,7 @@ test_expect_success ".gitmodules ignore=dirty doesn't suppress submodule summary git config --add -f .gitmodules submodule.subname.ignore dirty && git config --add -f .gitmodules submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config -f .gitmodules --remove-section submodule.subname ' @@ -1354,7 +1451,7 @@ test_expect_success ".git/config ignore=dirty doesn't suppress submodule summary git config --add submodule.subname.ignore dirty && git config --add submodule.subname.path sm && git status >output && - test_cmp expect output && + test_cmp expect-modified-gitmodules output && git config --remove-section submodule.subname && git config -f .gitmodules --remove-section submodule.subname ' @@ -1387,7 +1484,6 @@ cat > expect << EOF ; ; Untracked files: ; (use "git add <file>..." to include in what will be committed) -; .gitmodules ; dir1/untracked ; dir2/modified ; dir2/untracked @@ -1420,7 +1516,6 @@ Changes not staged for commit: Untracked files: (use "git add <file>..." to include in what will be committed) - .gitmodules dir1/untracked dir2/modified dir2/untracked @@ -1446,11 +1541,11 @@ Changes to be committed: Changes not staged for commit: (use "git add <file>..." to update what will be committed) (use "git restore <file>..." to discard changes in working directory) + modified: .gitmodules modified: dir1/modified Untracked files: (use "git add <file>..." to include in what will be committed) - .gitmodules dir1/untracked dir2/modified dir2/untracked @@ -1566,6 +1661,7 @@ Changes to be committed: Changes not staged for commit: (use "git add <file>..." to update what will be committed) (use "git restore <file>..." to discard changes in working directory) + modified: .gitmodules modified: dir1/modified Untracked files not listed (use -u option to show untracked files) -- 2.39.2.722.g9855ee24e9-goog ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 4/6] tests: use `git submodule add` and fix expected status 2023-02-28 18:56 ` [PATCH v2 4/6] tests: use `git submodule add` and fix expected status Calvin Wan @ 2023-03-07 0:15 ` Glen Choo 0 siblings, 0 replies; 40+ messages in thread From: Glen Choo @ 2023-03-07 0:15 UTC (permalink / raw) To: Calvin Wan, git; +Cc: Josh Steadmon, peff, gitster, Calvin Wan Calvin Wan <calvinwan@google.com> writes: > @@ -122,25 +123,30 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match)' > ' > > test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) [.gitmodules]' ' > + git branch pristine-gitmodules && > git config diff.ignoreSubmodules dirty && > git diff HEAD >actual && > test_must_be_empty actual && > git config --add -f .gitmodules submodule.subname.ignore none && > git config --add -f .gitmodules submodule.subname.path sub && > + git commit -m "Update .gitmodules" .gitmodules && > git diff HEAD >actual && > sed -e "1,/^@@/d" actual >actual.body && > expect_from_to >expect.body $subprev $subprev-dirty && > test_cmp expect.body actual.body && > git config -f .gitmodules submodule.subname.ignore all && > git config -f .gitmodules submodule.subname.path sub && > + git commit -m "Update .gitmodules" .gitmodules && > git diff HEAD >actual2 && > test_must_be_empty actual2 && > git config -f .gitmodules submodule.subname.ignore untracked && > + git commit -m "Update .gitmodules" .gitmodules && > git diff HEAD >actual3 && > sed -e "1,/^@@/d" actual3 >actual3.body && > expect_from_to >expect.body $subprev $subprev-dirty && > test_cmp expect.body actual3.body && > git config -f .gitmodules submodule.subname.ignore dirty && > + git commit -m "Update .gitmodules" .gitmodules && > git diff HEAD >actual4 && > test_must_be_empty actual4 && > git config submodule.subname.ignore none && > @@ -152,7 +158,7 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) > git config --remove-section submodule.subname && > git config --remove-section -f .gitmodules submodule.subname && > git config --unset diff.ignoreSubmodules && > - rm .gitmodules > + git reset --hard pristine-gitmodules > ' This looks like the perfect use case for test_when_finished :) > @@ -190,12 +196,15 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)' > test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.gitmodules]' ' > git config --add -f .gitmodules submodule.subname.ignore all && > git config --add -f .gitmodules submodule.subname.path sub && > + git commit -m "Update .gitmodules" .gitmodules && > git diff HEAD >actual2 && > test_must_be_empty actual2 && > git config -f .gitmodules submodule.subname.ignore untracked && > + git commit -m "Update .gitmodules" .gitmodules && > git diff HEAD >actual3 && > test_must_be_empty actual3 && > git config -f .gitmodules submodule.subname.ignore dirty && > + git commit -m "Update .gitmodules" .gitmodules && > git diff HEAD >actual4 && > test_must_be_empty actual4 && > git config submodule.subname.ignore none && Like the previous patch, I wonder a little whether we should be diffing with :!.gitmodules, but at least here we are focused on diffing with submodules in general (and not the specific "git diff --submodule=" behavior), so I thnk this is okay to keep. > @@ -206,7 +215,7 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) > test_cmp expect.body actual.body && > git config --remove-section submodule.subname && > git config --remove-section -f .gitmodules submodule.subname && > - rm .gitmodules > + git reset --hard pristine-gitmodules Ditto about test_when_finished. > ' > > test_expect_success 'git diff between submodule commits' ' > @@ -243,7 +252,7 @@ test_expect_success 'git diff between submodule commits [.gitmodules]' ' > expect_from_to >expect.body $subtip $subprev && > git config --remove-section submodule.subname && > git config --remove-section -f .gitmodules submodule.subname && > - rm .gitmodules > + git reset --hard pristine-gitmodules > ' > Ditto > @@ -1152,8 +1156,37 @@ test_expect_success '.gitmodules ignore=untracked suppresses submodules with unt > test_cmp expect output && > git config --add -f .gitmodules submodule.subname.ignore untracked && > git config --add -f .gitmodules submodule.subname.path sm && > + cat > expect-modified-gitmodules << EOF && > +On branch main > +Your branch and '\''upstream'\'' have diverged, > +and have 2 and 2 different commits each, respectively. > + (use "git pull" to merge the remote branch into yours) > + > +Changes to be committed: > + (use "git restore --staged <file>..." to unstage) > + modified: sm > + > +Changes not staged for commit: > + (use "git add <file>..." to update what will be committed) > + (use "git restore <file>..." to discard changes in working directory) > + modified: .gitmodules > + modified: dir1/modified > + > +Submodule changes to be committed: > + > +* sm $head...$new_head (1): > + > Add bar > + > +Untracked files: > + (use "git add <file>..." to include in what will be committed) > + dir1/untracked > + dir2/modified > + dir2/untracked > + untracked > + > +EOF > git status >output && > - test_cmp expect output && > + test_cmp expect-modified-gitmodules output && > git config -f .gitmodules --remove-section submodule.subname > ' That another giant snapshot makes me a bit wary, since it's harder to tell whether the "modifed .gitmodules" and "unmodified .gitmodules" are really checking the same things, but there might not be a way around it. The following tests check various combinations of values (dirty, untracked, etc) and sources "--ignore-submodules", ".git/config ignore=" and ".gitmodules ignore=". For the .gitmodules tests, we really do have to modify .gitmodules to test that it gives us the behavior we want. As a hack, we could preemptively modify .gitmodules, so that it's modified in all of the snapshots we're diffing. That feels too hacky to me, but maybe others think it's fine. (Side note: I recall a previous conversation with Junio about how we shouldn't be changing behavior based on .gitmodules. If we had that, we wouldn't need to worry about this right now.) ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 5/6] tests: remove duplicate .gitmodules path 2023-02-28 18:52 ` [PATCH v2 0/6] add: block invalid submodules Calvin Wan ` (3 preceding siblings ...) 2023-02-28 18:56 ` [PATCH v2 4/6] tests: use `git submodule add` and fix expected status Calvin Wan @ 2023-02-28 18:56 ` Calvin Wan 2023-02-28 23:35 ` Junio C Hamano 2023-03-07 0:51 ` Glen Choo 2023-02-28 18:56 ` [PATCH v2 6/6] add: reject nested repositories Calvin Wan 5 siblings, 2 replies; 40+ messages in thread From: Calvin Wan @ 2023-02-28 18:56 UTC (permalink / raw) To: git; +Cc: Calvin Wan, steadmon, peff, gitster Swapping `git add <submodule>` to `git submodule add <submodule>` in a previous patch created a .gitmodules file with multiple submodules pointing to the same path in certain tests. Fix tests so that they are run on the original added submodule rather than a separate manually configured submodule. Signed-off-by: Calvin Wan <calvinwan@google.com> --- t/t4027-diff-submodule.sh | 41 ++++++-------- t/t7508-status.sh | 114 +++++++++++++++----------------------- 2 files changed, 62 insertions(+), 93 deletions(-) diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh index 2ee9f18b38..ce335534b9 100755 --- a/t/t4027-diff-submodule.sh +++ b/t/t4027-diff-submodule.sh @@ -127,36 +127,33 @@ test_expect_success 'git diff HEAD with dirty submodule (work tree, refs match) git config diff.ignoreSubmodules dirty && git diff HEAD >actual && test_must_be_empty actual && - git config --add -f .gitmodules submodule.subname.ignore none && - git config --add -f .gitmodules submodule.subname.path sub && + git config --add -f .gitmodules submodule.sub.ignore none && git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual && sed -e "1,/^@@/d" actual >actual.body && expect_from_to >expect.body $subprev $subprev-dirty && test_cmp expect.body actual.body && - git config -f .gitmodules submodule.subname.ignore all && - git config -f .gitmodules submodule.subname.path sub && + git config -f .gitmodules submodule.sub.ignore all && git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual2 && test_must_be_empty actual2 && - git config -f .gitmodules submodule.subname.ignore untracked && + git config -f .gitmodules submodule.sub.ignore untracked && git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual3 && sed -e "1,/^@@/d" actual3 >actual3.body && expect_from_to >expect.body $subprev $subprev-dirty && test_cmp expect.body actual3.body && - git config -f .gitmodules submodule.subname.ignore dirty && + git config -f .gitmodules submodule.sub.ignore dirty && git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual4 && test_must_be_empty actual4 && - git config submodule.subname.ignore none && - git config submodule.subname.path sub && + git config submodule.sub.ignore none && + git config submodule.sub.path sub && git diff HEAD >actual && sed -e "1,/^@@/d" actual >actual.body && expect_from_to >expect.body $subprev $subprev-dirty && test_cmp expect.body actual.body && - git config --remove-section submodule.subname && - git config --remove-section -f .gitmodules submodule.subname && + git config --unset submodule.sub.ignore && git config --unset diff.ignoreSubmodules && git reset --hard pristine-gitmodules ' @@ -194,27 +191,24 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)' ' test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.gitmodules]' ' - git config --add -f .gitmodules submodule.subname.ignore all && - git config --add -f .gitmodules submodule.subname.path sub && + git config --add -f .gitmodules submodule.sub.ignore all && git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual2 && test_must_be_empty actual2 && - git config -f .gitmodules submodule.subname.ignore untracked && + git config -f .gitmodules submodule.sub.ignore untracked && git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual3 && test_must_be_empty actual3 && - git config -f .gitmodules submodule.subname.ignore dirty && + git config -f .gitmodules submodule.sub.ignore dirty && git commit -m "Update .gitmodules" .gitmodules && git diff HEAD >actual4 && test_must_be_empty actual4 && - git config submodule.subname.ignore none && - git config submodule.subname.path sub && + git config submodule.sub.ignore none && git diff HEAD >actual && sed -e "1,/^@@/d" actual >actual.body && expect_from_to >expect.body $subprev $subprev-dirty && test_cmp expect.body actual.body && - git config --remove-section submodule.subname && - git config --remove-section -f .gitmodules submodule.subname && + git config --unset submodule.sub.ignore && git reset --hard pristine-gitmodules ' @@ -236,22 +230,19 @@ test_expect_success 'git diff between submodule commits [.gitmodules]' ' sed -e "1,/^@@/d" actual >actual.body && expect_from_to >expect.body $subtip $subprev && test_cmp expect.body actual.body && - git config --add -f .gitmodules submodule.subname.ignore dirty && - git config --add -f .gitmodules submodule.subname.path sub && + git config --add -f .gitmodules submodule.sub.ignore dirty && git diff HEAD^..HEAD >actual && sed -e "1,/^@@/d" actual >actual.body && expect_from_to >expect.body $subtip $subprev && test_cmp expect.body actual.body && - git config -f .gitmodules submodule.subname.ignore all && + git config -f .gitmodules submodule.sub.ignore all && git diff HEAD^..HEAD >actual && test_must_be_empty actual && - git config submodule.subname.ignore dirty && - git config submodule.subname.path sub && + git config submodule.sub.ignore dirty && git diff HEAD^..HEAD >actual && sed -e "1,/^@@/d" actual >actual.body && expect_from_to >expect.body $subtip $subprev && - git config --remove-section submodule.subname && - git config --remove-section -f .gitmodules submodule.subname && + git config --unset submodule.sub.ignore && git reset --hard pristine-gitmodules ' diff --git a/t/t7508-status.sh b/t/t7508-status.sh index 5808339997..3d934bfb86 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh @@ -1154,8 +1154,7 @@ test_expect_success '.gitmodules ignore=untracked suppresses submodules with unt test_config diff.ignoreSubmodules dirty && git status >output && test_cmp expect output && - git config --add -f .gitmodules submodule.subname.ignore untracked && - git config --add -f .gitmodules submodule.subname.path sm && + git config --add -f .gitmodules submodule.sm.ignore untracked && cat > expect-modified-gitmodules << EOF && On branch main Your branch and '\''upstream'\'' have diverged, @@ -1187,18 +1186,16 @@ Untracked files: EOF git status >output && test_cmp expect-modified-gitmodules output && - git config -f .gitmodules --remove-section submodule.subname + git config -f .gitmodules --unset submodule.sm.ignore ' test_expect_success '.git/config ignore=untracked suppresses submodules with untracked content' ' - git config --add -f .gitmodules submodule.subname.ignore none && - git config --add -f .gitmodules submodule.subname.path sm && - git config --add submodule.subname.ignore untracked && - git config --add submodule.subname.path sm && + git config --add -f .gitmodules submodule.sm.ignore none && + git config --add submodule.sm.ignore untracked && git status >output && test_cmp expect-modified-gitmodules output && - git config --remove-section submodule.subname && - git config --remove-section -f .gitmodules submodule.subname + git config --unset submodule.sm.ignore && + git config -f .gitmodules --unset submodule.sm.ignore ' test_expect_success '--ignore-submodules=dirty suppresses submodules with untracked content' ' @@ -1210,22 +1207,19 @@ test_expect_success '.gitmodules ignore=dirty suppresses submodules with untrack test_config diff.ignoreSubmodules dirty && git status >output && ! test -s actual && - git config --add -f .gitmodules submodule.subname.ignore dirty && - git config --add -f .gitmodules submodule.subname.path sm && + git config --add -f .gitmodules submodule.sm.ignore dirty && git status >output && test_cmp expect-modified-gitmodules output && - git config -f .gitmodules --remove-section submodule.subname + git config -f .gitmodules --unset submodule.sm.ignore ' test_expect_success '.git/config ignore=dirty suppresses submodules with untracked content' ' - git config --add -f .gitmodules submodule.subname.ignore none && - git config --add -f .gitmodules submodule.subname.path sm && - git config --add submodule.subname.ignore dirty && - git config --add submodule.subname.path sm && + git config --add -f .gitmodules submodule.sm.ignore none && + git config --add submodule.sm.ignore dirty && git status >output && test_cmp expect-modified-gitmodules output && - git config --remove-section submodule.subname && - git config -f .gitmodules --remove-section submodule.subname + git config --unset submodule.sm.ignore && + git config -f .gitmodules --unset submodule.sm.ignore ' test_expect_success '--ignore-submodules=dirty suppresses submodules with modified content' ' @@ -1235,22 +1229,19 @@ test_expect_success '--ignore-submodules=dirty suppresses submodules with modifi ' test_expect_success '.gitmodules ignore=dirty suppresses submodules with modified content' ' - git config --add -f .gitmodules submodule.subname.ignore dirty && - git config --add -f .gitmodules submodule.subname.path sm && + git config --add -f .gitmodules submodule.sm.ignore dirty && git status >output && test_cmp expect-modified-gitmodules output && - git config -f .gitmodules --remove-section submodule.subname + git config -f .gitmodules --unset submodule.sm.ignore ' test_expect_success '.git/config ignore=dirty suppresses submodules with modified content' ' - git config --add -f .gitmodules submodule.subname.ignore none && - git config --add -f .gitmodules submodule.subname.path sm && - git config --add submodule.subname.ignore dirty && - git config --add submodule.subname.path sm && + git config --add -f .gitmodules submodule.sm.ignore none && + git config --add submodule.sm.ignore dirty && git status >output && test_cmp expect-modified-gitmodules output && - git config --remove-section submodule.subname && - git config -f .gitmodules --remove-section submodule.subname + git config --unset submodule.sm.ignore && + git config -f .gitmodules --unset submodule.sm.ignore ' test_expect_success "--ignore-submodules=untracked doesn't suppress submodules with modified content" ' @@ -1289,8 +1280,7 @@ EOF ' test_expect_success ".gitmodules ignore=untracked doesn't suppress submodules with modified content" ' - git config --add -f .gitmodules submodule.subname.ignore untracked && - git config --add -f .gitmodules submodule.subname.path sm && + git config --add -f .gitmodules submodule.sm.ignore untracked && cat > expect-modified-gitmodules << EOF && On branch main Your branch and '\''upstream'\'' have diverged, @@ -1324,18 +1314,16 @@ Untracked files: EOF git status >output && test_cmp expect-modified-gitmodules output && - git config -f .gitmodules --remove-section submodule.subname + git config -f .gitmodules --unset submodule.sm.ignore ' test_expect_success ".git/config ignore=untracked doesn't suppress submodules with modified content" ' - git config --add -f .gitmodules submodule.subname.ignore none && - git config --add -f .gitmodules submodule.subname.path sm && - git config --add submodule.subname.ignore untracked && - git config --add submodule.subname.path sm && + git config --add -f .gitmodules submodule.sm.ignore none && + git config --add submodule.sm.ignore untracked && git status >output && test_cmp expect-modified-gitmodules output && - git config --remove-section submodule.subname && - git config -f .gitmodules --remove-section submodule.subname + git config --unset submodule.sm.ignore && + git config -f .gitmodules --unset submodule.sm.ignore ' head2=$(cd sm && git commit -q -m "2nd commit" foo && git rev-parse --short=7 --verify HEAD) @@ -1415,22 +1403,19 @@ Untracked files: untracked EOF - git config --add -f .gitmodules submodule.subname.ignore untracked && - git config --add -f .gitmodules submodule.subname.path sm && + git config --add -f .gitmodules submodule.sm.ignore untracked && git status >output && test_cmp expect-modified-gitmodules output && - git config -f .gitmodules --remove-section submodule.subname + git config -f .gitmodules --unset submodule.sm.ignore ' test_expect_success ".git/config ignore=untracked doesn't suppress submodule summary" ' - git config --add -f .gitmodules submodule.subname.ignore none && - git config --add -f .gitmodules submodule.subname.path sm && - git config --add submodule.subname.ignore untracked && - git config --add submodule.subname.path sm && + git config --add -f .gitmodules submodule.sm.ignore none && + git config --add submodule.sm.ignore untracked && git status >output && test_cmp expect-modified-gitmodules output && - git config --remove-section submodule.subname && - git config -f .gitmodules --remove-section submodule.subname + git config --unset submodule.sm.ignore && + git config -f .gitmodules --unset submodule.sm.ignore ' test_expect_success "--ignore-submodules=dirty doesn't suppress submodule summary" ' @@ -1438,22 +1423,19 @@ test_expect_success "--ignore-submodules=dirty doesn't suppress submodule summar test_cmp expect output ' test_expect_success ".gitmodules ignore=dirty doesn't suppress submodule summary" ' - git config --add -f .gitmodules submodule.subname.ignore dirty && - git config --add -f .gitmodules submodule.subname.path sm && + git config --add -f .gitmodules submodule.sm.ignore dirty && git status >output && test_cmp expect-modified-gitmodules output && - git config -f .gitmodules --remove-section submodule.subname + git config -f .gitmodules --unset submodule.sm.ignore ' test_expect_success ".git/config ignore=dirty doesn't suppress submodule summary" ' - git config --add -f .gitmodules submodule.subname.ignore none && - git config --add -f .gitmodules submodule.subname.path sm && - git config --add submodule.subname.ignore dirty && - git config --add submodule.subname.path sm && + git config --add -f .gitmodules submodule.sm.ignore none && + git config --add submodule.sm.ignore dirty && git status >output && test_cmp expect-modified-gitmodules output && - git config --remove-section submodule.subname && - git config -f .gitmodules --remove-section submodule.subname + git config --unset submodule.sm.ignore && + git config -f .gitmodules --unset submodule.sm.ignore ' cat > expect << EOF @@ -1552,22 +1534,19 @@ Untracked files: untracked EOF - git config --add -f .gitmodules submodule.subname.ignore all && - git config --add -f .gitmodules submodule.subname.path sm && + git config --add -f .gitmodules submodule.sm.ignore all && git status > output && test_cmp expect output && - git config -f .gitmodules --remove-section submodule.subname + git config -f .gitmodules --unset submodule.sm.ignore ' test_expect_success '.git/config ignore=all suppresses unstaged submodule summary' ' - git config --add -f .gitmodules submodule.subname.ignore none && - git config --add -f .gitmodules submodule.subname.path sm && - git config --add submodule.subname.ignore all && - git config --add submodule.subname.path sm && + git config --add -f .gitmodules submodule.sm.ignore none && + git config --add submodule.sm.ignore all && git status > output && test_cmp expect output && - git config --remove-section submodule.subname && - git config -f .gitmodules --remove-section submodule.subname + git config --unset submodule.sm.ignore && + git config -f .gitmodules --unset submodule.sm.ignore ' test_expect_success 'setup of test environment' ' @@ -1633,9 +1612,8 @@ test_expect_success 'Restore default test environment' ' ' test_expect_success 'git commit will commit a staged but ignored submodule' ' - git config --add -f .gitmodules submodule.subname.ignore all && - git config --add -f .gitmodules submodule.subname.path sm && - git config --add submodule.subname.ignore all && + git config --add -f .gitmodules submodule.sm.ignore all && + git config --add submodule.sm.ignore all && git status -s --ignore-submodules=dirty >output && test_i18ngrep "^M. sm" output && GIT_EDITOR="echo hello >>\"\$1\"" && @@ -1676,8 +1654,8 @@ test_expect_success 'git commit -m will commit a staged but ignored submodule' ' git commit -uno -m message && git status -s --ignore-submodules=dirty >output && test_i18ngrep ! "^M. sm" output && - git config --remove-section submodule.subname && - git config -f .gitmodules --remove-section submodule.subname + git config --unset submodule.sm.ignore && + git config -f .gitmodules --unset submodule.sm.ignore ' test_expect_success 'show stash info with "--show-stash"' ' -- 2.39.2.722.g9855ee24e9-goog ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 5/6] tests: remove duplicate .gitmodules path 2023-02-28 18:56 ` [PATCH v2 5/6] tests: remove duplicate .gitmodules path Calvin Wan @ 2023-02-28 23:35 ` Junio C Hamano 2023-03-02 23:09 ` Calvin Wan 2023-03-07 0:51 ` Glen Choo 1 sibling, 1 reply; 40+ messages in thread From: Junio C Hamano @ 2023-02-28 23:35 UTC (permalink / raw) To: Calvin Wan; +Cc: git, steadmon, peff Calvin Wan <calvinwan@google.com> writes: > Swapping `git add <submodule>` to `git submodule add <submodule>` > in a previous patch created a .gitmodules file with multiple > submodules pointing to the same path in certain tests. Fix tests > so that they are run on the original added submodule rather than > a separate manually configured submodule. Doesn't "git submodule add" have a way to give a specific name other than the default taken from the path? If "git add sub" is converted to "git submodule add --name subname ./sub", wouldn't these changes become unnecessary? ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 5/6] tests: remove duplicate .gitmodules path 2023-02-28 23:35 ` Junio C Hamano @ 2023-03-02 23:09 ` Calvin Wan 0 siblings, 0 replies; 40+ messages in thread From: Calvin Wan @ 2023-03-02 23:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, steadmon, peff On Tue, Feb 28, 2023 at 3:35 PM Junio C Hamano <gitster@pobox.com> wrote: > > Calvin Wan <calvinwan@google.com> writes: > > > Swapping `git add <submodule>` to `git submodule add <submodule>` > > in a previous patch created a .gitmodules file with multiple > > submodules pointing to the same path in certain tests. Fix tests > > so that they are run on the original added submodule rather than > > a separate manually configured submodule. > > Doesn't "git submodule add" have a way to give a specific name other > than the default taken from the path? If "git add sub" is converted > to "git submodule add --name subname ./sub", wouldn't these changes > become unnecessary? > If we converted to "git submodule add --name subname ./sub", we would instead have a different set of problems. For example, instances of git config --add -f .gitmodules submodule.subname.path sub git config -f .gitmodules submodule.subname.path sub and other similar lines would still need to be removed to prevent duplicate paths. That, however, seems like a better alternative than my current patch which replaces those removals with different ones. ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH v2 5/6] tests: remove duplicate .gitmodules path 2023-02-28 18:56 ` [PATCH v2 5/6] tests: remove duplicate .gitmodules path Calvin Wan 2023-02-28 23:35 ` Junio C Hamano @ 2023-03-07 0:51 ` Glen Choo 1 sibling, 0 replies; 40+ messages in thread From: Glen Choo @ 2023-03-07 0:51 UTC (permalink / raw) To: Calvin Wan, git; +Cc: Calvin Wan, steadmon, peff, gitster Calvin Wan <calvinwan@google.com> writes: > Swapping `git add <submodule>` to `git submodule add <submodule>` > in a previous patch created a .gitmodules file with multiple > submodules pointing to the same path in certain tests. Fix tests > so that they are run on the original added submodule rather than > a separate manually configured submodule. Without reading the diff, I thought that having multiple configured submodules was a bug that needed to be fixed (I also wasn't sure which previous patch was being referenced). But after reading the diff, this seems much more like a test simplification. I think this is might be better squashed with the previous patch (the combined diff of both patches looks ok to me) with some extra clarification, e.g.: In some of the tests, we modify .gitmodules to check whether the value of "submodule.subname.ignore" is respected. When we were using "git add", we also had to temporarily turn the gitlink into a submodule by setting "submodule.subname.path", but since "git submodule add" handles that for us, let's use the real submodule instead. > @@ -194,27 +191,24 @@ test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match)' > ' > > test_expect_success 'git diff HEAD with dirty submodule (untracked, refs match) [.gitmodules]' ' > - git config --add -f .gitmodules submodule.subname.ignore all && > - git config --add -f .gitmodules submodule.subname.path sub && > + git config --add -f .gitmodules submodule.sub.ignore all && > git commit -m "Update .gitmodules" .gitmodules && > git diff HEAD >actual2 && > test_must_be_empty actual2 && > - git config -f .gitmodules submodule.subname.ignore untracked && > + git config -f .gitmodules submodule.sub.ignore untracked && > git commit -m "Update .gitmodules" .gitmodules && > git diff HEAD >actual3 && > test_must_be_empty actual3 && > - git config -f .gitmodules submodule.subname.ignore dirty && > + git config -f .gitmodules submodule.sub.ignore dirty && > git commit -m "Update .gitmodules" .gitmodules && > git diff HEAD >actual4 && > test_must_be_empty actual4 && > - git config submodule.subname.ignore none && > - git config submodule.subname.path sub && > + git config submodule.sub.ignore none && > git diff HEAD >actual && > sed -e "1,/^@@/d" actual >actual.body && > expect_from_to >expect.body $subprev $subprev-dirty && > test_cmp expect.body actual.body && > - git config --remove-section submodule.subname && > - git config --remove-section -f .gitmodules submodule.subname && > + git config --unset submodule.sub.ignore && > git reset --hard pristine-gitmodules > ' Not the fault of this patch, but I think that this diff would have been easier to read if the latter part were moved into "test_when_finished", and it would be clear that we are just undoing what we are setting up a few lines earlier (instead of a whole test block earlier). ^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH v2 6/6] add: reject nested repositories 2023-02-28 18:52 ` [PATCH v2 0/6] add: block invalid submodules Calvin Wan ` (4 preceding siblings ...) 2023-02-28 18:56 ` [PATCH v2 5/6] tests: remove duplicate .gitmodules path Calvin Wan @ 2023-02-28 18:56 ` Calvin Wan 2023-03-07 2:04 ` Glen Choo 5 siblings, 1 reply; 40+ messages in thread From: Calvin Wan @ 2023-02-28 18:56 UTC (permalink / raw) To: git; +Cc: Josh Steadmon, peff, gitster, Calvin Wan From: Josh Steadmon <steadmon@google.com> As noted in 532139940c (add: warn when adding an embedded repository, 2017-06-14), adding embedded repositories results in subpar experience compared to submodules, due to the lack of a corresponding .gitmodules entry, which means later clones of the top-level repository cannot locate the embedded repo. We expect that this situation is usually unintentional, which is why 532139940c added a warning message and advice when users attempt to add an embedded repo. At $dayjob, we have found that even this advice is insufficient to stop users from committing unclonable embedded repos in shared projects. This causes toil for the owners of the top-level project repository as they must clean up the resulting gitlinks. Additionally, these mistakes are often made by partners outside of $dayjob, which means that a simple organization-wide change to the default Git config would be insufficient to prevent these mistakes. Due to this experience, we believe that Git's default behavior should be changed to disallow adding embedded repositories. This commit changes the existing warning into a fatal error, rewrites the advice given, and deprecates `--no-warn-embedded-repo` in favor of `--allow-embedded-repo` to bypass the fatal error. Signed-off-by: Josh Steadmon <steadmon@google.com> Signed-off-by: Calvin Wan <calvinwan@google.com> --- Documentation/git-add.txt | 15 +++++++---- builtin/add.c | 37 +++++++++++++++++--------- builtin/submodule--helper.c | 4 +-- t/t0008-ignores.sh | 2 +- t/t2103-update-index-ignore-missing.sh | 2 +- t/t4035-diff-quiet.sh | 2 +- t/t6430-merge-recursive.sh | 2 +- t/t7400-submodule-basic.sh | 4 +-- t/t7412-submodule-absorbgitdirs.sh | 2 +- t/t7414-submodule-mistakes.sh | 29 +++++++++++--------- t/t7450-bad-git-dotfiles.sh | 2 +- 11 files changed, 61 insertions(+), 40 deletions(-) diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index a030d33c6e..b90e305110 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -176,12 +176,17 @@ for "git add --no-all <pathspec>...", i.e. ignored removed files. be ignored, no matter if they are already present in the work tree or not. ---no-warn-embedded-repo:: - By default, `git add` will warn when adding an embedded +--allow-embedded-repo:: + By default, `git add` will error out when adding an embedded repository to the index without using `git submodule add` to - create an entry in `.gitmodules`. This option will suppress the - warning (e.g., if you are manually performing operations on - submodules). + create an entry in `.gitmodules`. This option will allow the + embedded repository to be added. (e.g., if you are manually + performing operations on submodules). + +--no-warn-embedded-repo:: + This option is deprecated in favor of '--add-embedded-repo'. + Passing this option still suppresses advice but does not bypass + the error. --renormalize:: Apply the "clean" process freshly to all tracked files to diff --git a/builtin/add.c b/builtin/add.c index 76277df326..c1590129bb 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -354,6 +354,7 @@ N_("The following paths are ignored by one of your .gitignore files:\n"); static int verbose, show_only, ignored_too, refresh_only; static int ignore_add_errors, intent_to_add, ignore_missing; static int warn_on_embedded_repo = 1; +static int allow_embedded_repo = 0; #define ADDREMOVE_DEFAULT 1 static int addremove = ADDREMOVE_DEFAULT; @@ -392,6 +393,8 @@ static struct option builtin_add_options[] = { N_("override the executable bit of the listed files")), OPT_HIDDEN_BOOL(0, "warn-embedded-repo", &warn_on_embedded_repo, N_("warn when adding an embedded repository")), + OPT_HIDDEN_BOOL(0, "allow-embedded-repo", &allow_embedded_repo, + N_("allow adding an embedded repository")), OPT_PATHSPEC_FROM_FILE(&pathspec_from_file), OPT_PATHSPEC_FILE_NUL(&pathspec_file_nul), OPT_END(), @@ -409,48 +412,53 @@ static int add_config(const char *var, const char *value, void *cb) } static const char embedded_advice[] = N_( -"You've added another git repository inside your current repository.\n" +"You attempted to add another git repository inside your current repository.\n" "Clones of the outer repository will not contain the contents of\n" "the embedded repository and will not know how to obtain it.\n" "If you meant to add a submodule, use:\n" "\n" " git submodule add <url> %s\n" "\n" -"If you added this path by mistake, you can remove it from the\n" -"index with:\n" +"See \"git help submodule\" for more information.\n" "\n" -" git rm --cached %s\n" +"If you cannot use submodules, you may bypass this check with:\n" "\n" -"See \"git help submodule\" for more information." +" git add --allow-embedded-repo %s\n" ); -static void check_embedded_repo(const char *path) +static int check_embedded_repo(const char *path) { + int ret = 0; struct strbuf name = STRBUF_INIT; static int adviced_on_embedded_repo = 0; - if (!warn_on_embedded_repo) - return; + if (allow_embedded_repo) + goto cleanup; if (!ends_with(path, "/")) - return; + goto cleanup; + + ret = 1; /* Drop trailing slash for aesthetics */ strbuf_addstr(&name, path); strbuf_strip_suffix(&name, "/"); - warning(_("adding embedded git repository: %s"), name.buf); + error(_("cannot add embedded git repository: %s"), name.buf); if (!adviced_on_embedded_repo && - advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) { + warn_on_embedded_repo && + advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) { advise(embedded_advice, name.buf, name.buf); adviced_on_embedded_repo = 1; } +cleanup: strbuf_release(&name); + return ret; } static int add_files(struct dir_struct *dir, int flags) { - int i, exit_status = 0; + int i, exit_status = 0, embedded_repo = 0; struct string_list matched_sparse_paths = STRING_LIST_INIT_NODUP; if (dir->ignored_nr) { @@ -476,10 +484,13 @@ static int add_files(struct dir_struct *dir, int flags) die(_("adding files failed")); exit_status = 1; } else { - check_embedded_repo(dir->entries[i]->name); + embedded_repo |= check_embedded_repo(dir->entries[i]->name); } } + if (embedded_repo) + die(_("refusing to add embedded git repositories")); + if (matched_sparse_paths.nr) { advise_on_updating_sparse_paths(&matched_sparse_paths); exit_status = 1; diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 05f2c9bc98..534ed22c9e 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -3129,7 +3129,7 @@ static void configure_added_submodule(struct add_data *add_data) add_submod.git_cmd = 1; strvec_pushl(&add_submod.args, "add", - "--no-warn-embedded-repo", NULL); + "--allow-embedded-repo", NULL); if (add_data->force) strvec_push(&add_submod.args, "--force"); strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL); @@ -3318,7 +3318,7 @@ static int module_add(int argc, const char **argv, const char *prefix) cp.git_cmd = 1; cp.no_stdout = 1; strvec_pushl(&cp.args, "add", "--dry-run", "--ignore-missing", - "--no-warn-embedded-repo", add_data.sm_path, NULL); + "--allow-embedded-repo", add_data.sm_path, NULL); if ((ret = pipe_command(&cp, NULL, 0, NULL, 0, &sb, 0))) { strbuf_complete_line(&sb); fputs(sb.buf, stderr); diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh index c70d11bc91..a53dac5931 100755 --- a/t/t0008-ignores.sh +++ b/t/t0008-ignores.sh @@ -191,7 +191,7 @@ test_expect_success 'setup' ' git add a && git commit -m"commit in submodule" ) && - git add a/submodule && + git add --allow-embedded-repo a/submodule && cat <<-\EOF >.gitignore && one ignored-* diff --git a/t/t2103-update-index-ignore-missing.sh b/t/t2103-update-index-ignore-missing.sh index 11bc136f6e..1ce4fc49fa 100755 --- a/t/t2103-update-index-ignore-missing.sh +++ b/t/t2103-update-index-ignore-missing.sh @@ -36,7 +36,7 @@ test_expect_success basics ' git add file && git commit -m "sub initial" ) && - git add ./xyzzy && + git add --allow-embedded-repo ./xyzzy && test_tick && git commit -m initial && diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh index 76f8034c60..bfd87891f4 100755 --- a/t/t4035-diff-quiet.sh +++ b/t/t4035-diff-quiet.sh @@ -66,7 +66,7 @@ test_expect_success 'git diff-index --cached HEAD^' ' test_expect_success 'git diff-index --cached HEAD^' ' echo text >>b && echo 3 >c && - git add . && + git add --allow-embedded-repo . && test_expect_code 1 git diff-index --quiet --cached HEAD^ >cnt && test_line_count = 0 cnt ' diff --git a/t/t6430-merge-recursive.sh b/t/t6430-merge-recursive.sh index 07067bb347..ae435fa492 100755 --- a/t/t6430-merge-recursive.sh +++ b/t/t6430-merge-recursive.sh @@ -677,7 +677,7 @@ test_expect_success 'merging with triple rename across D/F conflict' ' echo content3 >sub2/file3 && mkdir simple && echo base >simple/bar && - git add -A && + git add -A --allow-embedded-repo && test_tick && git commit -m base && diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index eae6a46ef3..18ef9141b7 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -118,7 +118,7 @@ test_expect_success 'setup - repository in init subdirectory' ' test_expect_success 'setup - commit with gitlink' ' echo a >a && echo z >z && - git add a init z && + git add --allow-embedded-repo a init z && git commit -m "super commit 1" ' @@ -771,7 +771,7 @@ test_expect_success 'set up for relative path tests' ' git init && test_commit foo ) && - git add sub && + git add --allow-embedded-repo sub && git config -f .gitmodules submodule.sub.path sub && git config -f .gitmodules submodule.sub.url ../subrepo && cp .git/config pristine-.git-config && diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh index 2859695c6d..d1662aa23c 100755 --- a/t/t7412-submodule-absorbgitdirs.sh +++ b/t/t7412-submodule-absorbgitdirs.sh @@ -100,7 +100,7 @@ test_expect_success 'absorb the git dir in a nested submodule' ' test_expect_success 'setup a gitlink with missing .gitmodules entry' ' git init sub2 && test_commit -C sub2 first && - git add sub2 && + git add --allow-embedded-repo sub2 && git commit -m superproject ' diff --git a/t/t7414-submodule-mistakes.sh b/t/t7414-submodule-mistakes.sh index 101afff30f..d38cf34dff 100755 --- a/t/t7414-submodule-mistakes.sh +++ b/t/t7414-submodule-mistakes.sh @@ -10,31 +10,36 @@ test_expect_success 'create embedded repository' ' test_commit -C embed one ' -test_expect_success 'git-add on embedded repository warns' ' - test_when_finished "git rm --cached -f embed" && - git add embed 2>stderr && - test_i18ngrep warning stderr +test_expect_success 'git-add on embedded repository dies' ' + test_must_fail git add embed 2>stderr && + test_i18ngrep fatal stderr ' -test_expect_success '--no-warn-embedded-repo suppresses warning' ' +test_expect_success '--allow-embedded-repo adds embedded repository and suppresses error message' ' test_when_finished "git rm --cached -f embed" && - git add --no-warn-embedded-repo embed 2>stderr && - test_i18ngrep ! warning stderr + git add --allow-embedded-repo embed 2>stderr && + test_i18ngrep ! fatal stderr +' + +test_expect_success '--no-warn-embedded-repo dies and suppresses advice' ' + test_must_fail git add --no-warn-embedded-repo embed 2>stderr && + test_i18ngrep ! hint stderr && + test_i18ngrep fatal stderr ' -test_expect_success 'no warning when updating entry' ' +test_expect_success 'no error message when updating entry' ' test_when_finished "git rm --cached -f embed" && - git add embed && + git add --allow-embedded-repo embed && git -C embed commit --allow-empty -m two && git add embed 2>stderr && - test_i18ngrep ! warning stderr + test_i18ngrep ! fatal stderr ' -test_expect_success 'submodule add does not warn' ' +test_expect_success 'submodule add neither fails nor issues error message' ' test_when_finished "git rm -rf submodule .gitmodules" && git -c protocol.file.allow=always \ submodule add ./embed submodule 2>stderr && - test_i18ngrep ! warning stderr + test_i18ngrep ! fatal stderr ' test_done diff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh index ba1f569bcb..4b3010c9e2 100755 --- a/t/t7450-bad-git-dotfiles.sh +++ b/t/t7450-bad-git-dotfiles.sh @@ -307,7 +307,7 @@ test_expect_success 'git dirs of sibling submodules must not be nested' ' EOF git clone . thing1 && git clone . thing2 && - git add .gitmodules thing1 thing2 && + git add --allow-embedded-repo .gitmodules thing1 thing2 && test_tick && git commit -m nested ) && -- 2.39.2.722.g9855ee24e9-goog ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [PATCH v2 6/6] add: reject nested repositories 2023-02-28 18:56 ` [PATCH v2 6/6] add: reject nested repositories Calvin Wan @ 2023-03-07 2:04 ` Glen Choo 0 siblings, 0 replies; 40+ messages in thread From: Glen Choo @ 2023-03-07 2:04 UTC (permalink / raw) To: Calvin Wan, git; +Cc: Josh Steadmon, peff, gitster, Calvin Wan Calvin Wan <calvinwan@google.com> writes: > At $dayjob, we have found that even this advice is insufficient to stop > users from committing unclonable embedded repos in shared projects. > This causes toil for the owners of the top-level project repository as > they must clean up the resulting gitlinks. Additionally, these mistakes > are often made by partners outside of $dayjob, which means that a simple > organization-wide change to the default Git config would be insufficient > to prevent these mistakes. > > Due to this experience, we believe that Git's default behavior should be > changed to disallow adding embedded repositories. This commit changes > the existing warning into a fatal error, rewrites the advice given, and > deprecates `--no-warn-embedded-repo` in favor of `--allow-embedded-repo` > to bypass the fatal error. Sounds good, thanks. > ---no-warn-embedded-repo:: > - By default, `git add` will warn when adding an embedded > +--allow-embedded-repo:: > + By default, `git add` will error out when adding an embedded > repository to the index without using `git submodule add` to > - create an entry in `.gitmodules`. This option will suppress the > - warning (e.g., if you are manually performing operations on > - submodules). > + create an entry in `.gitmodules`. This option will allow the > + embedded repository to be added. (e.g., if you are manually > + performing operations on submodules). Okay. > +--no-warn-embedded-repo:: > + This option is deprecated in favor of '--add-embedded-repo'. > + Passing this option still suppresses advice but does not bypass > + the error. Hm, why would a user want to suppress the warning but still have "git add" fail? If this is for backwards compatibility, i.e. users get the same behavior if they pass "--no-warn-embedded-repo" before and after this patch, then shouldn't this also allow the embedded repo to be added (i.e. it is an alias of --allow-embedded-repo)? > @@ -409,48 +412,53 @@ static int add_config(const char *var, const char *value, void *cb) > } > > static const char embedded_advice[] = N_( > -"You've added another git repository inside your current repository.\n" > +"You attempted to add another git repository inside your current repository.\n" > "Clones of the outer repository will not contain the contents of\n" > "the embedded repository and will not know how to obtain it.\n" > "If you meant to add a submodule, use:\n" > "\n" > " git submodule add <url> %s\n" > "\n" > -"If you added this path by mistake, you can remove it from the\n" > -"index with:\n" > +"See \"git help submodule\" for more information.\n" > "\n" > -" git rm --cached %s\n" > +"If you cannot use submodules, you may bypass this check with:\n" > "\n" > -"See \"git help submodule\" for more information." > +" git add --allow-embedded-repo %s\n" > ); Is there a particular reason you reordered the "See \"git help submodule\" for more information.\n" line? I personally like it at the end, and if a user is already used to looking for it at the end, they can keep looking there. > -static void check_embedded_repo(const char *path) > +static int check_embedded_repo(const char *path) > { > + int ret = 0; > struct strbuf name = STRBUF_INIT; > static int adviced_on_embedded_repo = 0; > > - if (!warn_on_embedded_repo) > - return; > + if (allow_embedded_repo) > + goto cleanup; > if (!ends_with(path, "/")) > - return; > + goto cleanup; > + > + ret = 1; > > /* Drop trailing slash for aesthetics */ > strbuf_addstr(&name, path); > strbuf_strip_suffix(&name, "/"); > > - warning(_("adding embedded git repository: %s"), name.buf); > + error(_("cannot add embedded git repository: %s"), name.buf); > if (!adviced_on_embedded_repo && > - advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) { > + warn_on_embedded_repo && > + advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) { > advise(embedded_advice, name.buf, name.buf); > adviced_on_embedded_repo = 1; > } > > +cleanup: > strbuf_release(&name); > + return ret; > } So we give an error message when we first encounter the embedded repo, okay... > static int add_files(struct dir_struct *dir, int flags) > { > - int i, exit_status = 0; > + int i, exit_status = 0, embedded_repo = 0; > struct string_list matched_sparse_paths = STRING_LIST_INIT_NODUP; > > if (dir->ignored_nr) { > @@ -476,10 +484,13 @@ static int add_files(struct dir_struct *dir, int flags) > die(_("adding files failed")); > exit_status = 1; > } else { > - check_embedded_repo(dir->entries[i]->name); > + embedded_repo |= check_embedded_repo(dir->entries[i]->name); > } > } > > + if (embedded_repo) > + die(_("refusing to add embedded git repositories")); > + And then we die(), giving a similar message again. That feels like overkill, but I'm not sure if it's unidiomatic. Is there a reason why we shouldn't just die() the first time we encounter the embedded repo? The difference is that in this patch, we actually add all of the non-submodule files before die()-ing, but I'm not sure if that's what users would expect. Personally at least, I'd expect "git add" to abort the moment it encountered something wrong. > diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh > index c70d11bc91..a53dac5931 100755 > --- a/t/t0008-ignores.sh > +++ b/t/t0008-ignores.sh > @@ -191,7 +191,7 @@ test_expect_success 'setup' ' > git add a && > git commit -m"commit in submodule" > ) && > - git add a/submodule && > + git add --allow-embedded-repo a/submodule && > cat <<-\EOF >.gitignore && > one > ignored-* Given that --allow-embedded-repo is meant to be an escape hatch, and we've already adjusted so many tests to avoid using it, I'd imagine that any uses of it in the test suite have to be quite well-justified. It would have been helpful to see such a justification in the comments on each of the changes or some general principles in the commit message. I've tried testing most of these with "git submodule add". This one works around a test failure in test 346 "submodule", which checks that ignores should fail if the path is in the submodule, like test_check_ignore "a/submodule/one" 128 && test_stderr "fatal: Pathspec '\''a/submodule/one'\'' is in submodule '\''a/submodule'\''" I don't understand why git-check-ignore should succeed on submodules but not an embedded repo. I'll investigate more later. > diff --git a/t/t2103-update-index-ignore-missing.sh b/t/t2103-update-index-ignore-missing.sh > index 11bc136f6e..1ce4fc49fa 100755 > --- a/t/t2103-update-index-ignore-missing.sh > +++ b/t/t2103-update-index-ignore-missing.sh > @@ -36,7 +36,7 @@ test_expect_success basics ' > git add file && > git commit -m "sub initial" > ) && > - git add ./xyzzy && > + git add --allow-embedded-repo ./xyzzy && > > test_tick && > git commit -m initial && This seems to pass with "git submodule add", but maybe we don't want to introduce .gitmodules into a low level test. > diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh > index 76f8034c60..bfd87891f4 100755 > --- a/t/t4035-diff-quiet.sh > +++ b/t/t4035-diff-quiet.sh > @@ -66,7 +66,7 @@ test_expect_success 'git diff-index --cached HEAD^' ' > test_expect_success 'git diff-index --cached HEAD^' ' > echo text >>b && > echo 3 >c && > - git add . && > + git add --allow-embedded-repo . && > test_expect_code 1 git diff-index --quiet --cached HEAD^ >cnt && > test_line_count = 0 cnt > ' This is also a low-level test for gitlinks in the index, so eliminating noise from .gitmodules also seems okay I think. > diff --git a/t/t6430-merge-recursive.sh b/t/t6430-merge-recursive.sh > index 07067bb347..ae435fa492 100755 > --- a/t/t6430-merge-recursive.sh > +++ b/t/t6430-merge-recursive.sh > @@ -677,7 +677,7 @@ test_expect_success 'merging with triple rename across D/F conflict' ' > echo content3 >sub2/file3 && > mkdir simple && > echo base >simple/bar && > - git add -A && > + git add -A --allow-embedded-repo && > test_tick && > git commit -m base && > Similarly, this tests low-level gitlink merging, so eliminating .gitmodules sounds okay. > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > index eae6a46ef3..18ef9141b7 100755 > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh > @@ -118,7 +118,7 @@ test_expect_success 'setup - repository in init subdirectory' ' > test_expect_success 'setup - commit with gitlink' ' > echo a >a && > echo z >z && > - git add a init z && > + git add --allow-embedded-repo a init z && > git commit -m "super commit 1" > ' > This is needed because test 38 test_expect_success 'status should fail for unmapped paths' ' test_must_fail git submodule status ' presumably requires the submodule to not be in .gitmodules. This needs a comment, I think. > @@ -771,7 +771,7 @@ test_expect_success 'set up for relative path tests' ' > git init && > test_commit foo > ) && > - git add sub && > + git add --allow-embedded-repo sub && > git config -f .gitmodules submodule.sub.path sub && > git config -f .gitmodules submodule.sub.url ../subrepo && > cp .git/config pristine-.git-config && In the next test, we're checking that "git submodule init" fixes a gitlink without .gitmodules. Okay. Might benefit from a comment. > diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh > index 2859695c6d..d1662aa23c 100755 > --- a/t/t7412-submodule-absorbgitdirs.sh > +++ b/t/t7412-submodule-absorbgitdirs.sh > @@ -100,7 +100,7 @@ test_expect_success 'absorb the git dir in a nested submodule' ' > test_expect_success 'setup a gitlink with missing .gitmodules entry' ' > git init sub2 && > test_commit -C sub2 first && > - git add sub2 && > + git add --allow-embedded-repo sub2 && > git commit -m superproject > ' The test is literally called 'setup a gitlink with missing .gitmodules entry', so okay. > diff --git a/t/t7450-bad-git-dotfiles.sh b/t/t7450-bad-git-dotfiles.sh > index ba1f569bcb..4b3010c9e2 100755 > --- a/t/t7450-bad-git-dotfiles.sh > +++ b/t/t7450-bad-git-dotfiles.sh > @@ -307,7 +307,7 @@ test_expect_success 'git dirs of sibling submodules must not be nested' ' > EOF > git clone . thing1 && > git clone . thing2 && > - git add .gitmodules thing1 thing2 && > + git add --allow-embedded-repo .gitmodules thing1 thing2 && > test_tick && > git commit -m nested > ) && This one can be adjusted. The important thing is that .gitmodules is bad _after_ we have "git submodule add"-ed the submodules, so this works: git init nested && test_commit -C nested nested && ( cd nested && git clone . thing1 && git clone . thing2 && git submodule add ./thing1 && git submodule add ./thing2 && cat >.gitmodules <<-EOF && [submodule "hippo"] url = . path = thing1 [submodule "hippo/hooks"] url = . path = thing2 EOF git add .gitmodules && test_tick && git commit -m nested ) && test_must_fail git clone --recurse-submodules nested clone 2>err && test_i18ngrep "is inside git dir" err > -- > 2.39.2.722.g9855ee24e9-goog ^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2023-03-07 2:04 UTC | newest] Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-02-13 18:21 [RFC PATCH 0/6] add: block invalid submodules Calvin Wan 2023-02-13 18:21 ` [RFC PATCH 1/6] leak fix: cache_put_path Calvin Wan 2023-02-13 19:23 ` Junio C Hamano 2023-02-14 19:56 ` Calvin Wan 2023-02-14 21:08 ` Junio C Hamano 2023-02-14 21:39 ` Calvin Wan 2023-02-14 21:59 ` Junio C Hamano 2023-02-13 18:21 ` [RFC PATCH 2/6] t4041, t4060: modernize test style Calvin Wan 2023-02-13 19:41 ` Junio C Hamano 2023-02-14 20:22 ` Calvin Wan 2023-02-13 18:21 ` [RFC PATCH 3/6] tests: Use `git submodule add` instead of `git add` Calvin Wan 2023-02-13 18:21 ` [RFC PATCH 4/6] tests: use `git submodule add` and fix expected diffs Calvin Wan 2023-02-13 23:07 ` Junio C Hamano 2023-02-13 23:19 ` Junio C Hamano 2023-02-13 18:21 ` [RFC PATCH 5/6] tests: use `git submodule add` and fix expected status Calvin Wan 2023-02-13 18:21 ` [RFC PATCH 6/6] add: reject nested repositories Calvin Wan 2023-02-13 20:42 ` Jeff King 2023-02-14 2:17 ` Junio C Hamano 2023-02-14 16:07 ` Jeff King 2023-02-14 16:32 ` Junio C Hamano 2023-02-14 21:45 ` Calvin Wan 2023-02-28 18:52 ` [PATCH v2 0/6] add: block invalid submodules Calvin Wan 2023-02-28 18:56 ` [PATCH v2 1/6] t4041, t4060: modernize test style Calvin Wan 2023-03-06 19:32 ` Glen Choo 2023-03-06 20:40 ` Calvin Wan 2023-02-28 18:56 ` [PATCH v2 2/6] tests: Use `git submodule add` instead of `git add` Calvin Wan 2023-02-28 23:30 ` Junio C Hamano 2023-03-03 0:16 ` Calvin Wan 2023-03-06 21:26 ` Glen Choo 2023-02-28 18:56 ` [PATCH v2 3/6] tests: use `git submodule add` and fix expected diffs Calvin Wan 2023-03-06 23:34 ` Glen Choo 2023-03-06 23:57 ` Junio C Hamano 2023-02-28 18:56 ` [PATCH v2 4/6] tests: use `git submodule add` and fix expected status Calvin Wan 2023-03-07 0:15 ` Glen Choo 2023-02-28 18:56 ` [PATCH v2 5/6] tests: remove duplicate .gitmodules path Calvin Wan 2023-02-28 23:35 ` Junio C Hamano 2023-03-02 23:09 ` Calvin Wan 2023-03-07 0:51 ` Glen Choo 2023-02-28 18:56 ` [PATCH v2 6/6] add: reject nested repositories Calvin Wan 2023-03-07 2:04 ` Glen Choo
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).