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