On Wed, Mar 31 2021, Jeff King wrote:
> On Mon, Mar 29, 2021 at 10:50:18PM -0700, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>>
>> > 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 ;-)
>
> So going with the discussion elsewhere in the thread, I'd probably say
> something like:
>
> error: object <oid> seen as both a commit and a tree
>
> which precisely says what we do know, without implying which is correct.
>
> Ævar's patch tries to improve the case where we might _know_ which is
> correct (because we're actually parsing the object contents), but of
> course it covers only a fraction of cases. I'm not really opposed to
> that per se, but I probably wouldn't bother myself.
What fraction of cases? As far as I can tell it covers all cases where
we get this error.
If there is a case like what you're describing I haven't found it.
I.e. it happens when we have an un-parsed "struct object" whose type is
inferred, and parse it to find out it's not what we expected.
It's not ambigious at all what the object actually is. It's just that
the previous code was leaking the *assumption* about the type at the
time of emitting the error, due to an apparent oversight with parsed
v.s. non-parsed.
Or in other words, we're leaking the implementation detail that we
pre-allocated an object struct of a given type in anticipation of
holding a parsed version of that object soon.
> Side note: this is all making the assumption that what is in the
> object itself is "correct", but of course that is not necessarily
> true, even. All of these cases are the result of bugs, so it is
> possible that the bug was in the writing of the original object
> contents, and not the object that is referring to it. Likewise, I'd
> imagine an easy way to get into this situation is with a bogus
> refs/replace object that switches type.
Perhaps, I haven't tested that in any detail.
>> > 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").
>
> Yeah, I also found that very confusing. AFAICT lookup_tag_type() would
> only ever see OBJ_NONE or OBJ_TAG. Making it more than a boolean makes
> both the interface and implementation more complicated.
I don't feel strongly either way, but one concern here is that these are
very hot functions, and maybe it's better to give the compiler a better
chance to work with them without considering an extra argument, but I
haven't tested that...
> I also think the manual handling of OBJ_NONE in each lookup_* function
> is confusing. They all call object_as_type() because the point of that
> function is both to type-check the struct and to convert it away from
> OBJ_NONE.
>
> If we handled this error there, then I think it would be much more
> natural, because we'd have already covered the OBJ_NONE case, and
> because it's already the place we're emitting the existing error. E.g.:
>
> diff --git a/object.c b/object.c
> index 2c32691dc4..e6345541f7 100644
> --- a/object.c
> +++ b/object.c
> @@ -157,7 +157,7 @@ void *create_object(struct repository *r, const struct object_id *oid, void *o)
> return obj;
> }
>
> -void *object_as_type(struct object *obj, enum object_type type, int quiet)
> +void *object_as_type(struct object *obj, enum object_type type, unsigned flags)
> {
> if (obj->type == type)
> return obj;
> @@ -169,10 +169,16 @@ void *object_as_type(struct object *obj, enum object_type type, int quiet)
> return obj;
> }
> else {
> - if (!quiet)
> - error(_("object %s is a %s, not a %s"),
> - oid_to_hex(&obj->oid),
> - type_name(obj->type), type_name(type));
> + if (!(flags & OBJECT_AS_TYPE_QUIET)) {
> + if (flags & OBJECT_AS_TYPE_EXPECT_PARSED)
> + error(_("object %s is a %s, but was referred to as a %s"),
> + oid_to_hex(&obj->oid), type_name(obj->type),
> + type_name(type));
> + else
> + error(_("object %s referred to as both a %s and a %s"),
> + oid_to_hex(&obj->oid),
> + type_name(obj->type), type_name(type));
> + }
> return NULL;
> }
> }
Per the above I don't understand how you think there's any uncertainty
here.
If I'm right and there isn't then first of all I don't see how we could
emit 1/2 of those errors. The whole problem here is that we don't know
the type of the un-parsed object (and presumably don't want to eagerly
know, it would mean hitting the object store).
But when we do know why would we beat around the bush and say "was
referred to as X and Y" once we know what it is.
AFAICT there's no more reason to think that parse_object_buffer() will
be wrong about the type than "git cat-file -t" will be. They both use
the same underlying functions to get that information.
