threads / patch / 39915

patchdiff-tree: do not show the sha1 of the given head with --quiet

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

## tl;dr

14 messages between Jul 22, 2015 and Jul 24, 2015. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

Sebastian Schuberth· Jul 22, 2015, 09:29 UTC · lore

"--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(-)
Show changes to 2 files +3 −3

log-tree.c, t/t4035-diff-quiet.sh

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
Sebastian Schuberth· Jul 22, 2015, 11:56 UTC · re: Johannes Schindelin · lore

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

"--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(-)
Show changes to 2 files +3 −3

log-tree.c, t/t4035-diff-quiet.sh

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
Junio C Hamano· Jul 22, 2015, 20:32 UTC · re: Sebastian Schuberth · lore

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

Sebastian Schuberth <sschuberth@gmail.com> writes:
Show 8 quoted lines
> "--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.

Sebastian Schuberth· Jul 23, 2015, 07:06 UTC · re: Junio C Hamano · lore

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

On Wed, Jul 22, 2015 at 10:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
>> "--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
Junio C Hamano· Jul 23, 2015, 16:38 UTC · re: Sebastian Schuberth · lore

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

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.

Junio C Hamano· Jul 23, 2015, 17:06 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 16 quoted lines
> 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.
Sebastian Schuberth· Jul 23, 2015, 20:13 UTC · re: Junio C Hamano · lore

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

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
Jeff King· Jul 23, 2015, 18:08 UTC · re: Sebastian Schuberth · lore

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

On Thu, Jul 23, 2015 at 09:06:01AM +0200, Sebastian Schuberth wrote:
Show 17 quoted lines
> 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
Junio C Hamano· Jul 23, 2015, 19:39 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
> 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 ;-).

Sebastian Schuberth· Jul 23, 2015, 20:19 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Jul 23, 2015, 20:43 UTC · re: Sebastian Schuberth · lore

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

Sebastian Schuberth <sschuberth@gmail.com> writes:
Show 8 quoted lines
> 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.

Sebastian Schuberth· Jul 23, 2015, 20:02 UTC · re: Jeff King · lore

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

On Thu, Jul 23, 2015 at 8:08 PM, Jeff King <peff@peff.net> wrote:
Show 5 quoted lines
> 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
Jeff King· Jul 24, 2015, 06:56 UTC · re: Sebastian Schuberth · lore

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

On Thu, Jul 23, 2015 at 10:02:27PM +0200, Sebastian Schuberth wrote:
Show 11 quoted lines
> 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

← back to recent threads