From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 10 Jan 2012 19:23:01 GMT
Subject: Re: [PATCH] attr: don't confuse prefixes with leading directories
Message-ID: <7vlipf9xbe.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120110182140.GB15273@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

> 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) {
>           ...

Yeah, that makes sense, as that existing check "attr_stack &&" was a
misguided defensive coding, that was _not_ defensive at all as we didn't
do anything after we stop iterating from that loop and without checking
dereferenced attr_stack->origin, which was a simple bogosity.

>
> 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).

And this is even more so.