Re: [PATCH 1/1] builtin/pack-objects.c: avoid iterating all refs

[Jeff King <peff@peff.net>]

On Wed, Jan 20, 2021 at 09:50:19AM +0100, Ævar Arnfjörð Bjarmason wrote:

> To elaborate on other things that aren't really your problem & Taylor's
> E-Mail downthread we originally added this because:
> 
>     If new annotated tags have been introduced then we can also include
>     them in the packfile, saving the client from needing to request them
>     through a second connection.
> 
> I've barked up this whole tag fetch tree before 97716d217c (fetch: add a
> --prune-tags option and fetch.pruneTags config, 2018-02-09) but really
> not this specific area.
> 
> I wonder if longer term simply moving this to be work the client does
> wouldn't make more sense. I.e. if we just delete this for_each_ref()
> loop.
>     
> We're just saving a client from saying they "want" a tag. I.e. with the
> whole thing removed we need this to make t5503-tagfollow.sh pass (see
> [1] at the end for the dialog, the tag is 3253df4d...):

Isn't this an ordering problem, though? The client cannot mention
auto-followed tags in a "want", because they first need to "want" the
main history, receive the pack, and then realize they want the others.

So we are not just saving the client from sending a "want", but making a
second full connection to do so. That seems to be an optimization worth
continuing to have.

But here...

>     diff --git a/t/t5503-tagfollow.sh b/t/t5503-tagfollow.sh
>     index 6041a4dd32..1ddc430aef 100755
>     --- a/t/t5503-tagfollow.sh
>     +++ b/t/t5503-tagfollow.sh
>     @@ -134,6 +134,7 @@ test_expect_success 'fetch B, S (commit and tag : 1 connection)' '
>      test_expect_success 'setup expect' '
>      cat - <<EOF >expect
>      want $B
>     +want $T
>      want $S
>      EOF
>      '
>     @@ -146,6 +147,7 @@ test_expect_success 'new clone fetch master and tags' '
>                     cd clone2 &&
>                     git init &&
>                     git remote add origin .. &&
>     +               git config --add remote.origin.fetch "+refs/tags/*:refs/tags/*" &&
>                     GIT_TRACE_PACKET=$UPATH git fetch &&
>                     test $B = $(git rev-parse --verify origin/master) &&
>                     test $S = $(git rev-parse --verify tag2) &&
> 
> We're also saving the client the work of having to go through
> refs/tags/* and figure out whether there are tags there that aren't on
> our main history.

You seem to be against auto-following at all. And certainly I can see an
argument that it is not worth the trouble it causes. But it is the
default behavior, and I suspect many people are relying on it. Fetching
every tag indiscriminately is going to grab a bunch of extra unwanted
objects in some repos. An obvious case is any time "clone
--single-branch --depth" is used.

Maybe I'm not quite understanding what you're proposing.

-Peff