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

From: Caleb White <cdwhite3@pm.me>
Date: Oct 7, 2024, 02:26 UTC
Message-ID: <o7jQV4JUYr2iQtErKFhCrjCySiGn7_I18gmdInKbOC9TAsvqazG0O_fPUtiWZ5UfiGLr25OZ5xvqIhfOIwCG-s1RGgri-BJIdHqiokIw4z0=@pm.me>
In-Reply-To: <CAPig+cQC45_Pofw31h2RSydj_CjqpfajLLa5O_sR9m0AQh5OVw@mail.gmail.com>

On Sunday, October 6th, 2024 at 13:41, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 5 quoted lines

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

Show 11 quoted lines

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