git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 2/5] worktree: add `write_worktree_linking_files` function

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Oct 29, 2024, 14:52 UTC
Message-ID
<4a316a5f-9360-4f24-843f-bcbb5b3378c4@gmail.com>
In-Reply-To
<20241028-wt_relative_options-v2-2-33a5021bd7bb@pm.me>
Hi Caleb
On 28/10/2024 19:09, Caleb White wrote:
Show 5 quoted lines
> A new helper function, `write_worktree_linking_files()`, centralizes
> the logic for computing and writing either relative or absolute
> paths, based on the provided configuration. This function accepts
> `strbuf` pointers to both the worktree’s `.git` link and the
> repository’s `gitdir`, and then writes the appropriate path to each.

That sounds like a useful change. I think it would be better to pass an extra parameter "use_relative_paths" rather than relying on a global varibale in worktree.c. Thank you for adding some documentaion for the new function.

> This also teachs `git worktree repair` to fix the linking files if
> there is an absolute/relative paths but the links are correct. E.g.,
> `git worktree repair` can be used to convert a valid worktree between
> absolute and relative paths.

This might be better as a separate step so that reviewers can concentrate on the correctness of write_werktree_linking_files() when reviewing this patch.

Best Wishes
Phillip
Show 340 quoted lines
> Signed-off-by: Caleb White <cdwhite3@pm.me>
> ---
>   builtin/worktree.c |  11 +----
>   worktree.c         | 118 +++++++++++++++++++++++++++--------------------------
>   worktree.h         |  12 ++++++
>   3 files changed, 74 insertions(+), 67 deletions(-)
> 
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index c1130be5890c905c0b648782a834eb8dfcd79ba5..bb06830d6fe82aa97833c6e87f034115dfaa23bd 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -417,8 +417,7 @@ 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, sb_tmp = STRBUF_INIT;
> -	struct strbuf sb_path_realpath = STRBUF_INIT, sb_repo_realpath = STRBUF_INIT;
> +	struct strbuf sb = STRBUF_INIT;
>   	const char *name;
>   	struct strvec child_env = STRVEC_INIT;
>   	unsigned int counter = 0;
> @@ -494,10 +493,7 @@ static int add_worktree(const char *path, const char *refname,
>   
>   	strbuf_reset(&sb);
>   	strbuf_addf(&sb, "%s/gitdir", sb_repo.buf);
> -	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));
> -	write_file(sb_git.buf, "gitdir: %s", relative_path(sb_repo_realpath.buf, sb_path_realpath.buf, &sb_tmp));
> +	write_worktree_linking_files(sb_git, sb);
>   	strbuf_reset(&sb);
>   	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
>   	write_file(sb.buf, "../..");
> @@ -581,12 +577,9 @@ static int add_worktree(const char *path, const char *refname,
>   
>   	strvec_clear(&child_env);
>   	strbuf_release(&sb);
> -	strbuf_release(&sb_tmp);
>   	strbuf_release(&symref);
>   	strbuf_release(&sb_repo);
> -	strbuf_release(&sb_repo_realpath);
>   	strbuf_release(&sb_git);
> -	strbuf_release(&sb_path_realpath);
>   	strbuf_release(&sb_name);
>   	free_worktree(wt);
>   	return ret;
> diff --git a/worktree.c b/worktree.c
> index de5c5e53a5f2a758ddf470b5d6a9ad6c66247181..f4cee73d7a1edecafdff30b6d5e2d9dd1365b93e 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -381,29 +381,24 @@ 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 file = STRBUF_INIT;
> -	struct strbuf tmp = STRBUF_INIT;
> +	struct strbuf dotgit = STRBUF_INIT;
> +	struct strbuf gitdir = STRBUF_INIT;
>   
>   	if (is_main_worktree(wt))
>   		BUG("can't relocate main worktree");
>   
> -	strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
> +	strbuf_realpath(&gitdir, git_common_path("worktrees/%s/gitdir", wt->id), 1);
>   	strbuf_realpath(&path, path_, 1);
> +	strbuf_addf(&dotgit, "%s/.git", path.buf);
>   	if (fspathcmp(wt->path, path.buf)) {
> -		strbuf_addf(&file, "%s/gitdir", repo.buf);
> -		write_file(file.buf, "%s/.git", relative_path(path.buf, repo.buf, &tmp));
> -		strbuf_reset(&file);
> -		strbuf_addf(&file, "%s/.git", path.buf);
> -		write_file(file.buf, "gitdir: %s", relative_path(repo.buf, path.buf, &tmp));
> +		write_worktree_linking_files(dotgit, gitdir);
>   
>   		free(wt->path);
>   		wt->path = strbuf_detach(&path, NULL);
>   	}
>   	strbuf_release(&path);
> -	strbuf_release(&repo);
> -	strbuf_release(&file);
> -	strbuf_release(&tmp);
> +	strbuf_release(&dotgit);
> +	strbuf_release(&gitdir);
>   }
>   
>   int is_worktree_being_rebased(const struct worktree *wt,
> @@ -582,9 +577,9 @@ static void repair_gitfile(struct worktree *wt,
>   			   worktree_repair_fn fn, void *cb_data)
>   {
>   	struct strbuf dotgit = STRBUF_INIT;
> +	struct strbuf gitdir = STRBUF_INIT;
>   	struct strbuf repo = STRBUF_INIT;
>   	struct strbuf backlink = STRBUF_INIT;
> -	struct strbuf tmp = STRBUF_INIT;
>   	char *dotgit_contents = NULL;
>   	const char *repair = NULL;
>   	int err;
> @@ -600,6 +595,7 @@ static void repair_gitfile(struct worktree *wt,
>   
>   	strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
>   	strbuf_addf(&dotgit, "%s/.git", wt->path);
> +	strbuf_addf(&gitdir, "%s/gitdir", repo.buf);
>   	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
>   
>   	if (dotgit_contents) {
> @@ -617,18 +613,20 @@ static void repair_gitfile(struct worktree *wt,
>   		repair = _(".git file broken");
>   	else if (fspathcmp(backlink.buf, repo.buf))
>   		repair = _(".git file incorrect");
> +	else if (use_relative_paths == is_absolute_path(dotgit_contents))
> +		repair = _(".git file absolute/relative path mismatch");
>   
>   	if (repair) {
>   		fn(0, wt->path, repair, cb_data);
> -		write_file(dotgit.buf, "gitdir: %s", relative_path(repo.buf, wt->path, &tmp));
> +		write_worktree_linking_files(dotgit, gitdir);
>   	}
>   
>   done:
>   	free(dotgit_contents);
>   	strbuf_release(&repo);
>   	strbuf_release(&dotgit);
> +	strbuf_release(&gitdir);
>   	strbuf_release(&backlink);
> -	strbuf_release(&tmp);
>   }
>   
>   static void repair_noop(int iserr UNUSED,
> @@ -653,45 +651,30 @@ void repair_worktrees(worktree_repair_fn fn, void *cb_data)
>   
>   void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path)
>   {
> -	struct strbuf path = STRBUF_INIT;
> -	struct strbuf repo = STRBUF_INIT;
>   	struct strbuf gitdir = STRBUF_INIT;
>   	struct strbuf dotgit = STRBUF_INIT;
> -	struct strbuf olddotgit = STRBUF_INIT;
> -	struct strbuf tmp = STRBUF_INIT;
>   
>   	if (is_main_worktree(wt))
>   		goto done;
>   
> -	strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
> -	strbuf_addf(&gitdir, "%s/gitdir", repo.buf);
> +	strbuf_realpath(&gitdir, git_common_path("worktrees/%s/gitdir", wt->id), 1);
>   
> -	if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)
> +	if (strbuf_read_file(&dotgit, gitdir.buf, 0) < 0)
>   		goto done;
>   
> -	strbuf_rtrim(&olddotgit);
> -	if (is_absolute_path(olddotgit.buf)) {
> -		strbuf_addbuf(&dotgit, &olddotgit);
> -	} else {
> -		strbuf_addf(&dotgit, "%s/worktrees/%s/%s", old_path, wt->id, olddotgit.buf);
> +	strbuf_rtrim(&dotgit);
> +	if (!is_absolute_path(dotgit.buf)) {
> +		strbuf_insertf(&dotgit, 0, "%s/worktrees/%s/", old_path, wt->id);
>   		strbuf_realpath_forgiving(&dotgit, dotgit.buf, 0);
>   	}
>   
>   	if (!file_exists(dotgit.buf))
>   		goto done;
>   
> -	strbuf_addbuf(&path, &dotgit);
> -	strbuf_strip_suffix(&path, "/.git");
> -
> -	write_file(dotgit.buf, "gitdir: %s", relative_path(repo.buf, path.buf, &tmp));
> -	write_file(gitdir.buf, "%s", relative_path(dotgit.buf, repo.buf, &tmp));
> +	write_worktree_linking_files(dotgit, gitdir);
>   done:
> -	strbuf_release(&path);
> -	strbuf_release(&repo);
>   	strbuf_release(&gitdir);
>   	strbuf_release(&dotgit);
> -	strbuf_release(&olddotgit);
> -	strbuf_release(&tmp);
>   }
>   
>   void repair_worktrees_after_gitdir_move(const char *old_path)
> @@ -766,13 +749,10 @@ void repair_worktree_at_path(const char *path,
>   			     worktree_repair_fn fn, void *cb_data)
>   {
>   	struct strbuf dotgit = STRBUF_INIT;
> -	struct strbuf realdotgit = STRBUF_INIT;
>   	struct strbuf backlink = STRBUF_INIT;
>   	struct strbuf inferred_backlink = STRBUF_INIT;
>   	struct strbuf gitdir = STRBUF_INIT;
>   	struct strbuf olddotgit = STRBUF_INIT;
> -	struct strbuf realolddotgit = STRBUF_INIT;
> -	struct strbuf tmp = STRBUF_INIT;
>   	char *dotgit_contents = NULL;
>   	const char *repair = NULL;
>   	int err;
> @@ -784,25 +764,25 @@ void repair_worktree_at_path(const char *path,
>   		goto done;
>   
>   	strbuf_addf(&dotgit, "%s/.git", path);
> -	if (!strbuf_realpath(&realdotgit, dotgit.buf, 0)) {
> +	if (!strbuf_realpath(&dotgit, dotgit.buf, 0)) {
>   		fn(1, path, _("not a valid path"), cb_data);
>   		goto done;
>   	}
>   
> -	infer_backlink(realdotgit.buf, &inferred_backlink);
> +	infer_backlink(dotgit.buf, &inferred_backlink);
>   	strbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);
> -	dotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
> +	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
>   	if (dotgit_contents) {
>   		if (is_absolute_path(dotgit_contents)) {
>   			strbuf_addstr(&backlink, dotgit_contents);
>   		} else {
> -			strbuf_addbuf(&backlink, &realdotgit);
> +			strbuf_addbuf(&backlink, &dotgit);
>   			strbuf_strip_suffix(&backlink, ".git");
>   			strbuf_addstr(&backlink, dotgit_contents);
>   			strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
>   		}
>   	} else if (err == READ_GITFILE_ERR_NOT_A_FILE) {
> -		fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
> +		fn(1, dotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
>   		goto done;
>   	} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
>   		if (inferred_backlink.len) {
> @@ -815,11 +795,11 @@ void repair_worktree_at_path(const char *path,
>   			 */
>   			strbuf_swap(&backlink, &inferred_backlink);
>   		} else {
> -			fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
> +			fn(1, dotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
>   			goto done;
>   		}
>   	} else {
> -		fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
> +		fn(1, dotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
>   		goto done;
>   	}
>   
> @@ -841,39 +821,35 @@ void repair_worktree_at_path(const char *path,
>   	 * in the "copy" repository. In this case, point the "copy" worktree's
>   	 * .git file at the "copy" repository.
>   	 */
> -	if (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf)) {
> +	if (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf))
>   		strbuf_swap(&backlink, &inferred_backlink);
> -	}
>   
>   	strbuf_addf(&gitdir, "%s/gitdir", backlink.buf);
>   	if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)
>   		repair = _("gitdir unreadable");
> +	else if (use_relative_paths == is_absolute_path(olddotgit.buf))
> +		repair = _("gitdir absolute/relative path mismatch");
>   	else {
>   		strbuf_rtrim(&olddotgit);
> -		if (is_absolute_path(olddotgit.buf)) {
> -			strbuf_addbuf(&realolddotgit, &olddotgit);
> -		} else {
> -			strbuf_addf(&realolddotgit, "%s/%s", backlink.buf, olddotgit.buf);
> -			strbuf_realpath_forgiving(&realolddotgit, realolddotgit.buf, 0);
> +		if (!is_absolute_path(olddotgit.buf)) {
> +			strbuf_insertf(&olddotgit, 0, "%s/", backlink.buf);
> +			strbuf_realpath_forgiving(&olddotgit, olddotgit.buf, 0);
>   		}
> -		if (fspathcmp(realolddotgit.buf, realdotgit.buf))
> +		if (fspathcmp(olddotgit.buf, dotgit.buf))
>   			repair = _("gitdir incorrect");
>   	}
>   
>   	if (repair) {
>   		fn(0, gitdir.buf, repair, cb_data);
> -		write_file(gitdir.buf, "%s", relative_path(realdotgit.buf, backlink.buf, &tmp));
> +		write_worktree_linking_files(dotgit, gitdir);
>   	}
>   done:
>   	free(dotgit_contents);
>   	strbuf_release(&olddotgit);
> -	strbuf_release(&realolddotgit);
>   	strbuf_release(&backlink);
>   	strbuf_release(&inferred_backlink);
>   	strbuf_release(&gitdir);
> -	strbuf_release(&realdotgit);
>   	strbuf_release(&dotgit);
> -	strbuf_release(&tmp);
>   }
>   
>   int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, timestamp_t expire)
> @@ -1034,3 +1010,29 @@ int init_worktree_config(struct repository *r)
>   	free(main_worktree_file);
>   	return res;
>   }
> +
> +void write_worktree_linking_files(struct strbuf dotgit, struct strbuf gitdir)
> +{
> +	struct strbuf path = STRBUF_INIT;
> +	struct strbuf repo = STRBUF_INIT;
> +	struct strbuf tmp = STRBUF_INIT;
> +
> +	strbuf_addbuf(&path, &dotgit);
> +	strbuf_strip_suffix(&path, "/.git");
> +	strbuf_realpath(&path, path.buf, 1);
> +	strbuf_addbuf(&repo, &gitdir);
> +	strbuf_strip_suffix(&repo, "/gitdir");
> +	strbuf_realpath(&repo, repo.buf, 1);
> +
> +	if (use_relative_paths) {
> +		write_file(gitdir.buf, "%s/.git", relative_path(path.buf, repo.buf, &tmp));
> +		write_file(dotgit.buf, "gitdir: %s", relative_path(repo.buf, path.buf, &tmp));
> +	} else {
> +		write_file(gitdir.buf, "%s/.git", path.buf);
> +		write_file(dotgit.buf, "gitdir: %s", repo.buf);
> +	}
> +
> +	strbuf_release(&path);
> +	strbuf_release(&repo);
> +	strbuf_release(&tmp);
> +}
> diff --git a/worktree.h b/worktree.h
> index 37e65d508ed23d3e7a29850bb938285072a3aaa6..5929089891c97318a8f5329f7938264c717050d5 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -217,4 +217,16 @@ void strbuf_worktree_ref(const struct worktree *wt,
>    */
>   int init_worktree_config(struct repository *r);
>   
> +/**
> + * Write the .git file and gitdir file that links the worktree to the repository.
> + *
> + * The `dotgit` parameter is the path to the worktree's .git file, and `gitdir`
> + * is the path to the repository's `gitdir` file.
> + *
> + * Example
> + *  dotgit: "/path/to/foo/.git"
> + *  gitdir: "/path/to/repo/worktrees/foo/gitdir"
> + */
> +void write_worktree_linking_files(struct strbuf dotgit, struct strbuf gitdir);
> +
>   #endif
>
Previous: Caleb WhiteNext: Taylor Blau
Message 12 of 26 in “Allow relative worktree linking to be configured by the user”
  1. 0/5 Allow relative worktree linking to be configured by the userCaleb White, Oct 28, 2024
  2. 1/5 worktree: add CLI/config options for relative path linkingCaleb White, Oct 28, 2024
  3. Phillip WoodOct 29, 2024
  4. Caleb WhiteOct 30, 2024
  5. Taylor BlauOct 30, 2024
  6. Caleb WhiteOct 30, 2024
  7. phillip.wood123@gmail.comOct 30, 2024
  8. Caleb WhiteOct 30, 2024
  9. Taylor BlauOct 29, 2024
  10. Caleb WhiteOct 30, 2024
  11. 2/5 worktree: add `write_worktree_linking_files` functionCaleb White, Oct 28, 2024
  12. Phillip WoodOct 29, 2024
  13. Taylor BlauOct 29, 2024
  14. Caleb WhiteOct 30, 2024
  15. Caleb WhiteOct 30, 2024
  16. 3/5 worktree: add tests for worktrees with relative pathsCaleb White, Oct 28, 2024
  17. Phillip WoodOct 29, 2024
  18. Caleb WhiteOct 29, 2024
  19. phillip.wood123@gmail.comOct 29, 2024
  20. Caleb WhiteOct 30, 2024
  21. Taylor BlauOct 29, 2024
  22. Caleb WhiteOct 30, 2024
  23. 4/5 setup: correctly reinitialize repository versionCaleb White, Oct 28, 2024
  24. 5/5 worktree: add `relativeWorktrees` extensionCaleb White, Oct 28, 2024
  25. Phillip WoodOct 29, 2024
  26. Caleb WhiteOct 30, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.