Re: [PATCH] attr: don't confuse prefixes with leading directories
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 10, 2012, 20:25 UTC
- Message-ID
- <7v7h0z9ufu.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20120110192810.GA16018@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 27 quoted lines
> On Tue, Jan 10, 2012 at 11:23:01AM -0800, Junio C Hamano wrote:
>
>> > 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.
>
> I wasn't clear: the second one is "even more so" making sense, or "even
> more so" misguided defensive coding?Sorry for sending a half-baked response. The initial draft of my response had just "that makes sense" and nothing else in the first paragraph.
If the original meant to be defensive, it should have had your "extra defensive" die(), but it didn't.
But the condition to break out of that loop is either we hit an elem that satisfy the condition (in which case that elem cannot be NULL) or we successfully saw that attr_stack->origin is NULL (in which case attr_stack couldn't have been NULL), so it is pointless to check the NULLness of attr_stack itself. Assertion _before_ going into the while loop might make sense, but if we look at what bootstrap_attr_stack() does, it should be pretty obvious that that cannot happen.
An assert(attr_stack->origin) before we use it may make sense, though, in order to make sure we do not mistakenly pop the root one and expose the built-in ones whose origin are set to NULL.
diff --git a/attr.c b/attr.c index ad7eb9c..4d3b61a 100644 --- a/attr.c +++ b/attr.c @@ -566,7 +567,9 @@ static void prepare_attr_stack(const char *path, int dirlen) /* * Pop the ones from directories that are not the prefix of - * the path we are checking. + * the path we are checking. Break out of the loop when we see + * the root one (whose origin is an empty string "") or the builtin + * one (whose origin is NULL) without popping it. */ while (attr_stack->origin) { int namelen = strlen(attr_stack->origin); @@ -586,6 +589,13 @@ static void prepare_attr_stack(const char *path, int dirlen) * Read from parent directories and push them down */ if (!is_bare_repository() || direction == GIT_ATTR_INDEX) { + /* + * bootstrap_attr_stack() should have added, and the + * above loop should have stopped before popping, the + * root element whose attr_stack->origin is set to an + * empty string. + */ + assert(attr_stack->origin); while (1) { char *cp;