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

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

[Jonathan Chang]

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

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

[Jonathan Chang]

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

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

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

[Jonathan Chang]

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

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

[Ævar Arnfjörð Bjarmason]

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

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

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

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

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

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

[jonathan chang]

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)


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

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

[Ævar Arnfjörð Bjarmason]

On Sun, Mar 24 2019, jonathan chang wrote:

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

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

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

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

[Jonathan Chang]

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
 '
 
 test_expect_success 'very long name in the index handled sanely' '
-- 
2.21.0

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

[Ævar Arnfjörð Bjarmason]

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

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

[Thomas Gummerer]

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.

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

[Ævar Arnfjörð Bjarmason]

On Sun, Mar 17 2019, Thomas Gummerer wrote:

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

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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