From: Phillip Wood Date: Tue, 29 Oct 2024 14:52:28 GMT Subject: Re: [PATCH v2 1/5] worktree: add CLI/config options for relative path linking Message-ID: <0bea06b8-11d0-497f-88e1-153cb61eb06d@gmail.com> In-Reply-To: <20241028-wt_relative_options-v2-1-33a5021bd7bb@pm.me> Hi Caleb Thanks for working on this. On 28/10/2024 19:09, Caleb White wrote: > This patch introduces the `--[no-]relative-paths` CLI option for "This patch" is a bit redundant, I'd say "Introduce a `--[no-]-relative-paths ..` > `git worktree {add, move, repair}` commands, as well as the > `worktree.useRelativePaths` configuration setting. When enabled, > these options allow worktrees to be linked using relative paths, > enhancing portability across environments where absolute paths > may differ (e.g., containerized setups, shared network drives). > Git still creates absolute paths by default, but these options allow > users to opt-in to relative paths if desired. This sounds good, I'm not sure the patch actually implements anything other than the option parsing though. I think it would make sense to add these options at the end of the patch series once the implementation has been changed to support them. I'd start with patches 4 and 5 to add the new extension setting first, then refactor worktree.c to handle creating worktrees with relative or absolute paths and set the extension if appropriate, then add the --relative-path option to "git worktree" > Using the `--relative-paths` option with `worktree {move, repair}` > will convert absolute paths to relative ones, while `--no-relative-paths` > does the reverse. For cases where users want consistency in path > handling, the config option `worktree.useRelativePaths` provides > a persistent setting. > > In response to reviewer feedback from the initial patch series[1], > this revision includes slight refactoring for improved > maintainability and clarity in the code base. Please don't mix cleanups with other code changes as it makes it hard to check that the cleanups don't change the behavior. > [1]: https://lore.kernel.org/git/20241007-wt_relative_paths-v3-0-622cf18c45eb@pm.me > > Suggested-by: Junio C Hamano > Signed-off-by: Caleb White > --- > Documentation/config/worktree.txt | 5 +++++ > Documentation/git-worktree.txt | 12 ++++++++++++ > builtin/worktree.c | 9 +++++++++ > t/t2408-worktree-relative.sh | 39 --------------------------------------- > worktree.c | 17 ++++++++++------- > worktree.h | 2 ++ > 6 files changed, 38 insertions(+), 46 deletions(-) > > diff --git a/Documentation/config/worktree.txt b/Documentation/config/worktree.txt > index 048e349482df6c892055720eb53cdcd6c327b6ed..44b783c2774dc5ff65e3fa232b0c25cd5254876b 100644 > --- a/Documentation/config/worktree.txt > +++ b/Documentation/config/worktree.txt > @@ -7,3 +7,8 @@ worktree.guessRemote:: > such a branch exists, it is checked out and set as "upstream" > for the new branch. If no such match can be found, it falls > back to creating a new branch from the current HEAD. > +worktree.useRelativePaths:: > + If set to `true`, worktrees will be linked to the repository using > + relative paths rather than using absolute paths. This is particularly > + useful for setups where the repository and worktrees may be moved between > + different locations or environments. I think it would be helpful to spell out the implications of this to the user - namely that you cannot use older versions of git on a repository with worktrees using relative paths and it may break third-party software as well. > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > index 70437c815f13852bd2eb862176b8b933e6de0acf..975dc3c46d480480457ec4857988a6b8bc67b647 100644 > --- a/Documentation/git-worktree.txt > +++ b/Documentation/git-worktree.txt > @@ -216,6 +216,18 @@ To remove a locked worktree, specify `--force` twice. > This can also be set up as the default behaviour by using the > `worktree.guessRemote` config option. > > +--[no-]relative-paths:: > + Worktrees will be linked to the repository using relative paths > + rather than using absolute paths. This is particularly useful for setups > + where the repository and worktrees may be moved between different > + locations or environments. Again we should spell out the implications of using relative paths. > +With `repair`, the linking files will be updated if there's an absolute/relative > +mismatch, even if the links are correct. > ++ > +This can also be set up as the default behaviour by using the > +`worktree.useRelativePaths` config option. > + > --[no-]track:: > When creating a new branch, if `` is a branch, > mark it as "upstream" from the new branch. This is the > diff --git a/builtin/worktree.c b/builtin/worktree.c > index dae63dedf4cac2621f51f95a39aa456b33acd894..c1130be5890c905c0b648782a834eb8dfcd79ba5 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -134,6 +134,9 @@ static int git_worktree_config(const char *var, const char *value, > if (!strcmp(var, "worktree.guessremote")) { > guess_remote = git_config_bool(var, value); > return 0; > + } else if (!strcmp(var, "worktree.userelativepaths")) { > + use_relative_paths = git_config_bool(var, value); As we're trying to remove global variables from libgit.a as part of the libification effort I'd be much happier if "use_relative_paths" was declared as a "static int" in this file and then passed down to the functions that need it rather than declaring it as a global in "worktree.c". > diff --git a/t/t2408-worktree-relative.sh b/t/t2408-worktree-relative.sh > deleted file mode 100755 There's no explanation for this change in the commit message Best Wishes Phillip > index a3136db7e28cb20926ff44211e246ce625a6e51a..0000000000000000000000000000000000000000 > --- a/t/t2408-worktree-relative.sh > +++ /dev/null > @@ -1,39 +0,0 @@ > -#!/bin/sh > - > -test_description='test worktrees linked with relative paths' > - > -TEST_PASSES_SANITIZE_LEAK=true > -. ./test-lib.sh > - > -test_expect_success 'links worktrees with relative paths' ' > - test_when_finished rm -rf repo && > - git init repo && > - ( > - cd repo && > - test_commit initial && > - git worktree add wt1 && > - echo "../../../wt1/.git" >expected_gitdir && > - cat .git/worktrees/wt1/gitdir >actual_gitdir && > - echo "gitdir: ../.git/worktrees/wt1" >expected_git && > - cat wt1/.git >actual_git && > - test_cmp expected_gitdir actual_gitdir && > - test_cmp expected_git actual_git > - ) > -' > - > -test_expect_success 'move repo without breaking relative internal links' ' > - test_when_finished rm -rf repo moved && > - git init repo && > - ( > - cd repo && > - test_commit initial && > - git worktree add wt1 && > - cd .. && > - mv repo moved && > - cd moved/wt1 && > - git status >out 2>err && > - test_must_be_empty err > - ) > -' > - > -test_done > diff --git a/worktree.c b/worktree.c > index 77ff484d3ec48c547ee4e3d958dfa28a52c1eaa7..de5c5e53a5f2a758ddf470b5d6a9ad6c66247181 100644 > --- a/worktree.c > +++ b/worktree.c > @@ -14,6 +14,8 @@ > #include "wt-status.h" > #include "config.h" > > +int use_relative_paths = 0; > + > void free_worktree(struct worktree *worktree) > { > if (!worktree) > @@ -111,9 +113,9 @@ struct worktree *get_linked_worktree(const char *id, > strbuf_strip_suffix(&worktree_path, "/.git"); > > if (!is_absolute_path(worktree_path.buf)) { > - strbuf_strip_suffix(&path, "gitdir"); > - strbuf_addbuf(&path, &worktree_path); > - strbuf_realpath_forgiving(&worktree_path, path.buf, 0); > + strbuf_strip_suffix(&path, "gitdir"); > + strbuf_addbuf(&path, &worktree_path); > + strbuf_realpath_forgiving(&worktree_path, path.buf, 0); > } > > CALLOC_ARRAY(worktree, 1); > @@ -725,12 +727,15 @@ static int is_main_worktree_path(const char *path) > * won't know which /worktrees//gitdir to repair. However, we may > * be able to infer the gitdir by manually reading /path/to/worktree/.git, > * extracting the , and checking if /worktrees/ exists. > + * > + * Returns -1 on failure and strbuf.len on success. > */ > static int infer_backlink(const char *gitfile, struct strbuf *inferred) > { > struct strbuf actual = STRBUF_INIT; > const char *id; > > + strbuf_reset(inferred); > if (strbuf_read_file(&actual, gitfile, 0) < 0) > goto error; > if (!starts_with(actual.buf, "gitdir:")) > @@ -741,18 +746,16 @@ static int infer_backlink(const char *gitfile, struct strbuf *inferred) > id++; /* advance past '/' to point at */ > if (!*id) > goto error; > - strbuf_reset(inferred); > strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id); > if (!is_directory(inferred->buf)) > goto error; > > strbuf_release(&actual); > - return 1; > - > + return inferred->len; > error: > strbuf_release(&actual); > strbuf_reset(inferred); /* clear invalid path */ > - return 0; > + return -1; > } > > /* > diff --git a/worktree.h b/worktree.h > index e96118621638667d87c5d7e0452ed10bd1ddf606..37e65d508ed23d3e7a29850bb938285072a3aaa6 100644 > --- a/worktree.h > +++ b/worktree.h > @@ -5,6 +5,8 @@ > > struct strbuf; > > +extern int use_relative_paths; > + > struct worktree { > /* The repository this worktree belongs to. */ > struct repository *repo; >