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

[Patrick Steinhardt <ps@pks.im>]

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

On Tue, Jun 11, 2024 at 11:24:30PM +0000, brian m. carlson wrote:
> On 2024-06-11 at 11:57:33, Patrick Steinhardt wrote:
> > 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.

The only cases where I add new calls to `_any()` are in test helpers:

  - "t/helper/test-oidtree.c". This one is getting converted to a unit
    test by Ghanshyam, so I'll leave it to him to improve this.

  - "t/helper/test-proc-receive.c". Here we don't care about the actual
    algorithm, the only thing we care about is that we can correctly
    parse them and then eventually emit them via `oid_to_hex()` again.
    So even if we introduce a second hash function with the same length
    this code would continue to work alright.

So I think it should be fine in the context of this series. But the
remark is certainly valid and something we should be cautious about
going forward.

Thanks!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]