nbelakovski@xxxxxxxxx writes:
> From: Nickolai Belakovski <nbelakovski@xxxxxxxxx>
>
> lock_reason_valid is renamed to is_locked and lock_reason is removed as
> a field of the worktree struct. Lock reason can be obtained instead by a
> standalone function.
>
> This is done in order to make the worktree struct more intuitive when it
> is used elsewhere in the codebase.
So a mere action of getting an in-core worktree instance now has to
make an extra call to file_exists(), and in addition, the callers
who want to learn why the worktree is locked, they need to open and
read the contents of the file in addition?
Why is that an improvement?
>
> Some unused variables are cleaned up as well.
>
> Signed-off-by: Nickolai Belakovski <nbelakovski@xxxxxxxxx>
> ---
> builtin/worktree.c | 16 ++++++++--------
> worktree.c | 55 ++++++++++++++++++++++++++++--------------------------
> worktree.h | 8 +++-----
> 3 files changed, 40 insertions(+), 39 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 41e771439..844789a21 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -634,8 +634,8 @@ static int lock_worktree(int ac, const char **av, const char *prefix)
> if (is_main_worktree(wt))
> die(_("The main working tree cannot be locked or unlocked"));
>
> - old_reason = is_worktree_locked(wt);
> - if (old_reason) {
> + if (wt->is_locked) {
> + old_reason = worktree_locked_reason(wt);
> if (*old_reason)
> die(_("'%s' is already locked, reason: %s"),
> av[0], old_reason);
> @@ -666,7 +666,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)
> die(_("'%s' is not a working tree"), av[0]);
> if (is_main_worktree(wt))
> die(_("The main working tree cannot be locked or unlocked"));
> - if (!is_worktree_locked(wt))
> + if (!wt->is_locked)
> die(_("'%s' is not locked"), av[0]);
> ret = unlink_or_warn(git_common_path("worktrees/%s/locked", wt->id));
> free_worktrees(worktrees);
> @@ -734,8 +734,8 @@ static int move_worktree(int ac, const char **av, const char *prefix)
>
> validate_no_submodules(wt);
>
> - reason = is_worktree_locked(wt);
> - if (reason) {
> + if (wt->is_locked) {
> + reason = worktree_locked_reason(wt);
> if (*reason)
> die(_("cannot move a locked working tree, lock reason: %s"),
> reason);
> @@ -860,11 +860,11 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
> die(_("'%s' is not a working tree"), av[0]);
> if (is_main_worktree(wt))
> die(_("'%s' is a main working tree"), av[0]);
> - reason = is_worktree_locked(wt);
> - if (reason) {
> + if (wt->is_locked) {
> + reason = worktree_locked_reason(wt);
> if (*reason)
> die(_("cannot remove a locked working tree, lock reason: %s"),
> - reason);
> + reason);
> die(_("cannot remove a locked working tree"));
> }
> if (validate_worktree(wt, &errmsg, WT_VALIDATE_WORKTREE_MISSING_OK))
> diff --git a/worktree.c b/worktree.c
> index 97cda5f97..a3082d19d 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -14,7 +14,6 @@ void free_worktrees(struct worktree **worktrees)
> free(worktrees[i]->path);
> free(worktrees[i]->id);
> free(worktrees[i]->head_ref);
> - free(worktrees[i]->lock_reason);
> free(worktrees[i]);
> }
> free (worktrees);
> @@ -41,13 +40,29 @@ static void add_head_info(struct worktree *wt)
> wt->is_detached = 1;
> }
>
> +
> +/**
> + * Return 1 if the worktree is locked, 0 otherwise
> + */
> +static int is_worktree_locked(const struct worktree *wt)
> +{
> + struct strbuf path = STRBUF_INIT;
> + int locked_file_exists;
> +
> + assert(!is_main_worktree(wt));
> +
> + strbuf_addstr(&path, worktree_git_path(wt, "locked"));
> + locked_file_exists = file_exists(path.buf);
> + strbuf_release(&path);
> + return locked_file_exists;
> +}
> +
> /**
> * get the main worktree
> */
> static struct worktree *get_main_worktree(void)
> {
> struct worktree *worktree = NULL;
> - struct strbuf path = STRBUF_INIT;
> struct strbuf worktree_path = STRBUF_INIT;
> int is_bare = 0;
>
> @@ -56,14 +71,11 @@ static struct worktree *get_main_worktree(void)
> if (is_bare)
> strbuf_strip_suffix(&worktree_path, "/.");
>
> - strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
> -
> worktree = xcalloc(1, sizeof(*worktree));
> worktree->path = strbuf_detach(&worktree_path, NULL);
> worktree->is_bare = is_bare;
> add_head_info(worktree);
>
> - strbuf_release(&path);
> strbuf_release(&worktree_path);
> return worktree;
> }
> @@ -89,12 +101,10 @@ static struct worktree *get_linked_worktree(const char *id)
> strbuf_strip_suffix(&worktree_path, "/.");
> }
>
> - strbuf_reset(&path);
> - strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
> -
> worktree = xcalloc(1, sizeof(*worktree));
> worktree->path = strbuf_detach(&worktree_path, NULL);
> worktree->id = xstrdup(id);
> + worktree->is_locked = is_worktree_locked(worktree);
> add_head_info(worktree);
>
> done:
> @@ -231,27 +241,20 @@ int is_main_worktree(const struct worktree *wt)
> return !wt->id;
> }
>
> -const char *is_worktree_locked(struct worktree *wt)
> +const char *worktree_locked_reason(const struct worktree *wt)
> {
> - assert(!is_main_worktree(wt));
> + struct strbuf path = STRBUF_INIT;
> + struct strbuf lock_reason = STRBUF_INIT;
>
> - if (!wt->lock_reason_valid) {
> - struct strbuf path = STRBUF_INIT;
> -
> - strbuf_addstr(&path, worktree_git_path(wt, "locked"));
> - if (file_exists(path.buf)) {
> - struct strbuf lock_reason = STRBUF_INIT;
> - if (strbuf_read_file(&lock_reason, path.buf, 0) < 0)
> - die_errno(_("failed to read '%s'"), path.buf);
> - strbuf_trim(&lock_reason);
> - wt->lock_reason = strbuf_detach(&lock_reason, NULL);
> - } else
> - wt->lock_reason = NULL;
> - wt->lock_reason_valid = 1;
> - strbuf_release(&path);
> - }
> + assert(!is_main_worktree(wt));
> + assert(wt->is_locked);
>
> - return wt->lock_reason;
> + strbuf_addstr(&path, worktree_git_path(wt, "locked"));
> + if (strbuf_read_file(&lock_reason, path.buf, 0) < 0)
> + die_errno(_("failed to read '%s'"), path.buf);
> + strbuf_trim(&lock_reason);
> + strbuf_release(&path);
> + return strbuf_detach(&lock_reason, NULL);
> }
>
> /* convenient wrapper to deal with NULL strbuf */
> diff --git a/worktree.h b/worktree.h
> index df3fc30f7..6717287e8 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -10,12 +10,11 @@ struct worktree {
> char *path;
> char *id;
> char *head_ref; /* NULL if HEAD is broken or detached */
> - char *lock_reason; /* internal use */
> struct object_id head_oid;
> int is_detached;
> int is_bare;
> int is_current;
> - int lock_reason_valid;
> + int is_locked;
> };
>
> /* Functions for acting on the information about worktrees. */
> @@ -57,10 +56,9 @@ extern struct worktree *find_worktree(struct worktree **list,
> extern int is_main_worktree(const struct worktree *wt);
>
> /*
> - * Return the reason string if the given worktree is locked or NULL
> - * otherwise.
> + * Return the reason string if the given worktree is locked or die
> */
> -extern const char *is_worktree_locked(struct worktree *wt);
> +extern const char *worktree_locked_reason(const struct worktree *wt);
>
> #define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)
