[PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files

[PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files

[nbelakovski]

From: Nickolai Belakovski <nbelakovski@gmail.com>

lock_reason is now populated during the execution of get_worktrees

is_worktree_locked has been simplified, renamed, and changed to internal
linkage. It is simplified to only return the lock reason (or NULL in case
there is no lock reason) and to not have any side effects on the inputs.
As such it made sense to rename it since it only returns the reason.

Since this function was now being used to populate the worktree struct's
lock_reason field, it made sense to move the function to internal
linkage and have callers refer to the lock_reason field. The
lock_reason_valid field was removed since a NULL/non-NULL value of
lock_reason accomplishes the same effect.

Some unused variables within worktree source code were removed.

Signed-off-by: Nickolai Belakovski <nbelakovski@gmail.com>
---

Notes:
    Travis CI results: https://travis-ci.org/nbelakovski/git/builds/445500127

 builtin/worktree.c | 27 +++++++++++----------------
 worktree.c         | 53 +++++++++++++++++++++++------------------------------
 worktree.h         |  9 +--------
 3 files changed, 35 insertions(+), 54 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 41e771439..fb203f61d 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -615,7 +615,7 @@ static int list(int ac, const char **av, const char *prefix)
 
 static int lock_worktree(int ac, const char **av, const char *prefix)
 {
-	const char *reason = "", *old_reason;
+	const char *reason = "";
 	struct option options[] = {
 		OPT_STRING(0, "reason", &reason, N_("string"),
 			   N_("reason for locking")),
@@ -634,11 +634,10 @@ 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 (*old_reason)
+	if (wt->lock_reason) {
+		if (*wt->lock_reason)
 			die(_("'%s' is already locked, reason: %s"),
-			    av[0], old_reason);
+			    av[0], wt->lock_reason);
 		die(_("'%s' is already locked"), av[0]);
 	}
 
@@ -666,7 +665,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->lock_reason)
 		die(_("'%s' is not locked"), av[0]);
 	ret = unlink_or_warn(git_common_path("worktrees/%s/locked", wt->id));
 	free_worktrees(worktrees);
@@ -703,7 +702,6 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	struct worktree **worktrees, *wt;
 	struct strbuf dst = STRBUF_INIT;
 	struct strbuf errmsg = STRBUF_INIT;
-	const char *reason;
 	char *path;
 
 	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
@@ -734,11 +732,10 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 
 	validate_no_submodules(wt);
 
-	reason = is_worktree_locked(wt);
-	if (reason) {
-		if (*reason)
+	if (wt->lock_reason) {
+		if (*wt->lock_reason)
 			die(_("cannot move a locked working tree, lock reason: %s"),
-			    reason);
+			    wt->lock_reason);
 		die(_("cannot move a locked working tree"));
 	}
 	if (validate_worktree(wt, &errmsg, 0))
@@ -847,7 +844,6 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
 	};
 	struct worktree **worktrees, *wt;
 	struct strbuf errmsg = STRBUF_INIT;
-	const char *reason;
 	int ret = 0;
 
 	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
@@ -860,11 +856,10 @@ 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 (*reason)
+	if (wt->lock_reason) {
+		if (*wt->lock_reason)
 			die(_("cannot remove a locked working tree, lock reason: %s"),
-			    reason);
+			    wt->lock_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..3bd25983c 100644
--- a/worktree.c
+++ b/worktree.c
@@ -41,13 +41,34 @@ static void add_head_info(struct worktree *wt)
 		wt->is_detached = 1;
 }
 
+/**
+ * Return the reason the worktree is locked, or NULL if it is not locked
+ */
+static char *worktree_locked_reason(const struct worktree *wt)
+{
+	struct strbuf lock_reason = STRBUF_INIT;
+	struct strbuf path = STRBUF_INIT;
+
+	assert(!is_main_worktree(wt));
+
+	strbuf_addstr(&path, worktree_git_path(wt, "locked"));
+	if (file_exists(path.buf)) {
+		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);
+	}
+	strbuf_release(&path);
+	return NULL;
+}
+
 /**
  * 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 +77,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 +107,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->lock_reason = worktree_locked_reason(worktree);
 	add_head_info(worktree);
 
 done:
@@ -231,29 +247,6 @@ int is_main_worktree(const struct worktree *wt)
 	return !wt->id;
 }
 
-const char *is_worktree_locked(struct worktree *wt)
-{
-	assert(!is_main_worktree(wt));
-
-	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);
-	}
-
-	return wt->lock_reason;
-}
-
 /* convenient wrapper to deal with NULL strbuf */
 static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...)
 {
diff --git a/worktree.h b/worktree.h
index df3fc30f7..0214630fd 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 */
+	char *lock_reason;
 	struct object_id head_oid;
 	int is_detached;
 	int is_bare;
 	int is_current;
-	int lock_reason_valid;
 };
 
 /* Functions for acting on the information about worktrees. */
