On Sunday, October 6th, 2024 at 10:37, shejialuo <shejialuo@xxxxxxxxx> 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.
> > 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...
Attachment:
signature.asc
Description: OpenPGP digital signature
