diff mbox series

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

Message ID 20181024063904.36096-1-nbelakovski@gmail.com (mailing list archive)
State New, archived
Headers show
Series worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files | expand

Commit Message

Nickolai Belakovski Oct. 24, 2018, 6:39 a.m. UTC 
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(-)

Comments

Eric Sunshine Oct. 24, 2018, 8:11 a.m. UTC | #1 
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/
Nickolai Belakovski Oct. 25, 2018, 5:46 a.m. UTC | #2 
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/
Eric Sunshine Oct. 25, 2018, 7:14 p.m. UTC | #3 
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/
diff mbox series

Patch

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)
 
 /*