Jeff King <peff@xxxxxxxx> writes: > On Tue, May 09, 2017 at 11:48:17AM +0900, Junio C Hamano wrote: > >> > I guess what I was asking was: do you still think it was unclear, or do >> > you think you were just being dense? >> > >> > I don't feel like I gave any information in the follow-on explanation >> > that wasn't in the commit message, so I wasn't clear if I worded it >> > better or if it just sunk in better. >> >> At least, "the current code is buggy when --local and friend are >> given and includes needless objects in the result" was something I >> learned only during the discussion, and would never have guessed by >> reading the log message. The second paragraph does talk about "This >> bug has been present since...", but the first paragraph does not say >> anything about the current output being broken. > > While waiting for your response I took a look to see if I could improve > it and came to the same conclusion. The result is below. Looks good to me. I really like how the third-paragraph reasons about pros and cons and decides to just disable the codepath. I see this as an example of omitting something you know so well as "too obvious", and it turns out that it isn't so obvious to others; I commit the same sin all the time myself. Catching instances of these is part of the review process. Thanks. > -- >8 -- > Subject: pack-objects: disable pack reuse for object-selection options > > If certain options like --honor-pack-keep, --local, or > --incremental are used with pack-objects, then we need to > feed each potential object to want_object_in_pack() to see > if it should be filtered out. But when the bitmap > reuse_packfile optimization is in effect, we do not call > that function at all, and in fact skip adding the objects to > the to_pack list entirely. This means we have a bug: for > certain requests we will silently ignore those options and > include objects in that pack that should not be there. > > The problem has been present since the inception of the > pack-reuse code in 6b8fda2db (pack-objects: use bitmaps when > packing objects, 2013-12-21), but it was unlikely to come up > in practice. These options are generally used for on-disk > packing, not transfer packs (which go to stdout), but we've > never allowed pack reuse for non-stdout packs (until > 645c432d6, we did not even use bitmaps, which the reuse > optimization relies on; after that, we explicitly turned it > off when not packing to stdout). > > We can fix this by just disabling the reuse_packfile > optimization when the options are in use. In theory we could > teach the pack-reuse code to satisfy these checks, but it's > not worth the complexity. The purpose of the optimization is > to keep the amount of per-object work we do to a minimum. > But these options inherently require us to search for other > copies of each object, drowning out any benefit of the > pack-reuse optimization. But note that the optimizations > from 56dfeb626 (pack-objects: compute local/ignore_pack_keep > early, 2016-07-29) happen before pack-reuse, meaning that > specifying "--honor-pack-keep" in a repository with no .keep > files can still follow the fast path. > > There are tests in t5310 that check these options with > bitmaps and --stdout, but they didn't catch the bug, and > it's hard to adapt them to do so. > > One problem is that they don't use --delta-base-offset; > without that option, we always disable the reuse > optimization entirely. It would be fine to add it in (it > actually makes the test more realistic), but that still > isn't quite enough. > > The other problem is that the reuse code is very picky; it > only kicks in when it can reuse most of a pack, starting > from the first byte. So we'd have to start from a fully > repacked and bitmapped state to trigger it. But the tests > for these options use a much more subtle state; they want to > be sure that the want_object_in_pack() code is allowing some > objects but not others. Doing a full repack runs counter to > that. > > So this patch adds new tests at the end of the script which > create the fully-packed state and make sure that each option > is not fooled by reusable pack. > > Signed-off-by: Jeff King <peff@xxxxxxxx>