git diff/log --check exitcode and PAGER environment variable

Using my default PAGER=less, git log --check exits with exit code 0, contrary to documentation.

There is this old thread: "[PATCH 1/5] "diff --check" should affect exit status" http://thread.gmane.org/gmane.comp.version-control.git/68145/focus=68148 which seemed not to reach a conclusion.

For git log, I still have not been able to make it exit with anything other than 0 - contrary to documentation.

May I propose a change to either documentation or behavior of "git diff --check". The current one has:

--check::
	Warn if changes introduce trailing whitespace
	or an indent that uses a space before a tab. Exits with
	non-zero status if problems are found. Not compatible with
	--exit-code.

This, clearly, is not correct:

$ PAGER=less git diff --check (my default PAGER) or $ unset PAGER ; git diff --check always exits with exit code 0. But

$ git --no-pager diff --check or $ PAGER=cat git diff --check or $ PAGER= git diff --check exits with exit code 2 on error (curiously PAGER= and unset PAGER give different results)

But the --exit-code overrides any of that:

$ git --no-pager diff --check --exit-code exits with exit code 3 on error (with or without the --no-pager).

I'm not sure about a good rephrasing. How about: '... "git diff" exits with non-zero status if problems are found and run with --exit-code.'

While this documentation string is found in diff-options.txt and included in:

git-diff-files.txt git-diff-index.txt git-diff-tree.txt git-diff.txt git-format-patch.txt git-log.txt

At least for the git-log cases, the behavior is not the same as for git-diff:

$ PAGER=cat git --no-pager log HEAD~20..HEAD --check --exit-code $ echo $? 0 Though there are several check failures (red squares in output), it exits with 0, even when using all the tricks that work with "git diff".

Clearly here, the documentation is "even more wrong". Hence the explicit mention of "git diff" in the help string for the --check option.

What do you think?

Peter

"Peter Valdemar Mørch (Lists)"  <4ux6as402@sneakemail.com> writes:

Conclusion was (1) if you really care about the exit code, do not use pager; (2) after 1.6.0 we will swap the child/parent between git and pager to expose exit code from us, but not before.

Or am I mistaken?

Junio C Hamano gitster-at-pobox.com |Lists| wrote:

Perhaps a more correct statement on my part would have been that I couldn't find the conclusion. :-)

It ended with Junio C Hamano saying:

I wasn't subscribed to the list back then and couldn't follow beyond that thread in GMane.

Regardless of what happened or not back then, the current documentation does not match the current code. Not for git-diff, and certainly not for git-log.

Or am I mistaken?

I didn't see a reference in that thread to post 1.6.0 changes or to child/parent relationships, but if this is known and planned for post-1.6.0, then cool: I'll get on with my life and let you get on with yours!

Peter

As this is not limited to diff command at all, let's do this instead.

-- >8 -- Document use of pager means you will see exit code from the pager

Whenever we run pager (either a subcommand that implies use of pager by default, or by explicit request with "git -p cmd"), the main git process becomes the upstream of the pipe that feed the pager, and the exit code from the command as a whole comes from the pager. Long time users may have already got used to this without being documented, but it should be documented.

We may be swapping the process ordering in the future so that the exit code from the main git process is always exposed, and at that point this comment should be removed.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git.txt |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

Junio C Hamano gitster-at-pobox.com |Lists| wrote:

Thank you for the attention, Junio.

I don't want to be a troll... But in my original post, I write that git log exits with 0 even when there are --check failures *and* --no-pager is used.

$ PAGER=cat git --no-pager log HEAD~20..HEAD --check --exit-code $ echo $? 0

Here I don't think the pager is involved, and so perhaps this is an unrelated issue.

Since the pager/exit code issue is going to be looked at post 1.6.0 herhaps this is low-priority: Nowhere in man git-diff does it mention the pager or less or that git-diff by default behaves as if -p/--paginate from "man git" had been given. I personally would not have thought to look there or caught the connection. But perhaps I'm bikeshedding.

If I'm percieved as trolling: Please let me know. This documentation string took time out of my day. (Less, though than this thread has! :D)

Peter

Hi,

On Fri, 8 Aug 2008, "Peter Valdemar Mørch (Lists)" wrote:

You seem to care enough. That is good. Because I will give you a few pointers to help yourself, and you can in return help us by submitting a patch:

