--exit-code (and --quiet) broken in git-diff?

--exit-code (and --quiet) broken in git-diff?

[Wincent Colaiuta]

The git-diff man page documents an "--exit-code" option, as well as a
"--quiet" option which automatically implies the former.

In my tests on Mac OS X and Bash 3, however, "git diff" always return an
exit code of 0, never of 1, regardless of how I use the "--quiet" and
"--exit-code" options. I see that there are tests in t/t4017-quiet.sh for
the lower-level git-diff-files, git-diff-index, git-diff-tree commands,
but none for the porcelain git-diff.

Is this a bug with a missing test case? Or am I using this incorrectly? In
the example below I'm looking for differences between the working tree and
the last commit, so I'm using "git diff HEAD", but as you can see, the
exit code is always 0 for "git diff" and "git diff --cached" as well:

$ git --version
git version 1.5.2.4
$ mkdir example
$ cd example
$ git init
Initialized empty Git repository in .git/
$ echo "start" > foo
$ git add foo
$ git commit -m "Add foo"
Created initial commit 85954f6: Add foo
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 foo
$ git diff --quiet HEAD; echo $?
0
$ echo "more" >> foo
$ git diff --quiet HEAD; echo $?
0
$ git add foo
$ git diff --quiet HEAD; echo $?
0
$ git diff --quiet; echo $?
0
$ git diff --exit-code; echo $?
0
$ git diff --cached --quiet; echo $?
0

Cheers,
Wincent

Re: --exit-code (and --quiet) broken in git-diff?

[René Scharfe]

Wincent Colaiuta schrieb:
> The git-diff man page documents an "--exit-code" option, as well as a
> "--quiet" option which automatically implies the former.
> 
> In my tests on Mac OS X and Bash 3, however, "git diff" always return an
> exit code of 0, never of 1, regardless of how I use the "--quiet" and
> "--exit-code" options. I see that there are tests in t/t4017-quiet.sh for
> the lower-level git-diff-files, git-diff-index, git-diff-tree commands,
> but none for the porcelain git-diff.
> 
> Is this a bug with a missing test case? Or am I using this incorrectly? In
> the example below I'm looking for differences between the working tree and
> the last commit, so I'm using "git diff HEAD", but as you can see, the
> exit code is always 0 for "git diff" and "git diff --cached" as well:
> 
> $ git --version
> git version 1.5.2.4
> $ mkdir example
> $ cd example
> $ git init
> Initialized empty Git repository in .git/
> $ echo "start" > foo
> $ git add foo
> $ git commit -m "Add foo"
> Created initial commit 85954f6: Add foo
>  1 files changed, 1 insertions(+), 0 deletions(-)
>  create mode 100644 foo
> $ git diff --quiet HEAD; echo $?
> 0
> $ echo "more" >> foo
> $ git diff --quiet HEAD; echo $?
> 0
> $ git add foo
> $ git diff --quiet HEAD; echo $?
> 0
> $ git diff --quiet; echo $?
> 0
> $ git diff --exit-code; echo $?
> 0
> $ git diff --cached --quiet; echo $?
> 0

git diff passes the output through your pager by default, so you see the
exit code of that instead of diff's.  Set PAGER=cat or redirect the
output to /dev/null to get rid of it.

A test case for diff would be nice regardless, though. :)

René

Re: --exit-code (and --quiet) broken in git-diff?

[Wincent Colaiuta]

René Scharfe wrote:
>
> git diff passes the output through your pager by default, so you see the
> exit code of that instead of diff's.  Set PAGER=cat or redirect the
> output to /dev/null to get rid of it.

Ah, thanks very much René!

Best wishes,
Wincent

Re: --exit-code (and --quiet) broken in git-diff?

[David Kastrup]

"Wincent Colaiuta" <win@wincent.com> writes:

> René Scharfe wrote:
>>
>> git diff passes the output through your pager by default, so you see the
>> exit code of that instead of diff's.  Set PAGER=cat or redirect the
>> output to /dev/null to get rid of it.
>
> Ah, thanks very much René!

I think I would call that a mistake.  However, I don't see that fixing
it would actually be useful: if a pager gets called, this means that
git-diff might die with SIGPIPE (when the user quits the pager), and
that in turn has pretty much no meaning.  So one really needs to
redirect the output, anyway.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: --exit-code (and --quiet) broken in git-diff?

[Steven Grimm]

David Kastrup wrote:
> I think I would call that a mistake.  However, I don't see that fixing
> it would actually be useful: if a pager gets called, this means that
> git-diff might die with SIGPIPE (when the user quits the pager), and
> that in turn has pretty much no meaning.  So one really needs to
> redirect the output, anyway.
>   

It does sort of make one wonder, though, if there's much point ever 
launching a pager when git-diff is run with --quiet -- it will never 
produce any output to page, so running a pager is guaranteed to always 
be a waste of cycles.

Unfortunately the pager is launched before the option processing code 
knows whether --quiet is being used or not; I'm not sure it's worth 
refactoring the pager launch code just to handle this one special case. 
(Or are there other cases where programs would want to be able to 
control the use of the pager?)

-Steve

Re: --exit-code (and --quiet) broken in git-diff?

[David Kastrup]

Steven Grimm <koreth@midwinter.com> writes:

