threads / patch / 49189

patch, 2 partst0020-crlf: check the right file

Subject: [PATCH 1/2] t0020-crlf: check the right file

## tl;dr

4 messages between Aug 22, 2018 and Aug 22, 2018. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

SZEDER Gábor· Aug 22, 2018, 12:44 UTC · lore

In the test 'checkout with autocrlf=input' in 't0020-crlf.sh', one of the 'has_cr' checks looks at the non-existing file 'two' instead of 'dir/two'. The test still succeeds, without actually checking what it was supposed to, because this check is expected to fail anyway.

As a minimal fix, fix the name of the file to be checked.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t0020-crlf.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t0020-crlf.sh +1 −1
diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index 5f056982a5..854da0ae16 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -160,7 +160,7 @@ test_expect_success 'checkout with autocrlf=input' '
 	git config core.autocrlf input &&
 	git read-tree --reset -u HEAD &&
 	test_must_fail has_cr one &&
-	test_must_fail has_cr two &&
+	test_must_fail has_cr dir/two &&
 	git update-index -- one dir/two &&
 	test "$one" = $(git hash-object --stdin <one) &&
 	test "$two" = $(git hash-object --stdin <dir/two) &&

-- 
2.19.0.rc0.136.gd2dd172e64
SZEDER Gábor· Aug 22, 2018, 12:44 UTC · re: SZEDER Gábor · lore

[PATCH 2/2] t4051-diff-function-context: read the right file

The test ' context does not include preceding empty lines' in the block of tests 'change with long common tail and no context' in 't4051-diff-function-context.sh' tries to read the file 'long_common_tail.diff.diff', but that file doesn't exist as its name contains one more '.diff' suffixes than necessary.

Despite this error the test still succeeded without checking what it's supposed to, because this erroneous read is done on the line:

  test "$(first_context_line <long_common_tail.diff.diff)" != " "
which means that:
  - the command substitution hides the error, so it won't fail the
    test, and
  - the result of the command substitution is the empty string, which
    is, of course, not equal to a single space character, so the
    condition is fulfilled, and the test succeeds.
As a minimal fix, fix the name of the file to be read.

In the future we might want to reorganize this test script (1) to use 'test_cmp' instead of 'test's and command substitutions to catch failing commands and to provide helpful error messages, and (2) to specify what the expected result actually _is_ instead of what it isn't.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t4051-diff-function-context.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t4051-diff-function-context.sh +1 −1
diff --git a/t/t4051-diff-function-context.sh b/t/t4051-diff-function-context.sh
index 2d76a971c4..4838a1df8b 100755
--- a/t/t4051-diff-function-context.sh
+++ b/t/t4051-diff-function-context.sh
@@ -174,7 +174,7 @@ test_expect_success ' context does not include other functions' '
 '
 
 test_expect_success ' context does not include preceding empty lines' '
-	test "$(first_context_line <long_common_tail.diff.diff)" != " "
+	test "$(first_context_line <long_common_tail.diff)" != " "
 '
 
 check_diff changed_hello_appended 'changed function plus appended function'

-- 
2.19.0.rc0.136.gd2dd172e64
Junio C Hamano· Aug 22, 2018, 16:11 UTC · re: SZEDER Gábor · lore

Re: [PATCH 1/2] t0020-crlf: check the right file

SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 9 quoted lines
> In the test 'checkout with autocrlf=input' in 't0020-crlf.sh', one of
> the 'has_cr' checks looks at the non-existing file 'two' instead of
> 'dir/two'.  The test still succeeds, without actually checking what it
> was supposed to, because this check is expected to fail anyway.
>
> As a minimal fix, fix the name of the file to be checked.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---

This originates from fd777141 ("t0020: fix ignored exit code inside loops", 2015-03-25) where a loop

    for f in one dir/two
    do
            do things on "$f" || break
    done

was unrolled to correctly break out of the &&-chain. The filenames on the update-index line correctly copied one and dir/two, but the has_cr line somehow lost dir/ prefix.

Thanks.  Will queue.
Show 16 quoted lines
>  t/t0020-crlf.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
> index 5f056982a5..854da0ae16 100755
> --- a/t/t0020-crlf.sh
> +++ b/t/t0020-crlf.sh
> @@ -160,7 +160,7 @@ test_expect_success 'checkout with autocrlf=input' '
>  	git config core.autocrlf input &&
>  	git read-tree --reset -u HEAD &&
>  	test_must_fail has_cr one &&
> -	test_must_fail has_cr two &&
> +	test_must_fail has_cr dir/two &&
>  	git update-index -- one dir/two &&
>  	test "$one" = $(git hash-object --stdin <one) &&
>  	test "$two" = $(git hash-object --stdin <dir/two) &&
Jeff King· Aug 22, 2018, 16:30 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] t0020-crlf: check the right file

On Wed, Aug 22, 2018 at 09:11:08AM -0700, Junio C Hamano wrote:
Show 13 quoted lines
> This originates from fd777141 ("t0020: fix ignored exit code inside
> loops", 2015-03-25) where a loop
> 
>     for f in one dir/two
>     do
>             do things on "$f" || break
>     done
> 
> was unrolled to correctly break out of the &&-chain.  The filenames
> on the update-index line correctly copied one and dir/two, but the
> has_cr line somehow lost dir/ prefix.
> 
> Thanks.  Will queue.
Whoops, my error. Thanks for catching it.
Show 10 quoted lines
> > diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
> > index 5f056982a5..854da0ae16 100755
> > --- a/t/t0020-crlf.sh
> > +++ b/t/t0020-crlf.sh
> > @@ -160,7 +160,7 @@ test_expect_success 'checkout with autocrlf=input' '
> >  	git config core.autocrlf input &&
> >  	git read-tree --reset -u HEAD &&
> >  	test_must_fail has_cr one &&
> > -	test_must_fail has_cr two &&
> > +	test_must_fail has_cr dir/two &&

I think I'm also responsible for these mis-uses of test_must_fail, which probably ought to be "!".

-Peff

← back to recent threads