threads / patch / 50766

v3, 3 partsAvoid using pipes

Subject: [GSoC][PATCH v3 0/3] Avoid using pipes

## tl;dr

11 messages between Mar 17, 2019 and Mar 24, 2019. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Jonathan Chang· Mar 17, 2019, 15:23 UTC · lore
Hi,

This is my SoC 2019 Microproject: "Avoid pipes in git related commands in test scripts".

Changes since v2:
	- drop changes to other 2 files as suggested by Christian Couder[1]
	- rename commit message from "t0000-basic:" to "t0000:"[1]
	- use better commit message suggested by Thomas Gummerer[2]
		and Christian Couder[3]
	- fix the incorrect transformation pointed out by Eric Sunshine[4]
	- rename commit message to "fix indentation" as seen in:
		25e3d28efd ("submodule.c: fix indentation", 2018-11-28)
		d4cddd66d7 ("worktree.c: fix indentation", 2016-01-18)

[v1]: https://public-inbox.org/git/20190309154555.33407-1-ttjtftx@gmail.com/ [v2]: https://public-inbox.org/git/20190310080739.63984-1-ttjtftx@gmail.com/

[1]: https://public-inbox.org/git/CAP8UFD274-iDkqPm8-WGXbUmcVqjDE7bSg2bwA-17TWJivn0jA@mail.gmail.com/ [2]: https://public-inbox.org/git/20190310175924.GF31533@hank.intra.tgummerer.com/ [3]: https://public-inbox.org/git/CAP8UFD09QZd=6HyB5Om1PfV=67+CvfyQkcpLU0tukT48QccD0Q@mail.gmail.com/ [4]: https://public-inbox.org/git/CAPig+cSMZrQFrLXoO5KE1uonUxmnYHikr-e6GAq_n6vx3+sPJA@mail.gmail.com/

Jonathan Chang (3):
  t0000: fix indentation
  t0000: avoid using pipes
  t0000: use test_line_count instead of wc -l
 t/t0000-basic.sh | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)
Range-diff against v2:
1:  dd3e868436 ! 1:  c3c231d117 t0000-basic: fix an indentation error
    @@ -1,9 +1,11 @@
     Author: Jonathan Chang <ttjtftx@gmail.com>
     
    -    t0000-basic: fix an indentation error
    +    t0000: fix indentation
     
    -    This is a preparatory step prior to removing the pipes after git
    -    commands, which discards git's exit code and may mask a crash.
    +    Fix indentation of a line containing a pipeline to reduce the
    +    noise when refactoring the pipeline in a subsequent commit.
    +    This has been wrong since the refactoring done in 1b5b2b641a
    +    ("t0000: modernise style", 2012-03-02), but carries no meaning.
     
         Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
     
2:  5bc591e557 ! 2:  5a3c6e24eb t0000-basic: avoid using pipes
    @@ -1,6 +1,6 @@
     Author: Jonathan Chang <ttjtftx@gmail.com>
     
    -    t0000-basic: avoid using pipes
    +    t0000: avoid using pipes
     
         The exit code of the upstream in a pipe is ignored thus we should avoid
         using it. By writing out the output of the git command to a file, we can
3:  be6a218d83 < -:  ---------- t0003-attributes: avoid using pipes
4:  35564c86a1 < -:  ---------- t0022-crlf-rename: avoid using pipes
5:  17acadea53 ! 3:  bc3dee82a9 t0000-basic: use test_line_count instead of wc -l
    @@ -1,6 +1,6 @@
     Author: Jonathan Chang <ttjtftx@gmail.com>
     
    -    t0000-basic: use test_line_count instead of wc -l
    +    t0000: use test_line_count instead of wc -l
     
         Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
     
    @@ -13,8 +13,8 @@
      	git show --pretty=raw $commit2 >actual &&
     -	numparent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l) &&
     -	test $numparent = 1
    -+	sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l >numparent &&
    -+	test_line_count = 1 numparent
    ++	sed -n -e "s/^parent //p" -e "/^author /q" actual >parents &&
    ++	test_line_count = 1 parents
      '
      
      test_expect_success 'update-index D/F conflict' '
    @@ -24,8 +24,7 @@
      	git ls-files path0 >actual &&
     -	numpath0=$(wc -l <actual) &&
     -	test $numpath0 = 1
    -+	wc -l <actual >numpath0 &&
    -+	test_line_count = 1 numpath0
    ++	test_line_count = 1 actual
      '
      
      test_expect_success 'very long name in the index handled sanely' '
