{"thread":{"id":"58362","subject":"[PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths","startedAt":"2022-08-26T07:09:25Z","lastAt":"2022-11-29T13:21:19Z","messageCount":67,"participants":["Teng Long","Junio C Hamano","Taylor Blau","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"461976","messageId":"20220826070912.70274-1-dyroneteng@gmail.com","threadId":"58362","inReplyTo":null,"subject":"[PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-08-26T07:09:11Z","receivedAt":"2022-08-26T07:09:25Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"This patch tries to avoid the problem of leaking sensitive information that\ncould output the absolute path of the repository when try to open multiple\nbitmaps. For example, in \"alternates\" scenario, where the repository\n\"alternate_repo\" serves as alternate object stores for repository\n\"want_to_borrow\" , and each of both has it's own bitmap file, then we run\n`git rev-list --use-bitmap-index HEAD`, the output might be:\n\n $ cd want_to_borrow.git\n $ git rev-list --test-bitmap HEAD\n warning: ignoring extra bitmap file: /Users/tenglong.tl/Downloads/alternate_repo.git/.git/objects/pack/pack-bff67e2a7a154e6933afe61b3681726cf9bd8e8b.pack\n Bitmap v1 test (1 entries loaded)\n Found bitmap for 30f146d41a7a86930fae3f4a5e1f1c1f2bfacbba. 64 bits / 11030517 checksum\n Verifying bitmap entries: 100% (3/3), done.\n OK!\n\nAfter apply this patch:\n\n $ git rev-list --test-bitmap HEAD\n warning: ignoring extra bitmap files\n Bitmap v1 test (1 entries loaded)\n Found bitmap for 30f146d41a7a86930fae3f4a5e1f1c1f2bfacbba. 64 bits / 11030517 checksum\n Verifying bitmap entries: 100% (3/3), done.\n OK!\n\nThanks.\n\nTeng Long (1):\n pack-bitmap.c: avoid exposing absolute paths\n\n pack-bitmap.c | 8 ++++----\n t/t5310-pack-bitmaps.sh | 2 +-\n t/t5326-multi-pack-bitmaps.sh | 2 +-\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\n-- \n2.37.2.1.g1591e7ee52e\n\n"},{"id":"461977","messageId":"20220826070912.70274-2-dyroneteng@gmail.com","threadId":"58362","inReplyTo":"20220826070912.70274-1-dyroneteng@gmail.com","subject":"[PATCH 1/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-08-26T07:09:12Z","receivedAt":"2022-08-26T07:09:27Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"In \"open_midx_bitmap_1()\" and \"open_pack_bitmap_1()\", when we find that\nthere are multiple bitmaps, we will only open the first one and then\nleave warnings about the remaining pack information, the information\nwill contain the absolute path of the repository, for example in a\nalternates usage scenario. So let's hide this kind of potentially\nsensitive information in this commit.\n\nFound-by: XingXin \nSigned-off-by: Teng Long \n---\n pack-bitmap.c | 8 ++++----\n t/t5310-pack-bitmaps.sh | 2 +-\n t/t5326-multi-pack-bitmaps.sh | 2 +-\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 36134222d7a..5103d91d18a 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -331,8 +331,8 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n \tif (bitmap_git->pack || bitmap_git->midx) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tget_midx_filename(&buf, midx->object_dir);\n-\t\t/* ignore extra bitmap file; we can only handle one */\n-\t\twarning(\"ignoring extra bitmap file: %s\", buf.buf);\n+\t\t/* ignore extra midx bitmap files; we can only handle one */\n+\t\twarning(\"ignoring extra midx bitmap files\");\n \t\tclose(fd);\n \t\tstrbuf_release(&buf);\n \t\treturn -1;\n@@ -402,8 +402,8 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \t}\n \n \tif (bitmap_git->pack || bitmap_git->midx) {\n-\t\t/* ignore extra bitmap file; we can only handle one */\n-\t\twarning(\"ignoring extra bitmap file: %s\", packfile->pack_name);\n+\t\t/* ignore extra bitmap files; we can only handle one */\n+\t\twarning(\"ignoring extra bitmap files\");\n \t\tclose(fd);\n \t\treturn -1;\n \t}\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex f775fc1ce69..7cd6d79a022 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -421,7 +421,7 @@ test_expect_success 'complains about multiple pack bitmaps' '\n \t\ttest_line_count = 2 bitmaps &&\n \n \t\tgit rev-list --use-bitmap-index HEAD 2>err &&\n-\t\tgrep \"ignoring extra bitmap file\" err\n+\t\tgrep \"ignoring extra bitmap files\" err\n \t)\n '\n \ndiff --git a/t/t5326-multi-pack-bitmaps.sh b/t/t5326-multi-pack-bitmaps.sh\nindex 4fe57414c13..1786f28376a 100755\n--- a/t/t5326-multi-pack-bitmaps.sh\n+++ b/t/t5326-multi-pack-bitmaps.sh\n@@ -303,7 +303,7 @@ test_expect_success 'graceful fallback when missing reverse index' '\n \n \t\tGIT_TEST_MIDX_READ_RIDX=0 \\\n \t\t\tgit rev-list --use-bitmap-index HEAD 2>err &&\n-\t\t! grep \"ignoring extra bitmap file\" err\n+\t\tgrep \"multi-pack bitmap is missing required reverse index\" err\n \t)\n '\n \n-- \n2.37.2.1.g1591e7ee52e\n\n"},{"id":"462001","messageId":"xmqqtu5zyndk.fsf@gitster.g","threadId":"58362","inReplyTo":"20220826070912.70274-1-dyroneteng@gmail.com","subject":"Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-26T16:34:47Z","receivedAt":"2022-08-26T16:34:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> This patch tries to avoid the problem of leaking sensitive information that\n> could output the absolute path of the repository when try to open multiple\n> bitmaps. For example, in \"alternates\" scenario, where the repository\n> \"alternate_repo\" serves as alternate object stores for repository\n> \"want_to_borrow\" , and each of both has it's own bitmap file, then we run\n> `git rev-list --use-bitmap-index HEAD`, the output might be:\n\n> $ cd want_to_borrow.git\n> $ git rev-list --test-bitmap HEAD\n> warning: ignoring extra bitmap file: /Users/tenglong.tl/Downloads/alternate_repo.git/.git/objects/pack/pack-bff67e2a7a154e6933afe61b3681726cf9bd8e8b.pack\n> Bitmap v1 test (1 entries loaded)\n> Found bitmap for 30f146d41a7a86930fae3f4a5e1f1c1f2bfacbba. 64 bits / 11030517 checksum\n> Verifying bitmap entries: 100% (3/3), done.\n> OK!\n>\n> After apply this patch:\n>\n> $ git rev-list --test-bitmap HEAD\n> warning: ignoring extra bitmap files\n> Bitmap v1 test (1 entries loaded)\n> Found bitmap for 30f146d41a7a86930fae3f4a5e1f1c1f2bfacbba. 64 bits / 11030517 checksum\n> Verifying bitmap entries: 100% (3/3), done.\n> OK!\n\nI am not sure why it is an improvement though.\n\nDoesn't it make it harder to diagnose where the extra bitmap file\nyou do not need exists (so that you can check if you can/want\nto/need to remove it)?\n\nIf the \"ignoring extra\" is a totally expected situation (e.g. it is\nnot suprising if we always ignore the bitmapfile in the alternate\nwhen we have our own), perhaps we should squelch the warning in such\nexpected cases altogether (while warning other cases where we see\nmore bitmap files than we expect to see, which may be an anomaly\nworth warning about), and that may be an improvement worth spending\ndevelopment cycles on, but I am not sure about this one.\n\n"},{"id":"462065","messageId":"20220829024803.47496-1-tenglong.tl@alibaba-inc.com","threadId":"58362","inReplyTo":"xmqqtu5zyndk.fsf@gitster.g","subject":"Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-08-29T02:48:03Z","receivedAt":"2022-08-29T02:48:15Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Junio C Hamano writes:\n\n> Doesn't it make it harder to diagnose where the extra bitmap file\n> you do not need exists (so that you can check if you can/want\n> to/need to remove it)?\n\nYes, if we want that warnings for diagnosing we shouldn't remove the related\npath but I think it should build on the security concerns. I don't mean that\nto warning the packfile name in a absolute path is a danger, but when serving\na git server maybe we don't want to expose the real path information on server\nto client users.\n\n> If the \"ignoring extra\" is a totally expected situation (e.g. it is\n> not suprising if we always ignore the bitmapfile in the alternate\n> when we have our own), perhaps we should squelch the warning in such\n> expected cases altogether (while warning other cases where we see\n> more bitmap files than we expect to see, which may be an anomaly\n> worth warning about), and that may be an improvement worth spending\n> development cycles on, but I am not sure about this one.\n\nThat's exactly good suggestion. In my opinion, I think to avoid the sensitive\nwarning and the same time we keep some information to let the users know \"Oh,\nthere are some extra existing bitmaps we just ignored then maybe can do some\noptimization works\", but I think just remove the total warning here is\nreasonable also, i'm good with it.\n\nThanks.\n"},{"id":"465843","messageId":"Y1mp23NHB0qzKsPR@nand.local","threadId":"58362","inReplyTo":"20220829024803.47496-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-26T21:42:51Z","receivedAt":"2022-10-26T21:42:55Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Aug 29, 2022 at 10:48:03AM +0800, Teng Long wrote:\n> > If the \"ignoring extra\" is a totally expected situation (e.g. it is\n> > not suprising if we always ignore the bitmapfile in the alternate\n> > when we have our own), perhaps we should squelch the warning in such\n> > expected cases altogether (while warning other cases where we see\n> > more bitmap files than we expect to see, which may be an anomaly\n> > worth warning about), and that may be an improvement worth spending\n> > development cycles on, but I am not sure about this one.\n>\n> That's exactly good suggestion. In my opinion, I think to avoid the sensitive\n> warning and the same time we keep some information to let the users know \"Oh,\n> there are some extra existing bitmaps we just ignored then maybe can do some\n> optimization works\", but I think just remove the total warning here is\n> reasonable also, i'm good with it.\n\nI think that it is somewhat of a step backwards to remove it entirely,\nbut let me qualify that a little bit.\n\nAt GitHub, we actually *do* remove this warning entirely:\n\n--- >8 ---\n\nFrom: Jeff King \nSubject: [PATCH] pack-bitmap: drop \"ignoring extra bitmap\" warning\n\nFor simplicity, the bitmap code only handles a single bitmap\nfile at a time. If you have two, it emits a warning.\n\nThis is usually the sign of a misconfiguration, but it can\nalso happen racily during maintenance. We create and install\nthe new bitmap file and then remove the old one. As a\nresult, users may see something like:\n\n $ git fetch\n ...\n remote: warning: ignoring extra bitmap file: /path/to/pack-xyz.bitmap\n\nThis is scary for them (even though the condition is totally\nharmless), and it exposes a bunch of internal paths. Let's\njust silence the warning entirely. It's possible that this\nmay make debugging some obscure case harder, but it seems\nrather unlikely.\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 d3fde3d3b28..283da33a648 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -298,7 +298,7 @@ static int open_pack_bitmap_1(struct packed_git *packfile)\n \t}\n\n \tif (bitmap_git.pack) {\n-\t\twarning(\"ignoring extra bitmap file: %s\", packfile->pack_name);\n+\t\t/* ignore extra bitmap file; we can only handle one */\n \t\tclose(fd);\n \t\treturn -1;\n \t}\n--\n2.38.0.16.g393fd4c6db\n\n--- 8< ---\n\n...and as the patch message notes, this is done mostly to prevent\nconfusion when racily fetching from GitHub, or due to some\nmisconfiguration.\n\nAnd in that instance, I think the patch from Peff is right. If there is\na legitimate bug, we'd see it elsewhere and have sufficiently powerful\ntools to investigate it. But the warning is useless and confusing to\nusers who don't have access to such tools.\n\nFor the general case of what ships in git.git, I *do* find this warning\nuseful. It's helpful when hacking on pack-bitmap.c to know if you've\nmessed up, and it's useful to see the filename of the affected\nbitmap(s).\n\nI think we could reasonably change the warning to\n\n warning(_(\"ignoring extra bitmap file: %s\"),\n basename(packfile->pack_name));\n\nsince the rest of the path is obvious based on which repository you're\nworking in. So that would be a reasonable change to shorten up the\noutput a little bit.\n\nYou could also imagine adding a configuration knob here to control\nwhether or not the warning is shown, but I find that to be kind of\ngross.\n\nSo I think that the warning is--in general--too useful to consider\nremoving it entirely, and that at most we should consider just emitting\nthe basename of the pack, but nothing else.\n\nThanks,\nTaylor\n"},{"id":"465848","messageId":"221027.86czaeb1xt.gmgdl@evledraar.gmail.com","threadId":"58362","inReplyTo":"Y1mp23NHB0qzKsPR@nand.local","subject":"Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-26T23:19:16Z","receivedAt":"2022-10-26T23:28:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Oct 26 2022, Taylor Blau wrote:\n\n> On Mon, Aug 29, 2022 at 10:48:03AM +0800, Teng Long wrote:\n>> > If the \"ignoring extra\" is a totally expected situation (e.g. it is\n>> > not suprising if we always ignore the bitmapfile in the alternate\n>> > when we have our own), perhaps we should squelch the warning in such\n>> > expected cases altogether (while warning other cases where we see\n>> > more bitmap files than we expect to see, which may be an anomaly\n>> > worth warning about), and that may be an improvement worth spending\n>> > development cycles on, but I am not sure about this one.\n>>\n>> That's exactly good suggestion. In my opinion, I think to avoid the sensitive\n>> warning and the same time we keep some information to let the users know \"Oh,\n>> there are some extra existing bitmaps we just ignored then maybe can do some\n>> optimization works\", but I think just remove the total warning here is\n>> reasonable also, i'm good with it.\n>\n> I think that it is somewhat of a step backwards to remove it entirely,\n> but let me qualify that a little bit.\n>\n> At GitHub, we actually *do* remove this warning entirely:\n\nYou at GitHub also added it entirely :) => fff42755efc (pack-bitmap: add\nsupport for bitmap indexes, 2013-12-21).\n\nAnyway, I'm fine with removing it. From skimming that commit it was\nprobably added for no particularly strong reason. But I found the\nomission of \"it was added in xyz commit\" to be sometihng that could be\nadded to the commit message in this case, and....\n\n> You could also imagine adding a configuration knob here to control\n> whether or not the warning is shown, but I find that to be kind of\n> gross.\n\nFWIW I don't find that to be particularly gross. I think it's fine to\njust delete it.\n\nBut isn't this a general sign that we should perhaps have different\noutput when \"pack-objects\" and the like is run \"locally\", v.s. when\nwe're running via some server process, and end up spewing a message out\nthat the user can't do anything about?\n\n"},{"id":"465908","messageId":"Y1rt+uOKwlP5PIrT@coredump.intra.peff.net","threadId":"58362","inReplyTo":"Y1mp23NHB0qzKsPR@nand.local","subject":"Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-27T20:45:46Z","receivedAt":"2022-10-27T20:54:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 26, 2022 at 05:42:51PM -0400, Taylor Blau wrote:\n\n> At GitHub, we actually *do* remove this warning entirely:\n> \n> --- >8 ---\n> \n> From: Jeff King \n> Subject: [PATCH] pack-bitmap: drop \"ignoring extra bitmap\" warning\n\nA blast from the past. You didn't include the Date field, but this was\nfrom ~2017.\n\n> For the general case of what ships in git.git, I *do* find this warning\n> useful. It's helpful when hacking on pack-bitmap.c to know if you've\n> messed up, and it's useful to see the filename of the affected\n> bitmap(s).\n\nIt feels like you might be a special case, though. Most people are not\nhacking on the bitmap code. :)\n\nI wonder if you'd be better served by having good tracing for the bitmap\ncode. Then it could tell you lots of things, like which bitmap it\nactually opened, which ones it ignored, etc. Of course you'd have to\nremember to enable that tracing when you're working in the area.\n\nThe other thing that has always bugged me about this warning (and would\nbe true of tracing code, too) is that it inherently requires looking at\nall of the packfiles. I.e., open_pack_bitmap() doesn't return when it\nfinds something; it keeps going just in case it finds another bitmap so\nit can say \"hey, there's an extra one!\".\n\nThis complicates the code a little bit. But it's also inefficient. If we\nhave N packs, we'll do N open() calls, most of which will fail. Weirder,\nthough, is that we open the pack .idx first. So we're literally opening,\nmapping, and checking the idx for every pack for no reason!\n\nNow this might not be as bad as it seems:\n\n - in the long run, we might open those idx files anyway, if we have to\n access those packs. So it's really just overriding the lazy-open\n behavior.\n\n - in the worst case, the one bitmap file is at the end of the list, so\n you hit all N anyway. And this is actually pretty common, since we\n sort in reverse-chronological order, and the bitmap is usually on\n the oldest and biggest pack.\n\n - in general, having a lot of packs has bad performance for other\n reasons. So you'd generally want to keep N small-ish in the first\n place.\n\nSo it may not be worth worrying about. It does seem like it would be\neasy to reorder open_pack_bitmap_1() to look for a bitmap file first and\nonly open the idx if it finds something.\n\nIt's possible we have a weird race-dependency on opening those idx files\nearly (certainly there's some subtlety with pack-objects keeping the\n.pack files themselves open which prevents problems with a simultaneous\nrepack). But I think it would be OK; we don't commit to using a\nparticular .idx file in the general case, and one that goes away would\njust cause us to reprepare_packed_git() and continue. For a particular\n.bitmap, we do commit to having the matching .idx file, but obviously\nwe'd want to open and check that one before considering the bitmap to be\nsuccessfully opened.\n\n> I think we could reasonably change the warning to\n> \n> warning(_(\"ignoring extra bitmap file: %s\"),\n> basename(packfile->pack_name));\n> \n> since the rest of the path is obvious based on which repository you're\n> working in. So that would be a reasonable change to shorten up the\n> output a little bit.\n\nYeah. If we are going to keep the warning, this makes much better sense.\nPossibly we could even just skip_prefix() on the object directory,\nthough I think in practice it amounts to the same thing.\n\n-Peff\n"},{"id":"466065","messageId":"Y17FeGeWqEE7CWQS@nand.local","threadId":"58362","inReplyTo":"Y1rt+uOKwlP5PIrT@coredump.intra.peff.net","subject":"Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-10-30T18:42:00Z","receivedAt":"2022-10-30T18:42:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Oct 27, 2022 at 04:45:46PM -0400, Jeff King wrote:\n> On Wed, Oct 26, 2022 at 05:42:51PM -0400, Taylor Blau wrote:\n>\n> > At GitHub, we actually *do* remove this warning entirely:\n> >\n> > --- >8 ---\n> >\n> > From: Jeff King \n> > Subject: [PATCH] pack-bitmap: drop \"ignoring extra bitmap\" warning\n>\n> A blast from the past. You didn't include the Date field, but this was\n> from ~2017.\n\nI'll blame you for that one, since this output comes from your\n'git print-patch's script, which only prints out the From: and Subject:\nheaders. ;-)\n\n> > For the general case of what ships in git.git, I *do* find this warning\n> > useful. It's helpful when hacking on pack-bitmap.c to know if you've\n> > messed up, and it's useful to see the filename of the affected\n> > bitmap(s).\n>\n> It feels like you might be a special case, though. Most people are not\n> hacking on the bitmap code. :)\n>\n> I wonder if you'd be better served by having good tracing for the bitmap\n> code. Then it could tell you lots of things, like which bitmap it\n> actually opened, which ones it ignored, etc. Of course you'd have to\n> remember to enable that tracing when you're working in the area.\n\nYeah, I wouldn't be sad to see this get moved to the trace2 mechanism.\nIn fact, I almost prefer that approach, since server operators and\ndevelopers can still debug the bitmap machinery, but end-users of forges\nlike GitHub won't see the output.\n\nMaybe that is the best approach forward. Teng Long: does that sound OK\nto you?\n\n> So it may not be worth worrying about. It does seem like it would be\n> easy to reorder open_pack_bitmap_1() to look for a bitmap file first and\n> only open the idx if it finds something.\n\nYeah, I had always thought that it was odd that we had to go through all\nof the repository's possible bitmaps and try to open them one by one. It\nmight be interesting to have a file that lists the path of _the_\nbitmap, which we consult first and then try and open that pack or MIDX\nbitmap. But I dunno.\n\n> > I think we could reasonably change the warning to\n> >\n> > warning(_(\"ignoring extra bitmap file: %s\"),\n> > basename(packfile->pack_name));\n> >\n> > since the rest of the path is obvious based on which repository you're\n> > working in. So that would be a reasonable change to shorten up the\n> > output a little bit.\n>\n> Yeah. If we are going to keep the warning, this makes much better sense.\n> Possibly we could even just skip_prefix() on the object directory,\n> though I think in practice it amounts to the same thing.\n\nI agree, they end up the same in either approach you take.\n\nThanks,\nTaylor\n"},{"id":"466083","messageId":"20221031122234.22473-1-dyroneteng@gmail.com","threadId":"58362","inReplyTo":"Y17FeGeWqEE7CWQS@nand.local","subject":"Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths Taylor Blau writes:","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-10-31T12:22:34Z","receivedAt":"2022-10-31T12:22:59Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"> Maybe that is the best approach forward. Teng Long: does that sound OK > to you? \nSorry, there are some problems on my Mac (I can't run coverage but it's ok now), I will read the context and reply again a little bit later. Thanks for Taylor Blau, Ævar Arnfjörð Bjarmason and Jeff King to supply the discussions on this topic. \n"},{"id":"466087","messageId":"20221031131356.67007-1-dyroneteng@gmail.com","threadId":"58362","inReplyTo":"Y1mp23NHB0qzKsPR@nand.local","subject":"Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-10-31T13:13:56Z","receivedAt":"2022-10-31T13:14:05Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Taylor Blau writes:\n\n> --- >8 ---\n\nWhat does \">8\" means? \n\nIt's like a Github internal patch between the \">8\"\nand \"<8\" marks, right?\n\n> ...and as the patch message notes, this is done mostly to prevent\n> confusion when racily fetching from GitHub, or due to some\n> misconfiguration.\n\n> And in that instance, I think the patch from Peff is right. If there is\n> a legitimate bug, we'd see it elsewhere and have sufficiently powerful\n> tools to investigate it. But the warning is useless and confusing to\n> users who don't have access to such tools.\n\nYes, currently there will be only one bitmap to be process and other\nones will be skipped, so we should remove it I think because it's no\nactual meanings for client uses, they do not care about what the server\nskipped and hope to avoid the disturbing information.\n\n> For the general case of what ships in git.git, I *do* find this warning\n> ...\n\n\"ships in git.git\" means as the git developers, right?\n\n> useful. It's helpful when hacking on pack-bitmap.c to know if you've\n> messed up, and it's useful to see the filename of the affected\n> bitmap(s).\n\nI agree, let things expose entirely, but it's a trade-off.\n\n> You could also imagine adding a configuration knob here to control\n> whether or not the warning is shown, but I find that to be kind of\n> gross.\n> \n> So I think that the warning is--in general--too useful to consider\n> removing it entirely, and that at most we should consider just emitting\n> the basename of the pack, but nothing else. \n\nI think it's no need for a new config knob here, I agree with you on this\npoint.\n\nAnd keep the pack basename seems like definitly a better solution than\nthe current one and I have a other solution which is to replace \"warn()\"\nto a trace2 api, so the developer of git or ships in git.git if I\nunderstand it right ;-) could get the warning in trace2 output by:\n\n\n➜ pack git:(master) GIT_TRACE2_PERF=1 git rev-list --test-bitmap HEAD <<<\n21:04:08.702523 common-main.c:49 | d0 | main | version | | | | | 2.38.0\n21:04:08.702791 common-main.c:50 | d0 | main | start | | 0.001922 | | | /usr/local/bin/git rev-list --test-bitmap HEAD\n21:04:08.703553 git.c:461 | d0 | main | cmd_name | | | | | rev-list (rev-list)\n21:04:08.704889 usage.c:86 | d0 | main | error | | | | | ignoring extra bitmap file: '/Users/tenglong.tl/Downloads/trace-test/.git/objects/pack/pack-3cea516b416961285fd8f519e12102b19bcf257e.pack'\nwarning: ignoring extra bitmap file: '/Users/tenglong.tl/Downloads/trace-test/.git/objects/pack/pack-3cea516b416961285fd8f519e12102b19bcf257e.pack'\nBitmap v1 test (2 entries loaded)\nFound bitmap for '2c5959955b5e6167181d08eeb30ee4099b4a4c5b'. 64 bits / ca44d5df checksum\n21:04:08.705305 progress.c:268 | d0 | main | region_enter | r0 | 0.004448 | | progress | label:Verifying bitmap entries\nVerifying bitmap entries: 100% (6/6), done.\n21:04:08.705814 progress.c:340 | d0 | main | data | r0 | 0.004955 | 0.000507 | progress | ..total_objects:6\n21:04:08.705829 progress.c:346 | d0 | main | region_leave | r0 | 0.004972 | 0.000524 | progress | label:Verifying bitmap entries\nOK!\n21:04:08.705883 git.c:721 | d0 | main | exit | | 0.005026 | | | code:0\n21:04:08.705899 trace2/tr2_tgt_perf.c:215 | d0 | main | atexit | | 0.005043 | | | code:0\n\n\n\nI don't know which is better and I will go on reading the left replies.\n\nThanks for the hard working on this patchset, Taylor, really appreciate for that.\n"},{"id":"466088","messageId":"20221031132002.72629-1-dyroneteng@gmail.com","threadId":"58362","inReplyTo":"221027.86czaeb1xt.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-10-31T13:20:02Z","receivedAt":"2022-10-31T13:20:13Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"\"Ævar Arnfjörð Bjarmason\" writes:\n\n> But isn't this a general sign that we should perhaps have different\n> output when \"pack-objects\" and the like is run \"locally\", v.s. when\n> we're running via some server process, and end up spewing a message out\n> that the user can't do anything about?\n\n\nI think that maybe we could just use \"trace2\" to print the detailed\ninformations we (git developers) care about for some diagnosing work \nand do not output the warnings to the client users.\n\nMaybe it's a solution that satisfies both scenarios.\n\nThanks.\n"},{"id":"466279","messageId":"20221102053748.7366-1-tenglong.tl@alibaba-inc.com","threadId":"58362","inReplyTo":"Y1rt+uOKwlP5PIrT@coredump.intra.peff.net","subject":"Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-02T05:37:48Z","receivedAt":"2022-11-02T05:38:04Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Jeff King writes：\n\n> Now this might not be as bad as it seems:\n>\n> - in the long run, we might open those idx files anyway, if we have to\n> access those packs. So it's really just overriding the lazy-open\n> behavior.\n\nSorry, can you explain it a bit more. When we might open idxes anyway? Do you\nmean if the pack idx files will be opened sooner or later if a repo serves\ngit-upload-pack many times in the long run. So, the system-wide table or the\nmmap space will not be wasted so much in practice.\n\n> - in the worst case, the one bitmap file is at the end of the list, so\n> you hit all N anyway. And this is actually pretty common, since we\n> sort in reverse-chronological order, and the bitmap is usually on\n> the oldest and biggest pack.\n\nYes, in \"sort_pack()\" I think.\n\n> So it may not be worth worrying about. It does seem like it would be\n> easy to reorder open_pack_bitmap_1() to look for a bitmap file first and\n> only open the idx if it finds something.\n\nI think it may be worthy if we have lots of packs and the bitmap is refer to\nan older one, but I didn't make the test. At least, the scenario is common, I\nagree with that, so maybe we could shuffle the sort order in \"open_pack_bitmap()\".\n\nThanks.\n"},{"id":"466292","messageId":"Y2IiSU1L+bJPUioV@coredump.intra.peff.net","threadId":"58362","inReplyTo":"20221102053748.7366-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-02T07:54:49Z","receivedAt":"2022-11-02T07:55:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 02, 2022 at 01:37:48PM +0800, Teng Long wrote:\n\n> Jeff King writes：\n> \n> > Now this might not be as bad as it seems:\n> >\n> > - in the long run, we might open those idx files anyway, if we have to\n> > access those packs. So it's really just overriding the lazy-open\n> > behavior.\n> \n> Sorry, can you explain it a bit more. When we might open idxes anyway? Do you\n> mean if the pack idx files will be opened sooner or later if a repo serves\n> git-upload-pack many times in the long run. So, the system-wide table or the\n> mmap space will not be wasted so much in practice.\n\nI mean that later in the process, if we need to find an object we may\nopen the .idx file to look for it. So by opening them all up front, we\n_might_ just be doing work that would get done later.\n\nBut it's not guaranteed. Imagine you have 10,000 small packs, and one\nbig bitmapped pack. If you can serve the request from just the big pack,\nthen you'd never need to open those other .idx files at all. However,\nthe current code will open them anyway.\n\nI care less about mmap space, and more that it's work (syscalls, and\nexamining the contents of the idx) to open each one. It's probably not\neven measurable unless you have a ton of packs, though.\n\n> > So it may not be worth worrying about. It does seem like it would be\n> > easy to reorder open_pack_bitmap_1() to look for a bitmap file first and\n> > only open the idx if it finds something.\n> \n> I think it may be worthy if we have lots of packs and the bitmap is refer to\n> an older one, but I didn't make the test. At least, the scenario is common, I\n> agree with that, so maybe we could shuffle the sort order in \"open_pack_bitmap()\".\n\nI don't mean the order in which we look at packs. I mean the order of\noperations in open_pack_bitmap_1(), something like:\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 440407f1be..1df2f6c8b6 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -411,9 +411,6 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \tstruct stat st;\n \tchar *bitmap_name;\n \n-\tif (open_pack_index(packfile))\n-\t\treturn -1;\n-\n \tbitmap_name = pack_bitmap_filename(packfile);\n \tfd = git_open(bitmap_name);\n \n@@ -438,6 +435,10 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \t\treturn -1;\n \t}\n \n+\t/* now we know we have a plausible bitmap; make sure the idx is OK, too */\n+\tif (open_pack_index(packfile))\n+\t\treturn -1;\n+\n \tif (!is_pack_valid(packfile)) {\n \t\tclose(fd);\n \t\treturn -1;\n\nBut we can further observe that the first thing is_pack_valid() will do\nis open the idx file. :) So we can really just drop this line entirely,\nI'd think.\n\nBTW, another oddity I noticed in this function. We check:\n\n if (bitmap_git->pack || bitmap_git->midx) {\n\t /* ignore extra bitmap file; we can only handle one */\n\t ...\n }\n\nbut it's impossible for bitmap_git->midx to be set here. If we opened\nthe midx bitmap, we'll skip calling open_pack_bitmap() entirely.\n\n-Peff\n"},{"id":"466302","messageId":"221102.86tu3h3e1w.gmgdl@evledraar.gmail.com","threadId":"58362","inReplyTo":"20220826070912.70274-1-dyroneteng@gmail.com","subject":"Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-02T09:20:31Z","receivedAt":"2022-11-02T09:26:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Aug 26 2022, Teng Long wrote:\n\n> This patch tries to avoid the problem of leaking sensitive information that\n> could output the absolute path of the repository when try to open multiple\n> bitmaps. For example, in \"alternates\" scenario, where the repository\n> \"alternate_repo\" serves as alternate object stores for repository\n> \"want_to_borrow\" , and each of both has it's own bitmap file, then we run\n> `git rev-list --use-bitmap-index HEAD`, the output might be:\n>\n> $ cd want_to_borrow.git\n> $ git rev-list --test-bitmap HEAD\n> warning: ignoring extra bitmap file: /Users/tenglong.tl/Downloads/alternate_repo.git/.git/objects/pack/pack-bff67e2a7a154e6933afe61b3681726cf9bd8e8b.pack\n> Bitmap v1 test (1 entries loaded)\n> Found bitmap for 30f146d41a7a86930fae3f4a5e1f1c1f2bfacbba. 64 bits / 11030517 checksum\n> Verifying bitmap entries: 100% (3/3), done.\n> OK!\n>\n> After apply this patch:\n>\n> $ git rev-list --test-bitmap HEAD\n> warning: ignoring extra bitmap files\n> Bitmap v1 test (1 entries loaded)\n> Found bitmap for 30f146d41a7a86930fae3f4a5e1f1c1f2bfacbba. 64 bits / 11030517 checksum\n> Verifying bitmap entries: 100% (3/3), done.\n> OK!\n\nI see that downthread of here there's discussion about keeping the\nwarning, adding tracing, etc. etc.\n\nMaybe it's been brought up (I was skimming, sorry), but for the problem\nyou have isn't a narrow and acceptable solution to you to keep the\nwarning, but just don't print the absolute path?\n\nI.e.:\n\n\twarning: ignoring extra bitmap file: /Users/tenglong.tl/Downloads/alternate_repo.git/.git/objects/pack/pack-bff67e2a7a154e6933afe61b3681726cf9bd8e8b.pack\n\nTo:\n\n\twarning: ignoring extra bitmap file: ../alternate_repo.git/.git/objects/pack/pack-bff67e2a7a154e6933afe61b3681726cf9bd8e8b.pack\n\nOr would the relative path to the alternate also be sensitive?\n\nWe might also want to just remove this etc., but that's a different\nquestion than \"should we print these absolute paths?\".\n"},{"id":"466308","messageId":"cover.1667393419.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"20220826070912.70274-1-dyroneteng@gmail.com","subject":"[PATCH v2 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-02T12:56:04Z","receivedAt":"2022-11-02T12:56:17Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nDiff since the first patch:\n\n * remove the \"ignore extra bitmap file\" warning\n * add \"ignore extra bitmap file\" in trace2 debugging output\n * modify case \"t5310\"\n\nThanks.\n\nTeng Long (1):\n pack-bitmap.c: avoid exposing absolute paths\n\n pack-bitmap.c | 12 ++++++++----\n t/t5310-pack-bitmaps.sh | 11 ++++++++---\n 2 files changed, 16 insertions(+), 7 deletions(-)\n\nRange-diff against v1:\n1: 1591e7ee52e < -: ----------- pack-bitmap.c: avoid exposing absolute paths\n-: ----------- > 1: 87a494e5ac0 pack-bitmap.c: avoid exposing absolute paths\n-- \n2.37.2.1.g87a494e5ac0\n\n"},{"id":"466309","messageId":"87a494e5ac0cc992689944ab13600d097c51e54a.1667393419.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"cover.1667393419.git.dyroneteng@gmail.com","subject":"[PATCH v2 1/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-02T12:56:05Z","receivedAt":"2022-11-02T12:56:22Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nIn \"open_midx_bitmap_1()\" and \"open_pack_bitmap_1()\", when we find that\nthere are multiple bitmaps, we will only open the first one and then\nleave warnings about the remaining pack information, the information\nwill contain the absolute path of the repository, for example in a\nalternates usage scenario. So let's hide this kind of potentially\nsensitive information in this commit.\n\nFound-by: XingXin \nSigned-off-by: Teng Long \n---\n pack-bitmap.c | 12 ++++++++----\n t/t5310-pack-bitmaps.sh | 11 ++++++++---\n 2 files changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 36134222d7a..a8c76056637 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -331,8 +331,9 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n \tif (bitmap_git->pack || bitmap_git->midx) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tget_midx_filename(&buf, midx->object_dir);\n-\t\t/* ignore extra bitmap file; we can only handle one */\n-\t\twarning(\"ignoring extra bitmap file: %s\", buf.buf);\n+\t\t/* ignore extra midx bitmap files; we can only handle one */\n+\t\ttrace2_data_string(\"bitmap\", the_repository,\n+\t\t\t\t \"ignoring extra midx bitmap file\", basename(buf.buf));\n \t\tclose(fd);\n \t\tstrbuf_release(&buf);\n \t\treturn -1;\n@@ -402,8 +403,9 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \t}\n \n \tif (bitmap_git->pack || bitmap_git->midx) {\n-\t\t/* ignore extra bitmap file; we can only handle one */\n-\t\twarning(\"ignoring extra bitmap file: %s\", packfile->pack_name);\n+\t\t/* ignore extra bitmap files; we can only handle one */\n+\t\ttrace2_data_string(\"bitmap\", the_repository,\n+\t\t\t\t \"ignoring extra bitmap file\", basename(packfile->pack_name));\n \t\tclose(fd);\n \t\treturn -1;\n \t}\n@@ -428,6 +430,8 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \t\treturn -1;\n \t}\n \n+\ttrace2_data_string(\"bitmap\", the_repository, \"opened bitmap file\",\n+\t\t\t basename(packfile->pack_name));\n \treturn 0;\n }\n \ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex f775fc1ce69..614586b0181 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -397,7 +397,7 @@ test_expect_success 'pack.preferBitmapTips' '\n \t)\n '\n \n-test_expect_success 'complains about multiple pack bitmaps' '\n+test_expect_success 'complains about multiple pack bitmaps when debugging by trace2' '\n \trm -fr repo &&\n \tgit init repo &&\n \ttest_when_finished \"rm -fr repo\" &&\n@@ -420,8 +420,13 @@ test_expect_success 'complains about multiple pack bitmaps' '\n \t\ttest_line_count = 2 packs &&\n \t\ttest_line_count = 2 bitmaps &&\n \n-\t\tgit rev-list --use-bitmap-index HEAD 2>err &&\n-\t\tgrep \"ignoring extra bitmap file\" err\n+\t\tls -tr .git/objects/pack | grep -e \"^pack-.*\\.pack$\" > sorted-packs &&\n+\t\tignored_pack=\"$(cat sorted-packs | awk 'NR==1{print}')\" &&\n+\t\topen_pack=\"$(cat sorted-packs | awk 'NR==2{print}')\" &&\n+\n+\t\tGIT_TRACE2_PERF=1 git rev-list --use-bitmap-index HEAD 2>err &&\n+\t\tgrep \"opened bitmap file:$opened_pack\" err &&\n+\t\tgrep \"ignoring extra bitmap file:$ignored_pack\" err\n \t)\n '\n \n-- \n2.37.2.1.g87a494e5ac0\n\n"},{"id":"466310","messageId":"20221102130405.91866-1-tenglong.tl@alibaba-inc.com","threadId":"58362","inReplyTo":"221102.86tu3h3e1w.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-02T13:04:05Z","receivedAt":"2022-11-02T13:04:20Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"\"Ævar Arnfjörð Bjarmason\" writes：\n\n> I see that downthread of here there's discussion about keeping the\n> warning, adding tracing, etc. etc.\n>\n> Maybe it's been brought up (I was skimming, sorry), but for the problem\n> you have isn't a narrow and acceptable solution to you to keep the\n> warning, but just don't print the absolute path?\n>\n> I.e.:\n>\n> \twarning: ignoring extra bitmap file: /Users/tenglong.tl/Downloads/alternate_repo.git/.git/objects/pack/pack-bff67e2a7a154e6933afe61b3681726cf9bd8e8b.pack\n>\n> To:\n>\n> \twarning: ignoring extra bitmap file: ../alternate_repo.git/.git/objects/pack/pack-bff67e2a7a154e6933afe61b3681726cf9bd8e8b.pack\n>\n> Or would the relative path to the alternate also be sensitive?\n>\n> We might also want to just remove this etc., but that's a different\n> question than \"should we print these absolute paths?\".\n\nOops, sorry for that mistake, and I just sent the patch v2, and the trace2\noutput might like:\n\n19:33:24.360684 common-main.c:49 | d0 | main | version | | | | | 2.37.2.1.g520f5131b7c.dirty\n19:33:24.360963 common-main.c:50 | d0 | main | start | | 0.001820 | | | /usr/local/bin/git rev-list --test-bitmap HEAD\n19:33:24.361487 git.c:461 | d0 | main | cmd_name | | | | | rev-list (rev-list)\n19:33:24.364617 pack-bitmap.c:435 | d0 | main | data | r0 | 0.005497 | 0.005497 | bitmap | opened bitmap file:pack-c9fe9d2dc5d002d4a4b622626ffa282bcbccb7ee.pack\n19:33:24.365635 pack-bitmap.c:409 | d0 | main | data | r0 | 0.006518 | 0.006518 | bitmap | ignoring extra bitmap file:pack-3cea516b416961285fd8f519e12102b19bcf257e.pack\nBitmap v1 test (2 entries loaded)\nFound bitmap for 2c5959955b5e6167181d08eeb30ee4099b4a4c5b. 64 bits / ca44d5df checksum\n19:33:24.365970 progress.c:268 | d0 | main | region_enter | r0 | 0.006853 | | progress | label:Verifying bitmap entries\nVerifying bitmap entries: 100% (6/6), done.\n19:33:24.366479 progress.c:340 | d0 | main | data | r0 | 0.007362 | 0.000509 | progress | ..total_objects:6\n19:33:24.366493 progress.c:346 | d0 | main | region_leave | r0 | 0.007377 | 0.000524 | progress | label:Verifying bitmap entries\nOK!\n19:33:24.366558 wrapper.c:621 | d0 | main | data | r0 | 0.007442 | 0.007442 | fsync | fsync/writeout-only:0\n19:33:24.366572 wrapper.c:622 | d0 | main | data | r0 | 0.007456 | 0.007456 | fsync | fsync/hardware-flush:0\n19:33:24.366578 git.c:720 | d0 | main | exit | | 0.007463 | | | code:0\n19:33:24.366592 trace2/tr2_tgt_perf.c:215 | d0 | main | atexit | | 0.007476 | | | code:0\n\nIn short, the \"related path\" will not appear in output now but the \"basename\"\n(packname) instead. I think maybe it will help the experts like Taylor who\noften hacking the bitmap code.\n\nThanks.\n"},{"id":"466311","messageId":"20221102135245.97998-1-tenglong.tl@alibaba-inc.com","threadId":"58362","inReplyTo":"Y2IiSU1L+bJPUioV@coredump.intra.peff.net","subject":"Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-02T13:52:45Z","receivedAt":"2022-11-02T13:53:00Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Jeff King writes:\n\n> I mean that later in the process, if we need to find an object we may\n> open the .idx file to look for it. So by opening them all up front, we\n> _might_ just be doing work that would get done later.\n>\n> But it's not guaranteed. Imagine you have 10,000 small packs, and one\n> big bitmapped pack. If you can serve the request from just the big pack,\n> then you'd never need to open those other .idx files at all. However,\n> the current code will open them anyway.\n>\n> I care less about mmap space, and more that it's work (syscalls, and\n> examining the contents of the idx) to open each one. It's probably not\n> even measurable unless you have a ton of packs, though.\n>\n> > > So it may not be worth worrying about. It does seem like it would be\n> > > easy to reorder open_pack_bitmap_1() to look for a bitmap file first and\n> > > only open the idx if it finds something.\n> >\n> > I think it may be worthy if we have lots of packs and the bitmap is refer to\n> > an older one, but I didn't make the test. At least, the scenario is common, I\n> > agree with that, so maybe we could shuffle the sort order in \"open_pack_bitmap()\".\n>\n> I don't mean the order in which we look at packs. I mean the order of\n> operations in open_pack_bitmap_1(), something like:\n\nThank you for the explanation. Make sense.\n\nI run a test under a repo with 3 packs and without bitmaps,it seems like now\nwill open every idx and failed at last:\n\n➜ pack git:(master) git rev-list --test-bitmap HEAD\npack: /Users/tenglong.tl/Downloads/trace-test/.git/objects/pack/pack-c9fe9d2dc5d002d4a4b622626ffa282bcbccb7ee.pack\npack: /Users/tenglong.tl/Downloads/trace-test/.git/objects/pack/pack-08841c0c4c1fd176c354bdbd25c5a1b152ea95d0.pack\npack: /Users/tenglong.tl/Downloads/trace-test/.git/objects/pack/pack-3cea516b416961285fd8f519e12102b19bcf257e.pack\nfatal: failed to load bitmap indexes\n\nSo we're now looping for packs first, then try to find the corresponded bitmap\nof it. In that case, why can't we start the search from the bitmap files at\nfirst? If this is possible, when we found the first bitmap file or an\nappropriate one under some mechanism (biggest or newest maybe？I'm not deep into\nit right now) then break the loop and open it.\n\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index 440407f1be..1df2f6c8b6 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -411,9 +411,6 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n> \tstruct stat st;\n> \tchar *bitmap_name;\n>\n> -\tif (open_pack_index(packfile))\n> -\t\treturn -1;\n> -\n> \tbitmap_name = pack_bitmap_filename(packfile);\n> \tfd = git_open(bitmap_name);\n>\n> @@ -438,6 +435,10 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n> \t\treturn -1;\n> \t}\n>\n> +\t/* now we know we have a plausible bitmap; make sure the idx is OK, too */\n> +\tif (open_pack_index(packfile))\n> +\t\treturn -1;\n> +\n> \tif (!is_pack_valid(packfile)) {\n> \t\tclose(fd);\n> \t\treturn -1;\n>\n> But we can further observe that the first thing is_pack_valid() will do\n> is open the idx file. :) So we can really just drop this line entirely,\n> I'd think.\n\nI agree that and I think it could append to patch v3, maybe.\n\n> BTW, another oddity I noticed in this function. We check:\n>\n> if (bitmap_git->pack || bitmap_git->midx) {\n> \t /* ignore extra bitmap file; we can only handle one */\n> \t ...\n> }\n>\n> but it's impossible for bitmap_git->midx to be set here. If we opened\n> the midx bitmap, we'll skip calling open_pack_bitmap() entirely.\n\nOh, I remember that and it's mentioned in another patchset at Tue, 29 Mar 2022:\n\nhttps://public-inbox.org/git/20220329024949.62091-1-dyroneteng@gmail.com/\n\nI agree with Taylor with\nhttps://public-inbox.org/git/YkPGq0mDL4NG6D1o@nand.local/\n\nBut I'm ok if you think it should be solved.\n\nThank you very much for your help.\n"},{"id":"466367","messageId":"Y2MSkttIGVVVInKy@nand.local","threadId":"58362","inReplyTo":"20221031131356.67007-1-dyroneteng@gmail.com","subject":"Re: [PATCH 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-03T01:00:02Z","receivedAt":"2022-11-03T01:00:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 31, 2022 at 09:13:56PM +0800, Teng Long wrote:\n> Taylor Blau writes:\n>\n> > --- >8 ---\n>\n> What does \">8\" means?\n>\n> It's like a Github internal patch between the \">8\"\n> and \"<8\" marks, right?\n\nThey are scissor marks. Tools like 'git am -c' will trim everything\nbefore a scissors line when applying patches.\n\nI used them to deliniate between the patch contents (which can be\napplied) and the rest of my mail here (which can't).\n\n> > For the general case of what ships in git.git, I *do* find this warning\n> > ...\n>\n> \"ships in git.git\" means as the git developers, right?\n\nKind of, but not entirely. I meant \"what is in Junio's tree\": so, what\ngets bundled by package maintainer, as well as the source that Git\ndeveloper build from.\n\nThanks,\nTaylor\n"},{"id":"466369","messageId":"Y2MWeE2f/P1iXPCY@nand.local","threadId":"58362","inReplyTo":"87a494e5ac0cc992689944ab13600d097c51e54a.1667393419.git.dyroneteng@gmail.com","subject":"Re: [PATCH v2 1/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-03T01:16:40Z","receivedAt":"2022-11-03T01:16:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 02, 2022 at 08:56:05PM +0800, Teng Long wrote:\n> From: Teng Long \n>\n> In \"open_midx_bitmap_1()\" and \"open_pack_bitmap_1()\", when we find that\n> there are multiple bitmaps, we will only open the first one and then\n> leave warnings about the remaining pack information, the information\n> will contain the absolute path of the repository, for example in a\n> alternates usage scenario. So let's hide this kind of potentially\n> sensitive information in this commit.\n>\n> Found-by: XingXin \n> Signed-off-by: Teng Long \n> ---\n> pack-bitmap.c | 12 ++++++++----\n> t/t5310-pack-bitmaps.sh | 11 ++++++++---\n> 2 files changed, 16 insertions(+), 7 deletions(-)\n>\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index 36134222d7a..a8c76056637 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -331,8 +331,9 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n> \tif (bitmap_git->pack || bitmap_git->midx) {\n> \t\tstruct strbuf buf = STRBUF_INIT;\n> \t\tget_midx_filename(&buf, midx->object_dir);\n> -\t\t/* ignore extra bitmap file; we can only handle one */\n> -\t\twarning(\"ignoring extra bitmap file: %s\", buf.buf);\n\nOK... here we're getting rid of the user-visible warning, which makes\nsense and is the point of this patch.\n\n> +\t\t/* ignore extra midx bitmap files; we can only handle one */\n> +\t\ttrace2_data_string(\"bitmap\", the_repository,\n> +\t\t\t\t \"ignoring extra midx bitmap file\", basename(buf.buf));\n\nBut we replace it with a trace2_data_string() that only includes the\nbasename? I'd think that the point of moving this warning into\ntrace2-land is that we could emit the full path without worrying about\nwho sees it, since trace2 data is typically not plumbed through to\nend-users.\n\nIOW, I would have expected to see a patch something along the lines of:\n\n> -\t\t/* ignore extra bitmap file; we can only handle one */\n> -\t\twarning(\"ignoring extra bitmap file: %s\", buf.buf);\n> +\t\t/* ignore extra midx bitmap files; we can only handle one */\n> +\t\ttrace2_data_string(\"bitmap\", the_repository,\n> +\t\t\t\t \"ignoring extra midx bitmap file\", buf.buf);\n\n> @@ -402,8 +403,9 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n> \t}\n>\n> \tif (bitmap_git->pack || bitmap_git->midx) {\n> -\t\t/* ignore extra bitmap file; we can only handle one */\n> -\t\twarning(\"ignoring extra bitmap file: %s\", packfile->pack_name);\n> +\t\t/* ignore extra bitmap files; we can only handle one */\n> +\t\ttrace2_data_string(\"bitmap\", the_repository,\n> +\t\t\t\t \"ignoring extra bitmap file\", basename(packfile->pack_name));\n\nSame note here.\n\n> +\ttrace2_data_string(\"bitmap\", the_repository, \"opened bitmap file\",\n> +\t\t\t basename(packfile->pack_name));\n\nIf we get later bitmap-related information in the trace2 stream, we know\nthat we opened a bitmap. And at the moment we read a bitmap, there is\nonly one such bitmap in the repository.\n\nI suppose that this is race-proof in the sense that if the bitmaps\nchange after reading, we can still usefully debug the trace2 stream\nafter the fact.\n\nSo even though my first reaction was that this was unnecessary, on\nbalance I do find it useful.\n\n> -test_expect_success 'complains about multiple pack bitmaps' '\n> +test_expect_success 'complains about multiple pack bitmaps when debugging by trace2' '\n> \trm -fr repo &&\n> \tgit init repo &&\n> \ttest_when_finished \"rm -fr repo\" &&\n> @@ -420,8 +420,13 @@ test_expect_success 'complains about multiple pack bitmaps' '\n> \t\ttest_line_count = 2 packs &&\n> \t\ttest_line_count = 2 bitmaps &&\n>\n> -\t\tgit rev-list --use-bitmap-index HEAD 2>err &&\n> -\t\tgrep \"ignoring extra bitmap file\" err\n> +\t\tls -tr .git/objects/pack | grep -e \"^pack-.*\\.pack$\" > sorted-packs &&\n> +\t\tignored_pack=\"$(cat sorted-packs | awk 'NR==1{print}')\" &&\n> +\t\topen_pack=\"$(cat sorted-packs | awk 'NR==2{print}')\" &&\n\nHmmph. This test only passes if 'ls -tr' gives us the packs in order\nthat they are read by readdir(), which doesn't seem all that portable to\nme. At the very least, it is tightly coupled to the implementation of\nopen_pack_bitmap() and friends.\n\n> +\t\tGIT_TRACE2_PERF=1 git rev-list --use-bitmap-index HEAD 2>err &&\n> +\t\tgrep \"opened bitmap file:$opened_pack\" err &&\n> +\t\tgrep \"ignoring extra bitmap file:$ignored_pack\" err\n\nDo we necessarily care about which .bitmap is read and which isn't? The\nexisting test doesn't look too closely, which I think is fine (though as\nthe author of that test, I might be biased ;-)).\n\nI would be equally happy to write:\n\n> +\t\tGIT_TRACE2_PERF=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&\n> +\t\tgrep \"opened bitmap\" trace2.txt &&\n> +\t\tgrep \"ignoring extra bitmap\" trace2.txt\n\nThanks,\nTaylor\n"},{"id":"466370","messageId":"Y2MXgVZx6rqGkMzM@nand.local","threadId":"58362","inReplyTo":"cover.1667393419.git.dyroneteng@gmail.com","subject":"Re: [PATCH v2 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-03T01:21:05Z","receivedAt":"2022-11-03T01:21:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 02, 2022 at 08:56:04PM +0800, Teng Long wrote:\n> pack-bitmap.c | 12 ++++++++----\n> t/t5310-pack-bitmaps.sh | 11 ++++++++---\n> 2 files changed, 16 insertions(+), 7 deletions(-)\n\nThis is based on a somewhat out-of-date version of the pack-bitmap.c\ncode which has changed quite a bit since Abhradeep Chakraborty worked on\nit over the summer.\n\nResolving the merge conflicts during am wasn't too hard, and my rerere\ncache should save me from having to do it again. But if you don't mind\nrebasing on the tip of 'master' before sending the next round, that\nwould be appreciated.\n\nThanks.\n\nTaylor\n"},{"id":"466380","messageId":"20221103084202.36131-1-tenglong.tl@alibaba-inc.com","threadId":"58362","inReplyTo":"Y2MXgVZx6rqGkMzM@nand.local","subject":"Re: [PATCH v2 0/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-03T08:42:02Z","receivedAt":"2022-11-03T08:42:13Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Taylor Blau writes:\n\n> This is based on a somewhat out-of-date version of the pack-bitmap.c\n> code which has changed quite a bit since Abhradeep Chakraborty worked on\n> it over the summer.\n>\n> Resolving the merge conflicts during am wasn't too hard, and my rerere\n> cache should save me from having to do it again. But if you don't mind\n> rebasing on the tip of 'master' before sending the next round, that\n> would be appreciated.\n\nSorry for that, I will do rebase on 'master' before sending v3.\n\nThanks.\n"},{"id":"466388","messageId":"20221103093532.40511-1-tenglong.tl@alibaba-inc.com","threadId":"58362","inReplyTo":"Y2MWeE2f/P1iXPCY@nand.local","subject":"Re: [PATCH v2 1/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-03T09:35:32Z","receivedAt":"2022-11-03T09:35:43Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Taylor Blau writes:\n\n> OK... here we're getting rid of the user-visible warning, which makes\n> sense and is the point of this patch.\n\nYes, my point is to avoid exposing some sensitive informations to end- users.\n\n> But we replace it with a trace2_data_string() that only includes the\n> basename? I'd think that the point of moving this warning into\n> trace2-land is that we could emit the full path without worrying about\n> who sees it, since trace2 data is typically not plumbed through to\n> end-users.\n\nI'm not sure if trace2 data will be given to end-users, at least as I understand\nit as you, it's not. If so, your opinion is very reasonable.\n\nSo... maybe we could add a new configuration like \"core.hideSensitive\", and\nthere are some supported values , \"loose\", \"normal \" and \"strict\":\n\n loose: plumbing full information in trace2, even warning.\n normal: plumbing full information in trace2, but not warning.\n strict: plumbing part of information in trace2, but not warning\n\nBut it looks like heavy, maybe not worthy... So, currently I will remove\nbasename and print the pack with path if there are no new inputs here.\n\nThanks.\n\n> If we get later bitmap-related information in the trace2 stream, we know\n> that we opened a bitmap. And at the moment we read a bitmap, there is\n> only one such bitmap in the repository.\n>\n> I suppose that this is race-proof in the sense that if the bitmaps\n> change after reading, we can still usefully debug the trace2 stream\n> after the fact.\n>\n> So even though my first reaction was that this was unnecessary, on\n> balance I do find it useful.\n\nYes... I think it's useful as least for me to do some bitmap tests and\nI think print a bit more related information in trace2 data might be OK.\n\n> > -test_expect_success 'complains about multiple pack bitmaps' '\n> > +test_expect_success 'complains about multiple pack bitmaps when debugging by trace2' '\n> > \trm -fr repo &&\n> > \tgit init repo &&\n> > \ttest_when_finished \"rm -fr repo\" &&\n> > @@ -420,8 +420,13 @@ test_expect_success 'complains about multiple pack bitmaps' '\n> > \t\ttest_line_count = 2 packs &&\n> > \t\ttest_line_count = 2 bitmaps &&\n> >\n> > -\t\tgit rev-list --use-bitmap-index HEAD 2>err &&\n> > -\t\tgrep \"ignoring extra bitmap file\" err\n> > +\t\tls -tr .git/objects/pack | grep -e \"^pack-.*\\.pack$\" > sorted-packs &&\n> > +\t\tignored_pack=\"$(cat sorted-packs | awk 'NR==1{print}')\" &&\n> > +\t\topen_pack=\"$(cat sorted-packs | awk 'NR==2{print}')\" &&\n>\n> Hmmph. This test only passes if 'ls -tr' gives us the packs in order\n> that they are read by readdir(), which doesn't seem all that portable to\n> me. At the very least, it is tightly coupled to the implementation of\n> open_pack_bitmap() and friends.\n\nYes, because the _order_ matters here I think originally. May you explain a\nlittle more about _portable_ problem please?\n\n> Do we necessarily care about which .bitmap is read and which isn't? The\n> existing test doesn't look too closely, which I think is fine (though as\n> the author of that test, I might be biased ;-)).\n>\n> I would be equally happy to write:\n>\n> > +\t\tGIT_TRACE2_PERF=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&\n> > +\t\tgrep \"opened bitmap\" trace2.txt &&\n> > +\t\tgrep \"ignoring extra bitmap\" trace2.txt\n\nOrz. Actually, I wrote it on the same way, but I think it maybe not so\nsufficient for my patch. So...\n\nBut I think you are right afterI look other test cases, will fix.\n\nThanks.\n"},{"id":"466479","messageId":"cover.1667470481.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"cover.1667393419.git.dyroneteng@gmail.com","subject":"[PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-04T03:17:08Z","receivedAt":"2022-11-04T03:19:37Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nDiff since v2:\n\n1. trace2 log will include the path info of pack. not only \"basename\".\n2. A new commit which removes unnecessary \"open_pack_index()\" calls in\n\"open_midx_bitmap_1()\".\n3. The patch v2 still base on 2.37, rebase on 'master' to prevent conflicts.\n\nThanks.\n\nTeng Long (2):\n pack-bitmap.c: avoid exposing absolute paths\n pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls\n\n pack-bitmap.c | 15 ++++++++-------\n t/t5310-pack-bitmaps.sh | 5 +++--\n 2 files changed, 11 insertions(+), 9 deletions(-)\n\nRange-diff against v2:\n1: 1591e7ee52 < -: ---------- pack-bitmap.c: avoid exposing absolute paths\n-: ---------- > 1: de941f58f9 pack-bitmap.c: avoid exposing absolute paths\n-: ---------- > 2: 7ac9b859f3 pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls\n-- \n2.38.1.383.g35840fde1dd\n\n"},{"id":"466480","messageId":"de941f58f9eb7d0287fa1f7a5ffd343a22e5e46c.1667470481.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"cover.1667470481.git.dyroneteng@gmail.com","subject":"[PATCH v3 1/2] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-04T03:17:09Z","receivedAt":"2022-11-04T03:19:51Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nIn \"open_midx_bitmap_1()\" and \"open_pack_bitmap_1()\", when we find that\nthere are multiple bitmaps, we will only open the first one and then\nleave warnings about the remaining pack information, the information\nwill contain the absolute path of the repository, for example in a\nalternates usage scenario. So let's hide this kind of potentially\nsensitive information in this commit.\n\nFound-by: XingXin \nSigned-off-by: Teng Long \n---\n pack-bitmap.c | 12 ++++++++----\n t/t5310-pack-bitmaps.sh | 5 +++--\n 2 files changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 440407f1be..9443b7adca 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -354,8 +354,9 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n \tif (bitmap_git->pack || bitmap_git->midx) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tget_midx_filename(&buf, midx->object_dir);\n-\t\t/* ignore extra bitmap file; we can only handle one */\n-\t\twarning(_(\"ignoring extra bitmap file: '%s'\"), buf.buf);\n+\t\t/* ignore extra midx bitmap files; we can only handle one */\n+\t\ttrace2_data_string(\"bitmap\", the_repository,\n+\t\t\t\t \"ignoring extra midx bitmap file\", buf.buf);\n \t\tclose(fd);\n \t\tstrbuf_release(&buf);\n \t\treturn -1;\n@@ -432,8 +433,9 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \t}\n \n \tif (bitmap_git->pack || bitmap_git->midx) {\n-\t\t/* ignore extra bitmap file; we can only handle one */\n-\t\twarning(_(\"ignoring extra bitmap file: '%s'\"), packfile->pack_name);\n+\t\t/* ignore extra bitmap files; we can only handle one */\n+\t\ttrace2_data_string(\"bitmap\", the_repository,\n+\t\t\t\t \"ignoring extra bitmap file\", packfile->pack_name);\n \t\tclose(fd);\n \t\treturn -1;\n \t}\n@@ -458,6 +460,8 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \t\treturn -1;\n \t}\n \n+\ttrace2_data_string(\"bitmap\", the_repository, \"opened bitmap file\",\n+\t\t\t packfile->pack_name);\n \treturn 0;\n }\n \ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 6d693eef82..0b422c8a63 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -428,8 +428,9 @@ test_bitmap_cases () {\n \t\t\ttest_line_count = 2 packs &&\n \t\t\ttest_line_count = 2 bitmaps &&\n \n-\t\t\tgit rev-list --use-bitmap-index HEAD 2>err &&\n-\t\t\tgrep \"ignoring extra bitmap file\" err\n+\t\t\tGIT_TRACE2_PERF=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&\n+\t\t\tgrep \"opened bitmap\" trace2.txt &&\n+\t\t\tgrep \"ignoring extra bitmap\" trace2.txt\n \t\t)\n \t'\n }\n-- \n2.38.1.383.g35840fde1dd\n\n"},{"id":"466481","messageId":"7ac9b859f31b2b3efb4a04896892ccd094a98734.1667470481.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"cover.1667470481.git.dyroneteng@gmail.com","subject":"[PATCH v3 2/2] pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-04T03:17:10Z","receivedAt":"2022-11-04T03:20:06Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nEverytime when calling \"open_pack_bitmap_1()\", we will firstly call\n\"open_pack_index(packfile)\" to check the index, then further check\nagain in \"is_pack_valid()\" before mmap the bitmap file. So, let's\nremove the first check here.\n\nThe relate discussion:\nhttps://public-inbox.org/git/Y2IiSU1L+bJPUioV@coredump.intra.peff.net/#t\n\nSigned-off-by: Teng Long \n---\n pack-bitmap.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 9443b7adca..503c95f0b8 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -412,9 +412,6 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \tstruct stat st;\n \tchar *bitmap_name;\n \n-\tif (open_pack_index(packfile))\n-\t\treturn -1;\n-\n \tbitmap_name = pack_bitmap_filename(packfile);\n \tfd = git_open(bitmap_name);\n \n-- \n2.38.1.383.g35840fde1dd\n\n"},{"id":"466562","messageId":"Y2WNnqOOu+1QaOPq@nand.local","threadId":"58362","inReplyTo":"7ac9b859f31b2b3efb4a04896892ccd094a98734.1667470481.git.dyroneteng@gmail.com","subject":"Re: [PATCH v3 2/2] pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-04T22:09:34Z","receivedAt":"2022-11-04T22:09:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Nov 04, 2022 at 11:17:10AM +0800, Teng Long wrote:\n> From: Teng Long \n>\n> Everytime when calling \"open_pack_bitmap_1()\", we will firstly call\n> \"open_pack_index(packfile)\" to check the index, then further check\n> again in \"is_pack_valid()\" before mmap the bitmap file. So, let's\n> remove the first check here.\n>\n> The relate discussion:\n> https://public-inbox.org/git/Y2IiSU1L+bJPUioV@coredump.intra.peff.net/#t\n\nThe related discussion is good, but I would have liked to see some of\nthe details make its way into the patch message for future readers.\n\nMaybe something like:\n\n When trying to open a pack bitmap, we call open_pack_bitmap_1() in a\n loop, during which it tries to open up the pack index corresponding\n with each available pack.\n\n It's likely that we'll end up relying on objects in that pack later\n in the process (in which case we're doing the work of opening the\n pack index optimistically), but not guaranteed.\n\n For instance, consider a repository with a large number of small\n packs, and one large pack with a bitmap. If we see that bitmap pack\n last in our loop which calls open_pack_bitmap_1(), the current code\n will have opened *all* pack index files in the repository. If the\n request can be served out of the bitmapped pack alone, then the time\n spent opening these idx files was wasted.S\n\n Since open_pack_bitmap_1() calls is_pack_valid() later on (which in\n turns calls open_pack_index() itself), we can just drop the earlier\n call altogether.\n\nThanks,\nTaylor\n"},{"id":"466563","messageId":"Y2WOLMC2381ktwgg@nand.local","threadId":"58362","inReplyTo":"de941f58f9eb7d0287fa1f7a5ffd343a22e5e46c.1667470481.git.dyroneteng@gmail.com","subject":"Re: [PATCH v3 1/2] pack-bitmap.c: avoid exposing absolute paths","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-04T22:11:56Z","receivedAt":"2022-11-04T22:12:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Nov 04, 2022 at 11:17:09AM +0800, Teng Long wrote:\n> @@ -354,8 +354,9 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n> \tif (bitmap_git->pack || bitmap_git->midx) {\n> \t\tstruct strbuf buf = STRBUF_INIT;\n> \t\tget_midx_filename(&buf, midx->object_dir);\n> -\t\t/* ignore extra bitmap file; we can only handle one */\n> -\t\twarning(_(\"ignoring extra bitmap file: '%s'\"), buf.buf);\n> +\t\t/* ignore extra midx bitmap files; we can only handle one */\n> +\t\ttrace2_data_string(\"bitmap\", the_repository,\n> +\t\t\t\t \"ignoring extra midx bitmap file\", buf.buf);\n\nThis is looking good, though I think we *could* drop the comment here.\nIt's as redundant in the new version as it was in the old (i.e., we say\nthat we ignore extra bitmap files in a comment, and then issue a warning\nwith basically the same message).\n\nBut I don't feel strongly about it, so I think this patch is fine with\nor without that comment.\n\n> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\n> index 6d693eef82..0b422c8a63 100755\n> --- a/t/t5310-pack-bitmaps.sh\n> +++ b/t/t5310-pack-bitmaps.sh\n> @@ -428,8 +428,9 @@ test_bitmap_cases () {\n> \t\t\ttest_line_count = 2 packs &&\n> \t\t\ttest_line_count = 2 bitmaps &&\n>\n> -\t\t\tgit rev-list --use-bitmap-index HEAD 2>err &&\n> -\t\t\tgrep \"ignoring extra bitmap file\" err\n> +\t\t\tGIT_TRACE2_PERF=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&\n\nDo we need to clean up an existing trace2.txt here? Looks like not, so\nOK. Is there a reason we need GIT_TRACE2_PERF and not GIT_TRACE2_EVENT\nlike the rest of the tests in t5310?\n\n> +\t\t\tgrep \"opened bitmap\" trace2.txt &&\n> +\t\t\tgrep \"ignoring extra bitmap\" trace2.txt\n\nThese two look good.\n\nThanks,\nTaylor\n"},{"id":"466564","messageId":"Y2WOeClE+E8C0hiP@nand.local","threadId":"58362","inReplyTo":"cover.1667470481.git.dyroneteng@gmail.com","subject":"Re: [PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-04T22:13:12Z","receivedAt":"2022-11-04T22:13:17Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Nov 04, 2022 at 11:17:08AM +0800, Teng Long wrote:\n> From: Teng Long \n>\n> Diff since v2:\n>\n> 1. trace2 log will include the path info of pack. not only \"basename\".\n> 2. A new commit which removes unnecessary \"open_pack_index()\" calls in\n> \"open_midx_bitmap_1()\".\n> 3. The patch v2 still base on 2.37, rebase on 'master' to prevent conflicts.\n>\n> Thanks.\n\nThis version is looking pretty good, and I very much appreciate the\nrebase onto 'master' in the meantime.\n\nI left a couple of notes throughout which I think merit a small reroll.\nOtherwise, this is looking just about ready to go.\n\n\nThanks,\nTaylor\n"},{"id":"466570","messageId":"Y2Wv4b7y0KS8KYHy@nand.local","threadId":"58362","inReplyTo":"20221103093532.40511-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH v2 1/1] pack-bitmap.c: avoid exposing absolute paths","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-05T00:35:45Z","receivedAt":"2022-11-05T00:40:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 03, 2022 at 05:35:32PM +0800, Teng Long wrote:\n> I'm not sure if trace2 data will be given to end-users, at least as I understand\n> it as you, it's not. If so, your opinion is very reasonable.\n>\n> So... maybe we could add a new configuration like \"core.hideSensitive\", and\n> there are some supported values , \"loose\", \"normal \" and \"strict\":\n>\n> loose: plumbing full information in trace2, even warning.\n> normal: plumbing full information in trace2, but not warning.\n> strict: plumbing part of information in trace2, but not warning\n>\n> But it looks like heavy, maybe not worthy... So, currently I will remove\n> basename and print the pack with path if there are no new inputs here.\n\ntrace2 data isn't sent to users. So having read this after I took a look\nat the updated round, I am glad that you pursued the approach that you\ndid :-).\n\n> > > -test_expect_success 'complains about multiple pack bitmaps' '\n> > > +test_expect_success 'complains about multiple pack bitmaps when debugging by trace2' '\n> > > \trm -fr repo &&\n> > > \tgit init repo &&\n> > > \ttest_when_finished \"rm -fr repo\" &&\n> > > @@ -420,8 +420,13 @@ test_expect_success 'complains about multiple pack bitmaps' '\n> > > \t\ttest_line_count = 2 packs &&\n> > > \t\ttest_line_count = 2 bitmaps &&\n> > >\n> > > -\t\tgit rev-list --use-bitmap-index HEAD 2>err &&\n> > > -\t\tgrep \"ignoring extra bitmap file\" err\n> > > +\t\tls -tr .git/objects/pack | grep -e \"^pack-.*\\.pack$\" > sorted-packs &&\n> > > +\t\tignored_pack=\"$(cat sorted-packs | awk 'NR==1{print}')\" &&\n> > > +\t\topen_pack=\"$(cat sorted-packs | awk 'NR==2{print}')\" &&\n> >\n> > Hmmph. This test only passes if 'ls -tr' gives us the packs in order\n> > that they are read by readdir(), which doesn't seem all that portable to\n> > me. At the very least, it is tightly coupled to the implementation of\n> > open_pack_bitmap() and friends.\n>\n> Yes, because the _order_ matters here I think originally. May you explain a\n> little more about _portable_ problem please?\n\nWe're depending on the loop in open_pack_bitmap() seeing packs in an\norder compatible with the output of 'ls -tr' here. In other words, for\nthis test to pass, we care very much that the pack we collected as\n\"$ignored_pack\" is seen *after* the pack that we designate as\n\"$open_pack\".\n\nDoes that help?\n\nThanks,\nTaylor\n"},{"id":"467035","messageId":"cover.1668063122.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"cover.1667470481.git.dyroneteng@gmail.com","subject":"[PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-10T07:10:10Z","receivedAt":"2022-11-10T07:10:26Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nDiff since v2:\n\n* remove unnecessary comments.\n* use \"GIT_TRACE2_EVENT\" instead of \"GIT_TRACE_PERF\".\n* improve commit message of [1/2].\n\nThanks.\n\nTeng Long (2):\n pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls\n pack-bitmap.c: avoid exposing absolute paths\n\n pack-bitmap.c | 13 ++++++-------\n t/t5310-pack-bitmaps.sh | 5 +++--\n 2 files changed, 9 insertions(+), 9 deletions(-)\n\nRange-diff against v2:\n2: 7ac9b859f3 ! 1: aaeb021538 pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls\n @@ Metadata\n ## Commit message ##\n pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls\n \n - Everytime when calling \"open_pack_bitmap_1()\", we will firstly call\n - \"open_pack_index(packfile)\" to check the index, then further check\n - again in \"is_pack_valid()\" before mmap the bitmap file. So, let's\n - remove the first check here.\n + When trying to open a pack bitmap, we call open_pack_bitmap_1() in a\n + loop, during which it tries to open up the pack index corresponding\n + with each available pack.\n \n - The relate discussion:\n - https://public-inbox.org/git/Y2IiSU1L+bJPUioV@coredump.intra.peff.net/#t\n + It's likely that we'll end up relying on objects in that pack later\n + in the process (in which case we're doing the work of opening the\n + pack index optimistically), but not guaranteed.\n +\n + For instance, consider a repository with a large number of small\n + packs, and one large pack with a bitmap. If we see that bitmap pack\n + last in our loop which calls open_pack_bitmap_1(), the current code\n + will have opened *all* pack index files in the repository. If the\n + request can be served out of the bitmapped pack alone, then the time\n + spent opening these idx files was wasted.S\n +\n + Since open_pack_bitmap_1() calls is_pack_valid() later on (which in\n + turns calls open_pack_index() itself), we can just drop the earlier\n + call altogether.\n \n Signed-off-by: Teng Long \n \n1: de941f58f9 ! 2: 9d5a491887 pack-bitmap.c: avoid exposing absolute paths\n @@ pack-bitmap.c: static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n \t\tget_midx_filename(&buf, midx->object_dir);\n -\t\t/* ignore extra bitmap file; we can only handle one */\n -\t\twarning(_(\"ignoring extra bitmap file: '%s'\"), buf.buf);\n -+\t\t/* ignore extra midx bitmap files; we can only handle one */\n +\t\ttrace2_data_string(\"bitmap\", the_repository,\n +\t\t\t\t \"ignoring extra midx bitmap file\", buf.buf);\n \t\tclose(fd);\n @@ pack-bitmap.c: static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, st\n \tif (bitmap_git->pack || bitmap_git->midx) {\n -\t\t/* ignore extra bitmap file; we can only handle one */\n -\t\twarning(_(\"ignoring extra bitmap file: '%s'\"), packfile->pack_name);\n -+\t\t/* ignore extra bitmap files; we can only handle one */\n +\t\ttrace2_data_string(\"bitmap\", the_repository,\n +\t\t\t\t \"ignoring extra bitmap file\", packfile->pack_name);\n \t\tclose(fd);\n @@ t/t5310-pack-bitmaps.sh: test_bitmap_cases () {\n \n -\t\t\tgit rev-list --use-bitmap-index HEAD 2>err &&\n -\t\t\tgrep \"ignoring extra bitmap file\" err\n -+\t\t\tGIT_TRACE2_PERF=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&\n ++\t\t\tGIT_TRACE2_EVENT=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&\n +\t\t\tgrep \"opened bitmap\" trace2.txt &&\n +\t\t\tgrep \"ignoring extra bitmap\" trace2.txt\n \t\t)\n-- \n2.38.1.383.g7ac9b859f31.dirty\n\n"},{"id":"467036","messageId":"aaeb021538cdfeb83dc6004fe7b3ac35a23aef49.1668063122.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"cover.1668063122.git.dyroneteng@gmail.com","subject":"[PATCH v3 1/2] pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-10T07:10:11Z","receivedAt":"2022-11-10T07:10:30Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nWhen trying to open a pack bitmap, we call open_pack_bitmap_1() in a\nloop, during which it tries to open up the pack index corresponding\nwith each available pack.\n\nIt's likely that we'll end up relying on objects in that pack later\nin the process (in which case we're doing the work of opening the\npack index optimistically), but not guaranteed.\n\nFor instance, consider a repository with a large number of small\npacks, and one large pack with a bitmap. If we see that bitmap pack\nlast in our loop which calls open_pack_bitmap_1(), the current code\nwill have opened *all* pack index files in the repository. If the\nrequest can be served out of the bitmapped pack alone, then the time\nspent opening these idx files was wasted.S\n\nSince open_pack_bitmap_1() calls is_pack_valid() later on (which in\nturns calls open_pack_index() itself), we can just drop the earlier\ncall altogether.\n\nSigned-off-by: Teng Long \n---\n pack-bitmap.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 440407f1be..982e286bac 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -411,9 +411,6 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \tstruct stat st;\n \tchar *bitmap_name;\n \n-\tif (open_pack_index(packfile))\n-\t\treturn -1;\n-\n \tbitmap_name = pack_bitmap_filename(packfile);\n \tfd = git_open(bitmap_name);\n \n-- \n2.38.1.383.g7ac9b859f31.dirty\n\n"},{"id":"467037","messageId":"9d5a491887b57bbcc30010aa7efb63e316c6d190.1668063122.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"cover.1668063122.git.dyroneteng@gmail.com","subject":"[PATCH v3 2/2] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-10T07:10:12Z","receivedAt":"2022-11-10T07:10:32Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nIn \"open_midx_bitmap_1()\" and \"open_pack_bitmap_1()\", when we find that\nthere are multiple bitmaps, we will only open the first one and then\nleave warnings about the remaining pack information, the information\nwill contain the absolute path of the repository, for example in a\nalternates usage scenario. So let's hide this kind of potentially\nsensitive information in this commit.\n\nFound-by: XingXin \nSigned-off-by: Teng Long \n---\n pack-bitmap.c | 10 ++++++----\n t/t5310-pack-bitmaps.sh | 5 +++--\n 2 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 982e286bac..aaa2d9a104 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -354,8 +354,8 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n \tif (bitmap_git->pack || bitmap_git->midx) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tget_midx_filename(&buf, midx->object_dir);\n-\t\t/* ignore extra bitmap file; we can only handle one */\n-\t\twarning(_(\"ignoring extra bitmap file: '%s'\"), buf.buf);\n+\t\ttrace2_data_string(\"bitmap\", the_repository,\n+\t\t\t\t \"ignoring extra midx bitmap file\", buf.buf);\n \t\tclose(fd);\n \t\tstrbuf_release(&buf);\n \t\treturn -1;\n@@ -429,8 +429,8 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \t}\n \n \tif (bitmap_git->pack || bitmap_git->midx) {\n-\t\t/* ignore extra bitmap file; we can only handle one */\n-\t\twarning(_(\"ignoring extra bitmap file: '%s'\"), packfile->pack_name);\n+\t\ttrace2_data_string(\"bitmap\", the_repository,\n+\t\t\t\t \"ignoring extra bitmap file\", packfile->pack_name);\n \t\tclose(fd);\n \t\treturn -1;\n \t}\n@@ -455,6 +455,8 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \t\treturn -1;\n \t}\n \n+\ttrace2_data_string(\"bitmap\", the_repository, \"opened bitmap file\",\n+\t\t\t packfile->pack_name);\n \treturn 0;\n }\n \ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 6d693eef82..7d8dee41b0 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -428,8 +428,9 @@ test_bitmap_cases () {\n \t\t\ttest_line_count = 2 packs &&\n \t\t\ttest_line_count = 2 bitmaps &&\n \n-\t\t\tgit rev-list --use-bitmap-index HEAD 2>err &&\n-\t\t\tgrep \"ignoring extra bitmap file\" err\n+\t\t\tGIT_TRACE2_EVENT=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&\n+\t\t\tgrep \"opened bitmap\" trace2.txt &&\n+\t\t\tgrep \"ignoring extra bitmap\" trace2.txt\n \t\t)\n \t'\n }\n-- \n2.38.1.383.g7ac9b859f31.dirty\n\n"},{"id":"467169","messageId":"Y27MH2LtfVzKEM65@nand.local","threadId":"58362","inReplyTo":"cover.1668063122.git.dyroneteng@gmail.com","subject":"Re: [PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-11T22:26:39Z","receivedAt":"2022-11-11T22:26:47Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 10, 2022 at 03:10:10PM +0800, Teng Long wrote:\n> From: Teng Long \n>\n> Diff since v2:\n>\n> * remove unnecessary comments.\n> * use \"GIT_TRACE2_EVENT\" instead of \"GIT_TRACE_PERF\".\n> * improve commit message of [1/2].\n\nThanks, this version looks great. I'm very satisfied with where it ended\nup, and I'm feeling comfortable with merging it down.\n\nThanks,\nTaylor\n"},{"id":"467267","messageId":"Y3K7TEpB8EzczjTb@coredump.intra.peff.net","threadId":"58362","inReplyTo":"aaeb021538cdfeb83dc6004fe7b3ac35a23aef49.1668063122.git.dyroneteng@gmail.com","subject":"Re: [PATCH v3 1/2] pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-14T22:03:56Z","receivedAt":"2022-11-14T22:04:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 10, 2022 at 03:10:11PM +0800, Teng Long wrote:\n\n> It's likely that we'll end up relying on objects in that pack later\n> in the process (in which case we're doing the work of opening the\n> pack index optimistically), but not guaranteed.\n> \n> For instance, consider a repository with a large number of small\n> packs, and one large pack with a bitmap. If we see that bitmap pack\n> last in our loop which calls open_pack_bitmap_1(), the current code\n> will have opened *all* pack index files in the repository. If the\n> request can be served out of the bitmapped pack alone, then the time\n> spent opening these idx files was wasted.S\n\nBy the way, I wondered if it was possible to measure a slowdown in this\ncase. It is, but you have to try pretty hard. Something like this:\n\n # one bitmapped pack\n git repack -adb\n\n # and then a bunch of other packs\n git rev-list HEAD |\n head -10000 |\n while read commit; do\n echo $commit | git pack-objects .git/objects/pack/pack\n done\n\n # make the bitmapped one newest, since otherwise our non-bitmap lookup\n # of the initial traversal commit causes us to open all the other\n # packs first!\n bitmap=$(echo .git/objects/pack/pack-*.bitmap)\n touch ${bitmap%.bitmap}.*\n\n hyperfine -L v old,new './git.{v} rev-list --count --use-bitmap-index HEAD'\n\nwhere \"new\" and \"old\" are builds with and without this patch. I get:\n\n Benchmark 1: ./git.old rev-list --count --use-bitmap-index HEAD\n Time (mean ± σ): 117.9 ms ± 1.8 ms [User: 26.9 ms, System: 90.0 ms]\n Range (min … max): 113.4 ms … 120.3 ms 25 runs\n \n Benchmark 2: ./git.new rev-list --count --use-bitmap-index HEAD\n Time (mean ± σ): 71.8 ms ± 2.6 ms [User: 21.2 ms, System: 50.5 ms]\n Range (min … max): 67.0 ms … 75.1 ms 41 runs\n \n Summary\n './git.new rev-list --count --use-bitmap-index HEAD' ran\n 1.64 ± 0.06 times faster than './git.old rev-list --count --use-bitmap-index HEAD'\n\nwhich implies to me two things:\n\n - this probably isn't helping anybody much in the real world, as\n evidenced by the contortions I had to go through to set up the\n situation (and which would be made much better by repacking, which\n would also speed up non-bitmap operations).\n\n - it's worth doing anyway. Even if it only shaves off microseconds,\n the existing call is just pointless.\n\n-Peff\n"},{"id":"467269","messageId":"Y3K9zVx7L9K85NrE@nand.local","threadId":"58362","inReplyTo":"Y3K7TEpB8EzczjTb@coredump.intra.peff.net","subject":"Re: [PATCH v3 1/2] pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-14T22:14:37Z","receivedAt":"2022-11-14T22:14:45Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 14, 2022 at 05:03:56PM -0500, Jeff King wrote:\n> On Thu, Nov 10, 2022 at 03:10:11PM +0800, Teng Long wrote:\n>\n> > It's likely that we'll end up relying on objects in that pack later\n> > in the process (in which case we're doing the work of opening the\n> > pack index optimistically), but not guaranteed.\n> >\n> > For instance, consider a repository with a large number of small\n> > packs, and one large pack with a bitmap. If we see that bitmap pack\n> > last in our loop which calls open_pack_bitmap_1(), the current code\n> > will have opened *all* pack index files in the repository. If the\n> > request can be served out of the bitmapped pack alone, then the time\n> > spent opening these idx files was wasted.S\n>\n> By the way, I wondered if it was possible to measure a slowdown in this\n> case. It is, but you have to try pretty hard. Something like this:\n>\n> # one bitmapped pack\n> git repack -adb\n>\n> # and then a bunch of other packs\n> git rev-list HEAD |\n> head -10000 |\n> while read commit; do\n> echo $commit | git pack-objects .git/objects/pack/pack\n> done\n\nOK, so with 10K packs, we see about a 1.6-fold improvement, which is\ndefinitely substantial.\n\nOn a fresh clone of git.git, repeating your experiment with only 1K\npacks (which is definitely a number of packs that GitHub sees in\nunder-maintained repositories), the runtime goes from 25.3ms -> 20.9ms\non my machine, or about a 1.2-fold improvement.\n\nSo definitely smaller, but even at 1/10th the number of packs from your\nexperiment, still noticeable.\n\n> - this probably isn't helping anybody much in the real world, as\n> evidenced by the contortions I had to go through to set up the\n> situation (and which would be made much better by repacking, which\n> would also speed up non-bitmap operations).\n\nPer above, I'm not sure I totally agree ;-). 1K packs is definitely an\nextreme amount of packs, but not out-of-this-world. It probably would\nshow up in carefully-picked graphs, but not in \"overall git rev-list\ntime\" or something as broad/noisy as that.\n\n> - it's worth doing anyway. Even if it only shaves off microseconds,\n> the existing call is just pointless.\n\nYes, definitely.\n\nThanks,\nTaylor\n"},{"id":"467271","messageId":"Y3K//kO3fxD7Pl3/@coredump.intra.peff.net","threadId":"58362","inReplyTo":"Y27MH2LtfVzKEM65@nand.local","subject":"Re: [PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-14T22:23:58Z","receivedAt":"2022-11-14T22:24:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 11, 2022 at 05:26:39PM -0500, Taylor Blau wrote:\n\n> On Thu, Nov 10, 2022 at 03:10:10PM +0800, Teng Long wrote:\n> > From: Teng Long \n> >\n> > Diff since v2:\n> >\n> > * remove unnecessary comments.\n> > * use \"GIT_TRACE2_EVENT\" instead of \"GIT_TRACE_PERF\".\n> > * improve commit message of [1/2].\n> \n> Thanks, this version looks great. I'm very satisfied with where it ended\n> up, and I'm feeling comfortable with merging it down.\n\nMe too. If we wanted to go further, there are two obvious next steps.\n\nOne, we can break out of the bitmap loop early if we're not tracing:\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex aaa2d9a104..3b6c2f804a 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -527,8 +527,15 @@ static int open_pack_bitmap(struct repository *r,\n \tassert(!bitmap_git->map);\n \n \tfor (p = get_all_packs(r); p; p = p->next) {\n-\t\tif (open_pack_bitmap_1(bitmap_git, p) == 0)\n+\t\tif (open_pack_bitmap_1(bitmap_git, p) == 0) {\n \t\t\tret = 0;\n+\t\t\t/*\n+\t\t\t * The only reason to keep looking is to report\n+\t\t\t * duplicates.\n+\t\t\t */\n+\t\t\tif (!trace2_is_enabled())\n+\t\t\t\tbreak;\n+\t\t}\n \t}\n \n \treturn ret;\n\nI doubt this buys us much in practice. After patch 2, looking for extra\nbitmaps is much cheaper. It's one open() call per pack (which will\nreturn ENOENT normally) looking for a bitmap. And while it's only 2\nlines of code, it does increase coupling of assumptions between the two\nfunctions. So maybe not worth doing. I dunno.\n\nAnd two, we could complain when there is both a midx and a pack bitmap,\nlike so:\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 3b6c2f804a..44a80ed8f2 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -524,8 +524,6 @@ static int open_pack_bitmap(struct repository *r,\n \tstruct packed_git *p;\n \tint ret = -1;\n \n-\tassert(!bitmap_git->map);\n-\n \tfor (p = get_all_packs(r); p; p = p->next) {\n \t\tif (open_pack_bitmap_1(bitmap_git, p) == 0) {\n \t\t\tret = 0;\n@@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r,\n static int open_bitmap(struct repository *r,\n \t\t struct bitmap_index *bitmap_git)\n {\n+\tint found = 0;\n+\n \tassert(!bitmap_git->map);\n \n-\tif (!open_midx_bitmap(r, bitmap_git))\n-\t\treturn 0;\n-\treturn open_pack_bitmap(r, bitmap_git);\n+\tfound = !open_midx_bitmap(r, bitmap_git);\n+\n+\t/*\n+\t * these will all be skipped if we opened a midx bitmap; but run it\n+\t * anyway if tracing is enabled to report the duplicates\n+\t */\n+\tif (!found || trace2_is_enabled())\n+\t\tfound |= !open_pack_bitmap(r, bitmap_git);\n+\n+\treturn found ? 0 : -1;\n }\n \n struct bitmap_index *prepare_bitmap_git(struct repository *r)\n\nBut I'm not sure if that is even something we want. I know we retained\npack bitmaps as a quick recovery mechanism while test-deploying midx\nbitmaps. OTOH, now that the feature is stable, I doubt anybody else\nwould ever do that.\n\nIt also suffers from the same coupling issues as the other.\n\nSo I don't know that either is worth pursuing (hence this message and\nnot fully prepared patches), but these are just the two leftover things\nI noticed from the series, so I wanted to record them for posterity.\n\n-Peff\n"},{"id":"467274","messageId":"Y3LBtl+x2rCzsoIh@coredump.intra.peff.net","threadId":"58362","inReplyTo":"Y3K9zVx7L9K85NrE@nand.local","subject":"Re: [PATCH v3 1/2] pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-14T22:31:18Z","receivedAt":"2022-11-14T22:31:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 14, 2022 at 05:14:37PM -0500, Taylor Blau wrote:\n\n> OK, so with 10K packs, we see about a 1.6-fold improvement, which is\n> definitely substantial.\n> \n> On a fresh clone of git.git, repeating your experiment with only 1K\n> packs (which is definitely a number of packs that GitHub sees in\n> under-maintained repositories), the runtime goes from 25.3ms -> 20.9ms\n> on my machine, or about a 1.2-fold improvement.\n> \n> So definitely smaller, but even at 1/10th the number of packs from your\n> experiment, still noticeable.\n\nInteresting. I had tried it initially with 1000 and the improvements were\nmuch smaller. I just did it again, though, and got the same 20% speedup.\nI'm not sure what I screwed up earlier (I may have been confused by the\ntimestamp/sorting issue; I only realized it was important midway through\nlooking into this).\n\n> > - this probably isn't helping anybody much in the real world, as\n> > evidenced by the contortions I had to go through to set up the\n> > situation (and which would be made much better by repacking, which\n> > would also speed up non-bitmap operations).\n> \n> Per above, I'm not sure I totally agree ;-). 1K packs is definitely an\n> extreme amount of packs, but not out-of-this-world. It probably would\n> show up in carefully-picked graphs, but not in \"overall git rev-list\n> time\" or something as broad/noisy as that.\n\nYeah, I agree that 1k is a lot more compelling. The big impractical\nthing I think is that if the bitmapped pack is older (and it usually\nis), then we'd often open all the other packs anyway:\n\n - if the start of the traversal is in the bitmapped pack, then we\n fruitlessly open each of the others looking for the object (since\n the bitmapped one will come last in the reverse-chronological\n sorting)\n\n - if it isn't in the bitmapped pack, then we'll end up opening all\n those other packs anyway to fill out the bitmap (since by definition\n it can't be included in the on-disk bitmaps)\n\nSo I'd be surprised if it ever mattered in the real world. Though again,\nI think the new code is less surprising in general, and could matter if\nwe changed other things (e.g., if we prioritized lookups in a pack with\na .bitmap).\n\n-Peff\n"},{"id":"467281","messageId":"Y3LGQ+yV1lJiWR21@nand.local","threadId":"58362","inReplyTo":"Y3LBtl+x2rCzsoIh@coredump.intra.peff.net","subject":"Re: [PATCH v3 1/2] pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-14T22:50:43Z","receivedAt":"2022-11-14T22:50:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 14, 2022 at 05:31:18PM -0500, Jeff King wrote:\n> Yeah, I agree that 1k is a lot more compelling. The big impractical\n> thing I think is that if the bitmapped pack is older (and it usually\n> is), then we'd often open all the other packs anyway:\n>\n> - if the start of the traversal is in the bitmapped pack, then we\n> fruitlessly open each of the others looking for the object (since\n> the bitmapped one will come last in the reverse-chronological\n> sorting)\n>\n> - if it isn't in the bitmapped pack, then we'll end up opening all\n> those other packs anyway to fill out the bitmap (since by definition\n> it can't be included in the on-disk bitmaps)\n>\n> So I'd be surprised if it ever mattered in the real world. Though again,\n> I think the new code is less surprising in general, and could matter if\n> we changed other things (e.g., if we prioritized lookups in a pack with\n> a .bitmap).\n\nI completely agree. It's definitely worth doing purely based on the\nprinciple of least-surprise. But the potential performance improvements\nare just gravy on top ;-).\n\nThanks,\nTaylor\n"},{"id":"467446","messageId":"20221117141933.73790-1-tenglong.tl@alibaba-inc.com","threadId":"58362","inReplyTo":"Y3K//kO3fxD7Pl3/@coredump.intra.peff.net","subject":"Re: [PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-17T14:19:33Z","receivedAt":"2022-11-17T14:19:47Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"> Me too. If we wanted to go further, there are two obvious next steps.\n>\n> One, we can break out of the bitmap loop early if we're not tracing:\n\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index aaa2d9a104..3b6c2f804a 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -527,8 +527,15 @@ static int open_pack_bitmap(struct repository *r,\n> \tassert(!bitmap_git->map);\n>\n> \tfor (p = get_all_packs(r); p; p = p->next) {\n> -\t\tif (open_pack_bitmap_1(bitmap_git, p) == 0)\n> +\t\tif (open_pack_bitmap_1(bitmap_git, p) == 0) {\n> \t\t\tret = 0;\n> +\t\t\t/*\n> +\t\t\t * The only reason to keep looking is to report\n> +\t\t\t * duplicates.\n> +\t\t\t */\n> +\t\t\tif (!trace2_is_enabled())\n> +\t\t\t\tbreak;\n> +\t\t}\n> \t}\n>\n> \treturn ret;\n>\n> I doubt this buys us much in practice. After patch 2, looking for extra\n> bitmaps is much cheaper. It's one open() call per pack (which will\n> return ENOENT normally) looking for a bitmap. And while it's only 2\n> lines of code, it does increase coupling of assumptions between the two\n> functions. So maybe not worth doing. I dunno.\n\nI agree and I think it's reasonable.\n\nSo If I bring it into the patch how about the commit message:\n\nAuthor: Jeff King \nDate: Thu Nov 17 19:55:23 2022 +0800\n\n pack-bitmap.c: break out of the bitmap loop early if not tracing\n\n When we successfully open a bitmap, we will continue to try to open\n other packs, and when trace2 is enabled, we will report any subsequent\n bitmap ignored information in the log. So when we find that trace2 is\n not enabled, we can actually terminate the loop early.\n\n Signed-off-by: Jeff King \n Signed-off-by: Teng Long \n\n> And two, we could complain when there is both a midx and a pack bitmap,\n> like so:\n>\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index 3b6c2f804a..44a80ed8f2 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -524,8 +524,6 @@ static int open_pack_bitmap(struct repository *r,\n> \tstruct packed_git *p;\n> \tint ret = -1;\n>\n> -\tassert(!bitmap_git->map);\n> -\n> \tfor (p = get_all_packs(r); p; p = p->next) {\n> \t\tif (open_pack_bitmap_1(bitmap_git, p) == 0) {\n> \t\t\tret = 0;\n> @@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r,\n> static int open_bitmap(struct repository *r,\n> \t\t struct bitmap_index *bitmap_git)\n> {\n> +\tint found = 0;\n> +\n> \tassert(!bitmap_git->map);\n>\n> -\tif (!open_midx_bitmap(r, bitmap_git))\n> -\t\treturn 0;\n> -\treturn open_pack_bitmap(r, bitmap_git);\n> +\tfound = !open_midx_bitmap(r, bitmap_git);\n> +\n> +\t/*\n> +\t * these will all be skipped if we opened a midx bitmap; but run it\n> +\t * anyway if tracing is enabled to report the duplicates\n> +\t */\n> +\tif (!found || trace2_is_enabled())\n> +\t\tfound |= !open_pack_bitmap(r, bitmap_git);\n> +\n> +\treturn found ? 0 : -1;\n> }\n>\n> struct bitmap_index *prepare_bitmap_git(struct repository *r)\n>\n> But I'm not sure if that is even something we want. I know we retained\n> pack bitmaps as a quick recovery mechanism while test-deploying midx\n> bitmaps. OTOH, now that the feature is stable, I doubt anybody else\n> would ever do that.\n>\n> It also suffers from the same coupling issues as the other.\n>\n> So I don't know that either is worth pursuing (hence this message and\n> not fully prepared patches), but these are just the two leftover things\n> I noticed from the series, so I wanted to record them for posterity.\n\nSince this is an internal mechanism, and we are doing reminders in trace2, so\nthe diff looks good to me. How about the commit message if I need to take it:\n\nAuthor: Jeff King \nDate: Thu Nov 17 20:25:18 2022 +0800\n\n pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found\n\n We retained pack bitmaps as a quick recovery mechanism while\n test-deploying midx bitmaps. This is an internal mechanism, and we\n want to expose this rule externally through trace2 so that end users,\n repo-maintainers, and debuggers know what is happening in the process.\n\n Signed-off-by: Jeff King \n Signed-off-by: Teng Long \n\nThank you Peff for providing such detailed suggestions for improvement.\n"},{"id":"467447","messageId":"Y3ZNKhr3kMZLmQnh@coredump.intra.peff.net","threadId":"58362","inReplyTo":"20221117141933.73790-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-17T15:03:06Z","receivedAt":"2022-11-17T15:03:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 17, 2022 at 10:19:33PM +0800, Teng Long wrote:\n\n> > I doubt this buys us much in practice. After patch 2, looking for extra\n> > bitmaps is much cheaper. It's one open() call per pack (which will\n> > return ENOENT normally) looking for a bitmap. And while it's only 2\n> > lines of code, it does increase coupling of assumptions between the two\n> > functions. So maybe not worth doing. I dunno.\n> \n> I agree and I think it's reasonable.\n> \n> So If I bring it into the patch how about the commit message:\n> [...]\n\nBoth the commit messages you proposed look accurate to me.\n\nI was a bit skeptical on the second one (warning about skipping the\npack bitmap when a midx is present) just because we really did do that\nintentionally at one point. But then I remembered that these are no\nlonger producing warnings, but trace output. So in that sense, nobody\nwill really be bothered by them.\n\nI do wonder if there are people who have trace2 on all the time (for\nperformance tracing, telemetry, etc) who would find these to be junk in\ntheir logs. IMHO the trace2 mechanism is a bit coarse grained in that we\ncan only check \"is it on\". One of the nice things about the original\ntrace facility is that we could stuff this behind GIT_TRACE_BITMAPS\nwhich would really only be turned on if somebody wanted to debug\nbitmaps.\n\n-Peff\n"},{"id":"467467","messageId":"Y3auMxCWJW6Otqhy@nand.local","threadId":"58362","inReplyTo":"Y3ZNKhr3kMZLmQnh@coredump.intra.peff.net","subject":"Re: [PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-17T21:57:07Z","receivedAt":"2022-11-17T21:58:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 17, 2022 at 10:03:06AM -0500, Jeff King wrote:\n> On Thu, Nov 17, 2022 at 10:19:33PM +0800, Teng Long wrote:\n>\n> > > I doubt this buys us much in practice. After patch 2, looking for extra\n> > > bitmaps is much cheaper. It's one open() call per pack (which will\n> > > return ENOENT normally) looking for a bitmap. And while it's only 2\n> > > lines of code, it does increase coupling of assumptions between the two\n> > > functions. So maybe not worth doing. I dunno.\n> >\n> > I agree and I think it's reasonable.\n> >\n> > So If I bring it into the patch how about the commit message:\n> > [...]\n>\n> Both the commit messages you proposed look accurate to me.\n\nYep, ditto. Teng -- would you mind sending them as a short series to the\nlist so that I can pick them up? Otherwise, I can do it if you don't\nhave time or interest.\n\nThanks,\nTaylor\n"},{"id":"467652","messageId":"20221121032747.76571-1-tenglong.tl@alibaba-inc.com","threadId":"58362","inReplyTo":"Y3auMxCWJW6Otqhy@nand.local","subject":"Re: [PATCH v3 0/2] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-21T03:27:47Z","receivedAt":"2022-11-21T03:28:00Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"\nTaylor Blau writes:\n\n> Yep, ditto. Teng -- would you mind sending them as a short series to the\n> list so that I can pick them up? Otherwise, I can do it if you don't\n> have time or interest.\n\nSorry, will send today.\n\nThanks\n"},{"id":"467674","messageId":"cover.1669032425.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"cover.1667470481.git.dyroneteng@gmail.com","subject":"[PATCH v4 0/4] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-21T12:16:11Z","receivedAt":"2022-11-21T12:16:31Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nSorry for relying late, Taylor..\n\nDiff since v3:\n\nI picked Peff's two new constructive patches[1] which I think are meaningful.\nPeff also mentioned whether to design a \"GIT_TRACE2_BITMAP\" ENV to help\ndebugging bitmap, and I think it's very interesting but maybe it's a bigger\nscale on current patchset, so I prefer to do it in another series.\n\n1. https://public-inbox.org/git/Y3K%2F%2FkO3fxD7Pl3%2F@coredump.intra.peff.net/\n\nThanks.\n\nJeff King (2):\n pack-bitmap.c: break out of the bitmap loop early if not tracing\n pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found\n\nTeng Long (2):\n pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls\n pack-bitmap.c: avoid exposing absolute paths\n\n pack-bitmap.c | 39 ++++++++++++++++++++++++++-------------\n t/t5310-pack-bitmaps.sh | 5 +++--\n 2 files changed, 29 insertions(+), 15 deletions(-)\n\nRange-diff against v3:\n-: ---------- > 1: aaeb021538 pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls\n1: de941f58f9 ! 2: 9d5a491887 pack-bitmap.c: avoid exposing absolute paths\n @@ pack-bitmap.c: static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n \t\tget_midx_filename(&buf, midx->object_dir);\n -\t\t/* ignore extra bitmap file; we can only handle one */\n -\t\twarning(_(\"ignoring extra bitmap file: '%s'\"), buf.buf);\n -+\t\t/* ignore extra midx bitmap files; we can only handle one */\n +\t\ttrace2_data_string(\"bitmap\", the_repository,\n +\t\t\t\t \"ignoring extra midx bitmap file\", buf.buf);\n \t\tclose(fd);\n @@ pack-bitmap.c: static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, st\n \tif (bitmap_git->pack || bitmap_git->midx) {\n -\t\t/* ignore extra bitmap file; we can only handle one */\n -\t\twarning(_(\"ignoring extra bitmap file: '%s'\"), packfile->pack_name);\n -+\t\t/* ignore extra bitmap files; we can only handle one */\n +\t\ttrace2_data_string(\"bitmap\", the_repository,\n +\t\t\t\t \"ignoring extra bitmap file\", packfile->pack_name);\n \t\tclose(fd);\n @@ t/t5310-pack-bitmaps.sh: test_bitmap_cases () {\n \n -\t\t\tgit rev-list --use-bitmap-index HEAD 2>err &&\n -\t\t\tgrep \"ignoring extra bitmap file\" err\n -+\t\t\tGIT_TRACE2_PERF=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&\n ++\t\t\tGIT_TRACE2_EVENT=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&\n +\t\t\tgrep \"opened bitmap\" trace2.txt &&\n +\t\t\tgrep \"ignoring extra bitmap\" trace2.txt\n \t\t)\n-: ---------- > 3: f6c5b45bdc pack-bitmap.c: break out of the bitmap loop early if not tracing\n-: ---------- > 4: 2acaa3f097 pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found\n-- \n2.38.1.383.g9d5a491887b\n\n"},{"id":"467675","messageId":"aaeb021538cdfeb83dc6004fe7b3ac35a23aef49.1669032426.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"cover.1669032425.git.dyroneteng@gmail.com","subject":"[PATCH v4 1/4] pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-21T12:16:12Z","receivedAt":"2022-11-21T12:16:33Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nWhen trying to open a pack bitmap, we call open_pack_bitmap_1() in a\nloop, during which it tries to open up the pack index corresponding\nwith each available pack.\n\nIt's likely that we'll end up relying on objects in that pack later\nin the process (in which case we're doing the work of opening the\npack index optimistically), but not guaranteed.\n\nFor instance, consider a repository with a large number of small\npacks, and one large pack with a bitmap. If we see that bitmap pack\nlast in our loop which calls open_pack_bitmap_1(), the current code\nwill have opened *all* pack index files in the repository. If the\nrequest can be served out of the bitmapped pack alone, then the time\nspent opening these idx files was wasted.S\n\nSince open_pack_bitmap_1() calls is_pack_valid() later on (which in\nturns calls open_pack_index() itself), we can just drop the earlier\ncall altogether.\n\nSigned-off-by: Teng Long \n---\n pack-bitmap.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 440407f1be..982e286bac 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -411,9 +411,6 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \tstruct stat st;\n \tchar *bitmap_name;\n \n-\tif (open_pack_index(packfile))\n-\t\treturn -1;\n-\n \tbitmap_name = pack_bitmap_filename(packfile);\n \tfd = git_open(bitmap_name);\n \n-- \n2.38.1.383.g9d5a491887b\n\n"},{"id":"467676","messageId":"9d5a491887b57bbcc30010aa7efb63e316c6d190.1669032426.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"cover.1669032425.git.dyroneteng@gmail.com","subject":"[PATCH v4 2/4] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-21T12:16:13Z","receivedAt":"2022-11-21T12:16:40Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nIn \"open_midx_bitmap_1()\" and \"open_pack_bitmap_1()\", when we find that\nthere are multiple bitmaps, we will only open the first one and then\nleave warnings about the remaining pack information, the information\nwill contain the absolute path of the repository, for example in a\nalternates usage scenario. So let's hide this kind of potentially\nsensitive information in this commit.\n\nFound-by: XingXin \nSigned-off-by: Teng Long \n---\n pack-bitmap.c | 10 ++++++----\n t/t5310-pack-bitmaps.sh | 5 +++--\n 2 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 982e286bac..aaa2d9a104 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -354,8 +354,8 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n \tif (bitmap_git->pack || bitmap_git->midx) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tget_midx_filename(&buf, midx->object_dir);\n-\t\t/* ignore extra bitmap file; we can only handle one */\n-\t\twarning(_(\"ignoring extra bitmap file: '%s'\"), buf.buf);\n+\t\ttrace2_data_string(\"bitmap\", the_repository,\n+\t\t\t\t \"ignoring extra midx bitmap file\", buf.buf);\n \t\tclose(fd);\n \t\tstrbuf_release(&buf);\n \t\treturn -1;\n@@ -429,8 +429,8 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \t}\n \n \tif (bitmap_git->pack || bitmap_git->midx) {\n-\t\t/* ignore extra bitmap file; we can only handle one */\n-\t\twarning(_(\"ignoring extra bitmap file: '%s'\"), packfile->pack_name);\n+\t\ttrace2_data_string(\"bitmap\", the_repository,\n+\t\t\t\t \"ignoring extra bitmap file\", packfile->pack_name);\n \t\tclose(fd);\n \t\treturn -1;\n \t}\n@@ -455,6 +455,8 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \t\treturn -1;\n \t}\n \n+\ttrace2_data_string(\"bitmap\", the_repository, \"opened bitmap file\",\n+\t\t\t packfile->pack_name);\n \treturn 0;\n }\n \ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 6d693eef82..7d8dee41b0 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -428,8 +428,9 @@ test_bitmap_cases () {\n \t\t\ttest_line_count = 2 packs &&\n \t\t\ttest_line_count = 2 bitmaps &&\n \n-\t\t\tgit rev-list --use-bitmap-index HEAD 2>err &&\n-\t\t\tgrep \"ignoring extra bitmap file\" err\n+\t\t\tGIT_TRACE2_EVENT=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&\n+\t\t\tgrep \"opened bitmap\" trace2.txt &&\n+\t\t\tgrep \"ignoring extra bitmap\" trace2.txt\n \t\t)\n \t'\n }\n-- \n2.38.1.383.g9d5a491887b\n\n"},{"id":"467677","messageId":"f6c5b45bdcfd05eba2984163edf38d9915255047.1669032426.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"cover.1669032425.git.dyroneteng@gmail.com","subject":"[PATCH v4 3/4] pack-bitmap.c: break out of the bitmap loop early if not tracing","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-21T12:16:14Z","receivedAt":"2022-11-21T12:16:41Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Jeff King \n\nWhen we successfully open a bitmap, we will continue to try to open\nother packs, and when trace2 is enabled, we will report any subsequent\nbitmap ignored information in the log. So when we find that trace2 is\nnot enabled, we can actually terminate the loop early.\n\nSigned-off-by: Jeff King \nSigned-off-by: Teng Long \n---\n pack-bitmap.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex aaa2d9a104..3b6c2f804a 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -527,8 +527,15 @@ static int open_pack_bitmap(struct repository *r,\n \tassert(!bitmap_git->map);\n \n \tfor (p = get_all_packs(r); p; p = p->next) {\n-\t\tif (open_pack_bitmap_1(bitmap_git, p) == 0)\n+\t\tif (open_pack_bitmap_1(bitmap_git, p) == 0) {\n \t\t\tret = 0;\n+\t\t\t/*\n+\t\t\t * The only reason to keep looking is to report\n+\t\t\t * duplicates.\n+\t\t\t */\n+\t\t\tif (!trace2_is_enabled())\n+\t\t\t\tbreak;\n+\t\t}\n \t}\n \n \treturn ret;\n-- \n2.38.1.383.g9d5a491887b\n\n"},{"id":"467678","messageId":"2acaa3f097e0ab08a63ae1719454f5e11bb15a44.1669032426.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"cover.1669032425.git.dyroneteng@gmail.com","subject":"[PATCH v4 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-21T12:16:15Z","receivedAt":"2022-11-21T12:17:06Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Jeff King \n\nWe retained pack bitmaps as a quick recovery mechanism while\ntest-deploying midx bitmaps. This is an internal mechanism, and we\nwant to expose this rule externally through trace2 so that end users,\nrepo-maintainers, and debuggers know what is happening in the process.\n\nSigned-off-by: Jeff King \nSigned-off-by: Teng Long \n---\n pack-bitmap.c | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 3b6c2f804a..44a80ed8f2 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -524,8 +524,6 @@ static int open_pack_bitmap(struct repository *r,\n \tstruct packed_git *p;\n \tint ret = -1;\n \n-\tassert(!bitmap_git->map);\n-\n \tfor (p = get_all_packs(r); p; p = p->next) {\n \t\tif (open_pack_bitmap_1(bitmap_git, p) == 0) {\n \t\t\tret = 0;\n@@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r,\n static int open_bitmap(struct repository *r,\n \t\t struct bitmap_index *bitmap_git)\n {\n+\tint found = 0;\n+\n \tassert(!bitmap_git->map);\n \n-\tif (!open_midx_bitmap(r, bitmap_git))\n-\t\treturn 0;\n-\treturn open_pack_bitmap(r, bitmap_git);\n+\tfound = !open_midx_bitmap(r, bitmap_git);\n+\n+\t/*\n+\t * these will all be skipped if we opened a midx bitmap; but run it\n+\t * anyway if tracing is enabled to report the duplicates\n+\t */\n+\tif (!found || trace2_is_enabled())\n+\t\tfound |= !open_pack_bitmap(r, bitmap_git);\n+\n+\treturn found ? 0 : -1;\n }\n \n struct bitmap_index *prepare_bitmap_git(struct repository *r)\n-- \n2.38.1.383.g9d5a491887b\n\n"},{"id":"467711","messageId":"Y3vL1zWa99z/Liy7@coredump.intra.peff.net","threadId":"58362","inReplyTo":"cover.1669032425.git.dyroneteng@gmail.com","subject":"Re: [PATCH v4 0/4] pack-bitmap.c: avoid exposing absolute paths","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-21T19:04:55Z","receivedAt":"2022-11-21T19:05:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 21, 2022 at 08:16:11PM +0800, Teng Long wrote:\n\n> Diff since v3:\n> \n> I picked Peff's two new constructive patches[1] which I think are meaningful.\n> Peff also mentioned whether to design a \"GIT_TRACE2_BITMAP\" ENV to help\n> debugging bitmap, and I think it's very interesting but maybe it's a bigger\n> scale on current patchset, so I prefer to do it in another series.\n\nI think the first two are already in 'next', so they aren't eligible for\nre-roll, though from the range diff it looks like those changes are\nalready there?\n\n-Peff\n"},{"id":"467712","messageId":"Y3vM3GZYFy+l006d@coredump.intra.peff.net","threadId":"58362","inReplyTo":"2acaa3f097e0ab08a63ae1719454f5e11bb15a44.1669032426.git.dyroneteng@gmail.com","subject":"Re: [PATCH v4 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-21T19:09:16Z","receivedAt":"2022-11-21T19:09:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 21, 2022 at 08:16:15PM +0800, Teng Long wrote:\n\n> From: Jeff King \n> \n> We retained pack bitmaps as a quick recovery mechanism while\n> test-deploying midx bitmaps. This is an internal mechanism, and we\n> want to expose this rule externally through trace2 so that end users,\n> repo-maintainers, and debuggers know what is happening in the process.\n\nRe-reading this outside the context of the earlier thread, I think \"this\nrule\" is a little vague. Maybe:\n\n When we find a midx bitmap, we do not bother checking for pack\n bitmaps, since we can use only one. But since we will warn of unused\n bitmaps via trace2, let's continue looking for pack bitmaps when\n tracing is enabled.\n\n> @@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r,\n> static int open_bitmap(struct repository *r,\n> \t\t struct bitmap_index *bitmap_git)\n> {\n> +\tint found = 0;\n> +\n> \tassert(!bitmap_git->map);\n> \n> -\tif (!open_midx_bitmap(r, bitmap_git))\n> -\t\treturn 0;\n> -\treturn open_pack_bitmap(r, bitmap_git);\n> +\tfound = !open_midx_bitmap(r, bitmap_git);\n\nI think we can drop the initialization of \"found = 0\"; that value is\nunused, since we'll always assign to it (I think my initial attempt had\nsetting it to 1 inside the conditional).\n\nIt's not hurting anything functionally, but it makes the code slightly\nmore confusing to read.\n\n-Peff\n"},{"id":"467724","messageId":"xmqqfsebvqg8.fsf@gitster.g","threadId":"58362","inReplyTo":"f6c5b45bdcfd05eba2984163edf38d9915255047.1669032426.git.dyroneteng@gmail.com","subject":"Re: [PATCH v4 3/4] pack-bitmap.c: break out of the bitmap loop early if not tracing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-21T23:27:19Z","receivedAt":"2022-11-21T23:27:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Teng Long writes:\n\n> From: Jeff King \n>\n> When we successfully open a bitmap, we will continue to try to open\n> other packs, and when trace2 is enabled, we will report any subsequent\n> bitmap ignored information in the log. So when we find that trace2 is\n> not enabled, we can actually terminate the loop early.\n\nThe above took me a few reads to understand what it wants to say,\nprobably because the \"and when trace2 is enabled\" comes a bit too\nlate to explain why \"try to open other\" is done. After reading it a\nfew times, here is what I think it wants to say:\n\n After opening a bitmap successfully, we try opening others only\n because we want to report that other bitmap files are ignored in\n the trace2 log. When trace2 is not enabled, we do not have to\n do any of that.\n\n\n\n>\n> Signed-off-by: Jeff King \n> Signed-off-by: Teng Long \n> ---\n> pack-bitmap.c | 9 ++++++++-\n> 1 file changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index aaa2d9a104..3b6c2f804a 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -527,8 +527,15 @@ static int open_pack_bitmap(struct repository *r,\n> \tassert(!bitmap_git->map);\n> \n> \tfor (p = get_all_packs(r); p; p = p->next) {\n> -\t\tif (open_pack_bitmap_1(bitmap_git, p) == 0)\n> +\t\tif (open_pack_bitmap_1(bitmap_git, p) == 0) {\n> \t\t\tret = 0;\n> +\t\t\t/*\n> +\t\t\t * The only reason to keep looking is to report\n> +\t\t\t * duplicates.\n> +\t\t\t */\n> +\t\t\tif (!trace2_is_enabled())\n> +\t\t\t\tbreak;\n> +\t\t}\n> \t}\n> \n> \treturn ret;\n"},{"id":"467725","messageId":"xmqqbkozvqbt.fsf@gitster.g","threadId":"58362","inReplyTo":"Y3vM3GZYFy+l006d@coredump.intra.peff.net","subject":"Re: [PATCH v4 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-21T23:29:58Z","receivedAt":"2022-11-21T23:30:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ...\n> It's not hurting anything functionally, but it makes the code slightly\n> more confusing to read.\n\nThanks for all these good suggestions.\n\nAs you pointed out, the first two seems to be identical to what\nTaylor queued in 'next' already (so they do not have to be re-sent\nbut it is not a huge deal if they did---it would be a huge deal if\nthey were rewritten, though), and both of the two follow-on patches\nmake sense with suggested (minor) updates. I'll expect a reroll\nbefore queuing these two and mark the topic in 'next' to be waiting\nfor them.\n\nThanks, both.\n"},{"id":"468077","messageId":"20221128122913.53518-1-tenglong.tl@alibaba-inc.com","threadId":"58362","inReplyTo":"xmqqbkozvqbt.fsf@gitster.g","subject":"Re: [PATCH v4 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-28T12:29:13Z","receivedAt":"2022-11-28T12:30:32Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Junio C Hamano write:\n\n> Thanks for all these good suggestions.\n>\n> As you pointed out, the first two seems to be identical to what\n> Taylor queued in 'next' already (so they do not have to be re-sent\n> but it is not a huge deal if they did---it would be a huge deal if\n> they were rewritten, though), and both of the two follow-on patches\n> make sense with suggested (minor) updates. I'll expect a reroll\n> before queuing these two and mark the topic in 'next' to be waiting\n> for them.\n\nSorry, Not online for a few days last week.\nWill send a reroll today.\n\nThanks.\n"},{"id":"468079","messageId":"20221128123740.54250-1-tenglong.tl@alibaba-inc.com","threadId":"58362","inReplyTo":"Y3vM3GZYFy+l006d@coredump.intra.peff.net","subject":"Re: [PATCH v4 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-28T12:37:40Z","receivedAt":"2022-11-28T12:37:55Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Jeff King writes:\n\n> > From: Jeff King \n> >\n> > We retained pack bitmaps as a quick recovery mechanism while\n> > test-deploying midx bitmaps. This is an internal mechanism, and we\n> > want to expose this rule externally through trace2 so that end users,\n> > repo-maintainers, and debuggers know what is happening in the process.\n>\n> Re-reading this outside the context of the earlier thread, I think \"this\n> rule\" is a little vague. Maybe:\n>\n> When we find a midx bitmap, we do not bother checking for pack\n> bitmaps, since we can use only one. But since we will warn of unused\n> bitmaps via trace2, let's continue looking for pack bitmaps when\n> tracing is enabled.\n\nThanks. That's more clear and will be applies on next patch.\n\n> > @@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r,\n> > static int open_bitmap(struct repository *r,\n> > \t\t struct bitmap_index *bitmap_git)\n> > {\n> > +\tint found = 0;\n> > +\n> > \tassert(!bitmap_git->map);\n> >\n> > -\tif (!open_midx_bitmap(r, bitmap_git))\n> > -\t\treturn 0;\n> > -\treturn open_pack_bitmap(r, bitmap_git);\n> > +\tfound = !open_midx_bitmap(r, bitmap_git);\n>\n> I think we can drop the initialization of \"found = 0\"; that value is\n> unused, since we'll always assign to it (I think my initial attempt had\n> setting it to 1 inside the conditional).\n>\n> It's not hurting anything functionally, but it makes the code slightly\n> more confusing to read.\n\nI agree it's not hurting here, it's OK for me to make the improvement\nhere. But I have a question, do we prefer to omit the initialization\nin such scenarios if we make sure it will initialized correctly？\n\nThanks.\n"},{"id":"468082","messageId":"20221128124807.54707-1-tenglong.tl@alibaba-inc.com","threadId":"58362","inReplyTo":"Y3vL1zWa99z/Liy7@coredump.intra.peff.net","subject":"Re: [PATCH v4 0/4] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-28T12:48:07Z","receivedAt":"2022-11-28T12:48:21Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Jeff King writes:\n\n> > Diff since v3:\n> >\n> > I picked Peff's two new constructive patches[1] which I think are meaningful.\n> > Peff also mentioned whether to design a \"GIT_TRACE2_BITMAP\" ENV to help\n> > debugging bitmap, and I think it's very interesting but maybe it's a bigger\n> > scale on current patchset, so I prefer to do it in another series.\n>\n> I think the first two are already in 'next', so they aren't eligible for\n> re-roll, though from the range diff it looks like those changes are\n> already there?\n\nOops, I created the wrong range-diff. I didn't check the 'next', but now\nI think I shall make the reroll here because the new replies from Junio\nand you.\n\nThanks.\n"},{"id":"468087","messageId":"20221128130940.56464-1-dyroneteng@gmail.com","threadId":"58362","inReplyTo":"xmqqfsebvqg8.fsf@gitster.g","subject":"Re: [PATCH v4 3/4] pack-bitmap.c: break out of the bitmap loop early if not tracing","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-28T13:09:40Z","receivedAt":"2022-11-28T13:09:51Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Junio C Hamano writes:\n\n> From: Jeff King \n>\n> When we successfully open a bitmap, we will continue to try to open\n> other packs, and when trace2 is enabled, we will report any subsequent\n> bitmap ignored information in the log. So when we find that trace2 is\n> not enabled, we can actually terminate the loop early.\n\n> The above took me a few reads to understand what it wants to say,\n> probably because the \"and when trace2 is enabled\" comes a bit too\n> late to explain why \"try to open other\" is done. After reading it a\n> few times, here is what I think it wants to say:\n>\n> After opening a bitmap successfully, we try opening others only\n> because we want to report that other bitmap files are ignored in\n> the trace2 log. When trace2 is not enabled, we do not have to\n> do any of that.\n\nThanks for the reword, will apply in reroll.\n"},{"id":"468095","messageId":"cover.1669644101.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"cover.1669032425.git.dyroneteng@gmail.com","subject":"[PATCH v5 0/4] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-28T14:09:49Z","receivedAt":"2022-11-28T14:10:11Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nChanges since v4:\n\n* [3/4] Reroll the commit message.\n* [4/4] Reroll the commit message and remove the initialization of 'found'.\n\nBesides of 3/4 and 4/4, the 1/4 and 2/4 seem already in 'next' as\n8ddc06631b184e6f43051f7ea8ac9bbc86817127 and\n2aa84d5f3ea1966a81477ad31bee0136e39d3917 , but I send again here just for\nthe completeness of the patchset, I think it will be nice to merge Peff's\ncommits of there are no new suggestions.\n\nThanks.\n\nJeff King (2):\n pack-bitmap.c: break out of the bitmap loop early if not tracing\n pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found\n\nTeng Long (2):\n pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls\n pack-bitmap.c: avoid exposing absolute paths\n\n pack-bitmap.c | 39 ++++++++++++++++++++++++++-------------\n t/t5310-pack-bitmaps.sh | 5 +++--\n 2 files changed, 29 insertions(+), 15 deletions(-)\n\nRange-diff against v4:\n-: ---------- > 1: aaeb021538 pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls\n-: ---------- > 2: 9d5a491887 pack-bitmap.c: avoid exposing absolute paths\n1: f6c5b45bdc ! 3: 22deec6aab pack-bitmap.c: break out of the bitmap loop early if not tracing\n @@ Metadata\n ## Commit message ##\n pack-bitmap.c: break out of the bitmap loop early if not tracing\n \n - When we successfully open a bitmap, we will continue to try to open\n - other packs, and when trace2 is enabled, we will report any subsequent\n - bitmap ignored information in the log. So when we find that trace2 is\n - not enabled, we can actually terminate the loop early.\n + After opening a bitmap successfully, we try opening others only\n + because we want to report that other bitmap files are ignored in\n + the trace2 log. When trace2 is not enabled, we do not have to\n + do any of that.\n \n Signed-off-by: Jeff King \n Signed-off-by: Teng Long \n2: 2acaa3f097 ! 4: 28306b7e8d pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found\n @@ Metadata\n ## Commit message ##\n pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found\n \n - We retained pack bitmaps as a quick recovery mechanism while\n - test-deploying midx bitmaps. This is an internal mechanism, and we\n - want to expose this rule externally through trace2 so that end users,\n - repo-maintainers, and debuggers know what is happening in the process.\n + When we find a midx bitmap, we do not bother checking for pack\n + bitmaps, since we can use only one. But since we will warn of unused\n + bitmaps via trace2, let's continue looking for pack bitmaps when\n + tracing is enabled.\n \n Signed-off-by: Jeff King \n Signed-off-by: Teng Long \n @@ pack-bitmap.c: static int open_midx_bitmap(struct repository *r,\n static int open_bitmap(struct repository *r,\n \t\t struct bitmap_index *bitmap_git)\n {\n -+\tint found = 0;\n ++\tint found;\n +\n \tassert(!bitmap_git->map);\n \n-- \n2.38.1.385.g28306b7e8d5\n\n"},{"id":"468096","messageId":"aaeb021538cdfeb83dc6004fe7b3ac35a23aef49.1669644101.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"cover.1669644101.git.dyroneteng@gmail.com","subject":"[PATCH v5 1/4] pack-bitmap.c: remove unnecessary \"open_pack_index()\" calls","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-28T14:09:50Z","receivedAt":"2022-11-28T14:10:14Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nWhen trying to open a pack bitmap, we call open_pack_bitmap_1() in a\nloop, during which it tries to open up the pack index corresponding\nwith each available pack.\n\nIt's likely that we'll end up relying on objects in that pack later\nin the process (in which case we're doing the work of opening the\npack index optimistically), but not guaranteed.\n\nFor instance, consider a repository with a large number of small\npacks, and one large pack with a bitmap. If we see that bitmap pack\nlast in our loop which calls open_pack_bitmap_1(), the current code\nwill have opened *all* pack index files in the repository. If the\nrequest can be served out of the bitmapped pack alone, then the time\nspent opening these idx files was wasted.S\n\nSince open_pack_bitmap_1() calls is_pack_valid() later on (which in\nturns calls open_pack_index() itself), we can just drop the earlier\ncall altogether.\n\nSigned-off-by: Teng Long \n---\n pack-bitmap.c | 3 ---\n 1 file changed, 3 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 440407f1be..982e286bac 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -411,9 +411,6 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \tstruct stat st;\n \tchar *bitmap_name;\n \n-\tif (open_pack_index(packfile))\n-\t\treturn -1;\n-\n \tbitmap_name = pack_bitmap_filename(packfile);\n \tfd = git_open(bitmap_name);\n \n-- \n2.38.1.385.g28306b7e8d5\n\n"},{"id":"468097","messageId":"9d5a491887b57bbcc30010aa7efb63e316c6d190.1669644101.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"cover.1669644101.git.dyroneteng@gmail.com","subject":"[PATCH v5 2/4] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-28T14:09:51Z","receivedAt":"2022-11-28T14:10:17Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Teng Long \n\nIn \"open_midx_bitmap_1()\" and \"open_pack_bitmap_1()\", when we find that\nthere are multiple bitmaps, we will only open the first one and then\nleave warnings about the remaining pack information, the information\nwill contain the absolute path of the repository, for example in a\nalternates usage scenario. So let's hide this kind of potentially\nsensitive information in this commit.\n\nFound-by: XingXin \nSigned-off-by: Teng Long \n---\n pack-bitmap.c | 10 ++++++----\n t/t5310-pack-bitmaps.sh | 5 +++--\n 2 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 982e286bac..aaa2d9a104 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -354,8 +354,8 @@ static int open_midx_bitmap_1(struct bitmap_index *bitmap_git,\n \tif (bitmap_git->pack || bitmap_git->midx) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tget_midx_filename(&buf, midx->object_dir);\n-\t\t/* ignore extra bitmap file; we can only handle one */\n-\t\twarning(_(\"ignoring extra bitmap file: '%s'\"), buf.buf);\n+\t\ttrace2_data_string(\"bitmap\", the_repository,\n+\t\t\t\t \"ignoring extra midx bitmap file\", buf.buf);\n \t\tclose(fd);\n \t\tstrbuf_release(&buf);\n \t\treturn -1;\n@@ -429,8 +429,8 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \t}\n \n \tif (bitmap_git->pack || bitmap_git->midx) {\n-\t\t/* ignore extra bitmap file; we can only handle one */\n-\t\twarning(_(\"ignoring extra bitmap file: '%s'\"), packfile->pack_name);\n+\t\ttrace2_data_string(\"bitmap\", the_repository,\n+\t\t\t\t \"ignoring extra bitmap file\", packfile->pack_name);\n \t\tclose(fd);\n \t\treturn -1;\n \t}\n@@ -455,6 +455,8 @@ static int open_pack_bitmap_1(struct bitmap_index *bitmap_git, struct packed_git\n \t\treturn -1;\n \t}\n \n+\ttrace2_data_string(\"bitmap\", the_repository, \"opened bitmap file\",\n+\t\t\t packfile->pack_name);\n \treturn 0;\n }\n \ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 6d693eef82..7d8dee41b0 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -428,8 +428,9 @@ test_bitmap_cases () {\n \t\t\ttest_line_count = 2 packs &&\n \t\t\ttest_line_count = 2 bitmaps &&\n \n-\t\t\tgit rev-list --use-bitmap-index HEAD 2>err &&\n-\t\t\tgrep \"ignoring extra bitmap file\" err\n+\t\t\tGIT_TRACE2_EVENT=$(pwd)/trace2.txt git rev-list --use-bitmap-index HEAD &&\n+\t\t\tgrep \"opened bitmap\" trace2.txt &&\n+\t\t\tgrep \"ignoring extra bitmap\" trace2.txt\n \t\t)\n \t'\n }\n-- \n2.38.1.385.g28306b7e8d5\n\n"},{"id":"468098","messageId":"22deec6aab6649b228af7d858b93672f9ce2b925.1669644101.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"cover.1669644101.git.dyroneteng@gmail.com","subject":"[PATCH v5 3/4] pack-bitmap.c: break out of the bitmap loop early if not tracing","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-28T14:09:52Z","receivedAt":"2022-11-28T14:10:19Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Jeff King \n\nAfter opening a bitmap successfully, we try opening others only\nbecause we want to report that other bitmap files are ignored in\nthe trace2 log. When trace2 is not enabled, we do not have to\ndo any of that.\n\nSigned-off-by: Jeff King \nSigned-off-by: Teng Long \n---\n pack-bitmap.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex aaa2d9a104..3b6c2f804a 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -527,8 +527,15 @@ static int open_pack_bitmap(struct repository *r,\n \tassert(!bitmap_git->map);\n \n \tfor (p = get_all_packs(r); p; p = p->next) {\n-\t\tif (open_pack_bitmap_1(bitmap_git, p) == 0)\n+\t\tif (open_pack_bitmap_1(bitmap_git, p) == 0) {\n \t\t\tret = 0;\n+\t\t\t/*\n+\t\t\t * The only reason to keep looking is to report\n+\t\t\t * duplicates.\n+\t\t\t */\n+\t\t\tif (!trace2_is_enabled())\n+\t\t\t\tbreak;\n+\t\t}\n \t}\n \n \treturn ret;\n-- \n2.38.1.385.g28306b7e8d5\n\n"},{"id":"468099","messageId":"28306b7e8d560f48d3546527c1f60f5126eeb8a1.1669644101.git.dyroneteng@gmail.com","threadId":"58362","inReplyTo":"cover.1669644101.git.dyroneteng@gmail.com","subject":"[PATCH v5 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-28T14:09:53Z","receivedAt":"2022-11-28T14:10:40Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"From: Jeff King \n\nWhen we find a midx bitmap, we do not bother checking for pack\nbitmaps, since we can use only one. But since we will warn of unused\nbitmaps via trace2, let's continue looking for pack bitmaps when\ntracing is enabled.\n\nSigned-off-by: Jeff King \nSigned-off-by: Teng Long \n---\n pack-bitmap.c | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 3b6c2f804a..d2a42abf28 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -524,8 +524,6 @@ static int open_pack_bitmap(struct repository *r,\n \tstruct packed_git *p;\n \tint ret = -1;\n \n-\tassert(!bitmap_git->map);\n-\n \tfor (p = get_all_packs(r); p; p = p->next) {\n \t\tif (open_pack_bitmap_1(bitmap_git, p) == 0) {\n \t\t\tret = 0;\n@@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r,\n static int open_bitmap(struct repository *r,\n \t\t struct bitmap_index *bitmap_git)\n {\n+\tint found;\n+\n \tassert(!bitmap_git->map);\n \n-\tif (!open_midx_bitmap(r, bitmap_git))\n-\t\treturn 0;\n-\treturn open_pack_bitmap(r, bitmap_git);\n+\tfound = !open_midx_bitmap(r, bitmap_git);\n+\n+\t/*\n+\t * these will all be skipped if we opened a midx bitmap; but run it\n+\t * anyway if tracing is enabled to report the duplicates\n+\t */\n+\tif (!found || trace2_is_enabled())\n+\t\tfound |= !open_pack_bitmap(r, bitmap_git);\n+\n+\treturn found ? 0 : -1;\n }\n \n struct bitmap_index *prepare_bitmap_git(struct repository *r)\n-- \n2.38.1.385.g28306b7e8d5\n\n"},{"id":"468130","messageId":"Y4VDkq9vlLjFfnCx@nand.local","threadId":"58362","inReplyTo":"22deec6aab6649b228af7d858b93672f9ce2b925.1669644101.git.dyroneteng@gmail.com","subject":"Re: [PATCH v5 3/4] pack-bitmap.c: break out of the bitmap loop early if not tracing","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-28T23:26:10Z","receivedAt":"2022-11-28T23:26:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 28, 2022 at 10:09:52PM +0800, Teng Long wrote:\n> diff --git a/pack-bitmap.c b/pack-bitmap.c\n> index aaa2d9a104..3b6c2f804a 100644\n> --- a/pack-bitmap.c\n> +++ b/pack-bitmap.c\n> @@ -527,8 +527,15 @@ static int open_pack_bitmap(struct repository *r,\n> \tassert(!bitmap_git->map);\n>\n> \tfor (p = get_all_packs(r); p; p = p->next) {\n> -\t\tif (open_pack_bitmap_1(bitmap_git, p) == 0)\n> +\t\tif (open_pack_bitmap_1(bitmap_git, p) == 0) {\n\nWhile we're here, we *could* change this line to read:\n\n if (!open_pack_bitmap_1(bitmap_git, p))\n\nwhich more closely adheres to our conventions. But I don't think just\nbecause we could do something necessarily means that we have to, so I'm\nhappy to leave it alone, too. It definitely does not merit a reroll on\nits own.\n\nOtherwise this patch looks quite reasonable.\n\nThanks,\nTaylor\n"},{"id":"468131","messageId":"Y4VEeIC+iInY4MuB@nand.local","threadId":"58362","inReplyTo":"cover.1669644101.git.dyroneteng@gmail.com","subject":"Re: [PATCH v5 0/4] pack-bitmap.c: avoid exposing absolute paths","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-28T23:30:00Z","receivedAt":"2022-11-28T23:30:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 28, 2022 at 10:09:49PM +0800, Teng Long wrote:\n> From: Teng Long \n>\n> Changes since v4:\n>\n> * [3/4] Reroll the commit message.\n> * [4/4] Reroll the commit message and remove the initialization of 'found'.\n>\n> Besides of 3/4 and 4/4, the 1/4 and 2/4 seem already in 'next' as\n> 8ddc06631b184e6f43051f7ea8ac9bbc86817127 and\n> 2aa84d5f3ea1966a81477ad31bee0136e39d3917 , but I send again here just for\n> the completeness of the patchset, I think it will be nice to merge Peff's\n> commits of there are no new suggestions.\n\nAs you note, the first couple of patches are already in 'next', so those\ndo not need to be queued again.\n\nBut the latter two patches look good, and are ready to go.\n\nThanks for continuing to work on this :-)..\n\nThanks,\nTaylor\n"},{"id":"468143","messageId":"Y4VgG0Bu1P2NW4d5@coredump.intra.peff.net","threadId":"58362","inReplyTo":"20221128123740.54250-1-tenglong.tl@alibaba-inc.com","subject":"Re: [PATCH v4 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-29T01:27:55Z","receivedAt":"2022-11-29T01:28:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 28, 2022 at 08:37:40PM +0800, Teng Long wrote:\n\n> > > @@ -559,11 +557,20 @@ static int open_midx_bitmap(struct repository *r,\n> > > static int open_bitmap(struct repository *r,\n> > > \t\t struct bitmap_index *bitmap_git)\n> > > {\n> > > +\tint found = 0;\n> > > +\n> > > \tassert(!bitmap_git->map);\n> > >\n> > > -\tif (!open_midx_bitmap(r, bitmap_git))\n> > > -\t\treturn 0;\n> > > -\treturn open_pack_bitmap(r, bitmap_git);\n> > > +\tfound = !open_midx_bitmap(r, bitmap_git);\n> >\n> > I think we can drop the initialization of \"found = 0\"; that value is\n> > unused, since we'll always assign to it (I think my initial attempt had\n> > setting it to 1 inside the conditional).\n> >\n> > It's not hurting anything functionally, but it makes the code slightly\n> > more confusing to read.\n> \n> I agree it's not hurting here, it's OK for me to make the improvement\n> here. But I have a question, do we prefer to omit the initialization\n> in such scenarios if we make sure it will initialized correctly？\n\nI don't know that we have a particular rule here, but I would say that\nyes, if you know the initialization isn't used, then skip it. That\ncommunicates the expectation to anybody reading the code. And if you're\nwrong, the compiler will generally notice.\n\n-Peff\n"},{"id":"468178","messageId":"20221129131422.82642-1-tenglong.tl@alibaba-inc.com","threadId":"58362","inReplyTo":"Y4VgG0Bu1P2NW4d5@coredump.intra.peff.net","subject":"Re: [PATCH v4 4/4] pack-bitmap.c: trace bitmap ignore logs when midx-bitmap is found","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-29T13:14:22Z","receivedAt":"2022-11-29T13:15:30Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Jeff King writes:\n\n> > I agree it's not hurting here, it's OK for me to make the improvement\n> > here. But I have a question, do we prefer to omit the initialization\n> > in such scenarios if we make sure it will initialized correctly？\n>\n> I don't know that we have a particular rule here, but I would say that\n> yes, if you know the initialization isn't used, then skip it. That\n> communicates the expectation to anybody reading the code. And if you're\n> wrong, the compiler will generally notice.\n\nThanks for the explanation!\n"},{"id":"468183","messageId":"20221129131739.82847-1-tenglong.tl@alibaba-inc.com","threadId":"58362","inReplyTo":"Y4VDkq9vlLjFfnCx@nand.local","subject":"Re: [PATCH v5 3/4] pack-bitmap.c: break out of the bitmap loop early if not tracing","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-29T13:17:39Z","receivedAt":"2022-11-29T13:18:13Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Taylor Blau writes:\n\n> > diff --git a/pack-bitmap.c b/pack-bitmap.c\n> > index aaa2d9a104..3b6c2f804a 100644\n> > --- a/pack-bitmap.c\n> > +++ b/pack-bitmap.c\n> > @@ -527,8 +527,15 @@ static int open_pack_bitmap(struct repository *r,\n> > \tassert(!bitmap_git->map);\n> >\n> > \tfor (p = get_all_packs(r); p; p = p->next) {\n> > -\t\tif (open_pack_bitmap_1(bitmap_git, p) == 0)\n> > +\t\tif (open_pack_bitmap_1(bitmap_git, p) == 0) {\n>\n> While we're here, we *could* change this line to read:\n>\n> if (!open_pack_bitmap_1(bitmap_git, p))\n\nFeeling a bit sad why I didn't notice here.\n\n> which more closely adheres to our conventions. But I don't think just\n> because we could do something necessarily means that we have to, so I'm\n> happy to leave it alone, too. It definitely does not merit a reroll on\n> its own.\n>\n> Otherwise this patch looks quite reasonable.\n\nThat's a good catch and thanks for your suggestion.\n"},{"id":"468185","messageId":"20221129132107.82953-1-tenglong.tl@alibaba-inc.com","threadId":"58362","inReplyTo":"Y4VEeIC+iInY4MuB@nand.local","subject":"Re: [PATCH v5 0/4] pack-bitmap.c: avoid exposing absolute paths","fromName":"Teng Long","fromEmail":"dyroneteng@gmail.com","sentAt":"2022-11-29T13:21:07Z","receivedAt":"2022-11-29T13:21:19Z","isPatch":true,"sender":{"key":"dyroneteng@gmail.com","avatar":"https://avatars.githubusercontent.com/u/7803958?v=4"},"body":"Taylor Blau writes:\n\n> > Besides of 3/4 and 4/4, the 1/4 and 2/4 seem already in 'next' as\n> > 8ddc06631b184e6f43051f7ea8ac9bbc86817127 and\n> > 2aa84d5f3ea1966a81477ad31bee0136e39d3917 , but I send again here just for\n> > the completeness of the patchset, I think it will be nice to merge Peff's\n> > commits of there are no new suggestions.\n>\n> As you note, the first couple of patches are already in 'next', so those\n> do not need to be queued again.\n>\n> But the latter two patches look good, and are ready to go.\n>\n> Thanks for continuing to work on this :-)..\n\nThank you for your help, Taylor Blau.\n"}]}