{"thread":{"id":"35717","subject":"WIth git-next, writing bitmaps fails when keep files are present","startedAt":"2014-01-23T02:38:57Z","lastAt":"2014-03-03T20:04:20Z","messageCount":27,"participants":["Siddharth Agarwal","Jeff King","Vicent Martí","Junio C Hamano","Nasser Grainawi","Shawn Pearce"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"233583","messageId":"52E080C1.4030402@fb.com","threadId":"35717","inReplyTo":null,"subject":"WIth git-next, writing bitmaps fails when keep files are present","fromName":"Siddharth Agarwal","fromEmail":"sid0@fb.com","sentAt":"2014-01-23T02:38:57Z","receivedAt":"2014-01-23T02:38:57Z","isPatch":false,"sender":{"key":"sid0@fb.com","avatar":null},"body":"Running git-next, writing bitmap indexes fails if a keep file is present \nfrom an earlier pack.\n\nWith git at b139ac2, the following commands demonstrate the problem:\n\ngit init test\ncd test\ntouch a\ngit add a\ngit commit -m \"a\"\n\ngit repack -ad  # generate a pack file\nfor f in .git/objects/pack/*.pack; touch ${f/%pack/keep}  # mark it as \nto keep\n\ntouch b\ngit add b\ngit commit -m \"b\"\ngit repack -adb\n\nThis fails at the bitmap writing stage with something like:\n\nCounting objects: 2, done.\nDelta compression using up to 24 threads.\nCompressing objects: 100% (2/2), done.\nWriting objects: 100% (2/2), done.\nfatal: Failed to write bitmap index. Packfile doesn't have full closure \n(object 7388a015938147155b600eaacc59af6e78c75e5a is missing)\n\nIn our case we have .keep files lying around from ages ago (possibly due \nto kill -9s run on the server). It also means that running repack -a \nwith bitmap writing enabled on a repo becomes problematic if a fetch is \nrun concurrently.\n\nEven if we practice good .keep hygiene, this seems like a bug in git \nthat should be fixed.\n"},{"id":"233631","messageId":"52E17D50.5030301@fb.com","threadId":"35717","inReplyTo":"52E080C1.4030402@fb.com","subject":"Re: WIth git-next, writing bitmaps fails when keep files are present","fromName":"Siddharth Agarwal","fromEmail":"sid0@fb.com","sentAt":"2014-01-23T20:36:32Z","receivedAt":"2014-01-23T20:36:32Z","isPatch":false,"sender":{"key":"sid0@fb.com","avatar":null},"body":"On 01/22/2014 06:38 PM, Siddharth Agarwal wrote:\n> In our case we have .keep files lying around from ages ago (possibly \n> due to kill -9s run on the server). It also means that running repack \n> -a with bitmap writing enabled on a repo becomes problematic if a \n> fetch is run concurrently.\n\nWe briefly discussed locking our repos while the repack was run, but the \nrepo that would benefit the most from repacks cannot be locked to pushes \nfor even a tenth of the time that repack takes on it.\n"},{"id":"233652","messageId":"20140123225238.GB2567@sigill.intra.peff.net","threadId":"35717","inReplyTo":"52E080C1.4030402@fb.com","subject":"[PATCH] pack-objects: turn off bitmaps when skipping objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-23T22:52:39Z","receivedAt":"2014-01-23T22:52:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 22, 2014 at 06:38:57PM -0800, Siddharth Agarwal wrote:\n\n> Running git-next, writing bitmap indexes fails if a keep file is\n> present from an earlier pack.\n\nRight, that's expected.\n\nThe bitmap format cannot represent objects that are not present in the\npack. So we cannot write a bitmap index if any object reachable from a\npacked commit is omitted from the pack.\n\nWe could be nicer and downgrade it to a warning, though. The patch below\ndoes that.\n\n> In our case we have .keep files lying around from ages ago (possibly\n> due to kill -9s run on the server).\n\nWe ran into that problem at GitHub, too. We just turn off\n`--honor-pack-keep` during our repacks, as we never want them on anyway\n(and we would prefer to ignore the .keep than to abort the bitmap).\n\n> It also means that running repack -a with bitmap writing enabled on a\n> repo becomes problematic if a fetch is run concurrently.\n\nFor the most part, no. The .keep file should generally only be set\nduring the period between indexing the pack and updating the refs (so\nwhile checking connectivity and running hooks). But pack-objects starts\nfrom the ref tips and walks backwards. Until they are updated, it will\nnot try to pack the objects in the .keep files, as nobody references\nthem. There are two loopholes, though:\n\n  1. In some instances, a remote may send an object we already have\n     (e.g., because it is a blob referenced in an old commit, but newly\n     referenced again due to a revert; we do not do a full object\n     difference during the protocol negotiation, for reasons of\n     efficiency). If that is the case, we may omit it if pack-objects\n     starts during the period that the .pack and .keep files exist.\n\n  2. Once the fetch updates the refs, it removes the .keep file. But\n     this isn't atomic. A repack which starts between the two may pick\n     up the new ref values, but also see the .keep file.\n\nThese are both unlikely, but possible on a very busy repository. The\npatch below will downgrade each to a warning, rather than aborting the\nrepack.\n\nSo this should just work out of the box with this patch.  But if bitmaps\nare important to you (say, you are running a very busy site and want\nto make sure you always have bitmaps turned on) and you do not otherwise\ncare about .keep files, you may want to disable them, too.\n\n-Peff\n\n-- >8 --\nSubject: pack-objects: turn off bitmaps when skipping objects\n\nThe pack bitmap format requires that we have a single bit\nfor each object in the pack, and that each object's bitmap\nrepresents its complete set of reachable objects. Therefore\nwe have no way to represent the bitmap of an object which\nreferences objects outside the pack.\n\nWe notice this problem while generating the bitmaps, as we\ntry to find the offset of a particular object and realize\nthat we do not have it. In this case we die, and neither the\nbitmap nor the pack is generated. This is correct, but\nperhaps a little unfriendly. If you have bitmaps turned on\nin the config, many repacks will fail which would otherwise\nsucceed. E.g., incremental repacks, repacks with \"-l\" when\nyou have alternates, \".keep\" files.\n\nInstead, this patch notices early that we are omitting some\nobjects from the pack and turns off bitmaps (with a\nwarning). Note that this is not strictly correct, as it's\npossible that the object being omitted is not reachable from\nany other object in the pack. In practice, this is almost\nnever the case, and there are two advantages to doing it\nthis way:\n\n  1. The code is much simpler, as we do not have to cleanly\n     abort the bitmap-generation process midway through.\n\n  2. We do not waste time partially generating bitmaps only\n     to find out that some object deep in the history is not\n     being packed.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI tried to keep the warning to an 80-character line without making it\ntoo confusing. Suggestions welcome if it doesn't make sense to people.\n\n builtin/pack-objects.c  | 12 +++++++++++-\n t/t5310-pack-bitmaps.sh |  5 ++++-\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 8364fbd..76831d9 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1000,6 +1000,10 @@ static void create_object_entry(const unsigned char *sha1,\n \tentry->no_try_delta = no_try_delta;\n }\n \n+static const char no_closure_warning[] = N_(\n+\"disabling bitmap writing, as some objects are not being packed\"\n+);\n+\n static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \t\t\t    const char *name, int exclude)\n {\n@@ -1010,8 +1014,14 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n \tif (have_duplicate_entry(sha1, exclude, &index_pos))\n \t\treturn 0;\n \n-\tif (!want_object_in_pack(sha1, exclude, &found_pack, &found_offset))\n+\tif (!want_object_in_pack(sha1, 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\twrite_bitmap_index = 0;\n+\t\t}\n \t\treturn 0;\n+\t}\n \n \tcreate_object_entry(sha1, type, pack_name_hash(name),\n \t\t\t    exclude, name && no_try_delta(name),\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex d3a3afa..f13525c 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -91,7 +91,10 @@ test_expect_success 'fetch (partial bitmap)' '\n \n test_expect_success 'incremental repack cannot create bitmaps' '\n \ttest_commit more-1 &&\n-\ttest_must_fail git repack -d\n+\tfind .git/objects/pack -name \"*.bitmap\" >expect &&\n+\tgit repack -d &&\n+\tfind .git/objects/pack -name \"*.bitmap\" >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'incremental repack can disable bitmaps' '\n-- \n1.8.5.2.500.g8060133\n"},{"id":"233658","messageId":"52E1A99D.6010809@fb.com","threadId":"35717","inReplyTo":"20140123225238.GB2567@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: turn off bitmaps when skipping objects","fromName":"Siddharth Agarwal","fromEmail":"sid0@fb.com","sentAt":"2014-01-23T23:45:33Z","receivedAt":"2014-01-23T23:45:33Z","isPatch":true,"sender":{"key":"sid0@fb.com","avatar":null},"body":"On 01/23/2014 02:52 PM, Jeff King wrote:\n> Right, that's expected.\n>\n> The bitmap format cannot represent objects that are not present in the\n> pack. So we cannot write a bitmap index if any object reachable from a\n> packed commit is omitted from the pack.\n>\n> We could be nicer and downgrade it to a warning, though. The patch below\n> does that.\n\nThis makes sense.\n\n>> In our case we have .keep files lying around from ages ago (possibly\n>> due to kill -9s run on the server).\n> We ran into that problem at GitHub, too. We just turn off\n> `--honor-pack-keep` during our repacks, as we never want them on anyway\n> (and we would prefer to ignore the .keep than to abort the bitmap).\n\nYes, we'd prefer to do that too. How do you actually do this, though? I \ndon't see a way to pass `--honor-pack-keep` (shouldn't I pass in its \ninverse?) down to `git-pack-objects`.\n\n>> It also means that running repack -a with bitmap writing enabled on a\n>> repo becomes problematic if a fetch is run concurrently.\n> For the most part, no. The .keep file should generally only be set\n> during the period between indexing the pack and updating the refs (so\n> while checking connectivity and running hooks). But pack-objects starts\n> from the ref tips and walks backwards. Until they are updated, it will\n> not try to pack the objects in the .keep files, as nobody references\n> them.\n\nThe worry is less certain objects not being packed and more the old \npacks being deleted by git repack, isn't it? From the man page for \ngit-index-pack:\n\n--keep\nBefore moving the index into its final destination create an empty .keep \nfile for the associated pack file. This option is usually necessary with\n--stdin to prevent a simultaneous git repack process from deleting the \nnewly constructed pack and index before refs can be updated to use \nobjects contained in the pack.\n\nI could be misunderstanding things here, though. From the description in \nthe man page it's not clear what the actual failure mode here is.\n\n> There are two loopholes, though:\n>\n>    1. In some instances, a remote may send an object we already have\n>       (e.g., because it is a blob referenced in an old commit, but newly\n>       referenced again due to a revert; we do not do a full object\n>       difference during the protocol negotiation, for reasons of\n>       efficiency). If that is the case, we may omit it if pack-objects\n>       starts during the period that the .pack and .keep files exist.\n>\n>    2. Once the fetch updates the refs, it removes the .keep file. But\n>       this isn't atomic. A repack which starts between the two may pick\n>       up the new ref values, but also see the .keep file.\n>\n> These are both unlikely, but possible on a very busy repository. The\n> patch below will downgrade each to a warning, rather than aborting the\n> repack.\n>\n> So this should just work out of the box with this patch.  But if bitmaps\n> are important to you (say, you are running a very busy site and want\n> to make sure you always have bitmaps turned on) and you do not otherwise\n> care about .keep files, you may want to disable them, too.\n\nWe need to make sure bitmaps are always turned on, but we need to be \neven more certain that pushes don't fail due to races.\n\n> -Peff\n>\n> -- >8 --\n> Subject: pack-objects: turn off bitmaps when skipping objects\n>\n> The pack bitmap format requires that we have a single bit\n> for each object in the pack, and that each object's bitmap\n> represents its complete set of reachable objects. Therefore\n> we have no way to represent the bitmap of an object which\n> references objects outside the pack.\n>\n> We notice this problem while generating the bitmaps, as we\n> try to find the offset of a particular object and realize\n> that we do not have it. In this case we die, and neither the\n> bitmap nor the pack is generated. This is correct, but\n> perhaps a little unfriendly. If you have bitmaps turned on\n> in the config, many repacks will fail which would otherwise\n> succeed. E.g., incremental repacks, repacks with \"-l\" when\n> you have alternates, \".keep\" files.\n>\n> Instead, this patch notices early that we are omitting some\n> objects from the pack and turns off bitmaps (with a\n> warning). Note that this is not strictly correct, as it's\n> possible that the object being omitted is not reachable from\n> any other object in the pack. In practice, this is almost\n> never the case, and there are two advantages to doing it\n> this way:\n>\n>    1. The code is much simpler, as we do not have to cleanly\n>       abort the bitmap-generation process midway through.\n>\n>    2. We do not waste time partially generating bitmaps only\n>       to find out that some object deep in the history is not\n>       being packed.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> I tried to keep the warning to an 80-character line without making it\n> too confusing. Suggestions welcome if it doesn't make sense to people.\n>\n>   builtin/pack-objects.c  | 12 +++++++++++-\n>   t/t5310-pack-bitmaps.sh |  5 ++++-\n>   2 files changed, 15 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index 8364fbd..76831d9 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -1000,6 +1000,10 @@ static void create_object_entry(const unsigned char *sha1,\n>   \tentry->no_try_delta = no_try_delta;\n>   }\n>   \n> +static const char no_closure_warning[] = N_(\n> +\"disabling bitmap writing, as some objects are not being packed\"\n> +);\n> +\n>   static int add_object_entry(const unsigned char *sha1, enum object_type type,\n>   \t\t\t    const char *name, int exclude)\n>   {\n> @@ -1010,8 +1014,14 @@ static int add_object_entry(const unsigned char *sha1, enum object_type type,\n>   \tif (have_duplicate_entry(sha1, exclude, &index_pos))\n>   \t\treturn 0;\n>   \n> -\tif (!want_object_in_pack(sha1, exclude, &found_pack, &found_offset))\n> +\tif (!want_object_in_pack(sha1, 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\twrite_bitmap_index = 0;\n> +\t\t}\n>   \t\treturn 0;\n> +\t}\n>   \n>   \tcreate_object_entry(sha1, type, pack_name_hash(name),\n>   \t\t\t    exclude, name && no_try_delta(name),\n> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\n> index d3a3afa..f13525c 100755\n> --- a/t/t5310-pack-bitmaps.sh\n> +++ b/t/t5310-pack-bitmaps.sh\n> @@ -91,7 +91,10 @@ test_expect_success 'fetch (partial bitmap)' '\n>   \n>   test_expect_success 'incremental repack cannot create bitmaps' '\n>   \ttest_commit more-1 &&\n> -\ttest_must_fail git repack -d\n> +\tfind .git/objects/pack -name \"*.bitmap\" >expect &&\n> +\tgit repack -d &&\n> +\tfind .git/objects/pack -name \"*.bitmap\" >actual &&\n> +\ttest_cmp expect actual\n>   '\n>   \n>   test_expect_success 'incremental repack can disable bitmaps' '\n"},{"id":"233660","messageId":"52E1AB78.1000504@fb.com","threadId":"35717","inReplyTo":"52E1A99D.6010809@fb.com","subject":"Re: [PATCH] pack-objects: turn off bitmaps when skipping objects","fromName":"Siddharth Agarwal","fromEmail":"sid0@fb.com","sentAt":"2014-01-23T23:53:28Z","receivedAt":"2014-01-23T23:53:28Z","isPatch":true,"sender":{"key":"sid0@fb.com","avatar":null},"body":"On 01/23/2014 03:45 PM, Siddharth Agarwal wrote:\n>\n> The worry is less certain objects not being packed and more the old \n> packs being deleted by git repack, isn't it? From the man page for \n> git-index-pack:\n\nThis should probably be \"new pack\" and not \"old packs\", I guess. Not \nknowing much about how this actually works, I'm assuming the scenario \nhere is something like:\n\n(1) git receive-pack receives a pack P.pack and writes it to disk\n(2) git index-pack runs on P.pack\n(3) git repack runs separately, finds pack P.pack with no refs pointing \nto it, and deletes it\n(4) everything goes wrong\n\nWith a keep file, this would be averted because\n\n(1) git receive-pack receives a pack P.pack and writes it to disk\n(2) git index-pack writes a keep file for P.pack, called P.keep\n(3) git repack runs separately, finds pack P.pack with a keep file, \ndoesn't touch it\n(4) git index-pack finishes, and something updates refs to point to \nP.pack and deletes P.keep\n"},{"id":"233662","messageId":"CAFFjANQ6JkxqSfQaOXF29ETW9ecMXVQTz3x86h_tDjsRdT80HQ@mail.gmail.com","threadId":"35717","inReplyTo":"52E1A99D.6010809@fb.com","subject":"Re: [PATCH] pack-objects: turn off bitmaps when skipping objects","fromName":"Vicent Martí","fromEmail":"tanoku@gmail.com","sentAt":"2014-01-23T23:56:17Z","receivedAt":"2014-01-23T23:56:17Z","isPatch":true,"sender":{"key":"tanoku@gmail.com","avatar":"https://gravatar.com/avatar/271386991cb4c2b8f1e1ed1d059f3422cc3485de7a598f65043f70be021d095b?d=mp&s=160"},"body":"On Fri, Jan 24, 2014 at 12:45 AM, Siddharth Agarwal <sid0@fb.com> wrote:\n> Yes, we'd prefer to do that too. How do you actually do this, though? I\n> don't see a way to pass `--honor-pack-keep` (shouldn't I pass in its\n> inverse?) down to `git-pack-objects`.\n\nWe run with this patch in production, it may be of use to you:\nhttps://gist.github.com/vmg/8589317\n\nIn fact, it may be worth upstreaming too. I'll kindly ask peff to do\nit when he has a moment.\n\nApologies for not attaching the patch inline, the GMail web UI doesn't\nmix well with patch workflow.\n\nCheers,\nvmg\n"},{"id":"233676","messageId":"20140124022621.GB4521@sigill.intra.peff.net","threadId":"35717","inReplyTo":"CAFFjANQ6JkxqSfQaOXF29ETW9ecMXVQTz3x86h_tDjsRdT80HQ@mail.gmail.com","subject":"Re: [PATCH] pack-objects: turn off bitmaps when skipping objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-24T02:26:21Z","receivedAt":"2014-01-24T02:26:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 24, 2014 at 12:56:17AM +0100, Vicent Martí wrote:\n\n> On Fri, Jan 24, 2014 at 12:45 AM, Siddharth Agarwal <sid0@fb.com> wrote:\n> > Yes, we'd prefer to do that too. How do you actually do this, though? I\n> > don't see a way to pass `--honor-pack-keep` (shouldn't I pass in its\n> > inverse?) down to `git-pack-objects`.\n> \n> We run with this patch in production, it may be of use to you:\n> https://gist.github.com/vmg/8589317\n> \n> In fact, it may be worth upstreaming too. I'll kindly ask peff to do\n> it when he has a moment.\n\nI was actually looking at it earlier when I sent this message. The\ntricky thing about the patch is that it turns off --honor-pack-keep, but\ndoes _not_ teach git-repack to clean up the .keep file.\n\nWhich I think is the right and safe thing to do, as otherwise you might\nblow away a pack with .keep, even though you did not just pack its\nobjects (i.e., because it was written by a fetch or push which did not\nyet update the refs). So the safe thing is to actually duplicate those\nobjects, leave the .keep pack around, and then assume it will get\ncleaned up on the next repack.\n\nIf you _do_ have a stale .keep file, though, then that stale pack will\nhang around forever (presumably with its objects duplicated in the\n\"real\" pack).\n\nSo I think the patch is doing the right thing, but I was still figuring\nout how to explain it (and I hope I just did). I'll post it with a full\ncommit message tomorrow.\n\n-Peff\n"},{"id":"233677","messageId":"20140124022822.GC4521@sigill.intra.peff.net","threadId":"35717","inReplyTo":"52E1AB78.1000504@fb.com","subject":"Re: [PATCH] pack-objects: turn off bitmaps when skipping objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-24T02:28:22Z","receivedAt":"2014-01-24T02:28:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 23, 2014 at 03:53:28PM -0800, Siddharth Agarwal wrote:\n\n> On 01/23/2014 03:45 PM, Siddharth Agarwal wrote:\n> >\n> >The worry is less certain objects not being packed and more the old\n> >packs being deleted by git repack, isn't it? From the man page for\n> >git-index-pack:\n> \n> This should probably be \"new pack\" and not \"old packs\", I guess. Not\n> knowing much about how this actually works, I'm assuming the scenario\n> here is something like:\n> \n> (1) git receive-pack receives a pack P.pack and writes it to disk\n> (2) git index-pack runs on P.pack\n> (3) git repack runs separately, finds pack P.pack with no refs\n> pointing to it, and deletes it\n> (4) everything goes wrong\n> \n> With a keep file, this would be averted because\n> \n> (1) git receive-pack receives a pack P.pack and writes it to disk\n> (2) git index-pack writes a keep file for P.pack, called P.keep\n> (3) git repack runs separately, finds pack P.pack with a keep file,\n> doesn't touch it\n> (4) git index-pack finishes, and something updates refs to point to\n> P.pack and deletes P.keep\n\nI think your understanding is accurate here. So we want repack to\nrespect keep files for deletion, but we _not_ necessarily want\npack-objects to avoid packing an object just because it's in a pack\nmarked by .keep (see my other email).\n\n-Peff\n"},{"id":"233678","messageId":"52E1D39B.4050103@fb.com","threadId":"35717","inReplyTo":"20140124022822.GC4521@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: turn off bitmaps when skipping objects","fromName":"Siddharth Agarwal","fromEmail":"sid0@fb.com","sentAt":"2014-01-24T02:44:43Z","receivedAt":"2014-01-24T02:44:43Z","isPatch":true,"sender":{"key":"sid0@fb.com","avatar":null},"body":"On 01/23/2014 06:28 PM, Jeff King wrote:\n> I think your understanding is accurate here. So we want repack to\n> respect keep files for deletion, but we _not_ necessarily want\n> pack-objects to avoid packing an object just because it's in a pack\n> marked by .keep (see my other email).\n\nYes, that makes sense and sounds pretty safe.\n\nSo the right solution for us probably is to apply the patch Vicent \nposted, set repack.honorpackkeep to false, and also have a cron job that \ncleans up stale .keep files so that subsequent repacks clean it up.\n"},{"id":"233860","messageId":"20140128060954.GA26401@sigill.intra.peff.net","threadId":"35717","inReplyTo":"52E1D39B.4050103@fb.com","subject":"[PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-01-28T06:09:54Z","receivedAt":"2014-01-28T06:09:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 23, 2014 at 06:44:43PM -0800, Siddharth Agarwal wrote:\n\n> On 01/23/2014 06:28 PM, Jeff King wrote:\n> >I think your understanding is accurate here. So we want repack to\n> >respect keep files for deletion, but we _not_ necessarily want\n> >pack-objects to avoid packing an object just because it's in a pack\n> >marked by .keep (see my other email).\n> \n> Yes, that makes sense and sounds pretty safe.\n> \n> So the right solution for us probably is to apply the patch Vicent\n> posted, set repack.honorpackkeep to false, and also have a cron job\n> that cleans up stale .keep files so that subsequent repacks clean it\n> up.\n\nYes, that matches what we do at GitHub.\n\nHere's Vicent's patch, with documentation and an expanded commit\nmessage. I think it should be suitable for upstream git.\n\n-- >8 --\nFrom: Vicent Marti <tanoku@gmail.com>\nSubject: repack: add `repack.honorpackkeep` config var\n\nThe git-repack command always passes `--honor-pack-keep`\nto pack-objects. This has traditionally been a good thing,\nas we do not want to duplicate those objects in a new pack,\nand we are not going to delete the old pack.\n\nHowever, when bitmaps are in use, it is important for a full\nrepack to include all reachable objects, even if they may be\nduplicated in a .keep pack. Otherwise, we cannot generate\nthe bitmaps, as the on-disk format requires the set of\nobjects in the pack to be fully closed.\n\nEven if the repository does not generally have .keep files,\na simultaneous push could cause a race condition in which a\n.keep file exists at the moment of a repack. The repack may\ntry to include those objects in one of two situations:\n\n  1. The pushed .keep pack contains objects that were\n     already in the repository (e.g., blobs due to a revert of\n     an old commit).\n\n  2. Receive-pack updates the refs, making the objects\n     reachable, but before it removes the .keep file, the\n     repack runs.\n\nIn either case, we may prefer to duplicate some objects in\nthe new, full pack, and let the next repack (after the .keep\nfile is cleaned up) take care of removing them.\n\nThis patch introduces an option to disable the\n`--honor-pack-keep` option.  It is not triggered by default,\neven when pack.writeBitmaps is turned on, because its use\ndepends on your overall packing strategy and use of .keep\nfiles.\n\nNote that this option just disables the pack-objects\nbehavior. We still leave packs with a .keep in place, as we\ndo not necessarily know that we have duplicated all of their\nobjects.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIntended for the jk/pack-bitmap topic.\n\n Documentation/config.txt | 8 ++++++++\n builtin/repack.c         | 8 +++++++-\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 947e6f8..5036a10 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2128,6 +2128,14 @@ repack.usedeltabaseoffset::\n \t\"false\" and repack. Access from old Git versions over the\n \tnative protocol are unaffected by this option.\n \n+repack.honorPackKeep::\n+\tIf set to false, include objects in `.keep` files when repacking\n+\tvia `git repack`. Note that we still do not delete `.keep` packs\n+\tafter `pack-objects` finishes. This means that we may duplicate\n+\tobjects, but this makes the option safe to use when there are\n+\tconcurrent pushes or fetches. This option is generally only\n+\tuseful if you have set `pack.writeBitmaps`. Defaults to true.\n+\n rerere.autoupdate::\n \tWhen set to true, `git-rerere` updates the index with the\n \tresulting contents after it cleanly resolves conflicts using\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a9c4593..585c41d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -9,6 +9,7 @@\n #include \"argv-array.h\"\n \n static int delta_base_offset = 1;\n+static int honor_pack_keep = 1;\n static char *packdir, *packtmp;\n \n static const char *const git_repack_usage[] = {\n@@ -22,6 +23,10 @@ static int repack_config(const char *var, const char *value, void *cb)\n \t\tdelta_base_offset = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"repack.honorpackkeep\")) {\n+\t\thonor_pack_keep = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -190,10 +195,11 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \targv_array_push(&cmd_args, \"pack-objects\");\n \targv_array_push(&cmd_args, \"--keep-true-parents\");\n-\targv_array_push(&cmd_args, \"--honor-pack-keep\");\n \targv_array_push(&cmd_args, \"--non-empty\");\n \targv_array_push(&cmd_args, \"--all\");\n \targv_array_push(&cmd_args, \"--reflog\");\n+\tif (honor_pack_keep)\n+\t\targv_array_push(&cmd_args, \"--honor-pack-keep\");\n \tif (window)\n \t\targv_array_pushf(&cmd_args, \"--window=%u\", window);\n \tif (window_memory)\n-- \n1.8.5.2.500.g8060133\n"},{"id":"233862","messageId":"xmqq8uu0mpg8.fsf@gitster.dls.corp.google.com","threadId":"35717","inReplyTo":"20140128060954.GA26401@sigill.intra.peff.net","subject":"Re: [PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-01-28T09:21:43Z","receivedAt":"2014-01-28T09:21:43Z","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> The git-repack command always passes `--honor-pack-keep`\n> to pack-objects. This has traditionally been a good thing,\n> as we do not want to duplicate those objects in a new pack,\n> and we are not going to delete the old pack.\n> ...\n> Note that this option just disables the pack-objects\n> behavior. We still leave packs with a .keep in place, as we\n> do not necessarily know that we have duplicated all of their\n> objects.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Intended for the jk/pack-bitmap topic.\n\nTwo comments.\n\n - It seems that this adds a configuration variable that cannot be\n   countermanded from the command line. It has to come with either a\n   very good justification in the documentation describing why it is\n   a bad idea to even allow overriding from the command line in a\n   repository that sets it, or a command line option to let the\n   users override it. I personally prefer the latter, because that\n   will be one less thing for users to remember (i.e. \"usually you\n   can override the configured default from the command line, but\n   this variable cannot be because of these very good reasons\").\n\n - In the context of \"pack-objects\", the name \"--honor-pack-keep\"\n   makes sense; it is understood that pack-objects will _not_ remove\n   kept packfile, so \"honoring\" can only mean \"do not attempt to\n   pick objects out of kept packs to add to the pack being\n   generated.\" and there is no room for --no-honor-pack-keep to be\n   mistaken as \"you canremove the ones marked to be kept after\n   saving the still-used objects in it away.\"\n\n   But does the same name make sense in the context of \"repack\"?\n\nThanks. \n"},{"id":"235227","messageId":"20140224082459.GA32594@sigill.intra.peff.net","threadId":"35717","inReplyTo":"xmqq8uu0mpg8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-24T08:24:59Z","receivedAt":"2014-02-24T08:24:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 28, 2014 at 01:21:43AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The git-repack command always passes `--honor-pack-keep`\n> > to pack-objects. This has traditionally been a good thing,\n> > as we do not want to duplicate those objects in a new pack,\n> > and we are not going to delete the old pack.\n> > ...\n> > Note that this option just disables the pack-objects\n> > behavior. We still leave packs with a .keep in place, as we\n> > do not necessarily know that we have duplicated all of their\n> > objects.\n> >\n> > Signed-off-by: Jeff King <peff@peff.net>\n> > ---\n> > Intended for the jk/pack-bitmap topic.\n> \n> Two comments.\n\nSorry, this one slipped through the cracks. Here's a re-roll addressing\nyour comments.\n\n>  - It seems that this adds a configuration variable that cannot be\n>    countermanded from the command line. It has to come with either a\n>    very good justification in the documentation describing why it is\n>    a bad idea to even allow overriding from the command line in a\n>    repository that sets it, or a command line option to let the\n>    users override it. I personally prefer the latter, because that\n>    will be one less thing for users to remember (i.e. \"usually you\n>    can override the configured default from the command line, but\n>    this variable cannot be because of these very good reasons\").\n\nIt was less \"it is a bad idea to override\" and more \"I cannot conceive\nof any good reason to override\". And since you can always use \"git -c\",\nI didn't think it was worth cluttering repack's options.\n\nHowever, I suppose if one were explicitly bitmapping a single invocation\nwith `git repack -adb`, it might make sense to use it on the command\nline. Fixed in the re-roll.\n\n>  - In the context of \"pack-objects\", the name \"--honor-pack-keep\"\n>    makes sense; it is understood that pack-objects will _not_ remove\n>    kept packfile, so \"honoring\" can only mean \"do not attempt to\n>    pick objects out of kept packs to add to the pack being\n>    generated.\" and there is no room for --no-honor-pack-keep to be\n>    mistaken as \"you canremove the ones marked to be kept after\n>    saving the still-used objects in it away.\"\n> \n>    But does the same name make sense in the context of \"repack\"?\n\nI think the distinction you are making is to capture the second second\nfrom the docs:\n\n  If set to false, include objects in `.keep` files when repacking via\n  `git repack`. Note that we still do not delete `.keep` packs after\n  `pack-objects` finishes.\n\nThe best name I could come up with is \"--pack-keep-objects\", since that\nis literally what it is doing. I'm not wild about the name because it is\neasy to read \"keep\" as a verb (and \"pack\" as a noun). I think it's OK,\nbut suggestions are welcome.\n\n-- >8 --\nFrom: Vicent Marti <tanoku@gmail.com>\nSubject: repack: add `repack.packKeepObjects` config var\n\nThe git-repack command always passes `--honor-pack-keep`\nto pack-objects. This has traditionally been a good thing,\nas we do not want to duplicate those objects in a new pack,\nand we are not going to delete the old pack.\n\nHowever, when bitmaps are in use, it is important for a full\nrepack to include all reachable objects, even if they may be\nduplicated in a .keep pack. Otherwise, we cannot generate\nthe bitmaps, as the on-disk format requires the set of\nobjects in the pack to be fully closed.\n\nEven if the repository does not generally have .keep files,\na simultaneous push could cause a race condition in which a\n.keep file exists at the moment of a repack. The repack may\ntry to include those objects in one of two situations:\n\n  1. The pushed .keep pack contains objects that were\n     already in the repository (e.g., blobs due to a revert of\n     an old commit).\n\n  2. Receive-pack updates the refs, making the objects\n     reachable, but before it removes the .keep file, the\n     repack runs.\n\nIn either case, we may prefer to duplicate some objects in\nthe new, full pack, and let the next repack (after the .keep\nfile is cleaned up) take care of removing them.\n\nThis patch introduces an option to disable the\n`--honor-pack-keep` option.  It is not triggered by default,\neven when pack.writeBitmaps is turned on, because its use\ndepends on your overall packing strategy and use of .keep\nfiles.\n\nNote that this option just disables the pack-objects\nbehavior. We still leave packs with a .keep in place, as we\ndo not necessarily know that we have duplicated all of their\nobjects.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI added a test, too, mostly to make sure I didn't bungle the option,\nsince it's negated from its former name.\n\n Documentation/config.txt     |  5 +++++\n Documentation/git-repack.txt |  8 ++++++++\n builtin/repack.c             | 10 +++++++++-\n t/t7700-repack.sh            | 16 ++++++++++++++++\n 4 files changed, 38 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex becbade..8ad081e 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2136,6 +2136,11 @@ repack.usedeltabaseoffset::\n \t\"false\" and repack. Access from old Git versions over the\n \tnative protocol are unaffected by this option.\n \n+repack.packKeepObjects::\n+\tIf set to true, makes `git repack` act as if\n+\t`--pack-keep-objects` was passed. See linkgit:git-repack[1] for\n+\tdetails. Defaults to false.\n+\n rerere.autoupdate::\n \tWhen set to true, `git-rerere` updates the index with the\n \tresulting contents after it cleanly resolves conflicts using\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 002cfd5..0c1ffbd 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -117,6 +117,14 @@ other objects in that pack they already have locally.\n \tmust be able to refer to all reachable objects. This option\n \toverrides the setting of `pack.writebitmaps`.\n \n+--pack-keep-objects::\n+\tInclude objects in `.keep` files when repacking.  Note that we\n+\tstill do not delete `.keep` packs after `pack-objects` finishes.\n+\tThis means that we may duplicate objects, but this makes the\n+\toption safe to use when there are concurrent pushes or fetches.\n+\tThis option is generally only useful if you are writing bitmaps\n+\twith `-b` or `pack.writebitmaps`, as it ensures that the\n+\tbitmapped packfile has the necessary objects.\n \n Configuration\n -------------\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 49f5857..0785a4e 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -9,6 +9,7 @@\n #include \"argv-array.h\"\n \n static int delta_base_offset = 1;\n+static int pack_keep_objects;\n static char *packdir, *packtmp;\n \n static const char *const git_repack_usage[] = {\n@@ -22,6 +23,10 @@ static int repack_config(const char *var, const char *value, void *cb)\n \t\tdelta_base_offset = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"repack.packkeepobjects\")) {\n+\t\tpack_keep_objects = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -175,6 +180,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"limits the maximum delta depth\")),\n \t\tOPT_STRING(0, \"max-pack-size\", &max_pack_size, N_(\"bytes\"),\n \t\t\t\tN_(\"maximum size of each packfile\")),\n+\t\tOPT_BOOL(0, \"pack-keep-objects\", &pack_keep_objects,\n+\t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n \t\tOPT_END()\n \t};\n \n@@ -190,7 +197,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \targv_array_push(&cmd_args, \"pack-objects\");\n \targv_array_push(&cmd_args, \"--keep-true-parents\");\n-\targv_array_push(&cmd_args, \"--honor-pack-keep\");\n+\tif (!pack_keep_objects)\n+\t\targv_array_push(&cmd_args, \"--honor-pack-keep\");\n \targv_array_push(&cmd_args, \"--non-empty\");\n \targv_array_push(&cmd_args, \"--all\");\n \targv_array_push(&cmd_args, \"--reflog\");\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex b45bd1e..13ca93c 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -35,6 +35,22 @@ test_expect_success 'objects in packs marked .keep are not repacked' '\n \ttest -z \"$found_duplicate_object\"\n '\n \n+test_expect_success '--pack-keep-objects duplicates objects' '\n+\t# build on $objsha1, $packsha1, and .keep state from previous\n+\tgit repack -Adl --pack-keep-objects &&\n+\ttest_when_finished \"found_duplicate_object=\" &&\n+\tfor p in .git/objects/pack/*.idx; do\n+\t\tidx=$(basename $p)\n+\t\ttest \"pack-$packsha1.idx\" = \"$idx\" && continue\n+\t\tif git verify-pack -v $p | egrep \"^$objsha1\"; then\n+\t\t\tfound_duplicate_object=1\n+\t\t\techo \"DUPLICATE OBJECT FOUND\"\n+\t\t\tbreak\n+\t\tfi\n+\tdone &&\n+\ttest \"$found_duplicate_object\" = 1\n+'\n+\n test_expect_success 'loose objects in alternate ODB are not repacked' '\n \tmkdir alt_objects &&\n \techo `pwd`/alt_objects > .git/objects/info/alternates &&\n-- \n1.8.5.2.500.g8060133\n"},{"id":"235281","messageId":"xmqq1tys9vie.fsf@gitster.dls.corp.google.com","threadId":"35717","inReplyTo":"20140224082459.GA32594@sigill.intra.peff.net","subject":"Re: [PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-24T19:10:49Z","receivedAt":"2014-02-24T19:10:49Z","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> Sorry, this one slipped through the cracks. Here's a re-roll addressing\n> your comments.\n> ...\n>>  - In the context of \"pack-objects\", the name \"--honor-pack-keep\"\n>>    makes sense; it is understood that pack-objects will _not_ remove\n>>    kept packfile, so \"honoring\" can only mean \"do not attempt to\n>>    pick objects out of kept packs to add to the pack being\n>>    generated.\" and there is no room for --no-honor-pack-keep to be\n>>    mistaken as \"you canremove the ones marked to be kept after\n>>    saving the still-used objects in it away.\"\n>> \n>>    But does the same name make sense in the context of \"repack\"?\n>\n> I think the distinction you are making is to capture the second second\n> from the docs:\n>\n>   If set to false, include objects in `.keep` files when repacking via\n>   `git repack`. Note that we still do not delete `.keep` packs after\n>   `pack-objects` finishes.\n>\n> The best name I could come up with is \"--pack-keep-objects\", since that\n> is literally what it is doing. I'm not wild about the name because it is\n> easy to read \"keep\" as a verb (and \"pack\" as a noun). I think it's OK,\n> but suggestions are welcome.\n\npack-kept-objects then?\n"},{"id":"235359","messageId":"20140226101353.GA25711@sigill.intra.peff.net","threadId":"35717","inReplyTo":"xmqq1tys9vie.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-26T10:13:53Z","receivedAt":"2014-02-26T10:13:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 24, 2014 at 11:10:49AM -0800, Junio C Hamano wrote:\n\n> > The best name I could come up with is \"--pack-keep-objects\", since that\n> > is literally what it is doing. I'm not wild about the name because it is\n> > easy to read \"keep\" as a verb (and \"pack\" as a noun). I think it's OK,\n> > but suggestions are welcome.\n> \n> pack-kept-objects then?\n\nHmm. That does address my point above, but somehow the word \"kept\" feels\nawkward to me. I'm ambivalent between the two.\n\nHere's the \"kept\" version if you want to apply that.\n\n-- >8 --\nFrom: Vicent Marti <tanoku@gmail.com>\nSubject: [PATCH] repack: add `repack.packKeptObjects` config var\n\nThe git-repack command always passes `--honor-pack-keep`\nto pack-objects. This has traditionally been a good thing,\nas we do not want to duplicate those objects in a new pack,\nand we are not going to delete the old pack.\n\nHowever, when bitmaps are in use, it is important for a full\nrepack to include all reachable objects, even if they may be\nduplicated in a .keep pack. Otherwise, we cannot generate\nthe bitmaps, as the on-disk format requires the set of\nobjects in the pack to be fully closed.\n\nEven if the repository does not generally have .keep files,\na simultaneous push could cause a race condition in which a\n.keep file exists at the moment of a repack. The repack may\ntry to include those objects in one of two situations:\n\n  1. The pushed .keep pack contains objects that were\n     already in the repository (e.g., blobs due to a revert of\n     an old commit).\n\n  2. Receive-pack updates the refs, making the objects\n     reachable, but before it removes the .keep file, the\n     repack runs.\n\nIn either case, we may prefer to duplicate some objects in\nthe new, full pack, and let the next repack (after the .keep\nfile is cleaned up) take care of removing them.\n\nThis patch introduces an option to disable the\n`--honor-pack-keep` option.  It is not triggered by default,\neven when pack.writeBitmaps is turned on, because its use\ndepends on your overall packing strategy and use of .keep\nfiles.\n\nNote that this option just disables the pack-objects\nbehavior. We still leave packs with a .keep in place, as we\ndo not necessarily know that we have duplicated all of their\nobjects.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/config.txt     |  5 +++++\n Documentation/git-repack.txt |  8 ++++++++\n builtin/repack.c             | 10 +++++++++-\n t/t7700-repack.sh            | 16 ++++++++++++++++\n 4 files changed, 38 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex becbade..3a3d84f 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2136,6 +2136,11 @@ repack.usedeltabaseoffset::\n \t\"false\" and repack. Access from old Git versions over the\n \tnative protocol are unaffected by this option.\n \n+repack.packKeptObjects::\n+\tIf set to true, makes `git repack` act as if\n+\t`--pack-kept-objects` was passed. See linkgit:git-repack[1] for\n+\tdetails. Defaults to false.\n+\n rerere.autoupdate::\n \tWhen set to true, `git-rerere` updates the index with the\n \tresulting contents after it cleanly resolves conflicts using\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 002cfd5..4786a78 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -117,6 +117,14 @@ other objects in that pack they already have locally.\n \tmust be able to refer to all reachable objects. This option\n \toverrides the setting of `pack.writebitmaps`.\n \n+--pack-kept-objects::\n+\tInclude objects in `.keep` files when repacking.  Note that we\n+\tstill do not delete `.keep` packs after `pack-objects` finishes.\n+\tThis means that we may duplicate objects, but this makes the\n+\toption safe to use when there are concurrent pushes or fetches.\n+\tThis option is generally only useful if you are writing bitmaps\n+\twith `-b` or `pack.writebitmaps`, as it ensures that the\n+\tbitmapped packfile has the necessary objects.\n \n Configuration\n -------------\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 49f5857..49947b2 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -9,6 +9,7 @@\n #include \"argv-array.h\"\n \n static int delta_base_offset = 1;\n+static int pack_kept_objects;\n static char *packdir, *packtmp;\n \n static const char *const git_repack_usage[] = {\n@@ -22,6 +23,10 @@ static int repack_config(const char *var, const char *value, void *cb)\n \t\tdelta_base_offset = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"repack.packkeptobjects\")) {\n+\t\tpack_kept_objects = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -175,6 +180,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"limits the maximum delta depth\")),\n \t\tOPT_STRING(0, \"max-pack-size\", &max_pack_size, N_(\"bytes\"),\n \t\t\t\tN_(\"maximum size of each packfile\")),\n+\t\tOPT_BOOL(0, \"pack-kept-objects\", &pack_kept_objects,\n+\t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n \t\tOPT_END()\n \t};\n \n@@ -190,7 +197,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \targv_array_push(&cmd_args, \"pack-objects\");\n \targv_array_push(&cmd_args, \"--keep-true-parents\");\n-\targv_array_push(&cmd_args, \"--honor-pack-keep\");\n+\tif (!pack_kept_objects)\n+\t\targv_array_push(&cmd_args, \"--honor-pack-keep\");\n \targv_array_push(&cmd_args, \"--non-empty\");\n \targv_array_push(&cmd_args, \"--all\");\n \targv_array_push(&cmd_args, \"--reflog\");\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex b45bd1e..f8431a8 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -35,6 +35,22 @@ test_expect_success 'objects in packs marked .keep are not repacked' '\n \ttest -z \"$found_duplicate_object\"\n '\n \n+test_expect_success '--pack-kept-objects duplicates objects' '\n+\t# build on $objsha1, $packsha1, and .keep state from previous\n+\tgit repack -Adl --pack-kept-objects &&\n+\ttest_when_finished \"found_duplicate_object=\" &&\n+\tfor p in .git/objects/pack/*.idx; do\n+\t\tidx=$(basename $p)\n+\t\ttest \"pack-$packsha1.idx\" = \"$idx\" && continue\n+\t\tif git verify-pack -v $p | egrep \"^$objsha1\"; then\n+\t\t\tfound_duplicate_object=1\n+\t\t\techo \"DUPLICATE OBJECT FOUND\"\n+\t\t\tbreak\n+\t\tfi\n+\tdone &&\n+\ttest \"$found_duplicate_object\" = 1\n+'\n+\n test_expect_success 'loose objects in alternate ODB are not repacked' '\n \tmkdir alt_objects &&\n \techo `pwd`/alt_objects > .git/objects/info/alternates &&\n-- \n1.8.5.2.500.g8060133\n"},{"id":"235411","messageId":"xmqqr46p39cj.fsf@gitster.dls.corp.google.com","threadId":"35717","inReplyTo":"20140226101353.GA25711@sigill.intra.peff.net","subject":"Re: [PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-26T20:30:36Z","receivedAt":"2014-02-26T20:30:36Z","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> On Mon, Feb 24, 2014 at 11:10:49AM -0800, Junio C Hamano wrote:\n>\n>> > The best name I could come up with is \"--pack-keep-objects\", since that\n>> > is literally what it is doing. I'm not wild about the name because it is\n>> > easy to read \"keep\" as a verb (and \"pack\" as a noun). I think it's OK,\n>> > but suggestions are welcome.\n>> \n>> pack-kept-objects then?\n>\n> Hmm. That does address my point above, but somehow the word \"kept\" feels\n> awkward to me. I'm ambivalent between the two.\n\nThat word does make my backside somewhat itchy ;-)\n\nWould it help to take a step back and think what the option really\ndoes?  Perhaps we should call it --pack-all-objects, which is short\nfor --pack-all-objectsregardless-of-where-they-currently-are-stored,\nor something?  The word \"all\" gives a wrong connotation in a\ndifferent way (e.g. \"regardless of reachability\" is a possible wrong\ninterpretation), so that does not sound too good, either.\n\n\"--repack-kept-objects\"?  \"--include-kept-objects\"?\n"},{"id":"235459","messageId":"20140227112734.GC29668@sigill.intra.peff.net","threadId":"35717","inReplyTo":"xmqqr46p39cj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-27T11:27:34Z","receivedAt":"2014-02-27T11:27:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 26, 2014 at 12:30:36PM -0800, Junio C Hamano wrote:\n\n> >> pack-kept-objects then?\n> >\n> > Hmm. That does address my point above, but somehow the word \"kept\" feels\n> > awkward to me. I'm ambivalent between the two.\n> \n> That word does make my backside somewhat itchy ;-)\n> \n> Would it help to take a step back and think what the option really\n> does?  Perhaps we should call it --pack-all-objects, which is short\n> for --pack-all-objectsregardless-of-where-they-currently-are-stored,\n> or something?  The word \"all\" gives a wrong connotation in a\n> different way (e.g. \"regardless of reachability\" is a possible wrong\n> interpretation), so that does not sound too good, either.\n\nI do not think \"all\" is what we want to say. It only affects \"kept\"\nobjects, not any of the other exclusions (e.g., \"-l\").\n\n> \"--repack-kept-objects\"?  \"--include-kept-objects\"?\n\nOf all of them, I think --pack-kept-objects is probably the best. And I\nthink we are hitting diminishing returns in thinking too much more on\nthe name. :)\n\n-Peff\n"},{"id":"235486","messageId":"xmqqy50wzb2b.fsf@gitster.dls.corp.google.com","threadId":"35717","inReplyTo":"20140227112734.GC29668@sigill.intra.peff.net","subject":"Re: [PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-27T18:04:44Z","receivedAt":"2014-02-27T18:04:44Z","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> Of all of them, I think --pack-kept-objects is probably the best. And I\n> think we are hitting diminishing returns in thinking too much more on\n> the name. :)\n\nTrue enough.\n\nI wonder if it makes sense to link it with \"pack.writebitmaps\" more\ntightly, without even exposing it as a seemingly orthogonal knob\nthat can be tweaked, though.\n\nI think that is because I do not fully understand the \", because ...\"\npart of the below:\n\n>> This patch introduces an option to disable the\n>> `--honor-pack-keep` option.  It is not triggered by default,\n>> even when pack.writeBitmaps is turned on, because its use\n>> depends on your overall packing strategy and use of .keep\n>> files.\n\nIf you ask --write-bitmap-index (or have pack.writeBitmaps on), you\ndo want the bitmap-index to be written, and unless you tell\npack-objects to ignore the .keep marker, it cannot do so, no?\n\nDoes the \", because ...\" part above mean \"you may have an overall\npacking strategy to use .keep file to not ever repack some subset of\nthe objects, so we will not silently explode the kept objects into a\nnew pack\"?\n"},{"id":"235558","messageId":"20140228085546.GA11709@sigill.intra.peff.net","threadId":"35717","inReplyTo":"xmqqy50wzb2b.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-28T08:55:46Z","receivedAt":"2014-02-28T08:55:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 27, 2014 at 10:04:44AM -0800, Junio C Hamano wrote:\n\n> I wonder if it makes sense to link it with \"pack.writebitmaps\" more\n> tightly, without even exposing it as a seemingly orthogonal knob\n> that can be tweaked, though.\n> \n> I think that is because I do not fully understand the \", because ...\"\n> part of the below:\n> \n> >> This patch introduces an option to disable the\n> >> `--honor-pack-keep` option.  It is not triggered by default,\n> >> even when pack.writeBitmaps is turned on, because its use\n> >> depends on your overall packing strategy and use of .keep\n> >> files.\n> \n> If you ask --write-bitmap-index (or have pack.writeBitmaps on), you\n> do want the bitmap-index to be written, and unless you tell\n> pack-objects to ignore the .keep marker, it cannot do so, no?\n> \n> Does the \", because ...\" part above mean \"you may have an overall\n> packing strategy to use .keep file to not ever repack some subset of\n> the objects, so we will not silently explode the kept objects into a\n> new pack\"?\n\nExactly. The two features (bitmaps and .keep) are not compatible with\neach other, so you have to prioritize one. If you are using static .keep\nfiles, you might want them to continue being respected at the expense of\nusing bitmaps for that repo. So I think you want a separate option from\n--write-bitmap-index to allow the appropriate flexibility.\n\nThe default is another matter.  I think most people using .bitmaps on a\nserver would probably want to set repack.packKeptObjects.  They would\nwant to repack often to take advantage of the .bitmaps anyway, so they\nprobably don't care about .keep files (any they see are due to races\nwith incoming pushes).\n\nSo we could do something like falling back to turning the option on if\n--write-bitmap-index is on _and_ the user didn't specify\n--pack-kept-objects. The existing default is mostly there because it is\nthe conservative choice (.keep files continue to do their thing as\nnormal unless you say otherwise). But the fallback thing would be one\nless knob that bitmap users would need to turn in the common case.\n\nHere's the interdiff for doing the fallback:\n\n---\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 3a3d84f..a8ddc7f 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2139,7 +2139,9 @@ repack.usedeltabaseoffset::\n repack.packKeptObjects::\n \tIf set to true, makes `git repack` act as if\n \t`--pack-kept-objects` was passed. See linkgit:git-repack[1] for\n-\tdetails. Defaults to false.\n+\tdetails. Defaults to `false` normally, but `true` if a bitmap\n+\tindex is being written (either via `--write-bitmap-index` or\n+\t`pack.writeBitmaps`).\n \n rerere.autoupdate::\n \tWhen set to true, `git-rerere` updates the index with the\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 49947b2..6b0b62d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -9,7 +9,7 @@\n #include \"argv-array.h\"\n \n static int delta_base_offset = 1;\n-static int pack_kept_objects;\n+static int pack_kept_objects = -1;\n static char *packdir, *packtmp;\n \n static const char *const git_repack_usage[] = {\n@@ -190,6 +190,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, builtin_repack_options,\n \t\t\t\tgit_repack_usage, 0);\n \n+\tif (pack_kept_objects < 0)\n+\t\tpack_kept_objects = write_bitmap;\n+\n \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n \tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n \ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex f8431a8..b1eed5c 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -21,7 +21,7 @@ test_expect_success 'objects in packs marked .keep are not repacked' '\n \tobjsha1=$(git verify-pack -v pack-$packsha1.idx | head -n 1 |\n \t\tsed -e \"s/^\\([0-9a-f]\\{40\\}\\).*/\\1/\") &&\n \tmv pack-* .git/objects/pack/ &&\n-\tgit repack -A -d -l &&\n+\tgit repack --no-pack-kept-objects -A -d -l &&\n \tgit prune-packed &&\n \tfor p in .git/objects/pack/*.idx; do\n \t\tidx=$(basename $p)\n@@ -35,9 +35,9 @@ test_expect_success 'objects in packs marked .keep are not repacked' '\n \ttest -z \"$found_duplicate_object\"\n '\n \n-test_expect_success '--pack-kept-objects duplicates objects' '\n+test_expect_success 'writing bitmaps duplicates .keep objects' '\n \t# build on $objsha1, $packsha1, and .keep state from previous\n-\tgit repack -Adl --pack-kept-objects &&\n+\tgit repack -Adl &&\n \ttest_when_finished \"found_duplicate_object=\" &&\n \tfor p in .git/objects/pack/*.idx; do\n \t\tidx=$(basename $p)\n"},{"id":"235639","messageId":"2E523500-558A-42CF-A761-618DD2821347@codeaurora.org","threadId":"35717","inReplyTo":"20140228085546.GA11709@sigill.intra.peff.net","subject":"Re: [PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Nasser Grainawi","fromEmail":"nasser@codeaurora.org","sentAt":"2014-02-28T17:09:08Z","receivedAt":"2014-02-28T17:09:08Z","isPatch":true,"sender":{"key":"nasser@codeaurora.org","avatar":"https://avatars.githubusercontent.com/u/757421?v=4"},"body":"On Feb 28, 2014, at 1:55 AM, Jeff King <peff@peff.net> wrote:\n\n> On Thu, Feb 27, 2014 at 10:04:44AM -0800, Junio C Hamano wrote:\n> \n>> I wonder if it makes sense to link it with \"pack.writebitmaps\" more\n>> tightly, without even exposing it as a seemingly orthogonal knob\n>> that can be tweaked, though.\n>> \n>> I think that is because I do not fully understand the \", because ...\"\n>> part of the below:\n>> \n>>>> This patch introduces an option to disable the\n>>>> `--honor-pack-keep` option.  It is not triggered by default,\n>>>> even when pack.writeBitmaps is turned on, because its use\n>>>> depends on your overall packing strategy and use of .keep\n>>>> files.\n>> \n>> If you ask --write-bitmap-index (or have pack.writeBitmaps on), you\n>> do want the bitmap-index to be written, and unless you tell\n>> pack-objects to ignore the .keep marker, it cannot do so, no?\n>> \n>> Does the \", because ...\" part above mean \"you may have an overall\n>> packing strategy to use .keep file to not ever repack some subset of\n>> the objects, so we will not silently explode the kept objects into a\n>> new pack\"?\n> \n> Exactly. The two features (bitmaps and .keep) are not compatible with\n> each other, so you have to prioritize one. If you are using static .keep\n> files, you might want them to continue being respected at the expense of\n> using bitmaps for that repo. So I think you want a separate option from\n> --write-bitmap-index to allow the appropriate flexibility.\n\nHas anyone thought about how to make them compatible? We're using Martin Fick's git-exproll script which makes heavy use of keeps to reduce pack file churn. In addition to the on-disk benefits we get there, the driving factor behind creating exproll was to prevent Gerrit from having two large (30GB+) mostly duplicated pack files open in memory at the same time. Repacking in JGit would help in a single-master environment, but we'd be back to having this problem once we go to a multi-master setup.\n\nPerhaps the solution here is actually something in JGit where it could aggressively try to close references to pack files, but that still doesn't help the disk churn problem. As Peff says below, we would want to repack often to get up-to-date bitmaps, but ideally we could do that without writing hundreds of GBs to disk (which is obviously worse when \"disk\" is a NFS mount).\n\n> \n> The default is another matter.  I think most people using .bitmaps on a\n> server would probably want to set repack.packKeptObjects.  They would\n> want to repack often to take advantage of the .bitmaps anyway, so they\n> probably don't care about .keep files (any they see are due to races\n> with incoming pushes).\n> \n> So we could do something like falling back to turning the option on if\n> --write-bitmap-index is on _and_ the user didn't specify\n> --pack-kept-objects. The existing default is mostly there because it is\n> the conservative choice (.keep files continue to do their thing as\n> normal unless you say otherwise). But the fallback thing would be one\n> less knob that bitmap users would need to turn in the common case.\n> \n> Here's the interdiff for doing the fallback:\n> \n> ---\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 3a3d84f..a8ddc7f 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -2139,7 +2139,9 @@ repack.usedeltabaseoffset::\n> repack.packKeptObjects::\n> \tIf set to true, makes `git repack` act as if\n> \t`--pack-kept-objects` was passed. See linkgit:git-repack[1] for\n> -\tdetails. Defaults to false.\n> +\tdetails. Defaults to `false` normally, but `true` if a bitmap\n> +\tindex is being written (either via `--write-bitmap-index` or\n> +\t`pack.writeBitmaps`).\n> \n> rerere.autoupdate::\n> \tWhen set to true, `git-rerere` updates the index with the\n> diff --git a/builtin/repack.c b/builtin/repack.c\n> index 49947b2..6b0b62d 100644\n> --- a/builtin/repack.c\n> +++ b/builtin/repack.c\n> @@ -9,7 +9,7 @@\n> #include \"argv-array.h\"\n> \n> static int delta_base_offset = 1;\n> -static int pack_kept_objects;\n> +static int pack_kept_objects = -1;\n> static char *packdir, *packtmp;\n> \n> static const char *const git_repack_usage[] = {\n> @@ -190,6 +190,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n> \targc = parse_options(argc, argv, prefix, builtin_repack_options,\n> \t\t\t\tgit_repack_usage, 0);\n> \n> +\tif (pack_kept_objects < 0)\n> +\t\tpack_kept_objects = write_bitmap;\n> +\n> \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n> \tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n> \n> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\n> index f8431a8..b1eed5c 100755\n> --- a/t/t7700-repack.sh\n> +++ b/t/t7700-repack.sh\n> @@ -21,7 +21,7 @@ test_expect_success 'objects in packs marked .keep are not repacked' '\n> \tobjsha1=$(git verify-pack -v pack-$packsha1.idx | head -n 1 |\n> \t\tsed -e \"s/^\\([0-9a-f]\\{40\\}\\).*/\\1/\") &&\n> \tmv pack-* .git/objects/pack/ &&\n> -\tgit repack -A -d -l &&\n> +\tgit repack --no-pack-kept-objects -A -d -l &&\n> \tgit prune-packed &&\n> \tfor p in .git/objects/pack/*.idx; do\n> \t\tidx=$(basename $p)\n> @@ -35,9 +35,9 @@ test_expect_success 'objects in packs marked .keep are not repacked' '\n> \ttest -z \"$found_duplicate_object\"\n> '\n> \n> -test_expect_success '--pack-kept-objects duplicates objects' '\n> +test_expect_success 'writing bitmaps duplicates .keep objects' '\n> \t# build on $objsha1, $packsha1, and .keep state from previous\n> -\tgit repack -Adl --pack-kept-objects &&\n> +\tgit repack -Adl &&\n> \ttest_when_finished \"found_duplicate_object=\" &&\n> \tfor p in .git/objects/pack/*.idx; do\n> \t\tidx=$(basename $p)\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"235653","messageId":"xmqqob1ruld8.fsf@gitster.dls.corp.google.com","threadId":"35717","inReplyTo":"20140228085546.GA11709@sigill.intra.peff.net","subject":"Re: [PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-28T18:45:39Z","receivedAt":"2014-02-28T18:45:39Z","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> On Thu, Feb 27, 2014 at 10:04:44AM -0800, Junio C Hamano wrote:\n>\n>> I wonder if it makes sense to link it with \"pack.writebitmaps\" more\n>> tightly, without even exposing it as a seemingly orthogonal knob\n>> that can be tweaked, though.\n>> \n>> I think that is because I do not fully understand the \", because ...\"\n>> part of the below:\n>> \n>> >> This patch introduces an option to disable the\n>> >> `--honor-pack-keep` option.  It is not triggered by default,\n>> >> even when pack.writeBitmaps is turned on, because its use\n>> >> depends on your overall packing strategy and use of .keep\n>> >> files.\n>> \n>> If you ask --write-bitmap-index (or have pack.writeBitmaps on), you\n>> do want the bitmap-index to be written, and unless you tell\n>> pack-objects to ignore the .keep marker, it cannot do so, no?\n>> \n>> Does the \", because ...\" part above mean \"you may have an overall\n>> packing strategy to use .keep file to not ever repack some subset of\n>> the objects, so we will not silently explode the kept objects into a\n>> new pack\"?\n>\n> Exactly. The two features (bitmaps and .keep) are not compatible with\n> each other, so you have to prioritize one. If you are using static .keep\n> files, you might want them to continue being respected at the expense of\n> using bitmaps for that repo. So I think you want a separate option from\n> --write-bitmap-index to allow the appropriate flexibility.\n\nWhat is \"the appropriate flexibility\", though?  If the user wants to\nuse bitmap, we would need to drop .keep, no?  Doesn't always having\ntwo copies in two packs degrade performance unnecessarily (without\neven talking about wasted diskspace)?  An explicit .keep exists in\nthe repository because it is expensive and undesirable to duplicate\nwhat is in there in the first place, so it feels to me that either\n\n - Disable with warning, or outright refuse, the \"-b\" option if\n   there is .keep (if we want to give precedence to .keep); or\n\n - Remove .keep with warning when \"-b\" option is given (if we want\n   to give precedence to \"-b\").\n\nand nothing else would be a reasonable option.  Unfortunately, we\ncan do neither automatically because there could be a transient .keep\nfile in an active repository.\n\nSo I think I agree with this...\n\n> The default is another matter.  I think most people using .bitmaps on a\n> server would probably want to set repack.packKeptObjects.  They would\n> want to repack often to take advantage of the .bitmaps anyway, so they\n> probably don't care about .keep files (any they see are due to races\n> with incoming pushes).\n\n... which makes me think that repack.packKeptObjects is merely a\ndistraction---it should be enough to just pass \"--pack-kept-objects\"\nwhen \"-b\" is asked, without giving any extra configurability, no?\n\n> So we could do something like falling back to turning the option on if\n> --write-bitmap-index is on _and_ the user didn't specify\n> --pack-kept-objects.\n\nIf you mean \"didn't specify --no-pack-kept-objects\", then I think\nthat is sensible.  I still do not know why we would want the\nconfiguration variable, though.\n"},{"id":"235693","messageId":"20140301054350.GA20397@sigill.intra.peff.net","threadId":"35717","inReplyTo":"xmqqob1ruld8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-01T05:43:50Z","receivedAt":"2014-03-01T05:43:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 28, 2014 at 10:45:39AM -0800, Junio C Hamano wrote:\n\n> > Exactly. The two features (bitmaps and .keep) are not compatible with\n> > each other, so you have to prioritize one. If you are using static .keep\n> > files, you might want them to continue being respected at the expense of\n> > using bitmaps for that repo. So I think you want a separate option from\n> > --write-bitmap-index to allow the appropriate flexibility.\n> \n> What is \"the appropriate flexibility\", though?  If the user wants to\n> use bitmap, we would need to drop .keep, no?\n\nOr the flip side: if the user wants to use .keep, we should drop\nbitmaps. My point is that we do not know which way the user wants to\ngo, so we should not tie the options together.\n\n> Doesn't always having two copies in two packs degrade performance\n> unnecessarily (without even talking about wasted diskspace)?  An\n> explicit .keep exists in the repository because it is expensive and\n> undesirable to duplicate what is in there in the first place, so it\n> feels to me that either\n\nThe benefits of static .keep files are (I think):\n\n  1. less I/O during repacks, as you do not rewrite a static set of\n     objects\n\n  2. less turnover of packfiles, which can make dumb access more\n     efficient (both for dumb clients, but also for things like\n     non-git-aware backups).\n\nI think the existence of smart-http more or less nullifies (2). For (1),\nit helps at first, but you get diminishing returns as your non-keep\npackfile grows. I think it only helps in pathological cases (e.g., you\nmark 10GB worth of giant blobs in a .keep pack, and then pack the other\n10MB of trees, commits, and normal-sized blobs as usual).\n\n>  - Disable with warning, or outright refuse, the \"-b\" option if\n>    there is .keep (if we want to give precedence to .keep); or\n> \n>  - Remove .keep with warning when \"-b\" option is given (if we want\n>    to give precedence to \"-b\").\n> \n> and nothing else would be a reasonable option.  Unfortunately, we\n> can do neither automatically because there could be a transient .keep\n> file in an active repository.\n\nRight, the transient ones complicate the issue. But I think even for\nstatic .keep versus bitmaps, there is question. See below...\n\n> > The default is another matter.  I think most people using .bitmaps on a\n> > server would probably want to set repack.packKeptObjects.  They would\n> > want to repack often to take advantage of the .bitmaps anyway, so they\n> > probably don't care about .keep files (any they see are due to races\n> > with incoming pushes).\n> \n> ... which makes me think that repack.packKeptObjects is merely a\n> distraction---it should be enough to just pass \"--pack-kept-objects\"\n> when \"-b\" is asked, without giving any extra configurability, no?\n\nBut you do not necessarily ask for \"-b\" explicitly; it might come from\nthe config, too. Imagine you have a server with many repos. You want to\nuse bitmaps when you can, so you set pack.writeBitmaps in\n/etc/gitconfig. But in a few repos, you want to use .keep files, and\nit's more important for you to use it than bitmaps (e.g., because it is\none of the pathological cases above). So you set repack.packKeptObjects\nto false in /etc/gitconfig, to prefer .keep to bitmaps where\nappropriate.\n\nIf you did not have that config option, your alternative would be to\nturn off pack.writeBitmaps in the repositories with .keep files. But\nthen you need to per-repo keep that flag in sync with whether or not the\nrepo has .keep files.\n\nTo be clear, at GitHub we do not plan on ever having\nrepack.packKeptObjects off (for now we have it on explicitly, but if it\nwere connected to pack.writeBitmaps, then we would be happy with that\ndefault). I am mostly trying to give an escape hatch to let people use\ndifferent optimization strategies if they want.\n\nIf we are going to have --pack-kept-objects (and I think we should),\nI think we also should have a matching config option. Because it is\nuseful when matched with the bitmap code, and that can be turned on both\nfrom the command-line or from the config. Wherever you are doing that,\nyou would want to be able to make the matching .keep decision.\n\nAnd I don't think it hurts much.  With the fallback-to-on behavior, you\ndo not have to even care that it is there unless you are doing something\nclever.\n\n> > So we could do something like falling back to turning the option on if\n> > --write-bitmap-index is on _and_ the user didn't specify\n> > --pack-kept-objects.\n> \n> If you mean \"didn't specify --no-pack-kept-objects\", then I think\n> that is sensible.  I still do not know why we would want the\n> configuration variable, though.\n\nRight, I meant \"if the pack-kept-objects variable is set, either on or\noff, either via the command line or in the config\".\n\nAs far as what the config is good for, see above.\n\n-Peff\n"},{"id":"235694","messageId":"20140301060550.GB20397@sigill.intra.peff.net","threadId":"35717","inReplyTo":"2E523500-558A-42CF-A761-618DD2821347@codeaurora.org","subject":"Re: [PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-01T06:05:50Z","receivedAt":"2014-03-01T06:05:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 28, 2014 at 10:09:08AM -0700, Nasser Grainawi wrote:\n\n> > Exactly. The two features (bitmaps and .keep) are not compatible with\n> > each other, so you have to prioritize one. If you are using static .keep\n> > files, you might want them to continue being respected at the expense of\n> > using bitmaps for that repo. So I think you want a separate option from\n> > --write-bitmap-index to allow the appropriate flexibility.\n> \n> Has anyone thought about how to make them compatible?\n\nYes, but it's complicated and not likely to happen soon.\n\nHaving .keep files means that you are not including some objects in the\nnewly created pack. Each bit in a commit's bitmap corresponds to one\nobject in the pack, and whether it is reachable from that commit. The\nbitmap is only useful if we can calculate the full reachability from it,\nand it has no way to specify objects outside of the pack.\n\nTo fix this, you would need to change the on-disk format of the bitmaps\nto somehow reference objects outside of the pack. Either by having the\nbitmaps index a repo-global set of objects, or by permitting a list of\n\"edge\" objects that are referenced from the pack, but not included (and\nthen when assembling the full reachable list, you would have to recurse\nacross \"edge\" objects to find their reachable list in another pack,\netc).\n\nSo it's possible, but it would complicate the scheme quite a bit, and\nwould not be backwards compatible with either JGit or C Git.\n\n> We're using Martin Fick's git-exproll script which makes heavy use of\n> keeps to reduce pack file churn. In addition to the on-disk benefits\n> we get there, the driving factor behind creating exproll was to\n> prevent Gerrit from having two large (30GB+) mostly duplicated pack\n> files open in memory at the same time. Repacking in JGit would help in\n> a single-master environment, but we'd be back to having this problem\n> once we go to a multi-master setup.\n> \n> Perhaps the solution here is actually something in JGit where it could\n> aggressively try to close references to pack files\n\nIn C git we don't worry about this too much, because our programs tend\nto be short-lived, and references to the old pack will go away quickly.\nPlus it is all mmap'd, so as we simply stop accessing the pages of the\nold pack, they should eventually be dropped if there is memory pressure.\n\nI seem to recall that JGit does not mmap its packfiles. Does it pread?\nIn that case, I'd expect unused bits from the duplicated packfile to get\ndropped from the disk cache over time. If it loads whole packfiles into\nmemory, then yes, it should probably close more aggressively.\n\n> , but that still\n> doesn't help the disk churn problem. As Peff says below, we would want\n> to repack often to get up-to-date bitmaps, but ideally we could do\n> that without writing hundreds of GBs to disk (which is obviously worse\n> when \"disk\" is a NFS mount).\n\nUltimately I think the solution to the churn problem is a packfile-like\nstorage that allows true appending of deltas. You can come up with a\nscheme to allow deltas between on-disk packs (i.e., \"thin\" packs on\ndisk). The trick there is handling the dependencies and cycles. I think\nyou could get by with a strict ordering of packs and a few rules:\n\n  1. An object in a pack with weight A cannot have as a base an object\n     in a pack with weight <= A.\n\n  2. A pack with weight A cannot be deleted if there exists a pack with\n     weight > A.\n\nBut you'd want to also add in a single update-able index over all the\npackfiles, and even then you'd still want to pack occasionally (because\nyou'd end up with deltas on bases going back in time, but you really\nprefer your bases to be near the tip of history).\n\nSo I am not volunteering to work on it. :)\n\nAt GitHub we mostly deal with the churn by throwing more server\nresources at it. But we have the advantage of having a very large number\nof small-to-medium repos, which is relatively easy to scale up. A small\nnumber of huge repos is trickier.\n\n-Peff\n"},{"id":"235892","messageId":"xmqqeh2jrvz8.fsf@gitster.dls.corp.google.com","threadId":"35717","inReplyTo":"20140301054350.GA20397@sigill.intra.peff.net","subject":"Re: [PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-03T18:13:47Z","receivedAt":"2014-03-03T18:13:47Z","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> On Fri, Feb 28, 2014 at 10:45:39AM -0800, Junio C Hamano wrote:\n>\n>> > Exactly. The two features (bitmaps and .keep) are not compatible with\n>> > each other, so you have to prioritize one. If you are using static .keep\n>> > files, you might want them to continue being respected at the expense of\n>> > using bitmaps for that repo. So I think you want a separate option from\n>> > --write-bitmap-index to allow the appropriate flexibility.\n>> \n>> What is \"the appropriate flexibility\", though?  If the user wants to\n>> use bitmap, we would need to drop .keep, no?\n>\n> Or the flip side: if the user wants to use .keep, we should drop\n> bitmaps. My point is that we do not know which way the user wants to\n> go, so we should not tie the options together.\n\nHmph.  I think the short of your later explanation is \"global config\nmay tell us to use bitmap, in which case we would need a way to\ndefeat that and have existing .keep honored, and it makes it easier\nto do so if these two are kept separate, because you do not want to\nrun around and selectively disable bitmaps in these repositories.\nWe can instead do so with repack.packKeptObjects in the global\nconfiguration.\" and I tend to agree with the reasoning.\n\nThanks.\n"},{"id":"235893","messageId":"20140303181558.GA16523@sigill.intra.peff.net","threadId":"35717","inReplyTo":"xmqqeh2jrvz8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-03T18:15:58Z","receivedAt":"2014-03-03T18:15:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 03, 2014 at 10:13:47AM -0800, Junio C Hamano wrote:\n\n> > Or the flip side: if the user wants to use .keep, we should drop\n> > bitmaps. My point is that we do not know which way the user wants to\n> > go, so we should not tie the options together.\n> \n> Hmph.  I think the short of your later explanation is \"global config\n> may tell us to use bitmap, in which case we would need a way to\n> defeat that and have existing .keep honored, and it makes it easier\n> to do so if these two are kept separate, because you do not want to\n> run around and selectively disable bitmaps in these repositories.\n> We can instead do so with repack.packKeptObjects in the global\n> configuration.\" and I tend to agree with the reasoning.\n\nYes. Do you need a re-roll from me? I think the last version I sent +\nthe squash to tie the default to bitmap-writing makes the most sense.\n\n-Peff\n"},{"id":"235907","messageId":"CAJo=hJthjtsa7uuW-Zq5uWrJYfk1G6mzkEq_toPMtjATwDMKTA@mail.gmail.com","threadId":"35717","inReplyTo":"20140301060550.GB20397@sigill.intra.peff.net","subject":"Re: [PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2014-03-03T19:12:29Z","receivedAt":"2014-03-03T19:12:29Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Fri, Feb 28, 2014 at 10:05 PM, Jeff King <peff@peff.net> wrote:\n> On Fri, Feb 28, 2014 at 10:09:08AM -0700, Nasser Grainawi wrote:\n>\n>> > Exactly. The two features (bitmaps and .keep) are not compatible with\n>> > each other, so you have to prioritize one. If you are using static .keep\n>> > files, you might want them to continue being respected at the expense of\n>> > using bitmaps for that repo. So I think you want a separate option from\n>> > --write-bitmap-index to allow the appropriate flexibility.\n>>\n>> Has anyone thought about how to make them compatible?\n>\n> Yes, but it's complicated and not likely to happen soon.\n>\n> Having .keep files means that you are not including some objects in the\n> newly created pack. Each bit in a commit's bitmap corresponds to one\n> object in the pack, and whether it is reachable from that commit. The\n> bitmap is only useful if we can calculate the full reachability from it,\n> and it has no way to specify objects outside of the pack.\n>\n> To fix this, you would need to change the on-disk format of the bitmaps\n> to somehow reference objects outside of the pack. Either by having the\n> bitmaps index a repo-global set of objects, or by permitting a list of\n> \"edge\" objects that are referenced from the pack, but not included (and\n> then when assembling the full reachable list, you would have to recurse\n> across \"edge\" objects to find their reachable list in another pack,\n> etc).\n>\n> So it's possible, but it would complicate the scheme quite a bit, and\n> would not be backwards compatible with either JGit or C Git.\n\nColby Ranger always wanted to add this to the bitmap scheme. Construct\na partial pack bitmap on a partial pack of \"recent\" objects, with edge\npointers naming objects that are not in this pack but whose closures\nneed to be considered part of the bitmap. Its complicated in-memory\nbecause you need to fuse together two or more bitmaps (the partial\npack one, and the larger historical kept pack) before running the\n\"want AND NOT have\" computation.\n\nColby did not find time to work on this in JGit, so it just didn't get\nimplemented. But we did consider it, as the servers at Google we built\nbitmap for use a multi-level pack scheme and don't want to rebuild\npacks all of the time.\n\n>> We're using Martin Fick's git-exproll script which makes heavy use of\n>> keeps to reduce pack file churn. In addition to the on-disk benefits\n>> we get there, the driving factor behind creating exproll was to\n>> prevent Gerrit from having two large (30GB+) mostly duplicated pack\n>> files open in memory at the same time. Repacking in JGit would help in\n>> a single-master environment, but we'd be back to having this problem\n>> once we go to a multi-master setup.\n>>\n>> Perhaps the solution here is actually something in JGit where it could\n>> aggressively try to close references to pack files\n>\n> In C git we don't worry about this too much, because our programs tend\n> to be short-lived, and references to the old pack will go away quickly.\n> Plus it is all mmap'd, so as we simply stop accessing the pages of the\n> old pack, they should eventually be dropped if there is memory pressure.\n>\n> I seem to recall that JGit does not mmap its packfiles. Does it pread?\n\nJGit does not mmap because you can't munmap() until the Java GC gets\naround to freeing the tiny little header object that contains the\nmemory address of the start of the mmap segment. This can take ages,\nto the point where you run out of virtual address space in the process\nand s**t starts to fail left and right inside of the JVM. The GC is\njust unable to prioritize finding those tiny headers and getting them\nout of the heap so the munmap can take place safely.\n\nSo yea, JGit does pread() for the blocks but it holds those in its own\nbuffer cache inside of the Java heap. Where a 4K disk block is a 4K\nmemory array that puts pressure on the GC to actually wake up and free\nresources that are unused. What Nasser is talking about is JGit may\ntake a long time to realize one pack is unused and start kicking those\nblocks out of its buffer cache. Those blocks are reference counted and\nthe file descriptor JGit preads from is held open so long as at least\none block is in the buffer cache. By keeping the file open we force\nthe filesystem to keep the inode alive a lot longer, which means the\ndisk needs a huge amount of free space to store the unlinked but still\nopen 30G pack files from prior GC generations.\n\n> In that case, I'd expect unused bits from the duplicated packfile to get\n> dropped from the disk cache over time. If it loads whole packfiles into\n> memory, then yes, it should probably close more aggressively.\n\nIts more than that, its the inode being kept alive by the open file\ndescriptor...\n"},{"id":"235911","messageId":"xmqqeh2joyc5.fsf@gitster.dls.corp.google.com","threadId":"35717","inReplyTo":"20140303181558.GA16523@sigill.intra.peff.net","subject":"Re: [PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-03T19:51:06Z","receivedAt":"2014-03-03T19:51:06Z","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> On Mon, Mar 03, 2014 at 10:13:47AM -0800, Junio C Hamano wrote:\n>\n>> > Or the flip side: if the user wants to use .keep, we should drop\n>> > bitmaps. My point is that we do not know which way the user wants to\n>> > go, so we should not tie the options together.\n>> \n>> Hmph.  I think the short of your later explanation is \"global config\n>> may tell us to use bitmap, in which case we would need a way to\n>> defeat that and have existing .keep honored, and it makes it easier\n>> to do so if these two are kept separate, because you do not want to\n>> run around and selectively disable bitmaps in these repositories.\n>> We can instead do so with repack.packKeptObjects in the global\n>> configuration.\" and I tend to agree with the reasoning.\n>\n> Yes. Do you need a re-roll from me? I think the last version I sent +\n> the squash to tie the default to bitmap-writing makes the most sense.\n\nI have 9e20b390 (repack: add `repack.packKeptObjects` config var,\n2014-02-26); I do not recall I've squashed anything into it, though.\n\nDo you mean this one?\n\nHere's the interdiff for doing the fallback:\n\n---\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 3a3d84f..a8ddc7f 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2139,7 +2139,9 @@ repack.usedeltabaseoffset::\n repack.packKeptObjects::\n \tIf set to true, makes `git repack` act as if\n \t`--pack-kept-objects` was passed. See linkgit:git-repack[1] for\n-\tdetails. Defaults to false.\n+\tdetails. Defaults to `false` normally, but `true` if a bitmap\n+\tindex is being written (either via `--write-bitmap-index` or\n+\t`pack.writeBitmaps`).\n \n rerere.autoupdate::\n \tWhen set to true, `git-rerere` updates the index with the\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 49947b2..6b0b62d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -9,7 +9,7 @@\n #include \"argv-array.h\"\n \n static int delta_base_offset = 1;\n-static int pack_kept_objects;\n+static int pack_kept_objects = -1;\n static char *packdir, *packtmp;\n \n static const char *const git_repack_usage[] = {\n@@ -190,6 +190,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, builtin_repack_options,\n \t\t\t\tgit_repack_usage, 0);\n \n+\tif (pack_kept_objects < 0)\n+\t\tpack_kept_objects = write_bitmap;\n+\n \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n \tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n \ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex f8431a8..b1eed5c 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -21,7 +21,7 @@ test_expect_success 'objects in packs marked .keep are not repacked' '\n \tobjsha1=$(git verify-pack -v pack-$packsha1.idx | head -n 1 |\n \t\tsed -e \"s/^\\([0-9a-f]\\{40\\}\\).*/\\1/\") &&\n \tmv pack-* .git/objects/pack/ &&\n-\tgit repack -A -d -l &&\n+\tgit repack --no-pack-kept-objects -A -d -l &&\n \tgit prune-packed &&\n \tfor p in .git/objects/pack/*.idx; do\n \t\tidx=$(basename $p)\n@@ -35,9 +35,9 @@ test_expect_success 'objects in packs marked .keep are not repacked' '\n \ttest -z \"$found_duplicate_object\"\n '\n \n-test_expect_success '--pack-kept-objects duplicates objects' '\n+test_expect_success 'writing bitmaps duplicates .keep objects' '\n \t# build on $objsha1, $packsha1, and .keep state from previous\n-\tgit repack -Adl --pack-kept-objects &&\n+\tgit repack -Adl &&\n \ttest_when_finished \"found_duplicate_object=\" &&\n \tfor p in .git/objects/pack/*.idx; do\n \t\tidx=$(basename $p)\n"},{"id":"235915","messageId":"20140303200420.GA20675@sigill.intra.peff.net","threadId":"35717","inReplyTo":"xmqqeh2joyc5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] repack: add `repack.honorpackkeep` config var","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-03T20:04:20Z","receivedAt":"2014-03-03T20:04:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 03, 2014 at 11:51:06AM -0800, Junio C Hamano wrote:\n\n> > Yes. Do you need a re-roll from me? I think the last version I sent +\n> > the squash to tie the default to bitmap-writing makes the most sense.\n> \n> I have 9e20b390 (repack: add `repack.packKeptObjects` config var,\n> 2014-02-26); I do not recall I've squashed anything into it, though.\n> \n> Do you mean this one?\n> \n> Here's the interdiff for doing the fallback:\n> [...]\n\nYes. Though I just noticed that the commit message needs updating if\nthat is squashed in. Here is the whole patch, with that update.\n\nAnd I am dropping Vicent as the author, since I think there are now\nliterally zero lines of his left. ;)\n\n-- >8 --\nSubject: [PATCH] repack: add `repack.packKeptObjects` config var\n\nThe git-repack command always passes `--honor-pack-keep`\nto pack-objects. This has traditionally been a good thing,\nas we do not want to duplicate those objects in a new pack,\nand we are not going to delete the old pack.\n\nHowever, when bitmaps are in use, it is important for a full\nrepack to include all reachable objects, even if they may be\nduplicated in a .keep pack. Otherwise, we cannot generate\nthe bitmaps, as the on-disk format requires the set of\nobjects in the pack to be fully closed.\n\nEven if the repository does not generally have .keep files,\na simultaneous push could cause a race condition in which a\n.keep file exists at the moment of a repack. The repack may\ntry to include those objects in one of two situations:\n\n  1. The pushed .keep pack contains objects that were\n     already in the repository (e.g., blobs due to a revert of\n     an old commit).\n\n  2. Receive-pack updates the refs, making the objects\n     reachable, but before it removes the .keep file, the\n     repack runs.\n\nIn either case, we may prefer to duplicate some objects in\nthe new, full pack, and let the next repack (after the .keep\nfile is cleaned up) take care of removing them.\n\nThis patch introduces both a command-line and config option\nto disable the `--honor-pack-keep` option.  By default, it\nis triggered when pack.writeBitmaps (or `--write-bitmap-index`\nis turned on), but specifying it explicitly can override the\nbehavior (e.g., in cases where you prefer .keep files to\nbitmaps, but only when they are present).\n\nNote that this option just disables the pack-objects\nbehavior. We still leave packs with a .keep in place, as we\ndo not necessarily know that we have duplicated all of their\nobjects.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/config.txt     |  7 +++++++\n Documentation/git-repack.txt |  8 ++++++++\n builtin/repack.c             | 13 ++++++++++++-\n t/t7700-repack.sh            | 18 +++++++++++++++++-\n 4 files changed, 44 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex becbade..a8ddc7f 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2136,6 +2136,13 @@ repack.usedeltabaseoffset::\n \t\"false\" and repack. Access from old Git versions over the\n \tnative protocol are unaffected by this option.\n \n+repack.packKeptObjects::\n+\tIf set to true, makes `git repack` act as if\n+\t`--pack-kept-objects` was passed. See linkgit:git-repack[1] for\n+\tdetails. Defaults to `false` normally, but `true` if a bitmap\n+\tindex is being written (either via `--write-bitmap-index` or\n+\t`pack.writeBitmaps`).\n+\n rerere.autoupdate::\n \tWhen set to true, `git-rerere` updates the index with the\n \tresulting contents after it cleanly resolves conflicts using\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 002cfd5..4786a78 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -117,6 +117,14 @@ other objects in that pack they already have locally.\n \tmust be able to refer to all reachable objects. This option\n \toverrides the setting of `pack.writebitmaps`.\n \n+--pack-kept-objects::\n+\tInclude objects in `.keep` files when repacking.  Note that we\n+\tstill do not delete `.keep` packs after `pack-objects` finishes.\n+\tThis means that we may duplicate objects, but this makes the\n+\toption safe to use when there are concurrent pushes or fetches.\n+\tThis option is generally only useful if you are writing bitmaps\n+\twith `-b` or `pack.writebitmaps`, as it ensures that the\n+\tbitmapped packfile has the necessary objects.\n \n Configuration\n -------------\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 49f5857..6b0b62d 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -9,6 +9,7 @@\n #include \"argv-array.h\"\n \n static int delta_base_offset = 1;\n+static int pack_kept_objects = -1;\n static char *packdir, *packtmp;\n \n static const char *const git_repack_usage[] = {\n@@ -22,6 +23,10 @@ static int repack_config(const char *var, const char *value, void *cb)\n \t\tdelta_base_offset = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"repack.packkeptobjects\")) {\n+\t\tpack_kept_objects = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \treturn git_default_config(var, value, cb);\n }\n \n@@ -175,6 +180,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"limits the maximum delta depth\")),\n \t\tOPT_STRING(0, \"max-pack-size\", &max_pack_size, N_(\"bytes\"),\n \t\t\t\tN_(\"maximum size of each packfile\")),\n+\t\tOPT_BOOL(0, \"pack-kept-objects\", &pack_kept_objects,\n+\t\t\t\tN_(\"repack objects in packs marked with .keep\")),\n \t\tOPT_END()\n \t};\n \n@@ -183,6 +190,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, builtin_repack_options,\n \t\t\t\tgit_repack_usage, 0);\n \n+\tif (pack_kept_objects < 0)\n+\t\tpack_kept_objects = write_bitmap;\n+\n \tpackdir = mkpathdup(\"%s/pack\", get_object_directory());\n \tpacktmp = mkpathdup(\"%s/.tmp-%d-pack\", packdir, (int)getpid());\n \n@@ -190,7 +200,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \targv_array_push(&cmd_args, \"pack-objects\");\n \targv_array_push(&cmd_args, \"--keep-true-parents\");\n-\targv_array_push(&cmd_args, \"--honor-pack-keep\");\n+\tif (!pack_kept_objects)\n+\t\targv_array_push(&cmd_args, \"--honor-pack-keep\");\n \targv_array_push(&cmd_args, \"--non-empty\");\n \targv_array_push(&cmd_args, \"--all\");\n \targv_array_push(&cmd_args, \"--reflog\");\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex b45bd1e..b1eed5c 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -21,7 +21,7 @@ test_expect_success 'objects in packs marked .keep are not repacked' '\n \tobjsha1=$(git verify-pack -v pack-$packsha1.idx | head -n 1 |\n \t\tsed -e \"s/^\\([0-9a-f]\\{40\\}\\).*/\\1/\") &&\n \tmv pack-* .git/objects/pack/ &&\n-\tgit repack -A -d -l &&\n+\tgit repack --no-pack-kept-objects -A -d -l &&\n \tgit prune-packed &&\n \tfor p in .git/objects/pack/*.idx; do\n \t\tidx=$(basename $p)\n@@ -35,6 +35,22 @@ test_expect_success 'objects in packs marked .keep are not repacked' '\n \ttest -z \"$found_duplicate_object\"\n '\n \n+test_expect_success 'writing bitmaps can duplicate .keep objects' '\n+\t# build on $objsha1, $packsha1, and .keep state from previous\n+\tgit repack -Adl &&\n+\ttest_when_finished \"found_duplicate_object=\" &&\n+\tfor p in .git/objects/pack/*.idx; do\n+\t\tidx=$(basename $p)\n+\t\ttest \"pack-$packsha1.idx\" = \"$idx\" && continue\n+\t\tif git verify-pack -v $p | egrep \"^$objsha1\"; then\n+\t\t\tfound_duplicate_object=1\n+\t\t\techo \"DUPLICATE OBJECT FOUND\"\n+\t\t\tbreak\n+\t\tfi\n+\tdone &&\n+\ttest \"$found_duplicate_object\" = 1\n+'\n+\n test_expect_success 'loose objects in alternate ODB are not repacked' '\n \tmkdir alt_objects &&\n \techo `pwd`/alt_objects > .git/objects/info/alternates &&\n-- \n1.8.5.2.500.g8060133\n"}]}