Re: [PATCH v2 10/10] tag: don't misreport type of tagged objects in errors
flat view
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-03-31 18:32:05
Possibly related (same subject, not in this thread)
- 2021-03-31 · Re: [PATCH v2 10/10] tag: don't misreport type of tagged objects in errors · Jeff King <hidden>
- 2021-03-31 · Re: [PATCH v2 10/10] tag: don't misreport type of tagged objects in errors · Junio C Hamano <hidden>
- 2021-03-31 · Re: [PATCH v2 10/10] tag: don't misreport type of tagged objects in errors · Junio C Hamano <hidden>
- 2021-03-28 · [PATCH v2 10/10] tag: don't misreport type of tagged objects in errors · Ævar Arnfjörð Bjarmason <hidden>
On Wed, Mar 31 2021, Jeff King wrote:
On Mon, Mar 29, 2021 at 10:50:18PM -0700, Junio C Hamano wrote:quoted
Ævar Arnfjörð Bjarmason [off-list ref] writes:quoted
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.
quoted
quoted
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...
quoted hunk ↗ jump to hunk
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.