Re: [PATCH] attr: don't confuse prefixes with leading directories

From: Jeff King <peff@peff.net>
Date: Jan 10, 2012, 18:21 UTC
Message-ID: <20120110182140.GB15273@sigill.intra.peff.net>
In-Reply-To: <20120110180820.GA15273@sigill.intra.peff.net>

On Tue, Jan 10, 2012 at 01:08:21PM -0500, Jeff King wrote:

Show 12 quoted lines

> diff --git a/attr.c b/attr.c
> index 76b079f..fa975da 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -582,7 +582,8 @@ static void prepare_attr_stack(const char *path)
>  
>  		elem = attr_stack;
>  		if (namelen <= dirlen &&
> -		    !strncmp(elem->origin, path, namelen))
> +		    !strncmp(elem->origin, path, namelen) &&
> +		    (!namelen || path[namelen] == '/'))
>  			break;

Side note. One thing that confused me about this code is that prepare_attr_stack does a popping loop like this:

  while (attr_stack && attr_stack->origin) {
          if (/* attr_stack->origin is a prefix */)
                  break;
          /* otherwise, pop */
          elem = attr_stack;
          attr_stack = elem->prev;
          free(elem);
  }

  /* now push our new ones */
  ...
  len = strlen(attr_stack->origin);

IOW, our loop breaks out when attr_stack is NULL, but then we go on to assume that attr_stack is _not_ NULL. This isn't a bug, because it turns out that we always leave something in the attr_stack: the root gitattributes file (and the builtins). But it is slightly confusing to a reader because of the useless loop condition.

I'm not sure if the right solution is to change the popping loop to:

  /* we will never run out of stack, because we always have the root */
  while (attr_stack->origin) {
          ...

Or to be extra defensive and put:

  if (!attr_stack)
          die("BUG: we ran out of attr stack!?");

after the loop, or to somehow handle the case of an empty attr stack below (which is hard to do, because it can't be triggered, so I have no idea what it would mean).

-Peff