From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sun, 04 Mar 2018 02:47:20 GMT
Subject: Re: [PATCH 13/44] pack: move approximate object count to object store
Message-ID: <CAPig+cSE_x2f_8RG151MX3+FJhiofBPoVPXP+mxdJpoHXwjv9g@mail.gmail.com>
In-Reply-To: <20180303113637.26518-14-pclouds@gmail.com>

On Sat, Mar 3, 2018 at 6:36 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> The approximate_object_count() function maintains a rough count of
> objects in a repository to estimate how long object name abbreviates
> should be.  Object names are scoped to a repository and the
> appropriate length may differ by repository, so the object count
> should not be global.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> diff --git a/packfile.c b/packfile.c
> @@ -813,8 +811,8 @@ static int approximate_object_count_valid;
>  unsigned long approximate_object_count(void)
>  {
> -       static unsigned long count;
> -       if (!approximate_object_count_valid) {
> +       if (!the_repository->objects.approximate_object_count_valid) {
> +               unsigned long count;
>                 struct packed_git *p;
>
>                 prepare_packed_git();
> @@ -824,8 +822,9 @@ unsigned long approximate_object_count(void)
>                                 continue;
>                         count += p->num_objects;
>                 }
> +               the_repository->objects.approximate_object_count = count;
>         }
> -       return count;
> +       return the_repository->objects.approximate_object_count;
>  }
> @@ -900,7 +899,7 @@ void prepare_packed_git(void)
>  void reprepare_packed_git(void)
>  {
> -       approximate_object_count_valid = 0;
> +       the_repository->objects.approximate_object_count_valid = 0;

Not an issue specific to this patch, but where, how, when does
'approximate_object_count_valid' ever get set to anything other than
0? Even in the existing code (without this patch), there doesn't seem
to be anyplace which sets this to a non-zero value. Am I missing
something obvious (or subtle)?