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@xxxxx>
> ---
> 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
