patch, 2 partst5318: use 'test_must_be_empty'

Here's a short pair of patches that I wrote this morning after looking at Stolee's most recent coverage report.

The first patch is just cleanup, and the second patch is the real change. It would have been nice to parameterize these tests over the arguments to 'git commit graph' (ie., have three tests for 'write', 'verify', and 'write --stdin-commits'), but '--stdin-commits' is special since it requires input.

These patches are based off the tip of 'next', but really only need my changes from 'tb/commit-graph-no-check-oids'.

Taylor Blau (2):
  t5318: use 'test_must_be_empty'
  t5318: test that '--stdin-commits' respects '--[no-]progress'

 t/t5318-commit-graph.sh | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

-- 2.26.2.1052.gcc6b3749ab

A handful of tests in t5318 use 'test_line_count = 0 ...' to make sure that some command does not write any output. While correct, it is more helpful to use 'test_must_be_empty' instead, since the latter prints the contents of the file if it is non-empty.

Since 'test_line_count' only prints the expected and actual line count, not the contents, using 'test_must_be_empty' may be more helpful for debugging if there is regression in any of these tests.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t5318-commit-graph.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

On Mon, Jun 01, 2020 at 12:01:27PM -0600, Taylor Blau wrote:

These two paragraphs essentially say the same thing, so I think only one would be sufficient, but... Both paragraphs are wrong, because 'test_line_count' does include the content of the file on failure:

  expecting success of 9999.1 'test': 
          cat >foo <<-EOF &&
          Add
          some
          content
          EOF
          test_line_count = 0 foo
  
  test_line_count: line count for foo != 0
  Add
  some
  content
  not ok 1 - test

Having said that, I think that the change itself is good, because 'test_must_be_empty foo' is more idiomatic.

On Tue, Jun 02, 2020 at 08:04:03PM +0200, SZEDER Gábor wrote:

Sounds good. Let's use this version of the patch instead, and otherwise I think this should be ready to go:

--- >8 ---

Subject: [PATCH] t5318: use 'test_must_be_empty'

A handful of tests in t5318 use 'test_line_count = 0 ...' to make sure that some command does not write any output. While correct, it is more idiomatic to use 'test_must_be_empty' instead. Switch the former invocations to use the latter instead.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t5318-commit-graph.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

The following lines were not covered in a recent line-coverage test against Git:

  builtin/commit-graph.c
  5b6653e5 244) progress = start_delayed_progress(
  5b6653e5 268) stop_progress(&progress);

These statements are executed when both '--stdin-commits' and '--progress' are passed. Introduce a trio of tests that exercise various combinations of these options to ensure that these lines are covered.

More importantly, this is exercising a (somewhat) previously-ignored feature of '--stdin-commits', which is that it respects '--progress'. Prior to 5b6653e523 (builtin/commit-graph.c: dereference tags in builtin, 2020-05-13), dereferencing input from '--stdin-commits' was done inside of commit-graph.c.

Now that an additional progress meter may be generated from outside of commit-graph.c, add a corresponding test to make sure that it also respects '--[no]-progress'.

The other location that generates progress meter output (from d335ce8f24 (commit-graph.c: show progress of finding reachable commits, 2020-05-13)) is already covered by any test that passes '--reachable'.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t5318-commit-graph.sh | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

On 6/1/2020 2:01 PM, Taylor Blau wrote:

The first patch is an obviously good patch, and it even has a good justification in the message.

The second is also good. The case of forcing "--progress" would be enough for covering your new-ish progress meter. Perhaps the other tests (or at least the one specifying "--no-progress") could be removed, but I don't feel strongly about that.

Thanks, -Stolee

On Mon, Jun 01, 2020 at 03:35:31PM -0400, Derrick Stolee wrote:

Thanks.

Yeah, I don't feel strongly either. I figured that it would at least be more consistent with the surrounding tests to have the three variants. I guess we can see what others think, too.

Thanks, Taylor