Re: [PATCH v2 1/5] worktree: add CLI/config options for relative path linking

[Caleb White <cdwhite3@pm.me>]

On Tue Oct 29, 2024 at 9:52 AM CDT, Phillip Wood wrote:
>> This patch introduces the `--[no-]relative-paths` CLI option for
>
> "This patch" is a bit redundant, I'd say "Introduce a
> `--[no-]-relative-paths ..`

I removed "patch".

>> `git worktree {add, move, repair}` commands, as well as the
>> `worktree.useRelativePaths` configuration setting. When enabled,
>> these options allow worktrees to be linked using relative paths,
>> enhancing portability across environments where absolute paths
>> may differ (e.g., containerized setups, shared network drives).
>> Git still creates absolute paths by default, but these options allow
>> users to opt-in to relative paths if desired.
>
> This sounds good, I'm not sure the patch actually implements anything
> other than the option parsing though. I think it would make sense to add
> these options at the end of the patch series once the implementation has
> been changed to support them. I'd start with patches 4 and 5 to add the
> new extension setting first, then refactor worktree.c to handle creating
> worktrees with relative or absolute paths and set the extension if
> appropriate, then add the --relative-path option to "git worktree"

That makes sense, I can reorder the patches as you described.

>> Using the `--relative-paths` option with `worktree {move, repair}`
>> will convert absolute paths to relative ones, while `--no-relative-paths`
>> does the reverse. For cases where users want consistency in path
>> handling, the config option `worktree.useRelativePaths` provides
>> a persistent setting.
>>
>> In response to reviewer feedback from the initial patch series[1],
>> this revision includes slight refactoring for improved
>> maintainability and clarity in the code base.
>
> Please don't mix cleanups with other code changes as it makes it hard to
> check that the cleanups don't change the behavior.

Fair enough, I'll separate the cleanups into their own patch.

>> +worktree.useRelativePaths::
>> +	If set to `true`, worktrees will be linked to the repository using
>> +	relative paths rather than using absolute paths. This is particularly
>> +	useful for setups where the repository and worktrees may be moved between
>> +	different locations or environments.
>
> I think it would be helpful to spell out the implications of this to the
> user - namely that you cannot use older versions of git on a repository
> with worktrees using relative paths and it may break third-party
> software as well.

I've added a note that you cannot use older versions of Git with
worktrees when using relative paths.

>> +--[no-]relative-paths::
>> +	Worktrees will be linked to the repository using relative paths
>> +	rather than using absolute paths. This is particularly useful for setups
>> +	where the repository and worktrees may be moved between different
>> +	locations or environments.
>
> Again we should spell out the implications of using relative paths.

This has been revised to simply point to the config docs.

>> +With `repair`, the linking files will be updated if there's an absolute/relative
>> +mismatch, even if the links are correct.
>> ++
>> +This can also be set up as the default behaviour by using the
>> +`worktree.useRelativePaths` config option.
>> +
>>   --[no-]track::
>>   	When creating a new branch, if `<commit-ish>` is a branch,
>>   	mark it as "upstream" from the new branch.  This is the
>> 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()`

>> diff --git a/t/t2408-worktree-relative.sh b/t/t2408-worktree-relative.sh
>> deleted file mode 100755
>
> There's no explanation for this change in the commit message

I added an explanation for this deletion.

Best,
Caleb