[PATCH] t/t3700: convert two uses of negation operator '!' to use test_must_fail

[PATCH] t/t3700: convert two uses of negation operator '!' to use test_must_fail

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>

These two lines use the negation '!' operator to negate the result of a
simple command.  Since these commands do not contain any pipes or other
complexities, the test_must_fail function can be used and is preferred
since it will additionally detect termination due to a signal.

This was noticed because the second use of '!' does not include a space
between the '!' and the opening parens.  Ksh interprets this as follows:

   !(pattern-list)
      Matches anything except one of the given patterns.

Ksh performs a file glob using the pattern-list and then tries to execute
the first file in the list.  If a space is added between the '!' and the
open parens, then Ksh will not interpret it as a pattern list, but in this
case, it is preferred to use test_must_fail, so lets do so.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 t/t3700-add.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 47fbf53..d03495d 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -268,7 +268,7 @@ test_expect_success 'git add --dry-run of existing changed file' "
 
 test_expect_success 'git add --dry-run of non-existing file' "
 	echo ignored-file >>.gitignore &&
-	! (git add --dry-run track-this ignored-file >actual 2>&1) &&
+	test_must_fail git add --dry-run track-this ignored-file >actual 2>&1 &&
 	echo \"fatal: pathspec 'ignored-file' did not match any files\" | test_cmp - actual
 "
 
@@ -281,7 +281,7 @@ add 'track-this'
 EOF
 
 test_expect_success 'git add --dry-run --ignore-missing of non-existing file' '
-	!(git add --dry-run --ignore-missing track-this ignored-file >actual 2>&1) &&
+	test_must_fail git add --dry-run --ignore-missing track-this ignored-file >actual 2>&1 &&
 	test_cmp expect actual
 '
 
-- 
1.6.6.2

Re: [PATCH] t/t3700: convert two uses of negation operator '!' to use test_must_fail

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 20, 2010 at 15:24, Brandon Casey <casey@nrlssc.navy.mil> wrote:

> These two lines use the negation '!' operator to negate the result of a
> simple command.  Since these commands do not contain any pipes or other
> complexities, the test_must_fail function can be used and is preferred
> since it will additionally detect termination due to a signal.

Maybe I'm missing something, but unless `git add --dry-run` is special
in being killed due to a signal this seems misguided. We actually
prefer to use !, from t/README:

 - test_must_fail <git-command>

   Run a git command and ensure it fails in a controlled way.  Use
   this instead of "! <git-command>" to fail when git commands
   segfault.

> This was noticed because the second use of '!' does not include a space
> between the '!' and the opening parens.  Ksh interprets this as follows:
>
>   !(pattern-list)
>      Matches anything except one of the given patterns.
>
> Ksh performs a file glob using the pattern-list and then tries to execute
> the first file in the list.  If a space is added between the '!' and the
> open parens, then Ksh will not interpret it as a pattern list, but in this
> case, it is preferred to use test_must_fail, so lets do so.

Isn't this a completely seperate thing? Was this test really the only
bit in the test suite that did "!foo" instead of "! foo" ?

Does the test pass for you if you just:

    @@ -281,7 +281,7 @@ add 'track-this'
     EOF

     test_expect_success 'git add --dry-run --ignore-missing of
non-existing file' '
    -       !(git add --dry-run --ignore-missing track-this
ignored-file >actual 2>&1) &&
    +       ! (git add --dry-run --ignore-missing track-this
ignored-file >actual 2>&1) &&
           test_cmp expect actual
     '

?


> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
>  t/t3700-add.sh |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index 47fbf53..d03495d 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -268,7 +268,7 @@ test_expect_success 'git add --dry-run of existing changed file' "
>
>  test_expect_success 'git add --dry-run of non-existing file' "
>        echo ignored-file >>.gitignore &&
> -       ! (git add --dry-run track-this ignored-file >actual 2>&1) &&
> +       test_must_fail git add --dry-run track-this ignored-file >actual 2>&1 &&
>        echo \"fatal: pathspec 'ignored-file' did not match any files\" | test_cmp - actual
>  "
>
> @@ -281,7 +281,7 @@ add 'track-this'
>  EOF
>
>  test_expect_success 'git add --dry-run --ignore-missing of non-existing file' '
> -       !(git add --dry-run --ignore-missing track-this ignored-file >actual 2>&1) &&
> +       test_must_fail git add --dry-run --ignore-missing track-this ignored-file >actual 2>&1 &&
>        test_cmp expect actual
>  '
>
> --
> 1.6.6.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] t/t3700: convert two uses of negation operator '!' to use test_must_fail

[Brandon Casey]

On 07/20/2010 10:55 AM, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Jul 20, 2010 at 15:24, Brandon Casey <casey@nrlssc.navy.mil> wrote:
> 
>> These two lines use the negation '!' operator to negate the result of a
>> simple command.  Since these commands do not contain any pipes or other
>> complexities, the test_must_fail function can be used and is preferred
>> since it will additionally detect termination due to a signal.
> 
> Maybe I'm missing something, but unless `git add --dry-run` is special
> in being killed due to a signal this seems misguided. We actually
> prefer to use !, from t/README:
> 
>  - test_must_fail <git-command>
> 
>    Run a git command and ensure it fails in a controlled way.  Use
>    this instead of "! <git-command>" to fail when git commands
>    segfault.

I think you have misunderstood the explanation of test_must_fail.  The
paragraph you quoted actually recommends using test_must_fail instead
of "! <git-command>".

It says:

   Use this instead of "! <git-command>" to fail when git commands
   segfault.

Or with a slight rewording:

   Use test_must_fail instead of "! <git-command>" since test_must_fail
   will fail when <git-command> segfaults.

See, if "! <git-command>" is used, then if "<git-command>" is terminated
due to some flaw in git (like a segfault), then the statement will still
be interpreted as a success.  When test_must_fail is used, termination
due to segfault or other signal is detected, and the statement will
fail.

>> This was noticed because the second use of '!' does not include a space
>> between the '!' and the opening parens.  Ksh interprets this as follows:
>>
>>   !(pattern-list)
>>      Matches anything except one of the given patterns.
>>
>> Ksh performs a file glob using the pattern-list and then tries to execute
>> the first file in the list.  If a space is added between the '!' and the
>> open parens, then Ksh will not interpret it as a pattern list, but in this
>> case, it is preferred to use test_must_fail, so lets do so.
> 
> Isn't this a completely seperate thing? Was this test really the only
> bit in the test suite that did "!foo" instead of "! foo" ?

This was the only instance of "!()" that was failing for me.  I didn't
look before, but now that I have, there is another instance of "!()" in
t5541 that should be fixed.  t5541 hasn't caused a problem for me because
GIT_TEST_HTTPD must be set in order to enable it, and I haven't done so.

> Does the test pass for you if you just:

Yes.

>     @@ -281,7 +281,7 @@ add 'track-this'
>      EOF
> 
>      test_expect_success 'git add --dry-run --ignore-missing of
> non-existing file' '
>     -       !(git add --dry-run --ignore-missing track-this
> ignored-file >actual 2>&1) &&
>     +       ! (git add --dry-run --ignore-missing track-this
> ignored-file >actual 2>&1) &&
>            test_cmp expect actual
>      '
> 
> ?

Re: [PATCH] t/t3700: convert two uses of negation operator '!' to use test_must_fail

[Jared Hance]

On Tue, Jul 20, 2010 at 11:32:33AM -0500, Brandon Casey wrote:
> I think you have misunderstood the explanation of test_must_fail.  The
> paragraph you quoted actually recommends using test_must_fail instead
> of "! <git-command>".
> 
> It says:
> 
>    Use this instead of "! <git-command>" to fail when git commands
>    segfault.
> 
> Or with a slight rewording:
> 
>    Use test_must_fail instead of "! <git-command>" since test_must_fail
>    will fail when <git-command> segfaults.

I think the wording of description of test_must_fail is slightly
ambiguous. I read it to mean that:

    Use test_must_fail only when you are testing to see if git will
    segfault.

Rather than:
    
    Use test_must_fail to be safe from git segfaults.


Perhaps the description should be updated to be a bit more clear?

[PATCH] t/README: clarify test_must_fail description

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>

Some have found the wording of the description to be somewhat ambiguous
with respect to when it is desirable to use test_must_fail instead of
"! <git-command>".  Tweak the wording somewhat to hopefully clarify that
it is _because_ test_must_fail can detect segmentation fault that it is
desirable to use it instead of "! <git-command>".

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---


On 07/20/2010 11:38 AM, Jared Hance wrote:
> I think the wording of description of test_must_fail is slightly
> ambiguous. I read it to mean that:
> 
>     Use test_must_fail only when you are testing to see if git will
>     segfault.

I think that is a correct interpretation.  But I ask you this:
Are there times when we would _not_ want to test for segfault? :)

> Rather than:
>     
>     Use test_must_fail to be safe from git segfaults.
> 
> 
> Perhaps the description should be updated to be a bit more clear?

How about this?


 t/README |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/README b/t/README
index b906ceb..a830daa 100644
--- a/t/README
+++ b/t/README
@@ -451,8 +451,8 @@ library for your script to use.
  - test_must_fail <git-command>
 
    Run a git command and ensure it fails in a controlled way.  Use
-   this instead of "! <git-command>" to fail when git commands
-   segfault.
+   this instead of "! <git-command>" since it will fail when git
+   commands segfault.
 
  - test_might_fail <git-command>
 
-- 
1.6.6.2

Re: [PATCH] t/README: clarify test_must_fail description

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> From: Brandon Casey <drafnel@gmail.com>
>
> Some have found the wording of the description to be somewhat ambiguous
> with respect to when it is desirable to use test_must_fail instead of
> "! <git-command>".  Tweak the wording somewhat to hopefully clarify that
> it is _because_ test_must_fail can detect segmentation fault that it is
> desirable to use it instead of "! <git-command>".
>
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
>
>
> On 07/20/2010 11:38 AM, Jared Hance wrote:
>> I think the wording of description of test_must_fail is slightly
>> ambiguous. I read it to mean that:
>> 
>>     Use test_must_fail only when you are testing to see if git will
>>     segfault.
>
> I think that is a correct interpretation.  But I ask you this:
> Are there times when we would _not_ want to test for segfault? :)
>
>> Rather than:
>>     
>>     Use test_must_fail to be safe from git segfaults.
>> 
>> 
>> Perhaps the description should be updated to be a bit more clear?
>
> How about this?
>
>
>  t/README |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/README b/t/README
> index b906ceb..a830daa 100644
> --- a/t/README
> +++ b/t/README
> @@ -451,8 +451,8 @@ library for your script to use.
>   - test_must_fail <git-command>
>  
>     Run a git command and ensure it fails in a controlled way.  Use
> -   this instead of "! <git-command>" to fail when git commands
> -   segfault.
> +   this instead of "! <git-command>" since it will fail when git
> +   commands segfault.

I found your earlier

   Use test_must_fail instead of "! <git-command>" since test_must_fail
   will fail when <git-command> segfaults.

slightly clearer.

 - "it" in "since it will fail" is a bit ambiguous: is it "!" or
   "test_must_fail"?

 - it is not obvious if "fail" in "since it will fail" is a good thing or
   a bad thing; as we are discussing test_MUST_fail, it may even be a good
   thing that it "will fail"---which is not what we want our audience to
   read from this.

How about being more explicit?

    Run a git command and ensure it fails in a controlled way.  Use
    this instead of "! <git-command>".  When git-command dies due to a
    segfault, test_must_fail diagnoses it as an error; "! <git-command>"
    treats it as just another expected failure. letting such a bug go
    unnoticed.

