{"thread":{"id":"54793","subject":"[PATCH] worktree: teach `repair` to fix multi-directional breakage","startedAt":"2020-12-08T17:39:31Z","lastAt":"2020-12-21T08:18:29Z","messageCount":7,"participants":["Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"411724","messageId":"20201208173705.5770-1-sunshine@sunshineco.com","threadId":"54793","inReplyTo":null,"subject":"[PATCH] worktree: teach `repair` to fix multi-directional breakage","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-08T17:37:05Z","receivedAt":"2020-12-08T17:39:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"`git worktree repair` knows how to repair the two-way links between the\nrepository and a worktree as long as a link in one or the other\ndirection is sound. For instance, if a linked worktree is moved (without\nusing `git worktree move`), repair is possible because the worktree\nstill knows the location of the repository even though the repository no\nlonger knows where the worktree is. Similarly, if the repository is\nmoved, repair is possible since the repository still knows the locations\nof the worktrees even though the worktrees no longer know where the\nrepository is.\n\nHowever, if both the repository and the worktrees are moved, then links\nare severed in both directions, and no repair is possible. This is the\ncase even when the new worktree locations are specified as arguments to\n`git worktree repair`. The reason for this limitation is twofold. First,\nwhen `repair` consults the worktree's gitfile (/path/to/worktree/.git)\nto determine the corresponding /worktrees//gitdir file to fix,\n is the old path to the repository, thus it is unable to fix the\n`gitdir` file at its new location since it doesn't know where it is.\nSecond, when `repair` consults /worktrees//gitdir to find the\nlocation of the worktree's gitfile (/path/to/worktree/.git), the path\nrecorded in `gitdir` is the old location of the worktree's gitfile, thus\nit is unable to repair the gitfile since it doesn't know where it is.\n\nFix these shortcomings by teaching `repair` to attempt to infer the new\nlocation of the /worktrees//gitdir file when the location\nrecorded in the worktree's gitfile has become stale but the file is\notherwise well-formed.\n\nSigned-off-by: Eric Sunshine \n---\n\nThis patch addresses a limitation of `git worktree repair` in which it\nis unable to repair the two-way links between the repository and\nsecondary worktrees if both the repository and the worktrees have been\nmoved manually. This limitation was known at the time `repair` was\nintroduced but I left it and other bells-and-whistles unimplemented\nbecause bare bone `repair` was already fairly complex and I worried\nabout reviewer fatigue. However, a recent report[1] by Philippe, in\nwhich he encountered exactly this situation, prompted me to think about\nhow `repair` should deal with the case, and this patch is the result.\n\n[1]: https://lore.kernel.org/git/63AC7AC2-5D32-479B-BF9E-0E5C31351A1B@gmail.com/\n\n Documentation/git-worktree.txt | 5 +++++\n builtin/worktree.c | 2 +-\n t/t2406-worktree-repair.sh | 25 +++++++++++++++++++++\n worktree.c | 41 ++++++++++++++++++++++++++++++++++\n 4 files changed, 72 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex af06128cc9..02a706c4c0 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -143,6 +143,11 @@ locate it. Running `repair` within the recently-moved working tree will\n reestablish the connection. If multiple linked working trees are moved,\n running `repair` from any working tree with each tree's new `` as\n an argument, will reestablish the connection to all the specified paths.\n++\n+If both the main working tree and linked working trees have been moved\n+manually, then running `repair` in the main working tree and specifying the\n+new `` of each linked working tree will reestablish all connections\n+in both directions.\n \n unlock::\n \ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 197fd24a55..71287b2da6 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -1052,10 +1052,10 @@ static int repair(int ac, const char **av, const char *prefix)\n \tint rc = 0;\n \n \tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n-\trepair_worktrees(report_repair, &rc);\n \tp = ac > 0 ? av : self;\n \tfor (; *p; p++)\n \t\trepair_worktree_at_path(*p, report_repair, &rc);\n+\trepair_worktrees(report_repair, &rc);\n \treturn rc;\n }\n \ndiff --git a/t/t2406-worktree-repair.sh b/t/t2406-worktree-repair.sh\nindex 1fe468bfe8..687342bfa7 100755\n--- a/t/t2406-worktree-repair.sh\n+++ b/t/t2406-worktree-repair.sh\n@@ -104,6 +104,16 @@ test_expect_success 'repo not found; .git not file' '\n \ttest_i18ngrep \".git is not a file\" err\n '\n \n+test_expect_success 'repo not found; .git not referencing repo' '\n+\ttest_when_finished \"rm -rf side not-a-repo && git worktree prune\" &&\n+\tgit worktree add --detach side &&\n+\tsed s,\\.git/worktrees/side$,not-a-repo, side/.git >side/.newgit &&\n+\tmv side/.newgit side/.git &&\n+\tmkdir not-a-repo &&\n+\ttest_must_fail git worktree repair side 2>err &&\n+\ttest_i18ngrep \".git file does not reference a repository\" err\n+'\n+\n test_expect_success 'repo not found; .git file broken' '\n \ttest_when_finished \"rm -rf orig moved && git worktree prune\" &&\n \tgit worktree add --detach orig &&\n@@ -176,4 +186,19 @@ test_expect_success 'repair multiple gitdir files' '\n \ttest_must_be_empty err\n '\n \n+test_expect_success 'repair moved main and linked worktrees' '\n+\ttest_when_finished \"rm -rf orig moved\" &&\n+\ttest_create_repo orig/main &&\n+\ttest_commit -C orig/main init &&\n+\tgit -C orig/main worktree add --detach ../side &&\n+\tsed s,orig/side/\\.git,moved/side/.git, \\\n+\t\torig/main/.git/worktrees/side/gitdir >expect-gitdir &&\n+\tsed s,orig/main/.git/worktrees/side,moved/main/.git/worktrees/side, \\\n+\t\torig/side/.git >expect-gitfile &&\n+\tmv orig moved &&\n+\tgit -C moved/main worktree repair ../side &&\n+\ttest_cmp expect-gitdir moved/main/.git/worktrees/side/gitdir &&\n+\ttest_cmp expect-gitfile moved/side/.git\n+'\n+\n test_done\ndiff --git a/worktree.c b/worktree.c\nindex f84ceae87d..821b233479 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -644,6 +644,42 @@ static int is_main_worktree_path(const char *path)\n \treturn !cmp;\n }\n \n+/*\n+ * If both the main worktree and linked worktree have been moved, then the\n+ * gitfile /path/to/worktree/.git won't point into the repository, thus we\n+ * won't know which /worktrees//gitdir to repair. However, we may\n+ * be able to infer the gitdir by manually reading /path/to/worktree/.git,\n+ * extracting the , and checking if /worktrees/ exists.\n+ */\n+static char *infer_backlink(const char *gitfile)\n+{\n+\tstruct strbuf actual = STRBUF_INIT;\n+\tstruct strbuf inferred = STRBUF_INIT;\n+\tconst char *id;\n+\n+\tif (strbuf_read_file(&actual, gitfile, 0) < 0)\n+\t\tgoto error;\n+\tif (!starts_with(actual.buf, \"gitdir:\"))\n+\t\tgoto error;\n+\tif (!(id = find_last_dir_sep(actual.buf)))\n+\t\tgoto error;\n+\tstrbuf_trim(&actual);\n+\tid++; /* advance past '/' to point at */\n+\tif (!*id)\n+\t\tgoto error;\n+\tstrbuf_git_common_path(&inferred, the_repository, \"worktrees/%s\", id);\n+\tif (!is_directory(inferred.buf))\n+\t\tgoto error;\n+\n+\tstrbuf_release(&actual);\n+\treturn strbuf_detach(&inferred, NULL);\n+\n+error:\n+\tstrbuf_release(&actual);\n+\tstrbuf_release(&inferred);\n+\treturn NULL;\n+}\n+\n /*\n * Repair /worktrees//gitdir if missing, corrupt, or not pointing at\n * the worktree's path.\n@@ -675,6 +711,11 @@ void repair_worktree_at_path(const char *path,\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\t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file does not reference a repository\"), cb_data);\n+\t\t\tgoto done;\n+\t\t}\n \t} else if (err) {\n \t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file broken\"), cb_data);\n \t\tgoto done;\n-- \n2.29.2.684.gfbc64c5ab5\n\n"},{"id":"411751","messageId":"xmqqwnxs55rn.fsf@gitster.c.googlers.com","threadId":"54793","inReplyTo":"20201208173705.5770-1-sunshine@sunshineco.com","subject":"Re: [PATCH] worktree: teach `repair` to fix multi-directional breakage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-08T21:47:56Z","receivedAt":"2020-12-08T21:49:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> However, if both the repository and the worktrees are moved, then links\n> are severed in both directions, and no repair is possible. This is the\n> case even when the new worktree locations are specified as arguments to\n> `git worktree repair`. The reason for this limitation is twofold. First,\n> when `repair` consults the worktree's gitfile (/path/to/worktree/.git)\n> to determine the corresponding /worktrees//gitdir file to fix,\n> is the old path to the repository, thus it is unable to fix the\n> `gitdir` file at its new location since it doesn't know where it is.\n> Second, when `repair` consults /worktrees//gitdir to find the\n> location of the worktree's gitfile (/path/to/worktree/.git), the path\n> recorded in `gitdir` is the old location of the worktree's gitfile, thus\n> it is unable to repair the gitfile since it doesn't know where it is.\n\nThird, when a worktree of an unrelated repository is moved to\nlocation the worktree moved like that used to occupy, or an\nunrelated repository is moved to the repository moved like that used\nto occupy, the gitfile of the moved worktree would point at an\nunrelated repository (which may not even be a clone of the same\nproject).\n\nThere probably are other failure cases possible. Perhaps having to\nnotice and fail when both worktree and repository moved (i.e. the\ncase your patch handles) indicates that we would need to be able to\nhandle such a situation sensibly as well? Do we leave some clue to\nhelp us validate that the repository the gitfile points at indeed\ncorresponds to the one our worktree works with and vice versa? If\nwe don't currently, should we?\n\n> Fix these shortcomings by teaching `repair` to attempt to infer the new\n> location of the /worktrees//gitdir file when the location\n> recorded in the worktree's gitfile has become stale but the file is\n> otherwise well-formed.\n\nHmph, after reading the \"first\" and \"second\" above a few times, it\nis unclear how this \"inference\" would work. The gitfile points at\nthe old location of the repository, which was moved to elsewhere\nwithout telling the working tree (i.e. \"First\" problem), so...?\n\n> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\n> index af06128cc9..02a706c4c0 100644\n> --- a/Documentation/git-worktree.txt\n> +++ b/Documentation/git-worktree.txt\n> @@ -143,6 +143,11 @@ locate it. Running `repair` within the recently-moved working tree will\n> reestablish the connection. If multiple linked working trees are moved,\n> running `repair` from any working tree with each tree's new `` as\n> an argument, will reestablish the connection to all the specified paths.\n> ++\n> +If both the main working tree and linked working trees have been moved\n> +manually, then running `repair` in the main working tree and specifying the\n> +new `` of each linked working tree will reestablish all connections\n> +in both directions.\n\nThis sounds like miracles, but it is perfectly the right thing to do\nto say what we can do without telling users how we do so.\n\n> diff --git a/worktree.c b/worktree.c\n> index f84ceae87d..821b233479 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -644,6 +644,42 @@ static int is_main_worktree_path(const char *path)\n> \treturn !cmp;\n> }\n> \n> +/*\n> + * If both the main worktree and linked worktree have been moved, then the\n> + * gitfile /path/to/worktree/.git won't point into the repository, thus we\n> + * won't know which /worktrees//gitdir to repair. However, we may\n> + * be able to infer the gitdir by manually reading /path/to/worktree/.git,\n> + * extracting the , and checking if /worktrees/ exists.\n> + */\n> +static char *infer_backlink(const char *gitfile)\n> +{\n> +\tstruct strbuf actual = STRBUF_INIT;\n> +\tstruct strbuf inferred = STRBUF_INIT;\n> +\tconst char *id;\n> +\n> +\tif (strbuf_read_file(&actual, gitfile, 0) < 0)\n> +\t\tgoto error;\n> +\tif (!starts_with(actual.buf, \"gitdir:\"))\n> +\t\tgoto error;\n> +\tif (!(id = find_last_dir_sep(actual.buf)))\n> +\t\tgoto error;\n> +\tstrbuf_trim(&actual);\n> +\tid++; /* advance past '/' to point at */\n> +\tif (!*id)\n> +\t\tgoto error;\n> +\tstrbuf_git_common_path(&inferred, the_repository, \"worktrees/%s\", id);\n> +\tif (!is_directory(inferred.buf))\n> +\t\tgoto error;\n> +\n> +\tstrbuf_release(&actual);\n> +\treturn strbuf_detach(&inferred, NULL);\n> +\n> +error:\n> +\tstrbuf_release(&actual);\n> +\tstrbuf_release(&inferred);\n> +\treturn NULL;\n> +}\n> +\n> /*\n> * Repair /worktrees//gitdir if missing, corrupt, or not pointing at\n> * the worktree's path.\n> @@ -675,6 +711,11 @@ void repair_worktree_at_path(const char *path,\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\t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file does not reference a repository\"), cb_data);\n> +\t\t\tgoto done;\n> +\t\t}\n> \t} else if (err) {\n> \t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file broken\"), cb_data);\n> \t\tgoto done;\n\n\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 197fd24a55..71287b2da6 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -1052,10 +1052,10 @@ static int repair(int ac, const char **av, const char *prefix)\n> \tint rc = 0;\n> \n> \tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n> -\trepair_worktrees(report_repair, &rc);\n> \tp = ac > 0 ? av : self;\n> \tfor (; *p; p++)\n> \t\trepair_worktree_at_path(*p, report_repair, &rc);\n> +\trepair_worktrees(report_repair, &rc);\n> \treturn rc;\n> }\n\nThe reason why repair_worktrees() has to wait until the individual\nrepairing is done for all the given paths in the new code is...?\n\n> diff --git a/t/t2406-worktree-repair.sh b/t/t2406-worktree-repair.sh\n> index 1fe468bfe8..687342bfa7 100755\n> --- a/t/t2406-worktree-repair.sh\n> +++ b/t/t2406-worktree-repair.sh\n> @@ -104,6 +104,16 @@ test_expect_success 'repo not found; .git not file' '\n> \ttest_i18ngrep \".git is not a file\" err\n> '\n> \n> +test_expect_success 'repo not found; .git not referencing repo' '\n> +\ttest_when_finished \"rm -rf side not-a-repo && git worktree prune\" &&\n> +\tgit worktree add --detach side &&\n> +\tsed s,\\.git/worktrees/side$,not-a-repo, side/.git >side/.newgit &&\n> +\tmv side/.newgit side/.git &&\n> +\tmkdir not-a-repo &&\n> +\ttest_must_fail git worktree repair side 2>err &&\n> +\ttest_i18ngrep \".git file does not reference a repository\" err\n> +'\n\nFailing upon empty directory and things that do not make sense is\ngood. A more interesting or realistic case would have the side/.git\ngitlink points at an unrelated repository because the user moved the\nrepository and replaced it with another repository, which has the\n\".git/worktree/\" directory and even \".git/worktree/side\" directory.\n\n> +test_expect_success 'repair moved main and linked worktrees' '\n> +\ttest_when_finished \"rm -rf orig moved\" &&\n> +\ttest_create_repo orig/main &&\n> +\ttest_commit -C orig/main init &&\n> +\tgit -C orig/main worktree add --detach ../side &&\n> +\tsed s,orig/side/\\.git,moved/side/.git, \\\n\nThe pattern needs anchored with '$' to the right, in order not to\nget confused by a substring like \"ooorig/side/.gitto/\" that appears\nin the $(cwd).\n\n> +\t\torig/main/.git/worktrees/side/gitdir >expect-gitdir &&\n> +\tsed s,orig/main/.git/worktrees/side,moved/main/.git/worktrees/side, \\\n> +\t\torig/side/.git >expect-gitfile &&\n\n> +\tmv orig moved &&\n\nHmmm, so this single move is what moves both the repository with the\nprimary worktree and the secondary worktree. Does the \"fix\" assume\nthat the relative location between them do not change? How realistic\nis that, I wonder?\n\nIn any case, I think the problem description in the proposed log\nmessage is quite well written but the solution seems unclear.\n\nThanks.\n\n> +\tgit -C moved/main worktree repair ../side &&\n> +\ttest_cmp expect-gitdir moved/main/.git/worktrees/side/gitdir &&\n> +\ttest_cmp expect-gitfile moved/side/.git\n> +'\n> +\n> test_done\n"},{"id":"412486","messageId":"CAPig+cQT2Cj4i=2kq_svMyKiFmNLxvpa5kG=XNeaT+8UD5238Q@mail.gmail.com","threadId":"54793","inReplyTo":"xmqqwnxs55rn.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] worktree: teach `repair` to fix multi-directional breakage","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-17T10:22:54Z","receivedAt":"2020-12-17T10:23:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 8, 2020 at 4:48 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > However, if both the repository and the worktrees are moved, then links\n> > are severed in both directions, and no repair is possible. [...]\n>\n> Third, when a worktree of an unrelated repository is moved to\n> location the worktree moved like that used to occupy, or an\n> unrelated repository is moved to the repository moved like that used\n> to occupy, the gitfile of the moved worktree would point at an\n> unrelated repository (which may not even be a clone of the same\n> project).\n>\n> There probably are other failure cases possible.\n\nThere are indeed many ways to shoot oneself in the foot.\n\n> Perhaps having to\n> notice and fail when both worktree and repository moved (i.e. the\n> case your patch handles) indicates that we would need to be able to\n> handle such a situation sensibly as well? Do we leave some clue to\n> help us validate that the repository the gitfile points at indeed\n> corresponds to the one our worktree works with and vice versa? If\n> we don't currently, should we?\n\nWe only have the paths making the two-way link between the repository\nand the secondary worktrees. Specifically, the outgoing\n/worktrees//gitdir points at /path/to/worktree/.git, and the\nincoming /path/to/worktree/.git points at /worktrees/.\n\nAs for the scenario you mention about moving a worktree to the\nlocation previously occupied by a worktree belonging to a different\nrepository, or moving a repository to a location previously occupied\nby a different repository, it is certainly possible to get into odd\nsituations. For instance, if \"worktree-A\" from repository \"A\" is\nmoved to the location previously occupied by \"worktree-B\" from\nrepository \"B\", then \"worktree-A\" won't consider itself broken, but\nboth repositories \"A\" and \"B\" will consider the worktree broken\n(because \"A\" will complain that \"worktree-A\" is missing, and \"B\" will\ncomplain that \"worktree-A\" doesn't point back at \"B\").\n\nI alluded to such confusion in my original response[1] to Philippe by\nsaying that, if not careful, a heuristic for inferring the new\nlocation could end up hijacking a worktree from a different\nrepository.\n\nSo, there is no mechanism for reliably associating a worktree with a\nrepository, and this limitation isn't specific to secondary worktrees\ncreated by `git worktree add`. There is no reliable association\nbetween the repository (which may have been split off with\n--separate-git-dir, for instance) and the main worktree either. This\nmay suggest the need to assign unique signatures to the repository and\neach worktree (including the main worktree), but such a change\nrequires plenty of thought and is far outside the scope of this simple\nenhancement to `git worktree repair`.\n\n[1]: https://lore.kernel.org/git/CAPig+cT=-nNcfrzjSmTXymhFkB22bPFE6QRKXqPtat2ipUdboQ@mail.gmail.com/\n\n> > Fix these shortcomings by teaching `repair` to attempt to infer the new\n> > location of the /worktrees//gitdir file when the location\n> > recorded in the worktree's gitfile has become stale but the file is\n> > otherwise well-formed.\n>\n> Hmph, after reading the \"first\" and \"second\" above a few times, it\n> is unclear how this \"inference\" would work. The gitfile points at\n> the old location of the repository, which was moved to elsewhere\n> without telling the working tree (i.e. \"First\" problem), so...?\n\nThe inference is intentionally simple-minded. There is no path-based\ninference or other heuristic at play; the entire inference is based\nupon . The worktree's path is specified as an argument. `git\nworktree repair` manually reads the \".git\" gitfile at that location\nand, if it is well-formed, extracts the . It then searches for a\ncorresponding in /worktrees/ and, if found, concludes that\nthere is a reasonable match and \"repairs\" /worktrees//gitdir\nto point at the specified worktree path.\n\nI can update the commit message to describe the heuristic rather than\nrequiring the reader consult infer_backlink() in the patch itself, if\nthat would help.\n\nThe patch incorporates two safeguards to avoid hijacking a worktree\nfrom a different repository. The first, as described above, is that it\nrequires an match between the repository and the worktree. That\nitself is not foolproof for preventing hijack, so the second safeguard\nis that the inference will only kick in if the worktree's\n/path/to/worktree/.git gitfile does not point at a repository. I can\nmention these safeguards in the commit message, as well.\n\nIt's still possible to bypass the safeguards by moving the foreign\nrepository too, in which case the foreign worktree being repaired\nwon't point at its repository anymore, thus there is no 100% guarantee\nof safety (without the introduction of unique signatures mentioned\nabove). However, the goal of this patch (and `git worktree repair` in\ngeneral) is to help users recover from what are likely to be common\nproblems, not to protect users from shooting themselves in the foot by\nthe many means available.\n\nA couple observations if, despite the safeguards, the user does manage\nto hijack a worktree: (1) the problem will likely be noticed quickly\nwhen, for instance, `git status` reports the bulk of the worktree's\nfiles as untracked and other expected files missing due to the\nmismatched `index`; (2) it's easy enough to recover from the situation\n(without damage) by manually patching /worktrees//gitdir and\n/path/to/worktree/.git, which is what the user would have had to do\nanyhow prior to the existence of `git worktree repair`.\n\n> > - repair_worktrees(report_repair, &rc);\n> > p = ac > 0 ? av : self;\n> > for (; *p; p++)\n> > repair_worktree_at_path(*p, report_repair, &rc);\n> > + repair_worktrees(report_repair, &rc);\n>\n> The reason why repair_worktrees() has to wait until the individual\n> repairing is done for all the given paths in the new code is...?\n\nBecause the repair can't succeed if both halves of the two-way link\nare broken, but can succeed if at least one half is sound. As\ndescribed above, the worktree path is specified as an argument to `git\nworktree repair` and the new inference may allow\nrepair_worktree_at_path() to repair the outgoing\n/worktrees//gitdir, thus restoring one half of the two-way\nlink, which then allows the subsequent repair_worktrees() to repair\nthe other half, the incoming /path/to/worktree/.git file.\n\nI realized even when writing the commit message that the reason for\nthis particular change might not be obvious, but I also was concerned\nthat I wouldn't be able to explain it in a succinct way without\noverwhelming the rest of the commit message. However, I can try to\ncome up with suitable phrasing and incorporate the explanation into\nthe commit message.\n\n> > +test_expect_success 'repo not found; .git not referencing repo' '\n> > + test_when_finished \"rm -rf side not-a-repo && git worktree prune\" &&\n> > + git worktree add --detach side &&\n> > + sed s,\\.git/worktrees/side$,not-a-repo, side/.git >side/.newgit &&\n> > + mv side/.newgit side/.git &&\n> > + mkdir not-a-repo &&\n> > + test_must_fail git worktree repair side 2>err &&\n> > + test_i18ngrep \".git file does not reference a repository\" err\n> > +'\n>\n> Failing upon empty directory and things that do not make sense is\n> good. A more interesting or realistic case would have the side/.git\n> gitlink points at an unrelated repository because the user moved the\n> repository and replaced it with another repository, which has the\n> \".git/worktree/\" directory and even \".git/worktree/side\" directory.\n\nDetecting that sort of situation is outside the scope of this patch\nsince `git worktree repair` itself can't presently detect it. Some\nfuture enhancement may make this possible, but the current change\ndoesn't deal with it.\n\nThis test is aimed specifically at verifying that\nrepair_worktree_at_path() now specially handles\nREAD_GITFILE_ERR_NOT_A_REPO returned by read_gitfile_gently() rather\nthan lumping it together with the other generic errors returned by the\nfunction. It recognizes this specific error because, as a safety\nmechanism against hijacking, the new inference is only attempted when\nREAD_GITFILE_ERR_NOT_A_REPO is returned.\n\n> > +test_expect_success 'repair moved main and linked worktrees' '\n> > + test_when_finished \"rm -rf orig moved\" &&\n> > + test_create_repo orig/main &&\n> > + test_commit -C orig/main init &&\n> > + git -C orig/main worktree add --detach ../side &&\n> > + sed s,orig/side/\\.git,moved/side/.git, \\\n>\n> The pattern needs anchored with '$' to the right, in order not to\n> get confused by a substring like \"ooorig/side/.gitto/\" that appears\n> in the $(cwd).\n\nWell spotted. The test I added just above this test correctly anchors\nthe pattern, but I forgot it here. Will fix.\n\n> > + orig/main/.git/worktrees/side/gitdir >expect-gitdir &&\n> > + sed s,orig/main/.git/worktrees/side,moved/main/.git/worktrees/side, \\\n> > + orig/side/.git >expect-gitfile &&\n>\n> > + mv orig moved &&\n>\n> Hmmm, so this single move is what moves both the repository with the\n> primary worktree and the secondary worktree. Does the \"fix\" assume\n> that the relative location between them do not change? How realistic\n> is that, I wonder?\n\nAs mentioned above, the inference is not path-based; it relies only\nupon matching up the . So this `mv orig moved` is just a\nconvenient way to relocate both the repository and worktree; it is not\nintended to convey any meaning about the inference mechanism. However,\nto avoid potentially misleading readers, I can revise the test to work\nwith distinct paths and manipulate them separately.\n\n> In any case, I think the problem description in the proposed log\n> message is quite well written but the solution seems unclear.\n\nI'll update the commit message to talk about the -based inference.\n"},{"id":"412501","messageId":"xmqqwnxgxlfi.fsf@gitster.c.googlers.com","threadId":"54793","inReplyTo":"CAPig+cQT2Cj4i=2kq_svMyKiFmNLxvpa5kG=XNeaT+8UD5238Q@mail.gmail.com","subject":"Re: [PATCH] worktree: teach `repair` to fix multi-directional breakage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-17T19:49:21Z","receivedAt":"2020-12-17T19:50:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> We only have the paths making the two-way link between the repository\n> and the secondary worktrees. Specifically, the outgoing\n> /worktrees//gitdir points at /path/to/worktree/.git, and the\n> incoming /path/to/worktree/.git points at /worktrees/.\n\nYes.\n\n> The inference is intentionally simple-minded. There is no path-based\n> inference or other heuristic at play; the entire inference is based\n> upon . The worktree's path is specified as an argument. `git\n> worktree repair` manually reads the \".git\" gitfile at that location\n> and, if it is well-formed, extracts the . It then searches for a\n> corresponding in /worktrees/ and,...\n\nThat is exactly the point I got confused. The worktree's path comes\nas an argument from the user, so we'd trust it. And it has \".git\"\nthat is a gitfile that used to point at but because we are\ntrying to deal with a situation where both worktree and repo moved,\nit cannot be used to learn where is. Then, even after\nlearning what to use, how would we know where to use that \nto find /worktrees/, when the location of is unknown?\n\nI think the answer is \"where the user starts the 'git worktree'\ncommand has to be under control of some repository (perhaps found by\na call to setup_git_directory()), and we'd use that one as \".\n\nSince I did not see that (in hindsight an obvious) piece, I failed\nto see how it could possibly work. But now it is clear.\n\nThanks.\n"},{"id":"412502","messageId":"CAPig+cTpdp4fbzBo9ZWt85O_6e7q5br7CnsBp_ZwJLo02LOjcw@mail.gmail.com","threadId":"54793","inReplyTo":"xmqqwnxgxlfi.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] worktree: teach `repair` to fix multi-directional breakage","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-17T20:02:57Z","receivedAt":"2020-12-17T20:03:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Dec 17, 2020 at 2:49 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > The inference is intentionally simple-minded. There is no path-based\n> > inference or other heuristic at play; the entire inference is based\n> > upon . The worktree's path is specified as an argument. `git\n> > worktree repair` manually reads the \".git\" gitfile at that location\n> > and, if it is well-formed, extracts the . It then searches for a\n> > corresponding in /worktrees/ and,...\n>\n> That is exactly the point I got confused. The worktree's path comes\n> as an argument from the user, so we'd trust it. And it has \".git\"\n> that is a gitfile that used to point at but because we are\n> trying to deal with a situation where both worktree and repo moved,\n> it cannot be used to learn where is. Then, even after\n> learning what to use, how would we know where to use that \n> to find /worktrees/, when the location of is unknown?\n>\n> I think the answer is \"where the user starts the 'git worktree'\n> command has to be under control of some repository (perhaps found by\n> a call to setup_git_directory()), and we'd use that one as \".\n\nCorrect. This is why the documentation update:\n\n If both the main working tree and linked working trees have been\n moved manually, then running `repair` in the main working tree and\n specifying the new `` of each linked working tree will\n reestablish all connections in both directions.\n\nsays explicitly that `git worktree repair` must be run in the main\nworktree for this particular case. (For a bare repository, the command\nwould be run in the bare repository instead, but I omitted that bit to\navoid bogging down the documentation, and because the couple preceding\nparagraphs already mention the bare repository case, so I figured the\nreader would understand.)\n\nI could also have mentioned in the commit message the requirement of\nrunning `git worktree repair` in the main worktree (or bare repo), but\ndidn't want to repeat what the patch itself was already saying. But I\nthink I'll update the commit message to mention it when re-rolling\nsince it's important information for understanding how the repair\nworks.\n"},{"id":"412710","messageId":"20201221081601.55546-1-sunshine@sunshineco.com","threadId":"54793","inReplyTo":"20201208173705.5770-1-sunshine@sunshineco.com","subject":"[PATCH v2 0/1] teach `worktree repair` to fix two-way linkage","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-21T08:16:00Z","receivedAt":"2020-12-21T08:18:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"This is a re-roll of [1] which addresses a limitation of `git worktree\nrepair` in which it is unable to repair the two-way links between the\nrepository and its secondary worktrees if both the repository and the\nworktrees have been moved manually.\n\nThe primary change in v2 is that the commit message has been expanded\nquite a bit to explain how the new repair mechanism works, rather than\nexpecting the reader to glean the necessary information from the patch\nitself, as well as to express the reasoning behind a couple changes\nwhich might not be obvious at first glance.\n\nThe patch itself is unchanged with the exception of two minor\nalterations to one of the two added tests. First, anchoring has been\nadded to a couple `sed` expressions. Second, the rename of the main and\nsecondary worktrees is now done with two distinct `mv` commands rather\nthan a single \"clever\" `mv` so as to avoid giving the reader the false\nimpression that the cleverness implies something about how the repair\nmechanism works (it doesn't imply anything, it was just done as a\nconvenience in v1).\n\n[1]: https://lore.kernel.org/git/20201208173705.5770-1-sunshine@sunshineco.com/\n\nEric Sunshine (1):\n worktree: teach `repair` to fix multi-directional breakage\n\n Documentation/git-worktree.txt | 5 +++++\n builtin/worktree.c | 2 +-\n t/t2406-worktree-repair.sh | 26 +++++++++++++++++++++\n worktree.c | 41 ++++++++++++++++++++++++++++++++++\n 4 files changed, 73 insertions(+), 1 deletion(-)\n\nRange-diff against v1:\n1: 956e61bbc8 ! 1: cd38528672 worktree: teach `repair` to fix multi-directional breakage\n @@ Commit message\n Fix these shortcomings by teaching `repair` to attempt to infer the new\n location of the /worktrees//gitdir file when the location\n recorded in the worktree's gitfile has become stale but the file is\n - otherwise well-formed.\n + otherwise well-formed. The inference is intentionally simple-minded.\n + For each worktree path specified as an argument, `git worktree repair`\n + manually reads the \".git\" gitfile at that location and, if it is\n + well-formed, extracts the . It then searches for a corresponding\n + in /worktrees/ and, if found, concludes that there is a\n + reasonable match and updates /worktrees//gitdir to point at\n + the specified worktree path. In order for to be known, `git\n + worktree repair` must be run in the main worktree or bare repository.\n +\n + `git worktree repair` first attempts to repair each incoming\n + /path/to/worktree/.git gitfile to point at the repository, and then\n + attempts to repair outgoing /worktrees//gitdir files to point\n + at the worktrees. This sequence was chosen arbitrarily when originally\n + implemented since the order of fixes is immaterial as long as one side\n + of the two-way link between the repository and a worktree is sound.\n + However, for this new repair technique to work, the order must be\n + reversed. This is because the new inference mechanism, when it is\n + successful, allows the outgoing /worktrees//gitdir file to be\n + repaired, thus fixing one side of the two-way link. Once that side is\n + fixed, the other side can be fixed by the existing repair mechanism,\n + hence the order of repairs is now significant.\n +\n + Two safeguards are employed to avoid hijacking a worktree from a\n + different repository if the user accidentally specifies a foreign\n + worktree as an argument. The first, as described above, is that it\n + requires an match between the repository and the worktree. That\n + itself is not foolproof for preventing hijack, so the second safeguard\n + is that the inference will only kick in if the worktree's\n + /path/to/worktree/.git gitfile does not point at a repository.\n \n Signed-off-by: Eric Sunshine \n \n @@ t/t2406-worktree-repair.sh: test_expect_success 'repair multiple gitdir files' '\n '\n \n +test_expect_success 'repair moved main and linked worktrees' '\n -+\ttest_when_finished \"rm -rf orig moved\" &&\n -+\ttest_create_repo orig/main &&\n -+\ttest_commit -C orig/main init &&\n -+\tgit -C orig/main worktree add --detach ../side &&\n -+\tsed s,orig/side/\\.git,moved/side/.git, \\\n -+\t\torig/main/.git/worktrees/side/gitdir >expect-gitdir &&\n -+\tsed s,orig/main/.git/worktrees/side,moved/main/.git/worktrees/side, \\\n -+\t\torig/side/.git >expect-gitfile &&\n -+\tmv orig moved &&\n -+\tgit -C moved/main worktree repair ../side &&\n -+\ttest_cmp expect-gitdir moved/main/.git/worktrees/side/gitdir &&\n -+\ttest_cmp expect-gitfile moved/side/.git\n ++\ttest_when_finished \"rm -rf main side mainmoved sidemoved\" &&\n ++\ttest_create_repo main &&\n ++\ttest_commit -C main init &&\n ++\tgit -C main worktree add --detach ../side &&\n ++\tsed \"s,side/\\.git$,sidemoved/.git,\" \\\n ++\t\tmain/.git/worktrees/side/gitdir >expect-gitdir &&\n ++\tsed \"s,main/.git/worktrees/side$,mainmoved/.git/worktrees/side,\" \\\n ++\t\tside/.git >expect-gitfile &&\n ++\tmv main mainmoved &&\n ++\tmv side sidemoved &&\n ++\tgit -C mainmoved worktree repair ../sidemoved &&\n ++\ttest_cmp expect-gitdir mainmoved/.git/worktrees/side/gitdir &&\n ++\ttest_cmp expect-gitfile sidemoved/.git\n +'\n +\n test_done\n-- \n2.30.0.rc1.243.g5298b911bd\n\n"},{"id":"412711","messageId":"20201221081601.55546-2-sunshine@sunshineco.com","threadId":"54793","inReplyTo":"20201221081601.55546-1-sunshine@sunshineco.com","subject":"[PATCH v2 1/1] worktree: teach `repair` to fix multi-directional breakage","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-21T08:16:01Z","receivedAt":"2020-12-21T08:18:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"`git worktree repair` knows how to repair the two-way links between the\nrepository and a worktree as long as a link in one or the other\ndirection is sound. For instance, if a linked worktree is moved (without\nusing `git worktree move`), repair is possible because the worktree\nstill knows the location of the repository even though the repository no\nlonger knows where the worktree is. Similarly, if the repository is\nmoved, repair is possible since the repository still knows the locations\nof the worktrees even though the worktrees no longer know where the\nrepository is.\n\nHowever, if both the repository and the worktrees are moved, then links\nare severed in both directions, and no repair is possible. This is the\ncase even when the new worktree locations are specified as arguments to\n`git worktree repair`. The reason for this limitation is twofold. First,\nwhen `repair` consults the worktree's gitfile (/path/to/worktree/.git)\nto determine the corresponding /worktrees//gitdir file to fix,\n is the old path to the repository, thus it is unable to fix the\n`gitdir` file at its new location since it doesn't know where it is.\nSecond, when `repair` consults /worktrees//gitdir to find the\nlocation of the worktree's gitfile (/path/to/worktree/.git), the path\nrecorded in `gitdir` is the old location of the worktree's gitfile, thus\nit is unable to repair the gitfile since it doesn't know where it is.\n\nFix these shortcomings by teaching `repair` to attempt to infer the new\nlocation of the /worktrees//gitdir file when the location\nrecorded in the worktree's gitfile has become stale but the file is\notherwise well-formed. The inference is intentionally simple-minded.\nFor each worktree path specified as an argument, `git worktree repair`\nmanually reads the \".git\" gitfile at that location and, if it is\nwell-formed, extracts the . It then searches for a corresponding\n in /worktrees/ and, if found, concludes that there is a\nreasonable match and updates /worktrees//gitdir to point at\nthe specified worktree path. In order for to be known, `git\nworktree repair` must be run in the main worktree or bare repository.\n\n`git worktree repair` first attempts to repair each incoming\n/path/to/worktree/.git gitfile to point at the repository, and then\nattempts to repair outgoing /worktrees//gitdir files to point\nat the worktrees. This sequence was chosen arbitrarily when originally\nimplemented since the order of fixes is immaterial as long as one side\nof the two-way link between the repository and a worktree is sound.\nHowever, for this new repair technique to work, the order must be\nreversed. This is because the new inference mechanism, when it is\nsuccessful, allows the outgoing /worktrees//gitdir file to be\nrepaired, thus fixing one side of the two-way link. Once that side is\nfixed, the other side can be fixed by the existing repair mechanism,\nhence the order of repairs is now significant.\n\nTwo safeguards are employed to avoid hijacking a worktree from a\ndifferent repository if the user accidentally specifies a foreign\nworktree as an argument. The first, as described above, is that it\nrequires an match between the repository and the worktree. That\nitself is not foolproof for preventing hijack, so the second safeguard\nis that the inference will only kick in if the worktree's\n/path/to/worktree/.git gitfile does not point at a repository.\n\nSigned-off-by: Eric Sunshine \n---\n Documentation/git-worktree.txt | 5 +++++\n builtin/worktree.c | 2 +-\n t/t2406-worktree-repair.sh | 26 +++++++++++++++++++++\n worktree.c | 41 ++++++++++++++++++++++++++++++++++\n 4 files changed, 73 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex af06128cc9..02a706c4c0 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -143,6 +143,11 @@ locate it. Running `repair` within the recently-moved working tree will\n reestablish the connection. If multiple linked working trees are moved,\n running `repair` from any working tree with each tree's new `` as\n an argument, will reestablish the connection to all the specified paths.\n++\n+If both the main working tree and linked working trees have been moved\n+manually, then running `repair` in the main working tree and specifying the\n+new `` of each linked working tree will reestablish all connections\n+in both directions.\n \n unlock::\n \ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 197fd24a55..71287b2da6 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -1052,10 +1052,10 @@ static int repair(int ac, const char **av, const char *prefix)\n \tint rc = 0;\n \n \tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n-\trepair_worktrees(report_repair, &rc);\n \tp = ac > 0 ? av : self;\n \tfor (; *p; p++)\n \t\trepair_worktree_at_path(*p, report_repair, &rc);\n+\trepair_worktrees(report_repair, &rc);\n \treturn rc;\n }\n \ndiff --git a/t/t2406-worktree-repair.sh b/t/t2406-worktree-repair.sh\nindex 1fe468bfe8..f73741886b 100755\n--- a/t/t2406-worktree-repair.sh\n+++ b/t/t2406-worktree-repair.sh\n@@ -104,6 +104,16 @@ test_expect_success 'repo not found; .git not file' '\n \ttest_i18ngrep \".git is not a file\" err\n '\n \n+test_expect_success 'repo not found; .git not referencing repo' '\n+\ttest_when_finished \"rm -rf side not-a-repo && git worktree prune\" &&\n+\tgit worktree add --detach side &&\n+\tsed s,\\.git/worktrees/side$,not-a-repo, side/.git >side/.newgit &&\n+\tmv side/.newgit side/.git &&\n+\tmkdir not-a-repo &&\n+\ttest_must_fail git worktree repair side 2>err &&\n+\ttest_i18ngrep \".git file does not reference a repository\" err\n+'\n+\n test_expect_success 'repo not found; .git file broken' '\n \ttest_when_finished \"rm -rf orig moved && git worktree prune\" &&\n \tgit worktree add --detach orig &&\n@@ -176,4 +186,20 @@ test_expect_success 'repair multiple gitdir files' '\n \ttest_must_be_empty err\n '\n \n+test_expect_success 'repair moved main and linked worktrees' '\n+\ttest_when_finished \"rm -rf main side mainmoved sidemoved\" &&\n+\ttest_create_repo main &&\n+\ttest_commit -C main init &&\n+\tgit -C main worktree add --detach ../side &&\n+\tsed \"s,side/\\.git$,sidemoved/.git,\" \\\n+\t\tmain/.git/worktrees/side/gitdir >expect-gitdir &&\n+\tsed \"s,main/.git/worktrees/side$,mainmoved/.git/worktrees/side,\" \\\n+\t\tside/.git >expect-gitfile &&\n+\tmv main mainmoved &&\n+\tmv side sidemoved &&\n+\tgit -C mainmoved worktree repair ../sidemoved &&\n+\ttest_cmp expect-gitdir mainmoved/.git/worktrees/side/gitdir &&\n+\ttest_cmp expect-gitfile sidemoved/.git\n+'\n+\n test_done\ndiff --git a/worktree.c b/worktree.c\nindex f84ceae87d..821b233479 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -644,6 +644,42 @@ static int is_main_worktree_path(const char *path)\n \treturn !cmp;\n }\n \n+/*\n+ * If both the main worktree and linked worktree have been moved, then the\n+ * gitfile /path/to/worktree/.git won't point into the repository, thus we\n+ * won't know which /worktrees//gitdir to repair. However, we may\n+ * be able to infer the gitdir by manually reading /path/to/worktree/.git,\n+ * extracting the , and checking if /worktrees/ exists.\n+ */\n+static char *infer_backlink(const char *gitfile)\n+{\n+\tstruct strbuf actual = STRBUF_INIT;\n+\tstruct strbuf inferred = STRBUF_INIT;\n+\tconst char *id;\n+\n+\tif (strbuf_read_file(&actual, gitfile, 0) < 0)\n+\t\tgoto error;\n+\tif (!starts_with(actual.buf, \"gitdir:\"))\n+\t\tgoto error;\n+\tif (!(id = find_last_dir_sep(actual.buf)))\n+\t\tgoto error;\n+\tstrbuf_trim(&actual);\n+\tid++; /* advance past '/' to point at */\n+\tif (!*id)\n+\t\tgoto error;\n+\tstrbuf_git_common_path(&inferred, the_repository, \"worktrees/%s\", id);\n+\tif (!is_directory(inferred.buf))\n+\t\tgoto error;\n+\n+\tstrbuf_release(&actual);\n+\treturn strbuf_detach(&inferred, NULL);\n+\n+error:\n+\tstrbuf_release(&actual);\n+\tstrbuf_release(&inferred);\n+\treturn NULL;\n+}\n+\n /*\n * Repair /worktrees//gitdir if missing, corrupt, or not pointing at\n * the worktree's path.\n@@ -675,6 +711,11 @@ void repair_worktree_at_path(const char *path,\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\t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file does not reference a repository\"), cb_data);\n+\t\t\tgoto done;\n+\t\t}\n \t} else if (err) {\n \t\tfn(1, realdotgit.buf, _(\"unable to locate repository; .git file broken\"), cb_data);\n \t\tgoto done;\n-- \n2.30.0.rc1.243.g5298b911bd\n\n"}]}