Re: [PATCH 00/21] Introduce `USE_THE_REPOSITORY_VARIABLE` macro

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Jun 11, 2024, 23:24 UTC
Message-ID: <ZmjcrqnDnHAZjr6V@tapette.crustytoothpaste.net>
In-Reply-To: <cover.1718106284.git.ps@pks.im>

On 2024-06-11 at 11:57:33, Patrick Steinhardt wrote:

Show 24 quoted lines

> Hi,
> 
> use of the `the_repository` variable is nowadays considered to be
> deprecated, and over time we want to convert our codebase to stop using
> it in favor of explicitly passing down the repository to functions via
> parameters. This effort faces some important problems though.
> 
>   - It is hard to prove that a certain code unit does not use
>     `the_repository` anymore when sending patch series. The reviewer has
>     no way to verify that it's not used anymore without reading through
>     the code itself.
> 
>   - It is easy to sneak in new usages of `the_repository` by accident
>     into a code unit that is already `the_repository`-clean.
> 
>   - There are many functions which implicitly use `the_repository`,
>     which is really hard to spot.
> 
> This patch series aims to address those problems by introducing a new
> `USE_THE_REPOSITORY_VARIABLE` macro. When unset, then the declarations
> of `the_repository`, `the_hash_algo` and some functions that implicitly
> depend on them will be hidden away. This makes it trivial to demonstrate
> that a code unit is `the_repository`-free by removing the definition of
> any such macro.

Overall, I left a few comments, but I think this definitely moves us in the right direction and I'm glad to see it. This obviously improves the experience with libification and unit testing in a lot of ways, which is good.

My only caution is that using the *_any functions will cause us a world of pain if we ever adopt another 256-bit hash function, since it will be ambiguous which algorithm is to be used. That's why, traditionally, we haven't assumed a hash algorithm based on the object ID length. I don't think the amount of uses we have is excessive, even with your changes, but we'll need to be mindful of that going forward.

-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA