[0/2] t5318: test '--stdin-commits' with '--[no-]progress'

Message

Taylor Blau June 1, 2020, 6:01 p.m. UTC

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

Comments

Derrick Stolee June 1, 2020, 7:35 p.m. UTC | #1

On 6/1/2020 2:01 PM, Taylor Blau wrote:
> 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'.

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

Taylor Blau June 1, 2020, 7:36 p.m. UTC | #2

On Mon, Jun 01, 2020 at 03:35:31PM -0400, Derrick Stolee wrote:
> On 6/1/2020 2:01 PM, Taylor Blau wrote:
> > 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'.
>
> The first patch is an obviously good patch, and it even has a good
> justification in the message.

Thanks.

> 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.

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,
> -Stolee

Thanks,
Taylor