Re: [PATCH v4] upload-pack.c: fix filter spec quoting bug

From: Jeff King <peff@peff.net>
Date: Jan 28, 2021, 21:51 UTC
Message-ID: <YBMx7yuBit8TecYp@coredump.intra.peff.net>
In-Reply-To: <CADMWQoPgscKm2wVOo4uEqSSwgrZQTp7_eoMJpG=pdDmc1bJr=g@mail.gmail.com>

On Thu, Jan 28, 2021 at 10:12:42PM +0100, Jacob Vosmaer wrote:

Show 12 quoted lines

> Now that we're talking about the test, I was wondering about something else.
> 
> In my original patch, I purposely did not add a test. Why? Because
> --filter is just one option of several that upload-pack passes to
> pack-objects (think of --shallow-file and --include-tag, for
> instance). Why is --filter special? If the original quoting bug had
> not happened, would we be testing various permutations of clone
> options in combination with packObjectsHook?
> 
> As a reader looking at t5544, unless I know the backstory of the bug,
> I do not understand why --filter gets a test but those other things do
> not.

You're definitely not wrong that this is somewhat closing the barn door after horse has left, and there may be other barns still to be fixed.

But usually we'll add a test that demonstrates the breakage, if only because the fact that something so mundane and easy to trigger _wasn't_ caught by the existing test is pretty bad. So we should at least make sure the combination is now covered, which your test does. And then over time we build up a set of coverage.

Of course it is sometimes nice to be exhaustive, too. The organically built-up set of tests is going to have holes if it's not done systematically. But Git also isn't a black box; we know that the implementation of this option was weirdly different.

I guess the argument you are making is that now that it's fixed, it _isn't_ different, and we're unlikely to reintroduce the bug. I can buy that, but I think we just do the "test the fixed bug" thing on principle.

-Peff