(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@xxxxxxxxxxx>
> - 1 files changed, 1 insertions(+), 0 deletions(-)
> + 1 file changed, 1 insertion(+)
> [master OBJID] third
> Author: A U Thor <author@xxxxxxxxxxx>
> - 1 files changed, 1 insertions(+), 0 deletions(-)
> + 1 file changed, 1 insertion(+)
> [master OBJID] fourth
> Author: A U Thor <author@xxxxxxxxxxx>
> - 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html