>  
>   - test_might_fail <git-command>
>  
> -- 
> 1.6.6.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t/README: clarify test_must_fail description

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 20, 2010 at 18:00, Junio C Hamano <gitster@pobox.com> wrote:

>    Run a git command and ensure it fails in a controlled way.  Use
>    this instead of "! <git-command>".  When git-command dies due to a
>    segfault, test_must_fail diagnoses it as an error; "! <git-command>"
>    treats it as just another expected failure. letting such a bug go
>    unnoticed.

To add to that:

    Don't use test_must_fail to negate the return values of commands
    on the system like grep, sed etc. If we can't trust that the core
    utilities won't randomly segfault we might as well die horribly.

Re: [PATCH] t/README: clarify test_must_fail description

[Jared Hance]

I've written a patch that converted most (all?) of the "! git" to
"test_must_fail git". I'll send it as soon as I make sure I didn't
somehow break the testsuite with it.

[PATCH] Convert "! git" to "test_must_fail" git.

[Jared Hance]

test_must_fail prevents problems with segfault, so it is better to use
rather than "! git".

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---
 t/t1001-read-tree-m-2way.sh                |    2 +-
 t/t3301-notes.sh                           |    6 +++---
 t/t3306-notes-prune.sh                     |    8 ++++----
 t/t3410-rebase-preserve-dropped-merges.sh  |    8 ++++----
 t/t6037-merge-ours-theirs.sh               |    2 +-
 t/t7509-commit.sh                          |    4 ++--
 t/t7607-merge-overwrite.sh                 |   12 ++++++------
 t/t7810-grep.sh                            |    4 ++--
 t/t9100-git-svn-basic.sh                   |    2 +-
 t/t9130-git-svn-authors-file.sh            |    4 ++--
 t/t9139-git-svn-non-utf8-commitencoding.sh |    2 +-
 t/t9140-git-svn-reset.sh                   |    2 +-
 12 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
index 0c562bb..93ca84f 100755
--- a/t/t1001-read-tree-m-2way.sh
+++ b/t/t1001-read-tree-m-2way.sh
@@ -359,7 +359,7 @@ test_expect_success \
 
 test_expect_success \
     'a/b (untracked) vs a, plus c/d case test.' \
-    '! git read-tree -u -m "$treeH" "$treeM" &&
+    'test_must_fail git read-tree -u -m "$treeH" "$treeM" &&
      git ls-files --stage &&
      test -f a/b'
 
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 2d67a40..421c988 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -299,7 +299,7 @@ cat expect-F >> expect-rm-F
 test_expect_success 'verify note removal with -F /dev/null' '
 	git log -4 > output &&
 	test_cmp expect-rm-F output &&
-	! git notes show
+	test_must_fail git notes show
 '
 
 test_expect_success 'do not create empty note with -m "" (setup)' '
@@ -309,7 +309,7 @@ test_expect_success 'do not create empty note with -m "" (setup)' '
 test_expect_success 'verify non-creation of note with -m ""' '
 	git log -4 > output &&
 	test_cmp expect-rm-F output &&
-	! git notes show
+	test_must_fail git notes show
 '
 
 cat > expect-combine_m_and_F << EOF
@@ -357,7 +357,7 @@ cat expect-multiline >> expect-rm-remove
 test_expect_success 'verify note removal with "git notes remove"' '
 	git log -4 > output &&
 	test_cmp expect-rm-remove output &&
-	! git notes show HEAD^
+	test_must_fail git notes show HEAD^
 '
 
 cat > expect << EOF
diff --git a/t/t3306-notes-prune.sh b/t/t3306-notes-prune.sh
index b455404..c428217 100755
--- a/t/t3306-notes-prune.sh
+++ b/t/t3306-notes-prune.sh
@@ -67,7 +67,7 @@ test_expect_success 'remove some commits' '
 
 test_expect_success 'verify that commits are gone' '
 
-	! git cat-file -p 5ee1c35e83ea47cd3cc4f8cbee0568915fbbbd29 &&
+	test_must_fail git cat-file -p 5ee1c35e83ea47cd3cc4f8cbee0568915fbbbd29 &&
 	git cat-file -p 08341ad9e94faa089d60fd3f523affb25c6da189 &&
 	git cat-file -p ab5f302035f2e7aaf04265f08b42034c23256e1f
 '
@@ -106,7 +106,7 @@ test_expect_success 'prune notes' '
 
 test_expect_success 'verify that notes are gone' '
 
-	! git notes show 5ee1c35e83ea47cd3cc4f8cbee0568915fbbbd29 &&
+	test_must_fail git notes show 5ee1c35e83ea47cd3cc4f8cbee0568915fbbbd29 &&
 	git notes show 08341ad9e94faa089d60fd3f523affb25c6da189 &&
 	git notes show ab5f302035f2e7aaf04265f08b42034c23256e1f
 '
@@ -130,8 +130,8 @@ test_expect_success 'prune -v notes' '
 
 test_expect_success 'verify that notes are gone' '
 
-	! git notes show 5ee1c35e83ea47cd3cc4f8cbee0568915fbbbd29 &&
-	! git notes show 08341ad9e94faa089d60fd3f523affb25c6da189 &&
+	test_must_fail git notes show 5ee1c35e83ea47cd3cc4f8cbee0568915fbbbd29 &&
+	test_must_fail git notes show 08341ad9e94faa089d60fd3f523affb25c6da189 &&
 	git notes show ab5f302035f2e7aaf04265f08b42034c23256e1f
 '
 
diff --git a/t/t3410-rebase-preserve-dropped-merges.sh b/t/t3410-rebase-preserve-dropped-merges.sh
index c49143a..6f73b95 100755
--- a/t/t3410-rebase-preserve-dropped-merges.sh
+++ b/t/t3410-rebase-preserve-dropped-merges.sh
@@ -43,11 +43,11 @@ test_expect_success 'setup' '
 # G2 = same changes as G
 test_expect_success 'skip same-resolution merges with -p' '
 	git checkout H &&
-	! git merge E &&
+	test_must_fail git merge E &&
 	test_commit L file1 23 &&
 	git checkout I &&
 	test_commit G2 file1 3 &&
-	! git merge E &&
+	test_must_fail git merge E &&
 	test_commit J file1 23 &&
 	test_commit K file7 file7 &&
 	git rebase -i -p L &&
@@ -65,11 +65,11 @@ test_expect_success 'skip same-resolution merges with -p' '
 # G2 = different changes as G
 test_expect_success 'keep different-resolution merges with -p' '
 	git checkout H &&
-	! git merge E &&
+	test_must_fail git merge E &&
 	test_commit L2 file1 23 &&
 	git checkout I &&
 	test_commit G3 file1 4 &&
-	! git merge E &&
+	test_must_fail git merge E &&
 	test_commit J2 file1 24 &&
 	test_commit K2 file7 file7 &&
 	test_must_fail git rebase -i -p L2 &&
diff --git a/t/t6037-merge-ours-theirs.sh b/t/t6037-merge-ours-theirs.sh
index 8ab3d61..2cf42c7 100755
--- a/t/t6037-merge-ours-theirs.sh
+++ b/t/t6037-merge-ours-theirs.sh
@@ -58,7 +58,7 @@ test_expect_success 'pull with -X' '
 	git reset --hard master && git pull -s recursive -X ours . side &&
 	git reset --hard master && git pull -s recursive -Xtheirs . side &&
 	git reset --hard master && git pull -s recursive -X theirs . side &&
-	git reset --hard master && ! git pull -s recursive -X bork . side
+	git reset --hard master && test_must_fail git pull -s recursive -X bork . side
 '
 
 test_done
diff --git a/t/t7509-commit.sh b/t/t7509-commit.sh
index 3ea33db..643ab03 100755
--- a/t/t7509-commit.sh
+++ b/t/t7509-commit.sh
@@ -111,7 +111,7 @@ test_expect_success '--amend option with empty author' '
 	test_when_finished "git checkout Initial" &&
 	echo "Empty author test" >>foo &&
 	test_tick &&
-	! git commit -a -m "empty author" --amend 2>err &&
+	test_must_fail git commit -a -m "empty author" --amend 2>err &&
 	grep "empty ident" err
 '
 
@@ -125,7 +125,7 @@ test_expect_success '--amend option with missing author' '
 	test_when_finished "git checkout Initial" &&
 	echo "Missing author test" >>foo &&
 	test_tick &&
-	! git commit -a -m "malformed author" --amend 2>err &&
+	test_must_fail git commit -a -m "malformed author" --amend 2>err &&
 	grep "empty ident" err
 '
 
diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh
index 49f4e15..d82349a 100755
--- a/t/t7607-merge-overwrite.sh
+++ b/t/t7607-merge-overwrite.sh
@@ -31,7 +31,7 @@ test_expect_success 'setup' '
 test_expect_success 'will not overwrite untracked file' '
 	git reset --hard c1 &&
 	cat important > c2.c &&
-	! git merge c2 &&
+	test_must_fail git merge c2 &&
 	test_cmp important c2.c
 '
 
@@ -39,7 +39,7 @@ test_expect_success 'will not overwrite new file' '
 	git reset --hard c1 &&
 	cat important > c2.c &&
 	git add c2.c &&
-	! git merge c2 &&
+	test_must_fail git merge c2 &&
 	test_cmp important c2.c
 '
 
@@ -48,7 +48,7 @@ test_expect_success 'will not overwrite staged changes' '
 	cat important > c2.c &&
 	git add c2.c &&
 	rm c2.c &&
-	! git merge c2 &&
+	test_must_fail git merge c2 &&
 	git checkout c2.c &&
 	test_cmp important c2.c
 '
@@ -58,7 +58,7 @@ test_expect_success 'will not overwrite removed file' '
 	git rm c1.c &&
 	git commit -m "rm c1.c" &&
 	cat important > c1.c &&
-	! git merge c1a &&
+	test_must_fail git merge c1a &&
 	test_cmp important c1.c
 '
 
@@ -68,7 +68,7 @@ test_expect_success 'will not overwrite re-added file' '
 	git commit -m "rm c1.c" &&
 	cat important > c1.c &&
 	git add c1.c &&
-	! git merge c1a &&
+	test_must_fail git merge c1a &&
 	test_cmp important c1.c
 '
 
@@ -79,7 +79,7 @@ test_expect_success 'will not overwrite removed file with staged changes' '
 	cat important > c1.c &&
 	git add c1.c &&
 	rm c1.c &&
-	! git merge c1a &&
+	test_must_fail git merge c1a &&
 	git checkout c1.c &&
 	test_cmp important c1.c
 '
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 8a63227..b20edff 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -65,7 +65,7 @@ do
 
 	test_expect_success "grep -w $L (w)" '
 		: >expected &&
-		! git grep -n -w -e "^w" >actual &&
+		test_must_fail git grep -n -w -e "^w" >actual &&
 		test_cmp expected actual
 	'
 
@@ -134,7 +134,7 @@ do
 	'
 
 	test_expect_success "grep -c $L (no /dev/null)" '
-		! git grep -c test $H | grep /dev/null
+		test_must_fail git grep -c test $H | grep /dev/null
         '
 
 	test_expect_success "grep --max-depth -1 $L" '
diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
index 13766ab..9e86e9f 100755
--- a/t/t9100-git-svn-basic.sh
+++ b/t/t9100-git-svn-basic.sh
@@ -245,7 +245,7 @@ test_expect_success 'dcommit $rev does not clobber current branch' '
 	test $old_head = $(git rev-parse HEAD) &&
 	test refs/heads/my-bar = $(git symbolic-ref HEAD) &&
 	git log refs/remotes/bar | grep "change 1" &&