@@ -56,12 +55,6 @@ 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.
- */
-extern const char *is_worktree_locked(struct worktree *wt);
-
 #define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)
 
 /*
-- 
2.14.2

Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files

[Eric Sunshine]

On Wed, Oct 24, 2018 at 2:39 AM <nbelakovski@gmail.com> wrote:
> lock_reason is now populated during the execution of get_worktrees
>
> is_worktree_locked has been simplified, renamed, and changed to internal
> linkage. It is simplified to only return the lock reason (or NULL in case
> there is no lock reason) and to not have any side effects on the inputs.
> As such it made sense to rename it since it only returns the reason.
>
> Since this function was now being used to populate the worktree struct's
> lock_reason field, it made sense to move the function to internal
> linkage and have callers refer to the lock_reason field. The
> lock_reason_valid field was removed since a NULL/non-NULL value of
> lock_reason accomplishes the same effect.
>
> Some unused variables within worktree source code were removed.

Thanks for the submission.

One thing which isn't clear from this commit message is _why_ this
change is desirable at this time, aside from the obvious
simplification of the code and client interaction (or perhaps those
are the _why_?).

Although I had envisioned populating the "reason" field greedily in
the way this patch does, not everyone agrees that doing so is
desirable. In particular, Junio argued[1,2] for populating it lazily,
which accounts for the current implementation. That's why I ask about
the _why_ of this change since it will likely need to be justified in
a such a way to convince Junio to change his mind.

Thanks.

[1]: https://public-inbox.org/git/xmqq8tyq5czn.fsf@gitster.mtv.corp.google.com/
[2]: https://public-inbox.org/git/xmqq4m9d0w6v.fsf@gitster.mtv.corp.google.com/

Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files

[Nickolai Belakovski]

The motivation for the change is some work that I'm doing to add a
worktree atom in ref-filter.c. I wanted that atom to be able to access
all fields of the worktree struct and noticed that lock_reason wasn't
getting populated so I figured I'd go and fix that.

I figured that since get_worktrees is already hitting the filesystem
for the directory it wouldn't matter much if it also went and got the
lock reason.

Reviewing this work in the context of your feedback and Junio's
previous comments, I think it makes sense to only have a field in the
struct indicating whether or not the worktree is locked, and have a
separate function for getting the reason. Since the only cases in
which the reason is retrieved in the current codebase are cases where
the program immediately dies, caching seems a moot point. I'll send an
updated patch just after this message.

Thanks for the feedback, happy to receive more.
On Wed, Oct 24, 2018 at 1:11 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Wed, Oct 24, 2018 at 2:39 AM <nbelakovski@gmail.com> wrote:
> > lock_reason is now populated during the execution of get_worktrees
> >
> > is_worktree_locked has been simplified, renamed, and changed to internal
> > linkage. It is simplified to only return the lock reason (or NULL in case
> > there is no lock reason) and to not have any side effects on the inputs.
> > As such it made sense to rename it since it only returns the reason.
> >
> > Since this function was now being used to populate the worktree struct's
> > lock_reason field, it made sense to move the function to internal
> > linkage and have callers refer to the lock_reason field. The
> > lock_reason_valid field was removed since a NULL/non-NULL value of
> > lock_reason accomplishes the same effect.
> >
> > Some unused variables within worktree source code were removed.
>
> Thanks for the submission.
>
> One thing which isn't clear from this commit message is _why_ this
> change is desirable at this time, aside from the obvious
> simplification of the code and client interaction (or perhaps those
> are the _why_?).
>
> Although I had envisioned populating the "reason" field greedily in
> the way this patch does, not everyone agrees that doing so is
> desirable. In particular, Junio argued[1,2] for populating it lazily,
> which accounts for the current implementation. That's why I ask about
> the _why_ of this change since it will likely need to be justified in
> a such a way to convince Junio to change his mind.
>
> Thanks.
>
> [1]: https://public-inbox.org/git/xmqq8tyq5czn.fsf@gitster.mtv.corp.google.com/
> [2]: https://public-inbox.org/git/xmqq4m9d0w6v.fsf@gitster.mtv.corp.google.com/

[PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible

[nbelakovski]

From: Nickolai Belakovski <nbelakovski@gmail.com>

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.

Some unused variables are cleaned up as well.

Signed-off-by: Nickolai Belakovski <nbelakovski@gmail.com>
---
 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)
 
-- 
2.14.2

Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible

[Junio C Hamano]

nbelakovski@gmail.com writes:

> From: Nickolai Belakovski <nbelakovski@gmail.com>
>
> 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@gmail.com>
> ---
>  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)

Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible

[Nickolai Belakovski]

This was meant to be a reply to
https://public-inbox.org/git/CAC05386F1X7TsPr6kgkuLWEwsmdiQ4VKTF5RxaHvzpkwbmXPBw@mail.gmail.com/T/#m8898c8f7c68e1ea234aca21cb2d7776b375c6f51,
please look there for some more context. I think it both did and
didn't get listed as a reply? In my mailbox I see two separate threads
but in public-inbox.org/git it looks like it correctly got labelled as
1 thread. This whole mailing list thing is new to me, thanks for
bearing with me as I figure it out :). Next time I'll make sure to
change the subject line on updated patches as PATCH v2 (that's the
convention, right?).

This is an improvement because it fixes an issue in which the fields
lock_reason and lock_reason_valid of the worktree struct were not
being populated. This is related to work I'm doing to add a worktree
atom to ref-filter.c.

I see your concerns about extra hits to the filesystem when calling
get_worktrees and about users interested in lock_reason having to make
extra calls. As regards hits to the filesystem, I could remove
is_locked from the worktree struct entirely. To address the second
concern, I could refactor worktree_locked_reason to return null if the
wt is not locked. I would still want to keep is_worktree_locked around
to provide a facility to check whether or not the worktree is locked
without having to go get the reason.

There's also been some concerns raised about caching. As I pointed out
in the other thread, the current use cases for this information die
upon accessing it, so caching is a moot point. For the use case of a
worktree atom, caching would be relevant, but it could be done within
ref-filter.c. Another option is to add the lock_reason back to the
worktree struct and have two functions for populating it:
get_worktrees_wo_lock_reason and get_worktrees_with_lock_reason. A bit
more verbose, but it makes it clear to the caller what they're getting
and what they're not getting. I might suggest starting with doing the
caching within ref-filter.c first, and if more use cases appear for
caching lock_reason we can consider the second option. It could also
be get_worktrees and get_worktrees_wo_lock_reason, though I think most
callers would be calling the latter name.

So, my proposal for driving this patch to completion would be to:
-remove is_locked from the worktree struct
-refactor worktree_locked_reason to return null if the wt is not locked
-refactor calls to is_locked within builtin/worktree.c to call either
the refactored worktree_locked_reason or is_worktree_locked

In addition to making the worktree code clearer, this patch fixes a
bug in which the current is_worktree_locked over-eagerly sets
lock_reason_valid. There are currently no consumers of
lock_reason_valid within master, but obviously we should fix this
before they appear :)

Thoughts?

On Wed, Oct 24, 2018 at 11:56 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> nbelakovski@gmail.com writes:
>
> > From: Nickolai Belakovski <nbelakovski@gmail.com>
> >
> > 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@gmail.com>
> > ---
> >  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)

Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible

[Junio C Hamano]

Nickolai Belakovski <nbelakovski@gmail.com> writes:

> This is an improvement because it fixes an issue in which the fields
> lock_reason and lock_reason_valid of the worktree struct were not
> being populated.

If the field "reason" should always be populated, there is *no*
reason why we need the "valid" boolean.  They work as a pair to
realize lazy population of rarely used field.  The lazy evaluation
technique is used as an optimization for common case, where majority
of operations do not care if worktrees are locked and if so why they
are locked, so that only rare operations that do want to find out
can ask "is this locked and why?" via is_worktree_locked() interface,
and at that point we lazily find it out by reading "locked" file.

So it is by design that these fields are not always populated, but
are populated on demand as book-keeping info internal to the API's
implementation.  It is not "an issue", and changing it is not a
"fix".

In addition, if we have already checked, then we do not even do the
same check again.  If in an earlier call we found out that a worktree
is not locked, we flip the _valid bit to true while setting _reason
to NULL, so that the next call can say "oh, that's not locked and we
can tell that without looking at the filesystem again" [*1*].

You are forcing the callers of get_worktrees() to pay the cost to
check, open and read the "why is this worktree locked?" file for all
worktrees, whether they care if these worktrees are locked or why
they are locked.  Such a change can be an improvement *ONLY* if you
can demonstrate that in the current code most codepaths that call
get_worktrees() end up calling is_worktree_locked() on all worktrees
anyways.  If that were the case, not having to lazily evaluate the
"locked"-ness, but always check upfront, would have a simplification
value, as either approach would be spending the same cost to open
and read these "locked" files.

But I do not think it is the case.  Outside builtin/worktree.c (and
you need to admit "git worktree" is a rather rare command in the
first place, so you shouldn't be optimizing for that if it hurts
other codepaths), builtin/branch.c wants to go to all worktrees and
update their HEAD when a branch is renamed (if the old HEAD is
pointing at the original name, of course), but that code won't care
if the worktree is locked at all.  I do not think of any caller of
get_worktrees() that want to know if it is locked and why for each
and every one of them, and I'd be surprised if that *is* the
majority, but as a proposer to burden get_worktrees() with this
extra cost, you certainly would have audited the callers and made
sure it is worth making them pay the extra cost?

If we are going to change anything around this area, I'd not be
surprised that the right move is to go in the opposite direction.
Right now, you cannot just get "is it locked?" boolean answer (which
can be obtained by a simple stat(2) call) without getting "why is it
locked?" (which takes open(2) & read(2) & close(2)), and if you are
planning a new application that wants to ask "is it locked?" a lot
without having to know the reason, you may want to make the lazy
evaluation even lazier by splitting _valid field into two (i.e. a
"do we know if this is locked?" valid bit covers "is_locked" bit,
and another "do we know why this is locked?" valid bit accompanies
"locked_reason" string).  And the callers would ask two separate
questions: is_worktree_locked() that says true or false, and then
why_worktree_locked() that yields NULL or string (i.e. essentially
that is what we have as is_worktree_locked() today).  Of course,
such a change must also be justified with a code audit to
demonstrate that only minority case of the callers of is-locked?
wants to know why


[Footnote]

*1* The codepaths that want to know if a worktree is locked or not
(and wants to learn the reason) are so rare and concentrated in
builtin/worktree.c, and more importantly, they do not need to ask
the same question twice, so we can stop caching and make
is_worktree_locked() always go to the filesystem, I think, and that
may be a valid change _if_ we allow worktrees to be randomly locked
and unlocked while we are looking at them, but if we want to worry
about such concurrent and competing uses, we need a big
repository-wide lock anyway, and it is the least of our problems
that the current caching may go stale without getting invalidated.
The code will be racing against such concurrent processes even if
you made it to go to the filesystem all the time.

Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible

[Nickolai Belakovski]

On Sun, Oct 28, 2018 at 8:52 PM Junio C Hamano <gitster@pobox.com> wrote:
>
>
> If the field "reason" should always be populated, there is *no*
> reason why we need the "valid" boolean.  They work as a pair to
> realize lazy population of rarely used field.  The lazy evaluation
> technique is used as an optimization for common case, where majority
> of operations do not care if worktrees are locked and if so why they
> are locked, so that only rare operations that do want to find out
> can ask "is this locked and why?" via is_worktree_locked() interface,
> and at that point we lazily find it out by reading "locked" file.
>
> So it is by design that these fields are not always populated, but
> are populated on demand as book-keeping info internal to the API's
> implementation.  It is not "an issue", and changing it is not a
> "fix".

Having fields in a struct that are not populated by a getter function
with no documentation indicating that they are not populated and no
documentation explaining how to populate them is the issue here.

>
> In addition, if we have already checked, then we do not even do the
> same check again.  If in an earlier call we found out that a worktree
> is not locked, we flip the _valid bit to true while setting _reason
> to NULL, so that the next call can say "oh, that's not locked and we
> can tell that without looking at the filesystem again" [*1*].

I clearly misunderstood the use case of the _valid flag, thanks for
pointing it out.

>
> You are forcing the callers of get_worktrees() to pay the cost to
> check, open and read the "why is this worktree locked?" file for all
> worktrees, whether they care if these worktrees are locked or why
> they are locked.  Such a change can be an improvement *ONLY* if you
> can demonstrate that in the current code most codepaths that call
> get_worktrees() end up calling is_worktree_locked() on all worktrees
> anyways.  If that were the case, not having to lazily evaluate the
> "locked"-ness, but always check upfront, would have a simplification
> value, as either approach would be spending the same cost to open
> and read these "locked" files.
>
> But I do not think it is the case.  Outside builtin/worktree.c (and
> you need to admit "git worktree" is a rather rare command in the
> first place, so you shouldn't be optimizing for that if it hurts
> other codepaths), builtin/branch.c wants to go to all worktrees and
> update their HEAD when a branch is renamed (if the old HEAD is
> pointing at the original name, of course), but that code won't care
> if the worktree is locked at all.  I do not think of any caller of
> get_worktrees() that want to know if it is locked and why for each
> and every one of them, and I'd be surprised if that *is* the
> majority, but as a proposer to burden get_worktrees() with this
> extra cost, you certainly would have audited the callers and made
> sure it is worth making them pay the extra cost?
>
> If we are going to change anything around this area, I'd not be
> surprised that the right move is to go in the opposite direction.
> Right now, you cannot just get "is it locked?" boolean answer (which
> can be obtained by a simple stat(2) call) without getting "why is it
> locked?" (which takes open(2) & read(2) & close(2)), and if you are
> planning a new application that wants to ask "is it locked?" a lot
> without having to know the reason, you may want to make the lazy
> evaluation even lazier by splitting _valid field into two (i.e. a
> "do we know if this is locked?" valid bit covers "is_locked" bit,
> and another "do we know why this is locked?" valid bit accompanies
> "locked_reason" string).  And the callers would ask two separate
> questions: is_worktree_locked() that says true or false, and then
> why_worktree_locked() that yields NULL or string (i.e. essentially
> that is what we have as is_worktree_locked() today).  Of course,
> such a change must also be justified with a code audit to
> demonstrate that only minority case of the callers of is-locked?
> wants to know why
>
>
> [Footnote]
>
> *1* The codepaths that want to know if a worktree is locked or not
> (and wants to learn the reason) are so rare and concentrated in
> builtin/worktree.c, and more importantly, they do not need to ask
> the same question twice, so we can stop caching and make
> is_worktree_locked() always go to the filesystem, I think, and that
> may be a valid change _if_ we allow worktrees to be randomly locked
> and unlocked while we are looking at them, but if we want to worry
> about such concurrent and competing uses, we need a big
> repository-wide lock anyway, and it is the least of our problems
> that the current caching may go stale without getting invalidated.
> The code will be racing against such concurrent processes even if
> you made it to go to the filesystem all the time.
>

Basically, I already implemented most of what you're saying. The v2
proposal does force all callers of get_worktrees to check the lock
status, but by calling stat, not open/read/close. That being said
you're right that even forcing them to call stat when most don't care
is imposing an extra cost for no gain. The v2 proposal no longer
caches the lock reason (in fact it removes it from the worktree
struct), since not only do current users have no need to ask for the
lock_reason twice, none of them ask for it twice in the first place.
The v2 proposal provides a standalone function for getting the actual
reason (leaving it up to callers to cache the result if they like).

I'd be up for removing is_locked from the struct as well and making a
separate standalone function for that.

Either way, I do see an issue with the current code that anybody who
wants to know the lock status and/or lock reason of a worktree gets
faced with a confusing, misleading, and opaque piece of code. I see
two possible remedies:

a) Remove these fields from the worktree struct and provide standalone
functions for answering these questions. Pros are that we follow
single responsibility principle with two standalone functions for
doing so, and we make the route for answering these questions less
circuitous (IMO). Cons are that we remove the caching currently in
place since we're no longer storing this info in the struct, but then
again that caching is not currently being used and can be implemented
by callers if they really need it

b) Update the comments in the code to state that lock_reason and
lock_reason_valid are to be considered private fields and to use
is_worktree_locked for populating them. Pros are that no actual code
changes need to be made. Cons are that, IMO, it's still a strange
piece of code in that it's doing some sort of quasi object oriented
stuff in C, and if we can take the opportunity to make the code look a
bit more canonical I think we should, but that's just my 2 cents.

Of course there's also option c, which is that I leave this alone and
just go back to making my worktree atom :)

Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible

[Junio C Hamano]

Nickolai Belakovski <nbelakovski@gmail.com> writes:

> Either way, I do see an issue with the current code that anybody who
> wants to know the lock status and/or lock reason of a worktree gets
> faced with a confusing, misleading, and opaque piece of code.

Sorry, I don't.  I do not mind a better documentation for
is_worktree_locked() without doing anything else.

I do not see any reason to remove fields, split the helper funciton
into two, drop the caching, etc., especially when the only
justification is "I am new to the codebase and find it confusing".

Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible

[Eric Sunshine]

On Sun, Oct 28, 2018 at 5:55 PM Nickolai Belakovski
> <nbelakovski@gmail.com> wrote: This was meant to be a reply to
> https://public-inbox.org/git/CAC05386F1X7TsPr6kgkuLWEwsmdiQ4VKTF5RxaHvzpkwbmXPBw@mail.gmail.com/T/#m8898c8f7c68e1ea234aca21cb2d7776b375c6f51,
> please look there for some more context. I think it both did and
> didn't get listed as a reply? In my mailbox I see two separate
> threads but in public-inbox.org/git it looks like it correctly got
> labelled as 1 thread. This whole mailing list thing is new to me,
> thanks for bearing with me as I figure it out :).

Gmail threads messages entirely by subject; it doesn't pay attention
to In-Reply-To: or other headers for threading, which is why you see
two separate threads. public-inbox.org, on the other hand, does pay
attention to the headers, thus understands that all the messages
belong to the same thread. Gmail's behavior may be considered
anomalous.

> Next time I'll make sure to change the subject line on updated
> patches as PATCH v2 (that's the convention, right?).

That's correct.

> This is an improvement because it fixes an issue in which the fields
> lock_reason and lock_reason_valid of the worktree struct were not
> being populated. This is related to work I'm doing to add a worktree
> atom to ref-filter.c.

Those fields are considered private/internal. They are not intended to
be accessed by calling code. (Unfortunately, only 'lock_reason' is
thus marked; 'lock_reason_valid' should be marked "internal".) Clients
are expected to retrieve the lock reason only through the provided
API, is_worktree_locked().

> I see your concerns about extra hits to the filesystem when calling
> get_worktrees and about users interested in lock_reason having to
> make extra calls. As regards hits to the filesystem, I could remove
> is_locked from the worktree struct entirely. To address the second
> concern, I could refactor worktree_locked_reason to return null if
> the wt is not locked. I would still want to keep is_worktree_locked
> around to provide a facility to check whether or not the worktree is
> locked without having to go get the reason.
>
> There's also been some concerns raised about caching. As I pointed
> out in the other thread, the current use cases for this information
> die upon accessing it, so caching is a moot point. For the use case
> of a worktree atom, caching would be relevant, but it could be done
> within ref-filter.c. Another option is to add the lock_reason back
> to the worktree struct and have two functions for populating it:
> get_worktrees_wo_lock_reason and get_worktrees_with_lock_reason. A
> bit more verbose, but it makes it clear to the caller what they're
> getting and what they're not getting. I might suggest starting with
> doing the caching within ref-filter.c first, and if more use cases
> appear for caching lock_reason we can consider the second option. It
> could also be get_worktrees and get_worktrees_wo_lock_reason, though
> I think most callers would be calling the latter name.
>
> So, my proposal for driving this patch to completion would be to:
> -remove is_locked from the worktree struct
> -refactor worktree_locked_reason to return null if the wt is not locked
> -refactor calls to is_locked within builtin/worktree.c to call
> either the refactored worktree_locked_reason or is_worktree_locked

My impression, thus far, is that this all seems to be complicating
rather than simplifying. These changes also seem entirely unnecessary.
In [1], I made the observation that it seemed that your new ref-filter
atom could be implemented with the existing is_worktree_locked() API.
As far as I can tell, it can indeed be implemented without having to
make any changes to the worktree API or implementation at all.

The worktree API is both compact and orthogonal, and I haven't yet
seen a compelling reason to change it. That said, though, the API
documentation in worktree.h may be lacking, even if the implementation
is not. I'll say a bit more about that below.

> In addition to making the worktree code clearer, this patch fixes a
> bug in which the current is_worktree_locked over-eagerly sets
> lock_reason_valid. There are currently no consumers of
> lock_reason_valid within master, but obviously we should fix this
> before they appear :)

As noted above, 'lock_reason_valid' is private/internal. It's an
accident that it is not annotated such (like 'lock_reason', which is
correctly annotated as "internal"). So, there should never be any
external consumers of that field. It also means that there is no bug
in the current code (as far as I can see) since that field is
correctly consulted (internally) to determine whether the lock reason
has been looked up yet.

The missing "internal only" annotation is unfortunate since it may
have led you down this road of considering the implementation and API
broken.

Moreover, the documentation for is_worktree_locked() apparently
doesn't convey strongly enough that it serves the dual purpose of (1)
telling you whether or not the worktree is locked, and (2) telling you
the reason it is locked.

A patch which adds the missing "internal only" annotation to
'lock_reason_valid', and which makes it easier to understand the dual
purpose of is_worktree_locked() would be welcome, especially if it
helps avoid such confusion in the future.

Aside from that, it doesn't seem like worktree needs any changes for
the ref-filter atom you have in mind. (Don't interpret this
observation as me being averse to changes to the API; I'm open to
improvements, but haven't seen anything yet indicating a bug or
showing that the API is more difficult than it ought to be.)

[1]: https://public-inbox.org/git/CAPig+cTvKd2DVu7wW_A31p_o7BaNJszu14kNRz9sqk8h45H4-g@mail.gmail.com/

Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible

[Nickolai Belakovski]

On Sun, Oct 28, 2018 at 4:03 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Oct 28, 2018 at 5:55 PM Nickolai Belakovski
> > <nbelakovski@gmail.com> wrote: This was meant to be a reply to
> > https://public-inbox.org/git/CAC05386F1X7TsPr6kgkuLWEwsmdiQ4VKTF5RxaHvzpkwbmXPBw@mail.gmail.com/T/#m8898c8f7c68e1ea234aca21cb2d7776b375c6f51,
> > please look there for some more context. I think it both did and
> > didn't get listed as a reply? In my mailbox I see two separate
> > threads but in public-inbox.org/git it looks like it correctly got
> > labelled as 1 thread. This whole mailing list thing is new to me,
> > thanks for bearing with me as I figure it out :).
>
> Gmail threads messages entirely by subject; it doesn't pay attention
> to In-Reply-To: or other headers for threading, which is why you see
> two separate threads. public-inbox.org, on the other hand, does pay
> attention to the headers, thus understands that all the messages
> belong to the same thread. Gmail's behavior may be considered
> anomalous.
>

Got it, thanks!

> > Next time I'll make sure to change the subject line on updated
> > patches as PATCH v2 (that's the convention, right?).
>
> That's correct.
>

(thumbs up)

> > This is an improvement because it fixes an issue in which the fields
> > lock_reason and lock_reason_valid of the worktree struct were not
> > being populated. This is related to work I'm doing to add a worktree
> > atom to ref-filter.c.
>
> Those fields are considered private/internal. They are not intended to
> be accessed by calling code. (Unfortunately, only 'lock_reason' is
> thus marked; 'lock_reason_valid' should be marked "internal".) Clients
> are expected to retrieve the lock reason only through the provided
> API, is_worktree_locked().
>
> > I see your concerns about extra hits to the filesystem when calling
> > get_worktrees and about users interested in lock_reason having to
> > make extra calls. As regards hits to the filesystem, I could remove
> > is_locked from the worktree struct entirely. To address the second
> > concern, I could refactor worktree_locked_reason to return null if
> > the wt is not locked. I would still want to keep is_worktree_locked
> > around to provide a facility to check whether or not the worktree is
> > locked without having to go get the reason.
> >
> > There's also been some concerns raised about caching. As I pointed
> > out in the other thread, the current use cases for this information
> > die upon accessing it, so caching is a moot point. For the use case
> > of a worktree atom, caching would be relevant, but it could be done
> > within ref-filter.c. Another option is to add the lock_reason back
> > to the worktree struct and have two functions for populating it:
> > get_worktrees_wo_lock_reason and get_worktrees_with_lock_reason. A
> > bit more verbose, but it makes it clear to the caller what they're
> > getting and what they're not getting. I might suggest starting with
> > doing the caching within ref-filter.c first, and if more use cases
> > appear for caching lock_reason we can consider the second option. It
> > could also be get_worktrees and get_worktrees_wo_lock_reason, though
> > I think most callers would be calling the latter name.
> >
> > So, my proposal for driving this patch to completion would be to:
> > -remove is_locked from the worktree struct
> > -refactor worktree_locked_reason to return null if the wt is not locked
> > -refactor calls to is_locked within builtin/worktree.c to call
> > either the refactored worktree_locked_reason or is_worktree_locked
>
> My impression, thus far, is that this all seems to be complicating
> rather than simplifying. These changes also seem entirely unnecessary.
> In [1], I made the observation that it seemed that your new ref-filter
> atom could be implemented with the existing is_worktree_locked() API.
> As far as I can tell, it can indeed be implemented without having to
> make any changes to the worktree API or implementation at all.
>
> The worktree API is both compact and orthogonal, and I haven't yet
> seen a compelling reason to change it. That said, though, the API
> documentation in worktree.h may be lacking, even if the implementation
> is not. I'll say a bit more about that below.
>
> > In addition to making the worktree code clearer, this patch fixes a
> > bug in which the current is_worktree_locked over-eagerly sets
> > lock_reason_valid. There are currently no consumers of
> > lock_reason_valid within master, but obviously we should fix this
> > before they appear :)
>
> As noted above, 'lock_reason_valid' is private/internal. It's an
> accident that it is not annotated such (like 'lock_reason', which is
> correctly annotated as "internal"). So, there should never be any
> external consumers of that field. It also means that there is no bug
> in the current code (as far as I can see) since that field is
> correctly consulted (internally) to determine whether the lock reason
> has been looked up yet.

Thank you for explaining this. Looking at the code now it seems
crystal clear, but, yea I clearly got on the wrong path initially.

>
> The missing "internal only" annotation is unfortunate since it may
> have led you down this road of considering the implementation and API
> broken.
>
> Moreover, the documentation for is_worktree_locked() apparently
> doesn't convey strongly enough that it serves the dual purpose of (1)
> telling you whether or not the worktree is locked, and (2) telling you
> the reason it is locked.
>
> A patch which adds the missing "internal only" annotation to
> 'lock_reason_valid', and which makes it easier to understand the dual
> purpose of is_worktree_locked() would be welcome, especially if it
> helps avoid such confusion in the future.
>
> Aside from that, it doesn't seem like worktree needs any changes for
> the ref-filter atom you have in mind. (Don't interpret this
> observation as me being averse to changes to the API; I'm open to
> improvements, but haven't seen anything yet indicating a bug or
> showing that the API is more difficult than it ought to be.)
>
> [1]: https://public-inbox.org/git/CAPig+cTvKd2DVu7wW_A31p_o7BaNJszu14kNRz9sqk8h45H4-g@mail.gmail.com/

You're right that these changes are not necessary in order to make a
worktree atom.

If there's no interest in this patch I'll withdraw it.

I had found it really surprising that lock_reason was not populated
when I was accessing it while working on the worktree atom. When
digging into it, the "internal use" comment told me nothing, both
because there's no convention (that I'm aware of) within C to mark
fields as such and because it fails to direct the reader to
is_worktree_locked.

How about this, I can make a patch that changes the comment next to
lock_reason to say "/* private - use is_worktree_locked */" (choosing
the word "private" since it's a reserved keyword in C++ and other
languages for implementation details that are meant to be
inaccessible) and a comment next to lock_reason_valid that just says
"/* private */"? I would also suggest renaming is_worktree_locked to
worktree_lock_reason, the former makes me think the function is
returning a boolean, whereas the latter more clearly conveys that a
more detailed piece of information is being returned.

