{"thread":{"id":"46330","subject":"[PATCH] urlmatch: use hex2chr() in append_normalized_escapes()","startedAt":"2017-07-08T08:59:52Z","lastAt":"2017-07-08T15:15:42Z","messageCount":3,"participants":["René Scharfe","Kyle J. McKay"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"323988","messageId":"eb5e7bb5-d0a9-c8df-e89c-a2bd2430e8b6@web.de","threadId":"46330","inReplyTo":null,"subject":"[PATCH] urlmatch: use hex2chr() in append_normalized_escapes()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-08T08:59:19Z","receivedAt":"2017-07-08T08:59:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Simplify the code by using hex2chr() to convert and check for invalid\ncharacters at the same time instead of doing that sequentially with\none table lookup for each.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n urlmatch.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/urlmatch.c b/urlmatch.c\nindex 4bbde924e8..3e42bd7504 100644\n--- a/urlmatch.c\n+++ b/urlmatch.c\n@@ -42,12 +42,12 @@ static int append_normalized_escapes(struct strbuf *buf,\n \n \t\tfrom_len--;\n \t\tif (ch == '%') {\n-\t\t\tif (from_len < 2 ||\n-\t\t\t    !isxdigit(from[0]) ||\n-\t\t\t    !isxdigit(from[1]))\n+\t\t\tif (from_len < 2)\n \t\t\t\treturn 0;\n-\t\t\tch = hexval(*from++) << 4;\n-\t\t\tch |= hexval(*from++);\n+\t\t\tch = hex2chr(from);\n+\t\t\tif (ch < 0)\n+\t\t\t\treturn 0;\n+\t\t\tfrom += 2;\n \t\t\tfrom_len -= 2;\n \t\t\twas_esc = 1;\n \t\t}\n-- \n2.13.2\n"},{"id":"323997","messageId":"A1589486-3E84-494C-9B8D-3FB1724B3145@gmail.com","threadId":"46330","inReplyTo":"eb5e7bb5-d0a9-c8df-e89c-a2bd2430e8b6@web.de","subject":"Re: [PATCH] urlmatch: use hex2chr() in append_normalized_escapes()","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2017-07-08T14:28:53Z","receivedAt":"2017-07-08T14:29:01Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Jul 8, 2017, at 01:59, René Scharfe wrote:\n\n> Simplify the code by using hex2chr() to convert and check for invalid\n> characters at the same time instead of doing that sequentially with\n> one table lookup for each.\n\nI think that comment may be a bit misleading as the changes are just\nswitching from one set of inlines to another.  Essentially the same\nsequential check takes place in the hex2chr inlined function which is\nbeing used to replace the \"one table lookup for each\".  An optimizing\ncompiler will likely eliminate any difference between the before and\nafter patch versions.  Nothing immediately comes to mind as an alternate\ncomment though, so I'm not proposing any changes to the comment.\n\nThe before version only requires knowledge of the standards-defined  \nisxdigit\nand the hexval function which is Git-specific, but its semantics are  \nfairly\nobvious from the surrounding code.\n\nI suspect the casual reader of the function will have to go check and  \nsee\nwhat hex2chr does exactly.  For example, does it accept \"0x41\" or not?\n(It doesn't.)  What does it do with a single hex digit? (An error.)\nIt does do pretty much the same thing as the code it's\nreplacing (although that's not immediately obvious unless you go look\nat it), so this seems like a reasonable change.\n\n From the perspective of how many characters the original is versus how\nmany characters the replacement is, it's certainly a simplification.\n\nBut from the perspective of a reviewer of the urlmatch functionality\nattempting to determine how well the code does or does not match the\nrespective standards it requires more work.  Now one must examine the\nhex2chr function to be certain it doesn't include any extra unwanted\nbehavior with regards to how well urlmatch complies with the applicable\nstandards.  And in that sense it is not a simplification at all.\n\nBut that's all really just nit picking since hex2chr is a simple\ninlined function that's relatively easy to find (and understand).\n\nTherefore I don't have any objections to this change.\n\nAcked-by: Kyle J. McKay\n\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n> urlmatch.c | 10 +++++-----\n> 1 file changed, 5 insertions(+), 5 deletions(-)\n>\n> diff --git a/urlmatch.c b/urlmatch.c\n> index 4bbde924e8..3e42bd7504 100644\n> --- a/urlmatch.c\n> +++ b/urlmatch.c\n> @@ -42,12 +42,12 @@ static int append_normalized_escapes(struct  \n> strbuf *buf,\n>\n> \t\tfrom_len--;\n> \t\tif (ch == '%') {\n> -\t\t\tif (from_len < 2 ||\n> -\t\t\t    !isxdigit(from[0]) ||\n> -\t\t\t    !isxdigit(from[1]))\n> +\t\t\tif (from_len < 2)\n> \t\t\t\treturn 0;\n> -\t\t\tch = hexval(*from++) << 4;\n> -\t\t\tch |= hexval(*from++);\n> +\t\t\tch = hex2chr(from);\n> +\t\t\tif (ch < 0)\n> +\t\t\t\treturn 0;\n> +\t\t\tfrom += 2;\n> \t\t\tfrom_len -= 2;\n> \t\t\twas_esc = 1;\n> \t\t}\n\n\n\n"},{"id":"323998","messageId":"46294b4f-5e6d-bbf3-8b93-9d23df5ce07b@web.de","threadId":"46330","inReplyTo":"A1589486-3E84-494C-9B8D-3FB1724B3145@gmail.com","subject":"Re: [PATCH] urlmatch: use hex2chr() in append_normalized_escapes()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-07-08T15:15:30Z","receivedAt":"2017-07-08T15:15:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.07.2017 um 16:28 schrieb Kyle J. McKay:\n> On Jul 8, 2017, at 01:59, René Scharfe wrote:\n> \n>> Simplify the code by using hex2chr() to convert and check for invalid\n>> characters at the same time instead of doing that sequentially with\n>> one table lookup for each.\n> \n> I think that comment may be a bit misleading as the changes are just\n> switching from one set of inlines to another.  Essentially the same\n> sequential check takes place in the hex2chr inlined function which is\n> being used to replace the \"one table lookup for each\".  An optimizing\n> compiler will likely eliminate any difference between the before and\n> after patch versions.  Nothing immediately comes to mind as an alternate\n> comment though, so I'm not proposing any changes to the comment.\n\nRight, the table lookups for isxdigit and hexval are not duplicated when\ncompiling with -O2.\n\nRené\n"}]}