{"thread":{"id":"42027","subject":"[PATCH 1/2] fsck_commit_buffer(): do not special case the last validation","startedAt":"2016-04-14T18:07:08Z","lastAt":"2016-04-15T15:06:30Z","messageCount":13,"participants":["Junio C Hamano","Jeff King","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"283463","messageId":"20160414180709.28968-1-gitster@pobox.com","threadId":"42027","inReplyTo":null,"subject":"[PATCH 1/2] fsck_commit_buffer(): do not special case the last validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-14T18:07:08Z","receivedAt":"2016-04-14T18:07:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The pattern taken by all the validations in this function is:\n\n\tif (notice a violation exists) {\n\t\terr = report(... VIOLATION_KIND ...);\n\t\tif (err)\n\t\t\treturn err;\n\t}\n\nwhere report() returns zero if specified kind of violation is set to\nbe ignored, and otherwise shows an error message and returns non-zero.\n\nThe last validation in the function immediately after the function\nreturns 0 to declare \"all good\" can cheat and directly return the\nreturn value from report(), and the current code does so, i.e.\n\n\tif (notice a violation exists)\n\t\treturn report(... VIOLATION_KIND ...);\n\treturn 0;\n\nBut that is a selfish code that declares it is the ultimate and\nfinal form of the function, never to be enhanced later. To allow\nand invite future enhancements, make the last test follow the same\npattern.\n\nSigned-off-by: Junio C Hamano \n---\n fsck.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex ca4c685..21dfa5f 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -666,9 +666,11 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n \terr = fsck_ident(&buffer, &commit->object, options);\n \tif (err)\n \t\treturn err;\n-\tif (!commit->tree)\n-\t\treturn report(options, &commit->object, FSCK_MSG_BAD_TREE, \"could not load commit's tree %s\", sha1_to_hex(tree_sha1));\n-\n+\tif (!commit->tree) {\n+\t\terr = report(options, &commit->object, FSCK_MSG_BAD_TREE, \"could not load commit's tree %s\", sha1_to_hex(tree_sha1));\n+\t\tif (err)\n+\t\t\treturn err;\n+\t}\n \treturn 0;\n }\n \n-- \n2.8.1-355-gcea30bb\n"},{"id":"283464","messageId":"20160414180709.28968-2-gitster@pobox.com","threadId":"42027","inReplyTo":"20160414180709.28968-1-gitster@pobox.com","subject":"[PATCH 2/2] fsck: detect and warn a commit with embedded NUL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-14T18:07:09Z","receivedAt":"2016-04-14T18:07:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Even though a Git commit object is designed to be capable of storing\nany binary data as its payload, in practice people use it to describe\nthe changes in textual form, and tools like \"git log\" are designed to\ntreat the payload as text.\n\nDetect and warn when we see any commit object with a NUL byte in\nit.\n\nNote that a NUL byte in the header part is already detected as a\ngrave error. This change is purely about the message part.\n\nSigned-off-by: Junio C Hamano \n---\n fsck.c | 8 ++++++++\n t/t1450-fsck.sh | 17 +++++++++++++++++\n 2 files changed, 25 insertions(+)\n\ndiff --git a/fsck.c b/fsck.c\nindex 21dfa5f..3366b3f 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -59,6 +59,7 @@\n \tFUNC(HAS_DOTGIT, WARN) \\\n \tFUNC(NULL_SHA1, WARN) \\\n \tFUNC(ZERO_PADDED_FILEMODE, WARN) \\\n+\tFUNC(NUL_IN_COMMIT, WARN) \\\n \t/* infos (reported as warnings, but ignored by default) */ \\\n \tFUNC(BAD_TAG_NAME, INFO) \\\n \tFUNC(MISSING_TAGGER_ENTRY, INFO)\n@@ -610,6 +611,7 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n \tstruct commit_graft *graft;\n \tunsigned parent_count, parent_line_count = 0, author_count;\n \tint err;\n+\tconst char *buffer_begin = buffer;\n \n \tif (verify_headers(buffer, size, &commit->object, options))\n \t\treturn -1;\n@@ -671,6 +673,12 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n \t\tif (err)\n \t\t\treturn err;\n \t}\n+\tif (memchr(buffer_begin, '\\0', size)) {\n+\t\terr = report(options, &commit->object, FSCK_MSG_NUL_IN_COMMIT,\n+\t\t\t \"NUL byte in the commit object body\");\n+\t\tif (err)\n+\t\t\treturn err;\n+\t}\n \treturn 0;\n }\n \ndiff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh\nindex e66b7cb..9012046 100755\n--- a/t/t1450-fsck.sh\n+++ b/t/t1450-fsck.sh\n@@ -427,6 +427,23 @@\n \t)\n '\n \n+test_expect_success 'NUL in commit' '\n+\trm -fr nul-in-commit &&\n+\tgit init nul-in-commit &&\n+\t(\n+\t\tcd nul-in-commit &&\n+\t\tgit commit --allow-empty -m \"initial commitQNUL after message\" &&\n+\t\tgit cat-file commit HEAD >original &&\n+\t\tq_to_nul munged &&\n+\t\tgit hash-object -w -t commit --stdin name &&\n+\t\tgit branch bad $(cat name) &&\n+\n+\t\ttest_must_fail git -c fsck.nulInCommit=error fsck 2>warn.1 &&\n+\t\tgit fsck 2>warn.2 &&\n+\t\tgrep nulInCommit warn.2\n+\t)\n+'\n+\n # create a static test repo which is broken by omitting\n # one particular object ($1, which is looked up via rev-parse\n # in the new repository).\n-- \n2.8.1-355-gcea30bb\n"},{"id":"283465","messageId":"20160414181030.GA22068@sigill.intra.peff.net","threadId":"42027","inReplyTo":"20160414180709.28968-1-gitster@pobox.com","subject":"Re: [PATCH 1/2] fsck_commit_buffer(): do not special case the last validation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-14T18:10:30Z","receivedAt":"2016-04-14T18:10:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 14, 2016 at 11:07:08AM -0700, Junio C Hamano wrote:\n\n> The pattern taken by all the validations in this function is:\n> \n> \tif (notice a violation exists) {\n> \t\terr = report(... VIOLATION_KIND ...);\n> \t\tif (err)\n> \t\t\treturn err;\n> \t}\n> \n> where report() returns zero if specified kind of violation is set to\n> be ignored, and otherwise shows an error message and returns non-zero.\n> \n> The last validation in the function immediately after the function\n> returns 0 to declare \"all good\" can cheat and directly return the\n> return value from report(), and the current code does so, i.e.\n> \n> \tif (notice a violation exists)\n> \t\treturn report(... VIOLATION_KIND ...);\n> \treturn 0;\n> \n> But that is a selfish code that declares it is the ultimate and\n> final form of the function, never to be enhanced later. To allow\n> and invite future enhancements, make the last test follow the same\n> pattern.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> fsck.c | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n\nPatch looks good, and nicely explained.\n\n-Peff\n"},{"id":"283467","messageId":"xmqq8u0ggipg.fsf@gitster.mtv.corp.google.com","threadId":"42027","inReplyTo":"20160414181030.GA22068@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] fsck_commit_buffer(): do not special case the last validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-14T18:15:39Z","receivedAt":"2016-04-14T18:15:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Apr 14, 2016 at 11:07:08AM -0700, Junio C Hamano wrote:\n>\n>> The pattern taken by all the validations in this function is:\n>> \n>> \tif (notice a violation exists) {\n>> \t\terr = report(... VIOLATION_KIND ...);\n>> \t\tif (err)\n>> \t\t\treturn err;\n>> \t}\n>> \n>> where report() returns zero if specified kind of violation is set to\n>> be ignored, and otherwise shows an error message and returns non-zero.\n>> \n>> The last validation in the function immediately after the function\n\ns/after/before/\n\n>> returns 0 to declare \"all good\" can cheat and directly return the\n>> return value from report(), and the current code does so, i.e.\n>> \n>> \tif (notice a violation exists)\n>> \t\treturn report(... VIOLATION_KIND ...);\n>> \treturn 0;\n>> \n>> But that is a selfish code that declares it is the ultimate and\n>> final form of the function, never to be enhanced later. To allow\n>> and invite future enhancements, make the last test follow the same\n>> pattern.\n>> \n>> Signed-off-by: Junio C Hamano \n>> ---\n>> fsck.c | 8 +++++---\n>> 1 file changed, 5 insertions(+), 3 deletions(-)\n>\n> Patch looks good, and nicely explained.\n>\n> -Peff\n"},{"id":"283471","messageId":"20160414182102.GB22068@sigill.intra.peff.net","threadId":"42027","inReplyTo":"20160414180709.28968-2-gitster@pobox.com","subject":"Re: [PATCH 2/2] fsck: detect and warn a commit with embedded NUL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-14T18:21:03Z","receivedAt":"2016-04-14T18:21:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 14, 2016 at 11:07:09AM -0700, Junio C Hamano wrote:\n\n> Even though a Git commit object is designed to be capable of storing\n> any binary data as its payload, in practice people use it to describe\n> the changes in textual form, and tools like \"git log\" are designed to\n> treat the payload as text.\n> \n> Detect and warn when we see any commit object with a NUL byte in\n> it.\n> \n> Note that a NUL byte in the header part is already detected as a\n> grave error. This change is purely about the message part.\n> \n> Signed-off-by: Junio C Hamano \n\nThanks, I was just reading over some of the old threads, and wondering\nif it was time to resurrect this idea.\n\n> @@ -610,6 +611,7 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n> \tstruct commit_graft *graft;\n> \tunsigned parent_count, parent_line_count = 0, author_count;\n> \tint err;\n> +\tconst char *buffer_begin = buffer;\n> \n> \tif (verify_headers(buffer, size, &commit->object, options))\n> \t\treturn -1;\n\nYou need this \"buffer_begin\" because we move the \"buffer\" pointer\nforward as we parse. But perhaps whole-buffer checks should simply go at\nthe top (next to verify_headers) before we start advancing the pointer.\nTo me, that makes the function's flow more natural.\n\nBut alternatively...\n\n> @@ -671,6 +673,12 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n> \t\tif (err)\n> \t\t\treturn err;\n> \t}\n> +\tif (memchr(buffer_begin, '\\0', size)) {\n> +\t\terr = report(options, &commit->object, FSCK_MSG_NUL_IN_COMMIT,\n> +\t\t\t \"NUL byte in the commit object body\");\n> +\t\tif (err)\n> +\t\t\treturn err;\n> +\t}\n\nHere we've parsed to the end of the headers we know about. We know\nthere's no NUL there, because verify_headers() would have complained.\nAnd because the individual header parsers would have complained. So I\nactually think we could check from \"buffer\" (of course we do still need\nto record the beginning of the buffer to adjust \"size\" appropriately).\n\nIt's a little more efficient (we don't have to memchr over the same\nbytes again). But I'd worry a little that doing it that way would\nintroduce coupling between this check and verify_headers(), though (so\nthat if the latter ever changes, our check may start missing cases).\n\nSo yet another alternative would be to include this check in\nverify_headers(). It would parse to the end of the headers as now, and\nthen from there additionally look for a NUL in the body.\n\nOf the three approaches, I think I like that third one. It's the most\nefficient, and I think the flow is pretty clear. We'd probably want to\nrename verify_headers(), though. :)\n\n-Peff\n"},{"id":"283473","messageId":"xmqq1t68gi92.fsf@gitster.mtv.corp.google.com","threadId":"42027","inReplyTo":"20160414182102.GB22068@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] fsck: detect and warn a commit with embedded NUL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-14T18:25:29Z","receivedAt":"2016-04-14T18:25:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> +\tconst char *buffer_begin = buffer;\n>> \n>> \tif (verify_headers(buffer, size, &commit->object, options))\n>> \t\treturn -1;\n>\n> You need this \"buffer_begin\" because we move the \"buffer\" pointer\n> forward as we parse. But perhaps whole-buffer checks should simply go at\n> the top (next to verify_headers) before we start advancing the pointer.\n> To me, that makes the function's flow more natural.\n\nThat was my second iteration. I didn't want the function return\nwith warning without checking more serious errors that may be in the\nobject.\n\n> But alternatively...\n>\n>> @@ -671,6 +673,12 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,\n>> \t\tif (err)\n>> \t\t\treturn err;\n>> \t}\n>> +\tif (memchr(buffer_begin, '\\0', size)) {\n>> +\t\terr = report(options, &commit->object, FSCK_MSG_NUL_IN_COMMIT,\n>> +\t\t\t \"NUL byte in the commit object body\");\n>> +\t\tif (err)\n>> +\t\t\treturn err;\n>> +\t}\n>\n> Here we've parsed to the end of the headers we know about. We know\n> there's no NUL there, because verify_headers() would have complained.\n> And because the individual header parsers would have complained. So I\n> actually think we could check from \"buffer\" (of course we do still need\n> to record the beginning of the buffer to adjust \"size\" appropriately).\n\nYes, keeping the \"begin\" pointer is a cheap way to do an equivalent\nof \"adjusting size\".\n\n> It's a little more efficient (we don't have to memchr over the same\n> bytes again). But I'd worry a little that doing it that way would\n> introduce coupling between this check and verify_headers(), though (so\n> that if the latter ever changes, our check may start missing cases).\n>\n> So yet another alternative would be to include this check in\n> verify_headers(). It would parse to the end of the headers as now, and\n> then from there additionally look for a NUL in the body.\n>\n> Of the three approaches, I think I like that third one. It's the most\n> efficient, and I think the flow is pretty clear. We'd probably want to\n> rename verify_headers(), though. :)\n\nSounds sensible, except the \"should a mere warning hide potentially\nmore serious errors?\" question remains.\n"},{"id":"283474","messageId":"20160414182544.GC22068@sigill.intra.peff.net","threadId":"42027","inReplyTo":"20160414182102.GB22068@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] fsck: detect and warn a commit with embedded NUL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-14T18:25:45Z","receivedAt":"2016-04-14T18:25:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 14, 2016 at 02:21:03PM -0400, Jeff King wrote:\n\n> So yet another alternative would be to include this check in\n> verify_headers(). It would parse to the end of the headers as now, and\n> then from there additionally look for a NUL in the body.\n\nHmm. Looking at verify_headers(), I think it has another bug.\n\nIt wants to \"return report(...)\" as soon as it sees a problem, and stops\nlooking for other ones. But the new model for reporting errors is that\nreport() might ignore a problem if configured to do so.\n\nSo I think if you were to ignore NUL_IN_HEADER, then a commit with\nNUL_IN_HEADER could still pass fsck, even though it should trigger\nUNTERMINATED_HEADER.\n\n-Peff\n"},{"id":"283476","messageId":"20160414182900.GD22068@sigill.intra.peff.net","threadId":"42027","inReplyTo":"xmqq1t68gi92.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] fsck: detect and warn a commit with embedded NUL","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-14T18:29:00Z","receivedAt":"2016-04-14T18:29:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 14, 2016 at 11:25:29AM -0700, Junio C Hamano wrote:\n\n> > You need this \"buffer_begin\" because we move the \"buffer\" pointer\n> > forward as we parse. But perhaps whole-buffer checks should simply go at\n> > the top (next to verify_headers) before we start advancing the pointer.\n> > To me, that makes the function's flow more natural.\n> \n> That was my second iteration. I didn't want the function return\n> with warning without checking more serious errors that may be in the\n> object.\n\nAh, I didn't consider that. In general I'm not sure the distinction\nbetween \"warning\" and \"error\" is all that important, or has been applied\nall that consistently. transfer.fsckObjects will barf on either.\n\nBut I do agree in general that we should be checking as many things as\nwe can. And that we are already wrong to return an error immediately\nwhen verify_headers() complains. We should be accumulating problems in\nan error code and progressing as far as possible. I think fsck_tree() is\na good example of how to do this.\n\n-Peff\n"},{"id":"283479","messageId":"xmqqpotsf34j.fsf@gitster.mtv.corp.google.com","threadId":"42027","inReplyTo":"20160414182544.GC22068@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] fsck: detect and warn a commit with embedded NUL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-14T18:37:32Z","receivedAt":"2016-04-14T18:37:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Apr 14, 2016 at 02:21:03PM -0400, Jeff King wrote:\n>\n>> So yet another alternative would be to include this check in\n>> verify_headers(). It would parse to the end of the headers as now, and\n>> then from there additionally look for a NUL in the body.\n>\n> Hmm. Looking at verify_headers(), I think it has another bug.\n>\n> It wants to \"return report(...)\" as soon as it sees a problem, and stops\n> looking for other ones. But the new model for reporting errors is that\n> report() might ignore a problem if configured to do so.\n\nYup, it should be doing the \"do not return early unless report says\nit is questionable\", at least. Accumulating the error with err |=\nreport() is an improvement, but even without that, doing the usual\n\n\tcase '\\0':\n \terr = report(...);\n return err if err;\n\nwould be a real bugfix.\n"},{"id":"283480","messageId":"xmqqlh4gf1vh.fsf@gitster.mtv.corp.google.com","threadId":"42027","inReplyTo":"20160414182900.GD22068@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] fsck: detect and warn a commit with embedded NUL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-14T19:04:34Z","receivedAt":"2016-04-14T19:04:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But I do agree in general that we should be checking as many things as\n> we can.\n\nI was about to say \"I agree with that in principle, but there are\ncases where you would want to say 'if the object does not pass even\nthis basic check, it is not worth validating it further', and\nverify-headers may fall into that category\". That's another way of\nsaying that something that we cannot even parse into constituent\nfields we cannot check the validity of each field for the semantics.\n\nHowever, with the current \"violations of various classes can be\nconfigured out\" way of doing things, that is a difficult stance to\ntake. If you choose to accept syntax violation that makes us unable\nto parse, you must accept warnings and errors coming from \"missing\"\nfields due to our inability to parse and your telling us to proceed\nanyway.\n\nSo in the end, I do agree that we should be checking as many things\nas we can.\n"},{"id":"283527","messageId":"alpine.DEB.2.20.1604151538230.2967@virtualbox","threadId":"42027","inReplyTo":"20160414180709.28968-1-gitster@pobox.com","subject":"Re: [PATCH 1/2] fsck_commit_buffer(): do not special case the last validation","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-15T13:41:16Z","receivedAt":"2016-04-15T13:41:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 14 Apr 2016, Junio C Hamano wrote:\n\n> The pattern taken by all the validations in this function is:\n> \n> \tif (notice a violation exists) {\n> \t\terr = report(... VIOLATION_KIND ...);\n> \t\tif (err)\n> \t\t\treturn err;\n> \t}\n> \n> where report() returns zero if specified kind of violation is set to\n> be ignored, and otherwise shows an error message and returns non-zero.\n> \n> The last validation in the function immediately after the function\n> returns 0 to declare \"all good\" can cheat and directly return the\n> return value from report(), and the current code does so, i.e.\n> \n> \tif (notice a violation exists)\n> \t\treturn report(... VIOLATION_KIND ...);\n> \treturn 0;\n> \n> But that is a selfish code that declares it is the ultimate and\n> final form of the function, never to be enhanced later. To allow\n> and invite future enhancements, make the last test follow the same\n> pattern.\n\nFWIW I agree with this reasoning. Sorry for leaving this to you to clean\nup.\n\nCiao,\nDscho\n"},{"id":"283528","messageId":"alpine.DEB.2.20.1604151541580.2967@virtualbox","threadId":"42027","inReplyTo":"20160414180709.28968-2-gitster@pobox.com","subject":"Re: [PATCH 2/2] fsck: detect and warn a commit with embedded NUL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-04-15T13:43:52Z","receivedAt":"2016-04-15T13:43:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 14 Apr 2016, Junio C Hamano wrote:\n\n> Even though a Git commit object is designed to be capable of storing\n> any binary data as its payload, in practice people use it to describe\n> the changes in textual form, and tools like \"git log\" are designed to\n> treat the payload as text.\n> \n> Detect and warn when we see any commit object with a NUL byte in\n> it.\n> \n> Note that a NUL byte in the header part is already detected as a\n> grave error. This change is purely about the message part.\n\nIIRC there was some discussion going on about this, and since it was 1)\nlegal and 2) not checked before, I decided not to make this an error.\n\nBut of course it is good to introduce a warning!\n\nThank you,\nDscho\n"},{"id":"283531","messageId":"xmqqy48edi89.fsf@gitster.mtv.corp.google.com","threadId":"42027","inReplyTo":"alpine.DEB.2.20.1604151538230.2967@virtualbox","subject":"Re: [PATCH 1/2] fsck_commit_buffer(): do not special case the last validation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-15T15:06:30Z","receivedAt":"2016-04-15T15:06:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> But that is a selfish code that declares it is the ultimate and\n>> final form of the function, never to be enhanced later. To allow\n>> and invite future enhancements, make the last test follow the same\n>> pattern.\n>\n> FWIW I agree with this reasoning. Sorry for leaving this to you to clean\n> up.\n\nThanks. I often fall into the same trap, and I suspect everybody\ndoes. After doing a large-ish change, it is easy to think that what\nwas just finished is the final one for quite some time to come,\nwithout even realizing that I am being selfish.\n"}]}