Lemme know what you think.

Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible

[Eric Sunshine]

On Sun, Oct 28, 2018 at 9:11 PM Nickolai Belakovski
<nbelakovski@gmail.com> wrote:
> On Sun, Oct 28, 2018 at 4:03 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > Aside from that, it doesn't seem like worktree needs any changes for
> > the ref-filter atom you have in mind. (Don't interpret this
> > observation as me being averse to changes to the API; I'm open to
> > improvements, but haven't seen anything yet indicating a bug or
> > showing that the API is more difficult than it ought to be.)
>
> You're right that these changes are not necessary in order to make a
> worktree atom.
> If there's no interest in this patch I'll withdraw it.

Withdrawing this patch seems reasonable.

> I had found it really surprising that lock_reason was not populated
> when I was accessing it while working on the worktree atom. When
> digging into it, the "internal use" comment told me nothing, both
> because there's no convention (that I'm aware of) within C to mark
> fields as such and because it fails to direct the reader to
> is_worktree_locked.
>
> How about this, I can make a patch that changes the comment next to
> lock_reason to say "/* private - use is_worktree_locked */" (choosing
> the word "private" since it's a reserved keyword in C++ and other
> languages for implementation details that are meant to be
> inaccessible) and a comment next to lock_reason_valid that just says
> "/* private */"?

