{"thread":{"id":"58806","subject":"[RFC][PATCH] object.c: use has_object() instead of repo_has_object_file()","startedAt":"2022-11-16T16:45:47Z","lastAt":"2022-12-30T06:07:15Z","messageCount":22,"participants":["Kousik Sanagavarapu","Jeff King","Jonathan Tan","Ævar Arnfjörð Bjarmason","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"467375","messageId":"20221116163956.1039137-1-five231003@gmail.com","threadId":"58806","inReplyTo":null,"subject":"[RFC][PATCH] object.c: use has_object() instead of repo_has_object_file()","fromName":"Kousik Sanagavarapu","fromEmail":"five231003@gmail.com","sentAt":"2022-11-16T16:39:56Z","receivedAt":"2022-11-16T16:45:47Z","isPatch":true,"sender":{"key":"five231003@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75560439?v=4"},"body":"It is mentioned in object-store.h that the function\nrepo_has_object_file() is deprecated. One possible alternative for this\nfunction is has_object() (or atleast that is how I understood it).\n\nThe file object-store.h also mentions that repo_has_object_file() and\nits fellow functions and macros can be removed once the migrations take\nplace. This patch therefore is an attempt to reduce the usage of these\nfunctions and macros.\n\nI request for comments as I'm not really sure about the \"flags\" argument\nof the has_object() function and its usage in this patch.\n\nSigned-off-by: Kousik Sanagavarapu \n---\n object.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex 8a74eb85e9..0a9516137a 100644\n--- a/object.c\n+++ b/object.c\n@@ -286,8 +286,8 @@ struct object *parse_object_with_flags(struct repository *r,\n \t\t\treturn &commit->object;\n \t}\n \n-\tif ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||\n-\t (!obj && repo_has_object_file(r, oid) &&\n+\tif ((obj && obj->type == OBJ_BLOB && has_object(r, oid, 0)) ||\n+\t (!obj && has_object(r, oid, 0) &&\n \t oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\n \t\tif (!skip_hash && stream_object_signature(r, repl) < 0) {\n \t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n-- \n2.25.1\n\n"},{"id":"467376","messageId":"Y3Up5Vi75Up8LaGQ@coredump.intra.peff.net","threadId":"58806","inReplyTo":"20221116163956.1039137-1-five231003@gmail.com","subject":"Re: [RFC][PATCH] object.c: use has_object() instead of repo_has_object_file()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-16T18:20:21Z","receivedAt":"2022-11-16T18:20:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 16, 2022 at 10:09:56PM +0530, Kousik Sanagavarapu wrote:\n\n> It is mentioned in object-store.h that the function\n> repo_has_object_file() is deprecated. One possible alternative for this\n> function is has_object() (or atleast that is how I understood it).\n> \n> The file object-store.h also mentions that repo_has_object_file() and\n> its fellow functions and macros can be removed once the migrations take\n> place. This patch therefore is an attempt to reduce the usage of these\n> functions and macros.\n> \n> I request for comments as I'm not really sure about the \"flags\" argument\n> of the has_object() function and its usage in this patch.\n\nSo you've stumbled into quite a tricky spot. :)\n\nYes, without specifying new flags, this patch has a change of behavior\nwhich I think is not what we want. From 1d8d9cb620 (sha1-file: introduce\nno-lazy-fetch has_object(), 2020-08-05):\n\n There have been a few bugs wherein Git fetches missing objects\n whenever the existence of an object is checked, even though it does\n not need to perform such a fetch. To resolve these bugs, we could look\n at all the places that has_object_file() (or a similar function) is\n used. As a first step, introduce a new function has_object() that\n checks for the existence of an object, with a default behavior of not\n fetching if the object is missing and the repository is a partial\n clone. As we verify each has_object_file() (or similar) usage, we can\n replace it with has_object(), and we will know that we are done when\n we can delete has_object_file() (and the other similar functions).\n \n Also, the new function has_object() has more appropriate defaults:\n besides not fetching, it also does not recheck packed storage.\n\nSo the new function will:\n\n - not recheck packed objects unless we pass HAS_OBJECT_RECHECK_PACKED;\n this is done in the default paths because a simultaneous git-gc may\n be repacking objects (and we would rather double-check than racily\n miss it). So it's appropriate behavior if the caller is\n speculatively asking \"hey, we _might_ have this object\", but not if\n we expect to have it.\n\n - not lazily fetch objects in a partial-clone repository. This again\n depends on the caller being in a situation where they are OK saying\n \"no we don't have it\" for an object we _could_ get if it was worth\n spending effort\n\nThe call you're touching here is in parse_object_with_flags(), which is\nusing the check as part of the \"is it a blob that we should stream?\"\ncheck. If this call returns \"no we don't have it\", when we could get it\n(either due to racy repack or by fetching), then we'll hit the non-blob\npath that calls repo_read_object_file(). Where we would do a fresh\nlookup, including re-checking packs and/or lazily fetching!\n\nSo the new behavior seems strictly worse to me. We don't avoid those\nbehaviors, _and_ we fail to follow the streaming-blob code path (which\nmeans we may accidentally read a huge blob into memory. I think we'd\nwant to leave it as-is, or if we really want to eventually drop to a\nsingle interface, we need has_object() to learn a new flag to enable the\nlazy-fetch (and then use it along with RECHECK_PACKED).\n\nNow all of that said, I am skeptical that these calls to\nrepo_has_object_file() are even doing anything useful at all. Looking at\nthe existing code (dropping the \"+\" lines from your patch):\n\n> -\tif ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||\n> -\t (!obj && repo_has_object_file(r, oid) &&\n> \t oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\n> \t\tif (!skip_hash && stream_object_signature(r, repl) < 0) {\n> \t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n\nwe are checking that it exits if either:\n\n - another object (e.g., a tree) referred to it as a blob during this\n process, and we created an in-memory \"struct object\" with that type\n\n - nobody has referred to it, and we want to check its type via\n oid_object_info()\n\nIn the second case, this seems totally pointless. We can just ask\noid_object_info() what it's type is, and it will say \"no, we don't have\nit\" if appropriate. It will do the usual recheck-pack and lazy-fetch,\nbut so is repo_has_object_file(), so the short-circuit \"&&\" is not\nhelping. _If_ we were to switch to has_object() it would start to do\nsomething, but I think that's a bad idea for the reasons given above.\n\nIn the first case, I'd likewise argue it's not doing anything useful. It\nis confirming that we have the object, but so would the call to\nstream_object_signature() immediately below (assuming skip_hash is not\nset). If skip_hash is set, then we could either:\n\n 1. Just assume we have it. The point of the caller passing skip_hash\n is that we don't care about checking the integrity for this use\n case, and \"missing\" is not really any different than \"there's a\n file on disk but it might contain garbage bytes\".\n\n 2. Check repo_has_object_file() in this code path only when skip_hash\n is set. That retains the same \"do we even have it\" check for\n skip_hash that is performed now.\n\nI.e., I'd suggest this patch to remove both calls entirely:\n\ndiff --git a/object.c b/object.c\nindex 248530ba7b..a370545405 100644\n--- a/object.c\n+++ b/object.c\n@@ -285,9 +285,8 @@ struct object *parse_object_with_flags(struct repository *r,\n \t\t\treturn &commit->object;\n \t}\n \n-\tif ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||\n-\t (!obj && repo_has_object_file(r, oid) &&\n-\t oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\n+\tif ((obj && obj->type == OBJ_BLOB) ||\n+\t (!obj && oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\n \t\tif (!skip_hash && stream_object_signature(r, repl) < 0) {\n \t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n \t\t\treturn NULL;\n\nBut there may be some subtlety I'm missing. I'm cc-ing Jonathan Tan, who\nadded has_object(), and who added the top call to repo_has_object_file()\nvia df11e19648 (rev-list: support termination at promisor objects,\n2017-12-08). The second call is from 090ea12671 (parse_object: avoid\nputting whole blob in core, 2012-03-07), but he's no longer active on\nthe project. Looking at the commit, I think it was just a case of \"let's\nbe extra careful\". But as far as I can tell, it's not helping anything,\nand both calls are introducing extra work doing object lookups.\n\n-Peff\n"},{"id":"467395","messageId":"20221116211419.439356-1-jonathantanmy@google.com","threadId":"58806","inReplyTo":"Y3Up5Vi75Up8LaGQ@coredump.intra.peff.net","subject":"Re: [RFC][PATCH] object.c: use has_object() instead of repo_has_object_file()","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-11-16T21:14:18Z","receivedAt":"2022-11-16T21:14:29Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Jeff King writes:\n> I.e., I'd suggest this patch to remove both calls entirely:\n> \n> diff --git a/object.c b/object.c\n> index 248530ba7b..a370545405 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -285,9 +285,8 @@ struct object *parse_object_with_flags(struct repository *r,\n> \t\t\treturn &commit->object;\n> \t}\n> \n> -\tif ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||\n> -\t (!obj && repo_has_object_file(r, oid) &&\n> -\t oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\n> +\tif ((obj && obj->type == OBJ_BLOB) ||\n> +\t (!obj && oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\n> \t\tif (!skip_hash && stream_object_signature(r, repl) < 0) {\n> \t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n> \t\t\treturn NULL;\n> \n> But there may be some subtlety I'm missing. I'm cc-ing Jonathan Tan, who\n> added has_object(), and who added the top call to repo_has_object_file()\n> via df11e19648 (rev-list: support termination at promisor objects,\n> 2017-12-08).\n\nThanks for CC-ing me on this. Looking at that commit and the code at that time,\nI'm not sure why I added that call either. My best guess is that I was worried\nthat the streaming interface wouldn't support missing objects, but both then\nand now, a call to istream_source() is made before any streaming occurs (which\ndoes perform the lazy fetch).\n\nSo yes, I also think that you can remove these calls.\n"},{"id":"467472","messageId":"Y3a3qcqNG8W3ueeb@coredump.intra.peff.net","threadId":"58806","inReplyTo":"20221116211419.439356-1-jonathantanmy@google.com","subject":"[PATCH 0/2] fixing parse_object() check for type mismatch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-17T22:37:29Z","receivedAt":"2022-11-17T22:37:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 16, 2022 at 01:14:18PM -0800, Jonathan Tan wrote:\n\n> > But there may be some subtlety I'm missing. I'm cc-ing Jonathan Tan, who\n> > added has_object(), and who added the top call to repo_has_object_file()\n> > via df11e19648 (rev-list: support termination at promisor objects,\n> > 2017-12-08).\n> \n> Thanks for CC-ing me on this. Looking at that commit and the code at that time,\n> I'm not sure why I added that call either. My best guess is that I was worried\n> that the streaming interface wouldn't support missing objects, but both then\n> and now, a call to istream_source() is made before any streaming occurs (which\n> does perform the lazy fetch).\n> \n> So yes, I also think that you can remove these calls.\n\nThanks. After staring at this a bit, I noticed there is an even more\nsubtle issue with the case you touched back then, which is that it fails\nto notice when a think we expect to be a blob isn't one. Your old patch\ndidn't make anything worse there, but it also wasn't sufficient to catch\nthe problem. See patch 2 for details.\n\nI'm adding Taylor to the cc as the author of t6102, when we were\ntracking down all of these \"oops, it's not really a blob\" cases. This\nfixes one of the lingering cases from that test script.\n\n [1/2]: parse_object(): drop extra \"has\" check before checking object type\n [2/2]: parse_object(): check on-disk type of suspected blob\n\n object.c | 5 ++---\n t/t6102-rev-list-unexpected-objects.sh | 4 ++--\n 2 files changed, 4 insertions(+), 5 deletions(-)\n\n-Peff\n"},{"id":"467473","messageId":"Y3a3xnYNyiDQknTM@coredump.intra.peff.net","threadId":"58806","inReplyTo":"Y3a3qcqNG8W3ueeb@coredump.intra.peff.net","subject":"[PATCH 1/2] parse_object(): drop extra \"has\" check before checking object type","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-17T22:37:58Z","receivedAt":"2022-11-17T22:38:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When parsing an object of unknown type, we check to see if it's a blob,\nso we can use our streaming code path. This uses oid_object_info() to\ncheck the type, but before doing so we call repo_has_object_file(). This\nlatter is pointless, as oid_object_info() will already fail if the\nobject is missing. Checking it ahead of time just complicates the code\nand is a waste of resources (albeit small).\n\nLet's drop the redundant check.\n\nSigned-off-by: Jeff King \n---\n object.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex 8a74eb85e9..16eb944e98 100644\n--- a/object.c\n+++ b/object.c\n@@ -287,8 +287,7 @@ struct object *parse_object_with_flags(struct repository *r,\n \t}\n \n \tif ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||\n-\t (!obj && repo_has_object_file(r, oid) &&\n-\t oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\n+\t (!obj && oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\n \t\tif (!skip_hash && stream_object_signature(r, repl) < 0) {\n \t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n \t\t\treturn NULL;\n-- \n2.38.1.890.g50b10763b9\n\n"},{"id":"467474","messageId":"Y3a4jKzsHSooYFqj@coredump.intra.peff.net","threadId":"58806","inReplyTo":"Y3a3qcqNG8W3ueeb@coredump.intra.peff.net","subject":"[PATCH 2/2] parse_object(): check on-disk type of suspected blob","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-17T22:41:16Z","receivedAt":"2022-11-17T22:41:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In parse_object(), we try to handle blobs by streaming rather than\nloading them entirely into memory. The most common case here will be\nthat we haven't seen the object yet and check oid_object_info(), which\ntells us we have a blob.\n\nBut we trigger this code on one other case: when we have an in-memory\nobject struct with type OBJ_BLOB (and without its \"parsed\" flag set,\nsince otherwise we'd return early from the function). This indicates\nthat some other part of the code suspected we have a blob (e.g., it was\nmentioned by a tree or tag) but we haven't yet looked at the on-disk\ncopy.\n\nIn this case before hitting the streaming path, we check if we have the\nobject on-disk at all. This is mostly pointless extra work, as the\nstreaming path would complain if it couldn't open the object (albeit\nwith the message \"hash mismatch\", which is a little misleading).\n\nBut it's also insufficient to catch all problems. The streaming code\nwill only tell us \"yes, the on-disk object matches the oid\". But it\ndoesn't actually confirm that what we found was indeed a blob, and\nneither does repo_has_object_file().\n\nOne way to improve this would be to teach stream_object_signature() to\ncheck the type (either by returning it to us to check, or taking an\n\"expected\" type). But there's an even simpler fix here: if we suspect\nthe object is a blob, just call oid_object_info() to confirm that we\nhave it on-disk, and that it really is a blob.\n\nThis is slightly less efficient than teaching stream_object_signature()\nto do it (since it has to open the object already). But this case very\nrarely comes up. In practice, we usually don't have any clue what the\ntype is, in which case we already call oid_object_info(). This\n\"suspected\" case happens only when some other code created an object\nstruct but didn't actually parse the blob, which is actually tricky to\ntrigger at all (see the discussion of the test below).\n\nI reworked the conditional a bit so that instead of:\n\n if ((suspected_blob && oid_object_info() == OBJ_BLOB)\n (no_clue && oid_object_info() == OBJ_BLOB)\n\nwe have the simpler:\n\n if ((suspected_blob || no_clue) && oid_object_info() == OBJ_BLOB)\n\nThis is shorter, but also reflects what we really want say, which is\n\"have we ruled out this being a blob; if not, check it on-disk\".\n\nIn either case, if oid_object_info() fails to tell us it's a blob, we'll\nskip the streaming code path and call repo_read_object_file(), just as\nbefore. And if we really do have a mismatch with the existing object\nstruct, we'll eventually call lookup_commit(), etc, via\nparse_object_buffer(), which will complain that it doesn't match our\nexisting obj->type.\n\nSo this fixes one of the lingering expect_failure cases from 0616617c7e\n(t: introduce tests for unexpected object types, 2019-04-09). That test\nworks by peeling a tag that claims to point to a blob (triggering us to\ncreate the struct), but really points to something else, which we later\ndiscover when we call parse_object() as part of the actual traversal).\nPrior to this commit, we'd quietly check the sha1 and mark the blob as\n\"parsed\". Now we correctly complain about the mismatch.\n\nSigned-off-by: Jeff King \n---\nAs an aside, I found the \"this test is marked as success but testing the\nwrong thing\" pattern here confusing to deal with (since I had to dig in\nhistory to understand what was going on and what the test was _supposed_\nto say).\n\nIt comes from cf10c5b4cf (rev-list tests: don't hide abort() in\n\"test_expect_failure\", 2022-03-07). I'm skeptical that it was worth\nswitching those tests for leak detection purposes.\n\nBut more importantly, it looks like pw/test-todo would provide us with a\nmuch nicer pattern there. It seems to be stalled on review, so let's see\nif we can get that moving again.\n\n object.c | 4 ++--\n t/t6102-rev-list-unexpected-objects.sh | 4 ++--\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex 16eb944e98..fad1a5af4a 100644\n--- a/object.c\n+++ b/object.c\n@@ -286,8 +286,8 @@ struct object *parse_object_with_flags(struct repository *r,\n \t\t\treturn &commit->object;\n \t}\n \n-\tif ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||\n-\t (!obj && oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\n+\tif ((!obj || (obj && obj->type == OBJ_BLOB)) &&\n+\t oid_object_info(r, oid, NULL) == OBJ_BLOB) {\n \t\tif (!skip_hash && stream_object_signature(r, repl) < 0) {\n \t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n \t\t\treturn NULL;\ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nindex 4a9a4436e2..9350b5fd2c 100755\n--- a/t/t6102-rev-list-unexpected-objects.sh\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -121,8 +121,8 @@ test_expect_success 'setup unexpected non-blob tag' '\n \ttag=$(git hash-object -w --literally -t tag broken-tag)\n '\n \n-test_expect_success 'TODO (should fail!): traverse unexpected non-blob tag (lone)' '\n-\tgit rev-list --objects $tag\n+test_expect_success 'traverse unexpected non-blob tag (lone)' '\n+\ttest_must_fail git rev-list --objects $tag\n '\n \n test_expect_success 'traverse unexpected non-blob tag (seen)' '\n-- \n2.38.1.890.g50b10763b9\n"},{"id":"467477","messageId":"221118.86cz9lgjxu.gmgdl@evledraar.gmail.com","threadId":"58806","inReplyTo":"Y3a4jKzsHSooYFqj@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] parse_object(): check on-disk type of suspected blob","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-18T00:36:23Z","receivedAt":"2022-11-18T00:55:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 17 2022, Jeff King wrote:\n\n> I reworked the conditional a bit so that instead of:\n>\n> if ((suspected_blob && oid_object_info() == OBJ_BLOB)\n> (no_clue && oid_object_info() == OBJ_BLOB)\n>\n> we have the simpler:\n>\n> if ((suspected_blob || no_clue) && oid_object_info() == OBJ_BLOB)\n> [...]\n> @@ -286,8 +286,8 @@ struct object *parse_object_with_flags(struct repository *r,\n> \t\t\treturn &commit->object;\n> \t}\n> \n> -\tif ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||\n> -\t (!obj && oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\n> +\tif ((!obj || (obj && obj->type == OBJ_BLOB)) &&\n> +\t oid_object_info(r, oid, NULL) == OBJ_BLOB) {\n> \t\tif (!skip_hash && stream_object_signature(r, repl) < 0) {\n> \t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n> \t\t\treturn NULL;\n\nBut why:\n\n\tif ((!x || (x && x->m)) && ...)\n\nInstead of:\n\n\tif ((!x || x->m)) && ...)\n\nIf \"!obj\" is false then \"obj\" must be non-NULL, so you don't need to\ncheck it again and can lose the \"obj &&\".\n\n> [...]\n> So this fixes one of the lingering expect_failure cases from 0616617c7e\n> (t: introduce tests for unexpected object types, 2019-04-09). That test\n> works by peeling a tag that claims to point to a blob (triggering us to\n> create the struct), but really points to something else, which we later\n> discover when we call parse_object() as part of the actual traversal).\n> Prior to this commit, we'd quietly check the sha1 and mark the blob as\n> \"parsed\". Now we correctly complain about the mismatch.\n\nI applied this on top of \"master\", and adjusted your test to be this\ninstead:\n\n\ttest_expect_success 'traverse unexpected non-blob tag (lone)' '\n\t\tcat >expect <<-EOF &&\n\t\terror: object $commit is a blob, not a commit\n\t\tfatal: bad object $commit\n\t\tEOF\n\t\ttest_must_fail git rev-list --objects $tag >out 2>actual &&\n\t\ttest_must_be_empty out &&\n\t\ttest_cmp expect actual\n\t'\n\nWhich passes, showing that we're still not correctly identifying it, but\nwe are doing it for the purposes of erroring out, but the incorrect type\npersists.\n\nNow, this all does seem quite familiar... :) :\nhttps://lore.kernel.org/git/patch-10.11-a84f670ac24-20210328T021238Z-avarab@gmail.com/\n\nI.e. that's the rest of the fix for this issue. I applied this change on\nmy local branch with that, and they combine nicely. the \"test_must_fail\"\nhere works as intended, *and* we'll correctly report & store the type.\n\n> As an aside, I found the \"this test is marked as success but testing the\n> wrong thing\" pattern here confusing to deal with (since I had to dig in\n> history to understand what was going on and what the test was _supposed_\n> to say).\n>\n> It comes from cf10c5b4cf (rev-list tests: don't hide abort() in\n> \"test_expect_failure\", 2022-03-07). I'm skeptical that it was worth\n> switching those tests for leak detection purposes.\n\nIt's not just for leak detection purposes that it was a good idea to\nswitch it away from test_expect_failure, but also that we've been\nensuring that this didn't turn into a segfault all this time by not\nusing \"test_expect_failure\".\n\n> But more importantly, it looks like pw/test-todo would provide us with a\n> much nicer pattern there. It seems to be stalled on review, so let's see\n> if we can get that moving again.\n\nThe \"TODO (should fail!)\" didn't stand out? But yeah, having a \"todo\" or\n\"test_expect_todo\" or \"test_expect_failure\" not suck would be nice.\n\nFWIW I think\nhttps://lore.kernel.org/git/221006.86v8owr986.gmgdl@evledraar.gmail.com/\noutlines a good way forward for it that I think should make everyone\nhappy.\n\n> diff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\n> index 4a9a4436e2..9350b5fd2c 100755\n> --- a/t/t6102-rev-list-unexpected-objects.sh\n> +++ b/t/t6102-rev-list-unexpected-objects.sh\n> @@ -121,8 +121,8 @@ test_expect_success 'setup unexpected non-blob tag' '\n> \ttag=$(git hash-object -w --literally -t tag broken-tag)\n> '\n> \n> -test_expect_success 'TODO (should fail!): traverse unexpected non-blob tag (lone)' '\n> -\tgit rev-list --objects $tag\n> +test_expect_success 'traverse unexpected non-blob tag (lone)' '\n> +\ttest_must_fail git rev-list --objects $tag\n> '\n\nI think you'll either want the test_cmp I noted above, or to do that in\na subsequent test_expect_failure.\n\nI know that your stance is that you prefer not to \"test the bad\nbehavior\" as it were. Personally I thought an all-caps TODO comment\nmight make it less confusing, but anyway.\n\nIn this case your commit message claims you're happy with the end\nresult, so I think you'd want to test what we actually emit on stderr,\nas it's quite ... unintuative.\n\nOr, which I think probably makes more sense, add that as a subsequent\ntest_expect_failure or whatever. FWIW this somewhat un-idiomatic pattern\nwill get around the current caveats with it:\n\n\ttest_expect_success .... '\n\t\t... >actual.non-blob-tag\n\t'\n\ttest_lazy_prereq HAVE_NON_BLOB_TAG 'test -e actual.non-blob-tag'\n\ttest_expect_failure HAVE_NON_BLOB_TAG '...' '\n\t\tcat >expect.non-blob-tag <<-\\EOF &&\n\t\t...\n\t\tEOF\n\t\ttest_cmp expect.non-blob-tag actual.non-blob-tag\n\t'\n\nI.e. peel off the 'test_cmp\" that should have a known-good state from\nthe already-good status code.\n"},{"id":"467509","messageId":"patch-1.4-2be8477cd78-20221118T113442Z-avarab@gmail.com","threadId":"58806","inReplyTo":"cover-0.4-00000000000-20221118T113442Z-avarab@gmail.com","subject":"[PATCH 1/4] object-file.c: free the \"t.tag\" in check_tag()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-18T11:46:56Z","receivedAt":"2022-11-18T11:47:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a memory leak that's been with us ever since c879daa2372 (Make\nhash-object more robust against malformed objects, 2011-02-05). With\n\"HASH_FORMAT_CHECK\" (used by \"hash-object\" and \"replace\") we'll parse\ntags into a throwaway variable on the stack, but weren't freeing the\n\"item->tag\" we might malloc() when doing so.\n\nMark the tests that now pass in their entirety as passing under\n\"SANITIZE=leak\", which means we'll test them as part of the\n\"linux-leaks\" CI job.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n object-file.c | 1 +\n t/t3800-mktag.sh | 1 +\n t/t5302-pack-index.sh | 2 ++\n 3 files changed, 4 insertions(+)\n\ndiff --git a/object-file.c b/object-file.c\nindex 957790098fa..a13edba0753 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -2338,6 +2338,7 @@ static void check_tag(const void *buf, size_t size)\n \tmemset(&t, 0, sizeof(t));\n \tif (parse_tag_buffer(the_repository, &t, buf, size))\n \t\tdie(_(\"corrupt tag\"));\n+\tfree(t.tag);\n }\n \n static int index_mem(struct index_state *istate,\ndiff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh\nindex e3cf0ffbe59..d3e428ff46e 100755\n--- a/t/t3800-mktag.sh\n+++ b/t/t3800-mktag.sh\n@@ -4,6 +4,7 @@\n \n test_description='git mktag: tag object verify test'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n ###########################################################\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex b0095ab41d3..54b11f81c63 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -4,6 +4,8 @@\n #\n \n test_description='pack index with 64-bit offsets and object CRC'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n-- \n2.38.0.1511.gcdcff1f1dc2\n\n"},{"id":"467510","messageId":"patch-2.4-1b5544ec868-20221118T113442Z-avarab@gmail.com","threadId":"58806","inReplyTo":"cover-0.4-00000000000-20221118T113442Z-avarab@gmail.com","subject":"[PATCH 2/4] object tests: add test for unexpected objects in tags","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-18T11:46:57Z","receivedAt":"2022-11-18T11:47:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a blind spot in the tests added in 0616617c7e1 (t: introduce tests\nfor unexpected object types, 2019-04-09), there were no meaningful\ntests for checking how we reported on finding the incorrect object\ntype in a tag, i.e. one that broke the \"type\" promise in the tag\nheader.\n\nWe'll report the wrong object type in these cases, and thus fail on\nthe \"test_cmp\", e.g. for the first \"error: \" output being tested here\nwe should say \"$commit is a tag, not a commit\", instead we say\n\"$commit is a commit, not a tag\". This will be fixed in a subsequent\ncommit.\n\nSee the discussion & notes in [1] and downthread of there for test\nsnippets that are adapted here.\n\nIn the case of \"fsck\" which objects we visit in what order, and if we\nreport errors on them depends on their OIDs. So the test uses the\ntechnique of extracting the OID/type combinations that fsck does\nreport, and asserting that those are correct (currently, it's far from\ncorrect).\n\n1. https://lore.kernel.org/git/YGTGgFI19fS7Uv6I@coredump.intra.peff.net/\n\nHelped-by: Jeff King \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t6102-rev-list-unexpected-objects.sh | 146 +++++++++++++++++++++++++\n 1 file changed, 146 insertions(+)\n\ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nindex 9350b5fd2c2..ac49f7182fd 100755\n--- a/t/t6102-rev-list-unexpected-objects.sh\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -130,4 +130,150 @@ test_expect_success 'traverse unexpected non-blob tag (seen)' '\n \ttest_i18ngrep \"not a blob\" output\n '\n \n+test_expect_success 'setup unexpected non-tag tag' '\n+\ttest_when_finished \"git tag -d tag-commit tag-tag\" &&\n+\n+\tgit tag -a -m\"my tagged commit\" tag-commit $commit &&\n+\ttag_commit=$(git rev-parse tag-commit) &&\n+\tgit tag -a -m\"my tagged tag\" tag-tag tag-commit &&\n+\ttag_tag=$(git rev-parse tag-tag) &&\n+\n+\tgit cat-file tag tag-tag >good-tag-tag &&\n+\tgit cat-file tag tag-commit >good-commit-tag &&\n+\n+\tsed -e \"s/$tag_commit/$commit/\" broken-tag-tag-commit &&\n+\tsed -e \"s/$tag_commit/$tree/\" broken-tag-tag-tree &&\n+\tsed -e \"s/$tag_commit/$blob/\" broken-tag-tag-blob &&\n+\n+\tsed -e \"s/$commit/$tag_commit/\" broken-commit-tag-tag &&\n+\tsed -e \"s/$commit/$tree/\" broken-commit-tag-tree &&\n+\tsed -e \"s/$commit/$blob/\" broken-commit-tag-blob &&\n+\n+\ttag_tag_commit=$(git hash-object -w -t tag broken-tag-tag-commit) &&\n+\ttag_tag_tree=$(git hash-object -w -t tag broken-tag-tag-tree) &&\n+\ttag_tag_blob=$(git hash-object -w -t tag broken-tag-tag-blob) &&\n+\n+\tgit update-ref refs/tags/tag_tag_commit $tag_tag_commit &&\n+\tgit update-ref refs/tags/tag_tag_tree $tag_tag_tree &&\n+\tgit update-ref refs/tags/tag_tag_blob $tag_tag_blob &&\n+\n+\tcommit_tag_tag=$(git hash-object -w -t tag broken-commit-tag-tag) &&\n+\tcommit_tag_tree=$(git hash-object -w -t tag broken-commit-tag-tree) &&\n+\tcommit_tag_blob=$(git hash-object -w -t tag broken-commit-tag-blob) &&\n+\n+\tgit update-ref refs/tags/commit_tag_tag $commit_tag_tag &&\n+\tgit update-ref refs/tags/commit_tag_tree $commit_tag_tree &&\n+\tgit update-ref refs/tags/commit_tag_blob $commit_tag_blob\n+'\n+\n+test_expect_failure 'traverse unexpected incorrectly typed tag (to commit & tag)' '\n+\ttest_must_fail git rev-list --objects $tag_tag_commit 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: object $commit is a commit, not a tag\n+\tfatal: bad object $commit\n+\tEOF\n+\ttest_cmp expect err &&\n+\n+\ttest_must_fail git rev-list --objects $commit_tag_tag 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: object $tag_commit is a tag, not a commit\n+\tfatal: bad object $tag_commit\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n+test_expect_failure 'traverse unexpected incorrectly typed tag (to tree)' '\n+\ttest_must_fail git rev-list --objects $tag_tag_tree 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: object $tree is a tree, not a tag\n+\tfatal: bad object $tree\n+\tEOF\n+\ttest_cmp expect err &&\n+\n+\ttest_must_fail git rev-list --objects $commit_tag_tree 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: object $tree is a tree, not a commit\n+\tfatal: bad object $tree\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n+test_expect_failure 'traverse unexpected incorrectly typed tag (to blob)' '\n+\ttest_must_fail git rev-list --objects $tag_tag_blob 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: object $blob is a blob, not a tag\n+\tfatal: bad object $blob\n+\tEOF\n+\ttest_cmp expect err &&\n+\n+\ttest_must_fail git rev-list --objects $commit_tag_blob 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: object $blob is a blob, not a commit\n+\tfatal: bad object $blob\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n+test_expect_failure 'traverse unexpected non-tag tag (tree seen to blob)' '\n+\ttest_must_fail git rev-list --objects $tree $commit_tag_blob 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: object $blob is a blob, not a commit\n+\tfatal: bad object $blob\n+\tEOF\n+\ttest_cmp expect err &&\n+\n+\ttest_must_fail git rev-list --objects $tree $tag_tag_blob 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: object $blob is a blob, not a tag\n+\tfatal: bad object $blob\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n+\n+test_expect_failure 'traverse unexpected objects with for-each-ref' '\n+\tcat >expect <<-EOF &&\n+\terror: bad tag pointer to $tree in $tag_tag_tree\n+\tfatal: parse_object_buffer failed on $tag_tag_tree for refs/tags/tag_tag_tree\n+\tEOF\n+\ttest_must_fail git for-each-ref --format=\"%(*objectname)\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+>fsck-object-isa\n+test_expect_failure 'setup: unexpected objects with fsck' '\n+\ttest_must_fail git fsck 2>err &&\n+\tsed -n -e \"/^error: object .* is a .*, not a .*$/ {\n+\t\ts/^error: object \\([0-9a-f]*\\) is a \\([a-z]*\\), not a [a-z]*$/\\\\1 \\\\2/;\n+\t\tp;\n+\t}\" fsck-object-isa\n+'\n+\n+while read oid type\n+do\n+\ttest_expect_failure \"fsck knows unexpected object $oid is $type\" '\n+\t\tgit cat-file -t $oid >expect &&\n+\t\techo $type >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done err &&\n+\tcat >expected <<-EOF &&\n+\terror: object $blob is a blob, not a commit\n+\terror: bad tag pointer to $blob in $commit_tag_blob\n+\tfatal: bad object $commit_tag_blob\n+\tEOF\n+\ttest_cmp expected err &&\n+\n+\ttest_must_fail git rev-list --objects $blob $tag_tag_blob 2>err &&\n+\tcat >expected <<-EOF &&\n+\terror: object $blob is a blob, not a tag\n+\terror: bad tag pointer to $blob in $tag_tag_blob\n+\tfatal: bad object $tag_tag_blob\n+\tEOF\n+\ttest_cmp expected err\n+'\n+\n test_done\n-- \n2.38.0.1511.gcdcff1f1dc2\n\n"},{"id":"467511","messageId":"cover-0.4-00000000000-20221118T113442Z-avarab@gmail.com","threadId":"58806","inReplyTo":"Y3a3qcqNG8W3ueeb@coredump.intra.peff.net","subject":"[PATCH 0/4] tag: don't misreport type of tagged objects in errors","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-18T11:46:55Z","receivedAt":"2022-11-18T11:47:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series fixes a very long-standing issue where we'll get confused\nwhen we parse a tag whose \"type\" lies about the type of the target\nobject.\n\nIt goes on top of Jeff King's just-submitted [1], and the two\ncompliment one another. See [2] for my feedback about what was left\nover, which this fixes.\n\nCurrently we'll parse tags and note what the \"type\" claims to be. Say\na pointer to a \"blob\" object that claims to be a \"commit\" in the\nenvelope.\n\nThen when we we'd try to parse that supposed \"commit' for real we'd\nemit a message like:\n\n\terror: object is a blob, not a commit\n\nWhich is reversed, i.e. we'd remember the first \"blob\" we saw, and\nthen get confused about seeing a \"commit\" when we did the actual\nparsing.\n\nThis is now fixed in almost all cases by having the one caller of\nparse_tag() which actually knows the type tell it \"yes, I'm sure this\nis a commit\".\n\nWe'll then be able to see that we have a non-parsed object as\nscaffolding, but that it's really a commit, and emit the correct:\n\n\terror: object is a commit not a blob\n\nWhich goes along with other errors where the tag object itself yells\nabout being unhappy with the object reference.\n\nI submitted a version of these patches back in early 2021[3], this is\nsignificantly slimmed down since then.\n\nAt the time Jeff King noted that this approach inherently can't cover\nall possible scenarios. I.e. sometimes our parsing of the envelope\nisn't followed up by the \"real\" parse.\n\nEven in those cases we can \"get it right as 4/4 here demonstrates.\n\nBut there are going to be cases left where we get it wrong, but\nthey're all cases where we get it wrong now. It's probably not worth\nfixing the long tail of those issues, but now we'll emit a sensible\nerror on the common case of \"log\" etc.\n\n1. https://lore.kernel.org/git/Y3a3qcqNG8W3ueeb@coredump.intra.peff.net/\n2. https://lore.kernel.org/git/221118.86cz9lgjxu.gmgdl@evledraar.gmail.com/\n3. https://lore.kernel.org/git/YGTGgFI19fS7Uv6I@coredump.intra.peff.net/\n\nÆvar Arnfjörð Bjarmason (4):\n object-file.c: free the \"t.tag\" in check_tag()\n object tests: add test for unexpected objects in tags\n tag: don't misreport type of tagged objects in errors\n tag: don't emit potentially incorrect \"object is a X, not a Y\"\n\n blob.c | 11 +-\n blob.h | 3 +\n commit.c | 11 +-\n commit.h | 2 +\n object-file.c | 1 +\n object.c | 20 +++-\n object.h | 2 +\n t/t3800-mktag.sh | 1 +\n t/t5302-pack-index.sh | 2 +\n t/t6102-rev-list-unexpected-objects.sh | 146 +++++++++++++++++++++++++\n tag.c | 22 +++-\n tag.h | 2 +\n tree.c | 11 +-\n tree.h | 2 +\n 14 files changed, 222 insertions(+), 14 deletions(-)\n\n-- \n2.38.0.1511.gcdcff1f1dc2\n\n"},{"id":"467512","messageId":"patch-4.4-1a9dcb9e05d-20221118T113442Z-avarab@gmail.com","threadId":"58806","inReplyTo":"cover-0.4-00000000000-20221118T113442Z-avarab@gmail.com","subject":"[PATCH 4/4] tag: don't emit potentially incorrect \"object is a X, not a Y\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-18T11:46:59Z","receivedAt":"2022-11-18T11:47:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As noted in the preceding commit we weren't handling cases where we\nsee a reference to a bad \"type\" in a \"tag\", but then end up not fully\nparsing the object.\n\nIn those cases let's only claim that we have a bad tag pointer, but\nemit \"is a %s, not a %s\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t6102-rev-list-unexpected-objects.sh | 6 +++---\n tag.c | 5 +++--\n 2 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nindex 2e36d8bcfd9..ffc1672d7dc 100755\n--- a/t/t6102-rev-list-unexpected-objects.sh\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -231,7 +231,7 @@ test_expect_success 'traverse unexpected non-tag tag (tree seen to blob)' '\n '\n \n \n-test_expect_failure 'traverse unexpected objects with for-each-ref' '\n+test_expect_success 'traverse unexpected objects with for-each-ref' '\n \tcat >expect <<-EOF &&\n \terror: bad tag pointer to $tree in $tag_tag_tree\n \tfatal: parse_object_buffer failed on $tag_tag_tree for refs/tags/tag_tag_tree\n@@ -241,7 +241,7 @@ test_expect_failure 'traverse unexpected objects with for-each-ref' '\n '\n \n >fsck-object-isa\n-test_expect_failure 'setup: unexpected objects with fsck' '\n+test_expect_success 'setup: unexpected objects with fsck' '\n \ttest_must_fail git fsck 2>err &&\n \tsed -n -e \"/^error: object .* is a .*, not a .*$/ {\n \t\ts/^error: object \\([0-9a-f]*\\) is a \\([a-z]*\\), not a [a-z]*$/\\\\1 \\\\2/;\n@@ -251,7 +251,7 @@ test_expect_failure 'setup: unexpected objects with fsck' '\n \n while read oid type\n do\n-\ttest_expect_failure \"fsck knows unexpected object $oid is $type\" '\n+\ttest_expect_success \"fsck knows unexpected object $oid is $type\" '\n \t\tgit cat-file -t $oid >expect &&\n \t\techo $type >actual &&\n \t\ttest_cmp expect actual\ndiff --git a/tag.c b/tag.c\nindex 19453c2edbf..ad92cf89209 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -193,8 +193,9 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u\n \t}\n \n \tif (!item->tagged || strcmp(type_name(item->tagged->type), type)) {\n-\t\terror(_(\"object %s is a %s, not a %s\"), oid_to_hex(&oid),\n-\t\t type_name(item->tagged->type), type);\n+\t\tif (item->tagged && item->tagged->parsed)\n+\t\t\terror(_(\"object %s is a %s, not a %s\"), oid_to_hex(&oid),\n+\t\t\t type_name(item->tagged->type), type);\n \t\treturn error(\"bad tag pointer to %s in %s\",\n \t\t\t oid_to_hex(&oid),\n \t\t\t oid_to_hex(&item->object.oid));\n-- \n2.38.0.1511.gcdcff1f1dc2\n\n"},{"id":"467513","messageId":"patch-3.4-468af961dc4-20221118T113442Z-avarab@gmail.com","threadId":"58806","inReplyTo":"cover-0.4-00000000000-20221118T113442Z-avarab@gmail.com","subject":"[PATCH 3/4] tag: don't misreport type of tagged objects in errors","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-18T11:46:58Z","receivedAt":"2022-11-18T11:47:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a regression in 89e4202f982 ([PATCH] Parse tags for absent\nobjects, 2005-06-21) (yes, that ancient!) and correctly report an\nerror on a tag like:\n\n object \n type commit\n\nAs:\n\n error: object is tree, not a commit\n\nInstead of our long-standing misbehavior of inverting the two, and\nreporting:\n\n error: object is commit, not a tree\n\nWhich, as can be trivially seen with 'git cat-file -t '\nis incorrect.\n\nThe reason for this misreporting is that in parse_tag_buffer() we end\nup doing a lookup_{blob,commit,tag,tree}() depending on what we read\nout of the \"type\" line.\n\nIf we haven't parsed that object before we end up dispatching to the\ntype-specific lookup functions, e.g. this for commit.c in\nlookup_commit_type():\n\n\tstruct object *obj = lookup_object(r, oid);\n\tif (!obj)\n\t\treturn create_object(r, oid, alloc_commit_node(r));\n\nIts allocation will then set the obj->type according to what the tag\ntold us the type was, but which we've never validated. At this point\nwe've got an object in memory that hasn't been parsed, and whose type\nis incorrect, since we mistrusted a tag to tell us the type.\n\nThen when we actually load the object with parse_object() we read it\nand find that it's a \"tree\". See 8ff226a9d5e (add object_as_type\nhelper for casting objects, 2014-07-13) for that behavior (that's just\na refactoring commit, but shows all the code involved).\n\nWhich explains why we inverted the error report. Normally when\nobject_as_type() is called it's by the lookup_{blob,commit,tag,tree}()\nfunctions via parse_object(). At that point we can trust the\nobj->type.\n\nIn the case of parsing objects we've learned about via a tag with an\nincorrect type it's the opposite, the obj->type isn't correct and\nholds the mislabeled type, but we're parsing the object and know for\nsure what object type we're dealing with.\n\nSo, let's add \"lookup_{blob,commit,tag,tree}_type()\" functions to go\nwith the existing \"\"lookup_{blob,commit,tag,tree}()\", we'll call these\nfrom \"parse_object_buffer()\" where we actually know the type, as\nopposed to the \"parse_tag_buffer()\" code where we're just guessing\nwhat it might be.\n\nThis only help with the cases where we do see the tag reference, and\nthen end up doing a full parse of the object. But as seen in the\n\"for-each-ref\" and \"fsck\" tests we have cases where we'll never fully\nparse it.\n\nThose will be handled in a subsequent commit, but for now this handles\nthe common case of \"show\" etc. running into these.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n blob.c | 11 +++++++++--\n blob.h | 3 +++\n commit.c | 11 +++++++++--\n commit.h | 2 ++\n object.c | 20 ++++++++++++++++----\n object.h | 2 ++\n t/t6102-rev-list-unexpected-objects.sh | 8 ++++----\n tag.c | 21 +++++++++++++++++----\n tag.h | 2 ++\n tree.c | 11 +++++++++--\n tree.h | 2 ++\n 11 files changed, 75 insertions(+), 18 deletions(-)\n\ndiff --git a/blob.c b/blob.c\nindex 182718aba9f..ca30a22b2e8 100644\n--- a/blob.c\n+++ b/blob.c\n@@ -5,12 +5,19 @@\n \n const char *blob_type = \"blob\";\n \n-struct blob *lookup_blob(struct repository *r, const struct object_id *oid)\n+struct blob *lookup_blob_type(struct repository *r,\n+\t\t\t const struct object_id *oid,\n+\t\t\t enum object_type type)\n {\n \tstruct object *obj = lookup_object(r, oid);\n \tif (!obj)\n \t\treturn create_object(r, oid, alloc_blob_node(r));\n-\treturn object_as_type(obj, OBJ_BLOB, 0);\n+\treturn object_as_type_hint(obj, OBJ_BLOB, type);\n+}\n+\n+struct blob *lookup_blob(struct repository *r, const struct object_id *oid)\n+{\n+\treturn lookup_blob_type(r, oid, OBJ_NONE);\n }\n \n int parse_blob_buffer(struct blob *item, void *buffer, unsigned long size)\ndiff --git a/blob.h b/blob.h\nindex 16648720557..066a2effcbf 100644\n--- a/blob.h\n+++ b/blob.h\n@@ -10,6 +10,9 @@ struct blob {\n };\n \n struct blob *lookup_blob(struct repository *r, const struct object_id *oid);\n+struct blob *lookup_blob_type(struct repository *r,\n+\t\t\t const struct object_id *oid,\n+\t\t\t enum object_type type);\n \n int parse_blob_buffer(struct blob *item, void *buffer, unsigned long size);\n \ndiff --git a/commit.c b/commit.c\nindex 572301b80a2..8a90f279e24 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -67,12 +67,19 @@ struct commit *lookup_commit_object(struct repository *r,\n \n }\n \n-struct commit *lookup_commit(struct repository *r, const struct object_id *oid)\n+struct commit *lookup_commit_type(struct repository *r,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t enum object_type type)\n {\n \tstruct object *obj = lookup_object(r, oid);\n \tif (!obj)\n \t\treturn create_object(r, oid, alloc_commit_node(r));\n-\treturn object_as_type(obj, OBJ_COMMIT, 0);\n+\treturn object_as_type_hint(obj, OBJ_COMMIT, type);\n+}\n+\n+struct commit *lookup_commit(struct repository *r, const struct object_id *oid)\n+{\n+\treturn lookup_commit_type(r, oid, OBJ_NONE);\n }\n \n struct commit *lookup_commit_reference_by_name(const char *name)\ndiff --git a/commit.h b/commit.h\nindex fa39202fa6b..95001a29d6b 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -78,6 +78,8 @@ struct commit *lookup_commit_object(struct repository *r, const struct object_id\n * \"oid\" is not in the object cache.\n */\n struct commit *lookup_commit(struct repository *r, const struct object_id *oid);\n+struct commit *lookup_commit_type(struct repository *r, const struct object_id *oid,\n+\t\t\t\t enum object_type type);\n struct commit *lookup_commit_reference(struct repository *r,\n \t\t\t\t const struct object_id *oid);\n struct commit *lookup_commit_reference_gently(struct repository *r,\ndiff --git a/object.c b/object.c\nindex fad1a5af4a6..cc17ed0606e 100644\n--- a/object.c\n+++ b/object.c\n@@ -177,6 +177,18 @@ void *object_as_type(struct object *obj, enum object_type type, int quiet)\n \t}\n }\n \n+void *object_as_type_hint(struct object *obj, enum object_type type,\n+\t\t\t enum object_type hint)\n+{\n+\tif (hint != OBJ_NONE && obj->type != OBJ_NONE && obj->type != type) {\n+\t\terror(_(\"object %s is a %s, not a %s\"), oid_to_hex(&obj->oid),\n+\t\t type_name(type), type_name(obj->type));\n+\t\tobj->type = type;\n+\t\treturn NULL;\n+\t}\n+\treturn object_as_type(obj, type, 0);;\n+}\n+\n struct object *lookup_unknown_object(struct repository *r, const struct object_id *oid)\n {\n \tstruct object *obj = lookup_object(r, oid);\n@@ -210,14 +222,14 @@ struct object *parse_object_buffer(struct repository *r, const struct object_id\n \n \tobj = NULL;\n \tif (type == OBJ_BLOB) {\n-\t\tstruct blob *blob = lookup_blob(r, oid);\n+\t\tstruct blob *blob = lookup_blob_type(r, oid, type);\n \t\tif (blob) {\n \t\t\tif (parse_blob_buffer(blob, buffer, size))\n \t\t\t\treturn NULL;\n \t\t\tobj = &blob->object;\n \t\t}\n \t} else if (type == OBJ_TREE) {\n-\t\tstruct tree *tree = lookup_tree(r, oid);\n+\t\tstruct tree *tree = lookup_tree_type(r, oid, type);\n \t\tif (tree) {\n \t\t\tobj = &tree->object;\n \t\t\tif (!tree->buffer)\n@@ -229,7 +241,7 @@ struct object *parse_object_buffer(struct repository *r, const struct object_id\n \t\t\t}\n \t\t}\n \t} else if (type == OBJ_COMMIT) {\n-\t\tstruct commit *commit = lookup_commit(r, oid);\n+\t\tstruct commit *commit = lookup_commit_type(r, oid, type);\n \t\tif (commit) {\n \t\t\tif (parse_commit_buffer(r, commit, buffer, size, 1))\n \t\t\t\treturn NULL;\n@@ -241,7 +253,7 @@ struct object *parse_object_buffer(struct repository *r, const struct object_id\n \t\t\tobj = &commit->object;\n \t\t}\n \t} else if (type == OBJ_TAG) {\n-\t\tstruct tag *tag = lookup_tag(r, oid);\n+\t\tstruct tag *tag = lookup_tag_type(r, oid, type);\n \t\tif (tag) {\n \t\t\tif (parse_tag_buffer(r, tag, buffer, size))\n \t\t\t return NULL;\ndiff --git a/object.h b/object.h\nindex 31ebe114585..042c304d3a4 100644\n--- a/object.h\n+++ b/object.h\n@@ -122,6 +122,8 @@ struct object *lookup_object(struct repository *r, const struct object_id *oid);\n void *create_object(struct repository *r, const struct object_id *oid, void *obj);\n \n void *object_as_type(struct object *obj, enum object_type type, int quiet);\n+void *object_as_type_hint(struct object *obj, enum object_type type,\n+\t\t\t enum object_type hint);\n \n /*\n * Returns the object, having parsed it to find out what it is.\ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nindex ac49f7182fd..2e36d8bcfd9 100755\n--- a/t/t6102-rev-list-unexpected-objects.sh\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -166,7 +166,7 @@ test_expect_success 'setup unexpected non-tag tag' '\n \tgit update-ref refs/tags/commit_tag_blob $commit_tag_blob\n '\n \n-test_expect_failure 'traverse unexpected incorrectly typed tag (to commit & tag)' '\n+test_expect_success 'traverse unexpected incorrectly typed tag (to commit & tag)' '\n \ttest_must_fail git rev-list --objects $tag_tag_commit 2>err &&\n \tcat >expect <<-EOF &&\n \terror: object $commit is a commit, not a tag\n@@ -182,7 +182,7 @@ test_expect_failure 'traverse unexpected incorrectly typed tag (to commit & tag)\n \ttest_cmp expect err\n '\n \n-test_expect_failure 'traverse unexpected incorrectly typed tag (to tree)' '\n+test_expect_success 'traverse unexpected incorrectly typed tag (to tree)' '\n \ttest_must_fail git rev-list --objects $tag_tag_tree 2>err &&\n \tcat >expect <<-EOF &&\n \terror: object $tree is a tree, not a tag\n@@ -198,7 +198,7 @@ test_expect_failure 'traverse unexpected incorrectly typed tag (to tree)' '\n \ttest_cmp expect err\n '\n \n-test_expect_failure 'traverse unexpected incorrectly typed tag (to blob)' '\n+test_expect_success 'traverse unexpected incorrectly typed tag (to blob)' '\n \ttest_must_fail git rev-list --objects $tag_tag_blob 2>err &&\n \tcat >expect <<-EOF &&\n \terror: object $blob is a blob, not a tag\n@@ -214,7 +214,7 @@ test_expect_failure 'traverse unexpected incorrectly typed tag (to blob)' '\n \ttest_cmp expect err\n '\n \n-test_expect_failure 'traverse unexpected non-tag tag (tree seen to blob)' '\n+test_expect_success 'traverse unexpected non-tag tag (tree seen to blob)' '\n \ttest_must_fail git rev-list --objects $tree $commit_tag_blob 2>err &&\n \tcat >expect <<-EOF &&\n \terror: object $blob is a blob, not a commit\ndiff --git a/tag.c b/tag.c\nindex dfbcd7fcc24..19453c2edbf 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -100,12 +100,18 @@ struct object *deref_tag_noverify(struct object *o)\n \treturn o;\n }\n \n-struct tag *lookup_tag(struct repository *r, const struct object_id *oid)\n+struct tag *lookup_tag_type(struct repository *r, const struct object_id *oid,\n+\t\t\t enum object_type type)\n {\n \tstruct object *obj = lookup_object(r, oid);\n \tif (!obj)\n \t\treturn create_object(r, oid, alloc_tag_node(r));\n-\treturn object_as_type(obj, OBJ_TAG, 0);\n+\treturn object_as_type_hint(obj, OBJ_TAG, type);\n+}\n+\n+struct tag *lookup_tag(struct repository *r, const struct object_id *oid)\n+{\n+\treturn lookup_tag_type(r, oid, OBJ_NONE);\n }\n \n static timestamp_t parse_tag_date(const char *buf, const char *tail)\n@@ -135,6 +141,7 @@ void release_tag_memory(struct tag *t)\n \n int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, unsigned long size)\n {\n+\tstruct object *obj;\n \tstruct object_id oid;\n \tchar type[20];\n \tconst char *bufptr = data;\n@@ -169,7 +176,10 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u\n \ttype[nl - bufptr] = '\\0';\n \tbufptr = nl + 1;\n \n-\tif (!strcmp(type, blob_type)) {\n+\tobj = lookup_object(r, &oid);\n+\tif (obj) {\n+\t\titem->tagged = obj;\n+\t} else if (!strcmp(type, blob_type)) {\n \t\titem->tagged = (struct object *)lookup_blob(r, &oid);\n \t} else if (!strcmp(type, tree_type)) {\n \t\titem->tagged = (struct object *)lookup_tree(r, &oid);\n@@ -182,10 +192,13 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u\n \t\t\t type, oid_to_hex(&item->object.oid));\n \t}\n \n-\tif (!item->tagged)\n+\tif (!item->tagged || strcmp(type_name(item->tagged->type), type)) {\n+\t\terror(_(\"object %s is a %s, not a %s\"), oid_to_hex(&oid),\n+\t\t type_name(item->tagged->type), type);\n \t\treturn error(\"bad tag pointer to %s in %s\",\n \t\t\t oid_to_hex(&oid),\n \t\t\t oid_to_hex(&item->object.oid));\n+\t}\n \n \tif (bufptr + 4 < tail && starts_with(bufptr, \"tag \"))\n \t\t; \t\t/* good */\ndiff --git a/tag.h b/tag.h\nindex 3ce8e721924..42bd3e64011 100644\n--- a/tag.h\n+++ b/tag.h\n@@ -12,6 +12,8 @@ struct tag {\n \ttimestamp_t date;\n };\n struct tag *lookup_tag(struct repository *r, const struct object_id *oid);\n+struct tag *lookup_tag_type(struct repository *r, const struct object_id *oid,\n+\t\t\t enum object_type type);\n int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, unsigned long size);\n int parse_tag(struct tag *item);\n void release_tag_memory(struct tag *t);\ndiff --git a/tree.c b/tree.c\nindex 410e3b477e5..1a730249bb8 100644\n--- a/tree.c\n+++ b/tree.c\n@@ -102,12 +102,19 @@ int cmp_cache_name_compare(const void *a_, const void *b_)\n \t\t\t\t ce2->name, ce2->ce_namelen, ce_stage(ce2));\n }\n \n-struct tree *lookup_tree(struct repository *r, const struct object_id *oid)\n+struct tree *lookup_tree_type(struct repository *r,\n+\t\t\t const struct object_id *oid,\n+\t\t\t enum object_type type)\n {\n \tstruct object *obj = lookup_object(r, oid);\n \tif (!obj)\n \t\treturn create_object(r, oid, alloc_tree_node(r));\n-\treturn object_as_type(obj, OBJ_TREE, 0);\n+\treturn object_as_type_hint(obj, OBJ_TREE, type);\n+}\n+\n+struct tree *lookup_tree(struct repository *r, const struct object_id *oid)\n+{\n+\treturn lookup_tree_type(r, oid, OBJ_NONE);\n }\n \n int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size)\ndiff --git a/tree.h b/tree.h\nindex 6efff003e21..4af3b617f3d 100644\n--- a/tree.h\n+++ b/tree.h\n@@ -15,6 +15,8 @@ struct tree {\n extern const char *tree_type;\n \n struct tree *lookup_tree(struct repository *r, const struct object_id *oid);\n+struct tree *lookup_tree_type(struct repository *r, const struct object_id *oid,\n+\t\t\t enum object_type type);\n \n int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size);\n \n-- \n2.38.0.1511.gcdcff1f1dc2\n\n"},{"id":"467531","messageId":"Y3fXYKj8PdS4EKLB@nand.local","threadId":"58806","inReplyTo":"Y3a3qcqNG8W3ueeb@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] fixing parse_object() check for type mismatch","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-18T19:05:04Z","receivedAt":"2022-11-18T19:07:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 17, 2022 at 05:37:29PM -0500, Jeff King wrote:\n> I'm adding Taylor to the cc as the author of t6102, when we were\n> tracking down all of these \"oops, it's not really a blob\" cases. This\n> fixes one of the lingering cases from that test script.\n>\n> [1/2]: parse_object(): drop extra \"has\" check before checking object type\n> [2/2]: parse_object(): check on-disk type of suspected blob\n>\n> object.c | 5 ++---\n> t/t6102-rev-list-unexpected-objects.sh | 4 ++--\n> 2 files changed, 4 insertions(+), 5 deletions(-)\n\nA blast from the past :-).\n\nI took a careful look at both of these patches and they looked good to\nme, so let's start merging them down.\n\nThanks,\nTaylor\n"},{"id":"467717","messageId":"Y3vP16OJSP19VMMy@coredump.intra.peff.net","threadId":"58806","inReplyTo":"221118.86cz9lgjxu.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/2] parse_object(): check on-disk type of suspected blob","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-21T19:21:59Z","receivedAt":"2022-11-21T19:22:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 18, 2022 at 01:36:23AM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > -\tif ((obj && obj->type == OBJ_BLOB && repo_has_object_file(r, oid)) ||\n> > -\t (!obj && oid_object_info(r, oid, NULL) == OBJ_BLOB)) {\n> > +\tif ((!obj || (obj && obj->type == OBJ_BLOB)) &&\n> > +\t oid_object_info(r, oid, NULL) == OBJ_BLOB) {\n> > \t\tif (!skip_hash && stream_object_signature(r, repl) < 0) {\n> > \t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n> > \t\t\treturn NULL;\n> \n> But why:\n> \n> \tif ((!x || (x && x->m)) && ...)\n> \n> Instead of:\n> \n> \tif ((!x || x->m)) && ...)\n> \n> If \"!obj\" is false then \"obj\" must be non-NULL, so you don't need to\n> check it again and can lose the \"obj &&\".\n\nJust that it was one more round of refactoring than I did. :)\n\nI agree that it's much more readable. It looks like the original hit\n'next', so I'll send a patch on top.\n\n> I applied this on top of \"master\", and adjusted your test to be this\n> instead:\n> \n> \ttest_expect_success 'traverse unexpected non-blob tag (lone)' '\n> \t\tcat >expect <<-EOF &&\n> \t\terror: object $commit is a blob, not a commit\n> \t\tfatal: bad object $commit\n> \t\tEOF\n> \t\ttest_must_fail git rev-list --objects $tag >out 2>actual &&\n> \t\ttest_must_be_empty out &&\n> \t\ttest_cmp expect actual\n> \t'\n> \n> Which passes, showing that we're still not correctly identifying it, but\n> we are doing it for the purposes of erroring out, but the incorrect type\n> persists.\n> \n> Now, this all does seem quite familiar... :) :\n> https://lore.kernel.org/git/patch-10.11-a84f670ac24-20210328T021238Z-avarab@gmail.com/\n> \n> I.e. that's the rest of the fix for this issue. I applied this change on\n> my local branch with that, and they combine nicely. the \"test_must_fail\"\n> here works as intended, *and* we'll correctly report & store the type.\n\nRight. It's hitting the exact same code path as all of the other object\ntypes now. You suggested adding to the test here, but I'd prefer not to\ndo that. Noticing that we have a type mismatch is what is fixed, and it\nnow does that just like all the other object types. Dealing with the\nmessage reversal is orthogonal.\n\n> > But more importantly, it looks like pw/test-todo would provide us with a\n> > much nicer pattern there. It seems to be stalled on review, so let's see\n> > if we can get that moving again.\n> \n> The \"TODO (should fail!)\" didn't stand out? But yeah, having a \"todo\" or\n> \"test_expect_todo\" or \"test_expect_failure\" not suck would be nice.\n\nI did double-take on the \"TODO\" just because that is not our usual\npattern, but that was easily fixed. What I really don't like about the\n\"switch failure to success\" pattern is that it requires rewriting the\ntest to expect the wrong thing! So when somebody later fixes the bug,\nthey get a confusing failure, but must also rewrite the test back to\nwhat it originally should have been.\n\nThat was not too hard here, where it was just replacing a\ntest_must_fail, but that earlier hunk in cf10c5b4cf that actualy adds in\nexpected output (that we know is the wrong thing to be printing!) seems\na bit over the top to me. Anybody who encounters it has to dig into the\nhistory to understand what is going on.\n\n-Peff\n"},{"id":"467718","messageId":"Y3vQ/6QcTEFfpjLt@coredump.intra.peff.net","threadId":"58806","inReplyTo":"Y3fXYKj8PdS4EKLB@nand.local","subject":"Re: [PATCH 0/2] fixing parse_object() check for type mismatch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-21T19:26:55Z","receivedAt":"2022-11-21T19:27:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 18, 2022 at 02:05:04PM -0500, Taylor Blau wrote:\n\n> On Thu, Nov 17, 2022 at 05:37:29PM -0500, Jeff King wrote:\n> > I'm adding Taylor to the cc as the author of t6102, when we were\n> > tracking down all of these \"oops, it's not really a blob\" cases. This\n> > fixes one of the lingering cases from that test script.\n> >\n> > [1/2]: parse_object(): drop extra \"has\" check before checking object type\n> > [2/2]: parse_object(): check on-disk type of suspected blob\n> >\n> > object.c | 5 ++---\n> > t/t6102-rev-list-unexpected-objects.sh | 4 ++--\n> > 2 files changed, 4 insertions(+), 5 deletions(-)\n> \n> A blast from the past :-).\n> \n> I took a careful look at both of these patches and they looked good to\n> me, so let's start merging them down.\n\nI saw this hit 'next', but I think Ævar's simplification suggestion is\nworth taking. So here is a patch on top to do so (the original branch is\njk/parse-object-type-mismatch for the benefit of any newly-returned\nmaintainers).\n\nI was going to do a \"helped-by\", but since the only thing in the patch\nis the suggested change, I just handed over authorship. :)\n\nI didn't forge a signoff, and I think mine is sufficient under DCO's\npart (b), but Ævar please indicate if that's OK.\n\n-- >8 --\nFrom: Ævar Arnfjörð Bjarmason \nSubject: [PATCH] parse_object(): simplify blob conditional\n\nCommit 8db2dad7a0 (parse_object(): check on-disk type of suspected blob,\n2022-11-17) simplified the conditional for checking if we might have a\nblob. But we can simplify it further. In:\n\n !obj || (obj && obj->type == OBJ_BLOB)\n\nthe short-circuit \"OR\" means \"obj\" will always be true on the right-hand\nside. The compiler almost certainly optimized that out anyway, but\ndropping it makes the conditional easier to understand for humans.\n\nSigned-off-by: Jeff King \n---\n object.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/object.c b/object.c\nindex fad1a5af4a..682b852a46 100644\n--- a/object.c\n+++ b/object.c\n@@ -286,7 +286,7 @@ struct object *parse_object_with_flags(struct repository *r,\n \t\t\treturn &commit->object;\n \t}\n \n-\tif ((!obj || (obj && obj->type == OBJ_BLOB)) &&\n+\tif ((!obj || obj->type == OBJ_BLOB) &&\n \t oid_object_info(r, oid, NULL) == OBJ_BLOB) {\n \t\tif (!skip_hash && stream_object_signature(r, repl) < 0) {\n \t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n-- \n2.38.1.950.g65ed8c1df8\n\n"},{"id":"467731","messageId":"221122.86h6yrc0o6.gmgdl@evledraar.gmail.com","threadId":"58806","inReplyTo":"Y3vQ/6QcTEFfpjLt@coredump.intra.peff.net","subject":"Re: [PATCH 0/2] fixing parse_object() check for type mismatch","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-22T00:05:22Z","receivedAt":"2022-11-22T00:07:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 21 2022, Jeff King wrote:\n\n> On Fri, Nov 18, 2022 at 02:05:04PM -0500, Taylor Blau wrote:\n>\n>> On Thu, Nov 17, 2022 at 05:37:29PM -0500, Jeff King wrote:\n>> > I'm adding Taylor to the cc as the author of t6102, when we were\n>> > tracking down all of these \"oops, it's not really a blob\" cases. This\n>> > fixes one of the lingering cases from that test script.\n>> >\n>> > [1/2]: parse_object(): drop extra \"has\" check before checking object type\n>> > [2/2]: parse_object(): check on-disk type of suspected blob\n>> >\n>> > object.c | 5 ++---\n>> > t/t6102-rev-list-unexpected-objects.sh | 4 ++--\n>> > 2 files changed, 4 insertions(+), 5 deletions(-)\n>> \n>> A blast from the past :-).\n>> \n>> I took a careful look at both of these patches and they looked good to\n>> me, so let's start merging them down.\n>\n> I saw this hit 'next', but I think Ævar's simplification suggestion is\n> worth taking. So here is a patch on top to do so (the original branch is\n> jk/parse-object-type-mismatch for the benefit of any newly-returned\n> maintainers).\n>\n> I was going to do a \"helped-by\", but since the only thing in the patch\n> is the suggested change, I just handed over authorship. :)\n>\n> I didn't forge a signoff, and I think mine is sufficient under DCO's\n> part (b), but Ævar please indicate if that's OK.\n\nThis looks good to me, thanks for following up. In case my SOB is needed\nfeel free to add it, but it's fine without that too as far as I'm\nconcerned.\n\n> -- >8 --\n> From: Ævar Arnfjörð Bjarmason \n> Subject: [PATCH] parse_object(): simplify blob conditional\n>\n> Commit 8db2dad7a0 (parse_object(): check on-disk type of suspected blob,\n> 2022-11-17) simplified the conditional for checking if we might have a\n> blob. But we can simplify it further. In:\n>\n> !obj || (obj && obj->type == OBJ_BLOB)\n>\n> the short-circuit \"OR\" means \"obj\" will always be true on the right-hand\n> side. The compiler almost certainly optimized that out anyway, but\n> dropping it makes the conditional easier to understand for humans.\n>\n> Signed-off-by: Jeff King \n> ---\n> object.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/object.c b/object.c\n> index fad1a5af4a..682b852a46 100644\n> --- a/object.c\n> +++ b/object.c\n> @@ -286,7 +286,7 @@ struct object *parse_object_with_flags(struct repository *r,\n> \t\t\treturn &commit->object;\n> \t}\n> \n> -\tif ((!obj || (obj && obj->type == OBJ_BLOB)) &&\n> +\tif ((!obj || obj->type == OBJ_BLOB) &&\n> \t oid_object_info(r, oid, NULL) == OBJ_BLOB) {\n> \t\tif (!skip_hash && stream_object_signature(r, repl) < 0) {\n> \t\t\terror(_(\"hash mismatch %s\"), oid_to_hex(oid));\n\n"},{"id":"469640","messageId":"patch-v2-1.3-0abf873f1e3-20221230T011725Z-avarab@gmail.com","threadId":"58806","inReplyTo":"cover-v2-0.3-00000000000-20221230T011725Z-avarab@gmail.com","subject":"[PATCH v2 1/3] object tests: add test for unexpected objects in tags","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-30T01:52:14Z","receivedAt":"2022-12-30T01:52:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a blind spot in the tests added in 0616617c7e1 (t: introduce tests\nfor unexpected object types, 2019-04-09), there were no meaningful\ntests for checking how we reported on finding the incorrect object\ntype in a tag, i.e. one that broke the \"type\" promise in the tag\nheader.\n\nWe'll report the wrong object type in these cases, and thus fail on\nthe \"test_cmp\", e.g. for the first \"error: \" output being tested here\nwe should say \"$commit is a tag, not a commit\", instead we say\n\"$commit is a commit, not a tag\". This will be fixed in a subsequent\ncommit.\n\nSee the discussion & notes in [1] and downthread of there for test\nsnippets that are adapted here.\n\nIn the case of \"fsck\" which objects we visit in what order, and if we\nreport errors on them depends on their OIDs. So the test uses the\ntechnique of extracting the OID/type combinations that fsck does\nreport, and asserting that those are correct (currently, it's far from\ncorrect).\n\nAs these tests happen to run into a memory leak skip them under\nSANITIZE=leak, as the test file was previously marked leak-free in\n[3]. There is a concurrent fix for the leak in question[4].\n\n1. https://lore.kernel.org/git/YGTGgFI19fS7Uv6I@coredump.intra.peff.net/\n2. https://lore.kernel.org/git/patch-18.20-aa4df0e1b5c-20221228T175512Z-avarab@gmail.com/\n3. dd9cede9136 (leak tests: mark some rev-list tests as passing with\n SANITIZE=leak, 2021-10-31)\n4. https://lore.kernel.org/git/patch-18.20-aa4df0e1b5c-20221228T175512Z-avarab@gmail.com/\n\nHelped-by: Jeff King \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t6102-rev-list-unexpected-objects.sh | 146 +++++++++++++++++++++++++\n 1 file changed, 146 insertions(+)\n\ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nindex 9350b5fd2c2..f1c30db2654 100755\n--- a/t/t6102-rev-list-unexpected-objects.sh\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -130,4 +130,150 @@ test_expect_success 'traverse unexpected non-blob tag (seen)' '\n \ttest_i18ngrep \"not a blob\" output\n '\n \n+test_expect_success !SANITIZE_LEAK 'setup unexpected non-tag tag' '\n+\ttest_when_finished \"git tag -d tag-commit tag-tag\" &&\n+\n+\tgit tag -a -m\"my tagged commit\" tag-commit $commit &&\n+\ttag_commit=$(git rev-parse tag-commit) &&\n+\tgit tag -a -m\"my tagged tag\" tag-tag tag-commit &&\n+\ttag_tag=$(git rev-parse tag-tag) &&\n+\n+\tgit cat-file tag tag-tag >good-tag-tag &&\n+\tgit cat-file tag tag-commit >good-commit-tag &&\n+\n+\tsed -e \"s/$tag_commit/$commit/\" broken-tag-tag-commit &&\n+\tsed -e \"s/$tag_commit/$tree/\" broken-tag-tag-tree &&\n+\tsed -e \"s/$tag_commit/$blob/\" broken-tag-tag-blob &&\n+\n+\tsed -e \"s/$commit/$tag_commit/\" broken-commit-tag-tag &&\n+\tsed -e \"s/$commit/$tree/\" broken-commit-tag-tree &&\n+\tsed -e \"s/$commit/$blob/\" broken-commit-tag-blob &&\n+\n+\ttag_tag_commit=$(git hash-object -w -t tag broken-tag-tag-commit) &&\n+\ttag_tag_tree=$(git hash-object -w -t tag broken-tag-tag-tree) &&\n+\ttag_tag_blob=$(git hash-object -w -t tag broken-tag-tag-blob) &&\n+\n+\tgit update-ref refs/tags/tag_tag_commit $tag_tag_commit &&\n+\tgit update-ref refs/tags/tag_tag_tree $tag_tag_tree &&\n+\tgit update-ref refs/tags/tag_tag_blob $tag_tag_blob &&\n+\n+\tcommit_tag_tag=$(git hash-object -w -t tag broken-commit-tag-tag) &&\n+\tcommit_tag_tree=$(git hash-object -w -t tag broken-commit-tag-tree) &&\n+\tcommit_tag_blob=$(git hash-object -w -t tag broken-commit-tag-blob) &&\n+\n+\tgit update-ref refs/tags/commit_tag_tag $commit_tag_tag &&\n+\tgit update-ref refs/tags/commit_tag_tree $commit_tag_tree &&\n+\tgit update-ref refs/tags/commit_tag_blob $commit_tag_blob\n+'\n+\n+test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to commit & tag)' '\n+\ttest_must_fail git rev-list --objects $tag_tag_commit 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: object $commit is a commit, not a tag\n+\tfatal: bad object $commit\n+\tEOF\n+\ttest_cmp expect err &&\n+\n+\ttest_must_fail git rev-list --objects $commit_tag_tag 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: object $tag_commit is a tag, not a commit\n+\tfatal: bad object $tag_commit\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n+test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to tree)' '\n+\ttest_must_fail git rev-list --objects $tag_tag_tree 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: object $tree is a tree, not a tag\n+\tfatal: bad object $tree\n+\tEOF\n+\ttest_cmp expect err &&\n+\n+\ttest_must_fail git rev-list --objects $commit_tag_tree 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: object $tree is a tree, not a commit\n+\tfatal: bad object $tree\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n+test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to blob)' '\n+\ttest_must_fail git rev-list --objects $tag_tag_blob 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: object $blob is a blob, not a tag\n+\tfatal: bad object $blob\n+\tEOF\n+\ttest_cmp expect err &&\n+\n+\ttest_must_fail git rev-list --objects $commit_tag_blob 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: object $blob is a blob, not a commit\n+\tfatal: bad object $blob\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n+test_expect_failure !SANITIZE_LEAK 'traverse unexpected non-tag tag (tree seen to blob)' '\n+\ttest_must_fail git rev-list --objects $tree $commit_tag_blob 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: object $blob is a blob, not a commit\n+\tfatal: bad object $blob\n+\tEOF\n+\ttest_cmp expect err &&\n+\n+\ttest_must_fail git rev-list --objects $tree $tag_tag_blob 2>err &&\n+\tcat >expect <<-EOF &&\n+\terror: object $blob is a blob, not a tag\n+\tfatal: bad object $blob\n+\tEOF\n+\ttest_cmp expect err\n+'\n+\n+\n+test_expect_failure !SANITIZE_LEAK 'traverse unexpected objects with for-each-ref' '\n+\tcat >expect <<-EOF &&\n+\terror: bad tag pointer to $tree in $tag_tag_tree\n+\tfatal: parse_object_buffer failed on $tag_tag_tree for refs/tags/tag_tag_tree\n+\tEOF\n+\ttest_must_fail git for-each-ref --format=\"%(*objectname)\" 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+>fsck-object-isa\n+test_expect_success 'setup: unexpected objects with fsck' '\n+\ttest_must_fail git fsck 2>err &&\n+\tsed -n -e \"/^error: object .* is a .*, not a .*$/ {\n+\t\ts/^error: object \\([0-9a-f]*\\) is a \\([a-z]*\\), not a [a-z]*$/\\\\1 \\\\2/;\n+\t\tp;\n+\t}\" fsck-object-isa\n+'\n+\n+while read oid type\n+do\n+\ttest_expect_failure \"fsck knows unexpected object $oid is $type\" '\n+\t\tgit cat-file -t $oid >expect &&\n+\t\techo $type >actual &&\n+\t\ttest_cmp expect actual\n+\t'\n+done err &&\n+\tcat >expected <<-EOF &&\n+\terror: object $blob is a blob, not a commit\n+\terror: bad tag pointer to $blob in $commit_tag_blob\n+\tfatal: bad object $commit_tag_blob\n+\tEOF\n+\ttest_cmp expected err &&\n+\n+\ttest_must_fail git rev-list --objects $blob $tag_tag_blob 2>err &&\n+\tcat >expected <<-EOF &&\n+\terror: object $blob is a blob, not a tag\n+\terror: bad tag pointer to $blob in $tag_tag_blob\n+\tfatal: bad object $tag_tag_blob\n+\tEOF\n+\ttest_cmp expected err\n+'\n+\n test_done\n-- \n2.39.0.1153.g589e4efe9dc\n\n"},{"id":"469641","messageId":"cover-v2-0.3-00000000000-20221230T011725Z-avarab@gmail.com","threadId":"58806","inReplyTo":"cover-0.4-00000000000-20221118T113442Z-avarab@gmail.com","subject":"[PATCH v2 0/3] tag: don't misreport type of tagged objects in errors","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-30T01:52:13Z","receivedAt":"2022-12-30T01:52:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series fixes a very long-standing issue where we'll get confused\nwhen we parse a tag whose \"type\" lies about the type of the target\nobject.\n\nThe v1 was on top of jk/parse-object-type-mismatch, which has since\nlanded on \"master\". As I noted in [1] this covers remaining\nmisreporting cases which weren't addressed in that series.\n\nCurrently we'll parse tags and note what the \"type\" claims to be. Say\na pointer to a \"blob\" object that claims to be a \"commit\" in the\nenvelope.\n\nThen when we we'd try to parse that supposed \"commit' for real we'd\nemit a message like:\n\n\terror: object is a blob, not a commit\n\nWhich is reversed, i.e. we'd remember the first \"blob\" we saw, and\nthen get confused about seeing a \"commit\" when we did the actual\nparsing.\n\nThis is now fixed in almost all cases by having the one caller of\nparse_tag() which actually knows the type tell it \"yes, I'm sure this\nis a commit\".\n\nWe'll then be able to see that we have a non-parsed object as\nscaffolding, but that it's really a commit, and emit the correct:\n\n\terror: object is a commit not a blob\n\nWhich goes along with other errors where the tag object itself yells\nabout being unhappy with the object reference.\n\nI submitted a version of these patches back in early 2021[2], this is\nsignificantly slimmed down since then.\n\nAt the time Jeff King noted[3] that this approach inherently can't cover\nall possible scenarios. I.e. sometimes our parsing of the envelope\nisn't followed up by the \"real\" parse.\n\nEven in those cases we can \"get it right as 3/3 here demonstrates.\n\nBut there are going to be cases left where we get it wrong, but\nthey're all cases where we get it wrong now. It's probably not worth\nfixing the long tail of those issues, but now we'll emit a sensible\nerror on the common case of \"log\" etc.\n\nChanges since v1:\n\n* The v1 of this included a fix for the t.tag memory leak, which has\n now been ejected. I'm fixing that in another series[4]\n\n As a result we need to mark the new tests with !SANITIZE_LEAK, once\n some version of [4] lands we can un-mark these, so we'll test them\n under SANITIZE=leak.\n\n* In the previous 1st patch I marked a \"setup\" test as\n \"test_expect_failure\", which will pass at that point, let's make it\n \"test_expect_success\" from the outset.\n\nCI & branch at [5]. The \"win build\" CI failure is unrelated, it also\nhappens when I re-push master, root cause unknown, but unrelated to\nthis topic.\n\n1. https://lore.kernel.org/git/221118.86cz9lgjxu.gmgdl@evledraar.gmail.com/\n2. https://lore.kernel.org/git/cover-00.11-00000000000-20210328T021238Z-avarab@gmail.com/\n3. https://lore.kernel.org/git/YGTGgFI19fS7Uv6I@coredump.intra.peff.net/\n4. https://lore.kernel.org/git/cover-00.20-00000000000-20221228T175512Z-avarab@gmail.com/\n5. https://github.com/avar/git/tree/avar/correct-object-as-type-minimal-2\n\n\nÆvar Arnfjörð Bjarmason (3):\n object tests: add test for unexpected objects in tags\n tag: don't misreport type of tagged objects in errors\n tag: don't emit potentially incorrect \"object is a X, not a Y\"\n\n blob.c | 11 +-\n blob.h | 3 +\n commit.c | 11 +-\n commit.h | 2 +\n object.c | 20 +++-\n object.h | 2 +\n t/t6102-rev-list-unexpected-objects.sh | 146 +++++++++++++++++++++++++\n tag.c | 22 +++-\n tag.h | 2 +\n tree.c | 11 +-\n tree.h | 2 +\n 11 files changed, 218 insertions(+), 14 deletions(-)\n\nRange-diff against v1:\n1: 2be8477cd78 < -: ----------- object-file.c: free the \"t.tag\" in check_tag()\n2: 1b5544ec868 ! 1: 0abf873f1e3 object tests: add test for unexpected objects in tags\n @@ Commit message\n report, and asserting that those are correct (currently, it's far from\n correct).\n \n + As these tests happen to run into a memory leak skip them under\n + SANITIZE=leak, as the test file was previously marked leak-free in\n + [3]. There is a concurrent fix for the leak in question[4].\n +\n 1. https://lore.kernel.org/git/YGTGgFI19fS7Uv6I@coredump.intra.peff.net/\n + 2. https://lore.kernel.org/git/patch-18.20-aa4df0e1b5c-20221228T175512Z-avarab@gmail.com/\n + 3. dd9cede9136 (leak tests: mark some rev-list tests as passing with\n + SANITIZE=leak, 2021-10-31)\n + 4. https://lore.kernel.org/git/patch-18.20-aa4df0e1b5c-20221228T175512Z-avarab@gmail.com/\n \n Helped-by: Jeff King \n Signed-off-by: Ævar Arnfjörð Bjarmason \n @@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected\n \ttest_i18ngrep \"not a blob\" output\n '\n \n -+test_expect_success 'setup unexpected non-tag tag' '\n ++test_expect_success !SANITIZE_LEAK 'setup unexpected non-tag tag' '\n +\ttest_when_finished \"git tag -d tag-commit tag-tag\" &&\n +\n +\tgit tag -a -m\"my tagged commit\" tag-commit $commit &&\n @@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected\n +\tgit update-ref refs/tags/commit_tag_blob $commit_tag_blob\n +'\n +\n -+test_expect_failure 'traverse unexpected incorrectly typed tag (to commit & tag)' '\n ++test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to commit & tag)' '\n +\ttest_must_fail git rev-list --objects $tag_tag_commit 2>err &&\n +\tcat >expect <<-EOF &&\n +\terror: object $commit is a commit, not a tag\n @@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected\n +\ttest_cmp expect err\n +'\n +\n -+test_expect_failure 'traverse unexpected incorrectly typed tag (to tree)' '\n ++test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to tree)' '\n +\ttest_must_fail git rev-list --objects $tag_tag_tree 2>err &&\n +\tcat >expect <<-EOF &&\n +\terror: object $tree is a tree, not a tag\n @@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected\n +\ttest_cmp expect err\n +'\n +\n -+test_expect_failure 'traverse unexpected incorrectly typed tag (to blob)' '\n ++test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to blob)' '\n +\ttest_must_fail git rev-list --objects $tag_tag_blob 2>err &&\n +\tcat >expect <<-EOF &&\n +\terror: object $blob is a blob, not a tag\n @@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected\n +\ttest_cmp expect err\n +'\n +\n -+test_expect_failure 'traverse unexpected non-tag tag (tree seen to blob)' '\n ++test_expect_failure !SANITIZE_LEAK 'traverse unexpected non-tag tag (tree seen to blob)' '\n +\ttest_must_fail git rev-list --objects $tree $commit_tag_blob 2>err &&\n +\tcat >expect <<-EOF &&\n +\terror: object $blob is a blob, not a commit\n @@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected\n +'\n +\n +\n -+test_expect_failure 'traverse unexpected objects with for-each-ref' '\n ++test_expect_failure !SANITIZE_LEAK 'traverse unexpected objects with for-each-ref' '\n +\tcat >expect <<-EOF &&\n +\terror: bad tag pointer to $tree in $tag_tag_tree\n +\tfatal: parse_object_buffer failed on $tag_tag_tree for refs/tags/tag_tag_tree\n @@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected\n +'\n +\n +>fsck-object-isa\n -+test_expect_failure 'setup: unexpected objects with fsck' '\n ++test_expect_success 'setup: unexpected objects with fsck' '\n +\ttest_must_fail git fsck 2>err &&\n +\tsed -n -e \"/^error: object .* is a .*, not a .*$/ {\n +\t\ts/^error: object \\([0-9a-f]*\\) is a \\([a-z]*\\), not a [a-z]*$/\\\\1 \\\\2/;\n @@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected\n +\t'\n +done err &&\n +\tcat >expected <<-EOF &&\n +\terror: object $blob is a blob, not a commit\n3: 468af961dc4 ! 2: 96398731841 tag: don't misreport type of tagged objects in errors\n @@ blob.c\n +\treturn lookup_blob_type(r, oid, OBJ_NONE);\n }\n \n - int parse_blob_buffer(struct blob *item, void *buffer, unsigned long size)\n + void parse_blob_buffer(struct blob *item)\n \n ## blob.h ##\n @@ blob.h: struct blob {\n @@ blob.h: struct blob {\n +\t\t\t const struct object_id *oid,\n +\t\t\t enum object_type type);\n \n - int parse_blob_buffer(struct blob *item, void *buffer, unsigned long size);\n - \n + /**\n + * Blobs do not contain references to other objects and do not have\n \n ## commit.c ##\n @@ commit.c: struct commit *lookup_commit_object(struct repository *r,\n @@ object.c: struct object *parse_object_buffer(struct repository *r, const struct\n -\t\tstruct blob *blob = lookup_blob(r, oid);\n +\t\tstruct blob *blob = lookup_blob_type(r, oid, type);\n \t\tif (blob) {\n - \t\t\tif (parse_blob_buffer(blob, buffer, size))\n - \t\t\t\treturn NULL;\n + \t\t\tparse_blob_buffer(blob);\n \t\t\tobj = &blob->object;\n \t\t}\n \t} else if (type == OBJ_TREE) {\n @@ object.h: struct object *lookup_object(struct repository *r, const struct object\n * Returns the object, having parsed it to find out what it is.\n \n ## t/t6102-rev-list-unexpected-objects.sh ##\n -@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'setup unexpected non-tag tag' '\n +@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success !SANITIZE_LEAK 'setup unexpected non-tag tag' '\n \tgit update-ref refs/tags/commit_tag_blob $commit_tag_blob\n '\n \n --test_expect_failure 'traverse unexpected incorrectly typed tag (to commit & tag)' '\n -+test_expect_success 'traverse unexpected incorrectly typed tag (to commit & tag)' '\n +-test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to commit & tag)' '\n ++test_expect_success !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to commit & tag)' '\n \ttest_must_fail git rev-list --objects $tag_tag_commit 2>err &&\n \tcat >expect <<-EOF &&\n \terror: object $commit is a commit, not a tag\n -@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_failure 'traverse unexpected incorrectly typed tag (to commit & tag)\n +@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (t\n \ttest_cmp expect err\n '\n \n --test_expect_failure 'traverse unexpected incorrectly typed tag (to tree)' '\n -+test_expect_success 'traverse unexpected incorrectly typed tag (to tree)' '\n +-test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to tree)' '\n ++test_expect_success !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to tree)' '\n \ttest_must_fail git rev-list --objects $tag_tag_tree 2>err &&\n \tcat >expect <<-EOF &&\n \terror: object $tree is a tree, not a tag\n -@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_failure 'traverse unexpected incorrectly typed tag (to tree)' '\n +@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (t\n \ttest_cmp expect err\n '\n \n --test_expect_failure 'traverse unexpected incorrectly typed tag (to blob)' '\n -+test_expect_success 'traverse unexpected incorrectly typed tag (to blob)' '\n +-test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to blob)' '\n ++test_expect_success !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to blob)' '\n \ttest_must_fail git rev-list --objects $tag_tag_blob 2>err &&\n \tcat >expect <<-EOF &&\n \terror: object $blob is a blob, not a tag\n -@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_failure 'traverse unexpected incorrectly typed tag (to blob)' '\n +@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (t\n \ttest_cmp expect err\n '\n \n --test_expect_failure 'traverse unexpected non-tag tag (tree seen to blob)' '\n -+test_expect_success 'traverse unexpected non-tag tag (tree seen to blob)' '\n +-test_expect_failure !SANITIZE_LEAK 'traverse unexpected non-tag tag (tree seen to blob)' '\n ++test_expect_success !SANITIZE_LEAK 'traverse unexpected non-tag tag (tree seen to blob)' '\n \ttest_must_fail git rev-list --objects $tree $commit_tag_blob 2>err &&\n \tcat >expect <<-EOF &&\n \terror: object $blob is a blob, not a commit\n4: 1a9dcb9e05d ! 3: 2493988c41c tag: don't emit potentially incorrect \"object is a X, not a Y\"\n @@ Commit message\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## t/t6102-rev-list-unexpected-objects.sh ##\n -@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'traverse unexpected non-tag tag (tree seen to blob)' '\n +@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success !SANITIZE_LEAK 'traverse unexpected non-tag tag (tree seen t\n '\n \n \n --test_expect_failure 'traverse unexpected objects with for-each-ref' '\n -+test_expect_success 'traverse unexpected objects with for-each-ref' '\n +-test_expect_failure !SANITIZE_LEAK 'traverse unexpected objects with for-each-ref' '\n ++test_expect_success !SANITIZE_LEAK 'traverse unexpected objects with for-each-ref' '\n \tcat >expect <<-EOF &&\n \terror: bad tag pointer to $tree in $tag_tag_tree\n \tfatal: parse_object_buffer failed on $tag_tag_tree for refs/tags/tag_tag_tree\n -@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_failure 'traverse unexpected objects with for-each-ref' '\n - '\n - \n - >fsck-object-isa\n --test_expect_failure 'setup: unexpected objects with fsck' '\n -+test_expect_success 'setup: unexpected objects with fsck' '\n - \ttest_must_fail git fsck 2>err &&\n - \tsed -n -e \"/^error: object .* is a .*, not a .*$/ {\n - \t\ts/^error: object \\([0-9a-f]*\\) is a \\([a-z]*\\), not a [a-z]*$/\\\\1 \\\\2/;\n -@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_failure 'setup: unexpected objects with fsck' '\n +@@ t/t6102-rev-list-unexpected-objects.sh: test_expect_success 'setup: unexpected objects with fsck' '\n \n while read oid type\n do\n-- \n2.39.0.1153.g589e4efe9dc\n\n"},{"id":"469642","messageId":"patch-v2-3.3-2493988c41c-20221230T011725Z-avarab@gmail.com","threadId":"58806","inReplyTo":"cover-v2-0.3-00000000000-20221230T011725Z-avarab@gmail.com","subject":"[PATCH v2 3/3] tag: don't emit potentially incorrect \"object is a X, not a Y\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-30T01:52:16Z","receivedAt":"2022-12-30T01:52:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As noted in the preceding commit we weren't handling cases where we\nsee a reference to a bad \"type\" in a \"tag\", but then end up not fully\nparsing the object.\n\nIn those cases let's only claim that we have a bad tag pointer, but\nemit \"is a %s, not a %s\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t6102-rev-list-unexpected-objects.sh | 4 ++--\n tag.c | 5 +++--\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nindex 590e2523d0c..98e36e73698 100755\n--- a/t/t6102-rev-list-unexpected-objects.sh\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -231,7 +231,7 @@ test_expect_success !SANITIZE_LEAK 'traverse unexpected non-tag tag (tree seen t\n '\n \n \n-test_expect_failure !SANITIZE_LEAK 'traverse unexpected objects with for-each-ref' '\n+test_expect_success !SANITIZE_LEAK 'traverse unexpected objects with for-each-ref' '\n \tcat >expect <<-EOF &&\n \terror: bad tag pointer to $tree in $tag_tag_tree\n \tfatal: parse_object_buffer failed on $tag_tag_tree for refs/tags/tag_tag_tree\n@@ -251,7 +251,7 @@ test_expect_success 'setup: unexpected objects with fsck' '\n \n while read oid type\n do\n-\ttest_expect_failure \"fsck knows unexpected object $oid is $type\" '\n+\ttest_expect_success \"fsck knows unexpected object $oid is $type\" '\n \t\tgit cat-file -t $oid >expect &&\n \t\techo $type >actual &&\n \t\ttest_cmp expect actual\ndiff --git a/tag.c b/tag.c\nindex 19453c2edbf..ad92cf89209 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -193,8 +193,9 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u\n \t}\n \n \tif (!item->tagged || strcmp(type_name(item->tagged->type), type)) {\n-\t\terror(_(\"object %s is a %s, not a %s\"), oid_to_hex(&oid),\n-\t\t type_name(item->tagged->type), type);\n+\t\tif (item->tagged && item->tagged->parsed)\n+\t\t\terror(_(\"object %s is a %s, not a %s\"), oid_to_hex(&oid),\n+\t\t\t type_name(item->tagged->type), type);\n \t\treturn error(\"bad tag pointer to %s in %s\",\n \t\t\t oid_to_hex(&oid),\n \t\t\t oid_to_hex(&item->object.oid));\n-- \n2.39.0.1153.g589e4efe9dc\n\n"},{"id":"469643","messageId":"patch-v2-2.3-96398731841-20221230T011725Z-avarab@gmail.com","threadId":"58806","inReplyTo":"cover-v2-0.3-00000000000-20221230T011725Z-avarab@gmail.com","subject":"[PATCH v2 2/3] tag: don't misreport type of tagged objects in errors","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-12-30T01:52:15Z","receivedAt":"2022-12-30T01:52:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a regression in 89e4202f982 ([PATCH] Parse tags for absent\nobjects, 2005-06-21) (yes, that ancient!) and correctly report an\nerror on a tag like:\n\n object \n type commit\n\nAs:\n\n error: object is tree, not a commit\n\nInstead of our long-standing misbehavior of inverting the two, and\nreporting:\n\n error: object is commit, not a tree\n\nWhich, as can be trivially seen with 'git cat-file -t '\nis incorrect.\n\nThe reason for this misreporting is that in parse_tag_buffer() we end\nup doing a lookup_{blob,commit,tag,tree}() depending on what we read\nout of the \"type\" line.\n\nIf we haven't parsed that object before we end up dispatching to the\ntype-specific lookup functions, e.g. this for commit.c in\nlookup_commit_type():\n\n\tstruct object *obj = lookup_object(r, oid);\n\tif (!obj)\n\t\treturn create_object(r, oid, alloc_commit_node(r));\n\nIts allocation will then set the obj->type according to what the tag\ntold us the type was, but which we've never validated. At this point\nwe've got an object in memory that hasn't been parsed, and whose type\nis incorrect, since we mistrusted a tag to tell us the type.\n\nThen when we actually load the object with parse_object() we read it\nand find that it's a \"tree\". See 8ff226a9d5e (add object_as_type\nhelper for casting objects, 2014-07-13) for that behavior (that's just\na refactoring commit, but shows all the code involved).\n\nWhich explains why we inverted the error report. Normally when\nobject_as_type() is called it's by the lookup_{blob,commit,tag,tree}()\nfunctions via parse_object(). At that point we can trust the\nobj->type.\n\nIn the case of parsing objects we've learned about via a tag with an\nincorrect type it's the opposite, the obj->type isn't correct and\nholds the mislabeled type, but we're parsing the object and know for\nsure what object type we're dealing with.\n\nSo, let's add \"lookup_{blob,commit,tag,tree}_type()\" functions to go\nwith the existing \"\"lookup_{blob,commit,tag,tree}()\", we'll call these\nfrom \"parse_object_buffer()\" where we actually know the type, as\nopposed to the \"parse_tag_buffer()\" code where we're just guessing\nwhat it might be.\n\nThis only help with the cases where we do see the tag reference, and\nthen end up doing a full parse of the object. But as seen in the\n\"for-each-ref\" and \"fsck\" tests we have cases where we'll never fully\nparse it.\n\nThose will be handled in a subsequent commit, but for now this handles\nthe common case of \"show\" etc. running into these.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n blob.c | 11 +++++++++--\n blob.h | 3 +++\n commit.c | 11 +++++++++--\n commit.h | 2 ++\n object.c | 20 ++++++++++++++++----\n object.h | 2 ++\n t/t6102-rev-list-unexpected-objects.sh | 8 ++++----\n tag.c | 21 +++++++++++++++++----\n tag.h | 2 ++\n tree.c | 11 +++++++++--\n tree.h | 2 ++\n 11 files changed, 75 insertions(+), 18 deletions(-)\n\ndiff --git a/blob.c b/blob.c\nindex 8f83523b0cd..d6e3d64213d 100644\n--- a/blob.c\n+++ b/blob.c\n@@ -5,12 +5,19 @@\n \n const char *blob_type = \"blob\";\n \n-struct blob *lookup_blob(struct repository *r, const struct object_id *oid)\n+struct blob *lookup_blob_type(struct repository *r,\n+\t\t\t const struct object_id *oid,\n+\t\t\t enum object_type type)\n {\n \tstruct object *obj = lookup_object(r, oid);\n \tif (!obj)\n \t\treturn create_object(r, oid, alloc_blob_node(r));\n-\treturn object_as_type(obj, OBJ_BLOB, 0);\n+\treturn object_as_type_hint(obj, OBJ_BLOB, type);\n+}\n+\n+struct blob *lookup_blob(struct repository *r, const struct object_id *oid)\n+{\n+\treturn lookup_blob_type(r, oid, OBJ_NONE);\n }\n \n void parse_blob_buffer(struct blob *item)\ndiff --git a/blob.h b/blob.h\nindex 74555c90c44..b91b1600bda 100644\n--- a/blob.h\n+++ b/blob.h\n@@ -10,6 +10,9 @@ struct blob {\n };\n \n struct blob *lookup_blob(struct repository *r, const struct object_id *oid);\n+struct blob *lookup_blob_type(struct repository *r,\n+\t\t\t const struct object_id *oid,\n+\t\t\t enum object_type type);\n \n /**\n * Blobs do not contain references to other objects and do not have\ndiff --git a/commit.c b/commit.c\nindex d00780bee5f..8e30c21816c 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -67,12 +67,19 @@ struct commit *lookup_commit_object(struct repository *r,\n \n }\n \n-struct commit *lookup_commit(struct repository *r, const struct object_id *oid)\n+struct commit *lookup_commit_type(struct repository *r,\n+\t\t\t\t const struct object_id *oid,\n+\t\t\t\t enum object_type type)\n {\n \tstruct object *obj = lookup_object(r, oid);\n \tif (!obj)\n \t\treturn create_object(r, oid, alloc_commit_node(r));\n-\treturn object_as_type(obj, OBJ_COMMIT, 0);\n+\treturn object_as_type_hint(obj, OBJ_COMMIT, type);\n+}\n+\n+struct commit *lookup_commit(struct repository *r, const struct object_id *oid)\n+{\n+\treturn lookup_commit_type(r, oid, OBJ_NONE);\n }\n \n struct commit *lookup_commit_reference_by_name(const char *name)\ndiff --git a/commit.h b/commit.h\nindex fa39202fa6b..95001a29d6b 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -78,6 +78,8 @@ struct commit *lookup_commit_object(struct repository *r, const struct object_id\n * \"oid\" is not in the object cache.\n */\n struct commit *lookup_commit(struct repository *r, const struct object_id *oid);\n+struct commit *lookup_commit_type(struct repository *r, const struct object_id *oid,\n+\t\t\t\t enum object_type type);\n struct commit *lookup_commit_reference(struct repository *r,\n \t\t\t\t const struct object_id *oid);\n struct commit *lookup_commit_reference_gently(struct repository *r,\ndiff --git a/object.c b/object.c\nindex 344087de4de..81ec18f0739 100644\n--- a/object.c\n+++ b/object.c\n@@ -177,6 +177,18 @@ void *object_as_type(struct object *obj, enum object_type type, int quiet)\n \t}\n }\n \n+void *object_as_type_hint(struct object *obj, enum object_type type,\n+\t\t\t enum object_type hint)\n+{\n+\tif (hint != OBJ_NONE && obj->type != OBJ_NONE && obj->type != type) {\n+\t\terror(_(\"object %s is a %s, not a %s\"), oid_to_hex(&obj->oid),\n+\t\t type_name(type), type_name(obj->type));\n+\t\tobj->type = type;\n+\t\treturn NULL;\n+\t}\n+\treturn object_as_type(obj, type, 0);;\n+}\n+\n struct object *lookup_unknown_object(struct repository *r, const struct object_id *oid)\n {\n \tstruct object *obj = lookup_object(r, oid);\n@@ -210,13 +222,13 @@ struct object *parse_object_buffer(struct repository *r, const struct object_id\n \n \tobj = NULL;\n \tif (type == OBJ_BLOB) {\n-\t\tstruct blob *blob = lookup_blob(r, oid);\n+\t\tstruct blob *blob = lookup_blob_type(r, oid, type);\n \t\tif (blob) {\n \t\t\tparse_blob_buffer(blob);\n \t\t\tobj = &blob->object;\n \t\t}\n \t} else if (type == OBJ_TREE) {\n-\t\tstruct tree *tree = lookup_tree(r, oid);\n+\t\tstruct tree *tree = lookup_tree_type(r, oid, type);\n \t\tif (tree) {\n \t\t\tobj = &tree->object;\n \t\t\tif (!tree->buffer)\n@@ -228,7 +240,7 @@ struct object *parse_object_buffer(struct repository *r, const struct object_id\n \t\t\t}\n \t\t}\n \t} else if (type == OBJ_COMMIT) {\n-\t\tstruct commit *commit = lookup_commit(r, oid);\n+\t\tstruct commit *commit = lookup_commit_type(r, oid, type);\n \t\tif (commit) {\n \t\t\tif (parse_commit_buffer(r, commit, buffer, size, 1))\n \t\t\t\treturn NULL;\n@@ -240,7 +252,7 @@ struct object *parse_object_buffer(struct repository *r, const struct object_id\n \t\t\tobj = &commit->object;\n \t\t}\n \t} else if (type == OBJ_TAG) {\n-\t\tstruct tag *tag = lookup_tag(r, oid);\n+\t\tstruct tag *tag = lookup_tag_type(r, oid, type);\n \t\tif (tag) {\n \t\t\tif (parse_tag_buffer(r, tag, buffer, size))\n \t\t\t return NULL;\ndiff --git a/object.h b/object.h\nindex 31ebe114585..042c304d3a4 100644\n--- a/object.h\n+++ b/object.h\n@@ -122,6 +122,8 @@ struct object *lookup_object(struct repository *r, const struct object_id *oid);\n void *create_object(struct repository *r, const struct object_id *oid, void *obj);\n \n void *object_as_type(struct object *obj, enum object_type type, int quiet);\n+void *object_as_type_hint(struct object *obj, enum object_type type,\n+\t\t\t enum object_type hint);\n \n /*\n * Returns the object, having parsed it to find out what it is.\ndiff --git a/t/t6102-rev-list-unexpected-objects.sh b/t/t6102-rev-list-unexpected-objects.sh\nindex f1c30db2654..590e2523d0c 100755\n--- a/t/t6102-rev-list-unexpected-objects.sh\n+++ b/t/t6102-rev-list-unexpected-objects.sh\n@@ -166,7 +166,7 @@ test_expect_success !SANITIZE_LEAK 'setup unexpected non-tag tag' '\n \tgit update-ref refs/tags/commit_tag_blob $commit_tag_blob\n '\n \n-test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to commit & tag)' '\n+test_expect_success !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to commit & tag)' '\n \ttest_must_fail git rev-list --objects $tag_tag_commit 2>err &&\n \tcat >expect <<-EOF &&\n \terror: object $commit is a commit, not a tag\n@@ -182,7 +182,7 @@ test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (t\n \ttest_cmp expect err\n '\n \n-test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to tree)' '\n+test_expect_success !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to tree)' '\n \ttest_must_fail git rev-list --objects $tag_tag_tree 2>err &&\n \tcat >expect <<-EOF &&\n \terror: object $tree is a tree, not a tag\n@@ -198,7 +198,7 @@ test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (t\n \ttest_cmp expect err\n '\n \n-test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to blob)' '\n+test_expect_success !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to blob)' '\n \ttest_must_fail git rev-list --objects $tag_tag_blob 2>err &&\n \tcat >expect <<-EOF &&\n \terror: object $blob is a blob, not a tag\n@@ -214,7 +214,7 @@ test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (t\n \ttest_cmp expect err\n '\n \n-test_expect_failure !SANITIZE_LEAK 'traverse unexpected non-tag tag (tree seen to blob)' '\n+test_expect_success !SANITIZE_LEAK 'traverse unexpected non-tag tag (tree seen to blob)' '\n \ttest_must_fail git rev-list --objects $tree $commit_tag_blob 2>err &&\n \tcat >expect <<-EOF &&\n \terror: object $blob is a blob, not a commit\ndiff --git a/tag.c b/tag.c\nindex dfbcd7fcc24..19453c2edbf 100644\n--- a/tag.c\n+++ b/tag.c\n@@ -100,12 +100,18 @@ struct object *deref_tag_noverify(struct object *o)\n \treturn o;\n }\n \n-struct tag *lookup_tag(struct repository *r, const struct object_id *oid)\n+struct tag *lookup_tag_type(struct repository *r, const struct object_id *oid,\n+\t\t\t enum object_type type)\n {\n \tstruct object *obj = lookup_object(r, oid);\n \tif (!obj)\n \t\treturn create_object(r, oid, alloc_tag_node(r));\n-\treturn object_as_type(obj, OBJ_TAG, 0);\n+\treturn object_as_type_hint(obj, OBJ_TAG, type);\n+}\n+\n+struct tag *lookup_tag(struct repository *r, const struct object_id *oid)\n+{\n+\treturn lookup_tag_type(r, oid, OBJ_NONE);\n }\n \n static timestamp_t parse_tag_date(const char *buf, const char *tail)\n@@ -135,6 +141,7 @@ void release_tag_memory(struct tag *t)\n \n int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, unsigned long size)\n {\n+\tstruct object *obj;\n \tstruct object_id oid;\n \tchar type[20];\n \tconst char *bufptr = data;\n@@ -169,7 +176,10 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u\n \ttype[nl - bufptr] = '\\0';\n \tbufptr = nl + 1;\n \n-\tif (!strcmp(type, blob_type)) {\n+\tobj = lookup_object(r, &oid);\n+\tif (obj) {\n+\t\titem->tagged = obj;\n+\t} else if (!strcmp(type, blob_type)) {\n \t\titem->tagged = (struct object *)lookup_blob(r, &oid);\n \t} else if (!strcmp(type, tree_type)) {\n \t\titem->tagged = (struct object *)lookup_tree(r, &oid);\n@@ -182,10 +192,13 @@ int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, u\n \t\t\t type, oid_to_hex(&item->object.oid));\n \t}\n \n-\tif (!item->tagged)\n+\tif (!item->tagged || strcmp(type_name(item->tagged->type), type)) {\n+\t\terror(_(\"object %s is a %s, not a %s\"), oid_to_hex(&oid),\n+\t\t type_name(item->tagged->type), type);\n \t\treturn error(\"bad tag pointer to %s in %s\",\n \t\t\t oid_to_hex(&oid),\n \t\t\t oid_to_hex(&item->object.oid));\n+\t}\n \n \tif (bufptr + 4 < tail && starts_with(bufptr, \"tag \"))\n \t\t; \t\t/* good */\ndiff --git a/tag.h b/tag.h\nindex 3ce8e721924..42bd3e64011 100644\n--- a/tag.h\n+++ b/tag.h\n@@ -12,6 +12,8 @@ struct tag {\n \ttimestamp_t date;\n };\n struct tag *lookup_tag(struct repository *r, const struct object_id *oid);\n+struct tag *lookup_tag_type(struct repository *r, const struct object_id *oid,\n+\t\t\t enum object_type type);\n int parse_tag_buffer(struct repository *r, struct tag *item, const void *data, unsigned long size);\n int parse_tag(struct tag *item);\n void release_tag_memory(struct tag *t);\ndiff --git a/tree.c b/tree.c\nindex 410e3b477e5..1a730249bb8 100644\n--- a/tree.c\n+++ b/tree.c\n@@ -102,12 +102,19 @@ int cmp_cache_name_compare(const void *a_, const void *b_)\n \t\t\t\t ce2->name, ce2->ce_namelen, ce_stage(ce2));\n }\n \n-struct tree *lookup_tree(struct repository *r, const struct object_id *oid)\n+struct tree *lookup_tree_type(struct repository *r,\n+\t\t\t const struct object_id *oid,\n+\t\t\t enum object_type type)\n {\n \tstruct object *obj = lookup_object(r, oid);\n \tif (!obj)\n \t\treturn create_object(r, oid, alloc_tree_node(r));\n-\treturn object_as_type(obj, OBJ_TREE, 0);\n+\treturn object_as_type_hint(obj, OBJ_TREE, type);\n+}\n+\n+struct tree *lookup_tree(struct repository *r, const struct object_id *oid)\n+{\n+\treturn lookup_tree_type(r, oid, OBJ_NONE);\n }\n \n int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size)\ndiff --git a/tree.h b/tree.h\nindex 6efff003e21..4af3b617f3d 100644\n--- a/tree.h\n+++ b/tree.h\n@@ -15,6 +15,8 @@ struct tree {\n extern const char *tree_type;\n \n struct tree *lookup_tree(struct repository *r, const struct object_id *oid);\n+struct tree *lookup_tree_type(struct repository *r, const struct object_id *oid,\n+\t\t\t enum object_type type);\n \n int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size);\n \n-- \n2.39.0.1153.g589e4efe9dc\n\n"},{"id":"469666","messageId":"xmqqzgb5jz5c.fsf@gitster.g","threadId":"58806","inReplyTo":"patch-v2-1.3-0abf873f1e3-20221230T011725Z-avarab@gmail.com","subject":"Re: [PATCH v2 1/3] object tests: add test for unexpected objects in tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-30T04:25:51Z","receivedAt":"2022-12-30T04:25:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> +test_expect_success !SANITIZE_LEAK 'setup unexpected non-tag tag' '\n> +\ttest_when_finished \"git tag -d tag-commit tag-tag\" &&\n> +\n> +\tgit tag -a -m\"my tagged commit\" tag-commit $commit &&\n> +\ttag_commit=$(git rev-parse tag-commit) &&\n> +\tgit tag -a -m\"my tagged tag\" tag-tag tag-commit &&\n> +\ttag_tag=$(git rev-parse tag-tag) &&\n> +\n> +\tgit cat-file tag tag-tag >good-tag-tag &&\n> +\tgit cat-file tag tag-commit >good-commit-tag &&\n> +\n> +\tsed -e \"s/$tag_commit/$commit/\" broken-tag-tag-commit &&\n> +\tsed -e \"s/$tag_commit/$tree/\" broken-tag-tag-tree &&\n> +\tsed -e \"s/$tag_commit/$blob/\" broken-tag-tag-blob &&\n> +\n> +\tsed -e \"s/$commit/$tag_commit/\" broken-commit-tag-tag &&\n> +\tsed -e \"s/$commit/$tree/\" broken-commit-tag-tree &&\n> +\tsed -e \"s/$commit/$blob/\" broken-commit-tag-blob &&\n> +\n> +\ttag_tag_commit=$(git hash-object -w -t tag broken-tag-tag-commit) &&\n> +\ttag_tag_tree=$(git hash-object -w -t tag broken-tag-tag-tree) &&\n> +\ttag_tag_blob=$(git hash-object -w -t tag broken-tag-tag-blob) &&\n\nIf the second block of 3 sed commands to prepare data for tags that\nincorrectly claim to point at a commit are moved a bit, i.e. make\n\"sed's && hash-object's && update-ref's\" as a logical group, the\nabove would become slightly easier to read, but in any case the\nset-up step looks quite repetitive and boring to read ;-)\n\nThere is no strong reason to use broken-tag-* temporary files,\nthough. Each of them is used exactly once, but you can just\npipe the output from \"sed\" to \"git hash-object --stdin\" without\nlosing any exit status, e.g.\n\n\ttag_tag_commit=$(sed -e '...' good-commit-tag |\n\t\t\tgit hash-object -w -t tag --stdin)\n\n> +\tgit update-ref refs/tags/tag_tag_commit $tag_tag_commit &&\n> +\tgit update-ref refs/tags/tag_tag_tree $tag_tag_tree &&\n> +\tgit update-ref refs/tags/tag_tag_blob $tag_tag_blob &&\n> +\n> +\tcommit_tag_tag=$(git hash-object -w -t tag broken-commit-tag-tag) &&\n> +\tcommit_tag_tree=$(git hash-object -w -t tag broken-commit-tag-tree) &&\n> +\tcommit_tag_blob=$(git hash-object -w -t tag broken-commit-tag-blob) &&\n> +\n> +\tgit update-ref refs/tags/commit_tag_tag $commit_tag_tag &&\n> +\tgit update-ref refs/tags/commit_tag_tree $commit_tag_tree &&\n> +\tgit update-ref refs/tags/commit_tag_blob $commit_tag_blob\n> +'\n> +\n> +test_expect_failure !SANITIZE_LEAK 'traverse unexpected incorrectly typed tag (to commit & tag)' '\n> +\ttest_must_fail git rev-list --objects $tag_tag_commit 2>err &&\n\nDoes this have to be \"rev-list --objects\" or would something like\n\"cat-file -t $tag_tag_commit^0\" do?\n\nEspecially because \"rev-list --objects\" is a rather heavy-weight\ncommand that does a lot of checking, I am wondering if it is\nsensible to rely on the assumption that the errors expected below\nwill stay to be the only errors we get before the command exits\n(hence a wish to replace it with a more narrowly focused comamnd).\n\n> +\tcat >expect <<-EOF &&\n> +\terror: object $commit is a commit, not a tag\n> +\tfatal: bad object $commit\n> +\tEOF\n> +\ttest_cmp expect err &&\n\n\n\n> +test_expect_failure !SANITIZE_LEAK 'traverse unexpected objects with for-each-ref' '\n> +\tcat >expect <<-EOF &&\n> +\terror: bad tag pointer to $tree in $tag_tag_tree\n> +\tfatal: parse_object_buffer failed on $tag_tag_tree for refs/tags/tag_tag_tree\n> +\tEOF\n\nThis depends on the fact that among the broken ones tag_tag_tree\nsorts the earliest (and the command stops after barfing on a single\nbad object), doesn't it? I wonder if it makes the test more robust\nby feeding refs/tags/tag_tag_tree from the command line to limit the\ntips the command needs to inspect.\n\n> +>fsck-object-isa\n\nMove it inside the setup as the first command in case \"git fsck\"\nsucceeds?\n\n> +test_expect_success 'setup: unexpected objects with fsck' '\n> +\ttest_must_fail git fsck 2>err &&\n> +\tsed -n -e \"/^error: object .* is a .*, not a .*$/ {\n> +\t\ts/^error: object \\([0-9a-f]*\\) is a \\([a-z]*\\), not a [a-z]*$/\\\\1 \\\\2/;\n> +\t\tp;\n> +\t}\" fsck-object-isa\n> +'\n"},{"id":"469667","messageId":"xmqqsfgxjugi.fsf@gitster.g","threadId":"58806","inReplyTo":"patch-v2-2.3-96398731841-20221230T011725Z-avarab@gmail.com","subject":"Re: [PATCH v2 2/3] tag: don't misreport type of tagged objects in errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-30T06:07:09Z","receivedAt":"2022-12-30T06:07:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> diff --git a/blob.c b/blob.c\n> index 8f83523b0cd..d6e3d64213d 100644\n> --- a/blob.c\n> +++ b/blob.c\n> @@ -5,12 +5,19 @@\n> \n> const char *blob_type = \"blob\";\n> \n> -struct blob *lookup_blob(struct repository *r, const struct object_id *oid)\n> +struct blob *lookup_blob_type(struct repository *r,\n> +\t\t\t const struct object_id *oid,\n> +\t\t\t enum object_type type)\n> {\n> \tstruct object *obj = lookup_object(r, oid);\n> \tif (!obj)\n> \t\treturn create_object(r, oid, alloc_blob_node(r));\n> -\treturn object_as_type(obj, OBJ_BLOB, 0);\n> +\treturn object_as_type_hint(obj, OBJ_BLOB, type);\n> +}\n> +\n> +struct blob *lookup_blob(struct repository *r, const struct object_id *oid)\n> +{\n> +\treturn lookup_blob_type(r, oid, OBJ_NONE);\n> }\n\nBetween lookup_blob() and lookup_blob_type(), the distinction is\nthat the latter calls object_as_type_hint() to ensure that the real\ntype of the given object is of the type, which is given as \"hint\"?\n\nVery confusing naming. Perhaps because \"hint\" argument given to\n\"as-type-hint\" is playing a role that is FAR stronger than \"hint\"?\nIt sounds more like enforcement. Perhaps s/hint/check/ or something?\n\nI am not sure exactly why, but lookup_blob_type() does look\nconfusing. A natural question anybody would ask after seeing the\nfunction mame and the extra parmater is: \"If we want/expect to see a\nblob, why do we give an extra type?\" To answer that question, \"_type\"\nat the end of the function name and \"type\" that is an extra argument\nshould say what that thing is used for. \"enum object_type\" is enough\nto say what the extra argument is---we should use the parameter name\nto convey what it is there for.\n\n> +void *object_as_type_hint(struct object *obj, enum object_type type,\n> +\t\t\t enum object_type hint)\n> +{\n> +\tif (hint != OBJ_NONE && obj->type != OBJ_NONE && obj->type != type) {\n> +\t\terror(_(\"object %s is a %s, not a %s\"), oid_to_hex(&obj->oid),\n> +\t\t type_name(type), type_name(obj->type));\n> +\t\tobj->type = type;\n> +\t\treturn NULL;\n> +\t}\n\nThere must be something utterly wrong in the above implementation.\nAs long as \"hint\" is ANY type other than OBJ_NONE, \"check if type\nand obj->type match and otherwise complain\" logic kicks in, and the\nactual value of \"hint\" does not contribute anything to the outcome.\n\nIOW, the \"hint\" parameter is misleading to be \"enum object_type\", as\nit is only used as a Boolean \"do we bother to check?\".\n\n> +\treturn object_as_type(obj, type, 0);;\n\nStray extra semicolon at the end.\n\n> +}\n"}]}