# --exit-code (and --quiet) broken in git-diff? 12 messages from 2007-08-11 to 2007-08-13. Participants: Wincent Colaiuta, René Scharfe, David Kastrup, Steffen Prohaska, Steven Grimm, Junio C Hamano. Thread: https://gitlist.dev/t/9497 ## Wincent Colaiuta, 2007-08-11 23:12 Subject: --exit-code (and --quiet) broken in git-diff? Message-ID: <17875.88.10.191.55.1186873960.squirrel@secure.wincent.com> URL: https://gitlist.dev/e/17875.88.10.191.55.1186873960.squirrel%40secure.wincent.com ``` 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 ``` ## René Scharfe, 2007-08-12 09:40 Subject: Re: --exit-code (and --quiet) broken in git-diff? Message-ID: <46BED5AA.7050900@lsrfire.ath.cx> URL: https://gitlist.dev/e/46BED5AA.7050900%40lsrfire.ath.cx In-Reply-To: <17875.88.10.191.55.1186873960.squirrel@secure.wincent.com> ``` 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é ``` ## Wincent Colaiuta, 2007-08-12 11:24 Subject: Re: --exit-code (and --quiet) broken in git-diff? Message-ID: <24332.88.10.191.55.1186917895.squirrel@secure.wincent.com> URL: https://gitlist.dev/e/24332.88.10.191.55.1186917895.squirrel%40secure.wincent.com In-Reply-To: <46BED5AA.7050900@lsrfire.ath.cx> ``` 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 ``` ## David Kastrup, 2007-08-12 11:31 Subject: Re: --exit-code (and --quiet) broken in git-diff? Message-ID: <85643lq99h.fsf@lola.goethe.zz> URL: https://gitlist.dev/e/85643lq99h.fsf%40lola.goethe.zz In-Reply-To: <24332.88.10.191.55.1186917895.squirrel@secure.wincent.com> ``` "Wincent Colaiuta" 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 ``` ## Steffen Prohaska, 2007-08-12 11:33 Subject: Re: --exit-code (and --quiet) broken in git-diff? Message-ID: <7C233CA0-D6A4-42F2-B072-B04B1874D991@zib.de> URL: https://gitlist.dev/e/7C233CA0-D6A4-42F2-B072-B04B1874D991%40zib.de In-Reply-To: <46BED5AA.7050900@lsrfire.ath.cx> ``` 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 ``` ## Steven Grimm, 2007-08-12 13:02 Subject: Re: --exit-code (and --quiet) broken in git-diff? Message-ID: <46BF04F0.5020304@midwinter.com> URL: https://gitlist.dev/e/46BF04F0.5020304%40midwinter.com In-Reply-To: <85643lq99h.fsf@lola.goethe.zz> ``` 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 ``` ## David Kastrup, 2007-08-12 13:29 Subject: Re: --exit-code (and --quiet) broken in git-diff? Message-ID: <85bqdcq3sv.fsf@lola.goethe.zz> URL: https://gitlist.dev/e/85bqdcq3sv.fsf%40lola.goethe.zz In-Reply-To: <46BF04F0.5020304@midwinter.com> ``` Steven Grimm 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 ``` ## Wincent Colaiuta, 2007-08-12 16:57 Subject: Re: --exit-code (and --quiet) broken in git-diff? Message-ID: <8C80CB6C-83A5-437D-9360-AC93B73EAA54@wincent.com> URL: https://gitlist.dev/e/8C80CB6C-83A5-437D-9360-AC93B73EAA54%40wincent.com In-Reply-To: <46BF04F0.5020304@midwinter.com> ``` 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 () 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 ``` ## René Scharfe, 2007-08-12 17:46 Subject: [PATCH] diff: don't run pager if user asked for a diff style exit code Message-ID: <46BF478F.7030603@lsrfire.ath.cx> URL: https://gitlist.dev/e/46BF478F.7030603%40lsrfire.ath.cx In-Reply-To: <17875.88.10.191.55.1186873960.squirrel@secure.wincent.com> ``` 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 --- 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 }, ``` ## Wincent Colaiuta, 2007-08-13 09:57 Subject: Re: [PATCH] diff: don't run pager if user asked for a diff style exit code Message-ID: <13516449-DEDB-41A1-8880-42CB124A6C0A@wincent.com> URL: https://gitlist.dev/e/13516449-DEDB-41A1-8880-42CB124A6C0A%40wincent.com In-Reply-To: <46BF478F.7030603@lsrfire.ath.cx> ``` 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 ``` ## David Kastrup, 2007-08-13 10:23 Subject: Re: [PATCH] diff: don't run pager if user asked for a diff style exit code Message-ID: <86d4xrka23.fsf@lola.quinscape.zz> URL: https://gitlist.dev/e/86d4xrka23.fsf%40lola.quinscape.zz In-Reply-To: <13516449-DEDB-41A1-8880-42CB124A6C0A@wincent.com> ``` Wincent Colaiuta 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 ``` ## Junio C Hamano, 2007-08-13 23:42 Subject: Re: [PATCH] diff: don't run pager if user asked for a diff style exit code Message-ID: <7v7inzf1cm.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v7inzf1cm.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <46BF478F.7030603@lsrfire.ath.cx> ``` René Scharfe 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. ```