Re: [PATCH 4/4] cygwin: Use native Win32 API for stat

On Sat, Sep 27, 2008 at 08:35:03PM +0200, Johannes Sixt wrote:

quoted

+core.cygwinNativeStat::

This name is *really* odd, for two reasons:

- If I read "native" in connection with Windows, I would understand Windows's 
implementation as "native". Cygwin is not native - it's a bolted-on feature.

- This name talks about the implementation, not about its effect.

Perhaps a better name would be core.ignoreCygwinFSFeatures, and the 
description would only mention that setting this to true (the default) makes 
many operations much faster, but makes it impossible to use File System 
Features A and B and C in the repository. "If you need one of these features, 
set this to false."

(And after writing above paragraphs I notice, that you actually really meant 
Windows's "native" stat; see how confusing the name is?)

It was Shawn's suggestion. I don't care much about the name as long as
it is explained in the documentation... Therefore, I accepted what Shawn
said without giving it any thought. Now, when you bring this name to my
attention, I believe core.useCygwinStat (in the opposite to the current
core.cygwinNativeStat) would be a better name. Your name is okay too,
but a bit too long for my taste and not specific enough (I suppose
Cygwin does many FS related tricks). Anyway, I don't have a strong
opinion here, so just whatever most people like is fine with me :)

quoted

+static inline void filetime_to_timespec(const FILETIME *ft, struct
timespec *ts) +{
+	long long winTime = ((long long)ft->dwHighDateTime << 32) +
ft->dwLowDateTime; +	winTime -= 116444736000000000LL; /* Windows to Unix
Epoch conversion */ +	ts->tv_sec = (time_t)(winTime/10000000); /*
100-nanosecond interval to seconds */ +	ts->tv_nsec = (long)(winTime -
ts->tv_sec*10000000LL) * 100; /* nanoseconds */ +}

Shorter lines in this function would be appreciated (and not just because my 
MUA can't deal with them ;).

I am sorry, I did not notice that the line got longer than 80 columns.
I will resent the patch once the issue with the name of the option is
resolved.

Dmitry