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

From: Taylor Blau <me@ttaylorr.com>
Date: Jan 19, 2021, 23:08 UTC
Message-ID: <YAdmb3EfNVHYavNk@nand.local>
In-Reply-To: <20210119143348.27535-2-jacob@gitlab.com>

On Tue, Jan 19, 2021 at 03:33:48PM +0100, Jacob Vosmaer wrote:

> In git-pack-objects, we iterate over all the tags if the --include-tag
> option is passed on the command line. For some reason this uses
> for_each_ref which is expensive if the repo has many refs. We should
> use a prefix iterator instead.

OK, makes sense to me.

Show 15 quoted lines

> Signed-off-by: Jacob Vosmaer <jacob@gitlab.com>
> ---
>  builtin/pack-objects.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 2a00358f34..2b32bc93bd 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3740,7 +3740,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
>  	}
>  	cleanup_preferred_base();
>  	if (include_tag && nr_result)
> -		for_each_ref(add_ref_tag, NULL);
> +		for_each_fullref_in("refs/tags/", add_ref_tag, NULL, 0);

Makes sense. I was curious why it wasn't this way from the beginning in f0a24aa56e (git-pack-objects: Automatically pack annotated tags if object was packed, 2008-03-03).

The patch doesn't say, but presumably it was because there wasn't any speed-up to be had iterating only a subset of references back then if they were packed (did packed refs even exist in 2008? unsure). In any case, builtin/pack-objects.c:add_ref_tag() discards anything that doesn't start with "refs/tags/", so I think your change is doing the right thing here.

That said, you could shorten this to use 'for_each_tag_ref()' instead (which compiles to the exact same thing).

It probably wouldn't be a bad time to drop the extra prefix check in add_ref_tag().

Thanks, Taylor