[PATCH v3 0/3] Link worktrees with relative paths

[PATCH v3 0/3] Link worktrees with relative paths

[Caleb White via B4 Relay]

Git currently stores absolute paths to both the main repository and
linked worktrees. However, this causes problems when moving repositories
or working in containerized environments where absolute paths differ
between systems. The worktree links break, and users are required to
manually execute `worktree repair` to repair them, leading to workflow
disruptions. Additionally, mapping repositories inside of containerized
environments renders the repository unusable inside the containers, and
this is not repairable as repairing the worktrees inside the containers
will result in them being broken outside the containers.

To address this, this patch series makes Git always write relative paths
when linking worktrees. Relative paths increase the resilience of the
worktree links across various systems and environments, particularly
when the worktrees are self-contained inside the main repository (such
as when using a bare repository with worktrees). This improves
portability, workflow efficiency, and reduces overall breakages.

Although Git now writes relative paths, existing repositories with
absolute paths are still supported. There are no breaking changes
to workflows based on absolute paths, ensuring backward compatibility.

Signed-off-by: Caleb White <cdwhite3@pm.me>
---
Changes in v3:
- Squashed patch [3/4] into patch [2/4] to streamline changes.
- Dropped patch [4/4] as it was no longer necessary.
- Rebased onto 20240923075416.54289-1-ericsunshine@charter.net
- Updated `infer_backlink()` to return 1 on success for consistency.
- Swapped the order of `infer_backlink()` arguments for clarity.
- Clear `inferred` if error occurs in `infer_backlink()`.
- Renamed `git_contents` to `dotgit_contents` for clearer semantics.
- Cleaned up `dotgit_contents` logic in `repair_worktree_at_path()`.
- Replaced multiple `xstrfmt/free` calls with a single `strbuf`.
- Added a test case covering a failure noted in a separate patch series.
- Improved commit messages.
- Link to v2: https://lore.kernel.org/r/20241006060017.171788-1-cdwhite3@pm.me
Changes in v2:
- No changes, just a resubmission
- Link to v1: https://lore.kernel.org/git/20241006045847.159937-1-cdwhite3@pm.me/

Range-diff against v2:
1:  735a90355e ! 1:  b5060dba05 worktree: refactor infer_backlink() to use *strbuf
    @@ Metadata
      ## Commit message ##
         worktree: refactor infer_backlink() to use *strbuf
     
    -    This refactors the `infer_backlink` function to return an integer
    -    result and use a pre-allocated `strbuf` for the inferred backlink
    -    path, replacing the previous `char*` return type.
    +    This lays the groundwork for the next patch, which needs the backlink
    +    returned from infer_backlink() as a `strbuf`. It seemed inefficient to
    +    convert from `strbuf` to `char*` and back to `strbuf` again.
     
    -    This lays the groundwork for the next patch, which needs the
    -    resultant backlink as a `strbuf`. There was no need to go from
    -    `strbuf -> char* -> strbuf` again. This change also aligns the
    -    function's signature with other `strbuf`-based functions.
    +    This refactors infer_backlink() to return an integer result and use a
    +    pre-allocated `strbuf` for the inferred backlink path, replacing the
    +    previous `char*` return type and improving efficiency.
     
         Signed-off-by: Caleb White <cdwhite3@pm.me>
     
    @@ worktree.c: static int is_main_worktree_path(const char *path)
       * extracting the <id>, and checking if <repo>/worktrees/<id> exists.
       */
     -static char *infer_backlink(const char *gitfile)
    -+static int infer_backlink(struct strbuf *inferred, const char *gitfile)
    ++static int infer_backlink(const char *gitfile, struct strbuf *inferred)
      {
        struct strbuf actual = STRBUF_INIT;
     -	struct strbuf inferred = STRBUF_INIT;
    @@ worktree.c: static char *infer_backlink(const char *gitfile)
                goto error;
     -	strbuf_git_common_path(&inferred, the_repository, "worktrees/%s", id);
     -	if (!is_directory(inferred.buf))
    ++	strbuf_reset(inferred);
     +	strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id);
     +	if (!is_directory(inferred->buf))
                goto error;
      
        strbuf_release(&actual);
     -	return strbuf_detach(&inferred, NULL);
    -+	return 0;
    ++	return 1;
      
      error:
        strbuf_release(&actual);
     -	strbuf_release(&inferred);
     -	return NULL;
    -+	return 1;
    ++	strbuf_reset(inferred); /* clear invalid path */
    ++	return 0;
      }
      
      /*
    @@ worktree.c: void repair_worktree_at_path(const char *path,
        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;
     -	char *backlink = NULL;
    -+	char *git_contents = NULL;
    +-	char *inferred_backlink = NULL;
    ++	char *dotgit_contents = NULL;
        const char *repair = NULL;
        int err;
      
    @@ worktree.c: void repair_worktree_at_path(const char *path,
                goto done;
        }
      
    +-	inferred_backlink = infer_backlink(realdotgit.buf);
     -	backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
    -+	git_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
    - 	if (err == READ_GITFILE_ERR_NOT_A_FILE) {
    +-	if (err == READ_GITFILE_ERR_NOT_A_FILE) {
    ++	infer_backlink(realdotgit.buf, &inferred_backlink);
    ++	dotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
    ++	if (dotgit_contents) {
    ++		strbuf_addstr(&backlink, dotgit_contents);
    ++	} else if (err == READ_GITFILE_ERR_NOT_A_FILE) {
                fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
                goto done;
        } else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
    --		if (!(backlink = infer_backlink(realdotgit.buf))) {
    -+		if (infer_backlink(&backlink, realdotgit.buf)) {
    +-		if (inferred_backlink) {
    ++		if (inferred_backlink.len) {
    + 			/*
    + 			 * Worktree's .git file does not point at a repository
    + 			 * but we found a .git/worktrees/<id> in this
    + 			 * repository with the same <id> as recorded in the
    + 			 * worktree's .git file so make the worktree point at
    +-			 * the discovered .git/worktrees/<id>. (Note: backlink
    +-			 * is already NULL, so no need to free it first.)
    ++			 * the discovered .git/worktrees/<id>.
    + 			 */
    +-			backlink = inferred_backlink;
    +-			inferred_backlink = NULL;
    ++			strbuf_swap(&backlink, &inferred_backlink);
    + 		} else {
                        fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
                        goto done;
    - 		}
    - 	} else if (err) {
    - 		fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
    - 		goto done;
    -+	} else if (git_contents) {
    -+		strbuf_addstr(&backlink, git_contents);
    +@@ worktree.c: 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 && fspathcmp(backlink, inferred_backlink)) {
    +-		free(backlink);
    +-		backlink = inferred_backlink;
    +-		inferred_backlink = NULL;
    ++	if (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf)) {
    ++		strbuf_swap(&backlink, &inferred_backlink);
        }
      
     -	strbuf_addf(&gitdir, "%s/gitdir", backlink);
    @@ worktree.c: void repair_worktree_at_path(const char *path,
        }
      done:
     -	free(backlink);
    -+	free(git_contents);
    +-	free(inferred_backlink);
    ++	free(dotgit_contents);
        strbuf_release(&olddotgit);
     +	strbuf_release(&backlink);
    ++	strbuf_release(&inferred_backlink);
        strbuf_release(&gitdir);
        strbuf_release(&realdotgit);
        strbuf_release(&dotgit);
