{"thread":{"id":"50766","subject":"[GSoC][PATCH v3 0/3] Avoid using pipes","startedAt":"2019-03-17T15:24:20Z","lastAt":"2019-03-24T19:04:40Z","messageCount":11,"participants":["Jonathan Chang","Ævar Arnfjörð Bjarmason","Thomas Gummerer","Junio C Hamano","jonathan chang"],"isPatch":true,"patchVersion":3,"patchTotal":3},"messages":[{"id":"371730","messageId":"cover.1552835153.git.ttjtftx@gmail.com","threadId":"50766","inReplyTo":null,"subject":"[GSoC][PATCH v3 0/3] Avoid using pipes","fromName":"Jonathan Chang","fromEmail":"ttjtftx@gmail.com","sentAt":"2019-03-17T15:23:35Z","receivedAt":"2019-03-17T15:24:20Z","isPatch":true,"sender":{"key":"ttjtftx@gmail.com","avatar":null},"body":"Hi,\n\nThis is my SoC 2019 Microproject: \"Avoid pipes in git related commands\nin test scripts\".\n\nChanges since v2:\n\t- drop changes to other 2 files as suggested by Christian Couder[1]\n\t- rename commit message from \"t0000-basic:\" to \"t0000:\"[1]\n\t- use better commit message suggested by Thomas Gummerer[2]\n\t\tand Christian Couder[3]\n\t- fix the incorrect transformation pointed out by Eric Sunshine[4]\n\t- rename commit message to \"fix indentation\" as seen in:\n\t\t25e3d28efd (\"submodule.c: fix indentation\", 2018-11-28)\n\t\td4cddd66d7 (\"worktree.c: fix indentation\", 2016-01-18)\n\n[v1]: https://public-inbox.org/git/20190309154555.33407-1-ttjtftx@gmail.com/\n[v2]: https://public-inbox.org/git/20190310080739.63984-1-ttjtftx@gmail.com/\n\n[1]: https://public-inbox.org/git/CAP8UFD274-iDkqPm8-WGXbUmcVqjDE7bSg2bwA-17TWJivn0jA@mail.gmail.com/\n[2]: https://public-inbox.org/git/20190310175924.GF31533@hank.intra.tgummerer.com/\n[3]: https://public-inbox.org/git/CAP8UFD09QZd=6HyB5Om1PfV=67+CvfyQkcpLU0tukT48QccD0Q@mail.gmail.com/\n[4]: https://public-inbox.org/git/CAPig+cSMZrQFrLXoO5KE1uonUxmnYHikr-e6GAq_n6vx3+sPJA@mail.gmail.com/\n\nJonathan Chang (3):\n t0000: fix indentation\n t0000: avoid using pipes\n t0000: use test_line_count instead of wc -l\n\n t/t0000-basic.sh | 31 +++++++++++++++----------------\n 1 file changed, 15 insertions(+), 16 deletions(-)\n\nRange-diff against v2:\n1: dd3e868436 ! 1: c3c231d117 t0000-basic: fix an indentation error\n @@ -1,9 +1,11 @@\n Author: Jonathan Chang \n \n - t0000-basic: fix an indentation error\n + t0000: fix indentation\n \n - This is a preparatory step prior to removing the pipes after git\n - commands, which discards git's exit code and may mask a crash.\n + Fix indentation of a line containing a pipeline to reduce the\n + noise when refactoring the pipeline in a subsequent commit.\n + This has been wrong since the refactoring done in 1b5b2b641a\n + (\"t0000: modernise style\", 2012-03-02), but carries no meaning.\n \n Signed-off-by: Jonathan Chang \n \n2: 5bc591e557 ! 2: 5a3c6e24eb t0000-basic: avoid using pipes\n @@ -1,6 +1,6 @@\n Author: Jonathan Chang \n \n - t0000-basic: avoid using pipes\n + t0000: avoid using pipes\n \n The exit code of the upstream in a pipe is ignored thus we should avoid\n using it. By writing out the output of the git command to a file, we can\n3: be6a218d83 < -: ---------- t0003-attributes: avoid using pipes\n4: 35564c86a1 < -: ---------- t0022-crlf-rename: avoid using pipes\n5: 17acadea53 ! 3: bc3dee82a9 t0000-basic: use test_line_count instead of wc -l\n @@ -1,6 +1,6 @@\n Author: Jonathan Chang \n \n - t0000-basic: use test_line_count instead of wc -l\n + t0000: use test_line_count instead of wc -l\n \n Signed-off-by: Jonathan Chang \n \n @@ -13,8 +13,8 @@\n \tgit show --pretty=raw $commit2 >actual &&\n -\tnumparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | wc -l) &&\n -\ttest $numparent = 1\n -+\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | wc -l >numparent &&\n -+\ttest_line_count = 1 numparent\n ++\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" actual >parents &&\n ++\ttest_line_count = 1 parents\n '\n \n test_expect_success 'update-index D/F conflict' '\n @@ -24,8 +24,7 @@\n \tgit ls-files path0 >actual &&\n -\tnumpath0=$(wc -l numpath0 &&\n -+\ttest_line_count = 1 numpath0\n ++\ttest_line_count = 1 actual\n '\n \n test_expect_success 'very long name in the index handled sanely' '\n-- \n2.21.0\n\n"},{"id":"371731","messageId":"c3c231d117604faa167acc75d21738fc97435cd5.1552835153.git.ttjtftx@gmail.com","threadId":"50766","inReplyTo":"cover.1552835153.git.ttjtftx@gmail.com","subject":"[GSoC][PATCH v3 1/3] t0000: fix indentation","fromName":"Jonathan Chang","fromEmail":"ttjtftx@gmail.com","sentAt":"2019-03-17T15:23:36Z","receivedAt":"2019-03-17T15:24:23Z","isPatch":true,"sender":{"key":"ttjtftx@gmail.com","avatar":null},"body":"Fix indentation of a line containing a pipeline to reduce the\nnoise when refactoring the pipeline in a subsequent commit.\nThis has been wrong since the refactoring done in 1b5b2b641a\n(\"t0000: modernise style\", 2012-03-02), but carries no meaning.\n\nSigned-off-by: Jonathan Chang \n---\n t/t0000-basic.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex b6566003dd..53821f5817 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -1132,7 +1132,7 @@ test_expect_success 'git commit-tree records the correct parent in a commit' '\n \n test_expect_success 'git commit-tree omits duplicated parent in a commit' '\n \tcommit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&\n-\t parent=$(git show --pretty=raw $commit2 |\n+\tparent=$(git show --pretty=raw $commit2 |\n \t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" |\n \t\tsort -u) &&\n \ttest \"z$commit0\" = \"z$parent\" &&\n-- \n2.21.0\n\n"},{"id":"371732","messageId":"5a3c6e24eb901c830e8e43608d81aef5d7b60315.1552835153.git.ttjtftx@gmail.com","threadId":"50766","inReplyTo":"cover.1552835153.git.ttjtftx@gmail.com","subject":"[GSoC][PATCH v3 2/3] t0000: avoid using pipes","fromName":"Jonathan Chang","fromEmail":"ttjtftx@gmail.com","sentAt":"2019-03-17T15:23:37Z","receivedAt":"2019-03-17T15:24:25Z","isPatch":true,"sender":{"key":"ttjtftx@gmail.com","avatar":null},"body":"The exit code of the upstream in a pipe is ignored thus we should avoid\nusing it. By writing out the output of the git command to a file, we can\ntest the exit codes of both the commands.\n\nSigned-off-by: Jonathan Chang \n---\n t/t0000-basic.sh | 28 ++++++++++++++--------------\n 1 file changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 53821f5817..47666b013e 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -1118,27 +1118,25 @@ P=$(test_oid root)\n \n test_expect_success 'git commit-tree records the correct tree in a commit' '\n \tcommit0=$(echo NO | git commit-tree $P) &&\n-\ttree=$(git show --pretty=raw $commit0 |\n-\t\t sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n+\tgit show --pretty=raw $commit0 >actual &&\n+\ttree=$(sed -n -e \"s/^tree //p\" -e \"/^author /q\" actual) &&\n \ttest \"z$tree\" = \"z$P\"\n '\n \n test_expect_success 'git commit-tree records the correct parent in a commit' '\n \tcommit1=$(echo NO | git commit-tree $P -p $commit0) &&\n-\tparent=$(git show --pretty=raw $commit1 |\n-\t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\") &&\n+\tgit show --pretty=raw $commit1 >actual &&\n+\tparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual) &&\n \ttest \"z$commit0\" = \"z$parent\"\n '\n \n test_expect_success 'git commit-tree omits duplicated parent in a commit' '\n \tcommit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&\n-\tparent=$(git show --pretty=raw $commit2 |\n-\t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" |\n-\t\tsort -u) &&\n+\tgit show --pretty=raw $commit2 >actual &&\n+\tparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | sort -u) &&\n \ttest \"z$commit0\" = \"z$parent\" &&\n-\tnumparent=$(git show --pretty=raw $commit2 |\n-\t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" |\n-\t\twc -l) &&\n+\tgit show --pretty=raw $commit2 >actual &&\n+\tnumparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | wc -l) &&\n \ttest $numparent = 1\n '\n \n@@ -1147,7 +1145,8 @@ test_expect_success 'update-index D/F conflict' '\n \tmv path2 path0 &&\n \tmv tmp path2 &&\n \tgit update-index --add --replace path2 path0/file2 &&\n-\tnumpath0=$(git ls-files path0 | wc -l) &&\n+\tgit ls-files path0 >actual &&\n+\tnumpath0=$(wc -l path4 &&\n \tgit update-index --add path4 &&\n \t(\n-\t\tgit ls-files -s path4 |\n-\t\tsed -e \"s/\t.*/\t/\" |\n+\t\tgit ls-files -s path4 >actual &&\n+\t\tsed -e \"s/\t.*/\t/\" actual |\n \t\ttr -d \"\\012\" &&\n \t\techo \"$a\"\n \t) | git update-index --index-info &&\n-\tlen=$(git ls-files \"a*\" | wc -c) &&\n+\tgit ls-files \"a*\" >actual &&\n+\tlen=$(wc -c \n---\n t/t0000-basic.sh | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 47666b013e..3de13daabe 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -1136,8 +1136,8 @@ test_expect_success 'git commit-tree omits duplicated parent in a commit' '\n \tparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | sort -u) &&\n \ttest \"z$commit0\" = \"z$parent\" &&\n \tgit show --pretty=raw $commit2 >actual &&\n-\tnumparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | wc -l) &&\n-\ttest $numparent = 1\n+\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" actual >parents &&\n+\ttest_line_count = 1 parents\n '\n \n test_expect_success 'update-index D/F conflict' '\n@@ -1146,8 +1146,7 @@ test_expect_success 'update-index D/F conflict' '\n \tmv tmp path2 &&\n \tgit update-index --add --replace path2 path0/file2 &&\n \tgit ls-files path0 >actual &&\n-\tnumpath0=$(wc -l The exit code of the upstream in a pipe is ignored thus we should avoid\n> using it. By writing out the output of the git command to a file, we can\n> test the exit codes of both the commands.\n>\n> Signed-off-by: Jonathan Chang \n> ---\n> t/t0000-basic.sh | 28 ++++++++++++++--------------\n> 1 file changed, 14 insertions(+), 14 deletions(-)\n>\n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> index 53821f5817..47666b013e 100755\n> --- a/t/t0000-basic.sh\n> +++ b/t/t0000-basic.sh\n> @@ -1118,27 +1118,25 @@ P=$(test_oid root)\n>\n> test_expect_success 'git commit-tree records the correct tree in a commit' '\n> \tcommit0=$(echo NO | git commit-tree $P) &&\n> -\ttree=$(git show --pretty=raw $commit0 |\n> -\t\t sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n> +\tgit show --pretty=raw $commit0 >actual &&\n> +\ttree=$(sed -n -e \"s/^tree //p\" -e \"/^author /q\" actual) &&\n> \ttest \"z$tree\" = \"z$P\"\n\nThis change is an improvement just changing the \"git\" invocations. But I\nwonder as we're reviewing this / churning this if we couldn't also\nmodernize this style to just:\n\n git .. >tmp &&\n sed -n -e actual &&\n test_must_be_empty actual\n\n> '\n>\n> test_expect_success 'git commit-tree records the correct parent in a commit' '\n> \tcommit1=$(echo NO | git commit-tree $P -p $commit0) &&\n> -\tparent=$(git show --pretty=raw $commit1 |\n> -\t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\") &&\n> +\tgit show --pretty=raw $commit1 >actual &&\n> +\tparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual) &&\n> \ttest \"z$commit0\" = \"z$parent\"\n\nditto.\n\n> '\n>\n> test_expect_success 'git commit-tree omits duplicated parent in a commit' '\n> \tcommit2=$(echo NO | git commit-tree $P -p $commit0 -p $commit0) &&\n> -\tparent=$(git show --pretty=raw $commit2 |\n> -\t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" |\n> -\t\tsort -u) &&\n> +\tgit show --pretty=raw $commit2 >actual &&\n> +\tparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | sort -u) &&\n> \ttest \"z$commit0\" = \"z$parent\" &&\n> -\tnumparent=$(git show --pretty=raw $commit2 |\n> -\t\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" |\n> -\t\twc -l) &&\n> +\tgit show --pretty=raw $commit2 >actual &&\n> +\tnumparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | wc -l) &&\n> \ttest $numparent = 1\n\nAnd stuff like this to (skipping the wc -l):\n\n sed -n -e actual &&\n test_line_count = 1 actual\n\n> '\n>\n> @@ -1147,7 +1145,8 @@ test_expect_success 'update-index D/F conflict' '\n> \tmv path2 path0 &&\n> \tmv tmp path2 &&\n> \tgit update-index --add --replace path2 path0/file2 &&\n> -\tnumpath0=$(git ls-files path0 | wc -l) &&\n> +\tgit ls-files path0 >actual &&\n> +\tnumpath0=$(wc -l \ttest $numpath0 = 1\n\nditto.\n\n> '\n>\n> @@ -1162,12 +1161,13 @@ test_expect_success 'very long name in the index handled sanely' '\n> \t>path4 &&\n> \tgit update-index --add path4 &&\n> \t(\n> -\t\tgit ls-files -s path4 |\n> -\t\tsed -e \"s/\t.*/\t/\" |\n> +\t\tgit ls-files -s path4 >actual &&\n> +\t\tsed -e \"s/\t.*/\t/\" actual |\n> \t\ttr -d \"\\012\" &&\n> \t\techo \"$a\"\n> \t) | git update-index --index-info &&\n> -\tlen=$(git ls-files \"a*\" | wc -c) &&\n> +\tgit ls-files \"a*\" >actual &&\n> +\tlen=$(wc -c \ttest $len = 4098\n\nDitto. Maybe the initial author wanted to avoid writing out 4k lines,\nbut now that we're doing so anyway...\n"},{"id":"371735","messageId":"87h8c1a1mi.fsf@evledraar.gmail.com","threadId":"50766","inReplyTo":"bc3dee82a91592e50fd39c5a332b13b76fefa009.1552835153.git.ttjtftx@gmail.com","subject":"Re: [GSoC][PATCH v3 3/3] t0000: use test_line_count instead of wc -l","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-17T16:48:05Z","receivedAt":"2019-03-17T16:48:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Mar 17 2019, Jonathan Chang wrote:\n\n> Signed-off-by: Jonathan Chang \n> ---\n> t/t0000-basic.sh | 7 +++----\n> 1 file changed, 3 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> index 47666b013e..3de13daabe 100755\n> --- a/t/t0000-basic.sh\n> +++ b/t/t0000-basic.sh\n> @@ -1136,8 +1136,8 @@ test_expect_success 'git commit-tree omits duplicated parent in a commit' '\n> \tparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | sort -u) &&\n> \ttest \"z$commit0\" = \"z$parent\" &&\n> \tgit show --pretty=raw $commit2 >actual &&\n> -\tnumparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | wc -l) &&\n> -\ttest $numparent = 1\n> +\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" actual >parents &&\n> +\ttest_line_count = 1 parents\n> '\n>\n> test_expect_success 'update-index D/F conflict' '\n> @@ -1146,8 +1146,7 @@ test_expect_success 'update-index D/F conflict' '\n> \tmv tmp path2 &&\n> \tgit update-index --add --replace path2 path0/file2 &&\n> \tgit ls-files path0 >actual &&\n> -\tnumpath0=$(wc -l -\ttest $numpath0 = 1\n> +\ttest_line_count = 1 actual\n> '\n\n...of course reading this series in sequence I find that 50% of my\nsuggestions for 2/3 are then done in this patch... :)\n"},{"id":"371751","messageId":"20190317200609.GA1216@hank.intra.tgummerer.com","threadId":"50766","inReplyTo":"87h8c1a1mi.fsf@evledraar.gmail.com","subject":"Re: [GSoC][PATCH v3 3/3] t0000: use test_line_count instead of wc -l","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-03-17T20:06:09Z","receivedAt":"2019-03-17T20:06:14Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/17, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Sun, Mar 17 2019, Jonathan Chang wrote:\n> \n> > Signed-off-by: Jonathan Chang \n> > ---\n> > t/t0000-basic.sh | 7 +++----\n> > 1 file changed, 3 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> > index 47666b013e..3de13daabe 100755\n> > --- a/t/t0000-basic.sh\n> > +++ b/t/t0000-basic.sh\n> > @@ -1136,8 +1136,8 @@ test_expect_success 'git commit-tree omits duplicated parent in a commit' '\n> > \tparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | sort -u) &&\n> > \ttest \"z$commit0\" = \"z$parent\" &&\n> > \tgit show --pretty=raw $commit2 >actual &&\n> > -\tnumparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | wc -l) &&\n> > -\ttest $numparent = 1\n> > +\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" actual >parents &&\n> > +\ttest_line_count = 1 parents\n> > '\n> >\n> > test_expect_success 'update-index D/F conflict' '\n> > @@ -1146,8 +1146,7 @@ test_expect_success 'update-index D/F conflict' '\n> > \tmv tmp path2 &&\n> > \tgit update-index --add --replace path2 path0/file2 &&\n> > \tgit ls-files path0 >actual &&\n> > -\tnumpath0=$(wc -l > -\ttest $numpath0 = 1\n> > +\ttest_line_count = 1 actual\n> > '\n> \n> ...of course reading this series in sequence I find that 50% of my\n> suggestions for 2/3 are then done in this patch... :)\n\nIndeed. I think doing this in a separate patch is a good idea, as it\nmakes the previous patch slightly easier to review imho. But I think\nsomething to take away from this for Jonathan would be that this\nshould have been described in the commit message of the previous\ncommit. Maybe something like\n\n This commit doesn't make any additional simplifications, such as\n using the test_line_count function for counting the lines in the\n output. These simplifications will be made in a subsequent commit.\n\nin addition to the existing commit message would have helped save a\nbit of review effort.\n"},{"id":"371791","messageId":"87ftrkab2w.fsf@evledraar.gmail.com","threadId":"50766","inReplyTo":"20190317200609.GA1216@hank.intra.tgummerer.com","subject":"Re: [GSoC][PATCH v3 3/3] t0000: use test_line_count instead of wc -l","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-18T07:36:07Z","receivedAt":"2019-03-18T07:36:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Mar 17 2019, Thomas Gummerer wrote:\n\n> On 03/17, Ævar Arnfjörð Bjarmason wrote:\n>>\n>> On Sun, Mar 17 2019, Jonathan Chang wrote:\n>>\n>> > Signed-off-by: Jonathan Chang \n>> > ---\n>> > t/t0000-basic.sh | 7 +++----\n>> > 1 file changed, 3 insertions(+), 4 deletions(-)\n>> >\n>> > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n>> > index 47666b013e..3de13daabe 100755\n>> > --- a/t/t0000-basic.sh\n>> > +++ b/t/t0000-basic.sh\n>> > @@ -1136,8 +1136,8 @@ test_expect_success 'git commit-tree omits duplicated parent in a commit' '\n>> > \tparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | sort -u) &&\n>> > \ttest \"z$commit0\" = \"z$parent\" &&\n>> > \tgit show --pretty=raw $commit2 >actual &&\n>> > -\tnumparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | wc -l) &&\n>> > -\ttest $numparent = 1\n>> > +\tsed -n -e \"s/^parent //p\" -e \"/^author /q\" actual >parents &&\n>> > +\ttest_line_count = 1 parents\n>> > '\n>> >\n>> > test_expect_success 'update-index D/F conflict' '\n>> > @@ -1146,8 +1146,7 @@ test_expect_success 'update-index D/F conflict' '\n>> > \tmv tmp path2 &&\n>> > \tgit update-index --add --replace path2 path0/file2 &&\n>> > \tgit ls-files path0 >actual &&\n>> > -\tnumpath0=$(wc -l > > -\ttest $numpath0 = 1\n>> > +\ttest_line_count = 1 actual\n>> > '\n>>\n>> ...of course reading this series in sequence I find that 50% of my\n>> suggestions for 2/3 are then done in this patch... :)\n>\n> Indeed. I think doing this in a separate patch is a good idea, as it\n> makes the previous patch slightly easier to review imho. But I think\n> something to take away from this for Jonathan would be that this\n> should have been described in the commit message of the previous\n> commit. Maybe something like\n>\n> This commit doesn't make any additional simplifications, such as\n> using the test_line_count function for counting the lines in the\n> output. These simplifications will be made in a subsequent commit.\n>\n> in addition to the existing commit message would have helped save a\n> bit of review effort.\n\nFWIW I chuck this up to just my blindness / expedience in reading the\nthing.\n\nNo objections to changing this, but I don't think it's the fault of a\ncommit message if someone reading it doesn't get an explanation for a\nfuture unrelated improvement.\n\nThe times when a commit should have such an explanation are cases like\ne.g. introducing a function that's not used yet to make a subsequent\ncommit smaller, or other such cases where the change is incomplete in\nsome way.\n"},{"id":"371794","messageId":"xmqqpnqozjh1.fsf@gitster-ct.c.googlers.com","threadId":"50766","inReplyTo":"87ftrkab2w.fsf@evledraar.gmail.com","subject":"Re: [GSoC][PATCH v3 3/3] t0000: use test_line_count instead of wc -l","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-18T08:15:38Z","receivedAt":"2019-03-18T08:15:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> No objections to changing this, but I don't think it's the fault of a\n> commit message if someone reading it doesn't get an explanation for a\n> future unrelated improvement.\n>\n> The times when a commit should have such an explanation are cases like\n> e.g. introducing a function that's not used yet to make a subsequent\n> commit smaller, or other such cases where the change is incomplete in\n> some way.\n\nDeliberately omitting an obvious improvement \"while at it\" would be\nsimilar to adding a function that is not used yet. Careful readers\nnotice \"why don't do it at this step while touching the same area?\"\njust like they would notice \"why do it at this step when it is not\nyet used?\". While it certainly is *not* required, they would find\nit helpful to answer such questions, erring on the safer side.\n"},{"id":"372350","messageId":"CAOAu_YJs_ZL0nAARNmdSFjmdh6mSP+e7KvzNB3wj-JGe5sHcdA@mail.gmail.com","threadId":"50766","inReplyTo":"87imwha1o3.fsf@evledraar.gmail.com","subject":"Re: [GSoC][PATCH v3 2/3] t0000: avoid using pipes","fromName":"jonathan chang","fromEmail":"ttjtftx@gmail.com","sentAt":"2019-03-24T11:26:32Z","receivedAt":"2019-03-24T11:26:47Z","isPatch":true,"sender":{"key":"ttjtftx@gmail.com","avatar":null},"body":"On Mon, Mar 18, 2019 at 12:47 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Sun, Mar 17 2019, Jonathan Chang wrote:\n>\n> > The exit code of the upstream in a pipe is ignored thus we should avoid\n> > using it. By writing out the output of the git command to a file, we can\n> > test the exit codes of both the commands.\n> >\n> > Signed-off-by: Jonathan Chang \n> > ---\n> > t/t0000-basic.sh | 28 ++++++++++++++--------------\n> > 1 file changed, 14 insertions(+), 14 deletions(-)\n> >\n> > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> > index 53821f5817..47666b013e 100755\n> > --- a/t/t0000-basic.sh\n> > +++ b/t/t0000-basic.sh\n> > @@ -1118,27 +1118,25 @@ P=$(test_oid root)\n> >\n> > test_expect_success 'git commit-tree records the correct tree in a commit' '\n> > commit0=$(echo NO | git commit-tree $P) &&\n> > - tree=$(git show --pretty=raw $commit0 |\n> > - sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n> > + git show --pretty=raw $commit0 >actual &&\n> > + tree=$(sed -n -e \"s/^tree //p\" -e \"/^author /q\" actual) &&\n> > test \"z$tree\" = \"z$P\"\n>\n> This change is an improvement just changing the \"git\" invocations. But I\n> wonder as we're reviewing this / churning this if we couldn't also\n> modernize this style to just:\n>\n> git .. >tmp &&\n> sed -n -e actual &&\n> test_must_be_empty actual\n\nDo you mean something like this:\n\n- git show --pretty=raw $commit0 >actual &&\n- tree=$(sed -n -e \"s/^tree //p\" -e \"/^author /q\" actual) &&\n- test \"z$tree\" = \"z$P\"\n+ git show --pretty=raw $commit0 >tmp &&\n+ sed -n -e \"/$P/d\" -e \"s/^tree //p\" -e \"/^author /q\" tmp >actual &&\n+ test_must_be_empty actual\n\nIt works. But the semantic is different if we use test_must_be_empty.\nI wonder if you mean test_cmp because I found some commits[1 2 3]\nthat changes 'test \"z...' to use test_cmp with 'git log -G 'test \"z' --oneline'\nand git-show. However, they are all around 2013, so I'm not so sure\nthis is what you mean either.\n\nI did found some use of sed's 'd' function in conjunction with\ntest_must_be_empty, using:\n git grep -A 5 'sed .*/d' | grep -B 5 'test_must_be_empty'\nHowever, they don't use parameter expansion in sed.\n\nThere are some places where parameter expansion is used in sed, but\nthat would require test_cmp in this case, and would need to write to\nanother file to compare.\n\nMaybe this 'test \"z$A\" = \"z$B\"' syntax is fine, yet most of the existing\nusages are added around 12 years ago according git-blame I saw on\ngithub.\n\n\n[1]: 03c893cbf9 (\"t1006: modernize output comparisons\", 2013-07-10)\n[2]: 848575d833 (\"push test: simplify check of push result\", 2013-03-18)\n[3]: ed838e6615 (\"t1300: style updates\", 2012-10-23)\n\n\n> > @@ -1162,12 +1161,13 @@ test_expect_success 'very long name in the index handled sanely' '\n> > >path4 &&\n> > git update-index --add path4 &&\n> > (\n> > - git ls-files -s path4 |\n> > - sed -e \"s/ .*/ /\" |\n> > + git ls-files -s path4 >actual &&\n> > + sed -e \"s/ .*/ /\" actual |\n> > tr -d \"\\012\" &&\n> > echo \"$a\"\n> > ) | git update-index --index-info &&\n> > - len=$(git ls-files \"a*\" | wc -c) &&\n> > + git ls-files \"a*\" >actual &&\n> > + len=$(wc -c > test $len = 4098\n>\n> Ditto. Maybe the initial author wanted to avoid writing out 4k lines,\n> but now that we're doing so anyway...\n\nThis is 'wc -c', so I think I don't have to modify it?\n\nBy the way, I found 2 lines that can be changed to use test_must_be_empty\nin t0000-basic.sh. I paste the diff below:\n---\n@@ -51,7 +51,7 @@ test_expect_success 'verify that the running shell\nsupports \"local\"' '\n\n test_expect_success '.git/objects should be empty after git init in\nan empty repo' '\n find .git/objects -type f -print >should-be-empty &&\n- test_line_count = 0 should-be-empty\n+ test_must_be_empty should-be-empty\n '\n\n # also it should have 2 subdirectories; no fan-out anymore, pack, and info.\n@@ -1110,7 +1110,7 @@ test_expect_success 'git update-index --refresh\nshould succeed' '\n\n test_expect_success 'no diff after checkout and git update-index --refresh' '\n git diff-files >current &&\n- cmp -s current /dev/null\n+ test_must_be_empty current\n '\n"},{"id":"372376","messageId":"875zs8ccvx.fsf@evledraar.gmail.com","threadId":"50766","inReplyTo":"CAOAu_YJs_ZL0nAARNmdSFjmdh6mSP+e7KvzNB3wj-JGe5sHcdA@mail.gmail.com","subject":"Re: [GSoC][PATCH v3 2/3] t0000: avoid using pipes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-03-24T19:04:34Z","receivedAt":"2019-03-24T19:04:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Mar 24 2019, jonathan chang wrote:\n\n> On Mon, Mar 18, 2019 at 12:47 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>>\n>> On Sun, Mar 17 2019, Jonathan Chang wrote:\n>>\n>> > The exit code of the upstream in a pipe is ignored thus we should avoid\n>> > using it. By writing out the output of the git command to a file, we can\n>> > test the exit codes of both the commands.\n>> >\n>> > Signed-off-by: Jonathan Chang \n>> > ---\n>> > t/t0000-basic.sh | 28 ++++++++++++++--------------\n>> > 1 file changed, 14 insertions(+), 14 deletions(-)\n>> >\n>> > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n>> > index 53821f5817..47666b013e 100755\n>> > --- a/t/t0000-basic.sh\n>> > +++ b/t/t0000-basic.sh\n>> > @@ -1118,27 +1118,25 @@ P=$(test_oid root)\n>> >\n>> > test_expect_success 'git commit-tree records the correct tree in a commit' '\n>> > commit0=$(echo NO | git commit-tree $P) &&\n>> > - tree=$(git show --pretty=raw $commit0 |\n>> > - sed -n -e \"s/^tree //p\" -e \"/^author /q\") &&\n>> > + git show --pretty=raw $commit0 >actual &&\n>> > + tree=$(sed -n -e \"s/^tree //p\" -e \"/^author /q\" actual) &&\n>> > test \"z$tree\" = \"z$P\"\n>>\n>> This change is an improvement just changing the \"git\" invocations. But I\n>> wonder as we're reviewing this / churning this if we couldn't also\n>> modernize this style to just:\n>>\n>> git .. >tmp &&\n>> sed -n -e actual &&\n>> test_must_be_empty actual\n>\n> Do you mean something like this:\n>\n> - git show --pretty=raw $commit0 >actual &&\n> - tree=$(sed -n -e \"s/^tree //p\" -e \"/^author /q\" actual) &&\n> - test \"z$tree\" = \"z$P\"\n> + git show --pretty=raw $commit0 >tmp &&\n> + sed -n -e \"/$P/d\" -e \"s/^tree //p\" -e \"/^author /q\" tmp >actual &&\n> + test_must_be_empty actual\n>\n> It works. But the semantic is different if we use test_must_be_empty.\n> I wonder if you mean test_cmp because I found some commits[1 2 3]\n> that changes 'test \"z...' to use test_cmp with 'git log -G 'test \"z' --oneline'\n> and git-show. However, they are all around 2013, so I'm not so sure\n> this is what you mean either.\n>\n> I did found some use of sed's 'd' function in conjunction with\n> test_must_be_empty, using:\n> git grep -A 5 'sed .*/d' | grep -B 5 'test_must_be_empty'\n> However, they don't use parameter expansion in sed.\n>\n> There are some places where parameter expansion is used in sed, but\n> that would require test_cmp in this case, and would need to write to\n> another file to compare.\n>\n> Maybe this 'test \"z$A\" = \"z$B\"' syntax is fine, yet most of the existing\n> usages are added around 12 years ago according git-blame I saw on\n> github.\n>\n>\n> [1]: 03c893cbf9 (\"t1006: modernize output comparisons\", 2013-07-10)\n> [2]: 848575d833 (\"push test: simplify check of push result\", 2013-03-18)\n> [3]: ed838e6615 (\"t1300: style updates\", 2012-10-23)\n\nYeah there's many ways to do it. Generally we've been moving to the\n\"test_*\" wrapper that e.g. given X lines print out an explanation that\nwe were expecting Y instead.\n\n>> > @@ -1162,12 +1161,13 @@ test_expect_success 'very long name in the index handled sanely' '\n>> > >path4 &&\n>> > git update-index --add path4 &&\n>> > (\n>> > - git ls-files -s path4 |\n>> > - sed -e \"s/ .*/ /\" |\n>> > + git ls-files -s path4 >actual &&\n>> > + sed -e \"s/ .*/ /\" actual |\n>> > tr -d \"\\012\" &&\n>> > echo \"$a\"\n>> > ) | git update-index --index-info &&\n>> > - len=$(git ls-files \"a*\" | wc -c) &&\n>> > + git ls-files \"a*\" >actual &&\n>> > + len=$(wc -c > > test $len = 4098\n>>\n>> Ditto. Maybe the initial author wanted to avoid writing out 4k lines,\n>> but now that we're doing so anyway...\n>\n> This is 'wc -c', so I think I don't have to modify it?\n\nYes, that's a brainfart of mine. Although as an aside I have a WIP\nseries that adds test_byte_count which can be used for these types of\ncases.\n\n> By the way, I found 2 lines that can be changed to use test_must_be_empty\n> in t0000-basic.sh. I paste the diff below:\n> ---\n> @@ -51,7 +51,7 @@ test_expect_success 'verify that the running shell\n> supports \"local\"' '\n>\n> test_expect_success '.git/objects should be empty after git init in\n> an empty repo' '\n> find .git/objects -type f -print >should-be-empty &&\n> - test_line_count = 0 should-be-empty\n> + test_must_be_empty should-be-empty\n> '\n>\n> # also it should have 2 subdirectories; no fan-out anymore, pack, and info.\n> @@ -1110,7 +1110,7 @@ test_expect_success 'git update-index --refresh\n> should succeed' '\n>\n> test_expect_success 'no diff after checkout and git update-index --refresh' '\n> git diff-files >current &&\n> - cmp -s current /dev/null\n> + test_must_be_empty current\n> '\n"}]}