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

From: shejialuo <shejialuo@gmail.com>
Date: Oct 7, 2024, 04:12 UTC
Message-ID: <ZwNfuLoVn4aU4mgQ@ArchLinux>
In-Reply-To: <o7jQV4JUYr2iQtErKFhCrjCySiGn7_I18gmdInKbOC9TAsvqazG0O_fPUtiWZ5UfiGLr25OZ5xvqIhfOIwCG-s1RGgri-BJIdHqiokIw4z0=@pm.me>

On Mon, Oct 07, 2024 at 02:26:51AM +0000, Caleb White wrote:

Show 24 quoted lines

> On Sunday, October 6th, 2024 at 13:41, Eric Sunshine <sunshine@sunshineco.com> 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