patcht/t3700: convert two uses of negation operator '!' to use test_must_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.

> 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
>

> 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" ?

>     @@ -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
>      '
> 
> ?

> 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?

> 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
>
> --
> 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

>    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.

> @@ -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
>  	'

>> +++ 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

> 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

> 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.

> Æ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".

> Ę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.

> 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
> 
> :)

> 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.

>
> 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
>

>> --->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
>> +       )
>>  '

> 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.

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

> 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/.

> Æ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:

> Æ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”.

> Æ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.

> 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.

> 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' '

> 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.

> 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.

>>> 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
>>      '

> 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.

> 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 .= " ⋅ next";
>  		}
>  
> -		if ($#commitlist >= 100) {
> -		}
> -