Re: [PATCH 4/5] cocci: apply rules to rewrite callers of "refs" interfaces

[Taylor Blau <me@ttaylorr.com>]

On Fri, May 03, 2024 at 08:28:14AM +0200, Patrick Steinhardt wrote:
> Apply the rules that rewrite callers of "refs" interfaces to explicitly
> pass `struct ref_store`. The resulting patch has been applied with the
> `--whitespace=fix` option.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  add-interactive.c           | 17 +++++++----
>  bisect.c                    | 25 ++++++++++-----
>  blame.c                     |  4 +--
>  branch.c                    |  5 +--
>  builtin/am.c                | 38 ++++++++++++++---------
>  builtin/bisect.c            | 44 +++++++++++++++-----------
>  builtin/blame.c             |  4 +--
>  builtin/branch.c            | 49 ++++++++++++++++-------------
>  builtin/checkout.c          | 35 +++++++++++++--------
>  builtin/clone.c             | 36 +++++++++++++---------
>  builtin/describe.c          |  3 +-
>  builtin/fast-import.c       | 11 ++++---
>  builtin/fetch.c             | 20 ++++++++----
>  builtin/fsck.c              | 11 +++++--
>  builtin/gc.c                |  3 +-
>  builtin/log.c               |  6 ++--
>  builtin/merge.c             | 34 +++++++++++++--------
>  builtin/name-rev.c          |  5 +--
>  builtin/notes.c             | 26 +++++++++-------
>  builtin/pack-objects.c      | 10 ++++--
>  builtin/pull.c              |  2 +-
>  builtin/rebase.c            | 18 ++++++-----
>  builtin/receive-pack.c      | 15 ++++++---
>  builtin/reflog.c            | 25 ++++++++-------
>  builtin/remote.c            | 37 +++++++++++++---------
>  builtin/repack.c            |  7 +++--
>  builtin/replace.c           |  9 +++---
>  builtin/reset.c             | 13 +++++---
>  builtin/rev-parse.c         | 25 ++++++++++-----
>  builtin/show-branch.c       | 22 ++++++++-----
>  builtin/show-ref.c          | 19 ++++++++----
>  builtin/stash.c             | 23 ++++++++------
>  builtin/submodule--helper.c |  7 +++--
>  builtin/symbolic-ref.c      | 13 +++++---
>  builtin/tag.c               | 11 ++++---
>  builtin/update-index.c      |  2 +-
>  builtin/update-ref.c        | 21 ++++++++-----
>  builtin/worktree.c          | 19 +++++++-----
>  bundle-uri.c                | 12 +++++---
>  bundle.c                    |  2 +-
>  commit-graph.c              |  3 +-
>  commit.c                    |  3 +-
>  config.c                    |  3 +-
>  delta-islands.c             |  3 +-
>  fetch-pack.c                |  6 ++--
>  fmt-merge-msg.c             |  4 ++-
>  help.c                      |  5 +--
>  http-backend.c              | 13 +++++---
>  log-tree.c                  |  9 ++++--
>  ls-refs.c                   | 10 +++---
>  midx-write.c                |  3 +-
>  negotiator/default.c        |  3 +-
>  negotiator/skipping.c       |  3 +-
>  notes-cache.c               |  6 ++--
>  notes-merge.c               |  2 +-
>  notes-utils.c               |  7 +++--
>  notes.c                     |  5 +--
>  reachable.c                 |  5 +--
>  ref-filter.c                | 35 +++++++++++++++------
>  reflog-walk.c               | 27 +++++++++++-----
>  reflog.c                    | 20 +++++++-----
>  refs.c                      |  9 ++++--
>  remote.c                    | 38 ++++++++++++++---------
>  reset.c                     | 29 ++++++++++--------
>  revision.c                  | 27 ++++++++++------
>  sequencer.c                 | 61 ++++++++++++++++++++-----------------
>  server-info.c               |  3 +-
>  setup.c                     |  2 +-
>  shallow.c                   | 16 ++++++----
>  submodule.c                 |  6 ++--
>  transport-helper.c          | 29 ++++++++++--------
>  transport.c                 | 16 ++++++----
>  upload-pack.c               | 20 +++++++-----
>  walker.c                    |  6 ++--
>  wt-status.c                 | 22 +++++++------
>  75 files changed, 711 insertions(+), 436 deletions(-)
>
> diff --git a/add-interactive.c b/add-interactive.c
> index e17602b5e4..b5d6cd689a 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -532,8 +532,9 @@ static int get_modified_files(struct repository *r,
>  			      size_t *binary_count)
>  {
>  	struct object_id head_oid;
> -	int is_initial = !resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
> -					     &head_oid, NULL);
> +	int is_initial = !refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
> +						  "HEAD", RESOLVE_REF_READING,
> +						  &head_oid, NULL);
>  	struct collection_status s = { 0 };
>  	int i;

OK, so this is the patch that applies the Coccinelle script from the
previous step.

This all makes sense, but I wonder if we should be more careful than
blindly replacing these functions with the same set of arguments
following a new

    get_main_ref_store(the_repository)

For instance, in this hunk, we have a 'struct repository *' in scope via
the 'r' parameter in add-interactive.c::get_modified_files().

I don't think it's wrong per-se to use the_repository here, but it does
create something to clean up in the future.

I haven't looked at other spots throughout this patch, just noticed
this in the first hunk and thought that I'd say something about it.
Otherwise the transformation seems obviously correct.

Thanks,
Taylor