Re: [PATCH v2] Use correct grammar in diffstat summary line

[Jonathan Nieder <jrnieder@gmail.com>]

(dropping Thomas from cc)
Nguyễn Thái Ngọc Duy wrote:

>  And this patch's diffstat looks just scary due to test suite's updates.

Not about this patch, but: let's see if we can chisel away at that.

Hidden below are also two minor bug reports about the patch.  See (*)
and (**) below.

> --- a/t/t0023-crlf-am.sh
> +++ b/t/t0023-crlf-am.sh
> @@ -12,7 +12,7 @@ Subject: test1
>  
>  ---
>   foo |    1 +
> - 1 files changed, 1 insertions(+), 0 deletions(-)
> + 1 file changed, 1 insertion(+)
>   create mode 100644 foo

This patchfile is sample input to "git am", and this patch hunk
changes it to match the modified format-patch output format.  But
don't we want "git am" to work with old patches, too?  So this hunk is
unnecessary.

[...]
> --- a/t/t1200-tutorial.sh
> +++ b/t/t1200-tutorial.sh
> @@ -156,7 +156,7 @@ Updating VARIABLE..VARIABLE
>  FASTFORWARD (no commit created; -m option ignored)
>   example |    1 +
>   hello   |    1 +
> - 2 files changed, 2 insertions(+), 0 deletions(-)
> + 2 files changed, 2 insertions(+)
>  EOF

Yes, this one's necessary.

(*) It's reminding us to update the gitcore-tutorial. :)

[...]
> --- a/t/t3300-funny-names.sh
> +++ b/t/t3300-funny-names.sh
> @@ -167,7 +167,7 @@ test_expect_success TABS_IN_FILENAMES 'git diff-tree delete with-funny' \
>  test_expect_success TABS_IN_FILENAMES 'setup expect' '
>  cat >expected <<\EOF
>   "tabs\t,\" (dq) and spaces"
> - 1 files changed, 0 insertions(+), 0 deletions(-)
> + 1 file changed, 0 insertions(+), 0 deletions(-)
>  EOF
>  '

I'm not sure why this and other tests in the same file use "git apply
--stat" to check their work.  The descriptions refer to diff-tree but
the tests themselves use diff-index | apply --stat.  Puzzling.

[...]
> --- a/t/t3508-cherry-pick-many-commits.sh
> +++ b/t/t3508-cherry-pick-many-commits.sh
> @@ -38,13 +38,13 @@ test_expect_success 'cherry-pick first..fourth works' '
>  	cat <<-\EOF >expected &&
>  	[master OBJID] second
>  	 Author: A U Thor <author@example.com>
> -	 1 files changed, 1 insertions(+), 0 deletions(-)
> +	 1 file changed, 1 insertion(+)
>  	[master OBJID] third
>  	 Author: A U Thor <author@example.com>
> -	 1 files changed, 1 insertions(+), 0 deletions(-)
> +	 1 file changed, 1 insertion(+)
>  	[master OBJID] fourth
>  	 Author: A U Thor <author@example.com>
> -	 1 files changed, 1 insertions(+), 0 deletions(-)
> +	 1 file changed, 1 insertion(+)
>  	EOF

Probably should be split out as a separate test so cherry-pick's
behavior and progress reporting can be tested separately.  Aside from
that detail, makes sense.

[...]
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -444,7 +444,7 @@ test_expect_success 'stash show - stashes on stack, stash-like argument' '
>  	git reset --hard &&
>  	cat >expected <<-EOF &&
>  	 file |    1 +
> -	 1 files changed, 1 insertions(+), 0 deletions(-)
> +	 1 file changed, 1 insertion(+)

Hm.  "git stash show" defaults to --stat, but these tests are not
really about that.  Maybe there should be one test that checks that
the default uses --stat and others could use --numstat or similar?

This will be a problem with GETTEXT_POISON (except your patch bypasses
poison so it got missed --- will send a relevant fix as a reply).

[...]
> +++ b/t/t4013/diff.diff-tree_--cc_--patch-with-stat_--summary_master
> +++ b/t/t4013/diff.diff-tree_--cc_--patch-with-stat_--summary_side
> +++ b/t/t4013/diff.diff-tree_--cc_--patch-with-stat_master
> +++ b/t/t4013/diff.diff-tree_--cc_--stat_--summary_master
[...]
> +++ b/t/t4013/diff.format-patch_--attach_--stdout_initial..side
> +++ b/t/t4013/diff.format-patch_--inline_--stdout_--numbered-files_initial..master
> +++ b/t/t4013/diff.format-patch_--inline_--stdout_--subject-prefix=TESTCASE_initial..master
[...]

