Re: [PATCH/RFC v3 7/9] write_entry(): use fstat() instead of lstat() when file is open
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Sixt <j.sixt@viscovery.net> writes:
Show 7 quoted lines
>> I've a bad gut feeling about this: It may not work as expected on Windows
>> because there is this statement in the documentation:
>>
>> "The only guarantee about a file timestamp is that the file time is
>> correctly reflected when the handle that makes the change is closed."
>>
>> (http://msdn.microsoft.com/en-us/library/ms724290(VS.85).aspx)
Ok, I admit I was not aware of this Windows fact.
Show 12 quoted lines
>> We are operating on a temporary file. It could happen that the fstat()
>> returns the time when the file was created, as opposed to when the
>> write_in_full() was completed successfully. The fstat()ed time ends up in
>> the index, but it can be different from what later lstat() calls report
>> (and the file would be regarded as modified).
>>
>> I have the suspicion that the gain from this patch is minimal. Would you
>> mind playing it safe and drop this patch?
>
> Hmm, write_entry() is actually called once per one path we write out, and
> the fstat() is added to the common case (no --tempfile, no --prefix=<dir>
> checkout),
Yes, I had to make sure that the path string and ce->name was the
exact same string, so therefore I had to add the test "&& !to_tempfile
&& !state->base_dir_len" to the if-test.
> which would mean that if there were any performance gain from
> this change, it was obtained by trading correctness away. Sad.
Sorry about this. Yes, I agree that we should drop this patch.
And, yes, since each lstat() call cost approximately 44 microseconds
compared to 12-16 for each lstat() on my Linux box, there was a little
performance gain from this patch.
Junio, is it OK to ask that you drop this patch if/when you update the
pu branch, such that I do not have to resend the patch series almost
unchanged to the mailinglist (except for one missing patch)?
Ok, maybe wait one day, just in case there will be more comments.
And, thanks for the review!
-- kjetil