patchdiff --shortstat --dirstat: remove duplicate output

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

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

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

Johan Herland <johan@herland.net> writes:

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

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:

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 03:23:37PM +0100, Michael J Gruber wrote: []

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:

Thanks for the clarification. That looks worthwhile.

Michael

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

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

Hi,

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

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

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

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

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

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