On Thursday, October 10th, 2024 at 12:26, shejialuo <shejialuo@xxxxxxxxx> 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:
> >
> > 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"?
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.
> 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,Attachment:
signature.asc
Description: OpenPGP digital signature
