Re: [PATCH] add GIT_FAST_STAT mode for Cygwin

From: Dmitry Potapov <dpotapov@gmail.com>
Date: Sep 23, 2008, 16:52 UTC
Message-ID: <20080923165247.GO21650@dpotapov.dyndns.org>
In-Reply-To: <81b0412b0809230737s7498e214w4c58991e79f76507@mail.gmail.com>

On Tue, Sep 23, 2008 at 04:37:14PM +0200, Alex Riesen wrote:

Show 7 quoted lines

> 2008/9/23 Dmitry Potapov <dpotapov@gmail.com>:
> >
> > This fast mode works only for relative paths. It is assumed that the
> > whole repository is located inside one directory without using Cygwin
> > mount to bind external paths inside of the current tree.
> 
> Why runtime conditional? Why conditional at all?

I thought that in rather unusual circumstances (such as using Cygwin mount to connect separately directories in one tree), this fast version may not work. So, I made it conditional. It is runtime conditional, because most users do not build Git themselves but install a ready Cygwin package.

> Why not fallback
> to cygwin's slow stat on absolute pathnames like you do for symlinks?

Of course, I do:

+       if (file_name[0] == '/')
+               return cygstat (file_name, buf);

Sorry, if it was not clear from my above comment.

Show 5 quoted lines

> 
> > +/*
> > + * This are startup stubs, which choose what implementation of lstat/stat
> 
> why do you need two of them? Isn't one not enough?

I did not want to give people reasons to say that I broke lstat :) You can opt for the standard Cygwin version of it if for some reason, this new function does not work. Now, I know only one case -- it is when you use Cygwin mount inside of Git repo. Yet, I don't know enough about Cygwin to be sure that there is no other cases. So, I just wanted to be extra careful and not to break anything.

Show 12 quoted lines

> 
> > +stat_fn_t cygwin_stat_fn = cygwin_stat_choice;
> > +stat_fn_t cygwin_lstat_fn = cygwin_lstat_choice;
> ...
> > +typedef int (*stat_fn_t)(const char*, struct stat*);
> > +extern stat_fn_t cygwin_stat_fn;
> > +extern stat_fn_t cygwin_lstat_fn;
> 
>     extern int (*cygwin_stat_fn)(const char *, struct stat *);
> 
> Is shorter, easier to read and easier to understand (for a C person).
> You don't even use the type anywhere else, it is just for the declaration sake!

I use it in description of a parameter of another function:

static int do_stat(const char *file_name, struct stat *buf, stat_fn_t cygstat)

Of course, you can avoid it here too, but the declaration will become somewhat longer:

static int do_stat(const char *file_name, struct stat *buf,
       int (*cygstat)(const char *, struct stat *));

so I am not sure that removing stat_fn_t improves readability, but if there are other people who think so, I will correct that.

Dmitry