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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 30, 2021, 05:50 UTC
Message-ID: <xmqq35wdfaw5.fsf@gitster.g>
In-Reply-To: <patch-10.11-a84f670ac24-20210328T021238Z-avarab@gmail.com>

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

Show 18 quoted lines

> Fix a regression in 89e4202f982 ([PATCH] Parse tags for absent
> objects, 2005-06-21) (yes, that ancient!) and correctly report an
> error on a tag like:
>
>     object <a tree hash>
>     type commit
>
> As:
>
>     error: object <a tree hash> is tree, not a commit
>
> Instead of our long-standing misbehavior of inverting the two, and
> reporting:
>
>     error: object <a tree hash> is commit, not a tree
>
> Which, as can be trivially seen with 'git cat-file -t <a tree hash>'
> is incorrect.

Hmph, I've always thought it is just "supposed to be a" missing in the sentence ;-)

> Hence the non-intuitive solution of adding a
> lookup_{blob,commit,tag,tree}_type() function. It's to distinguish
> calls from parse_object_buffer() where we actually know the type, from
> a parse_tag_buffer() where we're just guessing about the type.

I think it makes sense to allow the caller to express distinction between "I know that this object is a blob, because I just read its object header" and "Another object tells me that this object must be a blob, because it is in a tree entry whose mode bits are 100644".

I wish we found a set of names better than lookup_<type>_type() for that, though. It's just between

      lookup_tag_type(r, oid, OBJ_NONE);
      lookup_tag_type(r, oid, OBJ_TAG);

I cannot quite tell which one is which. I also wonder if the last arg should just be a boolean ("I know it is a tag" vs "I heard it must be a tag").