2:  f128dfa53c ! 2:  017f199988 worktree: link worktrees with relative paths
    @@ Metadata
      ## Commit message ##
         worktree: link worktrees with relative paths
     
    -    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.
    +    Git currently stores absolute paths to both the main repository and
    +    linked worktrees. However, this causes problems when moving repositories
    +    or working in containerized environments where absolute paths differ
    +    between systems. The worktree links break, and users are required to
    +    manually execute `worktree repair` to repair them, leading to workflow
    +    disruptions. Additionally, mapping repositories inside of containerized
    +    environments renders the repository unusable inside the containers, and
    +    this is not repairable as repairing the worktrees inside the containers
    +    will result in them being broken outside the containers.
     
    -    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.
    +    To address this, this patch makes Git always write relative paths when
    +    linking worktrees. Relative paths increase the resilience of the
    +    worktree links across various systems and environments, particularly
    +    when the worktrees are self-contained inside the main repository (such
    +    as when using a bare repository with worktrees). This improves
    +    portability, workflow efficiency, and reduces overall 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.
    +    Although Git now writes relative paths, existing repositories with
    +    absolute paths are still supported. There are no breaking changes
    +    to workflows based on absolute paths, ensuring backward compatibility.
    +
    +    At a low level, the changes involve modifying functions in `worktree.c`
    +    and `builtin/worktree.c` to use `relative_path()` when writing the
    +    worktree’s `.git` file and the main repository’s `gitdir` reference.
    +    Instead of hardcoding absolute paths, Git now computes the relative path
    +    between the worktree and the repository, ensuring that these links are
    +    portable. Locations where these respective file are read have also been
    +    updated to properly handle both absolute and relative paths. Generally,
    +    relative paths are always resolved into absolute paths before any
    +    operations or comparisons are performed.
    +
    +    Additionally, `repair_worktrees_after_gitdir_move()` has been introduced
    +    to address the case where both the `<worktree>/.git` and
    +    `<repo>/worktrees/<id>/gitdir` links are broken after the gitdir is
    +    moved (such as during a re-initialization). This function repairs both
    +    sides of the worktree link using the old gitdir path to reestablish the
    +    correct paths after a move.
    +
    +    The `worktree.path` struct member has also been updated to always store
    +    the absolute path of a worktree. This ensures that worktree consumers
    +    never have to worry about trying to resolve the absolute path themselves.
     
         Signed-off-by: Caleb White <cdwhite3@pm.me>
     
    @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
     +	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);
     +	write_file(sb_git.buf, "gitdir: %s", relative_path(sb_repo_realpath.buf, sb_path_realpath.buf, &sb_tmp));
        strbuf_reset(&sb);
        strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
    @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
        return ret;
      }
     
    + ## setup.c ##
    +@@ setup.c: static void separate_git_dir(const char *git_dir, const char *git_link)
    + 
    + 		if (rename(src, git_dir))
    + 			die_errno(_("unable to move %s to %s"), src, git_dir);
    +-		repair_worktrees(NULL, NULL);
    ++		repair_worktrees_after_gitdir_move(src);
    + 	}
    + 
    + 	write_file(git_link, "gitdir: %s", git_dir);
    +
      ## t/t2408-worktree-relative.sh (new) ##
     @@
     +#!/bin/sh
    @@ worktree.c: int validate_worktree(const struct worktree *wt, struct strbuf *errm
      {
        struct strbuf path = STRBUF_INIT;
     +	struct strbuf repo = STRBUF_INIT;
    ++	struct strbuf file = STRBUF_INIT;
     +	struct strbuf tmp = STRBUF_INIT;
    -+	char *file = NULL;
      
        if (is_main_worktree(wt))
                BUG("can't relocate main worktree");
    @@ worktree.c: int validate_worktree(const struct worktree *wt, struct strbuf *errm
        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);
    -+		write_file(file, "gitdir: %s", relative_path(repo.buf, path.buf, &tmp));
    ++		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));
     +
                free(wt->path);
                wt->path = strbuf_detach(&path, NULL);
        }
    -+	free(file);
        strbuf_release(&path);
     +	strbuf_release(&repo);
    ++	strbuf_release(&file);
     +	strbuf_release(&tmp);
      }
      
    @@ worktree.c: static void repair_gitfile(struct worktree *wt,
     -	char *backlink;
     +	struct strbuf backlink = STRBUF_INIT;
     +	struct strbuf tmp = STRBUF_INIT;
    -+	char *git_contents = NULL;
    ++	char *dotgit_contents = NULL;
        const char *repair = NULL;
        int err;
      
    @@ worktree.c: 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);
     -	backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
    -+	git_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
    ++	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
     +
    -+	if (git_contents) {
    -+		if (is_absolute_path(git_contents)) {
    -+			strbuf_addstr(&backlink, git_contents);
    ++	if (dotgit_contents) {
    ++		if (is_absolute_path(dotgit_contents)) {
    ++			strbuf_addstr(&backlink, dotgit_contents);
     +		} else {
    -+			strbuf_addf(&backlink, "%s/%s", wt->path, git_contents);
    ++			strbuf_addf(&backlink, "%s/%s", wt->path, dotgit_contents);
     +			strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
     +		}
     +	}
    @@ worktree.c: static void repair_gitfile(struct worktree *wt,
      
     -	free(backlink);
     +done:
    -+	free(git_contents);
    ++	free(dotgit_contents);
        strbuf_release(&repo);
        strbuf_release(&dotgit);
     +	strbuf_release(&backlink);
    @@ worktree.c: static void repair_gitfile(struct worktree *wt,
      }
      
      static void repair_noop(int iserr UNUSED,
    +@@ worktree.c: void repair_worktrees(worktree_repair_fn fn, void *cb_data)
    + 	free_worktrees(worktrees);
    + }
    + 
    ++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);
    ++
    ++	if (strbuf_read_file(&olddotgit, 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_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));
    ++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)
    ++{
    ++	struct worktree **worktrees = get_worktrees_internal(1);
    ++	struct worktree **wt = worktrees + 1; /* +1 skips main worktree */
    ++
    ++	for (; *wt; wt++)
    ++		repair_worktree_after_gitdir_move(*wt, old_path);
    ++	free_worktrees(worktrees);
    ++}
    ++
    + static int is_main_worktree_path(const char *path)
    + {
    + 	struct strbuf target = STRBUF_INIT;
     @@ worktree.c: void repair_worktree_at_path(const char *path,
    - 	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 *git_contents = NULL;
    + 	char *dotgit_contents = NULL;
        const char *repair = NULL;
        int err;
     @@ worktree.c: void repair_worktree_at_path(const char *path,
    - 		fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
    - 		goto done;
    - 	} else if (git_contents) {
    --		strbuf_addstr(&backlink, git_contents);
    -+		if (is_absolute_path(git_contents)) {
    -+			strbuf_addstr(&backlink, git_contents);
    + 	}
    + 
    + 	infer_backlink(realdotgit.buf, &inferred_backlink);
    ++	strbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);
    + 	dotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
    + 	if (dotgit_contents) {
    +-		strbuf_addstr(&backlink, dotgit_contents);
    ++		if (is_absolute_path(dotgit_contents)) {
    ++			strbuf_addstr(&backlink, dotgit_contents);
     +		} else {
     +			strbuf_addbuf(&backlink, &realdotgit);
     +			strbuf_strip_suffix(&backlink, ".git");
    -+			strbuf_addstr(&backlink, git_contents);
    ++			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);
    + 		goto done;
    +@@ worktree.c: void repair_worktree_at_path(const char *path,
    + 			fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
    + 			goto done;
    + 		}
    +-	} else if (err) {
    ++	} else {
    + 		fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
    + 		goto done;
        }
    - 
    -+	strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
    - 	strbuf_addf(&gitdir, "%s/gitdir", backlink.buf);
    - 	if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)
    +@@ worktree.c: void repair_worktree_at_path(const char *path,
                repair = _("gitdir unreadable");
        else {
                strbuf_rtrim(&olddotgit);
    @@ worktree.c: void repair_worktree_at_path(const char *path,
     +		write_file(gitdir.buf, "%s", relative_path(realdotgit.buf, backlink.buf, &tmp));
        }
      done:
    - 	free(git_contents);
    + 	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);
    @@ worktree.c: void repair_worktree_at_path(const char *path,
     +	struct strbuf dotgit = STRBUF_INIT;
     +	struct strbuf gitdir = STRBUF_INIT;
     +	struct strbuf repo = STRBUF_INIT;
    ++	struct strbuf file = STRBUF_INIT;
     +	char *path = NULL;
    -+	char *file = NULL;
     +	int rc = 0;
        int fd;
        size_t len;
    @@ worktree.c: void repair_worktree_at_path(const char *path,
     -	if (file_exists(git_path("worktrees/%s/locked", id)))
     -		return 0;
     -	if (stat(git_path("worktrees/%s/gitdir", id), &st)) {
    -+	file = xstrfmt("%s/locked", repo.buf);
    -+	if (file_exists(file)) {
    ++	strbuf_addf(&file, "%s/locked", repo.buf);
    ++	if (file_exists(file.buf)) {
     +		goto done;
     +	}
     +	if (stat(gitdir.buf, &st)) {
    @@ worktree.c: void repair_worktree_at_path(const char *path,
     +		strbuf_realpath_forgiving(&dotgit, dotgit.buf, 0);
     +	}
     +	if (!file_exists(dotgit.buf)) {
    -+		free(file);
    -+		file = xstrfmt("%s/index", repo.buf);
    -+		if (stat(file, &st) || st.st_mtime <= expire) {
    ++		strbuf_reset(&file);
    ++		strbuf_addf(&file, "%s/index", repo.buf);
    ++		if (stat(file.buf, &st) || st.st_mtime <= expire) {
                        strbuf_addstr(reason, _("gitdir file points to non-existent location"));
     -			free(path);
     -			return 1;
    @@ worktree.c: void repair_worktree_at_path(const char *path,
     +	*wtpath = strbuf_detach(&dotgit, NULL);
     +done:
     +	free(path);
    -+	free(file);
     +	strbuf_release(&dotgit);
     +	strbuf_release(&gitdir);
     +	strbuf_release(&repo);
    ++	strbuf_release(&file);
     +	return rc;
      }
      
      static int move_config_setting(const char *key, const char *value,
    +
    + ## worktree.h ##
    +@@ worktree.h: typedef void (* worktree_repair_fn)(int iserr, const char *path,
    +  */
    + void repair_worktrees(worktree_repair_fn, void *cb_data);
    + 
    ++/*
    ++ * Repair the linked worktrees after the gitdir has been moved.
    ++ */
    ++void repair_worktrees_after_gitdir_move(const char *old_path);
    ++
    ++/*
    ++ * Repair the linked worktree after the gitdir has been moved.
    ++ */
    ++void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path);
    ++
    + /*
    +  * Repair administrative files corresponding to the worktree at the given path.
    +  * The worktree's .git file pointing at the repository must be intact for the
3:  2347affc96 < -:  ---------- worktree: sync worktree paths after gitdir move
4:  64638e39c3 < -:  ---------- worktree: prevent null pointer dereference
-:  ---------- > 3:  a3ea6625f2 worktree: add test for path handling in linked worktrees

---
Caleb White (3):
      worktree: refactor infer_backlink() to use *strbuf
      worktree: link worktrees with relative paths
      worktree: add test for path handling in linked worktrees

 builtin/worktree.c           |  16 +--
 setup.c                      |   2 +-
 t/t2401-worktree-prune.sh    |  19 ++++
 t/t2408-worktree-relative.sh |  39 +++++++
 worktree.c                   | 253 ++++++++++++++++++++++++++++++++-----------
 worktree.h                   |  10 ++
 6 files changed, 265 insertions(+), 74 deletions(-)
---
base-commit: 4ec4435df758668055cc904ef64c275bc8d1089b
change-id: 20241007-wt_relative_paths-88f9cf5a070c
prerequisite-message-id: <20240923075416.54289-1-ericsunshine@charter.net>
prerequisite-patch-id: 78371f4dbb635e6edab8c51ee7857b903e60df8f

Best regards,
-- 
Caleb White <cdwhite3@pm.me>

[PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf

[Caleb White via B4 Relay]

From: Caleb White <cdwhite3@pm.me>

This lays the groundwork for the next patch, which needs the backlink
returned from infer_backlink() as a `strbuf`. It seemed inefficient to
convert from `strbuf` to `char*` and back to `strbuf` again.

This refactors infer_backlink() to return an integer result and use a
pre-allocated `strbuf` for the inferred backlink path, replacing the
previous `char*` return type and improving efficiency.

Signed-off-by: Caleb White <cdwhite3@pm.me>
---
 worktree.c | 48 ++++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/worktree.c b/worktree.c
index ec95ea2986107b3bc12d38b0825d7c6e87402bc6..0cba0d6e6e9ad02ace04a0301104a04a07cbef65 100644
--- a/worktree.c
+++ b/worktree.c
@@ -642,10 +642,9 @@ static int is_main_worktree_path(const char *path)
  * be able to infer the gitdir by manually reading /path/to/worktree/.git,
  * extracting the <id>, and checking if <repo>/worktrees/<id> exists.
  */
-static char *infer_backlink(const char *gitfile)
+static int infer_backlink(const char *gitfile, struct strbuf *inferred)
 {
 	struct strbuf actual = STRBUF_INIT;
-	struct strbuf inferred = STRBUF_INIT;
 	const char *id;
 
 	if (strbuf_read_file(&actual, gitfile, 0) < 0)
@@ -658,17 +657,18 @@ static char *infer_backlink(const char *gitfile)
 	id++; /* advance past '/' to point at <id> */
 	if (!*id)
 		goto error;
-	strbuf_git_common_path(&inferred, the_repository, "worktrees/%s", id);
-	if (!is_directory(inferred.buf))
+	strbuf_reset(inferred);
+	strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id);
+	if (!is_directory(inferred->buf))
 		goto error;
 
 	strbuf_release(&actual);
-	return strbuf_detach(&inferred, NULL);
+	return 1;
 
 error:
 	strbuf_release(&actual);
-	strbuf_release(&inferred);
-	return NULL;
+	strbuf_reset(inferred); /* clear invalid path */
+	return 0;
 }
 
 /*
@@ -680,10 +680,11 @@ void repair_worktree_at_path(const char *path,
 {
 	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;
-	char *backlink = NULL;
-	char *inferred_backlink = NULL;
+	char *dotgit_contents = NULL;
 	const char *repair = NULL;
 	int err;
 
@@ -699,23 +700,23 @@ void repair_worktree_at_path(const char *path,
 		goto done;
 	}
 
-	inferred_backlink = infer_backlink(realdotgit.buf);
-	backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
-	if (err == READ_GITFILE_ERR_NOT_A_FILE) {
+	infer_backlink(realdotgit.buf, &inferred_backlink);
+	dotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
+	if (dotgit_contents) {
+		strbuf_addstr(&backlink, dotgit_contents);
+	} else if (err == READ_GITFILE_ERR_NOT_A_FILE) {
 		fn(1, realdotgit.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) {
+		if (inferred_backlink.len) {
 			/*
 			 * Worktree's .git file does not point at a repository
 			 * but we found a .git/worktrees/<id> in this
 			 * repository with the same <id> as recorded in the
 			 * worktree's .git file so make the worktree point at
-			 * the discovered .git/worktrees/<id>. (Note: backlink
-			 * is already NULL, so no need to free it first.)
+			 * the discovered .git/worktrees/<id>.
 			 */
-			backlink = inferred_backlink;
-			inferred_backlink = NULL;
+			strbuf_swap(&backlink, &inferred_backlink);
 		} else {
 			fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
 			goto done;
@@ -743,13 +744,11 @@ 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 && fspathcmp(backlink, inferred_backlink)) {
-		free(backlink);
-		backlink = inferred_backlink;
-		inferred_backlink = NULL;
+	if (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf)) {
+		strbuf_swap(&backlink, &inferred_backlink);
 	}
 
-	strbuf_addf(&gitdir, "%s/gitdir", backlink);
+	strbuf_addf(&gitdir, "%s/gitdir", backlink.buf);
 	if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)
 		repair = _("gitdir unreadable");
 	else {
@@ -763,9 +762,10 @@ void repair_worktree_at_path(const char *path,
 		write_file(gitdir.buf, "%s", realdotgit.buf);
 	}
 done:
-	free(backlink);
-	free(inferred_backlink);
+	free(dotgit_contents);
 	strbuf_release(&olddotgit);
+	strbuf_release(&backlink);
+	strbuf_release(&inferred_backlink);
 	strbuf_release(&gitdir);
 	strbuf_release(&realdotgit);
 	strbuf_release(&dotgit);

-- 
2.46.2

[PATCH v3 2/3] worktree: link worktrees with relative paths

[Caleb White via B4 Relay]

From: Caleb White <cdwhite3@pm.me>

Git currently stores absolute paths to both the main repository and
linked worktrees. However, this causes problems when moving repositories
or working in containerized environments where absolute paths differ
between systems. The worktree links break, and users are required to
manually execute `worktree repair` to repair them, leading to workflow
disruptions. Additionally, mapping repositories inside of containerized
environments renders the repository unusable inside the containers, and
this is not repairable as repairing the worktrees inside the containers
will result in them being broken outside the containers.

To address this, this patch makes Git always write relative paths when
linking worktrees. Relative paths increase the resilience of the
worktree links across various systems and environments, particularly
when the worktrees are self-contained inside the main repository (such
as when using a bare repository with worktrees). This improves
portability, workflow efficiency, and reduces overall breakages.

Although Git now writes relative paths, existing repositories with
absolute paths are still supported. There are no breaking changes
to workflows based on absolute paths, ensuring backward compatibility.

At a low level, the changes involve modifying functions in `worktree.c`
and `builtin/worktree.c` to use `relative_path()` when writing the
worktree’s `.git` file and the main repository’s `gitdir` reference.
Instead of hardcoding absolute paths, Git now computes the relative path
between the worktree and the repository, ensuring that these links are
portable. Locations where these respective file are read have also been
updated to properly handle both absolute and relative paths. Generally,
relative paths are always resolved into absolute paths before any
operations or comparisons are performed.

Additionally, `repair_worktrees_after_gitdir_move()` has been introduced
to address the case where both the `<worktree>/.git` and
`<repo>/worktrees/<id>/gitdir` links are broken after the gitdir is
moved (such as during a re-initialization). This function repairs both
sides of the worktree link using the old gitdir path to reestablish the
correct paths after a move.

The `worktree.path` struct member has also been updated to always store
the absolute path of a worktree. This ensures that worktree consumers
never have to worry about trying to resolve the absolute path themselves.

Signed-off-by: Caleb White <cdwhite3@pm.me>
---
 builtin/worktree.c           |  16 ++--
 setup.c                      |   2 +-
 t/t2408-worktree-relative.sh |  39 ++++++++
 worktree.c                   | 207 ++++++++++++++++++++++++++++++++++---------
 worktree.h                   |  10 +++
 5 files changed, 223 insertions(+), 51 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index fc31d072a620d7b455d7f150bd3a9e773ee9d4ed..dae63dedf4cac2621f51f95a39aa456b33acd894 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,10 @@ 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));
+	write_file(sb_git.buf, "gitdir: %s", relative_path(sb_repo_realpath.buf, sb_path_realpath.buf, &sb_tmp));
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
 	write_file(sb.buf, "../..");
@@ -578,11 +578,13 @@ 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);
-	strbuf_release(&realpath);
 	free_worktree(wt);
 	return ret;
 }
diff --git a/setup.c b/setup.c
index 94e79b2e487f3faa537547e190acf9b7ea0be3b5..7b648de0279116b381eea46800ad130606926103 100644
--- a/setup.c
+++ b/setup.c
@@ -2420,7 +2420,7 @@ static void separate_git_dir(const char *git_dir, const char *git_link)
 
 		if (rename(src, git_dir))
 			die_errno(_("unable to move %s to %s"), src, git_dir);
-		repair_worktrees(NULL, NULL);
+		repair_worktrees_after_gitdir_move(src);
 	}
 
 	write_file(git_link, "gitdir: %s", git_dir);
diff --git a/t/t2408-worktree-relative.sh b/t/t2408-worktree-relative.sh
new file mode 100755
index 0000000000000000000000000000000000000000..a3136db7e28cb20926ff44211e246ce625a6e51a
--- /dev/null
+++ b/t/t2408-worktree-relative.sh
@@ -0,0 +1,39 @@
+#!/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 0cba0d6e6e9ad02ace04a0301104a04a07cbef65..77ff484d3ec48c547ee4e3d958dfa28a52c1eaa7 100644
--- a/worktree.c
+++ b/worktree.c
@@ -110,6 +110,12 @@ struct worktree *get_linked_worktree(const char *id,
 	strbuf_rtrim(&worktree_path);
 	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);
+	}
+
 	CALLOC_ARRAY(worktree, 1);
 	worktree->repo = the_repository;
 	worktree->path = strbuf_detach(&worktree_path, NULL);
@@ -373,18 +379,29 @@ 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;
 
 	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);
+		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));
+
 		free(wt->path);
 		wt->path = strbuf_detach(&path, NULL);
 	}
 	strbuf_release(&path);
