| Message ID | 20241028-wt_relative_options-v2-1-33a5021bd7bb@pm.me (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Allow relative worktree linking to be configured by the user | expand |
Hi Caleb Thanks for working on this. On 28/10/2024 19:09, Caleb White wrote: > This patch introduces the `--[no-]relative-paths` CLI option for "This patch" is a bit redundant, I'd say "Introduce a `--[no-]-relative-paths ..` > `git worktree {add, move, repair}` commands, as well as the > `worktree.useRelativePaths` configuration setting. When enabled, > these options allow worktrees to be linked using relative paths, > enhancing portability across environments where absolute paths > may differ (e.g., containerized setups, shared network drives). > Git still creates absolute paths by default, but these options allow > users to opt-in to relative paths if desired. This sounds good, I'm not sure the patch actually implements anything other than the option parsing though. I think it would make sense to add these options at the end of the patch series once the implementation has been changed to support them. I'd start with patches 4 and 5 to add the new extension setting first, then refactor worktree.c to handle creating worktrees with relative or absolute paths and set the extension if appropriate, then add the --relative-path option to "git worktree" > Using the `--relative-paths` option with `worktree {move, repair}` > will convert absolute paths to relative ones, while `--no-relative-paths` > does the reverse. For cases where users want consistency in path > handling, the config option `worktree.useRelativePaths` provides > a persistent setting. > > In response to reviewer feedback from the initial patch series[1], > this revision includes slight refactoring for improved > maintainability and clarity in the code base. Please don't mix cleanups with other code changes as it makes it hard to check that the cleanups don't change the behavior. > [1]: https://lore.kernel.org/git/20241007-wt_relative_paths-v3-0-622cf18c45eb@pm.me > > Suggested-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: Caleb White <cdwhite3@pm.me> > --- > Documentation/config/worktree.txt | 5 +++++ > Documentation/git-worktree.txt | 12 ++++++++++++ > builtin/worktree.c | 9 +++++++++ > t/t2408-worktree-relative.sh | 39 --------------------------------------- > worktree.c | 17 ++++++++++------- > worktree.h | 2 ++ > 6 files changed, 38 insertions(+), 46 deletions(-) > > diff --git a/Documentation/config/worktree.txt b/Documentation/config/worktree.txt > index 048e349482df6c892055720eb53cdcd6c327b6ed..44b783c2774dc5ff65e3fa232b0c25cd5254876b 100644 > --- a/Documentation/config/worktree.txt > +++ b/Documentation/config/worktree.txt > @@ -7,3 +7,8 @@ worktree.guessRemote:: > such a branch exists, it is checked out and set as "upstream" > for the new branch. If no such match can be found, it falls > back to creating a new branch from the current HEAD. > +worktree.useRelativePaths:: > + If set to `true`, worktrees will be linked to the repository using > + relative paths rather than using absolute paths. This is particularly > + useful for setups where the repository and worktrees may be moved between > + different locations or environments. I think it would be helpful to spell out the implications of this to the user - namely that you cannot use older versions of git on a repository with worktrees using relative paths and it may break third-party software as well. > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > index 70437c815f13852bd2eb862176b8b933e6de0acf..975dc3c46d480480457ec4857988a6b8bc67b647 100644 > --- a/Documentation/git-worktree.txt > +++ b/Documentation/git-worktree.txt > @@ -216,6 +216,18 @@ To remove a locked worktree, specify `--force` twice. > This can also be set up as the default behaviour by using the > `worktree.guessRemote` config option. > > +--[no-]relative-paths:: > + Worktrees will be linked to the repository using relative paths > + rather than using absolute paths. This is particularly useful for setups > + where the repository and worktrees may be moved between different > + locations or environments. Again we should spell out the implications of using relative paths. > +With `repair`, the linking files will be updated if there's an absolute/relative > +mismatch, even if the links are correct. > ++ > +This can also be set up as the default behaviour by using the > +`worktree.useRelativePaths` config option. > + > --[no-]track:: > When creating a new branch, if `<commit-ish>` is a branch, > mark it as "upstream" from the new branch. This is the > diff --git a/builtin/worktree.c b/builtin/worktree.c > index dae63dedf4cac2621f51f95a39aa456b33acd894..c1130be5890c905c0b648782a834eb8dfcd79ba5 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -134,6 +134,9 @@ static int git_worktree_config(const char *var, const char *value, > if (!strcmp(var, "worktree.guessremote")) { > guess_remote = git_config_bool(var, value); > return 0; > + } else if (!strcmp(var, "worktree.userelativepaths")) { > + use_relative_paths = git_config_bool(var, value); As we're trying to remove global variables from libgit.a as part of the libification effort I'd be much happier if "use_relative_paths" was declared as a "static int" in this file and then passed down to the functions that need it rather than declaring it as a global in "worktree.c". > diff --git a/t/t2408-worktree-relative.sh b/t/t2408-worktree-relative.sh > deleted file mode 100755 There's no explanation for this change in the commit message Best Wishes Phillip > index a3136db7e28cb20926ff44211e246ce625a6e51a..0000000000000000000000000000000000000000 > --- a/t/t2408-worktree-relative.sh > +++ /dev/null > @@ -1,39 +0,0 @@ > -#!/bin/sh > - > -test_description='test worktrees linked with relative paths' > - > -TEST_PASSES_SANITIZE_LEAK=true > -. ./test-lib.sh > - > -test_expect_success 'links worktrees with relative paths' ' > - test_when_finished rm -rf repo && > - git init repo && > - ( > - cd repo && > - test_commit initial && > - git worktree add wt1 && > - echo "../../../wt1/.git" >expected_gitdir && > - cat .git/worktrees/wt1/gitdir >actual_gitdir && > - echo "gitdir: ../.git/worktrees/wt1" >expected_git && > - cat wt1/.git >actual_git && > - test_cmp expected_gitdir actual_gitdir && > - test_cmp expected_git actual_git > - ) > -' > - > -test_expect_success 'move repo without breaking relative internal links' ' > - test_when_finished rm -rf repo moved && > - git init repo && > - ( > - cd repo && > - test_commit initial && > - git worktree add wt1 && > - cd .. && > - mv repo moved && > - cd moved/wt1 && > - git status >out 2>err && > - test_must_be_empty err > - ) > -' > - > -test_done > diff --git a/worktree.c b/worktree.c > index 77ff484d3ec48c547ee4e3d958dfa28a52c1eaa7..de5c5e53a5f2a758ddf470b5d6a9ad6c66247181 100644 > --- a/worktree.c > +++ b/worktree.c > @@ -14,6 +14,8 @@ > #include "wt-status.h" > #include "config.h" > > +int use_relative_paths = 0; > + > void free_worktree(struct worktree *worktree) > { > if (!worktree) > @@ -111,9 +113,9 @@ struct worktree *get_linked_worktree(const char *id, > strbuf_strip_suffix(&worktree_path, "/.git"); > > if (!is_absolute_path(worktree_path.buf)) { > - strbuf_strip_suffix(&path, "gitdir"); > - strbuf_addbuf(&path, &worktree_path); > - strbuf_realpath_forgiving(&worktree_path, path.buf, 0); > + strbuf_strip_suffix(&path, "gitdir"); > + strbuf_addbuf(&path, &worktree_path); > + strbuf_realpath_forgiving(&worktree_path, path.buf, 0); > } > > CALLOC_ARRAY(worktree, 1); > @@ -725,12 +727,15 @@ static int is_main_worktree_path(const char *path) > * won't know which <repo>/worktrees/<id>/gitdir to repair. However, we may > * be able to infer the gitdir by manually reading /path/to/worktree/.git, > * extracting the <id>, and checking if <repo>/worktrees/<id> exists. > + * > + * Returns -1 on failure and strbuf.len on success. > */ > static int infer_backlink(const char *gitfile, struct strbuf *inferred) > { > struct strbuf actual = STRBUF_INIT; > const char *id; > > + strbuf_reset(inferred); > if (strbuf_read_file(&actual, gitfile, 0) < 0) > goto error; > if (!starts_with(actual.buf, "gitdir:")) > @@ -741,18 +746,16 @@ static int infer_backlink(const char *gitfile, struct strbuf *inferred) > id++; /* advance past '/' to point at <id> */ > if (!*id) > goto error; > - strbuf_reset(inferred); > strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id); > if (!is_directory(inferred->buf)) > goto error; > > strbuf_release(&actual); > - return 1; > - > + return inferred->len; > error: > strbuf_release(&actual); > strbuf_reset(inferred); /* clear invalid path */ > - return 0; > + return -1; > } > > /* > diff --git a/worktree.h b/worktree.h > index e96118621638667d87c5d7e0452ed10bd1ddf606..37e65d508ed23d3e7a29850bb938285072a3aaa6 100644 > --- a/worktree.h > +++ b/worktree.h > @@ -5,6 +5,8 @@ > > struct strbuf; > > +extern int use_relative_paths; > + > struct worktree { > /* The repository this worktree belongs to. */ > struct repository *repo; >
On Mon, Oct 28, 2024 at 07:09:37PM +0000, Caleb White wrote: > This patch introduces the `--[no-]relative-paths` CLI option for > `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. > > 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. This is great. This addresses the main concerns that you and I discussed in the earlier round of this series, which was making sure that the new behavior be opt-in, as it breaks backwards compatibility and thus requires a new extension to quarantine older Git versions from touching repositories that list their worktrees out with relative paths. This approach makes the most sense to me because it doesn't impose such a breakage between Git versions when the user doesn't explicitly opt-in to the new behavior, which is the right approach to take here IMO. > 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. Great :-). > 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. I would have thought there would be a blank line in between this and the section on worktree.guessRemote. ASCIIDoc doesn't require it because this is a labeled list, but it does improve the readability of the raw ASCIIDoc itself. So not a big deal, but if you end up sending out another version of this series it would be nice to include. > +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. This is a good start, but I have a few suggestions on top that I'm curious of your thoughts on. First: what is the default? Users should have some insight into what the default is. Likewise, they should know that that the default behavior does not introduce the repository extension, but that setting this configuration to 'true' does. Maybe something like the following on top? --- 8< --- diff --git a/Documentation/config/worktree.txt b/Documentation/config/worktree.txt index 44b783c277..666cb3c190 100644 --- a/Documentation/config/worktree.txt +++ b/Documentation/config/worktree.txt @@ -7,8 +7,13 @@ 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. + Link worktrees using relative paths (when "true") or absolute + paths (when "false"). This is particularly useful for setups + where the repository and worktrees may be moved between + different locations or environments. Defaults to "false". ++ +Note that setting `worktree.useRelativePaths` to "true" implies +enabling the "relativeWorktrees" repository extension, thus making it +incompatible with older versions of Git. --- >8 --- > 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. This paragraph is redundant with what you wrote in git-config(1). I think all we want to say is that it overrides the setting of that configuration variable, and refer users there with linkgit. > ++ > +With `repair`, the linking files will be updated if there's an absolute/relative > +mismatch, even if the links are correct. This is worth keeping. > +This can also be set up as the default behaviour by using the > +`worktree.useRelativePaths` config option. > + This should get folded into my suggestion above. > diff --git a/t/t2408-worktree-relative.sh b/t/t2408-worktree-relative.sh > deleted file mode 100755 > index a3136db7e28cb20926ff44211e246ce625a6e51a..0000000000000000000000000000000000000000 > --- a/t/t2408-worktree-relative.sh > +++ /dev/null > @@ -1,39 +0,0 @@ Was removing t2408 intentional here? I don't see the tests being re-added elsewhere in this patch (though they may be introduced elsewhere later in the series, I haven't read that far yet). Either way, it may be worth mentioning in the commit message to avoid confusing readers. > 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; I wondered whether 'use_relative_paths' should be static, or if we need to extern it in from somewhere else in the tree. But we do, from worktree.[ch], which seems reasonable. It would be nice if there was some way to thread that into the worktree.h API, but I think that this is a reasonable measure to take for now. > + > 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); Whitespace change? > } > > CALLOC_ARRAY(worktree, 1); > @@ -725,12 +727,15 @@ static int is_main_worktree_path(const char *path) > * won't know which <repo>/worktrees/<id>/gitdir to repair. However, we may > * be able to infer the gitdir by manually reading /path/to/worktree/.git, > * extracting the <id>, and checking if <repo>/worktrees/<id> exists. > + * > + * Returns -1 on failure and strbuf.len on success. > */ > static int infer_backlink(const char *gitfile, struct strbuf *inferred) Should this return an ssize_t instead, then? I don't think we're going to have worktree paths that are actually larger than INT_MAX, but it seems hygienic and good to prevent any accidental overflow issues. Thanks, Taylor
On Tue Oct 29, 2024 at 1:42 PM CDT, Taylor Blau wrote: > On Mon, Oct 28, 2024 at 07:09:37PM +0000, Caleb White wrote: >> 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. > > I would have thought there would be a blank line in between this and the > section on worktree.guessRemote. ASCIIDoc doesn't require it because > this is a labeled list, but it does improve the readability of the raw > ASCIIDoc itself. > > So not a big deal, but if you end up sending out another version of this > series it would be nice to include. I'll add the blank line in the next version of the patch. >> +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. > > This is a good start, but I have a few suggestions on top that I'm > curious of your thoughts on. First: what is the default? Users > should have some insight into what the default is. Likewise, they should > know that that the default behavior does not introduce the repository > extension, but that setting this configuration to 'true' does. > > Maybe something like the following on top? > > --- 8< --- > diff --git a/Documentation/config/worktree.txt b/Documentation/config/worktree.txt > index 44b783c277..666cb3c190 100644 > --- a/Documentation/config/worktree.txt > +++ b/Documentation/config/worktree.txt > @@ -7,8 +7,13 @@ 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. > + Link worktrees using relative paths (when "true") or absolute > + paths (when "false"). This is particularly useful for setups > + where the repository and worktrees may be moved between > + different locations or environments. Defaults to "false". > ++ > +Note that setting `worktree.useRelativePaths` to "true" implies > +enabling the "relativeWorktrees" repository extension, thus making it > +incompatible with older versions of Git. Sounds good to me. I'll update. >> 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. > > This paragraph is redundant with what you wrote in git-config(1). I > think all we want to say is that it overrides the setting of that > configuration variable, and refer users there with linkgit. I agree. I'll update this paragraph to refer to the config documentation. >> ++ >> +With `repair`, the linking files will be updated if there's an absolute/relative >> +mismatch, even if the links are correct. > > This is worth keeping. Keeping this as is. >> +This can also be set up as the default behaviour by using the >> +`worktree.useRelativePaths` config option. >> + > > This should get folded into my suggestion above. Done. >> diff --git a/t/t2408-worktree-relative.sh b/t/t2408-worktree-relative.sh >> deleted file mode 100755 >> index a3136db7e28cb20926ff44211e246ce625a6e51a..0000000000000000000000000000000000000000 >> --- a/t/t2408-worktree-relative.sh >> +++ /dev/null >> @@ -1,39 +0,0 @@ > > Was removing t2408 intentional here? I don't see the tests being > re-added elsewhere in this patch (though they may be introduced > elsewhere later in the series, I haven't read that far yet). Either way, > it may be worth mentioning in the commit message to avoid confusing > readers. Yes, this was intentional. This was added in the original round when the default was changed to use relative paths. I added more comprehensive tests in the various worktree files to cover the new functionality. I will make sure to mention this in the commit message. >> 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; > > I wondered whether 'use_relative_paths' should be static, or if we need to extern it in > from somewhere else in the tree. But we do, from worktree.[ch], which > seems reasonable. It would be nice if there was some way to thread that > into the worktree.h API, but I think that this is a reasonable measure > to take for now. I can add a getter/setter to the worktree API so we're not using a global variable. >> + >> 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); > > Whitespace change? Yes, this was added in the original round, I didn't notice that it used 4 spaces instead of a tab. I had fixed this in v4 of the original round, but this was merged in v3 so I just went ahead and fixed it here. >> CALLOC_ARRAY(worktree, 1); >> @@ -725,12 +727,15 @@ static int is_main_worktree_path(const char *path) >> * won't know which <repo>/worktrees/<id>/gitdir to repair. However, we may >> * be able to infer the gitdir by manually reading /path/to/worktree/.git, >> * extracting the <id>, and checking if <repo>/worktrees/<id> exists. >> + * >> + * Returns -1 on failure and strbuf.len on success. >> */ >> static int infer_backlink(const char *gitfile, struct strbuf *inferred) > > Should this return an ssize_t instead, then? I don't think we're going > to have worktree paths that are actually larger than INT_MAX, but it > seems hygienic and good to prevent any accidental overflow issues. I thought about this, but you'll run into OS limits long before you hit INT_MAX. However, I can make this change to be hygienic. Best, Caleb
On Tue Oct 29, 2024 at 9:52 AM CDT, Phillip Wood 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 ..` I removed "patch". >> `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" That makes sense, I can reorder the patches as you described. >> 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. Fair enough, I'll separate the cleanups into their own patch. >> +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. I've added a note that you cannot use older versions of Git with worktrees when using relative paths. >> +--[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. This has been revised to simply point to the config docs. >> +With `repair`, the linking files will be updated if there's an absolute/relative >> +mismatch, even if the links are correct. >> ++ >> +This can also be set up as the default behaviour by using the >> +`worktree.useRelativePaths` config option. >> + >> --[no-]track:: >> When creating a new branch, if `<commit-ish>` is a branch, >> mark it as "upstream" from the new branch. This is the >> diff --git a/builtin/worktree.c b/builtin/worktree.c >> index dae63dedf4cac2621f51f95a39aa456b33acd894..c1130be5890c905c0b648782a834eb8dfcd79ba5 100644 >> --- a/builtin/worktree.c >> +++ b/builtin/worktree.c >> @@ -134,6 +134,9 @@ static int git_worktree_config(const char *var, const char *value, >> if (!strcmp(var, "worktree.guessremote")) { >> guess_remote = git_config_bool(var, value); >> return 0; >> + } else if (!strcmp(var, "worktree.userelativepaths")) { >> + use_relative_paths = git_config_bool(var, value); > > As we're trying to remove global variables from libgit.a as part of the > libification effort I'd be much happier if "use_relative_paths" was > declared as a "static int" in this file and then passed down to the > functions that need it rather than declaring it as a global in "worktree.c". I can create a getter/setter in the worktree API to handle this, but I'd rather not pass it as an argument to every function that needs it as that would be a lot of changes. All of these functions would need their signatures updated to include the new parameter: - `add_worktree()` - `update_worktree_location()` - `repair_worktree_at_path()` - `repair_worktrees()` - `repair_worktree()` - `write_worktree_linking_files()` >> 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 I added an explanation for this deletion. Best, Caleb
On Wed, Oct 30, 2024 at 05:27:33AM +0000, Caleb White wrote: > >> 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". > > I can create a getter/setter in the worktree API to handle this, but > I'd rather not pass it as an argument to every function that needs it as > that would be a lot of changes. All of these functions would need their > signatures updated to include the new parameter: > > - `add_worktree()` > - `update_worktree_location()` > - `repair_worktree_at_path()` > - `repair_worktrees()` > - `repair_worktree()` > - `write_worktree_linking_files()` There is no reason to have a "getter" and "setter" for a extern'd variable. I agree that it would be preferable to have use_relative_paths be a static int within this compilation unit and to pass it to the above functions. Thanks, Taylor
On Wed Oct 30, 2024 at 3:16 PM CDT, Taylor Blau wrote: > On Wed, Oct 30, 2024 at 05:27:33AM +0000, Caleb White wrote: >> >> 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". >> >> I can create a getter/setter in the worktree API to handle this, but >> I'd rather not pass it as an argument to every function that needs it as >> that would be a lot of changes. All of these functions would need their >> signatures updated to include the new parameter: >> >> - `add_worktree()` >> - `update_worktree_location()` >> - `repair_worktree_at_path()` >> - `repair_worktrees()` >> - `repair_worktree()` >> - `write_worktree_linking_files()` > > There is no reason to have a "getter" and "setter" for a extern'd > variable. > > I agree that it would be preferable to have use_relative_paths be a > static int within this compilation unit and to pass it to the above > functions. If I created a getter/setter then the variable would no longer be extern'd. To be clear, you're advocating that I change the function signature for all of the functions listed above to include the new parameter? That seems like a lot of parameter bloat when I could just set the variable in this compilation unit and access it directly in the `write_worktree_linking_files()` function. Best, Caleb
Hi Caleb On 30/10/2024 20:21, Caleb White wrote: > On Wed Oct 30, 2024 at 3:16 PM CDT, Taylor Blau wrote: >> On Wed, Oct 30, 2024 at 05:27:33AM +0000, Caleb White wrote: >>>>> 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". >>> >>> I can create a getter/setter in the worktree API to handle this, but >>> I'd rather not pass it as an argument to every function that needs it as >>> that would be a lot of changes. All of these functions would need their >>> signatures updated to include the new parameter: >>> >>> - `add_worktree()` >>> - `update_worktree_location()` >>> - `repair_worktree_at_path()` >>> - `repair_worktrees()` >>> - `repair_worktree()` >>> - `write_worktree_linking_files()` >> >> There is no reason to have a "getter" and "setter" for a extern'd >> variable. >> >> I agree that it would be preferable to have use_relative_paths be a >> static int within this compilation unit and to pass it to the above >> functions. > > If I created a getter/setter then the variable would no longer be > extern'd. > > To be clear, you're advocating that I change the function signature > for all of the functions listed above to include the new parameter? That > seems like a lot of parameter bloat It's a bit of a pain to have to change the function signatures and pass the parameter down but it's not difficult to do. > when I could just set the variable > in this compilation unit and access it directly in the > `write_worktree_linking_files()` function. The problem with that is that the variable is still effectively global. The aim of the libification work is to be able to work on more than one repository from a single process while respecting each repository's config settings. Best Wishes Phillip > Best, > Caleb >
On Wed Oct 30, 2024 at 3:30 PM CDT, phillip.wood123 wrote: > On 30/10/2024 20:21, Caleb White wrote: >> If I created a getter/setter then the variable would no longer be >> extern'd. >> >> To be clear, you're advocating that I change the function signature >> for all of the functions listed above to include the new parameter? That >> seems like a lot of parameter bloat > > It's a bit of a pain to have to change the function signatures and pass > the parameter down but it's not difficult to do. > >> when I could just set the variable >> in this compilation unit and access it directly in the >> `write_worktree_linking_files()` function. > > The problem with that is that the variable is still effectively global. > The aim of the libification work is to be able to work on more than one > repository from a single process while respecting each repository's > config settings. > I see your point. I'll make the changes you've suggested. Thanks for the feedback. Best, Caleb
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. 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. ++ +With `repair`, the linking files will be updated if there's an absolute/relative +mismatch, even if the links are correct. ++ +This can also be set up as the default behaviour by using the +`worktree.useRelativePaths` config option. + --[no-]track:: When creating a new branch, if `<commit-ish>` is a branch, mark it as "upstream" from the new branch. This is the diff --git a/builtin/worktree.c b/builtin/worktree.c index dae63dedf4cac2621f51f95a39aa456b33acd894..c1130be5890c905c0b648782a834eb8dfcd79ba5 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -134,6 +134,9 @@ static int git_worktree_config(const char *var, const char *value, if (!strcmp(var, "worktree.guessremote")) { guess_remote = git_config_bool(var, value); return 0; + } else if (!strcmp(var, "worktree.userelativepaths")) { + use_relative_paths = git_config_bool(var, value); + return 0; } return git_default_config(var, value, ctx, cb); @@ -796,6 +799,8 @@ static int add(int ac, const char **av, const char *prefix) PARSE_OPT_NOARG | PARSE_OPT_OPTARG), OPT_BOOL(0, "guess-remote", &guess_remote, N_("try to match the new branch name with a remote-tracking branch")), + OPT_BOOL(0, "relative-paths", &use_relative_paths, + N_("use relative paths for worktrees")), OPT_END() }; int ret; @@ -1189,6 +1194,8 @@ static int move_worktree(int ac, const char **av, const char *prefix) OPT__FORCE(&force, N_("force move even if worktree is dirty or locked"), PARSE_OPT_NOCOMPLETE), + OPT_BOOL(0, "relative-paths", &use_relative_paths, + N_("use relative paths for worktrees")), OPT_END() }; struct worktree **worktrees, *wt; @@ -1382,6 +1389,8 @@ static int repair(int ac, const char **av, const char *prefix) const char **p; const char *self[] = { ".", NULL }; struct option options[] = { + OPT_BOOL(0, "relative-paths", &use_relative_paths, + N_("use relative paths for worktrees")), OPT_END() }; int rc = 0; diff --git a/t/t2408-worktree-relative.sh b/t/t2408-worktree-relative.sh deleted file mode 100755 index a3136db7e28cb20926ff44211e246ce625a6e51a..0000000000000000000000000000000000000000 --- a/t/t2408-worktree-relative.sh +++ /dev/null @@ -1,39 +0,0 @@ -#!/bin/sh - -test_description='test worktrees linked with relative paths' - -TEST_PASSES_SANITIZE_LEAK=true -. ./test-lib.sh - -test_expect_success 'links worktrees with relative paths' ' - test_when_finished rm -rf repo && - git init repo && - ( - cd repo && - test_commit initial && - git worktree add wt1 && - echo "../../../wt1/.git" >expected_gitdir && - cat .git/worktrees/wt1/gitdir >actual_gitdir && - echo "gitdir: ../.git/worktrees/wt1" >expected_git && - cat wt1/.git >actual_git && - test_cmp expected_gitdir actual_gitdir && - test_cmp expected_git actual_git - ) -' - -test_expect_success 'move repo without breaking relative internal links' ' - test_when_finished rm -rf repo moved && - git init repo && - ( - cd repo && - test_commit initial && - git worktree add wt1 && - cd .. && - mv repo moved && - cd moved/wt1 && - git status >out 2>err && - test_must_be_empty err - ) -' - -test_done diff --git a/worktree.c b/worktree.c index 77ff484d3ec48c547ee4e3d958dfa28a52c1eaa7..de5c5e53a5f2a758ddf470b5d6a9ad6c66247181 100644 --- a/worktree.c +++ b/worktree.c @@ -14,6 +14,8 @@ #include "wt-status.h" #include "config.h" +int use_relative_paths = 0; + void free_worktree(struct worktree *worktree) { if (!worktree) @@ -111,9 +113,9 @@ struct worktree *get_linked_worktree(const char *id, strbuf_strip_suffix(&worktree_path, "/.git"); if (!is_absolute_path(worktree_path.buf)) { - strbuf_strip_suffix(&path, "gitdir"); - strbuf_addbuf(&path, &worktree_path); - strbuf_realpath_forgiving(&worktree_path, path.buf, 0); + strbuf_strip_suffix(&path, "gitdir"); + strbuf_addbuf(&path, &worktree_path); + strbuf_realpath_forgiving(&worktree_path, path.buf, 0); } CALLOC_ARRAY(worktree, 1); @@ -725,12 +727,15 @@ static int is_main_worktree_path(const char *path) * won't know which <repo>/worktrees/<id>/gitdir to repair. However, we may * be able to infer the gitdir by manually reading /path/to/worktree/.git, * extracting the <id>, and checking if <repo>/worktrees/<id> exists. + * + * Returns -1 on failure and strbuf.len on success. */ static int infer_backlink(const char *gitfile, struct strbuf *inferred) { struct strbuf actual = STRBUF_INIT; const char *id; + strbuf_reset(inferred); if (strbuf_read_file(&actual, gitfile, 0) < 0) goto error; if (!starts_with(actual.buf, "gitdir:")) @@ -741,18 +746,16 @@ static int infer_backlink(const char *gitfile, struct strbuf *inferred) id++; /* advance past '/' to point at <id> */ if (!*id) goto error; - strbuf_reset(inferred); strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id); if (!is_directory(inferred->buf)) goto error; strbuf_release(&actual); - return 1; - + return inferred->len; error: strbuf_release(&actual); strbuf_reset(inferred); /* clear invalid path */ - return 0; + return -1; } /* diff --git a/worktree.h b/worktree.h index e96118621638667d87c5d7e0452ed10bd1ddf606..37e65d508ed23d3e7a29850bb938285072a3aaa6 100644 --- a/worktree.h +++ b/worktree.h @@ -5,6 +5,8 @@ struct strbuf; +extern int use_relative_paths; + struct worktree { /* The repository this worktree belongs to. */ struct repository *repo;
This patch introduces the `--[no-]relative-paths` CLI option for `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. 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. [1]: https://lore.kernel.org/git/20241007-wt_relative_paths-v3-0-622cf18c45eb@pm.me Suggested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Caleb White <cdwhite3@pm.me> --- Documentation/config/worktree.txt | 5 +++++ Documentation/git-worktree.txt | 12 ++++++++++++ builtin/worktree.c | 9 +++++++++ t/t2408-worktree-relative.sh | 39 --------------------------------------- worktree.c | 17 ++++++++++------- worktree.h | 2 ++ 6 files changed, 38 insertions(+), 46 deletions(-)