Re: [PATCH v2] Use correct grammar in diffstat summary line
From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:52:54
(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.
quoted hunk ↗ jump to hunk
--- 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. [...]
quoted hunk ↗ jump to hunk
--- 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. :) [...]
quoted hunk ↗ jump to hunk
--- 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. [...]
quoted hunk ↗ jump to hunk
--- 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. [...]
quoted hunk ↗ jump to hunk
--- 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). [...]
quoted hunk ↗ jump to hunk
+++ 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
[...]
quoted hunk ↗ jump to hunk
+++ 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.) [...]
quoted hunk ↗ jump to hunk
--- 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. [...]
quoted hunk ↗ jump to hunk
--- 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. [...]
quoted hunk ↗ jump to hunk
--- 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. [...]
quoted hunk ↗ jump to hunk
--- 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. [...]
quoted hunk ↗ jump to hunk
+++ 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(-)
[...]
quoted hunk ↗ jump to hunk
+++ 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.
quoted hunk ↗ jump to hunk
--- 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. [...]
quoted hunk ↗ jump to hunk
+++ 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