{"thread":{"id":"26097","subject":"[PATCH 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid","startedAt":"2010-12-18T14:54:11Z","lastAt":"2010-12-20T19:28:34Z","messageCount":13,"participants":["Kirill Smelkov","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"158316","messageId":"b714f1939ef4fc73cb5f55c1d7784a08a34d3c3d.1292681111.git.kirr@landau.phys.spbu.ru","threadId":"26097","inReplyTo":null,"subject":"[PATCH 1/2] t/t8006: Demonstrate blame is broken when cachetextconv is on","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-12-18T14:54:11Z","receivedAt":"2010-12-18T14:54:11Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"I have a git repository with lots of .doc and .pdf files. There diff\nworks ok, but blaming is painfully slow without textconv cache, and with\ntextconv cache, blame says lots of lines are 'Not Yet Committed' which\nis wrong.\n\nHere is a test that demonstrates the problem.\n\nCc: Axel Bonnet \nCc: Clément Poulain \nCc: Diane Gasselin \nCc: Jeff King \nSigned-off-by: Kirill Smelkov \n---\n t/t8006-blame-textconv.sh | 22 ++++++++++++++++++++++\n 1 files changed, 22 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh\nindex dbf623b..fe90541 100755\n--- a/t/t8006-blame-textconv.sh\n+++ b/t/t8006-blame-textconv.sh\n@@ -73,6 +73,28 @@ test_expect_success 'blame --textconv going through revisions' '\n \ttest_cmp expected result\n '\n \n+test_expect_success 'setup +cachetextconv' '\n+\tgit config diff.test.cachetextconv true\n+'\n+\n+cat >expected_one <blame &&\n+\tfind_blame result &&\n+\ttest_cmp expected result &&\n+\tgit blame --textconv one.bin >blame &&\n+\tfind_blame result &&\n+\ttest_cmp expected_one result\n+'\n+\n+test_expect_success 'setup -cachetextconv' '\n+\tgit config diff.test.cachetextconv false\n+'\n+\n test_expect_success 'make a new commit' '\n \techo \"bin: test number 2 version 3\" >>two.bin &&\n \tGIT_AUTHOR_NAME=Number3 git commit -a -m Third --date=\"2010-01-01 22:00:00\"\n-- \n1.7.3.4.570.g14308\n"},{"id":"158315","messageId":"14308c2dd50037246e319649944d308b9f32fc39.1292681111.git.kirr@landau.phys.spbu.ru","threadId":"26097","inReplyTo":"b714f1939ef4fc73cb5f55c1d7784a08a34d3c3d.1292681111.git.kirr@landau.phys.spbu.ru","subject":"[PATCH 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-12-18T14:54:12Z","receivedAt":"2010-12-18T14:54:12Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"It turned out, under blame there are requests to fill_textconv() with\nsha1=0000000000000000000000000000000000000000 and sha1_valid=0.\n\nAs the code did not analyzed sha1 validity, we ended up putting 000000\ninto textconv cache which was fooling later blames to discover lots of\nlines in 'Not Yet Committed' state.\n\nFix it.\n\nCc: Axel Bonnet \nCc: Clément Poulain \nCc: Diane Gasselin \nCc: Jeff King \nSigned-off-by: Kirill Smelkov \n---\n diff.c | 4 ++--\n t/t8006-blame-textconv.sh | 3 +--\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 0a43869..5422c43 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4412,7 +4412,7 @@ size_t fill_textconv(struct userdiff_driver *driver,\n \t\treturn df->size;\n \t}\n \n-\tif (driver->textconv_cache) {\n+\tif (driver->textconv_cache && df->sha1_valid) {\n \t\t*outbuf = notes_cache_get(driver->textconv_cache, df->sha1,\n \t\t\t\t\t &size);\n \t\tif (*outbuf)\n@@ -4423,7 +4423,7 @@ size_t fill_textconv(struct userdiff_driver *driver,\n \tif (!*outbuf)\n \t\tdie(\"unable to read files to diff\");\n \n-\tif (driver->textconv_cache) {\n+\tif (driver->textconv_cache && df->sha1_valid) {\n \t\t/* ignore errors, as we might be in a readonly repository */\n \t\tnotes_cache_put(driver->textconv_cache, df->sha1, *outbuf,\n \t\t\t\tsize);\ndiff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh\nindex fe90541..ea64cd8 100755\n--- a/t/t8006-blame-textconv.sh\n+++ b/t/t8006-blame-textconv.sh\n@@ -81,8 +81,7 @@ cat >expected_one <blame &&\n \tfind_blame result &&\n \ttest_cmp expected result &&\n-- \n1.7.3.4.570.g14308\n"},{"id":"158317","messageId":"20101218161337.GB18643@sigill.intra.peff.net","threadId":"26097","inReplyTo":"14308c2dd50037246e319649944d308b9f32fc39.1292681111.git.kirr@landau.phys.spbu.ru","subject":"Re: [PATCH 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-12-18T16:13:37Z","receivedAt":"2010-12-18T16:13:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 18, 2010 at 05:54:12PM +0300, Kirill Smelkov wrote:\n\n> It turned out, under blame there are requests to fill_textconv() with\n> sha1=0000000000000000000000000000000000000000 and sha1_valid=0.\n> \n> As the code did not analyzed sha1 validity, we ended up putting 000000\n> into textconv cache which was fooling later blames to discover lots of\n> lines in 'Not Yet Committed' state.\n> [...]\n> -\tif (driver->textconv_cache) {\n> +\tif (driver->textconv_cache && df->sha1_valid) {\n> \t\t*outbuf = notes_cache_get(driver->textconv_cache, df->sha1,\n> \t\t\t\t\t &size);\n\nIn short:\n\n Acked-by: Jeff King \n\nBut it took some thinking to convince myself, so the long answer is\nbelow if anyone cares.\n\nI was dubious at first that this could be the right solution. We still\nend up putting the filespec through run_textconv, which didn't seem\nright if it is not valid.\n\nBut reading into it more, there are two levels of invalidity:\n\n 1. !DIFF_FILE_VALID(df) - we are not a valid file at all. I.e., we are\n /dev/null.\n\n 2. !df->sha1_valid - we are pointing to a working tree file whose sha1\n we don't know\n\nI think level (2) never happens at all in the regular diff code, which\nis why this case was completely unhandled. But it is OK in that case\n(required, even) to put the contents through run_textconv.\n\nIn theory we could actually calculate the sha1 in case (2) and cache\nunder that, but I don't know how much it would buy us in practice. It\nsaves us running the textconv filter at the expense of computing the\nsha1. Which is probably a win for most filters, but on the other hand,\nit is the wrong place to compute such a sha1. If it is a working tree\nfile, we should ideally update our stat info in the index so that the\ninfo can be reused.\n\n-Peff\n\nPS It is a little disturbing that in fill_textconv, we handle\ncase (1), !DIFF_FILE_VALID for the non-textconv case, but not so for the\ntextconv case. I think we are OK, as get_textconv will never load a\ntextconv driver for a !DIFF_FILE_VALID filespec, so we always follow the\nnon-textconv codepath in that case. But I am tempted to do this just to\nbe more defensive:\n\ndiff --git a/diff.c b/diff.c\nindex b0ee213..5320849 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4404,22 +4404,25 @@ size_t fill_textconv(struct userdiff_driver *driver,\n \tif (!driver || !driver->textconv) {\n \t\tif (!DIFF_FILE_VALID(df)) {\n \t\t\t*outbuf = \"\";\n \t\t\treturn 0;\n \t\t}\n \t\tif (diff_populate_filespec(df, 0))\n \t\t\tdie(\"unable to read files to diff\");\n \t\t*outbuf = df->data;\n \t\treturn df->size;\n \t}\n \n+\tif (!DIFF_FILE_VALID(df))\n+\t\tdie(\"BUG: attempt to textconv an invalid filespec\");\n+\n \tif (driver->textconv_cache) {\n \t\t*outbuf = notes_cache_get(driver->textconv_cache, df->sha1,\n \t\t\t\t\t &size);\n \t\tif (*outbuf)\n \t\t\treturn size;\n \t}\n \n \t*outbuf = run_textconv(driver->textconv, df, &size);\n \tif (!*outbuf)\n \t\tdie(\"unable to read files to diff\");\n \n"},{"id":"158335","messageId":"20101218205514.GA21249@landau.phys.spbu.ru","threadId":"26097","inReplyTo":"20101218161337.GB18643@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-12-18T20:55:15Z","receivedAt":"2010-12-18T20:55:15Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Sat, Dec 18, 2010 at 11:13:37AM -0500, Jeff King wrote:\n> On Sat, Dec 18, 2010 at 05:54:12PM +0300, Kirill Smelkov wrote:\n> \n> > It turned out, under blame there are requests to fill_textconv() with\n> > sha1=0000000000000000000000000000000000000000 and sha1_valid=0.\n> > \n> > As the code did not analyzed sha1 validity, we ended up putting 000000\n> > into textconv cache which was fooling later blames to discover lots of\n> > lines in 'Not Yet Committed' state.\n> > [...]\n> > -\tif (driver->textconv_cache) {\n> > +\tif (driver->textconv_cache && df->sha1_valid) {\n> > \t\t*outbuf = notes_cache_get(driver->textconv_cache, df->sha1,\n> > \t\t\t\t\t &size);\n> \n> In short:\n> \n> Acked-by: Jeff King \n> \n> But it took some thinking to convince myself, so the long answer is\n> below if anyone cares.\n> \n> I was dubious at first that this could be the right solution. We still\n> end up putting the filespec through run_textconv, which didn't seem\n> right if it is not valid.\n> \n> But reading into it more, there are two levels of invalidity:\n> \n> 1. !DIFF_FILE_VALID(df) - we are not a valid file at all. I.e., we are\n> /dev/null.\n> \n> 2. !df->sha1_valid - we are pointing to a working tree file whose sha1\n> we don't know\n> \n> I think level (2) never happens at all in the regular diff code, which\n> is why this case was completely unhandled. But it is OK in that case\n> (required, even) to put the contents through run_textconv.\n> \n> In theory we could actually calculate the sha1 in case (2) and cache\n> under that, but I don't know how much it would buy us in practice. It\n> saves us running the textconv filter at the expense of computing the\n> sha1. Which is probably a win for most filters, but on the other hand,\n> it is the wrong place to compute such a sha1. If it is a working tree\n> file, we should ideally update our stat info in the index so that the\n> info can be reused.\n\nJeff,\n\nThanks for your ACK and for the explanation.\n\nMy last patches to git were blame related so semi-intuitively I knew\nthat invalid sha1 are coming from files in worktree. Your description\nmakes things much more clear and I'd put it into patch log as well.\nWhat is the best practice for this? For me to re-roll, or for Junio to\nmerge texts?\n\nAlso experimenting shows that, as you say, regular diff does not have this\nproblem, and also that diff calculates sha1 for files in worktree and so\ntheir textconv results are cached. So clearly, there are some behaviour\ndifferences between diff and blame.\n\n\nThanks again,\nKirill\n"},{"id":"158352","messageId":"7vk4j6fnta.fsf@alter.siamese.dyndns.org","threadId":"26097","inReplyTo":"20101218205514.GA21249@landau.phys.spbu.ru","subject":"Re: [PATCH 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-19T03:23:29Z","receivedAt":"2010-12-19T03:23:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kirill Smelkov writes:\n\n> Thanks for your ACK and for the explanation.\n>\n> My last patches to git were blame related so semi-intuitively I knew\n> that invalid sha1 are coming from files in worktree. Your description\n> makes things much more clear and I'd put it into patch log as well.\n> What is the best practice for this? For me to re-roll, or for Junio to\n> merge texts?\n\nRe-rolling to explain changes in your own words is preferred; thanks.\n"},{"id":"158359","messageId":"20101219121059.GA10985@landau.phys.spbu.ru","threadId":"26097","inReplyTo":"7vk4j6fnta.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-12-19T12:10:59Z","receivedAt":"2010-12-19T12:10:59Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Sat, Dec 18, 2010 at 07:23:29PM -0800, Junio C Hamano wrote:\n> Kirill Smelkov writes:\n> \n> > Thanks for your ACK and for the explanation.\n> >\n> > My last patches to git were blame related so semi-intuitively I knew\n> > that invalid sha1 are coming from files in worktree. Your description\n> > makes things much more clear and I'd put it into patch log as well.\n> > What is the best practice for this? For me to re-roll, or for Junio to\n> > merge texts?\n> \n> Re-rolling to explain changes in your own words is preferred; thanks.\n\nI see, thanks.\n\nI'm not that familiar with git internals involved, so here is updated\npatch with added paragraph about \"df->sha1_valid=0 means files from\nworktree with unknown sha1\", and appropriate excerpt from Jeff's post.\nThat's the most reasonable I could come up with.\n\nThanks,\nKirill\n\nP.S. please don't forget to pick patch 1 which is unchanged.\n\n\n\n---- 8< ----\n\nFrom: Kirill Smelkov \nDate: Sat, 18 Dec 2010 16:27:28 +0300\nSubject: [PATCH v2 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid\nMIME-Version: 1.0\nContent-Type: text/plain; charset=UTF-8\nContent-Transfer-Encoding: 8bit\n\nIt turned out, under blame there are requests to fill_textconv() with\nsha1=0000000000000000000000000000000000000000 and sha1_valid=0.\n\nAs the code did not analyzed sha1 validity, we ended up putting 000000\ninto textconv cache which was fooling later blames to discover lots of\nlines in 'Not Yet Committed' state.\n\nIn practice df->sha1_valid=0 means blame requests to run textconv on a\nfile in worktree whose sha1 is not know yet.\n\nFix it.\n\nOn Sat, Dec 18, 2010 at 11:13:37AM -0500, Jeff King wrote:\n> \n> In short:\n> \n> Acked-by: Jeff King \n> \n> But it took some thinking to convince myself, so the long answer is\n> below if anyone cares.\n> \n> I was dubious at first that this could be the right solution. We still\n> end up putting the filespec through run_textconv, which didn't seem\n> right if it is not valid.\n> \n> But reading into it more, there are two levels of invalidity:\n> \n> 1. !DIFF_FILE_VALID(df) - we are not a valid file at all. I.e., we are\n> /dev/null.\n> \n> 2. !df->sha1_valid - we are pointing to a working tree file whose sha1\n> we don't know\n> \n> I think level (2) never happens at all in the regular diff code, which\n> is why this case was completely unhandled. But it is OK in that case\n> (required, even) to put the contents through run_textconv.\n\nCc: Axel Bonnet \nCc: Clц╘ment Poulain \nCc: Diane Gasselin \nCc: Jeff King \nSigned-off-by: Kirill Smelkov \nAcked-by: Jeff King \n---\n diff.c | 4 ++--\n t/t8006-blame-textconv.sh | 3 +--\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 0a43869..5422c43 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4412,7 +4412,7 @@ size_t fill_textconv(struct userdiff_driver *driver,\n \t\treturn df->size;\n \t}\n \n-\tif (driver->textconv_cache) {\n+\tif (driver->textconv_cache && df->sha1_valid) {\n \t\t*outbuf = notes_cache_get(driver->textconv_cache, df->sha1,\n \t\t\t\t\t &size);\n \t\tif (*outbuf)\n@@ -4423,7 +4423,7 @@ size_t fill_textconv(struct userdiff_driver *driver,\n \tif (!*outbuf)\n \t\tdie(\"unable to read files to diff\");\n \n-\tif (driver->textconv_cache) {\n+\tif (driver->textconv_cache && df->sha1_valid) {\n \t\t/* ignore errors, as we might be in a readonly repository */\n \t\tnotes_cache_put(driver->textconv_cache, df->sha1, *outbuf,\n \t\t\t\tsize);\ndiff --git a/t/t8006-blame-textconv.sh b/t/t8006-blame-textconv.sh\nindex fe90541..ea64cd8 100755\n--- a/t/t8006-blame-textconv.sh\n+++ b/t/t8006-blame-textconv.sh\n@@ -81,8 +81,7 @@ cat >expected_one <blame &&\n \tfind_blame result &&\n \ttest_cmp expected result &&\n-- \n1.7.3.4.570.g14308\n"},{"id":"158378","messageId":"7vr5dddvrk.fsf@alter.siamese.dyndns.org","threadId":"26097","inReplyTo":"20101218161337.GB18643@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-20T02:26:55Z","receivedAt":"2010-12-20T02:26:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> PS It is a little disturbing that in fill_textconv, we handle\n> case (1), !DIFF_FILE_VALID for the non-textconv case, but not so for the\n> textconv case. I think we are OK, as get_textconv will never load a\n> textconv driver for a !DIFF_FILE_VALID filespec, so we always follow the\n> non-textconv codepath in that case. But I am tempted to do this just to\n> be more defensive:\n\nFILE_VALID() is about \"does that side have a blob there, or is this\ncreate/delete diff?\", so the caller should be handling this properly as\nyou said, but your fill_textconv() already prepares for the case where the\ncaller for some reason calls this function with \"no blob on this side\" and\nreturns an empty string (see the precontext of your patch).\n\nI think it is fine to be defensive to prepare for such a case, but then\ndying like this patch does is inconsistent. Perhaps we should move the\nnew check higher and remove the *outbuf = \"\" while at it?\n\n> diff --git a/diff.c b/diff.c\n> index b0ee213..5320849 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -4404,22 +4404,25 @@ size_t fill_textconv(struct userdiff_driver *driver,\n> \tif (!driver || !driver->textconv) {\n> \t\tif (!DIFF_FILE_VALID(df)) {\n> \t\t\t*outbuf = \"\";\n> \t\t\treturn 0;\n> \t\t}\n> \t\tif (diff_populate_filespec(df, 0))\n> \t\t\tdie(\"unable to read files to diff\");\n> \t\t*outbuf = df->data;\n> \t\treturn df->size;\n> \t}\n> \n> +\tif (!DIFF_FILE_VALID(df))\n> +\t\tdie(\"BUG: attempt to textconv an invalid filespec\");\n> +\n"},{"id":"158379","messageId":"7vhbe9dvix.fsf@alter.siamese.dyndns.org","threadId":"26097","inReplyTo":"14308c2dd50037246e319649944d308b9f32fc39.1292681111.git.kirr@landau.phys.spbu.ru","subject":"Re: [PATCH 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-20T02:32:06Z","receivedAt":"2010-12-20T02:32:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kirill Smelkov writes:\n\n> It turned out, under blame there are requests to fill_textconv() with\n> sha1=0000000000000000000000000000000000000000 and sha1_valid=0.\n\nThe code shouldn't even look at sha1[] if sha1_valid is false, as we do\nnot know the hash value for the blob (reading from the working tree).\n\nThanks.\n"},{"id":"158380","messageId":"7vd3oxdv3h.fsf@alter.siamese.dyndns.org","threadId":"26097","inReplyTo":"20101219121059.GA10985@landau.phys.spbu.ru","subject":"Re: [PATCH 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-20T02:41:22Z","receivedAt":"2010-12-20T02:41:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kirill Smelkov writes:\n\n> On Sat, Dec 18, 2010 at 07:23:29PM -0800, Junio C Hamano wrote:\n>> Kirill Smelkov writes:\n>> \n>> > Thanks for your ACK and for the explanation.\n>> >\n>> > My last patches to git were blame related so semi-intuitively I knew\n>> > that invalid sha1 are coming from files in worktree. Your description\n>> > makes things much more clear and I'd put it into patch log as well.\n>> > What is the best practice for this? For me to re-roll, or for Junio to\n>> > merge texts?\n>> \n>> Re-rolling to explain changes in your own words is preferred; thanks.\n>\n> I see, thanks.\n>\n> I'm not that familiar with git internals involved, so here is updated\n> patch with added paragraph about \"df->sha1_valid=0 means files from\n> worktree with unknown sha1\", and appropriate excerpt from Jeff's post.\n> That's the most reasonable I could come up with.\n>\n> Thanks,\n> Kirill\n>\n> P.S. please don't forget to pick patch 1 which is unchanged.\n\nHere is how I would describe it.\n\ncommit 87bb04bb760659dd33d7a173333329cd900620a9\nAuthor: Kirill Smelkov \nDate: Sat Dec 18 17:54:12 2010 +0300\n\n fill_textconv(): Don't get/put cache if sha1 is not valid\n \n When blaming files in the working tree, the filespec is marked with\n !sha1_valid, as we have not given the contents an object name yet. The\n function to cache textconv results (keyed on the object name), however,\n didn't check this condition, and ended up on storing the cached result\n under a random object name.\n \n Signed-off-by: Kirill Smelkov \n"},{"id":"158382","messageId":"20101220044214.GA5942@sigill.intra.peff.net","threadId":"26097","inReplyTo":"7vr5dddvrk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-12-20T04:42:15Z","receivedAt":"2010-12-20T04:42:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 19, 2010 at 06:26:55PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > PS It is a little disturbing that in fill_textconv, we handle\n> > case (1), !DIFF_FILE_VALID for the non-textconv case, but not so for the\n> > textconv case. I think we are OK, as get_textconv will never load a\n> > textconv driver for a !DIFF_FILE_VALID filespec, so we always follow the\n> > non-textconv codepath in that case. But I am tempted to do this just to\n> > be more defensive:\n> \n> FILE_VALID() is about \"does that side have a blob there, or is this\n> create/delete diff?\", so the caller should be handling this properly as\n> you said, but your fill_textconv() already prepares for the case where the\n> caller for some reason calls this function with \"no blob on this side\" and\n> returns an empty string (see the precontext of your patch).\n> \n> I think it is fine to be defensive to prepare for such a case, but then\n> dying like this patch does is inconsistent. Perhaps we should move the\n> new check higher and remove the *outbuf = \"\" while at it?\n\nI'm not sure returning the empty string for a textconv is the right\nsolution. I am inclined to say that trying to textconv a\n!DIFF_FILE_VALID is simply an error. More on that in a second.\n\nIf we were to do anything else, I would think it would be to feed \"\" to\nthe textconv filter, in case it wanted to do something magical for the\ncreate/delete case. For example, imagine a textconv filter which turned\na string of bytes like \"foo\" into a length plus set of converted bytes,\nlike \"3: FOO\". You would want the /dev/null case to turn into \"0: \".\n\nNow, this is obviously a ridiculous toy case. I have no idea if anyone\nwould want to do something like that. So far most people have been happy\nwith /dev/null never being textconv'd, and always looking like the empty\nstring. Moreover, even if somebody did want this behavior, 99% of the\nother filters _wouldn't_ want the behavior. Because programs like\nodt2txt or exiftool that people _do_ use for textconv filters do not\nwant to be fed /dev/null; they will signal an error.\n\nSo that is my \"if we wanted it to do something useful, this is what it\nwould do\" case, and I don't see any real-world evidence that anybody\nwants that. Now on to being defensive.\n\nWhat we are defending against is a caller marking something as\nto-be-textconv'd, even though it is !DIFF_FILE_VALID. But what did the\ncaller want? One sensible behavior is what I described above. Or maybe\nthey did just want the empty string. Or more likely, it is simply a bug\nin the diff code. Since we haven't defined the semantics, I would much\nrather loudly scream \"BUG!\" than paper over it by returning what we\nguess they would have wanted (which may generate subtly wrong results).\nAnd then we can think about that use case and decide what the semantics,\nif any, should be.\n\nSo I stand by my thought that it should die(). But I don't think there\n_are_ any such bugs currently, so it probably doesn't matter much either\nway. I can live with \"return 0\", or even just leaving it alone.\n\n-Peff\n"},{"id":"158383","messageId":"20101220044655.GB5942@sigill.intra.peff.net","threadId":"26097","inReplyTo":"7vd3oxdv3h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-12-20T04:46:56Z","receivedAt":"2010-12-20T04:46:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 19, 2010 at 06:41:22PM -0800, Junio C Hamano wrote:\n\n> > I'm not that familiar with git internals involved, so here is updated\n> > patch with added paragraph about \"df->sha1_valid=0 means files from\n> > worktree with unknown sha1\", and appropriate excerpt from Jeff's post.\n> > That's the most reasonable I could come up with.\n> [...]\n> Here is how I would describe it.\n> \n> commit 87bb04bb760659dd33d7a173333329cd900620a9\n> Author: Kirill Smelkov \n> Date: Sat Dec 18 17:54:12 2010 +0300\n> \n> fill_textconv(): Don't get/put cache if sha1 is not valid\n> \n> When blaming files in the working tree, the filespec is marked with\n> !sha1_valid, as we have not given the contents an object name yet. The\n> function to cache textconv results (keyed on the object name), however,\n> didn't check this condition, and ended up on storing the cached result\n> under a random object name.\n> \n> Signed-off-by: Kirill Smelkov \n\nFWIW, I think that is a good description.\n\n-Peff\n"},{"id":"158390","messageId":"7v4oa8esy4.fsf@alter.siamese.dyndns.org","threadId":"26097","inReplyTo":"20101220044214.GA5942@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-20T08:42:27Z","receivedAt":"2010-12-20T08:42:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Dec 19, 2010 at 06:26:55PM -0800, Junio C Hamano wrote:\n> ...\n>> FILE_VALID() is about \"does that side have a blob there, or is this\n>> create/delete diff?\", so the caller should be handling this properly as\n>> you said, but your fill_textconv() already prepares for the case where the\n>> caller for some reason calls this function with \"no blob on this side\" and\n>> returns an empty string (see the precontext of your patch).\n>> \n>> I think it is fine to be defensive to prepare for such a case, but then\n>> dying like this patch does is inconsistent. Perhaps we should move the\n>> new check higher and remove the *outbuf = \"\" while at it?\n>\n> I'm not sure returning the empty string for a textconv is the right\n> solution....\n>\n> So I stand by my thought that it should die(). But I don't think there\n> _are_ any such bugs currently, so it probably doesn't matter much either\n> way. I can live with \"return 0\", or even just leaving it alone.\n\nI must have phrased it badly. I am actually Ok either way (i.e. make this\nfunction prepare for a future when we start passing the missing side to\nthe function, and have a special case for \"if (!DIFF_FILE_VALID)\" and\nreturning something like an empty string, or make this function refuse to\nbe fed the missing side by dying in \"if (!DIFF_FILE_VALID)\". I was only\npointing out that the result of applying your patch does one in one case\nand another in the other case, which is inconsistent. And we do not know\nin advance what is the reasonable fallback value for the missing side, so\nwe do not now \"something like an empty string\" is a reasonable thing to do\nyet. Hence \"move the new check higher and remove ...\" was my suggestion,\nwhich would look like the attached, which would be consistent with your\nmessage I am replying to. IOW, I think we are on the same page.\n\n diff.c | 15 +++++++++++----\n 1 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 5422c43..a04ab2f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4401,11 +4401,18 @@ size_t fill_textconv(struct userdiff_driver *driver,\n {\n \tsize_t size;\n \n+\t/*\n+\t * !DIFF_FILE_VALID(df) means this is a missing side of the\n+\t * diff (preimage of creation, or postimage of deletion diff).\n+\t * The caller should not try to textconv such a filespec, as\n+\t * there is no such blob to begin with!\n+\t */\n+\t\n+\tif (!DIFF_FILE_VALID(df))\n+\t\tdie(\"Feeding missing side to fill_textconv?: '%s'\",\n+\t\t df->path);\n+\n \tif (!driver || !driver->textconv) {\n-\t\tif (!DIFF_FILE_VALID(df)) {\n-\t\t\t*outbuf = \"\";\n-\t\t\treturn 0;\n-\t\t}\n \t\tif (diff_populate_filespec(df, 0))\n \t\t\tdie(\"unable to read files to diff\");\n \t\t*outbuf = df->data;\n"},{"id":"158410","messageId":"20101220192834.GA6464@landau.phys.spbu.ru","threadId":"26097","inReplyTo":"20101220044655.GB5942@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] fill_textconv(): Don't get/put cache if sha1 is not valid","fromName":"Kirill Smelkov","fromEmail":"kirr@landau.phys.spbu.ru","sentAt":"2010-12-20T19:28:34Z","receivedAt":"2010-12-20T19:28:34Z","isPatch":true,"sender":{"key":"kirr@navytux.spb.ru","avatar":"https://gravatar.com/avatar/cf3445fdad1849941e17ab25bf1ee7c5ea1be2deb444be25ff5f36b0e50a985f?d=mp&s=160"},"body":"On Sun, Dec 19, 2010 at 11:46:56PM -0500, Jeff King wrote:\n> On Sun, Dec 19, 2010 at 06:41:22PM -0800, Junio C Hamano wrote:\n> \n> > > I'm not that familiar with git internals involved, so here is updated\n> > > patch with added paragraph about \"df->sha1_valid=0 means files from\n> > > worktree with unknown sha1\", and appropriate excerpt from Jeff's post.\n> > > That's the most reasonable I could come up with.\n> > [...]\n> > Here is how I would describe it.\n> > \n> > commit 87bb04bb760659dd33d7a173333329cd900620a9\n> > Author: Kirill Smelkov \n> > Date: Sat Dec 18 17:54:12 2010 +0300\n> > \n> > fill_textconv(): Don't get/put cache if sha1 is not valid\n> > \n> > When blaming files in the working tree, the filespec is marked with\n> > !sha1_valid, as we have not given the contents an object name yet. The\n> > function to cache textconv results (keyed on the object name), however,\n> > didn't check this condition, and ended up on storing the cached result\n> > under a random object name.\n> > \n> > Signed-off-by: Kirill Smelkov \n> \n> FWIW, I think that is a good description.\n\nJunio, Jeff, thanks for re-wording it. Though I think my v2 text was\nsaying the same, only with more info + examples. My english is pretty\nbad this days, so I kind of understand why it was tempting to be redone :)\n\n\nThanks anyway, and for picking this into next,\nKirill\n\n\nP.S. somehow 'Acked-by: Jeff King ' was dropped.\n"}]}