Re: [PATCH 4/5] cocci: apply rules to rewrite callers of "refs" interfaces
- From
Taylor Blau <me@ttaylorr.com>
- Date
- May 3, 2024, 18:48 UTC
- Message-ID
- <ZjUxlLyyvmHNifDp@nand.local>
- In-Reply-To
- <5109468b356637b8b4521a3e96ba228622804ca0.1714717057.git.ps@pks.im>
On Fri, May 03, 2024 at 08:28:14AM +0200, Patrick Steinhardt wrote:
Show 98 quoted lines
> 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