Re: [PATCH v2 39/53] refs/files-backend: convert many internals to struct object_id

From: Jonathan Tan <jonathantanmy@google.com>
Date: May 1, 2017, 23:24 UTC
Message-ID: <95a85924-6f5b-a625-e460-ecd14540a621@google.com>
In-Reply-To: <20170501022946.258735-40-sandals@crustytoothpaste.net>

On 04/30/2017 07:29 PM, brian m. carlson wrote:

Show 20 quoted lines

> @@ -195,27 +195,18 @@ static const char PACKED_REFS_HEADER[] =
>   * Return a pointer to the refname within the line (null-terminated),
>   * or NULL if there was a problem.
>   */
> -static const char *parse_ref_line(struct strbuf *line, unsigned char *sha1)
> +static const char *parse_ref_line(struct strbuf *line, struct object_id *oid)
>  {
>  	const char *ref;
>
> -	/*
> -	 * 42: the answer to everything.
> -	 *
> -	 * In this case, it happens to be the answer to
> -	 *  40 (length of sha1 hex representation)
> -	 *  +1 (space in between hex and name)
> -	 *  +1 (newline at the end of the line)
> -	 */
> -	if (line->len <= 42)
> +	if (!line->len)
>  		return NULL;

I would omit this check - parse_oid_hex already exits early if the first character is NUL. (The existing code makes a bit more sense, in that it avoids checking the first few characters if we already know a bit more about the string.)

Show 12 quoted lines

>
> -	if (get_sha1_hex(line->buf, sha1) < 0)
> +	if (parse_oid_hex(line->buf, oid, &ref) < 0)
>  		return NULL;
> -	if (!isspace(line->buf[40]))
> +	if (!isspace(*ref++))
>  		return NULL;
>
> -	ref = line->buf + 41;
>  	if (isspace(*ref))
>  		return NULL;
>

Looks fine, up to here.

(Also, you requested extra-careful review for this patch, but this patch seems mostly mechanical to me.)