A patch clarifying the "private" state of 'lock_reason' and
'lock_reason_valid' and pointing the reader at is_worktree_locked()
would be welcome.

One extra point: It might be a good idea to mention in the
documentation of is_worktree_locked() that, in addition to returning
NULL or non-NULL indicating not-locked or locked, the returned
lock-reason might very well be empty ("") when no reason was given by
the locker.

> I would also suggest renaming is_worktree_locked to
> worktree_lock_reason, the former makes me think the function is
> returning a boolean, whereas the latter more clearly conveys that a
> more detailed piece of information is being returned.

I think the "boolean"-sounding name was intentional since most
(current) callers only care about that; so, the following reads very
naturally for such callers:

    if (is_worktree_locked(wt))
        die(_("worktree locked; aborting"));

That said, I wouldn't necessarily oppose renaming the function, but I
also don't think it's particularly important to do so.

Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible

[Nickolai Belakovski]

On Sun, Oct 28, 2018 at 9:01 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Oct 28, 2018 at 9:11 PM Nickolai Belakovski
> <nbelakovski@gmail.com> wrote:
> > I would also suggest renaming is_worktree_locked to
> > worktree_lock_reason, the former makes me think the function is
> > returning a boolean, whereas the latter more clearly conveys that a
> > more detailed piece of information is being returned.
>
> I think the "boolean"-sounding name was intentional since most
> (current) callers only care about that; so, the following reads very
> naturally for such callers:
>
>     if (is_worktree_locked(wt))
>         die(_("worktree locked; aborting"));
>
> That said, I wouldn't necessarily oppose renaming the function, but I
> also don't think it's particularly important to do so.

