On Sun, Oct 06, 2024 at 11:57:22PM +0000, Caleb White wrote:
[snip]
> > 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.
>
Yes, as you have said, the code style places most / all of the
declarations at the top and free at the bottom. But the trouble here is
we will free the "file" in the "if" block.
char *file = NULL;
if (...) {
file = xstrfmt(...);
free(file);
file = xstrfmt(...);
}
free(file);
If we want to follow the original code style, should we create two
variables at the top and free them at the bottom?
> > 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.
>
You don't need to create a new "strbuf" for each of the paths. You could
just use "strbuf_reset" for only one "struct strbuf".
struct strbuf file = STRBUF_INIT;
if (...) {
strbuf_addf(...);
strbuf_reset(&file);
strbuf_addf(...);
}
strbuf_release(&file);
> > > 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...
Actually, I somehow understand why in the original code, it will use
"xstrdup_or_null" to initialize the "backlink". Because in
"read_gitfile_gently", we will use a static "struct strbuf" as the
buffer.
I guess the intention of the original code is that if we call again
"read_gitfile_gently" in this function or we have another thread which
calls this function, the content of the buffer will be changed. So we
explicitly use "xstrdup_or_null" to create a copy here to avoid this.
But I wonder whether we really need to consider above problem?