-	! git log refs/remotes/bar | grep "change 2" &&
+	test_must_fail git log refs/remotes/bar | grep "change 2" &&
 	git checkout master &&
 	git branch -D my-bar
 	'
diff --git a/t/t9130-git-svn-authors-file.sh b/t/t9130-git-svn-authors-file.sh
index 134411e..3c4f319 100755
--- a/t/t9130-git-svn-authors-file.sh
+++ b/t/t9130-git-svn-authors-file.sh
@@ -20,7 +20,7 @@ test_expect_success 'setup svnrepo' '
 	'
 
 test_expect_success 'start import with incomplete authors file' '
-	! git svn clone --authors-file=svn-authors "$svnrepo" x
+	test_must_fail git svn clone --authors-file=svn-authors "$svnrepo" x
 	'
 
 test_expect_success 'imported 2 revisions successfully' '
@@ -63,7 +63,7 @@ test_expect_success 'authors-file against globs' '
 	'
 
 test_expect_success 'fetch fails on ee' '
-	( cd aa-work && ! git svn fetch --authors-file=../svn-authors )
+	( cd aa-work && test_must_fail git svn fetch --authors-file=../svn-authors )
 	'
 
 tmp_config_get () {
diff --git a/t/t9139-git-svn-non-utf8-commitencoding.sh b/t/t9139-git-svn-non-utf8-commitencoding.sh
index f337959..22d80b0 100755
--- a/t/t9139-git-svn-non-utf8-commitencoding.sh
+++ b/t/t9139-git-svn-non-utf8-commitencoding.sh
@@ -39,7 +39,7 @@ do
 	(
 		cd $H &&
 		git config --unset i18n.commitencoding &&
-		! git svn dcommit
+		test_must_fail git svn dcommit
 	)
 	'
 done
diff --git a/t/t9140-git-svn-reset.sh b/t/t9140-git-svn-reset.sh
index 0735526..e855904 100755
--- a/t/t9140-git-svn-reset.sh
+++ b/t/t9140-git-svn-reset.sh
@@ -41,7 +41,7 @@ test_expect_success 'modify hidden file in SVN repo' '
 test_expect_success 'fetch fails on modified hidden file' '
 	( cd g &&
 	  git svn find-rev refs/remotes/git-svn > ../expect &&
-	  ! git svn fetch 2> ../errors &&
+	  test_must_fail git svn fetch 2> ../errors &&
 	  git svn find-rev refs/remotes/git-svn > ../expect2 ) &&
 	fgrep "not found in commit" errors &&
 	test_cmp expect expect2
-- 
1.7.1.1

Re: [PATCH] Convert "! git" to "test_must_fail" git.

[Brandon Casey]

Are you sure these are working properly?  If I recall
correctly, test_must_fail does not act the same as '!'
when a '|' is involved.  I believe the '!' negates the
result of the entire statement, but test_must_fail only
operates on the arguments preceding the '|'.

The rest looks good, and I'm surprised there were so many.

On 07/20/2010 02:09 PM, Jared Hance wrote:
> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
> index 8a63227..b20edff 100755
> --- a/t/t7810-grep.sh
> +++ b/t/t7810-grep.sh

> @@ -134,7 +134,7 @@ do
>  	'
>  
>  	test_expect_success "grep -c $L (no /dev/null)" '
> -		! git grep -c test $H | grep /dev/null
> +		test_must_fail git grep -c test $H | grep /dev/null
>          '
>  
>  	test_expect_success "grep --max-depth -1 $L" '
> diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
> index 13766ab..9e86e9f 100755
> --- a/t/t9100-git-svn-basic.sh
> +++ b/t/t9100-git-svn-basic.sh
> @@ -245,7 +245,7 @@ test_expect_success 'dcommit $rev does not clobber current branch' '
>  	test $old_head = $(git rev-parse HEAD) &&
>  	test refs/heads/my-bar = $(git symbolic-ref HEAD) &&
>  	git log refs/remotes/bar | grep "change 1" &&
> -	! git log refs/remotes/bar | grep "change 2" &&
> +	test_must_fail git log refs/remotes/bar | grep "change 2" &&
>  	git checkout master &&
>  	git branch -D my-bar
>  	'

Re: [PATCH] Convert "! git" to "test_must_fail" git.

[Jonathan Nieder]

Brandon Casey wrote:

> I believe the '!' negates the
> result of the entire statement, but test_must_fail only
> operates on the arguments preceding the '|'.

Right.  For example:

> On 07/20/2010 02:09 PM, Jared Hance wrote:

[...]
>> +++ b/t/t7810-grep.sh
>> @@ -134,7 +134,7 @@ do
>>  	'
>>  
>>  	test_expect_success "grep -c $L (no /dev/null)" '
>> -		! git grep -c test $H | grep /dev/null
>> +		test_must_fail git grep -c test $H | grep /dev/null

should be

	git grep -c test $H | ! grep /dev/null

or if it is considered important to test the exit code of
git grep,

	git grep -c test $H >actual &&
	! grep /dev/null actual

Regards,
Jonathan

Re: [PATCH] Convert "! git" to "test_must_fail" git.

[Brandon Casey]

On 07/20/2010 02:48 PM, Jonathan Nieder wrote:
> Brandon Casey wrote:
> 
>> I believe the '!' negates the
>> result of the entire statement, but test_must_fail only
>> operates on the arguments preceding the '|'.
> 
> Right.  For example:
> 
>> On 07/20/2010 02:09 PM, Jared Hance wrote:
> 
> [...]
>>> +++ b/t/t7810-grep.sh
>>> @@ -134,7 +134,7 @@ do
>>>  	'
>>>  
>>>  	test_expect_success "grep -c $L (no /dev/null)" '
>>> -		! git grep -c test $H | grep /dev/null
>>> +		test_must_fail git grep -c test $H | grep /dev/null
> 
> should be
> 
> 	git grep -c test $H | ! grep /dev/null

I seem to recall that that doesn't work either.

   $ /bin/bash -c 'echo foo | ! grep bar && echo success || echo failure'
   /bin/bash: -c: line 0: syntax error near unexpected token `!'
   /bin/bash: -c: line 0: `echo foo | ! grep bar && echo success || echo failure'

-brandon

[PATCH v2] Convert "! git" to "test_must_fail git"

[Jared Hance]

test_must_fail will account for segfaults in git, so it should be used
instead of "! git"

This patch does not change any of the commands that use pipes.

Signed-off-by: Jared Hance <jaredhance@gmail.com>
---
 t/t1001-read-tree-m-2way.sh                |    2 +-
 t/t3301-notes.sh                           |    6 +++---
 t/t3306-notes-prune.sh                     |    8 ++++----
 t/t3410-rebase-preserve-dropped-merges.sh  |    8 ++++----
 t/t6037-merge-ours-theirs.sh               |    2 +-
 t/t7509-commit.sh                          |    4 ++--
 t/t7607-merge-overwrite.sh                 |   12 ++++++------
 t/t7810-grep.sh                            |    2 +-
 t/t9130-git-svn-authors-file.sh            |    4 ++--
 t/t9139-git-svn-non-utf8-commitencoding.sh |    2 +-
 t/t9140-git-svn-reset.sh                   |    2 +-
 11 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
index 0c562bb..93ca84f 100755
--- a/t/t1001-read-tree-m-2way.sh
+++ b/t/t1001-read-tree-m-2way.sh
@@ -359,7 +359,7 @@ test_expect_success \
 
 test_expect_success \
     'a/b (untracked) vs a, plus c/d case test.' \
-    '! git read-tree -u -m "$treeH" "$treeM" &&
+    'test_must_fail git read-tree -u -m "$treeH" "$treeM" &&
      git ls-files --stage &&
      test -f a/b'
 
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 2d67a40..421c988 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -299,7 +299,7 @@ cat expect-F >> expect-rm-F
 test_expect_success 'verify note removal with -F /dev/null' '
 	git log -4 > output &&
 	test_cmp expect-rm-F output &&
-	! git notes show
+	test_must_fail git notes show
 '
 
 test_expect_success 'do not create empty note with -m "" (setup)' '
@@ -309,7 +309,7 @@ test_expect_success 'do not create empty note with -m "" (setup)' '
 test_expect_success 'verify non-creation of note with -m ""' '
 	git log -4 > output &&
 	test_cmp expect-rm-F output &&
-	! git notes show
+	test_must_fail git notes show
 '
 
 cat > expect-combine_m_and_F << EOF
@@ -357,7 +357,7 @@ cat expect-multiline >> expect-rm-remove
 test_expect_success 'verify note removal with "git notes remove"' '
 	git log -4 > output &&
 	test_cmp expect-rm-remove output &&
-	! git notes show HEAD^
+	test_must_fail git notes show HEAD^
 '
 
 cat > expect << EOF
diff --git a/t/t3306-notes-prune.sh b/t/t3306-notes-prune.sh
index b455404..c428217 100755
--- a/t/t3306-notes-prune.sh
+++ b/t/t3306-notes-prune.sh
@@ -67,7 +67,7 @@ test_expect_success 'remove some commits' '
 
 test_expect_success 'verify that commits are gone' '
 
-	! git cat-file -p 5ee1c35e83ea47cd3cc4f8cbee0568915fbbbd29 &&
+	test_must_fail git cat-file -p 5ee1c35e83ea47cd3cc4f8cbee0568915fbbbd29 &&
 	git cat-file -p 08341ad9e94faa089d60fd3f523affb25c6da189 &&
 	git cat-file -p ab5f302035f2e7aaf04265f08b42034c23256e1f
 '
@@ -106,7 +106,7 @@ test_expect_success 'prune notes' '
 
 test_expect_success 'verify that notes are gone' '
 
-	! git notes show 5ee1c35e83ea47cd3cc4f8cbee0568915fbbbd29 &&
+	test_must_fail git notes show 5ee1c35e83ea47cd3cc4f8cbee0568915fbbbd29 &&
 	git notes show 08341ad9e94faa089d60fd3f523affb25c6da189 &&
 	git notes show ab5f302035f2e7aaf04265f08b42034c23256e1f
 '
@@ -130,8 +130,8 @@ test_expect_success 'prune -v notes' '
 
 test_expect_success 'verify that notes are gone' '
 
-	! git notes show 5ee1c35e83ea47cd3cc4f8cbee0568915fbbbd29 &&
-	! git notes show 08341ad9e94faa089d60fd3f523affb25c6da189 &&
+	test_must_fail git notes show 5ee1c35e83ea47cd3cc4f8cbee0568915fbbbd29 &&
+	test_must_fail git notes show 08341ad9e94faa089d60fd3f523affb25c6da189 &&
 	git notes show ab5f302035f2e7aaf04265f08b42034c23256e1f
 '
 
diff --git a/t/t3410-rebase-preserve-dropped-merges.sh b/t/t3410-rebase-preserve-dropped-merges.sh
index c49143a..6f73b95 100755
--- a/t/t3410-rebase-preserve-dropped-merges.sh
+++ b/t/t3410-rebase-preserve-dropped-merges.sh
@@ -43,11 +43,11 @@ test_expect_success 'setup' '
 # G2 = same changes as G
 test_expect_success 'skip same-resolution merges with -p' '
 	git checkout H &&
-	! git merge E &&
+	test_must_fail git merge E &&
 	test_commit L file1 23 &&
 	git checkout I &&
 	test_commit G2 file1 3 &&
-	! git merge E &&
+	test_must_fail git merge E &&
 	test_commit J file1 23 &&
 	test_commit K file7 file7 &&
 	git rebase -i -p L &&
@@ -65,11 +65,11 @@ test_expect_success 'skip same-resolution merges with -p' '
 # G2 = different changes as G
 test_expect_success 'keep different-resolution merges with -p' '
 	git checkout H &&
-	! git merge E &&
+	test_must_fail git merge E &&
 	test_commit L2 file1 23 &&
 	git checkout I &&
 	test_commit G3 file1 4 &&
-	! git merge E &&
+	test_must_fail git merge E &&
 	test_commit J2 file1 24 &&
 	test_commit K2 file7 file7 &&
 	test_must_fail git rebase -i -p L2 &&
diff --git a/t/t6037-merge-ours-theirs.sh b/t/t6037-merge-ours-theirs.sh
index 8ab3d61..2cf42c7 100755
--- a/t/t6037-merge-ours-theirs.sh
+++ b/t/t6037-merge-ours-theirs.sh
@@ -58,7 +58,7 @@ test_expect_success 'pull with -X' '
 	git reset --hard master && git pull -s recursive -X ours . side &&
 	git reset --hard master && git pull -s recursive -Xtheirs . side &&
 	git reset --hard master && git pull -s recursive -X theirs . side &&
-	git reset --hard master && ! git pull -s recursive -X bork . side
+	git reset --hard master && test_must_fail git pull -s recursive -X bork . side
 '
 
 test_done
diff --git a/t/t7509-commit.sh b/t/t7509-commit.sh
index 3ea33db..643ab03 100755
--- a/t/t7509-commit.sh
+++ b/t/t7509-commit.sh
@@ -111,7 +111,7 @@ test_expect_success '--amend option with empty author' '
 	test_when_finished "git checkout Initial" &&
 	echo "Empty author test" >>foo &&
 	test_tick &&
-	! git commit -a -m "empty author" --amend 2>err &&
+	test_must_fail git commit -a -m "empty author" --amend 2>err &&
 	grep "empty ident" err
 '
 
@@ -125,7 +125,7 @@ test_expect_success '--amend option with missing author' '
 	test_when_finished "git checkout Initial" &&
 	echo "Missing author test" >>foo &&
 	test_tick &&
-	! git commit -a -m "malformed author" --amend 2>err &&
+	test_must_fail git commit -a -m "malformed author" --amend 2>err &&
 	grep "empty ident" err
 '
 
diff --git a/t/t7607-merge-overwrite.sh b/t/t7607-merge-overwrite.sh
index 49f4e15..d82349a 100755
--- a/t/t7607-merge-overwrite.sh
+++ b/t/t7607-merge-overwrite.sh
@@ -31,7 +31,7 @@ test_expect_success 'setup' '
 test_expect_success 'will not overwrite untracked file' '
 	git reset --hard c1 &&
 	cat important > c2.c &&
-	! git merge c2 &&
+	test_must_fail git merge c2 &&
 	test_cmp important c2.c
 '
 
@@ -39,7 +39,7 @@ test_expect_success 'will not overwrite new file' '
 	git reset --hard c1 &&
 	cat important > c2.c &&
 	git add c2.c &&
-	! git merge c2 &&
+	test_must_fail git merge c2 &&
 	test_cmp important c2.c
 '
 
@@ -48,7 +48,7 @@ test_expect_success 'will not overwrite staged changes' '
 	cat important > c2.c &&
 	git add c2.c &&
 	rm c2.c &&
-	! git merge c2 &&
+	test_must_fail git merge c2 &&
 	git checkout c2.c &&
 	test_cmp important c2.c
 '
@@ -58,7 +58,7 @@ test_expect_success 'will not overwrite removed file' '
 	git rm c1.c &&
 	git commit -m "rm c1.c" &&
 	cat important > c1.c &&
-	! git merge c1a &&
+	test_must_fail git merge c1a &&
 	test_cmp important c1.c
 '
 
@@ -68,7 +68,7 @@ test_expect_success 'will not overwrite re-added file' '
 	git commit -m "rm c1.c" &&
 	cat important > c1.c &&
 	git add c1.c &&
-	! git merge c1a &&
+	test_must_fail git merge c1a &&
 	test_cmp important c1.c
 '
 
@@ -79,7 +79,7 @@ test_expect_success 'will not overwrite removed file with staged changes' '
 	cat important > c1.c &&
 	git add c1.c &&
 	rm c1.c &&
-	! git merge c1a &&
+	test_must_fail git merge c1a &&
 	git checkout c1.c &&
 	test_cmp important c1.c
 '
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 8a63227..023f225 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -65,7 +65,7 @@ do
 
 	test_expect_success "grep -w $L (w)" '
 		: >expected &&
-		! git grep -n -w -e "^w" >actual &&
+		test_must_fail git grep -n -w -e "^w" >actual &&
 		test_cmp expected actual
 	'
 
diff --git a/t/t9130-git-svn-authors-file.sh b/t/t9130-git-svn-authors-file.sh
index 134411e..3c4f319 100755
--- a/t/t9130-git-svn-authors-file.sh
+++ b/t/t9130-git-svn-authors-file.sh
@@ -20,7 +20,7 @@ test_expect_success 'setup svnrepo' '
 	'
 
 test_expect_success 'start import with incomplete authors file' '
-	! git svn clone --authors-file=svn-authors "$svnrepo" x
+	test_must_fail git svn clone --authors-file=svn-authors "$svnrepo" x
 	'
 
 test_expect_success 'imported 2 revisions successfully' '
@@ -63,7 +63,7 @@ test_expect_success 'authors-file against globs' '
 	'
 
 test_expect_success 'fetch fails on ee' '
-	( cd aa-work && ! git svn fetch --authors-file=../svn-authors )
+	( cd aa-work && test_must_fail git svn fetch --authors-file=../svn-authors )
 	'
 
 tmp_config_get () {
diff --git a/t/t9139-git-svn-non-utf8-commitencoding.sh b/t/t9139-git-svn-non-utf8-commitencoding.sh
index f337959..22d80b0 100755
--- a/t/t9139-git-svn-non-utf8-commitencoding.sh
+++ b/t/t9139-git-svn-non-utf8-commitencoding.sh
@@ -39,7 +39,7 @@ do
 	(
 		cd $H &&
 		git config --unset i18n.commitencoding &&
-		! git svn dcommit
+		test_must_fail git svn dcommit
 	)
 	'
 done
diff --git a/t/t9140-git-svn-reset.sh b/t/t9140-git-svn-reset.sh
index 0735526..e855904 100755
--- a/t/t9140-git-svn-reset.sh
+++ b/t/t9140-git-svn-reset.sh
@@ -41,7 +41,7 @@ test_expect_success 'modify hidden file in SVN repo' '
 test_expect_success 'fetch fails on modified hidden file' '
 	( cd g &&
 	  git svn find-rev refs/remotes/git-svn > ../expect &&
-	  ! git svn fetch 2> ../errors &&
+	  test_must_fail git svn fetch 2> ../errors &&
 	  git svn find-rev refs/remotes/git-svn > ../expect2 ) &&
 	fgrep "not found in commit" errors &&
 	test_cmp expect expect2
-- 
1.7.1.1

Re: [PATCH] t/README: clarify test_must_fail description

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Tue, Jul 20, 2010 at 18:00, Junio C Hamano <gitster@pobox.com> wrote:
>
>>    Run a git command and ensure it fails in a controlled way.  Use
>>    this instead of "! <git-command>".  When git-command dies due to a
>>    segfault, test_must_fail diagnoses it as an error; "! <git-command>"
>>    treats it as just another expected failure. letting such a bug go
>>    unnoticed.
>
> To add to that:
>
>     Don't use test_must_fail to negate the return values of commands
>     on the system like grep, sed etc. If we can't trust that the core
>     utilities won't randomly segfault we might as well die horribly.

I think you are being incoherent.  If we can't trust system "grep" and it
randomly segfaults, then a test:

    git some-command >actual &&
    ! grep string-that-should-not-be-in-the-output actual

would _pass_ when the command segfaults.  I do agree with you that "We
might as well die horribly", and the way you do so is by protecting the
test with test_must_fail, like this:

    git some-command >actual &&
    test_must_fail grep string-that-should-not-be-in-the-output actual

Having said that, as we _do_ trust system tools to a certain degree, we do
not care very deeply about this.  IOW, I wouldn't want to see a patch that
rewrites "! grep" to "test_must_fail grep".

Thanks.

Re: [PATCH] t/README: clarify test_must_fail description

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 20, 2010 at 18:34, Junio C Hamano <gitster@pobox.com> wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Tue, Jul 20, 2010 at 18:00, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>>    Run a git command and ensure it fails in a controlled way.  Use
>>>    this instead of "! <git-command>".  When git-command dies due to a
>>>    segfault, test_must_fail diagnoses it as an error; "! <git-command>"
>>>    treats it as just another expected failure. letting such a bug go
>>>    unnoticed.
>>
>> To add to that:
>>
>>     Don't use test_must_fail to negate the return values of commands
>>     on the system like grep, sed etc. If we can't trust that the core
>>     utilities won't randomly segfault we might as well die horribly.
>
> I think you are being incoherent.  If we can't trust system "grep" and it
> randomly segfaults, then a test:
>
>    git some-command >actual &&
>    ! grep string-that-should-not-be-in-the-output actual
>
> would _pass_ when the command segfaults.  I do agree with you that "We
> might as well die horribly", and the way you do so is by protecting the
> test with test_must_fail, like this:
>
>    git some-command >actual &&
>    test_must_fail grep string-that-should-not-be-in-the-output actual
>
> Having said that, as we _do_ trust system tools to a certain degree, we do
> not care very deeply about this.  IOW, I wouldn't want to see a patch that
> rewrites "! grep" to "test_must_fail grep".

An individual test would pass, yes. But if test or grep are
segfaulting we're going to bail out horribly eventually anyway, so I
don't think it's worth the effort to guard them with test_must_fail,
and I wouldn't write tests to do that. I'd just use !.

That's what we seem to be doing in the tests so far, i.e. test_must_fail
is reserved for git commands only.

Re: [PATCH] t/README: clarify test_must_fail description

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> That's what we seem to be doing in the tests so far, i.e. test_must_fail
> is reserved for git commands only.

test_must_fail relies on conventions for return value that cannot
necessarily be relied on from outside utilities.

Thanks for cleaning up my mess.
Jonathan

Re: [PATCH] t/README: clarify test_must_fail description

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 20, 2010 at 19:16, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ęvar Arnfjörš Bjarmason wrote:
>
>> That's what we seem to be doing in the tests so far, i.e. test_must_fail
>> is reserved for git commands only.
>
> test_must_fail relies on conventions for return value that cannot
> necessarily be relied on from outside utilities.

Right, someone should send a patch for these:

    ack 'test_must_fail (?!git)' *sh

:)

Re: [PATCH] t/README: clarify test_must_fail description

[Brandon Casey]

On 07/20/2010 03:49 PM, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Jul 20, 2010 at 19:16, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Ęvar Arnfjörš Bjarmason wrote:
>>
>>> That's what we seem to be doing in the tests so far, i.e. test_must_fail
>>> is reserved for git commands only.
>> test_must_fail relies on conventions for return value that cannot
>> necessarily be relied on from outside utilities.
> 
> Right, someone should send a patch for these:
> 
>     ack 'test_must_fail (?!git)' *sh
> 
> :)