Actually it's 3:2 in the current code for callers getting the reason
out of the function vs callers checking the value of the pointer for
null/not null. This leads to some rather unnatural looking code in the
current repo like

reason = is_worktree_locked(wt);

I think it would look a lot more natural if it were "reason =
worktree_lock_reason(wt)". The resulting if-statement wouldn't be too
bad, IMO

if (worktree_lock_reason(wt))
    die(_("worktree locked; aborting"));

To me, I would just go lookup the signature of worktree_lock_reason
and see that it returns a pointer and I'd be satisfied with that. I
could also infer that from looking at the code if I'm just skimming
through. But if I see code like "reason = is_worktree_locked(wt)" I'm
like hold on, what's going on here?! :P

Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible

[Eric Sunshine]

On Mon, Oct 29, 2018 at 1:45 AM Nickolai Belakovski
<nbelakovski@gmail.com> wrote:
> On Sun, Oct 28, 2018 at 9:01 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > That said, I wouldn't necessarily oppose renaming the function, but I
> > also don't think it's particularly important to do so.
>
> To me, I would just go lookup the signature of worktree_lock_reason
> and see that it returns a pointer and I'd be satisfied with that. I
> could also infer that from looking at the code if I'm just skimming
> through. But if I see code like "reason = is_worktree_locked(wt)" I'm
> like hold on, what's going on here?! :P

