Re: [PATCH v2 1/4] worktree: refactor infer_backlink() to use *strbuf
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Oct 6, 2024, 18:16 UTC
- Message-ID
- <CAPig+cSe0ykN=Mmb8Nwp112ROOCuhzzP0jofvL91PsVi8W4dHg@mail.gmail.com>
- In-Reply-To
- <20241006060017.171788-2-cdwhite3@pm.me>
On Sun, Oct 6, 2024 at 2:01 AM Caleb White <cdwhite3@pm.me> wrote:
Show 8 quoted lines
> 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...
Show 6 quoted lines
> 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.
Show 9 quoted lines
> @@ -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...
Show 5 quoted lines
> 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...
Show 6 quoted lines
> @@ -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...