Re: [PATCH V2 1/3] wt-status: replace uses of the_repository with local repository instances
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Feb 5, 2026, 10:59 UTC
- Message-ID
- <CAOLa=ZQOzNrCkKzwG7CuH5Ge+OPZcT4i0cY9nRkAJZO6T_QZQw@mail.gmail.com>
- In-Reply-To
- <20260205101524.125452-2-shreyanshpaliwalcmsmn@gmail.com>
Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> writes:
Show 109 quoted lines
> wt-status.c uses the global the_repository in several places even when
> a repository instance is already available via struct wt_status or
> struct worktree.
>
> Replace these direct uses of the_repository with the repository carried
> by the local structs (e.g. s->repo, wt->repo).
>
> The replacements of all the_repository with s->repo are mostly
> to cases where a repository instance is already available via
> struct wt_status. All functions operating on struct wt_status *s
> are only used after s is initialized by wt_status_prepare(),
> which sets s->repo from the repository provided by the caller.
> As a result, s->repo is guaranteed to be available and consistent
> whenever these functions are invoked.
>
> This reduces reliance on global state and keeps wt-status consistent,
> though many functions operating on struct wt_status *s
> are called via commit.c and it still relies on the_repository,
> but within wt-status.c the local repository pointer
> refers to the same underlying repository object.
>
> Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com>
> ---
> wt-status.c | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index e12adb26b9..f71addc35f 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -150,11 +150,11 @@ void wt_status_prepare(struct repository *r, struct wt_status *s)
> s->show_untracked_files = SHOW_NORMAL_UNTRACKED_FILES;
> s->use_color = GIT_COLOR_UNKNOWN;
> s->relative_paths = 1;
> - s->branch = refs_resolve_refdup(get_main_ref_store(the_repository),
> + s->branch = refs_resolve_refdup(get_main_ref_store(r),
> "HEAD", 0, NULL, NULL);
> s->reference = "HEAD";
> s->fp = stdout;
> - s->index_file = repo_get_index_file(the_repository);
> + s->index_file = repo_get_index_file(s->repo);
> s->change.strdup_strings = 1;
> s->untracked.strdup_strings = 1;
> s->ignored.strdup_strings = 1;
> @@ -646,7 +646,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)
>
> repo_init_revisions(s->repo, &rev, NULL);
> memset(&opt, 0, sizeof(opt));
> - opt.def = s->is_initial ? empty_tree_oid_hex(the_repository->hash_algo) : s->reference;
> + opt.def = s->is_initial ? empty_tree_oid_hex(s->repo->hash_algo) : s->reference;
> setup_revisions(0, NULL, &rev, &opt);
>
> rev.diffopt.flags.override_submodule_config = 1;
> @@ -1146,7 +1146,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
> rev.diffopt.ita_invisible_in_index = 1;
>
> memset(&opt, 0, sizeof(opt));
> - opt.def = s->is_initial ? empty_tree_oid_hex(the_repository->hash_algo) : s->reference;
> + opt.def = s->is_initial ? empty_tree_oid_hex(s->repo->hash_algo) : s->reference;
> setup_revisions(0, NULL, &rev, &opt);
>
> rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
> @@ -1317,9 +1317,9 @@ static int split_commit_in_progress(struct wt_status *s)
> !s->branch || strcmp(s->branch, "HEAD"))
> return 0;
>
> - if (refs_read_ref_full(get_main_ref_store(the_repository), "HEAD", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> + if (refs_read_ref_full(get_main_ref_store(s->repo), "HEAD", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> &head_oid, &head_flags) ||
> - refs_read_ref_full(get_main_ref_store(the_repository), "ORIG_HEAD", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> + refs_read_ref_full(get_main_ref_store(s->repo), "ORIG_HEAD", RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> &orig_head_oid, &orig_head_flags))
> return 0;
> if (head_flags & REF_ISSYMREF || orig_head_flags & REF_ISSYMREF)
> @@ -1432,7 +1432,7 @@ static void show_rebase_information(struct wt_status *s,
> i++)
> status_printf_ln(s, color, " %s", have_done.items[i].string);
> if (have_done.nr > nr_lines_to_show && s->hints) {
> - char *path = repo_git_path(the_repository, "rebase-merge/done");
> + char *path = repo_git_path(s->repo, "rebase-merge/done");
> status_printf_ln(s, color,
> _(" (see more in file %s)"), path);
> free(path);
> @@ -1534,7 +1534,7 @@ static void show_cherry_pick_in_progress(struct wt_status *s,
> else
> status_printf_ln(s, color,
> _("You are currently cherry-picking commit %s."),
> - repo_find_unique_abbrev(the_repository, &s->state.cherry_pick_head_oid,
> + repo_find_unique_abbrev(s->repo, &s->state.cherry_pick_head_oid,
> DEFAULT_ABBREV));
>
> if (s->hints) {
> @@ -1564,7 +1564,7 @@ static void show_revert_in_progress(struct wt_status *s,
> else
> status_printf_ln(s, color,
> _("You are currently reverting commit %s."),
> - repo_find_unique_abbrev(the_repository, &s->state.revert_head_oid,
> + repo_find_unique_abbrev(s->repo, &s->state.revert_head_oid,
> DEFAULT_ABBREV));
> if (s->hints) {
> if (has_unmerged(s))
> @@ -1624,7 +1624,7 @@ static char *get_branch(const struct worktree *wt, const char *path)
> struct object_id oid;
> const char *branch_name;
>
> - if (strbuf_read_file(&sb, worktree_git_path(the_repository, wt, "%s", path), 0) <= 0)
> + if (strbuf_read_file(&sb, worktree_git_path(wt->repo, wt, "%s", path), 0) <= 0)
> goto got_nothing;
>So if you look into `worktree_git_path()`, it has a certain check
if (wt && wt->repo != r)
BUG("worktree not connected to expected repository");But this is okay with your change, the only question is, do we know wt is always defined here? Unfortunately, wt can be NULL here, in the same file we have:
wt_status_check_rebase(NULL, state); -> get_branch(NULL, ...)
Which would crash, no? This is applicable for other parts of the code too were we're now using wt->repo.
This is also what I was requesting in the previous round, about explaining why it is safe to make a particular change.
[snip]