I don't feel strongly about it, and, as indicated, wouldn't
necessarily be opposed to it. If you do want to make that change,
perhaps send it as the second patch of a 2-patch series in which patch
1 just updates the API documentation. That way, if anyone does oppose
the rename in patch 2, then that patch can be dropped without having
to re-send.

[PATCH v3 1/2] worktree: update documentation for lock_reason and lock_reason_valid

[nbelakovski]

From: Nickolai Belakovski <nbelakovski@gmail.com>

Clarify that these fields are to be considered implementation details
and direct the reader to use the is_worktree_locked function to retrieve
said information.

Signed-off-by: Nickolai Belakovski <nbelakovski@gmail.com>
---
 worktree.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/worktree.h b/worktree.h
index df3fc30f7..6b12a3cf6 100644
--- a/worktree.h
+++ b/worktree.h
@@ -10,12 +10,12 @@ struct worktree {
 	char *path;
 	char *id;
 	char *head_ref;		/* NULL if HEAD is broken or detached */
-	char *lock_reason;	/* internal use */
+	char *lock_reason;	/* private - use is_worktree_locked */
 	struct object_id head_oid;
 	int is_detached;
 	int is_bare;
 	int is_current;
-	int lock_reason_valid;
+	int lock_reason_valid; /* private */
 };
 
 /* Functions for acting on the information about worktrees. */
