Re: [PATCH] pack-objects: disable pack reuse for object-selection options

From: Junio C Hamano <gitster@pobox.com>
Date: May 9, 2017, 02:48 UTC
Message-ID: <xmqq4lwu68zy.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170509022114.s3tpxrgtplsopr4x@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 23 quoted lines

> On Tue, May 09, 2017 at 11:14:18AM +0900, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> >> Ah, OK, and now I understand why you called this a "bug" (which is
>> >> older and do not need to be addressed as part of 2.13) in the
>> >> original message.  The new tests check requests that ought to
>> >> produce an empty packfile as the result actually do, but with the
>> >> current code, the reuse code does not work with --local and friends
>> >> and ends up including what was requested to be excluded.
>> >
>> > Right. Did you want me to try re-wording the commit message, or does it
>> > make sense now?
>> 
>> It does make sense to me now, but I do not speak for all future
>> readers of "git log", so...
>
> 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.

So, I am not sure if this was a case where I was dense. I think the first paragraph needs a bit more clarity.

    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.  This is totally contrary to
    the purpose of the pack-reuse optimization, which tries hard
    to avoid doing any per-object work.  Therefore we need to
    disable this optimization when these options are in use.

Perhaps "... should be filtered out." can be followed by "However, the current code fails to do so, and we end up including these unwanted objects in the output.", and then "This is totally..." can instead begin with "Besides, having to do per-object filtering is totally...". I wouldn't have been confused if it were like so.