Re: [PATCH 17/17] libs: use "struct repository *" argument, not "the_repository"

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 28, 2023, 10:53 UTC
Message-ID: <230328.86a5zxw31u.gmgdl@evledraar.gmail.com>
In-Reply-To: <CABPp-BEdpTO=DRjLq_p+dgX68M0HUVB--3yQR4Sdp8rnFYeyfA@mail.gmail.com>

On Sat, Mar 18 2023, Elijah Newren wrote:

Show 21 quoted lines

> On Fri, Mar 17, 2023 at 8:55 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> As can easily be seen from grepping in our sources, we had these uses
>> of "the_repository" in various library code in cases where the
>> function in question was already getting a "struct repository *"
>> argument. Let's use that argument instead.
>>
>> Out of these changes only the changes to "cache-tree.c",
>> "commit-reach.c", "shallow.c" and "upload-pack.c" would have cleanly
>> applied before the migration away from the "repo_*()" wrapper macros
>> in the preceding commits.
>>
>> The rest aren't new, as we'd previously implicitly refer to
>> "the_repository", but it's now more obvious that we were doing the
>> wrong thing all along, and should have used the parameter instead.
>
> YES!  I love seeing fixes like this.  I noticed some a while back in
> the merge code, and had some patches somewhere that I think I never
> got around to submitting, but which have been in the back of my mind
> bugging me.  Nice to see lots of these kinds of cases getting fixed.

Yeah, it will be nice to fix those...

Show 38 quoted lines

>> The change to change "get_index_format_default(the_repository)" in
>> "read-cache.c" to use the "r" variable instead should arguably have
>> been part of [1], or in the subsequent cleanup in [2]. Let's do it
>> here, as can be seen from the initial code in [3] it's not important
>> that we use "the_repository" there, but would prefer to always use the
>> current repository.
>>
>> This change excludes the "the_repository" use in "upload-pack.c"'s
>> upload_pack_advertise(), as the in-flight [4] makes that change.
>>
>> 1. ee1f0c242ef (read-cache: add index.skipHash config option,
>>    2023-01-06)
>> 2. 6269f8eaad0 (treewide: always have a valid "index_state.repo"
>>    member, 2023-01-17)
>> 3. 7211b9e7534 (repo-settings: consolidate some config settings,
>>    2019-08-13)
>> 4. <Y/hbUsGPVNAxTdmS@coredump.intra.peff.net>
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  add-interactive.c |   2 +-
>>  branch.c          |   8 ++--
>>  builtin/replace.c |   2 +-
>>  cache-tree.c      |   4 +-
>>  combine-diff.c    |   2 +-
>>  commit-graph.c    |   2 +-
>>  commit-reach.c    |   2 +-
>>  merge-recursive.c |   2 +-
>
> Doh, doesn't fix up the ones I had.  I don't remember the exact issue
> and can't find the patches right now, but looking around a little I
> see a write_object_file() which should be a repo_write_object_file(),
> but it's in a function that doesn't take a repository argument.
> However, the caller has access to a repository argument, in the form
> of opt->repo, so we've got some low-hanging fruit that could be fixed
> up.  Doesn't need to be part of your series, but it's nice that your
> series has done some of the groundwork to make these issues more
> obvious.

...yeah, I do have a WIP follow-up based on this topic which addresses the sort of issues you're rasing here, i.e. in many cases we have "the_repository", no "struct repository *" function argument, but we have a "x->repo", where "x" is the "struct" state object we have in play in that API.

My messy WIP follow-up for starting to fix those is at : https://github.com/avar/git/compare/6f86a34bf8b...2072d685a56

I didn't get to the merge/object code you mentioned, but we should do these sorts of changes as a follow-up.

I didn't do them as part of this topic, because this series was already quite long, and this 17/17 is arguably scope-creep, but I could argue for it on the basis of it being almost entirely a post-cleanup for the preceding changes.

Show 10 quoted lines

> [...]
> I started looking through it closely, but after a bit I realized that
> the only kind of error you would likely to be make here would be the
> kind caught by the compiler, so then I started just skimming over it.
> But I think that's safe for this kind of change.  I'm a big fan of
> these kinds of cleanups and fixups, though.
>
> I'm not sure I can give a Reviewed-by since I just don't know cocci,
> but it all looked pretty logical.  You definitely have my Acked-by on
> the series, though.

I won't add your Reviewed-by to a re-roll given that caveat, but FWIW I think in general those not familiar with cocci should be comfortable reviewing topics that modify code using the tool.

Even if you don't grok the patch lanugage, you can review the resulting diff, as you've done here.

I think the exception to that is if the change itself is proposing to institute a new cocci rule, that we can expect to apply going forward. In those cases really understanding the rule matters, e.g. the contrib/coccinelle/unused.cocci rule I added relatively recently is one such example.

But here the rules are just a one-off, and are just an aid to reproduce the changes.