Testing the --stat option.  Can't really be avoided.  (Though maybe
some more modular tests for these combinations can be made some day.)

[...]
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -520,7 +520,7 @@ test_expect_success 'shortlog of cover-letter wraps overly-long onelines' '
>  cat > expect << EOF
>  ---
>   file |   16 ++++++++++++++++
> - 1 files changed, 16 insertions(+), 0 deletions(-)
> + 1 file changed, 16 insertions(+)

Not relevant to the detail being tested, should probably be fuzzed
out.

[...]
> --- a/t/t4030-diff-textconv.sh
> +++ b/t/t4030-diff-textconv.sh
> @@ -86,7 +86,7 @@ test_expect_success 'status -v produces text' '
>  
>  cat >expect.stat <<'EOF'
>   file |  Bin 2 -> 4 bytes
> - 1 files changed, 0 insertions(+), 0 deletions(-)
> + 1 file changed, 0 insertions(+), 0 deletions(-)
>  EOF
>  test_expect_success 'diffstat does not run textconv' '
>  	echo file diff=fail >.gitattributes &&

Testing diffstat.  Would be nice to have an accompanying test of
numstat.

[...]
> --- a/t/t4045-diff-relative.sh
> +++ b/t/t4045-diff-relative.sh
> @@ -33,7 +33,7 @@ check_stat() {
>  expect=$1; shift
>  cat >expected <<EOF
>   $expect |    1 +
> - 1 files changed, 1 insertions(+), 0 deletions(-)
> + 1 file changed, 1 insertion(+)
>  EOF

Not the detail being tested, so technically it would be nicer to
fuzz the last line out.

[...]
> --- a/t/t4049-diff-stat-count.sh
> +++ b/t/t4049-diff-stat-count.sh
> @@ -16,7 +16,7 @@ test_expect_success setup '
>  	cat >expect <<-\EOF
>  	 a |    1 +
>  	 b |    1 +
> -	 2 files changed, 2 insertions(+), 0 deletions(-)
> +	 2 files changed, 2 insertions(+)
>  	EOF
>  	git diff --stat --stat-count=2 >actual &&
>  	test_cmp expect actual

Testing diffstat.  Looks sane.

[...]
> +++ b/t/t4100/t-apply-8.expect
> @@ -1,2 +1,2 @@
>   t/t4100-apply-stat.sh |    2 +-
> - 1 files changed, 1 insertions(+), 1 deletions(-)
> + 1 file changed, 1 insertion(+), 1 deletion(-)
[...]
> +++ b/t/t4100/t-apply-9.expect
> @@ -1,2 +1,2 @@
>   t/t4100-apply-stat.sh |    2 +-
> - 1 files changed, 1 insertions(+), 1 deletions(-)
> + 1 file changed, 1 insertion(+), 1 deletion(-)

Testing apply --stat.  Tests of apply --numstat would be more
interesting, but then we'd still need some tests like these to make
sure --stat is consistent with it, so it still seems sane.

> --- a/t/t5150-request-pull.sh
> +++ b/t/t5150-request-pull.sh
> @@ -95,7 +95,7 @@ test_expect_success 'setup: two scripts for reading pull requests' '
>  	b
>  	: diffstat
>  	n
> -	/ [0-9]* files changed/ {
> +	/ [0-9]* files\? changed/ {

Mimicking a human.

(**) Should probably use "*" instead of \? --- \? is a GNU extension,
not a BRE.

[...]
> +++ b/t/t7602-merge-octopus-many.sh
> @@ -57,7 +57,7 @@ Merge made by the 'octopus' strategy.
>   c2.c |    1 +
>   c3.c |    1 +
>   c4.c |    1 +
> - 3 files changed, 3 insertions(+), 0 deletions(-)
> + 3 files changed, 3 insertions(+)
>   create mode 100644 c2.c
>   create mode 100644 c3.c
>   create mode 100644 c4.c

Test is about the "Trying simple merge with" lines, not the final
diffstat summary.  Should probably be fuzzed out.

Thanks, that was interesting.

Jonathan