Re: [PATCH v2 2/3] tag: don't misreport type of tagged objects in errors
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 30, 2022, 06:07 UTC
- Message-ID
- <xmqqsfgxjugi.fsf@gitster.g>
- In-Reply-To
- <patch-v2-2.3-96398731841-20221230T011725Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 24 quoted lines
> 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.
Show 9 quoted lines
> +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.
> +}