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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Caleb

Thanks for working on this.

On 28/10/2024 19:09, Caleb White 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 ..`

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

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

> [1]: https://lore.kernel.org/git/20241007-wt_relative_paths-v3-0-622cf18c45eb@pm.me
> 
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Caleb White <cdwhite3@pm.me>
> ---
>   Documentation/config/worktree.txt |  5 +++++
>   Documentation/git-worktree.txt    | 12 ++++++++++++
>   builtin/worktree.c                |  9 +++++++++
>   t/t2408-worktree-relative.sh      | 39 ---------------------------------------
>   worktree.c                        | 17 ++++++++++-------
>   worktree.h                        |  2 ++
>   6 files changed, 38 insertions(+), 46 deletions(-)
> 
> 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.
> +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.

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

Again we should spell out the implications of using relative paths.

> +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".

> 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

Best Wishes

Phillip

> index a3136db7e28cb20926ff44211e246ce625a6e51a..0000000000000000000000000000000000000000
> --- a/t/t2408-worktree-relative.sh
> +++ /dev/null
> @@ -1,39 +0,0 @@
> -#!/bin/sh
> -
> -test_description='test worktrees linked with relative paths'
> -
> -TEST_PASSES_SANITIZE_LEAK=true
> -. ./test-lib.sh
> -
> -test_expect_success 'links worktrees with relative paths' '
> -	test_when_finished rm -rf repo &&
> -	git init repo &&
> -	(
> -		cd repo &&
> -		test_commit initial &&
> -		git worktree add wt1 &&
> -		echo "../../../wt1/.git" >expected_gitdir &&
> -		cat .git/worktrees/wt1/gitdir >actual_gitdir &&
> -		echo "gitdir: ../.git/worktrees/wt1" >expected_git &&
> -		cat wt1/.git >actual_git &&
> -		test_cmp expected_gitdir actual_gitdir &&
> -		test_cmp expected_git actual_git
> -	)
> -'
> -
> -test_expect_success 'move repo without breaking relative internal links' '
> -	test_when_finished rm -rf repo moved &&
> -	git init repo &&
> -	(
> -		cd repo &&
> -		test_commit initial &&
> -		git worktree add wt1 &&
> -		cd .. &&
> -		mv repo moved &&
> -		cd moved/wt1 &&
> -		git status >out 2>err &&
> -		test_must_be_empty err
> -	)
> -'
> -
> -test_done
> 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;
> +
>   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);
>   	}
>   
>   	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)
>   {
>   	struct strbuf actual = STRBUF_INIT;
>   	const char *id;
>   
> +	strbuf_reset(inferred);
>   	if (strbuf_read_file(&actual, gitfile, 0) < 0)
>   		goto error;
>   	if (!starts_with(actual.buf, "gitdir:"))
> @@ -741,18 +746,16 @@ static int infer_backlink(const char *gitfile, struct strbuf *inferred)
>   	id++; /* advance past '/' to point at <id> */
>   	if (!*id)
>   		goto error;
> -	strbuf_reset(inferred);
>   	strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id);
>   	if (!is_directory(inferred->buf))
>   		goto error;
>   
>   	strbuf_release(&actual);
> -	return 1;
> -
> +	return inferred->len;
>   error:
>   	strbuf_release(&actual);
>   	strbuf_reset(inferred); /* clear invalid path */
> -	return 0;
> +	return -1;
>   }
>   
>   /*
> diff --git a/worktree.h b/worktree.h
> index e96118621638667d87c5d7e0452ed10bd1ddf606..37e65d508ed23d3e7a29850bb938285072a3aaa6 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -5,6 +5,8 @@
>   
>   struct strbuf;
>   
> +extern int use_relative_paths;
> +
>   struct worktree {
>   	/* The repository this worktree belongs to. */
>   	struct repository *repo;
>