{"thread":{"id":"45808","subject":"[PATCH] read-cache: close index.lock in do_write_index","startedAt":"2017-04-26T20:05:38Z","lastAt":"2017-04-27T16:52:02Z","messageCount":5,"participants":["Johannes Schindelin","Jeff King","Junio C Hamano","Jeff Hostetler"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"317968","messageId":"e1b4f9c377ceee296112fa07bd06492a1de1be67.1493237111.git.johannes.schindelin@gmx.de","threadId":"45808","inReplyTo":null,"subject":"[PATCH] read-cache: close index.lock in do_write_index","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-04-26T20:05:23Z","receivedAt":"2017-04-26T20:05:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Jeff Hostetler \n\nTeach do_write_index() to close the index.lock file\nbefore getting the mtime and updating the istate.timestamp\nfields.\n\nOn Windows, a file's mtime is not updated until the file is\nclosed. On Linux, the mtime is set after the last flush.\n\nSigned-off-by: Jeff Hostetler \nSigned-off-by: Johannes Schindelin \n---\nPublished-As: https://github.com/dscho/git/releases/tag/do-write-index-mtime-v1\nFetch-It-Via: git fetch https://github.com/dscho/git do-write-index-mtime-v1\n\n read-cache.c | 13 +++++++++----\n 1 file changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 008b335844c..b0276fd5510 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2051,9 +2051,10 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfile\n \t\trollback_lock_file(lockfile);\n }\n \n-static int do_write_index(struct index_state *istate, int newfd,\n+static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n \t\t\t int strip_extensions)\n {\n+\tint newfd = tempfile->fd;\n \tgit_SHA_CTX c;\n \tstruct cache_header hdr;\n \tint i, err, removed, extended, hdr_version;\n@@ -2162,7 +2163,11 @@ static int do_write_index(struct index_state *istate, int newfd,\n \t\t\treturn -1;\n \t}\n \n-\tif (ce_flush(&c, newfd, istate->sha1) || fstat(newfd, &st))\n+\tif (ce_flush(&c, newfd, istate->sha1))\n+\t\treturn -1;\n+\tif (close_tempfile(tempfile))\n+\t\treturn error(_(\"could not close '%s'\"), tempfile->filename.buf);\n+\tif (lstat(tempfile->filename.buf, &st))\n \t\treturn -1;\n \tistate->timestamp.sec = (unsigned int)st.st_mtime;\n \tistate->timestamp.nsec = ST_MTIME_NSEC(st);\n@@ -2185,7 +2190,7 @@ static int commit_locked_index(struct lock_file *lk)\n static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,\n \t\t\t\t unsigned flags)\n {\n-\tint ret = do_write_index(istate, get_lock_file_fd(lock), 0);\n+\tint ret = do_write_index(istate, &lock->tempfile, 0);\n \tif (ret)\n \t\treturn ret;\n \tassert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=\n@@ -2282,7 +2287,7 @@ static int write_shared_index(struct index_state *istate,\n \t\treturn do_write_locked_index(istate, lock, flags);\n \t}\n \tmove_cache_to_base_index(istate);\n-\tret = do_write_index(si->base, fd, 1);\n+\tret = do_write_index(si->base, &temporary_sharedindex, 1);\n \tif (ret) {\n \t\tdelete_tempfile(&temporary_sharedindex);\n \t\treturn ret;\n\nbase-commit: e2cb6ab84c94f147f1259260961513b40c36108a\n-- \n2.12.2.windows.2.800.gede8f145e06\n"},{"id":"318028","messageId":"20170427031311.hfbit6glkx2tw3ru@sigill.intra.peff.net","threadId":"45808","inReplyTo":"e1b4f9c377ceee296112fa07bd06492a1de1be67.1493237111.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] read-cache: close index.lock in do_write_index","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-27T03:13:11Z","receivedAt":"2017-04-27T03:13:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 26, 2017 at 10:05:23PM +0200, Johannes Schindelin wrote:\n\n> From: Jeff Hostetler \n> \n> Teach do_write_index() to close the index.lock file\n> before getting the mtime and updating the istate.timestamp\n> fields.\n> \n> On Windows, a file's mtime is not updated until the file is\n> closed. On Linux, the mtime is set after the last flush.\n\nI wondered at first what this would mean for atomicity. The original\ncode does an fstat, so we're sure to get the timestamp of what we just\nwrote.\n\nI think we should be OK after your change, though. We're stat()ing the\nlockfile itself, so nobody else should be touching it (because they'd be\nviolating the lock to do so).\n\n> -static int do_write_index(struct index_state *istate, int newfd,\n> +static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n> \t\t\t int strip_extensions)\n> [...]\n> -\tif (ce_flush(&c, newfd, istate->sha1) || fstat(newfd, &st))\n> +\tif (ce_flush(&c, newfd, istate->sha1))\n> +\t\treturn -1;\n> +\tif (close_tempfile(tempfile))\n> +\t\treturn error(_(\"could not close '%s'\"), tempfile->filename.buf);\n> +\tif (lstat(tempfile->filename.buf, &st))\n> \t\treturn -1;\n\nSo now we unconditionally close in do_write_index(), but I don't see any\nclose_tempfile() calls going away. For the call in write_shared_index(),\nthat's because we either call delete_tempfile() or rename_tempfile(),\neither of which would close as needed, but can handle an already-closed\nfile.\n\nThe other caller is do_write_locked_index(), which accepts either a\nflag: either COMMIT_LOCK, CLOSE_LOCK, or neither. COMMIT_LOCK is OK; it\ncan handle the already-closed file. CLOSE_LOCK is obviously fine. It\njust becomes a noop. But when neither flag is set, now we close the\nlock. Are there any callers that will be affected?\n\nThere are three callers, but I think they all eventually trace up to\nwrite_locked_index(). And grepping for callers of that function, it\nlooks like each one uses either COMMIT_LOCK or CLOSE_LOCK.\n\nSo perhaps we'd want to squash in (or perhaps do as a preparatory\npatch) something like:\n\ndiff --git a/read-cache.c b/read-cache.c\nindex b0276fd55..db7a812af 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2193,14 +2193,16 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l\n \tint ret = do_write_index(istate, &lock->tempfile, 0);\n \tif (ret)\n \t\treturn ret;\n+\n+\t/* Callers must specify exactly one of COMMIT/CLOSE */\n \tassert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=\n \t (COMMIT_LOCK | CLOSE_LOCK));\n+\tassert((flags & (COMMIT_LOCK | CLOSE_LOCK)) != 0);\n+\n \tif (flags & COMMIT_LOCK)\n \t\treturn commit_locked_index(lock);\n-\telse if (flags & CLOSE_LOCK)\n-\t\treturn close_lock_file(lock);\n \telse\n-\t\treturn ret;\n+\t\treturn close_lock_file(lock);\n }\n \n static int write_split_index(struct index_state *istate,\n\nWe could also get rid of CLOSE_LOCK entirely at this point. Or since\nthese are the only two flags, just turn the flags field into a boolean\n\"int commit_lock\". But doing it as above is perhaps more readable\n(callers say CLOSE_LOCK instead of an unannotated \"0\"), and the extra\nassert will catch any topics in flight that add calls using \"0\" for\nflags.\n\n-Peff\n"},{"id":"318030","messageId":"xmqq1ssepmaw.fsf@gitster.mtv.corp.google.com","threadId":"45808","inReplyTo":"e1b4f9c377ceee296112fa07bd06492a1de1be67.1493237111.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] read-cache: close index.lock in do_write_index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-27T03:21:59Z","receivedAt":"2017-04-27T03:22:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> From: Jeff Hostetler \n>\n> Teach do_write_index() to close the index.lock file\n> before getting the mtime and updating the istate.timestamp\n> fields.\n>\n> On Windows, a file's mtime is not updated until the file is\n> closed. On Linux, the mtime is set after the last flush.\n>\n> Signed-off-by: Jeff Hostetler \n> Signed-off-by: Johannes Schindelin \n> ---\n> Published-As: https://github.com/dscho/git/releases/tag/do-write-index-mtime-v1\n> Fetch-It-Via: git fetch https://github.com/dscho/git do-write-index-mtime-v1\n>\n> read-cache.c | 13 +++++++++----\n> 1 file changed, 9 insertions(+), 4 deletions(-)\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index 008b335844c..b0276fd5510 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2051,9 +2051,10 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfile\n> \t\trollback_lock_file(lockfile);\n> }\n> \n> -static int do_write_index(struct index_state *istate, int newfd,\n> +static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n> \t\t\t int strip_extensions)\n> {\n> +\tint newfd = tempfile->fd;\n> \tgit_SHA_CTX c;\n> \tstruct cache_header hdr;\n> \tint i, err, removed, extended, hdr_version;\n> @@ -2162,7 +2163,11 @@ static int do_write_index(struct index_state *istate, int newfd,\n> \t\t\treturn -1;\n> \t}\n> \n> -\tif (ce_flush(&c, newfd, istate->sha1) || fstat(newfd, &st))\n> +\tif (ce_flush(&c, newfd, istate->sha1))\n> +\t\treturn -1;\n> +\tif (close_tempfile(tempfile))\n> +\t\treturn error(_(\"could not close '%s'\"), tempfile->filename.buf);\n> +\tif (lstat(tempfile->filename.buf, &st))\n> \t\treturn -1;\n\n\nstat/lstat with path may be slower than fstat on an open file\ndescriptor, and I think that is the reason why we do it this way,\nbut the performance difference would probably be unmeasurable and\ndoes not matter in practice.\n\nAs we are not using the fact that we still have the file descriptor\nopen when we do the stat for any purpose (e.g. like locking other\nprocesses out), this move to \"close first and then stat\" is a good\nworkaround for the problem. I wonder if we have been seeing false\n\"racy git\" problem more often due to this issue on Windows than\nother platforms.\n\nWhen code uses lstat, it gives a signal to the readers of the code\nthat the code is prepared to see a symlink and when it is a symlink\nit wants to grab the property of the link itself, not the target of\nthe link. I do not think the temporary index can be a symbolic\nlink, and even if that were the case, we do want the mtime of the\nlink target, so it is a wrong signal to give to the readers. Hence,\nit would be better to use stat() here from the readability's point\nof view. Of course, when the path is always a regular file, lstat()\nvs stat() technically does not give any different result, so this\ncomment is purely about the maintainability, not about correctness.\n\nOther than that, looks good to me.\n\n> \tistate->timestamp.sec = (unsigned int)st.st_mtime;\n> \tistate->timestamp.nsec = ST_MTIME_NSEC(st);\n> @@ -2185,7 +2190,7 @@ static int commit_locked_index(struct lock_file *lk)\n> static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,\n> \t\t\t\t unsigned flags)\n> {\n> -\tint ret = do_write_index(istate, get_lock_file_fd(lock), 0);\n> +\tint ret = do_write_index(istate, &lock->tempfile, 0);\n> \tif (ret)\n> \t\treturn ret;\n> \tassert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=\n> @@ -2282,7 +2287,7 @@ static int write_shared_index(struct index_state *istate,\n> \t\treturn do_write_locked_index(istate, lock, flags);\n> \t}\n> \tmove_cache_to_base_index(istate);\n> -\tret = do_write_index(si->base, fd, 1);\n> +\tret = do_write_index(si->base, &temporary_sharedindex, 1);\n> \tif (ret) {\n> \t\tdelete_tempfile(&temporary_sharedindex);\n> \t\treturn ret;\n>\n> base-commit: e2cb6ab84c94f147f1259260961513b40c36108a\n"},{"id":"318074","messageId":"c6a8bcbd-a0bd-e586-a672-77b01c4d2568@jeffhostetler.com","threadId":"45808","inReplyTo":"xmqq1ssepmaw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] read-cache: close index.lock in do_write_index","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-04-27T16:45:45Z","receivedAt":"2017-04-27T16:45:53Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 4/26/2017 11:21 PM, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n>\n>> From: Jeff Hostetler \n>>\n>> Teach do_write_index() to close the index.lock file\n>> before getting the mtime and updating the istate.timestamp\n>> fields.\n>>\n>> On Windows, a file's mtime is not updated until the file is\n>> closed. On Linux, the mtime is set after the last flush.\n>>\n>> Signed-off-by: Jeff Hostetler \n>> Signed-off-by: Johannes Schindelin \n>> ---\n>> Published-As: https://github.com/dscho/git/releases/tag/do-write-index-mtime-v1\n>> Fetch-It-Via: git fetch https://github.com/dscho/git do-write-index-mtime-v1\n>>\n>> read-cache.c | 13 +++++++++----\n>> 1 file changed, 9 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/read-cache.c b/read-cache.c\n>> index 008b335844c..b0276fd5510 100644\n>> --- a/read-cache.c\n>> +++ b/read-cache.c\n>> @@ -2051,9 +2051,10 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfile\n>> \t\trollback_lock_file(lockfile);\n>> }\n>>\n>> -static int do_write_index(struct index_state *istate, int newfd,\n>> +static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n>> \t\t\t int strip_extensions)\n>> {\n>> +\tint newfd = tempfile->fd;\n>> \tgit_SHA_CTX c;\n>> \tstruct cache_header hdr;\n>> \tint i, err, removed, extended, hdr_version;\n>> @@ -2162,7 +2163,11 @@ static int do_write_index(struct index_state *istate, int newfd,\n>> \t\t\treturn -1;\n>> \t}\n>>\n>> -\tif (ce_flush(&c, newfd, istate->sha1) || fstat(newfd, &st))\n>> +\tif (ce_flush(&c, newfd, istate->sha1))\n>> +\t\treturn -1;\n>> +\tif (close_tempfile(tempfile))\n>> +\t\treturn error(_(\"could not close '%s'\"), tempfile->filename.buf);\n>> +\tif (lstat(tempfile->filename.buf, &st))\n>> \t\treturn -1;\n>\n>\n> stat/lstat with path may be slower than fstat on an open file\n> descriptor, and I think that is the reason why we do it this way,\n> but the performance difference would probably be unmeasurable and\n> does not matter in practice.\n>\n> As we are not using the fact that we still have the file descriptor\n> open when we do the stat for any purpose (e.g. like locking other\n> processes out), this move to \"close first and then stat\" is a good\n> workaround for the problem. I wonder if we have been seeing false\n> \"racy git\" problem more often due to this issue on Windows than\n> other platforms.\n\nI was wondering about that too.\n\n>\n> When code uses lstat, it gives a signal to the readers of the code\n> that the code is prepared to see a symlink and when it is a symlink\n> it wants to grab the property of the link itself, not the target of\n> the link. I do not think the temporary index can be a symbolic\n> link, and even if that were the case, we do want the mtime of the\n> link target, so it is a wrong signal to give to the readers. Hence,\n> it would be better to use stat() here from the readability's point\n> of view. Of course, when the path is always a regular file, lstat()\n> vs stat() technically does not give any different result, so this\n> comment is purely about the maintainability, not about correctness.\n>\n> Other than that, looks good to me.\n\nForce of habit using lstat() rather than stat(). I'll change it.\nThanks.\n\n"},{"id":"318075","messageId":"c7c63b48-cb11-35f4-f41f-52286d66bef2@jeffhostetler.com","threadId":"45808","inReplyTo":"20170427031311.hfbit6glkx2tw3ru@sigill.intra.peff.net","subject":"Re: [PATCH] read-cache: close index.lock in do_write_index","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2017-04-27T16:51:56Z","receivedAt":"2017-04-27T16:52:02Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 4/26/2017 11:13 PM, Jeff King wrote:\n> On Wed, Apr 26, 2017 at 10:05:23PM +0200, Johannes Schindelin wrote:\n>\n>> From: Jeff Hostetler \n>>\n>> Teach do_write_index() to close the index.lock file\n>> before getting the mtime and updating the istate.timestamp\n>> fields.\n>>\n>> On Windows, a file's mtime is not updated until the file is\n>> closed. On Linux, the mtime is set after the last flush.\n>\n> I wondered at first what this would mean for atomicity. The original\n> code does an fstat, so we're sure to get the timestamp of what we just\n> wrote.\n>\n> I think we should be OK after your change, though. We're stat()ing the\n> lockfile itself, so nobody else should be touching it (because they'd be\n> violating the lock to do so).\n>\n>> -static int do_write_index(struct index_state *istate, int newfd,\n>> +static int do_write_index(struct index_state *istate, struct tempfile *tempfile,\n>> \t\t\t int strip_extensions)\n>> [...]\n>> -\tif (ce_flush(&c, newfd, istate->sha1) || fstat(newfd, &st))\n>> +\tif (ce_flush(&c, newfd, istate->sha1))\n>> +\t\treturn -1;\n>> +\tif (close_tempfile(tempfile))\n>> +\t\treturn error(_(\"could not close '%s'\"), tempfile->filename.buf);\n>> +\tif (lstat(tempfile->filename.buf, &st))\n>> \t\treturn -1;\n>\n> So now we unconditionally close in do_write_index(), but I don't see any\n> close_tempfile() calls going away. For the call in write_shared_index(),\n> that's because we either call delete_tempfile() or rename_tempfile(),\n> either of which would close as needed, but can handle an already-closed\n> file.\n>\n> The other caller is do_write_locked_index(), which accepts either a\n> flag: either COMMIT_LOCK, CLOSE_LOCK, or neither. COMMIT_LOCK is OK; it\n> can handle the already-closed file. CLOSE_LOCK is obviously fine. It\n> just becomes a noop. But when neither flag is set, now we close the\n> lock. Are there any callers that will be affected?\n>\n> There are three callers, but I think they all eventually trace up to\n> write_locked_index(). And grepping for callers of that function, it\n> looks like each one uses either COMMIT_LOCK or CLOSE_LOCK.\n>\n\nYes, we only took a casual look at the calling environment(s)\nand didn't try to do a full reduction/refactoring. In the\nabsence of any other red-flags, I'll look at doing this.\n\nThanks!\n\n> So perhaps we'd want to squash in (or perhaps do as a preparatory\n> patch) something like:\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index b0276fd55..db7a812af 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -2193,14 +2193,16 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l\n> \tint ret = do_write_index(istate, &lock->tempfile, 0);\n> \tif (ret)\n> \t\treturn ret;\n> +\n> +\t/* Callers must specify exactly one of COMMIT/CLOSE */\n> \tassert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=\n> \t (COMMIT_LOCK | CLOSE_LOCK));\n> +\tassert((flags & (COMMIT_LOCK | CLOSE_LOCK)) != 0);\n> +\n> \tif (flags & COMMIT_LOCK)\n> \t\treturn commit_locked_index(lock);\n> -\telse if (flags & CLOSE_LOCK)\n> -\t\treturn close_lock_file(lock);\n> \telse\n> -\t\treturn ret;\n> +\t\treturn close_lock_file(lock);\n> }\n>\n> static int write_split_index(struct index_state *istate,\n>\n> We could also get rid of CLOSE_LOCK entirely at this point. Or since\n> these are the only two flags, just turn the flags field into a boolean\n> \"int commit_lock\". But doing it as above is perhaps more readable\n> (callers say CLOSE_LOCK instead of an unannotated \"0\"), and the extra\n> assert will catch any topics in flight that add calls using \"0\" for\n> flags.\n>\n> -Peff\n>\n"}]}