{"thread":{"id":"49658","subject":"[PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files","startedAt":"2018-10-24T06:39:23Z","lastAt":"2018-10-31T02:41:33Z","messageCount":19,"participants":["nbelakovski@gmail.com","Eric Sunshine","Nickolai Belakovski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"361371","messageId":"20181024063904.36096-1-nbelakovski@gmail.com","threadId":"49658","inReplyTo":null,"subject":"[PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files","fromName":"","fromEmail":"nbelakovski@gmail.com","sentAt":"2018-10-24T06:39:04Z","receivedAt":"2018-10-24T06:39:23Z","isPatch":true,"sender":{"key":"nbelakovski@gmail.com","avatar":"https://avatars.githubusercontent.com/u/864630?v=4"},"body":"From: Nickolai Belakovski \n\nlock_reason is now populated during the execution of get_worktrees\n\nis_worktree_locked has been simplified, renamed, and changed to internal\nlinkage. It is simplified to only return the lock reason (or NULL in case\nthere is no lock reason) and to not have any side effects on the inputs.\nAs such it made sense to rename it since it only returns the reason.\n\nSince this function was now being used to populate the worktree struct's\nlock_reason field, it made sense to move the function to internal\nlinkage and have callers refer to the lock_reason field. The\nlock_reason_valid field was removed since a NULL/non-NULL value of\nlock_reason accomplishes the same effect.\n\nSome unused variables within worktree source code were removed.\n\nSigned-off-by: Nickolai Belakovski \n---\n\nNotes:\n Travis CI results: https://travis-ci.org/nbelakovski/git/builds/445500127\n\n builtin/worktree.c | 27 +++++++++++----------------\n worktree.c | 53 +++++++++++++++++++++++------------------------------\n worktree.h | 9 +--------\n 3 files changed, 35 insertions(+), 54 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 41e771439..fb203f61d 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -615,7 +615,7 @@ static int list(int ac, const char **av, const char *prefix)\n \n static int lock_worktree(int ac, const char **av, const char *prefix)\n {\n-\tconst char *reason = \"\", *old_reason;\n+\tconst char *reason = \"\";\n \tstruct option options[] = {\n \t\tOPT_STRING(0, \"reason\", &reason, N_(\"string\"),\n \t\t\t N_(\"reason for locking\")),\n@@ -634,11 +634,10 @@ static int lock_worktree(int ac, const char **av, const char *prefix)\n \tif (is_main_worktree(wt))\n \t\tdie(_(\"The main working tree cannot be locked or unlocked\"));\n \n-\told_reason = is_worktree_locked(wt);\n-\tif (old_reason) {\n-\t\tif (*old_reason)\n+\tif (wt->lock_reason) {\n+\t\tif (*wt->lock_reason)\n \t\t\tdie(_(\"'%s' is already locked, reason: %s\"),\n-\t\t\t av[0], old_reason);\n+\t\t\t av[0], wt->lock_reason);\n \t\tdie(_(\"'%s' is already locked\"), av[0]);\n \t}\n \n@@ -666,7 +665,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)\n \t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n \tif (is_main_worktree(wt))\n \t\tdie(_(\"The main working tree cannot be locked or unlocked\"));\n-\tif (!is_worktree_locked(wt))\n+\tif (!wt->lock_reason)\n \t\tdie(_(\"'%s' is not locked\"), av[0]);\n \tret = unlink_or_warn(git_common_path(\"worktrees/%s/locked\", wt->id));\n \tfree_worktrees(worktrees);\n@@ -703,7 +702,6 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \tstruct worktree **worktrees, *wt;\n \tstruct strbuf dst = STRBUF_INIT;\n \tstruct strbuf errmsg = STRBUF_INIT;\n-\tconst char *reason;\n \tchar *path;\n \n \tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n@@ -734,11 +732,10 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \n \tvalidate_no_submodules(wt);\n \n-\treason = is_worktree_locked(wt);\n-\tif (reason) {\n-\t\tif (*reason)\n+\tif (wt->lock_reason) {\n+\t\tif (*wt->lock_reason)\n \t\t\tdie(_(\"cannot move a locked working tree, lock reason: %s\"),\n-\t\t\t reason);\n+\t\t\t wt->lock_reason);\n \t\tdie(_(\"cannot move a locked working tree\"));\n \t}\n \tif (validate_worktree(wt, &errmsg, 0))\n@@ -847,7 +844,6 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n \t};\n \tstruct worktree **worktrees, *wt;\n \tstruct strbuf errmsg = STRBUF_INIT;\n-\tconst char *reason;\n \tint ret = 0;\n \n \tac = parse_options(ac, av, prefix, options, worktree_usage, 0);\n@@ -860,11 +856,10 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n \t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n \tif (is_main_worktree(wt))\n \t\tdie(_(\"'%s' is a main working tree\"), av[0]);\n-\treason = is_worktree_locked(wt);\n-\tif (reason) {\n-\t\tif (*reason)\n+\tif (wt->lock_reason) {\n+\t\tif (*wt->lock_reason)\n \t\t\tdie(_(\"cannot remove a locked working tree, lock reason: %s\"),\n-\t\t\t reason);\n+\t\t\t wt->lock_reason);\n \t\tdie(_(\"cannot remove a locked working tree\"));\n \t}\n \tif (validate_worktree(wt, &errmsg, WT_VALIDATE_WORKTREE_MISSING_OK))\ndiff --git a/worktree.c b/worktree.c\nindex 97cda5f97..3bd25983c 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -41,13 +41,34 @@ static void add_head_info(struct worktree *wt)\n \t\twt->is_detached = 1;\n }\n \n+/**\n+ * Return the reason the worktree is locked, or NULL if it is not locked\n+ */\n+static char *worktree_locked_reason(const struct worktree *wt)\n+{\n+\tstruct strbuf lock_reason = STRBUF_INIT;\n+\tstruct strbuf path = STRBUF_INIT;\n+\n+\tassert(!is_main_worktree(wt));\n+\n+\tstrbuf_addstr(&path, worktree_git_path(wt, \"locked\"));\n+\tif (file_exists(path.buf)) {\n+\t\tif (strbuf_read_file(&lock_reason, path.buf, 0) < 0)\n+\t\t\tdie_errno(_(\"failed to read '%s'\"), path.buf);\n+\t\tstrbuf_trim(&lock_reason);\n+\t\tstrbuf_release(&path);\n+\t\treturn strbuf_detach(&lock_reason, NULL);\n+\t}\n+\tstrbuf_release(&path);\n+\treturn NULL;\n+}\n+\n /**\n * get the main worktree\n */\n static struct worktree *get_main_worktree(void)\n {\n \tstruct worktree *worktree = NULL;\n-\tstruct strbuf path = STRBUF_INIT;\n \tstruct strbuf worktree_path = STRBUF_INIT;\n \tint is_bare = 0;\n \n@@ -56,14 +77,11 @@ static struct worktree *get_main_worktree(void)\n \tif (is_bare)\n \t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n \n-\tstrbuf_addf(&path, \"%s/HEAD\", get_git_common_dir());\n-\n \tworktree = xcalloc(1, sizeof(*worktree));\n \tworktree->path = strbuf_detach(&worktree_path, NULL);\n \tworktree->is_bare = is_bare;\n \tadd_head_info(worktree);\n \n-\tstrbuf_release(&path);\n \tstrbuf_release(&worktree_path);\n \treturn worktree;\n }\n@@ -89,12 +107,10 @@ static struct worktree *get_linked_worktree(const char *id)\n \t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n \t}\n \n-\tstrbuf_reset(&path);\n-\tstrbuf_addf(&path, \"%s/worktrees/%s/HEAD\", get_git_common_dir(), id);\n-\n \tworktree = xcalloc(1, sizeof(*worktree));\n \tworktree->path = strbuf_detach(&worktree_path, NULL);\n \tworktree->id = xstrdup(id);\n+\tworktree->lock_reason = worktree_locked_reason(worktree);\n \tadd_head_info(worktree);\n \n done:\n@@ -231,29 +247,6 @@ int is_main_worktree(const struct worktree *wt)\n \treturn !wt->id;\n }\n \n-const char *is_worktree_locked(struct worktree *wt)\n-{\n-\tassert(!is_main_worktree(wt));\n-\n-\tif (!wt->lock_reason_valid) {\n-\t\tstruct strbuf path = STRBUF_INIT;\n-\n-\t\tstrbuf_addstr(&path, worktree_git_path(wt, \"locked\"));\n-\t\tif (file_exists(path.buf)) {\n-\t\t\tstruct strbuf lock_reason = STRBUF_INIT;\n-\t\t\tif (strbuf_read_file(&lock_reason, path.buf, 0) < 0)\n-\t\t\t\tdie_errno(_(\"failed to read '%s'\"), path.buf);\n-\t\t\tstrbuf_trim(&lock_reason);\n-\t\t\twt->lock_reason = strbuf_detach(&lock_reason, NULL);\n-\t\t} else\n-\t\t\twt->lock_reason = NULL;\n-\t\twt->lock_reason_valid = 1;\n-\t\tstrbuf_release(&path);\n-\t}\n-\n-\treturn wt->lock_reason;\n-}\n-\n /* convenient wrapper to deal with NULL strbuf */\n static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...)\n {\ndiff --git a/worktree.h b/worktree.h\nindex df3fc30f7..0214630fd 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -10,12 +10,11 @@ struct worktree {\n \tchar *path;\n \tchar *id;\n \tchar *head_ref;\t\t/* NULL if HEAD is broken or detached */\n-\tchar *lock_reason;\t/* internal use */\n+\tchar *lock_reason;\n \tstruct object_id head_oid;\n \tint is_detached;\n \tint is_bare;\n \tint is_current;\n-\tint lock_reason_valid;\n };\n \n /* Functions for acting on the information about worktrees. */\n@@ -56,12 +55,6 @@ extern struct worktree *find_worktree(struct worktree **list,\n */\n extern int is_main_worktree(const struct worktree *wt);\n \n-/*\n- * Return the reason string if the given worktree is locked or NULL\n- * otherwise.\n- */\n-extern const char *is_worktree_locked(struct worktree *wt);\n-\n #define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)\n \n /*\n-- \n2.14.2\n\n"},{"id":"361386","messageId":"CAPig+cRN_0VVe6dzhnmU73pgo-8ncPzmOx4bRrTBVvReLW6RfQ@mail.gmail.com","threadId":"49658","inReplyTo":"20181024063904.36096-1-nbelakovski@gmail.com","subject":"Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-10-24T08:11:31Z","receivedAt":"2018-10-24T08:11:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Oct 24, 2018 at 2:39 AM wrote:\n> lock_reason is now populated during the execution of get_worktrees\n>\n> is_worktree_locked has been simplified, renamed, and changed to internal\n> linkage. It is simplified to only return the lock reason (or NULL in case\n> there is no lock reason) and to not have any side effects on the inputs.\n> As such it made sense to rename it since it only returns the reason.\n>\n> Since this function was now being used to populate the worktree struct's\n> lock_reason field, it made sense to move the function to internal\n> linkage and have callers refer to the lock_reason field. The\n> lock_reason_valid field was removed since a NULL/non-NULL value of\n> lock_reason accomplishes the same effect.\n>\n> Some unused variables within worktree source code were removed.\n\nThanks for the submission.\n\nOne thing which isn't clear from this commit message is _why_ this\nchange is desirable at this time, aside from the obvious\nsimplification of the code and client interaction (or perhaps those\nare the _why_?).\n\nAlthough I had envisioned populating the \"reason\" field greedily in\nthe way this patch does, not everyone agrees that doing so is\ndesirable. In particular, Junio argued[1,2] for populating it lazily,\nwhich accounts for the current implementation. That's why I ask about\nthe _why_ of this change since it will likely need to be justified in\na such a way to convince Junio to change his mind.\n\nThanks.\n\n[1]: https://public-inbox.org/git/xmqq8tyq5czn.fsf@gitster.mtv.corp.google.com/\n[2]: https://public-inbox.org/git/xmqq4m9d0w6v.fsf@gitster.mtv.corp.google.com/\n"},{"id":"361476","messageId":"CAC05386F1X7TsPr6kgkuLWEwsmdiQ4VKTF5RxaHvzpkwbmXPBw@mail.gmail.com","threadId":"49658","inReplyTo":"CAPig+cRN_0VVe6dzhnmU73pgo-8ncPzmOx4bRrTBVvReLW6RfQ@mail.gmail.com","subject":"Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files","fromName":"Nickolai Belakovski","fromEmail":"nbelakovski@gmail.com","sentAt":"2018-10-25T05:46:48Z","receivedAt":"2018-10-25T05:47:17Z","isPatch":true,"sender":{"key":"nbelakovski@gmail.com","avatar":"https://avatars.githubusercontent.com/u/864630?v=4"},"body":"The motivation for the change is some work that I'm doing to add a\nworktree atom in ref-filter.c. I wanted that atom to be able to access\nall fields of the worktree struct and noticed that lock_reason wasn't\ngetting populated so I figured I'd go and fix that.\n\nI figured that since get_worktrees is already hitting the filesystem\nfor the directory it wouldn't matter much if it also went and got the\nlock reason.\n\nReviewing this work in the context of your feedback and Junio's\nprevious comments, I think it makes sense to only have a field in the\nstruct indicating whether or not the worktree is locked, and have a\nseparate function for getting the reason. Since the only cases in\nwhich the reason is retrieved in the current codebase are cases where\nthe program immediately dies, caching seems a moot point. I'll send an\nupdated patch just after this message.\n\nThanks for the feedback, happy to receive more.\nOn Wed, Oct 24, 2018 at 1:11 AM Eric Sunshine wrote:\n>\n> On Wed, Oct 24, 2018 at 2:39 AM wrote:\n> > lock_reason is now populated during the execution of get_worktrees\n> >\n> > is_worktree_locked has been simplified, renamed, and changed to internal\n> > linkage. It is simplified to only return the lock reason (or NULL in case\n> > there is no lock reason) and to not have any side effects on the inputs.\n> > As such it made sense to rename it since it only returns the reason.\n> >\n> > Since this function was now being used to populate the worktree struct's\n> > lock_reason field, it made sense to move the function to internal\n> > linkage and have callers refer to the lock_reason field. The\n> > lock_reason_valid field was removed since a NULL/non-NULL value of\n> > lock_reason accomplishes the same effect.\n> >\n> > Some unused variables within worktree source code were removed.\n>\n> Thanks for the submission.\n>\n> One thing which isn't clear from this commit message is _why_ this\n> change is desirable at this time, aside from the obvious\n> simplification of the code and client interaction (or perhaps those\n> are the _why_?).\n>\n> Although I had envisioned populating the \"reason\" field greedily in\n> the way this patch does, not everyone agrees that doing so is\n> desirable. In particular, Junio argued[1,2] for populating it lazily,\n> which accounts for the current implementation. That's why I ask about\n> the _why_ of this change since it will likely need to be justified in\n> a such a way to convince Junio to change his mind.\n>\n> Thanks.\n>\n> [1]: https://public-inbox.org/git/xmqq8tyq5czn.fsf@gitster.mtv.corp.google.com/\n> [2]: https://public-inbox.org/git/xmqq4m9d0w6v.fsf@gitster.mtv.corp.google.com/\n"},{"id":"361477","messageId":"20181025055142.38077-1-nbelakovski@gmail.com","threadId":"49658","inReplyTo":"CAC05386F1X7TsPr6kgkuLWEwsmdiQ4VKTF5RxaHvzpkwbmXPBw@mail.gmail.com","subject":"[PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible","fromName":"","fromEmail":"nbelakovski@gmail.com","sentAt":"2018-10-25T05:51:42Z","receivedAt":"2018-10-25T05:51:55Z","isPatch":true,"sender":{"key":"nbelakovski@gmail.com","avatar":"https://avatars.githubusercontent.com/u/864630?v=4"},"body":"From: Nickolai Belakovski \n\nlock_reason_valid is renamed to is_locked and lock_reason is removed as\na field of the worktree struct. Lock reason can be obtained instead by a\nstandalone function.\n\nThis is done in order to make the worktree struct more intuitive when it\nis used elsewhere in the codebase.\n\nSome unused variables are cleaned up as well.\n\nSigned-off-by: Nickolai Belakovski \n---\n builtin/worktree.c | 16 ++++++++--------\n worktree.c | 55 ++++++++++++++++++++++++++++--------------------------\n worktree.h | 8 +++-----\n 3 files changed, 40 insertions(+), 39 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 41e771439..844789a21 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -634,8 +634,8 @@ static int lock_worktree(int ac, const char **av, const char *prefix)\n \tif (is_main_worktree(wt))\n \t\tdie(_(\"The main working tree cannot be locked or unlocked\"));\n \n-\told_reason = is_worktree_locked(wt);\n-\tif (old_reason) {\n+\tif (wt->is_locked) {\n+\t\told_reason = worktree_locked_reason(wt);\n \t\tif (*old_reason)\n \t\t\tdie(_(\"'%s' is already locked, reason: %s\"),\n \t\t\t av[0], old_reason);\n@@ -666,7 +666,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)\n \t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n \tif (is_main_worktree(wt))\n \t\tdie(_(\"The main working tree cannot be locked or unlocked\"));\n-\tif (!is_worktree_locked(wt))\n+\tif (!wt->is_locked)\n \t\tdie(_(\"'%s' is not locked\"), av[0]);\n \tret = unlink_or_warn(git_common_path(\"worktrees/%s/locked\", wt->id));\n \tfree_worktrees(worktrees);\n@@ -734,8 +734,8 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \n \tvalidate_no_submodules(wt);\n \n-\treason = is_worktree_locked(wt);\n-\tif (reason) {\n+\tif (wt->is_locked) {\n+\t\treason = worktree_locked_reason(wt);\n \t\tif (*reason)\n \t\t\tdie(_(\"cannot move a locked working tree, lock reason: %s\"),\n \t\t\t reason);\n@@ -860,11 +860,11 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n \t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n \tif (is_main_worktree(wt))\n \t\tdie(_(\"'%s' is a main working tree\"), av[0]);\n-\treason = is_worktree_locked(wt);\n-\tif (reason) {\n+\tif (wt->is_locked) {\n+\t\treason = worktree_locked_reason(wt);\n \t\tif (*reason)\n \t\t\tdie(_(\"cannot remove a locked working tree, lock reason: %s\"),\n-\t\t\t reason);\n+\t\t\t\treason);\n \t\tdie(_(\"cannot remove a locked working tree\"));\n \t}\n \tif (validate_worktree(wt, &errmsg, WT_VALIDATE_WORKTREE_MISSING_OK))\ndiff --git a/worktree.c b/worktree.c\nindex 97cda5f97..a3082d19d 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -14,7 +14,6 @@ void free_worktrees(struct worktree **worktrees)\n \t\tfree(worktrees[i]->path);\n \t\tfree(worktrees[i]->id);\n \t\tfree(worktrees[i]->head_ref);\n-\t\tfree(worktrees[i]->lock_reason);\n \t\tfree(worktrees[i]);\n \t}\n \tfree (worktrees);\n@@ -41,13 +40,29 @@ static void add_head_info(struct worktree *wt)\n \t\twt->is_detached = 1;\n }\n \n+\n+/**\n+ * Return 1 if the worktree is locked, 0 otherwise\n+ */\n+static int is_worktree_locked(const struct worktree *wt)\n+{\n+\tstruct strbuf path = STRBUF_INIT;\n+\tint locked_file_exists;\n+\n+\tassert(!is_main_worktree(wt));\n+\n+\tstrbuf_addstr(&path, worktree_git_path(wt, \"locked\"));\n+\tlocked_file_exists = file_exists(path.buf);\n+\tstrbuf_release(&path);\n+\treturn locked_file_exists;\n+}\n+\n /**\n * get the main worktree\n */\n static struct worktree *get_main_worktree(void)\n {\n \tstruct worktree *worktree = NULL;\n-\tstruct strbuf path = STRBUF_INIT;\n \tstruct strbuf worktree_path = STRBUF_INIT;\n \tint is_bare = 0;\n \n@@ -56,14 +71,11 @@ static struct worktree *get_main_worktree(void)\n \tif (is_bare)\n \t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n \n-\tstrbuf_addf(&path, \"%s/HEAD\", get_git_common_dir());\n-\n \tworktree = xcalloc(1, sizeof(*worktree));\n \tworktree->path = strbuf_detach(&worktree_path, NULL);\n \tworktree->is_bare = is_bare;\n \tadd_head_info(worktree);\n \n-\tstrbuf_release(&path);\n \tstrbuf_release(&worktree_path);\n \treturn worktree;\n }\n@@ -89,12 +101,10 @@ static struct worktree *get_linked_worktree(const char *id)\n \t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n \t}\n \n-\tstrbuf_reset(&path);\n-\tstrbuf_addf(&path, \"%s/worktrees/%s/HEAD\", get_git_common_dir(), id);\n-\n \tworktree = xcalloc(1, sizeof(*worktree));\n \tworktree->path = strbuf_detach(&worktree_path, NULL);\n \tworktree->id = xstrdup(id);\n+\tworktree->is_locked = is_worktree_locked(worktree);\n \tadd_head_info(worktree);\n \n done:\n@@ -231,27 +241,20 @@ int is_main_worktree(const struct worktree *wt)\n \treturn !wt->id;\n }\n \n-const char *is_worktree_locked(struct worktree *wt)\n+const char *worktree_locked_reason(const struct worktree *wt)\n {\n-\tassert(!is_main_worktree(wt));\n+\tstruct strbuf path = STRBUF_INIT;\n+\tstruct strbuf lock_reason = STRBUF_INIT;\n \n-\tif (!wt->lock_reason_valid) {\n-\t\tstruct strbuf path = STRBUF_INIT;\n-\n-\t\tstrbuf_addstr(&path, worktree_git_path(wt, \"locked\"));\n-\t\tif (file_exists(path.buf)) {\n-\t\t\tstruct strbuf lock_reason = STRBUF_INIT;\n-\t\t\tif (strbuf_read_file(&lock_reason, path.buf, 0) < 0)\n-\t\t\t\tdie_errno(_(\"failed to read '%s'\"), path.buf);\n-\t\t\tstrbuf_trim(&lock_reason);\n-\t\t\twt->lock_reason = strbuf_detach(&lock_reason, NULL);\n-\t\t} else\n-\t\t\twt->lock_reason = NULL;\n-\t\twt->lock_reason_valid = 1;\n-\t\tstrbuf_release(&path);\n-\t}\n+\tassert(!is_main_worktree(wt));\n+\tassert(wt->is_locked);\n \n-\treturn wt->lock_reason;\n+\tstrbuf_addstr(&path, worktree_git_path(wt, \"locked\"));\n+\tif (strbuf_read_file(&lock_reason, path.buf, 0) < 0)\n+\t\tdie_errno(_(\"failed to read '%s'\"), path.buf);\n+\tstrbuf_trim(&lock_reason);\n+\tstrbuf_release(&path);\n+\treturn strbuf_detach(&lock_reason, NULL);\n }\n \n /* convenient wrapper to deal with NULL strbuf */\ndiff --git a/worktree.h b/worktree.h\nindex df3fc30f7..6717287e8 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -10,12 +10,11 @@ struct worktree {\n \tchar *path;\n \tchar *id;\n \tchar *head_ref;\t\t/* NULL if HEAD is broken or detached */\n-\tchar *lock_reason;\t/* internal use */\n \tstruct object_id head_oid;\n \tint is_detached;\n \tint is_bare;\n \tint is_current;\n-\tint lock_reason_valid;\n+\tint is_locked;\n };\n \n /* Functions for acting on the information about worktrees. */\n@@ -57,10 +56,9 @@ extern struct worktree *find_worktree(struct worktree **list,\n extern int is_main_worktree(const struct worktree *wt);\n \n /*\n- * Return the reason string if the given worktree is locked or NULL\n- * otherwise.\n+ * Return the reason string if the given worktree is locked or die\n */\n-extern const char *is_worktree_locked(struct worktree *wt);\n+extern const char *worktree_locked_reason(const struct worktree *wt);\n \n #define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)\n \n-- \n2.14.2\n\n"},{"id":"361481","messageId":"xmqq4ldajz05.fsf@gitster-ct.c.googlers.com","threadId":"49658","inReplyTo":"20181025055142.38077-1-nbelakovski@gmail.com","subject":"Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-25T06:56:10Z","receivedAt":"2018-10-25T06:56:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"nbelakovski@gmail.com writes:\n\n> From: Nickolai Belakovski \n>\n> lock_reason_valid is renamed to is_locked and lock_reason is removed as\n> a field of the worktree struct. Lock reason can be obtained instead by a\n> standalone function.\n>\n> This is done in order to make the worktree struct more intuitive when it\n> is used elsewhere in the codebase.\n\nSo a mere action of getting an in-core worktree instance now has to\nmake an extra call to file_exists(), and in addition, the callers\nwho want to learn why the worktree is locked, they need to open and\nread the contents of the file in addition?\n\nWhy is that an improvement?\n\n\n>\n> Some unused variables are cleaned up as well.\n>\n> Signed-off-by: Nickolai Belakovski \n> ---\n> builtin/worktree.c | 16 ++++++++--------\n> worktree.c | 55 ++++++++++++++++++++++++++++--------------------------\n> worktree.h | 8 +++-----\n> 3 files changed, 40 insertions(+), 39 deletions(-)\n>\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 41e771439..844789a21 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -634,8 +634,8 @@ static int lock_worktree(int ac, const char **av, const char *prefix)\n> \tif (is_main_worktree(wt))\n> \t\tdie(_(\"The main working tree cannot be locked or unlocked\"));\n> \n> -\told_reason = is_worktree_locked(wt);\n> -\tif (old_reason) {\n> +\tif (wt->is_locked) {\n> +\t\told_reason = worktree_locked_reason(wt);\n> \t\tif (*old_reason)\n> \t\t\tdie(_(\"'%s' is already locked, reason: %s\"),\n> \t\t\t av[0], old_reason);\n> @@ -666,7 +666,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)\n> \t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n> \tif (is_main_worktree(wt))\n> \t\tdie(_(\"The main working tree cannot be locked or unlocked\"));\n> -\tif (!is_worktree_locked(wt))\n> +\tif (!wt->is_locked)\n> \t\tdie(_(\"'%s' is not locked\"), av[0]);\n> \tret = unlink_or_warn(git_common_path(\"worktrees/%s/locked\", wt->id));\n> \tfree_worktrees(worktrees);\n> @@ -734,8 +734,8 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n> \n> \tvalidate_no_submodules(wt);\n> \n> -\treason = is_worktree_locked(wt);\n> -\tif (reason) {\n> +\tif (wt->is_locked) {\n> +\t\treason = worktree_locked_reason(wt);\n> \t\tif (*reason)\n> \t\t\tdie(_(\"cannot move a locked working tree, lock reason: %s\"),\n> \t\t\t reason);\n> @@ -860,11 +860,11 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n> \t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n> \tif (is_main_worktree(wt))\n> \t\tdie(_(\"'%s' is a main working tree\"), av[0]);\n> -\treason = is_worktree_locked(wt);\n> -\tif (reason) {\n> +\tif (wt->is_locked) {\n> +\t\treason = worktree_locked_reason(wt);\n> \t\tif (*reason)\n> \t\t\tdie(_(\"cannot remove a locked working tree, lock reason: %s\"),\n> -\t\t\t reason);\n> +\t\t\t\treason);\n> \t\tdie(_(\"cannot remove a locked working tree\"));\n> \t}\n> \tif (validate_worktree(wt, &errmsg, WT_VALIDATE_WORKTREE_MISSING_OK))\n> diff --git a/worktree.c b/worktree.c\n> index 97cda5f97..a3082d19d 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -14,7 +14,6 @@ void free_worktrees(struct worktree **worktrees)\n> \t\tfree(worktrees[i]->path);\n> \t\tfree(worktrees[i]->id);\n> \t\tfree(worktrees[i]->head_ref);\n> -\t\tfree(worktrees[i]->lock_reason);\n> \t\tfree(worktrees[i]);\n> \t}\n> \tfree (worktrees);\n> @@ -41,13 +40,29 @@ static void add_head_info(struct worktree *wt)\n> \t\twt->is_detached = 1;\n> }\n> \n> +\n> +/**\n> + * Return 1 if the worktree is locked, 0 otherwise\n> + */\n> +static int is_worktree_locked(const struct worktree *wt)\n> +{\n> +\tstruct strbuf path = STRBUF_INIT;\n> +\tint locked_file_exists;\n> +\n> +\tassert(!is_main_worktree(wt));\n> +\n> +\tstrbuf_addstr(&path, worktree_git_path(wt, \"locked\"));\n> +\tlocked_file_exists = file_exists(path.buf);\n> +\tstrbuf_release(&path);\n> +\treturn locked_file_exists;\n> +}\n> +\n> /**\n> * get the main worktree\n> */\n> static struct worktree *get_main_worktree(void)\n> {\n> \tstruct worktree *worktree = NULL;\n> -\tstruct strbuf path = STRBUF_INIT;\n> \tstruct strbuf worktree_path = STRBUF_INIT;\n> \tint is_bare = 0;\n> \n> @@ -56,14 +71,11 @@ static struct worktree *get_main_worktree(void)\n> \tif (is_bare)\n> \t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n> \n> -\tstrbuf_addf(&path, \"%s/HEAD\", get_git_common_dir());\n> -\n> \tworktree = xcalloc(1, sizeof(*worktree));\n> \tworktree->path = strbuf_detach(&worktree_path, NULL);\n> \tworktree->is_bare = is_bare;\n> \tadd_head_info(worktree);\n> \n> -\tstrbuf_release(&path);\n> \tstrbuf_release(&worktree_path);\n> \treturn worktree;\n> }\n> @@ -89,12 +101,10 @@ static struct worktree *get_linked_worktree(const char *id)\n> \t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n> \t}\n> \n> -\tstrbuf_reset(&path);\n> -\tstrbuf_addf(&path, \"%s/worktrees/%s/HEAD\", get_git_common_dir(), id);\n> -\n> \tworktree = xcalloc(1, sizeof(*worktree));\n> \tworktree->path = strbuf_detach(&worktree_path, NULL);\n> \tworktree->id = xstrdup(id);\n> +\tworktree->is_locked = is_worktree_locked(worktree);\n> \tadd_head_info(worktree);\n> \n> done:\n> @@ -231,27 +241,20 @@ int is_main_worktree(const struct worktree *wt)\n> \treturn !wt->id;\n> }\n> \n> -const char *is_worktree_locked(struct worktree *wt)\n> +const char *worktree_locked_reason(const struct worktree *wt)\n> {\n> -\tassert(!is_main_worktree(wt));\n> +\tstruct strbuf path = STRBUF_INIT;\n> +\tstruct strbuf lock_reason = STRBUF_INIT;\n> \n> -\tif (!wt->lock_reason_valid) {\n> -\t\tstruct strbuf path = STRBUF_INIT;\n> -\n> -\t\tstrbuf_addstr(&path, worktree_git_path(wt, \"locked\"));\n> -\t\tif (file_exists(path.buf)) {\n> -\t\t\tstruct strbuf lock_reason = STRBUF_INIT;\n> -\t\t\tif (strbuf_read_file(&lock_reason, path.buf, 0) < 0)\n> -\t\t\t\tdie_errno(_(\"failed to read '%s'\"), path.buf);\n> -\t\t\tstrbuf_trim(&lock_reason);\n> -\t\t\twt->lock_reason = strbuf_detach(&lock_reason, NULL);\n> -\t\t} else\n> -\t\t\twt->lock_reason = NULL;\n> -\t\twt->lock_reason_valid = 1;\n> -\t\tstrbuf_release(&path);\n> -\t}\n> +\tassert(!is_main_worktree(wt));\n> +\tassert(wt->is_locked);\n> \n> -\treturn wt->lock_reason;\n> +\tstrbuf_addstr(&path, worktree_git_path(wt, \"locked\"));\n> +\tif (strbuf_read_file(&lock_reason, path.buf, 0) < 0)\n> +\t\tdie_errno(_(\"failed to read '%s'\"), path.buf);\n> +\tstrbuf_trim(&lock_reason);\n> +\tstrbuf_release(&path);\n> +\treturn strbuf_detach(&lock_reason, NULL);\n> }\n> \n> /* convenient wrapper to deal with NULL strbuf */\n> diff --git a/worktree.h b/worktree.h\n> index df3fc30f7..6717287e8 100644\n> --- a/worktree.h\n> +++ b/worktree.h\n> @@ -10,12 +10,11 @@ struct worktree {\n> \tchar *path;\n> \tchar *id;\n> \tchar *head_ref;\t\t/* NULL if HEAD is broken or detached */\n> -\tchar *lock_reason;\t/* internal use */\n> \tstruct object_id head_oid;\n> \tint is_detached;\n> \tint is_bare;\n> \tint is_current;\n> -\tint lock_reason_valid;\n> +\tint is_locked;\n> };\n> \n> /* Functions for acting on the information about worktrees. */\n> @@ -57,10 +56,9 @@ extern struct worktree *find_worktree(struct worktree **list,\n> extern int is_main_worktree(const struct worktree *wt);\n> \n> /*\n> - * Return the reason string if the given worktree is locked or NULL\n> - * otherwise.\n> + * Return the reason string if the given worktree is locked or die\n> */\n> -extern const char *is_worktree_locked(struct worktree *wt);\n> +extern const char *worktree_locked_reason(const struct worktree *wt);\n> \n> #define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)\n"},{"id":"361550","messageId":"CAPig+cTvKd2DVu7wW_A31p_o7BaNJszu14kNRz9sqk8h45H4-g@mail.gmail.com","threadId":"49658","inReplyTo":"CAC05386F1X7TsPr6kgkuLWEwsmdiQ4VKTF5RxaHvzpkwbmXPBw@mail.gmail.com","subject":"Re: [PATCH] worktree: populate lock_reason in get_worktrees and light refactor/cleanup in worktree files","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-10-25T19:14:25Z","receivedAt":"2018-10-25T19:14:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Oct 25, 2018 at 1:47 AM Nickolai Belakovski\n wrote:\n> The motivation for the change is some work that I'm doing to add a\n> worktree atom in ref-filter.c. I wanted that atom to be able to access\n> all fields of the worktree struct and noticed that lock_reason wasn't\n> getting populated so I figured I'd go and fix that.\n>\n> Reviewing this work in the context of your feedback and Junio's\n> previous comments, I think it makes sense to only have a field in the\n> struct indicating whether or not the worktree is locked, and have a\n> separate function for getting the reason.\n\nIs your new ref-filter atom going to be boolean-only or will it also\nhave a form (or a separate atom) for retrieving the lock-reason? I\nimagine both could be desirable.\n\nIn any event, implementation-wise, I would think that such an atom (or\natoms) could be easily built with the existing worktree API (with its\nlazy-loading and caching), which might be an easy way forward since\nyou wouldn't need this patch or the updated one you posted[1], thus no\nneed to justify such a change.\n\n> Since the only cases in\n> which the reason is retrieved in the current codebase are cases where\n> the program immediately dies, caching seems a moot point.\n\nIf your new atom has a form for retrieving the lock reason, then\ncaching could potentially be beneficial(?).\n\n[1]: https://public-inbox.org/git/20181025055142.38077-1-nbelakovski@gmail.com/\n"},{"id":"361846","messageId":"CAC05385y3fCdG4fd2ADahoE0iT+a5KvEr846UCUCQZMOtzzYGg@mail.gmail.com","threadId":"49658","inReplyTo":"xmqq4ldajz05.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible","fromName":"Nickolai Belakovski","fromEmail":"nbelakovski@gmail.com","sentAt":"2018-10-28T21:56:05Z","receivedAt":"2018-10-28T21:56:35Z","isPatch":true,"sender":{"key":"nbelakovski@gmail.com","avatar":"https://avatars.githubusercontent.com/u/864630?v=4"},"body":"This was meant to be a reply to\nhttps://public-inbox.org/git/CAC05386F1X7TsPr6kgkuLWEwsmdiQ4VKTF5RxaHvzpkwbmXPBw@mail.gmail.com/T/#m8898c8f7c68e1ea234aca21cb2d7776b375c6f51,\nplease look there for some more context. I think it both did and\ndidn't get listed as a reply? In my mailbox I see two separate threads\nbut in public-inbox.org/git it looks like it correctly got labelled as\n1 thread. This whole mailing list thing is new to me, thanks for\nbearing with me as I figure it out :). Next time I'll make sure to\nchange the subject line on updated patches as PATCH v2 (that's the\nconvention, right?).\n\nThis is an improvement because it fixes an issue in which the fields\nlock_reason and lock_reason_valid of the worktree struct were not\nbeing populated. This is related to work I'm doing to add a worktree\natom to ref-filter.c.\n\nI see your concerns about extra hits to the filesystem when calling\nget_worktrees and about users interested in lock_reason having to make\nextra calls. As regards hits to the filesystem, I could remove\nis_locked from the worktree struct entirely. To address the second\nconcern, I could refactor worktree_locked_reason to return null if the\nwt is not locked. I would still want to keep is_worktree_locked around\nto provide a facility to check whether or not the worktree is locked\nwithout having to go get the reason.\n\nThere's also been some concerns raised about caching. As I pointed out\nin the other thread, the current use cases for this information die\nupon accessing it, so caching is a moot point. For the use case of a\nworktree atom, caching would be relevant, but it could be done within\nref-filter.c. Another option is to add the lock_reason back to the\nworktree struct and have two functions for populating it:\nget_worktrees_wo_lock_reason and get_worktrees_with_lock_reason. A bit\nmore verbose, but it makes it clear to the caller what they're getting\nand what they're not getting. I might suggest starting with doing the\ncaching within ref-filter.c first, and if more use cases appear for\ncaching lock_reason we can consider the second option. It could also\nbe get_worktrees and get_worktrees_wo_lock_reason, though I think most\ncallers would be calling the latter name.\n\nSo, my proposal for driving this patch to completion would be to:\n-remove is_locked from the worktree struct\n-refactor worktree_locked_reason to return null if the wt is not locked\n-refactor calls to is_locked within builtin/worktree.c to call either\nthe refactored worktree_locked_reason or is_worktree_locked\n\nIn addition to making the worktree code clearer, this patch fixes a\nbug in which the current is_worktree_locked over-eagerly sets\nlock_reason_valid. There are currently no consumers of\nlock_reason_valid within master, but obviously we should fix this\nbefore they appear :)\n\nThoughts?\n\nOn Wed, Oct 24, 2018 at 11:56 PM Junio C Hamano wrote:\n>\n> nbelakovski@gmail.com writes:\n>\n> > From: Nickolai Belakovski \n> >\n> > lock_reason_valid is renamed to is_locked and lock_reason is removed as\n> > a field of the worktree struct. Lock reason can be obtained instead by a\n> > standalone function.\n> >\n> > This is done in order to make the worktree struct more intuitive when it\n> > is used elsewhere in the codebase.\n>\n> So a mere action of getting an in-core worktree instance now has to\n> make an extra call to file_exists(), and in addition, the callers\n> who want to learn why the worktree is locked, they need to open and\n> read the contents of the file in addition?\n>\n> Why is that an improvement?\n>\n>\n> >\n> > Some unused variables are cleaned up as well.\n> >\n> > Signed-off-by: Nickolai Belakovski \n> > ---\n> > builtin/worktree.c | 16 ++++++++--------\n> > worktree.c | 55 ++++++++++++++++++++++++++++--------------------------\n> > worktree.h | 8 +++-----\n> > 3 files changed, 40 insertions(+), 39 deletions(-)\n> >\n> > diff --git a/builtin/worktree.c b/builtin/worktree.c\n> > index 41e771439..844789a21 100644\n> > --- a/builtin/worktree.c\n> > +++ b/builtin/worktree.c\n> > @@ -634,8 +634,8 @@ static int lock_worktree(int ac, const char **av, const char *prefix)\n> > if (is_main_worktree(wt))\n> > die(_(\"The main working tree cannot be locked or unlocked\"));\n> >\n> > - old_reason = is_worktree_locked(wt);\n> > - if (old_reason) {\n> > + if (wt->is_locked) {\n> > + old_reason = worktree_locked_reason(wt);\n> > if (*old_reason)\n> > die(_(\"'%s' is already locked, reason: %s\"),\n> > av[0], old_reason);\n> > @@ -666,7 +666,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)\n> > die(_(\"'%s' is not a working tree\"), av[0]);\n> > if (is_main_worktree(wt))\n> > die(_(\"The main working tree cannot be locked or unlocked\"));\n> > - if (!is_worktree_locked(wt))\n> > + if (!wt->is_locked)\n> > die(_(\"'%s' is not locked\"), av[0]);\n> > ret = unlink_or_warn(git_common_path(\"worktrees/%s/locked\", wt->id));\n> > free_worktrees(worktrees);\n> > @@ -734,8 +734,8 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n> >\n> > validate_no_submodules(wt);\n> >\n> > - reason = is_worktree_locked(wt);\n> > - if (reason) {\n> > + if (wt->is_locked) {\n> > + reason = worktree_locked_reason(wt);\n> > if (*reason)\n> > die(_(\"cannot move a locked working tree, lock reason: %s\"),\n> > reason);\n> > @@ -860,11 +860,11 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n> > die(_(\"'%s' is not a working tree\"), av[0]);\n> > if (is_main_worktree(wt))\n> > die(_(\"'%s' is a main working tree\"), av[0]);\n> > - reason = is_worktree_locked(wt);\n> > - if (reason) {\n> > + if (wt->is_locked) {\n> > + reason = worktree_locked_reason(wt);\n> > if (*reason)\n> > die(_(\"cannot remove a locked working tree, lock reason: %s\"),\n> > - reason);\n> > + reason);\n> > die(_(\"cannot remove a locked working tree\"));\n> > }\n> > if (validate_worktree(wt, &errmsg, WT_VALIDATE_WORKTREE_MISSING_OK))\n> > diff --git a/worktree.c b/worktree.c\n> > index 97cda5f97..a3082d19d 100644\n> > --- a/worktree.c\n> > +++ b/worktree.c\n> > @@ -14,7 +14,6 @@ void free_worktrees(struct worktree **worktrees)\n> > free(worktrees[i]->path);\n> > free(worktrees[i]->id);\n> > free(worktrees[i]->head_ref);\n> > - free(worktrees[i]->lock_reason);\n> > free(worktrees[i]);\n> > }\n> > free (worktrees);\n> > @@ -41,13 +40,29 @@ static void add_head_info(struct worktree *wt)\n> > wt->is_detached = 1;\n> > }\n> >\n> > +\n> > +/**\n> > + * Return 1 if the worktree is locked, 0 otherwise\n> > + */\n> > +static int is_worktree_locked(const struct worktree *wt)\n> > +{\n> > + struct strbuf path = STRBUF_INIT;\n> > + int locked_file_exists;\n> > +\n> > + assert(!is_main_worktree(wt));\n> > +\n> > + strbuf_addstr(&path, worktree_git_path(wt, \"locked\"));\n> > + locked_file_exists = file_exists(path.buf);\n> > + strbuf_release(&path);\n> > + return locked_file_exists;\n> > +}\n> > +\n> > /**\n> > * get the main worktree\n> > */\n> > static struct worktree *get_main_worktree(void)\n> > {\n> > struct worktree *worktree = NULL;\n> > - struct strbuf path = STRBUF_INIT;\n> > struct strbuf worktree_path = STRBUF_INIT;\n> > int is_bare = 0;\n> >\n> > @@ -56,14 +71,11 @@ static struct worktree *get_main_worktree(void)\n> > if (is_bare)\n> > strbuf_strip_suffix(&worktree_path, \"/.\");\n> >\n> > - strbuf_addf(&path, \"%s/HEAD\", get_git_common_dir());\n> > -\n> > worktree = xcalloc(1, sizeof(*worktree));\n> > worktree->path = strbuf_detach(&worktree_path, NULL);\n> > worktree->is_bare = is_bare;\n> > add_head_info(worktree);\n> >\n> > - strbuf_release(&path);\n> > strbuf_release(&worktree_path);\n> > return worktree;\n> > }\n> > @@ -89,12 +101,10 @@ static struct worktree *get_linked_worktree(const char *id)\n> > strbuf_strip_suffix(&worktree_path, \"/.\");\n> > }\n> >\n> > - strbuf_reset(&path);\n> > - strbuf_addf(&path, \"%s/worktrees/%s/HEAD\", get_git_common_dir(), id);\n> > -\n> > worktree = xcalloc(1, sizeof(*worktree));\n> > worktree->path = strbuf_detach(&worktree_path, NULL);\n> > worktree->id = xstrdup(id);\n> > + worktree->is_locked = is_worktree_locked(worktree);\n> > add_head_info(worktree);\n> >\n> > done:\n> > @@ -231,27 +241,20 @@ int is_main_worktree(const struct worktree *wt)\n> > return !wt->id;\n> > }\n> >\n> > -const char *is_worktree_locked(struct worktree *wt)\n> > +const char *worktree_locked_reason(const struct worktree *wt)\n> > {\n> > - assert(!is_main_worktree(wt));\n> > + struct strbuf path = STRBUF_INIT;\n> > + struct strbuf lock_reason = STRBUF_INIT;\n> >\n> > - if (!wt->lock_reason_valid) {\n> > - struct strbuf path = STRBUF_INIT;\n> > -\n> > - strbuf_addstr(&path, worktree_git_path(wt, \"locked\"));\n> > - if (file_exists(path.buf)) {\n> > - struct strbuf lock_reason = STRBUF_INIT;\n> > - if (strbuf_read_file(&lock_reason, path.buf, 0) < 0)\n> > - die_errno(_(\"failed to read '%s'\"), path.buf);\n> > - strbuf_trim(&lock_reason);\n> > - wt->lock_reason = strbuf_detach(&lock_reason, NULL);\n> > - } else\n> > - wt->lock_reason = NULL;\n> > - wt->lock_reason_valid = 1;\n> > - strbuf_release(&path);\n> > - }\n> > + assert(!is_main_worktree(wt));\n> > + assert(wt->is_locked);\n> >\n> > - return wt->lock_reason;\n> > + strbuf_addstr(&path, worktree_git_path(wt, \"locked\"));\n> > + if (strbuf_read_file(&lock_reason, path.buf, 0) < 0)\n> > + die_errno(_(\"failed to read '%s'\"), path.buf);\n> > + strbuf_trim(&lock_reason);\n> > + strbuf_release(&path);\n> > + return strbuf_detach(&lock_reason, NULL);\n> > }\n> >\n> > /* convenient wrapper to deal with NULL strbuf */\n> > diff --git a/worktree.h b/worktree.h\n> > index df3fc30f7..6717287e8 100644\n> > --- a/worktree.h\n> > +++ b/worktree.h\n> > @@ -10,12 +10,11 @@ struct worktree {\n> > char *path;\n> > char *id;\n> > char *head_ref; /* NULL if HEAD is broken or detached */\n> > - char *lock_reason; /* internal use */\n> > struct object_id head_oid;\n> > int is_detached;\n> > int is_bare;\n> > int is_current;\n> > - int lock_reason_valid;\n> > + int is_locked;\n> > };\n> >\n> > /* Functions for acting on the information about worktrees. */\n> > @@ -57,10 +56,9 @@ extern struct worktree *find_worktree(struct worktree **list,\n> > extern int is_main_worktree(const struct worktree *wt);\n> >\n> > /*\n> > - * Return the reason string if the given worktree is locked or NULL\n> > - * otherwise.\n> > + * Return the reason string if the given worktree is locked or die\n> > */\n> > -extern const char *is_worktree_locked(struct worktree *wt);\n> > +extern const char *worktree_locked_reason(const struct worktree *wt);\n> >\n> > #define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)\n"},{"id":"361852","messageId":"CAPig+cT1XYt60PsRGJ0FUa_qCn1vPjdXHygsWzYZYg2Ey=yqkg@mail.gmail.com","threadId":"49658","inReplyTo":"CAC05386cSUhBm4TLD5NUeb5Ut9GT5=h-1MvqDnFpuc+UdZFmwg@mail.gmail.com","subject":"Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-10-28T23:02:50Z","receivedAt":"2018-10-28T23:05:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 28, 2018 at 5:55 PM Nickolai Belakovski\n> wrote: This was meant to be a reply to\n> https://public-inbox.org/git/CAC05386F1X7TsPr6kgkuLWEwsmdiQ4VKTF5RxaHvzpkwbmXPBw@mail.gmail.com/T/#m8898c8f7c68e1ea234aca21cb2d7776b375c6f51,\n> please look there for some more context. I think it both did and\n> didn't get listed as a reply? In my mailbox I see two separate\n> threads but in public-inbox.org/git it looks like it correctly got\n> labelled as 1 thread. This whole mailing list thing is new to me,\n> thanks for bearing with me as I figure it out :).\n\nGmail threads messages entirely by subject; it doesn't pay attention\nto In-Reply-To: or other headers for threading, which is why you see\ntwo separate threads. public-inbox.org, on the other hand, does pay\nattention to the headers, thus understands that all the messages\nbelong to the same thread. Gmail's behavior may be considered\nanomalous.\n\n> Next time I'll make sure to change the subject line on updated\n> patches as PATCH v2 (that's the convention, right?).\n\nThat's correct.\n\n> This is an improvement because it fixes an issue in which the fields\n> lock_reason and lock_reason_valid of the worktree struct were not\n> being populated. This is related to work I'm doing to add a worktree\n> atom to ref-filter.c.\n\nThose fields are considered private/internal. They are not intended to\nbe accessed by calling code. (Unfortunately, only 'lock_reason' is\nthus marked; 'lock_reason_valid' should be marked \"internal\".) Clients\nare expected to retrieve the lock reason only through the provided\nAPI, is_worktree_locked().\n\n> I see your concerns about extra hits to the filesystem when calling\n> get_worktrees and about users interested in lock_reason having to\n> make extra calls. As regards hits to the filesystem, I could remove\n> is_locked from the worktree struct entirely. To address the second\n> concern, I could refactor worktree_locked_reason to return null if\n> the wt is not locked. I would still want to keep is_worktree_locked\n> around to provide a facility to check whether or not the worktree is\n> locked without having to go get the reason.\n>\n> There's also been some concerns raised about caching. As I pointed\n> out in the other thread, the current use cases for this information\n> die upon accessing it, so caching is a moot point. For the use case\n> of a worktree atom, caching would be relevant, but it could be done\n> within ref-filter.c. Another option is to add the lock_reason back\n> to the worktree struct and have two functions for populating it:\n> get_worktrees_wo_lock_reason and get_worktrees_with_lock_reason. A\n> bit more verbose, but it makes it clear to the caller what they're\n> getting and what they're not getting. I might suggest starting with\n> doing the caching within ref-filter.c first, and if more use cases\n> appear for caching lock_reason we can consider the second option. It\n> could also be get_worktrees and get_worktrees_wo_lock_reason, though\n> I think most callers would be calling the latter name.\n>\n> So, my proposal for driving this patch to completion would be to:\n> -remove is_locked from the worktree struct\n> -refactor worktree_locked_reason to return null if the wt is not locked\n> -refactor calls to is_locked within builtin/worktree.c to call\n> either the refactored worktree_locked_reason or is_worktree_locked\n\nMy impression, thus far, is that this all seems to be complicating\nrather than simplifying. These changes also seem entirely unnecessary.\nIn [1], I made the observation that it seemed that your new ref-filter\natom could be implemented with the existing is_worktree_locked() API.\nAs far as I can tell, it can indeed be implemented without having to\nmake any changes to the worktree API or implementation at all.\n\nThe worktree API is both compact and orthogonal, and I haven't yet\nseen a compelling reason to change it. That said, though, the API\ndocumentation in worktree.h may be lacking, even if the implementation\nis not. I'll say a bit more about that below.\n\n> In addition to making the worktree code clearer, this patch fixes a\n> bug in which the current is_worktree_locked over-eagerly sets\n> lock_reason_valid. There are currently no consumers of\n> lock_reason_valid within master, but obviously we should fix this\n> before they appear :)\n\nAs noted above, 'lock_reason_valid' is private/internal. It's an\naccident that it is not annotated such (like 'lock_reason', which is\ncorrectly annotated as \"internal\"). So, there should never be any\nexternal consumers of that field. It also means that there is no bug\nin the current code (as far as I can see) since that field is\ncorrectly consulted (internally) to determine whether the lock reason\nhas been looked up yet.\n\nThe missing \"internal only\" annotation is unfortunate since it may\nhave led you down this road of considering the implementation and API\nbroken.\n\nMoreover, the documentation for is_worktree_locked() apparently\ndoesn't convey strongly enough that it serves the dual purpose of (1)\ntelling you whether or not the worktree is locked, and (2) telling you\nthe reason it is locked.\n\nA patch which adds the missing \"internal only\" annotation to\n'lock_reason_valid', and which makes it easier to understand the dual\npurpose of is_worktree_locked() would be welcome, especially if it\nhelps avoid such confusion in the future.\n\nAside from that, it doesn't seem like worktree needs any changes for\nthe ref-filter atom you have in mind. (Don't interpret this\nobservation as me being averse to changes to the API; I'm open to\nimprovements, but haven't seen anything yet indicating a bug or\nshowing that the API is more difficult than it ought to be.)\n\n[1]: https://public-inbox.org/git/CAPig+cTvKd2DVu7wW_A31p_o7BaNJszu14kNRz9sqk8h45H4-g@mail.gmail.com/\n"},{"id":"361857","messageId":"CAC05387mfDhJ5_=LyzxZZX09MoY1hsmSB1gseNeLCmMOUx2O4A@mail.gmail.com","threadId":"49658","inReplyTo":"CAPig+cT1XYt60PsRGJ0FUa_qCn1vPjdXHygsWzYZYg2Ey=yqkg@mail.gmail.com","subject":"Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible","fromName":"Nickolai Belakovski","fromEmail":"nbelakovski@gmail.com","sentAt":"2018-10-29T01:10:54Z","receivedAt":"2018-10-29T01:11:24Z","isPatch":true,"sender":{"key":"nbelakovski@gmail.com","avatar":"https://avatars.githubusercontent.com/u/864630?v=4"},"body":"On Sun, Oct 28, 2018 at 4:03 PM Eric Sunshine wrote:\n>\n> On Sun, Oct 28, 2018 at 5:55 PM Nickolai Belakovski\n> > wrote: This was meant to be a reply to\n> > https://public-inbox.org/git/CAC05386F1X7TsPr6kgkuLWEwsmdiQ4VKTF5RxaHvzpkwbmXPBw@mail.gmail.com/T/#m8898c8f7c68e1ea234aca21cb2d7776b375c6f51,\n> > please look there for some more context. I think it both did and\n> > didn't get listed as a reply? In my mailbox I see two separate\n> > threads but in public-inbox.org/git it looks like it correctly got\n> > labelled as 1 thread. This whole mailing list thing is new to me,\n> > thanks for bearing with me as I figure it out :).\n>\n> Gmail threads messages entirely by subject; it doesn't pay attention\n> to In-Reply-To: or other headers for threading, which is why you see\n> two separate threads. public-inbox.org, on the other hand, does pay\n> attention to the headers, thus understands that all the messages\n> belong to the same thread. Gmail's behavior may be considered\n> anomalous.\n>\n\nGot it, thanks!\n\n> > Next time I'll make sure to change the subject line on updated\n> > patches as PATCH v2 (that's the convention, right?).\n>\n> That's correct.\n>\n\n(thumbs up)\n\n> > This is an improvement because it fixes an issue in which the fields\n> > lock_reason and lock_reason_valid of the worktree struct were not\n> > being populated. This is related to work I'm doing to add a worktree\n> > atom to ref-filter.c.\n>\n> Those fields are considered private/internal. They are not intended to\n> be accessed by calling code. (Unfortunately, only 'lock_reason' is\n> thus marked; 'lock_reason_valid' should be marked \"internal\".) Clients\n> are expected to retrieve the lock reason only through the provided\n> API, is_worktree_locked().\n>\n> > I see your concerns about extra hits to the filesystem when calling\n> > get_worktrees and about users interested in lock_reason having to\n> > make extra calls. As regards hits to the filesystem, I could remove\n> > is_locked from the worktree struct entirely. To address the second\n> > concern, I could refactor worktree_locked_reason to return null if\n> > the wt is not locked. I would still want to keep is_worktree_locked\n> > around to provide a facility to check whether or not the worktree is\n> > locked without having to go get the reason.\n> >\n> > There's also been some concerns raised about caching. As I pointed\n> > out in the other thread, the current use cases for this information\n> > die upon accessing it, so caching is a moot point. For the use case\n> > of a worktree atom, caching would be relevant, but it could be done\n> > within ref-filter.c. Another option is to add the lock_reason back\n> > to the worktree struct and have two functions for populating it:\n> > get_worktrees_wo_lock_reason and get_worktrees_with_lock_reason. A\n> > bit more verbose, but it makes it clear to the caller what they're\n> > getting and what they're not getting. I might suggest starting with\n> > doing the caching within ref-filter.c first, and if more use cases\n> > appear for caching lock_reason we can consider the second option. It\n> > could also be get_worktrees and get_worktrees_wo_lock_reason, though\n> > I think most callers would be calling the latter name.\n> >\n> > So, my proposal for driving this patch to completion would be to:\n> > -remove is_locked from the worktree struct\n> > -refactor worktree_locked_reason to return null if the wt is not locked\n> > -refactor calls to is_locked within builtin/worktree.c to call\n> > either the refactored worktree_locked_reason or is_worktree_locked\n>\n> My impression, thus far, is that this all seems to be complicating\n> rather than simplifying. These changes also seem entirely unnecessary.\n> In [1], I made the observation that it seemed that your new ref-filter\n> atom could be implemented with the existing is_worktree_locked() API.\n> As far as I can tell, it can indeed be implemented without having to\n> make any changes to the worktree API or implementation at all.\n>\n> The worktree API is both compact and orthogonal, and I haven't yet\n> seen a compelling reason to change it. That said, though, the API\n> documentation in worktree.h may be lacking, even if the implementation\n> is not. I'll say a bit more about that below.\n>\n> > In addition to making the worktree code clearer, this patch fixes a\n> > bug in which the current is_worktree_locked over-eagerly sets\n> > lock_reason_valid. There are currently no consumers of\n> > lock_reason_valid within master, but obviously we should fix this\n> > before they appear :)\n>\n> As noted above, 'lock_reason_valid' is private/internal. It's an\n> accident that it is not annotated such (like 'lock_reason', which is\n> correctly annotated as \"internal\"). So, there should never be any\n> external consumers of that field. It also means that there is no bug\n> in the current code (as far as I can see) since that field is\n> correctly consulted (internally) to determine whether the lock reason\n> has been looked up yet.\n\nThank you for explaining this. Looking at the code now it seems\ncrystal clear, but, yea I clearly got on the wrong path initially.\n\n>\n> The missing \"internal only\" annotation is unfortunate since it may\n> have led you down this road of considering the implementation and API\n> broken.\n>\n> Moreover, the documentation for is_worktree_locked() apparently\n> doesn't convey strongly enough that it serves the dual purpose of (1)\n> telling you whether or not the worktree is locked, and (2) telling you\n> the reason it is locked.\n>\n> A patch which adds the missing \"internal only\" annotation to\n> 'lock_reason_valid', and which makes it easier to understand the dual\n> purpose of is_worktree_locked() would be welcome, especially if it\n> helps avoid such confusion in the future.\n>\n> Aside from that, it doesn't seem like worktree needs any changes for\n> the ref-filter atom you have in mind. (Don't interpret this\n> observation as me being averse to changes to the API; I'm open to\n> improvements, but haven't seen anything yet indicating a bug or\n> showing that the API is more difficult than it ought to be.)\n>\n> [1]: https://public-inbox.org/git/CAPig+cTvKd2DVu7wW_A31p_o7BaNJszu14kNRz9sqk8h45H4-g@mail.gmail.com/\n\nYou're right that these changes are not necessary in order to make a\nworktree atom.\n\nIf there's no interest in this patch I'll withdraw it.\n\nI had found it really surprising that lock_reason was not populated\nwhen I was accessing it while working on the worktree atom. When\ndigging into it, the \"internal use\" comment told me nothing, both\nbecause there's no convention (that I'm aware of) within C to mark\nfields as such and because it fails to direct the reader to\nis_worktree_locked.\n\nHow about this, I can make a patch that changes the comment next to\nlock_reason to say \"/* private - use is_worktree_locked */\" (choosing\nthe word \"private\" since it's a reserved keyword in C++ and other\nlanguages for implementation details that are meant to be\ninaccessible) and a comment next to lock_reason_valid that just says\n\"/* private */\"? I would also suggest renaming is_worktree_locked to\nworktree_lock_reason, the former makes me think the function is\nreturning a boolean, whereas the latter more clearly conveys that a\nmore detailed piece of information is being returned.\n\nLemme know what you think.\n"},{"id":"361868","messageId":"xmqq36sp76kw.fsf@gitster-ct.c.googlers.com","threadId":"49658","inReplyTo":"CAC05385y3fCdG4fd2ADahoE0iT+a5KvEr846UCUCQZMOtzzYGg@mail.gmail.com","subject":"Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-29T03:52:15Z","receivedAt":"2018-10-29T03:55:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nickolai Belakovski writes:\n\n> This is an improvement because it fixes an issue in which the fields\n> lock_reason and lock_reason_valid of the worktree struct were not\n> being populated.\n\nIf the field \"reason\" should always be populated, there is *no*\nreason why we need the \"valid\" boolean. They work as a pair to\nrealize lazy population of rarely used field. The lazy evaluation\ntechnique is used as an optimization for common case, where majority\nof operations do not care if worktrees are locked and if so why they\nare locked, so that only rare operations that do want to find out\ncan ask \"is this locked and why?\" via is_worktree_locked() interface,\nand at that point we lazily find it out by reading \"locked\" file.\n\nSo it is by design that these fields are not always populated, but\nare populated on demand as book-keeping info internal to the API's\nimplementation. It is not \"an issue\", and changing it is not a\n\"fix\".\n\nIn addition, if we have already checked, then we do not even do the\nsame check again. If in an earlier call we found out that a worktree\nis not locked, we flip the _valid bit to true while setting _reason\nto NULL, so that the next call can say \"oh, that's not locked and we\ncan tell that without looking at the filesystem again\" [*1*].\n\nYou are forcing the callers of get_worktrees() to pay the cost to\ncheck, open and read the \"why is this worktree locked?\" file for all\nworktrees, whether they care if these worktrees are locked or why\nthey are locked. Such a change can be an improvement *ONLY* if you\ncan demonstrate that in the current code most codepaths that call\nget_worktrees() end up calling is_worktree_locked() on all worktrees\nanyways. If that were the case, not having to lazily evaluate the\n\"locked\"-ness, but always check upfront, would have a simplification\nvalue, as either approach would be spending the same cost to open\nand read these \"locked\" files.\n\nBut I do not think it is the case. Outside builtin/worktree.c (and\nyou need to admit \"git worktree\" is a rather rare command in the\nfirst place, so you shouldn't be optimizing for that if it hurts\nother codepaths), builtin/branch.c wants to go to all worktrees and\nupdate their HEAD when a branch is renamed (if the old HEAD is\npointing at the original name, of course), but that code won't care\nif the worktree is locked at all. I do not think of any caller of\nget_worktrees() that want to know if it is locked and why for each\nand every one of them, and I'd be surprised if that *is* the\nmajority, but as a proposer to burden get_worktrees() with this\nextra cost, you certainly would have audited the callers and made\nsure it is worth making them pay the extra cost?\n\nIf we are going to change anything around this area, I'd not be\nsurprised that the right move is to go in the opposite direction.\nRight now, you cannot just get \"is it locked?\" boolean answer (which\ncan be obtained by a simple stat(2) call) without getting \"why is it\nlocked?\" (which takes open(2) & read(2) & close(2)), and if you are\nplanning a new application that wants to ask \"is it locked?\" a lot\nwithout having to know the reason, you may want to make the lazy\nevaluation even lazier by splitting _valid field into two (i.e. a\n\"do we know if this is locked?\" valid bit covers \"is_locked\" bit,\nand another \"do we know why this is locked?\" valid bit accompanies\n\"locked_reason\" string). And the callers would ask two separate\nquestions: is_worktree_locked() that says true or false, and then\nwhy_worktree_locked() that yields NULL or string (i.e. essentially\nthat is what we have as is_worktree_locked() today). Of course,\nsuch a change must also be justified with a code audit to\ndemonstrate that only minority case of the callers of is-locked?\nwants to know why\n\n\n[Footnote]\n\n*1* The codepaths that want to know if a worktree is locked or not\n(and wants to learn the reason) are so rare and concentrated in\nbuiltin/worktree.c, and more importantly, they do not need to ask\nthe same question twice, so we can stop caching and make\nis_worktree_locked() always go to the filesystem, I think, and that\nmay be a valid change _if_ we allow worktrees to be randomly locked\nand unlocked while we are looking at them, but if we want to worry\nabout such concurrent and competing uses, we need a big\nrepository-wide lock anyway, and it is the least of our problems\nthat the current caching may go stale without getting invalidated.\nThe code will be racing against such concurrent processes even if\nyou made it to go to the filesystem all the time.\n\n"},{"id":"361869","messageId":"CAPig+cTTsbz1pygq6G281V+fR2VVMuchvy1Q1H-KEvJpjJ9ejg@mail.gmail.com","threadId":"49658","inReplyTo":"CAC05387mfDhJ5_=LyzxZZX09MoY1hsmSB1gseNeLCmMOUx2O4A@mail.gmail.com","subject":"Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-10-29T04:01:11Z","receivedAt":"2018-10-29T04:03:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 28, 2018 at 9:11 PM Nickolai Belakovski\n wrote:\n> On Sun, Oct 28, 2018 at 4:03 PM Eric Sunshine wrote:\n> > Aside from that, it doesn't seem like worktree needs any changes for\n> > the ref-filter atom you have in mind. (Don't interpret this\n> > observation as me being averse to changes to the API; I'm open to\n> > improvements, but haven't seen anything yet indicating a bug or\n> > showing that the API is more difficult than it ought to be.)\n>\n> You're right that these changes are not necessary in order to make a\n> worktree atom.\n> If there's no interest in this patch I'll withdraw it.\n\nWithdrawing this patch seems reasonable.\n\n> I had found it really surprising that lock_reason was not populated\n> when I was accessing it while working on the worktree atom. When\n> digging into it, the \"internal use\" comment told me nothing, both\n> because there's no convention (that I'm aware of) within C to mark\n> fields as such and because it fails to direct the reader to\n> is_worktree_locked.\n>\n> How about this, I can make a patch that changes the comment next to\n> lock_reason to say \"/* private - use is_worktree_locked */\" (choosing\n> the word \"private\" since it's a reserved keyword in C++ and other\n> languages for implementation details that are meant to be\n> inaccessible) and a comment next to lock_reason_valid that just says\n> \"/* private */\"?\n\nA patch clarifying the \"private\" state of 'lock_reason' and\n'lock_reason_valid' and pointing the reader at is_worktree_locked()\nwould be welcome.\n\nOne extra point: It might be a good idea to mention in the\ndocumentation of is_worktree_locked() that, in addition to returning\nNULL or non-NULL indicating not-locked or locked, the returned\nlock-reason might very well be empty (\"\") when no reason was given by\nthe locker.\n\n> I would also suggest renaming is_worktree_locked to\n> worktree_lock_reason, the former makes me think the function is\n> returning a boolean, whereas the latter more clearly conveys that a\n> more detailed piece of information is being returned.\n\nI think the \"boolean\"-sounding name was intentional since most\n(current) callers only care about that; so, the following reads very\nnaturally for such callers:\n\n if (is_worktree_locked(wt))\n die(_(\"worktree locked; aborting\"));\n\nThat said, I wouldn't necessarily oppose renaming the function, but I\nalso don't think it's particularly important to do so.\n"},{"id":"361876","messageId":"CAC05386YPtB5LmHFq3WrAaZ1vmZaBUdGr9hEbyR8KABzj+CzZQ@mail.gmail.com","threadId":"49658","inReplyTo":"xmqq36sp76kw.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible","fromName":"Nickolai Belakovski","fromEmail":"nbelakovski@gmail.com","sentAt":"2018-10-29T05:43:35Z","receivedAt":"2018-10-29T05:44:05Z","isPatch":true,"sender":{"key":"nbelakovski@gmail.com","avatar":"https://avatars.githubusercontent.com/u/864630?v=4"},"body":"On Sun, Oct 28, 2018 at 8:52 PM Junio C Hamano wrote:\n>\n>\n> If the field \"reason\" should always be populated, there is *no*\n> reason why we need the \"valid\" boolean. They work as a pair to\n> realize lazy population of rarely used field. The lazy evaluation\n> technique is used as an optimization for common case, where majority\n> of operations do not care if worktrees are locked and if so why they\n> are locked, so that only rare operations that do want to find out\n> can ask \"is this locked and why?\" via is_worktree_locked() interface,\n> and at that point we lazily find it out by reading \"locked\" file.\n>\n> So it is by design that these fields are not always populated, but\n> are populated on demand as book-keeping info internal to the API's\n> implementation. It is not \"an issue\", and changing it is not a\n> \"fix\".\n\nHaving fields in a struct that are not populated by a getter function\nwith no documentation indicating that they are not populated and no\ndocumentation explaining how to populate them is the issue here.\n\n>\n> In addition, if we have already checked, then we do not even do the\n> same check again. If in an earlier call we found out that a worktree\n> is not locked, we flip the _valid bit to true while setting _reason\n> to NULL, so that the next call can say \"oh, that's not locked and we\n> can tell that without looking at the filesystem again\" [*1*].\n\nI clearly misunderstood the use case of the _valid flag, thanks for\npointing it out.\n\n>\n> You are forcing the callers of get_worktrees() to pay the cost to\n> check, open and read the \"why is this worktree locked?\" file for all\n> worktrees, whether they care if these worktrees are locked or why\n> they are locked. Such a change can be an improvement *ONLY* if you\n> can demonstrate that in the current code most codepaths that call\n> get_worktrees() end up calling is_worktree_locked() on all worktrees\n> anyways. If that were the case, not having to lazily evaluate the\n> \"locked\"-ness, but always check upfront, would have a simplification\n> value, as either approach would be spending the same cost to open\n> and read these \"locked\" files.\n>\n> But I do not think it is the case. Outside builtin/worktree.c (and\n> you need to admit \"git worktree\" is a rather rare command in the\n> first place, so you shouldn't be optimizing for that if it hurts\n> other codepaths), builtin/branch.c wants to go to all worktrees and\n> update their HEAD when a branch is renamed (if the old HEAD is\n> pointing at the original name, of course), but that code won't care\n> if the worktree is locked at all. I do not think of any caller of\n> get_worktrees() that want to know if it is locked and why for each\n> and every one of them, and I'd be surprised if that *is* the\n> majority, but as a proposer to burden get_worktrees() with this\n> extra cost, you certainly would have audited the callers and made\n> sure it is worth making them pay the extra cost?\n>\n> If we are going to change anything around this area, I'd not be\n> surprised that the right move is to go in the opposite direction.\n> Right now, you cannot just get \"is it locked?\" boolean answer (which\n> can be obtained by a simple stat(2) call) without getting \"why is it\n> locked?\" (which takes open(2) & read(2) & close(2)), and if you are\n> planning a new application that wants to ask \"is it locked?\" a lot\n> without having to know the reason, you may want to make the lazy\n> evaluation even lazier by splitting _valid field into two (i.e. a\n> \"do we know if this is locked?\" valid bit covers \"is_locked\" bit,\n> and another \"do we know why this is locked?\" valid bit accompanies\n> \"locked_reason\" string). And the callers would ask two separate\n> questions: is_worktree_locked() that says true or false, and then\n> why_worktree_locked() that yields NULL or string (i.e. essentially\n> that is what we have as is_worktree_locked() today). Of course,\n> such a change must also be justified with a code audit to\n> demonstrate that only minority case of the callers of is-locked?\n> wants to know why\n>\n>\n> [Footnote]\n>\n> *1* The codepaths that want to know if a worktree is locked or not\n> (and wants to learn the reason) are so rare and concentrated in\n> builtin/worktree.c, and more importantly, they do not need to ask\n> the same question twice, so we can stop caching and make\n> is_worktree_locked() always go to the filesystem, I think, and that\n> may be a valid change _if_ we allow worktrees to be randomly locked\n> and unlocked while we are looking at them, but if we want to worry\n> about such concurrent and competing uses, we need a big\n> repository-wide lock anyway, and it is the least of our problems\n> that the current caching may go stale without getting invalidated.\n> The code will be racing against such concurrent processes even if\n> you made it to go to the filesystem all the time.\n>\n\nBasically, I already implemented most of what you're saying. The v2\nproposal does force all callers of get_worktrees to check the lock\nstatus, but by calling stat, not open/read/close. That being said\nyou're right that even forcing them to call stat when most don't care\nis imposing an extra cost for no gain. The v2 proposal no longer\ncaches the lock reason (in fact it removes it from the worktree\nstruct), since not only do current users have no need to ask for the\nlock_reason twice, none of them ask for it twice in the first place.\nThe v2 proposal provides a standalone function for getting the actual\nreason (leaving it up to callers to cache the result if they like).\n\nI'd be up for removing is_locked from the struct as well and making a\nseparate standalone function for that.\n\nEither way, I do see an issue with the current code that anybody who\nwants to know the lock status and/or lock reason of a worktree gets\nfaced with a confusing, misleading, and opaque piece of code. I see\ntwo possible remedies:\n\na) Remove these fields from the worktree struct and provide standalone\nfunctions for answering these questions. Pros are that we follow\nsingle responsibility principle with two standalone functions for\ndoing so, and we make the route for answering these questions less\ncircuitous (IMO). Cons are that we remove the caching currently in\nplace since we're no longer storing this info in the struct, but then\nagain that caching is not currently being used and can be implemented\nby callers if they really need it\n\nb) Update the comments in the code to state that lock_reason and\nlock_reason_valid are to be considered private fields and to use\nis_worktree_locked for populating them. Pros are that no actual code\nchanges need to be made. Cons are that, IMO, it's still a strange\npiece of code in that it's doing some sort of quasi object oriented\nstuff in C, and if we can take the opportunity to make the code look a\nbit more canonical I think we should, but that's just my 2 cents.\n\nOf course there's also option c, which is that I leave this alone and\njust go back to making my worktree atom :)\n"},{"id":"361877","messageId":"CAC05387rFq0yJ3nUVkb0jUyQy=EmZiCnBW9L53A6GS5=U0qUDg@mail.gmail.com","threadId":"49658","inReplyTo":"CAPig+cTTsbz1pygq6G281V+fR2VVMuchvy1Q1H-KEvJpjJ9ejg@mail.gmail.com","subject":"Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible","fromName":"Nickolai Belakovski","fromEmail":"nbelakovski@gmail.com","sentAt":"2018-10-29T05:45:12Z","receivedAt":"2018-10-29T05:45:41Z","isPatch":true,"sender":{"key":"nbelakovski@gmail.com","avatar":"https://avatars.githubusercontent.com/u/864630?v=4"},"body":"On Sun, Oct 28, 2018 at 9:01 PM Eric Sunshine wrote:\n>\n> On Sun, Oct 28, 2018 at 9:11 PM Nickolai Belakovski\n> wrote:\n> > I would also suggest renaming is_worktree_locked to\n> > worktree_lock_reason, the former makes me think the function is\n> > returning a boolean, whereas the latter more clearly conveys that a\n> > more detailed piece of information is being returned.\n>\n> I think the \"boolean\"-sounding name was intentional since most\n> (current) callers only care about that; so, the following reads very\n> naturally for such callers:\n>\n> if (is_worktree_locked(wt))\n> die(_(\"worktree locked; aborting\"));\n>\n> That said, I wouldn't necessarily oppose renaming the function, but I\n> also don't think it's particularly important to do so.\n\nActually it's 3:2 in the current code for callers getting the reason\nout of the function vs callers checking the value of the pointer for\nnull/not null. This leads to some rather unnatural looking code in the\ncurrent repo like\n\nreason = is_worktree_locked(wt);\n\nI think it would look a lot more natural if it were \"reason =\nworktree_lock_reason(wt)\". The resulting if-statement wouldn't be too\nbad, IMO\n\nif (worktree_lock_reason(wt))\n die(_(\"worktree locked; aborting\"));\n\nTo me, I would just go lookup the signature of worktree_lock_reason\nand see that it returns a pointer and I'd be satisfied with that. I\ncould also infer that from looking at the code if I'm just skimming\nthrough. But if I see code like \"reason = is_worktree_locked(wt)\" I'm\nlike hold on, what's going on here?! :P\n"},{"id":"361879","messageId":"CAPig+cS+djfZjoEbBNrVGpd4g6ZhsioSqK=ZyKprTwA0Hy4iiw@mail.gmail.com","threadId":"49658","inReplyTo":"CAC05387rFq0yJ3nUVkb0jUyQy=EmZiCnBW9L53A6GS5=U0qUDg@mail.gmail.com","subject":"Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-10-29T06:21:52Z","receivedAt":"2018-10-29T06:22:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Oct 29, 2018 at 1:45 AM Nickolai Belakovski\n wrote:\n> On Sun, Oct 28, 2018 at 9:01 PM Eric Sunshine wrote:\n> > That said, I wouldn't necessarily oppose renaming the function, but I\n> > also don't think it's particularly important to do so.\n>\n> To me, I would just go lookup the signature of worktree_lock_reason\n> and see that it returns a pointer and I'd be satisfied with that. I\n> could also infer that from looking at the code if I'm just skimming\n> through. But if I see code like \"reason = is_worktree_locked(wt)\" I'm\n> like hold on, what's going on here?! :P\n\nI don't feel strongly about it, and, as indicated, wouldn't\nnecessarily be opposed to it. If you do want to make that change,\nperhaps send it as the second patch of a 2-patch series in which patch\n1 just updates the API documentation. That way, if anyone does oppose\nthe rename in patch 2, then that patch can be dropped without having\nto re-send.\n"},{"id":"361880","messageId":"xmqqbm7d45k8.fsf@gitster-ct.c.googlers.com","threadId":"49658","inReplyTo":"CAC05386YPtB5LmHFq3WrAaZ1vmZaBUdGr9hEbyR8KABzj+CzZQ@mail.gmail.com","subject":"Re: [PATCH] worktree: refactor lock_reason_valid and lock_reason to be more sensible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-29T06:42:31Z","receivedAt":"2018-10-29T06:42:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nickolai Belakovski writes:\n\n> Either way, I do see an issue with the current code that anybody who\n> wants to know the lock status and/or lock reason of a worktree gets\n> faced with a confusing, misleading, and opaque piece of code.\n\nSorry, I don't. I do not mind a better documentation for\nis_worktree_locked() without doing anything else.\n\nI do not see any reason to remove fields, split the helper funciton\ninto two, drop the caching, etc., especially when the only\njustification is \"I am new to the codebase and find it confusing\".\n\n"},{"id":"361965","messageId":"20181030062409.42169-1-nbelakovski@gmail.com","threadId":"49658","inReplyTo":"20181025055142.38077-1-nbelakovski@gmail.com","subject":"[PATCH v3 1/2] worktree: update documentation for lock_reason and lock_reason_valid","fromName":"","fromEmail":"nbelakovski@gmail.com","sentAt":"2018-10-30T06:24:08Z","receivedAt":"2018-10-30T06:24:26Z","isPatch":true,"sender":{"key":"nbelakovski@gmail.com","avatar":"https://avatars.githubusercontent.com/u/864630?v=4"},"body":"From: Nickolai Belakovski \n\nClarify that these fields are to be considered implementation details\nand direct the reader to use the is_worktree_locked function to retrieve\nsaid information.\n\nSigned-off-by: Nickolai Belakovski \n---\n worktree.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/worktree.h b/worktree.h\nindex df3fc30f7..6b12a3cf6 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -10,12 +10,12 @@ struct worktree {\n \tchar *path;\n \tchar *id;\n \tchar *head_ref;\t\t/* NULL if HEAD is broken or detached */\n-\tchar *lock_reason;\t/* internal use */\n+\tchar *lock_reason;\t/* private - use is_worktree_locked */\n \tstruct object_id head_oid;\n \tint is_detached;\n \tint is_bare;\n \tint is_current;\n-\tint lock_reason_valid;\n+\tint lock_reason_valid; /* private */\n };\n \n /* Functions for acting on the information about worktrees. */\n-- \n2.14.2\n\n"},{"id":"361966","messageId":"20181030062409.42169-2-nbelakovski@gmail.com","threadId":"49658","inReplyTo":"20181030062409.42169-1-nbelakovski@gmail.com","subject":"[PATCH v3 2/2] worktree: rename is_worktree_locked to worktree_lock_reason","fromName":"","fromEmail":"nbelakovski@gmail.com","sentAt":"2018-10-30T06:24:09Z","receivedAt":"2018-10-30T06:24:49Z","isPatch":true,"sender":{"key":"nbelakovski@gmail.com","avatar":"https://avatars.githubusercontent.com/u/864630?v=4"},"body":"From: Nickolai Belakovski \n\nA function prefixed with 'is_' would be expected to return a boolean,\nhowever this function returns a string.\n\nSigned-off-by: Nickolai Belakovski \n---\n builtin/worktree.c | 10 +++++-----\n worktree.c | 2 +-\n worktree.h | 4 ++--\n 3 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex c4abbde2b..5e8402617 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -245,7 +245,7 @@ static void validate_worktree_add(const char *path, const struct add_opts *opts)\n \tif (!wt)\n \t\tgoto done;\n \n-\tlocked = !!is_worktree_locked(wt);\n+\tlocked = !!worktree_lock_reason(wt);\n \tif ((!locked && opts->force) || (locked && opts->force > 1)) {\n \t\tif (delete_git_dir(wt->id))\n \t\t die(_(\"unable to re-add worktree '%s'\"), path);\n@@ -682,7 +682,7 @@ static int lock_worktree(int ac, const char **av, const char *prefix)\n \tif (is_main_worktree(wt))\n \t\tdie(_(\"The main working tree cannot be locked or unlocked\"));\n \n-\told_reason = is_worktree_locked(wt);\n+\told_reason = worktree_lock_reason(wt);\n \tif (old_reason) {\n \t\tif (*old_reason)\n \t\t\tdie(_(\"'%s' is already locked, reason: %s\"),\n@@ -714,7 +714,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)\n \t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n \tif (is_main_worktree(wt))\n \t\tdie(_(\"The main working tree cannot be locked or unlocked\"));\n-\tif (!is_worktree_locked(wt))\n+\tif (!worktree_lock_reason(wt))\n \t\tdie(_(\"'%s' is not locked\"), av[0]);\n \tret = unlink_or_warn(git_common_path(\"worktrees/%s/locked\", wt->id));\n \tfree_worktrees(worktrees);\n@@ -787,7 +787,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n \tvalidate_no_submodules(wt);\n \n \tif (force < 2)\n-\t\treason = is_worktree_locked(wt);\n+\t\treason = worktree_lock_reason(wt);\n \tif (reason) {\n \t\tif (*reason)\n \t\t\tdie(_(\"cannot move a locked working tree, lock reason: %s\\nuse 'move -f -f' to override or unlock first\"),\n@@ -900,7 +900,7 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n \tif (is_main_worktree(wt))\n \t\tdie(_(\"'%s' is a main working tree\"), av[0]);\n \tif (force < 2)\n-\t\treason = is_worktree_locked(wt);\n+\t\treason = worktree_lock_reason(wt);\n \tif (reason) {\n \t\tif (*reason)\n \t\t\tdie(_(\"cannot remove a locked working tree, lock reason: %s\\nuse 'remove -f -f' to override or unlock first\"),\ndiff --git a/worktree.c b/worktree.c\nindex b0d0b5426..befdbe7fa 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -235,7 +235,7 @@ int is_main_worktree(const struct worktree *wt)\n \treturn !wt->id;\n }\n \n-const char *is_worktree_locked(struct worktree *wt)\n+const char *worktree_lock_reason(struct worktree *wt)\n {\n \tassert(!is_main_worktree(wt));\n \ndiff --git a/worktree.h b/worktree.h\nindex 6b12a3cf6..55d449b6a 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -10,7 +10,7 @@ struct worktree {\n \tchar *path;\n \tchar *id;\n \tchar *head_ref;\t\t/* NULL if HEAD is broken or detached */\n-\tchar *lock_reason;\t/* private - use is_worktree_locked */\n+\tchar *lock_reason;\t/* private - use worktree_lock_reason */\n \tstruct object_id head_oid;\n \tint is_detached;\n \tint is_bare;\n@@ -60,7 +60,7 @@ extern int is_main_worktree(const struct worktree *wt);\n * Return the reason string if the given worktree is locked or NULL\n * otherwise.\n */\n-extern const char *is_worktree_locked(struct worktree *wt);\n+extern const char *worktree_lock_reason(struct worktree *wt);\n \n #define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)\n \n-- \n2.14.2\n\n"},{"id":"362050","messageId":"xmqqlg6ezw6k.fsf@gitster-ct.c.googlers.com","threadId":"49658","inReplyTo":"20181030062409.42169-1-nbelakovski@gmail.com","subject":"Re: [PATCH v3 1/2] worktree: update documentation for lock_reason and lock_reason_valid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-31T02:28:35Z","receivedAt":"2018-10-31T02:31:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"nbelakovski@gmail.com writes:\n\n> From: Nickolai Belakovski \n>\n> Clarify that these fields are to be considered implementation details\n> and direct the reader to use the is_worktree_locked function to retrieve\n> said information.\n>\n> Signed-off-by: Nickolai Belakovski \n> ---\n> worktree.h | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/worktree.h b/worktree.h\n> index df3fc30f7..6b12a3cf6 100644\n> --- a/worktree.h\n> +++ b/worktree.h\n> @@ -10,12 +10,12 @@ struct worktree {\n> \tchar *path;\n> \tchar *id;\n> \tchar *head_ref;\t\t/* NULL if HEAD is broken or detached */\n> -\tchar *lock_reason;\t/* internal use */\n> +\tchar *lock_reason;\t/* private - use is_worktree_locked */\n\ns/use /used by/, probably.\n\n> \tstruct object_id head_oid;\n> \tint is_detached;\n> \tint is_bare;\n> \tint is_current;\n> -\tint lock_reason_valid;\n> +\tint lock_reason_valid; /* private */\n> };\n\nThese annotations to the two fields are not wrong per-se, but I have\na feeling that it would equally be important to document what the\nother \"non-private\" fields mean, if peeking them *is* the API this\nsubsystem offers.\n\nThanks.\n"},{"id":"362051","messageId":"xmqqh8h2zvl4.fsf@gitster-ct.c.googlers.com","threadId":"49658","inReplyTo":"20181030062409.42169-2-nbelakovski@gmail.com","subject":"Re: [PATCH v3 2/2] worktree: rename is_worktree_locked to worktree_lock_reason","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-31T02:41:27Z","receivedAt":"2018-10-31T02:41:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"nbelakovski@gmail.com writes:\n\n> From: Nickolai Belakovski \n>\n> A function prefixed with 'is_' would be expected to return a boolean,\n> however this function returns a string.\n>\n> Signed-off-by: Nickolai Belakovski \n> ---\n\nGiven that there is a clear documentation in worktree.h, and a\npointer that is not NULL is true in \"if/while(ptr)\", I'd say this\nchange is a borderline \"meh\".\n\nI'll queue this on top of 1/2 and try merging to the integration\ntopics to see if it interacts with any other topics in flight.\nSince the patch has already been written, let's not waste the effort\nif there is no conflict with anybody else (otherwise I'd discard\nthis one---a \"meh\" patch is not worth having to worry about conflict\nresolution).\n\nThanks.\n\n> builtin/worktree.c | 10 +++++-----\n> worktree.c | 2 +-\n> worktree.h | 4 ++--\n> 3 files changed, 8 insertions(+), 8 deletions(-)\n>\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index c4abbde2b..5e8402617 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -245,7 +245,7 @@ static void validate_worktree_add(const char *path, const struct add_opts *opts)\n> \tif (!wt)\n> \t\tgoto done;\n> \n> -\tlocked = !!is_worktree_locked(wt);\n> +\tlocked = !!worktree_lock_reason(wt);\n> \tif ((!locked && opts->force) || (locked && opts->force > 1)) {\n> \t\tif (delete_git_dir(wt->id))\n> \t\t die(_(\"unable to re-add worktree '%s'\"), path);\n> @@ -682,7 +682,7 @@ static int lock_worktree(int ac, const char **av, const char *prefix)\n> \tif (is_main_worktree(wt))\n> \t\tdie(_(\"The main working tree cannot be locked or unlocked\"));\n> \n> -\told_reason = is_worktree_locked(wt);\n> +\told_reason = worktree_lock_reason(wt);\n> \tif (old_reason) {\n> \t\tif (*old_reason)\n> \t\t\tdie(_(\"'%s' is already locked, reason: %s\"),\n> @@ -714,7 +714,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix)\n> \t\tdie(_(\"'%s' is not a working tree\"), av[0]);\n> \tif (is_main_worktree(wt))\n> \t\tdie(_(\"The main working tree cannot be locked or unlocked\"));\n> -\tif (!is_worktree_locked(wt))\n> +\tif (!worktree_lock_reason(wt))\n> \t\tdie(_(\"'%s' is not locked\"), av[0]);\n> \tret = unlink_or_warn(git_common_path(\"worktrees/%s/locked\", wt->id));\n> \tfree_worktrees(worktrees);\n> @@ -787,7 +787,7 @@ static int move_worktree(int ac, const char **av, const char *prefix)\n> \tvalidate_no_submodules(wt);\n> \n> \tif (force < 2)\n> -\t\treason = is_worktree_locked(wt);\n> +\t\treason = worktree_lock_reason(wt);\n> \tif (reason) {\n> \t\tif (*reason)\n> \t\t\tdie(_(\"cannot move a locked working tree, lock reason: %s\\nuse 'move -f -f' to override or unlock first\"),\n> @@ -900,7 +900,7 @@ static int remove_worktree(int ac, const char **av, const char *prefix)\n> \tif (is_main_worktree(wt))\n> \t\tdie(_(\"'%s' is a main working tree\"), av[0]);\n> \tif (force < 2)\n> -\t\treason = is_worktree_locked(wt);\n> +\t\treason = worktree_lock_reason(wt);\n> \tif (reason) {\n> \t\tif (*reason)\n> \t\t\tdie(_(\"cannot remove a locked working tree, lock reason: %s\\nuse 'remove -f -f' to override or unlock first\"),\n> diff --git a/worktree.c b/worktree.c\n> index b0d0b5426..befdbe7fa 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -235,7 +235,7 @@ int is_main_worktree(const struct worktree *wt)\n> \treturn !wt->id;\n> }\n> \n> -const char *is_worktree_locked(struct worktree *wt)\n> +const char *worktree_lock_reason(struct worktree *wt)\n> {\n> \tassert(!is_main_worktree(wt));\n> \n> diff --git a/worktree.h b/worktree.h\n> index 6b12a3cf6..55d449b6a 100644\n> --- a/worktree.h\n> +++ b/worktree.h\n> @@ -10,7 +10,7 @@ struct worktree {\n> \tchar *path;\n> \tchar *id;\n> \tchar *head_ref;\t\t/* NULL if HEAD is broken or detached */\n> -\tchar *lock_reason;\t/* private - use is_worktree_locked */\n> +\tchar *lock_reason;\t/* private - use worktree_lock_reason */\n> \tstruct object_id head_oid;\n> \tint is_detached;\n> \tint is_bare;\n> @@ -60,7 +60,7 @@ extern int is_main_worktree(const struct worktree *wt);\n> * Return the reason string if the given worktree is locked or NULL\n> * otherwise.\n> */\n> -extern const char *is_worktree_locked(struct worktree *wt);\n> +extern const char *worktree_lock_reason(struct worktree *wt);\n> \n> #define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)\n"}]}