On Mon, Feb 12, 2018 at 9:23 AM, Jeff King <peff@xxxxxxxx> wrote: > Prior to 644eb60bd0 (builtin/describe.c: describe a blob, > 2017-11-15), we noticed and complained about missing > objects, since they were not valid commits: > > $ git describe 0000000000000000000000000000000000000000 > fatal: 0000000000000000000000000000000000000000 is not a valid 'commit' object > > After that commit, we feed any non-commit to lookup_blob(), > and complain only if it returns NULL. But the lookup_* > functions do not actually look at the on-disk object > database at all. They return an entry from the in-memory > object hash if present (and if it matches the requested > type), and otherwise auto-create a "struct object" of the > requested type. > > A missing object would hit that latter case: we create a > bogus blob struct, walk all of history looking for it, and > then exit successfully having produced no output. > > One reason nobody may have noticed this is that some related > cases do still work OK: > > 1. If we ask for a tree by sha1, then the call to > lookup_commit_referecne_gently() would have parsed it, lookup_commit_reference_gently > and we would have its true type in the in-memory object > hash. > > 2. If we ask for a name that doesn't exist but isn't a > 40-hex sha1, then get_oid() would complain before we > even look at the objects at all. > > We can fix this by replacing the lookup_blob() call with a > check of the true type via sha1_object_info(). This is not > quite as efficient as we could possibly make this check. We > know in most cases that the object was already parsed in the > earlier commit lookup, so we could call lookup_object(), > which does auto-create, and check the resulting struct's > type (or NULL). However it's not worth the fragility nor > code complexity to save a single object lookup. > > The new tests cover this case, as well as that of a > tree-by-sha1 (which does work as described above, but was > not explicitly tested). > > Noticed-by: Michael Haggerty <mhagger@xxxxxxxxxxxx> > Signed-off-by: Jeff King <peff@xxxxxxxx> This makes sense. Reviewed-by: Stefan Beller <sbeller@xxxxxxxxxx> Thanks! Stefan
