On Tue, Jan 10, 2012 at 12:25:09PM -0800, Junio C Hamano wrote:
quoted hunk ↗ jump to hunk
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.
[...]
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;
Yeah, this version looks good to me (though I thought we usually spelled
assert as die("BUG: ...")).
-Peff