{"thread":{"id":"51528","subject":"Warnings in gc.log can prevent gc --auto from running","startedAt":"2019-07-26T02:25:08Z","lastAt":"2019-07-31T22:34:09Z","messageCount":11,"participants":["Gregory Szorc","Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"379323","messageId":"qhdnuh$5m5r$1@blaine.gmane.org","threadId":"51528","inReplyTo":null,"subject":"Warnings in gc.log can prevent gc --auto from running","fromName":"Gregory Szorc","fromEmail":"gregory.szorc@gmail.com","sentAt":"2019-07-26T02:18:57Z","receivedAt":"2019-07-26T02:25:08Z","isPatch":false,"sender":{"key":"gregory.szorc@gmail.com","avatar":"https://gravatar.com/avatar/2babe1f7c67be536dbaf27222e472d1d5b5e9d71fe725afb4c910ee3a1bf5a34?d=mp&s=160"},"body":"I think I've found some undesirable behavior with regards to the\nbehavior of `git gc --auto`. The tl;dr is that a warning message written\nto gc.log can result in `git gc --auto` effectively disabling itself for\ngc.logExpiry. The problem is easier to trigger in 2.22 as a result of\nenabling bitmap indices for bare repositories by default and the\nbehavior can easily result in performance degradation, especially on\nservers.\n\n`git gc --auto` will stop itself from running if a gc.log file newer\nthan gc.logExpiry (1 day by default) exists. The intention of this\nbehavior seems reasonable enough. However, it is relatively easy for a\nrelatively harmless gc.log file to exist and for that relatively\nharmless gc.log file to effectively disable `git gc --auto`.\n\nFor example, if bitmap indices are being produced (this is the default\nbehavior for bare repositories in Git 2.22) and the user has taken any\naction that would result in a `git gc` producing multiple packfiles\n(setting gc.bigPackThreshold, setting pack.packSizeLimit, annotating a\npackfile with a .keep file, etc) then a message like \"warning: disabling\nbitmap writing, as some objects are not being packed\" or \"warning:\ndisabling bitmap writing, packs are split due to pack.packSizeLimit\" may\nbe written to gc.log. This warning message will result in the presence\nof a gc.log file, which will cause `git gc --auto` to stop doing\nmeaningful work until gc.logExpiry has passed or the gc.log is cleaned\nup out-of-band.\n\nThe practical impact of this behavior is that an environment having only\nmade minor tweaks to tweak packfile behavior may end up inadvertently\ndisabling `git gc --auto` and having excessive amounts of packfiles and\nloose object files accumulate since `git gc --auto` isn't running. This\ncan result in performance degradation, especially for repositories\nreceiving hundreds or thousands of pushes a day - ask me how I know :)\n\nI was able to work around this in a server environment by removing\ngc.log if the contents were \"harmless\" warning messages, unblocking `git\ngc --auto`. However, the solution is a bit brittle. As an end-user of\nGit, I would prefer a `git gc --auto` execution mode that was less\nsensitive to the presence of non-fatal messages in gc.log. Lowering the\nvalue of gc.logExpiry is also a somewhat reasonable solution. I /think/\nyou could even make the value \"now\" to effectively disable the gc.log\ncheck, but I haven't tested this. I don't feel great about that\nworkaround though, as if there is an actual gc/repack error, I'd like to\nknow about it instead of sweeping it under the rug by continuously\ndeleting the gc.log file. I'm also not keen on triggering `git gc --auto\n--force` because --force will ignore lock files and I like respecting\nlock files.\n\nI don't prescribe to know the best way to solve this problem. I just\nknow it is a footgun sitting in the default Git configuration. And the\nfootgun became a lot easier to fire with the introduction of warning\nmessages related to bitmap indices and again when bitmap indices were\nenabled by default for bare repositories in Git 2.22.\n\nGregory Szorc\ngregory.szorc@gmail.com\n\n"},{"id":"379488","messageId":"20190729100745.GA2755@sigill.intra.peff.net","threadId":"51528","inReplyTo":"qhdnuh$5m5r$1@blaine.gmane.org","subject":"Re: Warnings in gc.log can prevent gc --auto from running","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-29T10:07:45Z","receivedAt":"2019-07-29T10:07:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 25, 2019 at 07:18:57PM -0700, Gregory Szorc wrote:\n\n> I think I've found some undesirable behavior with regards to the\n> behavior of `git gc --auto`. The tl;dr is that a warning message written\n> to gc.log can result in `git gc --auto` effectively disabling itself for\n> gc.logExpiry. The problem is easier to trigger in 2.22 as a result of\n> enabling bitmap indices for bare repositories by default and the\n> behavior can easily result in performance degradation, especially on\n> servers.\n\nYuck, thanks for reporting this.\n\nAs you note, this is a special case of a much larger problem. The other\ncommon case is the \"oops, you still have a lot of loose objects after\nrepacking\" warning. There's more discussion and some patches here:\n\n  https://public-inbox.org/git/20180716172717.237373-1-jonathantanmy@google.com/\n\nthough I don't think any of the work that came out of that fundamentally\nsolves the issue.\n\n> I don't prescribe to know the best way to solve this problem. I just\n> know it is a footgun sitting in the default Git configuration. And the\n> footgun became a lot easier to fire with the introduction of warning\n> messages related to bitmap indices and again when bitmap indices were\n> enabled by default for bare repositories in Git 2.22.\n\nIMHO one way to mitigate this is to simply warn less. In particular, if\nwe are auto-enabling bitmaps, then it doesn't necessarily make sense for\nus to warn about them being disabled.\n\nIn the case of .keep files, we've already got 7328482253 (repack:\ndisable bitmaps-by-default if .keep files exist, 2019-06-29), which\nshould be in the next released version of Git. But I suspect that's\nracy with respect to somebody creating .keep files, and as you note\nthere are other config options that might prevent us from generating\nbitmaps.\n\nInstead, it may make sense to turn the --write-bitmap-index option of\npack-objects into a tri-state: true/false/auto. Then pack-objects would\nknow that we are in best-effort mode, and would avoid warning in that\ncase. That would also let git-repack express its intentions better to\ngit-pack-objects, so we could replace 7328482253, and keep more of the\nlogic in pack-objects, which is ultimately what has to make the decision\nabout whether it can generate bitmaps.\n\n-Peff\n"},{"id":"379495","messageId":"87v9vl57in.fsf@evledraar.gmail.com","threadId":"51528","inReplyTo":"20190729100745.GA2755@sigill.intra.peff.net","subject":"Re: Warnings in gc.log can prevent gc --auto from running","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-07-29T12:50:56Z","receivedAt":"2019-07-29T12:51:01Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jul 29 2019, Jeff King wrote:\n\n> On Thu, Jul 25, 2019 at 07:18:57PM -0700, Gregory Szorc wrote:\n>\n>> I think I've found some undesirable behavior with regards to the\n>> behavior of `git gc --auto`. The tl;dr is that a warning message written\n>> to gc.log can result in `git gc --auto` effectively disabling itself for\n>> gc.logExpiry. The problem is easier to trigger in 2.22 as a result of\n>> enabling bitmap indices for bare repositories by default and the\n>> behavior can easily result in performance degradation, especially on\n>> servers.\n>\n> Yuck, thanks for reporting this.\n>\n> As you note, this is a special case of a much larger problem. The other\n> common case is the \"oops, you still have a lot of loose objects after\n> repacking\" warning. There's more discussion and some patches here:\n>\n>   https://public-inbox.org/git/20180716172717.237373-1-jonathantanmy@google.com/\n>\n> though I don't think any of the work that came out of that fundamentally\n> solves the issue.\n\nTo add to that Gregory probably finds these two old reports of mine\ninteresting. The former is pretty much his report (but for a different\nroot cause, the loose object issue):\nhttps://public-inbox.org/git/87inc89j38.fsf@evledraar.gmail.com/ &\nhttps://public-inbox.org/git/87fu6bmr0j.fsf@evledraar.gmail.com/\n\n>> I don't prescribe to know the best way to solve this problem. I just\n>> know it is a footgun sitting in the default Git configuration. And the\n>> footgun became a lot easier to fire with the introduction of warning\n>> messages related to bitmap indices and again when bitmap indices were\n>> enabled by default for bare repositories in Git 2.22.\n>\n> IMHO one way to mitigate this is to simply warn less. In particular, if\n> we are auto-enabling bitmaps, then it doesn't necessarily make sense for\n> us to warn about them being disabled.\n>\n> In the case of .keep files, we've already got 7328482253 (repack:\n> disable bitmaps-by-default if .keep files exist, 2019-06-29), which\n> should be in the next released version of Git. But I suspect that's\n> racy with respect to somebody creating .keep files, and as you note\n> there are other config options that might prevent us from generating\n> bitmaps.\n>\n> Instead, it may make sense to turn the --write-bitmap-index option of\n> pack-objects into a tri-state: true/false/auto. Then pack-objects would\n> know that we are in best-effort mode, and would avoid warning in that\n> case. That would also let git-repack express its intentions better to\n> git-pack-objects, so we could replace 7328482253, and keep more of the\n> logic in pack-objects, which is ultimately what has to make the decision\n> about whether it can generate bitmaps.\n\nSounds like pentastate to me :) (penta = 5, had to look it up). I.e. in\nmost cases of \"auto\" we pick a true/false at the outset, whereas this is\ntrue/true-but-dont-care-much/false/false-but-dont-care-much with \"auto\"\npicking the \"-but-dont-care-much\" versions of a \"soft\" true/false.\n\nOn this general topic a *soft* poke about relying to\nhttps://public-inbox.org/git/8736lnxlig.fsf@evledraar.gmail.com/ if you\nhave time. I think a \"loose pack\" might be a way forward for the loose\nobject proliferation, but maybe I'm wrong.\n\nMore generally we're really straining the gc.log pass-along-a-message\nfacility.\n"},{"id":"379638","messageId":"20190731042807.GA26237@sigill.intra.peff.net","threadId":"51528","inReplyTo":"87v9vl57in.fsf@evledraar.gmail.com","subject":"Re: Warnings in gc.log can prevent gc --auto from running","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-31T04:28:08Z","receivedAt":"2019-07-31T04:28:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 29, 2019 at 02:50:56PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > Instead, it may make sense to turn the --write-bitmap-index option of\n> > pack-objects into a tri-state: true/false/auto. Then pack-objects would\n> > know that we are in best-effort mode, and would avoid warning in that\n> > case. That would also let git-repack express its intentions better to\n> > git-pack-objects, so we could replace 7328482253, and keep more of the\n> > logic in pack-objects, which is ultimately what has to make the decision\n> > about whether it can generate bitmaps.\n> \n> Sounds like pentastate to me :) (penta = 5, had to look it up). I.e. in\n> most cases of \"auto\" we pick a true/false at the outset, whereas this is\n> true/true-but-dont-care-much/false/false-but-dont-care-much with \"auto\"\n> picking the \"-but-dont-care-much\" versions of a \"soft\" true/false.\n\nI don't think we care about false-but-dont-care-much. Pack-objects just\nneeds to know whether the bitmaps are the user's expressed intention, or\njust something that it should do if it's convenient.\n\nI'll see if I can work up a patch to demonstrate.\n\n> On this general topic a *soft* poke about relying to\n> https://public-inbox.org/git/8736lnxlig.fsf@evledraar.gmail.com/ if you\n> have time. I think a \"loose pack\" might be a way forward for the loose\n> object proliferation, but maybe I'm wrong.\n\nI just left a reply, though I think most of the discussion there is\nabout the actual pruning-corruption race. I'm totally on board with the\nidea of an \"unreachable pack\", but AFAIK nobody has produced any\npatches yet.\n\n> More generally we're really straining the gc.log pass-along-a-message\n> facility.\n\nI definitely agree with that. :)\n\n-Peff\n"},{"id":"379647","messageId":"20190731053703.GA16709@sigill.intra.peff.net","threadId":"51528","inReplyTo":"20190731042807.GA26237@sigill.intra.peff.net","subject":"[PATCH 0/3] handling warnings due to auto-enabled bitmaps","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-31T05:37:04Z","receivedAt":"2019-07-31T05:37:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 31, 2019 at 12:28:07AM -0400, Jeff King wrote:\n\n> On Mon, Jul 29, 2019 at 02:50:56PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> > > Instead, it may make sense to turn the --write-bitmap-index option of\n> > > pack-objects into a tri-state: true/false/auto. Then pack-objects would\n> > > know that we are in best-effort mode, and would avoid warning in that\n> > > case. That would also let git-repack express its intentions better to\n> > > git-pack-objects, so we could replace 7328482253, and keep more of the\n> > > logic in pack-objects, which is ultimately what has to make the decision\n> > > about whether it can generate bitmaps.\n> > \n> > Sounds like pentastate to me :) (penta = 5, had to look it up). I.e. in\n> > most cases of \"auto\" we pick a true/false at the outset, whereas this is\n> > true/true-but-dont-care-much/false/false-but-dont-care-much with \"auto\"\n> > picking the \"-but-dont-care-much\" versions of a \"soft\" true/false.\n> \n> I don't think we care about false-but-dont-care-much. Pack-objects just\n> needs to know whether the bitmaps are the user's expressed intention, or\n> just something that it should do if it's convenient.\n> \n> I'll see if I can work up a patch to demonstrate.\n\nThis actually turned out pretty well, I think. I wish I had thought of\nit when were initially looking at the .keep stuff. :) It was not too\nhard to clean that up in the third patch, though.\n\n  [1/3]: t7700: clean up .keep file in bitmap-writing test\n  [2/3]: repack: silence warnings when auto-enabled bitmaps cannot be built\n  [3/3]: repack: simplify handling of auto-bitmaps and .keep files\n\n builtin/pack-objects.c | 21 ++++++++++++++++-----\n builtin/repack.c       | 24 +++++++-----------------\n t/t7700-repack.sh      | 15 ++++++++++++++-\n 3 files changed, 37 insertions(+), 23 deletions(-)\n\n"},{"id":"379648","messageId":"20190731053735.GA16941@sigill.intra.peff.net","threadId":"51528","inReplyTo":"20190731053703.GA16709@sigill.intra.peff.net","subject":"[PATCH 1/3] t7700: clean up .keep file in bitmap-writing test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-31T05:37:36Z","receivedAt":"2019-07-31T05:37:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After our test snippet finishes, the .keep file is left in place, making\nit hard to do further tests of the auto-bitmap-writing code (since it\nsuppresses the feature completely). Let's clean it up.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t7700-repack.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 0e9af832c9..8d9a358df8 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -243,6 +243,7 @@ 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+\ttest_when_finished \"rm -f \\\"\\$keep\\\"\" &&\n \t>\"$keep\" &&\n \tgit -C bare.git repack -ad &&\n \tfind bare.git/objects/pack/ -type f -name \"*.bitmap\" >actual &&\n-- \n2.23.0.rc0.426.gbdee707ba7\n\n"},{"id":"379649","messageId":"20190731053927.GB16941@sigill.intra.peff.net","threadId":"51528","inReplyTo":"20190731053703.GA16709@sigill.intra.peff.net","subject":"[PATCH 2/3] repack: silence warnings when auto-enabled bitmaps cannot be built","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-31T05:39:27Z","receivedAt":"2019-07-31T05:39:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Depending on various config options, a full repack may not be able to\nbuild a reachability bitmap index (e.g., if pack.packSizeLimit forces us\nto write multiple packs). In these cases pack-objects may write a\nwarning to stderr.\n\nSince 36eba0323d (repack: enable bitmaps by default on bare repos,\n2019-03-14), we may generate these warnings even when the user did not\nexplicitly ask for bitmaps. This has two downsides:\n\n  - it can be confusing, if they don't know what bitmaps are\n\n  - a daemonized auto-gc will write this to its log file, and the\n    presence of the warning may suppress further auto-gc (until\n    gc.logExpiry has elapsed)\n\nLet's have repack communicate to pack-objects that the choice to turn on\nbitmaps was not made explicitly by the user, which in turn allows\npack-objects to suppress these warnings.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/pack-objects.c | 21 ++++++++++++++++-----\n builtin/repack.c       | 15 +++++++++------\n t/t7700-repack.sh      | 11 +++++++++++\n 3 files changed, 36 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 267c562b1f..76ce906946 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -96,7 +96,11 @@ static off_t reuse_packfile_offset;\n \n static int use_bitmap_index_default = 1;\n static int use_bitmap_index = -1;\n-static int write_bitmap_index;\n+static enum {\n+\tWRITE_BITMAP_FALSE = 0,\n+\tWRITE_BITMAP_QUIET,\n+\tWRITE_BITMAP_TRUE,\n+} write_bitmap_index;\n static uint16_t write_bitmap_options = BITMAP_OPT_HASH_CACHE;\n \n static int exclude_promisor_objects;\n@@ -892,7 +896,8 @@ static void write_pack_file(void)\n \t\t\t\t\t\t nr_written, oid.hash, offset);\n \t\t\tclose(fd);\n \t\t\tif (write_bitmap_index) {\n-\t\t\t\twarning(_(no_split_warning));\n+\t\t\t\tif (write_bitmap_index != WRITE_BITMAP_QUIET)\n+\t\t\t\t\twarning(_(no_split_warning));\n \t\t\t\twrite_bitmap_index = 0;\n \t\t\t}\n \t\t}\n@@ -1176,7 +1181,8 @@ static int add_object_entry(const struct object_id *oid, enum object_type type,\n \tif (!want_object_in_pack(oid, exclude, &found_pack, &found_offset)) {\n \t\t/* The pack is missing an object, so it will not have closure */\n \t\tif (write_bitmap_index) {\n-\t\t\twarning(_(no_closure_warning));\n+\t\t\tif (write_bitmap_index != WRITE_BITMAP_QUIET)\n+\t\t\t\twarning(_(no_closure_warning));\n \t\t\twrite_bitmap_index = 0;\n \t\t}\n \t\treturn 0;\n@@ -3313,8 +3319,13 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t    N_(\"do not hide commits by grafts\"), 0),\n \t\tOPT_BOOL(0, \"use-bitmap-index\", &use_bitmap_index,\n \t\t\t N_(\"use a bitmap index if available to speed up counting objects\")),\n-\t\tOPT_BOOL(0, \"write-bitmap-index\", &write_bitmap_index,\n-\t\t\t N_(\"write a bitmap index together with the pack index\")),\n+\t\tOPT_SET_INT(0, \"write-bitmap-index\", &write_bitmap_index,\n+\t\t\t    N_(\"write a bitmap index together with the pack index\"),\n+\t\t\t    WRITE_BITMAP_TRUE),\n+\t\tOPT_SET_INT_F(0, \"write-bitmap-index-quiet\",\n+\t\t\t      &write_bitmap_index,\n+\t\t\t      N_(\"write a bitmap index if possible\"),\n+\t\t\t      WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN),\n \t\tOPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),\n \t\t{ OPTION_CALLBACK, 0, \"missing\", NULL, N_(\"action\"),\n \t\t  N_(\"handling for missing objects\"), PARSE_OPT_NONEG,\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 30982ed2a2..db93ca3660 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -345,13 +345,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"--keep-unreachable and -A are incompatible\"));\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 keep_pack_list.nr == 0 &&\n-\t\t\t\t !has_pack_keep_file();\n+\t\tif (!(pack_everything & ALL_INTO_ONE) ||\n+\t\t    !is_bare_repository() ||\n+\t\t    keep_pack_list.nr != 0 ||\n+\t\t    has_pack_keep_file())\n+\t\t\twrite_bitmaps = 0;\n \t}\n \tif (pack_kept_objects < 0)\n-\t\tpack_kept_objects = write_bitmaps;\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@@ -375,8 +376,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \targv_array_push(&cmd.args, \"--indexed-objects\");\n \tif (repository_format_partial_clone)\n \t\targv_array_push(&cmd.args, \"--exclude-promisor-objects\");\n-\tif (write_bitmaps)\n+\tif (write_bitmaps > 0)\n \t\targv_array_push(&cmd.args, \"--write-bitmap-index\");\n+\telse if (write_bitmaps < 0)\n+\t\targv_array_push(&cmd.args, \"--write-bitmap-index-quiet\");\n \tif (use_delta_islands)\n \t\targv_array_push(&cmd.args, \"--delta-islands\");\n \ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 8d9a358df8..54f815b8b9 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -250,4 +250,15 @@ test_expect_success 'no bitmaps created if .keep files present' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'auto-bitmaps do not complain if unavailable' '\n+\ttest_config -C bare.git pack.packSizeLimit 1M &&\n+\tblob=$(test-tool genrandom big $((1024*1024)) |\n+\t       git -C bare.git hash-object -w --stdin) &&\n+\tgit -C bare.git update-ref refs/tags/big $blob &&\n+\tgit -C bare.git repack -ad 2>stderr &&\n+\ttest_must_be_empty stderr &&\n+\tfind bare.git/objects/pack -type f -name \"*.bitmap\" >actual &&\n+\ttest_must_be_empty actual\n+'\n+\n test_done\n-- \n2.23.0.rc0.426.gbdee707ba7\n\n"},{"id":"379650","messageId":"20190731054055.GC16941@sigill.intra.peff.net","threadId":"51528","inReplyTo":"20190731053703.GA16709@sigill.intra.peff.net","subject":"[PATCH 3/3] repack: simplify handling of auto-bitmaps and .keep files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-31T05:40:56Z","receivedAt":"2019-07-31T05:40:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 7328482253 (repack: disable bitmaps-by-default if .keep files\nexist, 2019-06-29) taught repack to prefer disabling bitmaps to\nduplicating objects (unless bitmaps were asked for explicitly).\n\nBut there's an easier way to do this: if we keep passing the\n--honor-pack-keep flag to pack-objects when auto-enabling bitmaps, then\npack-objects already makes the same decision (it will disable bitmaps\nrather than duplicate). Better still, pack-objects can actually decide\nto do so based not just on the presence of a .keep file, but on whether\nthat .keep file actually impacts the new pack we're making (so if we're\nracing with a push or fetch, for example, their temporary .keep file\nwill not block us from generating bitmaps if they haven't yet updated\ntheir refs).\n\nAnd because repack uses the --write-bitmap-index-quiet flag, we don't\nhave to worry about pack-objects generating confusing warnings when it\ndoes see a .keep file. We can confirm this by tweaking the .keep test to\ncheck repack's stderr.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/repack.c  | 17 ++---------------\n t/t7700-repack.sh |  3 ++-\n 2 files changed, 4 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex db93ca3660..632c0c0a79 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -89,17 +89,6 @@ 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@@ -346,13 +335,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \tif (write_bitmaps < 0) {\n \t\tif (!(pack_everything & ALL_INTO_ONE) ||\n-\t\t    !is_bare_repository() ||\n-\t\t    keep_pack_list.nr != 0 ||\n-\t\t    has_pack_keep_file())\n+\t\t    !is_bare_repository())\n \t\t\twrite_bitmaps = 0;\n \t}\n \tif (pack_kept_objects < 0)\n-\t\tpack_kept_objects = !!write_bitmaps;\n+\t\tpack_kept_objects = write_bitmaps > 0;\n \n \tif (write_bitmaps && !(pack_everything & ALL_INTO_ONE))\n \t\tdie(_(incremental_bitmap_conflict_error));\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 54f815b8b9..4e855bc21b 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -245,7 +245,8 @@ test_expect_success 'no bitmaps created if .keep files present' '\n \tkeep=${pack%.pack}.keep &&\n \ttest_when_finished \"rm -f \\\"\\$keep\\\"\" &&\n \t>\"$keep\" &&\n-\tgit -C bare.git repack -ad &&\n+\tgit -C bare.git repack -ad 2>stderr &&\n+\ttest_must_be_empty stderr &&\n \tfind bare.git/objects/pack/ -type f -name \"*.bitmap\" >actual &&\n \ttest_must_be_empty actual\n '\n-- \n2.23.0.rc0.426.gbdee707ba7\n"},{"id":"379713","messageId":"xmqqftmmosrf.fsf@gitster-ct.c.googlers.com","threadId":"51528","inReplyTo":"20190731053927.GB16941@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] repack: silence warnings when auto-enabled bitmaps cannot be built","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-31T20:26:12Z","receivedAt":"2019-07-31T20:26:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> @@ -3313,8 +3319,13 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n>  \t\t\t    N_(\"do not hide commits by grafts\"), 0),\n>  \t\tOPT_BOOL(0, \"use-bitmap-index\", &use_bitmap_index,\n>  \t\t\t N_(\"use a bitmap index if available to speed up counting objects\")),\n> -\t\tOPT_BOOL(0, \"write-bitmap-index\", &write_bitmap_index,\n> -\t\t\t N_(\"write a bitmap index together with the pack index\")),\n> +\t\tOPT_SET_INT(0, \"write-bitmap-index\", &write_bitmap_index,\n> +\t\t\t    N_(\"write a bitmap index together with the pack index\"),\n> +\t\t\t    WRITE_BITMAP_TRUE),\n> +\t\tOPT_SET_INT_F(0, \"write-bitmap-index-quiet\",\n> +\t\t\t      &write_bitmap_index,\n> +\t\t\t      N_(\"write a bitmap index if possible\"),\n> +\t\t\t      WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN),\n\nThe receiving end of this communication is pretty easy to follow.\nI'd have named an option to trigger \"if possible\" behaviour after\nthat \"if possible\" phrase and not \"quiet\", but this is entirely\ninternal that it does not matter.\n\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 30982ed2a2..db93ca3660 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -345,13 +345,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \t\tdie(_(\"--keep-unreachable and -A are incompatible\"));\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 keep_pack_list.nr == 0 &&\n> -\t\t\t\t !has_pack_keep_file();\n> +\t\tif (!(pack_everything & ALL_INTO_ONE) ||\n> +\t\t    !is_bare_repository() ||\n> +\t\t    keep_pack_list.nr != 0 ||\n> +\t\t    has_pack_keep_file())\n> +\t\t\twrite_bitmaps = 0;\n\nThis side of communication is a bit harder to follow, but not\nimpossible ;-) We leave it \"negative\" to signal \"the user did not\nspecify, but we enabled it by default\" here.\n\n>  \t}\n>  \tif (pack_kept_objects < 0)\n> -\t\tpack_kept_objects = write_bitmaps;\n> +\t\tpack_kept_objects = !!write_bitmaps;\n\nAnd non-zero write_bitmaps replaces \"true\" in the older world.\n\n>  \tif (write_bitmaps && !(pack_everything & ALL_INTO_ONE))\n\nSo this does not have to change.\n\n>  \t\tdie(_(incremental_bitmap_conflict_error));\n> @@ -375,8 +376,10 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \targv_array_push(&cmd.args, \"--indexed-objects\");\n>  \tif (repository_format_partial_clone)\n>  \t\targv_array_push(&cmd.args, \"--exclude-promisor-objects\");\n> -\tif (write_bitmaps)\n> +\tif (write_bitmaps > 0)\n>  \t\targv_array_push(&cmd.args, \"--write-bitmap-index\");\n> +\telse if (write_bitmaps < 0)\n> +\t\targv_array_push(&cmd.args, \"--write-bitmap-index-quiet\");\n\nAnd \"enabled by user\" and \"enabled by default\" are mapped to the two\noptions.\n\nAll makes sense.\n\nThanks.\n"},{"id":"379714","messageId":"20190731211144.GA1207@sigill.intra.peff.net","threadId":"51528","inReplyTo":"xmqqftmmosrf.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] repack: silence warnings when auto-enabled bitmaps cannot be built","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-07-31T21:11:44Z","receivedAt":"2019-07-31T21:11:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 31, 2019 at 01:26:12PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > @@ -3313,8 +3319,13 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> >  \t\t\t    N_(\"do not hide commits by grafts\"), 0),\n> >  \t\tOPT_BOOL(0, \"use-bitmap-index\", &use_bitmap_index,\n> >  \t\t\t N_(\"use a bitmap index if available to speed up counting objects\")),\n> > -\t\tOPT_BOOL(0, \"write-bitmap-index\", &write_bitmap_index,\n> > -\t\t\t N_(\"write a bitmap index together with the pack index\")),\n> > +\t\tOPT_SET_INT(0, \"write-bitmap-index\", &write_bitmap_index,\n> > +\t\t\t    N_(\"write a bitmap index together with the pack index\"),\n> > +\t\t\t    WRITE_BITMAP_TRUE),\n> > +\t\tOPT_SET_INT_F(0, \"write-bitmap-index-quiet\",\n> > +\t\t\t      &write_bitmap_index,\n> > +\t\t\t      N_(\"write a bitmap index if possible\"),\n> > +\t\t\t      WRITE_BITMAP_QUIET, PARSE_OPT_HIDDEN),\n> \n> The receiving end of this communication is pretty easy to follow.\n> I'd have named an option to trigger \"if possible\" behaviour after\n> that \"if possible\" phrase and not \"quiet\", but this is entirely\n> internal that it does not matter.\n\nHeh, that was actually the part of this series that I struggled the most\nwith. I didn't like \"if possible\" because that is already how we behave\n(we continue without bitmaps if we can't make them, even if the user\nasked for them explicitly). I had \"if convenient\" at one point, but it\nseemed too vague and too long. ;)\n\nSo I'm happy to change it, but it would require somebody coming up with\na better name.\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 keep_pack_list.nr == 0 &&\n> > -\t\t\t\t !has_pack_keep_file();\n> > +\t\tif (!(pack_everything & ALL_INTO_ONE) ||\n> > +\t\t    !is_bare_repository() ||\n> > +\t\t    keep_pack_list.nr != 0 ||\n> > +\t\t    has_pack_keep_file())\n> > +\t\t\twrite_bitmaps = 0;\n> \n> This side of communication is a bit harder to follow, but not\n> impossible ;-) We leave it \"negative\" to signal \"the user did not\n> specify, but we enabled it by default\" here.\n\nYeah, I also noticed that was a bit subtle. Maybe a comment:\n\n  /* leave as -1 to indicate \"auto bitmaps\" */\n\nor something would help. I also thought about writing it as:\n\n  if (write_bitmaps < 0) {\n\tif ((pack_everything & ALL_INTO_ONE) &&\n\t    is_bare_repository() &&\n\t    keep_pack_list.nr == 0 &&\n\t    !hash_pack_keep_file()) {\n\t\twrite_bitmaps = -1; /* indicates auto-enabled */\n\t} else {\n\t\twrite_bitmaps = 0;\n\t}\n  }\n\nBetter?\n\n-Peff\n"},{"id":"379721","messageId":"xmqqzhktomub.fsf@gitster-ct.c.googlers.com","threadId":"51528","inReplyTo":"20190731054055.GC16941@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] repack: simplify handling of auto-bitmaps and .keep files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-31T22:34:04Z","receivedAt":"2019-07-31T22:34:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Commit 7328482253 (repack: disable bitmaps-by-default if .keep files\n> exist, 2019-06-29) taught repack to prefer disabling bitmaps to\n> duplicating objects (unless bitmaps were asked for explicitly).\n>\n> But there's an easier way to do this: if we keep passing the\n> --honor-pack-keep flag to pack-objects when auto-enabling bitmaps, then\n> pack-objects already makes the same decision (it will disable bitmaps\n> rather than duplicate). Better still, pack-objects can actually decide\n> to do so based not just on the presence of a .keep file, but on whether\n> that .keep file actually impacts the new pack we're making (so if we're\n> racing with a push or fetch, for example, their temporary .keep file\n> will not block us from generating bitmaps if they haven't yet updated\n> their refs).\n>\n> And because repack uses the --write-bitmap-index-quiet flag, we don't\n> have to worry about pack-objects generating confusing warnings when it\n> does see a .keep file. We can confirm this by tweaking the .keep test to\n> check repack's stderr.\n\nThis change is a bit too dense so I'll need to think about it a bit\nlonger, but in the meantime it is queued alongside the other two.\n\nThanks.\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/repack.c  | 17 ++---------------\n>  t/t7700-repack.sh |  3 ++-\n>  2 files changed, 4 insertions(+), 16 deletions(-)\n>\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index db93ca3660..632c0c0a79 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -89,17 +89,6 @@ 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> @@ -346,13 +335,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n>  \n>  \tif (write_bitmaps < 0) {\n>  \t\tif (!(pack_everything & ALL_INTO_ONE) ||\n> -\t\t    !is_bare_repository() ||\n> -\t\t    keep_pack_list.nr != 0 ||\n> -\t\t    has_pack_keep_file())\n> +\t\t    !is_bare_repository())\n>  \t\t\twrite_bitmaps = 0;\n>  \t}\n>  \tif (pack_kept_objects < 0)\n> -\t\tpack_kept_objects = !!write_bitmaps;\n> +\t\tpack_kept_objects = write_bitmaps > 0;\n>  \n>  \tif (write_bitmaps && !(pack_everything & ALL_INTO_ONE))\n>  \t\tdie(_(incremental_bitmap_conflict_error));\n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index 54f815b8b9..4e855bc21b 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -245,7 +245,8 @@ test_expect_success 'no bitmaps created if .keep files present' '\n>  \tkeep=${pack%.pack}.keep &&\n>  \ttest_when_finished \"rm -f \\\"\\$keep\\\"\" &&\n>  \t>\"$keep\" &&\n> -\tgit -C bare.git repack -ad &&\n> +\tgit -C bare.git repack -ad 2>stderr &&\n> +\ttest_must_be_empty stderr &&\n>  \tfind bare.git/objects/pack/ -type f -name \"*.bitmap\" >actual &&\n>  \ttest_must_be_empty actual\n>  '\n"}]}