+	strbuf_release(&repo);
+	strbuf_release(&file);
+	strbuf_release(&tmp);
 }
 
 int is_worktree_being_rebased(const struct worktree *wt,
@@ -564,38 +581,52 @@ static void repair_gitfile(struct worktree *wt,
 {
 	struct strbuf dotgit = STRBUF_INIT;
 	struct strbuf repo = STRBUF_INIT;
-	char *backlink;
+	struct strbuf backlink = STRBUF_INIT;
+	struct strbuf tmp = STRBUF_INIT;
+	char *dotgit_contents = NULL;
 	const char *repair = NULL;
 	int err;
 
 	/* missing worktree can't be repaired */
 	if (!file_exists(wt->path))
-		return;
+		goto done;
 
 	if (!is_directory(wt->path)) {
 		fn(1, wt->path, _("not a directory"), cb_data);
-		return;
+		goto done;
 	}
 
 	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));
+	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_addf(&backlink, "%s/%s", wt->path, dotgit_contents);
+			strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
+		}
+	}
 
 	if (err == READ_GITFILE_ERR_NOT_A_FILE)
 		fn(1, wt->path, _(".git is not a file"), cb_data);
 	else if (err)
 		repair = _(".git file broken");
-	else if (fspathcmp(backlink, repo.buf))
+	else if (fspathcmp(backlink.buf, repo.buf))
 		repair = _(".git file incorrect");
 
 	if (repair) {
 		fn(0, wt->path, repair, cb_data);
-		write_file(dotgit.buf, "gitdir: %s", repo.buf);
+		write_file(dotgit.buf, "gitdir: %s", relative_path(repo.buf, wt->path, &tmp));
 	}
 
-	free(backlink);
+done:
+	free(dotgit_contents);
 	strbuf_release(&repo);
 	strbuf_release(&dotgit);
+	strbuf_release(&backlink);
+	strbuf_release(&tmp);
 }
 
 static void repair_noop(int iserr UNUSED,
@@ -618,6 +649,59 @@ void repair_worktrees(worktree_repair_fn fn, void *cb_data)
 	free_worktrees(worktrees);
 }
 
