| Message ID | 20191018194542.1316981-1-pjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/4] libgit: Add a read-only helper to test the worktree lock | expand |
Peter Jones <pjones@redhat.com> writes: > Subject: Re: [PATCH v2 1/4] libgit: Add a read-only helper to test the worktree lock Having a word "worktree" somewhere on the title is good, but have it as the "I am changing this area"; "libgit" does not give readers the hint that this is a step about the worktree subsystem. Subject: [PATCH v2 1/4] worktree: add is_worktree_locked() helper When the new helper function is properly named, like yours, there is not much need to explain what it does (i.e. "to test the worktree lock"), so just "worktree: add is_worktree_locked()" is sufficient. > Add the function is_worktree_locked(), which is a helper to tell if a > worktree is locked without having to be able to modify it. I do not see the reason why your proposed title and log message stress the fact that this helper can be used even by callers that are not permitted to modify the worktree (i.e. the emphasis on "read-only"). Asking for worktree_lock_reason() can be done by anybody, but I do not think we particularly advertise it as read-only. Perhaps drop "without having to..."? > - locked = !!worktree_lock_reason(wt); > + locked = is_worktree_locked(wt); > if ((!locked && opts->force) || (locked && opts->force > 1)) { > if (delete_git_dir(wt->id)) > die(_("unable to re-add worktree '%s'"), path); > diff --git a/worktree.c b/worktree.c > index 5b4793caa34..4924805c389 100644 > --- a/worktree.c > +++ b/worktree.c > @@ -244,6 +244,22 @@ int is_main_worktree(const struct worktree *wt) > return !wt->id; > } > > +int is_worktree_locked(const struct worktree *wt) > +{ > + struct strbuf path = STRBUF_INIT; > + int locked = 0; > + > + if (wt->lock_reason_valid && wt->lock_reason) > + return 1; > + > + strbuf_addstr(&path, worktree_git_path(wt, "locked")); > + if (file_exists(path.buf)) > + locked = 1; If you write locked = file_exists(path.buf); here, then readers do not have to scan backwards and find that the variable is initialized to zero, and that no other statement since its initialization touches its value, in order to see what value is returned when file does not exist. Writing the RHS !!file_exists() concisely allows readers to tell that this function returns only 0 or 1 without having to check what file_exists() returns, but that may probably be overkill. > + strbuf_release(&path); > + return locked; > +} I wondered why this is not just #define is_worktree_locked(wt) (!!worktree_lock_reason(wt)) There are a few differences compared to worktree_lock_reason(): - this can be called on the main worktree by mistake and would probably yield "not locked" (but the existing guard is a mere assert() which probably is stripped away in production builds) - this can be used by a process that cannot even read the contents of the locked file for the reason; - because reason is not read, reason or reason_valid fields are not updated, and repeated calls on the same worktree structure would result in repeated lstat() calls. Shouldn't we be advising the callers that the last one as a potential downside? The fact that the new helper is usable even by read-only callers hints that any caching of earlier results is disabled, but it is somewhat a round-about way to say so. As I do not see why being able to take "const struct worktree *", as opposed to non-const version is a huge advantage, for this helper, I wonder if it would make even more sense to introduce one more level to "lock-reason-valid" and allow caching of is_worktree_locked(). Currently, "lock-reason-valid" only tells us "lock-reason may be NULL, but that does not necessarily mean it is not locked---you have to check it" boolean, but it could be instead a tristate: A: lock-reason may be NULL but that is only because we haven't even tried to see if the lock file exists B: NULL-ness of lock-reason reliably tells if the worktree is locked or not because we have tried file_exists(), but if the field has non-NULL value, that is *not* the string we read; if you want to know the reason, you must read the file. C: NULL in lock-reason means it is not locked; non-NULL in lock-reason is what we read form the file. Also, it may make sense to correct the first difference and in a more meaningful way than assert(), given that the reason why this helper is introduced is eventually to perform an destructive action later in the series. Perhaps if (is_main_worktree(wt)) BUG("is-worktree-locked called for the main worktree"); at the front. Thanks. > const char *worktree_lock_reason(struct worktree *wt) > { > assert(!is_main_worktree(wt)); > diff --git a/worktree.h b/worktree.h > index caecc7a281c..5ff16c414b5 100644 > --- a/worktree.h > +++ b/worktree.h > @@ -56,6 +56,11 @@ struct worktree *find_worktree(struct worktree **list, > */ > int is_main_worktree(const struct worktree *wt); > > +/* > + * Return true if the given worktree is locked > + */ > +int is_worktree_locked(const struct worktree *wt); > + > /* > * Return the reason string if the given worktree is locked or NULL > * otherwise.
diff --git a/builtin/worktree.c b/builtin/worktree.c index 4de44f579af..86305cc1fe1 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -245,7 +245,7 @@ static void validate_worktree_add(const char *path, const struct add_opts *opts) if (!wt) goto done; - locked = !!worktree_lock_reason(wt); + locked = is_worktree_locked(wt); if ((!locked && opts->force) || (locked && opts->force > 1)) { if (delete_git_dir(wt->id)) die(_("unable to re-add worktree '%s'"), path); diff --git a/worktree.c b/worktree.c index 5b4793caa34..4924805c389 100644 --- a/worktree.c +++ b/worktree.c @@ -244,6 +244,22 @@ int is_main_worktree(const struct worktree *wt) return !wt->id; } +int is_worktree_locked(const struct worktree *wt) +{ + struct strbuf path = STRBUF_INIT; + int locked = 0; + + if (wt->lock_reason_valid && wt->lock_reason) + return 1; + + strbuf_addstr(&path, worktree_git_path(wt, "locked")); + if (file_exists(path.buf)) + locked = 1; + + strbuf_release(&path); + return locked; +} + const char *worktree_lock_reason(struct worktree *wt) { assert(!is_main_worktree(wt)); diff --git a/worktree.h b/worktree.h index caecc7a281c..5ff16c414b5 100644 --- a/worktree.h +++ b/worktree.h @@ -56,6 +56,11 @@ struct worktree *find_worktree(struct worktree **list, */ int is_main_worktree(const struct worktree *wt); +/* + * Return true if the given worktree is locked + */ +int is_worktree_locked(const struct worktree *wt); + /* * Return the reason string if the given worktree is locked or NULL * otherwise.
Add the function is_worktree_locked(), which is a helper to tell if a worktree is locked without having to be able to modify it. Signed-off-by: Peter Jones <pjones@redhat.com> --- builtin/worktree.c | 2 +- worktree.c | 16 ++++++++++++++++ worktree.h | 5 +++++ 3 files changed, 22 insertions(+), 1 deletion(-)