Re: [PATCH] urlmatch: use hex2chr() in append_normalized_escapes()

["Kyle J. McKay" <mackyle@gmail.com>]

On Jul 8, 2017, at 01:59, René Scharfe wrote:

> Simplify the code by using hex2chr() to convert and check for invalid
> characters at the same time instead of doing that sequentially with
> one table lookup for each.

I think that comment may be a bit misleading as the changes are just
switching from one set of inlines to another.  Essentially the same
sequential check takes place in the hex2chr inlined function which is
being used to replace the "one table lookup for each".  An optimizing
compiler will likely eliminate any difference between the before and
after patch versions.  Nothing immediately comes to mind as an alternate
comment though, so I'm not proposing any changes to the comment.

The before version only requires knowledge of the standards-defined  
isxdigit
and the hexval function which is Git-specific, but its semantics are  
fairly
obvious from the surrounding code.

I suspect the casual reader of the function will have to go check and  
see
what hex2chr does exactly.  For example, does it accept "0x41" or not?
(It doesn't.)  What does it do with a single hex digit? (An error.)
It does do pretty much the same thing as the code it's
replacing (although that's not immediately obvious unless you go look
at it), so this seems like a reasonable change.

 From the perspective of how many characters the original is versus how
many characters the replacement is, it's certainly a simplification.

But from the perspective of a reviewer of the urlmatch functionality
attempting to determine how well the code does or does not match the
respective standards it requires more work.  Now one must examine the
hex2chr function to be certain it doesn't include any extra unwanted
behavior with regards to how well urlmatch complies with the applicable
standards.  And in that sense it is not a simplification at all.

But that's all really just nit picking since hex2chr is a simple
inlined function that's relatively easy to find (and understand).

Therefore I don't have any objections to this change.

Acked-by: Kyle J. McKay

> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> urlmatch.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/urlmatch.c b/urlmatch.c
> index 4bbde924e8..3e42bd7504 100644
> --- a/urlmatch.c
> +++ b/urlmatch.c
> @@ -42,12 +42,12 @@ static int append_normalized_escapes(struct  
> strbuf *buf,
>
> 		from_len--;
> 		if (ch == '%') {
> -			if (from_len < 2 ||
> -			    !isxdigit(from[0]) ||
> -			    !isxdigit(from[1]))
> +			if (from_len < 2)
> 				return 0;
> -			ch = hexval(*from++) << 4;
> -			ch |= hexval(*from++);
> +			ch = hex2chr(from);
> +			if (ch < 0)
> +				return 0;
> +			from += 2;
> 			from_len -= 2;
> 			was_esc = 1;
> 		}