Re: [RFC/PATCH 0/1] cygwin: Remove the Win32 l/stat() functions

[Ramsay Jones <ramsay@ramsay1.demon.co.uk>]

Michael Haggerty wrote:
> On 06/25/2013 07:07 AM, Junio C Hamano wrote:
>> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
>>
>>> Michael Haggerty and Jeff King have been re-vamping the reference
>>> handling code. The failures noted above were provoked by patches
>>> in the 'mh/ref-races' branch. At the time I wrote this patch, that
>>> branch was only included in 'pu', but I notice that this topic has
>>> now progressed to 'next' (see commit 71f1a182).
>>
>> I had an impression that up to 98eeb09e (for_each_ref: load all
>> loose refs before packed refs, 2013-06-20) that is now in 'next'
>> does not agressively use the lstat timestamp of the packed-refs
>> file, and the "optional" bit 5d478f5c (refs: do not invalidate the
>> packed-refs cache unnecessarily, 2013-06-20), and the one in 'next'
>> should be safe with the cheating-lstat.  Isn't it the case?
>>
>> In any case, if removing the cheating-lstat will improve the
>> robustness without hurting performance, I am all for it.
>>
>> The less platform specific hacks, the better ;-).
> 
> The following patch in the "non-optional" commits (which has already
> been merged to next) uses stat() via the new stat_validity API:
> 
> ca9199300e get_packed_ref_cache: reload packed-refs file when it changes
> 
> This patch adds some *extra* cache invalidation that was heretofore
> missing.  If stat() is broken it could
> 
> (a) cause a false positive, resulting in some unnecessary cache
> invalidation and re-reading of packed-refs, which will hurt performance
> but not correctness; or
> 
> (b) cause a false negative, in which case the stale cache might be used
> for reading (but not writing), just as was *always* the case before this
> patch.
> 
> As far as I understand, the concern for cygwin is (a).  I will leave it
> to others to measure and/or decide whether the performance loss is too
> grave to endure until the cygwin stat() situation is fixed.
> 

Hmm, I'm not sure I understand ... However, I can confirm that the
'mh/ref-races' branch in next is broken on cygwin. (i.e. it is not
just a speed issue; it provokes fatal errors).

See reply to Junio.

ATB,
Ramsay Jones