[PATCH] t7300: fix broken && chains

[PATCH] t7300: fix broken && chains

[Erik Elfström]

While we are here, remove some boilerplate by using test_commit.

Signed-off-by: Erik Elfström <erik.elfstrom@gmail.com>
---
 t/t7300-clean.sh | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 27557d6..86ceb38 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -432,9 +432,7 @@ test_expect_success 'nested git work tree' '
 	(
 		cd foo &&
 		git init &&
-		>hello.world
-		git add . &&
-		git commit -a -m nested
+		test_commit nested hello.world
 	) &&
 	(
 		cd bar &&
@@ -443,9 +441,7 @@ test_expect_success 'nested git work tree' '
 	(
 		cd baz/boo &&
 		git init &&
-		>deeper.world
-		git add . &&
-		git commit -a -m deeply.nested
+		test_commit deeply.nested deeper.world
 	) &&
 	git clean -f -d &&
 	test -f foo/.git/index &&
@@ -601,9 +597,7 @@ test_expect_success 'force removal of nested git work tree' '
 	(
 		cd foo &&
 		git init &&
-		>hello.world
-		git add . &&
-		git commit -a -m nested
+		test_commit nested hello.world
 	) &&
 	(
 		cd bar &&
@@ -612,9 +606,7 @@ test_expect_success 'force removal of nested git work tree' '
 	(
 		cd baz/boo &&
 		git init &&
-		>deeper.world
-		git add . &&
-		git commit -a -m deeply.nested
+		test_commit deeply.nested deeper.world
 	) &&
 	git clean -f -f -d &&
 	! test -d foo &&
-- 
2.4.4.410.gd6567e3

Re: [PATCH] t7300: fix broken && chains

[Junio C Hamano]

Erik Elfström <erik.elfstrom@gmail.com> writes:

> While we are here, remove some boilerplate by using test_commit.
>
> Signed-off-by: Erik Elfström <erik.elfstrom@gmail.com>
> ---

Many of the constructs we see here shows clearly that this is an
ancient part of the codebase ;-), as we would be using the one
parameter form of "git init" and more test_* helpers if we were
writing this script in today's Git codebase.  It may have been
better if you didn't do "while we are here" and corrected only the
&&-chain in patch 1/2 and then updated the style of the tests to
take advantage of the newer facilities recent test-lib has in a
separate patch 2/2, but this will do at least for now.

Will queue.

Thanks.


>  t/t7300-clean.sh | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
>
> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
> index 27557d6..86ceb38 100755
> --- a/t/t7300-clean.sh
> +++ b/t/t7300-clean.sh
> @@ -432,9 +432,7 @@ test_expect_success 'nested git work tree' '
>  	(
>  		cd foo &&
>  		git init &&
> -		>hello.world
> -		git add . &&
> -		git commit -a -m nested
> +		test_commit nested hello.world
>  	) &&
>  	(
>  		cd bar &&
> @@ -443,9 +441,7 @@ test_expect_success 'nested git work tree' '
>  	(
>  		cd baz/boo &&
>  		git init &&
> -		>deeper.world
> -		git add . &&
> -		git commit -a -m deeply.nested
> +		test_commit deeply.nested deeper.world
>  	) &&
>  	git clean -f -d &&
>  	test -f foo/.git/index &&
> @@ -601,9 +597,7 @@ test_expect_success 'force removal of nested git work tree' '
>  	(
>  		cd foo &&
>  		git init &&
> -		>hello.world
> -		git add . &&
> -		git commit -a -m nested
> +		test_commit nested hello.world
>  	) &&
>  	(
>  		cd bar &&
> @@ -612,9 +606,7 @@ test_expect_success 'force removal of nested git work tree' '
>  	(
>  		cd baz/boo &&
>  		git init &&
> -		>deeper.world
> -		git add . &&
> -		git commit -a -m deeply.nested
> +		test_commit deeply.nested deeper.world
>  	) &&
>  	git clean -f -f -d &&
>  	! test -d foo &&

Re: [PATCH] t7300: fix broken && chains

[Jeff King]

On Sun, Aug 30, 2015 at 11:18:09AM +0200, Erik Elfström wrote:

> While we are here, remove some boilerplate by using test_commit.

Darn, I thought we were done finding these after flipping on
GIT_TEST_CHAIN_LINT. :)

> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
> index 27557d6..86ceb38 100755
> --- a/t/t7300-clean.sh
> +++ b/t/t7300-clean.sh
> @@ -432,9 +432,7 @@ test_expect_success 'nested git work tree' '
>  	(
>  		cd foo &&
>  		git init &&
> -		>hello.world
> -		git add . &&
> -		git commit -a -m nested
> +		test_commit nested hello.world
>  	) &&

Unfortunately CHAIN_LINT cannot reach inside a nested subshell. I cannot
think of a way to make it do so, and besides, that is also the way to
override the &&-chain precedence. So I think it is not really possible
to get better coverage here. And such cases as these are not really very
interesting (e.g., here we exclude only minor minor setup steps from the
&&-chain).

-Peff

Re: [PATCH] t7300: fix broken && chains

[erik elfström]

On Mon, Aug 31, 2015 at 8:54 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Aug 30, 2015 at 11:18:09AM +0200, Erik Elfström wrote:
>
> Unfortunately CHAIN_LINT cannot reach inside a nested subshell. I cannot
> think of a way to make it do so, and besides, that is also the way to
> override the &&-chain precedence. So I think it is not really possible
> to get better coverage here. And such cases as these are not really very
> interesting (e.g., here we exclude only minor minor setup steps from the
> &&-chain).
>
> -Peff

I actually hacked together an ugly script to check for more broken
chains. It's not working very well and I haven't checked the output
thoroughly but I did see quite a few broken chains. Many are of the
(mostly harmless) form:

     (
        echo "100644 $o5 0    a"
        echo "100644 $o0 0    c"
        echo "160000 $c1 0    d"
    ) >expected &&

I'd estimate that there are hundreds of these (see t3030 for
examples). I'm not sure if you care about these? As you say they are
not really very interesting cases.

/Erik

Re: [PATCH] t7300: fix broken && chains

[erik elfström]

On Mon, Aug 31, 2015 at 6:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Many of the constructs we see here shows clearly that this is an
> ancient part of the codebase ;-), as we would be using the one
> parameter form of "git init" and more test_* helpers if we were
> writing this script in today's Git codebase.  It may have been
> better if you didn't do "while we are here" and corrected only the
> &&-chain in patch 1/2 and then updated the style of the tests to
> take advantage of the newer facilities recent test-lib has in a
> separate patch 2/2, but this will do at least for now.
>
> Will queue.
>
> Thanks.


