Re: [PATCH v3 1/3] worktree: refactor infer_backlink() to use *strbuf

From: Caleb White <cdwhite3@pm.me>
Date: Oct 10, 2024, 16:41 UTC
Message-ID: <e1RptKNShhPZHXDhBkQBaCNCmKBKik4nQzRShqtgVfjcH7vBWpuLYV60PSHaJ0diX-oG3XiKHc7IebhIZM4eSkeYdQQZ_QYK2ixxsK-XwrE=@pm.me>
In-Reply-To: <Zwf4PT-vaLX35ELL@ArchLinux>

On Thursday, October 10th, 2024 at 10:52, shejialuo <shejialuo@gmail.com> wrote:

> On Mon, Oct 07, 2024 at 10:12:30PM -0500, Caleb White via B4 Relay wrote:
>

> > From: Caleb White cdwhite3@pm.me
> >

> > This lays the groundwork for the next patch, which needs the backlink
> > returned from infer_backlink() as a `strbuf`. It seemed inefficient to
> > convert from `strbuf` to `char*` and back to `strbuf` again.
>

> I think here we should first talk about the current behavior of the
> `infer_backlink`:
>

> `infer_backlink` initializes a "strbuf" for the inferred backlink
> and returns the result with type "char *" by detaching the "strbuf"
>

> And then you should tell your intention like the following (The reader
> like me does not know what is the intention of the next patch, you should
> explicitly mention this).
>

Show 7 quoted lines

> Because we decide to link worktrees with relative paths, we need to
> convert the returned inferred backlink "char *" to "strbuf".
> However, it is a bad idea to convert from `strbuf` to `char*` and
> back to `strbuf` again. Instead, we should just let the
> `infer_backlink` to accept the "struct strbuf *" parameter to
> improve efficiency.
>

> > This refactors infer_backlink() to return an integer result and use a
> > pre-allocated `strbuf` for the inferred backlink path, replacing the
> > previous `char*` return type and improving efficiency.
>

> I think you should also talk about that you make the
> `repair_worktree_at_path` function to align with above refactor.

Thank you for the feedback, I'll add these changes.

Show 9 quoted lines

> > if (strbuf_read_file(&actual, gitfile, 0) < 0)
> > @@ -658,17 +657,18 @@ static char *infer_backlink(const char gitfile)
> > id++; / advance past '/' to point at <id> */
> > if (!*id)
> > goto error;
> > - strbuf_git_common_path(&inferred, the_repository, "worktrees/%s", id);
> > - if (!is_directory(inferred.buf))
> > + strbuf_reset(inferred);
>

> Question here: should we use `strbuf_reset` here? I want to know the
> reason why you design this. Is the caller's responsibility to clear the
> "inferred" when calling this function?

Yes we should, sure it is the caller's responsibility but this just helps prevent bugs. There's plenty of functions that reset the strbuf that's passed to the function before modifying it.

> > + strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id);
> > + if (!is_directory(inferred->buf))
> > goto error;
> >

> > strbuf_release(&actual);
> > - return strbuf_detach(&inferred, NULL);
> > + return 1;
> >

Show 8 quoted lines

> > error:
> > strbuf_release(&actual);
> > - strbuf_release(&inferred);
> > - return NULL;
> > + strbuf_reset(inferred); /* clear invalid path */
> > + return 0;
> > }
>

> Design question, when calling `infer_backlink` successfully, we will
> return 1, if not, we will return 0. But in the later code, we use the
> "inferred.buf.len" to indicate whether we could get the inferred
> backlink successfully.

Originally, this function call was inside an `if` condition, and it made sense to keep the boolean return. However, the dependent patch that I later rebased on refactored this to be a standalone call. I didn't feel like introducing another variable just to capture the return value when I could glean the same information from the existing strbuf.

> We have two signals to indicate the success. I think it's a bad idea to
> use "inferred.buf.len". Let me give you an example here:

I don't see a problem with this---the two "signals" are guaranteed to always be in sync (either the return is 1 and len is > 0, or return is 0 and len is 0). Having the boolean return gives you flexibility in how you can call the function (if it can be placed inside an if condition).

> struct strbuf inferred_backlink = STRBUF_INIT;
> inferred_backlink = infer_backlink(realdotgit.buf);
>

> // What if I wrongly use the following statements?
> strbuf_addstr(&inferred_backlink, "foo");
>

> if (inferred_backlink.buf.len) {
>

> }

I'm sorry, but this example doesn't make sense. This will fail to compile
for several reasons:
- infer_backlink() requires two args
- you cannot assign an `int` to a `struct strbuf`
- even if inferred_backlink became an int then the strbuf_addstr()
  would fail because you can't pass an `int*` to a `struct strbuf*`
- `inferred_backlink.buf.len` doesn't exist, it's `inferred_backlink.len`
  (probably just a typo)

> If you insist using "inferred_backlink.buf.len" as the result, this
> function should return `void`. And you should add some comments for this
> function.

I can add comments, and I can change the return type to `void` if there's consensus, but I really don't see any issue with leaving it as-is.

Show 8 quoted lines

> > - if (inferred_backlink && fspathcmp(backlink, inferred_backlink)) {
> > - free(backlink);
> > - backlink = inferred_backlink;
> > - inferred_backlink = NULL;
> > + if (inferred_backlink.len && fspathcmp(backlink.buf, inferred_backlink.buf)) {
> > + strbuf_swap(&backlink, &inferred_backlink);
> > }
>

> For single line statement after "if", we should not use `{`.

The brackets were introduced by the patch that my series depends on. I can remove them, but perhaps it would be better to address that on the dependent patch?

Best,