-- 
2.21.0
Jonathan Chang· Mar 17, 2019, 15:23 UTC · re: Jonathan Chang · lore

[GSoC][PATCH v3 1/3] t0000: fix indentation

Fix indentation of a line containing a pipeline to reduce the noise when refactoring the pipeline in a subsequent commit. This has been wrong since the refactoring done in 1b5b2b641a ("t0000: modernise style", 2012-03-02), but carries no meaning.

Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
---
 t/t0000-basic.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t0000-basic.sh +1 −1
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index b6566003dd..53821f5817 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -1132,7 +1132,7 @@ test_expect_success 'git commit-tree records the correct parent in a commit' '
 
 test_expect_success 'git commit-tree omits duplicated parent in a commit' '
 	commit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&
-	     parent=$(git show --pretty=raw $commit2 |
+	parent=$(git show --pretty=raw $commit2 |
 		sed -n -e "s/^parent //p" -e "/^author /q" |
 		sort -u) &&
 	test "z$commit0" = "z$parent" &&

-- 
2.21.0
Jonathan Chang· Mar 17, 2019, 15:23 UTC · re: Jonathan Chang · lore

[GSoC][PATCH v3 2/3] t0000: avoid using pipes

The exit code of the upstream in a pipe is ignored thus we should avoid using it. By writing out the output of the git command to a file, we can test the exit codes of both the commands.

Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
---
 t/t0000-basic.sh | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)
Show changes to t/t0000-basic.sh +14 −14
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 53821f5817..47666b013e 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -1118,27 +1118,25 @@ P=$(test_oid root)
 
 test_expect_success 'git commit-tree records the correct tree in a commit' '
 	commit0=$(echo NO | git commit-tree $P) &&
-	tree=$(git show --pretty=raw $commit0 |
-		 sed -n -e "s/^tree //p" -e "/^author /q") &&
+	git show --pretty=raw $commit0 >actual &&
+	tree=$(sed -n -e "s/^tree //p" -e "/^author /q" actual) &&
 	test "z$tree" = "z$P"
 '
 
 test_expect_success 'git commit-tree records the correct parent in a commit' '
 	commit1=$(echo NO | git commit-tree $P -p $commit0) &&
-	parent=$(git show --pretty=raw $commit1 |
-		sed -n -e "s/^parent //p" -e "/^author /q") &&
+	git show --pretty=raw $commit1 >actual &&
+	parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual) &&
 	test "z$commit0" = "z$parent"
 '
 
 test_expect_success 'git commit-tree omits duplicated parent in a commit' '
 	commit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&
-	parent=$(git show --pretty=raw $commit2 |
-		sed -n -e "s/^parent //p" -e "/^author /q" |
-		sort -u) &&
+	git show --pretty=raw $commit2 >actual &&
+	parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | sort -u) &&
 	test "z$commit0" = "z$parent" &&
-	numparent=$(git show --pretty=raw $commit2 |
-		sed -n -e "s/^parent //p" -e "/^author /q" |
-		wc -l) &&
+	git show --pretty=raw $commit2 >actual &&
+	numparent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l) &&
 	test $numparent = 1
 '
 
@@ -1147,7 +1145,8 @@ test_expect_success 'update-index D/F conflict' '
 	mv path2 path0 &&
 	mv tmp path2 &&
 	git update-index --add --replace path2 path0/file2 &&
