Re: [PATCH] diff --shortstat --dirstat: remove duplicate output

Johan Herland [off-list ref] writes:

On Sat, Feb 28, 2015 at 2:19 PM, Mårten Kongstad
[off-list ref] wrote:
[...]

quoted

Signed-off-by: Mårten Kongstad <redacted>

Acked-by: Johan Herland <redacted>

Interesting.  So nobody in real life uses --dirstat and --shortstat
together?

I am not very happy with the added tests that hardcode exact numbers
that are shown, as the counting algorithm can be improved.  Can't we
do better?

Thanks.

When --shortstat is used in conjunction with --dirstat=changes, git diff will
output the dirstat information twice: first as calculated by the 'lines'
algorithm, then as calculated by the 'changes' algorithm:

    $ git diff --dirstat=changes,10 --shortstat v2.2.0..v2.2.1
     23 files changed, 453 insertions(+), 54 deletions(-)
      33.5% Documentation/RelNotes/
      26.2% t/
      46.6% Documentation/RelNotes/
      16.6% t/

The same duplication happens for --shortstat together with --dirstat=files, but
not for --shortstat together with --dirstat=lines.

Limit output to only include one dirstat part, calculated as specified
by the --dirstat parameter. Also, add test for this.

Signed-off-by: Mårten Kongstad <redacted>
---
Good point about hardcoded values in the tests. How about instead we check that
a specific directory appears exactly once in the output?

 diff.c                  |  2 +-
 t/t4047-diff-dirstat.sh | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index d1bd534..abc32c8 100644
--- a/diff.c
+++ b/diff.c

@@ -4541,7 +4541,7 @@ void diff_flush(struct diff_options *options)
 			show_stats(&diffstat, options);
 		if (output_format & DIFF_FORMAT_SHORTSTAT)
 			show_shortstats(&diffstat, options);
-		if (output_format & DIFF_FORMAT_DIRSTAT)
+		if (output_format & DIFF_FORMAT_DIRSTAT && dirstat_by_line)
 			show_dirstat_by_line(&diffstat, options);
 		free_diffstat_info(&diffstat);
 		separator++;

diff --git a/t/t4047-diff-dirstat.sh b/t/t4047-diff-dirstat.sh
index ed7e093..128f7bf 100755
--- a/t/t4047-diff-dirstat.sh
+++ b/t/t4047-diff-dirstat.sh

@@ -973,4 +973,15 @@ test_expect_success 'diff.dirstat=future_param,0,lines should warn, but still wo
 	test_i18ngrep -q "diff\\.dirstat" actual_error
 '
 
+test_expect_success '--shortstat --dirstat should output only one dirstat' '
+	git diff --shortstat --dirstat=changes HEAD^..HEAD >actual_diff_shortstat_dirstat_changes &&
+	test $(grep -c " dst/copy/changed/$" actual_diff_shortstat_dirstat_changes) = 1 &&
+
+	git diff --shortstat --dirstat=lines HEAD^..HEAD >actual_diff_shortstat_dirstat_lines &&
+	test $(grep -c " dst/copy/changed/$" actual_diff_shortstat_dirstat_lines) = 1 &&
+
+	git diff --shortstat --dirstat=files HEAD^..HEAD >actual_diff_shortstat_dirstat_files &&
+	test $(grep -c " dst/copy/changed/$" actual_diff_shortstat_dirstat_files) = 1
+'
+
 test_done

-- 
1.9.1

On 2015-03-01 08.39, Mårten Kongstad wrote:
[]
 index ed7e093..128f7bf 100755

