{"thread":{"id":"51366","subject":"2.22.0 repack -a duplicating pack contents","startedAt":"2019-06-23T12:15:51Z","lastAt":"2019-07-08T17:41:09Z","messageCount":20,"participants":["Janos Farkas","Ævar Arnfjörð Bjarmason","Jeff King","Eric Wong","SZEDER Gábor","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"377823","messageId":"CAEXt3sqno7RAtuwQ_OpD3aLkEORLaf6aNeNKGQL0BKezD+wWTw@mail.gmail.com","threadId":"51366","inReplyTo":null,"subject":"2.22.0 repack -a duplicating pack contents","fromName":"Janos Farkas","fromEmail":"chexum@gmail.com","sentAt":"2019-06-23T12:15:38Z","receivedAt":"2019-06-23T12:15:51Z","isPatch":false,"sender":{"key":"chexum@gmail.com","avatar":null},"body":"I'm using .keep files to... well.. keep packs to avoid some CPU time\nspent on repacking huge packs and make the process somewhat more\nincremental.\n\nSomething changed with 22.2.0. Now .bitmap files are also created,\nand no simple repacks re-create the pack data in a completely new\nfile, wasting quite some storage:\n\n02d03::master> find objects/pack/pack* -type f|xargs ls -sht\n108K objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.bitmap\n524K objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.idx\n4.7M objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.pack\n108K objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.bitmap\n524K objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.idx\n4.6M objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.pack\n116K objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.bitmap\n524K objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.idx\n4.6M objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.pack\n 0 objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.keep\n108K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.bitmap\n524K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.idx\n4.6M objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.pack\n02d03::master > git repack -af\nEnumerating objects: 19001, done.\nCounting objects: 100% (19001/19001), done.\nDelta compression using up to 2 threads\nCompressing objects: 100% (18952/18952), done.\nWriting objects: 100% (19001/19001), done.\nwarning: ignoring extra bitmap file:\n./objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.pack\nwarning: ignoring extra bitmap file:\n./objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.pack\nwarning: ignoring extra bitmap file:\n./objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.pack\nReusing bitmaps: 104, done.\nSelecting bitmap commits: 2550, done.\nBuilding bitmaps: 100% (130/130), done.\nTotal 19001 (delta 14837), reused 4162 (delta 0)\n02d03::master > find objects/pack/pack* -type f|xargs ls -sht\n108K objects/pack/pack-8702a2550b7e29940af8bc62bc6fca011ccbd455.bitmap\n524K objects/pack/pack-8702a2550b7e29940af8bc62bc6fca011ccbd455.idx\n4.6M objects/pack/pack-8702a2550b7e29940af8bc62bc6fca011ccbd455.pack <= ????\n108K objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.bitmap\n524K objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.idx\n4.7M objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.pack\n108K objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.bitmap\n524K objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.idx\n4.6M objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.pack\n116K objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.bitmap\n524K objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.idx\n4.6M objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.pack\n 0 objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.keep\n108K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.bitmap\n524K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.idx\n4.6M objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.pack\n\nThe ccbd455 pack and its metadata seem quite pointless to be\ncontaining apparently all the data based on the size.\n\nIf I use -ad, a new pack is still created,which, judging by the size,\nis essentially everything again, (but at least the extra packs are\nremoved)\n\n02d03::master> git repack -ad\nEnumerating objects: 19001, done.\nCounting objects: 100% (19001/19001), done.\nDelta compression using up to 2 threads\nCompressing objects: 100% (4114/4114), done.\nWriting objects: 100% (19001/19001), done.\nwarning: ignoring extra bitmap file:\n./objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.pack\nReusing bitmaps: 104, done.\nSelecting bitmap commits: 2550, done.\nBuilding bitmaps: 100% (130/130), done.\nTotal 19001 (delta 14838), reused 19001 (delta 14838)\n02d03::master 9060> find objects/pack/pack* -type f|xargs ls -sht\n116K objects/pack/pack-46ab64716d4220aac8d53b380d90a264d5293d3d.bitmap\n524K objects/pack/pack-46ab64716d4220aac8d53b380d90a264d5293d3d.idx\n4.6M objects/pack/pack-46ab64716d4220aac8d53b380d90a264d5293d3d.pack <= ????\n 0 objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.keep\n108K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.bitmap\n524K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.idx\n4.6M objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.pack\n\nPreviously, the kept pack would be kept, and no additional packs would\nbe created if no new objects were born in the repro.\n\nWith the .keep placeholder removed, the duplication does not happen,\nbut all the repro is rewritten into a new pack, which does not look\ncorrect. Am I doing something unexpected?\n"},{"id":"377825","messageId":"875zow8i85.fsf@evledraar.gmail.com","threadId":"51366","inReplyTo":"CAEXt3sqno7RAtuwQ_OpD3aLkEORLaf6aNeNKGQL0BKezD+wWTw@mail.gmail.com","subject":"Re: 2.22.0 repack -a duplicating pack contents","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-06-23T14:54:50Z","receivedAt":"2019-06-23T14:54:55Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Jun 23 2019, Janos Farkas wrote:\n\n> I'm using .keep files to... well.. keep packs to avoid some CPU time\n> spent on repacking huge packs and make the process somewhat more\n> incremental.\n>\n> Something changed with 22.2.0. Now .bitmap files are also created,\n> and no simple repacks re-create the pack data in a completely new\n> file, wasting quite some storage:\n>\n> 02d03::master> find objects/pack/pack* -type f|xargs ls -sht\n> 108K objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.bitmap\n> 524K objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.idx\n> 4.7M objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.pack\n> 108K objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.bitmap\n> 524K objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.idx\n> 4.6M objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.pack\n> 116K objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.bitmap\n> 524K objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.idx\n> 4.6M objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.pack\n> 0 objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.keep\n> 108K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.bitmap\n> 524K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.idx\n> 4.6M objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.pack\n> 02d03::master > git repack -af\n> Enumerating objects: 19001, done.\n> Counting objects: 100% (19001/19001), done.\n> Delta compression using up to 2 threads\n> Compressing objects: 100% (18952/18952), done.\n> Writing objects: 100% (19001/19001), done.\n> warning: ignoring extra bitmap file:\n> ./objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.pack\n> warning: ignoring extra bitmap file:\n> ./objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.pack\n> warning: ignoring extra bitmap file:\n> ./objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.pack\n> Reusing bitmaps: 104, done.\n> Selecting bitmap commits: 2550, done.\n> Building bitmaps: 100% (130/130), done.\n> Total 19001 (delta 14837), reused 4162 (delta 0)\n> 02d03::master > find objects/pack/pack* -type f|xargs ls -sht\n> 108K objects/pack/pack-8702a2550b7e29940af8bc62bc6fca011ccbd455.bitmap\n> 524K objects/pack/pack-8702a2550b7e29940af8bc62bc6fca011ccbd455.idx\n> 4.6M objects/pack/pack-8702a2550b7e29940af8bc62bc6fca011ccbd455.pack <= ????\n> 108K objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.bitmap\n> 524K objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.idx\n> 4.7M objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.pack\n> 108K objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.bitmap\n> 524K objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.idx\n> 4.6M objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.pack\n> 116K objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.bitmap\n> 524K objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.idx\n> 4.6M objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.pack\n> 0 objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.keep\n> 108K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.bitmap\n> 524K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.idx\n> 4.6M objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.pack\n>\n> The ccbd455 pack and its metadata seem quite pointless to be\n> containing apparently all the data based on the size.\n>\n> If I use -ad, a new pack is still created,which, judging by the size,\n> is essentially everything again, (but at least the extra packs are\n> removed)\n>\n> 02d03::master> git repack -ad\n> Enumerating objects: 19001, done.\n> Counting objects: 100% (19001/19001), done.\n> Delta compression using up to 2 threads\n> Compressing objects: 100% (4114/4114), done.\n> Writing objects: 100% (19001/19001), done.\n> warning: ignoring extra bitmap file:\n> ./objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.pack\n> Reusing bitmaps: 104, done.\n> Selecting bitmap commits: 2550, done.\n> Building bitmaps: 100% (130/130), done.\n> Total 19001 (delta 14838), reused 19001 (delta 14838)\n> 02d03::master 9060> find objects/pack/pack* -type f|xargs ls -sht\n> 116K objects/pack/pack-46ab64716d4220aac8d53b380d90a264d5293d3d.bitmap\n> 524K objects/pack/pack-46ab64716d4220aac8d53b380d90a264d5293d3d.idx\n> 4.6M objects/pack/pack-46ab64716d4220aac8d53b380d90a264d5293d3d.pack <= ????\n> 0 objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.keep\n> 108K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.bitmap\n> 524K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.idx\n> 4.6M objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.pack\n>\n> Previously, the kept pack would be kept, and no additional packs would\n> be created if no new objects were born in the repro.\n>\n> With the .keep placeholder removed, the duplication does not happen,\n> but all the repro is rewritten into a new pack, which does not look\n> correct. Am I doing something unexpected?\n\nI haven't looked at this for more than a couple of minutes (and don't\nhave more time now), but this is almost certainly due to 36eba0323d\n(\"repack: enable bitmaps by default on bare repos\", 2019-03-14). Can you\nconfirm when you re-run with repack.writeBitmaps=false in the config?\n\nI.e. something in the \"yes I want bitmaps\" code implies \"*.keep\"\nsemantics changing from \"keep\" to \"replace\", which is obvious in\nretrospect, since we can only have one *.bitmap per-repo.\n"},{"id":"377826","messageId":"CAEXt3srf-9O8GbDEFwa5ELHRfwZ7LWYqqm6DxEjnegRUMcECnA@mail.gmail.com","threadId":"51366","inReplyTo":"875zow8i85.fsf@evledraar.gmail.com","subject":"Re: 2.22.0 repack -a duplicating pack contents","fromName":"Janos Farkas","fromEmail":"chexum@gmail.com","sentAt":"2019-06-23T15:38:46Z","receivedAt":"2019-06-23T15:39:00Z","isPatch":false,"sender":{"key":"chexum@gmail.com","avatar":null},"body":"Thanks for looking at this, yes, it seems it's purely the .bitmap\nwriting. With disabled repack.writeBitmaps, it all behaves as before\n(and yes, these are bare repros for keeping an intermediate backup).\n\nWith the easy workaround in place, I'm back at 2.22 for now, thanks!\n\nOn Sun, Jun 23, 2019 at 3:54 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n>\n> On Sun, Jun 23 2019, Janos Farkas wrote:\n>\n> > I'm using .keep files to... well.. keep packs to avoid some CPU time\n> > spent on repacking huge packs and make the process somewhat more\n> > incremental.\n> >\n> > Something changed with 22.2.0. Now .bitmap files are also created,\n> > and no simple repacks re-create the pack data in a completely new\n> > file, wasting quite some storage:\n> >\n> > 02d03::master> find objects/pack/pack* -type f|xargs ls -sht\n> > 108K objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.bitmap\n> > 524K objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.idx\n> > 4.7M objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.pack\n> > 108K objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.bitmap\n> > 524K objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.idx\n> > 4.6M objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.pack\n> > 116K objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.bitmap\n> > 524K objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.idx\n> > 4.6M objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.pack\n> > 0 objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.keep\n> > 108K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.bitmap\n> > 524K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.idx\n> > 4.6M objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.pack\n> > 02d03::master > git repack -af\n> > Enumerating objects: 19001, done.\n> > Counting objects: 100% (19001/19001), done.\n> > Delta compression using up to 2 threads\n> > Compressing objects: 100% (18952/18952), done.\n> > Writing objects: 100% (19001/19001), done.\n> > warning: ignoring extra bitmap file:\n> > ./objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.pack\n> > warning: ignoring extra bitmap file:\n> > ./objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.pack\n> > warning: ignoring extra bitmap file:\n> > ./objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.pack\n> > Reusing bitmaps: 104, done.\n> > Selecting bitmap commits: 2550, done.\n> > Building bitmaps: 100% (130/130), done.\n> > Total 19001 (delta 14837), reused 4162 (delta 0)\n> > 02d03::master > find objects/pack/pack* -type f|xargs ls -sht\n> > 108K objects/pack/pack-8702a2550b7e29940af8bc62bc6fca011ccbd455.bitmap\n> > 524K objects/pack/pack-8702a2550b7e29940af8bc62bc6fca011ccbd455.idx\n> > 4.6M objects/pack/pack-8702a2550b7e29940af8bc62bc6fca011ccbd455.pack <= ????\n> > 108K objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.bitmap\n> > 524K objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.idx\n> > 4.7M objects/pack/pack-879f2c28d15e57d353eb8e0ddbcb540655c844c9.pack\n> > 108K objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.bitmap\n> > 524K objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.idx\n> > 4.6M objects/pack/pack-e7a7aebfc6dc6b1431f6f56bb8b2f7e730cc4a0c.pack\n> > 116K objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.bitmap\n> > 524K objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.idx\n> > 4.6M objects/pack/pack-994c76cb1999e3b29552677d05e6364e6be2ae5e.pack\n> > 0 objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.keep\n> > 108K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.bitmap\n> > 524K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.idx\n> > 4.6M objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.pack\n> >\n> > The ccbd455 pack and its metadata seem quite pointless to be\n> > containing apparently all the data based on the size.\n> >\n> > If I use -ad, a new pack is still created,which, judging by the size,\n> > is essentially everything again, (but at least the extra packs are\n> > removed)\n> >\n> > 02d03::master> git repack -ad\n> > Enumerating objects: 19001, done.\n> > Counting objects: 100% (19001/19001), done.\n> > Delta compression using up to 2 threads\n> > Compressing objects: 100% (4114/4114), done.\n> > Writing objects: 100% (19001/19001), done.\n> > warning: ignoring extra bitmap file:\n> > ./objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.pack\n> > Reusing bitmaps: 104, done.\n> > Selecting bitmap commits: 2550, done.\n> > Building bitmaps: 100% (130/130), done.\n> > Total 19001 (delta 14838), reused 19001 (delta 14838)\n> > 02d03::master 9060> find objects/pack/pack* -type f|xargs ls -sht\n> > 116K objects/pack/pack-46ab64716d4220aac8d53b380d90a264d5293d3d.bitmap\n> > 524K objects/pack/pack-46ab64716d4220aac8d53b380d90a264d5293d3d.idx\n> > 4.6M objects/pack/pack-46ab64716d4220aac8d53b380d90a264d5293d3d.pack <= ????\n> > 0 objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.keep\n> > 108K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.bitmap\n> > 524K objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.idx\n> > 4.6M objects/pack/pack-e5b8848e7c1096274dba2430323ccaf5320c6846.pack\n> >\n> > Previously, the kept pack would be kept, and no additional packs would\n> > be created if no new objects were born in the repro.\n> >\n> > With the .keep placeholder removed, the duplication does not happen,\n> > but all the repro is rewritten into a new pack, which does not look\n> > correct. Am I doing something unexpected?\n>\n> I haven't looked at this for more than a couple of minutes (and don't\n> have more time now), but this is almost certainly due to 36eba0323d\n> (\"repack: enable bitmaps by default on bare repos\", 2019-03-14). Can you\n> confirm when you re-run with repack.writeBitmaps=false in the config?\n>\n> I.e. something in the \"yes I want bitmaps\" code implies \"*.keep\"\n> semantics changing from \"keep\" to \"replace\", which is obvious in\n> retrospect, since we can only have one *.bitmap per-repo.\n"},{"id":"377829","messageId":"20190623180226.GA1100@sigill.intra.peff.net","threadId":"51366","inReplyTo":"875zow8i85.fsf@evledraar.gmail.com","subject":"Re: 2.22.0 repack -a duplicating pack contents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-23T18:02:27Z","receivedAt":"2019-06-23T18:02:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 23, 2019 at 04:54:50PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> I haven't looked at this for more than a couple of minutes (and don't\n> have more time now), but this is almost certainly due to 36eba0323d\n> (\"repack: enable bitmaps by default on bare repos\", 2019-03-14). Can you\n> confirm when you re-run with repack.writeBitmaps=false in the config?\n> \n> I.e. something in the \"yes I want bitmaps\" code implies \"*.keep\"\n> semantics changing from \"keep\" to \"replace\", which is obvious in\n> retrospect, since we can only have one *.bitmap per-repo.\n\nYeah, the .keep behavior with bitmaps is intended, though it means a\nfunny implication for the bitmap-by-default strategy.\n\nBasically, you never want to have .keep files if you have bitmaps turned\non, and so the default for \"respect .keeps\" is based on whether bitmaps\nare in use. See ee34a2bead (repack: add `repack.packKeptObjects` config\nvar, 2014-03-03).\n\nI'm not sure of the right solution. For maximal backwards-compatibility,\nthe default for bitmaps could become \"if not bare and if there are no\n.keep files\". But that would mean bitmaps sometimes not getting\ngenerated because of the problems that ee34a2bead was trying to solve.\n\nThat's probably OK, though; you can always flip the bitmap config to\n\"true\" yourself if you _must_ have bitmaps.\n\n-Peff\n"},{"id":"377830","messageId":"20190623180825.3ospajjgat3clwiu@dcvr","threadId":"51366","inReplyTo":"20190623180226.GA1100@sigill.intra.peff.net","subject":"Re: 2.22.0 repack -a duplicating pack contents","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-06-23T18:08:25Z","receivedAt":"2019-06-23T18:08:27Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King wrote:\n> On Sun, Jun 23, 2019 at 04:54:50PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> > I haven't looked at this for more than a couple of minutes (and don't\n> > have more time now), but this is almost certainly due to 36eba0323d\n> > (\"repack: enable bitmaps by default on bare repos\", 2019-03-14). Can you\n> > confirm when you re-run with repack.writeBitmaps=false in the config?\n> > \n> > I.e. something in the \"yes I want bitmaps\" code implies \"*.keep\"\n> > semantics changing from \"keep\" to \"replace\", which is obvious in\n> > retrospect, since we can only have one *.bitmap per-repo.\n> \n> Yeah, the .keep behavior with bitmaps is intended, though it means a\n> funny implication for the bitmap-by-default strategy.\n> \n> Basically, you never want to have .keep files if you have bitmaps turned\n> on, and so the default for \"respect .keeps\" is based on whether bitmaps\n> are in use. See ee34a2bead (repack: add `repack.packKeptObjects` config\n> var, 2014-03-03).\n> \n> I'm not sure of the right solution. For maximal backwards-compatibility,\n> the default for bitmaps could become \"if not bare and if there are no\n> .keep files\". But that would mean bitmaps sometimes not getting\n> generated because of the problems that ee34a2bead was trying to solve.\n> \n> That's probably OK, though; you can always flip the bitmap config to\n> \"true\" yourself if you _must_ have bitmaps.\n\nWhat about something like this? Needs tests but I need to leave, now.\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex caca113927..1d99fb449b 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -89,6 +89,25 @@ static void remove_pack_on_signal(int signo)\n \traise(signo);\n }\n \n+static int has_pack_keep_file(void)\n+{\n+\tDIR *dir;\n+\tstruct dirent *e;\n+\tint found = 0;\n+\n+\tif (!(dir = opendir(packdir)))\n+\t\treturn found;\n+\n+\twhile ((e = readdir(dir)) != NULL) {\n+\t\tif (ends_with(e->d_name, \".keep\")) {\n+\t\t\tfound = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\tclosedir(dir);\n+\treturn found;\n+}\n+\n /*\n * Adds all packs hex strings to the fname list, which do not\n * have a corresponding .keep file. These packs are not to\n@@ -343,16 +362,20 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE)))\n \t\tdie(_(\"--keep-unreachable and -A are incompatible\"));\n \n-\tif (write_bitmaps < 0)\n+\tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n+\n+\tif (write_bitmaps < 0) {\n \t\twrite_bitmaps = (pack_everything & ALL_INTO_ONE) &&\n-\t\t\t\t is_bare_repository();\n+\t\t\t\t is_bare_repository() &&\n+\t\t\t\t keep_pack_list.nr == 0 &&\n+\t\t\t\t !has_pack_keep_file();\n+\t}\n \tif (pack_kept_objects < 0)\n \t\tpack_kept_objects = write_bitmaps;\n \n \tif (write_bitmaps && !(pack_everything & ALL_INTO_ONE))\n \t\tdie(_(incremental_bitmap_conflict_error));\n \n-\tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n \tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n \n \tsigchain_push_common(remove_pack_on_signal);\n"},{"id":"377841","messageId":"20190623224244.GB1100@sigill.intra.peff.net","threadId":"51366","inReplyTo":"20190623180825.3ospajjgat3clwiu@dcvr","subject":"Re: 2.22.0 repack -a duplicating pack contents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-23T22:42:45Z","receivedAt":"2019-06-24T02:42:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 23, 2019 at 06:08:25PM +0000, Eric Wong wrote:\n\n> > I'm not sure of the right solution. For maximal backwards-compatibility,\n> > the default for bitmaps could become \"if not bare and if there are no\n> > .keep files\". But that would mean bitmaps sometimes not getting\n> > generated because of the problems that ee34a2bead was trying to solve.\n> > \n> > That's probably OK, though; you can always flip the bitmap config to\n> > \"true\" yourself if you _must_ have bitmaps.\n> \n> What about something like this? Needs tests but I need to leave, now.\n\nYeah, I think that's the right direction.\n\nThough...\n\n> +static int has_pack_keep_file(void)\n> +{\n> +\tDIR *dir;\n> +\tstruct dirent *e;\n> +\tint found = 0;\n> +\n> +\tif (!(dir = opendir(packdir)))\n> +\t\treturn found;\n> +\n> +\twhile ((e = readdir(dir)) != NULL) {\n> +\t\tif (ends_with(e->d_name, \".keep\")) {\n> +\t\t\tfound = 1;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\tclosedir(dir);\n> +\treturn found;\n> +}\n\nI think this can be replaced with just checking p->pack_keep for each\nitem in the packed_git list.\n\nThat's racy, but then so is your code here, since it's really the child\npack-objects which is going to deal with the .keep. I don't think we\nneed to care much about the race, though. Either:\n\n 1. Somebody has made an old intentional .keep, which would not be\n racy. We'd see it in both places.\n\n 2. Somebody _just_ made an intentional .keep; we'll race with that and\n maybe duplicate objects from the kept pack. But this is a rare\n occurrence, and there's no real ordering promise here anyway with\n somebody creating .keep files alongside a running repack.\n\n 3. An incoming fetch/push may create a .keep file as a temporary lock,\n which we see here but which goes away by the time pack-objects\n runs. That's OK; we err on the side of not generating bitmaps, but\n they're an optimization anyway (and if you really insist on having\n them, you should tell Git to definitely make them instead of\n relying on this default behavior).\n\n 4. Like (3), but we _don't _see the temporary .keep here but _do_ see\n it during pack-objects. That's OK, because we'll have told\n pack-objects to pack those objects anyway, which is the right\n thing.\n\n-Peff\n"},{"id":"377845","messageId":"874l4f8h4c.fsf@evledraar.gmail.com","threadId":"51366","inReplyTo":"20190623224244.GB1100@sigill.intra.peff.net","subject":"Re: 2.22.0 repack -a duplicating pack contents","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-06-24T09:30:59Z","receivedAt":"2019-06-24T09:31:04Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 24 2019, Jeff King wrote:\n\n> On Sun, Jun 23, 2019 at 06:08:25PM +0000, Eric Wong wrote:\n>\n>> > I'm not sure of the right solution. For maximal backwards-compatibility,\n>> > the default for bitmaps could become \"if not bare and if there are no\n>> > .keep files\". But that would mean bitmaps sometimes not getting\n>> > generated because of the problems that ee34a2bead was trying to solve.\n>> >\n>> > That's probably OK, though; you can always flip the bitmap config to\n>> > \"true\" yourself if you _must_ have bitmaps.\n>>\n>> What about something like this? Needs tests but I need to leave, now.\n>\n> Yeah, I think that's the right direction.\n>\n> Though...\n>\n>> +static int has_pack_keep_file(void)\n>> +{\n>> +\tDIR *dir;\n>> +\tstruct dirent *e;\n>> +\tint found = 0;\n>> +\n>> +\tif (!(dir = opendir(packdir)))\n>> +\t\treturn found;\n>> +\n>> +\twhile ((e = readdir(dir)) != NULL) {\n>> +\t\tif (ends_with(e->d_name, \".keep\")) {\n>> +\t\t\tfound = 1;\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t}\n>> +\tclosedir(dir);\n>> +\treturn found;\n>> +}\n>\n> I think this can be replaced with just checking p->pack_keep for each\n> item in the packed_git list.\n>\n> That's racy, but then so is your code here, since it's really the child\n> pack-objects which is going to deal with the .keep. I don't think we\n> need to care much about the race, though. Either:\n>\n> 1. Somebody has made an old intentional .keep, which would not be\n> racy. We'd see it in both places.\n>\n> 2. Somebody _just_ made an intentional .keep; we'll race with that and\n> maybe duplicate objects from the kept pack. But this is a rare\n> occurrence, and there's no real ordering promise here anyway with\n> somebody creating .keep files alongside a running repack.\n>\n> 3. An incoming fetch/push may create a .keep file as a temporary lock,\n> which we see here but which goes away by the time pack-objects\n> runs. That's OK; we err on the side of not generating bitmaps, but\n> they're an optimization anyway (and if you really insist on having\n> them, you should tell Git to definitely make them instead of\n> relying on this default behavior).\n\nThis sort of thing (#3) strikes me as a fairly pathological case we\nshould try to avoid. Now what we've turned on bitmaps by default people\nwill take the sort of performance increase noted in [1] for granted.\n\nSo they'll be happily running with that & then get a CPU/IO spike as the\n*.bitmap files they'd been implicitly relying on for years in their\ndefault config goes away, only to have it re-appear when \"repack\" runs\nnext.\n\nI can't think of some great solution for this case, some thoughts:\n\n a. Perhaps we should split the *.keep flag into two things or\n more.\n\n We're using it for all of \"I want this *.pack forever\"\n (e.g. debugging) and \"I want only this *.pack to contain the data\n found in it\" (I/O & CPU optimization, what Janos wants) and \"I'm\n git.git code avoiding a race with myself\" (what you describe in #3).\n\n So maybe for the last of those we could also use and understand\n *.tmp-keep, at which point we wouldn't have this race described in\n #3. The 1st of those is a *.noprune and the 2nd is *.highlander (but\n whether it's worth splitting all that out v.s. just having\n *.tmp-keep is another matter).\n\n b) Shouldn't we at least print some warning to STDERR in this case so\n e.g. gc.log will note the performance degradation of the repo in its\n current configuration?\n\n> 4. Like (3), but we _don't _see the temporary .keep here but _do_ see\n> it during pack-objects. That's OK, because we'll have told\n> pack-objects to pack those objects anyway, which is the right\n> thing.\n>\n> -Peff\n\n1. https://github.blog/2015-09-22-counting-objects/\n"},{"id":"378240","messageId":"20190628070211.hfweqcons6c6gy52@dcvr","threadId":"51366","inReplyTo":"20190623224244.GB1100@sigill.intra.peff.net","subject":"[PATCH] repack: disable bitmaps-by-default if .keep files exist","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-06-28T07:02:11Z","receivedAt":"2019-06-28T07:02:14Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King wrote:\n> On Sun, Jun 23, 2019 at 06:08:25PM +0000, Eric Wong wrote:\n> \n> > > I'm not sure of the right solution. For maximal backwards-compatibility,\n> > > the default for bitmaps could become \"if not bare and if there are no\n> > > .keep files\". But that would mean bitmaps sometimes not getting\n> > > generated because of the problems that ee34a2bead was trying to solve.\n> > > \n> > > That's probably OK, though; you can always flip the bitmap config to\n> > > \"true\" yourself if you _must_ have bitmaps.\n> > \n> > What about something like this? Needs tests but I need to leave, now.\n> \n> Yeah, I think that's the right direction.\n\nOK. I have a real patch with one additional test, below.\n(don't have a lot of time for hacking)\n\n> Though...\n> \n> > +static int has_pack_keep_file(void)\n> > +{\n> > +\tDIR *dir;\n> > +\tstruct dirent *e;\n> > +\tint found = 0;\n> > +\n> > +\tif (!(dir = opendir(packdir)))\n> > +\t\treturn found;\n> > +\n> > +\twhile ((e = readdir(dir)) != NULL) {\n> > +\t\tif (ends_with(e->d_name, \".keep\")) {\n> > +\t\t\tfound = 1;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\tclosedir(dir);\n> > +\treturn found;\n> > +}\n> \n> I think this can be replaced with just checking p->pack_keep for each\n> item in the packed_git list.\n\nGood point, I tend to forget git C API internals as soon as I\nlearn them :x\n\n> That's racy, but then so is your code here, since it's really the child\n> pack-objects which is going to deal with the .keep. I don't think we\n> need to care much about the race, though. Either:\n\nAgreed. \n\n--------8<-------\nSubject: [PATCH] repack: disable bitmaps-by-default if .keep files exist\n\nBitmaps aren't useful with multiple packs, and users with\n.keep files ended up with redundant packs when bitmaps\ngot enabled by default in bare repos.\n\nSo detect when .keep files exist and stop enabling bitmaps\nby default in that case.\n\nWasteful (but otherwise harmless) race conditions with .keep files\ndocumented by Jeff King still apply and there's a chance we'd\nstill end up with redundant data on the FS:\n\n https://public-inbox.org/git/20190623224244.GB1100@sigill.intra.peff.net/\n\nFixes: 36eba0323d3288a8 (\"repack: enable bitmaps by default on bare repos\")\nSigned-off-by: Eric Wong \nHelped-by: Jeff King \nReported-by: Janos Farkas \n---\n builtin/repack.c | 18 ++++++++++++++++--\n t/t7700-repack.sh | 10 ++++++++++\n 2 files changed, 26 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex caca113927..a9529d1afc 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -89,6 +89,17 @@ static void remove_pack_on_signal(int signo)\n \traise(signo);\n }\n \n+static int has_pack_keep_file(void)\n+{\n+\tstruct packed_git *p;\n+\n+\tfor (p = get_packed_git(the_repository); p; p = p->next) {\n+\t\tif (p->pack_keep)\n+\t\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n /*\n * Adds all packs hex strings to the fname list, which do not\n * have a corresponding .keep file. These packs are not to\n@@ -343,9 +354,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE)))\n \t\tdie(_(\"--keep-unreachable and -A are incompatible\"));\n \n-\tif (write_bitmaps < 0)\n+\tif (write_bitmaps < 0) {\n \t\twrite_bitmaps = (pack_everything & ALL_INTO_ONE) &&\n-\t\t\t\t is_bare_repository();\n+\t\t\t\t is_bare_repository() &&\n+\t\t\t\t keep_pack_list.nr == 0 &&\n+\t\t\t\t !has_pack_keep_file();\n+\t}\n \tif (pack_kept_objects < 0)\n \t\tpack_kept_objects = write_bitmaps;\n \ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 86d05160a3..0acde3b1f8 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -239,4 +239,14 @@ test_expect_success 'bitmaps can be disabled on bare repos' '\n \ttest -z \"$bitmap\"\n '\n \n+test_expect_success 'no bitmaps created if .keep files present' '\n+\tpack=$(ls bare.git/objects/pack/*.pack) &&\n+\ttest_path_is_file \"$pack\" &&\n+\tkeep=${pack%.pack}.keep &&\n+\t>\"$keep\" &&\n+\tgit -C bare.git repack -ad &&\n+\tbitmap=$(ls bare.git/objects/pack/*.bitmap 2>/dev/null || :) &&\n+\ttest -z \"$bitmap\"\n+'\n+\n test_done\n-- \nEW\n"},{"id":"378241","messageId":"87zhm26uq9.fsf@evledraar.gmail.com","threadId":"51366","inReplyTo":"20190628070211.hfweqcons6c6gy52@dcvr","subject":"Re: [PATCH] repack: disable bitmaps-by-default if .keep files exist","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-06-28T07:21:18Z","receivedAt":"2019-06-28T07:21:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jun 28 2019, Eric Wong wrote:\n\n> Jeff King wrote:\n>> On Sun, Jun 23, 2019 at 06:08:25PM +0000, Eric Wong wrote:\n>>\n>> > > I'm not sure of the right solution. For maximal backwards-compatibility,\n>> > > the default for bitmaps could become \"if not bare and if there are no\n>> > > .keep files\". But that would mean bitmaps sometimes not getting\n>> > > generated because of the problems that ee34a2bead was trying to solve.\n>> > >\n>> > > That's probably OK, though; you can always flip the bitmap config to\n>> > > \"true\" yourself if you _must_ have bitmaps.\n>> >\n>> > What about something like this? Needs tests but I need to leave, now.\n>>\n>> Yeah, I think that's the right direction.\n>\n> OK. I have a real patch with one additional test, below.\n> (don't have a lot of time for hacking)\n>\n>> Though...\n>>\n>> > +static int has_pack_keep_file(void)\n>> > +{\n>> > +\tDIR *dir;\n>> > +\tstruct dirent *e;\n>> > +\tint found = 0;\n>> > +\n>> > +\tif (!(dir = opendir(packdir)))\n>> > +\t\treturn found;\n>> > +\n>> > +\twhile ((e = readdir(dir)) != NULL) {\n>> > +\t\tif (ends_with(e->d_name, \".keep\")) {\n>> > +\t\t\tfound = 1;\n>> > +\t\t\tbreak;\n>> > +\t\t}\n>> > +\t}\n>> > +\tclosedir(dir);\n>> > +\treturn found;\n>> > +}\n>>\n>> I think this can be replaced with just checking p->pack_keep for each\n>> item in the packed_git list.\n>\n> Good point, I tend to forget git C API internals as soon as I\n> learn them :x\n>\n>> That's racy, but then so is your code here, since it's really the child\n>> pack-objects which is going to deal with the .keep. I don't think we\n>> need to care much about the race, though. Either:\n>\n> Agreed. \n>\n> --------8<-------\n> Subject: [PATCH] repack: disable bitmaps-by-default if .keep files exist\n>\n> Bitmaps aren't useful with multiple packs, and users with\n> .keep files ended up with redundant packs when bitmaps\n> got enabled by default in bare repos.\n>\n> So detect when .keep files exist and stop enabling bitmaps\n> by default in that case.\n>\n> Wasteful (but otherwise harmless) race conditions with .keep files\n> documented by Jeff King still apply and there's a chance we'd\n> still end up with redundant data on the FS:\n>\n> https://public-inbox.org/git/20190623224244.GB1100@sigill.intra.peff.net/\n>\n> Fixes: 36eba0323d3288a8 (\"repack: enable bitmaps by default on bare repos\")\n> Signed-off-by: Eric Wong \n> Helped-by: Jeff King \n> Reported-by: Janos Farkas \n> ---\n> builtin/repack.c | 18 ++++++++++++++++--\n> t/t7700-repack.sh | 10 ++++++++++\n> 2 files changed, 26 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index caca113927..a9529d1afc 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -89,6 +89,17 @@ static void remove_pack_on_signal(int signo)\n> \traise(signo);\n> }\n>\n> +static int has_pack_keep_file(void)\n> +{\n> +\tstruct packed_git *p;\n> +\n> +\tfor (p = get_packed_git(the_repository); p; p = p->next) {\n> +\t\tif (p->pack_keep)\n> +\t\t\treturn 1;\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> /*\n> * Adds all packs hex strings to the fname list, which do not\n> * have a corresponding .keep file. These packs are not to\n> @@ -343,9 +354,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE)))\n> \t\tdie(_(\"--keep-unreachable and -A are incompatible\"));\n>\n> -\tif (write_bitmaps < 0)\n> +\tif (write_bitmaps < 0) {\n> \t\twrite_bitmaps = (pack_everything & ALL_INTO_ONE) &&\n> -\t\t\t\t is_bare_repository();\n> +\t\t\t\t is_bare_repository() &&\n> +\t\t\t\t keep_pack_list.nr == 0 &&\n> +\t\t\t\t !has_pack_keep_file();\n> +\t}\n> \tif (pack_kept_objects < 0)\n> \t\tpack_kept_objects = write_bitmaps;\n>\n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index 86d05160a3..0acde3b1f8 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -239,4 +239,14 @@ test_expect_success 'bitmaps can be disabled on bare repos' '\n> \ttest -z \"$bitmap\"\n> '\n\nI have the feedback I posted before this patch in\nhttps://public-inbox.org/git/874l4f8h4c.fsf@evledraar.gmail.com/\n\nIn particular \"b\" there since \"a\" is clearly more work. I.e. shouldn't\nwe at least in interactive mode on a \"gc\" print something about skipping\nwhat we'd otherwise do.\n\nMaybe that's tricky with the gc.log functionality, but I think we should\nat least document this before the next guy shows up with \"sometimes my\n.bitmap files aren't generated...\".\n\n\n> +test_expect_success 'no bitmaps created if .keep files present' '\n> +\tpack=$(ls bare.git/objects/pack/*.pack) &&\n> +\ttest_path_is_file \"$pack\" &&\n> +\tkeep=${pack%.pack}.keep &&\n> +\t>\"$keep\" &&\n> +\tgit -C bare.git repack -ad &&\n> +\tbitmap=$(ls bare.git/objects/pack/*.bitmap 2>/dev/null || :) &&\n> +\ttest -z \"$bitmap\"\n\nMaybe more readable as:\n\n\tfind .git/objects/pack/ -type f -name '*.bitmap' >actual &&\n\ttest_must_be_empty actual\n\nI.e. it avoids the \"|| :\" & subshell.\n\n> +'\n> +\n> test_done\n"},{"id":"378321","messageId":"20190629080302.GD21574@szeder.dev","threadId":"51366","inReplyTo":"20190628070211.hfweqcons6c6gy52@dcvr","subject":"Re: [PATCH] repack: disable bitmaps-by-default if .keep files exist","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-29T08:03:02Z","receivedAt":"2019-06-29T08:03:09Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jun 28, 2019 at 07:02:11AM +0000, Eric Wong wrote:\n> Jeff King wrote:\n> > On Sun, Jun 23, 2019 at 06:08:25PM +0000, Eric Wong wrote:\n> > \n> > > > I'm not sure of the right solution. For maximal backwards-compatibility,\n> > > > the default for bitmaps could become \"if not bare and if there are no\n> > > > .keep files\". But that would mean bitmaps sometimes not getting\n> > > > generated because of the problems that ee34a2bead was trying to solve.\n> > > > \n> > > > That's probably OK, though; you can always flip the bitmap config to\n> > > > \"true\" yourself if you _must_ have bitmaps.\n> > > \n> > > What about something like this? Needs tests but I need to leave, now.\n> > \n> > Yeah, I think that's the right direction.\n> \n> OK. I have a real patch with one additional test, below.\n> (don't have a lot of time for hacking)\n> \n> > Though...\n> > \n> > > +static int has_pack_keep_file(void)\n> > > +{\n> > > +\tDIR *dir;\n> > > +\tstruct dirent *e;\n> > > +\tint found = 0;\n> > > +\n> > > +\tif (!(dir = opendir(packdir)))\n> > > +\t\treturn found;\n> > > +\n> > > +\twhile ((e = readdir(dir)) != NULL) {\n> > > +\t\tif (ends_with(e->d_name, \".keep\")) {\n> > > +\t\t\tfound = 1;\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t}\n> > > +\tclosedir(dir);\n> > > +\treturn found;\n> > > +}\n> > \n> > I think this can be replaced with just checking p->pack_keep for each\n> > item in the packed_git list.\n> \n> Good point, I tend to forget git C API internals as soon as I\n> learn them :x\n> \n> > That's racy, but then so is your code here, since it's really the child\n> > pack-objects which is going to deal with the .keep. I don't think we\n> > need to care much about the race, though. Either:\n> \n> Agreed. \n> \n> --------8<-------\n> Subject: [PATCH] repack: disable bitmaps-by-default if .keep files exist\n> \n> Bitmaps aren't useful with multiple packs, and users with\n> .keep files ended up with redundant packs when bitmaps\n> got enabled by default in bare repos.\n> \n> So detect when .keep files exist and stop enabling bitmaps\n> by default in that case.\n> \n> Wasteful (but otherwise harmless) race conditions with .keep files\n> documented by Jeff King still apply and there's a chance we'd\n> still end up with redundant data on the FS:\n> \n> https://public-inbox.org/git/20190623224244.GB1100@sigill.intra.peff.net/\n> \n> Fixes: 36eba0323d3288a8 (\"repack: enable bitmaps by default on bare repos\")\n> Signed-off-by: Eric Wong \n> Helped-by: Jeff King \n> Reported-by: Janos Farkas \n> ---\n> builtin/repack.c | 18 ++++++++++++++++--\n> t/t7700-repack.sh | 10 ++++++++++\n> 2 files changed, 26 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index caca113927..a9529d1afc 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -89,6 +89,17 @@ static void remove_pack_on_signal(int signo)\n> \traise(signo);\n> }\n> \n> +static int has_pack_keep_file(void)\n> +{\n> +\tstruct packed_git *p;\n> +\n> +\tfor (p = get_packed_git(the_repository); p; p = p->next) {\n> +\t\tif (p->pack_keep)\n> +\t\t\treturn 1;\n> +\t}\n> +\treturn 0;\n> +}\n> +\n> /*\n> * Adds all packs hex strings to the fname list, which do not\n> * have a corresponding .keep file. These packs are not to\n> @@ -343,9 +354,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE)))\n> \t\tdie(_(\"--keep-unreachable and -A are incompatible\"));\n> \n> -\tif (write_bitmaps < 0)\n> +\tif (write_bitmaps < 0) {\n> \t\twrite_bitmaps = (pack_everything & ALL_INTO_ONE) &&\n> -\t\t\t\t is_bare_repository();\n> +\t\t\t\t is_bare_repository() &&\n> +\t\t\t\t keep_pack_list.nr == 0 &&\n> +\t\t\t\t !has_pack_keep_file();\n> +\t}\n> \tif (pack_kept_objects < 0)\n> \t\tpack_kept_objects = write_bitmaps;\n> \n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index 86d05160a3..0acde3b1f8 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -239,4 +239,14 @@ test_expect_success 'bitmaps can be disabled on bare repos' '\n> \ttest -z \"$bitmap\"\n> '\n> \n> +test_expect_success 'no bitmaps created if .keep files present' '\n> +\tpack=$(ls bare.git/objects/pack/*.pack) &&\n> +\ttest_path_is_file \"$pack\" &&\n> +\tkeep=${pack%.pack}.keep &&\n> +\t>\"$keep\" &&\n> +\tgit -C bare.git repack -ad &&\n> +\tbitmap=$(ls bare.git/objects/pack/*.bitmap 2>/dev/null || :) &&\n> +\ttest -z \"$bitmap\"\n\nThis test fails when run with 'GIT_TEST_MULTI_PACK_INDEX=1':\n\n + ls bare.git/objects/pack/pack-51a6a758692f03f9e1f31fd087bba30584afad2f.pack\n + pack=bare.git/objects/pack/pack-51a6a758692f03f9e1f31fd087bba30584afad2f.pack\n + test_path_is_file bare.git/objects/pack/pack-51a6a758692f03f9e1f31fd087bba30584afad2f.pack\n + test -f bare.git/objects/pack/pack-51a6a758692f03f9e1f31fd087bba30584afad2f.pack\n + keep=bare.git/objects/pack/pack-51a6a758692f03f9e1f31fd087bba30584afad2f.keep\n +\n + git -C bare.git repack -ad\n + ls bare.git/objects/pack/pack-51a6a758692f03f9e1f31fd087bba30584afad2f.bitmap\n + bitmap=bare.git/objects/pack/pack-51a6a758692f03f9e1f31fd087bba30584afad2f.bitmap\n + test -z bare.git/objects/pack/pack-51a6a758692f03f9e1f31fd087bba30584afad2f.bitmap\n error: last command exited with $?=1\n not ok 15 - no bitmaps created if .keep files present\n\nWhen the new has_pack_keep_file() helper function calls\nget_packed_git(the_repository) in the loop it returns with NULL\nalready on the first iteration, so the keep file goes unnoticed.\n\n> +'\n> +\n> test_done\n> -- \n> EW\n"},{"id":"378332","messageId":"20190629191359.x2bzcg6ksb2i4xtm@dcvr","threadId":"51366","inReplyTo":"20190629080302.GD21574@szeder.dev","subject":"[PATCH v2] repack: disable bitmaps-by-default if .keep files exist","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-06-29T19:13:59Z","receivedAt":"2019-06-29T19:14:01Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"SZEDER Gábor wrote:\n> On Fri, Jun 28, 2019 at 07:02:11AM +0000, Eric Wong wrote:\n> > diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> > index 86d05160a3..0acde3b1f8 100755\n> > --- a/t/t7700-repack.sh\n> > +++ b/t/t7700-repack.sh\n> > @@ -239,4 +239,14 @@ test_expect_success 'bitmaps can be disabled on bare repos' '\n> > \ttest -z \"$bitmap\"\n> > '\n> > \n> > +test_expect_success 'no bitmaps created if .keep files present' '\n> > +\tpack=$(ls bare.git/objects/pack/*.pack) &&\n> > +\ttest_path_is_file \"$pack\" &&\n> > +\tkeep=${pack%.pack}.keep &&\n> > +\t>\"$keep\" &&\n> > +\tgit -C bare.git repack -ad &&\n> > +\tbitmap=$(ls bare.git/objects/pack/*.bitmap 2>/dev/null || :) &&\n> > +\ttest -z \"$bitmap\"\n> \n> This test fails when run with 'GIT_TEST_MULTI_PACK_INDEX=1':\n> \n> + ls bare.git/objects/pack/pack-51a6a758692f03f9e1f31fd087bba30584afad2f.pack\n> + pack=bare.git/objects/pack/pack-51a6a758692f03f9e1f31fd087bba30584afad2f.pack\n> + test_path_is_file bare.git/objects/pack/pack-51a6a758692f03f9e1f31fd087bba30584afad2f.pack\n> + test -f bare.git/objects/pack/pack-51a6a758692f03f9e1f31fd087bba30584afad2f.pack\n> + keep=bare.git/objects/pack/pack-51a6a758692f03f9e1f31fd087bba30584afad2f.keep\n> +\n> + git -C bare.git repack -ad\n> + ls bare.git/objects/pack/pack-51a6a758692f03f9e1f31fd087bba30584afad2f.bitmap\n> + bitmap=bare.git/objects/pack/pack-51a6a758692f03f9e1f31fd087bba30584afad2f.bitmap\n> + test -z bare.git/objects/pack/pack-51a6a758692f03f9e1f31fd087bba30584afad2f.bitmap\n> error: last command exited with $?=1\n> not ok 15 - no bitmaps created if .keep files present\n> \n> When the new has_pack_keep_file() helper function calls\n> get_packed_git(the_repository) in the loop it returns with NULL\n> already on the first iteration, so the keep file goes unnoticed.\n\nOops, s/get_packed_git/get_all_packs/ seems to fix it.\n\nThe following also improves readability as suggested by Ævar\nby avoiding a subshell.\n-------8<---------\nSubject: [PATCH] repack: disable bitmaps-by-default if .keep files exist\n\nBitmaps aren't useful with multiple packs, and users with\n.keep files ended up with redundant packs when bitmaps\ngot enabled by default in bare repos.\n\nSo detect when .keep files exist and stop enabling bitmaps\nby default in that case.\n\nWasteful (but otherwise harmless) race conditions with .keep files\ndocumented by Jeff King still apply and there's a chance we'd\nstill end up with redundant data on the FS:\n\n https://public-inbox.org/git/20190623224244.GB1100@sigill.intra.peff.net/\n\nv2: avoid subshell in test case, be multi-index aware\n\nFixes: 36eba0323d3288a8 (\"repack: enable bitmaps by default on bare repos\")\nSigned-off-by: Eric Wong \nHelped-by: Jeff King \nReported-by: Janos Farkas \n---\nInterdiff:\n diff --git a/builtin/repack.c b/builtin/repack.c\n index a9529d1afc..73250b2431 100644\n --- a/builtin/repack.c\n +++ b/builtin/repack.c\n @@ -93,7 +93,7 @@ static int has_pack_keep_file(void)\n {\n \tstruct packed_git *p;\n \n -\tfor (p = get_packed_git(the_repository); p; p = p->next) {\n +\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\tif (p->pack_keep)\n \t\t\treturn 1;\n \t}\n diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n index 0acde3b1f8..0e9af832c9 100755\n --- a/t/t7700-repack.sh\n +++ b/t/t7700-repack.sh\n @@ -245,8 +245,8 @@ test_expect_success 'no bitmaps created if .keep files present' '\n \tkeep=${pack%.pack}.keep &&\n \t>\"$keep\" &&\n \tgit -C bare.git repack -ad &&\n -\tbitmap=$(ls bare.git/objects/pack/*.bitmap 2>/dev/null || :) &&\n -\ttest -z \"$bitmap\"\n +\tfind bare.git/objects/pack/ -type f -name \"*.bitmap\" >actual &&\n +\ttest_must_be_empty actual\n '\n \n test_done\n\n builtin/repack.c | 18 ++++++++++++++++--\n t/t7700-repack.sh | 10 ++++++++++\n 2 files changed, 26 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex caca113927..73250b2431 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -89,6 +89,17 @@ static void remove_pack_on_signal(int signo)\n \traise(signo);\n }\n \n+static int has_pack_keep_file(void)\n+{\n+\tstruct packed_git *p;\n+\n+\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n+\t\tif (p->pack_keep)\n+\t\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n /*\n * Adds all packs hex strings to the fname list, which do not\n * have a corresponding .keep file. These packs are not to\n@@ -343,9 +354,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE)))\n \t\tdie(_(\"--keep-unreachable and -A are incompatible\"));\n \n-\tif (write_bitmaps < 0)\n+\tif (write_bitmaps < 0) {\n \t\twrite_bitmaps = (pack_everything & ALL_INTO_ONE) &&\n-\t\t\t\t is_bare_repository();\n+\t\t\t\t is_bare_repository() &&\n+\t\t\t\t keep_pack_list.nr == 0 &&\n+\t\t\t\t !has_pack_keep_file();\n+\t}\n \tif (pack_kept_objects < 0)\n \t\tpack_kept_objects = write_bitmaps;\n \ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 86d05160a3..0e9af832c9 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -239,4 +239,14 @@ test_expect_success 'bitmaps can be disabled on bare repos' '\n \ttest -z \"$bitmap\"\n '\n \n+test_expect_success 'no bitmaps created if .keep files present' '\n+\tpack=$(ls bare.git/objects/pack/*.pack) &&\n+\ttest_path_is_file \"$pack\" &&\n+\tkeep=${pack%.pack}.keep &&\n+\t>\"$keep\" &&\n+\tgit -C bare.git repack -ad &&\n+\tfind bare.git/objects/pack/ -type f -name \"*.bitmap\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n-- \nEW\n"},{"id":"378333","messageId":"20190629191600.nipp2ut37xd3mx56@dcvr","threadId":"51366","inReplyTo":"87zhm26uq9.fsf@evledraar.gmail.com","subject":"[PATCH 2/1] repack: warn if bitmaps are explicitly enabled with keep files","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2019-06-29T19:16:00Z","receivedAt":"2019-06-29T19:16:02Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Ævar Arnfjörð Bjarmason wrote:\n> I have the feedback I posted before this patch in\n> https://public-inbox.org/git/874l4f8h4c.fsf@evledraar.gmail.com/\n> \n> In particular \"b\" there since \"a\" is clearly more work. I.e. shouldn't\n> we at least in interactive mode on a \"gc\" print something about skipping\n> what we'd otherwise do.\n> \n> Maybe that's tricky with the gc.log functionality, but I think we should\n> at least document this before the next guy shows up with \"sometimes my\n> .bitmap files aren't generated...\".\n\nI'm not sure if the warning should be present by default;\nbecause we'll silently stop using bitmaps, now. But warning\nif '-b' is specified seems right:\n\n-------8<----------\nSubject: [PATCH] repack: warn if bitmaps are explicitly enabled with keep\n files\n\nIf a user explicitly enables bitmaps, we should warn if .keep\nfiles exist or are specified via --keep-pack\n\nSigned-off-by: Eric Wong \n---\n builtin/repack.c | 8 ++++++++\n t/t7700-repack.sh | 16 ++++++++++++++++\n 2 files changed, 24 insertions(+)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 73250b2431..b1eeee88a7 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -359,7 +359,15 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\t is_bare_repository() &&\n \t\t\t\t keep_pack_list.nr == 0 &&\n \t\t\t\t !has_pack_keep_file();\n+\t} else if (write_bitmaps > 0) {\n+\t\tif (keep_pack_list.nr)\n+\t\t\tfprintf(stderr,\n+\t\t\t\t_(\"WARNING: --keep-pack is incompatible with bitmaps\\n\"));\n+\t\tif (has_pack_keep_file())\n+\t\t\tfprintf(stderr,\n+\t\t\t\t_(\"WARNING: .keep files are incompatible with bitmaps\\n\"));\n \t}\n+\n \tif (pack_kept_objects < 0)\n \t\tpack_kept_objects = write_bitmaps;\n \ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 0e9af832c9..839484c7dc 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -249,4 +249,20 @@ test_expect_success 'no bitmaps created if .keep files present' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success '-b warns with .keep files present' '\n+\tpack=$(ls bare.git/objects/pack/*.pack) &&\n+\ttest_path_is_file \"$pack\" &&\n+\tkeep=${pack%.pack}.keep &&\n+\t>\"$keep\" &&\n+\tgit -C bare.git repack -adb 2>err &&\n+\ttest_i18ngrep -F \".keep files are incompatible\" err &&\n+\trm -f \"$keep\"\n+'\n+\n+test_expect_success '-b warns with --keep-pack specified' '\n+\tkeep=$(cd bare.git/objects/pack/ && ls *.pack) &&\n+\tgit -C bare.git repack -adb --keep-pack=\"$keep\" 2>err &&\n+\ttest_i18ngrep -F \"keep-pack is incompatible\" err\n+'\n+\n test_done\n-- \nEW\n"},{"id":"378420","messageId":"xmqq7e917h9x.fsf@gitster-ct.c.googlers.com","threadId":"51366","inReplyTo":"20190629191600.nipp2ut37xd3mx56@dcvr","subject":"Re: [PATCH 2/1] repack: warn if bitmaps are explicitly enabled with keep files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-01T18:15:38Z","receivedAt":"2019-07-01T18:15:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> Ævar Arnfjörð Bjarmason wrote:\n>> I have the feedback I posted before this patch in\n>> https://public-inbox.org/git/874l4f8h4c.fsf@evledraar.gmail.com/\n>> \n>> In particular \"b\" there since \"a\" is clearly more work. I.e. shouldn't\n>> we at least in interactive mode on a \"gc\" print something about skipping\n>> what we'd otherwise do.\n>> \n>> Maybe that's tricky with the gc.log functionality, but I think we should\n>> at least document this before the next guy shows up with \"sometimes my\n>> .bitmap files aren't generated...\".\n>\n> I'm not sure if the warning should be present by default;\n> because we'll silently stop using bitmaps, now. But warning\n> if '-b' is specified seems right:\n\nHmph... write_bitmaps can come either from the command line or from\nthe configuration. When repack.writebitmaps configuration is set,\nand some .keep files exist, the user probably is not even aware of\ndoing something that is unsupported.\n\nThe keep_pack_list will not be empty only if the \"--keep-pack\"\noption is explicitly given from the command line, so making it and\nan explicit \"-b\" on the command line a condition to trigger the\nwarning (or even \"fatal: can't do both at the same time\") is safe\nenough, I think. But anything more than that, I am not so sure.\n\nAlso, is it safe to unconditionally emit strings to the standard\nerror stream from the gc codepath these days? I recall that once we\nhad trouble with the logic to skip repeatedly failing \"gc --auto\"\nvaguely, but maybe I am remembering something else.\n\nThanks.\n\n\n> -------8<----------\n> Subject: [PATCH] repack: warn if bitmaps are explicitly enabled with keep\n> files\n>\n> If a user explicitly enables bitmaps, we should warn if .keep\n> files exist or are specified via --keep-pack\n>\n> Signed-off-by: Eric Wong \n> ---\n> builtin/repack.c | 8 ++++++++\n> t/t7700-repack.sh | 16 ++++++++++++++++\n> 2 files changed, 24 insertions(+)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 73250b2431..b1eeee88a7 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -359,7 +359,15 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \t\t\t\t is_bare_repository() &&\n> \t\t\t\t keep_pack_list.nr == 0 &&\n> \t\t\t\t !has_pack_keep_file();\n> +\t} else if (write_bitmaps > 0) {\n> +\t\tif (keep_pack_list.nr)\n> +\t\t\tfprintf(stderr,\n> +\t\t\t\t_(\"WARNING: --keep-pack is incompatible with bitmaps\\n\"));\n\n\n\n> +\t\tif (has_pack_keep_file())\n> +\t\t\tfprintf(stderr,\n> +\t\t\t\t_(\"WARNING: .keep files are incompatible with bitmaps\\n\"));\n> \t}\n> +\n> \tif (pack_kept_objects < 0)\n> \t\tpack_kept_objects = write_bitmaps;\n> \n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index 0e9af832c9..839484c7dc 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -249,4 +249,20 @@ test_expect_success 'no bitmaps created if .keep files present' '\n> \ttest_must_be_empty actual\n> '\n> \n> +test_expect_success '-b warns with .keep files present' '\n> +\tpack=$(ls bare.git/objects/pack/*.pack) &&\n> +\ttest_path_is_file \"$pack\" &&\n> +\tkeep=${pack%.pack}.keep &&\n> +\t>\"$keep\" &&\n> +\tgit -C bare.git repack -adb 2>err &&\n> +\ttest_i18ngrep -F \".keep files are incompatible\" err &&\n> +\trm -f \"$keep\"\n> +'\n> +\n> +test_expect_success '-b warns with --keep-pack specified' '\n> +\tkeep=$(cd bare.git/objects/pack/ && ls *.pack) &&\n> +\tgit -C bare.git repack -adb --keep-pack=\"$keep\" 2>err &&\n> +\ttest_i18ngrep -F \"keep-pack is incompatible\" err\n> +'\n> +\n> test_done\n"},{"id":"378553","messageId":"20190703173814.GA29348@sigill.intra.peff.net","threadId":"51366","inReplyTo":"xmqq7e917h9x.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/1] repack: warn if bitmaps are explicitly enabled with keep files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-03T17:38:14Z","receivedAt":"2019-07-03T17:38:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 01, 2019 at 11:15:38AM -0700, Junio C Hamano wrote:\n\n> >> Maybe that's tricky with the gc.log functionality, but I think we should\n> >> at least document this before the next guy shows up with \"sometimes my\n> >> .bitmap files aren't generated...\".\n> >\n> > I'm not sure if the warning should be present by default;\n> > because we'll silently stop using bitmaps, now. But warning\n> > if '-b' is specified seems right:\n> \n> Hmph... write_bitmaps can come either from the command line or from\n> the configuration. When repack.writebitmaps configuration is set,\n> and some .keep files exist, the user probably is not even aware of\n> doing something that is unsupported.\n\nI think one tricky thing here is that we do not know if the .keep files\nare meant to be there, or if they are simply transient locks.\n\nThe whole point of the current behavior is that we should be ignoring\nthe transient locks, and if you are explicitly using bitmaps you\nunderstand the tradeoff you are making.\n\nI think the important case to cover is the one where the user didn't\nexplicitly ask for them, and the initial patch (to disable them when\nthere's a .keep around) covers that.\n\n\nA much more robust solution would be to stop conflating user-provided\npermanent .keep files with temporary locks. I think that was a mistaken\ndesign added many years ago. We probably could introduce a different\nfilename for the temporary locks (though I am not entirely convinced\nthey are necessary in the first place, as gc expiration-times would\ngenerally save a racily-written packfile anyway).\n\nOr perhaps we could differentiate our temporary locks from \"real\" .keep\nfiles by looking at the content; I think our locks always say something\nlike \"(receive|receive)-pack \\d+ on .*\", and it wouldn't be too onerous\nto commit to that, I think (or even adjust it to something even more\nunambiguous).\n\nIt does muddy the meaning of packed_git.pack_keep a bit. Some callers\nwould want to consider it kept in either case (i.e., for purposes of\npruning, we delete neither) and some would want it kept only for\nnon-locks (for packing, duplicating the objects is OK). So I think we'd\nend up with two bits there, and callers would have to use one or the\nother as appropriate.\n\n-Peff\n"},{"id":"378555","messageId":"20190703174004.GB29348@sigill.intra.peff.net","threadId":"51366","inReplyTo":"874l4f8h4c.fsf@evledraar.gmail.com","subject":"Re: 2.22.0 repack -a duplicating pack contents","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-03T17:40:04Z","receivedAt":"2019-07-03T17:40:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 24, 2019 at 11:30:59AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> I can't think of some great solution for this case, some thoughts:\n> \n> a. Perhaps we should split the *.keep flag into two things or\n> more.\n> \n> We're using it for all of \"I want this *.pack forever\"\n> (e.g. debugging) and \"I want only this *.pack to contain the data\n> found in it\" (I/O & CPU optimization, what Janos wants) and \"I'm\n> git.git code avoiding a race with myself\" (what you describe in #3).\n> \n> So maybe for the last of those we could also use and understand\n> *.tmp-keep, at which point we wouldn't have this race described in\n> #3. The 1st of those is a *.noprune and the 2nd is *.highlander (but\n> whether it's worth splitting all that out v.s. just having\n> *.tmp-keep is another matter).\n\nYeah, this is exactly the crux of the problem. We have two very\ndifferent things that are not distinguished by the reader. I left some\nthoughts elsewhere in the thread:\n\n https://public-inbox.org/git/20190703173814.GA29348@sigill.intra.peff.net/\n\n-Peff\n"},{"id":"378558","messageId":"xmqqftnn0z1t.fsf@gitster-ct.c.googlers.com","threadId":"51366","inReplyTo":"20190703173814.GA29348@sigill.intra.peff.net","subject":"Re: [PATCH 2/1] repack: warn if bitmaps are explicitly enabled with keep files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-03T18:10:22Z","receivedAt":"2019-07-03T18:10:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>\n> A much more robust solution would be to stop conflating user-provided\n> permanent .keep files with temporary locks. I think that was a mistaken\n> design added many years ago. We probably could introduce a different\n> filename for the temporary locks (though I am not entirely convinced\n> they are necessary in the first place, as gc expiration-times would\n> generally save a racily-written packfile anyway).\n\nTrue, true (and I tend to agree).\n\n> Or perhaps we could differentiate our temporary locks from \"real\" .keep\n> files by looking at the content; I think our locks always say something\n> like \"(receive|receive)-pack \\d+ on .*\", and it wouldn't be too onerous\n> to commit to that, I think (or even adjust it to something even more\n> unambiguous).\n\nTrue, but it may be overkill to open and read.\n\n> It does muddy the meaning of packed_git.pack_keep a bit. Some callers\n> would want to consider it kept in either case (i.e., for purposes of\n> pruning, we delete neither) and some would want it kept only for\n> non-locks (for packing, duplicating the objects is OK). So I think we'd\n> end up with two bits there, and callers would have to use one or the\n> other as appropriate.\n\nYeah, I agree that we'd need to treat them separately in the longer\nrun.\n\nThanks.\n"},{"id":"378562","messageId":"xmqqy31fynen.fsf@gitster-ct.c.googlers.com","threadId":"51366","inReplyTo":"xmqqftnn0z1t.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/1] repack: warn if bitmaps are explicitly enabled with keep files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-03T18:37:52Z","receivedAt":"2019-07-03T18:37:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>>\n>> A much more robust solution would be to stop conflating user-provided\n>> permanent .keep files with temporary locks. I think that was a mistaken\n>> design added many years ago. We probably could introduce a different\n>> filename for the temporary locks (though I am not entirely convinced\n>> they are necessary in the first place, as gc expiration-times would\n>> generally save a racily-written packfile anyway).\n>\n> True, true (and I tend to agree).\n>\n>> Or perhaps we could differentiate our temporary locks from \"real\" .keep\n>> files by looking at the content; I think our locks always say something\n>> like \"(receive|receive)-pack \\d+ on .*\", and it wouldn't be too onerous\n>> to commit to that, I think (or even adjust it to something even more\n>> unambiguous).\n>\n> True, but it may be overkill to open and read.\n>\n>> It does muddy the meaning of packed_git.pack_keep a bit. Some callers\n>> would want to consider it kept in either case (i.e., for purposes of\n>> pruning, we delete neither) and some would want it kept only for\n>> non-locks (for packing, duplicating the objects is OK). So I think we'd\n>> end up with two bits there, and callers would have to use one or the\n>> other as appropriate.\n>\n> Yeah, I agree that we'd need to treat them separately in the longer\n> run.\n>\n> Thanks.\n\nIn the meantime, this is about patch 2/1; the underlying \"when .keep\nis there, disable bitmaps\" is probably good to go, still.\n\n-- >8 --\nFrom: Eric Wong \nDate: Sat, 29 Jun 2019 19:13:59 +0000\nSubject: [PATCH] repack: disable bitmaps-by-default if .keep files exist\n\nBitmaps aren't useful with multiple packs, and users with\n.keep files ended up with redundant packs when bitmaps\ngot enabled by default in bare repos.\n\nSo detect when .keep files exist and stop enabling bitmaps\nby default in that case.\n\nWasteful (but otherwise harmless) race conditions with .keep files\ndocumented by Jeff King still apply and there's a chance we'd\nstill end up with redundant data on the FS:\n\n https://public-inbox.org/git/20190623224244.GB1100@sigill.intra.peff.net/\n\nv2: avoid subshell in test case, be multi-index aware\n\nFixes: 36eba0323d3288a8 (\"repack: enable bitmaps by default on bare repos\")\nSigned-off-by: Eric Wong \nHelped-by: Jeff King \nReported-by: Janos Farkas \nSigned-off-by: Junio C Hamano \n---\n builtin/repack.c | 18 ++++++++++++++++--\n t/t7700-repack.sh | 10 ++++++++++\n 2 files changed, 26 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex caca113927..73250b2431 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -89,6 +89,17 @@ static void remove_pack_on_signal(int signo)\n \traise(signo);\n }\n \n+static int has_pack_keep_file(void)\n+{\n+\tstruct packed_git *p;\n+\n+\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n+\t\tif (p->pack_keep)\n+\t\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n /*\n * Adds all packs hex strings to the fname list, which do not\n * have a corresponding .keep file. These packs are not to\n@@ -343,9 +354,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t (unpack_unreachable || (pack_everything & LOOSEN_UNREACHABLE)))\n \t\tdie(_(\"--keep-unreachable and -A are incompatible\"));\n \n-\tif (write_bitmaps < 0)\n+\tif (write_bitmaps < 0) {\n \t\twrite_bitmaps = (pack_everything & ALL_INTO_ONE) &&\n-\t\t\t\t is_bare_repository();\n+\t\t\t\t is_bare_repository() &&\n+\t\t\t\t keep_pack_list.nr == 0 &&\n+\t\t\t\t !has_pack_keep_file();\n+\t}\n \tif (pack_kept_objects < 0)\n \t\tpack_kept_objects = write_bitmaps;\n \ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 86d05160a3..0e9af832c9 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -239,4 +239,14 @@ test_expect_success 'bitmaps can be disabled on bare repos' '\n \ttest -z \"$bitmap\"\n '\n \n+test_expect_success 'no bitmaps created if .keep files present' '\n+\tpack=$(ls bare.git/objects/pack/*.pack) &&\n+\ttest_path_is_file \"$pack\" &&\n+\tkeep=${pack%.pack}.keep &&\n+\t>\"$keep\" &&\n+\tgit -C bare.git repack -ad &&\n+\tfind bare.git/objects/pack/ -type f -name \"*.bitmap\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n-- \n2.22.0-214-g8dca754b1e\n\n\n"},{"id":"378574","messageId":"20190703212316.GB32035@sigill.intra.peff.net","threadId":"51366","inReplyTo":"xmqqftnn0z1t.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/1] repack: warn if bitmaps are explicitly enabled with keep files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-03T21:23:16Z","receivedAt":"2019-07-03T21:23:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 03, 2019 at 11:10:22AM -0700, Junio C Hamano wrote:\n\n> > Or perhaps we could differentiate our temporary locks from \"real\" .keep\n> > files by looking at the content; I think our locks always say something\n> > like \"(receive|receive)-pack \\d+ on .*\", and it wouldn't be too onerous\n> > to commit to that, I think (or even adjust it to something even more\n> > unambiguous).\n> \n> True, but it may be overkill to open and read.\n\nYeah, that cross my mind as well, but:\n\n 1. We'd only need to open them when we _see_ them. And they're pretty\n rare anyway.\n\n 2. Effort-wise, we're already opening and mmap-ing the .idx files, so\n this is on par.\n\n 3. Most callers don't care about keep-files anyway. We could turn\n packed_git.pack_keep into:\n\n enum {\n PACK_KEEP_NONE,\n PACK_KEEP_LOCK,\n\t PACK_KEEP_USER\n } check_packed_keep(struct packed_git *pack);\n\n and then most programs wouldn't pay anything.\n\nJust some thoughts. I don't have immediate plans to work on it, but\nmaybe somebody else is excited about it. :)\n\n-Peff\n"},{"id":"378575","messageId":"20190703212419.GC32035@sigill.intra.peff.net","threadId":"51366","inReplyTo":"xmqqy31fynen.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/1] repack: warn if bitmaps are explicitly enabled with keep files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-03T21:24:19Z","receivedAt":"2019-07-03T21:24:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 03, 2019 at 11:37:52AM -0700, Junio C Hamano wrote:\n\n> In the meantime, this is about patch 2/1; the underlying \"when .keep\n> is there, disable bitmaps\" is probably good to go, still.\n> \n> -- >8 --\n> From: Eric Wong \n> Date: Sat, 29 Jun 2019 19:13:59 +0000\n> Subject: [PATCH] repack: disable bitmaps-by-default if .keep files exist\n\nYeah, this one looks good to me. Thanks for keeping things moving.\n\n-Peff\n"},{"id":"378695","messageId":"xmqqlfx8zaol.fsf@gitster-ct.c.googlers.com","threadId":"51366","inReplyTo":"20190703212316.GB32035@sigill.intra.peff.net","subject":"Re: [PATCH 2/1] repack: warn if bitmaps are explicitly enabled with keep files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-08T17:40:58Z","receivedAt":"2019-07-08T17:41:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jul 03, 2019 at 11:10:22AM -0700, Junio C Hamano wrote:\n>\n>> > Or perhaps we could differentiate our temporary locks from \"real\" .keep\n>> > files by looking at the content; I think our locks always say something\n>> > like \"(receive|receive)-pack \\d+ on .*\", and it wouldn't be too onerous\n>> > to commit to that, I think (or even adjust it to something even more\n>> > unambiguous).\n>> \n>> True, but it may be overkill to open and read.\n>\n> Yeah, that cross my mind as well, but:\n>\n> 1. We'd only need to open them when we _see_ them. And they're pretty\n> rare anyway.\n>\n> 2. Effort-wise, we're already opening and mmap-ing the .idx files, so\n> this is on par.\n>\n> 3. Most callers don't care about keep-files anyway. We could turn\n> packed_git.pack_keep into:\n>\n> enum {\n> PACK_KEEP_NONE,\n> PACK_KEEP_LOCK,\n> \t PACK_KEEP_USER\n> } check_packed_keep(struct packed_git *pack);\n>\n> and then most programs wouldn't pay anything.\n>\n> Just some thoughts. I don't have immediate plans to work on it, but\n> maybe somebody else is excited about it. :)\n\nOK. I do agree that .keep would be rare enough to justify spending\na bit more extra cycles, as long as the benefit is big enough (and\nin this case it may be a good trade-off).\n"}]}