{"thread":{"id":"46909","subject":"[PATCH v1 0/2] fix temporary garbage in the cache entry","startedAt":"2017-10-05T10:46:11Z","lastAt":"2017-10-10T09:49:42Z","messageCount":26,"participants":["lars.schneider@autodesk.com","Jeff King","Junio C Hamano","Lars Schneider","Simon Ruderich"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"329746","messageId":"20171005104407.65948-1-lars.schneider@autodesk.com","threadId":"46909","inReplyTo":null,"subject":"[PATCH v1 0/2] fix temporary garbage in the cache entry","fromName":"","fromEmail":"lars.schneider@autodesk.com","sentAt":"2017-10-05T10:44:05Z","receivedAt":"2017-10-05T10:46:11Z","isPatch":true,"sender":{"key":"lars.schneider@autodesk.com","avatar":"https://gravatar.com/avatar/524188df3a38b02692619777bb90d9cc735db1ad75f5304e0bf09f8efd299775?d=mp&s=160"},"body":"From: Lars Schneider \n\nHi,\n\nin [1] Peff noticed that the delayed filter mechanism causes garbage\nin the cache entry for a brief moment during a delayed checkout process.\n\nThe first patch fixes this issue. The second patch ensures that we don't\nwrite garbage in that way to the cache entry caused by any other unknown\nor external reason ever again.\n\nPlease review carefully in case I overlooked some unintended side\neffect.\n\nThank you,\nLars\n\n[1] https://public-inbox.org/git/20171005034632.kzsspk7wsuk23kf2@sigill.intra.peff.net/\n\n\n\nBase Ref: v2.15.0-rc0\nWeb-Diff: https://github.com/larsxschneider/git/commit/d6fff1a941\nCheckout: git fetch https://github.com/larsxschneider/git filter-process/fix-cache-entry-v1 && git checkout d6fff1a941\n\nLars Schneider (2):\n entry.c: update cache entry only for existing files\n entry.c: check if file exists after checkout\n\n entry.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\n\nbase-commit: 217f2767cbcb562872437eed4dec62e00846d90c\n--\n2.14.2\n\n"},{"id":"329747","messageId":"20171005104407.65948-3-lars.schneider@autodesk.com","threadId":"46909","inReplyTo":"20171005104407.65948-1-lars.schneider@autodesk.com","subject":"[PATCH v1 2/2] entry.c: check if file exists after checkout","fromName":"","fromEmail":"lars.schneider@autodesk.com","sentAt":"2017-10-05T10:44:07Z","receivedAt":"2017-10-05T10:46:14Z","isPatch":true,"sender":{"key":"lars.schneider@autodesk.com","avatar":"https://gravatar.com/avatar/524188df3a38b02692619777bb90d9cc735db1ad75f5304e0bf09f8efd299775?d=mp&s=160"},"body":"From: Lars Schneider \n\nIf we are checking out a file and somebody else racily deletes our file,\nthen we would write garbage to the cache entry. Fix that by checking\nthe result of the lstat() call on that file. Print an error to the user\nif the file does not exist.\n\nReported-by: Jeff King \nSigned-off-by: Lars Schneider \n---\n entry.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/entry.c b/entry.c\nindex 5dab656364..2252d96756 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -355,7 +355,8 @@ static int write_entry(struct cache_entry *ce,\n \tif (state->refresh_cache) {\n \t\tassert(state->istate);\n \t\tif (!fstat_done)\n-\t\t\tlstat(ce->name, &st);\n+\t\t\tif (lstat(ce->name, &st) < 0)\n+\t\t\t\treturn error(\"unable to get status of file %s\", ce->name);\n \t\tfill_stat_cache_info(ce, &st);\n \t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n \t\tstate->istate->cache_changed |= CE_ENTRY_CHANGED;\n-- \n2.14.2\n\n"},{"id":"329748","messageId":"20171005104407.65948-2-lars.schneider@autodesk.com","threadId":"46909","inReplyTo":"20171005104407.65948-1-lars.schneider@autodesk.com","subject":"[PATCH v1 1/2] entry.c: update cache entry only for existing files","fromName":"","fromEmail":"lars.schneider@autodesk.com","sentAt":"2017-10-05T10:44:06Z","receivedAt":"2017-10-05T10:46:15Z","isPatch":true,"sender":{"key":"lars.schneider@autodesk.com","avatar":"https://gravatar.com/avatar/524188df3a38b02692619777bb90d9cc735db1ad75f5304e0bf09f8efd299775?d=mp&s=160"},"body":"From: Lars Schneider \n\nIn 2841e8f (\"convert: add \"status=delayed\" to filter process protocol\",\n2017-06-30) we taught the filter process protocol to delay responses.\n\nThat means an external filter might answer in the first write_entry()\ncall on a file that requires filtering \"I got your request, but I\ncan't answer right now. Ask again later!\". As Git got no answer, we do\nnot write anything to the filesystem. Consequently, the lstat() call in\nthe finish block of the function writes garbage to the cache entry.\nThe garbage is eventually overwritten when the filter answers with\nthe final file content in a subsequent write_entry() call.\n\nFix the brief time window of garbage in the cache entry by adding a\nspecial finish block that does nothing for delayed responses. The cache\nentry is written properly in a subsequent write_entry() call where\nthe filter responds with the final file content.\n\nReported-by: Jeff King \nSigned-off-by: Lars Schneider \n---\n entry.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/entry.c b/entry.c\nindex 1c7e3c11d5..5dab656364 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -304,7 +304,7 @@ static int write_entry(struct cache_entry *ce,\n \t\t\t\t\tce->name, new, size, &buf, dco);\n \t\t\t\tif (ret && string_list_has_string(&dco->paths, ce->name)) {\n \t\t\t\t\tfree(new);\n-\t\t\t\t\tgoto finish;\n+\t\t\t\t\tgoto delayed;\n \t\t\t\t}\n \t\t\t} else\n \t\t\t\tret = convert_to_working_tree(\n@@ -360,6 +360,7 @@ static int write_entry(struct cache_entry *ce,\n \t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n \t\tstate->istate->cache_changed |= CE_ENTRY_CHANGED;\n \t}\n+delayed:\n \treturn 0;\n }\n \n-- \n2.14.2\n\n"},{"id":"329751","messageId":"20171005111249.4fdezbjnysb6t2zm@sigill.intra.peff.net","threadId":"46909","inReplyTo":"20171005104407.65948-2-lars.schneider@autodesk.com","subject":"Re: [PATCH v1 1/2] entry.c: update cache entry only for existing files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-05T11:12:49Z","receivedAt":"2017-10-05T11:12:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 05, 2017 at 12:44:06PM +0200, lars.schneider@autodesk.com wrote:\n\n> From: Lars Schneider \n> \n> In 2841e8f (\"convert: add \"status=delayed\" to filter process protocol\",\n> 2017-06-30) we taught the filter process protocol to delay responses.\n> \n> That means an external filter might answer in the first write_entry()\n> call on a file that requires filtering \"I got your request, but I\n> can't answer right now. Ask again later!\". As Git got no answer, we do\n> not write anything to the filesystem. Consequently, the lstat() call in\n> the finish block of the function writes garbage to the cache entry.\n> The garbage is eventually overwritten when the filter answers with\n> the final file content in a subsequent write_entry() call.\n> \n> Fix the brief time window of garbage in the cache entry by adding a\n> special finish block that does nothing for delayed responses. The cache\n> entry is written properly in a subsequent write_entry() call where\n> the filter responds with the final file content.\n\nNicely explained and the patch looks correct. I also verified that MSan\nis happy with t0021 after this.\n\nThanks for the quick turnaround.\n\n-Peff\n"},{"id":"329752","messageId":"xmqqk2097sge.fsf@gitster.mtv.corp.google.com","threadId":"46909","inReplyTo":"20171005104407.65948-2-lars.schneider@autodesk.com","subject":"Re: [PATCH v1 1/2] entry.c: update cache entry only for existing files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-05T11:19:13Z","receivedAt":"2017-10-05T11:19:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"lars.schneider@autodesk.com writes:\n\n> diff --git a/entry.c b/entry.c\n> index 1c7e3c11d5..5dab656364 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -304,7 +304,7 @@ static int write_entry(struct cache_entry *ce,\n> \t\t\t\t\tce->name, new, size, &buf, dco);\n> \t\t\t\tif (ret && string_list_has_string(&dco->paths, ce->name)) {\n> \t\t\t\t\tfree(new);\n> -\t\t\t\t\tgoto finish;\n> +\t\t\t\t\tgoto delayed;\n> \t\t\t\t}\n> \t\t\t} else\n> \t\t\t\tret = convert_to_working_tree(\n\nThis is unrelated to the main topic of this patch, but we see this\njust before the precontext of this hunk:\n\n\t\t\tif (dco && dco->state != CE_NO_DELAY) {\n\t\t\t\t/* Do not send the blob in case of a retry. */\n\t\t\t\tif (dco->state == CE_RETRY) {\n\t\t\t\t\tnew = NULL;\n\t\t\t\t\tsize = 0;\n\t\t\t\t}\n\t\t\t\tret = async_convert_to_working_tree(\n\t\t\t\t\tce->name, new, size, &buf, dco);\n\nAren't we leaking \"new\" in that CE_RETRY case?\n"},{"id":"329753","messageId":"20171005112355.lsoqxybgsovpqriy@sigill.intra.peff.net","threadId":"46909","inReplyTo":"20171005104407.65948-3-lars.schneider@autodesk.com","subject":"Re: [PATCH v1 2/2] entry.c: check if file exists after checkout","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-05T11:23:55Z","receivedAt":"2017-10-05T11:24:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 05, 2017 at 12:44:07PM +0200, lars.schneider@autodesk.com wrote:\n\n> From: Lars Schneider \n> \n> If we are checking out a file and somebody else racily deletes our file,\n> then we would write garbage to the cache entry. Fix that by checking\n> the result of the lstat() call on that file. Print an error to the user\n> if the file does not exist.\n\nMy gut tells me this is the right thing to be doing, but this commit\nmessage gives very little analysis. Let's see if we can talk it out a\nbit.\n\nAside from bizarre lstat failures, the plausible reason for seeing this\nis that somebody racily deleted the file. I.e.,:\n\n 1. We wrote the file.\n\n 2. They deleted it.\n\n 3. We ran lstat() on it and found that it went away.\n\nBut imagine that the race went the other way, and (3) happened before\n(2). Then we'd actually get a real index entry, but the file would\nappear deleted to anybody who checks the filesystem against the stat\ndata.\n\nSo I guess my question is: is step 3 an integral part of the checkout\nprocedure, or is it simply an opportunity to refresh the index (since we\nknow we just wrote out the content)?\n\nIf it's an integral part, then I agree that the error return you add\nhere is the right thing to do. But if it's just an index refresh, then I\nwonder if we should report a successful checkout, but mark the entry as\nstat-dirty.\n\nI dunno. It's pretty philosophical, and I have a feeling that nobody\nreally cares all that much in practice. Certainly the error return seems\nlike the easiest fix.\n\n> diff --git a/entry.c b/entry.c\n> index 5dab656364..2252d96756 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -355,7 +355,8 @@ static int write_entry(struct cache_entry *ce,\n> \tif (state->refresh_cache) {\n> \t\tassert(state->istate);\n> \t\tif (!fstat_done)\n> -\t\t\tlstat(ce->name, &st);\n> +\t\t\tif (lstat(ce->name, &st) < 0)\n> +\t\t\t\treturn error(\"unable to get status of file %s\", ce->name);\n\nWe could probably be a bit more specific about the situation, since the\nuser will see this message with no context. Maybe something like:\n\n unable to stat just-written file %s\n\nor something. We should probably also use error_errno(). I'd bet if this\never triggers that it's likely to be ENOENT, but certainly if it _isn't_\nthat would be interesting information.\n\n-Peff\n"},{"id":"329754","messageId":"20171005112658.p7hohhtkdkcapwe6@sigill.intra.peff.net","threadId":"46909","inReplyTo":"xmqqk2097sge.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 1/2] entry.c: update cache entry only for existing files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-05T11:26:58Z","receivedAt":"2017-10-05T11:27:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 05, 2017 at 08:19:13PM +0900, Junio C Hamano wrote:\n\n> > diff --git a/entry.c b/entry.c\n> > index 1c7e3c11d5..5dab656364 100644\n> > --- a/entry.c\n> > +++ b/entry.c\n> > @@ -304,7 +304,7 @@ static int write_entry(struct cache_entry *ce,\n> > \t\t\t\t\tce->name, new, size, &buf, dco);\n> > \t\t\t\tif (ret && string_list_has_string(&dco->paths, ce->name)) {\n> > \t\t\t\t\tfree(new);\n> > -\t\t\t\t\tgoto finish;\n> > +\t\t\t\t\tgoto delayed;\n> > \t\t\t\t}\n> > \t\t\t} else\n> > \t\t\t\tret = convert_to_working_tree(\n> \n> This is unrelated to the main topic of this patch, but we see this\n> just before the precontext of this hunk:\n> \n> \t\t\tif (dco && dco->state != CE_NO_DELAY) {\n> \t\t\t\t/* Do not send the blob in case of a retry. */\n> \t\t\t\tif (dco->state == CE_RETRY) {\n> \t\t\t\t\tnew = NULL;\n> \t\t\t\t\tsize = 0;\n> \t\t\t\t}\n> \t\t\t\tret = async_convert_to_working_tree(\n> \t\t\t\t\tce->name, new, size, &buf, dco);\n> \n> Aren't we leaking \"new\" in that CE_RETRY case?\n\nYes, it certainly looks like it. Wouldn't we want to avoid reading the\nfile from disk entirely in that case?\n\nI.e., I think free(new) is sufficient to fix the leak you mentioned. But\nI think we'd want to protect the read_blob_entry() call at the top of\nthe case with a check for dco->state == CE_RETRY.\n\n-Peff\n"},{"id":"329823","messageId":"xmqqefqh6vxf.fsf@gitster.mtv.corp.google.com","threadId":"46909","inReplyTo":"20171005112658.p7hohhtkdkcapwe6@sigill.intra.peff.net","subject":"Re: [PATCH v1 1/2] entry.c: update cache entry only for existing files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-05T23:01:48Z","receivedAt":"2017-10-05T23:02:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Oct 05, 2017 at 08:19:13PM +0900, Junio C Hamano wrote:\n>\n>> This is unrelated to the main topic of this patch, but we see this\n>> just before the precontext of this hunk:\n>> \n>> \t\t\tif (dco && dco->state != CE_NO_DELAY) {\n>> \t\t\t\t/* Do not send the blob in case of a retry. */\n>> \t\t\t\tif (dco->state == CE_RETRY) {\n>> \t\t\t\t\tnew = NULL;\n>> \t\t\t\t\tsize = 0;\n>> \t\t\t\t}\n>> \t\t\t\tret = async_convert_to_working_tree(\n>> \t\t\t\t\tce->name, new, size, &buf, dco);\n>> \n>> Aren't we leaking \"new\" in that CE_RETRY case?\n>\n> Yes, it certainly looks like it. Wouldn't we want to avoid reading the\n> file from disk entirely in that case?\n\nProbably. But that is more of a removal of pessimization than a fix ;-)\n\n> I.e., I think free(new) is sufficient to fix the leak you\n> mentioned.\n\nIn addition to keeping the new = NULL assignment, of course.\n\n> But\n> I think we'd want to protect the read_blob_entry() call at the top of\n> the case with a check for dco->state == CE_RETRY.\n\nYeah, I think that makes more sense.\n\nA patch may look like this on top of these two patches, but I'd\nprefer to see Lars's eyeballing and possibly wrapping it up in an\napplicable patch after taking the authorship.\n\nI considered initializing new to NULL and size to 0 but decided\nagainst it, as that would lose the justification to have an if\nstatement that marks that \"dco->state == CE_RETRY\" is a special\ncase. I think explicit if() with clearing these two variables makes\nit clearer to show what is going on.\n\nBy the way, the S_IFLNK handling seems iffy with or without this\nchange (or for that matter, I suspect this iffy-ness existed before\nLars's delayed filtering change). On a platform without symlinks,\nwe do the same as S_IFREG, but obviously we do not want any content\nconversion that happens to the regular files in such a case. So we\nmay further want to fix that, but I left it outside the scope of\nfixing the leak of NULL and optimizing the blob reading out.\n\n\n entry.c | 26 +++++++++++++++++---------\n 1 file changed, 17 insertions(+), 9 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex cac5bf5af2..74e35f942c 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -274,14 +274,12 @@ static int write_entry(struct cache_entry *ce,\n \t}\n \n \tswitch (ce_mode_s_ifmt) {\n-\tcase S_IFREG:\n \tcase S_IFLNK:\n \t\tnew = read_blob_entry(ce, &size);\n \t\tif (!new)\n \t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n \t\t\t\tpath, oid_to_hex(&ce->oid));\n-\n-\t\tif (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) {\n+\t\tif (has_symlinks && !to_tempfile) {\n \t\t\tret = symlink(new, path);\n \t\t\tfree(new);\n \t\t\tif (ret)\n@@ -289,18 +287,28 @@ static int write_entry(struct cache_entry *ce,\n \t\t\t\t\t\t path);\n \t\t\tbreak;\n \t\t}\n-\n+\t\t/* fallthru */\n+\tcase S_IFREG:\n \t\t/*\n \t\t * Convert from git internal format to working tree format\n \t\t */\n \t\tif (ce_mode_s_ifmt == S_IFREG) {\n \t\t\tstruct delayed_checkout *dco = state->delayed_checkout;\n+\n+\t\t\t/* \n+\t\t\t * In case of a retry, we do not send blob, hence no\n+\t\t\t * need to read it, either.\n+\t\t\t */\n+\t\t\tif (dco && dco->state == CE_RETRY) {\n+\t\t\t\tnew = NULL;\n+\t\t\t\tsize = 0;\n+\t\t\t} else {\n+\t\t\t\tnew = read_blob_entry(ce, &size);\n+\t\t\t\tif (!new)\n+\t\t\t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n+\t\t\t\t\t\t path, oid_to_hex(&ce->oid));\n+\t\t\t}\n \t\t\tif (dco && dco->state != CE_NO_DELAY) {\n-\t\t\t\t/* Do not send the blob in case of a retry. */\n-\t\t\t\tif (dco->state == CE_RETRY) {\n-\t\t\t\t\tnew = NULL;\n-\t\t\t\t\tsize = 0;\n-\t\t\t\t}\n \t\t\t\tret = async_convert_to_working_tree(\n \t\t\t\t\tce->name, new, size, &buf, dco);\n \t\t\t\tif (ret && string_list_has_string(&dco->paths, ce->name)) {\n"},{"id":"329843","messageId":"xmqqlgkoyk8n.fsf@gitster.mtv.corp.google.com","threadId":"46909","inReplyTo":"20171005112355.lsoqxybgsovpqriy@sigill.intra.peff.net","subject":"Re: [PATCH v1 2/2] entry.c: check if file exists after checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-06T04:26:48Z","receivedAt":"2017-10-06T04:26:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> diff --git a/entry.c b/entry.c\n>> index 5dab656364..2252d96756 100644\n>> --- a/entry.c\n>> +++ b/entry.c\n>> @@ -355,7 +355,8 @@ static int write_entry(struct cache_entry *ce,\n>> \tif (state->refresh_cache) {\n>> \t\tassert(state->istate);\n>> \t\tif (!fstat_done)\n>> -\t\t\tlstat(ce->name, &st);\n>> +\t\t\tif (lstat(ce->name, &st) < 0)\n>> +\t\t\t\treturn error(\"unable to get status of file %s\", ce->name);\n>\n> We could probably be a bit more specific about the situation, since the\n> user will see this message with no context. Maybe something like:\n>\n> unable to stat just-written file %s\n>\n> or something. We should probably also use error_errno(). I'd bet if this\n> ever triggers that it's likely to be ENOENT, but certainly if it _isn't_\n> that would be interesting information.\n\nENOTDIR and to a lesser degree EACCES and ELOOP are also\nuninteresting, as we are talking about somebody else mucking with\nthe filesystem.\n\nTo tie the loose end, here is what will be queued and merged to\n'next' soonish.\n\nThanks.\n\n-- >8 --\nFrom: Lars Schneider \nDate: Thu, 5 Oct 2017 12:44:07 +0200\nSubject: [PATCH] entry.c: check if file exists after checkout\n\nIf we are checking out a file and somebody else racily deletes our file,\nthen we would write garbage to the cache entry. Fix that by checking\nthe result of the lstat() call on that file. Print an error to the user\nif the file does not exist.\n\nReported-by: Jeff King \nSigned-off-by: Lars Schneider \nSigned-off-by: Junio C Hamano \n---\n entry.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/entry.c b/entry.c\nindex f879758c73..6d9de3a5aa 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -341,7 +341,9 @@ static int write_entry(struct cache_entry *ce,\n \tif (state->refresh_cache) {\n \t\tassert(state->istate);\n \t\tif (!fstat_done)\n-\t\t\tlstat(ce->name, &st);\n+\t\t\tif (lstat(ce->name, &st) < 0)\n+\t\t\t\treturn error_errno(\"unable stat just-written file %s\",\n+\t\t\t\t\t\t ce->name);\n \t\tfill_stat_cache_info(ce, &st);\n \t\tce->ce_flags |= CE_UPDATE_IN_BASE;\n \t\tstate->istate->cache_changed |= CE_ENTRY_CHANGED;\n-- \n2.15.0-rc0-155-g07e9c1a78d\n\n"},{"id":"329848","messageId":"20171006045440.2imc2c7hvu5d3hdk@sigill.intra.peff.net","threadId":"46909","inReplyTo":"xmqqefqh6vxf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 1/2] entry.c: update cache entry only for existing files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-06T04:54:40Z","receivedAt":"2017-10-06T04:54:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 06, 2017 at 08:01:48AM +0900, Junio C Hamano wrote:\n\n> > But\n> > I think we'd want to protect the read_blob_entry() call at the top of\n> > the case with a check for dco->state == CE_RETRY.\n> \n> Yeah, I think that makes more sense.\n> \n> A patch may look like this on top of these two patches, but I'd\n> prefer to see Lars's eyeballing and possibly wrapping it up in an\n> applicable patch after taking the authorship.\n\nYeah, agreed.\n\n> I considered initializing new to NULL and size to 0 but decided\n> against it, as that would lose the justification to have an if\n> statement that marks that \"dco->state == CE_RETRY\" is a special\n> case. I think explicit if() with clearing these two variables makes\n> it clearer to show what is going on.\n\nAlso agreed.\n\n> By the way, the S_IFLNK handling seems iffy with or without this\n> change (or for that matter, I suspect this iffy-ness existed before\n> Lars's delayed filtering change). On a platform without symlinks,\n> we do the same as S_IFREG, but obviously we do not want any content\n> conversion that happens to the regular files in such a case. So we\n> may further want to fix that, but I left it outside the scope of\n> fixing the leak of NULL and optimizing the blob reading out.\n\nI think the current code is correct because the conversion all happens\nin the S_IFREG if-block. We just fall-through down to the actual write\nphase for the symlink case.\n\nThat said, I found the fall-through here confusing as hell. I actually\nthink it would be a lot clearer with a goto, which is saying something.\nHere's the \"diff -w\" of what I mean, for readability (the real patch is\nat the bottom for reference, but it adjusts the indentation quite a\nbit).\n\ndiff --git a/entry.c b/entry.c\nindex 1c7e3c11d5..d28b42d82d 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -261,6 +261,7 @@ static int write_entry(struct cache_entry *ce,\n \tsize_t newsize = 0;\n \tstruct stat st;\n \tconst struct submodule *sub;\n+\tstruct delayed_checkout *dco = state->delayed_checkout;\n \n \tif (ce_mode_s_ifmt == S_IFREG) {\n \t\tstruct stream_filter *filter = get_stream_filter(ce->name,\n@@ -273,33 +274,39 @@ static int write_entry(struct cache_entry *ce,\n \t}\n \n \tswitch (ce_mode_s_ifmt) {\n-\tcase S_IFREG:\n \tcase S_IFLNK:\n \t\tnew = read_blob_entry(ce, &size);\n \t\tif (!new)\n \t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n \t\t\t\tpath, oid_to_hex(&ce->oid));\n \n-\t\tif (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) {\n+\t\t/* fallback to handling it like a regular file if we must */\n+\t\tif (!has_symlinks || to_tempfile)\n+\t\t\tgoto write_out_file;\n+\n \t\tret = symlink(new, path);\n \t\tfree(new);\n \t\tif (ret)\n \t\t\treturn error_errno(\"unable to create symlink %s\",\n \t\t\t\t\t path);\n \t\tbreak;\n-\t\t}\n \n+\tcase S_IFREG:\n \t\t/*\n \t\t * Convert from git internal format to working tree format\n \t\t */\n-\t\tif (ce_mode_s_ifmt == S_IFREG) {\n-\t\t\tstruct delayed_checkout *dco = state->delayed_checkout;\n-\t\t\tif (dco && dco->state != CE_NO_DELAY) {\n-\t\t\t\t/* Do not send the blob in case of a retry. */\n-\t\t\t\tif (dco->state == CE_RETRY) {\n+\n+\t\tif (dco && dco->state == CE_RETRY) {\n \t\t\tnew = NULL;\n \t\t\tsize = 0;\n+\t\t} else {\n+\t\t\tnew = read_blob_entry(ce, &size);\n+\t\t\tif (!new)\n+\t\t\t\treturn error (\"unable to read sha1 file of %s (%s)\",\n+\t\t\t\t\t path, oid_to_hex(&ce->oid));\n \t\t}\n+\n+\t\tif (dco && dco->state != CE_NO_DELAY) {\n \t\t\tret = async_convert_to_working_tree(\n \t\t\t\t\t\t\t ce->name, new, size, &buf, dco);\n \t\t\tif (ret && string_list_has_string(&dco->paths, ce->name)) {\n@@ -320,8 +327,8 @@ static int write_entry(struct cache_entry *ce,\n \t\t * point. If the error would have been fatal (e.g.\n \t\t * filter is required), then we would have died already.\n \t\t */\n-\t\t}\n \n+write_out_file:\n \t\tfd = open_output_fd(path, ce, to_tempfile);\n \t\tif (fd < 0) {\n \t\t\tfree(new);\n\nThe \"structured\" way, of course, would be to put everything under\nwrite_out_file into a helper function and just call it from both places\nrather than relying on a spaghetti of gotos and switch-breaks.\n\nI'm OK with whatever structure we end up with, as long as it fixes the\nleak (and ideally the pessimization).\n\nAnyway, here's the real patch in case anybody wants to apply it and play\nwith it further.\n\n-- >8 --\ndiff --git a/entry.c b/entry.c\nindex 1c7e3c11d5..d28b42d82d 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -261,6 +261,7 @@ static int write_entry(struct cache_entry *ce,\n \tsize_t newsize = 0;\n \tstruct stat st;\n \tconst struct submodule *sub;\n+\tstruct delayed_checkout *dco = state->delayed_checkout;\n \n \tif (ce_mode_s_ifmt == S_IFREG) {\n \t\tstruct stream_filter *filter = get_stream_filter(ce->name,\n@@ -273,55 +274,61 @@ static int write_entry(struct cache_entry *ce,\n \t}\n \n \tswitch (ce_mode_s_ifmt) {\n-\tcase S_IFREG:\n \tcase S_IFLNK:\n \t\tnew = read_blob_entry(ce, &size);\n \t\tif (!new)\n \t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n \t\t\t\tpath, oid_to_hex(&ce->oid));\n \n-\t\tif (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) {\n-\t\t\tret = symlink(new, path);\n-\t\t\tfree(new);\n-\t\t\tif (ret)\n-\t\t\t\treturn error_errno(\"unable to create symlink %s\",\n-\t\t\t\t\t\t path);\n-\t\t\tbreak;\n-\t\t}\n+\t\t/* fallback to handling it like a regular file if we must */\n+\t\tif (!has_symlinks || to_tempfile)\n+\t\t\tgoto write_out_file;\n \n+\t\tret = symlink(new, path);\n+\t\tfree(new);\n+\t\tif (ret)\n+\t\t\treturn error_errno(\"unable to create symlink %s\",\n+\t\t\t\t\t path);\n+\t\tbreak;\n+\n+\tcase S_IFREG:\n \t\t/*\n \t\t * Convert from git internal format to working tree format\n \t\t */\n-\t\tif (ce_mode_s_ifmt == S_IFREG) {\n-\t\t\tstruct delayed_checkout *dco = state->delayed_checkout;\n-\t\t\tif (dco && dco->state != CE_NO_DELAY) {\n-\t\t\t\t/* Do not send the blob in case of a retry. */\n-\t\t\t\tif (dco->state == CE_RETRY) {\n-\t\t\t\t\tnew = NULL;\n-\t\t\t\t\tsize = 0;\n-\t\t\t\t}\n-\t\t\t\tret = async_convert_to_working_tree(\n-\t\t\t\t\tce->name, new, size, &buf, dco);\n-\t\t\t\tif (ret && string_list_has_string(&dco->paths, ce->name)) {\n-\t\t\t\t\tfree(new);\n-\t\t\t\t\tgoto finish;\n-\t\t\t\t}\n-\t\t\t} else\n-\t\t\t\tret = convert_to_working_tree(\n-\t\t\t\t\tce->name, new, size, &buf);\n \n-\t\t\tif (ret) {\n+\t\tif (dco && dco->state == CE_RETRY) {\n+\t\t\tnew = NULL;\n+\t\t\tsize = 0;\n+\t\t} else {\n+\t\t\tnew = read_blob_entry(ce, &size);\n+\t\t\tif (!new)\n+\t\t\t\treturn error (\"unable to read sha1 file of %s (%s)\",\n+\t\t\t\t\t path, oid_to_hex(&ce->oid));\n+\t\t}\n+\n+\t\tif (dco && dco->state != CE_NO_DELAY) {\n+\t\t\tret = async_convert_to_working_tree(\n+\t\t\t\t\t\t\t ce->name, new, size, &buf, dco);\n+\t\t\tif (ret && string_list_has_string(&dco->paths, ce->name)) {\n \t\t\t\tfree(new);\n-\t\t\t\tnew = strbuf_detach(&buf, &newsize);\n-\t\t\t\tsize = newsize;\n+\t\t\t\tgoto finish;\n \t\t\t}\n-\t\t\t/*\n-\t\t\t * No \"else\" here as errors from convert are OK at this\n-\t\t\t * point. If the error would have been fatal (e.g.\n-\t\t\t * filter is required), then we would have died already.\n-\t\t\t */\n+\t\t} else\n+\t\t\tret = convert_to_working_tree(\n+\t\t\t\t\t\t ce->name, new, size, &buf);\n+\n+\t\tif (ret) {\n+\t\t\tfree(new);\n+\t\t\tnew = strbuf_detach(&buf, &newsize);\n+\t\t\tsize = newsize;\n \t\t}\n+\t\t/*\n+\t\t * No \"else\" here as errors from convert are OK at this\n+\t\t * point. If the error would have been fatal (e.g.\n+\t\t * filter is required), then we would have died already.\n+\t\t */\n \n+write_out_file:\n \t\tfd = open_output_fd(path, ce, to_tempfile);\n \t\tif (fd < 0) {\n \t\t\tfree(new);\n"},{"id":"329849","messageId":"20171006045640.vihagnlnuximzmjs@sigill.intra.peff.net","threadId":"46909","inReplyTo":"xmqqlgkoyk8n.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 2/2] entry.c: check if file exists after checkout","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-06T04:56:40Z","receivedAt":"2017-10-06T04:56:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 06, 2017 at 01:26:48PM +0900, Junio C Hamano wrote:\n\n> > We could probably be a bit more specific about the situation, since the\n> > user will see this message with no context. Maybe something like:\n> >\n> > unable to stat just-written file %s\n> >\n> > or something. We should probably also use error_errno(). I'd bet if this\n> > ever triggers that it's likely to be ENOENT, but certainly if it _isn't_\n> > that would be interesting information.\n> \n> ENOTDIR and to a lesser degree EACCES and ELOOP are also\n> uninteresting, as we are talking about somebody else mucking with\n> the filesystem.\n\nTrue. The nice thing about the error() route is that we don't need to\nmake such judgements. The user can decide what is unexpected.\n\n> -- >8 --\n> From: Lars Schneider \n> Date: Thu, 5 Oct 2017 12:44:07 +0200\n> Subject: [PATCH] entry.c: check if file exists after checkout\n> \n> If we are checking out a file and somebody else racily deletes our file,\n> then we would write garbage to the cache entry. Fix that by checking\n> the result of the lstat() call on that file. Print an error to the user\n> if the file does not exist.\n\nI don't know if we wanted to capture any of the reasoning behind using\nerror() here or not. Frankly, I'm not sure how to argue for it\nsuccinctly. :) I'm happy with letting it live on in the list archive.\n\n> diff --git a/entry.c b/entry.c\n> index f879758c73..6d9de3a5aa 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -341,7 +341,9 @@ static int write_entry(struct cache_entry *ce,\n> \tif (state->refresh_cache) {\n> \t\tassert(state->istate);\n> \t\tif (!fstat_done)\n> -\t\t\tlstat(ce->name, &st);\n> +\t\t\tif (lstat(ce->name, &st) < 0)\n> +\t\t\t\treturn error_errno(\"unable stat just-written file %s\",\n> +\t\t\t\t\t\t ce->name);\n\ns/unable stat/unable to stat/, I think.\n\nOther than that, this looks fine to me.\n\n-Peff\n"},{"id":"329854","messageId":"xmqqd160x16i.fsf@gitster.mtv.corp.google.com","threadId":"46909","inReplyTo":"20171006045640.vihagnlnuximzmjs@sigill.intra.peff.net","subject":"Re: [PATCH v1 2/2] entry.c: check if file exists after checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-06T06:03:49Z","receivedAt":"2017-10-06T06:03:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I don't know if we wanted to capture any of the reasoning behind using\n> error() here or not. Frankly, I'm not sure how to argue for it\n> succinctly. :) I'm happy with letting it live on in the list archive.\n\nAre you talking about the \"philosophical\" thing? \n\nBecause we cannot quite tell between the two cases (one is error--we\nwrote or we thought we wrote, but we cannot find it, the other is\ndubious--somebody was racing with us in the filesystem), I think it\nis reasonable to err on the safer side, even though an error abort\nwhile doing \"as we know we wrote the thing that match the index, we\nmight as well lstat and mark the cache entry as up-to-date\" might be\na bit irritating.\n\n\n>> diff --git a/entry.c b/entry.c\n>> index f879758c73..6d9de3a5aa 100644\n>> --- a/entry.c\n>> +++ b/entry.c\n>> @@ -341,7 +341,9 @@ static int write_entry(struct cache_entry *ce,\n>> \tif (state->refresh_cache) {\n>> \t\tassert(state->istate);\n>> \t\tif (!fstat_done)\n>> -\t\t\tlstat(ce->name, &st);\n>> +\t\t\tif (lstat(ce->name, &st) < 0)\n>> +\t\t\t\treturn error_errno(\"unable stat just-written file %s\",\n>> +\t\t\t\t\t\t ce->name);\n>\n> s/unable stat/unable to stat/, I think.\n\nThanks.\n"},{"id":"329855","messageId":"20171006060542.llx4golnkuxksy7z@sigill.intra.peff.net","threadId":"46909","inReplyTo":"xmqqd160x16i.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1 2/2] entry.c: check if file exists after checkout","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-06T06:05:43Z","receivedAt":"2017-10-06T06:05:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 06, 2017 at 03:03:49PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I don't know if we wanted to capture any of the reasoning behind using\n> > error() here or not. Frankly, I'm not sure how to argue for it\n> > succinctly. :) I'm happy with letting it live on in the list archive.\n> \n> Are you talking about the \"philosophical\" thing?\n\nRight, whether we ought to just mark the entry as stat-dirty and return\nsuccess.\n\n> Because we cannot quite tell between the two cases (one is error--we\n> wrote or we thought we wrote, but we cannot find it, the other is\n> dubious--somebody was racing with us in the filesystem), I think it\n> is reasonable to err on the safer side, even though an error abort\n> while doing \"as we know we wrote the thing that match the index, we\n> might as well lstat and mark the cache entry as up-to-date\" might be\n> a bit irritating.\n\nOK. I can live with that line of thought.\n\n-Peff\n"},{"id":"329859","messageId":"CAPc5daU4K3=-qhOiMx73oyCX7tGCv87L4uB9pu3M2C90s2sLMg@mail.gmail.com","threadId":"46909","inReplyTo":"20171006060542.llx4golnkuxksy7z@sigill.intra.peff.net","subject":"Re: [PATCH v1 2/2] entry.c: check if file exists after checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-06T07:58:49Z","receivedAt":"2017-10-06T07:59:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Fri, Oct 6, 2017 at 3:05 PM, Jeff King wrote:\n>\n>> Because we cannot quite tell between the two cases (one is error--we\n>> wrote or we thought we wrote, but we cannot find it, the other is\n>> dubious--somebody was racing with us in the filesystem), I think it\n>> is reasonable to err on the safer side, even though an error abort\n>> while doing \"as we know we wrote the thing that match the index, we\n>> might as well lstat and mark the cache entry as up-to-date\" might be\n>> a bit irritating.\n>\n> OK. I can live with that line of thought.\n\nStill that, or any other, line of thought we follow to declare that it\nis a good change should be recorded in the log ;-)\n"},{"id":"330004","messageId":"FC9D1B20-C056-4739-9FE3-692CA75FC128@gmail.com","threadId":"46909","inReplyTo":"20171006045440.2imc2c7hvu5d3hdk@sigill.intra.peff.net","subject":"Re: [PATCH v1 1/2] entry.c: update cache entry only for existing files","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-10-08T21:37:14Z","receivedAt":"2017-10-08T21:37:22Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 06 Oct 2017, at 06:54, Jeff King wrote:\n> \n> On Fri, Oct 06, 2017 at 08:01:48AM +0900, Junio C Hamano wrote:\n> \n>>> But\n>>> I think we'd want to protect the read_blob_entry() call at the top of\n>>> the case with a check for dco->state == CE_RETRY.\n>> \n>> Yeah, I think that makes more sense.\n>> \n>> A patch may look like this on top of these two patches, but I'd\n>> prefer to see Lars's eyeballing and possibly wrapping it up in an\n>> applicable patch after taking the authorship.\n> \n\nThis looks all good to me. Thank you!\nA few minor style suggestions below.\n\n\n> ...\n> \n> The \"structured\" way, of course, would be to put everything under\n> write_out_file into a helper function and just call it from both places\n> rather than relying on a spaghetti of gotos and switch-breaks.\n> \n> I'm OK with whatever structure we end up with, as long as it fixes the\n> leak (and ideally the pessimization).\n> \n> Anyway, here's the real patch in case anybody wants to apply it and play\n> with it further.\n> \n> -- >8 --\n> diff --git a/entry.c b/entry.c\n> index 1c7e3c11d5..d28b42d82d 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -261,6 +261,7 @@ static int write_entry(struct cache_entry *ce,\n> \tsize_t newsize = 0;\n> \tstruct stat st;\n> \tconst struct submodule *sub;\n> +\tstruct delayed_checkout *dco = state->delayed_checkout;\n> \n> \tif (ce_mode_s_ifmt == S_IFREG) {\n> \t\tstruct stream_filter *filter = get_stream_filter(ce->name,\n> @@ -273,55 +274,61 @@ static int write_entry(struct cache_entry *ce,\n> \t}\n> \n> \tswitch (ce_mode_s_ifmt) {\n> -\tcase S_IFREG:\n> \tcase S_IFLNK:\n> \t\tnew = read_blob_entry(ce, &size);\n> \t\tif (!new)\n> \t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n> \t\t\t\tpath, oid_to_hex(&ce->oid));\n> \n> -\t\tif (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) {\n> -\t\t\tret = symlink(new, path);\n> -\t\t\tfree(new);\n> -\t\t\tif (ret)\n> -\t\t\t\treturn error_errno(\"unable to create symlink %s\",\n> -\t\t\t\t\t\t path);\n\nNit: This could go into one line now.\n\n\n> -\t\t\tbreak;\n> -\t\t}\n> +\t\t/* fallback to handling it like a regular file if we must */\n> +\t\tif (!has_symlinks || to_tempfile)\n> +\t\t\tgoto write_out_file;\n> \n> +\t\tret = symlink(new, path);\n> +\t\tfree(new);\n> +\t\tif (ret)\n> +\t\t\treturn error_errno(\"unable to create symlink %s\",\n> +\t\t\t\t\t path);\n> +\t\tbreak;\n> +\n> +\tcase S_IFREG:\n> \t\t/*\n> \t\t * Convert from git internal format to working tree format\n> \t\t */\n> -\t\tif (ce_mode_s_ifmt == S_IFREG) {\n> -\t\t\tstruct delayed_checkout *dco = state->delayed_checkout;\n> -\t\t\tif (dco && dco->state != CE_NO_DELAY) {\n> -\t\t\t\t/* Do not send the blob in case of a retry. */\n> -\t\t\t\tif (dco->state == CE_RETRY) {\n\nMaybe we could add here something like:\n /* The filer process got the blob already in case of a retry. Unnecessary to send it, again! */\n\n> -\t\t\t\t\tnew = NULL;\n> -\t\t\t\t\tsize = 0;\n> -\t\t\t\t}\n> -\t\t\t\tret = async_convert_to_working_tree(\n> -\t\t\t\t\tce->name, new, size, &buf, dco);\n\nNit: This could go into one line now.\n\n\n> -\t\t\t\tif (ret && string_list_has_string(&dco->paths, ce->name)) {\n> -\t\t\t\t\tfree(new);\n> -\t\t\t\t\tgoto finish;\n> -\t\t\t\t}\n> -\t\t\t} else\n> -\t\t\t\tret = convert_to_working_tree(\n> -\t\t\t\t\tce->name, new, size, &buf);\n\nNit: This could go into one line now.\n\n\n> \n> -\t\t\tif (ret) {\n> +\t\tif (dco && dco->state == CE_RETRY) {\n> +\t\t\tnew = NULL;\n> +\t\t\tsize = 0;\n> +\t\t} else {\n> +\t\t\tnew = read_blob_entry(ce, &size);\n> +\t\t\tif (!new)\n> +\t\t\t\treturn error (\"unable to read sha1 file of %s (%s)\",\n> +\t\t\t\t\t path, oid_to_hex(&ce->oid));\n> +\t\t}\n> +\n> +\t\tif (dco && dco->state != CE_NO_DELAY) {\n> +\t\t\tret = async_convert_to_working_tree(\n> +\t\t\t\t\t\t\t ce->name, new, size, &buf, dco);\n> +\t\t\tif (ret && string_list_has_string(&dco->paths, ce->name)) {\n> \t\t\t\tfree(new);\n> -\t\t\t\tnew = strbuf_detach(&buf, &newsize);\n> -\t\t\t\tsize = newsize;\n> +\t\t\t\tgoto finish;\n> \t\t\t}\n> -\t\t\t/*\n> -\t\t\t * No \"else\" here as errors from convert are OK at this\n> -\t\t\t * point. If the error would have been fatal (e.g.\n> -\t\t\t * filter is required), then we would have died already.\n> -\t\t\t */\n> +\t\t} else\n> +\t\t\tret = convert_to_working_tree(\n> +\t\t\t\t\t\t ce->name, new, size, &buf);\n> +\n> +\t\tif (ret) {\n> +\t\t\tfree(new);\n> +\t\t\tnew = strbuf_detach(&buf, &newsize);\n> +\t\t\tsize = newsize;\n> \t\t}\n> +\t\t/*\n> +\t\t * No \"else\" here as errors from convert are OK at this\n> +\t\t * point. If the error would have been fatal (e.g.\n> +\t\t * filter is required), then we would have died already.\n> +\t\t */\n> \n> +write_out_file:\n> \t\tfd = open_output_fd(path, ce, to_tempfile);\n> \t\tif (fd < 0) {\n> \t\t\tfree(new);\n\n...\n\n>\t\tbreak;\n>\tcase S_IFGITLINK:\n\nMaybe add a newline above \"S_IFGITLINK\" and \"default\" for readability. \nAbove \"case S_IFREG:\" we have a newline, too. \n\n\n- Lars\n\n\n\n\n\n\n"},{"id":"330005","messageId":"C7B91A82-CB55-4435-9204-D6A8F7F640CB@gmail.com","threadId":"46909","inReplyTo":"20171006045640.vihagnlnuximzmjs@sigill.intra.peff.net","subject":"Re: [PATCH v1 2/2] entry.c: check if file exists after checkout","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2017-10-08T21:41:13Z","receivedAt":"2017-10-08T21:41:20Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\n> On 06 Oct 2017, at 06:56, Jeff King wrote:\n> \n> On Fri, Oct 06, 2017 at 01:26:48PM +0900, Junio C Hamano wrote:\n> \n> ...\n>> -- >8 --\n>> From: Lars Schneider \n>> Date: Thu, 5 Oct 2017 12:44:07 +0200\n>> Subject: [PATCH] entry.c: check if file exists after checkout\n>> \n>> If we are checking out a file and somebody else racily deletes our file,\n>> then we would write garbage to the cache entry. Fix that by checking\n>> the result of the lstat() call on that file. Print an error to the user\n>> if the file does not exist.\n> \n> I don't know if we wanted to capture any of the reasoning behind using\n> error() here or not. Frankly, I'm not sure how to argue for it\n> succinctly. :) I'm happy with letting it live on in the list archive.\n> \n>> diff --git a/entry.c b/entry.c\n>> index f879758c73..6d9de3a5aa 100644\n>> --- a/entry.c\n>> +++ b/entry.c\n>> @@ -341,7 +341,9 @@ static int write_entry(struct cache_entry *ce,\n>> \tif (state->refresh_cache) {\n>> \t\tassert(state->istate);\n>> \t\tif (!fstat_done)\n>> -\t\t\tlstat(ce->name, &st);\n>> +\t\t\tif (lstat(ce->name, &st) < 0)\n>> +\t\t\t\treturn error_errno(\"unable stat just-written file %s\",\n>> +\t\t\t\t\t\t ce->name);\n> \n> s/unable stat/unable to stat/, I think.\n> \n> Other than that, this looks fine to me.\n> \n> -Peff\n\nLooks fine to me, too.\n\nThanks,\nLars\n"},{"id":"330041","messageId":"20171009174715.a6wziu6w535u6rd2@sigill.intra.peff.net","threadId":"46909","inReplyTo":"FC9D1B20-C056-4739-9FE3-692CA75FC128@gmail.com","subject":"Re: [PATCH v1 1/2] entry.c: update cache entry only for existing files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-09T17:47:15Z","receivedAt":"2017-10-09T17:47:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 08, 2017 at 11:37:14PM +0200, Lars Schneider wrote:\n\n> >> Yeah, I think that makes more sense.\n> >> \n> >> A patch may look like this on top of these two patches, but I'd\n> >> prefer to see Lars's eyeballing and possibly wrapping it up in an\n> >> applicable patch after taking the authorship.\n> > \n> \n> This looks all good to me. Thank you!\n> A few minor style suggestions below.\n\nThanks, these were all reasonable (I actually avoided unwrapping some of\nthe lines in the original to make the \"-w\" diff more readable :) ).\n\nI ended up breaking this into three commits, since I think that makes it\neasier to see what each of the changes is doing. Here's what I have (on\ntop of what Junio has already queued in ls/filter-process-delayed):\n\n [v2 1/3]: write_entry: fix leak when retrying delayed filter\n [v2 2/3]: write_entry: avoid reading blobs in CE_RETRY case\n [v2 3/3]: write_entry: untangle symlink and regular-file cases\n\n entry.c | 83 ++++++++++++++++++++++++++++++++++++++---------------------------\n 1 file changed, 48 insertions(+), 35 deletions(-)\n\n-Peff\n"},{"id":"330042","messageId":"20171009174824.tt5tpxdvcvzbyvnl@sigill.intra.peff.net","threadId":"46909","inReplyTo":"20171009174715.a6wziu6w535u6rd2@sigill.intra.peff.net","subject":"[PATCH 1/3] write_entry: fix leak when retrying delayed filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-09T17:48:24Z","receivedAt":"2017-10-09T17:48:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When write_entry() retries a delayed filter request, we\ndon't need to send the blob content to the filter again, and\nset the pointer to NULL. But doing so means we leak the\ncontents we read earlier from read_blob_entry(). Let's make\nsure to free it before dropping the pointer.\n\nSigned-off-by: Jeff King \n---\n entry.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/entry.c b/entry.c\nindex ab79f1f69c..637c5958b0 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -283,6 +283,7 @@ static int write_entry(struct cache_entry *ce,\n \t\t\tif (dco && dco->state != CE_NO_DELAY) {\n \t\t\t\t/* Do not send the blob in case of a retry. */\n \t\t\t\tif (dco->state == CE_RETRY) {\n+\t\t\t\t\tfree(new);\n \t\t\t\t\tnew = NULL;\n \t\t\t\t\tsize = 0;\n \t\t\t\t}\n-- \n2.15.0.rc0.421.gf5a676fd56\n\n"},{"id":"330043","messageId":"20171009174852.32dpy5xh3w3bfn6t@sigill.intra.peff.net","threadId":"46909","inReplyTo":"20171009174715.a6wziu6w535u6rd2@sigill.intra.peff.net","subject":"[PATCH 2/3] write_entry: avoid reading blobs in CE_RETRY case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-09T17:48:52Z","receivedAt":"2017-10-09T17:48:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When retrying a delayed filter-process request, we don't\nneed to send the blob to the filter a second time. However,\nwe read it unconditionally into a buffer, only to later\nthrow away that buffer. We can make this more efficient by\nskipping the read in the first place when it isn't\nnecessary.\n\nSigned-off-by: Jeff King \n---\n entry.c | 25 ++++++++++++++-----------\n 1 file changed, 14 insertions(+), 11 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex 637c5958b0..bec51e37a2 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -240,6 +240,7 @@ static int write_entry(struct cache_entry *ce,\n \t\t char *path, const struct checkout *state, int to_tempfile)\n {\n \tunsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;\n+\tstruct delayed_checkout *dco = state->delayed_checkout;\n \tint fd, ret, fstat_done = 0;\n \tchar *new;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -261,10 +262,19 @@ static int write_entry(struct cache_entry *ce,\n \tswitch (ce_mode_s_ifmt) {\n \tcase S_IFREG:\n \tcase S_IFLNK:\n-\t\tnew = read_blob_entry(ce, &size);\n-\t\tif (!new)\n-\t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n-\t\t\t\tpath, oid_to_hex(&ce->oid));\n+\t\t/*\n+\t\t * We do not send the blob in case of a retry, so do not\n+\t\t * bother reading it at all.\n+\t\t */\n+\t\tif (ce_mode_s_ifmt == S_IFREG && dco && dco->state == CE_RETRY) {\n+\t\t\tnew = NULL;\n+\t\t\tsize = 0;\n+\t\t} else {\n+\t\t\tnew = read_blob_entry(ce, &size);\n+\t\t\tif (!new)\n+\t\t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n+\t\t\t\t\t path, oid_to_hex(&ce->oid));\n+\t\t}\n \n \t\tif (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) {\n \t\t\tret = symlink(new, path);\n@@ -279,14 +289,7 @@ static int write_entry(struct cache_entry *ce,\n \t\t * Convert from git internal format to working tree format\n \t\t */\n \t\tif (ce_mode_s_ifmt == S_IFREG) {\n-\t\t\tstruct delayed_checkout *dco = state->delayed_checkout;\n \t\t\tif (dco && dco->state != CE_NO_DELAY) {\n-\t\t\t\t/* Do not send the blob in case of a retry. */\n-\t\t\t\tif (dco->state == CE_RETRY) {\n-\t\t\t\t\tfree(new);\n-\t\t\t\t\tnew = NULL;\n-\t\t\t\t\tsize = 0;\n-\t\t\t\t}\n \t\t\t\tret = async_convert_to_working_tree(\n \t\t\t\t\tce->name, new, size, &buf, dco);\n \t\t\t\tif (ret && string_list_has_string(&dco->paths, ce->name)) {\n-- \n2.15.0.rc0.421.gf5a676fd56\n\n"},{"id":"330044","messageId":"20171009175004.75hlgsfyvupe5z3p@sigill.intra.peff.net","threadId":"46909","inReplyTo":"20171009174715.a6wziu6w535u6rd2@sigill.intra.peff.net","subject":"[PATCH 3/3] write_entry: untangle symlink and regular-file cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-09T17:50:05Z","receivedAt":"2017-10-09T17:50:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The write_entry() function switches on the mode of the entry\nwe're going to write out. The cases for S_IFLNK and S_IFREG\nare lumped together. In earlier versions of the code, this\nmade some sense. They have a shared preamble (which reads\nthe blob content), a short type-specific body, and a shared\nconclusion (which writes out the file contents; always for\nS_IFREG and only sometimes for S_IFLNK).\n\nBut over time this has grown to make less sense. The preamble\nnow has conditional bits for each type, and the S_IFREG body\nhas grown a lot more complicated. It's hard to follow the\nlogic of which code is running for which mode.\n\nLet's give each mode its own case arm. We will still share\nthe conclusion code, which means we now jump to it with a\ngoto. Ideally we'd pull that shared code into its own\nfunction, but it touches so much internal state in the\nwrite_entry() function that the end result is actually\nharder to follow than the goto.\n\nWhile we're here, we'll touch up a few bits of whitespace to\nmake the beginning and endings of the cases easier to read.\n\nSigned-off-by: Jeff King \n---\n entry.c | 71 +++++++++++++++++++++++++++++++++++++----------------------------\n 1 file changed, 40 insertions(+), 31 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex bec51e37a2..206363fd15 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -260,13 +260,31 @@ static int write_entry(struct cache_entry *ce,\n \t}\n \n \tswitch (ce_mode_s_ifmt) {\n-\tcase S_IFREG:\n \tcase S_IFLNK:\n+\t\tnew = read_blob_entry(ce, &size);\n+\t\tif (!new)\n+\t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n+\t\t\t\t path, oid_to_hex(&ce->oid));\n+\n+\t\t/*\n+\t\t * We can't make a real symlink; write out a regular file entry\n+\t\t * with the symlink destination as its contents.\n+\t\t */\n+\t\tif (!has_symlinks || to_tempfile)\n+\t\t\tgoto write_file_entry;\n+\n+\t\tret = symlink(new, path);\n+\t\tfree(new);\n+\t\tif (ret)\n+\t\t\treturn error_errno(\"unable to create symlink %s\", path);\n+\t\tbreak;\n+\n+\tcase S_IFREG:\n \t\t/*\n \t\t * We do not send the blob in case of a retry, so do not\n \t\t * bother reading it at all.\n \t\t */\n-\t\tif (ce_mode_s_ifmt == S_IFREG && dco && dco->state == CE_RETRY) {\n+\t\tif (dco && dco->state == CE_RETRY) {\n \t\t\tnew = NULL;\n \t\t\tsize = 0;\n \t\t} else {\n@@ -276,42 +294,31 @@ static int write_entry(struct cache_entry *ce,\n \t\t\t\t\t path, oid_to_hex(&ce->oid));\n \t\t}\n \n-\t\tif (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) {\n-\t\t\tret = symlink(new, path);\n-\t\t\tfree(new);\n-\t\t\tif (ret)\n-\t\t\t\treturn error_errno(\"unable to create symlink %s\",\n-\t\t\t\t\t\t path);\n-\t\t\tbreak;\n-\t\t}\n-\n \t\t/*\n \t\t * Convert from git internal format to working tree format\n \t\t */\n-\t\tif (ce_mode_s_ifmt == S_IFREG) {\n-\t\t\tif (dco && dco->state != CE_NO_DELAY) {\n-\t\t\t\tret = async_convert_to_working_tree(\n-\t\t\t\t\tce->name, new, size, &buf, dco);\n-\t\t\t\tif (ret && string_list_has_string(&dco->paths, ce->name)) {\n-\t\t\t\t\tfree(new);\n-\t\t\t\t\tgoto delayed;\n-\t\t\t\t}\n-\t\t\t} else\n-\t\t\t\tret = convert_to_working_tree(\n-\t\t\t\t\tce->name, new, size, &buf);\n-\n-\t\t\tif (ret) {\n+\t\tif (dco && dco->state != CE_NO_DELAY) {\n+\t\t\tret = async_convert_to_working_tree(ce->name, new,\n+\t\t\t\t\t\t\t size, &buf, dco);\n+\t\t\tif (ret && string_list_has_string(&dco->paths, ce->name)) {\n \t\t\t\tfree(new);\n-\t\t\t\tnew = strbuf_detach(&buf, &newsize);\n-\t\t\t\tsize = newsize;\n+\t\t\t\tgoto delayed;\n \t\t\t}\n-\t\t\t/*\n-\t\t\t * No \"else\" here as errors from convert are OK at this\n-\t\t\t * point. If the error would have been fatal (e.g.\n-\t\t\t * filter is required), then we would have died already.\n-\t\t\t */\n+\t\t} else\n+\t\t\tret = convert_to_working_tree(ce->name, new, size, &buf);\n+\n+\t\tif (ret) {\n+\t\t\tfree(new);\n+\t\t\tnew = strbuf_detach(&buf, &newsize);\n+\t\t\tsize = newsize;\n \t\t}\n+\t\t/*\n+\t\t * No \"else\" here as errors from convert are OK at this\n+\t\t * point. If the error would have been fatal (e.g.\n+\t\t * filter is required), then we would have died already.\n+\t\t */\n \n+\twrite_file_entry:\n \t\tfd = open_output_fd(path, ce, to_tempfile);\n \t\tif (fd < 0) {\n \t\t\tfree(new);\n@@ -326,6 +333,7 @@ static int write_entry(struct cache_entry *ce,\n \t\tif (wrote != size)\n \t\t\treturn error(\"unable to write file %s\", path);\n \t\tbreak;\n+\n \tcase S_IFGITLINK:\n \t\tif (to_tempfile)\n \t\t\treturn error(\"cannot create temporary submodule %s\", path);\n@@ -337,6 +345,7 @@ static int write_entry(struct cache_entry *ce,\n \t\t\t\tNULL, oid_to_hex(&ce->oid),\n \t\t\t\tstate->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);\n \t\tbreak;\n+\n \tdefault:\n \t\treturn error(\"unknown file mode for %s in index\", path);\n \t}\n-- \n2.15.0.rc0.421.gf5a676fd56\n"},{"id":"330068","messageId":"xmqq376rswh1.fsf@gitster.mtv.corp.google.com","threadId":"46909","inReplyTo":"20171009174852.32dpy5xh3w3bfn6t@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] write_entry: avoid reading blobs in CE_RETRY case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-10T00:00:26Z","receivedAt":"2017-10-10T00:00:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When retrying a delayed filter-process request, we don't\n> need to send the blob to the filter a second time. However,\n> we read it unconditionally into a buffer, only to later\n> throw away that buffer. We can make this more efficient by\n> skipping the read in the first place when it isn't\n> necessary.\n>\n> Signed-off-by: Jeff King \n> ---\n> entry.c | 25 ++++++++++++++-----------\n> 1 file changed, 14 insertions(+), 11 deletions(-)\n\nAgain, looks obviously correct. Thanks.\n\n>\n> diff --git a/entry.c b/entry.c\n> index 637c5958b0..bec51e37a2 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -240,6 +240,7 @@ static int write_entry(struct cache_entry *ce,\n> \t\t char *path, const struct checkout *state, int to_tempfile)\n> {\n> \tunsigned int ce_mode_s_ifmt = ce->ce_mode & S_IFMT;\n> +\tstruct delayed_checkout *dco = state->delayed_checkout;\n> \tint fd, ret, fstat_done = 0;\n> \tchar *new;\n> \tstruct strbuf buf = STRBUF_INIT;\n> @@ -261,10 +262,19 @@ static int write_entry(struct cache_entry *ce,\n> \tswitch (ce_mode_s_ifmt) {\n> \tcase S_IFREG:\n> \tcase S_IFLNK:\n> -\t\tnew = read_blob_entry(ce, &size);\n> -\t\tif (!new)\n> -\t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n> -\t\t\t\tpath, oid_to_hex(&ce->oid));\n> +\t\t/*\n> +\t\t * We do not send the blob in case of a retry, so do not\n> +\t\t * bother reading it at all.\n> +\t\t */\n> +\t\tif (ce_mode_s_ifmt == S_IFREG && dco && dco->state == CE_RETRY) {\n> +\t\t\tnew = NULL;\n> +\t\t\tsize = 0;\n> +\t\t} else {\n> +\t\t\tnew = read_blob_entry(ce, &size);\n> +\t\t\tif (!new)\n> +\t\t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n> +\t\t\t\t\t path, oid_to_hex(&ce->oid));\n> +\t\t}\n> \n> \t\tif (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) {\n> \t\t\tret = symlink(new, path);\n> @@ -279,14 +289,7 @@ static int write_entry(struct cache_entry *ce,\n> \t\t * Convert from git internal format to working tree format\n> \t\t */\n> \t\tif (ce_mode_s_ifmt == S_IFREG) {\n> -\t\t\tstruct delayed_checkout *dco = state->delayed_checkout;\n> \t\t\tif (dco && dco->state != CE_NO_DELAY) {\n> -\t\t\t\t/* Do not send the blob in case of a retry. */\n> -\t\t\t\tif (dco->state == CE_RETRY) {\n> -\t\t\t\t\tfree(new);\n> -\t\t\t\t\tnew = NULL;\n> -\t\t\t\t\tsize = 0;\n> -\t\t\t\t}\n> \t\t\t\tret = async_convert_to_working_tree(\n> \t\t\t\t\tce->name, new, size, &buf, dco);\n> \t\t\t\tif (ret && string_list_has_string(&dco->paths, ce->name)) {\n"},{"id":"330069","messageId":"xmqq7ew3swhi.fsf@gitster.mtv.corp.google.com","threadId":"46909","inReplyTo":"20171009174824.tt5tpxdvcvzbyvnl@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] write_entry: fix leak when retrying delayed filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-10T00:00:09Z","receivedAt":"2017-10-10T00:00:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When write_entry() retries a delayed filter request, we\n> don't need to send the blob content to the filter again, and\n> set the pointer to NULL. But doing so means we leak the\n> contents we read earlier from read_blob_entry(). Let's make\n> sure to free it before dropping the pointer.\n>\n> Signed-off-by: Jeff King \n> ---\n> entry.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/entry.c b/entry.c\n> index ab79f1f69c..637c5958b0 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -283,6 +283,7 @@ static int write_entry(struct cache_entry *ce,\n> \t\t\tif (dco && dco->state != CE_NO_DELAY) {\n> \t\t\t\t/* Do not send the blob in case of a retry. */\n> \t\t\t\tif (dco->state == CE_RETRY) {\n> +\t\t\t\t\tfree(new);\n> \t\t\t\t\tnew = NULL;\n> \t\t\t\t\tsize = 0;\n> \t\t\t\t}\n\nLooks good to me. Thanks.\n"},{"id":"330070","messageId":"xmqqy3ojrhrb.fsf@gitster.mtv.corp.google.com","threadId":"46909","inReplyTo":"20171009175004.75hlgsfyvupe5z3p@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] write_entry: untangle symlink and regular-file cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-10T00:03:36Z","receivedAt":"2017-10-10T00:03:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The write_entry() function switches on the mode of the entry\n> we're going to write out. The cases for S_IFLNK and S_IFREG\n> are lumped together. In earlier versions of the code, this\n> made some sense. They have a shared preamble (which reads\n> the blob content), a short type-specific body, and a shared\n> conclusion (which writes out the file contents; always for\n> S_IFREG and only sometimes for S_IFLNK).\n>\n> But over time this has grown to make less sense. The preamble\n> now has conditional bits for each type, and the S_IFREG body\n> has grown a lot more complicated. It's hard to follow the\n> logic of which code is running for which mode.\n\nNicely done and looks correct. Thanks.\n\n>\n> Let's give each mode its own case arm. We will still share\n> the conclusion code, which means we now jump to it with a\n> goto. Ideally we'd pull that shared code into its own\n> function, but it touches so much internal state in the\n> write_entry() function that the end result is actually\n> harder to follow than the goto.\n\n>\n> While we're here, we'll touch up a few bits of whitespace to\n> make the beginning and endings of the cases easier to read.\n>\n> Signed-off-by: Jeff King \n> ---\n> entry.c | 71 +++++++++++++++++++++++++++++++++++++----------------------------\n> 1 file changed, 40 insertions(+), 31 deletions(-)\n>\n> diff --git a/entry.c b/entry.c\n> index bec51e37a2..206363fd15 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -260,13 +260,31 @@ static int write_entry(struct cache_entry *ce,\n> \t}\n> \n> \tswitch (ce_mode_s_ifmt) {\n> -\tcase S_IFREG:\n> \tcase S_IFLNK:\n> +\t\tnew = read_blob_entry(ce, &size);\n> +\t\tif (!new)\n> +\t\t\treturn error(\"unable to read sha1 file of %s (%s)\",\n> +\t\t\t\t path, oid_to_hex(&ce->oid));\n> +\n> +\t\t/*\n> +\t\t * We can't make a real symlink; write out a regular file entry\n> +\t\t * with the symlink destination as its contents.\n> +\t\t */\n> +\t\tif (!has_symlinks || to_tempfile)\n> +\t\t\tgoto write_file_entry;\n> +\n> +\t\tret = symlink(new, path);\n> +\t\tfree(new);\n> +\t\tif (ret)\n> +\t\t\treturn error_errno(\"unable to create symlink %s\", path);\n> +\t\tbreak;\n> +\n> +\tcase S_IFREG:\n> \t\t/*\n> \t\t * We do not send the blob in case of a retry, so do not\n> \t\t * bother reading it at all.\n> \t\t */\n> -\t\tif (ce_mode_s_ifmt == S_IFREG && dco && dco->state == CE_RETRY) {\n> +\t\tif (dco && dco->state == CE_RETRY) {\n> \t\t\tnew = NULL;\n> \t\t\tsize = 0;\n> \t\t} else {\n> @@ -276,42 +294,31 @@ static int write_entry(struct cache_entry *ce,\n> \t\t\t\t\t path, oid_to_hex(&ce->oid));\n> \t\t}\n> \n> -\t\tif (ce_mode_s_ifmt == S_IFLNK && has_symlinks && !to_tempfile) {\n> -\t\t\tret = symlink(new, path);\n> -\t\t\tfree(new);\n> -\t\t\tif (ret)\n> -\t\t\t\treturn error_errno(\"unable to create symlink %s\",\n> -\t\t\t\t\t\t path);\n> -\t\t\tbreak;\n> -\t\t}\n> -\n> \t\t/*\n> \t\t * Convert from git internal format to working tree format\n> \t\t */\n> -\t\tif (ce_mode_s_ifmt == S_IFREG) {\n> -\t\t\tif (dco && dco->state != CE_NO_DELAY) {\n> -\t\t\t\tret = async_convert_to_working_tree(\n> -\t\t\t\t\tce->name, new, size, &buf, dco);\n> -\t\t\t\tif (ret && string_list_has_string(&dco->paths, ce->name)) {\n> -\t\t\t\t\tfree(new);\n> -\t\t\t\t\tgoto delayed;\n> -\t\t\t\t}\n> -\t\t\t} else\n> -\t\t\t\tret = convert_to_working_tree(\n> -\t\t\t\t\tce->name, new, size, &buf);\n> -\n> -\t\t\tif (ret) {\n> +\t\tif (dco && dco->state != CE_NO_DELAY) {\n> +\t\t\tret = async_convert_to_working_tree(ce->name, new,\n> +\t\t\t\t\t\t\t size, &buf, dco);\n> +\t\t\tif (ret && string_list_has_string(&dco->paths, ce->name)) {\n> \t\t\t\tfree(new);\n> -\t\t\t\tnew = strbuf_detach(&buf, &newsize);\n> -\t\t\t\tsize = newsize;\n> +\t\t\t\tgoto delayed;\n> \t\t\t}\n> -\t\t\t/*\n> -\t\t\t * No \"else\" here as errors from convert are OK at this\n> -\t\t\t * point. If the error would have been fatal (e.g.\n> -\t\t\t * filter is required), then we would have died already.\n> -\t\t\t */\n> +\t\t} else\n> +\t\t\tret = convert_to_working_tree(ce->name, new, size, &buf);\n> +\n> +\t\tif (ret) {\n> +\t\t\tfree(new);\n> +\t\t\tnew = strbuf_detach(&buf, &newsize);\n> +\t\t\tsize = newsize;\n> \t\t}\n> +\t\t/*\n> +\t\t * No \"else\" here as errors from convert are OK at this\n> +\t\t * point. If the error would have been fatal (e.g.\n> +\t\t * filter is required), then we would have died already.\n> +\t\t */\n> \n> +\twrite_file_entry:\n> \t\tfd = open_output_fd(path, ce, to_tempfile);\n> \t\tif (fd < 0) {\n> \t\t\tfree(new);\n> @@ -326,6 +333,7 @@ static int write_entry(struct cache_entry *ce,\n> \t\tif (wrote != size)\n> \t\t\treturn error(\"unable to write file %s\", path);\n> \t\tbreak;\n> +\n> \tcase S_IFGITLINK:\n> \t\tif (to_tempfile)\n> \t\t\treturn error(\"cannot create temporary submodule %s\", path);\n> @@ -337,6 +345,7 @@ static int write_entry(struct cache_entry *ce,\n> \t\t\t\tNULL, oid_to_hex(&ce->oid),\n> \t\t\t\tstate->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);\n> \t\tbreak;\n> +\n> \tdefault:\n> \t\treturn error(\"unknown file mode for %s in index\", path);\n> \t}\n"},{"id":"330099","messageId":"20171010092319.ugim7ww7t6ks2vqf@ruderich.org","threadId":"46909","inReplyTo":"xmqq7ew3swhi.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] write_entry: fix leak when retrying delayed filter","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2017-10-10T09:23:19Z","receivedAt":"2017-10-10T09:23:25Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Tue, Oct 10, 2017 at 09:00:09AM +0900, Junio C Hamano wrote:\n>> --- a/entry.c\n>> +++ b/entry.c\n>> @@ -283,6 +283,7 @@ static int write_entry(struct cache_entry *ce,\n>> \t\t\tif (dco && dco->state != CE_NO_DELAY) {\n>> \t\t\t\t/* Do not send the blob in case of a retry. */\n>> \t\t\t\tif (dco->state == CE_RETRY) {\n>> +\t\t\t\t\tfree(new);\n>> \t\t\t\t\tnew = NULL;\n>> \t\t\t\t\tsize = 0;\n>> \t\t\t\t}\n\nFREE_AND_NULL(new)?\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"330100","messageId":"20171010092543.e4dh2lo4sj3w6w7j@sigill.intra.peff.net","threadId":"46909","inReplyTo":"20171010092319.ugim7ww7t6ks2vqf@ruderich.org","subject":"Re: [PATCH 1/3] write_entry: fix leak when retrying delayed filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-10T09:25:43Z","receivedAt":"2017-10-10T09:25:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 10, 2017 at 11:23:19AM +0200, Simon Ruderich wrote:\n\n> On Tue, Oct 10, 2017 at 09:00:09AM +0900, Junio C Hamano wrote:\n> >> --- a/entry.c\n> >> +++ b/entry.c\n> >> @@ -283,6 +283,7 @@ static int write_entry(struct cache_entry *ce,\n> >> \t\t\tif (dco && dco->state != CE_NO_DELAY) {\n> >> \t\t\t\t/* Do not send the blob in case of a retry. */\n> >> \t\t\t\tif (dco->state == CE_RETRY) {\n> >> +\t\t\t\t\tfree(new);\n> >> \t\t\t\t\tnew = NULL;\n> >> \t\t\t\t\tsize = 0;\n> >> \t\t\t\t}\n> \n> FREE_AND_NULL(new)?\n\nAh, yeah, I forgot we had that now. It would work here, but note that\nthis code ends up going away later in the series.\n\n-Peff\n"},{"id":"330102","messageId":"20171010094936.aivd63uispx6hks7@ruderich.org","threadId":"46909","inReplyTo":"20171010092543.e4dh2lo4sj3w6w7j@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] write_entry: fix leak when retrying delayed filter","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2017-10-10T09:49:36Z","receivedAt":"2017-10-10T09:49:42Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Tue, Oct 10, 2017 at 05:25:43AM -0400, Jeff King wrote:\n> On Tue, Oct 10, 2017 at 11:23:19AM +0200, Simon Ruderich wrote:\n>> On Tue, Oct 10, 2017 at 09:00:09AM +0900, Junio C Hamano wrote:\n>>>> --- a/entry.c\n>>>> +++ b/entry.c\n>>>> @@ -283,6 +283,7 @@ static int write_entry(struct cache_entry *ce,\n>>>> \t\t\tif (dco && dco->state != CE_NO_DELAY) {\n>>>> \t\t\t\t/* Do not send the blob in case of a retry. */\n>>>> \t\t\t\tif (dco->state == CE_RETRY) {\n>>>> +\t\t\t\t\tfree(new);\n>>>> \t\t\t\t\tnew = NULL;\n>>>> \t\t\t\t\tsize = 0;\n>>>> \t\t\t\t}\n>>\n>> FREE_AND_NULL(new)?\n>\n> Ah, yeah, I forgot we had that now. It would work here, but note that\n> this code ends up going away later in the series.\n\nAh sorry, missed that. Then never mind.\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"}]}