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

On Mon, Feb 12, 2018 at 12:23:06PM -0500, Jeff King wrote:

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.

By the way, I did notice one other inefficiency here: we always call
lookup_commit_reference_gently() first, which will call parse_object().
So if you were to "git describe" an enormous blob, we'd load the whole
thing into memory for no purpose. We could structure this as:

  type = sha1_object_info(oid.hash, NULL);
  if (type == OBJ_BLOB)
          describe_blob(&oid);
  else if (lookup_commit_reference_gently(&oid, 1))
          describe_commit(&oid);
  else
          describe("neither commit nor blob");

That incurs an extra object lookup for the commit case, but potentially
saves reading the blob. We could have our cake and eat it, too, if
sha1_file.c had a function like "parse this object unless it's a blob,
in which case just fill in the type info".

Arguably that should be the default when parse_object() is called on a
blob, but I suspect some older code may rely on parse_object() to check
that the object is present and consistent.

Anyway, I don't know that it's really worth caring about too much, but
just something I noticed.

Maybe a #leftoverbits if somebody cares.

-Peff