{"thread":{"id":"15108","subject":"diff --check is stupid about blank lines","startedAt":"2008-08-20T14:05:17Z","lastAt":"2008-08-20T22:39:25Z","messageCount":7,"participants":["Björn Steinbrink","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"87821","messageId":"20080820140517.GA1304@atjola.homenet","threadId":"15108","inReplyTo":null,"subject":"diff --check is stupid about blank lines","fromName":"Björn Steinbrink","fromEmail":"b.steinbrink@gmx.de","sentAt":"2008-08-20T14:05:17Z","receivedAt":"2008-08-20T14:05:17Z","isPatch":false,"sender":{"key":"b.steinbrink@gmx.de","avatar":"https://avatars.githubusercontent.com/u/230962?v=4"},"body":"Hi,\n\ndiff --check triggers not only for blank lines at the end of the file,\nbut also at the end of the changes. That seems broken to me, unless you\nreally dislike empty lines.\n\nFor example:\ngit (master) $ git diff\ndiff --git a/git.c b/git.c\nindex 37b1d76..3fa1aeb 100644\n--- a/git.c\n+++ b/git.c\n@@ -9,6 +9,8 @@ const char git_usage_string[] =\n const char git_more_info_string[] =\n \"See 'git help COMMAND' for more information on a specific command.\";\n \n+int new_var = 0;\n+\n static int use_pager = -1;\n struct pager_config {\n const char *cmd;\n\ngit (master) $ git diff --check\ngit.c:13: ends with blank lines.\n\nBut that blank line was of course intentional.\n\nI'm not quite sure why that happens though. The code in\ncheckdiff_consume seems to reset the flag when it sees context lines,\nbut apparently that does not work for some reason.\n\ngit version 1.6.0.36.g3814c\n\nBjörn\n"},{"id":"87830","messageId":"20080820172825.GC27773@sigill.intra.peff.net","threadId":"15108","inReplyTo":"20080820140517.GA1304@atjola.homenet","subject":"Re: diff --check is stupid about blank lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-08-20T17:28:25Z","receivedAt":"2008-08-20T17:28:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 20, 2008 at 04:05:17PM +0200, Björn Steinbrink wrote:\n\n> diff --check triggers not only for blank lines at the end of the file,\n> but also at the end of the changes. That seems broken to me, unless you\n> really dislike empty lines.\n\nHmm, yes, that seems wrong. The problem seems to be the conditional at\ndiff.c:1622:\n\n if ((data.ws_rule & WS_TRAILING_SPACE) &&\n data.trailing_blanks_start) {\n fprintf(o->file, \"%s:%d: ends with blank lines.\\n\",\n data.filename, data.trailing_blanks_start);\n data.status = 1; /* report errors */\n }\n\nthat should probably be \"if we care about trailing space, and the last\nthing we saw was a trailing blank, _and_ the last hunk adds to\nend-of-file, then...\". However, I'm not sure what is the best way to get\nthat information out of xdiff. Is there a \"this hunk hits eof\" signal\nanywhere? Is there a definitive line count we could use to calculate\nthat it is in the chunk of final lines in the file?\n\n-Peff\n"},{"id":"87837","messageId":"7v7iabsgfk.fsf@gitster.siamese.dyndns.org","threadId":"15108","inReplyTo":"20080820172825.GC27773@sigill.intra.peff.net","subject":"Re: diff --check is stupid about blank lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-20T18:13:35Z","receivedAt":"2008-08-20T18:13:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... The problem seems to be the conditional at\n> diff.c:1622:\n>\n> if ((data.ws_rule & WS_TRAILING_SPACE) &&\n> data.trailing_blanks_start) {\n> fprintf(o->file, \"%s:%d: ends with blank lines.\\n\",\n> data.filename, data.trailing_blanks_start);\n> data.status = 1; /* report errors */\n> }\n>\n> that should probably be \"if we care about trailing space, and the last\n> thing we saw was a trailing blank, _and_ the last hunk adds to\n> end-of-file, then...\".\n\nInstead, data.trailing_blanks_start is supposed to be reset to 0 every\ntime we see non-blank newline, a copied context line, or new hunk.\n\nSo if this triggers with -U0 I'd understand, but otherwise I do not see\nhow.\n"},{"id":"87844","messageId":"7vskszr0it.fsf@gitster.siamese.dyndns.org","threadId":"15108","inReplyTo":"7v7iabsgfk.fsf@gitster.siamese.dyndns.org","subject":"Re: diff --check is stupid about blank lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-20T18:42:34Z","receivedAt":"2008-08-20T18:42:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> ... The problem seems to be the conditional at\n>> diff.c:1622:\n>>\n>> if ((data.ws_rule & WS_TRAILING_SPACE) &&\n>> data.trailing_blanks_start) {\n>> fprintf(o->file, \"%s:%d: ends with blank lines.\\n\",\n>> data.filename, data.trailing_blanks_start);\n>> data.status = 1; /* report errors */\n>> }\n>>\n>> that should probably be \"if we care about trailing space, and the last\n>> thing we saw was a trailing blank, _and_ the last hunk adds to\n>> end-of-file, then...\".\n>\n> Instead, data.trailing_blanks_start is supposed to be reset to 0 every\n> time we see non-blank newline, a copied context line, or new hunk.\n>\n> So if this triggers with -U0 I'd understand, but otherwise I do not see\n> how.\n\nAhhh, what idiot wrote the logic for checking trailing blank lines in\ncheckdiff_consume().\n\nIt does not ask for any context lines. Sheesh.\n\nThis should fix it.\n\n\ndiff --git i/diff.c w/diff.c\nindex 10d5440..5923fe2 100644\n--- i/diff.c\n+++ w/diff.c\n@@ -1628,6 +1628,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,\n \t\txdemitcb_t ecb;\n \n \t\tmemset(&xecfg, 0, sizeof(xecfg));\n+\t\txecfg.ctxlen = 1; /* at least one context line */\n \t\txpp.flags = XDF_NEED_MINIMAL;\n \t\txdi_diff_outf(&mf1, &mf2, checkdiff_consume, &data,\n \t\t\t &xpp, &xecfg, &ecb);\n"},{"id":"87858","messageId":"20080820200201.GA3210@atjola.homenet","threadId":"15108","inReplyTo":"7vskszr0it.fsf@gitster.siamese.dyndns.org","subject":"Re: diff --check is stupid about blank lines","fromName":"Björn Steinbrink","fromEmail":"b.steinbrink@gmx.de","sentAt":"2008-08-20T20:02:01Z","receivedAt":"2008-08-20T20:02:01Z","isPatch":false,"sender":{"key":"b.steinbrink@gmx.de","avatar":"https://avatars.githubusercontent.com/u/230962?v=4"},"body":"On 2008.08.20 11:42:34 -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> > Instead, data.trailing_blanks_start is supposed to be reset to 0 every\n> > time we see non-blank newline, a copied context line, or new hunk.\n> >\n> > So if this triggers with -U0 I'd understand, but otherwise I do not see\n> > how.\n> \n> Ahhh, what idiot wrote the logic for checking trailing blank lines in\n> checkdiff_consume().\n> \n> It does not ask for any context lines. Sheesh.\n> \n> This should fix it.\n\nThanks, works for me :-)\n\nBjörn\n"},{"id":"87894","messageId":"20080820222048.GB8137@coredump.intra.peff.net","threadId":"15108","inReplyTo":"7vskszr0it.fsf@gitster.siamese.dyndns.org","subject":"Re: diff --check is stupid about blank lines","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-08-20T22:20:48Z","receivedAt":"2008-08-20T22:20:48Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 20, 2008 at 11:42:34AM -0700, Junio C Hamano wrote:\n\n> Ahhh, what idiot wrote the logic for checking trailing blank lines in\n> checkdiff_consume().\n> \n> It does not ask for any context lines. Sheesh.\n> \n> This should fix it.\n\nAh, that was much easier than I expected. ;)\n\nMaybe this should be squashed in?\n\n---\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 6110566..974efd0 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -341,4 +341,13 @@ test_expect_success 'checkdiff detects trailing blank lines' '\n \tgit diff --check | grep \"ends with blank\"\n '\n \n+test_expect_success 'checkdiff does not mistake trailing blank lines in hunk' '\n+\techo content >x &&\n+\tgit add x &&\n+\techo \"\" >x &&\n+\techo content >>x &&\n+\tgit diff --check >out &&\n+\t! grep \"ends with blank\" out\n+'\n+\n test_done\n"},{"id":"87899","messageId":"7v8wurnwf6.fsf@gitster.siamese.dyndns.org","threadId":"15108","inReplyTo":"20080820222048.GB8137@coredump.intra.peff.net","subject":"Re: diff --check is stupid about blank lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-08-20T22:39:25Z","receivedAt":"2008-08-20T22:39:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Aug 20, 2008 at 11:42:34AM -0700, Junio C Hamano wrote:\n>\n>> Ahhh, what idiot wrote the logic for checking trailing blank lines in\n>> checkdiff_consume().\n>> \n>> It does not ask for any context lines. Sheesh.\n>> \n>> This should fix it.\n>\n> Ah, that was much easier than I expected. ;)\n>\n> Maybe this should be squashed in?\n\nThanks but I've done that myself already.\n"}]}