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

[Patrick Steinhardt <ps@pks.im>]

On Mon, Jan 06, 2025 at 09:57:26PM +0100, Toon Claes wrote:
> 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