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

From: shejialuo <shejialuo@gmail.com>
Date: Oct 7, 2024, 03:45 UTC
Message-ID: <ZwNZRX1LHlxawJJc@ArchLinux>
In-Reply-To: <I3BmSHTyOELt2DzfSaLhRYLouu5iAPZIZGZ2Ne73AygO35CM0mq44INa68t6VD4XV61DgzbrfUW0m8fivd3N9Rejgm-tecXQHXQRs1BP9CQ=@pm.me>

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

[snip]

Show 7 quoted lines

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

Show 7 quoted lines

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

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

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?