Re: [PATCH v2 2/4] worktree: link worktrees with relative paths

From: shejialuo <shejialuo@gmail.com>
Date: Oct 6, 2024, 15:37 UTC
Message-ID: <ZwKuptTMf9ECd-kf@ArchLinux>
In-Reply-To: <20241006060017.171788-3-cdwhite3@pm.me>

On Sun, Oct 06, 2024 at 06:01:17AM +0000, Caleb White wrote:

Show 21 quoted lines

> This modifies Git’s handling of worktree linking to use relative
> paths instead of absolute paths. Previously, when creating a worktree,
> Git would store the absolute paths to both the main repository and the
> linked worktrees. These hardcoded absolute paths cause breakages such
> as when the repository is moved to a different directory or during
> containerized development where the absolute differs between systems.
> 
> By switching to relative paths, we help ensure that worktree links are
> more resilient when the repository is moved. While links external to the
> repository may still break, Git still automatically handles many common
> scenarios, reducing the need for manual repair. This is particularly
> useful in containerized or portable development environments, where the
> absolute path to the repository can differ between systems. Developers
> no longer need to reinitialize or repair worktrees after relocating the
> repository, improving workflow efficiency and reducing breakages.
> 
> For self-contained repositories (such as using a bare repository with
> worktrees), where both the repository and its worktrees are located
> within the same directory structure, using relative paths guarantees all
> links remain functional regardless of where the directory is located.
>

Eric has already commented on this commit message. I think this commit has done a lot of things which make the review painful.

Show 35 quoted lines

> Signed-off-by: Caleb White <cdwhite3@pm.me>
> ---
>  builtin/worktree.c           |  17 ++--
>  t/t2408-worktree-relative.sh |  39 +++++++++
>  worktree.c                   | 152 +++++++++++++++++++++++++----------
>  3 files changed, 159 insertions(+), 49 deletions(-)
>  create mode 100755 t/t2408-worktree-relative.sh
> 
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index fc31d07..99cee56 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -414,7 +414,8 @@ static int add_worktree(const char *path, const char *refname,
>  			const struct add_opts *opts)
>  {
>  	struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;
> -	struct strbuf sb = STRBUF_INIT, realpath = STRBUF_INIT;
> +	struct strbuf sb = STRBUF_INIT, sb_tmp = STRBUF_INIT;
> +	struct strbuf sb_path_realpath = STRBUF_INIT, sb_repo_realpath = STRBUF_INIT;
>  	const char *name;
>  	struct strvec child_env = STRVEC_INIT;
>  	unsigned int counter = 0;
> @@ -490,11 +491,11 @@ static int add_worktree(const char *path, const char *refname,
>  
>  	strbuf_reset(&sb);
>  	strbuf_addf(&sb, "%s/gitdir", sb_repo.buf);
> -	strbuf_realpath(&realpath, sb_git.buf, 1);
> -	write_file(sb.buf, "%s", realpath.buf);
> -	strbuf_realpath(&realpath, repo_get_common_dir(the_repository), 1);
> -	write_file(sb_git.buf, "gitdir: %s/worktrees/%s",
> -		   realpath.buf, name);
> +	strbuf_realpath(&sb_path_realpath, path, 1);
> +	strbuf_realpath(&sb_repo_realpath, sb_repo.buf, 1);
> +	write_file(sb.buf, "%s/.git", relative_path(sb_path_realpath.buf, sb_repo_realpath.buf, &sb_tmp));
> +	strbuf_reset(&sb_tmp);

Do we need reset the "sb_tmp"? I guess we do not need, "relative_path" does not care about "sb_tmp". It will always reset the value of the "sb_tmp". So, we may delete this line.

[snip]

Show 25 quoted lines

> diff --git a/worktree.c b/worktree.c
> index c6d2ede..fc14e9a 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -373,18 +379,30 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
>  void update_worktree_location(struct worktree *wt, const char *path_)
>  {
>  	struct strbuf path = STRBUF_INIT;
> +	struct strbuf repo = STRBUF_INIT;
> +	struct strbuf tmp = STRBUF_INIT;
> +	char *file = NULL;
>  
>  	if (is_main_worktree(wt))
>  		BUG("can't relocate main worktree");
>  
> +	strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
>  	strbuf_realpath(&path, path_, 1);
>  	if (fspathcmp(wt->path, path.buf)) {
> -		write_file(git_common_path("worktrees/%s/gitdir", wt->id),
> -			   "%s/.git", path.buf);
> +		file = xstrfmt("%s/gitdir", repo.buf);
> +		write_file(file, "%s/.git", relative_path(path.buf, repo.buf, &tmp));
> +		free(file);
> +		strbuf_reset(&tmp);
> +		file = xstrfmt("%s/.git", path.buf);

Still, we do not need to call "strbuf_reset" again for "tmp". But there is another question here. Should we define the "file" just in this "if" block and free "file" also in the block?

And I don't think it's a good idea to use "xstrfmt". Here, we need to allocate two times and free two times. Why not just define a "struct strbuf" and the use "strbuf_*" method here?

Show 14 quoted lines

> +		write_file(file, "gitdir: %s", relative_path(repo.buf, path.buf, &tmp));
> +
>  		free(wt->path);
>  		wt->path = strbuf_detach(&path, NULL);
>  	}
> +	free(file);
>  	strbuf_release(&path);
> +	strbuf_release(&repo);
> +	strbuf_release(&tmp);
>  }
>  
> @@ -564,38 +582,52 @@ static void repair_gitfile(struct worktree *wt,
>  {
>

[snip]

>  	strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
>  	strbuf_addf(&dotgit, "%s/.git", wt->path);
> -	backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
> +	git_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));

Why here we need to use "xstrdup_or_null". The life cycle of the "git_contents" variable is in the "repair_gitfile" function.

[snip]

I omit the review for the later code, there are too many codes to review. And due to my limited knowledge about worktree, the overhead will be too big for me.

Thanks, Jialuo