{"thread":{"id":"14887","subject":"git diff/log --check exitcode and PAGER environment variable","startedAt":"2008-08-08T09:39:39Z","lastAt":"2008-08-11T06:46:25Z","messageCount":18,"participants":["Peter Valdemar Mørch (Lists)","Junio C Hamano","Johannes Schindelin","Jeff King","Peter Valdemar Mørch"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"86475","messageId":"489C145B.5090400@sneakemail.com","threadId":"14887","inReplyTo":null,"subject":"git diff/log --check exitcode and PAGER environment variable","fromName":"Peter Valdemar Mørch (Lists)","fromEmail":"4ux6as402@sneakemail.com","sentAt":"2008-08-08T09:39:39Z","receivedAt":"2008-08-08T09:39:39Z","isPatch":false,"sender":{"key":"4ux6as402@sneakemail.com","avatar":null},"body":"Using my default PAGER=less, git log --check exits with exit code 0, \ncontrary to documentation.\n\nThere is this old thread:\n\"[PATCH 1/5] \"diff --check\" should affect exit status\"\nhttp://thread.gmane.org/gmane.comp.version-control.git/68145/focus=68148\nwhich seemed not to reach a conclusion.\n\nFor git log, I still have not been able to make it exit with anything \nother than 0 - contrary to documentation.\n\nMay I propose a change to either documentation or behavior of \"git diff \n--check\". The current one has:\n\n--check::\n\tWarn if changes introduce trailing whitespace\n\tor an indent that uses a space before a tab. Exits with\n\tnon-zero status if problems are found. Not compatible with\n\t--exit-code.\n\nThis, clearly, is not correct:\n\n$ PAGER=less git diff --check\n(my default PAGER)\nor\n$ unset PAGER ; git diff --check\nalways exits with exit code 0. But\n\n$ git --no-pager diff --check\nor\n$ PAGER=cat git diff --check\nor\n$ PAGER= git diff --check\nexits with exit code 2 on error\n(curiously PAGER= and unset PAGER give different results)\n\nBut the --exit-code overrides any of that:\n\n$ git --no-pager diff --check --exit-code\nexits with exit code 3 on error (with or without the --no-pager).\n\nI'm not sure about a good rephrasing. How about:\n'... \"git diff\" exits with non-zero status if problems are found and run \nwith --exit-code.'\n\nWhile this documentation string is found in diff-options.txt and \nincluded in:\n\ngit-diff-files.txt\ngit-diff-index.txt\ngit-diff-tree.txt\ngit-diff.txt\ngit-format-patch.txt\ngit-log.txt\n\nAt least for the git-log cases, the behavior is not the same as for \ngit-diff:\n\n$ PAGER=cat git --no-pager log HEAD~20..HEAD --check --exit-code\n$ echo $?\n0\nThough there are several check failures (red squares in output), it \nexits with 0, even when using all the tricks that work with \"git diff\".\n\nClearly here, the documentation is \"even more wrong\". Hence the explicit \nmention of \"git diff\" in the help string for the --check option.\n\nWhat do you think?\n\nPeter\n-- \nPeter Valdemar Mørch\nhttp://www.morch.com\n"},{"id":"86477","messageId":"7vfxpfet8a.fsf@gitster.siamese.dyndns.org","threadId":"14887","inReplyTo":"489C145B.5090400@sneakemail.com","subject":"Re: git diff/log --check exitcode and PAGER environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-08T09:44:37Z","receivedAt":"2008-08-08T09:44:37Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Peter Valdemar Mørch (Lists)\"  <4ux6as402@sneakemail.com> writes:\n\n> There is this old thread:\n> \"[PATCH 1/5] \"diff --check\" should affect exit status\"\n> http://thread.gmane.org/gmane.comp.version-control.git/68145/focus=68148\n> which seemed not to reach a conclusion.\n\nConclusion was (1) if you really care about the exit code, do not use\npager; (2) after 1.6.0 we will swap the child/parent between git and pager\nto expose exit code from us, but not before.\n\nOr am I mistaken?\n"},{"id":"86478","messageId":"489C1A40.9000001@sneakemail.com","threadId":"14887","inReplyTo":"7vfxpfet8a.fsf@gitster.siamese.dyndns.org","subject":"Re: git diff/log --check exitcode and PAGER environment variable","fromName":"Peter Valdemar Mørch (Lists)","fromEmail":"4ux6as402@sneakemail.com","sentAt":"2008-08-08T10:04:48Z","receivedAt":"2008-08-08T10:04:48Z","isPatch":false,"sender":{"key":"4ux6as402@sneakemail.com","avatar":null},"body":"Junio C Hamano gitster-at-pobox.com |Lists| wrote:\n>> There is this old thread:\n>> \"[PATCH 1/5] \"diff --check\" should affect exit status\"\n>> http://thread.gmane.org/gmane.comp.version-control.git/68145/focus=68148\n>> which seemed not to reach a conclusion.\n> \n> Conclusion was (1) if you really care about the exit code, do not use\n> pager; (2) after 1.6.0 we will swap the child/parent between git and pager\n> to expose exit code from us, but not before.\n> \n> Or am I mistaken?\n\nPerhaps a more correct statement on my part would have been that I \ncouldn't find the conclusion. :-)\n\nIt ended with Junio C Hamano saying:\n> Heh, I am about to push out fixed-up results, so it might save both of\n> us some time if you looked at it first and then complained on my\n> screwups.\n\nI wasn't subscribed to the list back then and couldn't follow beyond \nthat thread in GMane.\n\nRegardless of what happened or not back then, the current documentation \ndoes not match the current code. Not for git-diff, and certainly not for \ngit-log.\n\nOr am I mistaken?\n\nI didn't see a reference in that thread to post 1.6.0 changes or to \nchild/parent relationships, but if this is known and planned for \npost-1.6.0, then cool: I'll get on with my life and let you get on with \nyours!\n\nPeter\n-- \nPeter Valdemar Mørch\nhttp://www.morch.com\n"},{"id":"86479","messageId":"7v1w0zersg.fsf_-_@gitster.siamese.dyndns.org","threadId":"14887","inReplyTo":"7vfxpfet8a.fsf@gitster.siamese.dyndns.org","subject":"Re* git diff/log --check exitcode and PAGER environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-08T10:15:43Z","receivedAt":"2008-08-08T10:15:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"As this is not limited to diff command at all, let's do this instead.\n\n-- >8 --\nDocument use of pager means you will see exit code from the pager\n\nWhenever we run pager (either a subcommand that implies use of pager by\ndefault, or by explicit request with \"git -p cmd\"), the main git process\nbecomes the upstream of the pipe that feed the pager, and the exit code\nfrom the command as a whole comes from the pager.  Long time users may\nhave already got used to this without being documented, but it should be\ndocumented.\n\nWe may be swapping the process ordering in the future so that the exit\ncode from the main git process is always exposed, and at that point this\ncomment should be removed.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/git.txt |    4 +++-\n 1 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex b1cb972..d6ca400 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -150,7 +150,9 @@ help ...`.\n \n -p::\n --paginate::\n-\tPipe all output into 'less' (or if set, $PAGER).\n+\tPipe all output into 'less' (or if set, $PAGER).  Note that this\n+\timplies that the exit code you see from the command will be that\n+\tof the pager, not git.\n \n --no-pager::\n \tDo not pipe git output into a pager.\n"},{"id":"86486","messageId":"489C27DD.90603@sneakemail.com","threadId":"14887","inReplyTo":"7v1w0zersg.fsf_-_@gitster.siamese.dyndns.org","subject":"Re: Re* git diff/log --check exitcode and PAGER environment variable","fromName":"Peter Valdemar Mørch (Lists)","fromEmail":"4ux6as402@sneakemail.com","sentAt":"2008-08-08T11:02:53Z","receivedAt":"2008-08-08T11:02:53Z","isPatch":false,"sender":{"key":"4ux6as402@sneakemail.com","avatar":null},"body":"Junio C Hamano gitster-at-pobox.com |Lists| wrote:\n>  --paginate::\n> -\tPipe all output into 'less' (or if set, $PAGER).\n> +\tPipe all output into 'less' (or if set, $PAGER).  Note that this\n> +\timplies that the exit code you see from the command will be that\n> +\tof the pager, not git.\n\nThank you for the attention, Junio.\n\nI don't want to be a troll... But in my original post, I write that git\nlog exits with 0 even when there are --check failures *and* --no-pager\nis used.\n\n$ PAGER=cat git --no-pager log HEAD~20..HEAD --check --exit-code\n$ echo $?\n0\n\nHere I don't think the pager is involved, and so perhaps this is an\nunrelated issue.\n\nSince the pager/exit code issue is going to be looked at post 1.6.0\nherhaps this is low-priority: Nowhere in man git-diff does it mention\nthe pager or less or that git-diff by default behaves as if\n-p/--paginate from \"man git\" had been given. I personally would not have\nthought to look there or caught the connection. But perhaps I'm\nbikeshedding.\n\nIf I'm percieved as trolling: Please let me know. This documentation\nstring took time out of my day. (Less, though than this thread has! :D)\n\nPeter\n\n-- \nPeter Valdemar Mørch\nhttp://www.morch.com\n"},{"id":"86488","messageId":"alpine.DEB.1.00.0808081315060.9611@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"14887","inReplyTo":"489C27DD.90603@sneakemail.com","subject":"Re: Re* git diff/log --check exitcode and PAGER environment variable","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-08-08T11:23:03Z","receivedAt":"2008-08-08T11:23:03Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 8 Aug 2008, \"Peter Valdemar Mørch (Lists)\" wrote:\n\n> I don't want to be a troll... But in my original post, I write that git \n> log exits with 0 even when there are --check failures *and* --no-pager \n> is used.\n\nYou seem to care enough.  That is good.  Because I will give you a few \npointers to help yourself, and you can in return help us by submitting a \npatch:\n\n- the code to be changed lives in log-tree.c.  Look for calls to the \n  function log_tree_diff_flush().  You need to check the exit status\n  after that (needs to be done only when DIFF_OPT_TST(opt->diffopt, \n  EXIT_WITH_STATUS).\n\n- you can get at the exit status with the call \n  diff_result_code(opt->diffopt, 0) (see the implementation in diff.c to \n  find out what the 0 means, and why it is correct).\n\n- you need to accumulate the exit status (plural, with a long u) over all \n  calls to log_tree_diff(), best thing would be to add a member to the\n  log_info struct.\n\n- you need to test rev->loginfo->exit_code in the end, and return failure \n  if it is non-zero.  I think the place is in cmd_log_walk().\n\nBon chance,\nDscho\n"},{"id":"86494","messageId":"20080808131759.GA19705@sigill.intra.peff.net","threadId":"14887","inReplyTo":"7vfxpfet8a.fsf@gitster.siamese.dyndns.org","subject":"Re: git diff/log --check exitcode and PAGER environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-08-08T13:17:59Z","receivedAt":"2008-08-08T13:17:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 08, 2008 at 02:44:37AM -0700, Junio C Hamano wrote:\n\n> \"Peter Valdemar Mørch (Lists)\"  <4ux6as402@sneakemail.com> writes:\n> \n> > There is this old thread:\n> > \"[PATCH 1/5] \"diff --check\" should affect exit status\"\n> > http://thread.gmane.org/gmane.comp.version-control.git/68145/focus=68148\n> > which seemed not to reach a conclusion.\n> \n> Conclusion was (1) if you really care about the exit code, do not use\n> pager; (2) after 1.6.0 we will swap the child/parent between git and pager\n> to expose exit code from us, but not before.\n> \n> Or am I mistaken?\n\nYes, all of his testing with \"git diff\" is hampered by the pager hiding\nthe exit code. And that is dealt with by the patches in next (and I\ntested his examples with 'next', and they work fine).\n\nBut that still leaves the part about \"git log\" not changing its exit\ncode. I don't think it has ever been designed to, and I'm not even sure\nwhat the semantics would be (exit code != 0 if any logged commit has a\nwhitespace problem? That seems the most logical, and it might be useful\nfor limited ranges).\n\n-Peff\n"},{"id":"86495","messageId":"20080808131933.GB19705@sigill.intra.peff.net","threadId":"14887","inReplyTo":"20080808131759.GA19705@sigill.intra.peff.net","subject":"Re: git diff/log --check exitcode and PAGER environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-08-08T13:19:33Z","receivedAt":"2008-08-08T13:19:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 08, 2008 at 09:17:59AM -0400, Jeff King wrote:\n\n> Yes, all of his testing with \"git diff\" is hampered by the pager hiding\n> the exit code. And that is dealt with by the patches in next (and I\n> tested his examples with 'next', and they work fine).\n> \n> But that still leaves the part about \"git log\" not changing its exit\n> code. I don't think it has ever been designed to, and I'm not even sure\n> what the semantics would be (exit code != 0 if any logged commit has a\n> whitespace problem? That seems the most logical, and it might be useful\n> for limited ranges).\n\n...and then after writing this I realized that all of this was dealt\nwith later in the thread. Sorry for the noise.\n\n-Peff\n"},{"id":"86542","messageId":"7vsktfb5r1.fsf@gitster.siamese.dyndns.org","threadId":"14887","inReplyTo":"alpine.DEB.1.00.0808081315060.9611@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: Re* git diff/log --check exitcode and PAGER environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-08T20:40:02Z","receivedAt":"2008-08-08T20:40:02Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Fri, 8 Aug 2008, \"Peter Valdemar Mørch (Lists)\" wrote:\n>\n>> I don't want to be a troll... But in my original post, I write that git \n>> log exits with 0 even when there are --check failures *and* --no-pager \n>> is used.\n>\n> You seem to care enough.  That is good.  Because I will give you a few \n> pointers to help yourself, and you can in return help us by submitting a \n> patch:\n>\n> - the code to be changed lives in log-tree.c.  Look for calls to the \n>   function log_tree_diff_flush().  You need to check the exit status\n>   after that (needs to be done only when DIFF_OPT_TST(opt->diffopt, \n>   EXIT_WITH_STATUS).\n>\n> - you can get at the exit status with the call \n>   diff_result_code(opt->diffopt, 0) (see the implementation in diff.c to \n>   find out what the 0 means, and why it is correct).\n>\n> - you need to accumulate the exit status (plural, with a long u) over all \n>   calls to log_tree_diff(), best thing would be to add a member to the\n>   log_info struct.\n>\n> - you need to test rev->loginfo->exit_code in the end, and return failure \n>   if it is non-zero.  I think the place is in cmd_log_walk().\n>\n> Bon chance,\n> Dscho\n\nDscho, thanks for a nice writeup.\n\nAnd sorry, Peter, for being dense earlier.\n\nI somehow thought you were talking about \"diff\" but you are right; \"log\"\nhas been solely used for \"_view_ log with various format of diffs\" and\nnobody wanted it to pay attention to individual diff's exit status so far\n(I am not saying \"everybody wanted it not to pay attention to it\" -- it\nwas just nobody felt the need for log to report the diff exit status).\n"},{"id":"86576","messageId":"1218265054-19220-1-git-send-email-4ux6as402@sneakemail.com","threadId":"14887","inReplyTo":"alpine.DEB.1.00.0808081315060.9611@pacific.mpi-cbg.de.mpi-cbg.de","subject":"[PATCH] Teach git log --check to return an appropriate error code","fromName":"Peter Valdemar Mørch","fromEmail":"4ux6as402@sneakemail.com","sentAt":"2008-08-09T06:57:34Z","receivedAt":"2008-08-09T06:57:34Z","isPatch":true,"sender":{"key":"4ux6as402@sneakemail.com","avatar":null},"body":"From: Peter Valdemar Mørch <peter@morch.com>\n\n\nSigned-off-by: Peter Valdemar Mørch <peter@morch.com>\n---\n\n\tOk. I take on the callenge. Thanks for a very helpful writeup!\n\tThe patch in the end is very short. And since it doesn't\n\tfollow your writeup, let me explain my rationale:\n\t\n\tWhether or not a check fails is stored in the\n\tDIFF_OPT_CHECK_FAILED field of flags in struct diff_options.\n\tThis flag-field is only set (diff.c:1644), never cleared.\n\tSince the same diff_options is used throughout, it is enough\n\tto check that field at the end - it already does the\n\taccumulation because it never gets cleared.\n\t\n\tdiff_result_code: The second argument to it is never used\n\tsince (opt->output_format & DIFF_FORMAT_CHECKDIFF), so the\n\tvalue doesn't matter (0 would have been fine as you suggest).\n\tThe return value is a bitfield, with |= 1 if HAS_CHANGES\n\t(clearly log has changes \"always\" - except e.g. \"git log\n\tHEAD..HEAD\") and |= 2 if CHECK_FAILED.\n\t\n\tTherefore I was left with either:\n\t\n\t* Return the value of diff_result_code (\"always\" |=1,\n\tsometimes |=2 if a check failed. This would put the burden\n\ton the caller to check different values of $?.\n\t\n\t* Return value of (diff_result_code & 02). Then I would\n\tsuggest adding the constant 02 to a header file.\n\t\n\t* Pick out the logic from diff_result_code with respect to\n\tCHECK_FAILED. I chose this path. (I return 02 here too, and\n\tperhaps that *should* go in a header file. I decided not\n\tto.)\n\n\n builtin-log.c |    4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex f4975cf..45ce8ea 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -227,6 +227,10 @@ static int cmd_log_walk(struct rev_info *rev)\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\n \t}\n+\tif (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&\n+\t    DIFF_OPT_TST(&rev->diffopt, CHECK_FAILED)) {\n+\t\treturn 02;\n+\t}\n \treturn 0;\n }\n \n-- \n1.6.0.rc2.6.gcd432.dirty\n"},{"id":"86598","messageId":"alpine.DEB.1.00.0808091404230.24820@pacific.mpi-cbg.de.mpi-cbg.de","threadId":"14887","inReplyTo":"1218265054-19220-1-git-send-email-4ux6as402@sneakemail.com","subject":"Re: [PATCH] Teach git log --check to return an appropriate error code","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-08-09T12:05:01Z","receivedAt":"2008-08-09T12:05:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sat, 9 Aug 2008, Peter Valdemar Mørch wrote:\n\n> \tWhether or not a check fails is stored in the\n> \tDIFF_OPT_CHECK_FAILED field of flags in struct diff_options.\n> \tThis flag-field is only set (diff.c:1644), never cleared.\n\nThat is a side effect.  How wise is it to rely on that?\n\nCiao,\nDscho"},{"id":"86606","messageId":"7vljz66mmt.fsf@gitster.siamese.dyndns.org","threadId":"14887","inReplyTo":"1218265054-19220-1-git-send-email-4ux6as402@sneakemail.com","subject":"Re: [PATCH] Teach git log --check to return an appropriate error code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-09T18:58:50Z","receivedAt":"2008-08-09T18:58:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Peter Valdemar Mørch  <4ux6as402@sneakemail.com> writes:\n\n> \tThe return value is a bitfield, with |= 1 if HAS_CHANGES\n> \t(clearly log has changes \"always\" - except e.g. \"git log\n> \tHEAD..HEAD\")...\n\nIs it clear?  \"git log HEAD~20..HEAD -- path\" where path never changes\nwithin the range would be !HAS_CHANGES, wouldn't it?\n\nPerhaps \"git log --exit-code --raw A..B -- path\" should give the same exit\nstatus as '! test -z \"$(git rev-list A..B -- path)\"'?\n"},{"id":"86608","messageId":"7v8wv66l8d.fsf@gitster.siamese.dyndns.org","threadId":"14887","inReplyTo":"alpine.DEB.1.00.0808091404230.24820@pacific.mpi-cbg.de.mpi-cbg.de","subject":"Re: [PATCH] Teach git log --check to return an appropriate error code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-09T19:29:06Z","receivedAt":"2008-08-09T19:29:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Sat, 9 Aug 2008, Peter Valdemar Mørch wrote:\n>\n>> \tWhether or not a check fails is stored in the\n>> \tDIFF_OPT_CHECK_FAILED field of flags in struct diff_options.\n>> \tThis flag-field is only set (diff.c:1644), never cleared.\n>\n> That is a side effect.  How wise is it to rely on that?\n\nHmm, good point.\n\nThe bit will never be cleared during a single diff run by design, because\nit needs to be cumulative in order to check a patch that describes changes\nto multiple paths --- iow, the API sequence is (1) the caller to the diff\nmachinery resets the bit to zero and then (2) the caller exercises the\ndiff machinery and expects the machinery to set the bit if even a single\nfailure is detected, or leaves it unset if there is none.\n\nSo unless you (log_tree_diff(), the caller of diff machinery), decide to\nexplicitly reset the bit (or decide to use a freshly allocated and\ninitialized diff_options for each commit it feeds diff_tree_sha1()), the\nassumption would hold.  We need to see how plausible it would be for us to\nbreak that assumption in the future.\n\nFuture versions of log_tree_diff() may want to tweak opt->diffopt per\ncommit, when we have options for \"use larger -U<lines> value after hitting\nthis commit\", or \"use this pathspec to limit the diff output after hitting\nthis commit\", for example.  But even in these cases, I think it is\nimplausible to start from a freshly initialized diff_options structure.\nThe code most likely would start from the copy of what was in use and\nupdate only the necessary fields, without disturbing the state variables.\n\nSo I think you are worried a bit too much in this case, even though it is\na valid concern in principle.  It might warrant a comment somewhere inside\nlog_tree_diff() to tell people not to re-initialize opt->diffopt per\ncommit without thinking, though.\n\nOne interesting option that might be interesting to add to the log family\nwould be to show only commits that fail the checkdiff tests.  I suspect\nnecessary change for doing so would go to log_tree_diff() codepath.\n"},{"id":"86690","messageId":"489F1FE4.6090400@sneakemail.com","threadId":"14887","inReplyTo":"7v8wv66l8d.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] Teach git log --check to return an appropriate error code","fromName":"Peter Valdemar Mørch (Lists)","fromEmail":"4ux6as402@sneakemail.com","sentAt":"2008-08-10T17:05:40Z","receivedAt":"2008-08-10T17:05:40Z","isPatch":true,"sender":{"key":"4ux6as402@sneakemail.com","avatar":null},"body":"Junio C Hamano gitster-at-pobox.com |Lists| wrote:\n> Future versions of log_tree_diff() may want to tweak opt->diffopt per\n> commit, when we have options for \"use larger -U<lines> value after hitting\n> this commit\", or \"use this pathspec to limit the diff output after hitting\n> this commit\", for example.  But even in these cases, I think it is\n> implausible to start from a freshly initialized diff_options structure.\n> The code most likely would start from the copy of what was in use and\n> update only the necessary fields, without disturbing the state variables.\n> \n> So I think you are worried a bit too much in this case, even though it is\n> a valid concern in principle.  It might warrant a comment somewhere inside\n> log_tree_diff() to tell people not to re-initialize opt->diffopt per\n> commit without thinking, though.\n\nHmm... I've looked at the code... The while loop that iterates through \nthe revisions is in cmd_log_walk(), which calls log_tree_commit(), which \nin turn calls log_tree_diff().\n\nI'm thinking that cmd_log_walk() is where one \"would want\" to change \nrev->diffopt / opt->diffopt in the future, and hence I suggest to put \nthe comment there - given my limited understanding of connecting tissue. \nSomething like:\n\n/* For --check, the exit code is based on CHECK_FAILED\n    being accumulated in rev->diffopt, so be careful to retain\n    that state information if replacing rev->diffopt in this\n    loop */\n\nThat would also be 10-15 lines above the patch I posted earlier, so the \nconnection with retrieving the error code would be visible 15 lines below.\n\nWould such a comment in that place constiture and acceptable patch? I've \ntried to follow Dscho's write up and contribute a patch, even though \ngit-log's exit code was never my itch to begin with, because I'm exited \nto contribute.\n\n> One interesting option that might be interesting to add to the log family\n> would be to show only commits that fail the checkdiff tests.  I suspect\n> necessary change for doing so would go to log_tree_diff() codepath.\n\nI'm hoping that this is meant as \"aside from this current patch, one \ninteresting option...\" or do you mean \"in order for this patch to be \naccepted, I suggest this to be added ...\" ?\n\nThis is growing. I originally suggested a patch to documentation to make \nit match the code, but took on Dscho's invitation to contribute a code \npatch instead. But given that this patch, although working, still isn't \ngood enough and the new proposals : the new option above and --exit-code \nproposal elsewhere in this thread, I'm getting a little discouraged. I'm \nnot saying you meant it that way.\n\nPeter\n-- \nPeter Valdemar Mørch\nhttp://www.morch.com\n"},{"id":"86703","messageId":"7vk5eo3e99.fsf@gitster.siamese.dyndns.org","threadId":"14887","inReplyTo":"489F1FE4.6090400@sneakemail.com","subject":"Re: [PATCH] Teach git log --check to return an appropriate error code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-10T18:40:18Z","receivedAt":"2008-08-10T18:40:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Peter Valdemar Mørch (Lists)\"  <4ux6as402@sneakemail.com> writes:\n\n>> One interesting option that might be interesting to add to the log family\n>> would be to show only commits that fail the checkdiff tests.  I suspect\n>> necessary change for doing so would go to log_tree_diff() codepath.\n>\n> I'm hoping that this is meant as \"aside from this current patch, one\n> interesting option...\" or do you mean \"in order for this patch to be\n> accepted, I suggest this to be added ...\" ?\n\nSorry, not the latter.  I'll try to be clear in the future.\n"},{"id":"86750","messageId":"1218437185-6178-1-git-send-email-4ux6as402@sneakemail.com","threadId":"14887","inReplyTo":"1218265054-19220-1-git-send-email-4ux6as402@sneakemail.com","subject":"PATCH v2 0/2 Trying patch again","fromName":"Peter Valdemar Mørch","fromEmail":"4ux6as402@sneakemail.com","sentAt":"2008-08-11T06:46:23Z","receivedAt":"2008-08-11T06:46:23Z","isPatch":false,"sender":{"key":"4ux6as402@sneakemail.com","avatar":null},"body":"\nTrying again after adding comment as described in thread and implementing\n--exit-code for git-log\n"},{"id":"86751","messageId":"1218437185-6178-2-git-send-email-4ux6as402@sneakemail.com","threadId":"14887","inReplyTo":"1218437185-6178-1-git-send-email-4ux6as402@sneakemail.com","subject":"[PATCH v2 1/2] Teach git log --check to return an appropriate exit code","fromName":"Peter Valdemar Mørch","fromEmail":"4ux6as402@sneakemail.com","sentAt":"2008-08-11T06:46:24Z","receivedAt":"2008-08-11T06:46:24Z","isPatch":true,"sender":{"key":"4ux6as402@sneakemail.com","avatar":null},"body":"From: Peter Valdemar Mørch <peter@morch.com>\n\n\nSigned-off-by: Peter Valdemar Mørch <peter@morch.com>\n---\n builtin-log.c |    9 +++++++++\n 1 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex f4975cf..ae71540 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -217,6 +217,11 @@ static int cmd_log_walk(struct rev_info *rev)\n \tif (rev->early_output)\n \t\tfinish_early_output(rev);\n \n+\t/*\n+\t * For --check, the exit code is based on CHECK_FAILED being\n+\t * accumulated in rev->diffopt, so be careful to retain that state\n+\t * information if replacing rev->diffopt in this loop\n+\t */\n \twhile ((commit = get_revision(rev)) != NULL) {\n \t\tlog_tree_commit(rev, commit);\n \t\tif (!rev->reflog_info) {\n@@ -227,6 +232,10 @@ static int cmd_log_walk(struct rev_info *rev)\n \t\tfree_commit_list(commit->parents);\n \t\tcommit->parents = NULL;\n \t}\n+\tif (rev->diffopt.output_format & DIFF_FORMAT_CHECKDIFF &&\n+\t    DIFF_OPT_TST(&rev->diffopt, CHECK_FAILED)) {\n+\t\treturn 02;\n+\t}\n \treturn 0;\n }\n \n-- \n1.6.0.rc2.5.g3452.dirty\n"},{"id":"86752","messageId":"1218437185-6178-3-git-send-email-4ux6as402@sneakemail.com","threadId":"14887","inReplyTo":"1218437185-6178-2-git-send-email-4ux6as402@sneakemail.com","subject":"[PATCH v2 2/2] Teach git log --exit-code to return an appropriate exit code","fromName":"Peter Valdemar Mørch","fromEmail":"4ux6as402@sneakemail.com","sentAt":"2008-08-11T06:46:25Z","receivedAt":"2008-08-11T06:46:25Z","isPatch":true,"sender":{"key":"4ux6as402@sneakemail.com","avatar":null},"body":"From: Peter Valdemar Mørch <peter@morch.com>\n\n\nSigned-off-by: Peter Valdemar Mørch <peter@morch.com>\n---\n builtin-log.c |    8 ++++----\n log-tree.c    |    2 +-\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin-log.c b/builtin-log.c\nindex ae71540..3a79574 100644\n--- a/builtin-log.c\n+++ b/builtin-log.c\n@@ -218,9 +218,9 @@ static int cmd_log_walk(struct rev_info *rev)\n \t\tfinish_early_output(rev);\n \n \t/*\n-\t * For --check, the exit code is based on CHECK_FAILED being\n-\t * accumulated in rev->diffopt, so be careful to retain that state\n-\t * information if replacing rev->diffopt in this loop\n+\t * For --check and --exit-code, the exit code is based on CHECK_FAILED\n+\t * and HAS_CHANGES being accumulated in rev->diffopt, so be careful to\n+\t * retain that state information if replacing rev->diffopt in this loop\n \t */\n \twhile ((commit = get_revision(rev)) != NULL) {\n \t\tlog_tree_commit(rev, commit);\n@@ -236,7 +236,7 @@ static int cmd_log_walk(struct rev_info *rev)\n \t    DIFF_OPT_TST(&rev->diffopt, CHECK_FAILED)) {\n \t\treturn 02;\n \t}\n-\treturn 0;\n+\treturn diff_result_code(&rev->diffopt, 0);\n }\n \n static int git_log_config(const char *var, const char *value, void *cb)\ndiff --git a/log-tree.c b/log-tree.c\nindex bd8b9e4..30cd5bb 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -432,7 +432,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log\n \tstruct commit_list *parents;\n \tunsigned const char *sha1 = commit->object.sha1;\n \n-\tif (!opt->diff)\n+\tif (!opt->diff && !DIFF_OPT_TST(&opt->diffopt, EXIT_WITH_STATUS))\n \t\treturn 0;\n \n \t/* Root commit? */\n-- \n1.6.0.rc2.5.g3452.dirty\n"}]}