{"thread":{"id":"62284","subject":"Re: [PATCH] worktree: repair copied repository and linked worktrees","startedAt":"2024-10-08T15:30:49Z","lastAt":"2024-10-08T15:53:53Z","messageCount":2,"participants":["Caleb White"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"504451","messageId":"20241008153035.71178-1-cdwhite3@pm.me","threadId":"62284","inReplyTo":null,"subject":"Re: [PATCH] worktree: repair copied repository and linked worktrees","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-08T15:30:42Z","receivedAt":"2024-10-08T15:30:49Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"Eric Sunshine <ericsunshine@charter.net> writes:\n\n> +\tinferred_backlink = infer_backlink(realdotgit.buf);\n>  \tbacklink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));\n>  \tif (err == READ_GITFILE_ERR_NOT_A_FILE) {\n>  \t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git is not a file\"), cb_data);\n>  \t\tgoto done;\n>  \t} else if (err == READ_GITFILE_ERR_NOT_A_REPO) {\n> -\t\tif (!(backlink = infer_backlink(realdotgit.buf))) {\n> +\t\tif (inferred_backlink) {\n> +\t\t\t/*\n> +\t\t\t * Worktree's .git file does not point at a repository\n> +\t\t\t * but we found a .git/worktrees/<id> in this\n> +\t\t\t * repository with the same <id> as recorded in the\n> +\t\t\t * worktree's .git file so make the worktree point at\n> +\t\t\t * the discovered .git/worktrees/<id>. (Note: backlink\n> +\t\t\t * is already NULL, so no need to free it first.)\n> +\t\t\t */\n> +\t\t\tbacklink = inferred_backlink;\n> +\t\t\tinferred_backlink = NULL;\n> +\t\t} else {\n>  \t\t\tfn(1, realdotgit.buf, _(\"unable to locate reposito\nry; .git file does not reference a repository\"), cb_data);\n>  \t\t\tgoto done;\n>  \t\t}\n\nMoving the `infer_backlink()` call outside of the error condition caused me to\ndiscover a bug in my own refactor of infer_backlink() on my relative paths series.\nTurns out I needed to clear the `inferred` path if an error occurred.\nThis is OK.\n\n> +\t/*\n> +\t * If we got this far, either the worktree's .git file pointed at a\n> +\t * valid repository (i.e. read_gitfile_gently() returned success) or\n> +\t * the .git file did not point at a repository but we were able to\n> +\t * infer a suitable new value for the .git file by locating a\n> +\t * .git/worktrees/<id> in *this* repository corresponding to the <id>\n> +\t * recorded in the worktree's .git file.\n> +\t *\n> +\t * However, if, at this point, inferred_backlink is non-NULL (i.e. we\n> +\t * found a suitable .git/worktrees/<id> in *this* repository) *and* the\n> +\t * worktree's .git file points at a valid repository *and* those two\n> +\t * paths \ndiffer, then that indicates that the user probably *copied*\n> +\t * the main and linked worktrees to a new location as a unit rather\n> +\t * than *moving* them. Thus, the copied worktree's .git file actually\n> +\t * points at the .git/worktrees/<id> in the *original* repository, not\n> +\t * in the \"copy\" repository. In this case, point the \"copy\" worktree's\n> +\t * .git file at the \"copy\" repository.\n> +\t */\n> +\tif (inferred_backlink && fspathcmp(backlink, inferred_backlink)) {\n> +\t\tfree(backlink);\n> +\t\tbacklink = inferred_backlink;\n> +\t\tinferred_backlink = NULL;\n> +\t}\n\nThis edge case seems to primarily be an artifact of using absolute paths to link\nthe worktrees. The test case you provided passes on my relative path series even\nwith this condition commented out. However, we do still want to support absolute\npaths to maintain backwards compatibility so I think we should try to detect\nthis edge case if we can.\n\nUnfortunately, this change now \"repairs\" (i.e., breaks) worktr\nees from other\nrepositories if the worktree_id happens to be identical between the repositories.\nFor example, the following test passes on `master` (with absolute paths) and on\nmy relative path series (relative paths without your changes integrated), but it\nfails on your patch (and my relative patch series with your changes integrated):\n\n    test_expect_success 'does not repair worktrees from another repo' '\n        test_when_finished \"rm -rf repo1 repo2\" &&\n        mkdir -p repo1 &&\n        git -C repo1 init main &&\n        test_commit -C repo1/main nothing &&\n        git -C repo1/main worktree add ../linked &&\n        cp repo1/main/.git/worktrees/linked/gitdir repo1/main.expect &&\n        cp repo1/linked/.git repo1/linked.expect &&\n        mkdir -p repo2 &&\n        git -C repo2 init main &&\n        test_commit -C repo2/main nothing &&\n        git -C repo2/main worktree add ../linked &&\n        cp repo2/main/.git/worktrees/linked/gitdir repo2/main.expect &&\n        \ncp repo2/linked/.git repo2/linked.expect &&\n        git -C repo1/main worktree repair ../.. &&\n        test_cmp repo1/main.expect repo1/main/.git/worktrees/linked/gitdir &&\n        test_cmp repo1/linked.expect repo1/linked/.git &&\n        test_cmp repo2/main.expect repo2/main/.git/worktrees/linked/gitdir &&\n        test_cmp repo2/linked.expect repo2/linked/.git &&\n        git -C repo2/main worktree repair ../../repo1/linked &&\n        test_cmp repo1/main.expect repo1/main/.git/worktrees/linked/gitdir &&\n        test_cmp repo1/linked.expect repo1/linked/.git &&\n        test_cmp repo2/main.expect repo2/main/.git/worktrees/linked/gitdir &&\n        test_cmp repo2/linked.expect repo2/linked/.git\n    '\n\nGranted, this is likely a rare edge case, but I would not expect trying to repair\na valid worktree from another repository to change the pointers. I think a good\nsolution to this would be to introduce a unique hash on the worktree_id when\ncreating a worktree to guarantee uniqu\neness across repositories, e.g.,:\n\n    repo1/main/.git/worktrees/linked-6a4b5d/gitdir:\n        /path/to/repo1/linked/.git\n    repo1/linked/.git:\n        gitdir: /path/to/repo1/main/worktrees/linked-6a4b5d\n\n    repo2/main/.git/worktrees/linked-9ab26fe/gitdir:\n        /path/to/repo2/linked/.git\n    repo2/linked/.git:\n        gitdir: /path/to/repo2/main/worktrees/linked-9ab26fe\n\nThis would allow us to detect this copied case while not \"repairing\" valid\nworktrees from other repositories with the same linked worktree directory name.\n\n\nBest,\n\n- Caleb\n"},{"id":"504452","messageId":"r4zmcET41Skr_FMop47AKd7cms9E8bKPSvHuAUpnYavzKEY6JybJta0_7GfuYB0q-gD-XNcvh5VDTfiT3qthGKjqhS1sbT4M2lUABynOz2Q=@pm.me","threadId":"62284","inReplyTo":"20241008153035.71178-1-cdwhite3@pm.me","subject":"Re: [PATCH] worktree: repair copied repository and linked worktrees","fromName":"Caleb White","fromEmail":"cdwhite3@pm.me","sentAt":"2024-10-08T15:53:46Z","receivedAt":"2024-10-08T15:53:53Z","isPatch":true,"sender":{"key":"cdwhite3@pm.me","avatar":"https://avatars.githubusercontent.com/u/4176520?v=4"},"body":"On Tuesday, October 8th, 2024 at 10:30, Caleb White <cdwhite3@pm.me> wrote:\n\n> test_expect_success 'does not repair worktrees from another repo' '\n> test_when_finished \"rm -rf repo1 repo2\" &&\n> mkdir -p repo1 &&\n> git -C repo1 init main &&\n> test_commit -C repo1/main nothing &&\n> git -C repo1/main worktree add ../linked &&\n> cp repo1/main/.git/worktrees/linked/gitdir repo1/main.expect &&\n> cp repo1/linked/.git repo1/linked.expect &&\n> mkdir -p repo2 &&\n> git -C repo2 init main &&\n> test_commit -C repo2/main nothing &&\n> git -C repo2/main worktree add ../linked &&\n> cp repo2/main/.git/worktrees/linked/gitdir repo2/main.expect &&\n> cp repo2/linked/.git repo2/linked.expect &&\n> git -C repo1/main worktree repair ../.. &&\n> test_cmp repo1/main.expect repo1/main/.git/worktrees/linked/gitdir &&\n> test_cmp repo1/linked.expect repo1/linked/.git &&\n> test_cmp repo2/main.expect repo2/main/.git/worktrees/linked/gitdir &&\n> test_cmp repo2/linked.expect repo2/linked/.git &&\n> git -C repo2/main worktree repair ../../repo1/linked &&\n> test_cmp repo1/main.expect repo1/main/.git/worktrees/linked/gitdir &&\n> test_cmp repo1/linked.expect repo1/linked/.git &&\n> test_cmp repo2/main.expect repo2/main/.git/worktrees/linked/gitdir &&\n> test_cmp repo2/linked.expect repo2/linked/.git\n> '\n\nApologies on this test case, it should be:\n\n    test_expect_success 'does not repair worktrees from another repo' '\n\ttest_when_finished \"rm -rf repo1 repo2\" &&\n\tmkdir -p repo1 &&\n\tgit -C repo1 init main &&\n\ttest_commit -C repo1/main nothing &&\n\tgit -C repo1/main worktree add ../linked &&\n\tcp repo1/main/.git/worktrees/linked/gitdir repo1/main.expect &&\n\tcp repo1/linked/.git repo1/linked.expect &&\n\tmkdir -p repo2 &&\n\tgit -C repo2 init main &&\n\ttest_commit -C repo2/main nothing &&\n\tgit -C repo2/main worktree add ../linked &&\n\tcp repo2/main/.git/worktrees/linked/gitdir repo2/main.expect &&\n\tcp repo2/linked/.git repo2/linked.expect &&\n\tgit -C repo1/main worktree repair ../../repo2/linked &&\n\ttest_cmp repo1/main.expect repo1/main/.git/worktrees/linked/gitdir &&\n\ttest_cmp repo1/linked.expect repo1/linked/.git &&\n\ttest_cmp repo2/main.expect repo2/main/.git/worktrees/linked/gitdir &&\n\ttest_cmp repo2/linked.expect repo2/linked/.git &&\n\tgit -C repo2/main worktree repair ../../repo1/linked &&\n\ttest_cmp repo1/main.expect repo1/main/.git/worktrees/linked/gitdir &&\n\ttest_cmp repo1/linked.expect repo1/linked/.git &&\n\ttest_cmp repo2/main.expect repo2/main/.git/worktrees/linked/gitdir &&\n\ttest_cmp repo2/linked.expect repo2/linked/.git\n    '\n\n- Caleb"}]}