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

List:       git
Subject:    Re: [PATCH] add GIT_FAST_STAT mode for Cygwin
From:       Johannes Sixt <johannes.sixt () telecom ! at>
Date:       2008-09-23 20:41:42
Message-ID: 200809232241.42649.johannes.sixt () telecom ! at
[Download RAW message or body]

On Dienstag, 23. September 2008, Dmitry Potapov wrote:
> 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.

> > > +		/* 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.

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