From: Karthik Nayak Date: Thu, 05 Feb 2026 11:11:05 GMT Subject: Re: [PATCH V2 1/3] wt-status: replace uses of the_repository with local repository instances Message-ID: In-Reply-To: Karthik Nayak writes: > Shreyansh Paliwal writes: > >> 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 >> --- >> 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] One question, did you run the entire test suite with these changes? I would hope that we have tests which would fail if my inference is correct. If not, there's a gap in our tests too. - Karthik