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@xxxxxxxxx> 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
