| Message ID | 20241025-wt_relative_options-v1-2-c3005df76bf9@pm.me (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Allow relative worktree linking to be configured by the user | expand |
On Fri, Oct 25, 2024 at 10:29:47PM +0000, Caleb White wrote: > A new extension, `relativeWorktrees`, is added to indicate that at least > one worktree in the repository has been linked with relative paths. This > extension is automatically set when a worktree is created or repaired > using the `--relative-paths` option, or when the > `worktree.useRelativePaths` config is set to `true`. This approach feels like a very heavy hammer. I wonder if writing worktrees with relative paths by default was too aggressive of a step, and if we should instead make the new behavior opt-in rather than opt-out. Saying that new worktrees are written with relative paths, and that creating a worktree with a new version of Git breaks reading the repository on older versions feels very unsatisfying to me. > The `relativeWorktrees` extension ensures older Git versions do not > attempt to automatically prune worktrees with relative paths, as they > would not not recognize the paths as being valid. This is the piece of information that I was missing in the earlier round when I merged that down. I think we need to take a step back and think about how to make this change safely in a way that doesn't break compatibility with older versions of Git. Thanks, Taylor
On Sun Oct 27, 2024 at 7:20 PM CDT, Taylor Blau wrote: > On Fri, Oct 25, 2024 at 10:29:47PM +0000, Caleb White wrote: >> A new extension, `relativeWorktrees`, is added to indicate that at least >> one worktree in the repository has been linked with relative paths. This >> extension is automatically set when a worktree is created or repaired >> using the `--relative-paths` option, or when the >> `worktree.useRelativePaths` config is set to `true`. > > This approach feels like a very heavy hammer. I wonder if writing > worktrees with relative paths by default was too aggressive of a step, > and if we should instead make the new behavior opt-in rather than > opt-out. The v4 series of the original round added the cli options and updated the relative worktrees to be opt-in. This patch keeps the git default behavior of writing worktrees with absolute paths, and users can opt-in via the `--relative-paths` cli option or the `worktree.useRelativePaths` config. > Saying that new worktrees are written with relative paths, and that > creating a worktree with a new version of Git breaks reading the > repository on older versions feels very unsatisfying to me. > >> The `relativeWorktrees` extension ensures older Git versions do not >> attempt to automatically prune worktrees with relative paths, as they >> would not not recognize the paths as being valid. > > This is the piece of information that I was missing in the earlier round > when I merged that down. I think we need to take a step back and think > about how to make this change safely in a way that doesn't break > compatibility with older versions of Git. Adding the extension was the direction suggested by Junio in the previous round. Git did not account for the possibility of the linking files containing relative paths, so there's really no way to make this change without breaking compatibility with older versions of Git. Git had to be taught how to handle files that could contain either absolute or relative paths. Best, Caleb
On Mon, Oct 28, 2024 at 01:14:51AM +0000, Caleb White wrote: > On Sun Oct 27, 2024 at 7:20 PM CDT, Taylor Blau wrote: > > On Fri, Oct 25, 2024 at 10:29:47PM +0000, Caleb White wrote: > >> A new extension, `relativeWorktrees`, is added to indicate that at least > >> one worktree in the repository has been linked with relative paths. This > >> extension is automatically set when a worktree is created or repaired > >> using the `--relative-paths` option, or when the > >> `worktree.useRelativePaths` config is set to `true`. > > > > This approach feels like a very heavy hammer. I wonder if writing > > worktrees with relative paths by default was too aggressive of a step, > > and if we should instead make the new behavior opt-in rather than > > opt-out. > > The v4 series of the original round added the cli options and updated > the relative worktrees to be opt-in. This patch keeps the git default > behavior of writing worktrees with absolute paths, and users can opt-in > via the `--relative-paths` cli option or the `worktree.useRelativePaths` > config. OK, I think the mistake here is mine. I did not see https://lore.kernel.org/git/xmqqfrp4onjd.fsf@gitster.g/ when triaging the list after Junio went offline for vacation. Had I not lost that email, I would not have merged the earlier round without more discussion. That being said, it is still greatly appreciated when contributors can follow the WC reports when they have patches that are moving through the various integration branches. That way you can see my "Will merge to 'next'" comment and say "please hold, I am working on a new round that is substantially different / uncovers some backwards incompatibility / etc." and we can wait appropriately. Now we are in the rather unfortunate situation of having merged something to 'master' that (with the additional information that I missed earlier) it is not clear that I would have merged in its existing form at the time. But that's OK, and we can figure out a path forward here. I am just trying to say that this highlights the importance of following the WC reports regularly to catch cases where the maintainer missed some important piece of information. > > Saying that new worktrees are written with relative paths, and that > > creating a worktree with a new version of Git breaks reading the > > repository on older versions feels very unsatisfying to me. > > > >> The `relativeWorktrees` extension ensures older Git versions do not > >> attempt to automatically prune worktrees with relative paths, as they > >> would not not recognize the paths as being valid. > > > > This is the piece of information that I was missing in the earlier round > > when I merged that down. I think we need to take a step back and think > > about how to make this change safely in a way that doesn't break > > compatibility with older versions of Git. > > Adding the extension was the direction suggested by Junio in the > previous round. Git did not account for the possibility of the linking > files containing relative paths, so there's really no way to make this > change without breaking compatibility with older versions of Git. Git > had to be taught how to handle files that could contain either absolute > or relative paths. Yep, that makes sense. My preference here would be to make the new behavior opt *in*, rather than opt-out, so that: - Users who do not experience problems with writing worktrees that have absolute paths can continue to do so without any changes. - Users who use worktrees *and* do not write relative paths can upgrade between successive versions without requiring a new repository extension that would break older Git versions. - That we only add that extension to the repository's configuration if and when the user has opted into the new behavior. Reading this new series, I *think* that is the behavior that you settled on, which seems quite reasonable to me. Can you confirm that I'm reading this all correctly? Assuming so, I think that we are in a reasonable position[^1] to review this series instead of having to back out the new behavior. Thanks for bearing with me here, I am quite embarrassed to have missed Junio's mail that I mentioned earlier, but I appreciate your patience while we sort this out together. Thanks, Taylor [^1]: Making sure that it gets in before 2.48, and is not dropped, though we still have quite a lot of time on that front.
On Mon Oct 28, 2024 at 12:08 PM CDT, Taylor Blau wrote: > OK, I think the mistake here is mine. I did not see > > https://lore.kernel.org/git/xmqqfrp4onjd.fsf@gitster.g/ > > when triaging the list after Junio went offline for vacation. Had I not > lost that email, I would not have merged the earlier round without more > discussion. > > That being said, it is still greatly appreciated when contributors can > follow the WC reports when they have patches that are moving through the > various integration branches. That way you can see my "Will merge to > 'next'" comment and say "please hold, I am working on a new round that > is substantially different / uncovers some backwards incompatibility / > etc." and we can wait appropriately. > > Now we are in the rather unfortunate situation of having merged > something to 'master' that (with the additional information that I > missed earlier) it is not clear that I would have merged in its existing > form at the time. > > But that's OK, and we can figure out a path forward here. I am just > trying to say that this highlights the importance of following the WC > reports regularly to catch cases where the maintainer missed some > important piece of information. My apologies, this was my first patch submission to Git and I was not exactly the process by which topics progressed from `seen` to `next` to `master`. I will be sure to follow the reports more closely in the future. >> Adding the extension was the direction suggested by Junio in the >> previous round. Git did not account for the possibility of the linking >> files containing relative paths, so there's really no way to make this >> change without breaking compatibility with older versions of Git. Git >> had to be taught how to handle files that could contain either absolute >> or relative paths. > > Yep, that makes sense. My preference here would be to make the new > behavior opt *in*, rather than opt-out, so that: > > - Users who do not experience problems with writing worktrees that > have absolute paths can continue to do so without any changes. > > - Users who use worktrees *and* do not write relative paths can > upgrade between successive versions without requiring a new > repository extension that would break older Git versions. > > - That we only add that extension to the repository's configuration if > and when the user has opted into the new behavior. > > Reading this new series, I *think* that is the behavior that you settled > on, which seems quite reasonable to me. Can you confirm that I'm reading > this all correctly? Assuming so, I think that we are in a reasonable > position[^1] to review this series instead of having to back out the new > behavior. Yes this is correct. The new behavior is opt-in and the extension is only added to the repository configuration if the user creates a worktree with relative paths. > Thanks for bearing with me here, I am quite embarrassed to have missed > Junio's mail that I mentioned earlier, but I appreciate your patience > while we sort this out together. No worries! I appreciate your feedback and I'm glad we're able to sort this out. Best, Caleb
On Mon, Oct 28, 2024 at 07:00:11PM +0000, Caleb White wrote: > > But that's OK, and we can figure out a path forward here. I am just > > trying to say that this highlights the importance of following the WC > > reports regularly to catch cases where the maintainer missed some > > important piece of information. > > My apologies, this was my first patch submission to Git and I was not > exactly the process by which topics progressed from `seen` to `next` to > `master`. I will be sure to follow the reports more closely in the future. It's not a problem, and I am glad that you have found an interest in contributing to the Git project. We'll figure this one out :-). > >> Adding the extension was the direction suggested by Junio in the > >> previous round. Git did not account for the possibility of the linking > >> files containing relative paths, so there's really no way to make this > >> change without breaking compatibility with older versions of Git. Git > >> had to be taught how to handle files that could contain either absolute > >> or relative paths. > > > > Yep, that makes sense. My preference here would be to make the new > > behavior opt *in*, rather than opt-out, so that: > > > > - Users who do not experience problems with writing worktrees that > > have absolute paths can continue to do so without any changes. > > > > - Users who use worktrees *and* do not write relative paths can > > upgrade between successive versions without requiring a new > > repository extension that would break older Git versions. > > > > - That we only add that extension to the repository's configuration if > > and when the user has opted into the new behavior. > > > > Reading this new series, I *think* that is the behavior that you settled > > on, which seems quite reasonable to me. Can you confirm that I'm reading > > this all correctly? Assuming so, I think that we are in a reasonable > > position[^1] to review this series instead of having to back out the new > > behavior. > > Yes this is correct. The new behavior is opt-in and the extension is > only added to the repository configuration if the user creates > a worktree with relative paths. That is great. I'm glad that we're on the same page here, and that my understanding matches reality. > > Thanks for bearing with me here, I am quite embarrassed to have missed > > Junio's mail that I mentioned earlier, but I appreciate your patience > > while we sort this out together. > > No worries! I appreciate your feedback and I'm glad we're able to > sort this out. Not a problem, and again, I very much appreciate your patience and willingness to work on this. Thanks, Taylor
diff --git a/Documentation/config/extensions.txt b/Documentation/config/extensions.txt index 5dc569d1c9c77c15e32441493289f9c9dd5e7f0b..5cb4721a0e0ae1ed64f90492c0dc18b96473cb33 100644 --- a/Documentation/config/extensions.txt +++ b/Documentation/config/extensions.txt @@ -63,6 +63,12 @@ Note that this setting should only be set by linkgit:git-init[1] or linkgit:git-clone[1]. Trying to change it after initialization will not work and will produce hard-to-diagnose issues. +relativeWorktrees:: + If enabled, indicates at least one worktree has been linked with + relative paths. Automatically set if a worktree has been created or + repaired with either the `--relative-paths` option or with the + `worktree.useRelativePaths` config set to `true`. + worktreeConfig:: If enabled, then worktrees will load config settings from the `$GIT_DIR/config.worktree` file in addition to the diff --git a/repository.c b/repository.c index f988b8ae68a6a29792e7f2c980a02bd0e388a3b9..1a6a62bbd03a5dc4fdade3eb45ea2696968abc23 100644 --- a/repository.c +++ b/repository.c @@ -283,6 +283,7 @@ int repo_init(struct repository *repo, repo_set_compat_hash_algo(repo, format.compat_hash_algo); repo_set_ref_storage_format(repo, format.ref_storage_format); repo->repository_format_worktree_config = format.worktree_config; + repo->repository_format_relative_worktrees = format.relative_worktrees; /* take ownership of format.partial_clone */ repo->repository_format_partial_clone = format.partial_clone; diff --git a/repository.h b/repository.h index 24a66a496a6ff516ce06d47b7329b3d36eb701ca..c4c92b2ab9c9e3b425dc2974636e33d1f4089c69 100644 --- a/repository.h +++ b/repository.h @@ -150,6 +150,7 @@ struct repository { /* Configurations */ int repository_format_worktree_config; + int repository_format_relative_worktrees; /* Indicate if a repository has a different 'commondir' from 'gitdir' */ unsigned different_commondir:1; diff --git a/setup.c b/setup.c index 7b648de0279116b381eea46800ad130606926103..6bf56cf72c4b46a95f46f9b3901f7e77d702cec7 100644 --- a/setup.c +++ b/setup.c @@ -683,6 +683,9 @@ static enum extension_result handle_extension(const char *var, "extensions.refstorage", value); data->ref_storage_format = format; return EXTENSION_OK; + } else if (!strcmp(ext, "relativeworktrees")) { + data->relative_worktrees = git_config_bool(var, value); + return EXTENSION_OK; } return EXTENSION_UNKNOWN; } @@ -1854,6 +1857,8 @@ const char *setup_git_directory_gently(int *nongit_ok) repo_fmt.ref_storage_format); the_repository->repository_format_worktree_config = repo_fmt.worktree_config; + the_repository->repository_format_relative_worktrees = + repo_fmt.relative_worktrees; /* take ownership of repo_fmt.partial_clone */ the_repository->repository_format_partial_clone = repo_fmt.partial_clone; @@ -1950,6 +1955,8 @@ void check_repository_format(struct repository_format *fmt) fmt->ref_storage_format); the_repository->repository_format_worktree_config = fmt->worktree_config; + the_repository->repository_format_relative_worktrees = + fmt->relative_worktrees; the_repository->repository_format_partial_clone = xstrdup_or_null(fmt->partial_clone); clear_repository_format(&repo_fmt); diff --git a/setup.h b/setup.h index e496ab3e4de580c2d9f95a7ea0eaf90e0d41b070..18dc3b73686ce28fac2fe04282ce95f8bf3e6b74 100644 --- a/setup.h +++ b/setup.h @@ -129,6 +129,7 @@ struct repository_format { int precious_objects; char *partial_clone; /* value of extensions.partialclone */ int worktree_config; + int relative_worktrees; int is_bare; int hash_algo; int compat_hash_algo; diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh index 630c13230b3cc762ce8d943e22be8891aa2b1871..d36d8a4db0e924877787697579544f10f92dc0cf 100755 --- a/t/t2400-worktree-add.sh +++ b/t/t2400-worktree-add.sh @@ -1248,4 +1248,17 @@ test_expect_success 'move repo without breaking relative internal links' ' ) ' +test_expect_success 'relative worktree sets extension config' ' + test_when_finished "rm -rf repo" && + git init repo && + git -C repo commit --allow-empty -m base && + git -C repo worktree add --relative-paths ./foo && + git -C repo config get core.repositoryformatversion >actual && + echo 1 >expected && + test_cmp expected actual && + git -C repo config get extensions.relativeworktrees >actual && + echo true >expected && + test_cmp expected actual +' + test_done diff --git a/worktree.c b/worktree.c index f4cee73d7a1edecafdff30b6d5e2d9dd1365b93e..0ae4a5f796f10c017ef90413e969e1622b54a897 100644 --- a/worktree.c +++ b/worktree.c @@ -1024,6 +1024,14 @@ void write_worktree_linking_files(struct strbuf dotgit, struct strbuf gitdir) strbuf_strip_suffix(&repo, "/gitdir"); strbuf_realpath(&repo, repo.buf, 1); + if (use_relative_paths && !the_repository->repository_format_relative_worktrees) { + if (upgrade_repository_format(1) < 0) + die(_("unable to upgrade repository format to support relative worktrees")); + if (git_config_set_gently("extensions.relativeWorktrees", "true")) + die(_("unable to set extensions.relativeWorktrees setting")); + the_repository->repository_format_relative_worktrees = 1; + } + if (use_relative_paths) { write_file(gitdir.buf, "%s/.git", relative_path(path.buf, repo.buf, &tmp)); write_file(dotgit.buf, "gitdir: %s", relative_path(repo.buf, path.buf, &tmp));
A new extension, `relativeWorktrees`, is added to indicate that at least one worktree in the repository has been linked with relative paths. This extension is automatically set when a worktree is created or repaired using the `--relative-paths` option, or when the `worktree.useRelativePaths` config is set to `true`. The `relativeWorktrees` extension ensures older Git versions do not attempt to automatically prune worktrees with relative paths, as they would not not recognize the paths as being valid. Suggested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Caleb White <cdwhite3@pm.me> --- Documentation/config/extensions.txt | 6 ++++++ repository.c | 1 + repository.h | 1 + setup.c | 7 +++++++ setup.h | 1 + t/t2400-worktree-add.sh | 13 +++++++++++++ worktree.c | 8 ++++++++ 7 files changed, 37 insertions(+)