[PATCH] diff-tree: do not show the sha1 of the given head with --quiet

[PATCH] diff-tree: do not show the sha1 of the given head with --quiet

[Sebastian Schuberth]

"--quite" is documented to "Disable all output of the program". Yet
calling diff-tree with a single commit like

$ git diff-tree --quiet c925fe2

was logging

c925fe23684455735c3bb1903803643a24a58d8f

to the console despite "--quite" being given. This is inconsistent with
both the docs and the behavior if more than a single commit is passed to
diff-tree. Moreover, the output of that single line seems to be documented
nowhere except in a comment for a test. Fix this inconsistency by making
diff-tree really output nothing if "--quiet" is given and fix the test
accordingly.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 log-tree.c            | 3 ++-
 t/t4035-diff-quiet.sh | 3 +--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 01beb11..3c98234 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -741,7 +741,8 @@ int log_tree_diff_flush(struct rev_info *opt)
 	}
 
 	if (opt->loginfo && !opt->no_commit_id) {
-		show_log(opt);
+		if (!DIFF_OPT_TST(&opt->diffopt, QUICK))
+			show_log(opt);
 		if ((opt->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT) &&
 		    opt->verbose_header &&
 		    opt->commit_format != CMIT_FMT_ONELINE &&
diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh
index 461f4bb..9a8225f 100755
--- a/t/t4035-diff-quiet.sh
+++ b/t/t4035-diff-quiet.sh
@@ -40,11 +40,10 @@ test_expect_success 'git diff-tree HEAD^ HEAD -- b' '
 	test_expect_code 1 git diff-tree --quiet HEAD^ HEAD -- b >cnt &&
 	test_line_count = 0 cnt
 '
-# this diff outputs one line: sha1 of the given head
 test_expect_success 'echo HEAD | git diff-tree --stdin' '
 	echo $(git rev-parse HEAD) |
 	test_expect_code 1 git diff-tree --quiet --stdin >cnt &&
-	test_line_count = 1 cnt
+	test_line_count = 0 cnt
 '
 test_expect_success 'git diff-tree HEAD HEAD' '
 	test_expect_code 0 git diff-tree --quiet HEAD HEAD >cnt &&

---
https://github.com/git/git/pull/163

Re: [PATCH] diff-tree: do not show the sha1 of the given head with --quiet

[Johannes Schindelin]

On 2015-07-22 11:29, Sebastian Schuberth wrote:
> "--quite" is documented to "Disable all output of the program".

s/--quite/quiet/

[PATCH v2] diff-tree: do not show the sha1 of the given head with --quiet

[Sebastian Schuberth]

"--quiet" is documented to "Disable all output of the program". Yet
calling diff-tree with a single commit like

$ git diff-tree --quiet c925fe2

was logging

c925fe23684455735c3bb1903803643a24a58d8f

to the console despite "--quiet" being given. This is inconsistent with
both the docs and the behavior if more than a single commit is passed to
diff-tree. Moreover, the output of that single line seems to be documented
nowhere except in a comment for a test. Fix this inconsistency by making
diff-tree really output nothing if "--quiet" is given and fix the test
accordingly.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
 log-tree.c            | 3 ++-
 t/t4035-diff-quiet.sh | 3 +--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 01beb11..3c98234 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -741,7 +741,8 @@ int log_tree_diff_flush(struct rev_info *opt)
 	}
 
 	if (opt->loginfo && !opt->no_commit_id) {
-		show_log(opt);
+		if (!DIFF_OPT_TST(&opt->diffopt, QUICK))
+			show_log(opt);
 		if ((opt->diffopt.output_format & ~DIFF_FORMAT_NO_OUTPUT) &&
 		    opt->verbose_header &&
 		    opt->commit_format != CMIT_FMT_ONELINE &&
diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh
index 461f4bb..9a8225f 100755
--- a/t/t4035-diff-quiet.sh
+++ b/t/t4035-diff-quiet.sh
@@ -40,11 +40,10 @@ test_expect_success 'git diff-tree HEAD^ HEAD -- b' '
 	test_expect_code 1 git diff-tree --quiet HEAD^ HEAD -- b >cnt &&
 	test_line_count = 0 cnt
 '
-# this diff outputs one line: sha1 of the given head
 test_expect_success 'echo HEAD | git diff-tree --stdin' '
 	echo $(git rev-parse HEAD) |
 	test_expect_code 1 git diff-tree --quiet --stdin >cnt &&
-	test_line_count = 1 cnt
+	test_line_count = 0 cnt
 '
 test_expect_success 'git diff-tree HEAD HEAD' '
 	test_expect_code 0 git diff-tree --quiet HEAD HEAD >cnt &&

---
https://github.com/git/git/pull/163

Re: [PATCH] diff-tree: do not show the sha1 of the given head with --quiet

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> "--quite" is documented to "Disable all output of the program". Yet
> calling diff-tree with a single commit like
>
> $ git diff-tree --quiet c925fe2
>
> was logging
>
> c925fe23684455735c3bb1903803643a24a58d8f

At this point, unfortunately I think we need to call that a
documentation bug.  The "output" it refers to is output from the
"diff" portion, not the "poor-man's log" portion, of the program,
where diff-tree was the workhorse behind scripted "git log" that
gave the commit object name as the preamble for each commit it
shows information about.

Re: [PATCH] diff-tree: do not show the sha1 of the given head with --quiet

[Sebastian Schuberth]

On Wed, Jul 22, 2015 at 10:32 PM, Junio C Hamano <gitster@pobox.com> wrote:

>> "--quite" is documented to "Disable all output of the program". Yet
>> calling diff-tree with a single commit like
>>
>> $ git diff-tree --quiet c925fe2
>>
>> was logging
>>
>> c925fe23684455735c3bb1903803643a24a58d8f
>
> At this point, unfortunately I think we need to call that a
> documentation bug.  The "output" it refers to is output from the
> "diff" portion, not the "poor-man's log" portion, of the program,
> where diff-tree was the workhorse behind scripted "git log" that
> gave the commit object name as the preamble for each commit it
> shows information about.

Well, from a user's perspective it does not matter which part of the
internal implementation of diff-tree is responsible for printing that
single line, a user would just expect "--quiet" to really mean
"quiet". As for almost any bug, we could turn it into a feature by
"fixing" the docs and claiming it's documented behavior. To me the
question simply is whether it makes sense for "--quiet" to not be
quiet, and I think it does not make sense. If you run diff-tree this
way there is no added value in the given output.

My use-case (also see [1]) is that I wanted to checked whether some
given commits change nothing but whitespace. So I did

if git diff-tree --quiet --ignore-space-change $commit; then
    echo "$commit only changes whitespace."
fi

just to see those SHA1s being printed to the console.

I probably could instead do

if git diff-tree --exit-code --ignore-space-change $commit > /dev/null
2>&1; then
    echo "$commit only changes whitespace."
fi

but that defeats the purpose of having "--quiet" in the first place.

[1] http://article.gmane.org/gmane.comp.version-control.git/273975

-- 
Sebastian Schuberth

Re: [PATCH] diff-tree: do not show the sha1 of the given head with --quiet

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> Well, from a user's perspective it does not matter which part of the
> internal implementation of diff-tree is responsible for printing that
> single line,...

That is not "internal implementation", but "logically separate
parts".  View it more like "'git show -s' does squelch the diff part
but does not squelch the log output".  After all, a single commit form
of 'diff-tree' is a degenerate use case of feeding a single commit
to 'diff-tree --stdin' from its standard input, which is a rough
plumbing-level equivalent of 'show'.

Documenting the behaviour correctly is the best thing you could do
at this point, as this is one of the oldest part of the system that
existing scripts would rely on.

Re: [PATCH] diff-tree: do not show the sha1 of the given head with --quiet

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Sebastian Schuberth <sschuberth@gmail.com> writes:
>
>> Well, from a user's perspective it does not matter which part of the
>> internal implementation of diff-tree is responsible for printing that
>> single line,...
>
> That is not "internal implementation", but "logically separate
> parts".  View it more like "'git show -s' does squelch the diff part
> but does not squelch the log output".  After all, a single commit form
> of 'diff-tree' is a degenerate use case of feeding a single commit
> to 'diff-tree --stdin' from its standard input, which is a rough
> plumbing-level equivalent of 'show'.
>
> Documenting the behaviour correctly is the best thing you could do
> at this point, as this is one of the oldest part of the system that
> existing scripts would rely on.

Having said that.

Existing scripts by definition would not be using a new option you
will invent that used not to be a valid one.  So that would be one
way that you can shorten your script without breaking other people.

If we were living in an ideal world equipped with a time machine, I
would redesign "git diff-tree $commit" so that it does not show the
commit object name in its output at all, with or without "--quiet".

In "git rev-list ... | git diff-tree --stdin" output, the commit
object name is absolutely necessary, with or without --quiet, as it
serves as the sign that the output switched to talk about a
different commit.  But the case that feeds a single commit to the
command, used as a poor-man's "git show $commit", does not need
one---the caller knows exactly which commit the output is about.  It
is an unfortunate historical accident that a single commit usage is
defined to be a degenerate case of feeding a sequence of commits to
the command and the length of the sequence happens to be one.

But we do not live in an ideal world.

Re: [PATCH] diff-tree: do not show the sha1 of the given head with --quiet

[Sebastian Schuberth]

On Thu, Jul 23, 2015 at 7:06 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Existing scripts by definition would not be using a new option you
> will invent that used not to be a valid one.  So that would be one
> way that you can shorten your script without breaking other people.

True. If it was only for shortening my script, I still could do ">
/dev/null 2>&1" which is just as short (or long) as a newly introduced
"--really-quiet" option. But I'm also concerned about consistency and
making options do what they sound they would do.

> In "git rev-list ... | git diff-tree --stdin" output, the commit
> object name is absolutely necessary, with or without --quiet, as it

Why is printing the object name also necessary with "--quiet"? I'd
argue that any script that uses diff-tree that way uses --stdin
without --quiet, just like you do in your example, so suppressing the
object name if "--quiet" is given probably would not break as many
scripts as you think.

> But we do not live in an ideal world.

True, but we should never stop striving after making it one :-)

-- 
Sebastian Schuberth

Re: [PATCH] diff-tree: do not show the sha1 of the given head with --quiet

[Jeff King]

On Thu, Jul 23, 2015 at 09:06:01AM +0200, Sebastian Schuberth wrote:

> My use-case (also see [1]) is that I wanted to checked whether some
> given commits change nothing but whitespace. So I did
> 
> if git diff-tree --quiet --ignore-space-change $commit; then
>     echo "$commit only changes whitespace."
> fi
> 
> just to see those SHA1s being printed to the console.
> 
> I probably could instead do
> 
> if git diff-tree --exit-code --ignore-space-change $commit > /dev/null
> 2>&1; then
>     echo "$commit only changes whitespace."
> fi
> 
> but that defeats the purpose of having "--quiet" in the first place.

I have not been following the thread closely, but I do not recall seeing
anyone mention that the reason for the sha1-output is handing
only a single commit-ish to diff-tree is what puts it into its log-like
mode. Actually asking for a two-endpoint tree diff:

  git diff-tree --quiet --ignore-space-change $commit^ $commit

will do what you want.

I know that does not necessarily help the greater issue of "what
diff-tree is doing is confusing", but perhaps that sheds some light at
least on why it is doing what it is doing. :)

-Peff

Re: [PATCH] diff-tree: do not show the sha1 of the given head with --quiet

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I have not been following the thread closely, but I do not recall seeing
> anyone mention that the reason for the sha1-output is handing
> only a single commit-ish to diff-tree is what puts it into its log-like
> mode. Actually asking for a two-endpoint tree diff:
>
>   git diff-tree --quiet --ignore-space-change $commit^ $commit
>
> will do what you want.

Yeah, if we were living in an ideal world equipped with a time
machine, I would redesign "git diff-tree $commit" so that it does
not show the commit object name in its output at all, with or
without "--quiet".

In "git rev-list ... | git diff-tree --stdin" output, the commit
object name is absolutely necessary, with or without --quiet, as it
serves as the sign that the output switched to talk about a
different commit.

But the case that feeds a single commit to the command, used as a
poor-man's "git show $commit", does not need one---the caller knows
exactly which commit the output is about.

It is an unfortunate historical accident that a single commit usage
is defined to be a degenerate case of feeding a sequence of commits
to the command and the length of the sequence happens to be one.
"diff-tree $commit" could instead have been defined as a short-hand
for "diff-tree $commit^ $commit", but (1) we do not live in an ideal
world, and (2) it ignores $commit^2 and later parents.

This is a tangent, but I suspect that the current implementation of
"diff-tree --stdin --quiet" may be buggy and does not consistently
show the commits that touch the given path.

 $ git rev-list master..jc/rerere | git diff-tree --stdin -s rerere.h

gives what is expected (shows the commit object names, but being
silent on the differences), while s/-s/--quiet/ seems to omit every
other commit from the output, or something silly like that.

I haven't dug into why that happens, but possible ways to fix that
are to make "--quiet" output all (making it consistent with "-s") or
no (making the command totally silent) output at all ;-).