- the code to be changed lives in log-tree.c.  Look for calls to the 
  function log_tree_diff_flush().  You need to check the exit status
  after that (needs to be done only when DIFF_OPT_TST(opt->diffopt, 
  EXIT_WITH_STATUS).

- you can get at the exit status with the call 
  diff_result_code(opt->diffopt, 0) (see the implementation in diff.c to 
  find out what the 0 means, and why it is correct).

- you need to accumulate the exit status (plural, with a long u) over all 
  calls to log_tree_diff(), best thing would be to add a member to the
  log_info struct.

- you need to test rev->loginfo->exit_code in the end, and return failure 
  if it is non-zero.  I think the place is in cmd_log_walk().

Bon chance, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Dscho, thanks for a nice writeup.

And sorry, Peter, for being dense earlier.

I somehow thought you were talking about "diff" but you are right; "log" has been solely used for "_view_ log with various format of diffs" and nobody wanted it to pay attention to individual diff's exit status so far (I am not saying "everybody wanted it not to pay attention to it" -- it was just nobody felt the need for log to report the diff exit status).

From: Peter Valdemar Mørch <peter@morch.com>

Signed-off-by: Peter Valdemar Mørch <peter@morch.com>
---

	Ok. I take on the callenge. Thanks for a very helpful writeup!
	The patch in the end is very short. And since it doesn't
	follow your writeup, let me explain my rationale:
	
	Whether or not a check fails is stored in the
	DIFF_OPT_CHECK_FAILED field of flags in struct diff_options.
	This flag-field is only set (diff.c:1644), never cleared.
	Since the same diff_options is used throughout, it is enough
	to check that field at the end - it already does the
	accumulation because it never gets cleared.
	
	diff_result_code: The second argument to it is never used
	since (opt->output_format & DIFF_FORMAT_CHECKDIFF), so the
	value doesn't matter (0 would have been fine as you suggest).
	The return value is a bitfield, with |= 1 if HAS_CHANGES
	(clearly log has changes "always" - except e.g. "git log
	HEAD..HEAD") and |= 2 if CHECK_FAILED.
	
	Therefore I was left with either:
	
	* Return the value of diff_result_code ("always" |=1,
	sometimes |=2 if a check failed. This would put the burden
	on the caller to check different values of $?.
	
	* Return value of (diff_result_code & 02). Then I would
	suggest adding the constant 02 to a header file.
	
	* Pick out the logic from diff_result_code with respect to
	CHECK_FAILED. I chose this path. (I return 02 here too, and
	perhaps that *should* go in a header file. I decided not
	to.)

 builtin-log.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

Hi,

On Sat, 9 Aug 2008, Peter Valdemar Mørch wrote:

That is a side effect.  How wise is it to rely on that?

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Hmm, good point.

The bit will never be cleared during a single diff run by design, because it needs to be cumulative in order to check a patch that describes changes to multiple paths --- iow, the API sequence is (1) the caller to the diff machinery resets the bit to zero and then (2) the caller exercises the diff machinery and expects the machinery to set the bit if even a single failure is detected, or leaves it unset if there is none.

So unless you (log_tree_diff(), the caller of diff machinery), decide to explicitly reset the bit (or decide to use a freshly allocated and initialized diff_options for each commit it feeds diff_tree_sha1()), the assumption would hold. We need to see how plausible it would be for us to break that assumption in the future.

Future versions of log_tree_diff() may want to tweak opt->diffopt per commit, when we have options for "use larger -U<lines> value after hitting this commit", or "use this pathspec to limit the diff output after hitting this commit", for example. But even in these cases, I think it is implausible to start from a freshly initialized diff_options structure. The code most likely would start from the copy of what was in use and update only the necessary fields, without disturbing the state variables.

So I think you are worried a bit too much in this case, even though it is a valid concern in principle. It might warrant a comment somewhere inside log_tree_diff() to tell people not to re-initialize opt->diffopt per commit without thinking, though.

One interesting option that might be interesting to add to the log family would be to show only commits that fail the checkdiff tests. I suspect necessary change for doing so would go to log_tree_diff() codepath.

Junio C Hamano gitster-at-pobox.com |Lists| wrote:

Hmm... I've looked at the code... The while loop that iterates through the revisions is in cmd_log_walk(), which calls log_tree_commit(), which in turn calls log_tree_diff().

I'm thinking that cmd_log_walk() is where one "would want" to change rev->diffopt / opt->diffopt in the future, and hence I suggest to put the comment there - given my limited understanding of connecting tissue. Something like:

/* For --check, the exit code is based on CHECK_FAILED
    being accumulated in rev->diffopt, so be careful to retain
    that state information if replacing rev->diffopt in this
    loop */

That would also be 10-15 lines above the patch I posted earlier, so the connection with retrieving the error code would be visible 15 lines below.

Would such a comment in that place constiture and acceptable patch? I've tried to follow Dscho's write up and contribute a patch, even though git-log's exit code was never my itch to begin with, because I'm exited to contribute.

I'm hoping that this is meant as "aside from this current patch, one interesting option..." or do you mean "in order for this patch to be accepted, I suggest this to be added ..." ?

This is growing. I originally suggested a patch to documentation to make it match the code, but took on Dscho's invitation to contribute a code patch instead. But given that this patch, although working, still isn't good enough and the new proposals : the new option above and --exit-code proposal elsewhere in this thread, I'm getting a little discouraged. I'm not saying you meant it that way.

Peter

"Peter Valdemar Mørch (Lists)"  <4ux6as402@sneakemail.com> writes:

Sorry, not the latter.  I'll try to be clear in the future.

Peter Valdemar Mørch  <4ux6as402@sneakemail.com> writes:

Is it clear? "git log HEAD~20..HEAD -- path" where path never changes within the range would be !HAS_CHANGES, wouldn't it?

Perhaps "git log --exit-code --raw A..B -- path" should give the same exit status as '! test -z "$(git rev-list A..B -- path)"'?

Trying again after adding comment as described in thread and implementing --exit-code for git-log

From: Peter Valdemar Mørch <peter@morch.com>

Signed-off-by: Peter Valdemar Mørch <peter@morch.com>
---
 builtin-log.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

From: Peter Valdemar Mørch <peter@morch.com>

Signed-off-by: Peter Valdemar Mørch <peter@morch.com>
---
 builtin-log.c |    8 ++++----
 log-tree.c    |    2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

On Fri, Aug 08, 2008 at 02:44:37AM -0700, Junio C Hamano wrote:

Yes, all of his testing with "git diff" is hampered by the pager hiding the exit code. And that is dealt with by the patches in next (and I tested his examples with 'next', and they work fine).

But that still leaves the part about "git log" not changing its exit code. I don't think it has ever been designed to, and I'm not even sure what the semantics would be (exit code != 0 if any logged commit has a whitespace problem? That seems the most logical, and it might be useful for limited ranges).

-Peff

On Fri, Aug 08, 2008 at 09:17:59AM -0400, Jeff King wrote:

...and then after writing this I realized that all of this was dealt with later in the thread. Sorry for the noise.

-Peff