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

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

[René Scharfe]

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.

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;
 		}
-- 
2.13.2

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

[Kyle J. McKay]

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;
> 		}

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

[René Scharfe]

Am 08.07.2017 um 16:28 schrieb Kyle J. McKay:
> 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.

Right, the table lookups for isxdigit and hexval are not duplicated when
compiling with -O2.

René