Re: [PATCH 6/6] tag.c: use type_from_string_gently() when parsing tags

From: Jeff King <peff@peff.net>
Date: Apr 9, 2021, 18:42 UTC
Message-ID: <YHCgDm0KVygpjkNU@coredump.intra.peff.net>
In-Reply-To: <patch-6.6-3279d67d2b-20210409T082935Z-avarab@gmail.com>

On Fri, Apr 09, 2021 at 10:32:54AM +0200, Ævar Arnfjörð Bjarmason wrote:

> Change a series of strcmp() to instead use type_from_string_gently()
> to get the integer type early, and then use that for comparison.

The result looks much nicer.

One interesting note...

Show 10 quoted lines

> @@ -162,23 +162,24 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u
>  		return -1;
>  	bufptr += 5;
>  	nl = memchr(bufptr, '\n', tail - bufptr);
> -	if (!nl || sizeof(type) <= (nl - bufptr))
> +	if (!nl)
> +		return -1;
> +	type = type_from_string_gently(bufptr, nl - bufptr);
> +	if (type < 0)
>  		return -1;

If we got anything but the main-4 types here, we'll return an error. So we know here:

Show 17 quoted lines

> -	if (!strcmp(type, blob_type)) {
> +	if (type == OBJ_BLOB) {
>  		item->tagged = (struct object *)lookup_blob(r, &oid);
> -	} else if (!strcmp(type, tree_type)) {
> +	} else if (type == OBJ_TREE) {
>  		item->tagged = (struct object *)lookup_tree(r, &oid);
> -	} else if (!strcmp(type, commit_type)) {
> +	} else if (type == OBJ_COMMIT) {
>  		item->tagged = (struct object *)lookup_commit(r, &oid);
> -	} else if (!strcmp(type, tag_type)) {
> +	} else if (type == OBJ_TAG) {
>  		item->tagged = (struct object *)lookup_tag(r, &oid);
>  	} else {
>  		return error("unknown tag type '%s' in %s",
> -			     type, oid_to_hex(&item->object.oid));
> +			     type_name(type), oid_to_hex(&item->object.oid));
>  	}

That the final "else" clause can't be reached. I don't mind being defensive, but if it _is_ reached, then we'd feed that unknown type to type_name(), which will return NULL for unknown items (unless I guess it has also learned about the hypothetical new type).

I think this should just be:

  else {
          BUG("type_from_string gave us an unknown type: %d", (int)type);
  }

which makes it clear we expect the code can't be reached, and doesn't make any assumptions about what we can do with the odd value.

-Peff