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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 30, 2013, 19:50 UTC
Message-ID: <7v4ncfjs32.fsf@alter.siamese.dyndns.org>
In-Reply-To: <51D06AC8.70206@ramsay1.demon.co.uk>

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

Show 31 quoted lines

> 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...

Show 10 quoted lines

> +#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