+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);
+
+	if (strbuf_read_file(&olddotgit, 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_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));
+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)
+{
+	struct worktree **worktrees = get_worktrees_internal(1);
+	struct worktree **wt = worktrees + 1; /* +1 skips main worktree */
+
+	for (; *wt; wt++)
+		repair_worktree_after_gitdir_move(*wt, old_path);
+	free_worktrees(worktrees);
+}
+
 static int is_main_worktree_path(const char *path)
 {
 	struct strbuf target = STRBUF_INIT;
@@ -684,6 +768,8 @@ void repair_worktree_at_path(const char *path,
 	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;
@@ -701,9 +787,17 @@ void repair_worktree_at_path(const char *path,
 	}
 
 	infer_backlink(realdotgit.buf, &inferred_backlink);
+	strbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);
 	dotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
 	if (dotgit_contents) {
-		strbuf_addstr(&backlink, dotgit_contents);
+		if (is_absolute_path(dotgit_contents)) {
+			strbuf_addstr(&backlink, dotgit_contents);
+		} else {
+			strbuf_addbuf(&backlink, &realdotgit);
+			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);
 		goto done;
@@ -721,7 +815,7 @@ void repair_worktree_at_path(const char *path,
 			fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
 			goto done;
 		}
-	} else if (err) {
+	} else {
 		fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
 		goto done;
 	}
@@ -753,90 +847,117 @@ void repair_worktree_at_path(const char *path,
 		repair = _("gitdir unreadable");
 	else {
 		strbuf_rtrim(&olddotgit);
-		if (fspathcmp(olddotgit.buf, realdotgit.buf))
+		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 (fspathcmp(realolddotgit.buf, realdotgit.buf))
 			repair = _("gitdir incorrect");
 	}
 
 	if (repair) {
 		fn(0, gitdir.buf, repair, cb_data);
-		write_file(gitdir.buf, "%s", realdotgit.buf);
+		write_file(gitdir.buf, "%s", relative_path(realdotgit.buf, backlink.buf, &tmp));
 	}
 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)
 {
 	struct stat st;
-	char *path;
+	struct strbuf dotgit = STRBUF_INIT;
+	struct strbuf gitdir = STRBUF_INIT;
+	struct strbuf repo = STRBUF_INIT;
+	struct strbuf file = STRBUF_INIT;
+	char *path = NULL;
+	int rc = 0;
 	int fd;
 	size_t len;
 	ssize_t read_result;
 
 	*wtpath = NULL;
-	if (!is_directory(git_path("worktrees/%s", id))) {
+	strbuf_realpath(&repo, git_common_path("worktrees/%s", id), 1);
+	strbuf_addf(&gitdir, "%s/gitdir", repo.buf);
+	if (!is_directory(repo.buf)) {
 		strbuf_addstr(reason, _("not a valid directory"));
-		return 1;
+		rc = 1;
+		goto done;
 	}
-	if (file_exists(git_path("worktrees/%s/locked", id)))
-		return 0;
-	if (stat(git_path("worktrees/%s/gitdir", id), &st)) {
+	strbuf_addf(&file, "%s/locked", repo.buf);
+	if (file_exists(file.buf)) {
+		goto done;
+	}
+	if (stat(gitdir.buf, &st)) {
 		strbuf_addstr(reason, _("gitdir file does not exist"));
-		return 1;
+		rc = 1;
+		goto done;
 	}
-	fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY);
+	fd = open(gitdir.buf, O_RDONLY);
 	if (fd < 0) {
 		strbuf_addf(reason, _("unable to read gitdir file (%s)"),
 			    strerror(errno));
-		return 1;
+		rc = 1;
+		goto done;
 	}
 	len = xsize_t(st.st_size);
 	path = xmallocz(len);
 
 	read_result = read_in_full(fd, path, len);
+	close(fd);
 	if (read_result < 0) {
 		strbuf_addf(reason, _("unable to read gitdir file (%s)"),
 			    strerror(errno));
-		close(fd);
-		free(path);
-		return 1;
-	}
-	close(fd);
-
-	if (read_result != len) {
+		rc = 1;
+		goto done;
+	} else if (read_result != len) {
 		strbuf_addf(reason,
 			    _("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"),
 			    (uintmax_t)len, (uintmax_t)read_result);
-		free(path);
-		return 1;
+		rc = 1;
+		goto done;
 	}
 	while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
 		len--;
 	if (!len) {
 		strbuf_addstr(reason, _("invalid gitdir file"));
-		free(path);
-		return 1;
+		rc = 1;
+		goto done;
 	}
 	path[len] = '\0';
-	if (!file_exists(path)) {
-		if (stat(git_path("worktrees/%s/index", id), &st) ||
-		    st.st_mtime <= expire) {
+	if (is_absolute_path(path)) {
+		strbuf_addstr(&dotgit, path);
+	} else {
+		strbuf_addf(&dotgit, "%s/%s", repo.buf, path);
+		strbuf_realpath_forgiving(&dotgit, dotgit.buf, 0);
+	}
+	if (!file_exists(dotgit.buf)) {
+		strbuf_reset(&file);
+		strbuf_addf(&file, "%s/index", repo.buf);
+		if (stat(file.buf, &st) || st.st_mtime <= expire) {
 			strbuf_addstr(reason, _("gitdir file points to non-existent location"));
-			free(path);
-			return 1;
-		} else {
-			*wtpath = path;
-			return 0;
+			rc = 1;
+			goto done;
 		}
 	}
-	*wtpath = path;
-	return 0;
+	*wtpath = strbuf_detach(&dotgit, NULL);
+done:
+	free(path);
+	strbuf_release(&dotgit);
+	strbuf_release(&gitdir);
+	strbuf_release(&repo);
+	strbuf_release(&file);
+	return rc;
 }
 
 static int move_config_setting(const char *key, const char *value,
diff --git a/worktree.h b/worktree.h
index 11279d0c8fe249bb30642563bf221a8de7f3b0a3..e96118621638667d87c5d7e0452ed10bd1ddf606 100644
--- a/worktree.h
+++ b/worktree.h
@@ -131,6 +131,16 @@ typedef void (* worktree_repair_fn)(int iserr, const char *path,
  */
 void repair_worktrees(worktree_repair_fn, void *cb_data);
 
+/*
+ * Repair the linked worktrees after the gitdir has been moved.
+ */
+void repair_worktrees_after_gitdir_move(const char *old_path);
+
+/*
+ * Repair the linked worktree after the gitdir has been moved.
+ */
+void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path);
+
 /*
  * Repair administrative files corresponding to the worktree at the given path.
  * The worktree's .git file pointing at the repository must be intact for the

-- 
2.46.2

[PATCH v3 3/3] worktree: add test for path handling in linked worktrees

[Caleb White via B4 Relay]

From: Caleb White <cdwhite3@pm.me>

A failure scenario reported in an earlier patch series[1] that several
`git worktree` subcommands failed or misbehaved when invoked from within
linked worktrees that used relative paths.

This adds a test that executes a `worktree prune` command inside both an
internally and an externally linked worktree and asserts that the other
worktree was not pruned.

[1]: https://lore.kernel.org/git/CAPig+cQXFy=xPVpoSq6Wq0pxMRCjS=WbkgdO+3LySPX=q0nPCw@mail.gmail.com/

Reported-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Caleb White <cdwhite3@pm.me>
---
 t/t2401-worktree-prune.sh | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/t/t2401-worktree-prune.sh b/t/t2401-worktree-prune.sh
index 71aa9bcd620f8a504fe628a2d7332d8b47fd4701..976d048e3efc74be9cd909ce76d552b3944d2e10 100755
--- a/t/t2401-worktree-prune.sh
+++ b/t/t2401-worktree-prune.sh
@@ -120,4 +120,23 @@ test_expect_success 'prune duplicate (main/linked)' '
 	! test -d .git/worktrees/wt
 '
 
+test_expect_success 'not prune proper worktrees when run inside linked worktree' '
+	test_when_finished rm -rf repo wt_ext &&
+	git init repo &&
+	(
+	    cd repo &&
+	    echo content >file &&
+	    git add file &&
+	    git commit -m msg &&
+	    git worktree add ../wt_ext &&
+	    git worktree add wt_int &&
+	    cd wt_int &&
+	    git worktree prune -v >out &&
+	    test_must_be_empty out &&
+	    cd ../../wt_ext &&
+	    git worktree prune -v >out &&
+	    test_must_be_empty out
+	)
+'
+
 test_done

-- 
2.46.2

Re: [PATCH v3 0/3] Link worktrees with relative paths

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 1206 bytes --]

On Monday, October 7th, 2024 at 22:12, Caleb White via B4 Relay <devnull+cdwhite3.pm.me@kernel.org> wrote:

> Changes in v3:
> - Squashed patch [3/4] into patch [2/4] to streamline changes.
> - Dropped patch [4/4] as it was no longer necessary.
> - Rebased onto 20240923075416.54289-1-ericsunshine@charter.net
> - Updated `infer_backlink()` to return 1 on success for consistency.
> - Swapped the order of `infer_backlink()` arguments for clarity.
> - Clear `inferred` if error occurs in `infer_backlink()`.
> - Renamed `git_contents` to `dotgit_contents` for clearer semantics.
> - Cleaned up `dotgit_contents` logic in `repair_worktree_at_path()`.
> - Replaced multiple `xstrfmt/free` calls with a single `strbuf`.
> - Added a test case covering a failure noted in a separate patch series.
> - Improved commit messages.
> - Link to v2: https://lore.kernel.org/r/20241006060017.171788-1-cdwhite3@pm.me

All, my apologies for the patch corruption in the first two revisions (I suspect
it was due to my SMTP server). I've submitted this v3 series with b4 using the
public submission endpoint and I think it turned out much better. But please let
me know if you have any issues.

Thanks!

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v3 0/3] Link worktrees with relative paths

[Junio C Hamano]

Caleb White via B4 Relay <devnull+cdwhite3.pm.me@kernel.org> writes:

> - Rebased onto 20240923075416.54289-1-ericsunshine@charter.net
> ...
> base-commit: 4ec4435df758668055cc904ef64c275bc8d1089b
> change-id: 20241007-wt_relative_paths-88f9cf5a070c
> prerequisite-message-id: <20240923075416.54289-1-ericsunshine@charter.net>
> prerequisite-patch-id: 78371f4dbb635e6edab8c51ee7857b903e60df8f

It is more common on this list to explain how the base was prepared
in the cover letter, something like:

    The patch was built on the result of merging the
    es/worktree-repair-copied topic, i.e. 992f7a4f (worktree: repair
    copied repository and linked worktrees, 2024-09-23), to v2.47.0.

To help those who are reading from sidelines, the above may need
some deciphering, as the patches as posted do not cleanly apply to
the base-commit 4ec4435df, which is 777489f9 (Git 2.47, 2024-10-06).

    $ git grep '<2024092307...charter.net>' notes/amlog

finds that the message resulted in 992f7a4f (worktree: repair copied
repository and linked worktrees, 2024-09-23).  So the right base to
apply them is obtained by checking out the v2.47.0 and then merging
the es/worktree-repair-copied topic (which is a single commit topic
with the 992f7a4f on it).

    $ git checkout --detach 4ec4435df758^0
    $ git merge --into cw/worktree-relative es/worktree-repair-copied
    $ git am -s ./+cw-worktree-relative-3-patch-series

And of course the series applied cleanly ;-)  I haven't read them
yet, though.

Thanks.

Re: [PATCH v3 0/3] Link worktrees with relative paths

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 1854 bytes --]

On Tuesday, October 8th, 2024 at 14:00, Junio C Hamano <gitster@pobox.com> wrote:

> Caleb White via B4 Relay devnull+cdwhite3.pm.me@kernel.org writes:
> 

> > - Rebased onto 20240923075416.54289-1-ericsunshine@charter.net
> > ...
> > base-commit: 4ec4435df758668055cc904ef64c275bc8d1089b
> > change-id: 20241007-wt_relative_paths-88f9cf5a070c
> > prerequisite-message-id: 20240923075416.54289-1-ericsunshine@charter.net
> > prerequisite-patch-id: 78371f4dbb635e6edab8c51ee7857b903e60df8f
> 

> 

> It is more common on this list to explain how the base was prepared
> in the cover letter, something like:
> 

> The patch was built on the result of merging the
> es/worktree-repair-copied topic, i.e. 992f7a4f (worktree: repair
> copied repository and linked worktrees, 2024-09-23), to v2.47.0.
> 

> To help those who are reading from sidelines, the above may need
> some deciphering, as the patches as posted do not cleanly apply to
> the base-commit 4ec4435df, which is 777489f9 (Git 2.47, 2024-10-06).
> 

> $ git grep '<2024092307...charter.net>' notes/amlog
> 

> 

> finds that the message resulted in 992f7a4f (worktree: repair copied
> repository and linked worktrees, 2024-09-23). So the right base to
> apply them is obtained by checking out the v2.47.0 and then merging
> the es/worktree-repair-copied topic (which is a single commit topic
> with the 992f7a4f on it).
> 

> $ git checkout --detach 4ec4435df758^0
> $ git merge --into cw/worktree-relative es/worktree-repair-copied
> $ git am -s ./+cw-worktree-relative-3-patch-series

Ah, thank you, I'll make preparing the base more clear in the next revision.
This is just how b4 tracks things so it knows how to apply the deps.

> And of course the series applied cleanly ;-) I haven't read them
> yet, though.

That's good to hear :)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v3 2/3] worktree: link worktrees with relative paths

[Junio C Hamano]

Caleb White via B4 Relay <devnull+cdwhite3.pm.me@kernel.org> writes:

> From: Caleb White <cdwhite3@pm.me>
>
> Git currently stores absolute paths to both the main repository and

As always, drop "currently".

The usual way to compose a log message of this project is to

 - Give an observation on how the current system work in the present
   tense (so no need to say "Currently X is Y", just "X is Y"), and
   discuss what you perceive as a problem in it.

 - Propose a solution (optional---often, problem description
   trivially leads to an obvious solution in reader's minds).

 - Give commands to the codebase to "become like so".

in this order.

> linked worktrees. However, this causes problems when moving repositories

The above claim is way too strong.  When relative references are
used, you can move directories without problems only if you move all
the worktrees and the repository in unison without breaking their
relative locations, which is an exception and not the norm.

    The repository knows where its extra worktrees are by their
    absolute paths (and vice versa) to allow discovery of each
    other.  When a directory is moved, "git worktree repair" must be
    used to adjust these references.

    It however becomes possible, in a narrow special case, to do
    without "git worktree repair".  When the repository and the
    worktrees move in parallel without breaking their relative
    location, the repair operation becomes unneeded if we made them
    refer to each other using relative paths (think: tarring up both
    the reposity and the worktrees at the same time, and then
    untarring the result at a different place).

or something, perhaps.

> Although Git now writes relative paths, existing repositories with
> absolute paths are still supported. There are no breaking changes
> to workflows based on absolute paths, ensuring backward compatibility.

Good.  Being able to work with existing repositories is an absolute
requirement.  Are there good test cases?  I would imagine that you
would need to force a worktree and its repository to refer to each
other using absolute paths and then try to access with the updated
code, perhaps moving one of such "old-style" directory and then
using the updated "git worktree repair" and observing the result.
To allow such a test possible, you might even need an option to "git
worktree" to allow it to create these linkages using absolute paths,
not relative (and if you need to keep both possibilities anyway, you
might make the newer "relative" layout an optional feature,
triggered by a command line option given to "git worktree add" and
friends).

Have we considered how badly existing third-party tools, that has
learned to assume that the paths are aboslute, may break with this
change, though?  Or is this "we won't know until we inflict it on
real users and see who screams"?

Thanks.

Re: [PATCH v3 2/3] worktree: link worktrees with relative paths

[Phillip Wood]

Hi Caleb

On 08/10/2024 04:12, Caleb White via B4 Relay wrote:
> From: Caleb White <cdwhite3@pm.me>
> 
> Git currently stores absolute paths to both the main repository and
> linked worktrees. However, this causes problems when moving repositories
> or working in containerized environments where absolute paths differ
> between systems. The worktree links break, and users are required to
> manually execute `worktree repair` to repair them, leading to workflow
> disruptions. Additionally, mapping repositories inside of containerized
> environments renders the repository unusable inside the containers, and
> this is not repairable as repairing the worktrees inside the containers
> will result in them being broken outside the containers.
> 
> To address this, this patch makes Git always write relative paths when
> linking worktrees. Relative paths increase the resilience of the
> worktree links across various systems and environments, particularly
> when the worktrees are self-contained inside the main repository (such
> as when using a bare repository with worktrees). This improves
> portability, workflow efficiency, and reduces overall breakages.

This is quite a sweeping claim, it would be helpful to describe the 
trade off that this patch is making. I've not been following the 
discussion closely but as I understand it while there are cases such as 
sharing a drive between Windows and linux or moving all the worktrees 
including the main one together where using relative paths prevents 
problems there are other cases such as moving a single worktree that are 
fixable by running "git worktree repair" when using absolute paths but 
not with relative paths.

It was suggested in another thread I saw recently that storing both 
would be more resiliant.

Another possibility would be to store the an absolute path in the 
worktree's .git file and relative paths in worktrees/*/gitdir. That 
would enable "git worktree repair" run in a moved worktree to find the 
main worktree if the main worktree has not been moved as it does now. It 
would also allow "git worktree repair" run in the main worktree that has 
been moved to find the linked worktrees that were moved in tandam with it.

Best Wishes

Phillip


> Although Git now writes relative paths, existing repositories with
> absolute paths are still supported. There are no breaking changes
> to workflows based on absolute paths, ensuring backward compatibility.
> 
> At a low level, the changes involve modifying functions in `worktree.c`
> and `builtin/worktree.c` to use `relative_path()` when writing the
> worktree’s `.git` file and the main repository’s `gitdir` reference.
> Instead of hardcoding absolute paths, Git now computes the relative path
> between the worktree and the repository, ensuring that these links are
> portable. Locations where these respective file are read have also been
> updated to properly handle both absolute and relative paths. Generally,
> relative paths are always resolved into absolute paths before any
> operations or comparisons are performed.
> 
> Additionally, `repair_worktrees_after_gitdir_move()` has been introduced
> to address the case where both the `<worktree>/.git` and
> `<repo>/worktrees/<id>/gitdir` links are broken after the gitdir is
> moved (such as during a re-initialization). This function repairs both
> sides of the worktree link using the old gitdir path to reestablish the
> correct paths after a move.
> 
> The `worktree.path` struct member has also been updated to always store
> the absolute path of a worktree. This ensures that worktree consumers
> never have to worry about trying to resolve the absolute path themselves.
> 
> Signed-off-by: Caleb White <cdwhite3@pm.me>
> ---
>   builtin/worktree.c           |  16 ++--
>   setup.c                      |   2 +-
>   t/t2408-worktree-relative.sh |  39 ++++++++
>   worktree.c                   | 207 ++++++++++++++++++++++++++++++++++---------
>   worktree.h                   |  10 +++
>   5 files changed, 223 insertions(+), 51 deletions(-)
> 
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index fc31d072a620d7b455d7f150bd3a9e773ee9d4ed..dae63dedf4cac2621f51f95a39aa456b33acd894 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,10 @@ 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));
> +	write_file(sb_git.buf, "gitdir: %s", relative_path(sb_repo_realpath.buf, sb_path_realpath.buf, &sb_tmp));
>   	strbuf_reset(&sb);
>   	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
>   	write_file(sb.buf, "../..");
> @@ -578,11 +578,13 @@ 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);
> -	strbuf_release(&realpath);
>   	free_worktree(wt);
>   	return ret;
>   }
> diff --git a/setup.c b/setup.c
> index 94e79b2e487f3faa537547e190acf9b7ea0be3b5..7b648de0279116b381eea46800ad130606926103 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -2420,7 +2420,7 @@ static void separate_git_dir(const char *git_dir, const char *git_link)
>   
>   		if (rename(src, git_dir))
>   			die_errno(_("unable to move %s to %s"), src, git_dir);
> -		repair_worktrees(NULL, NULL);
> +		repair_worktrees_after_gitdir_move(src);
>   	}
>   
>   	write_file(git_link, "gitdir: %s", git_dir);
> diff --git a/t/t2408-worktree-relative.sh b/t/t2408-worktree-relative.sh
> new file mode 100755
> index 0000000000000000000000000000000000000000..a3136db7e28cb20926ff44211e246ce625a6e51a
> --- /dev/null
> +++ b/t/t2408-worktree-relative.sh
> @@ -0,0 +1,39 @@
> +#!/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 0cba0d6e6e9ad02ace04a0301104a04a07cbef65..77ff484d3ec48c547ee4e3d958dfa28a52c1eaa7 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -110,6 +110,12 @@ struct worktree *get_linked_worktree(const char *id,
>   	strbuf_rtrim(&worktree_path);
>   	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);
> +	}
> +
>   	CALLOC_ARRAY(worktree, 1);
>   	worktree->repo = the_repository;
>   	worktree->path = strbuf_detach(&worktree_path, NULL);
> @@ -373,18 +379,29 @@ 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;
>   
>   	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);
> +		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));
> +
>   		free(wt->path);
>   		wt->path = strbuf_detach(&path, NULL);
>   	}
>   	strbuf_release(&path);
> +	strbuf_release(&repo);
> +	strbuf_release(&file);
> +	strbuf_release(&tmp);
>   }
>   
>   int is_worktree_being_rebased(const struct worktree *wt,
> @@ -564,38 +581,52 @@ static void repair_gitfile(struct worktree *wt,
>   {
>   	struct strbuf dotgit = STRBUF_INIT;
>   	struct strbuf repo = STRBUF_INIT;
> -	char *backlink;
> +	struct strbuf backlink = STRBUF_INIT;
> +	struct strbuf tmp = STRBUF_INIT;
> +	char *dotgit_contents = NULL;
>   	const char *repair = NULL;
>   	int err;
>   
>   	/* missing worktree can't be repaired */
>   	if (!file_exists(wt->path))
> -		return;
> +		goto done;
>   
>   	if (!is_directory(wt->path)) {
>   		fn(1, wt->path, _("not a directory"), cb_data);
> -		return;
> +		goto done;
>   	}
>   
>   	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));
> +	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_addf(&backlink, "%s/%s", wt->path, dotgit_contents);
> +			strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
> +		}
> +	}
>   
>   	if (err == READ_GITFILE_ERR_NOT_A_FILE)
>   		fn(1, wt->path, _(".git is not a file"), cb_data);
>   	else if (err)
>   		repair = _(".git file broken");
> -	else if (fspathcmp(backlink, repo.buf))
> +	else if (fspathcmp(backlink.buf, repo.buf))
>   		repair = _(".git file incorrect");
>   
>   	if (repair) {
>   		fn(0, wt->path, repair, cb_data);
> -		write_file(dotgit.buf, "gitdir: %s", repo.buf);
> +		write_file(dotgit.buf, "gitdir: %s", relative_path(repo.buf, wt->path, &tmp));
>   	}
>   
> -	free(backlink);
> +done:
> +	free(dotgit_contents);
>   	strbuf_release(&repo);
>   	strbuf_release(&dotgit);
> +	strbuf_release(&backlink);
> +	strbuf_release(&tmp);
>   }
>   
>   static void repair_noop(int iserr UNUSED,
> @@ -618,6 +649,59 @@ void repair_worktrees(worktree_repair_fn fn, void *cb_data)
>   	free_worktrees(worktrees);
>   }
>   
> +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);
> +
> +	if (strbuf_read_file(&olddotgit, 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_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));
> +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)
> +{
> +	struct worktree **worktrees = get_worktrees_internal(1);
> +	struct worktree **wt = worktrees + 1; /* +1 skips main worktree */
> +
> +	for (; *wt; wt++)
> +		repair_worktree_after_gitdir_move(*wt, old_path);
> +	free_worktrees(worktrees);
> +}
> +
>   static int is_main_worktree_path(const char *path)
>   {
>   	struct strbuf target = STRBUF_INIT;
> @@ -684,6 +768,8 @@ void repair_worktree_at_path(const char *path,
>   	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;
> @@ -701,9 +787,17 @@ void repair_worktree_at_path(const char *path,
>   	}
>   
>   	infer_backlink(realdotgit.buf, &inferred_backlink);
> +	strbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);
>   	dotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
>   	if (dotgit_contents) {
> -		strbuf_addstr(&backlink, dotgit_contents);
> +		if (is_absolute_path(dotgit_contents)) {
> +			strbuf_addstr(&backlink, dotgit_contents);
> +		} else {
> +			strbuf_addbuf(&backlink, &realdotgit);
> +			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);
>   		goto done;
> @@ -721,7 +815,7 @@ void repair_worktree_at_path(const char *path,
>   			fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
>   			goto done;
>   		}
> -	} else if (err) {
> +	} else {
>   		fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
>   		goto done;
>   	}
> @@ -753,90 +847,117 @@ void repair_worktree_at_path(const char *path,
>   		repair = _("gitdir unreadable");
>   	else {
>   		strbuf_rtrim(&olddotgit);
> -		if (fspathcmp(olddotgit.buf, realdotgit.buf))
> +		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 (fspathcmp(realolddotgit.buf, realdotgit.buf))
>   			repair = _("gitdir incorrect");
>   	}
>   
>   	if (repair) {
>   		fn(0, gitdir.buf, repair, cb_data);
> -		write_file(gitdir.buf, "%s", realdotgit.buf);
> +		write_file(gitdir.buf, "%s", relative_path(realdotgit.buf, backlink.buf, &tmp));
>   	}
>   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)
>   {
>   	struct stat st;
> -	char *path;
> +	struct strbuf dotgit = STRBUF_INIT;
> +	struct strbuf gitdir = STRBUF_INIT;
> +	struct strbuf repo = STRBUF_INIT;
> +	struct strbuf file = STRBUF_INIT;
> +	char *path = NULL;
> +	int rc = 0;
>   	int fd;
>   	size_t len;
>   	ssize_t read_result;
>   
>   	*wtpath = NULL;
> -	if (!is_directory(git_path("worktrees/%s", id))) {
> +	strbuf_realpath(&repo, git_common_path("worktrees/%s", id), 1);
> +	strbuf_addf(&gitdir, "%s/gitdir", repo.buf);
> +	if (!is_directory(repo.buf)) {
>   		strbuf_addstr(reason, _("not a valid directory"));
> -		return 1;
> +		rc = 1;
> +		goto done;
>   	}
> -	if (file_exists(git_path("worktrees/%s/locked", id)))
> -		return 0;
> -	if (stat(git_path("worktrees/%s/gitdir", id), &st)) {
> +	strbuf_addf(&file, "%s/locked", repo.buf);
> +	if (file_exists(file.buf)) {
> +		goto done;
> +	}
> +	if (stat(gitdir.buf, &st)) {
>   		strbuf_addstr(reason, _("gitdir file does not exist"));
> -		return 1;
> +		rc = 1;
> +		goto done;
>   	}
> -	fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY);
> +	fd = open(gitdir.buf, O_RDONLY);
>   	if (fd < 0) {
>   		strbuf_addf(reason, _("unable to read gitdir file (%s)"),
>   			    strerror(errno));
> -		return 1;
> +		rc = 1;
> +		goto done;
>   	}
>   	len = xsize_t(st.st_size);
>   	path = xmallocz(len);
>   
>   	read_result = read_in_full(fd, path, len);
> +	close(fd);
>   	if (read_result < 0) {
>   		strbuf_addf(reason, _("unable to read gitdir file (%s)"),
>   			    strerror(errno));
> -		close(fd);
> -		free(path);
> -		return 1;
> -	}
> -	close(fd);
> -
> -	if (read_result != len) {
> +		rc = 1;
> +		goto done;
> +	} else if (read_result != len) {
>   		strbuf_addf(reason,
>   			    _("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"),
>   			    (uintmax_t)len, (uintmax_t)read_result);
> -		free(path);
> -		return 1;
> +		rc = 1;
> +		goto done;
>   	}
>   	while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
>   		len--;
>   	if (!len) {
>   		strbuf_addstr(reason, _("invalid gitdir file"));
> -		free(path);
> -		return 1;
> +		rc = 1;
> +		goto done;
>   	}
>   	path[len] = '\0';
> -	if (!file_exists(path)) {
> -		if (stat(git_path("worktrees/%s/index", id), &st) ||
> -		    st.st_mtime <= expire) {
> +	if (is_absolute_path(path)) {
> +		strbuf_addstr(&dotgit, path);
> +	} else {
> +		strbuf_addf(&dotgit, "%s/%s", repo.buf, path);
> +		strbuf_realpath_forgiving(&dotgit, dotgit.buf, 0);
> +	}
> +	if (!file_exists(dotgit.buf)) {
> +		strbuf_reset(&file);
> +		strbuf_addf(&file, "%s/index", repo.buf);
> +		if (stat(file.buf, &st) || st.st_mtime <= expire) {
>   			strbuf_addstr(reason, _("gitdir file points to non-existent location"));
> -			free(path);
> -			return 1;
> -		} else {
> -			*wtpath = path;
> -			return 0;
> +			rc = 1;
> +			goto done;
>   		}
>   	}
> -	*wtpath = path;
> -	return 0;
> +	*wtpath = strbuf_detach(&dotgit, NULL);
> +done:
> +	free(path);
> +	strbuf_release(&dotgit);
> +	strbuf_release(&gitdir);
> +	strbuf_release(&repo);
> +	strbuf_release(&file);
> +	return rc;
>   }
>   
>   static int move_config_setting(const char *key, const char *value,
> diff --git a/worktree.h b/worktree.h
> index 11279d0c8fe249bb30642563bf221a8de7f3b0a3..e96118621638667d87c5d7e0452ed10bd1ddf606 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -131,6 +131,16 @@ typedef void (* worktree_repair_fn)(int iserr, const char *path,
>    */
>   void repair_worktrees(worktree_repair_fn, void *cb_data);
>   
> +/*
> + * Repair the linked worktrees after the gitdir has been moved.
> + */
> +void repair_worktrees_after_gitdir_move(const char *old_path);
> +
> +/*
> + * Repair the linked worktree after the gitdir has been moved.
> + */
> +void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path);
> +
>   /*
>    * Repair administrative files corresponding to the worktree at the given path.
>    * The worktree's .git file pointing at the repository must be intact for the
> 

Re: [PATCH v3 2/3] worktree: link worktrees with relative paths

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 6379 bytes --]

On Tuesday, October 8th, 2024 at 18:09, Junio C Hamano <gitster@pobox.com> wrote:

> Caleb White via B4 Relay devnull+cdwhite3.pm.me@kernel.org writes:
>
> > From: Caleb White cdwhite3@pm.me
> >
> > Git currently stores absolute paths to both the main repository and
>
>
> As always, drop "currently".
>
> The usual way to compose a log message of this project is to
>
> - Give an observation on how the current system work in the present
> tense (so no need to say "Currently X is Y", just "X is Y"), and
> discuss what you perceive as a problem in it.
>
> - Propose a solution (optional---often, problem description
> trivially leads to an obvious solution in reader's minds).
>
> - Give commands to the codebase to "become like so".
>
> in this order.

Thank you for the feedback, I'll reword this. This is my first patch series
to this project (and my first mailing list patch submission, so I've been
learning a bunch), thank you for bearing with me! :)

> > linked worktrees. However, this causes problems when moving repositories
>
> The above claim is way too strong. When relative references are
> used, you can move directories without problems only if you move all
> the worktrees and the repository in unison without breaking their
> relative locations, which is an exception and not the norm.
>
> The repository knows where its extra worktrees are by their
> absolute paths (and vice versa) to allow discovery of each
> other. When a directory is moved, "git worktree repair" must be
> used to adjust these references.
>
> It however becomes possible, in a narrow special case, to do
> without "git worktree repair". When the repository and the
> worktrees move in parallel without breaking their relative
> location, the repair operation becomes unneeded if we made them
> refer to each other using relative paths (think: tarring up both
> the reposity and the worktrees at the same time, and then
> untarring the result at a different place).
>
> or something, perhaps.

I can reduce the strength of the language and add some better examples,
however, I believe this scenario is becoming more common. There's been
a rise in popularity with using worktrees in conjunction with bare
repositories that take the following form:

    /path/to/repo/
        .git/ (bare repo)
        master/
        develop/
        feature1/
        hotfix/

Essentially, you create a directory to act as the the "repository",
then you `clone --bare` the actual repository to `.git`, and you
create worktrees (there's some other steps that need to be performed
such as updating the refs to point to origin, but this is essentially
it) as needed. This pattern has the following advantages:
1. For high volume projects, this allows you to use worktrees to quickly
   switch between contexts. I might be working on a feature at work
   when I quickly need to switch focus to hotfix something that broke
   in production and I don't have to worry about committing / stashing.
2. This avoids polluting the parent directory with the different
   worktrees. I tend to have a directory that houses the various
   repos I work on, and I don't want to have worktrees intermixed
   with other repos.
3. This keeps the `repo` directory clean so you don't have worktrees
   intermixed with your source code on the main working tree.

In this pattern, everything is self contained inside the `repo` dir.
Moving this directory or tarring / untarring it (as you mentioned)
should not cause the links to break and require repair. Granted,
this is not the only use-case which is why both absolute and
relative paths should be supported.

> > Although Git now writes relative paths, existing repositories with
> > absolute paths are still supported. There are no breaking changes
> > to workflows based on absolute paths, ensuring backward compatibility.
>
> Good. Being able to work with existing repositories is an absolute
> requirement. Are there good test cases? I would imagine that you
> would need to force a worktree and its repository to refer to each
> other using absolute paths and then try to access with the updated
> code, perhaps moving one of such "old-style" directory and then
> using the updated "git worktree repair" and observing the result.
> To allow such a test possible, you might even need an option to "git
> worktree" to allow it to create these linkages using absolute paths,
> not relative (and if you need to keep both possibilities anyway, you
> might make the newer "relative" layout an optional feature,
> triggered by a command line option given to "git worktree add" and
> friends).

The tests passed before and after, but there's no tests that explicitly
use absolute paths. I can add a `worktree.useRelativePaths` config/cli
option. I was trying to avoid this, but this may be best to offer users
the most flexibility. With the addition of this config value, I might
split patch [2/3] into two patches: the first will handle reading
potentially relative paths from the files, and the second will add the
config option and handle the writing of relative paths to the files.

What's the best way to parameterize the worktree tests? I would like
to run the same tests for both absolute and relative paths and I'm
not particularly a fan of just copying them all into new *-relative.sh
files.

> Have we considered how badly existing third-party tools, that has
> learned to assume that the paths are aboslute, may break with this
> change, though? Or is this "we won't know until we inflict it on
> real users and see who screams"?

The `worktree` struct that's used internally is populated with the
absolute path, and `worktree list` and the porcelain options return
the absolute paths to the user, so third-party tools should not notice
a difference if they're using the api. The worktree docs even include
the following warning:

    The rule of thumb is do not make any assumption about whether a path belongs
    to $GIT_DIR or $GIT_COMMON_DIR when you need to directly access something
    inside $GIT_DIR. Use git rev-parse --git-path to get the final path.

so third-party tools really shouldn't be reading the files, and I'm not
aware of any that do, but we can monitor for feedback once users start
using the change.


Best,

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v3 2/3] worktree: link worktrees with relative paths

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 2179 bytes --]

On Wednesday, October 9th, 2024 at 05:11, Phillip Wood <phillip.wood123@gmail.com> wrote:

> This is quite a sweeping claim, it would be helpful to describe the
> trade off that this patch is making.

Thank you for your feedback, I'll adjust the language and add some
additional examples.

> but as I understand it while there are cases such as
> sharing a drive between Windows and linux or moving all the worktrees
> including the main one together where using relative paths prevents problems

This is true, there are several cases where relative paths prevent breakages.

> there are other cases such as moving a single worktree that are
> fixable by running "git worktree repair" when using absolute paths but
> not with relative paths.

This is not exactly true, if only the **repository** is moved then yes it
will no longer be able to find it's worktrees without the absolute path,
however, `git worktree repair` can still repair the worktree when provided
the path to the worktree (or the directory containing the worktrees I believe).

> It was suggested in another thread I saw recently that storing both
> would be more resiliant.

I'm not too sure what you mean here, but storing both the absolute
and relative paths in both files seems like an over-complication
(not sure if that's what you were talking about or not). However,
this patch series does support the linking files containing either
absolute or relative paths.

> Another possibility would be to store the an absolute path in the
> worktree's .git file and relative paths in worktrees/*/gitdir. That
> would enable "git worktree repair" run in a moved worktree to find the
> main worktree if the main worktree has not been moved as it does now. It
> would also allow "git worktree repair" run in the main worktree that has
> been moved to find the linked worktrees that were moved in tandam with it.

Storing the absolute path in the `.git` and relative paths in the `gitdir`
will not work---it will still suffer from the same limitations that we are
trying to avoid. And all the test cases related to `worktree repair` are
passing with relative paths.

Best,

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v3 2/3] worktree: link worktrees with relative paths

[Junio C Hamano]

Caleb White <cdwhite3@pm.me> writes:

> What's the best way to parameterize the worktree tests? I would like
> to run the same tests for both absolute and relative paths and I'm
> not particularly a fan of just copying them all into new *-relative.sh
> files.

What I meant by interoperability tests are a lot smaller scale.

A test that creates worktree/repository pair without the option to
use relative, and then tries to use such a worktree/repository pair
with the option would simulate "how well the newer Git handles an
existing repository", and another test that creates with the option
to use relative and uses the worktree/repository without the option
would simulate "how well existing versions of Git works when seeing
a worktree made with the newer git with the relative option".

By "parameterise", if you mean running a set of worktree/repository
tests without the "relative" option enabled, and run the same set of
tests with the option enabled, you could model it after how t8001
and t8002 (or t5560 and t5561) share a lot of same tests that are in
a file that is included by both of them.  In smaller scale, it is
common to have an ad-hoc construct like:

	for conf in relative absolute
	do
		test_expect_success ... 
		test_expect_success ... 
		test_expect_success ... 
	done

that has bunch of test_expect_success, which may change the
behaviour depending on the value of $conf, not &&-chained inside the
for loop.  You can use a nested loop (one for preparing, the other
for testing the use of worktree) if you want to test the full
matrix.

I do not offhand know if such parametralized tests are necessary in
the context of this change, though.

Thanks.

Re: [PATCH v3 2/3] worktree: link worktrees with relative paths

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 2517 bytes --]

> Caleb White cdwhite3@pm.me writes:
> 

> > What's the best way to parameterize the worktree tests? I would like
> > to run the same tests for both absolute and relative paths and I'm
> > not particularly a fan of just copying them all into new *-relative.sh
> > files.
> 

> 

> What I meant by interoperability tests are a lot smaller scale.
> 

> A test that creates worktree/repository pair without the option to
> use relative, and then tries to use such a worktree/repository pair
> with the option would simulate "how well the newer Git handles an
> existing repository", and another test that creates with the option
> to use relative and uses the worktree/repository without the option
> would simulate "how well existing versions of Git works when seeing
> a worktree made with the newer git with the relative option".
> 

> By "parameterise", if you mean running a set of worktree/repository
> tests without the "relative" option enabled, and run the same set of
> tests with the option enabled, you could model it after how t8001
> and t8002 (or t5560 and t5561) share a lot of same tests that are in
> a file that is included by both of them. In smaller scale, it is
> common to have an ad-hoc construct like:
> 

> for conf in relative absolute
> do
> test_expect_success ...
> test_expect_success ...
> test_expect_success ...
> done
> 

> that has bunch of test_expect_success, which may change the
> behaviour depending on the value of $conf, not &&-chained inside the
> for loop. You can use a nested loop (one for preparing, the other
> for testing the use of worktree) if you want to test the full
> matrix.
> 

> I do not offhand know if such parametralized tests are necessary in
> the context of this change, though.
> 

> Thanks.

Ah, I see what you mean, thank you for the details! I'll be sure to
look at the examples and try to determine which would be the best
paths forward.

> existing repository", and another test that creates with the option
> to use relative and uses the worktree/repository without the option
> would simulate "how well existing versions of Git works when seeing
> a worktree made with the newer git with the relative option".

I can already tell you that this particular case is not going to work
because existing versions of git expect the path to be absolute. Most
of the changes in this patch revolve around properly reading/handling
the relative paths, not writing the relative paths.

Best,

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v3 2/3] worktree: link worktrees with relative paths

[Junio C Hamano]

Caleb White <cdwhite3@pm.me> writes:

>> existing repository", and another test that creates with the option
>> to use relative and uses the worktree/repository without the option
>> would simulate "how well existing versions of Git works when seeing
>> a worktree made with the newer git with the relative option".
>
> I can already tell you that this particular case is not going to work
> because existing versions of git expect the path to be absolute. Most
> of the changes in this patch revolve around properly reading/handling
> the relative paths, not writing the relative paths.

If we are talking about making irreversible change to an existing
repository, we may need to grab one extensions bit (cf.
Documentation/technical/repository-version.txt and then refer also
to Documentation/config/extensions.txt [*]) and flip it when we
wrote a relative link to refer to an worktree and repository.

[Footnote]

 * The repository-version document claims that any extensions
   invented must be registered there, but config/extensions.txt that
   came later ignored it and seems to have acquired a few more than
   the "master list".  We should clean up the mess.

Re: [PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf

[shejialuo]

On Mon, Oct 07, 2024 at 10:12:30PM -0500, Caleb White via B4 Relay wrote:
> From: Caleb White <cdwhite3@pm.me>
> 
> This lays the groundwork for the next patch, which needs the backlink
> returned from infer_backlink() as a `strbuf`. It seemed inefficient to
> convert from `strbuf` to `char*` and back to `strbuf` again.
> 

I think here we should first talk about the current behavior of the
`infer_backlink`:

    `infer_backlink` initializes a "strbuf" for the inferred backlink
    and returns the result with type "char *" by detaching the "strbuf"

And then you should tell your intention like the following (The reader
like me does not know what is the intention of the next patch, you should
__explicitly__ mention this).

    Because we decide to link worktrees with relative paths, we need to
    convert the returned inferred backlink "char *" to "strbuf".
    However, it is a bad idea to convert from `strbuf` to `char*` and
    back to `strbuf` again. Instead, we should just let the
    `infer_backlink` to accept the "struct strbuf *" parameter to
    improve efficiency.

> This refactors infer_backlink() to return an integer result and use a
> pre-allocated `strbuf` for the inferred backlink path, replacing the
> previous `char*` return type and improving efficiency.
> 

I think you should also talk about that you make the
`repair_worktree_at_path` function to align with above refactor.

> Signed-off-by: Caleb White <cdwhite3@pm.me>
> ---
>  worktree.c | 48 ++++++++++++++++++++++++------------------------
>  1 file changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/worktree.c b/worktree.c
> index ec95ea2986107b3bc12d38b0825d7c6e87402bc6..0cba0d6e6e9ad02ace04a0301104a04a07cbef65 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -642,10 +642,9 @@ static int is_main_worktree_path(const char *path)
>   * be able to infer the gitdir by manually reading /path/to/worktree/.git,
>   * extracting the <id>, and checking if <repo>/worktrees/<id> exists.
>   */
> -static char *infer_backlink(const char *gitfile)
> +static int infer_backlink(const char *gitfile, struct strbuf *inferred)
>  {
>  	struct strbuf actual = STRBUF_INIT;
> -	struct strbuf inferred = STRBUF_INIT;
>  	const char *id;
>  
>  	if (strbuf_read_file(&actual, gitfile, 0) < 0)
> @@ -658,17 +657,18 @@ static char *infer_backlink(const char *gitfile)
>  	id++; /* advance past '/' to point at <id> */
>  	if (!*id)
>  		goto error;
> -	strbuf_git_common_path(&inferred, the_repository, "worktrees/%s", id);
> -	if (!is_directory(inferred.buf))
> +	strbuf_reset(inferred);

Question here: should we use `strbuf_reset` here? I want to know the
reason why you design this. Is the caller's responsibility to clear the
"inferred" when calling this function?

> +	strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id);
> +	if (!is_directory(inferred->buf))
>  		goto error;
>  
>  	strbuf_release(&actual);
> -	return strbuf_detach(&inferred, NULL);
> +	return 1;
>  
>  error:
>  	strbuf_release(&actual);
> -	strbuf_release(&inferred);
> -	return NULL;
> +	strbuf_reset(inferred); /* clear invalid path */
> +	return 0;
>  }

Design question, when calling `infer_backlink` successfully, we will
return 1, if not, we will return 0. But in the later code, we use the
"inferred.buf.len" to indicate whether we could get the inferred
backlink successfully.

We have two signals to indicate the success. I think it's a bad idea to
use "inferred.buf.len". Let me give you an example here:

    struct strbuf inferred_backlink = STRBUF_INIT;
    inferred_backlink = infer_backlink(realdotgit.buf);

    // What if I wrongly use the following statements?
    strbuf_addstr(&inferred_backlink, "foo");

    if (inferred_backlink.buf.len) {

    }

If you insist using "inferred_backlink.buf.len" as the result, this
function should return `void`. And you should add some comments for this
function.

>  
>  /*
> @@ -680,10 +680,11 @@ void repair_worktree_at_path(const char *path,
>  {
>  	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;
> -	char *backlink = NULL;
> -	char *inferred_backlink = NULL;
> +	char *dotgit_contents = NULL;
>  	const char *repair = NULL;
>  	int err;
>  
> @@ -699,23 +700,23 @@ void repair_worktree_at_path(const char *path,
>  		goto done;
>  	}
>  
> -	inferred_backlink = infer_backlink(realdotgit.buf);
> -	backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
> -	if (err == READ_GITFILE_ERR_NOT_A_FILE) {
> +	infer_backlink(realdotgit.buf, &inferred_backlink);
> +	dotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
> +	if (dotgit_contents) {
> +		strbuf_addstr(&backlink, dotgit_contents);
> +	} else if (err == READ_GITFILE_ERR_NOT_A_FILE) {
>  		fn(1, realdotgit.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) {
> +		if (inferred_backlink.len) {
>  			/*
>  			 * Worktree's .git file does not point at a repository
>  			 * but we found a .git/worktrees/<id> in this
>  			 * repository with the same <id> as recorded in the
>  			 * worktree's .git file so make the worktree point at
> -			 * the discovered .git/worktrees/<id>. (Note: backlink
> -			 * is already NULL, so no need to free it first.)
> +			 * the discovered .git/worktrees/<id>.
>  			 */
> -			backlink = inferred_backlink;
> -			inferred_backlink = NULL;
> +			strbuf_swap(&backlink, &inferred_backlink);
>  		} else {
>  			fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
>  			goto done;
> @@ -743,13 +744,11 @@ 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 && fspathcmp(backlink, inferred_backlink)) {
> -		free(backlink);
> -		backlink = inferred_backlink;
> -		inferred_backlink = NULL;
> +	if (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf)) {
> +		strbuf_swap(&backlink, &inferred_backlink);
>  	}

For single line statement after "if", we should not use `{`.

>  
> -	strbuf_addf(&gitdir, "%s/gitdir", backlink);
> +	strbuf_addf(&gitdir, "%s/gitdir", backlink.buf);
>  	if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0)
>  		repair = _("gitdir unreadable");
>  	else {
> @@ -763,9 +762,10 @@ void repair_worktree_at_path(const char *path,
>  		write_file(gitdir.buf, "%s", realdotgit.buf);
>  	}
>  done:
> -	free(backlink);
> -	free(inferred_backlink);
> +	free(dotgit_contents);
>  	strbuf_release(&olddotgit);
> +	strbuf_release(&backlink);
> +	strbuf_release(&inferred_backlink);
>  	strbuf_release(&gitdir);
>  	strbuf_release(&realdotgit);
>  	strbuf_release(&dotgit);
> 
> -- 
> 2.46.2
> 
> 

Sorry, I could not review other patches today (I need to go sleep).

Thanks,
Jialuo

Re: [PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 5400 bytes --]

On Thursday, October 10th, 2024 at 10:52, shejialuo <shejialuo@gmail.com> wrote:

> On Mon, Oct 07, 2024 at 10:12:30PM -0500, Caleb White via B4 Relay wrote:
> 

> > From: Caleb White cdwhite3@pm.me
> > 

> > This lays the groundwork for the next patch, which needs the backlink
> > returned from infer_backlink() as a `strbuf`. It seemed inefficient to
> > convert from `strbuf` to `char*` and back to `strbuf` again.
> 

> 

> I think here we should first talk about the current behavior of the
> `infer_backlink`:
> 

> `infer_backlink` initializes a "strbuf" for the inferred backlink
> and returns the result with type "char *" by detaching the "strbuf"
> 

> And then you should tell your intention like the following (The reader
> like me does not know what is the intention of the next patch, you should
> explicitly mention this).
> 

> Because we decide to link worktrees with relative paths, we need to
> convert the returned inferred backlink "char *" to "strbuf".
> However, it is a bad idea to convert from `strbuf` to `char*` and
> back to `strbuf` again. Instead, we should just let the
> `infer_backlink` to accept the "struct strbuf *" parameter to
> improve efficiency.
> 

> > This refactors infer_backlink() to return an integer result and use a
> > pre-allocated `strbuf` for the inferred backlink path, replacing the
> > previous `char*` return type and improving efficiency.
> 

> 

> I think you should also talk about that you make the
> `repair_worktree_at_path` function to align with above refactor.

Thank you for the feedback, I'll add these changes.

> > if (strbuf_read_file(&actual, gitfile, 0) < 0)
> > @@ -658,17 +657,18 @@ static char *infer_backlink(const char gitfile)
> > id++; / advance past '/' to point at <id> */
> > if (!*id)
> > goto error;
> > - strbuf_git_common_path(&inferred, the_repository, "worktrees/%s", id);
> > - if (!is_directory(inferred.buf))
> > + strbuf_reset(inferred);
> 

> 

> Question here: should we use `strbuf_reset` here? I want to know the
> reason why you design this. Is the caller's responsibility to clear the
> "inferred" when calling this function?

Yes we should, sure it is the caller's responsibility but this just helps
prevent bugs. There's plenty of functions that reset the strbuf that's
passed to the function before modifying it.


> > + strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id);
> > + if (!is_directory(inferred->buf))
> > goto error;
> > 

