From: Kyle J. McKay <mackyle@gmail.com>
Date: Thu, 12 Sep 2013 20:38:37 GMT
Subject: Re: [PATCH v2] urlmatch.c: recompute ptr after append_normalized_escapes
Message-ID: <85CEBFAA-8905-46EC-88C5-5DDE629B73F9@gmail.com>
In-Reply-To: <xmqqioy5q4dd.fsf@gitster.dls.corp.google.com>

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 '/' */