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

[shejialuo <shejialuo@gmail.com>]

On Mon, Oct 07, 2024 at 04:01:43AM +0000, Caleb White wrote:
> On Sunday, October 6th, 2024 at 22:56, shejialuo <shejialuo@gmail.com> wrote:
> > The reason why I don't think we need to check the "err" variable is that
> > the "git_contents" and "err" is relevant. If "git_contents" is not NULL,
> > the "err" must be zero unless there are bugs in "read_gitfile_gently".
> > So, if we already check "git_contents", why do we need to check again
> > for "err"?
> 
> There are two other error conditions we check, and one of them we try
> to find the inferred backlink (so it is not a failure path):
> 
> ```
> } else 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 (!infer_backlink(realdotgit.buf, &backlink)) {
> 		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;
> }
> 
> ```

I am sorry that my words make you not clear here. I want to express that
if "git_contents" is not NULL and there is no need for us to check
"err". This means we could use the following flows:

    if (git_contents && !err) {
        ...
    } else if (err == xxx) {
        ...
    }

However, from my perspective, the way proposed by Eric where we could
use "BUG" is more robust. Because the current method assumes that
"read_gitfile_gently" works as we want.

Thanks,
Jialuo