> > strbuf_release(&actual);
> > - return strbuf_detach(&inferred, NULL);
> > + return 1;
> > 

> > error:
> > strbuf_release(&actual);
> > - strbuf_release(&inferred);
> > - return NULL;
> > + strbuf_reset(inferred); /* clear invalid path */
> > + return 0;
> > }
> 

> 

> Design question, when calling `infer_backlink` successfully, we will
> return 1, if not, we will return 0. But in the later code, we use the
> "inferred.buf.len" to indicate whether we could get the inferred
> backlink successfully.

Originally, this function call was inside an `if` condition, and it made
sense to keep the boolean return. However, the dependent patch that I later
rebased on refactored this to be a standalone call. I didn't feel like
introducing another variable just to capture the return value when I could
glean the same information from the existing strbuf.

> We have two signals to indicate the success. I think it's a bad idea to
> use "inferred.buf.len". Let me give you an example here:

I don't see a problem with this---the two "signals" are guaranteed to
always be in sync (either the return is 1 and len is > 0, or return is
0 and len is 0). Having the boolean return gives you flexibility in how
you can call the function (if it can be placed inside an if condition).

> struct strbuf inferred_backlink = STRBUF_INIT;
> inferred_backlink = infer_backlink(realdotgit.buf);
> 

> // What if I wrongly use the following statements?
> strbuf_addstr(&inferred_backlink, "foo");
> 