-- 
2.14.2

Re: [PATCH v3 1/2] worktree: update documentation for lock_reason and lock_reason_valid

[Junio C Hamano]

nbelakovski@gmail.com writes:

> From: Nickolai Belakovski <nbelakovski@gmail.com>
>
> Clarify that these fields are to be considered implementation details
> and direct the reader to use the is_worktree_locked function to retrieve
> said information.
>
> Signed-off-by: Nickolai Belakovski <nbelakovski@gmail.com>
> ---
>  worktree.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/worktree.h b/worktree.h
> index df3fc30f7..6b12a3cf6 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -10,12 +10,12 @@ struct worktree {
>  	char *path;
>  	char *id;
>  	char *head_ref;		/* NULL if HEAD is broken or detached */
> -	char *lock_reason;	/* internal use */
> +	char *lock_reason;	/* private - use is_worktree_locked */

s/use /used by/, probably.

>  	struct object_id head_oid;
>  	int is_detached;
>  	int is_bare;
>  	int is_current;
> -	int lock_reason_valid;
> +	int lock_reason_valid; /* private */
>  };

These annotations to the two fields are not wrong per-se, but I have
a feeling that it would equally be important to document what the
other "non-private" fields mean, if peeking them *is* the API this
subsystem offers.

Thanks.

[PATCH v3 2/2] worktree: rename is_worktree_locked to worktree_lock_reason

[nbelakovski]

From: Nickolai Belakovski <nbelakovski@gmail.com>

A function prefixed with 'is_' would be expected to return a boolean,
however this function returns a string.

Signed-off-by: Nickolai Belakovski <nbelakovski@gmail.com>
---
 builtin/worktree.c | 10 +++++-----
 worktree.c         |  2 +-
 worktree.h         |  4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index c4abbde2b..5e8402617 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -245,7 +245,7 @@ static void validate_worktree_add(const char *path, const struct add_opts *opts)
 	if (!wt)
 		goto done;
 
