{"thread":{"id":"45848","subject":"[PATCH] pack-objects: disable pack reuse for object-selection options","startedAt":"2017-05-02T08:43:40Z","lastAt":"2017-05-09T03:08:24Z","messageCount":11,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"318476","messageId":"20170502084326.65eisqmr4th5cbf7@sigill.intra.peff.net","threadId":"45848","inReplyTo":null,"subject":"[PATCH] pack-objects: disable pack reuse for object-selection options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-02T08:43:26Z","receivedAt":"2017-05-02T08:43:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If certain options like --honor-pack-keep, --local, or\n--incremental are used with pack-objects, then we need to\nfeed each potential object to want_object_in_pack() to see\nif it should be filtered out.  This is totally contrary to\nthe purpose of the pack-reuse optimization, which tries hard\nto avoid doing any per-object work.  Therefore we need to\ndisable this optimization when these options are in use.\n\nThis bug has been present since the inception of the\npack-reuse code, but was unlikely to come up in practice.\nThese options are generally used for on-disk packing, not\ntransfer packs (which go to stdout), but we've never allowed\npack reuse for non-stdout packs (until 645c432d6, we did not\neven use bitmaps, which the reuse optimization relies on;\nafter that, we explicitly turned it off when not packing to\nstdout).\n\nThere are tests in t5310 that check these options with\nbitmaps and --stdout, but they didn't catch the bug, and\nit's hard to adapt them to do so.\n\nOne problem is that they don't use --delta-base-offset;\nwithout that option, we always disable the reuse\noptimization entirely. It would be fine to add it in (it\nactually makes the test more realistic), but that still\nisn't quite enough.\n\nAnother problem is that the reuse code is very picky; it\nonly kicks in when it can reuse most of a pack, starting\nfrom the first byte. So we'd have to start from a fully\nrepacked and bitmapped state to trigger it. But the tests\nfor these options use a much more subtle state; they want to\nbe sure that the want_object_in_pack() code is allowing some\nobjects but not others. Doing a full repack runs counter to\nthat.\n\nSo this patch adds new tests at the end of the script which\ncreate the fully-packed state and make sure that each option\nis not fooled by reusable pack.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI happened to notice this because I have a series which makes the reuse\ncode much less picky (it kicks in more often, and can even convert to\nnon-ofs-delta clients on the fly). And it fails the tests when merged\nwith 702d1b958 (pack-objects: respect --local/--honor-pack-keep/--incremental\nwhen bitmap is in use, 2016-09-10).\n\nBut the bug is much older than that.  So this isn't at all urgent for\nv2.13.\n\n builtin/pack-objects.c  |  6 +++++-\n t/t5310-pack-bitmaps.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 0fe35d1b5..50e01aa80 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2717,7 +2717,11 @@ static void loosen_unused_packed_objects(struct rev_info *revs)\n  */\n static int pack_options_allow_reuse(void)\n {\n-\treturn pack_to_stdout && allow_ofs_delta;\n+\treturn pack_to_stdout &&\n+\t       allow_ofs_delta &&\n+\t       !ignore_packed_keep &&\n+\t       (!local || !have_non_local_packs) &&\n+\t       !incremental;\n }\n \n static int get_object_list_from_bitmap(struct rev_info *revs)\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 424bec7d7..c3ddfa217 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -289,4 +289,42 @@ test_expect_success 'splitting packs does not generate bogus bitmaps' '\n \tgit -C no-bitmaps.git fetch .. HEAD\n '\n \n+test_expect_success 'set up reusable pack' '\n+\trm -f .git/objects/pack/*.keep &&\n+\tgit repack -adb &&\n+\treusable_pack () {\n+\t\tgit for-each-ref --format=\"%(objectname)\" |\n+\t\tgit pack-objects --delta-base-offset --revs --stdout \"$@\"\n+\t}\n+'\n+\n+test_expect_success 'pack reuse respects --honor-pack-keep' '\n+\ttest_when_finished \"rm -f .git/objects/pack/*.keep\" &&\n+\tfor i in .git/objects/pack/*.pack; do\n+\t\t>${i%.pack}.keep\n+\tdone &&\n+\treusable_pack --honor-pack-keep >empty.pack &&\n+\tgit index-pack empty.pack &&\n+\t>expect &&\n+\tgit show-index <empty.idx >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pack reuse respects --local' '\n+\tmv .git/objects/pack/* alt.git/objects/pack/ &&\n+\ttest_when_finished \"mv alt.git/objects/pack/* .git/objects/pack/\" &&\n+\treusable_pack --local >empty.pack &&\n+\tgit index-pack empty.pack &&\n+\t>expect &&\n+\tgit show-index <empty.idx >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pack reuse respects --incremental' '\n+\treusable_pack --incremental >empty.pack &&\n+\tgit index-pack empty.pack &&\n+\t>expect &&\n+\tgit show-index <empty.idx >actual &&\n+\ttest_cmp expect actual\n+'\n test_done\n-- \n2.13.0.rc1.437.g927e4246e\n"},{"id":"319050","messageId":"xmqq4lww9cas.fsf@gitster.mtv.corp.google.com","threadId":"45848","inReplyTo":"20170502084326.65eisqmr4th5cbf7@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: disable pack reuse for object-selection options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-08T04:56:27Z","receivedAt":"2017-05-08T04:56:37Z","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> If certain options like --honor-pack-keep, --local, or\n> --incremental are used with pack-objects, then we need to\n> feed each potential object to want_object_in_pack() to see\n> if it should be filtered out.  This is totally contrary to\n> the purpose of the pack-reuse optimization, which tries hard\n> to avoid doing any per-object work.  Therefore we need to\n> disable this optimization when these options are in use.\n\nI read this five times, as I wanted to understand what you are\nsaying, but I am not sure if I got it right.  One of the reasons why\nI was confused was that I originally thought this \"reuse\" was about\ndelta reuse, but it is not.  It is \"sending a slice of the original\npackfile straight to the output\".  But even after getting myself out\nof that confusion, I still do not see why we \"need to disable\".\nSurely, even if we need to exclude some objects from an existing\npackfile due to these selection options, we should be able to reuse\nthe non-excluded part, no?  The end result may involve having to\npick and reuse more and smaller slices from existing packfiles,\nwhich may be much less efficient, but it is no immediately obvious\nto me if it leads to \"need to disable\".  I would understand it if it\nwere \"it becomes much less efficient and we are better off not using\nthe bitmap code at all\", though.\n\nIs the real reason why we want to disable the \"reuse\" because it is\nnot easy to update the reuse_partial_packfile_from_bitmap() logic to\ntake these selection options into account?  If so, I would also\nunderstand why this is a good change.\n\nPuzzled.\n\n> This bug has been present since the inception of the\n> pack-reuse code, but was unlikely to come up in practice.\n\n\n> +test_expect_success 'pack reuse respects --honor-pack-keep' '\n> +\ttest_when_finished \"rm -f .git/objects/pack/*.keep\" &&\n> +\tfor i in .git/objects/pack/*.pack; do\n> +\t\t>${i%.pack}.keep\n> +\tdone &&\n\nMicronit: style.\n\n"},{"id":"319067","messageId":"20170508073143.lu73w5b54lvstty2@sigill.intra.peff.net","threadId":"45848","inReplyTo":"xmqq4lww9cas.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pack-objects: disable pack reuse for object-selection options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-08T07:31:43Z","receivedAt":"2017-05-08T07:31:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 08, 2017 at 01:56:27PM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > If certain options like --honor-pack-keep, --local, or\n> > --incremental are used with pack-objects, then we need to\n> > feed each potential object to want_object_in_pack() to see\n> > if it should be filtered out.  This is totally contrary to\n> > the purpose of the pack-reuse optimization, which tries hard\n> > to avoid doing any per-object work.  Therefore we need to\n> > disable this optimization when these options are in use.\n> \n> I read this five times, as I wanted to understand what you are\n> saying, but I am not sure if I got it right.  One of the reasons why\n> I was confused was that I originally thought this \"reuse\" was about\n> delta reuse, but it is not.  It is \"sending a slice of the original\n> packfile straight to the output\".\n\nRight. The \"send a slice\" goes under the name \"reuse_packfile\" in the\ncode, but I'm not surprised you're not familiar with it. We added it\nlong ago as part of the bitmap feature, and it's not often talked about\n(and in its current incarnation, isn't actually very useful; I have\npatches to improve that, but haven't gotten around to upstreaming them).\n\n> But even after getting myself out\n> of that confusion, I still do not see why we \"need to disable\".\n> Surely, even if we need to exclude some objects from an existing\n> packfile due to these selection options, we should be able to reuse\n> the non-excluded part, no?  The end result may involve having to\n> pick and reuse more and smaller slices from existing packfiles,\n> which may be much less efficient, but it is no immediately obvious\n> to me if it leads to \"need to disable\".  I would understand it if it\n> were \"it becomes much less efficient and we are better off not using\n> the bitmap code at all\", though.\n\nYes, it's this last bit. The main win of the packfile reuse code is that\nit builds on the bitmaps to avoid doing as much per-object work as\npossible. So the objects don't even get added to the list of \"struct\nobject_entry\", and we never consider them for the \"should they be in the\nresult\" checks beyond the have/want computation done by the bitmaps.\n\nWe could add those checks in, but what's the point? The idea of the\nreuse code is to be a fast path for serving vanilla clones. Searching\nthrough all of the packfiles for a .keep entry is the antithesis of\nthat.\n\n> Is the real reason why we want to disable the \"reuse\" because it is\n> not easy to update the reuse_partial_packfile_from_bitmap() logic to\n> take these selection options into account?  If so, I would also\n> understand why this is a good change.\n\nThis is a side concern for the current form. With the patches I\nmentioned above, it's not too hard to omit some objects and still reuse\nthe other bits verbatim. But again, even evaluating the function for\neach pack is too expensive, even if the implementation supported it.\n\n> > +test_expect_success 'pack reuse respects --honor-pack-keep' '\n> > +\ttest_when_finished \"rm -f .git/objects/pack/*.keep\" &&\n> > +\tfor i in .git/objects/pack/*.pack; do\n> > +\t\t>${i%.pack}.keep\n> > +\tdone &&\n> \n> Micronit: style.\n\nI assume you mean putting \"do\" on a separate line. Sorry, the \"; do\" is\nmy native tongue and I sometimes slip back into it without thinking.\n\n-Peff\n"},{"id":"319131","messageId":"xmqqd1bi7ta5.fsf@gitster.mtv.corp.google.com","threadId":"45848","inReplyTo":"20170508073143.lu73w5b54lvstty2@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: disable pack reuse for object-selection options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-09T00:44:50Z","receivedAt":"2017-05-09T00:44:58Z","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, May 08, 2017 at 01:56:27PM +0900, Junio C Hamano wrote:\n>\n>> Surely, even if we need to exclude some objects from an existing\n>> packfile due to these selection options, we should be able to reuse\n>> the non-excluded part, no?  The end result may involve having to\n>> pick and reuse more and smaller slices from existing packfiles,\n>> which may be much less efficient, but it is no immediately obvious\n>> to me if it leads to \"need to disable\".  I would understand it if it\n>> were \"it becomes much less efficient and we are better off not using\n>> the bitmap code at all\", though.\n>\n> Yes, it's this last bit. The main win of the packfile reuse code is that\n> it builds on the bitmaps to avoid doing as much per-object work as\n> possible. So the objects don't even get added to the list of \"struct\n> object_entry\", and we never consider them for the \"should they be in the\n> result\" checks beyond the have/want computation done by the bitmaps.\n>\n> We could add those checks in, but what's the point? The idea of the\n> reuse code is to be a fast path for serving vanilla clones. Searching\n> through all of the packfiles for a .keep entry is the antithesis of\n> that.\n\nAh, OK, and now I understand why you called this a \"bug\" (which is\nolder and do not need to be addressed as part of 2.13) in the\noriginal message.  The new tests check requests that ought to\nproduce an empty packfile as the result actually do, but with the\ncurrent code, the reuse code does not work with --local and friends\nand ends up including what was requested to be excluded.\n\nThanks.\n"},{"id":"319136","messageId":"20170509020010.meefcustv7uufard@sigill.intra.peff.net","threadId":"45848","inReplyTo":"xmqqd1bi7ta5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pack-objects: disable pack reuse for object-selection options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-09T02:00:10Z","receivedAt":"2017-05-09T02:00:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 09, 2017 at 09:44:50AM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Mon, May 08, 2017 at 01:56:27PM +0900, Junio C Hamano wrote:\n> >\n> >> Surely, even if we need to exclude some objects from an existing\n> >> packfile due to these selection options, we should be able to reuse\n> >> the non-excluded part, no?  The end result may involve having to\n> >> pick and reuse more and smaller slices from existing packfiles,\n> >> which may be much less efficient, but it is no immediately obvious\n> >> to me if it leads to \"need to disable\".  I would understand it if it\n> >> were \"it becomes much less efficient and we are better off not using\n> >> the bitmap code at all\", though.\n> >\n> > Yes, it's this last bit. The main win of the packfile reuse code is that\n> > it builds on the bitmaps to avoid doing as much per-object work as\n> > possible. So the objects don't even get added to the list of \"struct\n> > object_entry\", and we never consider them for the \"should they be in the\n> > result\" checks beyond the have/want computation done by the bitmaps.\n> >\n> > We could add those checks in, but what's the point? The idea of the\n> > reuse code is to be a fast path for serving vanilla clones. Searching\n> > through all of the packfiles for a .keep entry is the antithesis of\n> > that.\n> \n> Ah, OK, and now I understand why you called this a \"bug\" (which is\n> older and do not need to be addressed as part of 2.13) in the\n> original message.  The new tests check requests that ought to\n> produce an empty packfile as the result actually do, but with the\n> current code, the reuse code does not work with --local and friends\n> and ends up including what was requested to be excluded.\n\nRight. Did you want me to try re-wording the commit message, or does it\nmake sense now?\n\n-Peff\n"},{"id":"319140","messageId":"xmqqfuge6akl.fsf@gitster.mtv.corp.google.com","threadId":"45848","inReplyTo":"20170509020010.meefcustv7uufard@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: disable pack reuse for object-selection options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-09T02:14:18Z","receivedAt":"2017-05-09T02:14:24Z","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>> Ah, OK, and now I understand why you called this a \"bug\" (which is\n>> older and do not need to be addressed as part of 2.13) in the\n>> original message.  The new tests check requests that ought to\n>> produce an empty packfile as the result actually do, but with the\n>> current code, the reuse code does not work with --local and friends\n>> and ends up including what was requested to be excluded.\n>\n> Right. Did you want me to try re-wording the commit message, or does it\n> make sense now?\n\nIt does make sense to me now, but I do not speak for all future\nreaders of \"git log\", so...\n"},{"id":"319141","messageId":"20170509022114.s3tpxrgtplsopr4x@sigill.intra.peff.net","threadId":"45848","inReplyTo":"xmqqfuge6akl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pack-objects: disable pack reuse for object-selection options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-09T02:21:14Z","receivedAt":"2017-05-09T02:21:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 09, 2017 at 11:14:18AM +0900, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> Ah, OK, and now I understand why you called this a \"bug\" (which is\n> >> older and do not need to be addressed as part of 2.13) in the\n> >> original message.  The new tests check requests that ought to\n> >> produce an empty packfile as the result actually do, but with the\n> >> current code, the reuse code does not work with --local and friends\n> >> and ends up including what was requested to be excluded.\n> >\n> > Right. Did you want me to try re-wording the commit message, or does it\n> > make sense now?\n> \n> It does make sense to me now, but I do not speak for all future\n> readers of \"git log\", so...\n\nI guess what I was asking was: do you still think it was unclear, or do\nyou think you were just being dense?\n\nI don't feel like I gave any information in the follow-on explanation\nthat wasn't in the commit message, so I wasn't clear if I worded it\nbetter or if it just sunk in better.\n\n-Peff\n"},{"id":"319145","messageId":"xmqq4lwu68zy.fsf@gitster.mtv.corp.google.com","threadId":"45848","inReplyTo":"20170509022114.s3tpxrgtplsopr4x@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: disable pack reuse for object-selection options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-09T02:48:17Z","receivedAt":"2017-05-09T02:48:23Z","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 Tue, May 09, 2017 at 11:14:18AM +0900, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> >> Ah, OK, and now I understand why you called this a \"bug\" (which is\n>> >> older and do not need to be addressed as part of 2.13) in the\n>> >> original message.  The new tests check requests that ought to\n>> >> produce an empty packfile as the result actually do, but with the\n>> >> current code, the reuse code does not work with --local and friends\n>> >> and ends up including what was requested to be excluded.\n>> >\n>> > Right. Did you want me to try re-wording the commit message, or does it\n>> > make sense now?\n>> \n>> It does make sense to me now, but I do not speak for all future\n>> readers of \"git log\", so...\n>\n> I guess what I was asking was: do you still think it was unclear, or do\n> you think you were just being dense?\n>\n> I don't feel like I gave any information in the follow-on explanation\n> that wasn't in the commit message, so I wasn't clear if I worded it\n> better or if it just sunk in better.\n\nAt least, \"the current code is buggy when --local and friend are\ngiven and includes needless objects in the result\" was something I\nlearned only during the discussion, and would never have guessed by\nreading the log message.  The second paragraph does talk about \"This\nbug has been present since...\", but the first paragraph does not say\nanything about the current output being broken.\n\nSo, I am not sure if this was a case where I was dense.  I think the\nfirst paragraph needs a bit more clarity.\n\n    If certain options like --honor-pack-keep, --local, or\n    --incremental are used with pack-objects, then we need to\n    feed each potential object to want_object_in_pack() to see\n    if it should be filtered out.  This is totally contrary to\n    the purpose of the pack-reuse optimization, which tries hard\n    to avoid doing any per-object work.  Therefore we need to\n    disable this optimization when these options are in use.\n\nPerhaps \"... should be filtered out.\" can be followed by \"However,\nthe current code fails to do so, and we end up including these\nunwanted objects in the output.\", and then \"This is totally...\"  can\ninstead begin with \"Besides, having to do per-object filtering is\ntotally...\".  I wouldn't have been confused if it were like so.\n\n"},{"id":"319147","messageId":"20170509025412.6fkrlnbsdk6qmoia@sigill.intra.peff.net","threadId":"45848","inReplyTo":"xmqq4lwu68zy.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pack-objects: disable pack reuse for object-selection options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-09T02:54:13Z","receivedAt":"2017-05-09T02:54:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 09, 2017 at 11:48:17AM +0900, Junio C Hamano wrote:\n\n> > I guess what I was asking was: do you still think it was unclear, or do\n> > you think you were just being dense?\n> >\n> > I don't feel like I gave any information in the follow-on explanation\n> > that wasn't in the commit message, so I wasn't clear if I worded it\n> > better or if it just sunk in better.\n> \n> At least, \"the current code is buggy when --local and friend are\n> given and includes needless objects in the result\" was something I\n> learned only during the discussion, and would never have guessed by\n> reading the log message.  The second paragraph does talk about \"This\n> bug has been present since...\", but the first paragraph does not say\n> anything about the current output being broken.\n\nWhile waiting for your response I took a look to see if I could improve\nit and came to the same conclusion. The result is below.\n\n> So, I am not sure if this was a case where I was dense.  I think the\n> first paragraph needs a bit more clarity.\n\nYeah, you were not being dense. I just wrote it badly. Sorry\nfor the confusion.\n\n-- >8 --\nSubject: pack-objects: disable pack reuse for object-selection options\n\nIf certain options like --honor-pack-keep, --local, or\n--incremental are used with pack-objects, then we need to\nfeed each potential object to want_object_in_pack() to see\nif it should be filtered out. But when the bitmap\nreuse_packfile optimization is in effect, we do not call\nthat function at all, and in fact skip adding the objects to\nthe to_pack list entirely.  This means we have a bug: for\ncertain requests we will silently ignore those options and\ninclude objects in that pack that should not be there.\n\nThe problem has been present since the inception of the\npack-reuse code in 6b8fda2db (pack-objects: use bitmaps when\npacking objects, 2013-12-21), but it was unlikely to come up\nin practice.  These options are generally used for on-disk\npacking, not transfer packs (which go to stdout), but we've\nnever allowed pack reuse for non-stdout packs (until\n645c432d6, we did not even use bitmaps, which the reuse\noptimization relies on; after that, we explicitly turned it\noff when not packing to stdout).\n\nWe can fix this by just disabling the reuse_packfile\noptimization when the options are in use. In theory we could\nteach the pack-reuse code to satisfy these checks, but it's\nnot worth the complexity. The purpose of the optimization is\nto keep the amount of per-object work we do to a minimum.\nBut these options inherently require us to search for other\ncopies of each object, drowning out any benefit of the\npack-reuse optimization. But note that the optimizations\nfrom 56dfeb626 (pack-objects: compute local/ignore_pack_keep\nearly, 2016-07-29) happen before pack-reuse, meaning that\nspecifying \"--honor-pack-keep\" in a repository with no .keep\nfiles can still follow the fast path.\n\nThere are tests in t5310 that check these options with\nbitmaps and --stdout, but they didn't catch the bug, and\nit's hard to adapt them to do so.\n\nOne problem is that they don't use --delta-base-offset;\nwithout that option, we always disable the reuse\noptimization entirely. It would be fine to add it in (it\nactually makes the test more realistic), but that still\nisn't quite enough.\n\nThe other problem is that the reuse code is very picky; it\nonly kicks in when it can reuse most of a pack, starting\nfrom the first byte. So we'd have to start from a fully\nrepacked and bitmapped state to trigger it. But the tests\nfor these options use a much more subtle state; they want to\nbe sure that the want_object_in_pack() code is allowing some\nobjects but not others. Doing a full repack runs counter to\nthat.\n\nSo this patch adds new tests at the end of the script which\ncreate the fully-packed state and make sure that each option\nis not fooled by reusable pack.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nContents are the same. I decided to leave the \"; do\" as it\nmatches the rest of the script. If we're going to fix it, we\nshould do them all.\n\n builtin/pack-objects.c  |  6 +++++-\n t/t5310-pack-bitmaps.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 0fe35d1b5..50e01aa80 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -2717,7 +2717,11 @@ static void loosen_unused_packed_objects(struct rev_info *revs)\n  */\n static int pack_options_allow_reuse(void)\n {\n-\treturn pack_to_stdout && allow_ofs_delta;\n+\treturn pack_to_stdout &&\n+\t       allow_ofs_delta &&\n+\t       !ignore_packed_keep &&\n+\t       (!local || !have_non_local_packs) &&\n+\t       !incremental;\n }\n \n static int get_object_list_from_bitmap(struct rev_info *revs)\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 424bec7d7..c3ddfa217 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -289,4 +289,42 @@ test_expect_success 'splitting packs does not generate bogus bitmaps' '\n \tgit -C no-bitmaps.git fetch .. HEAD\n '\n \n+test_expect_success 'set up reusable pack' '\n+\trm -f .git/objects/pack/*.keep &&\n+\tgit repack -adb &&\n+\treusable_pack () {\n+\t\tgit for-each-ref --format=\"%(objectname)\" |\n+\t\tgit pack-objects --delta-base-offset --revs --stdout \"$@\"\n+\t}\n+'\n+\n+test_expect_success 'pack reuse respects --honor-pack-keep' '\n+\ttest_when_finished \"rm -f .git/objects/pack/*.keep\" &&\n+\tfor i in .git/objects/pack/*.pack; do\n+\t\t>${i%.pack}.keep\n+\tdone &&\n+\treusable_pack --honor-pack-keep >empty.pack &&\n+\tgit index-pack empty.pack &&\n+\t>expect &&\n+\tgit show-index <empty.idx >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pack reuse respects --local' '\n+\tmv .git/objects/pack/* alt.git/objects/pack/ &&\n+\ttest_when_finished \"mv alt.git/objects/pack/* .git/objects/pack/\" &&\n+\treusable_pack --local >empty.pack &&\n+\tgit index-pack empty.pack &&\n+\t>expect &&\n+\tgit show-index <empty.idx >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pack reuse respects --incremental' '\n+\treusable_pack --incremental >empty.pack &&\n+\tgit index-pack empty.pack &&\n+\t>expect &&\n+\tgit show-index <empty.idx >actual &&\n+\ttest_cmp expect actual\n+'\n test_done\n-- \n2.13.0.rc2.436.g524cea07c\n\n"},{"id":"319149","messageId":"20170509025946.7lozhjwblra7i6sp@sigill.intra.peff.net","threadId":"45848","inReplyTo":"20170509025412.6fkrlnbsdk6qmoia@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: disable pack reuse for object-selection options","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-09T02:59:46Z","receivedAt":"2017-05-09T02:59:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 08, 2017 at 10:54:12PM -0400, Jeff King wrote:\n\n> Contents are the same. I decided to leave the \"; do\" as it\n> matches the rest of the script. If we're going to fix it, we\n> should do them all.\n\nLike this, perhaps.\n\nI didn't go on a crusade against \"; do\" in the other scripts, but\nperhaps that is low-hanging fruit that somebody else might want to pick.\n\n-- >8 --\nSubject: [PATCH] t5310: fix \"; do\" style\n\nOur usual shell style is to put the \"do\" of a loop on its\nown line, like:\n\n  while $cond\n  do\n          something\n  done\n\ninstead of:\n\n  while $cond; do\n          something\n  done\n\nWe have a bit of both in our code base, but the former is\nwhat's in CodingGuidelines (and outnumbers the latter in t/\nby about 6:1).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5310-pack-bitmaps.sh | 12 ++++++++----\n 1 file changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex c3ddfa217..20e2473a0 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -20,11 +20,13 @@ has_any () {\n }\n \n test_expect_success 'setup repo with moderate-sized history' '\n-\tfor i in $(test_seq 1 10); do\n+\tfor i in $(test_seq 1 10)\n+\tdo\n \t\ttest_commit $i\n \tdone &&\n \tgit checkout -b other HEAD~5 &&\n-\tfor i in $(test_seq 1 10); do\n+\tfor i in $(test_seq 1 10)\n+\tdo\n \t\ttest_commit side-$i\n \tdone &&\n \tgit checkout master &&\n@@ -104,7 +106,8 @@ test_expect_success 'clone from bitmapped repository' '\n '\n \n test_expect_success 'setup further non-bitmapped commits' '\n-\tfor i in $(test_seq 1 10); do\n+\tfor i in $(test_seq 1 10)\n+\tdo\n \t\ttest_commit further-$i\n \tdone\n '\n@@ -300,7 +303,8 @@ test_expect_success 'set up reusable pack' '\n \n test_expect_success 'pack reuse respects --honor-pack-keep' '\n \ttest_when_finished \"rm -f .git/objects/pack/*.keep\" &&\n-\tfor i in .git/objects/pack/*.pack; do\n+\tfor i in .git/objects/pack/*.pack\n+\tdo\n \t\t>${i%.pack}.keep\n \tdone &&\n \treusable_pack --honor-pack-keep >empty.pack &&\n-- \n2.13.0.rc2.436.g524cea07c\n\n"},{"id":"319150","messageId":"xmqqpofi4ti7.fsf@gitster.mtv.corp.google.com","threadId":"45848","inReplyTo":"20170509025412.6fkrlnbsdk6qmoia@sigill.intra.peff.net","subject":"Re: [PATCH] pack-objects: disable pack reuse for object-selection options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-09T03:08:16Z","receivedAt":"2017-05-09T03:08:24Z","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 Tue, May 09, 2017 at 11:48:17AM +0900, Junio C Hamano wrote:\n>\n>> > I guess what I was asking was: do you still think it was unclear, or do\n>> > you think you were just being dense?\n>> >\n>> > I don't feel like I gave any information in the follow-on explanation\n>> > that wasn't in the commit message, so I wasn't clear if I worded it\n>> > better or if it just sunk in better.\n>> \n>> At least, \"the current code is buggy when --local and friend are\n>> given and includes needless objects in the result\" was something I\n>> learned only during the discussion, and would never have guessed by\n>> reading the log message.  The second paragraph does talk about \"This\n>> bug has been present since...\", but the first paragraph does not say\n>> anything about the current output being broken.\n>\n> While waiting for your response I took a look to see if I could improve\n> it and came to the same conclusion. The result is below.\n\nLooks good to me.  I really like how the third-paragraph reasons\nabout pros and cons and decides to just disable the codepath.\n\nI see this as an example of omitting something you know so well as\n\"too obvious\", and it turns out that it isn't so obvious to others;\nI commit the same sin all the time myself.  Catching instances of\nthese is part of the review process.\n\nThanks.\n\n> -- >8 --\n> Subject: pack-objects: disable pack reuse for object-selection options\n>\n> If certain options like --honor-pack-keep, --local, or\n> --incremental are used with pack-objects, then we need to\n> feed each potential object to want_object_in_pack() to see\n> if it should be filtered out. But when the bitmap\n> reuse_packfile optimization is in effect, we do not call\n> that function at all, and in fact skip adding the objects to\n> the to_pack list entirely.  This means we have a bug: for\n> certain requests we will silently ignore those options and\n> include objects in that pack that should not be there.\n>\n> The problem has been present since the inception of the\n> pack-reuse code in 6b8fda2db (pack-objects: use bitmaps when\n> packing objects, 2013-12-21), but it was unlikely to come up\n> in practice.  These options are generally used for on-disk\n> packing, not transfer packs (which go to stdout), but we've\n> never allowed pack reuse for non-stdout packs (until\n> 645c432d6, we did not even use bitmaps, which the reuse\n> optimization relies on; after that, we explicitly turned it\n> off when not packing to stdout).\n>\n> We can fix this by just disabling the reuse_packfile\n> optimization when the options are in use. In theory we could\n> teach the pack-reuse code to satisfy these checks, but it's\n> not worth the complexity. The purpose of the optimization is\n> to keep the amount of per-object work we do to a minimum.\n> But these options inherently require us to search for other\n> copies of each object, drowning out any benefit of the\n> pack-reuse optimization. But note that the optimizations\n> from 56dfeb626 (pack-objects: compute local/ignore_pack_keep\n> early, 2016-07-29) happen before pack-reuse, meaning that\n> specifying \"--honor-pack-keep\" in a repository with no .keep\n> files can still follow the fast path.\n>\n> There are tests in t5310 that check these options with\n> bitmaps and --stdout, but they didn't catch the bug, and\n> it's hard to adapt them to do so.\n>\n> One problem is that they don't use --delta-base-offset;\n> without that option, we always disable the reuse\n> optimization entirely. It would be fine to add it in (it\n> actually makes the test more realistic), but that still\n> isn't quite enough.\n>\n> The other problem is that the reuse code is very picky; it\n> only kicks in when it can reuse most of a pack, starting\n> from the first byte. So we'd have to start from a fully\n> repacked and bitmapped state to trigger it. But the tests\n> for these options use a much more subtle state; they want to\n> be sure that the want_object_in_pack() code is allowing some\n> objects but not others. Doing a full repack runs counter to\n> that.\n>\n> So this patch adds new tests at the end of the script which\n> create the fully-packed state and make sure that each option\n> is not fooled by reusable pack.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n"}]}