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

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

["Peter Valdemar Mørch (Lists)"]

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
http://www.morch.com

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

[Junio C Hamano]

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

> 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.

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?

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

["Peter Valdemar Mørch (Lists)"]

Junio C Hamano gitster-at-pobox.com |Lists| wrote:
>> 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.
> 
> 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?

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:
> Heh, I am about to push out fixed-up results, so it might save both of
> us some time if you looked at it first and then complained on my
> screwups.

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
-- 
Peter Valdemar Mørch
http://www.morch.com

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

[Junio C Hamano]

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(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index b1cb972..d6ca400 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -150,7 +150,9 @@ help ...`.
 
 -p::
 --paginate::
-	Pipe all output into 'less' (or if set, $PAGER).
+	Pipe all output into 'less' (or if set, $PAGER).  Note that this
+	implies that the exit code you see from the command will be that
+	of the pager, not git.
 
 --no-pager::
 	Do not pipe git output into a pager.

Re: Re* git diff/log --check exitcode and PAGER environment variable

["Peter Valdemar Mørch (Lists)"]

Junio C Hamano gitster-at-pobox.com |Lists| wrote:
>  --paginate::
> -	Pipe all output into 'less' (or if set, $PAGER).
> +	Pipe all output into 'less' (or if set, $PAGER).  Note that this
> +	implies that the exit code you see from the command will be that
> +	of the pager, not git.

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

-- 
Peter Valdemar Mørch
http://www.morch.com

Re: Re* git diff/log --check exitcode and PAGER environment variable

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1107 bytes --]

Hi,

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

> 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.

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

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

[Jeff King]

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

> "Peter Valdemar Mørch (Lists)"  <4ux6as402@sneakemail.com> writes:
> 
> > 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.
> 
> 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?

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

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

[Jeff King]

On Fri, Aug 08, 2008 at 09:17:59AM -0400, Jeff King 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).

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

-Peff

Re: Re* git diff/log --check exitcode and PAGER environment variable

[Junio C Hamano]

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

> On Fri, 8 Aug 2008, "Peter Valdemar Mørch (Lists)" wrote:
>
>> 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.
>
> 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

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).

[PATCH] Teach git log --check to return an appropriate error code

[Peter Valdemar Mørch]

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(-)

diff --git a/builtin-log.c b/builtin-log.c
index f4975cf..45ce8ea 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -227,6 +227,10 @@ static int cmd_log_walk(struct rev_info *rev)
 		free_commit_list(commit->parents);
 		commit->parents = NULL;
 	}
+	if (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&
+	    DIFF_OPT_TST(&rev->diffopt, CHECK_FAILED)) {
+		return 02;
+	}
 	return 0;
 }
 
-- 
1.6.0.rc2.6.gcd432.dirty

Re: [PATCH] Teach git log --check to return an appropriate error code

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 298 bytes --]

Hi,

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

> 	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.

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

Ciao,
Dscho

Re: [PATCH] Teach git log --check to return an appropriate error code

[Junio C Hamano]

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

> 	The return value is a bitfield, with |= 1 if HAS_CHANGES
> 	(clearly log has changes "always" - except e.g. "git log
> 	HEAD..HEAD")...

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)"'?

Re: [PATCH] Teach git log --check to return an appropriate error code

[Junio C Hamano]

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

> On Sat, 9 Aug 2008, Peter Valdemar Mørch wrote:
>
>> 	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.
>
> That is a side effect.  How wise is it to rely on that?

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.

Re: [PATCH] Teach git log --check to return an appropriate error code

["Peter Valdemar Mørch (Lists)"]

Junio C Hamano gitster-at-pobox.com |Lists| wrote:
> 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.

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.

> 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.

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
http://www.morch.com

Re: [PATCH] Teach git log --check to return an appropriate error code

[Junio C Hamano]

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

>> 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.
>
> 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 ..." ?

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

PATCH v2 0/2 Trying patch again

[Peter Valdemar Mørch]

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

[PATCH v2 1/2] Teach git log --check to return an appropriate exit code

[Peter Valdemar Mørch]

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(-)

diff --git a/builtin-log.c b/builtin-log.c
index f4975cf..ae71540 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -217,6 +217,11 @@ static int cmd_log_walk(struct rev_info *rev)
 	if (rev->early_output)
 		finish_early_output(rev);
 
+	/*
+	 * 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
+	 */
 	while ((commit = get_revision(rev)) != NULL) {
 		log_tree_commit(rev, commit);
 		if (!rev->reflog_info) {
@@ -227,6 +232,10 @@ static int cmd_log_walk(struct rev_info *rev)
 		free_commit_list(commit->parents);
 		commit->parents = NULL;
 	}
+	if (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&
+	    DIFF_OPT_TST(&rev->diffopt, CHECK_FAILED)) {
+		return 02;
+	}
 	return 0;
 }
 
-- 
1.6.0.rc2.5.g3452.dirty

[PATCH v2 2/2] Teach git log --exit-code to return an appropriate exit code

[Peter Valdemar Mørch]

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(-)

diff --git a/builtin-log.c b/builtin-log.c
index ae71540..3a79574 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -218,9 +218,9 @@ static int cmd_log_walk(struct rev_info *rev)
 		finish_early_output(rev);
 
 	/*
-	 * 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
+	 * For --check and --exit-code, the exit code is based on CHECK_FAILED
+	 * and HAS_CHANGES being accumulated in rev->diffopt, so be careful to
+	 * retain that state information if replacing rev->diffopt in this loop
 	 */
 	while ((commit = get_revision(rev)) != NULL) {
 		log_tree_commit(rev, commit);
@@ -236,7 +236,7 @@ static int cmd_log_walk(struct rev_info *rev)
 	    DIFF_OPT_TST(&rev->diffopt, CHECK_FAILED)) {
 		return 02;
 	}
-	return 0;
+	return diff_result_code(&rev->diffopt, 0);
 }
 
 static int git_log_config(const char *var, const char *value, void *cb)
diff --git a/log-tree.c b/log-tree.c
index bd8b9e4..30cd5bb 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -432,7 +432,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log
 	struct commit_list *parents;
 	unsigned const char *sha1 = commit->object.sha1;
 
-	if (!opt->diff)
+	if (!opt->diff && !DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS))
 		return 0;
 
 	/* Root commit? */
-- 
1.6.0.rc2.5.g3452.dirty