How portable is the "grep -c" usage ?
(I don't now it either, do we have other opinions ?), but the following seems to be more "Git-style":

test_expect_success '--shortstat --dirstat should output only one dirstat' '
	git diff --shortstat --dirstat=changes HEAD^..HEAD >actual_diff_shortstat_dirstat_changes &&
	grep " dst/copy/changed/$" actual_diff_shortstat_dirstat_changes >actual &&
	test_line_count = 1 actual

Torsten Bögershausen venit, vidit, dixit 01.03.2015 11:25:

On 2015-03-01 08.39, Mårten Kongstad wrote:
[]
 index ed7e093..128f7bf 100755

quoted

--- a/t/t4047-diff-dirstat.sh
+++ b/t/t4047-diff-dirstat.sh

@@ -973,4 +973,15 @@ test_expect_success 'diff.dirstat=future_param,0,lines should warn, but still wo
 	test_i18ngrep -q "diff\\.dirstat" actual_error
 '
 
+test_expect_success '--shortstat --dirstat should output only one dirstat' '
+	git diff --shortstat --dirstat=changes HEAD^..HEAD >actual_diff_shortstat_dirstat_changes &&
+	test $(grep -c " dst/copy/changed/$" actual_diff_shortstat_dirstat_changes) = 1 &&

How portable is the "grep -c" usage ?
(I don't now it either, do we have other opinions ?), but the following seems to be more "Git-style":

test_expect_success '--shortstat --dirstat should output only one dirstat' '
	git diff --shortstat --dirstat=changes HEAD^..HEAD >actual_diff_shortstat_dirstat_changes &&
	grep " dst/copy/changed/$" actual_diff_shortstat_dirstat_changes >actual &&
	test_line_count = 1 actual

If I would have had to guess from the documentation: What does "git diff
--dirstat --shortstat" do? I would have answered: It displays both the
dirstat and the shortstat.

So, is what you are trying to "fix" a peculiarity of
"--dirstat=changes", or do you simplify prefer --dirstat and --shortstat
to override each other?

Maybe I'm overlooking something (and that's not a rhetorical
conditional), but if you specify both options when you want the output
of only one them, the answer would be the obvious one, not a patch,
wouldn't it?

If there is indeed a good reason to change the behavior it should be
documented.

Michael

On Sun, Mar 01, 2015 at 11:25:53AM +0100, Torsten Bögershausen wrote:

On 2015-03-01 08.39, Mårten Kongstad wrote:
[]

quoted

+test_expect_success '--shortstat --dirstat should output only one dirstat' '
+	git diff --shortstat --dirstat=changes HEAD^..HEAD >actual_diff_shortstat_dirstat_changes &&
+	test $(grep -c " dst/copy/changed/$" actual_diff_shortstat_dirstat_changes) = 1 &&

How portable is the "grep -c" usage ?
(I don't now it either, do we have other opinions ?), but the following seems to be more "Git-style":

test_expect_success '--shortstat --dirstat should output only one dirstat' '
	git diff --shortstat --dirstat=changes HEAD^..HEAD >actual_diff_shortstat_dirstat_changes &&
	grep " dst/copy/changed/$" actual_diff_shortstat_dirstat_changes >actual &&
	test_line_count = 1 actual

From what I can see, both 'grep -c' and 'grep >file && test_line_count'
are used in the tests.

'grep -c' is used in these tests:
- t3404-rebase-interactive.sh
- t3507-cherry-pick-conflict.sh
- t4036-format-patch-signer-mime.sh
- t4150-am.sh
- t7810-grep.sh
- t8003-blame-corner-cases.sh
- t9350-fast-export.sh

'grep >file && test_line_count' is used in this test:
- t9400-git-cvsserver-server.sh

And to make matters more confusing, both 'grep -c' and 'grep >file &&
test_line_count' is used in this test:
- t9001-send-email.sh

Granted I didn't miss anything while trawling the tests for the above
numbers, it feels like the 'grep -c' option is more in line with the
existing tests. That said, I don't know if there is an ongoing trend to
deprecate 'grep -c' in favour of 'test_line_count'.

On Sun, Mar 01, 2015 at 03:23:37PM +0100, Michael J Gruber wrote:
[]

If I would have had to guess from the documentation: What does "git diff
--dirstat --shortstat" do? I would have answered: It displays both the
dirstat and the shortstat.

So, is what you are trying to "fix" a peculiarity of
"--dirstat=changes", or do you simplify prefer --dirstat and --shortstat
to override each other?

Maybe I'm overlooking something (and that's not a rhetorical
conditional), but if you specify both options when you want the output
of only one them, the answer would be the obvious one, not a patch,
wouldn't it?

If there is indeed a good reason to change the behavior it should be
documented.

I interpret the documentation the same way as you do. The problem is
that the dirstat is displayed twice for --dirstat=changes (or
--dirstat=files):

$ git diff --dirstat=changes,10 --shortstat v2.2.0..v2.2.1
 23 files changed, 453 insertions(+), 54 deletions(-)
  33.5% Documentation/RelNotes/
  26.2% t/
  46.6% Documentation/RelNotes/
  16.6% t/

but only once for --dirstat=lines:

$ git diff --dirstat=lines,10 --shortstat v2.2.0..v2.2.1
 23 files changed, 453 insertions(+), 54 deletions(-)
  33.5% Documentation/RelNotes/
  26.2% t/

This behaviour is either a bug, or an inconsistency not immediately apparent to
the user.

The proposed patch will make the 'changes' and 'files' cases behave like
'lines', i.e. output one shortstat and (only) one dirstat:

$ patched-version-of-git diff --dirstat=changes,10 --shortstat v2.2.0..v2.2.1
 23 files changed, 453 insertions(+), 54 deletions(-)
  46.6% Documentation/RelNotes/
  16.6% t/

Mårten Kongstad venit, vidit, dixit 01.03.2015 17:01:

On Sun, Mar 01, 2015 at 03:23:37PM +0100, Michael J Gruber wrote:
[]

quoted

If I would have had to guess from the documentation: What does "git diff
--dirstat --shortstat" do? I would have answered: It displays both the
dirstat and the shortstat.

So, is what you are trying to "fix" a peculiarity of
"--dirstat=changes", or do you simplify prefer --dirstat and --shortstat
to override each other?

Maybe I'm overlooking something (and that's not a rhetorical
conditional), but if you specify both options when you want the output
of only one them, the answer would be the obvious one, not a patch,
wouldn't it?

