{"thread":{"id":"47819","subject":"[PATCH] describe: confirm that blobs actually exist","startedAt":"2018-02-12T17:23:13Z","lastAt":"2018-02-12T18:38:46Z","messageCount":3,"participants":["Jeff King","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"339084","messageId":"20180212172306.GA4918@sigill.intra.peff.net","threadId":"47819","inReplyTo":null,"subject":"[PATCH] describe: confirm that blobs actually exist","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-12T17:23:06Z","receivedAt":"2018-02-12T17:23:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Prior to 644eb60bd0 (builtin/describe.c: describe a blob,\n2017-11-15), we noticed and complained about missing\nobjects, since they were not valid commits:\n\n $ git describe 0000000000000000000000000000000000000000\n fatal: 0000000000000000000000000000000000000000 is not a valid 'commit' object\n\nAfter that commit, we feed any non-commit to lookup_blob(),\nand complain only if it returns NULL. But the lookup_*\nfunctions do not actually look at the on-disk object\ndatabase at all. They return an entry from the in-memory\nobject hash if present (and if it matches the requested\ntype), and otherwise auto-create a \"struct object\" of the\nrequested type.\n\nA missing object would hit that latter case: we create a\nbogus blob struct, walk all of history looking for it, and\nthen exit successfully having produced no output.\n\nOne reason nobody may have noticed this is that some related\ncases do still work OK:\n\n 1. If we ask for a tree by sha1, then the call to\n lookup_commit_referecne_gently() would have parsed it,\n and we would have its true type in the in-memory object\n hash.\n\n 2. If we ask for a name that doesn't exist but isn't a\n 40-hex sha1, then get_oid() would complain before we\n even look at the objects at all.\n\nWe can fix this by replacing the lookup_blob() call with a\ncheck of the true type via sha1_object_info(). This is not\nquite as efficient as we could possibly make this check. We\nknow in most cases that the object was already parsed in the\nearlier commit lookup, so we could call lookup_object(),\nwhich does auto-create, and check the resulting struct's\ntype (or NULL). However it's not worth the fragility nor\ncode complexity to save a single object lookup.\n\nThe new tests cover this case, as well as that of a\ntree-by-sha1 (which does work as described above, but was\nnot explicitly tested).\n\nNoticed-by: Michael Haggerty \nSigned-off-by: Jeff King \n---\n builtin/describe.c | 2 +-\n t/t6120-describe.sh | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 6fe1c51281..18c68ec7a4 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -502,7 +502,7 @@ static void describe(const char *arg, int last_one)\n \n \tif (cmit)\n \t\tdescribe_commit(&oid, &sb);\n-\telse if (lookup_blob(&oid))\n+\telse if (sha1_object_info(oid.hash, NULL) == OBJ_BLOB)\n \t\tdescribe_blob(oid, &sb);\n \telse\n \t\tdie(_(\"%s is neither a commit nor blob\"), arg);\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex a5d9015024..bae78c4e89 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -378,4 +378,12 @@ check_describe tags/A --all A\n check_describe tags/c --all c\n check_describe heads/branch_A --all --match='branch_*' branch_A\n \n+test_expect_success 'describe complains about tree object' '\n+\ttest_must_fail git describe HEAD^{tree}\n+'\n+\n+test_expect_success 'describe complains about missing object' '\n+\ttest_must_fail git describe $_z40\n+'\n+\n test_done\n-- \n2.16.1.464.gc4bae515b7\n"},{"id":"339085","messageId":"20180212172921.GA5004@sigill.intra.peff.net","threadId":"47819","inReplyTo":"20180212172306.GA4918@sigill.intra.peff.net","subject":"Re: [PATCH] describe: confirm that blobs actually exist","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-12T17:29:21Z","receivedAt":"2018-02-12T17:29:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 12, 2018 at 12:23:06PM -0500, Jeff King wrote:\n\n> We can fix this by replacing the lookup_blob() call with a\n> check of the true type via sha1_object_info(). This is not\n> quite as efficient as we could possibly make this check. We\n> know in most cases that the object was already parsed in the\n> earlier commit lookup, so we could call lookup_object(),\n> which does auto-create, and check the resulting struct's\n> type (or NULL). However it's not worth the fragility nor\n> code complexity to save a single object lookup.\n\nBy the way, I did notice one other inefficiency here: we always call\nlookup_commit_reference_gently() first, which will call parse_object().\nSo if you were to \"git describe\" an enormous blob, we'd load the whole\nthing into memory for no purpose. We could structure this as:\n\n type = sha1_object_info(oid.hash, NULL);\n if (type == OBJ_BLOB)\n describe_blob(&oid);\n else if (lookup_commit_reference_gently(&oid, 1))\n describe_commit(&oid);\n else\n describe(\"neither commit nor blob\");\n\nThat incurs an extra object lookup for the commit case, but potentially\nsaves reading the blob. We could have our cake and eat it, too, if\nsha1_file.c had a function like \"parse this object unless it's a blob,\nin which case just fill in the type info\".\n\nArguably that should be the default when parse_object() is called on a\nblob, but I suspect some older code may rely on parse_object() to check\nthat the object is present and consistent.\n\nAnyway, I don't know that it's really worth caring about too much, but\njust something I noticed.\n\nMaybe a #leftoverbits if somebody cares.\n\n-Peff\n"},{"id":"339089","messageId":"CAGZ79kayhsgxuPjQ9koBEnpbM3Feq=mHEizdVwZa0EzR-aSEKg@mail.gmail.com","threadId":"47819","inReplyTo":"20180212172306.GA4918@sigill.intra.peff.net","subject":"Re: [PATCH] describe: confirm that blobs actually exist","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-02-12T18:38:40Z","receivedAt":"2018-02-12T18:38:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 12, 2018 at 9:23 AM, Jeff King wrote:\n> Prior to 644eb60bd0 (builtin/describe.c: describe a blob,\n> 2017-11-15), we noticed and complained about missing\n> objects, since they were not valid commits:\n>\n> $ git describe 0000000000000000000000000000000000000000\n> fatal: 0000000000000000000000000000000000000000 is not a valid 'commit' object\n>\n> After that commit, we feed any non-commit to lookup_blob(),\n> and complain only if it returns NULL. But the lookup_*\n> functions do not actually look at the on-disk object\n> database at all. They return an entry from the in-memory\n> object hash if present (and if it matches the requested\n> type), and otherwise auto-create a \"struct object\" of the\n> requested type.\n>\n> A missing object would hit that latter case: we create a\n> bogus blob struct, walk all of history looking for it, and\n> then exit successfully having produced no output.\n>\n> One reason nobody may have noticed this is that some related\n> cases do still work OK:\n>\n> 1. If we ask for a tree by sha1, then the call to\n> lookup_commit_referecne_gently() would have parsed it,\n\nlookup_commit_reference_gently\n\n> and we would have its true type in the in-memory object\n> hash.\n>\n> 2. If we ask for a name that doesn't exist but isn't a\n> 40-hex sha1, then get_oid() would complain before we\n> even look at the objects at all.\n>\n> We can fix this by replacing the lookup_blob() call with a\n> check of the true type via sha1_object_info(). This is not\n> quite as efficient as we could possibly make this check. We\n> know in most cases that the object was already parsed in the\n> earlier commit lookup, so we could call lookup_object(),\n> which does auto-create, and check the resulting struct's\n> type (or NULL). However it's not worth the fragility nor\n> code complexity to save a single object lookup.\n>\n> The new tests cover this case, as well as that of a\n> tree-by-sha1 (which does work as described above, but was\n> not explicitly tested).\n>\n> Noticed-by: Michael Haggerty \n> Signed-off-by: Jeff King \n\nThis makes sense.\nReviewed-by: Stefan Beller \n\nThanks!\nStefan\n"}]}