| Message ID | 20241006060017.171788-2-cdwhite3@pm.me (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Link worktrees with relative paths | expand |
On Sun, Oct 06, 2024 at 06:00:57AM +0000, Caleb White wrote: > This refactors the `infer_backlink` function to return an integer > result and use a pre-allocated `strbuf` for the inferred backlink > path, replacing the previous `char*` return type. > > This lays the groundwork for the next patch, which needs the > resultant backlink as a `strbuf`. There was no need to go from > `strbuf -> char* -> strbuf` again. This change also aligns the > function's signature with other `strbuf`-based functions. > 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. > Signed-off-by: Caleb White <cdwhite3@pm.me> > --- > worktree.c | 26 ++++++++++++++------------ > 1 file changed, 14 insertions(+), 12 deletions(-) > > diff --git a/worktree.c b/worktree.c > index 0f032cc..c6d2ede 100644 > --- a/worktree.c > +++ b/worktree.c > @@ -642,10 +642,9 @@ static int is_main_worktree_path(const char *path) > * be able to infer the gitdir by manually reading /path/to/worktree/.git, > * extracting the <id>, and checking if <repo>/worktrees/<id> exists. > */ > -static char *infer_backlink(const char *gitfile) > +static int infer_backlink(st > ruct strbuf *inferred, const char *gitfile) This line is so strange. Why it generates a newline here? > { > struct strbuf actual = STRBUF_INIT; > - struct strbuf inferred = STRBUF_INIT; > const char *id; > > if (strbuf_read_file(&actual, gitfile, 0) < 0) > @@ -658,17 +657,16 @@ static char *infer_backlink(const char *gitfile) > id++; /* advance past '/' to point at <id> */ > if (!*id) > goto error; > - strbuf_git_common_path(&inferred, the_repository, "worktrees/%s", id); > - if (!is_directory(inferred.buf)) > + strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id); > + if (!is_directory(inferred->buf)) > goto error; > > strbuf_release(&actual); > - return strbuf_detach(&inferred, NULL); > + return 0; > > error: > strbuf_release(&actual); > - strbuf_release(&inferred); 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. > - return NULL; > + return 1; > } > > /* > @@ -680,9 +678,10 @@ void repair_worktree_at_path(const char *path, > { > struct strbuf dotgit = STRBUF_INIT; > struct strbuf realdotgit = STRBUF_INIT; > + struct strbuf backlink = STRBUF_INIT; Here, we replace "char *backlink" to "struct strbuf backlink", we need to align the changed "infer_backlink" function. That's OK. > struct strbuf gitd > ir = STRBUF_INIT; Another strange newline here. > struct strbuf olddotgit = STRBUF_INIT; > - char *backlink = NULL; > + char *git_contents = NULL; > const char *repair = NULL; > int err; > > @@ -698,21 +697,23 @@ void repair_worktree_at_path(const char *path, > goto done; > } > > - 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". 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. > 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_conte > nts) { Another strange newline here. As I have talked about above, we should not check "git_contents" here. > + strbuf_addstr(&backlink, git_contents); > } > > - strbuf_addf(&gitdir, "%s/gitdir", backlink); > + strbuf_addf(&gitdir, "%s/gitdir", backlink.buf); > if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0) > repair = _("gitdir unreadable"); > else { > @@ -726,8 +727,9 @@ void repair_worktree_at_path(const char *path, > write_file(gitdir.buf, "%s", realdotgit.buf); > } > done: > - free(backlink); > + free(git_contents); > strbuf_release(&olddotgit); > + strbuf_release(&backlink); > strbuf_release(&gitdir); > strbuf_release(&realdotgit); > strbuf_release(&dotgit); > -- > 2.46.2 > Thanks, Jialuo
On Sun, Oct 6, 2024, at 17:09, shejialuo wrote: > On Sun, Oct 06, 2024 at 06:00:57AM +0000, Caleb White wrote: >> This refactors the `infer_backlink` function to return an integer >> result and use a pre-allocated `strbuf` for the inferred backlink >> path, replacing the previous `char*` return type. >> >> This lays the groundwork for the next patch, which needs the >> resultant backlink as a `strbuf`. There was no need to go from >> `strbuf -> char* -> strbuf` again. This change also aligns the >> function's signature with other `strbuf`-based functions. >> > > 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. > >> Signed-off-by: Caleb White <cdwhite3@pm.me> >> --- >> worktree.c | 26 ++++++++++++++------------ >> 1 file changed, 14 insertions(+), 12 deletions(-) >> >> diff --git a/worktree.c b/worktree.c >> index 0f032cc..c6d2ede 100644 >> --- a/worktree.c >> +++ b/worktree.c >> @@ -642,10 +642,9 @@ static int is_main_worktree_path(const char *path) >> * be able to infer the gitdir by manually reading /path/to/worktree/.git, >> * extracting the <id>, and checking if <repo>/worktrees/<id> exists. >> */ >> -static char *infer_backlink(const char *gitfile) >> +static int infer_backlink(st >> ruct strbuf *inferred, const char *gitfile) > > This line is so strange. Why it generates a newline here? The patches got corrupted by something. See the emails from Eric Sunshine. This resubmit didn’t fix the issue. https://lore.kernel.org/git/CAPig+cTB-sA-g4cdhfEjWwY1mnbWJ41e=bOCNwp=Y8JKvpmpRg@mail.gmail.com/
On Sun, Oct 6, 2024 at 2:01 AM Caleb White <cdwhite3@pm.me> wrote: > This refactors the `infer_backlink` function to return an integer > result and use a pre-allocated `strbuf` for the inferred backlink > path, replacing the previous `char*` return type. > > This lays the groundwork for the next patch, which needs the > resultant backlink as a `strbuf`. There was no need to go from > `strbuf -> char* -> strbuf` again. This change also aligns the > function's signature with other `strbuf`-based functions. Regarding aligning the signature with other strbuf-based functions... > Signed-off-by: Caleb White <cdwhite3@pm.me> > --- > diff --git a/worktree.c b/worktree.c > @@ -642,10 +642,9 @@ static int is_main_worktree_path(const char *path) > -static char *infer_backlink(const char *gitfile) > +static int infer_backlink(struct strbuf *inferred, const char *gitfile) ... it's subjective, but I find that placing the strbuf as the first argument makes the purpose of the function less clear. The direct purpose of all (or the majority of) functions in strbuf.h is to operate on strbufs, thus it makes sense for the strbuf to be the first argument (just like `this` is the invisible first argument of instance methods in C++ which operate on an instance of the class). However, infer_backlink()'s purpose isn't to operate on a strbuf; the fact that the original signature neither accepted nor returned a strbuf bears that out. The really important input to this function is `gitfile`, thus it makes sense for this argument to come first. The strbuf which this patch makes it use is a mere implementation detail (a micro-optimization) which doesn't otherwise change the function's original purpose, thus it belongs in a less prominent position in the argument list. > @@ -658,17 +657,16 @@ static char *infer_backlink(const char *gitfile) > - strbuf_git_common_path(&inferred, the_repository, "worktrees/%s", id); > - if (!is_directory(inferred.buf)) > + strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id); > + if (!is_directory(inferred->buf)) > goto error; > strbuf_release(&actual); > - return strbuf_detach(&inferred, NULL); > + return 0; On success, we now return zero... > error: > strbuf_release(&actual); > - strbuf_release(&inferred); > - return NULL; > + return 1; ... and on failure we return 1. To avoid confusing readers who are familiar with project idioms, it would probably be better to instead follow the convention used in most of the rest of the project (and in Unix system calls in general) of returning -1. However... > @@ -698,21 +697,23 @@ void repair_worktree_at_path(const char *path, > - 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; > } ... this code now becomes more than a little confusing to read. It says "if infer_backlink then signal error", which sounds rather backward and leaves the reader scratching his or her head. ("Why signal error if the function succeeded?"). Hence, infer_backlink() should probably return 1 on success and 0 on failure, which will make this code read more idiomatically: if (!infer_backlink(realdotgit.buf, &backlink)) { ...signal error...
On Sun, Oct 6, 2024 at 11:09 AM shejialuo <shejialuo@gmail.com> 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);
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. > 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.
On Sunday, October 6th, 2024 at 13:41, Eric Sunshine <sunshine@sunshineco.com> wrote: > 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. I will rename to `dotgit_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); We can't use BUG because this is handled as part of the err conditions. The contents can be NULL and `backlink` could be filled with the inferred backlink. I moved this to the top and I think it reads better.
On Sunday, October 6th, 2024 at 13:16, Eric Sunshine <sunshine@sunshineco.com> wrote: > ... it's subjective, but I find that placing the strbuf as the first > argument makes the purpose of the function less clear. The direct > purpose of all (or the majority of) functions in strbuf.h is to > operate on strbufs, thus it makes sense for the strbuf to be the first > argument (just like `this` is the invisible first argument of instance > methods in C++ which operate on an instance of the class). However, > infer_backlink()'s purpose isn't to operate on a strbuf; the fact that > the original signature neither accepted nor returned a strbuf bears > that out. The really important input to this function is `gitfile`, > thus it makes sense for this argument to come first. The strbuf which > this patch makes it use is a mere implementation detail (a > micro-optimization) which doesn't otherwise change the function's > original purpose, thus it belongs in a less prominent position in the > argument list. I can reverse the arguments. > ... this code now becomes more than a little confusing to read. It > says "if infer_backlink then signal error", which sounds rather > backward and leaves the reader scratching his or her head. ("Why > signal error if the function succeeded?"). Hence, infer_backlink() > should probably return 1 on success and 0 on failure, which will make > this code read more idiomatically: > > if (!infer_backlink(realdotgit.buf, &backlink)) { > ...signal error... This was my first thought, however, on unix it is fairly standard to return 0 if successful and a non-zero int if there's an error. I don't mind updating, but I want to follow what makes the most sense and would be most expected.
On Sun, Oct 6, 2024 at 10:42 PM Caleb White <cdwhite3@pm.me> wrote: > On Sunday, October 6th, 2024 at 13:16, Eric Sunshine <sunshine@sunshineco.com> wrote: > > ... this code now becomes more than a little confusing to read. It > > says "if infer_backlink then signal error", which sounds rather > > backward and leaves the reader scratching his or her head. ("Why > > signal error if the function succeeded?"). Hence, infer_backlink() > > should probably return 1 on success and 0 on failure, which will make > > this code read more idiomatically: > > > > if (!infer_backlink(realdotgit.buf, &backlink)) { > > ...signal error... > > This was my first thought, however, on unix it is fairly standard > to return 0 if successful and a non-zero int if there's an error. > I don't mind updating, but I want to follow what makes the most > sense and would be most expected. I mentioned it because it made my reading hiccup, but I don't feel too strongly about it one way or the other considering that this is an internal function.
On Sunday, October 6th, 2024 at 22:26, Eric Sunshine <sunshine@sunshineco.com> wrote: > I mentioned it because it made my reading hiccup, but I don't feel too > strongly about it one way or the other considering that this is an > internal function. I reversed the flags :thumsup:
On Sun, Oct 06, 2024 at 02:41:22PM -0400, Eric Sunshine wrote: [snip] > > 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. > > 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
On Sunday, October 6th, 2024 at 22:56, shejialuo <shejialuo@gmail.com> wrote: > 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; } ```
On Mon, Oct 07, 2024 at 02:26:51AM +0000, Caleb White wrote: > On Sunday, October 6th, 2024 at 13:41, Eric Sunshine <sunshine@sunshineco.com> wrote: > > 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. > > I will rename to `dotgit_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); I agree with the idea that Eric proposed. It's truly we should raise a bug here. That's would be much more clear. > We can't use BUG because this is handled as part of the err > conditions. The contents can be NULL and `backlink` could be > filled with the inferred backlink. I moved this to the top > and I think it reads better. That's not correct. It is true that the contents can be NULL and `backlink` could be filled with the infer_backlink. But do you notice that if `backlink` was filled with the infer_backlink, it will jump to the "done" label. What Erie means is the following: git_contents = read_gitfile_gently(...); if (err) { if (...) { } else if (err == RAED_GITFILE_ERR_NOT_A_REPO) { // handle backlink goto done; } } if (!git_contents) BUG(...); strbuf_addstr(&backlink, git_contents); done: // cleanup operations Thanks, Jialuo
On Sunday, October 6th, 2024 at 23:12, shejialuo <shejialuo@gmail.com> wrote: > That's not correct. It is true that the contents can be NULL and > `backlink` could be filled with the infer_backlink. But do you notice > that if `backlink` was filled with the infer_backlink, it will jump > to the "done" label. This is not correct, if backlink is filled with `infer_backlink` it continues on with the processing. I have made this more clear: dotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err)); if (dotgit_contents) { if (is_absolute_path(dotgit_contents)) { strbuf_addstr(&backlink, dotgit_contents); } else { strbuf_addbuf(&backlink, &realdotgit); strbuf_strip_suffix(&backlink, ".git"); strbuf_addstr(&backlink, dotgit_contents); } } 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; }
On Mon, Oct 07, 2024 at 04:01:43AM +0000, Caleb White wrote: > On Sunday, October 6th, 2024 at 22:56, shejialuo <shejialuo@gmail.com> wrote: > > 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
On Mon, Oct 07, 2024 at 04:19:22AM +0000, Caleb White wrote: > On Sunday, October 6th, 2024 at 23:12, shejialuo <shejialuo@gmail.com> wrote: > > That's not correct. It is true that the contents can be NULL and > > `backlink` could be filled with the infer_backlink. But do you notice > > that if `backlink` was filled with the infer_backlink, it will jump > > to the "done" label. > > This is not correct, if backlink is filled with `infer_backlink` it > continues on with the processing. I have made this more clear: > > dotgit_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err)); > if (dotgit_contents) { > if (is_absolute_path(dotgit_contents)) { > strbuf_addstr(&backlink, dotgit_contents); > } else { > strbuf_addbuf(&backlink, &realdotgit); > strbuf_strip_suffix(&backlink, ".git"); > strbuf_addstr(&backlink, dotgit_contents); > } > } 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; > } > > Thanks, you are correct here. We cannot use "BUG" here. And I think currently this is OK.
On Sunday, October 6th, 2024 at 23:28, shejialuo <shejialuo@gmail.com> wrote: > > } else if (err) { > > fn(1, realdotgit.buf, _("unable to locate repository; .git file broken"), cb_data); > > goto done; > > } > > > Thanks, you are correct here. We cannot use "BUG" here. And I think > currently this is OK. Granted, that last `else if` could just become an `else` statement now, I'll make that change.
diff --git a/worktree.c b/worktree.c index 0f032cc..c6d2ede 100644 --- a/worktree.c +++ b/worktree.c @@ -642,10 +642,9 @@ static int is_main_worktree_path(const char *path) * be able to infer the gitdir by manually reading /path/to/worktree/.git, * extracting the <id>, and checking if <repo>/worktrees/<id> exists. */ -static char *infer_backlink(const char *gitfile) +static int infer_backlink(st ruct strbuf *inferred, const char *gitfile) { struct strbuf actual = STRBUF_INIT; - struct strbuf inferred = STRBUF_INIT; const char *id;
This refactors the `infer_backlink` function to return an integer result and use a pre-allocated `strbuf` for the inferred backlink path, replacing the previous `char*` return type. This lays the groundwork for the next patch, which needs the resultant backlink as a `strbuf`. There was no need to go from `strbuf -> char* -> strbuf` again. This change also aligns the function's signature with other `strbuf`-based functions. Signed-off-by: Caleb White <cdwhite3@pm.me> --- worktree.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) if (strbuf_read_file(&actual, gitfile, 0) < 0) @@ -658,17 +657,16 @@ static char *infer_backlink(const char *gitfile) id++; /* advance past '/' to point at <id> */ if (!*id) goto error; - strbuf_git_common_path(&inferred, the_repository, "worktrees/%s", id); - if (!is_directory(inferred.buf)) + strbuf_git_common_path(inferred, the_repository, "worktrees/%s", id); + if (!is_directory(inferred->buf)) goto error; strbuf_release(&actual); - return strbuf_detach(&inferred, NULL); + return 0; error: strbuf_release(&actual); - strbuf_release(&inferred); - return NULL; + return 1; } /* @@ -680,9 +678,10 @@ void repair_worktree_at_path(const char *path, { struct strbuf dotgit = STRBUF_INIT; struct strbuf realdotgit = STRBUF_INIT; + struct strbuf backlink = STRBUF_INIT; struct strbuf gitd ir = STRBUF_INIT; struct strbuf olddotgit = STRBUF_INIT; - char *backlink = NULL; + char *git_contents = NULL; const char *repair = NULL; int err; @@ -698,21 +697,23 @@ void repair_worktree_at_path(const char *path, goto done; } - backlink = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err)); + git_contents = xstrdup_or_null(read_gitfile_gently(realdotgit.buf, &err)); 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_conte nts) { + strbuf_addstr(&backlink, git_contents); } - strbuf_addf(&gitdir, "%s/gitdir", backlink); + strbuf_addf(&gitdir, "%s/gitdir", backlink.buf); if (strbuf_read_file(&olddotgit, gitdir.buf, 0) < 0) repair = _("gitdir unreadable"); else { @@ -726,8 +727,9 @@ void repair_worktree_at_path(const char *path, write_file(gitdir.buf, "%s", realdotgit.buf); } done: - free(backlink); + free(git_contents); strbuf_release(&olddotgit); + strbuf_release(&backlink); strbuf_release(&gitdir); strbuf_release(&realdotgit); strbuf_release(&dotgit);