-	numpath0=$(git ls-files path0 | wc -l) &&
+	git ls-files path0 >actual &&
+	numpath0=$(wc -l <actual) &&
 	test $numpath0 = 1
 '
 
@@ -1162,12 +1161,13 @@ test_expect_success 'very long name in the index handled sanely' '
 	>path4 &&
 	git update-index --add path4 &&
 	(
-		git ls-files -s path4 |
-		sed -e "s/	.*/	/" |
+		git ls-files -s path4 >actual &&
+		sed -e "s/	.*/	/" actual |
 		tr -d "\012" &&
 		echo "$a"
 	) | git update-index --index-info &&
-	len=$(git ls-files "a*" | wc -c) &&
+	git ls-files "a*" >actual &&
+	len=$(wc -c <actual) &&
 	test $len = 4098
 '
 

-- 
2.21.0
Ævar Arnfjörð Bjarmason· Mar 17, 2019, 16:47 UTC · re: Jonathan Chang · lore

Re: [GSoC][PATCH v3 2/3] t0000: avoid using pipes

On Sun, Mar 17 2019, Jonathan Chang wrote:
Show 22 quoted lines
> The exit code of the upstream in a pipe is ignored thus we should avoid
> using it. By writing out the output of the git command to a file, we can
> test the exit codes of both the commands.
>
> Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
> ---
>  t/t0000-basic.sh | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index 53821f5817..47666b013e 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -1118,27 +1118,25 @@ P=$(test_oid root)
>
>  test_expect_success 'git commit-tree records the correct tree in a commit' '
>  	commit0=$(echo NO | git commit-tree $P) &&
> -	tree=$(git show --pretty=raw $commit0 |
> -		 sed -n -e "s/^tree //p" -e "/^author /q") &&
> +	git show --pretty=raw $commit0 >actual &&
> +	tree=$(sed -n -e "s/^tree //p" -e "/^author /q" actual) &&
>  	test "z$tree" = "z$P"

This change is an improvement just changing the "git" invocations. But I wonder as we're reviewing this / churning this if we couldn't also modernize this style to just:

    git .. >tmp &&
    sed -n -e <tmp >actual &&
    test_must_be_empty actual
Show 9 quoted lines
>  '
>
>  test_expect_success 'git commit-tree records the correct parent in a commit' '
>  	commit1=$(echo NO | git commit-tree $P -p $commit0) &&
> -	parent=$(git show --pretty=raw $commit1 |
> -		sed -n -e "s/^parent //p" -e "/^author /q") &&
> +	git show --pretty=raw $commit1 >actual &&
> +	parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual) &&
>  	test "z$commit0" = "z$parent"
ditto.
Show 16 quoted lines
>  '
>
>  test_expect_success 'git commit-tree omits duplicated parent in a commit' '
>  	commit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&
> -	parent=$(git show --pretty=raw $commit2 |
> -		sed -n -e "s/^parent //p" -e "/^author /q" |
> -		sort -u) &&
> +	git show --pretty=raw $commit2 >actual &&
> +	parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | sort -u) &&
>  	test "z$commit0" = "z$parent" &&
> -	numparent=$(git show --pretty=raw $commit2 |
> -		sed -n -e "s/^parent //p" -e "/^author /q" |
> -		wc -l) &&
> +	git show --pretty=raw $commit2 >actual &&
> +	numparent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l) &&
>  	test $numparent = 1
And stuff like this to (skipping the wc -l):
    sed -n -e <tmp >actual &&
    test_line_count = 1 actual
