From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 03 Feb 2026 10:58:05 GMT
Subject: Re: [PATCH 1/3] wt-status: replace uses of the_repository with local repository instances
Message-ID: <50791aed-c64b-48fe-8cc7-8cacaec9d295@gmail.com>
In-Reply-To: <20260202190155.79896-1-shreyanshpaliwalcmsmn@gmail.com>

On 02/02/2026 18:57, Shreyansh Paliwal wrote:
>>> Many instances of the_repository are used in wt-status.c even when a
>>> local repository is already available via struct wt_status or struct
>>> worktree.
>>>
>>
>> One missing information is why is it safe to make this change? If is a
>> repository field, is it holding the same information, is it always
>> defined?
> 
> Yes I should have included explanation as well.
> I have explained below, let me know if this thought process
> is valid or not.
> 
> The replacement of all the_repository with s->repo in this patch are mostly
> to cases where a repository instance is already available via struct wt_status.
> 
> In the current flow, all functions operating on struct wt_status *s
> are called via commit.c. There, status_init_config() calls
> wt_status_prepare(), which initializes the struct wt_status and
> assigns s->repo from the repository instance passed in by the caller.
> As a result, s->repo is guaranteed to be initialized whenever these
> functions are invoked.
> 
> And commit.c itself still relies on the_repository, within wt-status.c,
> the local repository pointer refers to the same underlying
> repository object that the_repository would have pointed to, indirectly
> until we make commit.c also free of the_repository.

Good explanation, that would be a very useful addition to the commit message

>>> diff --git a/wt-status.c b/wt-status.c
>>> index e12adb26b9..9f4d8fda7f 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(s->repo),
>>> 					"HEAD", 0, NULL, NULL);
>>
>> Wouldn't it make more sense to use the function argument 'r' here?
> 
> In wt_status_prepare(), s->repo is initialized to r at the top of
> the function, so both refer to the same repository instance. However,
> using r directly is more explicit and avoids indirect use.
> will change this in V2.

Yes, a few more context lines makes it is clear that either is safe.

>>> @@ -1723,18 +1723,18 @@ int wt_status_check_rebase(const struct worktree *wt,
>>>   {
>>> 	struct stat st;
>>>
>>> -	if (!stat(worktree_git_path(the_repository, wt, "rebase-apply"), &st)) {
>>> -		if (!stat(worktree_git_path(the_repository, wt, "rebase-apply/applying"), &st)) {
>>> +	if (!stat(worktree_git_path(wt->repo, wt, "rebase-apply"), &st)) {
>>> +		if (!stat(worktree_git_path(wt->repo, wt, "rebase-apply/applying"), &st)) {
>>
>> In the same file we make a call 'wt_status_check_rebase(NULL, state)',
>> so wouldn't this break?
> 
> Yes my bad, it would throw a segfault error.
> I think the best way to handle this is to explicitly check for the
> wt to be valid like this,
> 
>      if (wt==NULL)
>          return 0;

That would change the behavior of the function though as it will no 
longer check if the rebase directories exist. You should pass the 
repository down from the caller.

Thanks for working on this

Phillip

> Falling back to the_repository in this case, would probably
> defeat the purpose.
>