You joke, but thanks to your prodding, I discovered these broken
tests that should definitely all be fixed:

   $ perl -ne 'm/test_must_fail +[^ ]+=/ && print' *sh

        test_must_fail PAGER= git reflog show delta &&
        test_must_fail PAGER= git reflog show epsilon &&
        test_must_fail PAGER= git reflog show epsilon
        test_must_fail PAGER= git reflog show zeta &&
        test_must_fail PAGER= git reflog show eta &&
        test_must_fail PAGER= git reflog show eta
        test_must_fail PAGER= git reflog show beta
        test_must_fail MSG="yet another note" git notes add -c deadbeef &&

one-shot variable assignment does not work with test_must_fail.

See e2007832552ccea9befed9003580c494f09e666e for an explanation.

-brandon

Re: [PATCH] t/README: clarify test_must_fail description

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 20, 2010 at 21:12, Brandon Casey <casey@nrlssc.navy.mil> wrote:
> On 07/20/2010 03:49 PM, Ævar Arnfjörð Bjarmason wrote:
>> On Tue, Jul 20, 2010 at 19:16, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> Ęvar Arnfjörš Bjarmason wrote:
>>>
>>>> That's what we seem to be doing in the tests so far, i.e. test_must_fail
>>>> is reserved for git commands only.
>>> test_must_fail relies on conventions for return value that cannot
>>> necessarily be relied on from outside utilities.
>>
>> Right, someone should send a patch for these:
>>
>>     ack 'test_must_fail (?!git)' *sh
>>
>> :)
>
> You joke, but thanks to your prodding, I discovered these broken
> tests that should definitely all be fixed:

