Re: [PATCH] describe: confirm that blobs actually exist

[Stefan Beller <sbeller@google.com>]

On Mon, Feb 12, 2018 at 9:23 AM, Jeff King <peff@peff.net> 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@alum.mit.edu>
> Signed-off-by: Jeff King <peff@peff.net>

This makes sense.
Reviewed-by: Stefan Beller <sbeller@google.com>

Thanks!
Stefan