Show 10 quoted lines
>  '
>
> @@ -1147,7 +1145,8 @@ test_expect_success 'update-index D/F conflict' '
>  	mv path2 path0 &&
>  	mv tmp path2 &&
>  	git update-index --add --replace path2 path0/file2 &&
> -	numpath0=$(git ls-files path0 | wc -l) &&
> +	git ls-files path0 >actual &&
> +	numpath0=$(wc -l <actual) &&
>  	test $numpath0 = 1
ditto.
Show 17 quoted lines
>  '
>
> @@ -1162,12 +1161,13 @@ test_expect_success 'very long name in the index handled sanely' '
>  	>path4 &&
>  	git update-index --add path4 &&
>  	(
> -		git ls-files -s path4 |
> -		sed -e "s/	.*/	/" |
> +		git ls-files -s path4 >actual &&
> +		sed -e "s/	.*/	/" actual |
>  		tr -d "\012" &&
>  		echo "$a"
>  	) | git update-index --index-info &&
> -	len=$(git ls-files "a*" | wc -c) &&
> +	git ls-files "a*" >actual &&
> +	len=$(wc -c <actual) &&
>  	test $len = 4098

Ditto. Maybe the initial author wanted to avoid writing out 4k lines, but now that we're doing so anyway...

jonathan chang· Mar 24, 2019, 11:26 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [GSoC][PATCH v3 2/3] t0000: avoid using pipes

On Mon, Mar 18, 2019 at 12:47 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 34 quoted lines
>
>
> On Sun, Mar 17 2019, Jonathan Chang wrote:
>
> > The exit code of the upstream in a pipe is ignored thus we should avoid
> > using it. By writing out the output of the git command to a file, we can
> > test the exit codes of both the commands.
> >
> > Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
> > ---
> >  t/t0000-basic.sh | 28 ++++++++++++++--------------
> >  1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> > index 53821f5817..47666b013e 100755
> > --- a/t/t0000-basic.sh
> > +++ b/t/t0000-basic.sh
> > @@ -1118,27 +1118,25 @@ P=$(test_oid root)
> >
> >  test_expect_success 'git commit-tree records the correct tree in a commit' '
> >       commit0=$(echo NO | git commit-tree $P) &&
> > -     tree=$(git show --pretty=raw $commit0 |
> > -              sed -n -e "s/^tree //p" -e "/^author /q") &&
> > +     git show --pretty=raw $commit0 >actual &&
> > +     tree=$(sed -n -e "s/^tree //p" -e "/^author /q" actual) &&
> >       test "z$tree" = "z$P"
>
> This change is an improvement just changing the "git" invocations. But I
> wonder as we're reviewing this / churning this if we couldn't also
> modernize this style to just:
>
>     git .. >tmp &&
>     sed -n -e <tmp >actual &&
>     test_must_be_empty actual
Do you mean something like this:
-       git show --pretty=raw $commit0 >actual &&
-       tree=$(sed -n -e "s/^tree //p" -e "/^author /q" actual) &&
-       test "z$tree" = "z$P"
+       git show --pretty=raw $commit0 >tmp &&
+       sed -n -e "/$P/d" -e "s/^tree //p" -e "/^author /q" tmp >actual &&
+       test_must_be_empty actual

It works. But the semantic is different if we use test_must_be_empty. I wonder if you mean test_cmp because I found some commits[1 2 3] that changes 'test "z...' to use test_cmp with 'git log -G 'test "z' --oneline' and git-show. However, they are all around 2013, so I'm not so sure this is what you mean either.

I did found some use of sed's 'd' function in conjunction with
test_must_be_empty, using:
  git grep -A 5 'sed .*/d' | grep -B 5 'test_must_be_empty'
However, they don't use parameter expansion in sed.

There are some places where parameter expansion is used in sed, but that would require test_cmp in this case, and would need to write to another file to compare.

Maybe this 'test "z$A" = "z$B"' syntax is fine, yet most of the existing usages are added around 12 years ago according git-blame I saw on github.

[1]: 03c893cbf9 ("t1006: modernize output comparisons", 2013-07-10) [2]: 848575d833 ("push test: simplify check of push result", 2013-03-18) [3]: ed838e6615 ("t1300: style updates", 2012-10-23)

