Re: [PATCH v3 1/1] upload-pack.c: fix filter spec quoting bug

From: Jeff King <peff@peff.net>
Date: Jan 26, 2021, 21:07 UTC
Message-ID: <YBCEmZHapifbVqRy@coredump.intra.peff.net>
In-Reply-To: <874kj46mwf.fsf@evledraar.gmail.com>

On Tue, Jan 26, 2021 at 10:57:36AM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 14 quoted lines

> > This fixes a bug that occurs when you combine partial clone and
> > uploadpack.packobjectshook. You can reproduce it as follows:
> 
> Let's: 
> 
>  * Refer to the commit we're fixing a bug in, i.e. Junio's mention of
>    10ac85c7 (upload-pack: add object filtering for partial clone,
>    2017-12-08) upthread.
> 
>  * See also "imperative-mood" in SubmittingPatches. I.e. say "Fix a bug
>    in ..." not "This fixes ... can be reproduced as"
> 
>  * uploadpack.packObjectsHook not uploadpack.packobjectshook except in C
>    code.

Generally good advice (the imperative stuff IMHO is less important outside of the subject line, but a reasonable default way of writing).

Show 9 quoted lines

> > The problem is an unnecessary and harmful layer of quoting. I tried
> > digging through the history of this function and I think this quoting
> > was there from the start.
> 
> 
> ...So looked at "git log" but didn't try to check out 10ac85c7 and see
> if it had the same issue? If we're going to leave a note about this at
> all probably better to help future source spelunkers by being able to
> say the issue was there from the start.

I don't think it could be tested easily at that point. It implemented the server side, but not the client side. And later when the client side was added, the non-hook code path was fixed. (More discussion earlier in the thread).

-Peff