Re: [PATCH v2 2/4] worktree: link worktrees with relative paths

From: Caleb White <cdwhite3@pm.me>
Date: Oct 6, 2024, 23:57 UTC
Message-ID: <I3BmSHTyOELt2DzfSaLhRYLouu5iAPZIZGZ2Ne73AygO35CM0mq44INa68t6VD4XV61DgzbrfUW0m8fivd3N9Rejgm-tecXQHXQRs1BP9CQ=@pm.me>
In-Reply-To: <ZwKuptTMf9ECd-kf@ArchLinux>

On Sunday, October 6th, 2024 at 10:37, shejialuo <shejialuo@gmail.com> 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.

Show 5 quoted lines

> > 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...