Re: [PATCH v2] correct verify_path for Windows

From: Dmitry Potapov <dpotapov@gmail.com>
Date: Oct 12, 2008, 13:50 UTC
Message-ID: <20081012135048.GC21650@dpotapov.dyndns.org>
In-Reply-To: <81b0412b0810111558vb69be00if4842fa91d777c3b@mail.gmail.com>

On Sun, Oct 12, 2008 at 12:58:52AM +0200, Alex Riesen wrote:

Show 14 quoted lines

> 2008/10/11 Dmitry Potapov <dpotapov@gmail.com>:
> >> > +   /* On Windows, file names are case-insensitive */
> >> > +   case 'G':
> >> > +           if ((rest[1]|0x20) != 'i')
> >> > +                   break;
> >> > +           if ((rest[2]|0x20) != 't')
> >> > +                   break;
> >>
> >> We have tolower().
> >
> > I am aware of that, but I am not sure what we gain by using it. It seems
> > it makes only code bigger and slow.
> 
> It does? Care to look into git-compat-util.h?

As a matter of fact, I did, and I see the following:

  #define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)
  #define tolower(x) sane_case((unsigned char)(x), 0x20)

  static inline int sane_case(int x, int high)
  {
  	if (sane_istest(x, GIT_ALPHA))
  		x = (x & ~0x20) | high;
  	return x;
  }

So, it looks like an extra look up and an extra comparison here.

Show 9 quoted lines

> 
> > ... As to readability, I don't see much
> > improvement... Isn't obvious what this code does, especially with the
> > above comment?
> 
> You want to seriously argue that "a | 0x20" is as readable as "tolower(a)"?
> For the years to come? With a person who does not even know what ASCII is?
> Ok, I'm exaggerating. But the point is: it is not us who will be
> reading the code.

Obviously, for a person who don't know what ASCII is, tolower() will be
much easier to understand, but the question is what I can reasonable to
expect for a person reading this code later. A similar argument can be
made about adding extra parenthesis, i.e. instead of writing
  if (a == b || c == d)
you should always write
  if ((a == b) || (c == d))
because some people do not remember the priority of each operator.
(And I have seen such programmers who claim to have many experience of
writing in C, yet, they do not remember operator priority.)

For me, using tolower() does not make it more readable, but maybe I am too old-fashion assuming that people are supposed to know at least basic things about ASCII.

> BTW, is it such a critical path?

I am not sure whether it is critical or not. It is called for each name in path. So, if you have a long path, it may be called quite a few times per a single path. Also, some operation such 'git add' can call verify_path() more than once (IIRC, it was called thrice per each added file). But I have no numbers to tell whether it is noticeable or not.

> Can't the code be unified and do without #ifdef?

It will impose a extra restriction on what file names people can use, and I don't like extra restrictions for those who use sane file systems.

Dmitry