On Sunday, October 6th, 2024 at 10:37, shejialuo [off-list ref] wrote

Eric has already commented on this commit message. I think this commit
has done a lot of things which make the review painful.

My apologies, I will do better on this commit message.

Do we need reset the "sb_tmp"? I guess we do not need, "relative_path"
does not care about "sb_tmp". It will always reset the value of the
"sb_tmp". So, we may delete this line.

You are right, this is reset by relative_path(). I originally encountered
a bug and I thought not resetting this strbuf between relative_path() calls
was the cause but it must have been something else that I fixed.

Still, we do not need to call "strbuf_reset" again for "tmp". But there
is another question here. Should we define the "file" just in this "if"
block and free "file" also in the block?

The style this code uses seems to place most / all of the declarations at
the top of the function and frees at the bottom so I think this fits in.

And I don't think it's a good idea to use "xstrfmt". Here, we need to
allocate two times and free two times. Why not just define a "struct
strbuf" and the use "strbuf_*" method here?

I can use strbufs, I just wasn't sure if I really needed a strbuf for
each of the paths and was just trying to reuse a var.

quoted

strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
strbuf_addf(&dotgit, "%s/.git", wt->path);
- backlink = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
+ git_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));

Why here we need to use "xstrdup_or_null". The life cycle of the
"git_contents" variable is in the "repair_gitfile" function.

This what the existing code used and I saw no reason to change it...