Re: [PATCH v2 1/5] worktree: add CLI/config options for relative path linking
- From
Caleb White <cdwhite3@pm.me>
- Date
- Oct 30, 2024, 05:27 UTC
- Message-ID
- <D58W4HPIF8VX.2TWFTU2A2VSYY@pm.me>
- In-Reply-To
- <0bea06b8-11d0-497f-88e1-153cb61eb06d@gmail.com>
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".
Show 15 quoted lines
>> `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.
Show 12 quoted lines
>> 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.
Show 10 quoted lines
>> +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.
Show 7 quoted lines
>> +--[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.
Show 24 quoted lines
>> +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