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

From: Caleb White <cdwhite3@pm.me>
Date: Oct 6, 2024, 23:47 UTC
Message-ID: <Exjb4s8fML6YqFE6Nkf7SQoyvOJxinkwuk-V_TRJvjl5yRxDrrn3rRsPHsnmXqfuu2zFasuP26eO1A8MfNo7shKbIlj_YsPx-mzvelinkPw=@pm.me>
In-Reply-To: <ZwKoNg-HYGaohvCd@ArchLinux>

On Sunday, October 6th, 2024 at 10:09, shejialuo <shejialuo@gmail.com> wrote:

> I think we should first say why we need to add the change in the commit
> message which means we should express our motivation in the first. It's
> wired to say "I have done something" and then talk about the motivation
> why we do this.

I can reword this commit message.

> Because we leave the life cycle of the "inferred" to be outside of this
> function, we should not use "strbuf_release" to release the memory here.
> Make sense.

Yes, however, I just realized that I should likely reset the strbuf when it enters the function (or before it is written to) which is similar to what the relative_path() function does.

Show 29 quoted lines

> 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".
>
> 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).
>
> 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. If "git_contents" is not
> NULL, there is no need for us to check the "err" variable.

I was trying to not make huge changes to the logic flow, but I suppose I could revisit this if desired. I can likely move the `if(git_contents)` to the start instead of being at the end. I was not aware that if an err occurred that the function returned NULL, I thought that perhaps there was the possibility of git_contents being filled with something and an err still occurring.