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

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

[Mårten Kongstad]

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 <marten.kongstad@gmail.com>
---
 diff.c                  |  2 +-
 t/t4047-diff-dirstat.sh | 52 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 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..208760e 100755
--- a/t/t4047-diff-dirstat.sh
+++ b/t/t4047-diff-dirstat.sh
@@ -973,4 +973,56 @@ test_expect_success 'diff.dirstat=future_param,0,lines should warn, but still wo
 	test_i18ngrep -q "diff\\.dirstat" actual_error
 '
 
+cat <<EOF >expect_diff_shortstat_dirstat_changes
+ 11 files changed, 62 insertions(+), 32 deletions(-)
+  10.8% dst/copy/changed/
+  10.8% dst/copy/rearranged/
+  10.8% dst/copy/unchanged/
+  10.8% dst/move/changed/
+  10.8% dst/move/rearranged/
+  10.8% dst/move/unchanged/
+  10.8% src/move/changed/
+  10.8% src/move/rearranged/
+  10.8% src/move/unchanged/
+EOF
+
+cat <<EOF >expect_diff_shortstat_dirstat_lines
+ 11 files changed, 62 insertions(+), 32 deletions(-)
+  10.6% dst/copy/changed/
+  10.6% dst/copy/rearranged/
+  10.6% dst/copy/unchanged/
+  10.6% dst/move/changed/
+  10.6% dst/move/rearranged/
+  10.6% dst/move/unchanged/
+  10.6% src/move/changed/
+  10.6% src/move/rearranged/
+  10.6% src/move/unchanged/
+EOF
+
+cat <<EOF >expect_diff_shortstat_dirstat_files
+ 11 files changed, 62 insertions(+), 32 deletions(-)
+   9.0% changed/
+   9.0% dst/copy/changed/
+   9.0% dst/copy/rearranged/
+   9.0% dst/copy/unchanged/
+   9.0% dst/move/changed/
+   9.0% dst/move/rearranged/
+   9.0% dst/move/unchanged/
+   9.0% rearranged/
+   9.0% src/move/changed/
+   9.0% src/move/rearranged/
+   9.0% src/move/unchanged/
+EOF
+
+test_expect_success '--shortstat --dirstat should output only one dirstat' '
+	git diff --shortstat --dirstat=changes HEAD^..HEAD >actual_diff_shortstat_dirstat_changes &&
+	test_cmp expect_diff_shortstat_dirstat_changes actual_diff_shortstat_dirstat_changes &&
+
+	git diff --shortstat --dirstat=lines HEAD^..HEAD >actual_diff_shortstat_dirstat_lines &&
+	test_cmp expect_diff_shortstat_dirstat_lines actual_diff_shortstat_dirstat_lines &&
+
+	git diff --shortstat --dirstat=files HEAD^..HEAD >actual_diff_shortstat_dirstat_files &&
+	test_cmp expect_diff_shortstat_dirstat_files actual_diff_shortstat_dirstat_files
+'
+
 test_done
-- 
1.9.1

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

[Johan Herland]

On Sat, Feb 28, 2015 at 2:19 PM, Mårten Kongstad
<marten.kongstad@gmail.com> wrote:
[...]
> Signed-off-by: Mårten Kongstad <marten.kongstad@gmail.com>

Acked-by: Johan Herland <johan@herland.net>

-- 
Johan Herland, <johan@herland.net>
www.herland.net

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

[Junio C Hamano]

Johan Herland <johan@herland.net> writes:

> On Sat, Feb 28, 2015 at 2:19 PM, Mårten Kongstad
> <marten.kongstad@gmail.com> wrote:
> [...]
>> Signed-off-by: Mårten Kongstad <marten.kongstad@gmail.com>
>
> Acked-by: Johan Herland <johan@herland.net>

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.

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

[Mårten Kongstad]

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 <marten.kongstad@gmail.com>
---
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

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

[Torsten Bögershausen]

On 2015-03-01 08.39, Mårten Kongstad wrote:
[]
 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 &&
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

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

[Michael J Gruber]

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
>> --- 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

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

[Mårten Kongstad]

On Sun, Mar 01, 2015 at 11:25:53AM +0100, Torsten Bögershausen wrote:
> On 2015-03-01 08.39, Mårten Kongstad wrote:
> []
> > +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'.

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

[Mårten Kongstad]

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/

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

[Michael J Gruber]

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:
> []
>> 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

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

[SZEDER Gábor]

Hi,

Quoting Mårten Kongstad <marten.kongstad@gmail.com>:

> On Sun, Mar 01, 2015 at 11:25:53AM +0100, Torsten Bögershausen wrote:
>> On 2015-03-01 08.39, Mårten Kongstad wrote:
>> []
>>> +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
>>


> 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

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

[Mårten Kongstad]

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 <marten.kongstad@gmail.com>
---
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

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

[Jeff King]

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

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

[Junio C Hamano]

Mårten Kongstad <marten.kongstad@gmail.com> 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
 '