Oh I'm completely serious, I'm just too lazy to do these myself today :)

>   $ perl -ne 'm/test_must_fail +[^ ]+=/ && print' *sh
>
>        test_must_fail PAGER= git reflog show delta &&
>        test_must_fail PAGER= git reflog show epsilon &&
>        test_must_fail PAGER= git reflog show epsilon
>        test_must_fail PAGER= git reflog show zeta &&
>        test_must_fail PAGER= git reflog show eta &&
>        test_must_fail PAGER= git reflog show eta
>        test_must_fail PAGER= git reflog show beta
>        test_must_fail MSG="yet another note" git notes add -c deadbeef &&
>
> one-shot variable assignment does not work with test_must_fail.
>
> See e2007832552ccea9befed9003580c494f09e666e for an explanation.

Good catch.

[PATCH] t/: work around one-shot variable assignment with test_must_fail

[Brandon Casey]

No time to investigate, but here is an example patch and the
results of running the affected tests.  Looks like reflog may
be creating a reflog when it is not supposed to.

Erick, I added you to cc since you appear to be the author of the tests
in question.

$ ./t2017-checkout-orphan.sh
<snip>
not ok - 8 --orphan does not make reflog when core.logAllRefUpdates = false
#
#               git checkout master &&
#               git config core.logAllRefUpdates false &&
#               git checkout --orphan epsilon &&
#               ! test -f .git/logs/refs/heads/epsilon &&
#               (
#                       PAGER= &&
#                       export PAGER &&
#                       test_must_fail git reflog show epsilon
#               ) &&
#               git commit -m Epsilon &&
#               ! test -f .git/logs/refs/heads/epsilon &&
#               (
#                       PAGER= &&
#                       export PAGER &&
#                       test_must_fail git reflog show epsilon
#               )
#
<snip>

$ ./t3200-branch.sh
<snip>
not ok - 32 checkout -b does not make reflog when core.logAllRefUpdates = false
#
#               git checkout master &&
#               git config core.logAllRefUpdates false &&
#               git checkout -b beta &&
#               ! test -f .git/logs/refs/heads/beta &&
#               (
#                       PAGER= &&
#                       export PAGER &&
#                       test_must_fail git reflog show beta
#               )
#
<snip>


--->8---
From: Brandon Casey <drafnel@gmail.com>

See e2007832552ccea9befed9003580c494f09e666e
---
 t/t2017-checkout-orphan.sh |   36 ++++++++++++++++++++++++++++++------
 t/t3200-branch.sh          |    6 +++++-
 t/t3301-notes.sh           |    6 +++++-
 3 files changed, 40 insertions(+), 8 deletions(-)

diff --git a/t/t2017-checkout-orphan.sh b/t/t2017-checkout-orphan.sh
index be88d4b..81cb393 100755
--- a/t/t2017-checkout-orphan.sh
+++ b/t/t2017-checkout-orphan.sh
@@ -69,7 +69,11 @@ test_expect_success '--orphan makes reflog by default' '
 	git config --unset core.logAllRefUpdates &&
 	git checkout --orphan delta &&
 	! test -f .git/logs/refs/heads/delta &&
-	test_must_fail PAGER= git reflog show delta &&
+	(
+		PAGER= &&
+		export PAGER &&
+		test_must_fail git reflog show delta
+	) &&
 	git commit -m Delta &&
 	test -f .git/logs/refs/heads/delta &&
 	PAGER= git reflog show delta
@@ -80,17 +84,29 @@ test_expect_success '--orphan does not make reflog when core.logAllRefUpdates =
 	git config core.logAllRefUpdates false &&
 	git checkout --orphan epsilon &&
 	! test -f .git/logs/refs/heads/epsilon &&
-	test_must_fail PAGER= git reflog show epsilon &&
+	(
+		PAGER= &&
+		export PAGER &&
+		test_must_fail git reflog show epsilon
+	) &&
 	git commit -m Epsilon &&
 	! test -f .git/logs/refs/heads/epsilon &&
-	test_must_fail PAGER= git reflog show epsilon
+	(
+		PAGER= &&
+		export PAGER &&
+		test_must_fail git reflog show epsilon
+	)
 '
 
 test_expect_success '--orphan with -l makes reflog when core.logAllRefUpdates = false' '
 	git checkout master &&
 	git checkout -l --orphan zeta &&
 	test -f .git/logs/refs/heads/zeta &&
-	test_must_fail PAGER= git reflog show zeta &&
+	(
+		PAGER= &&
+		export PAGER &&
+		test_must_fail git reflog show zeta
+	) &&
 	git commit -m Zeta &&
 	PAGER= git reflog show zeta
 '
@@ -99,10 +115,18 @@ test_expect_success 'giving up --orphan not committed when -l and core.logAllRef
 	git checkout master &&
 	git checkout -l --orphan eta &&
 	test -f .git/logs/refs/heads/eta &&
-	test_must_fail PAGER= git reflog show eta &&
+	(
+		PAGER= &&
+		export PAGER &&
+		test_must_fail git reflog show eta
+	) &&
 	git checkout master &&
 	! test -f .git/logs/refs/heads/eta &&
-	test_must_fail PAGER= git reflog show eta
+	(
+		PAGER= &&
+		export PAGER &&
+		test_must_fail git reflog show eta
+	)
 '
 
 test_expect_success '--orphan is rejected with an existing name' '
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 859b99a..bf7747d 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -237,7 +237,11 @@ test_expect_success 'checkout -b does not make reflog when core.logAllRefUpdates
 	git config core.logAllRefUpdates false &&
 	git checkout -b beta &&
 	! test -f .git/logs/refs/heads/beta &&
-	test_must_fail PAGER= git reflog show beta
+	(
+		PAGER= &&
+		export PAGER &&
+		test_must_fail git reflog show beta
+	)
 '
 
 test_expect_success 'checkout -b with -l makes reflog when core.logAllRefUpdates = false' '
diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 2d67a40..1d82f79 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -693,7 +693,11 @@ test_expect_success 'create note from non-existing note with "git notes add -c"
 	git add a10 &&
 	test_tick &&
 	git commit -m 10th &&
-	test_must_fail MSG="yet another note" git notes add -c deadbeef &&
+	(
+		MSG="yet another note" &&
+		export MSG &&
+		test_must_fail git notes add -c deadbeef
+	) &&
 	test_must_fail git notes list HEAD
 '
 
-- 
1.6.6.2

Re: [PATCH] t/: work around one-shot variable assignment with test_must_fail

[Erick Mattos]

Hi,

2010/7/20 Brandon Casey <casey@nrlssc.navy.mil>
>
> No time to investigate, but here is an example patch and the
> results of running the affected tests.  Looks like reflog may
> be creating a reflog when it is not supposed to.
>
> Erick, I added you to cc since you appear to be the author of the tests
> in question.

Thanks for your kindness.

> $ ./t2017-checkout-orphan.sh
> <snip>
> not ok - 8 --orphan does not make reflog when core.logAllRefUpdates = false
> #
> #               git checkout master &&
> #               git config core.logAllRefUpdates false &&
> #               git checkout --orphan epsilon &&
> #               ! test -f .git/logs/refs/heads/epsilon &&
> #               (
> #                       PAGER= &&
> #                       export PAGER &&
> #                       test_must_fail git reflog show epsilon
> #               ) &&
> #               git commit -m Epsilon &&
> #               ! test -f .git/logs/refs/heads/epsilon &&
> #               (
> #                       PAGER= &&
> #                       export PAGER &&
> #                       test_must_fail git reflog show epsilon
> #               )
> #
> <snip>
>
> $ ./t3200-branch.sh
> <snip>
> not ok - 32 checkout -b does not make reflog when core.logAllRefUpdates = false
> #
> #               git checkout master &&
> #               git config core.logAllRefUpdates false &&
> #               git checkout -b beta &&
> #               ! test -f .git/logs/refs/heads/beta &&
> #               (
> #                       PAGER= &&
> #                       export PAGER &&
> #                       test_must_fail git reflog show beta
> #               )
> #
> <snip>

You have made cosmetic changes which do not do the same as the original.

>
> --->8---
> From: Brandon Casey <drafnel@gmail.com>
>
> See e2007832552ccea9befed9003580c494f09e666e
> ---
>  t/t2017-checkout-orphan.sh |   36 ++++++++++++++++++++++++++++++------
>  t/t3200-branch.sh          |    6 +++++-
>  t/t3301-notes.sh           |    6 +++++-
>  3 files changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/t/t2017-checkout-orphan.sh b/t/t2017-checkout-orphan.sh
> index be88d4b..81cb393 100755
> --- a/t/t2017-checkout-orphan.sh
> +++ b/t/t2017-checkout-orphan.sh
> @@ -69,7 +69,11 @@ test_expect_success '--orphan makes reflog by default' '
>        git config --unset core.logAllRefUpdates &&
>        git checkout --orphan delta &&
>        ! test -f .git/logs/refs/heads/delta &&
> -       test_must_fail PAGER= git reflog show delta &&
> +       (
> +               PAGER= &&
> +               export PAGER &&
> +               test_must_fail git reflog show delta
> +       ) &&
>        git commit -m Delta &&
>        test -f .git/logs/refs/heads/delta &&
>        PAGER= git reflog show delta
> @@ -80,17 +84,29 @@ test_expect_success '--orphan does not make reflog when core.logAllRefUpdates =
>        git config core.logAllRefUpdates false &&
>        git checkout --orphan epsilon &&
>        ! test -f .git/logs/refs/heads/epsilon &&
> -       test_must_fail PAGER= git reflog show epsilon &&
> +       (
> +               PAGER= &&
> +               export PAGER &&
> +               test_must_fail git reflog show epsilon
> +       ) &&
>        git commit -m Epsilon &&
>        ! test -f .git/logs/refs/heads/epsilon &&
> -       test_must_fail PAGER= git reflog show epsilon
> +       (
> +               PAGER= &&
> +               export PAGER &&
> +               test_must_fail git reflog show epsilon
> +       )
>  '
>
>  test_expect_success '--orphan with -l makes reflog when core.logAllRefUpdates = false' '
>        git checkout master &&
>        git checkout -l --orphan zeta &&
>        test -f .git/logs/refs/heads/zeta &&
> -       test_must_fail PAGER= git reflog show zeta &&
> +       (
> +               PAGER= &&
> +               export PAGER &&
> +               test_must_fail git reflog show zeta
> +       ) &&
>        git commit -m Zeta &&
>        PAGER= git reflog show zeta
>  '
> @@ -99,10 +115,18 @@ test_expect_success 'giving up --orphan not committed when -l and core.logAllRef
>        git checkout master &&
>        git checkout -l --orphan eta &&
>        test -f .git/logs/refs/heads/eta &&
> -       test_must_fail PAGER= git reflog show eta &&
> +       (
> +               PAGER= &&
> +               export PAGER &&
> +               test_must_fail git reflog show eta
> +       ) &&
>        git checkout master &&
>        ! test -f .git/logs/refs/heads/eta &&
> -       test_must_fail PAGER= git reflog show eta
> +       (
> +               PAGER= &&
> +               export PAGER &&
> +               test_must_fail git reflog show eta
> +       )
>  '
>
>  test_expect_success '--orphan is rejected with an existing name' '
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 859b99a..bf7747d 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -237,7 +237,11 @@ test_expect_success 'checkout -b does not make reflog when core.logAllRefUpdates
>        git config core.logAllRefUpdates false &&
>        git checkout -b beta &&
>        ! test -f .git/logs/refs/heads/beta &&
> -       test_must_fail PAGER= git reflog show beta
> +       (
> +               PAGER= &&
> +               export PAGER &&
> +               test_must_fail git reflog show beta
> +       )
>  '
>
>  test_expect_success 'checkout -b with -l makes reflog when core.logAllRefUpdates = false' '
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 2d67a40..1d82f79 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -693,7 +693,11 @@ test_expect_success 'create note from non-existing note with "git notes add -c"
>        git add a10 &&
>        test_tick &&
>        git commit -m 10th &&
> -       test_must_fail MSG="yet another note" git notes add -c deadbeef &&
> +       (
> +               MSG="yet another note" &&
> +               export MSG &&
> +               test_must_fail git notes add -c deadbeef
> +       ) &&
>        test_must_fail git notes list HEAD
>  '
>
> --
> 1.6.6.2
>

