git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Stefan Beller <sbeller@google.com>
Date
Feb 12, 2018, 18:38 UTC
Message-ID
<CAGZ79kayhsgxuPjQ9koBEnpbM3Feq=mHEizdVwZa0EzR-aSEKg@mail.gmail.com>
In-Reply-To
<20180212172306.GA4918@sigill.intra.peff.net>
On Mon, Feb 12, 2018 at 9:23 AM, Jeff King <peff@peff.net> wrote:
Show 24 quoted lines
> 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
Show 22 quoted lines
>      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

Previous: Jeff King
Message 3 of 3 in “describe: confirm that blobs actually exist”
  1. describe: confirm that blobs actually existJeff King, Feb 12, 2018
  2. Jeff KingFeb 12, 2018
  3. Stefan BellerFeb 12, 2018

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.