Re: [PATCH] read-cache: close index.lock in do_write_index
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 27, 2017, 03:21 UTC
- Message-ID
- <xmqq1ssepmaw.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <e1b4f9c377ceee296112fa07bd06492a1de1be67.1493237111.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 45 quoted lines
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> 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.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/do-write-index-mtime-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git do-write-index-mtime-v1
>
> read-cache.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 008b335844c..b0276fd5510 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -2051,9 +2051,10 @@ void update_index_if_able(struct index_state *istate, struct lock_file *lockfile
> rollback_lock_file(lockfile);
> }
>
> -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)
> {
> + int newfd = tempfile->fd;
> git_SHA_CTX c;
> struct cache_header hdr;
> int i, err, removed, extended, hdr_version;
> @@ -2162,7 +2163,11 @@ static int do_write_index(struct index_state *istate, int newfd,
> return -1;
> }
>
> - 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;stat/lstat with path may be slower than fstat on an open file descriptor, and I think that is the reason why we do it this way, but the performance difference would probably be unmeasurable and does not matter in practice.
As we are not using the fact that we still have the file descriptor open when we do the stat for any purpose (e.g. like locking other processes out), this move to "close first and then stat" is a good workaround for the problem. I wonder if we have been seeing false "racy git" problem more often due to this issue on Windows than other platforms.
When code uses lstat, it gives a signal to the readers of the code that the code is prepared to see a symlink and when it is a symlink it wants to grab the property of the link itself, not the target of the link. I do not think the temporary index can be a symbolic link, and even if that were the case, we do want the mtime of the link target, so it is a wrong signal to give to the readers. Hence, it would be better to use stat() here from the readability's point of view. Of course, when the path is always a regular file, lstat() vs stat() technically does not give any different result, so this comment is purely about the maintainability, not about correctness.
Other than that, looks good to me.
Show 22 quoted lines
> istate->timestamp.sec = (unsigned int)st.st_mtime;
> istate->timestamp.nsec = ST_MTIME_NSEC(st);
> @@ -2185,7 +2190,7 @@ static int commit_locked_index(struct lock_file *lk)
> static int do_write_locked_index(struct index_state *istate, struct lock_file *lock,
> unsigned flags)
> {
> - int ret = do_write_index(istate, get_lock_file_fd(lock), 0);
> + int ret = do_write_index(istate, &lock->tempfile, 0);
> if (ret)
> return ret;
> assert((flags & (COMMIT_LOCK | CLOSE_LOCK)) !=
> @@ -2282,7 +2287,7 @@ static int write_shared_index(struct index_state *istate,
> return do_write_locked_index(istate, lock, flags);
> }
> move_cache_to_base_index(istate);
> - ret = do_write_index(si->base, fd, 1);
> + ret = do_write_index(si->base, &temporary_sharedindex, 1);
> if (ret) {
> delete_tempfile(&temporary_sharedindex);
> return ret;
>
> base-commit: e2cb6ab84c94f147f1259260961513b40c36108a