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

From: Caleb White <cdwhite3@pm.me>
Date: Oct 10, 2024, 17:52 UTC
Message-ID: <J1frhnJqftB1IUvIGDnJiz6uYTDNl_Bc92NLj1gXJtgRxapI3Uy-fyFHYtv5_p8e7yzF0pegawV6NlHQTn0GHcXP8MZ_1X8CNkIvO3_MP9c=@pm.me>
In-Reply-To: <ZwgOURNkNhg8BsaU@ArchLinux>

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

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

Show 5 quoted lines

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

Show 5 quoted lines

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

I would say because the purpose of the return is a boolean, either the call was successful or it wasn't. The developer knowledge that you speak of should be a given---if the function returned true then there's obviously a path in the strbuf.

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

There's nothing wrong with this, it's on the developer to check the error condition before proceeding to use the strbuf.

Show 6 quoted lines

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

I think the intent is fairly self-evident and does not warrant a comment in the commit message? If the strbuf does not have a length then it should be obvious there is no inferred backlink?

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

I've added a comment to the function docs.

> 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

Ah I'm sorry, you're right---I'll adjust.

Best,