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 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