Re: [PATCH] diff-tree: do not show the sha1 of the given head with --quiet

[Sebastian Schuberth]

On Thu, Jul 23, 2015 at 9:39 PM, Junio C Hamano <gitster@pobox.com> wrote:

> I haven't dug into why that happens, but possible ways to fix that
> are to make "--quiet" output all (making it consistent with "-s") or
> no (making the command totally silent) output at all ;-).

Exactly, and I chose the latter to add some value to --quiet instead
of making it an alias for -s.

-- 
Sebastian Schuberth

Re: [PATCH] diff-tree: do not show the sha1 of the given head with --quiet

[Junio C Hamano]

Sebastian Schuberth <sschuberth@gmail.com> writes:

> On Thu, Jul 23, 2015 at 9:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> I haven't dug into why that happens, but possible ways to fix that
>> are to make "--quiet" output all (making it consistent with "-s") or
>> no (making the command totally silent) output at all ;-).
>
> Exactly, and I chose the latter to add some value to --quiet instead
> of making it an alias for -s.

Heh.  You didn't even know when "diff-tree --stdin --quiet" would be
useful, let alone that it had a bug that made it useless for that
exact use case.  So it cannot be "I chose the latter".

I just gave you a hint so that you can write a plausible-sounding
justification, and we both know that it is very different from your
original motivation.

