On Sun, Oct 6, 2024 at 11:09 AM shejialuo <shejialuo@xxxxxxxxx> wrote:
> On Sun, Oct 06, 2024 at 06:00:57AM +0000, Caleb White wrote:
> > - backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
> > + git_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err));
>
> 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".
Thanks for thinking through this logic. I have a few additional comments...
> 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.
> 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.
Upon reading this patch, I had a similar thought about this, that it
would be more reflective of the original code to set "backlink" early
here rather than waiting until the end of the if-else-cascade.
However, upon reflection, I don't mind that setting "backlink" is
delayed until after the if-else-chain, though I think it deserves at
least one change which I will explain below.
> 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.
> > 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 (!(backlink = infer_backlink(realdotgit.buf))) {
> > + if (infer_backlink(&backlink, realdotgit.buf)) {
> > 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;
> > + } else if (git_contents) {
> > + strbuf_addstr(&backlink, git_contents);
> > }
It certainly makes sense to check whether "git_contents" is NULL
before trying to copy it into the "backlink" strbuf, however, if
"git_contents" is NULL here, then what does that mean? What does it
mean to leave "backlink" empty? The only way (presumably) we get this
far is if read_gitfile_gently() succeeded, so (presumably)
"git_contents" should not be NULL. Thus, rather than conditionally
copying into "backlink", we should instead indicate clearly via BUG()
that it should be impossible for "git_contents" to be NULL. So, rather
than making this part of the existing if-else-cascade, we should do
this as a standalone `if`:
if (!git_contents)
BUG(...);
strbuf_addstr(&backlink, git_contents);
