| Message ID | 20241007-wt_relative_paths-v3-1-622cf18c45eb@pm.me (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Link worktrees with relative paths | expand |
On Mon, Oct 07, 2024 at 10:12:30PM -0500, Caleb White via B4 Relay wrote: > From: Caleb White <cdwhite3@pm.me> > > This lays the groundwork for the next patch, which needs the backlink > returned from infer_backlink() as a `strbuf`. It seemed inefficient to > convert from `strbuf` to `char*` and back to `strbuf` again. > I think here we should first talk about the current behavior of the `infer_backlink`: `infer_backlink` initializes a "strbuf" for the inferred backlink and returns the result with type "char *" by detaching the "strbuf" And then you should tell your intention like the following (The reader like me does not know what is the intention of the next patch, you should __explicitly__ mention this). Because we decide to link worktrees with relative paths, we need to convert the returned inferred backlink "char *" to "strbuf". However, it is a bad idea to convert from `strbuf` to `char*` and back to `strbuf` again. Instead, we should just let the `infer_backlink` to accept the "struct strbuf *" parameter to improve efficiency. > This refactors infer_backlink() to return an integer result and use a > pre-allocated `strbuf` for the inferred backlink path, replacing the > previous `char*` return type and improving efficiency. > I think you should also talk about that you make the `repair_worktree_at_path` function to align with above refactor. > Signed-off-by: Caleb White <cdwhite3@pm.me> > --- > worktree.c | 48 ++++++++++++++++++++++++------------------------ > 1 file changed, 24 insertions(+), 24 deletions(-) > > diff --git a/worktree.c b/worktree.c > index ec95ea2986107b3bc12d38b0825d7c6e87402bc6..0cba0d6e6e9ad02ace04a0301104a04a07cbef65 100644 > --- a/worktree.c > +++ b/worktree.c > @@ -642,10 +642,9 @@ static int is_main_worktree_path(const char *path) > * be able to infer the gitdir by manually reading /path/to/worktree/.git, > * extracting the <id>, and checking if <repo>/worktrees/<id> exists. > */ > -static char *infer_backlink(const char *gitfile) > +static int infer_backlink(const char *gitfile, struct strbuf *inferred) > { > struct strbuf actual = STRBUF_INIT; > - struct strbuf inferred = STRBUF_INIT; > const char *id; > > if (strbuf_read_file(&actual, gitfile, 0) < 0) > @@ -658,17 +657,18 @@ static char *infer_backlink(const char *gitfile) > id++; /* advance past '/' to point at <id> */ > if (!*id) > goto error; > - strbuf_git_common_path(&inferred, the_repository, "worktrees/%s", id); > - if (!is_directory(inferred.buf)) > + strbuf_reset(inferred); Question here: should we use `strbuf_reset` here? I want to know the reason why you design this. Is the caller's responsibility to clear the "inferred" when calling this function? > + strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id); > + if (!is_directory(inferred->buf)) > goto error; > > strbuf_release(&actual); > - return strbuf_detach(&inferred, NULL); > + return 1; > > error: > strbuf_release(&actual); > - strbuf_release(&inferred); > - return NULL; > + strbuf_reset(inferred); /* clear invalid path */ > + return 0; > } Design question, when calling `infer_backlink` successfully, we will return 1, if not, we will return 0. But in the later code, we use the "inferred.buf.len" to indicate whether we could get the inferred backlink successfully. We have two signals to indicate the success. I think it's a bad idea to use "inferred.buf.len". Let me give you an example here: struct strbuf inferred_backlink = STRBUF_INIT; inferred_backlink = infer_backlink(realdotgit.buf); // What if I wrongly use the following statements? strbuf_addstr(&inferred_backlink, "foo"); if (inferred_backlink.buf.len) { } If you insist using "inferred_backlink.buf.len" as the result, this function should return `void`. And you should add some comments for this function. > > /* > @@ -680,10 +680,11 @@ void repair_worktree_at_path(const char *path, > { > 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; > - char *backlink = NULL; > - char *inferred_backlink = NULL; > + char *dotgit_contents = NULL; > const char *repair = NULL; > int err; > > @@ -699,23 +700,23 @@ void repair_worktree_at_path(const char *path, > goto done; > } > > - inferred_backlink = infer_backlink(realdotgit.buf); > - backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err)); > - if (err == READ_GITFILE_ERR_NOT_A_FILE) { > + infer_backlink(realdotgit.buf, &inferred_backlink); > + dotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err)); > + if (dotgit_contents) { > + strbuf_addstr(&backlink, dotgit_contents); > + } else if (err == READ_GITFILE_ERR_NOT_A_FILE) { > fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data); > goto done; > } else if (err == READ_GITFILE_ERR_NOT_A_REPO) { > - if (inferred_backlink) { > + 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>. (Note: backlink > - * is already NULL, so no need to free it first.) > + * the discovered .git/worktrees/<id>. > */ > - backlink = inferred_backlink; > - inferred_backlink = NULL; > + strbuf_swap(&backlink, &inferred_backlink); > } else { > fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data); > goto done; > @@ -743,13 +744,11 @@ void repair_worktree_at_path(const char *path, > * in the "copy" repository. In this case, point the "copy" worktree's > * .git file at the "copy" repository. > */ > - if (inferred_backlink && fspathcmp(backlink, inferred_backlink)) { > - free(backlink); > - backlink = inferred_backlink; > - inferred_backlink = NULL; > + if (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf)) { > + strbuf_swap(&backlink, &inferred_backlink); > } For single line statement after "if", we should not use `{`. > > - strbuf_addf(&gitdir, "%s/gitdir", backlink); > + strbuf_addf(&gitdir, "%s/gitdir", backlink.buf); > if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0) > repair = _("gitdir unreadable"); > else { > @@ -763,9 +762,10 @@ void repair_worktree_at_path(const char *path, > write_file(gitdir.buf, "%s", realdotgit.buf); > } > done: > - free(backlink); > - free(inferred_backlink); > + free(dotgit_contents); > strbuf_release(&olddotgit); > + strbuf_release(&backlink); > + strbuf_release(&inferred_backlink); > strbuf_release(&gitdir); > strbuf_release(&realdotgit); > strbuf_release(&dotgit); > > -- > 2.46.2 > > Sorry, I could not review other patches today (I need to go sleep). Thanks, Jialuo
On Thursday, October 10th, 2024 at 10:52, shejialuo <shejialuo@gmail.com> wrote: > On Mon, Oct 07, 2024 at 10:12:30PM -0500, Caleb White via B4 Relay wrote: > > > From: Caleb White cdwhite3@pm.me > > > > This lays the groundwork for the next patch, which needs the backlink > > returned from infer_backlink() as a `strbuf`. It seemed inefficient to > > convert from `strbuf` to `char*` and back to `strbuf` again. > > > I think here we should first talk about the current behavior of the > `infer_backlink`: > > `infer_backlink` initializes a "strbuf" for the inferred backlink > and returns the result with type "char *" by detaching the "strbuf" > > And then you should tell your intention like the following (The reader > like me does not know what is the intention of the next patch, you should > explicitly mention this). > > Because we decide to link worktrees with relative paths, we need to > convert the returned inferred backlink "char *" to "strbuf". > However, it is a bad idea to convert from `strbuf` to `char*` and > back to `strbuf` again. Instead, we should just let the > `infer_backlink` to accept the "struct strbuf *" parameter to > improve efficiency. > > > This refactors infer_backlink() to return an integer result and use a > > pre-allocated `strbuf` for the inferred backlink path, replacing the > > previous `char*` return type and improving efficiency. > > > I think you should also talk about that you make the > `repair_worktree_at_path` function to align with above refactor. Thank you for the feedback, I'll add these changes. > > if (strbuf_read_file(&actual, gitfile, 0) < 0) > > @@ -658,17 +657,18 @@ static char *infer_backlink(const char gitfile) > > id++; / advance past '/' to point at <id> */ > > if (!*id) > > goto error; > > - strbuf_git_common_path(&inferred, the_repository, "worktrees/%s", id); > > - if (!is_directory(inferred.buf)) > > + strbuf_reset(inferred); > > > Question here: should we use `strbuf_reset` here? I want to know the > reason why you design this. Is the caller's responsibility to clear the > "inferred" when calling this function? Yes we should, sure it is the caller's responsibility but this just helps prevent bugs. There's plenty of functions that reset the strbuf that's passed to the function before modifying it. > > + strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id); > > + if (!is_directory(inferred->buf)) > > goto error; > > > > strbuf_release(&actual); > > - return strbuf_detach(&inferred, NULL); > > + return 1; > > > > error: > > strbuf_release(&actual); > > - strbuf_release(&inferred); > > - return NULL; > > + strbuf_reset(inferred); /* clear invalid path */ > > + return 0; > > } > > > Design question, when calling `infer_backlink` successfully, we will > return 1, if not, we will return 0. But in the later code, we use the > "inferred.buf.len" to indicate whether we could get the inferred > backlink successfully. Originally, this function call was inside an `if` condition, and it made sense to keep the boolean return. However, the dependent patch that I later rebased on refactored this to be a standalone call. I didn't feel like introducing another variable just to capture the return value when I could glean the same information from the existing strbuf. > We have two signals to indicate the success. I think it's a bad idea to > use "inferred.buf.len". Let me give you an example here: I don't see a problem with this---the two "signals" are guaranteed to always be in sync (either the return is 1 and len is > 0, or return is 0 and len is 0). Having the boolean return gives you flexibility in how you can call the function (if it can be placed inside an if condition). > struct strbuf inferred_backlink = STRBUF_INIT; > inferred_backlink = infer_backlink(realdotgit.buf); > > // What if I wrongly use the following statements? > strbuf_addstr(&inferred_backlink, "foo"); > > if (inferred_backlink.buf.len) { > > } I'm sorry, but this example doesn't make sense. This will fail to compile for several reasons: - infer_backlink() requires two args - you cannot assign an `int` to a `struct strbuf` - even if inferred_backlink became an int then the strbuf_addstr() would fail because you can't pass an `int*` to a `struct strbuf*` - `inferred_backlink.buf.len` doesn't exist, it's `inferred_backlink.len` (probably just a typo) > If you insist using "inferred_backlink.buf.len" as the result, this > function should return `void`. And you should add some comments for this > function. I can add comments, and I can change the return type to `void` if there's consensus, but I really don't see any issue with leaving it as-is. > > - if (inferred_backlink && fspathcmp(backlink, inferred_backlink)) { > > - free(backlink); > > - backlink = inferred_backlink; > > - inferred_backlink = NULL; > > + if (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf)) { > > + strbuf_swap(&backlink, &inferred_backlink); > > } > > > For single line statement after "if", we should not use `{`. The brackets were introduced by the patch that my series depends on. I can remove them, but perhaps it would be better to address that on the dependent patch? Best,
On Thu, Oct 10, 2024 at 04:41:03PM +0000, Caleb White wrote: [snip] > > > @@ -658,17 +657,18 @@ static char *infer_backlink(const char gitfile) > > > id++; / advance past '/' to point at <id> */ > > > if (!*id) > > > goto error; > > > - strbuf_git_common_path(&inferred, the_repository, "worktrees/%s", id); > > > - if (!is_directory(inferred.buf)) > > > + strbuf_reset(inferred); > > > > > > > > Question here: should we use `strbuf_reset` here? I want to know the > > reason why you design this. Is the caller's responsibility to clear the > > "inferred" when calling this function? > > Yes we should, sure it is the caller's responsibility but this just helps > prevent bugs. There's plenty of functions that reset the strbuf that's > passed to the function before modifying it. > Yes, that make senses. [snip] > > We have two signals to indicate the success. I think it's a bad idea to > > use "inferred.buf.len". Let me give you an example here: > > I don't see a problem with this---the two "signals" are guaranteed to > always be in sync (either the return is 1 and len is > 0, or return is > 0 and len is 0). Having the boolean return gives you flexibility in how > you can call the function (if it can be placed inside an if condition). > Yes, there is nothing wrong with this. But we also introduce a burden here, when we need to change/refactor `infer_backlink`, the developer should have the knowledge "when the return is 1 and len is >0 or return is 0 and len is 0". If so, why not just return "infer_backlink.len"? > > struct strbuf inferred_backlink = STRBUF_INIT; > > inferred_backlink = infer_backlink(realdotgit.buf); > > > > > // What if I wrongly use the following statements? > > strbuf_addstr(&inferred_backlink, "foo"); > > > > > if (inferred_backlink.buf.len) { > > > > > } > > I'm sorry, but this example doesn't make sense. This will fail to compile > for several reasons: > - infer_backlink() requires two args > - you cannot assign an `int` to a `struct strbuf` > - even if inferred_backlink became an int then the strbuf_addstr() > would fail because you can't pass an `int*` to a `struct strbuf*` > - `inferred_backlink.buf.len` doesn't exist, it's `inferred_backlink.len` > (probably just a typo) > I am sorry for this, I gave a wrong example here, it should be the following (I copied the wrong line in the previous email): struct strbuf inferred_backlink = STRBUF_INIT; infer_backlink(realdotgit.buf, &inferred_backlink); // What if I wronly use the following statements? strbuf_addstr(&inferred_backlink, "foo"); if (inferred_backlink.len) { ... } Actually, I am not against your way. Instead, you should mention why you choose "inferred_backlink.len" as the signal in the commit message. That's the main reason why I think we may add some comments here. The caller may do not know we should use "inferred_backlink.len" to indicate that we have successfully found the inferred backlink especially there is already a return code in the function. > > If you insist using "inferred_backlink.buf.len" as the result, this > > function should return `void`. And you should add some comments for this > > function. > > I can add comments, and I can change the return type to `void` if there's > consensus, but I really don't see any issue with leaving it as-is. > I agree with you that this function is NOT harmful. Actually, I do not think using "void" as the return type is a good idea. If we decide to use two signals, let's leave it as-is. Some comments should be enough. > > > - if (inferred_backlink && fspathcmp(backlink, inferred_backlink)) { > > > - free(backlink); > > > - backlink = inferred_backlink; > > > - inferred_backlink = NULL; > > > + if (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf)) { > > > + strbuf_swap(&backlink, &inferred_backlink); > > > } > > > > > > > > For single line statement after "if", we should not use `{`. > > The brackets were introduced by the patch that my series depends on. > I can remove them, but perhaps it would be better to address that > on the dependent patch? > The original patch has three lines. So it should use `{`. After your change, it only has one line, isn't it? You could refer to this to see the code style. https://github.com/git/git/blob/master/Documentation/CodingGuidelines > Best, Thanks, Jialuo
On Thursday, October 10th, 2024 at 12:26, shejialuo <shejialuo@gmail.com> wrote: > > > We have two signals to indicate the success. I think it's a bad idea to > > > use "inferred.buf.len". Let me give you an example here: > > > > I don't see a problem with this---the two "signals" are guaranteed to > > always be in sync (either the return is 1 and len is > 0, or return is > > 0 and len is 0). Having the boolean return gives you flexibility in how > > you can call the function (if it can be placed inside an if condition). > > Yes, there is nothing wrong with this. But we also introduce a burden here, > when we need to change/refactor `infer_backlink`, the developer should > have the knowledge "when the return is 1 and len is >0 or return is 0 > and len is 0". > > If so, why not just return "infer_backlink.len"? I would say because the purpose of the return is a boolean, either the call was successful or it wasn't. The developer knowledge that you speak of should be a given---if the function returned true then there's obviously a path in the strbuf. > I am sorry for this, I gave a wrong example here, it should be the > following (I copied the wrong line in the previous email): > > struct strbuf inferred_backlink = STRBUF_INIT; > infer_backlink(realdotgit.buf, &inferred_backlink); > > // What if I wronly use the following statements? > strbuf_addstr(&inferred_backlink, "foo"); There's nothing wrong with this, it's on the developer to check the error condition before proceeding to use the strbuf. > Actually, I am not against your way. Instead, you should mention why you > choose "inferred_backlink.len" as the signal in the commit message. > That's the main reason why I think we may add some comments here. The > caller may do not know we should use "inferred_backlink.len" to indicate > that we have successfully found the inferred backlink especially there > is already a return code in the function. I think the intent is fairly self-evident and does not warrant a comment in the commit message? If the strbuf does not have a length then it should be obvious there is no inferred backlink? > > > If you insist using "inferred_backlink.buf.len" as the result, this > > > function should return `void`. And you should add some comments for this > > > function. > > > > I can add comments, and I can change the return type to `void` if there's > > consensus, but I really don't see any issue with leaving it as-is. > > I agree with you that this function is NOT harmful. Actually, I do not > think using "void" as the return type is a good idea. If we decide to > use two signals, let's leave it as-is. Some comments should be enough. I've added a comment to the function docs. > The original patch has three lines. So it should use `{`. After your > change, it only has one line, isn't it? > > You could refer to this to see the code style. > > https://github.com/git/git/blob/master/Documentation/CodingGuidelines Ah I'm sorry, you're right---I'll adjust. Best,
Caleb White <cdwhite3@pm.me> writes: >> Question here: should we use `strbuf_reset` here? I want to know the >> reason why you design this. Is the caller's responsibility to clear the >> "inferred" when calling this function? > > Yes we should, sure it is the caller's responsibility but this just helps > prevent bugs. There's plenty of functions that reset the strbuf that's > passed to the function before modifying it. If the API says that it _appends_ what it found in the supplied strbuf, then it is the caller's responsibility to ensure that the strbuf being passed is empty, if the caller wants the resulting strbuf to hold only what the function found. If the API says it _returns_ what it found in the supplied strbuf, then the function is responsible to discard what the supplied strbuf originally has. As an API design, if it is rarely useful for callers to be able to append, then the latter is simpler to use, but there are many cases where the "append" semantics is useful (see strbuf.c to find examples). In this particular case, it is rarely useful for the caller to get the result appended to a string it already has before calling this function, so unconditionally discarding what the caller had, without telling the caller that it is responsible for passing an empty buffer, is the right thing, I wuld think. Thanks.
Caleb White <cdwhite3@pm.me> writes: >> If so, why not just return "infer_backlink.len"? > > I would say because the purpose of the return is a boolean, > either the call was successful or it wasn't. The developer > knowledge that you speak of should be a given---if the > function returned true then there's obviously a path > in the strbuf. That does not say what should be left in the strbuf when the request failed. If it is undefined, then using its .len (or .buf) is not quite right, without relying on too much implementation details of the called function. Usually, this project uses 0 to signal a success, while errors are signalled by returning a negative value (and if there can be multiple ways to fail, such a design leaves different negative values as a possibility). If the result the caller finds useful is always positive value (or non-negative value), however, it is perfectly fine and often more convenient if you used the "positive (or non-negative) value gives the intended result and signals a success, negative (or non-positive) value signals an error" convention. The caller can if (0 < do_the_thing()) ; /* success */ just fine, instead of the boolean "0 is success, negative values are various errors", if (!do_the_thing()) ; /* success */ when it does not care how/why the thing failed.
On Thursday, October 10th, 2024 at 15:03, Junio C Hamano <gitster@pobox.com> wrote: > That does not say what should be left in the strbuf when the request > failed. If it is undefined, then using its .len (or .buf) is not > quite right, without relying on too much implementation details of > the called function. > > Usually, this project uses 0 to signal a success, while errors are > signalled by returning a negative value (and if there can be > multiple ways to fail, such a design leaves different negative > values as a possibility). > > If the result the caller finds useful is always positive value (or > non-negative value), however, it is perfectly fine and often more > convenient if you used the "positive (or non-negative) value gives > the intended result and signals a success, negative (or > non-positive) value signals an error" convention. The caller can > > if (0 < do_the_thing()) > ; /* success / > > just fine, instead of the boolean "0 is success, negative values are > various errors", > > if (!do_the_thing()) > ; / success */ > > when it does not care how/why the thing failed. That makes sense. I can update the function to return -1 on error and the strbuf.len on success.
diff --git a/worktree.c b/worktree.c index ec95ea2986107b3bc12d38b0825d7c6e87402bc6..0cba0d6e6e9ad02ace04a0301104a04a07cbef65 100644 --- a/worktree.c +++ b/worktree.c @@ -642,10 +642,9 @@ static int is_main_worktree_path(const char *path) * be able to infer the gitdir by manually reading /path/to/worktree/.git, * extracting the <id>, and checking if <repo>/worktrees/<id> exists. */ -static char *infer_backlink(const char *gitfile) +static int infer_backlink(const char *gitfile, struct strbuf *inferred) { struct strbuf actual = STRBUF_INIT; - struct strbuf inferred = STRBUF_INIT; const char *id; if (strbuf_read_file(&actual, gitfile, 0) < 0) @@ -658,17 +657,18 @@ static char *infer_backlink(const char *gitfile) id++; /* advance past '/' to point at <id> */ if (!*id) goto error; - strbuf_git_common_path(&inferred, the_repository, "worktrees/%s", id); - if (!is_directory(inferred.buf)) + strbuf_reset(inferred); + strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id); + if (!is_directory(inferred->buf)) goto error; strbuf_release(&actual); - return strbuf_detach(&inferred, NULL); + return 1; error: strbuf_release(&actual); - strbuf_release(&inferred); - return NULL; + strbuf_reset(inferred); /* clear invalid path */ + return 0; } /* @@ -680,10 +680,11 @@ void repair_worktree_at_path(const char *path, { 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; - char *backlink = NULL; - char *inferred_backlink = NULL; + char *dotgit_contents = NULL; const char *repair = NULL; int err; @@ -699,23 +700,23 @@ void repair_worktree_at_path(const char *path, goto done; } - inferred_backlink = infer_backlink(realdotgit.buf); - backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err)); - if (err == READ_GITFILE_ERR_NOT_A_FILE) { + infer_backlink(realdotgit.buf, &inferred_backlink); + dotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err)); + if (dotgit_contents) { + strbuf_addstr(&backlink, dotgit_contents); + } else if (err == READ_GITFILE_ERR_NOT_A_FILE) { fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data); goto done; } else if (err == READ_GITFILE_ERR_NOT_A_REPO) { - if (inferred_backlink) { + 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>. (Note: backlink - * is already NULL, so no need to free it first.) + * the discovered .git/worktrees/<id>. */ - backlink = inferred_backlink; - inferred_backlink = NULL; + strbuf_swap(&backlink, &inferred_backlink); } else { fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data); goto done; @@ -743,13 +744,11 @@ void repair_worktree_at_path(const char *path, * in the "copy" repository. In this case, point the "copy" worktree's * .git file at the "copy" repository. */ - if (inferred_backlink && fspathcmp(backlink, inferred_backlink)) { - free(backlink); - backlink = inferred_backlink; - inferred_backlink = NULL; + if (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf)) { + strbuf_swap(&backlink, &inferred_backlink); } - strbuf_addf(&gitdir, "%s/gitdir", backlink); + strbuf_addf(&gitdir, "%s/gitdir", backlink.buf); if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0) repair = _("gitdir unreadable"); else { @@ -763,9 +762,10 @@ void repair_worktree_at_path(const char *path, write_file(gitdir.buf, "%s", realdotgit.buf); } done: - free(backlink); - free(inferred_backlink); + free(dotgit_contents); strbuf_release(&olddotgit); + strbuf_release(&backlink); + strbuf_release(&inferred_backlink); strbuf_release(&gitdir); strbuf_release(&realdotgit); strbuf_release(&dotgit);