Hi Caleb
On 01/11/2024 04:38, Caleb White wrote:
This teaches the `worktree repair` command to respect the
`--[no-]relative-paths` CLI option and `worktree.useRelativePaths`
config setting. If an existing worktree with an absolute path is repaired
with `--relative-paths`, the links will be replaced with relative paths,
even if the original path was correct. This allows a user to covert
existing worktrees between absolute/relative as desired.
This looks pretty good the strbuf changes rather mask the real meat of
the patch though.
Signed-off-by: Caleb White <cdwhite3@xxxxx>
diff --git a/t/t2406-worktree-repair.sh b/t/t2406-worktree-repair.sh
index 7686e60f6ad186519b275f11a5e14064c905b207..84451e903b2ef3c645c0311faf055c846588baf6 100755
--- a/t/t2406-worktree-repair.sh
+++ b/t/t2406-worktree-repair.sh
@@ -216,4 +216,30 @@ test_expect_success 'repair copied main and linked worktrees' '
test_cmp dup/linked.expect dup/linked/.git
'
+test_expect_success 'repair absolute worktree to use relative paths' '
+ test_when_finished "rm -rf main side sidemoved" &&
+ test_create_repo main &&
+ test_commit -C main init &&
+ git -C main worktree add --detach ../side &&
+ echo "../../../../sidemoved/.git" >expect-gitdir &&
+ echo "gitdir: ../main/.git/worktrees/side" >expect-gitfile &&
+ mv side sidemoved &&
+ git -C main worktree repair --relative-paths ../sidemoved &&
+ test_cmp expect-gitdir main/.git/worktrees/side/gitdir &&
+ test_cmp expect-gitfile sidemoved/.git
+'
+
+test_expect_success 'repair relative worktree to use absolute paths' '
+ test_when_finished "rm -rf main side sidemoved" &&
+ test_create_repo main &&
+ test_commit -C main init &&
+ git -C main worktree add --relative-paths --detach ../side &&
+ echo "$(pwd)/sidemoved/.git" >expect-gitdir &&
+ echo "gitdir: $(pwd)/main/.git/worktrees/side" >expect-gitfile &&
+ mv side sidemoved &&
+ git -C main worktree repair ../sidemoved &&
+ test_cmp expect-gitdir main/.git/worktrees/side/gitdir &&
+ test_cmp expect-gitfile sidemoved/.git
+'
These tests looks sensibile, we should probably check that "git worktree
repair" repects worktree.userelativepaths. I wonder if we have any
coverage of repair_worktrees() as I think in these tests the problem is
fixed by repair_worktree_at_path() before we call repair_worktrees().
test_done
diff --git a/worktree.c b/worktree.c
index 6b640cd9549ecb060236f7eddf1390caa181f1a0..2cb994ac462debf966ac51b5a4f33c30cfebd4ef 100644
--- a/worktree.c
+++ b/worktree.c
@@ -574,12 +574,14 @@ int other_head_refs(each_ref_fn fn, void *cb_data)
* pointing at <repo>/worktrees/<id>.
*/
static void repair_gitfile(struct worktree *wt,
- worktree_repair_fn fn, void *cb_data)
+ worktree_repair_fn fn,
+ void *cb_data,
Style wise leaving "fn" and "cb_data" on the same line would be fine.
That applies to all the functions.
+ int use_relative_paths)
{
[...]
if (dotgit_contents) {
@@ -612,18 +615,20 @@ static void repair_gitfile(struct worktree *wt,
repair = _(".git file broken");
else if (fspathcmp(backlink.buf, repo.buf))
repair = _(".git file incorrect");
+ else if (use_relative_paths == is_absolute_path(dotgit_contents))
+ repair = _(".git file absolute/relative path mismatch");
Comparing ints as booleans makes me nervous in case we have a non-zero
value that isn't 1 but is_absolute_path() returns 0 or 1 and we know
use_relative_paths is 0 or 1.
if (repair) {
fn(0, wt->path, repair, cb_data);
- write_file(dotgit.buf, "gitdir: %s", relative_path(repo.buf, wt->path, &tmp));
+ write_worktree_linking_files(dotgit, gitdir, use_relative_paths);
We used to update only the ".git", now we'll update both. In the case
where we're changing to/from absolute/relative paths that's good because
we'll update the "gitdir" file as well. In the other cases it looks like
we've we've found this worktree via the "gitdir" file so it should be
safe to write the same value back to that file.
[...]
void repair_worktree_at_path(const char *path,
- worktree_repair_fn fn, void *cb_data)
+ worktree_repair_fn fn,
+ void *cb_data,
+ int use_relative_paths)
{
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;
- struct strbuf realolddotgit = STRBUF_INIT;
- struct strbuf tmp = STRBUF_INIT;
char *dotgit_contents = NULL;
const char *repair = NULL;
int err;
@@ -779,25 +783,25 @@ void repair_worktree_at_path(const char *path,
goto done;
strbuf_addf(&dotgit, "%s/.git", path);
- if (!strbuf_realpath(&realdotgit, dotgit.buf, 0)) {
+ if (!strbuf_realpath(&dotgit, dotgit.buf, 0)) {
This works because strbuf_realpath() copies dotgit.buf before it resets
dotgit but that does not seem to be documented and looking at the output of
git grep strbuf_realpath | grep \\.buf
I don't see any other callers relying on this outside of your earlier
changes to this file. Given that I wonder if we should leave it as is
which would also simplify this patch as the interesting changes are
swamped by the strbuf tweaking.
[...]
if (repair) {
fn(0, gitdir.buf, repair, cb_data);
- write_file(gitdir.buf, "%s", relative_path(realdotgit.buf, backlink.buf, &tmp));
+ write_worktree_linking_files(dotgit, gitdir, use_relative_paths);
We used to just update "gitdir" but we now update ".git" as well. As
above that's good when we're repairing a relative/absolute mismatch. In
the other cases dotgit always contains the path to the ".git" file in
the worktree so that should be fine.
Best Wishes
Phillip
