On 06/09/2015 06:28 PM, brian m. carlson wrote:
> get_oid_hex is already available for parsing hex object IDs into struct
> object_id, but parsing code still must hard-code the number of bytes
> read. Introduce parse_oid_hex, which accepts an optional length, and
> also returns the number of bytes parsed on success, or 0 on failure.
> This makes it easier for code not to assume fixed values when parsing,
> and to move to larger hash functions in the future.
I have an open mind, but I have the feeling that this will not be a very
handy interface:
* It will usually require a new int variable to catch the return value.
* We don't put assignments within conditional expressions. So often this
will require one line of code to call the function and a second one to
test the return value.
For example, consider how awkward it will be to rewrite code that now
looks like
if (hex && !get_sha1_hex(hex, sha1) && hex[40] == '\n')
You might instead consider the style of interface used by skip_prefix(),
which I've really come to like:
int parse_oid_hex(const char *hex, struct object_id *oid,
const char **out)
{
if (get_sha1_hex(hex, oid->hash))
return -1; /* failure */
if (out)
*out = hex + GIT_SHA1_HEXSZ;
return 0;
}
This function could of course take a "len" parameter too. Note that it
uses our usual convention that success is denoted by returning 0.
With this definition, the above code could be rewritten to (if the old
value of hex is not needed again)
if (hex && !parse_oid_hex(hex, &oid, &hex) && *hex == '\n')
> ---
> cache.h | 9 +++++++++
> hex.c | 7 +++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/cache.h b/cache.h
> index fa1f067..f3b829f 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1012,6 +1012,15 @@ extern int for_each_abbrev(const char *prefix, each_abbrev_fn, void *);
> extern int get_sha1_hex(const char *hex, unsigned char *sha1);
> extern int get_oid_hex(const char *hex, struct object_id *sha1);
>
> +/*
> + * Like get_oid_hex, but accepts an optional length argument, which may be -1
> + * if the string is terminated by a non-hex character. As with get_oid_hex,
> + * reading stops if a NUL is encountered. Returns the number of characters
> + * read (40) on success and 0 on failure. This is designed to be easier to
> + * use for parsing data than get_oid_hex.
> + */
> +extern int parse_oid_hex(const char *hex, int len, 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 */
> extern int read_ref_full(const char *refname, int resolve_flags,
> diff --git a/hex.c b/hex.c
> index 899b74a..ba196d7 100644
> --- a/hex.c
> +++ b/hex.c
> @@ -61,6 +61,13 @@ 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, int len, struct object_id *oid)
> +{
> + if (len != -1 && len < GIT_SHA1_HEXSZ)
> + return 0;
> + return get_sha1_hex(hex, oid->hash) ? GIT_SHA1_HEXSZ : 0;
> +}
> +
Isn't this return-statement logic backwards? get_sha1_hex() returns 0 on
success, which you would want to convert to 40. This function would want
to return 0 on *error*.
Which is another disadvantage of this style. All the code that called
get_sha1_hex() had to treat 0 as success. With your function they would
have to be inverted to treat 0 as failure. I think this would cause
confusion.
> [...]
Michael
--
Michael Haggerty
mhagger@xxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html