Re: [PATCH 2/2] refs/files: use heuristic to decide whether to repack with `--auto`

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 3, 2024, 09:23 UTC
Message-ID: <ZtbVbn5rxrmLFuNa@pks.im>
In-Reply-To: <CAOLa=ZRGvU4LvX9kjvF3dJCTvKR6CC1CwPTp515c3Wt5M8a5vA@mail.gmail.com>

On Tue, Sep 03, 2024 at 02:00:16AM -0700, karthik nayak wrote:

Show 15 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/refs/files-backend.c b/refs/files-backend.c
> > index 1cff65f6ae5..261e2b8570f 100644
> > --- a/refs/files-backend.c
> > +++ b/refs/files-backend.c
> > @@ -1311,6 +1311,78 @@ static int should_pack_ref(struct files_ref_store *refs,
> >  	return 0;
> >  }
> >
> > +static size_t fastlog2(size_t sz)
> 
> Nit: We already `reftable/stack_test.c:fastlog2` and `bisect.c:log2i`. I
> wonder if it would be nice to consolidate all of them. But I guess it's
> okay, since the reftable code anyways is isolated from the rest of the
> Git code.

Yeah, the version we have here is in fact an (almost exact) copy of the function in our reftable codebase. And as you note, we do not want to deduplicate these such that the reftable library is somewhat isolated from the rest of the codebase.

I also noticed `log2i()`, but honestly the only reason why I didn't reuse it is that I had no clue where to put it. There isn't any header that would be a good fit for it, and creating a new "math.h" header for a single function felt overblown to me. So I decided to just not bother. I'm happy to adjust though if somebody has a suggestion for where to put it.

Patrick