[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:       Johannes Sixt <johannes.sixt () telecom ! at>
Date:       2008-09-27 18:35:03
Message-ID: 200809272035.03833.johannes.sixt () telecom ! at
[Download RAW message or body]

On Samstag, 27. September 2008, Dmitry Potapov wrote:
> lstat/stat functions in Cygwin are very slow, because they try to emulate
> some *nix things that Git does not actually need. This patch adds Win32
> specific implementation of these functions for Cygwin.
>
> This implementation handles most situation directly but in some rare cases
> it falls back on the implementation provided for Cygwin.

Even though I was concerned about code duplication earlier, with the 
factorization that you do in this series this is acceptable, in particular, 
since working out at a solution that deals with the time_t vs. timespec 
difference we would need dirty tricks that are not worth it.

(But see my comment about get_file_attr() in a separate mail.)

> +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?)

> +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 ;).

-- Hannes
--
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]