Re: [RFC PATCH 1/6] hex: add functionality for lowercase-only hex

[Junio C Hamano <gitster@pobox.com>]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> We currently allow both upper and lower case for all hex values in Git.
> However, in a future commit, we'll want to change that to allow only
> lowercase values in some cases.  To prepare for that case, provide a
> table to convert hex values using lowercase only and an enum to let us
> choose which we want, wiring it up to the hexval function.
>
> For now, keep things completely the same by specifying only the
> variant that accepts both lowercase and uppercase to avoid changing
> behavior.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---

In "some" cases?  I wonder what other cases there are that we MUST
accept uppercase variants.  Obviously the network protocol where we
are willing to talk to reimplementation of Git by others is one.  I
do not think we historically produced anything in uppercase.

> diff --git a/color.c b/color.c
> index 00b53f97ac..9015d0faf1 100644
> --- a/color.c
> +++ b/color.c
> @@ -72,7 +72,7 @@ static int get_hex_color(const char **inp, int width, unsigned char *out)
>  	unsigned int val;
>  
>  	assert(width == 1 || width == 2);
> -	val = (hexval(in[0]) << 4) | hexval(in[width - 1]);
> +	val = (hexval(in[0], HEX_KIND_MIXED) << 4) | hexval(in[width - 1], HEX_KIND_MIXED);
>  	if (val & ~0xff)
>  		return -1;
>  	*inp += width;
> diff --git a/hex-ll.c b/hex-ll.c
> index 4d7ece1de5..fa85e91827 100644
> --- a/hex-ll.c
> +++ b/hex-ll.c
> @@ -36,10 +36,45 @@ const signed char hexval_table[256] = {
>  	 -1, -1, -1, -1, -1, -1, -1, -1,		/* f8-ff */
>  };
>  
> +const signed char hexval_lc_table[256] = {
> +	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 00-07 */
> +	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 08-0f */
> +	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 10-17 */
> +	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 18-1f */
> +	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 20-27 */
> +	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 28-2f */
> +	  0,  1,  2,  3,  4,  5,  6,  7,		/* 30-37 */
> +	  8,  9, -1, -1, -1, -1, -1, -1,		/* 38-3f */
> +	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 40-47 */
> +	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 48-4f */
> +	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 50-57 */
> +	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 58-5f */
> +	 -1, 10, 11, 12, 13, 14, 15, -1,		/* 60-67 */
> ...
> +	 -1, -1, -1, -1, -1, -1, -1, -1,		/* f8-ff */
> +};
> +
>  int hex_to_bytes(unsigned char *binary, const char *hex, size_t len)
>  {
>  	for (; len; len--, hex += 2) {
> -		unsigned int val = (hexval(hex[0]) << 4) | hexval(hex[1]);
> +		unsigned int val = (hexval(hex[0], HEX_KIND_MIXED) << 4) | hexval(hex[1], HEX_KIND_MIXED);
>  
>  		if (val & ~0xff)
>  			return -1;
>
> diff --git a/hex-ll.h b/hex-ll.h
> index a381fa8556..da1b5239b2 100644
> --- a/hex-ll.h
> +++ b/hex-ll.h
> @@ -1,10 +1,16 @@
>  #ifndef HEX_LL_H
>  #define HEX_LL_H
>  
> +enum hexkind {
> +	HEX_KIND_MIXED = 0,
> +	HEX_KIND_LOWER = 1,
> +};
> +
>  extern const signed char hexval_table[256];
> -static inline unsigned int hexval(unsigned char c)
> +extern const signed char hexval_lc_table[256];
> +static inline unsigned int hexval(unsigned char c, enum hexkind kind)
>  {
> -	return hexval_table[c];
> +	return kind == HEX_KIND_MIXED ? hexval_table[c] : hexval_lc_table[c];
>  }

It is very welcome to make sure we are conservative in what we
produce, but be liberal in what we accept.  In that sense, use of
HEX_KIND_LOWER goes directly against the Robustness Principle.

>  
>  /*
> @@ -13,8 +19,8 @@ static inline unsigned int hexval(unsigned char c)
>   */
>  static inline int hex2chr(const char *s)
>  {
> -	unsigned int val = hexval(s[0]);
> -	return (val & ~0xf) ? val : (val << 4) | hexval(s[1]);
> +	unsigned int val = hexval(s[0], HEX_KIND_MIXED);
> +	return (val & ~0xf) ? val : (val << 4) | hexval(s[1], HEX_KIND_MIXED);
>  }
>  
>  /*
> diff --git a/pkt-line.c b/pkt-line.c
> index 3fc3e9ea70..338075558c 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -378,10 +378,10 @@ int packet_length(const char lenbuf_hex[4], size_t size)
>  {
>  	if (size < 4)
>  		BUG("buffer too small");
> -	return	hexval(lenbuf_hex[0]) << 12 |
> -		hexval(lenbuf_hex[1]) <<  8 |
> -		hexval(lenbuf_hex[2]) <<  4 |
> -		hexval(lenbuf_hex[3]);
> +	return	hexval(lenbuf_hex[0], HEX_KIND_MIXED) << 12 |
> +		hexval(lenbuf_hex[1], HEX_KIND_MIXED) <<  8 |
> +		hexval(lenbuf_hex[2], HEX_KIND_MIXED) <<  4 |
> +		hexval(lenbuf_hex[3], HEX_KIND_MIXED);
>  }
>  
>  static const char *find_packfile_uri_path(const char *buffer)