Re: [PATCH v2 14/19] hex: introduce parse_oid_hex

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Feb 17, 2017, 09:26 UTC
Message-ID: <dc402492-8b69-cf43-22d0-e4ec3af04e8c@alum.mit.edu>
In-Reply-To: <20170214023141.842922-15-sandals@crustytoothpaste.net>

On 02/14/2017 03:31 AM, brian m. carlson wrote:

Show 27 quoted lines

> Introduce a function, parse_oid_hex, which parses a hexadecimal object
> ID and if successful, sets a pointer to just beyond the last character.
> This allows for simpler, more robust parsing without needing to
> hard-code integer values throughout the codebase.
> 
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  cache.h | 8 ++++++++
>  hex.c   | 8 ++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/cache.h b/cache.h
> index 61fc86e6d7..5dc89a058c 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1319,6 +1319,14 @@ extern char *oid_to_hex_r(char *out, const struct object_id *oid);
>  extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
>  extern char *oid_to_hex(const struct object_id *oid);	/* same static buffer as sha1_to_hex */
>  
> +/*
> + * Parse a hexadecimal object ID starting from hex, updating the pointer
> + * specified by p when parsing stops.  The resulting object ID is stored in oid.
> + * Returns 0 on success.  Parsing will stop on the first NUL or other invalid
> + * character.
> + */
> +extern int parse_oid_hex(const char *hex, struct object_id *oid, const char **p);
> +

I like this function. This is a convenient kind of interface to work with. A few minor comments:

If you rename the nondescript `p` parameter to, say, `end`, its purpose would be more transparent. Alternatively, `skip_prefix()` calls the corresponding parameter `out`.

It would be nice for the docstring to mention that the object ID must be a full, 40-character hex string. Otherwise "Parsing will stop on the first NUL or other invalid character" makes it sound like the function might be satisfied with fewer than 40 characters.

Finally, you might mention the useful fact that `p` is only updated if the function succeeds.

> [...]

Michael