Be honest.

Perhaps the log message would say something like this:

	$ git rev-list ... | git diff-tree --stdin --quiet [$pathspec]

	is a way to list the commits that modifies the named paths,
        but this bug <<<analysis of the bug comes here>>> makes it
        not to emit all such commits.  It couldn't have been used
        by existing scripts with this longstanding bug.

	We could fix it so that it does not randomly skip commits
	that ought to be shown, but that feature is already
	available by the "-s" option instead of "--quiet".

        So let's change the meaning of "--quiet" to make it really
        quiet, without giving any output.  Strictly speaking, this
        may break backward compatibility but the existing behaviour
        to randomly omit commits couldn't have been useful, so there
        is no harm done.

	And as an added bonus,

	$ git diff-tree --quiet $commit [$pathspec]

	would stop showing the commit object name.

The analysis of the bug is really crucial for the above description
to work as justification for this change, substanciating the words
"longstanding" and "randomly omit" that are used to convince us that
this option couldn't have been used by real scripts.

Re: [PATCH] diff-tree: do not show the sha1 of the given head with --quiet

[Sebastian Schuberth]

On Thu, Jul 23, 2015 at 8:08 PM, Jeff King <peff@peff.net> wrote:

> mode. Actually asking for a two-endpoint tree diff:
>
>   git diff-tree --quiet --ignore-space-change $commit^ $commit
>
> will do what you want.

Yes, I know, thanks. But I deliberately wanted to specify only a
single commit as an optimization, hoping that it would be slightly
faster than computing a commit range.

-- 
Sebastian Schuberth

Re: [PATCH] diff-tree: do not show the sha1 of the given head with --quiet

[Jeff King]

On Thu, Jul 23, 2015 at 10:02:27PM +0200, Sebastian Schuberth wrote:

> On Thu, Jul 23, 2015 at 8:08 PM, Jeff King <peff@peff.net> wrote:
> 
> > mode. Actually asking for a two-endpoint tree diff:
> >
> >   git diff-tree --quiet --ignore-space-change $commit^ $commit
> >
> > will do what you want.
> 
> Yes, I know, thanks. But I deliberately wanted to specify only a
> single commit as an optimization, hoping that it would be slightly
> faster than computing a commit range.

Ah, I see. It should not be any faster, as git has to internally find
the first-parent of $commit either way. The big thing you lose with the
above syntax is that you are specifying two endpoints, so you cannot do
anything clever with merge commits (e.g., if you gave "--cc").

-Peff