> if (inferred_backlink.buf.len) {
> 

> }

I'm sorry, but this example doesn't make sense. This will fail to compile
for several reasons:
- infer_backlink() requires two args
- you cannot assign an `int` to a `struct strbuf`
- even if inferred_backlink became an int then the strbuf_addstr()
  would fail because you can't pass an `int*` to a `struct strbuf*`
- `inferred_backlink.buf.len` doesn't exist, it's `inferred_backlink.len`
  (probably just a typo)

> If you insist using "inferred_backlink.buf.len" as the result, this
> function should return `void`. And you should add some comments for this
> function.

I can add comments, and I can change the return type to `void` if there's
consensus, but I really don't see any issue with leaving it as-is.

> > - if (inferred_backlink && fspathcmp(backlink, inferred_backlink)) {
> > - free(backlink);
> > - backlink = inferred_backlink;
> > - inferred_backlink = NULL;
> > + if (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf)) {
> > + strbuf_swap(&backlink, &inferred_backlink);
> > }
> 

> 

> For single line statement after "if", we should not use `{`.

The brackets were introduced by the patch that my series depends on.
I can remove them, but perhaps it would be better to address that
on the dependent patch?

Best,

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf

[shejialuo]

On Thu, Oct 10, 2024 at 04:41:03PM +0000, Caleb White wrote:

[snip]

> > > @@ -658,17 +657,18 @@ static char *infer_backlink(const char gitfile)
> > > id++; / advance past '/' to point at <id> */
> > > if (!*id)
> > > goto error;
> > > - strbuf_git_common_path(&inferred, the_repository, "worktrees/%s", id);
> > > - if (!is_directory(inferred.buf))
> > > + strbuf_reset(inferred);
> > 
> 
> > 
> 
> > Question here: should we use `strbuf_reset` here? I want to know the
> > reason why you design this. Is the caller's responsibility to clear the
> > "inferred" when calling this function?
> 
> Yes we should, sure it is the caller's responsibility but this just helps
> prevent bugs. There's plenty of functions that reset the strbuf that's
> passed to the function before modifying it.
> 

Yes, that make senses.

[snip]

> > We have two signals to indicate the success. I think it's a bad idea to
> > use "inferred.buf.len". Let me give you an example here:
> 
> I don't see a problem with this---the two "signals" are guaranteed to
> always be in sync (either the return is 1 and len is > 0, or return is
> 0 and len is 0). Having the boolean return gives you flexibility in how
> you can call the function (if it can be placed inside an if condition).
> 

Yes, there is nothing wrong with this. But we also introduce a burden here,
when we need to change/refactor `infer_backlink`, the developer should
have the knowledge "when the return is 1 and len is >0 or return is 0
and len is 0".

