From: Caleb White Date: Mon, 07 Oct 2024 02:26:51 GMT Subject: Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf Message-ID: In-Reply-To: On Sunday, October 6th, 2024 at 13:41, Eric Sunshine 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.