On Sun, Oct 6, 2024 at 2:01 AM Caleb White <cdwhite3@xxxxx> 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@xxxxx>
> ---
> 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...
