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
