On Sep 12, 2013, at 11:30, Junio C Hamano wrote:
+ /* append_normalized_escapes can cause norm.buf to change */
+ seg_start = norm.buf + seg_start_off;
The change looks good, but I find that this comment is not placed in
the right place. It is good if the reader knows about an old bug to
put it here, but if the first thing a reader reads is this updated
version, the comment is better placed close to the place where the
start_ofs variable captures the original value (i.e. "because the
next call may relocate the buffer, we cannot grab seg_start upfront;
instead we need to record the start_ofs here, and that is what this
variable is about").
It is too minor a point for a reroll, so I'll try to tweak it
locally. Something like this (but now I think about it, the comment
may not even be necessary).
The longer comment looks good to me. If you think the code will be
safe from
simplification patches without a comment, that works for me too. I've
just seen
so many "simplification" patches go by on the list I'm concerned it
will be a
target otherwise leading to re-introduction of the problem.
diff --git a/urlmatch.c b/urlmatch.c
index 01c6746..d1600e2 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -282,9 +282,17 @@ char *url_normalize(const char *url, struct
url_info *out_info)
}
for (;;) {
const char *seg_start;
- size_t seg_start_off = norm.len;
+ size_t seg_start_off;
const char *next_slash = url + strcspn(url, "/?#");
int skip_add_slash = 0;
+
+ /*
+ * record the starting offset; appending escapes may
+ * relocate the buffer, so we cannot capture seg_start
+ * upfront and use it later.
+ */
+ seg_start_off = norm.len;
+
/*
* RFC 3689 indicates that any . or .. segments should be
* unescaped before being checked for.
@@ -298,7 +306,7 @@ char *url_normalize(const char *url, struct
url_info *out_info)
strbuf_release(&norm);
return NULL;
}
- /* append_normalized_escapes can cause norm.buf to change */
+
seg_start = norm.buf + seg_start_off;
if (!strcmp(seg_start, ".")) {
/* ignore a . segment; be careful not to remove initial '/' */
--
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