* [PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw
2018-08-07 16:49 ` [PATCHv3 0/5] " Elijah Newren
@ 2018-08-07 16:49 ` Elijah Newren
2018-08-07 17:29 ` Junio C Hamano
2018-08-07 16:49 ` [PATCHv3 2/5] t7406: avoid having git commands upstream of a pipe Elijah Newren
` (5 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Elijah Newren @ 2018-08-07 16:49 UTC (permalink / raw)
To: git; +Cc: gitster, sunshine, martin.agren, szeder.dev, Elijah Newren
We can get rid of some quoted tabs and make a few tests slightly easier
to read and edit by just asking for the names of the files modified,
since that's all these tests were interested in anyway.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t7406-submodule-update.sh | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f604ef7a72..e2405c96b5 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -579,9 +579,9 @@ test_expect_success 'submodule update - update=none in .git/config' '
git checkout master &&
compare_head
) &&
- git diff --raw | grep " submodule" &&
+ git diff --name-only | grep submodule &&
git submodule update &&
- git diff --raw | grep " submodule" &&
+ git diff --name-only | grep submodule &&
(cd submodule &&
compare_head
) &&
@@ -597,9 +597,9 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
git checkout master &&
compare_head
) &&
- git diff --raw | grep " submodule" &&
+ git diff --name-only | grep submodule &&
git submodule update --checkout &&
- test_must_fail git diff --raw \| grep " submodule" &&
+ test_must_fail git diff --name-only \| grep submodule &&
(cd submodule &&
test_must_fail compare_head
) &&
--
2.18.0.550.geb414df874.dirty
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw
2018-08-07 16:49 ` [PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw Elijah Newren
@ 2018-08-07 17:29 ` Junio C Hamano
2018-08-07 17:40 ` Elijah Newren
0 siblings, 1 reply; 33+ messages in thread
From: Junio C Hamano @ 2018-08-07 17:29 UTC (permalink / raw)
To: Elijah Newren; +Cc: git, sunshine, martin.agren, szeder.dev
Elijah Newren <newren@gmail.com> writes:
> We can get rid of some quoted tabs and make a few tests slightly easier
> to read and edit by just asking for the names of the files modified,
> since that's all these tests were interested in anyway.
Technically the quoted tab was making sure that we do not mistake
"subsubmodule" (if existed) as "submodule" we seek, so a faithful
replacement would be to find "^submodule", and "^submodule$" would
be an improvement. But we do not have paths with confusing names in
these tests, so we can leave it as-is, I guess.
I think 0/5 should fix the real bug you are deliberately keeping in
this patch, from the point of view of organization.
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> t/t7406-submodule-update.sh | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index f604ef7a72..e2405c96b5 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -579,9 +579,9 @@ test_expect_success 'submodule update - update=none in .git/config' '
> git checkout master &&
> compare_head
> ) &&
> - git diff --raw | grep " submodule" &&
> + git diff --name-only | grep submodule &&
> git submodule update &&
> - git diff --raw | grep " submodule" &&
> + git diff --name-only | grep submodule &&
> (cd submodule &&
> compare_head
> ) &&
> @@ -597,9 +597,9 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
> git checkout master &&
> compare_head
> ) &&
> - git diff --raw | grep " submodule" &&
> + git diff --name-only | grep submodule &&
> git submodule update --checkout &&
> - test_must_fail git diff --raw \| grep " submodule" &&
> + test_must_fail git diff --name-only \| grep submodule &&
> (cd submodule &&
> test_must_fail compare_head
> ) &&
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw
2018-08-07 17:29 ` Junio C Hamano
@ 2018-08-07 17:40 ` Elijah Newren
2018-08-07 18:18 ` Junio C Hamano
0 siblings, 1 reply; 33+ messages in thread
From: Elijah Newren @ 2018-08-07 17:40 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git Mailing List, Eric Sunshine, Martin Ågren, SZEDER Gábor
On Tue, Aug 7, 2018 at 10:29 AM Junio C Hamano <gitster@pobox.com> wrote:
> Elijah Newren <newren@gmail.com> writes:
>
> > We can get rid of some quoted tabs and make a few tests slightly easier
> > to read and edit by just asking for the names of the files modified,
> > since that's all these tests were interested in anyway.
>
> Technically the quoted tab was making sure that we do not mistake
> "subsubmodule" (if existed) as "submodule" we seek, so a faithful
> replacement would be to find "^submodule", and "^submodule$" would
> be an improvement. But we do not have paths with confusing names in
> these tests, so we can leave it as-is, I guess.
I knew someone would find additional issues. I'll add the anchors if
any other issues come up in review for the series.
> I think 0/5 should fix the real bug you are deliberately keeping in
> this patch, from the point of view of organization.
You mean 5/5? And yeah, it was just a temporary thing for
organizational purposes.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw
2018-08-07 17:40 ` Elijah Newren
@ 2018-08-07 18:18 ` Junio C Hamano
0 siblings, 0 replies; 33+ messages in thread
From: Junio C Hamano @ 2018-08-07 18:18 UTC (permalink / raw)
To: Elijah Newren
Cc: Git Mailing List, Eric Sunshine, Martin Ågren, SZEDER Gábor
Elijah Newren <newren@gmail.com> writes:
>> I think 0/5 should fix the real bug you are deliberately keeping in
>> this patch, from the point of view of organization.
>
> You mean 5/5? And yeah, it was just a temporary thing for
> organizational purposes.
I meant "a thing that comes before all the other steps".
As far as I can tell, everything else this 5-patch series improves
would still have caught a new bug in the subsystem being tested
(i.e. "submodule update") even without these improvements. Surely,
if we greak "git diff", "git diff --raw" that sits on the upstream
of a pipe would not have stopped these tests, but we have tests for
"diff" elsewhere and t7406 are not about catching the breakage of
"diff", so from that point of view, fixing them, although it is
necessary and important, is less important.
The "\| grep" thing was a real bug in that if we broke "submodule
update", it would not have helped to catch such a bug, as it wasn't
looking for 'submodule' string in the output.
That is why I felt it would have been a better organization to fix
"\| grep" in "a thing that comes before all the other steps" and
then fix the rest as lower priority clean up.
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCHv3 2/5] t7406: avoid having git commands upstream of a pipe
2018-08-07 16:49 ` [PATCHv3 0/5] " Elijah Newren
2018-08-07 16:49 ` [PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw Elijah Newren
@ 2018-08-07 16:49 ` Elijah Newren
2018-08-07 16:49 ` [PATCHv3 3/5] t7406: prefer test_* helper functions to test -[feds] Elijah Newren
` (4 subsequent siblings)
6 siblings, 0 replies; 33+ messages in thread
From: Elijah Newren @ 2018-08-07 16:49 UTC (permalink / raw)
To: git; +Cc: gitster, sunshine, martin.agren, szeder.dev, Elijah Newren
When a git command is on the left side of a pipe, the pipe will swallow
its exit status, preventing us from detecting failures in said commands.
Restructure the tests to put the output in a temporary file to avoid
this problem.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t7406-submodule-update.sh | 26 +++++++++++++++++---------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index e2405c96b5..c6b7b59350 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -481,7 +481,8 @@ test_expect_success 'recursive submodule update - command in .git/config catches
test_expect_success 'submodule init does not copy command into .git/config' '
(cd super &&
- H=$(git ls-files -s submodule | cut -d" " -f2) &&
+ git ls-files -s submodule >out &&
+ H=$(cut -d" " -f2 out) &&
mkdir submodule1 &&
git update-index --add --cacheinfo 160000 $H submodule1 &&
git config -f .gitmodules submodule.submodule1.path submodule1 &&
@@ -579,9 +580,11 @@ test_expect_success 'submodule update - update=none in .git/config' '
git checkout master &&
compare_head
) &&
- git diff --name-only | grep submodule &&
+ git diff --name-only >out &&
+ grep submodule out &&
git submodule update &&
- git diff --name-only | grep submodule &&
+ git diff --name-only >out &&
+ grep submodule out &&
(cd submodule &&
compare_head
) &&
@@ -597,7 +600,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
git checkout master &&
compare_head
) &&
- git diff --name-only | grep submodule &&
+ git diff --name-only >out &&
+ grep submodule out &&
git submodule update --checkout &&
test_must_fail git diff --name-only \| grep submodule &&
(cd submodule &&
@@ -885,7 +889,8 @@ test_expect_success 'submodule update properly revives a moved submodule' '
H=$(git rev-parse --short HEAD) &&
git commit -am "pre move" &&
H2=$(git rev-parse --short HEAD) &&
- git status | sed "s/$H/XXX/" >expect &&
+ git status >out &&
+ sed "s/$H/XXX/" out >expect &&
H=$(cd submodule2 && git rev-parse HEAD) &&
git rm --cached submodule2 &&
rm -rf submodule2 &&
@@ -894,7 +899,8 @@ test_expect_success 'submodule update properly revives a moved submodule' '
git config -f .gitmodules submodule.submodule2.path "moved/sub module" &&
git commit -am "post move" &&
git submodule update &&
- git status | sed "s/$H2/XXX/" >actual &&
+ git status > out &&
+ sed "s/$H2/XXX/" out >actual &&
test_cmp expect actual
)
'
@@ -912,7 +918,7 @@ test_expect_success SYMLINKS 'submodule update can handle symbolic links in pwd'
test_expect_success 'submodule update clone shallow submodule' '
test_when_finished "rm -rf super3" &&
- first=$(git -C cloned submodule status submodule |cut -c2-41) &&
+ first=$(git -C cloned rev-parse HEAD:submodule) &&
second=$(git -C submodule rev-parse HEAD) &&
commit_count=$(git -C submodule rev-list --count $first^..$second) &&
git clone cloned super3 &&
@@ -922,7 +928,8 @@ test_expect_success 'submodule update clone shallow submodule' '
sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
mv -f .gitmodules.tmp .gitmodules &&
git submodule update --init --depth=$commit_count &&
- test 1 = $(git -C submodule log --oneline | wc -l)
+ git -C submodule log --oneline >out &&
+ test_line_count = 1 out
)
'
@@ -938,7 +945,8 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'
test_i18ngrep "Direct fetching of that commit failed." actual &&
git -C ../submodule config uploadpack.allowReachableSHA1InWant true &&
git submodule update --init --depth=1 >actual &&
- test 1 = $(git -C submodule log --oneline | wc -l)
+ git -C submodule log --oneline >out &&
+ test_line_count = 1 out
)
'
--
2.18.0.550.geb414df874.dirty
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCHv3 3/5] t7406: prefer test_* helper functions to test -[feds]
2018-08-07 16:49 ` [PATCHv3 0/5] " Elijah Newren
2018-08-07 16:49 ` [PATCHv3 1/5] t7406: simplify by using diff --name-only instead of diff --raw Elijah Newren
2018-08-07 16:49 ` [PATCHv3 2/5] t7406: avoid having git commands upstream of a pipe Elijah Newren
@ 2018-08-07 16:49 ` Elijah Newren
2018-08-07 17:34 ` Junio C Hamano
2018-08-07 16:49 ` [PATCHv3 4/5] t7406: avoid using test_must_fail for commands other than git Elijah Newren
` (3 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Elijah Newren @ 2018-08-07 16:49 UTC (permalink / raw)
To: git; +Cc: gitster, sunshine, martin.agren, szeder.dev, Elijah Newren
test -e, test -s, etc. do not provide nice error messages when we hit
test failures, so use the test_* helper functions from
test-lib-functions.sh.
Note: The use of 'test_path_is_file submodule/.git' may look odd, but
it is a file which is populated with a
gitdir: ../.git/modules/submodule
directive. If, in the future, handling of the submodule is changed and
submodule/.git becomes a directory we can change this to
test_path_is_dir (or perhaps write a test_path_exists helper function
that doesn't care whether the path is a file or a directory).
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t7406-submodule-update.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index c6b7b59350..ab67e373c5 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -174,7 +174,7 @@ test_expect_success 'submodule update does not fetch already present commits' '
git submodule update > ../actual 2> ../actual.err
) &&
test_i18ncmp expected actual &&
- ! test -s actual.err
+ test_must_be_empty actual.err
'
test_expect_success 'submodule update should fail due to local changes' '
@@ -619,8 +619,8 @@ test_expect_success 'submodule update --init skips submodule with update=none' '
git clone super cloned &&
(cd cloned &&
git submodule update --init &&
- test -e submodule/.git &&
- test_must_fail test -e none/.git
+ test_path_is_file submodule/.git &&
+ test_path_is_missing none/.git
)
'
--
2.18.0.550.geb414df874.dirty
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCHv3 3/5] t7406: prefer test_* helper functions to test -[feds]
2018-08-07 16:49 ` [PATCHv3 3/5] t7406: prefer test_* helper functions to test -[feds] Elijah Newren
@ 2018-08-07 17:34 ` Junio C Hamano
0 siblings, 0 replies; 33+ messages in thread
From: Junio C Hamano @ 2018-08-07 17:34 UTC (permalink / raw)
To: Elijah Newren; +Cc: git, sunshine, martin.agren, szeder.dev
Elijah Newren <newren@gmail.com> writes:
> test -e, test -s, etc. do not provide nice error messages when we hit
> test failures, so use the test_* helper functions from
> test-lib-functions.sh.
Good explanation.
> Note: The use of 'test_path_is_file submodule/.git' may look odd, but
> it is a file which is populated with a
> gitdir: ../.git/modules/submodule
> directive. If, in the future, handling of the submodule is changed and
> submodule/.git becomes a directory we can change this to
> test_path_is_dir (or perhaps write a test_path_exists helper function
> that doesn't care whether the path is a file or a directory).
Yup, path_exists would be a good direction going forward. If we
already have "missing" and use it in this rewrite, it may make sense
to introduce "exists" and use it at the same time here.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> t/t7406-submodule-update.sh | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index c6b7b59350..ab67e373c5 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -174,7 +174,7 @@ test_expect_success 'submodule update does not fetch already present commits' '
> git submodule update > ../actual 2> ../actual.err
> ) &&
> test_i18ncmp expected actual &&
> - ! test -s actual.err
> + test_must_be_empty actual.err
> '
>
> test_expect_success 'submodule update should fail due to local changes' '
> @@ -619,8 +619,8 @@ test_expect_success 'submodule update --init skips submodule with update=none' '
> git clone super cloned &&
> (cd cloned &&
> git submodule update --init &&
> - test -e submodule/.git &&
> - test_must_fail test -e none/.git
> + test_path_is_file submodule/.git &&
> + test_path_is_missing none/.git
> )
> '
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCHv3 4/5] t7406: avoid using test_must_fail for commands other than git
2018-08-07 16:49 ` [PATCHv3 0/5] " Elijah Newren
` (2 preceding siblings ...)
2018-08-07 16:49 ` [PATCHv3 3/5] t7406: prefer test_* helper functions to test -[feds] Elijah Newren
@ 2018-08-07 16:49 ` Elijah Newren
2018-08-07 16:49 ` [PATCHv3 5/5] t7406: fix call that was failing for the wrong reason Elijah Newren
` (2 subsequent siblings)
6 siblings, 0 replies; 33+ messages in thread
From: Elijah Newren @ 2018-08-07 16:49 UTC (permalink / raw)
To: git; +Cc: gitster, sunshine, martin.agren, szeder.dev, Elijah Newren
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t7406-submodule-update.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index ab67e373c5..5b42bbe9fa 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -605,7 +605,7 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
git submodule update --checkout &&
test_must_fail git diff --name-only \| grep submodule &&
(cd submodule &&
- test_must_fail compare_head
+ ! compare_head
) &&
git config --unset submodule.submodule.update
)
--
2.18.0.550.geb414df874.dirty
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCHv3 5/5] t7406: fix call that was failing for the wrong reason
2018-08-07 16:49 ` [PATCHv3 0/5] " Elijah Newren
` (3 preceding siblings ...)
2018-08-07 16:49 ` [PATCHv3 4/5] t7406: avoid using test_must_fail for commands other than git Elijah Newren
@ 2018-08-07 16:49 ` Elijah Newren
2018-08-07 17:37 ` Junio C Hamano
2018-08-08 16:31 ` [PATCHv4 0/5] Simple fixes to t7406 Elijah Newren
2018-08-13 20:28 ` [PATCHv3 0/5] Simple fixes to t7406 SZEDER Gábor
6 siblings, 1 reply; 33+ messages in thread
From: Elijah Newren @ 2018-08-07 16:49 UTC (permalink / raw)
To: git; +Cc: gitster, sunshine, martin.agren, szeder.dev, Elijah Newren
A test making use of test_must_fail was failing like this:
fatal: ambiguous argument '|': unknown revision or path not in the working tree.
when the intent was to verify that a specific string was not found
in the output of the git diff command, i.e. that grep returned
non-zero. Fix the test to do that.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t7406-submodule-update.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 5b42bbe9fa..7dd1c86b02 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -603,7 +603,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
git diff --name-only >out &&
grep submodule out &&
git submodule update --checkout &&
- test_must_fail git diff --name-only \| grep submodule &&
+ git diff --name-only >out &&
+ ! grep submodule out &&
(cd submodule &&
! compare_head
) &&
--
2.18.0.550.geb414df874.dirty
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCHv3 5/5] t7406: fix call that was failing for the wrong reason
2018-08-07 16:49 ` [PATCHv3 5/5] t7406: fix call that was failing for the wrong reason Elijah Newren
@ 2018-08-07 17:37 ` Junio C Hamano
0 siblings, 0 replies; 33+ messages in thread
From: Junio C Hamano @ 2018-08-07 17:37 UTC (permalink / raw)
To: Elijah Newren; +Cc: git, sunshine, martin.agren, szeder.dev
Elijah Newren <newren@gmail.com> writes:
> A test making use of test_must_fail was failing like this:
> fatal: ambiguous argument '|': unknown revision or path not in the working tree.
> when the intent was to verify that a specific string was not found
> in the output of the git diff command, i.e. that grep returned
> non-zero. Fix the test to do that.
Nice. All other patches and the final shape of this script look
sensible.
Thanks.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
> t/t7406-submodule-update.sh | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 5b42bbe9fa..7dd1c86b02 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -603,7 +603,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
> git diff --name-only >out &&
> grep submodule out &&
> git submodule update --checkout &&
> - test_must_fail git diff --name-only \| grep submodule &&
> + git diff --name-only >out &&
> + ! grep submodule out &&
> (cd submodule &&
> ! compare_head
> ) &&
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCHv4 0/5] Simple fixes to t7406
2018-08-07 16:49 ` [PATCHv3 0/5] " Elijah Newren
` (4 preceding siblings ...)
2018-08-07 16:49 ` [PATCHv3 5/5] t7406: fix call that was failing for the wrong reason Elijah Newren
@ 2018-08-08 16:31 ` Elijah Newren
2018-08-08 16:31 ` [PATCHv4 1/5] t7406: fix call that was failing for the wrong reason Elijah Newren
` (4 more replies)
2018-08-13 20:28 ` [PATCHv3 0/5] Simple fixes to t7406 SZEDER Gábor
6 siblings, 5 replies; 33+ messages in thread
From: Elijah Newren @ 2018-08-08 16:31 UTC (permalink / raw)
To: gitster; +Cc: git, sunshine, martin.agren, szeder.dev, Elijah Newren
Changes since v3, all suggested/endorsed by Junio (range-diff at the end):
- Moved the actual fix from being last patch in the series to the first
(other patches in this series are just test code cleanups)
- Anchored regexes to avoid matching another filename as a substring
- Add test_path_exists() to test-lib-function.sh and use it (we had
test_path_is_dir, test_path_is_file, and test_path_is_missing, but
not simple test_path_exists)
Elijah Newren (5):
t7406: fix call that was failing for the wrong reason
t7406: simplify by using diff --name-only instead of diff --raw
t7406: avoid having git commands upstream of a pipe
t7406: prefer test_* helper functions to test -[feds]
t7406: avoid using test_must_fail for commands other than git
t/t7406-submodule-update.sh | 37 +++++++++++++++++++++++--------------
t/test-lib-functions.sh | 8 ++++++++
2 files changed, 31 insertions(+), 14 deletions(-)
-: ---------- > 1: 5f257af6c8 t7406: fix call that was failing for the wrong reason
1: 3c369bf73d ! 2: 9e5400a1ad t7406: simplify by using diff --name-only instead of diff --raw
@@ -16,10 +16,10 @@
compare_head
) &&
- git diff --raw | grep " submodule" &&
-+ git diff --name-only | grep submodule &&
++ git diff --name-only | grep ^submodule$ &&
git submodule update &&
- git diff --raw | grep " submodule" &&
-+ git diff --name-only | grep submodule &&
++ git diff --name-only | grep ^submodule$ &&
(cd submodule &&
compare_head
) &&
@@ -28,10 +28,12 @@
compare_head
) &&
- git diff --raw | grep " submodule" &&
-+ git diff --name-only | grep submodule &&
++ git diff --name-only | grep ^submodule$ &&
git submodule update --checkout &&
-- test_must_fail git diff --raw \| grep " submodule" &&
-+ test_must_fail git diff --name-only \| grep submodule &&
+- git diff --raw >out &&
+- ! grep " submodule" out &&
++ git diff --name-only >out &&
++ ! grep ^submodule$ out &&
(cd submodule &&
test_must_fail compare_head
) &&
2: ba50d6b0f3 ! 3: 4e8cdf60f4 t7406: avoid having git commands upstream of a pipe
@@ -26,13 +26,13 @@
git checkout master &&
compare_head
) &&
-- git diff --name-only | grep submodule &&
+- git diff --name-only | grep ^submodule$ &&
+ git diff --name-only >out &&
-+ grep submodule out &&
++ grep ^submodule$ out &&
git submodule update &&
-- git diff --name-only | grep submodule &&
+- git diff --name-only | grep ^submodule$ &&
+ git diff --name-only >out &&
-+ grep submodule out &&
++ grep ^submodule$ out &&
(cd submodule &&
compare_head
) &&
@@ -40,12 +40,12 @@
git checkout master &&
compare_head
) &&
-- git diff --name-only | grep submodule &&
+- git diff --name-only | grep ^submodule$ &&
+ git diff --name-only >out &&
-+ grep submodule out &&
++ grep ^submodule$ out &&
git submodule update --checkout &&
- test_must_fail git diff --name-only \| grep submodule &&
- (cd submodule &&
+ git diff --name-only >out &&
+ ! grep ^submodule$ out &&
@@
H=$(git rev-parse --short HEAD) &&
git commit -am "pre move" &&
3: 42f7b7f225 ! 4: f171cbcc9a t7406: prefer test_* helper functions to test -[feds]
@@ -6,13 +6,12 @@
test failures, so use the test_* helper functions from
test-lib-functions.sh.
- Note: The use of 'test_path_is_file submodule/.git' may look odd, but
- it is a file which is populated with a
+ Also, add test_path_exists() to test-lib-function.sh while at it, so
+ that we don't need to worry whether submodule/.git is a file or a
+ directory. It currently is a file with contents of the form
gitdir: ../.git/modules/submodule
- directive. If, in the future, handling of the submodule is changed and
- submodule/.git becomes a directory we can change this to
- test_path_is_dir (or perhaps write a test_path_exists helper function
- that doesn't care whether the path is a file or a directory).
+ but it could be changed in the future to be a directory; this test
+ only really cares that it exists.
Signed-off-by: Elijah Newren <newren@gmail.com>
@@ -34,8 +33,27 @@
git submodule update --init &&
- test -e submodule/.git &&
- test_must_fail test -e none/.git
-+ test_path_is_file submodule/.git &&
++ test_path_exists submodule/.git &&
+ test_path_is_missing none/.git
)
'
+
+diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
+--- a/t/test-lib-functions.sh
++++ b/t/test-lib-functions.sh
+@@
+ fi
+ }
+
++test_path_exists () {
++ if ! test -e "$1"
++ then
++ echo "Path $1 doesn't exist. $2"
++ false
++ fi
++}
++
+ # Check if the directory exists and is empty as expected, barf otherwise.
+ test_dir_is_empty () {
+ test_path_is_dir "$1" &&
4: 54cf6531ec ! 5: a44c566321 t7406: avoid using test_must_fail for commands other than git
@@ -8,8 +8,8 @@
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@
- git submodule update --checkout &&
- test_must_fail git diff --name-only \| grep submodule &&
+ git diff --name-only >out &&
+ ! grep ^submodule$ out &&
(cd submodule &&
- test_must_fail compare_head
+ ! compare_head
5: 3019f2d01c < -: ---------- t7406: fix call that was failing for the wrong reason
--
2.18.0.556.g1604670984
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCHv4 1/5] t7406: fix call that was failing for the wrong reason
2018-08-08 16:31 ` [PATCHv4 0/5] Simple fixes to t7406 Elijah Newren
@ 2018-08-08 16:31 ` Elijah Newren
2018-08-08 16:31 ` [PATCHv4 2/5] t7406: simplify by using diff --name-only instead of diff --raw Elijah Newren
` (3 subsequent siblings)
4 siblings, 0 replies; 33+ messages in thread
From: Elijah Newren @ 2018-08-08 16:31 UTC (permalink / raw)
To: gitster; +Cc: git, sunshine, martin.agren, szeder.dev, Elijah Newren
A test making use of test_must_fail was failing like this:
fatal: ambiguous argument '|': unknown revision or path not in the working tree.
when the intent was to verify that a specific string was not found
in the output of the git diff command, i.e. that grep returned
non-zero. Fix the test to do that.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t7406-submodule-update.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f604ef7a72..ccdc2f9039 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -599,7 +599,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
) &&
git diff --raw | grep " submodule" &&
git submodule update --checkout &&
- test_must_fail git diff --raw \| grep " submodule" &&
+ git diff --raw >out &&
+ ! grep " submodule" out &&
(cd submodule &&
test_must_fail compare_head
) &&
--
2.18.0.556.g1604670984
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCHv4 2/5] t7406: simplify by using diff --name-only instead of diff --raw
2018-08-08 16:31 ` [PATCHv4 0/5] Simple fixes to t7406 Elijah Newren
2018-08-08 16:31 ` [PATCHv4 1/5] t7406: fix call that was failing for the wrong reason Elijah Newren
@ 2018-08-08 16:31 ` Elijah Newren
2018-08-08 16:31 ` [PATCHv4 3/5] t7406: avoid having git commands upstream of a pipe Elijah Newren
` (2 subsequent siblings)
4 siblings, 0 replies; 33+ messages in thread
From: Elijah Newren @ 2018-08-08 16:31 UTC (permalink / raw)
To: gitster; +Cc: git, sunshine, martin.agren, szeder.dev, Elijah Newren
We can get rid of some quoted tabs and make a few tests slightly easier
to read and edit by just asking for the names of the files modified,
since that's all these tests were interested in anyway.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t7406-submodule-update.sh | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index ccdc2f9039..f821b01f15 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -579,9 +579,9 @@ test_expect_success 'submodule update - update=none in .git/config' '
git checkout master &&
compare_head
) &&
- git diff --raw | grep " submodule" &&
+ git diff --name-only | grep ^submodule$ &&
git submodule update &&
- git diff --raw | grep " submodule" &&
+ git diff --name-only | grep ^submodule$ &&
(cd submodule &&
compare_head
) &&
@@ -597,10 +597,10 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
git checkout master &&
compare_head
) &&
- git diff --raw | grep " submodule" &&
+ git diff --name-only | grep ^submodule$ &&
git submodule update --checkout &&
- git diff --raw >out &&
- ! grep " submodule" out &&
+ git diff --name-only >out &&
+ ! grep ^submodule$ out &&
(cd submodule &&
test_must_fail compare_head
) &&
--
2.18.0.556.g1604670984
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCHv4 3/5] t7406: avoid having git commands upstream of a pipe
2018-08-08 16:31 ` [PATCHv4 0/5] Simple fixes to t7406 Elijah Newren
2018-08-08 16:31 ` [PATCHv4 1/5] t7406: fix call that was failing for the wrong reason Elijah Newren
2018-08-08 16:31 ` [PATCHv4 2/5] t7406: simplify by using diff --name-only instead of diff --raw Elijah Newren
@ 2018-08-08 16:31 ` Elijah Newren
2018-08-08 16:31 ` [PATCHv4 4/5] t7406: prefer test_* helper functions to test -[feds] Elijah Newren
2018-08-08 16:31 ` [PATCHv4 5/5] t7406: avoid using test_must_fail for commands other than git Elijah Newren
4 siblings, 0 replies; 33+ messages in thread
From: Elijah Newren @ 2018-08-08 16:31 UTC (permalink / raw)
To: gitster; +Cc: git, sunshine, martin.agren, szeder.dev, Elijah Newren
When a git command is on the left side of a pipe, the pipe will swallow
its exit status, preventing us from detecting failures in said commands.
Restructure the tests to put the output in a temporary file to avoid
this problem.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t7406-submodule-update.sh | 26 +++++++++++++++++---------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f821b01f15..2463863a51 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -481,7 +481,8 @@ test_expect_success 'recursive submodule update - command in .git/config catches
test_expect_success 'submodule init does not copy command into .git/config' '
(cd super &&
- H=$(git ls-files -s submodule | cut -d" " -f2) &&
+ git ls-files -s submodule >out &&
+ H=$(cut -d" " -f2 out) &&
mkdir submodule1 &&
git update-index --add --cacheinfo 160000 $H submodule1 &&
git config -f .gitmodules submodule.submodule1.path submodule1 &&
@@ -579,9 +580,11 @@ test_expect_success 'submodule update - update=none in .git/config' '
git checkout master &&
compare_head
) &&
- git diff --name-only | grep ^submodule$ &&
+ git diff --name-only >out &&
+ grep ^submodule$ out &&
git submodule update &&
- git diff --name-only | grep ^submodule$ &&
+ git diff --name-only >out &&
+ grep ^submodule$ out &&
(cd submodule &&
compare_head
) &&
@@ -597,7 +600,8 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
git checkout master &&
compare_head
) &&
- git diff --name-only | grep ^submodule$ &&
+ git diff --name-only >out &&
+ grep ^submodule$ out &&
git submodule update --checkout &&
git diff --name-only >out &&
! grep ^submodule$ out &&
@@ -886,7 +890,8 @@ test_expect_success 'submodule update properly revives a moved submodule' '
H=$(git rev-parse --short HEAD) &&
git commit -am "pre move" &&
H2=$(git rev-parse --short HEAD) &&
- git status | sed "s/$H/XXX/" >expect &&
+ git status >out &&
+ sed "s/$H/XXX/" out >expect &&
H=$(cd submodule2 && git rev-parse HEAD) &&
git rm --cached submodule2 &&
rm -rf submodule2 &&
@@ -895,7 +900,8 @@ test_expect_success 'submodule update properly revives a moved submodule' '
git config -f .gitmodules submodule.submodule2.path "moved/sub module" &&
git commit -am "post move" &&
git submodule update &&
- git status | sed "s/$H2/XXX/" >actual &&
+ git status > out &&
+ sed "s/$H2/XXX/" out >actual &&
test_cmp expect actual
)
'
@@ -913,7 +919,7 @@ test_expect_success SYMLINKS 'submodule update can handle symbolic links in pwd'
test_expect_success 'submodule update clone shallow submodule' '
test_when_finished "rm -rf super3" &&
- first=$(git -C cloned submodule status submodule |cut -c2-41) &&
+ first=$(git -C cloned rev-parse HEAD:submodule) &&
second=$(git -C submodule rev-parse HEAD) &&
commit_count=$(git -C submodule rev-list --count $first^..$second) &&
git clone cloned super3 &&
@@ -923,7 +929,8 @@ test_expect_success 'submodule update clone shallow submodule' '
sed -e "s#url = ../#url = file://$pwd/#" <.gitmodules >.gitmodules.tmp &&
mv -f .gitmodules.tmp .gitmodules &&
git submodule update --init --depth=$commit_count &&
- test 1 = $(git -C submodule log --oneline | wc -l)
+ git -C submodule log --oneline >out &&
+ test_line_count = 1 out
)
'
@@ -939,7 +946,8 @@ test_expect_success 'submodule update clone shallow submodule outside of depth'
test_i18ngrep "Direct fetching of that commit failed." actual &&
git -C ../submodule config uploadpack.allowReachableSHA1InWant true &&
git submodule update --init --depth=1 >actual &&
- test 1 = $(git -C submodule log --oneline | wc -l)
+ git -C submodule log --oneline >out &&
+ test_line_count = 1 out
)
'
--
2.18.0.556.g1604670984
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCHv4 4/5] t7406: prefer test_* helper functions to test -[feds]
2018-08-08 16:31 ` [PATCHv4 0/5] Simple fixes to t7406 Elijah Newren
` (2 preceding siblings ...)
2018-08-08 16:31 ` [PATCHv4 3/5] t7406: avoid having git commands upstream of a pipe Elijah Newren
@ 2018-08-08 16:31 ` Elijah Newren
2018-08-08 16:31 ` [PATCHv4 5/5] t7406: avoid using test_must_fail for commands other than git Elijah Newren
4 siblings, 0 replies; 33+ messages in thread
From: Elijah Newren @ 2018-08-08 16:31 UTC (permalink / raw)
To: gitster; +Cc: git, sunshine, martin.agren, szeder.dev, Elijah Newren
test -e, test -s, etc. do not provide nice error messages when we hit
test failures, so use the test_* helper functions from
test-lib-functions.sh.
Also, add test_path_exists() to test-lib-function.sh while at it, so
that we don't need to worry whether submodule/.git is a file or a
directory. It currently is a file with contents of the form
gitdir: ../.git/modules/submodule
but it could be changed in the future to be a directory; this test
only really cares that it exists.
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t7406-submodule-update.sh | 6 +++---
t/test-lib-functions.sh | 8 ++++++++
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 2463863a51..24aa732312 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -174,7 +174,7 @@ test_expect_success 'submodule update does not fetch already present commits' '
git submodule update > ../actual 2> ../actual.err
) &&
test_i18ncmp expected actual &&
- ! test -s actual.err
+ test_must_be_empty actual.err
'
test_expect_success 'submodule update should fail due to local changes' '
@@ -620,8 +620,8 @@ test_expect_success 'submodule update --init skips submodule with update=none' '
git clone super cloned &&
(cd cloned &&
git submodule update --init &&
- test -e submodule/.git &&
- test_must_fail test -e none/.git
+ test_path_exists submodule/.git &&
+ test_path_is_missing none/.git
)
'
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 2b2181dca0..4207af4077 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -565,6 +565,14 @@ test_path_is_dir () {
fi
}
+test_path_exists () {
+ if ! test -e "$1"
+ then
+ echo "Path $1 doesn't exist. $2"
+ false
+ fi
+}
+
# Check if the directory exists and is empty as expected, barf otherwise.
test_dir_is_empty () {
test_path_is_dir "$1" &&
--
2.18.0.556.g1604670984
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCHv4 5/5] t7406: avoid using test_must_fail for commands other than git
2018-08-08 16:31 ` [PATCHv4 0/5] Simple fixes to t7406 Elijah Newren
` (3 preceding siblings ...)
2018-08-08 16:31 ` [PATCHv4 4/5] t7406: prefer test_* helper functions to test -[feds] Elijah Newren
@ 2018-08-08 16:31 ` Elijah Newren
4 siblings, 0 replies; 33+ messages in thread
From: Elijah Newren @ 2018-08-08 16:31 UTC (permalink / raw)
To: gitster; +Cc: git, sunshine, martin.agren, szeder.dev, Elijah Newren
Signed-off-by: Elijah Newren <newren@gmail.com>
---
t/t7406-submodule-update.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 24aa732312..815b60cb59 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -606,7 +606,7 @@ test_expect_success 'submodule update - update=none in .git/config but --checkou
git diff --name-only >out &&
! grep ^submodule$ out &&
(cd submodule &&
- test_must_fail compare_head
+ ! compare_head
) &&
git config --unset submodule.submodule.update
)
--
2.18.0.556.g1604670984
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCHv3 0/5] Simple fixes to t7406
2018-08-07 16:49 ` [PATCHv3 0/5] " Elijah Newren
` (5 preceding siblings ...)
2018-08-08 16:31 ` [PATCHv4 0/5] Simple fixes to t7406 Elijah Newren
@ 2018-08-13 20:28 ` SZEDER Gábor
2018-08-18 20:52 ` Elijah Newren
6 siblings, 1 reply; 33+ messages in thread
From: SZEDER Gábor @ 2018-08-13 20:28 UTC (permalink / raw)
To: Elijah Newren
Cc: Git mailing list, Junio C Hamano, Eric Sunshine, Martin Ågren
On Tue, Aug 7, 2018 at 6:49 PM Elijah Newren <newren@gmail.com> wrote:
> Since folks like to notice other problems with t7406 while reading my
> patches, here's a challenge:
>
> Find something *else* wrong with t7406 that neither I nor any of the
> reviewers so far have caught that could be fixed.
Well, I'd hate to be that guy... but since those who already
commented on previous rounds are not explicitly excluded from the
challenge, let's see.
- There are still a few command substitutions running git commands,
where the exit status of that command is ignored; just look for the
'[^=]$(' pattern in the test script.
(Is not noticing those cases considered as "flubbing"?)
- The 'compare_head' helper function defined in this test script looks
very similar to the generally available 'test_cmp_rev' function,
which has the benefit to provide some visible output on failure
(though, IMO, not a particularly useful output, because the diff of
two OIDs is not very informative, but at least it's something as
opposed to the silence of 'test $this = $that").
Now, since 'compare_head' always compares the same two revisions,
namely 'master' and HEAD, replacing 'compare_head' with an
appropriate 'test_cmp_rev' call would result in repeating 'master'
and 'HEAD' arguments all over the test script. I'm not sure whether
that's good or bad. Anyway, I think that 'compare_head' could be
turned into a wrapper around 'test_cmp_rev'.
> - You get bonus points if that thing is in the context region for
> one of my five patches.
> - Extra bonus points if the thing needing fixing was on a line I
> changed.
> - You win outright if it's something big enough that I give up and
> request to just have my series merged as-is and punt your
> suggested fixes down the road to someone else.
Well, there's always the indentation of the commands run in subshells,
which doesn't conform to our coding style...
Gah, now you made me that guy ;)
> (Note: If I flubbed something in v3, pointing it out doesn't count
> as finding "something else", but do please still point it out.)
>
> :-)
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHv3 0/5] Simple fixes to t7406
2018-08-13 20:28 ` [PATCHv3 0/5] Simple fixes to t7406 SZEDER Gábor
@ 2018-08-18 20:52 ` Elijah Newren
0 siblings, 0 replies; 33+ messages in thread
From: Elijah Newren @ 2018-08-18 20:52 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Git Mailing List, Junio C Hamano, Eric Sunshine, Martin Ågren
Hi,
On Mon, Aug 13, 2018 at 1:28 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Tue, Aug 7, 2018 at 6:49 PM Elijah Newren <newren@gmail.com> wrote:
>
> > Since folks like to notice other problems with t7406 while reading my
> > patches, here's a challenge:
> >
> > Find something *else* wrong with t7406 that neither I nor any of the
> > reviewers so far have caught that could be fixed.
>
> Well, I'd hate to be that guy... but since those who already
> commented on previous rounds are not explicitly excluded from the
> challenge, let's see.
>
> - There are still a few command substitutions running git commands,
> where the exit status of that command is ignored; just look for the
> '[^=]$(' pattern in the test script.
>
> (Is not noticing those cases considered as "flubbing"?)
Hmm, borderline.
> - The 'compare_head' helper function defined in this test script looks
> very similar to the generally available 'test_cmp_rev' function,
> which has the benefit to provide some visible output on failure
> (though, IMO, not a particularly useful output, because the diff of
> two OIDs is not very informative, but at least it's something as
> opposed to the silence of 'test $this = $that").
>
> Now, since 'compare_head' always compares the same two revisions,
> namely 'master' and HEAD, replacing 'compare_head' with an
> appropriate 'test_cmp_rev' call would result in repeating 'master'
> and 'HEAD' arguments all over the test script. I'm not sure whether
> that's good or bad. Anyway, I think that 'compare_head' could be
> turned into a wrapper around 'test_cmp_rev'.
Ooh, that does sound better.
> > - You get bonus points if that thing is in the context region for
> > one of my five patches.
> > - Extra bonus points if the thing needing fixing was on a line I
> > changed.
> > - You win outright if it's something big enough that I give up and
> > request to just have my series merged as-is and punt your
> > suggested fixes down the road to someone else.
>
> Well, there's always the indentation of the commands run in subshells,
> which doesn't conform to our coding style...
>
> Gah, now you made me that guy ;)
I read this on Monday and got a really good laugh. I meant to fix it
up, but fell asleep too soon the first couple nights...and now this
series is in next anyway and there are a couple other git things that
have my attention. You have pointed out a couple additional nice
fixups, like you always do, but I think at this point I'm just going
to declare you the winner and label these as #leftoverbits.
Thanks for always thoroughly looking over the testcase patches and
your constant work to improve the testsuite.
^ permalink raw reply [flat|nested] 33+ messages in thread