Show 18 quoted lines
> > @@ -1162,12 +1161,13 @@ test_expect_success 'very long name in the index handled sanely' '
> >       >path4 &&
> >       git update-index --add path4 &&
> >       (
> > -             git ls-files -s path4 |
> > -             sed -e "s/      .*/     /" |
> > +             git ls-files -s path4 >actual &&
> > +             sed -e "s/      .*/     /" actual |
> >               tr -d "\012" &&
> >               echo "$a"
> >       ) | git update-index --index-info &&
> > -     len=$(git ls-files "a*" | wc -c) &&
> > +     git ls-files "a*" >actual &&
> > +     len=$(wc -c <actual) &&
> >       test $len = 4098
>
> Ditto. Maybe the initial author wanted to avoid writing out 4k lines,
> but now that we're doing so anyway...
This is 'wc -c', so I think I don't have to modify it?

By the way, I found 2 lines that can be changed to use test_must_be_empty in t0000-basic.sh. I paste the diff below: ---

Show changes to diff +2 −2
@@ -51,7 +51,7 @@ test_expect_success 'verify that the running shell
supports "local"' '

 test_expect_success '.git/objects should be empty after git init in
an empty repo' '
        find .git/objects -type f -print >should-be-empty &&
-       test_line_count = 0 should-be-empty
+       test_must_be_empty should-be-empty
 '

 # also it should have 2 subdirectories; no fan-out anymore, pack, and info.
@@ -1110,7 +1110,7 @@ test_expect_success 'git update-index --refresh
should succeed' '

 test_expect_success 'no diff after checkout and git update-index --refresh' '
        git diff-files >current &&
-       cmp -s current /dev/null
+       test_must_be_empty current
 '
Ævar Arnfjörð Bjarmason· Mar 24, 2019, 19:04 UTC · re: jonathan chang · lore

Re: [GSoC][PATCH v3 2/3] t0000: avoid using pipes

On Sun, Mar 24 2019, jonathan chang wrote:
Show 69 quoted lines
> On Mon, Mar 18, 2019 at 12:47 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>>
>> On Sun, Mar 17 2019, Jonathan Chang wrote:
>>
>> > The exit code of the upstream in a pipe is ignored thus we should avoid
>> > using it. By writing out the output of the git command to a file, we can
>> > test the exit codes of both the commands.
>> >
>> > Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
>> > ---
>> >  t/t0000-basic.sh | 28 ++++++++++++++--------------
>> >  1 file changed, 14 insertions(+), 14 deletions(-)
>> >
>> > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
>> > index 53821f5817..47666b013e 100755
>> > --- a/t/t0000-basic.sh
>> > +++ b/t/t0000-basic.sh
>> > @@ -1118,27 +1118,25 @@ P=$(test_oid root)
>> >
>> >  test_expect_success 'git commit-tree records the correct tree in a commit' '
>> >       commit0=$(echo NO | git commit-tree $P) &&
>> > -     tree=$(git show --pretty=raw $commit0 |
>> > -              sed -n -e "s/^tree //p" -e "/^author /q") &&
>> > +     git show --pretty=raw $commit0 >actual &&
>> > +     tree=$(sed -n -e "s/^tree //p" -e "/^author /q" actual) &&
>> >       test "z$tree" = "z$P"
>>
>> This change is an improvement just changing the "git" invocations. But I
>> wonder as we're reviewing this / churning this if we couldn't also
>> modernize this style to just:
>>
>>     git .. >tmp &&
>>     sed -n -e <tmp >actual &&
>>     test_must_be_empty actual
>
> Do you mean something like this:
>
> -       git show --pretty=raw $commit0 >actual &&
> -       tree=$(sed -n -e "s/^tree //p" -e "/^author /q" actual) &&
> -       test "z$tree" = "z$P"
> +       git show --pretty=raw $commit0 >tmp &&
> +       sed -n -e "/$P/d" -e "s/^tree //p" -e "/^author /q" tmp >actual &&
> +       test_must_be_empty actual
>
> It works. But the semantic is different if we use test_must_be_empty.
> I wonder if you mean test_cmp because I found some commits[1 2 3]
> that changes 'test "z...' to use test_cmp with 'git log -G 'test "z' --oneline'
> and git-show. However, they are all around 2013, so I'm not so sure
> this is what you mean either.
>
> I did found some use of sed's 'd' function in conjunction with
> test_must_be_empty, using:
>   git grep -A 5 'sed .*/d' | grep -B 5 'test_must_be_empty'
> However, they don't use parameter expansion in sed.
>
> There are some places where parameter expansion is used in sed, but
> that would require test_cmp in this case, and would need to write to
> another file to compare.
>
> Maybe this 'test "z$A" = "z$B"' syntax is fine, yet most of the existing
> usages are added around 12 years ago according git-blame I saw on
> github.
>
>
> [1]: 03c893cbf9 ("t1006: modernize output comparisons", 2013-07-10)
> [2]: 848575d833 ("push test: simplify check of push result", 2013-03-18)
> [3]: ed838e6615 ("t1300: style updates", 2012-10-23)

