Re: [PATCH v2 2/7] gc: convert to using the_hash_algo

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 1738 bytes --]

Hi Ævar,

On Fri, 15 Mar 2019, Ævar Arnfjörð Bjarmason wrote:

> 
> On Fri, Mar 15 2019, Duy Nguyen wrote:
> 
> > On Thu, Mar 14, 2019 at 7:34 PM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >>
> >> There's been a lot of changing of the hardcoded "40" values to
> >> the_hash_algo->hexsz, but we've so far missed this one where we
> >> hardcoded 38 for the loose object file length.
> >
> > Wow. Good catch.
> >
> >> diff --git a/builtin/gc.c b/builtin/gc.c
> >> index 8c2312681c..733bd7bdf4 100644
> >> --- a/builtin/gc.c
> >> +++ b/builtin/gc.c
> >> @@ -156,6 +156,7 @@ static int too_many_loose_objects(void)
> >>         int auto_threshold;
> >>         int num_loose = 0;
> >>         int needed = 0;
> >> +       const unsigned hexsz_loose = the_hash_algo->hexsz - 2;
> >
> > Since you're removing hard coded numbers. Another option is a
> > combination of strlen(basename()) and
> > loose_object_path(the_repository, , null_oid). They should also give
> > the same 38. Then if loose object format is updated to use 3+ chars
> > for the directory part, this code will not need update anymore.
> >
> > The downside of course is a lot more code. Or you can just introduce a
> > new function to return this "hexsz - 2", keep that function close to
> > fill_loose_path() with a comment there that the two functions should
> > be aligned.
> 
> I think it's better to just keep hardcoding "2". We're very unlikely to
> ever change objects/?? in favor of e.g. objects/???, and if we were that
> would be a huge "hash function transition" of its own.

Of course, since it is *such* a common use case, we could also simply add
another field in `the_hash_algo`. That would also save on repeated magic
constants.

Ciao,
Dscho