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

[shejialuo <shejialuo@gmail.com>]

On Thu, Oct 10, 2024 at 04:41:03PM +0000, Caleb White wrote:

[snip]

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

Yes, that make senses.

[snip]

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

Yes, there is nothing wrong with this. But we also introduce a burden here,
when we need to change/refactor `infer_backlink`, the developer should
have the knowledge "when the return is 1 and len is >0 or return is 0
and len is 0".

If so, why not just return "infer_backlink.len"?

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

I am sorry for this, I gave a wrong example here, it should be the
following (I copied the wrong line in the previous email):

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

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

    if (inferred_backlink.len) {
        ...
    }

Actually, I am not against your way. Instead, you should mention why you
choose "inferred_backlink.len" as the signal in the commit message.
That's the main reason why I think we may add some comments here. The
caller may do not know we should use "inferred_backlink.len" to indicate
that we have successfully found the inferred backlink especially there
is already a return code in the function.

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

I agree with you that this function is NOT harmful. Actually, I do not
think using "void" as the return type is a good idea. If we decide to
use two signals, let's leave it as-is. Some comments should be enough.

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

The original patch has three lines. So it should use `{`. After your
change, it only has one line, isn't it?

You could refer to this to see the code style.

  https://github.com/git/git/blob/master/Documentation/CodingGuidelines

> Best,

Thanks,
Jialuo