Re: [PATCH 01/14] progress: stop using `the_repository`

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 7, 2025, 07:19 UTC
Message-ID: <Z3zVhZC437Kx60dm@pks.im>
In-Reply-To: <87v7urk6dl.fsf@iotcl.com>

On Mon, Jan 06, 2025 at 09:57:26PM +0100, Toon Claes wrote:

Show 21 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Stop using `the_repository` in the "progress" subsystem by passing in a
> > repository when initializing `struct progress`. Furthermore, store a
> > pointer to the repository in that struct so that we can pass it to the
> > trace2 API when logging information.
> >
> > Adjust callers accordingly by using `the_repository`. While there may be
> > some callers that have a repository available in their context, this
> > trivial conversion allows for easier verification and bubbles up the use
> > of `the_repository` by one level.
> 
> I'm not sure I agree here. Below I've marked all places where I think we
> are able to get the repo from somewhere else than `the_repository`. For
> example, looking at diffcore-rename.c, the already present calls to
> trace2_*() use `options->repo`, why shouldn't we do the same?
> 
> I understand what your angle is, you want to bubble up `the_repository`
> and make the changes easier to reason about, but it feels to me we're
> creating extra work. If most people disagree with me, I'm happy to take
> your approach.

The problem is that this could lead to a change in behaviour, as the repo we have available may or may not be the same as `the_repository`. So without auditing every single callsite I have no way of knowing, and that audit is quite involved when it touches a lot of subsystems at once.

That's why I'm instead pushing it further down the road: we know that injecting `the_repository` will yield the exact same behaviour as before, and we only need to audit a single subsystem, namely the one that we're currently converting. So it's one more step overall, but by separating mechanical from non-mechanical changes it makes the steps simpler overall.

Patrick