Jeff King [off-list ref] 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.
On Tue, Jan 10, 2012 at 11:23:01AM -0800, Junio C Hamano wrote:
quoted
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.
quoted
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?
-Peff
On Tue, Jan 10, 2012 at 02:28:10PM -0500, Jeff King wrote:
On Tue, Jan 10, 2012 at 11:23:01AM -0800, Junio C Hamano wrote:
quoted
quoted
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.
quoted
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?
If the latter, then I think we want this:
-- >8 --
Subject: [PATCH] attr: drop misguided defensive coding
In prepare_attr_stack, we pop the old elements of the stack
(which were left from a previous lookup and may or may not
be useful to us). Our loop to do so checks that we never
reach the top of the stack. However, the code immediately
afterwards will segfault if we did actually reach the top of
the stack.
Fortunately, this is not an actual bug, since we will never
pop all of the stack elements (we will always keep the root
gitattributes, as well as the builtin ones). So the extra
check in the loop condition simply clutters the code and
makes the intent less clear. Let's get rid of it.
Signed-off-by: Jeff King <redacted>
---
attr.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/attr.c b/attr.c
index fa975da..cf8f2bc 100644
--- a/attr.c
+++ b/attr.c
@@ -577,7 +577,7 @@ static void prepare_attr_stack(const char *path)
* Pop the ones from directories that are not the prefix of
* the path we are checking.
*/
- while (attr_stack && attr_stack->origin) {
+ while (attr_stack->origin) {
int namelen = strlen(attr_stack->origin);
elem = attr_stack;--
1.7.9.rc0.33.gd3c17