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

On Mon, Oct 07, 2024 at 04:01:43AM +0000, Caleb White wrote:

On Sunday, October 6th, 2024 at 22:56, shejialuo [off-list ref] wrote:

quoted

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