If so, why not just return "infer_backlink.len"?

> > struct strbuf inferred_backlink = STRBUF_INIT;
> > inferred_backlink = infer_backlink(realdotgit.buf);
> > 
> 
> > // What if I wrongly use the following statements?
> > strbuf_addstr(&inferred_backlink, "foo");
> > 
> 
> > if (inferred_backlink.buf.len) {
> > 
> 
> > }
> 
> I'm sorry, but this example doesn't make sense. This will fail to compile
> for several reasons:
> - infer_backlink() requires two args
> - you cannot assign an `int` to a `struct strbuf`
> - even if inferred_backlink became an int then the strbuf_addstr()
>   would fail because you can't pass an `int*` to a `struct strbuf*`
> - `inferred_backlink.buf.len` doesn't exist, it's `inferred_backlink.len`
>   (probably just a typo)
> 

I am sorry for this, I gave a wrong example here, it should be the
following (I copied the wrong line in the previous email):

    struct strbuf inferred_backlink = STRBUF_INIT;
    infer_backlink(realdotgit.buf, &inferred_backlink);

    // What if I wronly use the following statements?
    strbuf_addstr(&inferred_backlink, "foo");

    if (inferred_backlink.len) {
        ...
    }

Actually, I am not against your way. Instead, you should mention why you
choose "inferred_backlink.len" as the signal in the commit message.
That's the main reason why I think we may add some comments here. The
caller may do not know we should use "inferred_backlink.len" to indicate
that we have successfully found the inferred backlink especially there
is already a return code in the function.

> > If you insist using "inferred_backlink.buf.len" as the result, this
> > function should return `void`. And you should add some comments for this
> > function.
> 
> I can add comments, and I can change the return type to `void` if there's
> consensus, but I really don't see any issue with leaving it as-is.
> 

I agree with you that this function is NOT harmful. Actually, I do not
think using "void" as the return type is a good idea. If we decide to
use two signals, let's leave it as-is. Some comments should be enough.

> > > - if (inferred_backlink && fspathcmp(backlink, inferred_backlink)) {
> > > - free(backlink);
> > > - backlink = inferred_backlink;
> > > - inferred_backlink = NULL;
> > > + if (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf)) {
> > > + strbuf_swap(&backlink, &inferred_backlink);
> > > }
> > 
> 
> > 
> 
> > For single line statement after "if", we should not use `{`.
> 
> The brackets were introduced by the patch that my series depends on.
> I can remove them, but perhaps it would be better to address that
> on the dependent patch?
> 

The original patch has three lines. So it should use `{`. After your
change, it only has one line, isn't it?

You could refer to this to see the code style.

  https://github.com/git/git/blob/master/Documentation/CodingGuidelines

> Best,

Thanks,
Jialuo

Re: [PATCH v3 2/3] worktree: link worktrees with relative paths

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 1494 bytes --]

On Wednesday, October 9th, 2024 at 18:37, Junio C Hamano <gitster@pobox.com> wrote:

> Caleb White cdwhite3@pm.me writes:
> 

> > > existing repository", and another test that creates with the option
> > > to use relative and uses the worktree/repository without the option
> > > would simulate "how well existing versions of Git works when seeing
> > > a worktree made with the newer git with the relative option".
> > 

> > I can already tell you that this particular case is not going to work
> > because existing versions of git expect the path to be absolute. Most
> > of the changes in this patch revolve around properly reading/handling
> > the relative paths, not writing the relative paths.
> 

> 

> If we are talking about making irreversible change to an existing
> repository, we may need to grab one extensions bit (cf.
> Documentation/technical/repository-version.txt and then refer also
> to Documentation/config/extensions.txt [*]) and flip it when we
> wrote a relative link to refer to an worktree and repository.

Thanks, I'll take a look at the references.

> [Footnote]
> 

> * The repository-version document claims that any extensions
> invented must be registered there, but config/extensions.txt that
> came later ignored it and seems to have acquired a few more than
> the "master list". We should clean up the mess.

Would you like the contents of config/extensions.txt moved into
the repository-version document and then deleted?

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 3094 bytes --]

On Thursday, October 10th, 2024 at 12:26, shejialuo <shejialuo@gmail.com> wrote:

> > > We have two signals to indicate the success. I think it's a bad idea to
> > > use "inferred.buf.len". Let me give you an example here:
> > 

> > I don't see a problem with this---the two "signals" are guaranteed to
> > always be in sync (either the return is 1 and len is > 0, or return is
> > 0 and len is 0). Having the boolean return gives you flexibility in how
> > you can call the function (if it can be placed inside an if condition).
> 

> Yes, there is nothing wrong with this. But we also introduce a burden here,
> when we need to change/refactor `infer_backlink`, the developer should
> have the knowledge "when the return is 1 and len is >0 or return is 0
> and len is 0".
> 

> If so, why not just return "infer_backlink.len"?

I would say because the purpose of the return is a boolean,
either the call was successful or it wasn't. The developer
knowledge that you speak of should be a given---if the
function returned true then there's obviously a path
in the strbuf.

> I am sorry for this, I gave a wrong example here, it should be the
> following (I copied the wrong line in the previous email):
> 

> struct strbuf inferred_backlink = STRBUF_INIT;
> infer_backlink(realdotgit.buf, &inferred_backlink);
> 

> // What if I wronly use the following statements?
> strbuf_addstr(&inferred_backlink, "foo");

There's nothing wrong with this, it's on the developer
to check the error condition before proceeding to use
the strbuf.

> Actually, I am not against your way. Instead, you should mention why you
> choose "inferred_backlink.len" as the signal in the commit message.
> That's the main reason why I think we may add some comments here. The
> caller may do not know we should use "inferred_backlink.len" to indicate
> that we have successfully found the inferred backlink especially there
> is already a return code in the function.

I think the intent is fairly self-evident and does not warrant a
comment in the commit message? If the strbuf does not have a
length then it should be obvious there is no inferred backlink?

> > > If you insist using "inferred_backlink.buf.len" as the result, this
> > > function should return `void`. And you should add some comments for this
> > > function.
> > 

> > I can add comments, and I can change the return type to `void` if there's
> > consensus, but I really don't see any issue with leaving it as-is.
> 

> I agree with you that this function is NOT harmful. Actually, I do not
> think using "void" as the return type is a good idea. If we decide to
> use two signals, let's leave it as-is. Some comments should be enough.

I've added a comment to the function docs.

> The original patch has three lines. So it should use `{`. After your
> change, it only has one line, isn't it?
> 

> You could refer to this to see the code style.
> 

> https://github.com/git/git/blob/master/Documentation/CodingGuidelines

Ah I'm sorry, you're right---I'll adjust.

Best,

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf

[Junio C Hamano]

Caleb White <cdwhite3@pm.me> writes:

>> Question here: should we use `strbuf_reset` here? I want to know the
>> reason why you design this. Is the caller's responsibility to clear the
>> "inferred" when calling this function?
>
> Yes we should, sure it is the caller's responsibility but this just helps
> prevent bugs. There's plenty of functions that reset the strbuf that's
> passed to the function before modifying it.

If the API says that it _appends_ what it found in the supplied
strbuf, then it is the caller's responsibility to ensure that the
strbuf being passed is empty, if the caller wants the resulting
strbuf to hold only what the function found.

If the API says it _returns_ what it found in the supplied strbuf,
then the function is responsible to discard what the supplied strbuf
originally has.

As an API design, if it is rarely useful for callers to be able to
append, then the latter is simpler to use, but there are many cases
where the "append" semantics is useful (see strbuf.c to find
examples).

In this particular case, it is rarely useful for the caller to get
the result appended to a string it already has before calling this
function, so unconditionally discarding what the caller had, without
telling the caller that it is responsible for passing an empty buffer,
is the right thing, I wuld think.

Thanks.

Re: [PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf

[Junio C Hamano]

Caleb White <cdwhite3@pm.me> writes:

>> If so, why not just return "infer_backlink.len"?
>
> I would say because the purpose of the return is a boolean,
> either the call was successful or it wasn't. The developer
> knowledge that you speak of should be a given---if the
> function returned true then there's obviously a path
> in the strbuf.

That does not say what should be left in the strbuf when the request
failed.  If it is undefined, then using its .len (or .buf) is not
quite right, without relying on too much implementation details of
the called function.

Usually, this project uses 0 to signal a success, while errors are
signalled by returning a negative value (and if there can be
multiple ways to fail, such a design leaves different negative
values as a possibility).

If the result the caller finds useful is always positive value (or
non-negative value), however, it is perfectly fine and often more
convenient if you used the "positive (or non-negative) value gives
the intended result and signals a success, negative (or
non-positive) value signals an error" convention.  The caller can

    if (0 < do_the_thing())
	; /* success */

just fine, instead of the boolean "0 is success, negative values are
various errors",

    if (!do_the_thing())
	; /* success */

when it does not care how/why the thing failed.

Re: [PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf

[Caleb White]

[-- Attachment #1.1: Type: text/plain, Size: 1251 bytes --]

On Thursday, October 10th, 2024 at 15:03, Junio C Hamano <gitster@pobox.com> wrote:

> That does not say what should be left in the strbuf when the request
> failed. If it is undefined, then using its .len (or .buf) is not
> quite right, without relying on too much implementation details of
> the called function.
> 

> Usually, this project uses 0 to signal a success, while errors are
> signalled by returning a negative value (and if there can be
> multiple ways to fail, such a design leaves different negative
> values as a possibility).
> 

> If the result the caller finds useful is always positive value (or
> non-negative value), however, it is perfectly fine and often more
> convenient if you used the "positive (or non-negative) value gives
> the intended result and signals a success, negative (or
> non-positive) value signals an error" convention. The caller can
> 

> if (0 < do_the_thing())
> ; /* success /
> 

> just fine, instead of the boolean "0 is success, negative values are
> various errors",
> 

> if (!do_the_thing())
> ; / success */
> 

> when it does not care how/why the thing failed.

That makes sense. I can update the function to return -1 on error
and the strbuf.len on success.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 509 bytes --]

Re: [PATCH v3 2/3] worktree: link worktrees with relative paths

[Caleb White]

Empty Message

Re: [PATCH v3 2/3] worktree: link worktrees with relative paths

[Caleb White]

On Wed Oct 9, 2024 at 6:37 PM CDT, Junio C Hamano wrote:
> If we are talking about making irreversible change to an existing
> repository, we may need to grab one extensions bit (cf.
> Documentation/technical/repository-version.txt and then refer also
> to Documentation/config/extensions.txt [*]) and flip it when we
> wrote a relative link to refer to an worktree and repository.

This has been implemented.

> [Footnote]
>
>  * The repository-version document claims that any extensions
>    invented must be registered there, but config/extensions.txt that
>    came later ignored it and seems to have acquired a few more than
>    the "master list".  We should clean up the mess.

I cleaned up the mess in a separate patch[1].


[1]: https://lore.kernel.org/git/20241021-cleanup-extension-docs-v1-1-ab02cece3132@pm.me

Best,