Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

From: Johannes Sixt <johannes.sixt@telecom.at>
Date: Sep 23, 2008, 20:41 UTC
Message-ID: <200809232241.42649.johannes.sixt@telecom.at>
In-Reply-To: <20080923194802.GQ21650@dpotapov.dyndns.org>

On Dienstag, 23. September 2008, Dmitry Potapov wrote:

Show 16 quoted lines

> On Tue, Sep 23, 2008 at 09:03:08PM +0200, Johannes Sixt wrote:
> > On Dienstag, 23. September 2008, Dmitry Potapov wrote:
> > > +static int do_stat(const char *file_name, struct stat *buf, stat_fn_t
> > > cygstat) +{
> > > +	WIN32_FILE_ATTRIBUTE_DATA fdata;
> > > +
> > > +	if (file_name[0] == '/')
> > > +		return cygstat (file_name, buf);
> >
> > You should do this in the caller; it would make this function's
> > semantics much clearer.
>
> IMHO, the semantic of this function is clear: do_stat performs stat/lstat
> using Windows API with falling back on Cygwin implementation in those
> rare cases that it cannot handle correctly. Absolute path is just one of
> those cases. So, I am not sure what you win by moving this two lines out.

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.

Show 5 quoted lines

> > > +		/* st_dev, st_rdev are not used by Git */
> > > +		buf->st_dev = buf->st_rdev = 0;
>
> I set this to 0, while MinGW Git uses _getdrive(). I have no idea why
> it does so.

Indeed. Calling _getdrive() is absolutely useless.

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

-- Hannes