* [GSoC][PATCH] tests: avoid using pipes @ 2019-03-09 15:45 Jonathan Chang 2019-03-09 16:45 ` Thomas Gummerer 2019-03-10 6:05 ` [GSoC][PATCH] tests: avoid using pipes Christian Couder 0 siblings, 2 replies; 20+ messages in thread From: Jonathan Chang @ 2019-03-09 15:45 UTC (permalink / raw) To: git; +Cc: Jonathan Chang This is my attempt for this year's GSoC microproject. I copied the commit message of the commit[1] mentioned in the microproject page[2]. Is this OK? Here is a summary of what I did: - simple substitution as in c6f44e1da5[1]. - besides simple substitution, I moved some git command out of command substitution to improve readability, which was not possible with pipes, while in these cases keeping them inside won't discard git's exit code. - use actual1 and actual2 in cases where actual is already in use. - use `sort -o actual actual` to avoid using actual1 and actual2. [1] c6f44e1da5 ("t9813: avoid using pipes", 2017-01-04) [2] https://git.github.io/SoC-2019-Microprojects/ ---------------------->8------------------- 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 ++++++++++++++-------------- t/t0003-attributes.sh | 13 ++++++++----- t/t0022-crlf-rename.sh | 6 +++--- 3 files changed, 25 insertions(+), 22 deletions(-) diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index b6566003dd..adc9545973 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 ' diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh index 71e63d8b50..14274f1ced 100755 --- a/t/t0003-attributes.sh +++ b/t/t0003-attributes.sh @@ -203,15 +203,18 @@ test_expect_success 'attribute test: read paths from stdin' ' test_expect_success 'attribute test: --all option' ' grep -v unspecified <expect-all | sort >specified-all && sed -e "s/:.*//" <expect-all | uniq >stdin-all && - git check-attr --stdin --all <stdin-all | sort >actual && + git check-attr --stdin --all <stdin-all >actual && + sort -o actual actual && test_cmp specified-all actual ' test_expect_success 'attribute test: --cached option' ' - git check-attr --cached --stdin --all <stdin-all | sort >actual && + git check-attr --cached --stdin --all <stdin-all >actual && + sort -o actual actual && test_must_be_empty actual && git add .gitattributes a/.gitattributes a/b/.gitattributes && - git check-attr --cached --stdin --all <stdin-all | sort >actual && + git check-attr --cached --stdin --all <stdin-all >actual && + sort -o actual actual && test_cmp specified-all actual ' @@ -301,8 +304,8 @@ test_expect_success 'bare repository: check that --cached honors index' ' ( cd bare.git && GIT_INDEX_FILE=../.git/index \ - git check-attr --cached --stdin --all <../stdin-all | - sort >actual && + git check-attr --cached --stdin --all <../stdin-all >actual && + sort -o actual actual && test_cmp ../specified-all actual ) ' diff --git a/t/t0022-crlf-rename.sh b/t/t0022-crlf-rename.sh index 7af3fbcc7b..b4772b72f7 100755 --- a/t/t0022-crlf-rename.sh +++ b/t/t0022-crlf-rename.sh @@ -23,10 +23,10 @@ test_expect_success setup ' test_expect_success 'diff -M' ' - git diff-tree -M -r --name-status HEAD^ HEAD | - sed -e "s/R[0-9]*/RNUM/" >actual && + git diff-tree -M -r --name-status HEAD^ HEAD >actual1 && + sed -e "s/R[0-9]*/RNUM/" actual1 >actual2 && echo "RNUM sample elpmas" >expect && - test_cmp expect actual + test_cmp expect actual2 ' -- 2.21.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [GSoC][PATCH] tests: avoid using pipes 2019-03-09 15:45 [GSoC][PATCH] tests: avoid using pipes Jonathan Chang @ 2019-03-09 16:45 ` Thomas Gummerer 2019-03-10 8:07 ` [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error Jonathan Chang 2019-03-10 6:05 ` [GSoC][PATCH] tests: avoid using pipes Christian Couder 1 sibling, 1 reply; 20+ messages in thread From: Thomas Gummerer @ 2019-03-09 16:45 UTC (permalink / raw) To: Jonathan Chang; +Cc: git Hi, welcome and thanks for the patch! On 03/09, Jonathan Chang wrote: > This is my attempt for this year's GSoC microproject. > > I copied the commit message of the commit[1] mentioned in the microproject > page[2]. Is this OK? > > Here is a summary of what I did: > - simple substitution as in c6f44e1da5[1]. > - besides simple substitution, I moved some git command out of command > substitution to improve readability, which was not possible with pipes, > while in these cases keeping them inside won't discard git's exit code. > - use actual1 and actual2 in cases where actual is already in use. > - use `sort -o actual actual` to avoid using actual1 and actual2. > > [1] c6f44e1da5 ("t9813: avoid using pipes", 2017-01-04) > [2] https://git.github.io/SoC-2019-Microprojects/ > > ---------------------->8------------------- > 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 ++++++++++++++-------------- > t/t0003-attributes.sh | 13 ++++++++----- > t/t0022-crlf-rename.sh | 6 +++--- > 3 files changed, 25 insertions(+), 22 deletions(-) > > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh > index b6566003dd..adc9545973 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 | This line is a bit oddly indented. This is of course not something you did, but it did leave me puzzled why the indentation was here before and if the conversion is actually correct. To help reviewers, it would be nice to fix the indentation of this line in a preparatory patch, and clarify since when this indentation was there and that it is not correct. That is if I'm not missing something, and there is not actually a good reason for this to be indented, but judging from the original and the conversion in this commit, I don't think there is. > - 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) && This test is actually failing now (and so is the one just below). The reason is that 'wc -l <file>' outputs the number of lines followed by the filename, so numpath0 includes that. We have a helper function that avoids exactly that, 'test_line_count'. See t/README or t/test-lib-functions.sh for more information on that function. Before submitting a patch series, please also run the test suite, or if you are only modifying tests such as in this case, at least the tests that are modified. > 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 > ' > > diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh > index 71e63d8b50..14274f1ced 100755 > --- a/t/t0003-attributes.sh > +++ b/t/t0003-attributes.sh > @@ -203,15 +203,18 @@ test_expect_success 'attribute test: read paths from stdin' ' > test_expect_success 'attribute test: --all option' ' > grep -v unspecified <expect-all | sort >specified-all && > sed -e "s/:.*//" <expect-all | uniq >stdin-all && > - git check-attr --stdin --all <stdin-all | sort >actual && > + git check-attr --stdin --all <stdin-all >actual && > + sort -o actual actual && > test_cmp specified-all actual > ' > > test_expect_success 'attribute test: --cached option' ' > - git check-attr --cached --stdin --all <stdin-all | sort >actual && > + git check-attr --cached --stdin --all <stdin-all >actual && > + sort -o actual actual && > test_must_be_empty actual && > git add .gitattributes a/.gitattributes a/b/.gitattributes && > - git check-attr --cached --stdin --all <stdin-all | sort >actual && > + git check-attr --cached --stdin --all <stdin-all >actual && > + sort -o actual actual && > test_cmp specified-all actual > ' > > @@ -301,8 +304,8 @@ test_expect_success 'bare repository: check that --cached honors index' ' > ( > cd bare.git && > GIT_INDEX_FILE=../.git/index \ > - git check-attr --cached --stdin --all <../stdin-all | > - sort >actual && > + git check-attr --cached --stdin --all <../stdin-all >actual && > + sort -o actual actual && > test_cmp ../specified-all actual > ) > ' > diff --git a/t/t0022-crlf-rename.sh b/t/t0022-crlf-rename.sh > index 7af3fbcc7b..b4772b72f7 100755 > --- a/t/t0022-crlf-rename.sh > +++ b/t/t0022-crlf-rename.sh > @@ -23,10 +23,10 @@ test_expect_success setup ' > > test_expect_success 'diff -M' ' > > - git diff-tree -M -r --name-status HEAD^ HEAD | > - sed -e "s/R[0-9]*/RNUM/" >actual && > + git diff-tree -M -r --name-status HEAD^ HEAD >actual1 && > + sed -e "s/R[0-9]*/RNUM/" actual1 >actual2 && Nit: rather than actual1 and actual2, it might be nice to give the files slightly more meaningful names, maybe "output" and "normalized", but feel free to try and come up with something better than that. > echo "RNUM sample elpmas" >expect && > - test_cmp expect actual > + test_cmp expect actual2 > > ' > > -- > 2.21.0 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error 2019-03-09 16:45 ` Thomas Gummerer @ 2019-03-10 8:07 ` Jonathan Chang 2019-03-10 8:08 ` [GSoC][PATCH v2 2/5] t0000-basic: avoid using pipes Jonathan Chang 2019-03-10 17:59 ` [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error Thomas Gummerer 0 siblings, 2 replies; 20+ messages in thread From: Jonathan Chang @ 2019-03-10 8:07 UTC (permalink / raw) To: Thomas Gummerer; +Cc: Jonathan Chang, Christian Couder, git Hi, Thanks for the reviews. Here are the changes in the second version: - bug fixes - add preparatory patch - seperate different files to different patch - change to use test_line_count in a seperate patch Also I found that there is no such function as test_char_count, is it worthwile to add such function? Here are some stat: `git grep 'test_line_count' | wc -l` = 626 `git grep 'wc -l' | wc -l` = 294 `git grep 'wc -c' | wc -l` = 68 -- >8 -- This is a preparatory step prior to removing the pipes after git commands, which discards git's exit code and may mask a crash. Signed-off-by: Jonathan Chang <ttjtftx@gmail.com> 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [GSoC][PATCH v2 2/5] t0000-basic: avoid using pipes 2019-03-10 8:07 ` [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error Jonathan Chang @ 2019-03-10 8:08 ` Jonathan Chang 2019-03-10 8:09 ` [GSoC][PATCH v2 3/5] t0003-attributes: " Jonathan Chang 2019-03-10 17:59 ` [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error Thomas Gummerer 1 sibling, 1 reply; 20+ messages in thread From: Jonathan Chang @ 2019-03-10 8:08 UTC (permalink / raw) Cc: Jonathan Chang, Christian Couder, Thomas Gummerer, git 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> 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [GSoC][PATCH v2 3/5] t0003-attributes: avoid using pipes 2019-03-10 8:08 ` [GSoC][PATCH v2 2/5] t0000-basic: avoid using pipes Jonathan Chang @ 2019-03-10 8:09 ` Jonathan Chang 2019-03-10 8:10 ` [GSoC][PATCH v2 4/5] t0022-crlf-rename: " Jonathan Chang 2019-03-10 10:13 ` [GSoC][PATCH v2 3/5] t0003-attributes: " Eric Sunshine 0 siblings, 2 replies; 20+ messages in thread From: Jonathan Chang @ 2019-03-10 8:09 UTC (permalink / raw) Cc: Jonathan Chang, Christian Couder, Thomas Gummerer, git 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> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh index 71e63d8b50..14274f1ced 100755 --- a/t/t0003-attributes.sh +++ b/t/t0003-attributes.sh @@ -203,15 +203,18 @@ test_expect_success 'attribute test: read paths from stdin' ' test_expect_success 'attribute test: --all option' ' grep -v unspecified <expect-all | sort >specified-all && sed -e "s/:.*//" <expect-all | uniq >stdin-all && - git check-attr --stdin --all <stdin-all | sort >actual && + git check-attr --stdin --all <stdin-all >actual && + sort -o actual actual && test_cmp specified-all actual ' test_expect_success 'attribute test: --cached option' ' - git check-attr --cached --stdin --all <stdin-all | sort >actual && + git check-attr --cached --stdin --all <stdin-all >actual && + sort -o actual actual && test_must_be_empty actual && git add .gitattributes a/.gitattributes a/b/.gitattributes && - git check-attr --cached --stdin --all <stdin-all | sort >actual && + git check-attr --cached --stdin --all <stdin-all >actual && + sort -o actual actual && test_cmp specified-all actual ' @@ -301,8 +304,8 @@ test_expect_success 'bare repository: check that --cached honors index' ' ( cd bare.git && GIT_INDEX_FILE=../.git/index \ - git check-attr --cached --stdin --all <../stdin-all | - sort >actual && + git check-attr --cached --stdin --all <../stdin-all >actual && + sort -o actual actual && test_cmp ../specified-all actual ) ' -- 2.21.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [GSoC][PATCH v2 4/5] t0022-crlf-rename: avoid using pipes 2019-03-10 8:09 ` [GSoC][PATCH v2 3/5] t0003-attributes: " Jonathan Chang @ 2019-03-10 8:10 ` Jonathan Chang 2019-03-10 8:11 ` [GSoC][PATCH v2 5/5] t0000-basic: use test_line_count instead of wc -l Jonathan Chang 2019-03-10 10:03 ` [GSoC][PATCH v2 4/5] t0022-crlf-rename: avoid using pipes Eric Sunshine 2019-03-10 10:13 ` [GSoC][PATCH v2 3/5] t0003-attributes: " Eric Sunshine 1 sibling, 2 replies; 20+ messages in thread From: Jonathan Chang @ 2019-03-10 8:10 UTC (permalink / raw) Cc: Jonathan Chang, Christian Couder, Thomas Gummerer, git 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> diff --git a/t/t0022-crlf-rename.sh b/t/t0022-crlf-rename.sh index 7af3fbcc7b..05f443dcce 100755 --- a/t/t0022-crlf-rename.sh +++ b/t/t0022-crlf-rename.sh @@ -23,10 +23,10 @@ test_expect_success setup ' test_expect_success 'diff -M' ' - git diff-tree -M -r --name-status HEAD^ HEAD | - sed -e "s/R[0-9]*/RNUM/" >actual && + git diff-tree -M -r --name-status HEAD^ HEAD >actual && + sed -e "s/R[0-9]*/RNUM/" actual >output && echo "RNUM sample elpmas" >expect && - test_cmp expect actual + test_cmp expect output ' -- 2.21.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [GSoC][PATCH v2 5/5] t0000-basic: use test_line_count instead of wc -l 2019-03-10 8:10 ` [GSoC][PATCH v2 4/5] t0022-crlf-rename: " Jonathan Chang @ 2019-03-10 8:11 ` Jonathan Chang 2019-03-10 9:50 ` Eric Sunshine 2019-03-10 10:03 ` [GSoC][PATCH v2 4/5] t0022-crlf-rename: avoid using pipes Eric Sunshine 1 sibling, 1 reply; 20+ messages in thread From: Jonathan Chang @ 2019-03-10 8:11 UTC (permalink / raw) Cc: Jonathan Chang, Christian Couder, Thomas Gummerer, git Signed-off-by: Jonathan Chang <ttjtftx@gmail.com> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh index 47666b013e..aa25694b45 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 | wc -l >numparent && + test_line_count = 1 numparent ' test_expect_success 'update-index D/F conflict' ' @@ -1146,8 +1146,8 @@ 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 + wc -l <actual >numpath0 && + test_line_count = 1 numpath0 ' test_expect_success 'very long name in the index handled sanely' ' -- 2.21.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [GSoC][PATCH v2 5/5] t0000-basic: use test_line_count instead of wc -l 2019-03-10 8:11 ` [GSoC][PATCH v2 5/5] t0000-basic: use test_line_count instead of wc -l Jonathan Chang @ 2019-03-10 9:50 ` Eric Sunshine 2019-03-11 16:10 ` ttjtftx 0 siblings, 1 reply; 20+ messages in thread From: Eric Sunshine @ 2019-03-10 9:50 UTC (permalink / raw) To: Jonathan Chang; +Cc: Christian Couder, Thomas Gummerer, git On Sun, Mar 10, 2019 at 4:11 AM Jonathan Chang <ttjtftx@gmail.com> wrote: > Signed-off-by: Jonathan Chang <ttjtftx@gmail.com> > > diff --git 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' ' > - 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 This transformation makes no sense. The output of 'sed' is fed to 'wc -l' whose output is redirected to file "numparent", which means that the output file will end up containing a single line no matter how many "parent" lines are matched in the input. Since test_line_count() checks the number of lines in the named file, the test will succeed unconditionally (which makes for a pretty poor test). Also, the filename "numparent" doesn't do a good job of representing what the file is expected to contain. A better name might be "parents". So, a more correct transformation would be: sed -n -e "s/^parent //p" -e "/^author /q" actual >parents && test_line_count = 1 parents > @@ -1146,8 +1146,8 @@ test_expect_success 'update-index D/F conflict' ' > - numpath0=$(wc -l <actual) && > - test $numpath0 = 1 > + wc -l <actual >numpath0 && > + test_line_count = 1 numpath0 Same comment about bogus transformation. The entire sequence should collapse to a single line: test_line_count = 1 actual ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [GSoC][PATCH v2 5/5] t0000-basic: use test_line_count instead of wc -l 2019-03-10 9:50 ` Eric Sunshine @ 2019-03-11 16:10 ` ttjtftx 0 siblings, 0 replies; 20+ messages in thread From: ttjtftx @ 2019-03-11 16:10 UTC (permalink / raw) To: Eric Sunshine; +Cc: Christian Couder, Thomas Gummerer, git On Sun, Mar 10, 2019 at 5:51 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Sun, Mar 10, 2019 at 4:11 AM Jonathan Chang <ttjtftx@gmail.com> wrote: > > Signed-off-by: Jonathan Chang <ttjtftx@gmail.com> > > > > diff --git 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' ' > > - 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 > > This transformation makes no sense. The output of 'sed' is fed to 'wc > -l' whose output is redirected to file "numparent", which means that > the output file will end up containing a single line no matter how > many "parent" lines are matched in the input. Since test_line_count() > checks the number of lines in the named file, the test will succeed > unconditionally (which makes for a pretty poor test). You are right. I will make sure I have thoroughly reviewed my patch before submitting next time. > Also, the filename "numparent" doesn't do a good job of representing > what the file is expected to contain. A better name might be > "parents". So, a more correct transformation would be: > > sed -n -e "s/^parent //p" -e "/^author /q" actual >parents && > test_line_count = 1 parents > > > @@ -1146,8 +1146,8 @@ test_expect_success 'update-index D/F conflict' ' > > - numpath0=$(wc -l <actual) && > > - test $numpath0 = 1 > > + wc -l <actual >numpath0 && > > + test_line_count = 1 numpath0 > > Same comment about bogus transformation. The entire sequence should > collapse to a single line: > > test_line_count = 1 actual Thanks for the help. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [GSoC][PATCH v2 4/5] t0022-crlf-rename: avoid using pipes 2019-03-10 8:10 ` [GSoC][PATCH v2 4/5] t0022-crlf-rename: " Jonathan Chang 2019-03-10 8:11 ` [GSoC][PATCH v2 5/5] t0000-basic: use test_line_count instead of wc -l Jonathan Chang @ 2019-03-10 10:03 ` Eric Sunshine [not found] ` <CAOAu_YJKNjGd3mw7K17ySQJeF4XxC+V00FFEYA7o593riEGN1g@mail.gmail.com> 1 sibling, 1 reply; 20+ messages in thread From: Eric Sunshine @ 2019-03-10 10:03 UTC (permalink / raw) To: Jonathan Chang; +Cc: Christian Couder, Thomas Gummerer, git On Sun, Mar 10, 2019 at 4:10 AM Jonathan Chang <ttjtftx@gmail.com> 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> All of the patches in this series are malformed. There should be a "---" line right here below your sign-off. The "---" line is recognized by git-am/git-apply as separating the commit message from the actual diff(s). > diff --git a/t/t0022-crlf-rename.sh b/t/t0022-crlf-rename.sh > @@ -23,10 +23,10 @@ test_expect_success setup ' > test_expect_success 'diff -M' ' > > - git diff-tree -M -r --name-status HEAD^ HEAD | > - sed -e "s/R[0-9]*/RNUM/" >actual && > + git diff-tree -M -r --name-status HEAD^ HEAD >actual && > + sed -e "s/R[0-9]*/RNUM/" actual >output && > echo "RNUM sample elpmas" >expect && > - test_cmp expect actual > + test_cmp expect output It is a very well-established custom in Git tests for the files handed to test_cmp() to be named "expect" and "actual", so this change is not the most desirable. What you can do instead is: git diff-tree -M -r --name-status HEAD^ HEAD >output && sed -e "s/R[0-9]*/RNUM/" output >actual && which allows you to leave the test_cmp() line alone, thus (as a bonus) makes the patch less noisy. ^ permalink raw reply [flat|nested] 20+ messages in thread
[parent not found: <CAOAu_YJKNjGd3mw7K17ySQJeF4XxC+V00FFEYA7o593riEGN1g@mail.gmail.com>]
* Fwd: [GSoC][PATCH v2 4/5] t0022-crlf-rename: avoid using pipes [not found] ` <CAOAu_YJKNjGd3mw7K17ySQJeF4XxC+V00FFEYA7o593riEGN1g@mail.gmail.com> @ 2019-03-11 15:54 ` ttjtftx 0 siblings, 0 replies; 20+ messages in thread From: ttjtftx @ 2019-03-11 15:54 UTC (permalink / raw) To: git; +Cc: Christian Couder, Thomas Gummerer Sorry, I forgot to Cc. ---------- Forwarded message --------- From: ttjtftx <ttjtftx@gmail.com> Date: Mon, Mar 11, 2019 at 11:43 PM Subject: Re: [GSoC][PATCH v2 4/5] t0022-crlf-rename: avoid using pipes To: Eric Sunshine <sunshine@sunshineco.com> On Sun, Mar 10, 2019 at 6:03 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > All of the patches in this series are malformed. There should be a > "---" line right here below your sign-off. The "---" line is > recognized by git-am/git-apply as separating the commit message from > the actual diff(s). I will check it next time. > It is a very well-established custom in Git tests for the files handed > to test_cmp() to be named "expect" and "actual", so this change is not > the most desirable. What you can do instead is: > > git diff-tree -M -r --name-status HEAD^ HEAD >output && > sed -e "s/R[0-9]*/RNUM/" output >actual && > > which allows you to leave the test_cmp() line alone, thus (as a bonus) > makes the patch less noisy. Thanks for the tips! Now I see why "actual" is the most used name for output files. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [GSoC][PATCH v2 3/5] t0003-attributes: avoid using pipes 2019-03-10 8:09 ` [GSoC][PATCH v2 3/5] t0003-attributes: " Jonathan Chang 2019-03-10 8:10 ` [GSoC][PATCH v2 4/5] t0022-crlf-rename: " Jonathan Chang @ 2019-03-10 10:13 ` Eric Sunshine 2019-03-15 1:56 ` jonathan chang 1 sibling, 1 reply; 20+ messages in thread From: Eric Sunshine @ 2019-03-10 10:13 UTC (permalink / raw) To: Jonathan Chang; +Cc: Christian Couder, Thomas Gummerer, git On Sun, Mar 10, 2019 at 4:09 AM Jonathan Chang <ttjtftx@gmail.com> 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> > > diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh > @@ -203,15 +203,18 @@ test_expect_success 'attribute test: read paths from stdin' ' > test_expect_success 'attribute test: --all option' ' > grep -v unspecified <expect-all | sort >specified-all && > sed -e "s/:.*//" <expect-all | uniq >stdin-all && > - git check-attr --stdin --all <stdin-all | sort >actual && > + git check-attr --stdin --all <stdin-all >actual && > + sort -o actual actual && > test_cmp specified-all actual > ' There is no existing use of "sort -o" anywhere in the Git test suite (or, for that matter, anywhere else in Git), which should give one pause before using it here. Although -o is allowed by POSIX, and POSIX even says it's safe for the output file to have the same name as one of the input files, there is no guarantee that "sort -o" will be supported on all platforms, or that all platforms promise that the output filename can match an input filename (in fact, neither the MacOS nor FreeBSD man pages for 'sort' make this promise). Consequently, it would be better to err on the side of safety and avoid "sort -o", which is easily enough done by using another temporary file: git check-attr --stdin --all <stdin-all >output && sort output >actual && The same comment applies to the remaining changes. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [GSoC][PATCH v2 3/5] t0003-attributes: avoid using pipes 2019-03-10 10:13 ` [GSoC][PATCH v2 3/5] t0003-attributes: " Eric Sunshine @ 2019-03-15 1:56 ` jonathan chang 0 siblings, 0 replies; 20+ messages in thread From: jonathan chang @ 2019-03-15 1:56 UTC (permalink / raw) To: Eric Sunshine; +Cc: Christian Couder, Thomas Gummerer, git On Sun, Mar 10, 2019 at 6:13 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Sun, Mar 10, 2019 at 4:09 AM Jonathan Chang <ttjtftx@gmail.com> 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> > > > > diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh > > @@ -203,15 +203,18 @@ test_expect_success 'attribute test: read paths from stdin' ' > > test_expect_success 'attribute test: --all option' ' > > grep -v unspecified <expect-all | sort >specified-all && > > sed -e "s/:.*//" <expect-all | uniq >stdin-all && > > - git check-attr --stdin --all <stdin-all | sort >actual && > > + git check-attr --stdin --all <stdin-all >actual && > > + sort -o actual actual && > > test_cmp specified-all actual > > ' > > There is no existing use of "sort -o" anywhere in the Git test suite > (or, for that matter, anywhere else in Git), which should give one > pause before using it here. Although -o is allowed by POSIX, and POSIX > even says it's safe for the output file to have the same name as one > of the input files, there is no guarantee that "sort -o" will be > supported on all platforms, or that all platforms promise that the > output filename can match an input filename (in fact, neither the > MacOS nor FreeBSD man pages for 'sort' make this promise). > Consequently, it would be better to err on the side of safety and > avoid "sort -o", which is easily enough done by using another > temporary file: > > git check-attr --stdin --all <stdin-all >output && > sort output >actual && > > The same comment applies to the remaining changes. Noted. I did check POSIX, I should have also checked the use in Git. Thanks for the review. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error 2019-03-10 8:07 ` [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error Jonathan Chang 2019-03-10 8:08 ` [GSoC][PATCH v2 2/5] t0000-basic: avoid using pipes Jonathan Chang @ 2019-03-10 17:59 ` Thomas Gummerer 2019-03-15 1:55 ` jonathan chang 1 sibling, 1 reply; 20+ messages in thread From: Thomas Gummerer @ 2019-03-10 17:59 UTC (permalink / raw) To: Jonathan Chang; +Cc: Christian Couder, git On 03/10, Jonathan Chang wrote: > Hi, > > Thanks for the reviews. > > Here are the changes in the second version: > - bug fixes > - add preparatory patch > - seperate different files to different patch > - change to use test_line_count in a seperate patch > > Also I found that there is no such function as test_char_count, > is it worthwile to add such function? Here are some stat: > > `git grep 'test_line_count' | wc -l` = 626 > `git grep 'wc -l' | wc -l` = 294 > `git grep 'wc -c' | wc -l` = 68 I do think it would be helpful to introduce that helper, especially if it is useful in this patch series. There seem to be enough other places where it can be useful to make it worth adding the helper. > -- >8 -- > > This is a preparatory step prior to removing the pipes after git > commands, which discards git's exit code and may mask a crash. The commit message should also describe why we need this preparatory step. Maybe something like: To reduce the noise in when refactoring this pipeline in a subsequent commit fix the indentation. 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> > Out of curiosity, how did you create the patch? 'git format-patch' would add a '---' line followed by the diffstat, where you would usually put the commentary that you put before the scissors line. It seems like 'git am' still handles this fine, which I didn't know, just something I noticed because I'm used to the other format. Since this patch series is now 5 patches, that commentary should go into a cover letter (see the --cover-letter option in format-patch), so the reviewers can read that first, and read the patches with that in mind, focusing on the patch only, and not additional commentary that applies to the whole series when reading the patch. > 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 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error 2019-03-10 17:59 ` [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error Thomas Gummerer @ 2019-03-15 1:55 ` jonathan chang 2019-03-15 12:48 ` Christian Couder 0 siblings, 1 reply; 20+ messages in thread From: jonathan chang @ 2019-03-15 1:55 UTC (permalink / raw) To: Thomas Gummerer; +Cc: Eric Sunshine, Christian Couder, git On Mon, Mar 11, 2019 at 1:59 AM Thomas Gummerer <t.gummerer@gmail.com> wrote: > > On 03/10, Jonathan Chang wrote: > > Hi, > > > > Thanks for the reviews. > > > > Here are the changes in the second version: > > - bug fixes > > - add preparatory patch > > - seperate different files to different patch > > - change to use test_line_count in a seperate patch > > > > Also I found that there is no such function as test_char_count, > > is it worthwile to add such function? Here are some stat: > > > > `git grep 'test_line_count' | wc -l` = 626 > > `git grep 'wc -l' | wc -l` = 294 > > `git grep 'wc -c' | wc -l` = 68 > > I do think it would be helpful to introduce that helper, especially if > it is useful in this patch series. There seem to be enough other > places where it can be useful to make it worth adding the helper. Thanks for the feedback. > > -- >8 -- > > > > This is a preparatory step prior to removing the pipes after git > > commands, which discards git's exit code and may mask a crash. > > The commit message should also describe why we need this preparatory > step. Maybe something like: > > To reduce the noise in when refactoring this pipeline in a > subsequent commit fix the indentation. 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> > > > > Out of curiosity, how did you create the patch? 'git format-patch' > would add a '---' line followed by the diffstat, where you would > usually put the commentary that you put before the scissors line. It > seems like 'git am' still handles this fine, which I didn't know, just > something I noticed because I'm used to the other format. I believe I used git for that. But I cannot think of why it happened nor reproduce it. > Since this patch series is now 5 patches, that commentary should go > into a cover letter (see the --cover-letter option in format-patch), > so the reviewers can read that first, and read the patches with that > in mind, focusing on the patch only, and not additional commentary > that applies to the whole series when reading the patch. I wasn't aware of this option. I tried to produce the format in others cover letter using 'git diff' and options like '--stat', '--summary', with no success. I consulted Documentation/SubmittingPatches, where I got the idea of cover letter, but it doesn't mention the option '--cover-letter' and the idea of cover letter is even confused with '--notes'[1]. I just reread some of the GSoC related mails in the mailing list and found one[2] that introduced the usage of 'cover-letter', '--range-diff' and '--interdiff'. As a newbie, I personally think it would be helpful to include theses options along with others mentioned in SubmittingPatches. [1]: Documentation/SubmittingPatches: > You often want to add additional explanation about the patch, > other than the commit message itself. Place such "cover letter" > material between the three-dash line and the diffstat. For > patches requiring multiple iterations of review and discussion, > an explanation of changes between each iteration can be kept in > Git-notes and inserted automatically following the three-dash > line via `git format-patch --notes`. [2]: https://public-inbox.org/git/CAPig+cSsAufCnHPJfjQd8A778UNAsXEst1m+ekQ7T83=2mMUnw@mail.gmail.com/ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error 2019-03-15 1:55 ` jonathan chang @ 2019-03-15 12:48 ` Christian Couder 0 siblings, 0 replies; 20+ messages in thread From: Christian Couder @ 2019-03-15 12:48 UTC (permalink / raw) To: jonathan chang; +Cc: Thomas Gummerer, Eric Sunshine, git Hi, On Fri, Mar 15, 2019 at 2:55 AM jonathan chang <ttjtftx@gmail.com> wrote: > > On Mon, Mar 11, 2019 at 1:59 AM Thomas Gummerer <t.gummerer@gmail.com> wrote: > > > > On 03/10, Jonathan Chang wrote: > > > Also I found that there is no such function as test_char_count, > > > is it worthwile to add such function? Here are some stat: > > > > > > `git grep 'test_line_count' | wc -l` = 626 > > > `git grep 'wc -l' | wc -l` = 294 > > > `git grep 'wc -c' | wc -l` = 68 > > > > I do think it would be helpful to introduce that helper, especially if > > it is useful in this patch series. There seem to be enough other > > places where it can be useful to make it worth adding the helper. Yeah, it would be useful for Git, but it's not necessary of course for your microproject, which is already more than big enough. > > > This is a preparatory step prior to removing the pipes after git > > > commands, which discards git's exit code and may mask a crash. > > > > The commit message should also describe why we need this preparatory > > step. Maybe something like: > > > > To reduce the noise in when refactoring this pipeline in a > > subsequent commit fix the indentation. Or perhaps: 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> > > Since this patch series is now 5 patches, that commentary should go > > into a cover letter (see the --cover-letter option in format-patch), > > so the reviewers can read that first, and read the patches with that > > in mind, focusing on the patch only, and not additional commentary > > that applies to the whole series when reading the patch. > > I wasn't aware of this option. I tried to produce the format in others > cover letter using 'git diff' and options like '--stat', '--summary', with no > success. I consulted Documentation/SubmittingPatches, where I got the > idea of cover letter, but it doesn't mention the option '--cover-letter' and > the idea of cover letter is even confused with '--notes'[1]. The issue is that there are different things that can be considered "cover letter". I agree that it's not very clear in Documentation/SubmittingPatches. There is the following which is related to --cover-letter: "Multiple related patches should be grouped into their own e-mail thread to help readers find all parts of the series. To that end, send them as replies to either an additional "cover letter" message (see below), the first patch, or the respective preceding patch." but it doesn't mention --cover-letter and it contains "(see below)" though I don't see what that refers to. > I just reread some of the GSoC related mails in the mailing list and > found one[2] that introduced the usage of 'cover-letter', '--range-diff' and > '--interdiff'. As a newbie, I personally think it would be helpful to include > theses options along with others mentioned in SubmittingPatches. I agree that there is room for improvement in SubmittingPatches. Thanks, Christian. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [GSoC][PATCH] tests: avoid using pipes 2019-03-09 15:45 [GSoC][PATCH] tests: avoid using pipes Jonathan Chang 2019-03-09 16:45 ` Thomas Gummerer @ 2019-03-10 6:05 ` Christian Couder 2019-03-10 8:27 ` ttjtftx 1 sibling, 1 reply; 20+ messages in thread From: Christian Couder @ 2019-03-10 6:05 UTC (permalink / raw) To: Jonathan Chang; +Cc: git On Sat, Mar 9, 2019 at 4:50 PM Jonathan Chang <ttjtftx@gmail.com> wrote: > > This is my attempt for this year's GSoC microproject. Thanks for being interested in this year's GSoC and doing a microproject. > I copied the commit message of the commit[1] mentioned in the microproject > page[2]. Is this OK? > > Here is a summary of what I did: > - simple substitution as in c6f44e1da5[1]. If you take a look at c6f44e1da5 ("t9813: avoid using pipes", 2017-01-04) you can see the following: - it changes only one test file: t9813-git-p4-preserve-users.sh - its title starts with "t9813: " It would have been nice if you did the same in your patch (that is only change one test file and start the patch title with the test file number). On the microproject page, we say: "Students: Please attempt only ONE microproject. We want quality, not quantity!" And to other students working on a microproject, we also ask them to focus on only one test file when they make those kinds of changes. Best, Christian. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [GSoC][PATCH] tests: avoid using pipes 2019-03-10 6:05 ` [GSoC][PATCH] tests: avoid using pipes Christian Couder @ 2019-03-10 8:27 ` ttjtftx 2019-03-10 15:05 ` Christian Couder 0 siblings, 1 reply; 20+ messages in thread From: ttjtftx @ 2019-03-10 8:27 UTC (permalink / raw) To: Christian Couder; +Cc: git Thanks for the reminders. On Sun, Mar 10, 2019 at 2:06 PM Christian Couder <christian.couder@gmail.com> wrote: > If you take a look at c6f44e1da5 ("t9813: avoid using pipes", > 2017-01-04) you can see the following: > > - it changes only one test file: t9813-git-p4-preserve-users.sh > - its title starts with "t9813: " I adapted this format in my second version[1]. [1]: https://public-inbox.org/git/20190310080739.63984-1-ttjtftx@gmail.com Thanks, Jonathan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [GSoC][PATCH] tests: avoid using pipes 2019-03-10 8:27 ` ttjtftx @ 2019-03-10 15:05 ` Christian Couder [not found] ` <CAOAu_YL8heWLSznRV8pjLkRZBOEth_7CSmftupx+4+SSx5yztw@mail.gmail.com> 0 siblings, 1 reply; 20+ messages in thread From: Christian Couder @ 2019-03-10 15:05 UTC (permalink / raw) To: ttjtftx; +Cc: git On Sun, Mar 10, 2019 at 9:28 AM ttjtftx <ttjtftx@gmail.com> wrote: > > On Sun, Mar 10, 2019 at 2:06 PM Christian Couder > <christian.couder@gmail.com> wrote: > > > If you take a look at c6f44e1da5 ("t9813: avoid using pipes", > > 2017-01-04) you can see the following: > > > > - it changes only one test file: t9813-git-p4-preserve-users.sh > > - its title starts with "t9813: " > I adapted this format in my second version[1]. > > [1]: https://public-inbox.org/git/20190310080739.63984-1-ttjtftx@gmail.com It's better because each patch changes only one file, but I also wanted to say that for a microproject you only need to focus on only one file. So I would you suggest you keep only the patches that are about "t0000-basic.sh" and drop the patches about "t0003-attributes.sh" and "t0022-crlf-rename.sh". (https://git.github.io/SoC-2019-Microprojects/ has now a "Only ONE quality focused microproject per student" section with more content.) It's also a small nit but your patches start with "t0000-basic: " not just "t0000: " though the latter format is more frequent in existing commits than the former. ^ permalink raw reply [flat|nested] 20+ messages in thread
[parent not found: <CAOAu_YL8heWLSznRV8pjLkRZBOEth_7CSmftupx+4+SSx5yztw@mail.gmail.com>]
* Fwd: [GSoC][PATCH] tests: avoid using pipes [not found] ` <CAOAu_YL8heWLSznRV8pjLkRZBOEth_7CSmftupx+4+SSx5yztw@mail.gmail.com> @ 2019-03-11 16:45 ` jonathan chang 0 siblings, 0 replies; 20+ messages in thread From: jonathan chang @ 2019-03-11 16:45 UTC (permalink / raw) To: git Sorry, I forgot to Cc, again. ---------- Forwarded message --------- From: ttjtftx <ttjtftx@gmail.com> Date: Tue, Mar 12, 2019 at 12:32 AM Subject: Re: [GSoC][PATCH] tests: avoid using pipes To: Christian Couder <christian.couder@gmail.com> On Sun, Mar 10, 2019 at 11:05 PM Christian Couder <christian.couder@gmail.com> wrote: > > On Sun, Mar 10, 2019 at 9:28 AM ttjtftx <ttjtftx@gmail.com> wrote: > > > > On Sun, Mar 10, 2019 at 2:06 PM Christian Couder > > <christian.couder@gmail.com> wrote: > > > > > If you take a look at c6f44e1da5 ("t9813: avoid using pipes", > > > 2017-01-04) you can see the following: > > > > > > - it changes only one test file: t9813-git-p4-preserve-users.sh > > > - its title starts with "t9813: " > > I adapted this format in my second version[1]. > > > > [1]: https://public-inbox.org/git/20190310080739.63984-1-ttjtftx@gmail.com > > It's better because each patch changes only one file, but I also > wanted to say that for a microproject you only need to focus on only > one file. So I would you suggest you keep only the patches that are > about "t0000-basic.sh" and drop the patches about > "t0003-attributes.sh" and "t0022-crlf-rename.sh". I will do that as I have made too many mistakes. I decided to do more than one file because I thought it might be too small even for a microproject and that there would be more chances for feedback if I include more files, since the same file tend to have similar conversions needed. > It's also a small nit but your patches start with "t0000-basic: " not > just "t0000: " though the latter format is more frequent in existing > commits than the former. Got it. ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2019-03-15 12:48 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-09 15:45 [GSoC][PATCH] tests: avoid using pipes Jonathan Chang 2019-03-09 16:45 ` Thomas Gummerer 2019-03-10 8:07 ` [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error Jonathan Chang 2019-03-10 8:08 ` [GSoC][PATCH v2 2/5] t0000-basic: avoid using pipes Jonathan Chang 2019-03-10 8:09 ` [GSoC][PATCH v2 3/5] t0003-attributes: " Jonathan Chang 2019-03-10 8:10 ` [GSoC][PATCH v2 4/5] t0022-crlf-rename: " Jonathan Chang 2019-03-10 8:11 ` [GSoC][PATCH v2 5/5] t0000-basic: use test_line_count instead of wc -l Jonathan Chang 2019-03-10 9:50 ` Eric Sunshine 2019-03-11 16:10 ` ttjtftx 2019-03-10 10:03 ` [GSoC][PATCH v2 4/5] t0022-crlf-rename: avoid using pipes Eric Sunshine [not found] ` <CAOAu_YJKNjGd3mw7K17ySQJeF4XxC+V00FFEYA7o593riEGN1g@mail.gmail.com> 2019-03-11 15:54 ` Fwd: " ttjtftx 2019-03-10 10:13 ` [GSoC][PATCH v2 3/5] t0003-attributes: " Eric Sunshine 2019-03-15 1:56 ` jonathan chang 2019-03-10 17:59 ` [GSoC][PATCH v2 1/5] t0000-basic: fix an indentation error Thomas Gummerer 2019-03-15 1:55 ` jonathan chang 2019-03-15 12:48 ` Christian Couder 2019-03-10 6:05 ` [GSoC][PATCH] tests: avoid using pipes Christian Couder 2019-03-10 8:27 ` ttjtftx 2019-03-10 15:05 ` Christian Couder [not found] ` <CAOAu_YL8heWLSznRV8pjLkRZBOEth_7CSmftupx+4+SSx5yztw@mail.gmail.com> 2019-03-11 16:45 ` Fwd: " jonathan chang
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).