Re: [PATCH V2 1/3] wt-status: replace uses of the_repository with local repository instances

> Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> 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 <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]