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

From: Jacob Vosmaer <jacob@gitlab.com>
Date: Jan 28, 2021, 21:12 UTC
Message-ID: <CADMWQoPgscKm2wVOo4uEqSSwgrZQTp7_eoMJpG=pdDmc1bJr=g@mail.gmail.com>
In-Reply-To: <xmqqmtwsx4d9.fsf@gitster.c.googlers.com>

On Thu, Jan 28, 2021 at 8:12 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

>  * As readers know "clone" would get not just the current branch,
>    the way this rev-list traverses only from HEAD makes them wonder
>    if you are trying to exclude refs other than the current branch
>    for a reason.  Better write it as
>
>         git -C dst.git rev-list --objects --missing=print --all >objects &&

Good point!

Show 13 quoted lines

>  * The above says that we are happy if we can clone without erroring
>    out, as long as some objects are missing, but we could go a bit
>    stronger than that: among the objects we have, none should be a
>    blob object.  Is that something we can easily check?
>
>    Something along the lines of...
>
>         grep -v "^?" objects |
>         git -C dst.git cat-file --batch-check="%(objecttype)" >types &&
>         sed -e '/^commit/d' -e '/^tag/d' -e '/^tree/d' types >actual &&
>         test_must_be_empty actual
>
>    ... to ensure everything is either commit, tag or tree, perhaps?

Makes sense, I started down that route at first but I bumped my head against needing --missing to prevent the lazy fetching, and stopped once I had the question mark grep.

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.

I am not saying this to push back on going back and improving the test, I'm quite happy to. It's more that deleting the test may be the ultimate improvement. Thanks in advance for humoring this contrarian suggestion. :)

Cheers, Jacob