-	locked = !!is_worktree_locked(wt);
+	locked = !!worktree_lock_reason(wt);
 	if ((!locked && opts->force) || (locked && opts->force > 1)) {
 		if (delete_git_dir(wt->id))
 		    die(_("unable to re-add worktree '%s'"), path);
@@ -682,7 +682,7 @@ 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);
+	old_reason = worktree_lock_reason(wt);
 	if (old_reason) {
 		if (*old_reason)
 			die(_("'%s' is already locked, reason: %s"),
@@ -714,7 +714,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 (!worktree_lock_reason(wt))
 		die(_("'%s' is not locked"), av[0]);
 	ret = unlink_or_warn(git_common_path("worktrees/%s/locked", wt->id));
 	free_worktrees(worktrees);
@@ -787,7 +787,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)
 	validate_no_submodules(wt);
 
 	if (force < 2)
-		reason = is_worktree_locked(wt);
+		reason = worktree_lock_reason(wt);
 	if (reason) {
 		if (*reason)
 			die(_("cannot move a locked working tree, lock reason: %s\nuse 'move -f -f' to override or unlock first"),
@@ -900,7 +900,7 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
 	if (is_main_worktree(wt))
 		die(_("'%s' is a main working tree"), av[0]);
 	if (force < 2)
-		reason = is_worktree_locked(wt);
+		reason = worktree_lock_reason(wt);
 	if (reason) {
 		if (*reason)
 			die(_("cannot remove a locked working tree, lock reason: %s\nuse 'remove -f -f' to override or unlock first"),
diff --git a/worktree.c b/worktree.c
index b0d0b5426..befdbe7fa 100644
--- a/worktree.c
+++ b/worktree.c
@@ -235,7 +235,7 @@ int is_main_worktree(const struct worktree *wt)
 	return !wt->id;
 }
 
-const char *is_worktree_locked(struct worktree *wt)
+const char *worktree_lock_reason(struct worktree *wt)
 {
 	assert(!is_main_worktree(wt));
 
diff --git a/worktree.h b/worktree.h
index 6b12a3cf6..55d449b6a 100644
--- a/worktree.h
+++ b/worktree.h
@@ -10,7 +10,7 @@ struct worktree {
 	char *path;
 	char *id;
 	char *head_ref;		/* NULL if HEAD is broken or detached */
-	char *lock_reason;	/* private - use is_worktree_locked */
+	char *lock_reason;	/* private - use worktree_lock_reason */
 	struct object_id head_oid;
 	int is_detached;
 	int is_bare;
@@ -60,7 +60,7 @@ extern int is_main_worktree(const struct worktree *wt);
  * Return the reason string if the given worktree is locked or NULL
  * otherwise.
  */
-extern const char *is_worktree_locked(struct worktree *wt);
+extern const char *worktree_lock_reason(struct worktree *wt);
 
 #define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)
 
-- 
2.14.2

Re: [PATCH v3 2/2] worktree: rename is_worktree_locked to worktree_lock_reason

[Junio C Hamano]

nbelakovski@gmail.com writes:

> From: Nickolai Belakovski <nbelakovski@gmail.com>
>
> A function prefixed with 'is_' would be expected to return a boolean,
> however this function returns a string.
>
> Signed-off-by: Nickolai Belakovski <nbelakovski@gmail.com>
> ---

Given that there is a clear documentation in worktree.h, and a
pointer that is not NULL is true in "if/while(ptr)", I'd say this
change is a borderline "meh".

I'll queue this on top of 1/2 and try merging to the integration
topics to see if it interacts with any other topics in flight.
Since the patch has already been written, let's not waste the effort
if there is no conflict with anybody else (otherwise I'd discard
this one---a "meh" patch is not worth having to worry about conflict
resolution).

Thanks.

>  builtin/worktree.c | 10 +++++-----
>  worktree.c         |  2 +-
>  worktree.h         |  4 ++--
>  3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index c4abbde2b..5e8402617 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -245,7 +245,7 @@ static void validate_worktree_add(const char *path, const struct add_opts *opts)
>  	if (!wt)
>  		goto done;
>  
> -	locked = !!is_worktree_locked(wt);
> +	locked = !!worktree_lock_reason(wt);
>  	if ((!locked && opts->force) || (locked && opts->force > 1)) {
>  		if (delete_git_dir(wt->id))
>  		    die(_("unable to re-add worktree '%s'"), path);
> @@ -682,7 +682,7 @@ 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);
> +	old_reason = worktree_lock_reason(wt);
>  	if (old_reason) {
>  		if (*old_reason)
>  			die(_("'%s' is already locked, reason: %s"),
> @@ -714,7 +714,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 (!worktree_lock_reason(wt))
>  		die(_("'%s' is not locked"), av[0]);
>  	ret = unlink_or_warn(git_common_path("worktrees/%s/locked", wt->id));
>  	free_worktrees(worktrees);
> @@ -787,7 +787,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)
>  	validate_no_submodules(wt);
>  
>  	if (force < 2)
> -		reason = is_worktree_locked(wt);
> +		reason = worktree_lock_reason(wt);
>  	if (reason) {
>  		if (*reason)
>  			die(_("cannot move a locked working tree, lock reason: %s\nuse 'move -f -f' to override or unlock first"),
> @@ -900,7 +900,7 @@ static int remove_worktree(int ac, const char **av, const char *prefix)
>  	if (is_main_worktree(wt))
>  		die(_("'%s' is a main working tree"), av[0]);
>  	if (force < 2)
> -		reason = is_worktree_locked(wt);
> +		reason = worktree_lock_reason(wt);
>  	if (reason) {
>  		if (*reason)
>  			die(_("cannot remove a locked working tree, lock reason: %s\nuse 'remove -f -f' to override or unlock first"),
> diff --git a/worktree.c b/worktree.c
> index b0d0b5426..befdbe7fa 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -235,7 +235,7 @@ int is_main_worktree(const struct worktree *wt)
>  	return !wt->id;
>  }
>  
> -const char *is_worktree_locked(struct worktree *wt)
> +const char *worktree_lock_reason(struct worktree *wt)
>  {
>  	assert(!is_main_worktree(wt));
>  
> diff --git a/worktree.h b/worktree.h
> index 6b12a3cf6..55d449b6a 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -10,7 +10,7 @@ struct worktree {
>  	char *path;
>  	char *id;
>  	char *head_ref;		/* NULL if HEAD is broken or detached */
> -	char *lock_reason;	/* private - use is_worktree_locked */
> +	char *lock_reason;	/* private - use worktree_lock_reason */
>  	struct object_id head_oid;
>  	int is_detached;
>  	int is_bare;
> @@ -60,7 +60,7 @@ extern int is_main_worktree(const struct worktree *wt);
>   * Return the reason string if the given worktree is locked or NULL
>   * otherwise.
>   */
> -extern const char *is_worktree_locked(struct worktree *wt);
> +extern const char *worktree_lock_reason(struct worktree *wt);
>  
>  #define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)

Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files

[Eric Sunshine]

On Thu, Oct 25, 2018 at 1:47 AM Nickolai Belakovski
<nbelakovski@gmail.com> wrote:
> The motivation for the change is some work that I'm doing to add a
> worktree atom in ref-filter.c. I wanted that atom to be able to access
> all fields of the worktree struct and noticed that lock_reason wasn't
> getting populated so I figured I'd go and fix that.
>
> Reviewing this work in the context of your feedback and Junio's
> previous comments, I think it makes sense to only have a field in the
> struct indicating whether or not the worktree is locked, and have a
> separate function for getting the reason.

Is your new ref-filter atom going to be boolean-only or will it also
have a form (or a separate atom) for retrieving the lock-reason? I
imagine both could be desirable.

In any event, implementation-wise, I would think that such an atom (or
atoms) could be easily built with the existing worktree API (with its
lazy-loading and caching), which might be an easy way forward since
you wouldn't need this patch or the updated one you posted[1], thus no
need to justify such a change.

> Since the only cases in
> which the reason is retrieved in the current codebase are cases where
> the program immediately dies, caching seems a moot point.

If your new atom has a form for retrieving the lock reason, then
caching could potentially be beneficial(?).

[1]: https://public-inbox.org/git/20181025055142.38077-1-nbelakovski@gmail.com/