Re: [PATCH v2 2/3] tag: don't misreport type of tagged objects in errors

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/blob.c b/blob.c
> index 8f83523b0cd..d6e3d64213d 100644
> --- a/blob.c
> +++ b/blob.c
> @@ -5,12 +5,19 @@
>  
>  const char *blob_type = "blob";
>  
> -struct blob *lookup_blob(struct repository *r, const struct object_id *oid)
> +struct blob *lookup_blob_type(struct repository *r,
> +			      const struct object_id *oid,
> +			      enum object_type type)
>  {
>  	struct object *obj = lookup_object(r, oid);
>  	if (!obj)
>  		return create_object(r, oid, alloc_blob_node(r));
> -	return object_as_type(obj, OBJ_BLOB, 0);
> +	return object_as_type_hint(obj, OBJ_BLOB, type);
> +}
> +
> +struct blob *lookup_blob(struct repository *r, const struct object_id *oid)
> +{
> +	return lookup_blob_type(r, oid, OBJ_NONE);
>  }

Between lookup_blob() and lookup_blob_type(), the distinction is
that the latter calls object_as_type_hint() to ensure that the real
type of the given object is of the type, which is given as "hint"?

Very confusing naming.  Perhaps because "hint" argument given to
"as-type-hint" is playing a role that is FAR stronger than "hint"?
It sounds more like enforcement.  Perhaps s/hint/check/ or something?

I am not sure exactly why, but lookup_blob_type() does look
confusing.  A natural question anybody would ask after seeing the
function mame and the extra parmater is: "If we want/expect to see a
blob, why do we give an extra type?"  To answer that question, "_type"
at the end of the function name and "type" that is an extra argument
should say what that thing is used for.  "enum object_type" is enough
to say what the extra argument is---we should use the parameter name
to convey what it is there for.

> +void *object_as_type_hint(struct object *obj, enum object_type type,
> +			  enum object_type hint)
> +{
> +	if (hint != OBJ_NONE && obj->type != OBJ_NONE && obj->type != type) {
> +		error(_("object %s is a %s, not a %s"), oid_to_hex(&obj->oid),
> +		      type_name(type), type_name(obj->type));
> +		obj->type = type;
> +		return NULL;
> +	}

There must be something utterly wrong in the above implementation.
As long as "hint" is ANY type other than OBJ_NONE, "check if type
and obj->type match and otherwise complain" logic kicks in, and the
actual value of "hint" does not contribute anything to the outcome.

IOW, the "hint" parameter is misleading to be "enum object_type", as
it is only used as a Boolean "do we bother to check?".

> +	return object_as_type(obj, type, 0);;

Stray extra semicolon at the end.

> +}