On Sunday, October 6th, 2024 at 10:09, shejialuo <shejialuo@xxxxxxxxx> 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. > 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.
Attachment:
signature.asc
Description: OpenPGP digital signature