I can do a re-roll with the chain fix in the first patch and a more
thorough modernization of t7300 in separate patches if you'd like? I
almost went this way for v1 but decided to limit the scope for the
first version.

(Forgot to include the list in my first reply, sorry... And forgot to
turn off HTML in the second... sigh, sorry for the spam)

/Erik

Re: [PATCH] t7300: fix broken && chains

[Junio C Hamano]

erik elfström <erik.elfstrom@gmail.com> writes:

> On Mon, Aug 31, 2015 at 6:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> ...  It may have been
>> better if you didn't do "while we are here" and corrected only the
>> &&-chain in patch 1/2 and then updated the style of the tests to
>> take advantage of the newer facilities recent test-lib has in a
>> separate patch 2/2, but this will do at least for now.
>>
>> Will queue.
>>
>> Thanks.
>
> I can do a re-roll with the chain fix in the first patch and a more
> thorough modernization of t7300 in separate patches if you'd like? I
> almost went this way for v1 but decided to limit the scope for the
> first version.

I'd say it is too much work for something that we already have
reviewed and queued ;-) Let's have this patch as-is.

Modernization and clean-up from time to time is a good thing to do,
but we only have limited review bandwidth, so let's not overdo it.

Thanks.

Re: [PATCH] t7300: fix broken && chains

[Jeff King]

On Tue, Sep 01, 2015 at 08:27:59AM +0200, erik elfström wrote:

>      (
>         echo "100644 $o5 0    a"
>         echo "100644 $o0 0    c"
>         echo "160000 $c1 0    d"
>     ) >expected &&
> 
> I'd estimate that there are hundreds of these (see t3030 for
> examples). I'm not sure if you care about these? As you say they are
> not really very interesting cases.

I think patches for these are OK, but no, I don't consider it a high
priority if they are harmless (IMHO the real value of the patches is
that it removes the noise from the output of your script, so you can
find any cases that _do_ matter).

-Peff