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

[Junio C Hamano <gitster@pobox.com>]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

> Ramsay Jones wrote:
>> Michael Haggerty wrote:
>>> On 06/27/2013 12:35 AM, Jeff King wrote:
>> [ ... ]
>>>> I think Michael's assessment above is missing one thing.
>>>
>>> Peff is absolutely right; for some unknown reason I was thinking of the
>>> consistency check as having been already fixed.
>> 
>> Well, the "cygwin: Remove the Win32 l/stat() functions" patch *does* fix
>> the problem. :-D It's just a pity we can't use it on performance grounds. :(
>> 
>>>> [...#ifdef out consistency check on cygwin when lock is held...]
>>>
>>> Yes, this would work.
>>>
>>> But, taking a step back, I think it is a bad idea to have an unreliable
>>> stat() masquerading as a real stat().  If we want to allow the use of an
>>> unreliable stat for certain purposes, let's have two stat() interfaces:
>>>
>>> * the true stat() (in this case I guess cygwin's slow-but-correct
>>> implementation)
>>>
>>> * some fast_but_maybe_unreliable_stat(), which would map to stat() on
>>> most platforms but might map to the Windows stat() on cygwin when so
>>> configured.
>>>
>>> By default the true stat() would always be used.  It should have to be a
>>> conscious decision, taken only in specific, vetted scenarios, to use the
>>> unreliable stat.
>>  ...

I like the part that gets rid of that "get-mode-bits" but at the
same time, I find this part wanting a reasonable in-code comment.

At least, with the earlier get-mode-bits, it was clear why we are
doing something special in that codepath, both from the name of the
helper function/macro and the comment attached to it describing how
the "regular" one is cheating.

We must say why this "fast" is not used everywhere and what criteria
you should apply when deciding to use it (or not use it).  And the
"fast" name is much less descriptive.

I suspect (without thinking it through) that the rule would be
something like:

    The "fast" variant is to be used to read from the filesystem the
    "stat" bits that are stuffed into the index for quick "touch
    detection" (aka "cached stat info") and/or that are compared
    with the cached stat info, and should not be used anywhere else.

But then we always use lstat(2) and not stat(2) for that, so...

> +#ifndef GIT_FAST_STAT
> +static inline int fast_stat(const char *path, struct stat *st)
> +{
> +	return stat(path, st);
> +}
> +static inline int fast_lstat(const char *path, struct stat *st)
> +{
> +	return lstat(path, st);
> +}
> +#endif