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

From: shejialuo <shejialuo@gmail.com>
Date: Oct 7, 2024, 03:56 UTC
Message-ID: <ZwNb5Aabrub4tW5O@ArchLinux>
In-Reply-To: <CAPig+cQC45_Pofw31h2RSydj_CjqpfajLLa5O_sR9m0AQh5OVw@mail.gmail.com>

On Sun, Oct 06, 2024 at 02:41:22PM -0400, Eric Sunshine wrote:

[snip]

Show 10 quoted lines

> > 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.
>

Thanks, I don't know the context here.

Show 6 quoted lines

> > 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.
>

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"?

But, I agree with you that the existing logic is clear enough. I just explain further what I mean here.

Thanks, Jialuo