{"thread":{"id":"50703","subject":"[GSoC][PATCH] tests: avoid using pipes","startedAt":"2019-03-09T15:46:22Z","lastAt":"2019-03-15T12:48:26Z","messageCount":20,"participants":["Jonathan Chang","Thomas Gummerer","Christian Couder","ttjtftx","Eric Sunshine","jonathan chang"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"371031","messageId":"20190309154555.33407-1-ttjtftx@gmail.com","threadId":"50703","inReplyTo":null,"subject":"[GSoC][PATCH] tests: avoid using pipes","fromName":"Jonathan Chang","fromEmail":"ttjtftx@gmail.com","sentAt":"2019-03-09T15:45:55Z","receivedAt":"2019-03-09T15:46:22Z","isPatch":true,"sender":{"key":"ttjtftx@gmail.com","avatar":null},"body":"This is my attempt for this year's GSoC microproject.\n\nI copied the commit message of the commit[1] mentioned in the microproject\npage[2]. Is this OK?\n\nHere is a summary of what I did:\n\t- simple substitution as in c6f44e1da5[1]. \n\t- besides simple substitution, I moved some git command out of command \n\t\tsubstitution to improve readability, which was not possible with pipes, \n\t\twhile in these cases keeping them inside won't discard git's exit code.\n\t- use actual1 and actual2 in cases where actual is already in use.\n\t- use `sort -o actual actual` to avoid using actual1 and actual2.\n\n[1] c6f44e1da5 (\"t9813: avoid using pipes\", 2017-01-04)\n[2] https://git.github.io/SoC-2019-Microprojects/\n\n---------------------->8-------------------\nThe 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 <ttjtftx@gmail.com>\n---\n t/t0000-basic.sh       | 28 ++++++++++++++--------------\n t/t0003-attributes.sh  | 13 ++++++++-----\n t/t0022-crlf-rename.sh |  6 +++---\n 3 files changed, 25 insertions(+), 22 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex b6566003dd..adc9545973 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-\t     parent=$(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 actual) &&\n \ttest $numpath0 = 1\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 actual) &&\n \ttest $len = 4098\n '\n \ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 71e63d8b50..14274f1ced 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -203,15 +203,18 @@ test_expect_success 'attribute test: read paths from stdin' '\n test_expect_success 'attribute test: --all option' '\n \tgrep -v unspecified <expect-all | sort >specified-all &&\n \tsed -e \"s/:.*//\" <expect-all | uniq >stdin-all &&\n-\tgit check-attr --stdin --all <stdin-all | sort >actual &&\n+\tgit check-attr --stdin --all <stdin-all >actual &&\n+\tsort -o actual actual &&\n \ttest_cmp specified-all actual\n '\n \n test_expect_success 'attribute test: --cached option' '\n-\tgit check-attr --cached --stdin --all <stdin-all | sort >actual &&\n+\tgit check-attr --cached --stdin --all <stdin-all >actual &&\n+\tsort -o actual actual &&\n \ttest_must_be_empty actual &&\n \tgit add .gitattributes a/.gitattributes a/b/.gitattributes &&\n-\tgit check-attr --cached --stdin --all <stdin-all | sort >actual &&\n+\tgit check-attr --cached --stdin --all <stdin-all >actual &&\n+\tsort -o actual actual &&\n \ttest_cmp specified-all actual\n '\n \n@@ -301,8 +304,8 @@ test_expect_success 'bare repository: check that --cached honors index' '\n \t(\n \t\tcd bare.git &&\n \t\tGIT_INDEX_FILE=../.git/index \\\n-\t\tgit check-attr --cached --stdin --all <../stdin-all |\n-\t\tsort >actual &&\n+\t\tgit check-attr --cached --stdin --all <../stdin-all >actual &&\n+\t\tsort -o actual actual &&\n \t\ttest_cmp ../specified-all actual\n \t)\n '\ndiff --git a/t/t0022-crlf-rename.sh b/t/t0022-crlf-rename.sh\nindex 7af3fbcc7b..b4772b72f7 100755\n--- a/t/t0022-crlf-rename.sh\n+++ b/t/t0022-crlf-rename.sh\n@@ -23,10 +23,10 @@ test_expect_success setup '\n \n test_expect_success 'diff -M' '\n \n-\tgit diff-tree -M -r --name-status HEAD^ HEAD |\n-\tsed -e \"s/R[0-9]*/RNUM/\" >actual &&\n+\tgit diff-tree -M -r --name-status HEAD^ HEAD >actual1 &&\n+\tsed -e \"s/R[0-9]*/RNUM/\" actual1 >actual2 &&\n \techo \"RNUM\tsample\telpmas\" >expect &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual2\n \n '\n \n-- \n2.21.0\n\n"},{"id":"371034","messageId":"20190309164508.GB31533@hank.intra.tgummerer.com","threadId":"50703","inReplyTo":"20190309154555.33407-1-ttjtftx@gmail.com","subject":"Re: [GSoC][PATCH] tests: avoid using pipes","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-03-09T16:45:08Z","receivedAt":"2019-03-09T16:45:14Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"Hi,\n\nwelcome and thanks for the patch!\n\nOn 03/09, Jonathan Chang wrote:\n> This is my attempt for this year's GSoC microproject.\n> \n> I copied the commit message of the commit[1] mentioned in the microproject\n> page[2]. Is this OK?\n> \n> Here is a summary of what I did:\n> \t- simple substitution as in c6f44e1da5[1]. \n> \t- besides simple substitution, I moved some git command out of command \n> \t\tsubstitution to improve readability, which was not possible with pipes, \n> \t\twhile in these cases keeping them inside won't discard git's exit code.\n> \t- use actual1 and actual2 in cases where actual is already in use.\n> \t- use `sort -o actual actual` to avoid using actual1 and actual2.\n> \n> [1] c6f44e1da5 (\"t9813: avoid using pipes\", 2017-01-04)\n> [2] https://git.github.io/SoC-2019-Microprojects/\n> \n> ---------------------->8-------------------\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 <ttjtftx@gmail.com>\n> ---\n>  t/t0000-basic.sh       | 28 ++++++++++++++--------------\n>  t/t0003-attributes.sh  | 13 ++++++++-----\n>  t/t0022-crlf-rename.sh |  6 +++---\n>  3 files changed, 25 insertions(+), 22 deletions(-)\n> \n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> index b6566003dd..adc9545973 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> -\t     parent=$(git show --pretty=raw $commit2 |\n\nThis line is a bit oddly indented.  This is of course not something\nyou did, but it did leave me puzzled why the indentation was here\nbefore and if the conversion is actually correct.  To help reviewers,\nit would be nice to fix the indentation of this line in a preparatory\npatch, and clarify since when this indentation was there and that it\nis not correct.\n\nThat is if I'm not missing something, and there is not actually a\ngood reason for this to be indented, but judging from the original and\nthe conversion in this commit, I don't think there is.\n\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 actual) &&\n\nThis test is actually failing now (and so is the one just below).  The\nreason is that 'wc -l <file>' outputs the number of lines followed by\nthe filename, so numpath0 includes that.\n\nWe have a helper function that avoids exactly that,\n'test_line_count'.  See t/README or t/test-lib-functions.sh for more\ninformation on that function.\n\nBefore submitting a patch series, please also run the test suite, or\nif you are only modifying tests such as in this case, at least the\ntests that are modified.\n\n>  \ttest $numpath0 = 1\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 actual) &&\n>  \ttest $len = 4098\n>  '\n>  \n> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\n> index 71e63d8b50..14274f1ced 100755\n> --- a/t/t0003-attributes.sh\n> +++ b/t/t0003-attributes.sh\n> @@ -203,15 +203,18 @@ test_expect_success 'attribute test: read paths from stdin' '\n>  test_expect_success 'attribute test: --all option' '\n>  \tgrep -v unspecified <expect-all | sort >specified-all &&\n>  \tsed -e \"s/:.*//\" <expect-all | uniq >stdin-all &&\n> -\tgit check-attr --stdin --all <stdin-all | sort >actual &&\n> +\tgit check-attr --stdin --all <stdin-all >actual &&\n> +\tsort -o actual actual &&\n>  \ttest_cmp specified-all actual\n>  '\n>  \n>  test_expect_success 'attribute test: --cached option' '\n> -\tgit check-attr --cached --stdin --all <stdin-all | sort >actual &&\n> +\tgit check-attr --cached --stdin --all <stdin-all >actual &&\n> +\tsort -o actual actual &&\n>  \ttest_must_be_empty actual &&\n>  \tgit add .gitattributes a/.gitattributes a/b/.gitattributes &&\n> -\tgit check-attr --cached --stdin --all <stdin-all | sort >actual &&\n> +\tgit check-attr --cached --stdin --all <stdin-all >actual &&\n> +\tsort -o actual actual &&\n>  \ttest_cmp specified-all actual\n>  '\n>  \n> @@ -301,8 +304,8 @@ test_expect_success 'bare repository: check that --cached honors index' '\n>  \t(\n>  \t\tcd bare.git &&\n>  \t\tGIT_INDEX_FILE=../.git/index \\\n> -\t\tgit check-attr --cached --stdin --all <../stdin-all |\n> -\t\tsort >actual &&\n> +\t\tgit check-attr --cached --stdin --all <../stdin-all >actual &&\n> +\t\tsort -o actual actual &&\n>  \t\ttest_cmp ../specified-all actual\n>  \t)\n>  '\n> diff --git a/t/t0022-crlf-rename.sh b/t/t0022-crlf-rename.sh\n> index 7af3fbcc7b..b4772b72f7 100755\n> --- a/t/t0022-crlf-rename.sh\n> +++ b/t/t0022-crlf-rename.sh\n> @@ -23,10 +23,10 @@ test_expect_success setup '\n>  \n>  test_expect_success 'diff -M' '\n>  \n> -\tgit diff-tree -M -r --name-status HEAD^ HEAD |\n> -\tsed -e \"s/R[0-9]*/RNUM/\" >actual &&\n> +\tgit diff-tree -M -r --name-status HEAD^ HEAD >actual1 &&\n> +\tsed -e \"s/R[0-9]*/RNUM/\" actual1 >actual2 &&\n\nNit: rather than actual1 and actual2, it might be nice to give the\nfiles slightly more meaningful names, maybe \"output\" and \"normalized\",\nbut feel free to try and come up with something better than that.\n\n>  \techo \"RNUM\tsample\telpmas\" >expect &&\n> -\ttest_cmp expect actual\n> +\ttest_cmp expect actual2\n>  \n>  '\n>  \n> -- \n> 2.21.0\n> \n"},{"id":"371060","messageId":"CAP8UFD26Zxt1FvwAajV48eBbhPv_q5oSLTekWxApcNpg=xRn3Q@mail.gmail.com","threadId":"50703","inReplyTo":"20190309154555.33407-1-ttjtftx@gmail.com","subject":"Re: [GSoC][PATCH] tests: avoid using pipes","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-10T06:05:34Z","receivedAt":"2019-03-10T06:07:59Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Mar 9, 2019 at 4:50 PM Jonathan Chang <ttjtftx@gmail.com> wrote:\n>\n> This is my attempt for this year's GSoC microproject.\n\nThanks for being interested in this year's GSoC and doing a microproject.\n\n> I copied the commit message of the commit[1] mentioned in the microproject\n> page[2]. Is this OK?\n>\n> Here is a summary of what I did:\n>         - simple substitution as in c6f44e1da5[1].\n\nIf you take a look at c6f44e1da5 (\"t9813: avoid using pipes\",\n2017-01-04) you can see the following:\n\n    - it changes only one test file: t9813-git-p4-preserve-users.sh\n    - its title starts with \"t9813: \"\n\nIt would have been nice if you did the same in your patch (that is\nonly change one test file and start the patch title with the test file\nnumber).\n\nOn the microproject page, we say:\n\n\"Students: Please attempt only ONE microproject. We want quality, not quantity!\"\n\nAnd to other students working on a microproject, we also ask them to\nfocus on only one test file when they make those kinds of changes.\n\nBest,\nChristian.\n"},{"id":"371064","messageId":"20190310080739.63984-1-ttjtftx@gmail.com","threadId":"50703","inReplyTo":"20190309164508.GB31533@hank.intra.tgummerer.com","subject":"[GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error","fromName":"Jonathan Chang","fromEmail":"ttjtftx@gmail.com","sentAt":"2019-03-10T08:07:39Z","receivedAt":"2019-03-10T08:08:03Z","isPatch":true,"sender":{"key":"ttjtftx@gmail.com","avatar":null},"body":"Hi,\n\nThanks for the reviews.\n\nHere are the changes in the second version:\n\t- bug fixes\n\t- add preparatory patch\n\t- seperate different files to different patch\n\t- change to use test_line_count in a seperate patch\n\nAlso I found that there is no such function as test_char_count, \nis it worthwile to add such function? Here are some stat:\n\n`git grep 'test_line_count' | wc -l` = 626\n`git grep 'wc -l' | wc -l` = 294\n`git grep 'wc -c' | wc -l` = 68\n\n-- >8 --\n\nThis is a preparatory step prior to removing the pipes after git\ncommands, which discards git's exit code and may mask a crash.\n\nSigned-off-by: Jonathan Chang <ttjtftx@gmail.com>\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":"371065","messageId":"20190310080841.64082-1-ttjtftx@gmail.com","threadId":"50703","inReplyTo":"20190310080739.63984-1-ttjtftx@gmail.com","subject":"[GSoC][PATCH v2 2/5] t0000-basic: avoid using pipes","fromName":"Jonathan Chang","fromEmail":"ttjtftx@gmail.com","sentAt":"2019-03-10T08:08:41Z","receivedAt":"2019-03-10T08:08:54Z","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 <ttjtftx@gmail.com>\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 <actual) &&\n \ttest $numpath0 = 1\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 <actual) &&\n \ttest $len = 4098\n '\n \n-- \n2.21.0\n\n"},{"id":"371066","messageId":"20190310080931.64134-1-ttjtftx@gmail.com","threadId":"50703","inReplyTo":"20190310080841.64082-1-ttjtftx@gmail.com","subject":"[GSoC][PATCH v2 3/5] t0003-attributes: avoid using pipes","fromName":"Jonathan Chang","fromEmail":"ttjtftx@gmail.com","sentAt":"2019-03-10T08:09:31Z","receivedAt":"2019-03-10T08:09:42Z","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 <ttjtftx@gmail.com>\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 71e63d8b50..14274f1ced 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -203,15 +203,18 @@ test_expect_success 'attribute test: read paths from stdin' '\n test_expect_success 'attribute test: --all option' '\n \tgrep -v unspecified <expect-all | sort >specified-all &&\n \tsed -e \"s/:.*//\" <expect-all | uniq >stdin-all &&\n-\tgit check-attr --stdin --all <stdin-all | sort >actual &&\n+\tgit check-attr --stdin --all <stdin-all >actual &&\n+\tsort -o actual actual &&\n \ttest_cmp specified-all actual\n '\n \n test_expect_success 'attribute test: --cached option' '\n-\tgit check-attr --cached --stdin --all <stdin-all | sort >actual &&\n+\tgit check-attr --cached --stdin --all <stdin-all >actual &&\n+\tsort -o actual actual &&\n \ttest_must_be_empty actual &&\n \tgit add .gitattributes a/.gitattributes a/b/.gitattributes &&\n-\tgit check-attr --cached --stdin --all <stdin-all | sort >actual &&\n+\tgit check-attr --cached --stdin --all <stdin-all >actual &&\n+\tsort -o actual actual &&\n \ttest_cmp specified-all actual\n '\n \n@@ -301,8 +304,8 @@ test_expect_success 'bare repository: check that --cached honors index' '\n \t(\n \t\tcd bare.git &&\n \t\tGIT_INDEX_FILE=../.git/index \\\n-\t\tgit check-attr --cached --stdin --all <../stdin-all |\n-\t\tsort >actual &&\n+\t\tgit check-attr --cached --stdin --all <../stdin-all >actual &&\n+\t\tsort -o actual actual &&\n \t\ttest_cmp ../specified-all actual\n \t)\n '\n-- \n2.21.0\n\n"},{"id":"371067","messageId":"20190310081023.64186-1-ttjtftx@gmail.com","threadId":"50703","inReplyTo":"20190310080931.64134-1-ttjtftx@gmail.com","subject":"[GSoC][PATCH v2 4/5] t0022-crlf-rename: avoid using pipes","fromName":"Jonathan Chang","fromEmail":"ttjtftx@gmail.com","sentAt":"2019-03-10T08:10:23Z","receivedAt":"2019-03-10T08:10:34Z","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 <ttjtftx@gmail.com>\n\ndiff --git a/t/t0022-crlf-rename.sh b/t/t0022-crlf-rename.sh\nindex 7af3fbcc7b..05f443dcce 100755\n--- a/t/t0022-crlf-rename.sh\n+++ b/t/t0022-crlf-rename.sh\n@@ -23,10 +23,10 @@ test_expect_success setup '\n \n test_expect_success 'diff -M' '\n \n-\tgit diff-tree -M -r --name-status HEAD^ HEAD |\n-\tsed -e \"s/R[0-9]*/RNUM/\" >actual &&\n+\tgit diff-tree -M -r --name-status HEAD^ HEAD >actual &&\n+\tsed -e \"s/R[0-9]*/RNUM/\" actual >output &&\n \techo \"RNUM\tsample\telpmas\" >expect &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect output\n \n '\n \n-- \n2.21.0\n\n"},{"id":"371068","messageId":"20190310081106.64239-1-ttjtftx@gmail.com","threadId":"50703","inReplyTo":"20190310081023.64186-1-ttjtftx@gmail.com","subject":"[GSoC][PATCH v2 5/5] t0000-basic: use test_line_count instead of wc -l","fromName":"Jonathan Chang","fromEmail":"ttjtftx@gmail.com","sentAt":"2019-03-10T08:11:06Z","receivedAt":"2019-03-10T08:11:19Z","isPatch":true,"sender":{"key":"ttjtftx@gmail.com","avatar":null},"body":"Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 47666b013e..aa25694b45 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 | wc -l >numparent &&\n+\ttest_line_count = 1 numparent\n '\n \n test_expect_success 'update-index D/F conflict' '\n@@ -1146,8 +1146,8 @@ 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 <actual) &&\n-\ttest $numpath0 = 1\n+\twc -l <actual >numpath0 &&\n+\ttest_line_count = 1 numpath0\n '\n \n test_expect_success 'very long name in the index handled sanely' '\n-- \n2.21.0\n\n"},{"id":"371069","messageId":"CAOAu_Y+GeHHsxVmqLajtM41H67L83Rxxkp3SqJ-G4GBv=uTDYg@mail.gmail.com","threadId":"50703","inReplyTo":"CAP8UFD26Zxt1FvwAajV48eBbhPv_q5oSLTekWxApcNpg=xRn3Q@mail.gmail.com","subject":"Re: [GSoC][PATCH] tests: avoid using pipes","fromName":"ttjtftx","fromEmail":"ttjtftx@gmail.com","sentAt":"2019-03-10T08:27:50Z","receivedAt":"2019-03-10T08:33:50Z","isPatch":true,"sender":{"key":"ttjtftx@gmail.com","avatar":null},"body":"Thanks for the reminders.\n\nOn Sun, Mar 10, 2019 at 2:06 PM Christian Couder\n<christian.couder@gmail.com> wrote:\n\n> If you take a look at c6f44e1da5 (\"t9813: avoid using pipes\",\n> 2017-01-04) you can see the following:\n>\n>     - it changes only one test file: t9813-git-p4-preserve-users.sh\n>     - its title starts with \"t9813: \"\nI adapted this format in my second version[1].\n\n[1]: https://public-inbox.org/git/20190310080739.63984-1-ttjtftx@gmail.com\n\nThanks,\nJonathan\n"},{"id":"371070","messageId":"CAPig+cSMZrQFrLXoO5KE1uonUxmnYHikr-e6GAq_n6vx3+sPJA@mail.gmail.com","threadId":"50703","inReplyTo":"20190310081106.64239-1-ttjtftx@gmail.com","subject":"Re: [GSoC][PATCH v2 5/5] t0000-basic: use test_line_count instead of wc -l","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-10T09:50:58Z","receivedAt":"2019-03-10T09:51:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 10, 2019 at 4:11 AM Jonathan Chang <ttjtftx@gmail.com> wrote:\n> Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>\n>\n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> @@ -1136,8 +1136,8 @@ test_expect_success 'git commit-tree omits duplicated parent in a commit' '\n> -       numparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | wc -l) &&\n> -       test $numparent = 1\n> +       sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | wc -l >numparent &&\n> +       test_line_count = 1 numparent\n\nThis transformation makes no sense. The output of 'sed' is fed to 'wc\n-l' whose output is redirected to file \"numparent\", which means that\nthe output file will end up containing a single line no matter how\nmany \"parent\" lines are matched in the input. Since test_line_count()\nchecks the number of lines in the named file, the test will succeed\nunconditionally (which makes for a pretty poor test).\n\nAlso, the filename \"numparent\" doesn't do a good job of representing\nwhat the file is expected to contain. A better name might be\n\"parents\". So, a more correct transformation would be:\n\n    sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual >parents &&\n    test_line_count = 1 parents\n\n> @@ -1146,8 +1146,8 @@ test_expect_success 'update-index D/F conflict' '\n> -       numpath0=$(wc -l <actual) &&\n> -       test $numpath0 = 1\n> +       wc -l <actual >numpath0 &&\n> +       test_line_count = 1 numpath0\n\nSame comment about bogus transformation. The entire sequence should\ncollapse to a single line:\n\n    test_line_count = 1 actual\n"},{"id":"371071","messageId":"CAPig+cQxVfZ7L=cFfgAWOzL7Equ2HsFR7si=GxBxj3-mSpCUnQ@mail.gmail.com","threadId":"50703","inReplyTo":"20190310081023.64186-1-ttjtftx@gmail.com","subject":"Re: [GSoC][PATCH v2 4/5] t0022-crlf-rename: avoid using pipes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-10T10:03:31Z","receivedAt":"2019-03-10T10:03:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 10, 2019 at 4:10 AM Jonathan Chang <ttjtftx@gmail.com> wrote:\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 <ttjtftx@gmail.com>\n\nAll of the patches in this series are malformed. There should be a\n\"---\" line right here below your sign-off. The \"---\" line is\nrecognized by git-am/git-apply as separating the commit message from\nthe actual diff(s).\n\n> diff --git a/t/t0022-crlf-rename.sh b/t/t0022-crlf-rename.sh\n> @@ -23,10 +23,10 @@ test_expect_success setup '\n>  test_expect_success 'diff -M' '\n>\n> -       git diff-tree -M -r --name-status HEAD^ HEAD |\n> -       sed -e \"s/R[0-9]*/RNUM/\" >actual &&\n> +       git diff-tree -M -r --name-status HEAD^ HEAD >actual &&\n> +       sed -e \"s/R[0-9]*/RNUM/\" actual >output &&\n>         echo \"RNUM      sample  elpmas\" >expect &&\n> -       test_cmp expect actual\n> +       test_cmp expect output\n\nIt is a very well-established custom in Git tests for the files handed\nto test_cmp() to be named \"expect\" and \"actual\", so this change is not\nthe most desirable. What you can do instead is:\n\n    git diff-tree -M -r --name-status HEAD^ HEAD >output &&\n    sed -e \"s/R[0-9]*/RNUM/\" output >actual &&\n\nwhich allows you to leave the test_cmp() line alone, thus (as a bonus)\nmakes the patch less noisy.\n"},{"id":"371073","messageId":"CAPig+cQP74MC6fJC4fNCz9Rz3tDux1K4TGB7N+cOC8zScxOoAA@mail.gmail.com","threadId":"50703","inReplyTo":"20190310080931.64134-1-ttjtftx@gmail.com","subject":"Re: [GSoC][PATCH v2 3/5] t0003-attributes: avoid using pipes","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-10T10:13:19Z","receivedAt":"2019-03-10T10:13:33Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 10, 2019 at 4:09 AM Jonathan Chang <ttjtftx@gmail.com> wrote:\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 <ttjtftx@gmail.com>\n>\n> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\n> @@ -203,15 +203,18 @@ test_expect_success 'attribute test: read paths from stdin' '\n>  test_expect_success 'attribute test: --all option' '\n>         grep -v unspecified <expect-all | sort >specified-all &&\n>         sed -e \"s/:.*//\" <expect-all | uniq >stdin-all &&\n> -       git check-attr --stdin --all <stdin-all | sort >actual &&\n> +       git check-attr --stdin --all <stdin-all >actual &&\n> +       sort -o actual actual &&\n>         test_cmp specified-all actual\n>  '\n\nThere is no existing use of \"sort -o\" anywhere in the Git test suite\n(or, for that matter, anywhere else in Git), which should give one\npause before using it here. Although -o is allowed by POSIX, and POSIX\neven says it's safe for the output file to have the same name as one\nof the input files, there is no guarantee that \"sort -o\" will be\nsupported on all platforms, or that all platforms promise that the\noutput filename can match an input filename (in fact, neither the\nMacOS nor FreeBSD man pages for 'sort' make this promise).\nConsequently, it would be better to err on the side of safety and\navoid \"sort -o\", which is easily enough done by using another\ntemporary file:\n\n    git check-attr --stdin --all <stdin-all >output &&\n    sort output >actual &&\n\nThe same comment applies to the remaining changes.\n"},{"id":"371076","messageId":"CAP8UFD274-iDkqPm8-WGXbUmcVqjDE7bSg2bwA-17TWJivn0jA@mail.gmail.com","threadId":"50703","inReplyTo":"CAOAu_Y+GeHHsxVmqLajtM41H67L83Rxxkp3SqJ-G4GBv=uTDYg@mail.gmail.com","subject":"Re: [GSoC][PATCH] tests: avoid using pipes","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-10T15:05:43Z","receivedAt":"2019-03-10T15:06:00Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Mar 10, 2019 at 9:28 AM ttjtftx <ttjtftx@gmail.com> wrote:\n>\n> On Sun, Mar 10, 2019 at 2:06 PM Christian Couder\n> <christian.couder@gmail.com> wrote:\n>\n> > If you take a look at c6f44e1da5 (\"t9813: avoid using pipes\",\n> > 2017-01-04) you can see the following:\n> >\n> >     - it changes only one test file: t9813-git-p4-preserve-users.sh\n> >     - its title starts with \"t9813: \"\n> I adapted this format in my second version[1].\n>\n> [1]: https://public-inbox.org/git/20190310080739.63984-1-ttjtftx@gmail.com\n\nIt's better because each patch changes only one file, but I also\nwanted to say that for a microproject you only need to focus on only\none file. So I would you suggest you keep only the patches that are\nabout \"t0000-basic.sh\" and drop the patches about\n\"t0003-attributes.sh\" and \"t0022-crlf-rename.sh\".\n\n(https://git.github.io/SoC-2019-Microprojects/ has now a \"Only ONE\nquality focused microproject per student\" section with more content.)\n\nIt's also a small nit but your patches start with \"t0000-basic: \" not\njust \"t0000: \" though the latter format is more frequent in existing\ncommits than the former.\n"},{"id":"371081","messageId":"20190310175924.GF31533@hank.intra.tgummerer.com","threadId":"50703","inReplyTo":"20190310080739.63984-1-ttjtftx@gmail.com","subject":"Re: [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error","fromName":"Thomas Gummerer","fromEmail":"t.gummerer@gmail.com","sentAt":"2019-03-10T17:59:24Z","receivedAt":"2019-03-10T17:59:30Z","isPatch":true,"sender":{"key":"t.gummerer@gmail.com","avatar":"https://avatars.githubusercontent.com/u/191004?v=4"},"body":"On 03/10, Jonathan Chang wrote:\n> Hi,\n> \n> Thanks for the reviews.\n> \n> Here are the changes in the second version:\n> \t- bug fixes\n> \t- add preparatory patch\n> \t- seperate different files to different patch\n> \t- change to use test_line_count in a seperate patch\n> \n> Also I found that there is no such function as test_char_count, \n> is it worthwile to add such function? Here are some stat:\n> \n> `git grep 'test_line_count' | wc -l` = 626\n> `git grep 'wc -l' | wc -l` = 294\n> `git grep 'wc -c' | wc -l` = 68\n\nI do think it would be helpful to introduce that helper, especially if\nit is useful in this patch series.  There seem to be enough other\nplaces where it can be useful to make it worth adding the helper.\n\n> -- >8 --\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\nThe commit message should also describe why we need this preparatory\nstep. Maybe something like:\n\n      To reduce the noise in when refactoring this pipeline in a\n      subsequent commit fix the indentation.  This has been wrong\n      since the refactoring done in 1b5b2b641a (\"t0000: modernise\n      style\", 2012-03-02), but carries no meaning.\n\n> Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>\n>\n\nOut of curiosity, how did you create the patch?  'git format-patch'\nwould add a '---' line followed by the diffstat, where you would\nusually put the commentary that you put before the scissors line.  It\nseems like 'git am' still handles this fine, which I didn't know, just\nsomething I noticed because I'm used to the other format.\n\nSince this patch series is now 5 patches, that commentary should go\ninto a cover letter (see the --cover-letter option in format-patch),\nso the reviewers can read that first, and read the patches with that\nin mind, focusing on the patch only, and not additional commentary\nthat applies to the whole series when reading the patch.\n\n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> index 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> -- \n> 2.21.0\n> \n"},{"id":"371156","messageId":"CAOAu_YJHOQt=6H2q+hMnsmFGsqZyedDO0LpCqNH_8A484iPyWA@mail.gmail.com","threadId":"50703","inReplyTo":"CAOAu_YJKNjGd3mw7K17ySQJeF4XxC+V00FFEYA7o593riEGN1g@mail.gmail.com","subject":"Fwd: [GSoC][PATCH v2 4/5] t0022-crlf-rename: avoid using pipes","fromName":"ttjtftx","fromEmail":"ttjtftx@gmail.com","sentAt":"2019-03-11T15:54:04Z","receivedAt":"2019-03-11T15:54:17Z","isPatch":true,"sender":{"key":"ttjtftx@gmail.com","avatar":null},"body":"Sorry, I forgot to Cc.\n\n---------- Forwarded message ---------\nFrom: ttjtftx <ttjtftx@gmail.com>\nDate: Mon, Mar 11, 2019 at 11:43 PM\nSubject: Re: [GSoC][PATCH v2 4/5] t0022-crlf-rename: avoid using pipes\nTo: Eric Sunshine <sunshine@sunshineco.com>\n\n\nOn Sun, Mar 10, 2019 at 6:03 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n\n> All of the patches in this series are malformed. There should be a\n> \"---\" line right here below your sign-off. The \"---\" line is\n> recognized by git-am/git-apply as separating the commit message from\n> the actual diff(s).\n\nI will check it next time.\n\n> It is a very well-established custom in Git tests for the files handed\n> to test_cmp() to be named \"expect\" and \"actual\", so this change is not\n> the most desirable. What you can do instead is:\n>\n>     git diff-tree -M -r --name-status HEAD^ HEAD >output &&\n>     sed -e \"s/R[0-9]*/RNUM/\" output >actual &&\n>\n> which allows you to leave the test_cmp() line alone, thus (as a bonus)\n> makes the patch less noisy.\n\nThanks for the tips! Now I see why \"actual\" is the most used name for\noutput files.\n"},{"id":"371159","messageId":"CAOAu_YKas8ZL+8qwwD-bPhnkdxMWB_JybOmBJW1ea4BVxoMQNg@mail.gmail.com","threadId":"50703","inReplyTo":"CAPig+cSMZrQFrLXoO5KE1uonUxmnYHikr-e6GAq_n6vx3+sPJA@mail.gmail.com","subject":"Re: [GSoC][PATCH v2 5/5] t0000-basic: use test_line_count instead of wc -l","fromName":"ttjtftx","fromEmail":"ttjtftx@gmail.com","sentAt":"2019-03-11T16:10:04Z","receivedAt":"2019-03-11T16:10:17Z","isPatch":true,"sender":{"key":"ttjtftx@gmail.com","avatar":null},"body":"On Sun, Mar 10, 2019 at 5:51 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Sun, Mar 10, 2019 at 4:11 AM Jonathan Chang <ttjtftx@gmail.com> wrote:\n> > Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>\n> >\n> > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> > @@ -1136,8 +1136,8 @@ test_expect_success 'git commit-tree omits duplicated parent in a commit' '\n> > -       numparent=$(sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | wc -l) &&\n> > -       test $numparent = 1\n> > +       sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual | wc -l >numparent &&\n> > +       test_line_count = 1 numparent\n>\n> This transformation makes no sense. The output of 'sed' is fed to 'wc\n> -l' whose output is redirected to file \"numparent\", which means that\n> the output file will end up containing a single line no matter how\n> many \"parent\" lines are matched in the input. Since test_line_count()\n> checks the number of lines in the named file, the test will succeed\n> unconditionally (which makes for a pretty poor test).\n\nYou are right. I will make sure I have thoroughly reviewed my patch before\nsubmitting next time.\n\n> Also, the filename \"numparent\" doesn't do a good job of representing\n> what the file is expected to contain. A better name might be\n> \"parents\". So, a more correct transformation would be:\n>\n>     sed -n -e \"s/^parent //p\" -e \"/^author /q\" actual >parents &&\n>     test_line_count = 1 parents\n>\n> > @@ -1146,8 +1146,8 @@ test_expect_success 'update-index D/F conflict' '\n> > -       numpath0=$(wc -l <actual) &&\n> > -       test $numpath0 = 1\n> > +       wc -l <actual >numpath0 &&\n> > +       test_line_count = 1 numpath0\n>\n> Same comment about bogus transformation. The entire sequence should\n> collapse to a single line:\n>\n>     test_line_count = 1 actual\n\nThanks for the help.\n"},{"id":"371163","messageId":"CAOAu_YK6G8apWPoFJPCCNizr_O-puHUwtU4CE-0_UgCnTSWkgQ@mail.gmail.com","threadId":"50703","inReplyTo":"CAOAu_YL8heWLSznRV8pjLkRZBOEth_7CSmftupx+4+SSx5yztw@mail.gmail.com","subject":"Fwd: [GSoC][PATCH] tests: avoid using pipes","fromName":"jonathan chang","fromEmail":"ttjtftx@gmail.com","sentAt":"2019-03-11T16:45:03Z","receivedAt":"2019-03-11T16:45:16Z","isPatch":true,"sender":{"key":"ttjtftx@gmail.com","avatar":null},"body":"Sorry, I forgot to Cc, again.\n\n---------- Forwarded message ---------\nFrom: ttjtftx <ttjtftx@gmail.com>\nDate: Tue, Mar 12, 2019 at 12:32 AM\nSubject: Re: [GSoC][PATCH] tests: avoid using pipes\nTo: Christian Couder <christian.couder@gmail.com>\n\n\nOn Sun, Mar 10, 2019 at 11:05 PM Christian Couder\n<christian.couder@gmail.com> wrote:\n>\n> On Sun, Mar 10, 2019 at 9:28 AM ttjtftx <ttjtftx@gmail.com> wrote:\n> >\n> > On Sun, Mar 10, 2019 at 2:06 PM Christian Couder\n> > <christian.couder@gmail.com> wrote:\n> >\n> > > If you take a look at c6f44e1da5 (\"t9813: avoid using pipes\",\n> > > 2017-01-04) you can see the following:\n> > >\n> > >     - it changes only one test file: t9813-git-p4-preserve-users.sh\n> > >     - its title starts with \"t9813: \"\n> > I adapted this format in my second version[1].\n> >\n> > [1]: https://public-inbox.org/git/20190310080739.63984-1-ttjtftx@gmail.com\n>\n> It's better because each patch changes only one file, but I also\n> wanted to say that for a microproject you only need to focus on only\n> one file. So I would you suggest you keep only the patches that are\n> about \"t0000-basic.sh\" and drop the patches about\n> \"t0003-attributes.sh\" and \"t0022-crlf-rename.sh\".\n\nI will do that as I have made too many mistakes. I decided to do more\nthan one file because\nI thought it might be too small even for a microproject and that there\nwould be more chances\nfor feedback if I include more files, since the same file tend to have\nsimilar conversions needed.\n\n> It's also a small nit but your patches start with \"t0000-basic: \" not\n> just \"t0000: \" though the latter format is more frequent in existing\n> commits than the former.\n\nGot it.\n"},{"id":"371553","messageId":"CAOAu_YLTft7xmx4xvoM5Qqe0CHjqWrH+DUysHq0zfSEU8vodkg@mail.gmail.com","threadId":"50703","inReplyTo":"20190310175924.GF31533@hank.intra.tgummerer.com","subject":"Re: [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error","fromName":"jonathan chang","fromEmail":"ttjtftx@gmail.com","sentAt":"2019-03-15T01:55:39Z","receivedAt":"2019-03-15T01:55:52Z","isPatch":true,"sender":{"key":"ttjtftx@gmail.com","avatar":null},"body":"On Mon, Mar 11, 2019 at 1:59 AM Thomas Gummerer <t.gummerer@gmail.com> wrote:\n>\n> On 03/10, Jonathan Chang wrote:\n> > Hi,\n> >\n> > Thanks for the reviews.\n> >\n> > Here are the changes in the second version:\n> >       - bug fixes\n> >       - add preparatory patch\n> >       - seperate different files to different patch\n> >       - change to use test_line_count in a seperate patch\n> >\n> > Also I found that there is no such function as test_char_count,\n> > is it worthwile to add such function? Here are some stat:\n> >\n> > `git grep 'test_line_count' | wc -l` = 626\n> > `git grep 'wc -l' | wc -l` = 294\n> > `git grep 'wc -c' | wc -l` = 68\n>\n> I do think it would be helpful to introduce that helper, especially if\n> it is useful in this patch series.  There seem to be enough other\n> places where it can be useful to make it worth adding the helper.\n\nThanks for the feedback.\n\n> > -- >8 --\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>\n> The commit message should also describe why we need this preparatory\n> step. Maybe something like:\n>\n>       To reduce the noise in when refactoring this pipeline in a\n>       subsequent commit fix the indentation.  This has been wrong\n>       since the refactoring done in 1b5b2b641a (\"t0000: modernise\n>       style\", 2012-03-02), but carries no meaning.\n>\n> > Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>\n> >\n>\n> Out of curiosity, how did you create the patch?  'git format-patch'\n> would add a '---' line followed by the diffstat, where you would\n> usually put the commentary that you put before the scissors line.  It\n> seems like 'git am' still handles this fine, which I didn't know, just\n> something I noticed because I'm used to the other format.\n\nI believe I used git for that. But I cannot think of why it happened nor\nreproduce it.\n\n> Since this patch series is now 5 patches, that commentary should go\n> into a cover letter (see the --cover-letter option in format-patch),\n> so the reviewers can read that first, and read the patches with that\n> in mind, focusing on the patch only, and not additional commentary\n> that applies to the whole series when reading the patch.\n\nI wasn't aware of this option. I tried to produce the format in others\ncover letter using 'git diff' and options like '--stat', '--summary', with no\nsuccess. I consulted Documentation/SubmittingPatches, where I got the\nidea of cover letter, but it doesn't mention the option '--cover-letter' and\nthe idea of cover letter is even confused with '--notes'[1].\n\nI just reread some of the GSoC related mails in the mailing list and\nfound one[2] that introduced the usage of 'cover-letter', '--range-diff' and\n'--interdiff'. As a newbie, I personally think it would be helpful to include\ntheses options along with others mentioned in SubmittingPatches.\n\n[1]: Documentation/SubmittingPatches:\n\n> You often want to add additional explanation about the patch,\n> other than the commit message itself. Place such \"cover letter\"\n> material between the three-dash line and the diffstat. For\n> patches requiring multiple iterations of review and discussion,\n> an explanation of changes between each iteration can be kept in\n> Git-notes and inserted automatically following the three-dash\n> line via `git format-patch --notes`.\n\n[2]: https://public-inbox.org/git/CAPig+cSsAufCnHPJfjQd8A778UNAsXEst1m+ekQ7T83=2mMUnw@mail.gmail.com/\n"},{"id":"371554","messageId":"CAOAu_YLGDAije4ugkPYh-11QMKVUEbGZNC335F4VtnszJO8Q9A@mail.gmail.com","threadId":"50703","inReplyTo":"CAPig+cQP74MC6fJC4fNCz9Rz3tDux1K4TGB7N+cOC8zScxOoAA@mail.gmail.com","subject":"Re: [GSoC][PATCH v2 3/5] t0003-attributes: avoid using pipes","fromName":"jonathan chang","fromEmail":"ttjtftx@gmail.com","sentAt":"2019-03-15T01:56:33Z","receivedAt":"2019-03-15T01:56:46Z","isPatch":true,"sender":{"key":"ttjtftx@gmail.com","avatar":null},"body":"On Sun, Mar 10, 2019 at 6:13 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Sun, Mar 10, 2019 at 4:09 AM Jonathan Chang <ttjtftx@gmail.com> wrote:\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 <ttjtftx@gmail.com>\n> >\n> > diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\n> > @@ -203,15 +203,18 @@ test_expect_success 'attribute test: read paths from stdin' '\n> >  test_expect_success 'attribute test: --all option' '\n> >         grep -v unspecified <expect-all | sort >specified-all &&\n> >         sed -e \"s/:.*//\" <expect-all | uniq >stdin-all &&\n> > -       git check-attr --stdin --all <stdin-all | sort >actual &&\n> > +       git check-attr --stdin --all <stdin-all >actual &&\n> > +       sort -o actual actual &&\n> >         test_cmp specified-all actual\n> >  '\n>\n> There is no existing use of \"sort -o\" anywhere in the Git test suite\n> (or, for that matter, anywhere else in Git), which should give one\n> pause before using it here. Although -o is allowed by POSIX, and POSIX\n> even says it's safe for the output file to have the same name as one\n> of the input files, there is no guarantee that \"sort -o\" will be\n> supported on all platforms, or that all platforms promise that the\n> output filename can match an input filename (in fact, neither the\n> MacOS nor FreeBSD man pages for 'sort' make this promise).\n> Consequently, it would be better to err on the side of safety and\n> avoid \"sort -o\", which is easily enough done by using another\n> temporary file:\n>\n>     git check-attr --stdin --all <stdin-all >output &&\n>     sort output >actual &&\n>\n> The same comment applies to the remaining changes.\n\nNoted. I did check POSIX, I should have also checked the use in Git.\n\nThanks for the review.\n"},{"id":"371583","messageId":"CAP8UFD09QZd=6HyB5Om1PfV=67+CvfyQkcpLU0tukT48QccD0Q@mail.gmail.com","threadId":"50703","inReplyTo":"CAOAu_YLTft7xmx4xvoM5Qqe0CHjqWrH+DUysHq0zfSEU8vodkg@mail.gmail.com","subject":"Re: [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-03-15T12:48:08Z","receivedAt":"2019-03-15T12:48:26Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Fri, Mar 15, 2019 at 2:55 AM jonathan chang <ttjtftx@gmail.com> wrote:\n>\n> On Mon, Mar 11, 2019 at 1:59 AM Thomas Gummerer <t.gummerer@gmail.com> wrote:\n> >\n> > On 03/10, Jonathan Chang wrote:\n\n> > > Also I found that there is no such function as test_char_count,\n> > > is it worthwile to add such function? Here are some stat:\n> > >\n> > > `git grep 'test_line_count' | wc -l` = 626\n> > > `git grep 'wc -l' | wc -l` = 294\n> > > `git grep 'wc -c' | wc -l` = 68\n> >\n> > I do think it would be helpful to introduce that helper, especially if\n> > it is useful in this patch series.  There seem to be enough other\n> > places where it can be useful to make it worth adding the helper.\n\nYeah, it would be useful for Git, but it's not necessary of course for\nyour microproject, which is already more than big enough.\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> >\n> > The commit message should also describe why we need this preparatory\n> > step. Maybe something like:\n> >\n> >       To reduce the noise in when refactoring this pipeline in a\n> >       subsequent commit fix the indentation.\n\nOr perhaps:\n\n      Fix indentation of a line containing a pipeline to reduce the\nnoise when refactoring the pipeline in a subsequent commit.\n\n> This has been wrong\n> >       since the refactoring done in 1b5b2b641a (\"t0000: modernise\n> >       style\", 2012-03-02), but carries no meaning.\n> >\n> > > Signed-off-by: Jonathan Chang <ttjtftx@gmail.com>\n\n> > Since this patch series is now 5 patches, that commentary should go\n> > into a cover letter (see the --cover-letter option in format-patch),\n> > so the reviewers can read that first, and read the patches with that\n> > in mind, focusing on the patch only, and not additional commentary\n> > that applies to the whole series when reading the patch.\n>\n> I wasn't aware of this option. I tried to produce the format in others\n> cover letter using 'git diff' and options like '--stat', '--summary', with no\n> success. I consulted Documentation/SubmittingPatches, where I got the\n> idea of cover letter, but it doesn't mention the option '--cover-letter' and\n> the idea of cover letter is even confused with '--notes'[1].\n\nThe issue is that there are different things that can be considered\n\"cover letter\". I agree that it's not very clear in\nDocumentation/SubmittingPatches.\n\nThere is the following which is related to --cover-letter:\n\n\"Multiple related patches should be grouped into their own e-mail\nthread to help readers find all parts of the series.  To that end,\nsend them as replies to either an additional \"cover letter\" message\n(see below), the first patch, or the respective preceding patch.\"\n\nbut it doesn't mention --cover-letter and it contains \"(see below)\"\nthough I don't see what that refers to.\n\n> I just reread some of the GSoC related mails in the mailing list and\n> found one[2] that introduced the usage of 'cover-letter', '--range-diff' and\n> '--interdiff'. As a newbie, I personally think it would be helpful to include\n> theses options along with others mentioned in SubmittingPatches.\n\nI agree that there is room for improvement in SubmittingPatches.\n\nThanks,\nChristian.\n"}]}