Yeah there's many ways to do it. Generally we've been moving to the "test_*" wrapper that e.g. given X lines print out an explanation that we were expecting Y instead.

Show 20 quoted lines
>> > @@ -1162,12 +1161,13 @@ test_expect_success 'very long name in the index handled sanely' '
>> >       >path4 &&
>> >       git update-index --add path4 &&
>> >       (
>> > -             git ls-files -s path4 |
>> > -             sed -e "s/      .*/     /" |
>> > +             git ls-files -s path4 >actual &&
>> > +             sed -e "s/      .*/     /" actual |
>> >               tr -d "\012" &&
>> >               echo "$a"
>> >       ) | git update-index --index-info &&
>> > -     len=$(git ls-files "a*" | wc -c) &&
>> > +     git ls-files "a*" >actual &&
>> > +     len=$(wc -c <actual) &&
>> >       test $len = 4098
>>
>> Ditto. Maybe the initial author wanted to avoid writing out 4k lines,
>> but now that we're doing so anyway...
>
> This is 'wc -c', so I think I don't have to modify it?

Yes, that's a brainfart of mine. Although as an aside I have a WIP series that adds test_byte_count which can be used for these types of cases.

Show 22 quoted lines
> By the way, I found 2 lines that can be changed to use test_must_be_empty
> in t0000-basic.sh. I paste the diff below:
> ---
> @@ -51,7 +51,7 @@ test_expect_success 'verify that the running shell
> supports "local"' '
>
>  test_expect_success '.git/objects should be empty after git init in
> an empty repo' '
>         find .git/objects -type f -print >should-be-empty &&
> -       test_line_count = 0 should-be-empty
> +       test_must_be_empty should-be-empty
>  '
>
>  # also it should have 2 subdirectories; no fan-out anymore, pack, and info.
> @@ -1110,7 +1110,7 @@ test_expect_success 'git update-index --refresh
> should succeed' '
>
>  test_expect_success 'no diff after checkout and git update-index --refresh' '
>         git diff-files >current &&
> -       cmp -s current /dev/null
> +       test_must_be_empty current
>  '
Jonathan Chang· Mar 17, 2019, 15:23 UTC · re: Jonathan Chang · lore

[GSoC][PATCH v3 3/3] t0000: use test_line_count instead of wc -l

Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
---
 t/t0000-basic.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Show changes to t/t0000-basic.sh +3 −4
diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 47666b013e..3de13daabe 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -1136,8 +1136,8 @@ test_expect_success 'git commit-tree omits duplicated parent in a commit' '
 	parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | sort -u) &&
 	test "z$commit0" = "z$parent" &&
 	git show --pretty=raw $commit2 >actual &&
