From: Jeff Hostetler Date: Thu, 27 Apr 2017 16:51:56 GMT Subject: Re: [PATCH] read-cache: close index.lock in do_write_index Message-ID: In-Reply-To: <20170427031311.hfbit6glkx2tw3ru@sigill.intra.peff.net> On 4/26/2017 11:13 PM, Jeff King wrote: > On Wed, Apr 26, 2017 at 10:05:23PM +0200, Johannes Schindelin wrote: > >> From: Jeff Hostetler >> >> 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). > >> -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! > 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 >