[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:       Dmitry Potapov <dpotapov () gmail ! com>
Date:       2008-09-23 16:52:47
Message-ID: 20080923165247.GO21650 () dpotapov ! dyndns ! org
[Download RAW message or body]

On Tue, Sep 23, 2008 at 04:37:14PM +0200, Alex Riesen wrote:
> 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.

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

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