threads / patch / 40219

patcht7300: fix broken && chains

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

## tl;dr

7 messages between Aug 30, 2015 and Sep 1, 2015. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Erik Elfström· Aug 30, 2015, 09:18 UTC · lore

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

Show changes to t/t7300-clean.sh +4 −12

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

Junio C Hamano· Aug 31, 2015, 16:58 UTC · re: Erik Elfström · lore

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

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.

Show 51 quoted lines

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

erik elfström· Sep 1, 2015, 06:31 UTC · re: Junio C Hamano · lore

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

On Mon, Aug 31, 2015 at 6:58 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

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

Junio C Hamano· Sep 1, 2015, 16:23 UTC · re: erik elfström · lore

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

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

Show 16 quoted lines

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

Jeff King· Aug 31, 2015, 18:54 UTC · re: Erik Elfström · lore

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

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

Show 13 quoted lines

> 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

erik elfström· Sep 1, 2015, 06:27 UTC · re: Jeff King · lore

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

On Mon, Aug 31, 2015 at 8:54 PM, Jeff King <peff@peff.net> wrote:

Show 10 quoted lines

> 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

Jeff King· Sep 1, 2015, 17:36 UTC · re: erik elfström · lore

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

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

Show 9 quoted lines

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