On Mon, Oct 07, 2024 at 02:26:51AM +0000, Caleb White wrote:
> On Sunday, October 6th, 2024 at 13:41, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> > I found the name "git_contents" clear enough and understood its
> > purpose at-a-glance, so I think it's a reasonably good choice. A
> > slightly better name might be "gitfile_contents" or perhaps
> > "dotgit_contents" for consistency with other similarly-named variables
> > in this function.
>
> I will rename to `dotgit_contents`.
>
> > It certainly makes sense to check whether "git_contents" is NULL
> > before trying to copy it into the "backlink" strbuf, however, if
> > "git_contents" is NULL here, then what does that mean? What does it
> > mean to leave "backlink" empty? The only way (presumably) we get this
> > far is if read_gitfile_gently() succeeded, so (presumably)
> > "git_contents" should not be NULL. Thus, rather than conditionally
> > copying into "backlink", we should instead indicate clearly via BUG()
> > that it should be impossible for "git_contents" to be NULL. So, rather
> > than making this part of the existing if-else-cascade, we should do
> > this as a standalone `if`:
> >
>
> > if (!git_contents)
> > BUG(...);
> > strbuf_addstr(&backlink, git_contents);
I agree with the idea that Eric proposed. It's truly we should raise a
bug here. That's would be much more clear.
> We can't use BUG because this is handled as part of the err
> conditions. The contents can be NULL and `backlink` could be
> filled with the inferred backlink. I moved this to the top
> and I think it reads better.
That's not correct. It is true that the contents can be NULL and
`backlink` could be filled with the infer_backlink. But do you notice
that if `backlink` was filled with the infer_backlink, it will jump
to the "done" label.
What Erie means is the following:
git_contents = read_gitfile_gently(...);
if (err) {
if (...) {
} else if (err == RAED_GITFILE_ERR_NOT_A_REPO) {
// handle backlink
goto done;
}
}
if (!git_contents)
BUG(...);
strbuf_addstr(&backlink, git_contents);
done:
// cleanup operations
Thanks,
Jialuo
