Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files
On Thu, Oct 25, 2018 at 1:47 AM Nickolai Belakovski 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-nbelakov...@gmail.com/
Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files
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 wrote: > > On Wed, Oct 24, 2018 at 2:39 AM 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@gitster.mtv.corp.google.com/ > [2]: > https://public-inbox.org/git/xmqq4m9d0w6v@gitster.mtv.corp.google.com/
Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files
On Wed, Oct 24, 2018 at 2:39 AM 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@gitster.mtv.corp.google.com/ [2]: https://public-inbox.org/git/xmqq4m9d0w6v@gitster.mtv.corp.google.com/
[PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files
From: Nickolai Belakovski
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
---
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", , 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, , 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, , 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
+ */
