On Sunday, October 6th, 2024 at 13:41, Eric Sunshine [off-list ref] 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);

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.