Re: [PATCH] read-cache: close index.lock in do_write_index

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> 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.