{"thread":{"id":"62526","subject":"[PATCH] pack-bitmap.c: typofix in `find_boundary_objects()`","startedAt":"2024-11-21T22:50:45Z","lastAt":"2024-11-25T02:29:20Z","messageCount":5,"participants":["Taylor Blau","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"507851","messageId":"cf49115db4e8dcd406a17c946659c2eef3ec6045.1732229420.git.me@ttaylorr.com","threadId":"62526","inReplyTo":null,"subject":"[PATCH] pack-bitmap.c: typofix in `find_boundary_objects()`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-21T22:50:43Z","receivedAt":"2024-11-21T22:50:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"In the boundary-based bitmap traversal, we use the given 'rev_info'\nstructure to first do a commit-only walk in order to determine the\nboundary between interesting and uninteresting objects.\n\nThat walk only looks at commit objects, regardless of the state of\nrevs->blob_objects, revs->tree_objects, and so on. In order to do this,\nwe store the state of these variables in temporary fields before\nsetting them back to zero, performing the traversal, and then setting\nthem back.\n\nBut there is a typo here that dates back to b0afdce5da (pack-bitmap.c:\nuse commit boundary during bitmap traversal, 2023-05-08), where we\nincorrectly store the value of the \"tags\" field as \"revs->blob_objects\".\n\nThis could lead to problems later on if, say, the caller wants tag\nobjects but *not* blob objects. In the pre-image behavior, we'd set\nrevs->tag_objects back to the old value of revs->blob_objects, thus\nemitting fewer objects than expected back to the caller.\n\nFix that by correctly assigning the value of 'revs->tag_objects' to the\n'tmp_tags' field.\n\nSigned-off-by: Taylor Blau \n---\nNoticed while I was looking for an example of this pattern somewhere in\nthe codebase and was surprised when I found this typo ;-).\n\n pack-bitmap.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 4fa9dfc771..683f467051 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -1270,7 +1270,7 @@ static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,\n\n \ttmp_blobs = revs->blob_objects;\n \ttmp_trees = revs->tree_objects;\n-\ttmp_tags = revs->blob_objects;\n+\ttmp_tags = revs->tag_objects;\n \trevs->blob_objects = 0;\n \trevs->tree_objects = 0;\n \trevs->tag_objects = 0;\n\nbase-commit: 4083a6f05206077a50af7658bedc17a94c54607d\n--\n2.47.0.237.gc601277f4c4\n"},{"id":"507868","messageId":"xmqqiksgdrv8.fsf@gitster.g","threadId":"62526","inReplyTo":"cf49115db4e8dcd406a17c946659c2eef3ec6045.1732229420.git.me@ttaylorr.com","subject":"Re: [PATCH] pack-bitmap.c: typofix in `find_boundary_objects()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-22T00:39:07Z","receivedAt":"2024-11-22T00:39:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taylor Blau writes:\n\n> Fix that by correctly assigning the value of 'revs->tag_objects' to the\n> 'tmp_tags' field.\n\nMakes sense. This breakage would make no difference in practice\nright now, as {type}_objects members of the rev_info structure have\nalways been all flipped on together since their inception at\n3c90f03d (Prepare git-rev-list for tracking tag objects too,\n2005-06-29), so the original value of the tag_objects member is\nalways the same as that of the blob_objects member.\n\nA possible alternative \"fix\" for this typo could be to unify these\n{type}_objects members into a single .non_commit_objects member in\nthe rev_info structure; given that we (as far as I remember) never\nutilized the \"feature\" that, say, we can ask only for trees but not\nblobs for the past ~20 years, nobody knows if the apparent \"support\"\nfor that feature is subtly broken in multiple ways (and one of them\nyou just fixed with this patch) and the \"feature\" may not be worth\nkeeping.\n\nBut neverhteless, this is a correct thing to do unless we decide to\nrip out the support for individual types. Will queue.\n\nThanks.\n\n\n\n>\n> Signed-off-by: Taylor Blau \n> ---\n> Noticed while I was looking for an example of this pattern somewhere in\n> the codebase and was surprised when I found this typo ;-).\n>\n> pack-bitmap.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index 4fa9dfc771..683f467051 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -1270,7 +1270,7 @@ static struct bitmap *find_boundary_objects(struct bitmap_index *bitmap_git,\n>\n> \ttmp_blobs = revs->blob_objects;\n> \ttmp_trees = revs->tree_objects;\n> -\ttmp_tags = revs->blob_objects;\n> +\ttmp_tags = revs->tag_objects;\n> \trevs->blob_objects = 0;\n> \trevs->tree_objects = 0;\n> \trevs->tag_objects = 0;\n>\n> base-commit: 4083a6f05206077a50af7658bedc17a94c54607d\n> --\n> 2.47.0.237.gc601277f4c4\n"},{"id":"507935","messageId":"20241122154116.GA1738455@coredump.intra.peff.net","threadId":"62526","inReplyTo":"xmqqiksgdrv8.fsf@gitster.g","subject":"Re: [PATCH] pack-bitmap.c: typofix in `find_boundary_objects()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-22T15:41:16Z","receivedAt":"2024-11-22T15:41:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 22, 2024 at 09:39:07AM +0900, Junio C Hamano wrote:\n\n> Taylor Blau writes:\n> \n> > Fix that by correctly assigning the value of 'revs->tag_objects' to the\n> > 'tmp_tags' field.\n> \n> Makes sense. This breakage would make no difference in practice\n> right now, as {type}_objects members of the rev_info structure have\n> always been all flipped on together since their inception at\n> 3c90f03d (Prepare git-rev-list for tracking tag objects too,\n> 2005-06-29), so the original value of the tag_objects member is\n> always the same as that of the blob_objects member.\n\nYep, I concur.\n\n> A possible alternative \"fix\" for this typo could be to unify these\n> {type}_objects members into a single .non_commit_objects member in\n> the rev_info structure; given that we (as far as I remember) never\n> utilized the \"feature\" that, say, we can ask only for trees but not\n> blobs for the past ~20 years, nobody knows if the apparent \"support\"\n> for that feature is subtly broken in multiple ways (and one of them\n> you just fixed with this patch) and the \"feature\" may not be worth\n> keeping.\n\nI have been tempted to do that, too. But FWIW, I do remember\nimplementing something that set some but not all of the fields in a\nseries. Digging in my old branches, I think it may have been for a\nreachability check for remote git-archive, where we want to dig only as\ndeep as the object we are looking for (so if we are finding out if a\ntree is reachable, we do not need to ask about blobs, and looking for a\ntag needs neither trees nor blobs).\n\nIf no such code made it into the project in those 20 years, it may not\nworth worrying about too much. But I wanted to point it out as a\nplausible use case.\n\n> But neverhteless, this is a correct thing to do unless we decide to\n> rip out the support for individual types. Will queue.\n\nAgreed.\n\n-Peff\n"},{"id":"507936","messageId":"20241122154437.GB1738455@coredump.intra.peff.net","threadId":"62526","inReplyTo":"20241122154116.GA1738455@coredump.intra.peff.net","subject":"Re: [PATCH] pack-bitmap.c: typofix in `find_boundary_objects()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-22T15:44:37Z","receivedAt":"2024-11-22T15:44:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 22, 2024 at 10:41:16AM -0500, Jeff King wrote:\n\n> > A possible alternative \"fix\" for this typo could be to unify these\n> > {type}_objects members into a single .non_commit_objects member in\n> > the rev_info structure; given that we (as far as I remember) never\n> > utilized the \"feature\" that, say, we can ask only for trees but not\n> > blobs for the past ~20 years, nobody knows if the apparent \"support\"\n> > for that feature is subtly broken in multiple ways (and one of them\n> > you just fixed with this patch) and the \"feature\" may not be worth\n> > keeping.\n> \n> I have been tempted to do that, too. But FWIW, I do remember\n> implementing something that set some but not all of the fields in a\n> series. Digging in my old branches, I think it may have been for a\n> reachability check for remote git-archive, where we want to dig only as\n> deep as the object we are looking for (so if we are finding out if a\n> tree is reachable, we do not need to ask about blobs, and looking for a\n> tag needs neither trees nor blobs).\n> \n> If no such code made it into the project in those 20 years, it may not\n> worth worrying about too much. But I wanted to point it out as a\n> plausible use case.\n\nThere are also two curious cases added by f18b512bbb (bundle: create\nfiltered bundles, 2022-03-09) that set tree/blob, but not \"tag\". Not\nsure if that is a bug or not.\n\n-Peff\n"},{"id":"508004","messageId":"xmqq5xoc82rm.fsf@gitster.g","threadId":"62526","inReplyTo":"20241122154437.GB1738455@coredump.intra.peff.net","subject":"Re: [PATCH] pack-bitmap.c: typofix in `find_boundary_objects()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-25T02:29:17Z","receivedAt":"2024-11-25T02:29:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> There are also two curious cases added by f18b512bbb (bundle: create\n> filtered bundles, 2022-03-09) that set tree/blob, but not \"tag\". Not\n> sure if that is a bug or not.\n\nI do not think it is a good idea to single out bundles, but I can\nsort of see how it could be useful when making a partial clone that\ndoes not have blobs. `git pack-objects --objects=tag,commit,tree`\nmight want to be driven when preparing pack data to populate such a\nclone initially. Unifying these three object flags into \"all the\nothers\" is tempting but would close the door for such a tweak.\n\n\n\n"}]}