Re: [PATCH] describe: confirm that blobs actually exist
- Date: Mon, 12 Feb 2018 10:38:40 -0800
- From: Stefan Beller <sbeller@xxxxxxxxxx>
- Subject: Re: [PATCH] describe: confirm that blobs actually exist
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,
> and we would have its true type in the in-memory object
> 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>