Jeff King <peff@xxxxxxxx> writes:
> 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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html