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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 6, 2024, 18:41 UTC
Message-ID: <CAPig+cQC45_Pofw31h2RSydj_CjqpfajLLa5O_sR9m0AQh5OVw@mail.gmail.com>
In-Reply-To: <ZwKoNg-HYGaohvCd@ArchLinux>

On Sun, Oct 6, 2024 at 11:09 AM shejialuo <shejialuo@gmail.com> wrote:

Show 21 quoted lines

> On Sun, Oct 06, 2024 at 06:00:57AM +0000, Caleb White wrote:
> > -     backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
> > +     git_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
>
> So, we create a new variable "git_contents" here. I suspect this is a
> poor design. In the original logic, we will do the following things for
> "backlink".
>
>   1. Call the "read_gitfile_gently" function. If it encounters error, it
>      will return NULL and the "err" variable will be set to NON-zero.
>   2. If the value of "err" is 0, we would simply execute the
>      "strbuf_addstr(&gitdir, "%s/gitdir", backlink)".
>   3. If not, and the "err" is "READ_GITFILE_ERR_NOT_A_REPO". We will
>      call "infer_backlink" to set the "backlink".
>
> Because now "backlink" is "struct strbuf", we cannot just assign it by
> using "xstrdup_or_null(read_gitfile_gently(...))". So, we create a new
> variable "git_contents" here.
>
> And we will check whether "git_contents" is NULL to set the value for
> the "backlink".

Thanks for thinking through this logic. I have a few additional comments...

> Why not simply do the following things here (I don't think
> "git_contents" is a good name, however I am not familiar with the
> worktree, I cannot give some advice here).

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.

Show 6 quoted lines

>     const char *git_contents;
>     git_contents = read_gitfile_gently(...);
>     if (git_contents)
>         strbuf_addstr(&backlink, git_contents);
>
> Even more, we could enhance the logic here.

Upon reading this patch, I had a similar thought about this, that it would be more reflective of the original code to set "backlink" early here rather than waiting until the end of the if-else-cascade. However, upon reflection, I don't mind that setting "backlink" is delayed until after the if-else-chain, though I think it deserves at least one change which I will explain below.

> If "git_contents" is not
> NULL, there is no need for us to check the "err" variable.

I'm not sure we would want to change this; the existing logic seems clear enough.

Show 15 quoted lines

> >       if (err == READ_GITFILE_ERR_NOT_A_FILE) {
> >               fn(1, realdotgit.buf, _("unable to locate repository; .git is not a file"), cb_data);
> >               goto done;
> >       } else if (err == READ_GITFILE_ERR_NOT_A_REPO) {
> > -             if (!(backlink = infer_backlink(realdotgit.buf))) {
> > +             if (infer_backlink(&backlink, realdotgit.buf)) {
> >                       fn(1, realdotgit.buf, _("unable to locate repository; .git file does not reference a repository"), cb_data);
> >                       goto done;
> >               }
> >       } else if (err) {
> >               fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data);
> >               goto done;
> > +     } else if (git_contents) {
> > +             strbuf_addstr(&backlink, git_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);