Re: [PATCH v4 1/3] refs: add struct repository parameter in get_files_ref_lock_timeout_ms()
- From
Tian Yuchen <a3205153416@gmail.com>
- Date
- Apr 3, 2026, 17:40 UTC
- Message-ID
- <5017740b-4437-4e55-b019-244b33eed05a@gmail.com>
- In-Reply-To
- <20260403120938.1142533-2-shreyanshpaliwalcmsmn@gmail.com>
On 4/3/26 20:08, Shreyansh Paliwal wrote:
Show 8 quoted lines
> -long get_files_ref_lock_timeout_ms(void)
> +long get_files_ref_lock_timeout_ms(struct repository *repo)
> {
> static int configured = 0;
>
> @@ -997,7 +997,7 @@ long get_files_ref_lock_timeout_ms(void)
> static int timeout_ms = 100;
> A very minor and trivial question: the 'static' keyword is still present here. This is entirely understandable, given that you mentioned earlier that...
Show 11 quoted lines
> Hi Yuchen, > > I have acknowledged this in a previous reply to Burak. As stated there, > this is a valid issue and would require moving the config into > repo-settings struct. > In this patch, I focused on removing the dependency on > 'the_repository' while preserving existing behavior. Global state > removal and multi-repo correctness is an incremental process, > so I would prefer to handle this in a follow-up change. > I'll also update the patch title in the next version to better reflect > the scope of the change.
But if that is the case, the accuracy of this line in the commit message:
> This reduces reliance on the_repository global.
..is open to question. Or perhaps it would be worth mentioning:
"Note: This function still uses static variables, which means it does not fully support in-process multi-repo usage yet. This will be addressed in a follow-up by moving the configuration to the 'repo-settings' struct, but changing the signature is a necessary first step..."
or something (shorter)?
To reiterate, I think this is a minor issue, so it would be better if you decide for yourself. Other parts look good to me. ;)
Regards, Yuchen