-	numparent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l) &&
-	test $numparent = 1
+	sed -n -e "s/^parent //p" -e "/^author /q" actual >parents &&
+	test_line_count = 1 parents
 '
 
 test_expect_success 'update-index D/F conflict' '
@@ -1146,8 +1146,7 @@ test_expect_success 'update-index D/F conflict' '
 	mv tmp path2 &&
 	git update-index --add --replace path2 path0/file2 &&
 	git ls-files path0 >actual &&
-	numpath0=$(wc -l <actual) &&
-	test $numpath0 = 1
+	test_line_count = 1 actual
 '
 
 test_expect_success 'very long name in the index handled sanely' '

-- 
2.21.0
Ævar Arnfjörð Bjarmason· Mar 17, 2019, 16:48 UTC · re: Jonathan Chang · lore

Re: [GSoC][PATCH v3 3/3] t0000: use test_line_count instead of wc -l

On Sun, Mar 17 2019, Jonathan Chang wrote:
Show 28 quoted lines
> Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
> ---
>  t/t0000-basic.sh | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index 47666b013e..3de13daabe 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -1136,8 +1136,8 @@ test_expect_success 'git commit-tree omits duplicated parent in a commit' '
>  	parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | sort -u) &&
>  	test "z$commit0" = "z$parent" &&
>  	git show --pretty=raw $commit2 >actual &&
> -	numparent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l) &&
> -	test $numparent = 1
> +	sed -n -e "s/^parent //p" -e "/^author /q" actual >parents &&
> +	test_line_count = 1 parents
>  '
>
>  test_expect_success 'update-index D/F conflict' '
> @@ -1146,8 +1146,7 @@ test_expect_success 'update-index D/F conflict' '
>  	mv tmp path2 &&
>  	git update-index --add --replace path2 path0/file2 &&
>  	git ls-files path0 >actual &&
> -	numpath0=$(wc -l <actual) &&
> -	test $numpath0 = 1
> +	test_line_count = 1 actual
>  '

...of course reading this series in sequence I find that 50% of my suggestions for 2/3 are then done in this patch... :)

Thomas Gummerer· Mar 17, 2019, 20:06 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [GSoC][PATCH v3 3/3] t0000: use test_line_count instead of wc -l

On 03/17, Ævar Arnfjörð Bjarmason wrote:
Show 34 quoted lines
> 
> On Sun, Mar 17 2019, Jonathan Chang wrote:
> 
> > Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
> > ---
> >  t/t0000-basic.sh | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> > index 47666b013e..3de13daabe 100755
> > --- a/t/t0000-basic.sh
> > +++ b/t/t0000-basic.sh
> > @@ -1136,8 +1136,8 @@ test_expect_success 'git commit-tree omits duplicated parent in a commit' '
> >  	parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | sort -u) &&
> >  	test "z$commit0" = "z$parent" &&
> >  	git show --pretty=raw $commit2 >actual &&
> > -	numparent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l) &&
> > -	test $numparent = 1
> > +	sed -n -e "s/^parent //p" -e "/^author /q" actual >parents &&
> > +	test_line_count = 1 parents
> >  '
> >
> >  test_expect_success 'update-index D/F conflict' '
> > @@ -1146,8 +1146,7 @@ test_expect_success 'update-index D/F conflict' '
> >  	mv tmp path2 &&
> >  	git update-index --add --replace path2 path0/file2 &&
> >  	git ls-files path0 >actual &&
> > -	numpath0=$(wc -l <actual) &&
> > -	test $numpath0 = 1
> > +	test_line_count = 1 actual
> >  '
> 
> ...of course reading this series in sequence I find that 50% of my
> suggestions for 2/3 are then done in this patch... :)

