On Sun, Oct 06, 2024 at 11:57:22PM +0000, Caleb White wrote:

[snip]

quoted

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?

quoted

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);

quoted

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));

quoted

quoted

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?