git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Feb 1, 2012, 23:35 UTC
Message-ID
<20120201233515.GC29599@burratino>
In-Reply-To
<1328100907-20397-1-git-send-email-pclouds@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.

Show 9 quoted lines
> --- 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.

[...]
Show 9 quoted lines
> --- 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. :)
[...]
Show 10 quoted lines
> --- 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.

[...]
Show 17 quoted lines
> --- 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.

[...]
Show 8 quoted lines
> --- 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.)

[...]
Show 8 quoted lines
> --- 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.

[...]
Show 11 quoted lines
> --- 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.

[...]
Show 9 quoted lines
> --- 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.

[...]
Show 11 quoted lines
> --- 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.
[...]
Show 5 quoted lines
> +++ 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(-)
[...]
Show 5 quoted lines
> +++ 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.

Show 8 quoted lines
> --- 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.

[...]
Show 10 quoted lines
> +++ 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
Previous: Jeff KingNext: Jonathan Nieder
Message 17 of 32 in “Correct singular form in diff summary line for human interaction”
  1. Correct singular form in diff summary line for human interactionNguyễn Thái Ngọc Duy, Jan 31, 2012
  2. Jonathan NiederJan 31, 2012
  3. Junio C HamanoJan 31, 2012
  4. Nguyen Thai Ngoc DuyFeb 1, 2012
  5. Thomas DickeyFeb 1, 2012
  6. Thomas DickeyFeb 1, 2012
  7. Nguyen Thai Ngoc DuyFeb 1, 2012
  8. Junio C HamanoFeb 1, 2012
  9. Thomas DickeyFeb 1, 2012
  10. Use correct grammar in diffstat summary lineNguyễn Thái Ngọc Duy, Feb 1, 2012
  11. Junio C HamanoFeb 1, 2012
  12. Nguyen Thai Ngoc DuyFeb 2, 2012
  13. Junio C HamanoFeb 2, 2012
  14. Nguyen Thai Ngoc DuyFeb 3, 2012
  15. Junio C HamanoFeb 3, 2012
  16. Jeff KingFeb 3, 2012
  17. Jonathan NiederFeb 1, 2012
  18. i18n: do not define gettext/ngettext in NO_GETTEXT caseJonathan Nieder, Feb 2, 2012
  19. Jonathan NiederFeb 2, 2012
  20. Jonathan NiederFeb 2, 2012
  21. 0/7 tests: diffstat summary line varies by locale (Re: [PATCH v2] Use correct grammar in diffstat summary line)Jonathan Nieder, Mar 13, 2012
  22. 1/7 test: use test_i18ncmp when checking --stat outputJonathan Nieder, Mar 13, 2012
  23. Junio C HamanoMar 13, 2012
  24. Junio C HamanoMar 13, 2012
  25. Jonathan NiederMar 13, 2012
  26. Jonathan NiederMar 13, 2012
  27. 2/7 test: use numstat instead of diffstat in funny-names testJonathan Nieder, Mar 13, 2012
  28. 3/7 test: modernize funny-names test styleJonathan Nieder, Mar 13, 2012
  29. 4/7 test: test cherry-pick functionality and output separatelyJonathan Nieder, Mar 13, 2012
  30. 5/7 test: use --numstat instead of --stat in "git stash show" testsJonathan Nieder, Mar 13, 2012
  31. 6/7 test: use numstat instead of diffstat in binary-diff testJonathan Nieder, Mar 13, 2012
  32. 7/7 diffstat summary line varies by locale: miscellanyJonathan Nieder, Mar 13, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.