git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Cc: Michael Haggerty <mhagger@alum.mit.edu>,
	Stefan Beller <sbeller@google.com>
Subject: [PATCH] describe: confirm that blobs actually exist
Date: Mon, 12 Feb 2018 12:23:06 -0500	[thread overview]
Message-ID: <20180212172306.GA4918@sigill.intra.peff.net> (raw)

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
     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>
---
 builtin/describe.c  | 2 +-
 t/t6120-describe.sh | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 6fe1c51281..18c68ec7a4 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -502,7 +502,7 @@ static void describe(const char *arg, int last_one)
 
 	if (cmit)
 		describe_commit(&oid, &sb);
-	else if (lookup_blob(&oid))
+	else if (sha1_object_info(oid.hash, NULL) == OBJ_BLOB)
 		describe_blob(oid, &sb);
 	else
 		die(_("%s is neither a commit nor blob"), arg);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index a5d9015024..bae78c4e89 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -378,4 +378,12 @@ check_describe tags/A --all A
 check_describe tags/c --all c
 check_describe heads/branch_A --all --match='branch_*' branch_A
 
+test_expect_success 'describe complains about tree object' '
+	test_must_fail git describe HEAD^{tree}
+'
+
+test_expect_success 'describe complains about missing object' '
+	test_must_fail git describe $_z40
+'
+
 test_done
-- 
2.16.1.464.gc4bae515b7

             reply	other threads:[~2018-02-12 17:23 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-12 17:23 Jeff King [this message]
2018-02-12 17:29 ` [PATCH] describe: confirm that blobs actually exist Jeff King
2018-02-12 18:38 ` Stefan Beller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180212172306.GA4918@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=mhagger@alum.mit.edu \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).