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