Indeed. I think doing this in a separate patch is a good idea, as it makes the previous patch slightly easier to review imho. But I think something to take away from this for Jonathan would be that this should have been described in the commit message of the previous commit. Maybe something like

    This commit doesn't make any additional simplifications, such as
    using the test_line_count function for counting the lines in the
    output.  These simplifications will be made in a subsequent commit.

in addition to the existing commit message would have helped save a bit of review effort.

Ævar Arnfjörð Bjarmason· Mar 18, 2019, 07:36 UTC · re: Thomas Gummerer · lore

Re: [GSoC][PATCH v3 3/3] t0000: use test_line_count instead of wc -l

On Sun, Mar 17 2019, Thomas Gummerer wrote:
Show 48 quoted lines
> On 03/17, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Sun, Mar 17 2019, Jonathan Chang wrote:
>>
>> > Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>
>> > ---
>> >  t/t0000-basic.sh | 7 +++----
>> >  1 file changed, 3 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
>> > index 47666b013e..3de13daabe 100755
>> > --- a/t/t0000-basic.sh
>> > +++ b/t/t0000-basic.sh
>> > @@ -1136,8 +1136,8 @@ test_expect_success 'git commit-tree omits duplicated parent in a commit' '
>> >  	parent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | sort -u) &&
>> >  	test "z$commit0" = "z$parent" &&
>> >  	git show --pretty=raw $commit2 >actual &&
>> > -	numparent=$(sed -n -e "s/^parent //p" -e "/^author /q" actual | wc -l) &&
>> > -	test $numparent = 1
>> > +	sed -n -e "s/^parent //p" -e "/^author /q" actual >parents &&
>> > +	test_line_count = 1 parents
>> >  '
>> >
>> >  test_expect_success 'update-index D/F conflict' '
>> > @@ -1146,8 +1146,7 @@ test_expect_success 'update-index D/F conflict' '
>> >  	mv tmp path2 &&
>> >  	git update-index --add --replace path2 path0/file2 &&
>> >  	git ls-files path0 >actual &&
>> > -	numpath0=$(wc -l <actual) &&
>> > -	test $numpath0 = 1
>> > +	test_line_count = 1 actual
>> >  '
>>
>> ...of course reading this series in sequence I find that 50% of my
>> suggestions for 2/3 are then done in this patch... :)
>
> Indeed.  I think doing this in a separate patch is a good idea, as it
> makes the previous patch slightly easier to review imho.  But I think
> something to take away from this for Jonathan would be that this
> should have been described in the commit message of the previous
> commit.  Maybe something like
>
>     This commit doesn't make any additional simplifications, such as
>     using the test_line_count function for counting the lines in the
>     output.  These simplifications will be made in a subsequent commit.
>
> in addition to the existing commit message would have helped save a
> bit of review effort.

FWIW I chuck this up to just my blindness / expedience in reading the thing.

No objections to changing this, but I don't think it's the fault of a commit message if someone reading it doesn't get an explanation for a future unrelated improvement.

The times when a commit should have such an explanation are cases like e.g. introducing a function that's not used yet to make a subsequent commit smaller, or other such cases where the change is incomplete in some way.

Junio C Hamano· Mar 18, 2019, 08:15 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [GSoC][PATCH v3 3/3] t0000: use test_line_count instead of wc -l

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 8 quoted lines
> No objections to changing this, but I don't think it's the fault of a
> commit message if someone reading it doesn't get an explanation for a
> future unrelated improvement.
>
> The times when a commit should have such an explanation are cases like
> e.g. introducing a function that's not used yet to make a subsequent
> commit smaller, or other such cases where the change is incomplete in
> some way.

Deliberately omitting an obvious improvement "while at it" would be similar to adding a function that is not used yet. Careful readers notice "why don't do it at this step while touching the same area?" just like they would notice "why do it at this step when it is not yet used?". While it certainly is *not* required, they would find it helpful to answer such questions, erring on the safer side.

← back to recent threads