Re: [PATCH] read-cache: close index.lock in do_write_index
From: Jeff Hostetler <hidden>
Date: 2017-04-27 16:52:02
On 4/26/2017 11:13 PM, Jeff King wrote:
On Wed, Apr 26, 2017 at 10:05:23PM +0200, Johannes Schindelin wrote:quoted
From: Jeff Hostetler <redacted> Teach do_write_index() to close the index.lock file before getting the mtime and updating the istate.timestamp fields. On Windows, a file's mtime is not updated until the file is closed. On Linux, the mtime is set after the last flush.I wondered at first what this would mean for atomicity. The original code does an fstat, so we're sure to get the timestamp of what we just wrote. I think we should be OK after your change, though. We're stat()ing the lockfile itself, so nobody else should be touching it (because they'd be violating the lock to do so).quoted
-static int do_write_index(struct index_state *istate, int newfd, +static int do_write_index(struct index_state *istate, struct tempfile *tempfile, int strip_extensions) [...] - if (ce_flush(&c, newfd, istate->sha1) || fstat(newfd, &st)) + if (ce_flush(&c, newfd, istate->sha1)) + return -1; + if (close_tempfile(tempfile)) + return error(_("could not close '%s'"), tempfile->filename.buf); + if (lstat(tempfile->filename.buf, &st)) return -1;So now we unconditionally close in do_write_index(), but I don't see any close_tempfile() calls going away. For the call in write_shared_index(), that's because we either call delete_tempfile() or rename_tempfile(), either of which would close as needed, but can handle an already-closed file. The other caller is do_write_locked_index(), which accepts either a flag: either COMMIT_LOCK, CLOSE_LOCK, or neither. COMMIT_LOCK is OK; it can handle the already-closed file. CLOSE_LOCK is obviously fine. It just becomes a noop. But when neither flag is set, now we close the lock. Are there any callers that will be affected? There are three callers, but I think they all eventually trace up to write_locked_index(). And grepping for callers of that function, it looks like each one uses either COMMIT_LOCK or CLOSE_LOCK.
Yes, we only took a casual look at the calling environment(s) and didn't try to do a full reduction/refactoring. In the absence of any other red-flags, I'll look at doing this. Thanks!
quoted hunk ↗ jump to hunk
So perhaps we'd want to squash in (or perhaps do as a preparatory patch) something like:diff --git a/read-cache.c b/read-cache.c index b0276fd55..db7a812af 100644 --- a/read-cache.c +++ b/read-cache.c@@ -2193,14 +2193,16 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l int ret = do_write_index(istate, &lock->tempfile, 0); if (ret) return ret; + + /* Callers must specify exactly one of COMMIT/CLOSE */ assert((flags & (COMMIT_LOCK | CLOSE_LOCK)) != (COMMIT_LOCK | CLOSE_LOCK)); + assert((flags & (COMMIT_LOCK | CLOSE_LOCK)) != 0); + if (flags & COMMIT_LOCK) return commit_locked_index(lock); - else if (flags & CLOSE_LOCK) - return close_lock_file(lock); else - return ret; + return close_lock_file(lock); } static int write_split_index(struct index_state *istate,We could also get rid of CLOSE_LOCK entirely at this point. Or since these are the only two flags, just turn the flags field into a boolean "int commit_lock". But doing it as above is perhaps more readable (callers say CLOSE_LOCK instead of an unannotated "0"), and the extra assert will catch any topics in flight that add calls using "0" for flags. -Peff