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

[Michael Haggerty]

On 02/14/2017 03:31 AM, brian m. carlson wrote:
> 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 
> ---
>  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

[PATCH v2 14/19] hex: introduce parse_oid_hex

[brian m. carlson]

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 
---
 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);
+
 extern int interpret_branch_name(const char *str, int len, struct strbuf *);
 extern int get_oid_mb(const char *str, struct object_id *oid);
 
diff --git a/hex.c b/hex.c
index 845b01a874..54252b1445 100644
--- a/hex.c
+++ b/hex.c
@@ -53,6 +53,14 @@ int get_oid_hex(const char *hex, struct object_id *oid)
return get_sha1_hex(hex, oid->hash);
 }
 
+int parse_oid_hex(const char *hex, struct object_id *oid, const char **p)
+{
+   int ret = get_oid_hex(hex, oid);
+   if (!ret)
+   *p = hex + GIT_SHA1_HEXSZ;
+   return ret;
+}
+
 char *sha1_to_hex_r(char *buffer, const unsigned char *sha1)
 {
static const char hex[] = "0123456789abcdef";
-- 
2.11.0