> David Kastrup wrote:
>> I think I would call that a mistake.  However, I don't see that fixing
>> it would actually be useful: if a pager gets called, this means that
>> git-diff might die with SIGPIPE (when the user quits the pager), and
>> that in turn has pretty much no meaning.  So one really needs to
>> redirect the output, anyway.
>>   
>
> It does sort of make one wonder, though, if there's much point ever
> launching a pager when git-diff is run with --quiet -- it will never
> produce any output to page, so running a pager is guaranteed to always
> be a waste of cycles.
>
> Unfortunately the pager is launched before the option processing code
> knows whether --quiet is being used or not; I'm not sure it's worth
> refactoring the pager launch code just to handle this one special
> case. (Or are there other cases where programs would want to be able
> to control the use of the pager?)

I think it is reasonable not to start the pager at all when there is
no bulk material, but just a fixed amount of output such as a summary
lines.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: --exit-code (and --quiet) broken in git-diff?

[Wincent Colaiuta]

El 12/8/2007, a las 15:02, Steven Grimm escribió:

> David Kastrup wrote:
>> I think I would call that a mistake.  However, I don't see that  
>> fixing
>> it would actually be useful: if a pager gets called, this means that
>> git-diff might die with SIGPIPE (when the user quits the pager), and
>> that in turn has pretty much no meaning.  So one really needs to
>> redirect the output, anyway.
>>
>
> It does sort of make one wonder, though, if there's much point ever  
> launching a pager when git-diff is run with --quiet -- it will  
> never produce any output to page, so running a pager is guaranteed  
> to always be a waste of cycles.

Yes, I thought the same thing when I read in the initial release  
notes (<http://www.kernel.org/pub/software/scm/git/docs/ 
RelNotes-1.5.1.txt>) that the "--quiet" option was explicitly "meant  
for scripted use", where naturally you don't want or need a pager...  
turns out I was writing a script, you see, and I wanted to test the  
commands out manually to be sure they did what I thought they would...

But as you say, refactoring this to bypass the pager when "--quiet"  
is passed may be unpleasantly complicated.

Cheers,
Wincent

Re: --exit-code (and --quiet) broken in git-diff?

[Steffen Prohaska]

On Aug 12, 2007, at 11:40 AM, René Scharfe wrote:

>
> git diff passes the output through your pager by default, so you  
> see the
> exit code of that instead of diff's.  Set PAGER=cat or redirect the
> output to /dev/null to get rid of it.
>
> A test case for diff would be nice regardless, though. :)

Which is not that easy, because of the redirections done
by test-lib.sh. I think git-diff needs a tty to exhibit
the wrong behaviour, which it doesn't get from test-lib
if run in standard mode (non verbose).

I tried to write a test case once, but after it got more
and more complex, I was too lazy to complete it.

	Steffen

[PATCH] diff: don't run pager if user asked for a diff style exit code

[René Scharfe]

As Wincent Colaiuta found out, it's a bit unexpected for git diff to
start a pager even when the --quiet option is specified.  The problem
is that the pager hides the return code -- which is the only output
we're interested in in this case.

Push pager setup down into builtin-diff.c and don't start the pager
if --exit-code or --quiet (which implies --exit-code) was specified.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---

 builtin-diff.c |    6 ++++++
 git.c          |    2 +-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/builtin-diff.c b/builtin-diff.c
index b48121e..8dc17b0 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -235,6 +235,12 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
 	rev.diffopt.allow_external = 1;
 	rev.diffopt.recursive = 1;
 
+	/* If the user asked for our exit code then don't start a
+	 * pager or we would end up reporting its exit code instead.
+	 */
+	if (!rev.diffopt.exit_with_status)
+		setup_pager();
+
 	/* Do we have --cached and not have a pending object, then
 	 * default to HEAD by hand.  Eek.
 	 */
diff --git a/git.c b/git.c
index e5daae0..cab0e72 100644
--- a/git.c
+++ b/git.c
@@ -325,7 +325,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "config", cmd_config },
 		{ "count-objects", cmd_count_objects, RUN_SETUP },
 		{ "describe", cmd_describe, RUN_SETUP },
-		{ "diff", cmd_diff, USE_PAGER },
+		{ "diff", cmd_diff },
 		{ "diff-files", cmd_diff_files },
 		{ "diff-index", cmd_diff_index, RUN_SETUP },
 		{ "diff-tree", cmd_diff_tree, RUN_SETUP },

Re: [PATCH] diff: don't run pager if user asked for a diff style exit code

[Wincent Colaiuta]

El 12/8/2007, a las 19:46, René Scharfe escribió:

> Push pager setup down into builtin-diff.c and don't start the pager
> if --exit-code or --quiet (which implies --exit-code) was specified.

Great stuff, René. The change looks much simpler than I thought it  
would.

Cheers,
Wincent

Re: [PATCH] diff: don't run pager if user asked for a diff style exit code

[David Kastrup]

Wincent Colaiuta <win@wincent.com> writes:

> El 12/8/2007, a las 19:46, René Scharfe escribió:
>
>> Push pager setup down into builtin-diff.c and don't start the pager
>> if --exit-code or --quiet (which implies --exit-code) was specified.
>
> Great stuff, René. The change looks much simpler than I thought it  
> would.

Personally, I'd start the pager only when the first diff or stat gets
output, anyway.  For a summary message or even a silent exit, a pager
is annoying, regardless of whether --exit-code was specified.

-- 
David Kastrup

Re: [PATCH] diff: don't run pager if user asked for a diff style exit code

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> As Wincent Colaiuta found out, it's a bit unexpected for git diff to
> start a pager even when the --quiet option is specified.  The problem
> is that the pager hides the return code -- which is the only output
> we're interested in in this case.
>
> Push pager setup down into builtin-diff.c and don't start the pager
> if --exit-code or --quiet (which implies --exit-code) was specified.

Surprisingly nice, although I would have said "don't use
git-diff wrapper from scripts, use underlying diff-* plumbing"
;-).

Thanks.