Repository navigation
Conversation
yoichi
force-pushed
the
worktree-repair-keep-unrelated-gitfile
branch
from
September 13, 2026 03:13
920147e to
99aa341
Compare
Author
|
/submit |
|
Submitted as pull.2225.git.1789269613.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Yoichi NAKAYAMA wrote on the Git mailing list (how to reply to this email): Friendly ping on this :) |
|
User |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -637,6 +637,14 @@ int other_head_refs(struct repository *repo, | |||
| return ret; | |||
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:
> +static const char *get_worktree_id(const char *dotgit_contents)
> +{
> + const char *slash = find_last_dir_sep(dotgit_contents);
> + if (!slash)
> + return "";
> + return slash + 1;
> +}
This returns a pointer into dotgit_contents; it is the last
component of a pathname, similar to what basename(3) gives us.
> @@ -798,30 +806,20 @@ static int is_main_worktree_path(struct repository *repo, const char *path)
The unified diff is a bit hard to follow, so let's see if we can
compare preimage and postimage more easily.
> static ssize_t infer_backlink(struct repository *repo,
> - const char *gitfile,
> struct strbuf *inferred)
> {
> - struct strbuf actual = STRBUF_INIT;
> const char *id;
>
> - if (strbuf_read_file(&actual, gitfile, 0) < 0)
> - goto error;
> - if (!starts_with(actual.buf, "gitdir:"))
> - goto error;
> - if (!(id = find_last_dir_sep(actual.buf)))
> - goto error;
> - strbuf_trim(&actual);
> - id++; /* advance past '/' to point at <id> */
> if (!*id)
> goto error;
> repo_common_path_replace(repo, inferred, "worktrees/%s", id);
> if (!is_directory(inferred->buf))
> goto error;
>
> - strbuf_release(&actual);
> return inferred->len;
> error:
> - strbuf_release(&actual);
> strbuf_reset(inferred); /* clear invalid path */
> return -1;
We used to receive the filename of ".git", read it and made sure we
have "gitdir:" prefix, and find the last component, but then trimmed
the actual buffer. Which means a few things.
- If the contents of the gitfile were "gitdir:foo/bar/baz \n", our
id pointer found the slash after "foo/bar", trimmed the buffer to
have "gitdir:foo/bar/baz", and then incremented id, which now
points at "baz".
- If the contents of the gitfile were "gitdir: foo/bar/ \n", then
after triming, the buffer would have "gitdir: foo/bar/" and id
would be pointing at the NUL at the end, which would have lead us
to error.
Now let's look at the new code.
> @@ -798,30 +806,20 @@ static int is_main_worktree_path(struct repository *repo, const char *path)
> * Returns -1 on failure and strbuf.len on success.
> */
> static ssize_t infer_backlink(struct repository *repo,
> + const char *dotgit_contents,
> struct strbuf *inferred)
> {
> const char *id;
>
> + id = get_worktree_id(dotgit_contents);
> if (!*id)
> goto error;
> repo_common_path_replace(repo, inferred, "worktrees/%s", id);
> if (!is_directory(inferred->buf))
> goto error;
>
> return inferred->len;
> error:
> strbuf_reset(inferred); /* clear invalid path */
> return -1;
The caller is expected to give us the contents of gitfile read by
setup.c:read_gitfile_raw(), which reads the file in full, validates
that the file begins with "gitdir: " (notice the trailing space),
removes arbitrary run of CR or LF from the end, and then returns
the string after skipping "gitdir: " prefix (8 bytes).
In the normal case, read_gitfile_raw() would see "gitdir: foo/bar/baz\n"
in the file and returns "foo/bar/baz" to our caller. In fishy cases
we examined for the preimage above:
- If the contents of the gitfile were "gitdir:foo/bar/baz \n", our
caller would have received an error from read_gitfile_raw() and
wouldn't have called us.
- If the contents of the gitfile were "gitdir: foo/bar/ \n", our
caller would have given us "foo/bar/ ".
get_worktree_id() will give us "baz" in the normal case, and " "
in the last case. We fail to error out in the latter with "*id"
check, but is_directory() check will catch us, as the inferred
directory is "worktrees/ " in that bad case.
So there are certain differences in error cases, but they behave the
same in the most basic cases.
Now, this is the caller in the preimage (i.e., what we used to do).
> @@ -856,51 +855,49 @@ void repair_worktree_at_path(struct repository *repo,
> goto done;
> }
>
> - infer_backlink(repo, dotgit.buf, &inferred_backlink);
> - strbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);
> - dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
We used to have infer_backlink() read the .git file to compute "worktree/$id",
then again called read_gitfile_gently() to read it again.
> - if (dotgit_contents) {
> - strbuf_addstr(&backlink, dotgit_contents);
This is the happy path. We successfully read from .git and use it.
> - } else if (err == READ_GITFILE_ERR_NOT_A_FILE ||
> - err == READ_GITFILE_ERR_IS_A_DIR) {
> fn(1, dotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
> goto done;
This is inherited badness, but overly long lines like this one needs
to be fixed.
> - } else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
The _gently() did read something, but that does not point at a git
directory.
> - if (inferred_backlink.len) {
> - /*
> - * Worktree's .git file does not point at a repository
> - * but we found a .git/worktrees/<id> in this
> - * repository with the same <id> as recorded in the
> - * worktree's .git file so make the worktree point at
> - * the discovered .git/worktrees/<id>.
> - */
> - strbuf_swap(&backlink, &inferred_backlink);
If we had the "worktree/$id" thing, we use it.
> - } else {
> - fn(1, dotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
> - goto done;
> - }
> - } else {
> fn(1, dotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
> goto done;
> }
These lines to show error messages should also be folded to avoid
overly long lines.
So, what does the updated code in the postimage do?
> @@ -856,51 +855,49 @@ void repair_worktree_at_path(struct repository *repo,
> goto done;
> }
>
> + err = read_gitfile_raw(&contents, dotgit.buf);
We use read_gitfile_raw() just once.
> + if (err == READ_GITFILE_ERR_NOT_A_FILE ||
> + err == READ_GITFILE_ERR_IS_A_DIR) {
> fn(1, dotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
> goto done;
> + } else if (err) {
> fn(1, dotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
> goto done;
> }
The original code handled the happy case that read_gitfile_gently()
successfully returned first. Underlying read_gitfile_raw() would
not have given any of these errors when read_gitfile_gently()
succeeded, so handling the error cases first would not affect the
behaviour of the code in these cases. Again, these overlong lines
are annoying.
Now the simplest error cases are behind us. How would we do in the
happy case?
> + dotgit_contents = contents.buf;
> + infer_backlink(repo, dotgit_contents, &inferred_backlink);
> + strbuf_realpath_forgiving(&inferred_backlink, inferred_backlink.buf, 0);
We reuse what we already read with read_gitfile_raw(), which
prepared "worktrees/$id", and do the same realpath_forgiving()
the original used to do a bit earlier.
> + if (is_absolute_path(dotgit_contents)) {
> + strbuf_addstr(&backlink, dotgit_contents);
I am not sure which part of the original this logic corresponds to.
If the result from read_gitfile_raw() is an absolute path, even if
it later turns out not to be is_git_directory(), the inferred backlink
is not given a chance to act as a fallback. The original made a
call to read_gitfile_gently() which checked is_git_directory() to
give us an error, and that is how it allowed inferred backlink to
substitute for a bad contents stored in .git file. Now we do not
allow that fallback if .git file has an absolute path?
Ah, outside the context of this patch, before we barf for "unable to
locate repository" when we complain backlink.buf is not naming a git
directory, there is the fallback logic, and in order to reach there,
we have "if (!is_git_directory(backlink.buf) && !inferred_backlink.len)"
there. OK, so this may be doing the same thing as the original, but
it is rather hard to follow and convince readers that this is a
no-op conversion.
> + } else {
> + strbuf_addbuf(&backlink, &dotgit);
> + strbuf_strip_suffix(&backlink, ".git");
> + strbuf_addstr(&backlink, dotgit_contents);
> + strbuf_realpath_forgiving(&backlink, backlink.buf, 0);
This converts dotgit_contents relative to the computed backlink,
which needs to be done here because read_gitfile_gently() used to do
that for us, which we no longer use.
> + }
> +
> + if (!is_git_directory(backlink.buf) && !inferred_backlink.len) {
> + fn(1, dotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
> + goto done;
> + }
I'll stop here.There was a problem hiding this comment.
Yoichi NAKAYAMA wrote on the Git mailing list (how to reply to this email):
On Fri, Oct 9, 2026 at 5:55 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > +static const char *get_worktree_id(const char *dotgit_contents)
> > +{
> > + const char *slash = find_last_dir_sep(dotgit_contents);
> > + if (!slash)
> > + return "";
> > + return slash + 1;
> > +}
>
> This returns a pointer into dotgit_contents; it is the last
> component of a pathname, similar to what basename(3) gives us.
>
> > @@ -798,30 +806,20 @@ static int is_main_worktree_path(struct repository *repo, const char *path)
>
> The unified diff is a bit hard to follow, so let's see if we can
> compare preimage and postimage more easily.
>
> > static ssize_t infer_backlink(struct repository *repo,
> > - const char *gitfile,
> > struct strbuf *inferred)
> > {
> > - struct strbuf actual = STRBUF_INIT;
> > const char *id;
> >
> > - if (strbuf_read_file(&actual, gitfile, 0) < 0)
> > - goto error;
> > - if (!starts_with(actual.buf, "gitdir:"))
> > - goto error;
> > - if (!(id = find_last_dir_sep(actual.buf)))
> > - goto error;
> > - strbuf_trim(&actual);
> > - id++; /* advance past '/' to point at <id> */
> > if (!*id)
> > goto error;
> > repo_common_path_replace(repo, inferred, "worktrees/%s", id);
> > if (!is_directory(inferred->buf))
> > goto error;
> >
> > - strbuf_release(&actual);
> > return inferred->len;
> > error:
> > - strbuf_release(&actual);
> > strbuf_reset(inferred); /* clear invalid path */
> > return -1;
>
> We used to receive the filename of ".git", read it and made sure we
> have "gitdir:" prefix, and find the last component, but then trimmed
> the actual buffer. Which means a few things.
>
> - If the contents of the gitfile were "gitdir:foo/bar/baz \n", our
> id pointer found the slash after "foo/bar", trimmed the buffer to
> have "gitdir:foo/bar/baz", and then incremented id, which now
> points at "baz".
>
> - If the contents of the gitfile were "gitdir: foo/bar/ \n", then
> after triming, the buffer would have "gitdir: foo/bar/" and id
> would be pointing at the NUL at the end, which would have lead us
> to error.
>
> Now let's look at the new code.
>
> > @@ -798,30 +806,20 @@ static int is_main_worktree_path(struct repository *repo, const char *path)
> > * Returns -1 on failure and strbuf.len on success.
> > */
> > static ssize_t infer_backlink(struct repository *repo,
> > + const char *dotgit_contents,
> > struct strbuf *inferred)
> > {
> > const char *id;
> >
> > + id = get_worktree_id(dotgit_contents);
> > if (!*id)
> > goto error;
> > repo_common_path_replace(repo, inferred, "worktrees/%s", id);
> > if (!is_directory(inferred->buf))
> > goto error;
> >
> > return inferred->len;
> > error:
> > strbuf_reset(inferred); /* clear invalid path */
> > return -1;
>
> The caller is expected to give us the contents of gitfile read by
> setup.c:read_gitfile_raw(), which reads the file in full, validates
> that the file begins with "gitdir: " (notice the trailing space),
> removes arbitrary run of CR or LF from the end, and then returns
> the string after skipping "gitdir: " prefix (8 bytes).
>
> In the normal case, read_gitfile_raw() would see "gitdir: foo/bar/baz\n"
> in the file and returns "foo/bar/baz" to our caller. In fishy cases
> we examined for the preimage above:
>
> - If the contents of the gitfile were "gitdir:foo/bar/baz \n", our
> caller would have received an error from read_gitfile_raw() and
> wouldn't have called us.
>
> - If the contents of the gitfile were "gitdir: foo/bar/ \n", our
> caller would have given us "foo/bar/ ".
>
> get_worktree_id() will give us "baz" in the normal case, and " "
> in the last case. We fail to error out in the latter with "*id"
> check, but is_directory() check will catch us, as the inferred
> directory is "worktrees/ " in that bad case.
>
> So there are certain differences in error cases, but they behave the
> same in the most basic cases.
infer_backlink() in the preimage accepts the "gitdir:" prefix without requiring
a following space and uses strbuf_trim(), which does more than just remove
trailing newlines. This is more lenient than `read_gitfile_gently()`, which is
called when other Git commands are used.
I believe the primary purpose of `worktree repair` is to fix common issues
like worktree relocation, with repairing cases where the `.git` file's contents
were manually modified being a secondary concern.
Therefore, I have adjusted the behavior for error cases: if the file deviates
from the standard `.git` file format, it is treated as ".git file
broken"; if there
are trailing whitespaces, they are interpreted as part of the path and treated
as ".git file does not reference a repository."
I would like to split the commits and provide a detailed explanation for this.
> > - } else if (err == READ_GITFILE_ERR_NOT_A_FILE ||
> > - err == READ_GITFILE_ERR_IS_A_DIR) {
> > fn(1, dotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
> > goto done;
>
> This is inherited badness, but overly long lines like this one needs
> to be fixed.
I'll fold long lines near the changes.
> Ah, outside the context of this patch, before we barf for "unable to
> locate repository" when we complain backlink.buf is not naming a git
> directory, there is the fallback logic, and in order to reach there,
> we have "if (!is_git_directory(backlink.buf) && !inferred_backlink.len)"
> there. OK, so this may be doing the same thing as the original, but
> it is rather hard to follow and convince readers that this is a
> no-op conversion.
The first commit contained multiple changes, making it difficult to follow.
I will split the commit to distinguish between the parts where the behavior
is modified and those where it remains unchanged, making it easier to
verify that no changes were made to the latter.
Thanks,
--
Yoichi NAKAYAMAThere was a problem hiding this comment.
Yoichi NAKAYAMA wrote on the Git mailing list (how to reply to this email):
On Mon, Oct 12, 2026 at 1:16 AM Yoichi NAKAYAMA
<yoichi.nakayama@gmail.com> wrote:
>
> On Fri, Oct 9, 2026 at 5:55 AM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > "Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:
> >
> > > +static const char *get_worktree_id(const char *dotgit_contents)
> > > +{
> > > + const char *slash = find_last_dir_sep(dotgit_contents);
> > > + if (!slash)
> > > + return "";
> > > + return slash + 1;
> > > +}
> >
> > This returns a pointer into dotgit_contents; it is the last
> > component of a pathname, similar to what basename(3) gives us.
> >
> > > @@ -798,30 +806,20 @@ static int is_main_worktree_path(struct repository *repo, const char *path)
> >
> > The unified diff is a bit hard to follow, so let's see if we can
> > compare preimage and postimage more easily.
> >
> > > static ssize_t infer_backlink(struct repository *repo,
> > > - const char *gitfile,
> > > struct strbuf *inferred)
> > > {
> > > - struct strbuf actual = STRBUF_INIT;
> > > const char *id;
> > >
> > > - if (strbuf_read_file(&actual, gitfile, 0) < 0)
> > > - goto error;
> > > - if (!starts_with(actual.buf, "gitdir:"))
> > > - goto error;
> > > - if (!(id = find_last_dir_sep(actual.buf)))
> > > - goto error;
> > > - strbuf_trim(&actual);
> > > - id++; /* advance past '/' to point at <id> */
> > > if (!*id)
> > > goto error;
> > > repo_common_path_replace(repo, inferred, "worktrees/%s", id);
> > > if (!is_directory(inferred->buf))
> > > goto error;
> > >
> > > - strbuf_release(&actual);
> > > return inferred->len;
> > > error:
> > > - strbuf_release(&actual);
> > > strbuf_reset(inferred); /* clear invalid path */
> > > return -1;
> >
> > We used to receive the filename of ".git", read it and made sure we
> > have "gitdir:" prefix, and find the last component, but then trimmed
> > the actual buffer. Which means a few things.
> >
> > - If the contents of the gitfile were "gitdir:foo/bar/baz \n", our
> > id pointer found the slash after "foo/bar", trimmed the buffer to
> > have "gitdir:foo/bar/baz", and then incremented id, which now
> > points at "baz".
> >
> > - If the contents of the gitfile were "gitdir: foo/bar/ \n", then
> > after triming, the buffer would have "gitdir: foo/bar/" and id
> > would be pointing at the NUL at the end, which would have lead us
> > to error.
> >
> > Now let's look at the new code.
> >
> > > @@ -798,30 +806,20 @@ static int is_main_worktree_path(struct repository *repo, const char *path)
> > > * Returns -1 on failure and strbuf.len on success.
> > > */
> > > static ssize_t infer_backlink(struct repository *repo,
> > > + const char *dotgit_contents,
> > > struct strbuf *inferred)
> > > {
> > > const char *id;
> > >
> > > + id = get_worktree_id(dotgit_contents);
> > > if (!*id)
> > > goto error;
> > > repo_common_path_replace(repo, inferred, "worktrees/%s", id);
> > > if (!is_directory(inferred->buf))
> > > goto error;
> > >
> > > return inferred->len;
> > > error:
> > > strbuf_reset(inferred); /* clear invalid path */
> > > return -1;
> >
> > The caller is expected to give us the contents of gitfile read by
> > setup.c:read_gitfile_raw(), which reads the file in full, validates
> > that the file begins with "gitdir: " (notice the trailing space),
> > removes arbitrary run of CR or LF from the end, and then returns
> > the string after skipping "gitdir: " prefix (8 bytes).
> >
> > In the normal case, read_gitfile_raw() would see "gitdir: foo/bar/baz\n"
> > in the file and returns "foo/bar/baz" to our caller. In fishy cases
> > we examined for the preimage above:
> >
> > - If the contents of the gitfile were "gitdir:foo/bar/baz \n", our
> > caller would have received an error from read_gitfile_raw() and
> > wouldn't have called us.
> >
> > - If the contents of the gitfile were "gitdir: foo/bar/ \n", our
> > caller would have given us "foo/bar/ ".
> >
> > get_worktree_id() will give us "baz" in the normal case, and " "
> > in the last case. We fail to error out in the latter with "*id"
> > check, but is_directory() check will catch us, as the inferred
> > directory is "worktrees/ " in that bad case.
> >
> > So there are certain differences in error cases, but they behave the
> > same in the most basic cases.
>
> infer_backlink() in the preimage accepts the "gitdir:" prefix without requiring
> a following space and uses strbuf_trim(), which does more than just remove
> trailing newlines. This is more lenient than `read_gitfile_gently()`, which is
> called when other Git commands are used.
> I believe the primary purpose of `worktree repair` is to fix common issues
> like worktree relocation, with repairing cases where the `.git` file's contents
> were manually modified being a secondary concern.
> Therefore, I have adjusted the behavior for error cases: if the file deviates
> from the standard `.git` file format, it is treated as ".git file
> broken"; if there
> are trailing whitespaces, they are interpreted as part of the path and treated
> as ".git file does not reference a repository."
The former case (where there is no space after "gitdir:") behaved this way
even before the change. repair_worktree_at_path() calls read_gitfile_gently()
and returns `err = READ_GITFILE_ERR_INVALID_FORMAT`; this behavior
remains unchanged.
Thanks,
--
Yoichi NAKAYAMA
Author
|
On Mon, Oct 12, 2026 at 3:26 AM gitgitgadget[bot]
***@***.***> wrote:
@gitgitgadget[bot] commented on this pull request.
________________________________
In worktree.c:
> @@ -637,6 +637,14 @@ int other_head_refs(struct repository *repo,
return ret;
Yoichi NAKAYAMA wrote on the Git mailing list (how to reply to this email):
On Fri, Oct 9, 2026 at 5:55 AM Junio C Hamano ***@***.***> wrote:
>
> "Yoichi NAKAYAMA via GitGitGadget" ***@***.***> writes:
>
> > +static const char *get_worktree_id(const char *dotgit_contents)
> > +{
> > + const char *slash = find_last_dir_sep(dotgit_contents);
> > + if (!slash)
> > + return "";
> > + return slash + 1;
> > +}
>
> This returns a pointer into dotgit_contents; it is the last
> component of a pathname, similar to what basename(3) gives us.
>
> > @@ -798,30 +806,20 @@ static int is_main_worktree_path(struct repository *repo, const char *path)
>
> The unified diff is a bit hard to follow, so let's see if we can
> compare preimage and postimage more easily.
>
> > static ssize_t infer_backlink(struct repository *repo,
> > - const char *gitfile,
> > struct strbuf *inferred)
> > {
> > - struct strbuf actual = STRBUF_INIT;
> > const char *id;
> >
> > - if (strbuf_read_file(&actual, gitfile, 0) < 0)
> > - goto error;
> > - if (!starts_with(actual.buf, "gitdir:"))
> > - goto error;
> > - if (!(id = find_last_dir_sep(actual.buf)))
> > - goto error;
> > - strbuf_trim(&actual);
> > - id++; /* advance past '/' to point at <id> */
> > if (!*id)
> > goto error;
> > repo_common_path_replace(repo, inferred, "worktrees/%s", id);
> > if (!is_directory(inferred->buf))
> > goto error;
> >
> > - strbuf_release(&actual);
> > return inferred->len;
> > error:
> > - strbuf_release(&actual);
> > strbuf_reset(inferred); /* clear invalid path */
> > return -1;
>
> We used to receive the filename of ".git", read it and made sure we
> have "gitdir:" prefix, and find the last component, but then trimmed
> the actual buffer. Which means a few things.
>
> - If the contents of the gitfile were "gitdir:foo/bar/baz \n", our
> id pointer found the slash after "foo/bar", trimmed the buffer to
> have "gitdir:foo/bar/baz", and then incremented id, which now
> points at "baz".
>
> - If the contents of the gitfile were "gitdir: foo/bar/ \n", then
> after triming, the buffer would have "gitdir: foo/bar/" and id
> would be pointing at the NUL at the end, which would have lead us
> to error.
>
> Now let's look at the new code.
>
> > @@ -798,30 +806,20 @@ static int is_main_worktree_path(struct repository *repo, const char *path)
> > * Returns -1 on failure and strbuf.len on success.
> > */
> > static ssize_t infer_backlink(struct repository *repo,
> > + const char *dotgit_contents,
> > struct strbuf *inferred)
> > {
> > const char *id;
> >
> > + id = get_worktree_id(dotgit_contents);
> > if (!*id)
> > goto error;
> > repo_common_path_replace(repo, inferred, "worktrees/%s", id);
> > if (!is_directory(inferred->buf))
> > goto error;
> >
> > return inferred->len;
> > error:
> > strbuf_reset(inferred); /* clear invalid path */
> > return -1;
>
> The caller is expected to give us the contents of gitfile read by
> setup.c:read_gitfile_raw(), which reads the file in full, validates
> that the file begins with "gitdir: " (notice the trailing space),
> removes arbitrary run of CR or LF from the end, and then returns
> the string after skipping "gitdir: " prefix (8 bytes).
>
> In the normal case, read_gitfile_raw() would see "gitdir: foo/bar/baz\n"
> in the file and returns "foo/bar/baz" to our caller. In fishy cases
> we examined for the preimage above:
>
> - If the contents of the gitfile were "gitdir:foo/bar/baz \n", our
> caller would have received an error from read_gitfile_raw() and
> wouldn't have called us.
>
> - If the contents of the gitfile were "gitdir: foo/bar/ \n", our
> caller would have given us "foo/bar/ ".
>
> get_worktree_id() will give us "baz" in the normal case, and " "
> in the last case. We fail to error out in the latter with "*id"
> check, but is_directory() check will catch us, as the inferred
> directory is "worktrees/ " in that bad case.
>
> So there are certain differences in error cases, but they behave the
> same in the most basic cases.
infer_backlink() in the preimage accepts the "gitdir:" prefix without requiring
a following space and uses strbuf_trim(), which does more than just remove
trailing newlines. This is more lenient than `read_gitfile_gently()`, which is
called when other Git commands are used.
I believe the primary purpose of `worktree repair` is to fix common issues
like worktree relocation, with repairing cases where the `.git` file's contents
were manually modified being a secondary concern.
Therefore, I have adjusted the behavior for error cases: if the file deviates
from the standard `.git` file format, it is treated as ".git file
broken"; if there
are trailing whitespaces, they are interpreted as part of the path and treated
as ".git file does not reference a repository."
I would like to split the commits and provide a detailed explanation for this.
The former case (where there is no space after "gitdir:") behaved this way
even before the change. repair_worktree_at_path() calls read_gitfile_gently()
and returns `err = READ_GITFILE_ERR_INVALID_FORMAT`; this behavior
remains unchanged.
Thanks,
--
Yoichi NAKAYAMA
|
Member
|
@yoichi please reply on the list! |
yoichi
force-pushed
the
worktree-repair-keep-unrelated-gitfile
branch
2 times, most recently
from
October 13, 2026 14:38
70fb7dd to
01308d3
Compare
Use read_gitfile_raw() instead of read_gitfile_gently() in repair_worktree_at_path(), moving path resolution and repository validation to the caller. This makes .git file handling consistent with repair_gitfile() and avoids using the non-reentrant read_gitfile_gently(). A subsequent commit will update infer_backlink() to use read_gitfile_raw() as well. Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
Reorder the processing steps to centralize the logic for preferring the inferred_backlink. Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
Use read_gitfile_raw() in infer_backlink() to parse .git files consistently with repair_gitfile(). Unlike strbuf_trim(), read_gitfile_raw() does not silently discard trailing whitespace. Such whitespace is treated as part of the path and may cause the path to be treated as not referencing a repository. Extract get_worktree_id() for use in a subsequent commit. Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
Pass the loaded contents of the .git file to infer_backlink(). Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
Currently, `repair_gitfile()` does not verify whether the worktree ID recorded in the .git file matches the worktree being repaired, which can result in an unrelated .git file being corrupted. For instance, if two worktree directories are swapped without using 'git worktree move', running 'git worktree repair' in the main worktree accidentally swaps the links between their .git files and gitdirs. `repair_worktree_at_path()` proceeds even if it fails to infer the gitdir path. This can result in the corruption of an unrelated gitdir. For instance, if we copied a linked worktree to a new location X, running 'git worktree repair X' in a working tree which does not belong to the original repository can accidentally overwrite the gitdir in the original repository (the scope of impact should be limited to the repository where the command was executed). Resolve these issues by validating the worktree ID and stopping the repair when the ID does not match or the gitdir path cannot be inferred. Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com>
yoichi
force-pushed
the
worktree-repair-keep-unrelated-gitfile
branch
from
October 14, 2026 21:07
01308d3 to
0156021
Compare
Author
|
/submit |
|
Submitted as pull.2225.v2.git.1792012836.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 'git worktree repair' does not sufficiently validate the cross-references
> between a linked working tree and its administrative data before repairing
> them. This can cause the repair to modify the wrong .git file or gitdir in
> certain situations.
>
>
> v2 (0156021)
> ============
>
> Split the first commit of v1 to make the changes easier to understand. Fold
> long lines.
>
> * [1/5] align .git file handling with repair_gitfile()
> * [2/5] centralize inferred backlink selection
> * [3/5] use read_gitfile_raw() in infer_backlink()
> * [4/5] reduce .git file reads
> * [5/5] avoid breaking unrelated .git file and gitdir
Makes more sense to have this series as a logical progression of
smaller pieces compared to v1.
What is "0156021" by the way?
Thanks.
> t/t2406-worktree-repair.sh | 33 +++++++++--
> worktree.c | 113 +++++++++++++++++++++----------------
> 2 files changed, 91 insertions(+), 55 deletions(-)
|
|
Yoichi NAKAYAMA wrote on the Git mailing list (how to reply to this email): Sorry, I sent the previous email in HTML. I'm resending it as plain text.
On Thu, Oct 15, 2026 at 7:46 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > 'git worktree repair' does not sufficiently validate the cross-references
> > between a linked working tree and its administrative data before repairing
> > them. This can cause the repair to modify the wrong .git file or gitdir in
> > certain situations.
> >
> >
> > v2 (0156021)
> > ============
> >
> > Split the first commit of v1 to make the changes easier to understand. Fold
> > long lines.
> >
> > * [1/5] align .git file handling with repair_gitfile()
> > * [2/5] centralize inferred backlink selection
> > * [3/5] use read_gitfile_raw() in infer_backlink()
> > * [4/5] reduce .git file reads
> > * [5/5] avoid breaking unrelated .git file and gitdir
>
> Makes more sense to have this series as a logical progression of
> smaller pieces compared to v1.
>
> What is "0156021" by the way?
It is the first 7 characters of the commit hash
in https://github.com/gitgitgadget/git/
(Creating a commit from an email changes the
commit hash, so the identifier was meaningless.)
Thanks,
--
Yoichi NAKAYAMA |
| @@ -56,15 +56,12 @@ test_expect_success 'repair missing .git file' ' | |||
| ' | |||
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"Yoichi NAKAYAMA via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/worktree.c b/worktree.c
> index 879941a977..86094307b1 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -640,7 +640,11 @@ int other_head_refs(struct repository *repo,
> static const char *get_worktree_id(const char *dotgit_contents)
> {
> const char *slash = find_last_dir_sep(dotgit_contents);
> - if (!slash)
> + const char *prefix = "/worktrees";
> + int prefixlen = strlen(prefix);
> + if (!slash ||
> + slash - dotgit_contents < prefixlen ||
> + strncmp(slash - prefixlen, prefix, prefixlen))
> return "";
> return slash + 1;
Just being curious, but slash is found by scanning dotgit_contents
for either '/' or '\'. We make sure what comes before the slash
is "/worktree".
Could there be situations where this code needs to accept
"...\worktrees\my-worktree"
where slash points at the backslash before "my-worktree"? Because
somebody wrote the pathname with backslash before the worktree name,
it would be natural to expect another backslash before the "worktrees"
constant, no? For that matter, would this code need to also accept
".../worktrees//my-worktree"
where three are extra/duplicated slashes (or backslashes)? The
normal code paths to create these directories and records the path
to these directories should hopefully be using the canonical '/'
without unnecessary duplicate slashes, but (1) existing use of
find_last_dir_sep() might be a sign that we may have to deal with
backslashes, and (2) since this is about reparing a broken set-up,
it does not seem too far-fetched to attempt dealing with the above
two sample input, hence this question.
Thanks.
|
This branch is now known as |
|
This patch series was integrated into seen via git@b2f602b. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
'git worktree repair' does not sufficiently validate the cross-references
between a linked working tree and its administrative data before
repairing them. This can cause the repair to modify the wrong .git file
or gitdir in certain situations.
v2 (0156021)
Split the first commit of v1 to make the changes easier to understand.
Fold long lines.
v1 (99aa341)
This series first refactors the code to read the .git file once and extract
the worktree ID, then uses that information to validate the repair target
before modifying the cross-references.
cc: Eric Sunshine sunshine@sunshineco.com
cc: Yoichi NAKAYAMA yoichi.nakayama@gmail.com