From: phillip.wood123@gmail.com
Date: Wed, 30 Oct 2024 20:30:57 GMT
Subject: Re: [PATCH v2 1/5] worktree: add CLI/config options for relative path linking
Message-ID: <0f91514a-f1cb-40da-a398-56d0fe37600e@gmail.com>
In-Reply-To: <D59F4X1I5COT.1QECM7J128ZIG@pm.me>

Hi Caleb

On 30/10/2024 20:21, Caleb White wrote:
> On Wed Oct 30, 2024 at 3:16 PM CDT, Taylor Blau wrote:
>> On Wed, Oct 30, 2024 at 05:27:33AM +0000, Caleb White wrote:
>>>>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>>>>> index dae63dedf4cac2621f51f95a39aa456b33acd894..c1130be5890c905c0b648782a834eb8dfcd79ba5 100644
>>>>> --- a/builtin/worktree.c
>>>>> +++ b/builtin/worktree.c
>>>>> @@ -134,6 +134,9 @@ static int git_worktree_config(const char *var, const char *value,
>>>>>    	if (!strcmp(var, "worktree.guessremote")) {
>>>>>    		guess_remote = git_config_bool(var, value);
>>>>>    		return 0;
>>>>> +	} else if (!strcmp(var, "worktree.userelativepaths")) {
>>>>> +		use_relative_paths = git_config_bool(var, value);
>>>>
>>>> As we're trying to remove global variables from libgit.a as part of the
>>>> libification effort I'd be much happier if "use_relative_paths" was
>>>> declared as a "static int" in this file and then passed down to the
>>>> functions that need it rather than declaring it as a global in "worktree.c".
>>>
>>> I can create a getter/setter in the worktree API to handle this, but
>>> I'd rather not pass it as an argument to every function that needs it as
>>> that would be a lot of changes. All of these functions would need their
>>> signatures updated to include the new parameter:
>>>
>>> - `add_worktree()`
>>> - `update_worktree_location()`
>>> - `repair_worktree_at_path()`
>>> - `repair_worktrees()`
>>> - `repair_worktree()`
>>> - `write_worktree_linking_files()`
>>
>> There is no reason to have a "getter" and "setter" for a extern'd
>> variable.
>>
>> I agree that it would be preferable to have use_relative_paths be a
>> static int within this compilation unit and to pass it to the above
>> functions.
> 
> If I created a getter/setter then the variable would no longer be
> extern'd.
> 
> To be clear, you're advocating that I change the function signature
> for all of the functions listed above to include the new parameter? That
> seems like a lot of parameter bloat

It's a bit of a pain to have to change the function signatures and pass 
the parameter down but it's not difficult to do.

> when I could just set the variable
> in this compilation unit and access it directly in the
> `write_worktree_linking_files()` function.

The problem with that is that the variable is still effectively global. 
The aim of the libification work is to be able to work on more than one 
repository from a single process while respecting each repository's 
config settings.

Best Wishes

Phillip

> Best,
> Caleb
>