If there is indeed a good reason to change the behavior it should be
documented.

I interpret the documentation the same way as you do. The problem is
that the dirstat is displayed twice for --dirstat=changes (or
--dirstat=files):

$ git diff --dirstat=changes,10 --shortstat v2.2.0..v2.2.1
 23 files changed, 453 insertions(+), 54 deletions(-)
  33.5% Documentation/RelNotes/
  26.2% t/
  46.6% Documentation/RelNotes/
  16.6% t/

but only once for --dirstat=lines:

$ git diff --dirstat=lines,10 --shortstat v2.2.0..v2.2.1
 23 files changed, 453 insertions(+), 54 deletions(-)
  33.5% Documentation/RelNotes/
  26.2% t/

This behaviour is either a bug, or an inconsistency not immediately apparent to
the user.

The proposed patch will make the 'changes' and 'files' cases behave like
'lines', i.e. output one shortstat and (only) one dirstat:

$ patched-version-of-git diff --dirstat=changes,10 --shortstat v2.2.0..v2.2.1
 23 files changed, 453 insertions(+), 54 deletions(-)
  46.6% Documentation/RelNotes/
  16.6% t/

Thanks for the clarification. That looks worthwhile.

Michael

Hi,

Quoting Mårten Kongstad [off-list ref]:

On Sun, Mar 01, 2015 at 11:25:53AM +0100, Torsten Bögershausen wrote:

quoted

On 2015-03-01 08.39, Mårten Kongstad wrote:
[]

quoted

+test_expect_success '--shortstat --dirstat should output only

one dirstat' '

quoted

