{"thread":{"id":"53593","subject":"[PATCH 1/2] t5318: use 'test_must_be_empty'","startedAt":"2020-06-01T18:01:37Z","lastAt":"2020-06-03T22:21:11Z","messageCount":7,"participants":["Taylor Blau","Derrick Stolee","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"398984","messageId":"fddbe588147444fe2675224c94f50e24a1d12871.1591034353.git.me@ttaylorr.com","threadId":"53593","inReplyTo":"cover.1591034353.git.me@ttaylorr.com","subject":"[PATCH 1/2] t5318: use 'test_must_be_empty'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-01T18:01:27Z","receivedAt":"2020-06-01T18:01:37Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"A handful of tests in t5318 use 'test_line_count = 0 ...' to make sure\nthat some command does not write any output. While correct, it is more\nhelpful to use 'test_must_be_empty' instead, since the latter prints the\ncontents of the file if it is non-empty.\n\nSince 'test_line_count' only prints the expected and actual line count,\nnot the contents, using 'test_must_be_empty' may be more helpful for\ndebugging if there is regression in any of these tests.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t5318-commit-graph.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex a79c624875..d23986f603 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -147,7 +147,7 @@ test_expect_success 'Add more commits' '\n test_expect_success 'commit-graph write progress off for redirected stderr' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit commit-graph write 2>err &&\n-\ttest_line_count = 0 err\n+\ttest_must_be_empty err\n '\n \n test_expect_success 'commit-graph write force progress on for stderr' '\n@@ -159,13 +159,13 @@ test_expect_success 'commit-graph write force progress on for stderr' '\n test_expect_success 'commit-graph write with the --no-progress option' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit commit-graph write --no-progress 2>err &&\n-\ttest_line_count = 0 err\n+\ttest_must_be_empty err\n '\n \n test_expect_success 'commit-graph verify progress off for redirected stderr' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit commit-graph verify 2>err &&\n-\ttest_line_count = 0 err\n+\ttest_must_be_empty err\n '\n \n test_expect_success 'commit-graph verify force progress on for stderr' '\n@@ -177,7 +177,7 @@ test_expect_success 'commit-graph verify force progress on for stderr' '\n test_expect_success 'commit-graph verify with the --no-progress option' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit commit-graph verify --no-progress 2>err &&\n-\ttest_line_count = 0 err\n+\ttest_must_be_empty err\n '\n \n # Current graph structure:\n-- \n2.26.2.1052.gcc6b3749ab\n\n"},{"id":"398987","messageId":"cc6b3749abc039f8502f490d3b7c7cad485810bc.1591034353.git.me@ttaylorr.com","threadId":"53593","inReplyTo":"cover.1591034353.git.me@ttaylorr.com","subject":"[PATCH 2/2] t5318: test that '--stdin-commits' respects '--[no-]progress'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-01T18:01:31Z","receivedAt":"2020-06-01T18:55:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The following lines were not covered in a recent line-coverage test\nagainst Git:\n\n  builtin/commit-graph.c\n  5b6653e5 244) progress = start_delayed_progress(\n  5b6653e5 268) stop_progress(&progress);\n\nThese statements are executed when both '--stdin-commits' and\n'--progress' are passed. Introduce a trio of tests that exercise various\ncombinations of these options to ensure that these lines are covered.\n\nMore importantly, this is exercising a (somewhat) previously-ignored\nfeature of '--stdin-commits', which is that it respects '--progress'.\nPrior to 5b6653e523 (builtin/commit-graph.c: dereference tags in\nbuiltin, 2020-05-13), dereferencing input from '--stdin-commits' was\ndone inside of commit-graph.c.\n\nNow that an additional progress meter may be generated from outside of\ncommit-graph.c, add a corresponding test to make sure that it also\nrespects '--[no]-progress'.\n\nThe other location that generates progress meter output (from d335ce8f24\n(commit-graph.c: show progress of finding reachable commits,\n2020-05-13)) is already covered by any test that passes '--reachable'.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t5318-commit-graph.sh | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex d23986f603..26f332d6a3 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -162,6 +162,27 @@ test_expect_success 'commit-graph write with the --no-progress option' '\n \ttest_must_be_empty err\n '\n \n+test_expect_success 'commit-graph write --stdin-commits progress off for redirected stderr' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit rev-parse commits/5 >in &&\n+\tgit commit-graph write --stdin-commits <in 2>err &&\n+\ttest_must_be_empty err\n+'\n+\n+test_expect_success 'commit-graph write --stdin-commits force progress on for stderr' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit rev-parse commits/5 >in &&\n+\tGIT_PROGRESS_DELAY=0 git commit-graph write --stdin-commits --progress <in 2>err &&\n+\ttest_i18ngrep \"Collecting commits from input\" err\n+'\n+\n+test_expect_success 'commit-graph write --stdin-commits with the --no-progress option' '\n+\tcd \"$TRASH_DIRECTORY/full\" &&\n+\tgit rev-parse commits/5 >in &&\n+\tgit commit-graph write --stdin-commits --no-progress <in 2>err &&\n+\ttest_must_be_empty err\n+'\n+\n test_expect_success 'commit-graph verify progress off for redirected stderr' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit commit-graph verify 2>err &&\n-- \n2.26.2.1052.gcc6b3749ab\n"},{"id":"398988","messageId":"cover.1591034353.git.me@ttaylorr.com","threadId":"53593","inReplyTo":null,"subject":"[PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-01T18:01:23Z","receivedAt":"2020-06-01T18:55:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Here's a short pair of patches that I wrote this morning after looking\nat Stolee's most recent coverage report.\n\nThe first patch is just cleanup, and the second patch is the real\nchange. It would have been nice to parameterize these tests over the\narguments to 'git commit graph' (ie., have three tests for 'write',\n'verify', and 'write --stdin-commits'), but '--stdin-commits' is special\nsince it requires input.\n\nThese patches are based off the tip of 'next', but really only need my\nchanges from 'tb/commit-graph-no-check-oids'.\n\nTaylor Blau (2):\n  t5318: use 'test_must_be_empty'\n  t5318: test that '--stdin-commits' respects '--[no-]progress'\n\n t/t5318-commit-graph.sh | 29 +++++++++++++++++++++++++----\n 1 file changed, 25 insertions(+), 4 deletions(-)\n\n--\n2.26.2.1052.gcc6b3749ab\n"},{"id":"398991","messageId":"153faa41-2801-d6bc-1f48-05a0ac08b446@gmail.com","threadId":"53593","inReplyTo":"cover.1591034353.git.me@ttaylorr.com","subject":"Re: [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress'","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-06-01T19:35:31Z","receivedAt":"2020-06-01T19:35:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/1/2020 2:01 PM, Taylor Blau wrote:\n> Here's a short pair of patches that I wrote this morning after looking\n> at Stolee's most recent coverage report.\n> \n> The first patch is just cleanup, and the second patch is the real\n> change. It would have been nice to parameterize these tests over the\n> arguments to 'git commit graph' (ie., have three tests for 'write',\n> 'verify', and 'write --stdin-commits'), but '--stdin-commits' is special\n> since it requires input.\n> \n> These patches are based off the tip of 'next', but really only need my\n> changes from 'tb/commit-graph-no-check-oids'.\n\nThe first patch is an obviously good patch, and it even has a good\njustification in the message.\n\nThe second is also good. The case of forcing \"--progress\" would be\nenough for covering your new-ish progress meter. Perhaps the other\ntests (or at least the one specifying \"--no-progress\") could be\nremoved, but I don't feel strongly about that.\n\nThanks,\n-Stolee\n"},{"id":"398992","messageId":"20200601193648.GA32262@syl.local","threadId":"53593","inReplyTo":"153faa41-2801-d6bc-1f48-05a0ac08b446@gmail.com","subject":"Re: [PATCH 0/2] t5318: test '--stdin-commits' with '--[no-]progress'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-01T19:36:48Z","receivedAt":"2020-06-01T19:36:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jun 01, 2020 at 03:35:31PM -0400, Derrick Stolee wrote:\n> On 6/1/2020 2:01 PM, Taylor Blau wrote:\n> > Here's a short pair of patches that I wrote this morning after looking\n> > at Stolee's most recent coverage report.\n> >\n> > The first patch is just cleanup, and the second patch is the real\n> > change. It would have been nice to parameterize these tests over the\n> > arguments to 'git commit graph' (ie., have three tests for 'write',\n> > 'verify', and 'write --stdin-commits'), but '--stdin-commits' is special\n> > since it requires input.\n> >\n> > These patches are based off the tip of 'next', but really only need my\n> > changes from 'tb/commit-graph-no-check-oids'.\n>\n> The first patch is an obviously good patch, and it even has a good\n> justification in the message.\n\nThanks.\n\n> The second is also good. The case of forcing \"--progress\" would be\n> enough for covering your new-ish progress meter. Perhaps the other\n> tests (or at least the one specifying \"--no-progress\") could be\n> removed, but I don't feel strongly about that.\n\nYeah, I don't feel strongly either. I figured that it would at least be\nmore consistent with the surrounding tests to have the three variants. I\nguess we can see what others think, too.\n\n> Thanks,\n> -Stolee\n\nThanks,\nTaylor\n"},{"id":"399041","messageId":"20200602180403.GA4791@szeder.dev","threadId":"53593","inReplyTo":"fddbe588147444fe2675224c94f50e24a1d12871.1591034353.git.me@ttaylorr.com","subject":"Re: [PATCH 1/2] t5318: use 'test_must_be_empty'","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-06-02T18:04:03Z","receivedAt":"2020-06-02T18:04:13Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Jun 01, 2020 at 12:01:27PM -0600, Taylor Blau wrote:\n> A handful of tests in t5318 use 'test_line_count = 0 ...' to make sure\n> that some command does not write any output. While correct, it is more\n> helpful to use 'test_must_be_empty' instead, since the latter prints the\n> contents of the file if it is non-empty.\n> \n> Since 'test_line_count' only prints the expected and actual line count,\n> not the contents, using 'test_must_be_empty' may be more helpful for\n> debugging if there is regression in any of these tests.\n\nThese two paragraphs essentially say the same thing, so I think only\none would be sufficient, but...  Both paragraphs are wrong, because\n'test_line_count' does include the content of the file on failure:\n\n  expecting success of 9999.1 'test': \n          cat >foo <<-EOF &&\n          Add\n          some\n          content\n          EOF\n          test_line_count = 0 foo\n  \n  test_line_count: line count for foo != 0\n  Add\n  some\n  content\n  not ok 1 - test\n\nHaving said that, I think that the change itself is good, because\n'test_must_be_empty foo' is more idiomatic.\n\n"},{"id":"399095","messageId":"20200603222107.GA37471@syl.local","threadId":"53593","inReplyTo":"20200602180403.GA4791@szeder.dev","subject":"Re: [PATCH 1/2] t5318: use 'test_must_be_empty'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-06-03T22:21:07Z","receivedAt":"2020-06-03T22:21:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 02, 2020 at 08:04:03PM +0200, SZEDER Gábor wrote:\n> On Mon, Jun 01, 2020 at 12:01:27PM -0600, Taylor Blau wrote:\n> > A handful of tests in t5318 use 'test_line_count = 0 ...' to make sure\n> > that some command does not write any output. While correct, it is more\n> > helpful to use 'test_must_be_empty' instead, since the latter prints the\n> > contents of the file if it is non-empty.\n> >\n> > Since 'test_line_count' only prints the expected and actual line count,\n> > not the contents, using 'test_must_be_empty' may be more helpful for\n> > debugging if there is regression in any of these tests.\n>\n> These two paragraphs essentially say the same thing, so I think only\n> one would be sufficient, but...  Both paragraphs are wrong, because\n> 'test_line_count' does include the content of the file on failure:\n>\n>   expecting success of 9999.1 'test':\n>           cat >foo <<-EOF &&\n>           Add\n>           some\n>           content\n>           EOF\n>           test_line_count = 0 foo\n>\n>   test_line_count: line count for foo != 0\n>   Add\n>   some\n>   content\n>   not ok 1 - test\n>\n> Having said that, I think that the change itself is good, because\n> 'test_must_be_empty foo' is more idiomatic.\n\nSounds good. Let's use this version of the patch instead, and otherwise\nI think this should be ready to go:\n\n--- >8 ---\n\nSubject: [PATCH] t5318: use 'test_must_be_empty'\n\nA handful of tests in t5318 use 'test_line_count = 0 ...' to make sure\nthat some command does not write any output. While correct, it is more\nidiomatic to use 'test_must_be_empty' instead. Switch the former\ninvocations to use the latter instead.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n t/t5318-commit-graph.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh\nindex a79c624875..d23986f603 100755\n--- a/t/t5318-commit-graph.sh\n+++ b/t/t5318-commit-graph.sh\n@@ -147,7 +147,7 @@ test_expect_success 'Add more commits' '\n test_expect_success 'commit-graph write progress off for redirected stderr' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit commit-graph write 2>err &&\n-\ttest_line_count = 0 err\n+\ttest_must_be_empty err\n '\n\n test_expect_success 'commit-graph write force progress on for stderr' '\n@@ -159,13 +159,13 @@ test_expect_success 'commit-graph write force progress on for stderr' '\n test_expect_success 'commit-graph write with the --no-progress option' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit commit-graph write --no-progress 2>err &&\n-\ttest_line_count = 0 err\n+\ttest_must_be_empty err\n '\n\n test_expect_success 'commit-graph verify progress off for redirected stderr' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit commit-graph verify 2>err &&\n-\ttest_line_count = 0 err\n+\ttest_must_be_empty err\n '\n\n test_expect_success 'commit-graph verify force progress on for stderr' '\n@@ -177,7 +177,7 @@ test_expect_success 'commit-graph verify force progress on for stderr' '\n test_expect_success 'commit-graph verify with the --no-progress option' '\n \tcd \"$TRASH_DIRECTORY/full\" &&\n \tgit commit-graph verify --no-progress 2>err &&\n-\ttest_line_count = 0 err\n+\ttest_must_be_empty err\n '\n\n # Current graph structure:\n--\n2.27.0\n\n"}]}