I don't like this proposed patch because I don't see an improvement
when:
* you change a line to three;
* the original line is clear enough;
* the resulting lines improves nothing;
* it is for cosmetic purposes only inside a script;
* and it uses more resources, even if little more as by creating a
  subshell environment, to do the same.

Best regards

[PATCH] gitweb: clarify search results page when no matching commit found

[Jonathan Nieder]

Erick Mattos wrote:

> I did a search in gitweb
> (http://git.kernel.org/?p=git/git.git;a=summary) by commit
> e2007832552ccea9befed9003580c494f09e666e.
> 
> Looks as gitweb's search is broken and giving false results.

Maybe something like the following would help.

-- 8< --
When searching commits for a string that never occurs, the results
page looks something like this:

	projects / foo.git / search                                     \o/
	summary | ... | tree            [commit] search: [ kfjdkas ] [ ]re
	first ⋅ prev ⋅ next

	Merge branch 'maint'

	Foo: a demonstration project

Without a list of hits to compare it to, the header describing the
commit named by the hash parameter (usually HEAD) may itself look
like a hit.  Add some text (“No match.”) to replace the empty
list of hits to avoid this confusion.

Noticed-by: Erick Mattos <erick.mattos@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index cedc357..a47eed2 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -6522,12 +6522,13 @@ sub git_search {
 			$paging_nav .= " &sdot; next";
 		}
 
-		if ($#commitlist >= 100) {
-		}
-
 		git_print_page_nav('','', $hash,$co{'tree'},$hash, $paging_nav);
 		git_print_header_div('commit', esc_html($co{'title'}), $hash);
-		git_search_grep_body(\@commitlist, 0, 99, $next_link);
+		if ($page == 0 && $#commitlist == 0) {
+			print 'No match.\n';
+		} else {
+			git_search_grep_body(\@commitlist, 0, 99, $next_link);
+		}
 	}
 
 	if ($searchtype eq 'pickaxe') {
-- 

Re: [PATCH] gitweb: clarify search results page when no matching commit found

[Jakub Narebski]

On Wed, 21 Jul 2010, Jonathan Nieder wrote:
> Erick Mattos wrote:
> 
> > I did a search in gitweb
> > (http://git.kernel.org/?p=git/git.git;a=summary) by commit
> > e2007832552ccea9befed9003580c494f09e666e.
> > 
> > Looks as gitweb's search is broken and giving false results.
> 
> Maybe something like the following would help.
> 
> -- 8< --
> When searching commits for a string that never occurs, the results
> page looks something like this:
> 
> 	projects / foo.git / search                                     \o/
> 	summary | ... | tree            [commit] search: [ kfjdkas ] [ ]re
> 	first ⋅ prev ⋅ next
> 
> 	Merge branch 'maint'
> 
> 	Foo: a demonstration project
> 
> Without a list of hits to compare it to, the header describing the
> commit named by the hash parameter (usually HEAD) may itself look
> like a hit.  Add some text (“No match.”) to replace the empty
> list of hits to avoid this confusion.
> 
> Noticed-by: Erick Mattos <erick.mattos@gmail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Very good idea

Acked-by: Jakub Narebski <jnareb@gmail.com>

> ---
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index cedc357..a47eed2 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -6522,12 +6522,13 @@ sub git_search {
>  			$paging_nav .= " &sdot; next";
>  		}
>  
> -		if ($#commitlist >= 100) {
> -		}
> -

P.S. I wonder WTF was that...

-- 
Jakub Narebski
Poland

[PATCH v2] gitweb: clarify search results page when no matching commit found

[Jonathan Nieder]

When searching commits for a string that never occurs, the results
page looks something like this:

	projects / foo.git / search                                 \o/
	summary | ... | tree          [commit] search: [ kfjdkas ] [ ]re
	first ⋅ prev ⋅ next

	Merge branch 'maint'

	Foo: a demonstration project

Without a list of hits to compare it to, the header describing the
commit named by the hash parameter (usually HEAD) may itself look
like a hit.  Add some text (“No match.”) to replace the empty
list of hits to avoid this confusion.

While at it, remove some nearby dead code, left behind from a
simplification a few years ago (v1.5.4-rc0~276^2~4, 2007-11-01).

Noticed-by: Erick Mattos <erick.mattos@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Acked-by: Jakub Narebski <jnareb@gmail.com>
---
Jakub Narebski wrote:

> Very good idea

Shoddy implementation, though.  Apparently (yes, I should know this;
and testing reminded me before I sent the wrong patch) the index of
the last element ($#array) of an empty array is -1, not 0.

Sorry about that.

 gitweb/gitweb.perl |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index cedc357..801575d 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -6522,12 +6522,13 @@ sub git_search {
 			$paging_nav .= " &sdot; next";
 		}
 
-		if ($#commitlist >= 100) {
-		}
-
 		git_print_page_nav('','', $hash,$co{'tree'},$hash, $paging_nav);
 		git_print_header_div('commit', esc_html($co{'title'}), $hash);
-		git_search_grep_body(\@commitlist, 0, 99, $next_link);
+		if ($page == 0 && !@commitlist) {
+			print "<p>No match.\n</p>";
+		} else {
+			git_search_grep_body(\@commitlist, 0, 99, $next_link);
+		}
 	}
 
 	if ($searchtype eq 'pickaxe') {
-- 

Re: [PATCH] t/: work around one-shot variable assignment with test_must_fail

[Brandon Casey]

On 07/20/2010 06:19 PM, Erick Mattos wrote:
> 2010/7/20 Brandon Casey <casey@nrlssc.navy.mil>
>> No time to investigate, but here is an example patch and the
>> results of running the affected tests.

> You have made cosmetic changes which do not do the same as the original.

Nope, look closer.  The changes are not cosmetic.

Try this:

   run_it () { "$@"; }; run_it foo= true && echo success || echo failure

You probably get something like this (if you're using bash):

   bash: foo=: command not found

That's because the one-shot variable assignment doesn't work when
used like this.  It also means that the original tests which do:

   test_must_fail PAGER= git ...

are broken.  We ran into this problem a while back and fixed it in
the commit that I referenced (e2007832).  I fixed the new instances
in t2017, t3200, and t3301 in the patch that I sent.

For the tests in t2017 and t3200 that now fail, the originals seem to
expect 'git reflog show' to return non-zero when asked to show the reflog
for a ref which doesn't have a log.  reflog does not currently return
non-zero in this case.  Either the tests should be updated to reflect
the actual behavior of 'reflog show', or 'reflog show' should be updated
to return non-zero when passed a ref without a log.

-brandon

>> --->8---
>> From: Brandon Casey <drafnel@gmail.com>
>>
>> See e2007832552ccea9befed9003580c494f09e666e
>> ---
>>  t/t2017-checkout-orphan.sh |   36 ++++++++++++++++++++++++++++++------
>>  t/t3200-branch.sh          |    6 +++++-
>>  t/t3301-notes.sh           |    6 +++++-
>>  3 files changed, 40 insertions(+), 8 deletions(-)
>>
>> diff --git a/t/t2017-checkout-orphan.sh b/t/t2017-checkout-orphan.sh
>> index be88d4b..81cb393 100755
>> --- a/t/t2017-checkout-orphan.sh
>> +++ b/t/t2017-checkout-orphan.sh

>> @@ -80,17 +84,29 @@ test_expect_success '--orphan does not make reflog when core.logAllRefUpdates =
>>        git config core.logAllRefUpdates false &&
>>        git checkout --orphan epsilon &&
>>        ! test -f .git/logs/refs/heads/epsilon &&
>> -       test_must_fail PAGER= git reflog show epsilon &&
>> +       (
>> +               PAGER= &&
>> +               export PAGER &&
>> +               test_must_fail git reflog show epsilon
>> +       ) &&
>>        git commit -m Epsilon &&
>>        ! test -f .git/logs/refs/heads/epsilon &&
>> -       test_must_fail PAGER= git reflog show epsilon
>> +       (
>> +               PAGER= &&
>> +               export PAGER &&
>> +               test_must_fail git reflog show epsilon
>> +       )
>>  '

>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>> index 859b99a..bf7747d 100755
>> --- a/t/t3200-branch.sh
>> +++ b/t/t3200-branch.sh
>> @@ -237,7 +237,11 @@ test_expect_success 'checkout -b does not make reflog when core.logAllRefUpdates
>>        git config core.logAllRefUpdates false &&
>>        git checkout -b beta &&
>>        ! test -f .git/logs/refs/heads/beta &&
>> -       test_must_fail PAGER= git reflog show beta
>> +       (
>> +               PAGER= &&
>> +               export PAGER &&
>> +               test_must_fail git reflog show beta
>> +       )
>>  '

Re: [PATCH] t/: work around one-shot variable assignment with test_must_fail

[Erick Mattos]

Hi,

2010/7/21 Brandon Casey <casey@nrlssc.navy.mil>:
> On 07/20/2010 06:19 PM, Erick Mattos wrote:
>> 2010/7/20 Brandon Casey <casey@nrlssc.navy.mil>
>>> No time to investigate, but here is an example patch and the
>>> results of running the affected tests.
>
>> You have made cosmetic changes which do not do the same as the original.
>?
> Nope, look closer.  The changes are not cosmetic.

Now I see it.  I was not completely aware of the problem, only of your
email.  Maybe I should subscribe to the list at last... ;-D

> Try this:
>
>   run_it () { "$@"; }; run_it foo= true && echo success || echo failure
>
> You probably get something like this (if you're using bash):
>
>   bash: foo=: command not found

It would work if it was like:

run_it () { eval "$@"; }; run_it "foo= true" && echo success || echo failure

But I think your approach is better shaped for a fast solution.

> That's because the one-shot variable assignment doesn't work when
> used like this.  It also means that the original tests which do:
>
>   test_must_fail PAGER= git ...
>
> are broken.  We ran into this problem a while back and fixed it in
> the commit that I referenced (e2007832).  I fixed the new instances
> in t2017, t3200, and t3301 in the patch that I sent.
>
> For the tests in t2017 and t3200 that now fail, the originals seem to
> expect 'git reflog show' to return non-zero when asked to show the reflog
> for a ref which doesn't have a log.  reflog does not currently return
> non-zero in this case.  Either the tests should be updated to reflect
> the actual behavior of 'reflog show', or 'reflog show' should be updated
> to return non-zero when passed a ref without a log.

So fixing it is a need.

On t2017 and t3200 the problem that appeared was because no message
and error had been generated when there is no reflog.  Git reflog when
run on a ref with a nonexistent reflog file exits with 0 saying
nothing.

I don't see this as a correct behavior but as those tests were just to
enforce the previous "! test -f..." test which is already enough for
checking the intended behavior then I would think it was good enough
just to wipe out the "problematic git reflog" commands until deciding
how quiet git reflog command should be when there is no reflog file to
show.

Although I just realized Junio sent an email following this thread and
I bet he could give a better solution or his directions.  Going to
read it now.

Thanks for your clarifications.

Regards

Re: [PATCH] t/: work around one-shot variable assignment with test_must_fail

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 20, 2010 at 21:55, Brandon Casey <casey@nrlssc.navy.mil> wrote:

> -       test_must_fail PAGER= git reflog show delta &&
> +       (
> +               PAGER= &&
> +               export PAGER &&
> +               test_must_fail git reflog show delta
> +       ) &&

You must use "export PAGER;", not "export PAGER &&". export doesn't
return zero on all systems when exporting, see previous changes in
this regard in t/.

Re: [PATCH] t/: work around one-shot variable assignment with test_must_fail

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 20, 2010 at 23:44, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Tue, Jul 20, 2010 at 21:55, Brandon Casey <casey@nrlssc.navy.mil> wrote:
>
>> -       test_must_fail PAGER= git reflog show delta &&
>> +       (
>> +               PAGER= &&
>> +               export PAGER &&
>> +               test_must_fail git reflog show delta
>> +       ) &&
>
> You must use "export PAGER;", not "export PAGER &&". export doesn't
> return zero on all systems when exporting, see previous changes in
> this regard in t/.

Actually, see the t/README docs which explicitly mention this. Yay docs.

Re: [PATCH] t/: work around one-shot variable assignment with test_must_fail

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> You must use "export PAGER;", not "export PAGER &&". export doesn't
> return zero on all systems when exporting, see previous changes in
> this regard in t/.

Nope.  Sorry I missed this before.

diff --git a/t/README b/t/README
index b906ceb..f81998b 100644
--- a/t/README
+++ b/t/README
@@ -259,11 +259,11 @@ Do:
 	test ...
 
    That way all of the commands in your tests will succeed or fail. If
-   you must ignore the return value of something (e.g. the return
-   value of export is unportable) it's best to indicate so explicitly
-   with a semicolon:
+   you must ignore the return value of something (e.g., the return
+   after unsetting a variable that was already unset is unportable) it's
+   best to indicate so explicitly with a semicolon:
 
-	export HLAGH;
+	unset HLAGH;
 	git merge hla &&
 	git push gh &&
 	test ...
-- 

Re: [PATCH] t/: work around one-shot variable assignment with test_must_fail

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 21, 2010 at 00:01, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> You must use "export PAGER;", not "export PAGER &&". export doesn't
>> return zero on all systems when exporting, see previous changes in
>> this regard in t/.
>
> Nope.  Sorry I missed this before.
>
> diff --git a/t/README b/t/README
> index b906ceb..f81998b 100644
> --- a/t/README
> +++ b/t/README
> @@ -259,11 +259,11 @@ Do:
>        test ...
>
>    That way all of the commands in your tests will succeed or fail. If
> -   you must ignore the return value of something (e.g. the return
> -   value of export is unportable) it's best to indicate so explicitly
> -   with a semicolon:
> +   you must ignore the return value of something (e.g., the return
> +   after unsetting a variable that was already unset is unportable) it's
> +   best to indicate so explicitly with a semicolon:

We should have examples for both export and unset, but the prose
should mention both IMO

Re: [PATCH] t/: work around one-shot variable assignment with test_must_fail

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> We should have examples for both export and unset

What is unportable for “export” is the effect of exporting an unset
variable.  I am not even sure whether the return value is unportable,
but it doesn’t matter; that is an example of a “Don’t” rather than a
“Do it this way”.

See v1.5.6-rc0~61 (filter-branch: fix variable export logic,
2008-05-13) for an example.

> but the prose
> should mention both IMO

Yes, thanks for putting this portability guide together.

Jonathan

Re: [PATCH] t/: work around one-shot variable assignment with test_must_fail

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 21, 2010 at 00:14, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> We should have examples for both export and unset
>
> What is unportable for “export” is the effect of exporting an unset
> variable.  I am not even sure whether the return value is unportable,
> but it doesn’t matter; that is an example of a “Don’t” rather than a
> “Do it this way”.

I didn't know that. It'd be good if it were mentioned in the docs. I
thought it was just export in general.

> See v1.5.6-rc0~61 (filter-branch: fix variable export logic,
> 2008-05-13) for an example.

As an aside, how do you make these 61-commits-after-rc0 commit ids. Is
there a sha1->that tool that I haven't spotted?

>> but the prose
>> should mention both IMO
>
> Yes, thanks for putting this portability guide together.

Thanks for improving it, this change is a definite improvement, I just
had a "wait, what's the export doing there then?" question when
reading it.

git name-rev for fun and profit (Re: [PATCH] t/: work around one-shot variable assignment with test_must_fail)

[Jonathan Nieder]

Ævar Arnfjörð Bjarmason wrote:

> As an aside, how do you make these 61-commits-after-rc0 commit ids. Is
> there a sha1->that tool that I haven't spotted?

 GIT_PAGER_IN_USE=1 git log --all-match --grep=shell --grep=export |
 git -p name-rev --tags --stdin

v1.5.6-rc0~61 is 61 commits before rc0 (well, the 61st-generation
grandparent using the first parent where there is a choice).  The
distinction matters because “61 commits _after_ 1.5.6-rc0” is not
well-defined.

If you want the latter sort of description anyway, ‘git describe’
might help.

Re: git name-rev for fun and profit (Re: [PATCH] t/: work around one-shot variable assignment with test_must_fail)

[Ævar Arnfjörð Bjarmason]

On Wed, Jul 21, 2010 at 01:05, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> As an aside, how do you make these 61-commits-after-rc0 commit ids. Is
>> there a sha1->that tool that I haven't spotted?
>
>  GIT_PAGER_IN_USE=1 git log --all-match --grep=shell --grep=export |
>  git -p name-rev --tags --stdin
>
> v1.5.6-rc0~61 is 61 commits before rc0 (well, the 61st-generation
> grandparent using the first parent where there is a choice).  The
> distinction matters because “61 commits _after_ 1.5.6-rc0” is not
> well-defined.

Thanks, added that as an alias in my .gitconfig:

        abbrev-commit = "!f() { git name-rev --tags \"$@\" | sed
's|.*tags/||;s|\\([0-9a-f]\\{7\\}\\).*|\\1|'; }; f"

> If you want the latter sort of description anyway, ‘git describe’
> might help.

git describe was actually more like what I wanted. I'd used it before,
but forgotten it. Thanks.

Re: [PATCH] t/: work around one-shot variable assignment with test_must_fail

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> No time to investigate, but here is an example patch and the
> results of running the affected tests.  Looks like reflog may
> be creating a reflog when it is not supposed to.

Your later analysis is correct; "git reflog show <branch>" does not
complain when there is no reflog for <branch>, which might or might not be
a bug.

Because these tests are not about behaviour of "git reflog show" command,
let's do this for now.

Thanks.

-- >8 --
Subject: tests: correct "does reflog exist" tests

These two tests were not about how "git reflog show <branch>" exits when
there is no reflog, but were about whether "checkout" and "branch" create
or not create reflog when creating a new <branch>, update the tests to
check it in a more direct way, namely using "git rev-parse --verify".

Also lose tests based on "test -f .git/logs/refs/heads/<branch>" from
nearby, to avoid exposing this particular implementation detail
unnecessarily.

---
 t/t2017-checkout-orphan.sh |   47 +++++++------------------------------------
 t/t3200-branch.sh          |   13 ++---------
 2 files changed, 11 insertions(+), 49 deletions(-)

diff --git a/t/t2017-checkout-orphan.sh b/t/t2017-checkout-orphan.sh
index 81cb393..2d2f63f 100755
--- a/t/t2017-checkout-orphan.sh
+++ b/t/t2017-checkout-orphan.sh
@@ -68,65 +68,34 @@ test_expect_success '--orphan makes reflog by default' '
 	git checkout master &&
 	git config --unset core.logAllRefUpdates &&
 	git checkout --orphan delta &&
-	! test -f .git/logs/refs/heads/delta &&
-	(
-		PAGER= &&
-		export PAGER &&
-		test_must_fail git reflog show delta
-	) &&
+	test_must_fail git rev-parse --verify delta@{0} &&
 	git commit -m Delta &&
-	test -f .git/logs/refs/heads/delta &&
-	PAGER= git reflog show delta
+	git rev-parse --verify delta@{0}
 '
 
 test_expect_success '--orphan does not make reflog when core.logAllRefUpdates = false' '
 	git checkout master &&
 	git config core.logAllRefUpdates false &&
 	git checkout --orphan epsilon &&
-	! test -f .git/logs/refs/heads/epsilon &&
-	(
-		PAGER= &&
-		export PAGER &&
-		test_must_fail git reflog show epsilon
-	) &&
+	test_must_fail git rev-parse --verify epsilon@{0} &&
 	git commit -m Epsilon &&
-	! test -f .git/logs/refs/heads/epsilon &&
-	(
-		PAGER= &&
-		export PAGER &&
-		test_must_fail git reflog show epsilon
-	)
+	test_must_fail git rev-parse --verify epsilon@{0}
 '
 
 test_expect_success '--orphan with -l makes reflog when core.logAllRefUpdates = false' '
 	git checkout master &&
 	git checkout -l --orphan zeta &&
-	test -f .git/logs/refs/heads/zeta &&
-	(
-		PAGER= &&
-		export PAGER &&
-		test_must_fail git reflog show zeta
-	) &&
+	test_must_fail git rev-parse --verify zeta@{0} &&
 	git commit -m Zeta &&
-	PAGER= git reflog show zeta
+	git rev-parse --verify zeta@{0}
 '
 
 test_expect_success 'giving up --orphan not committed when -l and core.logAllRefUpdates = false deletes reflog' '
 	git checkout master &&
 	git checkout -l --orphan eta &&
-	test -f .git/logs/refs/heads/eta &&
-	(
-		PAGER= &&
-		export PAGER &&
-		test_must_fail git reflog show eta
-	) &&
+	test_must_fail git rev-parse --verify eta@{0} &&
 	git checkout master &&
-	! test -f .git/logs/refs/heads/eta &&
-	(
-		PAGER= &&
-		export PAGER &&
-		test_must_fail git reflog show eta
-	)
+	test_must_fail git rev-parse --verify eta@{0}
 '
 
 test_expect_success '--orphan is rejected with an existing name' '
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index bf7747d..f54a533 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -228,28 +228,21 @@ test_expect_success 'checkout -b makes reflog by default' '
 	git checkout master &&
 	git config --unset core.logAllRefUpdates &&
 	git checkout -b alpha &&
-	test -f .git/logs/refs/heads/alpha &&
-	PAGER= git reflog show alpha
+	git rev-parse --verify alpha@{0}
 '
 
 test_expect_success 'checkout -b does not make reflog when core.logAllRefUpdates = false' '
 	git checkout master &&
 	git config core.logAllRefUpdates false &&
 	git checkout -b beta &&
-	! test -f .git/logs/refs/heads/beta &&
-	(
-		PAGER= &&
-		export PAGER &&
-		test_must_fail git reflog show beta
-	)
+	test_must_fail git rev-parse --verify beta@{0}
 '
 
 test_expect_success 'checkout -b with -l makes reflog when core.logAllRefUpdates = false' '
 	git checkout master &&
 	git checkout -lb gamma &&
 	git config --unset core.logAllRefUpdates &&
-	test -f .git/logs/refs/heads/gamma &&
-	PAGER= git reflog show gamma
+	git rev-parse --verify gamma@{0}
 '
 
 test_expect_success 'avoid ambiguous track' '

Re: [PATCH] t/: work around one-shot variable assignment with test_must_fail

[Erick Mattos]

Hi,

2010/7/21 Junio C Hamano <gitster@pobox.com>:
> Brandon Casey <casey@nrlssc.navy.mil> writes:
>
>> No time to investigate, but here is an example patch and the
>> results of running the affected tests.  Looks like reflog may
>> be creating a reflog when it is not supposed to.
>
> Your later analysis is correct; "git reflog show <branch>" does not
> complain when there is no reflog for <branch>, which might or might not be
> a bug.

I think is not a bug but almost one.

> Because these tests are not about behaviour of "git reflog show" command,
> let's do this for now.
>
> Thanks.
>
> -- >8 --
> Subject: tests: correct "does reflog exist" tests
>
> These two tests were not about how "git reflog show <branch>" exits when
> there is no reflog, but were about whether "checkout" and "branch" create
> or not create reflog when creating a new <branch>, update the tests to
> check it in a more direct way, namely using "git rev-parse --verify".
>
> Also lose tests based on "test -f .git/logs/refs/heads/<branch>" from
> nearby, to avoid exposing this particular implementation detail
> unnecessarily.

Quite pretty.  Clean and optimized solution.  Better this way.

Regards

Re: [PATCH] t/: work around one-shot variable assignment with test_must_fail

[Brandon Casey]

btw, this works and is a definite improvement.

-Brandon

Junio C Hamano wrote:
> Brandon Casey <casey@nrlssc.navy.mil> writes:
> 
>> No time to investigate, but here is an example patch and the
>> results of running the affected tests.  Looks like reflog may
>> be creating a reflog when it is not supposed to.
> 
> Your later analysis is correct; "git reflog show <branch>" does not
> complain when there is no reflog for <branch>, which might or might not be
> a bug.
> 
> Because these tests are not about behaviour of "git reflog show" command,
> let's do this for now.
> 
> Thanks.
> 
> -- >8 --
> Subject: tests: correct "does reflog exist" tests
> 
> These two tests were not about how "git reflog show <branch>" exits when
> there is no reflog, but were about whether "checkout" and "branch" create
> or not create reflog when creating a new <branch>, update the tests to
> check it in a more direct way, namely using "git rev-parse --verify".
> 
> Also lose tests based on "test -f .git/logs/refs/heads/<branch>" from
> nearby, to avoid exposing this particular implementation detail
> unnecessarily.
> 
> ---
>  t/t2017-checkout-orphan.sh |   47 +++++++------------------------------------
>  t/t3200-branch.sh          |   13 ++---------
>  2 files changed, 11 insertions(+), 49 deletions(-)
> 
> diff --git a/t/t2017-checkout-orphan.sh b/t/t2017-checkout-orphan.sh
> index 81cb393..2d2f63f 100755
> --- a/t/t2017-checkout-orphan.sh
> +++ b/t/t2017-checkout-orphan.sh
> @@ -68,65 +68,34 @@ test_expect_success '--orphan makes reflog by default' '
>  	git checkout master &&
>  	git config --unset core.logAllRefUpdates &&
>  	git checkout --orphan delta &&
> -	! test -f .git/logs/refs/heads/delta &&
> -	(
> -		PAGER= &&
> -		export PAGER &&
> -		test_must_fail git reflog show delta
> -	) &&
> +	test_must_fail git rev-parse --verify delta@{0} &&
>  	git commit -m Delta &&
> -	test -f .git/logs/refs/heads/delta &&
> -	PAGER= git reflog show delta
> +	git rev-parse --verify delta@{0}
>  '
>  
>  test_expect_success '--orphan does not make reflog when core.logAllRefUpdates = false' '
>  	git checkout master &&
>  	git config core.logAllRefUpdates false &&
>  	git checkout --orphan epsilon &&
> -	! test -f .git/logs/refs/heads/epsilon &&
> -	(
> -		PAGER= &&
> -		export PAGER &&
> -		test_must_fail git reflog show epsilon
> -	) &&
> +	test_must_fail git rev-parse --verify epsilon@{0} &&
>  	git commit -m Epsilon &&
> -	! test -f .git/logs/refs/heads/epsilon &&
> -	(
> -		PAGER= &&
> -		export PAGER &&
> -		test_must_fail git reflog show epsilon
> -	)
> +	test_must_fail git rev-parse --verify epsilon@{0}
>  '
>  
>  test_expect_success '--orphan with -l makes reflog when core.logAllRefUpdates = false' '
>  	git checkout master &&
>  	git checkout -l --orphan zeta &&
> -	test -f .git/logs/refs/heads/zeta &&
> -	(
> -		PAGER= &&
> -		export PAGER &&
> -		test_must_fail git reflog show zeta
> -	) &&
> +	test_must_fail git rev-parse --verify zeta@{0} &&
>  	git commit -m Zeta &&
> -	PAGER= git reflog show zeta
> +	git rev-parse --verify zeta@{0}
>  '
>  
>  test_expect_success 'giving up --orphan not committed when -l and core.logAllRefUpdates = false deletes reflog' '
>  	git checkout master &&
>  	git checkout -l --orphan eta &&
> -	test -f .git/logs/refs/heads/eta &&
> -	(
> -		PAGER= &&
> -		export PAGER &&
> -		test_must_fail git reflog show eta
> -	) &&
> +	test_must_fail git rev-parse --verify eta@{0} &&
>  	git checkout master &&
> -	! test -f .git/logs/refs/heads/eta &&
> -	(
> -		PAGER= &&
> -		export PAGER &&
> -		test_must_fail git reflog show eta
> -	)
> +	test_must_fail git rev-parse --verify eta@{0}
>  '
>  
>  test_expect_success '--orphan is rejected with an existing name' '
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index bf7747d..f54a533 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -228,28 +228,21 @@ test_expect_success 'checkout -b makes reflog by default' '
>  	git checkout master &&
>  	git config --unset core.logAllRefUpdates &&
>  	git checkout -b alpha &&
> -	test -f .git/logs/refs/heads/alpha &&
> -	PAGER= git reflog show alpha
> +	git rev-parse --verify alpha@{0}
>  '
>  
>  test_expect_success 'checkout -b does not make reflog when core.logAllRefUpdates = false' '
>  	git checkout master &&
>  	git config core.logAllRefUpdates false &&
>  	git checkout -b beta &&
> -	! test -f .git/logs/refs/heads/beta &&
> -	(
> -		PAGER= &&
> -		export PAGER &&
> -		test_must_fail git reflog show beta
> -	)
> +	test_must_fail git rev-parse --verify beta@{0}
>  '
>  
>  test_expect_success 'checkout -b with -l makes reflog when core.logAllRefUpdates = false' '
>  	git checkout master &&
>  	git checkout -lb gamma &&
>  	git config --unset core.logAllRefUpdates &&
> -	test -f .git/logs/refs/heads/gamma &&
> -	PAGER= git reflog show gamma
> +	git rev-parse --verify gamma@{0}
>  '
>  
>  test_expect_success 'avoid ambiguous track' '

Re: [PATCH] t/README: clarify test_must_fail description

[Brandon Casey]

On 07/20/2010 01:00 PM, Junio C Hamano wrote:

> How about being more explicit?
> 
>     Run a git command and ensure it fails in a controlled way.  Use
>     this instead of "! <git-command>".  When git-command dies due to a
>     segfault, test_must_fail diagnoses it as an error; "! <git-command>"
>     treats it as just another expected failure. letting such a bug go
>     unnoticed.

Much better.  If you want the last sentence to be grammatically correct,
you could rephrase it like this:

   ...just another expected failure, which would let such a bug go unnoticed.

-brandon

Re: [PATCH] t/t3700: convert two uses of negation operator '!' to use test_must_fail

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 20, 2010 at 16:38, Jared Hance <jaredhance@gmail.com> wrote:
> On Tue, Jul 20, 2010 at 11:32:33AM -0500, Brandon Casey wrote:
>> I think you have misunderstood the explanation of test_must_fail.  The
>> paragraph you quoted actually recommends using test_must_fail instead
>> of "! <git-command>".
>>
>> It says:
>>
>>    Use this instead of "! <git-command>" to fail when git commands
>>    segfault.
>>
>> Or with a slight rewording:
>>
>>    Use test_must_fail instead of "! <git-command>" since test_must_fail
>>    will fail when <git-command> segfaults.
>
> I think the wording of description of test_must_fail is slightly
> ambiguous. I read it to mean that:
>
>    Use test_must_fail only when you are testing to see if git will
>    segfault.

I've interpreted it to mean that as well, but it's starting to look
like a good example of a garden path sentence.

Anyway, it looks like we're wrong and Brandon was right. But I'm going
to submit a doc patch to t/README. Here's the existing use of !
v.s. test_must_fail:

    $ cat *.sh | grep -c 'test_must_fail git'
    863
    $ cat *sh | grep -c '! git '
    30

I.e. ! is only used to negate non-git commands like test.

The example in the comments for test_must_fail in test-lib.sh backs this up:

    # Writing this as "! git checkout ../outerspace" is wrong, because
    # the failure could be due to a segv.  We want a controlled failure.

> Rather than:
>
>    Use test_must_fail to be safe from git segfaults.

Re: [PATCH] t/t3700: convert two uses of negation operator '!' to use test_must_fail

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 20, 2010 at 16:32, Brandon Casey <casey@nrlssc.navy.mil> wrote:

>>> This was noticed because the second use of '!' does not include a space
>>> between the '!' and the opening parens.  Ksh interprets this as follows:
>>>
>>>   !(pattern-list)
>>>      Matches anything except one of the given patterns.
>>>
>>> Ksh performs a file glob using the pattern-list and then tries to execute
>>> the first file in the list.  If a space is added between the '!' and the
>>> open parens, then Ksh will not interpret it as a pattern list, but in this
>>> case, it is preferred to use test_must_fail, so lets do so.
>>
>> Isn't this a completely seperate thing? Was this test really the only
>> bit in the test suite that did "!foo" instead of "! foo" ?
>
> This was the only instance of "!()" that was failing for me.  I didn't
> look before, but now that I have, there is another instance of "!()" in
> t5541 that should be fixed.  t5541 hasn't caused a problem for me because
> GIT_TEST_HTTPD must be set in order to enable it, and I haven't done so.
>
>> Does the test pass for you if you just:
>
> Yes.
>
>>     @@ -281,7 +281,7 @@ add 'track-this'
>>      EOF
>>
>>      test_expect_success 'git add --dry-run --ignore-missing of
>> non-existing file' '
>>     -       !(git add --dry-run --ignore-missing track-this
>> ignored-file >actual 2>&1) &&
>>     +       ! (git add --dry-run --ignore-missing track-this
>> ignored-file >actual 2>&1) &&
>>            test_cmp expect actual
>>      '

A seperate patch to clean up those two + doc patch to the "Do's,
don'ts & things to keep in mind" section in t/README would be really
useful IMO. I had no idea that "!cmd" was an issue no ksh.