+	git diff --shortstat --dirstat=changes HEAD^..HEAD
actual_diff_shortstat_dirstat_changes &&
+	test $(grep -c " dst/copy/changed/$"

actual_diff_shortstat_dirstat_changes) = 1 &&
How portable is the "grep -c" usage ?
(I don't now it either, do we have other opinions ?), but the
following seems to be more "Git-style":

test_expect_success '--shortstat --dirstat should output only one dirstat' '
	git diff --shortstat --dirstat=changes HEAD^..HEAD

quoted

actual_diff_shortstat_dirstat_changes &&

	grep " dst/copy/changed/$" actual_diff_shortstat_dirstat_changes >actual &&
	test_line_count = 1 actual

Granted I didn't miss anything while trawling the tests for the above
numbers, it feels like the 'grep -c' option is more in line with the
existing tests. That said, I don't know if there is an ongoing trend to
deprecate 'grep -c' in favour of 'test_line_count'.

It's not just 'grep -c' but the 'test' checking its output as well.

If something goes wrong and the line count doesn't match expectations
'test' fails silently leaving the developer clueless as to what went
wrong.

'test_line_count', on the other hand, produces useful output in case
of a failure:

    $ printf 'foo\nbar\n' >actual
    $ test_line_count = 1 actual
    test_line_count: line count for actual != 1
    foo
    bar

Since the name of the file in question is included in the output and
since there are three separate checks in this test, I would also
suggest writing 'grep's output into separate files
'actual_{changes,lines,files}'.

Gábor

When --shortstat is used in conjunction with --dirstat=changes, git diff will
output the dirstat information twice: first as calculated by the 'lines'
algorithm, then as calculated by the 'changes' algorithm:

    $ git diff --dirstat=changes,10 --shortstat v2.2.0..v2.2.1
     23 files changed, 453 insertions(+), 54 deletions(-)
      33.5% Documentation/RelNotes/
      26.2% t/
      46.6% Documentation/RelNotes/
      16.6% t/

The same duplication happens for --shortstat together with --dirstat=files, but
not for --shortstat together with --dirstat=lines.

Limit output to only include one dirstat part, calculated as specified
by the --dirstat parameter. Also, add test for this.

Signed-off-by: Mårten Kongstad <redacted>
---
v3: change how tests count (part of) the dirstat number of lines: instead of
'grep -c', use 'grep >filename && test_line_count'. Thanks to Torsten
Bögershausen and SZEDER Gábor for pointing out how to improve the tests.

 diff.c                  |  2 +-
 t/t4047-diff-dirstat.sh | 11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index d1bd534..abc32c8 100644
--- a/diff.c
+++ b/diff.c

@@ -4541,7 +4541,7 @@ void diff_flush(struct diff_options *options)
 			show_stats(&diffstat, options);
 		if (output_format & DIFF_FORMAT_SHORTSTAT)
 			show_shortstats(&diffstat, options);
-		if (output_format & DIFF_FORMAT_DIRSTAT)
+		if (output_format & DIFF_FORMAT_DIRSTAT && dirstat_by_line)
 			show_dirstat_by_line(&diffstat, options);
 		free_diffstat_info(&diffstat);
 		separator++;

diff --git a/t/t4047-diff-dirstat.sh b/t/t4047-diff-dirstat.sh
index ed7e093..065d74f 100755
--- a/t/t4047-diff-dirstat.sh
+++ b/t/t4047-diff-dirstat.sh

@@ -973,4 +973,15 @@ test_expect_success 'diff.dirstat=future_param,0,lines should warn, but still wo
 	test_i18ngrep -q "diff\\.dirstat" actual_error
 '
 
+test_expect_success '--shortstat --dirstat should output only one dirstat' '
+	git diff --shortstat --dirstat=changes HEAD^..HEAD | grep " dst/copy/changed/$" >actual_diff_shortstat_dirstat_changes &&
+	test_line_count = 1 actual_diff_shortstat_dirstat_changes &&
+
+	git diff --shortstat --dirstat=lines HEAD^..HEAD | grep " dst/copy/changed/$" >actual_diff_shortstat_dirstat_lines &&
+	test_line_count = 1 actual_diff_shortstat_dirstat_lines &&
+
+	git diff --shortstat --dirstat=files HEAD^..HEAD | grep " dst/copy/changed/$" >actual_diff_shortstat_dirstat_files &&
+	test_line_count = 1 actual_diff_shortstat_dirstat_files
+'
+
 test_done

-- 
1.9.1

On Mon, Mar 02, 2015 at 02:00:09AM +0100, SZEDER Gábor wrote:

It's not just 'grep -c' but the 'test' checking its output as well.

If something goes wrong and the line count doesn't match expectations
'test' fails silently leaving the developer clueless as to what went
wrong.

'test_line_count', on the other hand, produces useful output in case
of a failure:

   $ printf 'foo\nbar\n' >actual
   $ test_line_count = 1 actual
   test_line_count: line count for actual != 1
   foo
   bar

Since we have test_line_count, I think it makes sense to use it. But for
reference, I recently introduced the `verbose` function to test-lib.sh,
which lets you write:

  $ verbose test 1 = 2
  command failed:  'test' '1' '=' '2'

You can use it with any command that might fail without printing a
useful error message. The big downside is that it sees only the
arguments to the command, so if you write:

  $ test "$(do_something)" = 123

you will only see:

  command failed:  'test '456' '=' '123'

with no notion that $(do_something) was involved. So purpose-built
helpers like test_line_count will produce better output.

You may also be introduced in the "-x" option I recently introduced,
which can help with "quiet" failures. E.g.:

  $ ./t0001-init.sh -x --verbose-only=15
  [...]
  ok 13 - GIT_DIR & GIT_WORK_TREE (2)
  ok 14 - reinit
  
  expecting success: 
          mkdir template-source &&
          echo content >template-source/file &&
          git init --template=../template-source template-custom &&
          test_cmp template-source/file template-custom/.git/file
  
  + mkdir template-source
  + echo content
  + git init --template=../template-source template-custom
  Initialized empty Git repository in /home/peff/compile/git/t/trash directory.t0001-init/template-custom/.git/
  + test_cmp template-source/file template-custom/.git/file
  + diff -u template-source/file template-custom/.git/file
  ok 15 - init with --template
  ok 16 - init with --template (blank)

(ok, it's not that interesting because the test didn't fail, but
hopefully you get the point).

Now I'll stop hijacking your thread to advertise random test-lib
features. :)

