From: Kyle Ackerman <kack@kyleackerman.net>
Subject: Re: simplify worktree_open()
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Mon, 05 May 2025 00:19:26 +0000

On Monday, April 14th, 2025 at 3:09 AM, Stefan Sperling <stsp@stsp.name> wrote:

> 
> 
> On Thu, Apr 10, 2025 at 06:19:32PM +0200, Stefan Sperling wrote:
> 
> > I noticed this while investigating the tog issue reported by xs on IRC.
> > 
> > Opening a bunch of files in /tmp for pack temp files and opening the
> > repository just to validate the work tree's base-commit ID is overkill.
> > Simply parse the ID directly. If the commit object no longer exists we
> > will run into another error sooner or later. The commit is already
> > protected by the work tree reference. If it is missing then something
> > has badly gone wrong.
> > 
> > ok?
> 
> 
> Here is an alternative fix which obtains the hash algorithm from the
> Git config file in the repository. Omar pointed out on IRC that this
> might be better than guessing an algorithm based on strlen().
> 
> I don't mind either way, both approaches work for me.
> 
> The strlen() approach makes 'got status' on my got work tree a bit
> faster (0.05 seconds vs 0.10 seconds). But I do not actually care
> about saving time at such a small scale.
> 
I am almost leaning towards the first patch since it completely removes
got_repo_open from worktree_open. It seems common in got for something like
the following:

	error = got_worktree_open(&worktree, cwd, GOT_WORKTREE_GOT_DIR);
	error = got_repo_open(&repo, got_worktree_get_repo_path(worktree), NULL,

Which means that there are basically 2 calls to got_repo_open, which means there
two privsep calls to got-read-gitconfig. Which isn't a big deal, but my thoughts are to
prevent duplicate work calls.
> 
> M lib/worktree_open.c | 16+ 15-
> 
> 1 file changed, 16 insertions(+), 15 deletions(-)
> 
> commit - 288f62bc8681fd4d88d574527fe45052a7d641b7
> commit + 05215130761193aab0b03ffe941ad82ed9585e7e
> blob - 74e2affe2cf926ca05691a6b59ec0592908b3f12
> blob + 24c199dd19ba921a3ada632314946180a9803fb0
> --- lib/worktree_open.c
> +++ lib/worktree_open.c
> @@ -41,6 +41,7 @@
> 
> #include "got_lib_worktree.h"
> #include "got_lib_gotconfig.h"
> +#include "got_lib_hash.h"
> 
> #ifndef nitems
> #define nitems(_a) (sizeof((_a)) / sizeof((_a)[0]))
> @@ -124,14 +125,18 @@ open_worktree(struct got_worktree **worktree, const ch
> char *uuidstr = NULL;
> char *path_lock = NULL;
> char *base_commit_id_str = NULL;
> + struct got_object_id *base_commit_id = NULL;
> int version, fd = -1;
> const char *errstr;
> struct got_repository *repo = NULL;
> - int *pack_fds = NULL;
> uint32_t uuid_status;
> 
> *worktree = NULL;
> 
> + base_commit_id = calloc(1, sizeof(*base_commit_id));
> + if (base_commit_id == NULL)
> + return got_error_from_errno("calloc");
> +
> if (asprintf(&path_meta, "%s/%s", path, meta_dir) == -1) {
> err = got_error_from_errno("asprintf");
> path_meta = NULL;
> @@ -204,18 +209,19 @@ open_worktree(struct got_worktree **worktree, const ch
> goto done;
> }
> 
> - err = got_repo_pack_fds_open(&pack_fds);
> + err = got_repo_open(&repo, (*worktree)->repo_path, NULL, NULL);
> 
> if (err)
> goto done;
> 
> - err = got_repo_open(&repo, (*worktree)->repo_path, NULL, pack_fds);
> 
> - if (err)
> - goto done;
> + if (!got_parse_object_id(base_commit_id, base_commit_id_str,
> + got_repo_get_object_format(repo))) {
> + err = got_error_fmt(GOT_ERR_BAD_OBJ_ID_STR,
> + "%s/%s", meta_dir, GOT_WORKTREE_BASE_COMMIT);
> + goto done;
> + }
> 
> - err = got_object_resolve_id_str(&(*worktree)->base_commit_id, repo,
> 
> - base_commit_id_str);
> - if (err)
> - goto done;
> + (*worktree)->base_commit_id = base_commit_id;
> 
> + base_commit_id = NULL;
> 
> err = read_meta_file(&(*worktree)->head_ref_name, path_meta,
> 
> GOT_WORKTREE_HEAD_REF);
> @@ -246,15 +252,10 @@ done:
> if (err == NULL)
> err = close_err;
> }
> - if (pack_fds) {
> - const struct got_error *pack_err =
> - got_repo_pack_fds_close(pack_fds);
> - if (err == NULL)
> - err = pack_err;
> - }
> free(path_meta);
> free(path_lock);
> free(base_commit_id_str);
> + free(base_commit_id);
> free(uuidstr);
> free(formatstr);
> if (err) {