Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

From: Dmitry Potapov <dpotapov@gmail.com>
Date: Sep 23, 2008, 21:11 UTC
Message-ID: <20080923211124.GT21650@dpotapov.dyndns.org>
In-Reply-To: <200809232241.42649.johannes.sixt@telecom.at>

On Tue, Sep 23, 2008 at 10:41:42PM +0200, Johannes Sixt wrote:

Show 6 quoted lines

> 
> You copied the function from compat/mingw.c. There it has the meaning "Fill in 
> struct stat using Win32 API" and nothing else. Here it has the meaning "Fill 
> in struct stat using Win32 API if you can, and using cygstat() in certain 
> exceptional cases". If you stayed with the original meaning, it would be 
> slightly easier to factor out common code.

do_stat() always fills in the structure, but it can do that fast using Win32 API or fallback on cygstat() in exceptional cases. So, I don't think I change its meaning much, its implementation certainly differs.

Show 11 quoted lines

> > > You do duplicate a lot of code here. Any chances to factor out the
> > > common parts?
> >
> > I don't see much common code here. Initialization of 5 variables where
> > four of them are just constants? Perhaps, the biggest common part here
> > is conversion of dwFileAttributes to st_mode, but it is still 5 lines of
> > trivial code.
> 
> Sigh. I gave a pointer how to unify the two functions (although I missed the 
> fact that the member variables are named differently). I'd appreciate if you 
> did not make it more difficult than necessary to factor out common code.

Because the stat structure is different and handling exceptional situation is different, I don't think we can have a single do_stat function for Cygwin and MinGW. Yet, perhaps, it is possible to move some code in common functions even if it is just a few lines.

The first candidate is win_attr_to_st_mode(), which converts dwFileAttributes returned by GetFileAttributesExA to st_mode. Another possible function is that obtains and converts Win32 error code to errno value. These function can be placed into some common header (for example, win32.h), which will included by both implementations. Does it make sense?

Dmitry