-Peff

Mårten Kongstad [off-list ref] writes:

v3: change how tests count (part of) the dirstat number of lines: instead of
'grep -c', use 'grep >filename && test_line_count'. Thanks to Torsten
Bögershausen and SZEDER Gábor for pointing out how to improve the tests.

Thanks.

I'd squash the following on top before queuing it.

The overlong lines that ignores the exit status from "git diff"
looked problematic to me.

 t/t4047-diff-dirstat.sh | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/t/t4047-diff-dirstat.sh b/t/t4047-diff-dirstat.sh
index 065d74f..0d50dce 100755
--- a/t/t4047-diff-dirstat.sh
+++ b/t/t4047-diff-dirstat.sh

@@ -974,13 +974,16 @@ test_expect_success 'diff.dirstat=future_param,0,lines should warn, but still wo
 '
 
 test_expect_success '--shortstat --dirstat should output only one dirstat' '
-	git diff --shortstat --dirstat=changes HEAD^..HEAD | grep " dst/copy/changed/$" >actual_diff_shortstat_dirstat_changes &&
+	git diff --shortstat --dirstat=changes HEAD^..HEAD >out &&
+	grep " dst/copy/changed/$" out >actual_diff_shortstat_dirstat_changes &&
 	test_line_count = 1 actual_diff_shortstat_dirstat_changes &&
 
-	git diff --shortstat --dirstat=lines HEAD^..HEAD | grep " dst/copy/changed/$" >actual_diff_shortstat_dirstat_lines &&
+	git diff --shortstat --dirstat=lines HEAD^..HEAD >out &&
+	grep " dst/copy/changed/$" out >actual_diff_shortstat_dirstat_lines &&
 	test_line_count = 1 actual_diff_shortstat_dirstat_lines &&
 
-	git diff --shortstat --dirstat=files HEAD^..HEAD | grep " dst/copy/changed/$" >actual_diff_shortstat_dirstat_files &&
+	git diff --shortstat --dirstat=files HEAD^..HEAD >out &&
+	grep " dst/copy/changed/$" out >actual_diff_shortstat_dirstat_files &&
 	test_line_count = 1 actual_diff_shortstat_dirstat_files
 '