From: Phillip Wood Date: Thu, 28 Nov 2024 14:44:51 GMT Subject: Re: [PATCH v5 0/8] Allow relative worktree linking to be configured by the user Message-ID: <054b70c2-accd-4d85-a576-66910d35a26d@gmail.com> In-Reply-To: <20241125-wt_relative_options-v5-0-356d122ff3db@pm.me> Hi Caleb On 26/11/2024 01:51, Caleb White wrote: > Changes in v5: > - Added docs to `--relative-paths` option. > - Added test coverage for `repair_worktrees()` and relative paths. > - Move `strbuf_reset` call in `infer_backlink()`. > - Cleaned up tests. > - Slight stylistic changes. > - Tweaked commit messages. > - Updated base to 090d24e9af. Thanks for re-rolling, these changes sound good. Below is the range-diff of what is in seen today compared to last week. I've left it untrimmed so other people can check what's changed and I've added a couple of comments. The only thing I'm worried about is the deletion of a check for setting extensions.relativeWorktrees in patch 5 the rest of the changes look good, thank you for the extra test checks and log messages. Best Wishes Phillip 1: 67a622c7f2 ! 1: 0986f98022 setup: correctly reinitialize repository version @@ Commit message config to ensure that the repository version is set correctly. Signed-off-by: Caleb White - Signed-off-by: Taylor Blau + Signed-off-by: Junio C Hamano ## setup.c ## @@ setup.c: void initialize_repository_version(int hash_algo, 2: 85964909b0 ! 2: c36e1a59fa worktree: add `relativeWorktrees` extension @@ Commit message Suggested-by: Junio C Hamano Signed-off-by: Caleb White - Signed-off-by: Taylor Blau + Signed-off-by: Junio C Hamano ## Documentation/config/extensions.txt ## @@ Documentation/config/extensions.txt: Note that this setting should only be set by linkgit:git-init[1] or 3: a614c39333 ! 3: 5b19b63040 worktree: refactor infer_backlink return @@ Commit message [1]: https://lore.kernel.org/git/20241007-wt_relative_paths-v3-0-622cf18c45eb@pm.me Signed-off-by: Caleb White - Signed-off-by: Taylor Blau + Signed-off-by: Junio C Hamano ## worktree.c ## @@ worktree.c: struct worktree *get_linked_worktree(const char *id, @@ worktree.c: static int is_main_worktree_path(const char *path) { 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:")) @@ worktree.c: 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); 4: 83ea6d7ba0 ! 4: ec143ae00e worktree: add `write_worktree_linking_files()` function @@ Commit message is linked with relative paths. Signed-off-by: Caleb White - Signed-off-by: Taylor Blau + Signed-off-by: Junio C Hamano ## worktree.c ## @@ worktree.c: int init_worktree_config(struct repository *r) free(main_worktree_file); return res; } + -+void write_worktree_linking_files(struct strbuf dotgit, -+ struct strbuf gitdir, ++void write_worktree_linking_files(struct strbuf dotgit, struct strbuf gitdir, + int use_relative_paths) +{ + struct strbuf path = STRBUF_INIT; @@ worktree.h: void strbuf_worktree_ref(const struct worktree *wt, + * dotgit: "/path/to/foo/.git" + * gitdir: "/path/to/repo/worktrees/foo/gitdir" + */ -+void write_worktree_linking_files(struct strbuf dotgit, -+ struct strbuf gitdir, ++void write_worktree_linking_files(struct strbuf dotgit, struct strbuf gitdir, + int use_relative_paths); + #endif 5: 36d01dca84 ! 5: 237206b08f worktree: add relative cli/config options to `add` command @@ Commit message written for the various worktree operations in their own files. Signed-off-by: Caleb White - Signed-off-by: Taylor Blau + Signed-off-by: Junio C Hamano ## Documentation/config/worktree.txt ## @@ Documentation/config/worktree.txt: worktree.guessRemote:: @@ Documentation/git-worktree.txt: To remove a locked worktree, specify `--force` t `worktree.guessRemote` config option. +--[no-]relative-paths:: ++ Link worktrees using relative paths or absolute paths (default). + Overrides the `worktree.useRelativePaths` config option, see + linkgit:git-config[1]. + @@ t/t2400-worktree-add.sh: test_expect_success '"add" with initialized submodule, +test_expect_success 'can create worktrees with relative paths' ' + test_when_finished "git worktree remove relative" && -+ git config worktree.useRelativePaths false && ++ test_config worktree.useRelativePaths false && + git worktree add --relative-paths ./relative && -+ cat relative/.git >actual && + echo "gitdir: ../.git/worktrees/relative" >expect && -+ test_cmp expect actual && -+ cat .git/worktrees/relative/gitdir >actual && ++ test_cmp expect relative/.git && + echo "../../../relative/.git" >expect && -+ test_cmp expect actual -+ ++ test_cmp expect .git/worktrees/relative/gitdir +' + +test_expect_success 'can create worktrees with absolute paths' ' -+ git config worktree.useRelativePaths true && ++ test_config worktree.useRelativePaths true && + git worktree add ./relative && -+ cat relative/.git >actual && + echo "gitdir: ../.git/worktrees/relative" >expect && -+ test_cmp expect actual && ++ test_cmp expect relative/.git && + git worktree add --no-relative-paths ./absolute && -+ cat absolute/.git >actual && + echo "gitdir: $(pwd)/.git/worktrees/absolute" >expect && -+ test_cmp expect actual ++ test_cmp expect absolute/.git && ++ echo "$(pwd)/absolute/.git" >expect && ++ test_cmp expect .git/worktrees/absolute/gitdir +' + +test_expect_success 'move repo without breaking relative internal links' ' + test_when_finished rm -rf repo moved && + git init repo && + ( + cd repo && -+ git config worktree.useRelativePaths true && + test_commit initial && -+ git worktree add wt1 && ++ git worktree add --relative-paths wt1 && + cd .. && + mv repo moved && + cd moved/wt1 && -+ git status >out 2>err && ++ git worktree list >out 2>err && + test_must_be_empty err + ) +' @@ t/t2400-worktree-add.sh: test_expect_success '"add" with initialized submodule, + 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_cmp_config -C repo 1 core.repositoryformatversion +' We have lost the check for extensions.relativeworktrees here. Although we don't set worktree.useRelativePaths anymore we should still set the extension as we pase --relative-paths to "git worktree add" + test_done 6: 34401d680c ! 6: 8e4307f520 worktree: add relative cli/config options to `move` command @@ Commit message the new path will be relative (and visa-versa). Signed-off-by: Caleb White - Signed-off-by: Taylor Blau + Signed-off-by: Junio C Hamano ## builtin/worktree.c ## @@ builtin/worktree.c: static int move_worktree(int ac, const char **av, const char *prefix) @@ t/t2403-worktree-move.sh: test_expect_success 'not remove a repo with initialize ' +test_expect_success 'move worktree with absolute path to relative path' ' -+ git config worktree.useRelativePaths false && ++ test_config worktree.useRelativePaths false && + git worktree add ./absolute && + git worktree move --relative-paths absolute relative && -+ cat relative/.git >actual && + echo "gitdir: ../.git/worktrees/absolute" >expect && -+ test_cmp expect actual && -+ git config worktree.useRelativePaths true && ++ test_cmp expect relative/.git && ++ echo "../../../relative/.git" >expect && ++ test_cmp expect .git/worktrees/absolute/gitdir && ++ test_config worktree.useRelativePaths true && + git worktree move relative relative2 && -+ cat relative2/.git >actual && + echo "gitdir: ../.git/worktrees/absolute" >expect && -+ test_cmp expect actual ++ test_cmp expect relative2/.git && ++ echo "../../../relative2/.git" >expect && ++ test_cmp expect .git/worktrees/absolute/gitdir +' + +test_expect_success 'move worktree with relative path to absolute path' ' -+ git config worktree.useRelativePaths true && ++ test_config worktree.useRelativePaths true && + git worktree move --no-relative-paths relative2 absolute && -+ cat absolute/.git >actual && + echo "gitdir: $(pwd)/.git/worktrees/absolute" >expect && -+ test_cmp expect actual ++ test_cmp expect absolute/.git && ++ echo "$(pwd)/absolute/.git" >expect && ++ test_cmp expect .git/worktrees/absolute/gitdir +' + test_done @@ worktree.c: int validate_worktree(const struct worktree *wt, struct strbuf *errm } -void update_worktree_location(struct worktree *wt, const char *path_) -+void update_worktree_location(struct worktree *wt, -+ const char *path_, ++void update_worktree_location(struct worktree *wt, const char *path_, + int use_relative_paths) { struct strbuf path = STRBUF_INIT; @@ worktree.c: int validate_worktree(const struct worktree *wt, struct strbuf *errm ## worktree.h ## @@ worktree.h: int validate_worktree(const struct worktree *wt, + /* * Update worktrees/xxx/gitdir with the new path. */ - void update_worktree_location(struct worktree *wt, +-void update_worktree_location(struct worktree *wt, - const char *path_); -+ const char *path_, ++void update_worktree_location(struct worktree *wt, const char *path_, + int use_relative_paths); typedef void (* worktree_repair_fn)(int iserr, const char *path, 7: 30e9e41061 ! 7: 4f951f4088 worktree: add relative cli/config options to `repair` command @@ Commit message even if the original path was correct. This allows a user to covert existing worktrees between absolute/relative as desired. + To simplify things, both linking files are written when one of the files + needs to be repaired. In some cases, this fixes the other file before it + is checked, in other cases this results in a correct file being written + with the same contents. + Thanks for adding this, I think it will be really helpful for anyone trying to understand this change in the future. Signed-off-by: Caleb White - Signed-off-by: Taylor Blau + Signed-off-by: Junio C Hamano ## Documentation/git-worktree.txt ## @@ Documentation/git-worktree.txt: This can also be set up as the default behaviour by using the - --[no-]relative-paths:: + Link worktrees using relative paths or absolute paths (default). Overrides the `worktree.useRelativePaths` config option, see linkgit:git-config[1]. ++ @@ t/t2406-worktree-repair.sh: test_expect_success 'repair copied main and linked w test_cmp dup/linked.expect dup/linked/.git ' ++test_expect_success 'repair worktree with relative path with missing gitfile' ' ++ test_when_finished "rm -rf main wt" && ++ test_create_repo main && ++ git -C main config worktree.useRelativePaths true && ++ test_commit -C main init && ++ git -C main worktree add --detach ../wt && ++ rm wt/.git && ++ test_path_is_missing wt/.git && ++ git -C main worktree repair && ++ echo "gitdir: ../main/.git/worktrees/wt" >expect && ++ test_cmp expect wt/.git ++' ++ +test_expect_success 'repair absolute worktree to use relative paths' ' + test_when_finished "rm -rf main side sidemoved" && + test_create_repo main && @@ worktree.c: int other_head_refs(each_ref_fn fn, void *cb_data) */ static void repair_gitfile(struct worktree *wt, - worktree_repair_fn fn, void *cb_data) -+ worktree_repair_fn fn, -+ void *cb_data, ++ worktree_repair_fn fn, void *cb_data, + int use_relative_paths) { struct strbuf dotgit = STRBUF_INIT; @@ worktree.c: static ssize_t infer_backlink(const char *gitfile, struct strbuf *in */ void repair_worktree_at_path(const char *path, - worktree_repair_fn fn, void *cb_data) -+ worktree_repair_fn fn, -+ void *cb_data, ++ worktree_repair_fn fn, void *cb_data, + int use_relative_paths) { struct strbuf dotgit = STRBUF_INIT; 8: 298327f538 ! 8: 28eb7f66b2 worktree: refactor `repair_worktree_after_gitdir_move()` @@ Commit message `write_worktree_linking_files` function. It also preserves the relativity of the linking files; e.g., if an existing worktree used absolute paths then the repaired paths will be absolute (and visa-versa). + `repair_worktree_after_gitdir_move()` is used to repair both sets of + worktree linking files if the `.git` directory is moved during a + re-initialization using `git init`. Thanks for the extra explanation This also adds a test case for reinitializing a repository that has relative worktrees. Signed-off-by: Caleb White - Signed-off-by: Taylor Blau + Signed-off-by: Junio C Hamano ## t/t0001-init.sh ## @@ t/t0001-init.sh: test_expect_success SYMLINKS 're-init to move gitdir symlink' ' @@ t/t0001-init.sh: test_expect_success SYMLINKS 're-init to move gitdir symlink' ' git init mainwt && + if test "relative" = $2 + then -+ git -C mainwt config worktree.useRelativePaths true ++ test_config -C mainwt worktree.useRelativePaths true + else -+ git -C mainwt config worktree.useRelativePaths false ++ test_config -C mainwt worktree.useRelativePaths false + fi test_commit -C mainwt gumby && git -C mainwt worktree add --detach ../linkwt && > - Link to v4: https://lore.kernel.org/r/20241031-wt_relative_options-v4-0-07a3dc0f02a3@pm.me > Changes in v4: > - Fixed failing test in ci > - Link to v3: https://lore.kernel.org/r/20241031-wt_relative_options-v3-0-3e44ccdf64e6@pm.me > Changes in v3: > - Split patches into smaller edits. > - Moved tests into the patches with the relevant code changes. > - Removed global `use_relative_paths` and instead pass parameter to functions. > - Changed `infer_backlink` return type from `int` to `ssize_t`. > - Updated `worktree.useRelativePaths` and `--relative-paths` descriptions. > - Reordered patches > - Link to v2: https://lore.kernel.org/r/20241028-wt_relative_options-v2-0-33a5021bd7bb@pm.me > Changes in v2: > - Fixed a bug where repositories with valid extensions would be downgraded > to v0 during reinitialization, causing future operations to fail. > - Split patch [1/2] into three separate patches. > - Updated cover letter and commit messages. > - Updated documentation wording. > - Link to v1: https://lore.kernel.org/r/20241025-wt_relative_options-v1-0-c3005df76bf9@pm.me > > --- > Caleb White (8): > setup: correctly reinitialize repository version > worktree: add `relativeWorktrees` extension > worktree: refactor infer_backlink return > worktree: add `write_worktree_linking_files()` function > worktree: add relative cli/config options to `add` command > worktree: add relative cli/config options to `move` command > worktree: add relative cli/config options to `repair` command > worktree: refactor `repair_worktree_after_gitdir_move()` > > Documentation/config/extensions.txt | 6 ++ > Documentation/config/worktree.txt | 10 +++ > Documentation/git-worktree.txt | 8 ++ > builtin/worktree.c | 29 ++++--- > repository.c | 1 + > repository.h | 1 + > setup.c | 39 ++++++--- > setup.h | 1 + > t/t0001-init.sh | 22 ++++- > t/t2400-worktree-add.sh | 45 +++++++++++ > t/t2401-worktree-prune.sh | 3 +- > t/t2402-worktree-list.sh | 22 +++++ > t/t2403-worktree-move.sh | 25 ++++++ > t/t2406-worktree-repair.sh | 39 +++++++++ > t/t2408-worktree-relative.sh | 39 --------- > t/t5504-fetch-receive-strict.sh | 6 +- > worktree.c | 157 ++++++++++++++++++++---------------- > worktree.h | 22 ++++- > 18 files changed, 333 insertions(+), 142 deletions(-) > --- > base-commit: 090d24e9af6e9f59c3f7bee97c42bb1ae3c7f559 > change-id: 20241025-wt_relative_options-afa41987bc32 > > Best regards,