[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH 4/4] cygwin: Use native Win32 API for stat
From:       Dmitry Potapov <dpotapov () gmail ! com>
Date:       2008-09-27 21:54:06
Message-ID: 20080927215406.GG21650 () dpotapov ! dyndns ! org
[Download RAW message or body]

On Sat, Sep 27, 2008 at 08:35:03PM +0200, Johannes Sixt wrote:
> 
> > +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 :)

> 
> > +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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]