On Tue Oct 29, 2024 at 1:42 PM CDT, Taylor Blau wrote:
> On Mon, Oct 28, 2024 at 07:09:37PM +0000, Caleb White wrote:
>> diff --git a/Documentation/config/worktree.txt b/Documentation/config/worktree.txt
>> index 048e349482df6c892055720eb53cdcd6c327b6ed..44b783c2774dc5ff65e3fa232b0c25cd5254876b 100644
>> --- a/Documentation/config/worktree.txt
>> +++ b/Documentation/config/worktree.txt
>> @@ -7,3 +7,8 @@ worktree.guessRemote::
>> such a branch exists, it is checked out and set as "upstream"
>> for the new branch. If no such match can be found, it falls
>> back to creating a new branch from the current HEAD.
>
> I would have thought there would be a blank line in between this and the
> section on worktree.guessRemote. ASCIIDoc doesn't require it because
> this is a labeled list, but it does improve the readability of the raw
> ASCIIDoc itself.
>
> So not a big deal, but if you end up sending out another version of this
> series it would be nice to include.
I'll add the blank line in the next version of the 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.
>
> This is a good start, but I have a few suggestions on top that I'm
> curious of your thoughts on. First: what is the default? Users
> should have some insight into what the default is. Likewise, they should
> know that that the default behavior does not introduce the repository
> extension, but that setting this configuration to 'true' does.
>
> Maybe something like the following on top?
>
> --- 8< ---
> diff --git a/Documentation/config/worktree.txt b/Documentation/config/worktree.txt
> index 44b783c277..666cb3c190 100644
> --- a/Documentation/config/worktree.txt
> +++ b/Documentation/config/worktree.txt
> @@ -7,8 +7,13 @@ worktree.guessRemote::
> such a branch exists, it is checked out and set as "upstream"
> for the new branch. If no such match can be found, it falls
> back to creating a new branch from the current HEAD.
> +
> 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.
> + Link worktrees using relative paths (when "true") or absolute
> + paths (when "false"). This is particularly useful for setups
> + where the repository and worktrees may be moved between
> + different locations or environments. Defaults to "false".
> ++
> +Note that setting `worktree.useRelativePaths` to "true" implies
> +enabling the "relativeWorktrees" repository extension, thus making it
> +incompatible with older versions of Git.
Sounds good to me. I'll update.
>> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
>> index 70437c815f13852bd2eb862176b8b933e6de0acf..975dc3c46d480480457ec4857988a6b8bc67b647 100644
>> --- a/Documentation/git-worktree.txt
>> +++ b/Documentation/git-worktree.txt
>> @@ -216,6 +216,18 @@ To remove a locked worktree, specify `--force` twice.
>> This can also be set up as the default behaviour by using the
>> `worktree.guessRemote` config option.
>>
>> +--[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.
>
> This paragraph is redundant with what you wrote in git-config(1). I
> think all we want to say is that it overrides the setting of that
> configuration variable, and refer users there with linkgit.
I agree. I'll update this paragraph to refer to the config documentation.
>> ++
>> +With `repair`, the linking files will be updated if there's an absolute/relative
>> +mismatch, even if the links are correct.
>
> This is worth keeping.
Keeping this as is.
>> +This can also be set up as the default behaviour by using the
>> +`worktree.useRelativePaths` config option.
>> +
>
> This should get folded into my suggestion above.
Done.
>> diff --git a/t/t2408-worktree-relative.sh b/t/t2408-worktree-relative.sh
>> deleted file mode 100755
>> index a3136db7e28cb20926ff44211e246ce625a6e51a..0000000000000000000000000000000000000000
>> --- a/t/t2408-worktree-relative.sh
>> +++ /dev/null
>> @@ -1,39 +0,0 @@
>
> Was removing t2408 intentional here? I don't see the tests being
> re-added elsewhere in this patch (though they may be introduced
> elsewhere later in the series, I haven't read that far yet). Either way,
> it may be worth mentioning in the commit message to avoid confusing
> readers.
Yes, this was intentional. This was added in the original round when the
default was changed to use relative paths. I added more comprehensive
tests in the various worktree files to cover the new functionality. I
will make sure to mention this in the commit message.
>> diff --git a/worktree.c b/worktree.c
>> index 77ff484d3ec48c547ee4e3d958dfa28a52c1eaa7..de5c5e53a5f2a758ddf470b5d6a9ad6c66247181 100644
>> --- a/worktree.c
>> +++ b/worktree.c
>> @@ -14,6 +14,8 @@
>> #include "wt-status.h"
>> #include "config.h"
>>
>> +int use_relative_paths = 0;
>
> I wondered whether 'use_relative_paths' should be static, or if we need to extern it in
> from somewhere else in the tree. But we do, from worktree.[ch], which
> seems reasonable. It would be nice if there was some way to thread that
> into the worktree.h API, but I think that this is a reasonable measure
> to take for now.
I can add a getter/setter to the worktree API so we're not using a
global variable.
>> +
>> void free_worktree(struct worktree *worktree)
>> {
>> if (!worktree)
>> @@ -111,9 +113,9 @@ struct worktree *get_linked_worktree(const char *id,
>> strbuf_strip_suffix(&worktree_path, "/.git");
>>
>> if (!is_absolute_path(worktree_path.buf)) {
>> - strbuf_strip_suffix(&path, "gitdir");
>> - strbuf_addbuf(&path, &worktree_path);
>> - strbuf_realpath_forgiving(&worktree_path, path.buf, 0);
>> + strbuf_strip_suffix(&path, "gitdir");
>> + strbuf_addbuf(&path, &worktree_path);
>> + strbuf_realpath_forgiving(&worktree_path, path.buf, 0);
>
> Whitespace change?
Yes, this was added in the original round, I didn't notice that it used
4 spaces instead of a tab. I had fixed this in v4 of the original round,
but this was merged in v3 so I just went ahead and fixed it here.
>> CALLOC_ARRAY(worktree, 1);
>> @@ -725,12 +727,15 @@ static int is_main_worktree_path(const char *path)
>> * won't know which <repo>/worktrees/<id>/gitdir to repair. However, we may
>> * be able to infer the gitdir by manually reading /path/to/worktree/.git,
>> * extracting the <id>, and checking if <repo>/worktrees/<id> exists.
>> + *
>> + * Returns -1 on failure and strbuf.len on success.
>> */
>> static int infer_backlink(const char *gitfile, struct strbuf *inferred)
>
> Should this return an ssize_t instead, then? I don't think we're going
> to have worktree paths that are actually larger than INT_MAX, but it
> seems hygienic and good to prevent any accidental overflow issues.
I thought about this, but you'll run into OS limits long before you hit
INT_MAX. However, I can make this change to be hygienic.
Best,
Caleb
