* [PATCH v2 1/6] worktree: libify should_prune_worktree()
2021-01-17 23:42 ` [PATCH v2 0/6] " Rafael Silva
@ 2021-01-17 23:42 ` Rafael Silva
2021-01-17 23:42 ` [PATCH v2 2/6] worktree: teach worktree to lazy-load "prunable" reason Rafael Silva
` (6 subsequent siblings)
7 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-17 23:42 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Rafael Silva
As part of teaching "git worktree list" to annotate worktree that is a
candidate for pruning, let's move should_prune_worktree() from
builtin/worktree.c to worktree.c in order to make part of the worktree
public API.
should_prune_worktree() knows how to select the given worktree for
pruning based on an expiration date, however the expiration value is
stored in a static file-scope variable and it is not local to the
function. In order to move the function, teach should_prune_worktree()
to take the expiration date as an argument and document the new
parameter that is not immediately obvious.
Also, change the function comment to clearly state that the worktree's
path is returned in `wtpath` argument.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
builtin/worktree.c | 75 +---------------------------------------------
worktree.c | 68 +++++++++++++++++++++++++++++++++++++++++
worktree.h | 14 +++++++++
3 files changed, 83 insertions(+), 74 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 71287b2da6..dd886d5029 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -67,79 +67,6 @@ static void delete_worktrees_dir_if_empty(void)
rmdir(git_path("worktrees")); /* ignore failed removal */
}
-/*
- * Return true if worktree entry should be pruned, along with the reason for
- * pruning. Otherwise, return false and the worktree's path, or NULL if it
- * cannot be determined. Caller is responsible for freeing returned path.
- */
-static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)
-{
- struct stat st;
- char *path;
- int fd;
- size_t len;
- ssize_t read_result;
-
- *wtpath = NULL;
- if (!is_directory(git_path("worktrees/%s", id))) {
- strbuf_addstr(reason, _("not a valid directory"));
- return 1;
- }
- if (file_exists(git_path("worktrees/%s/locked", id)))
- return 0;
- if (stat(git_path("worktrees/%s/gitdir", id), &st)) {
- strbuf_addstr(reason, _("gitdir file does not exist"));
- return 1;
- }
- fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY);
- if (fd < 0) {
- strbuf_addf(reason, _("unable to read gitdir file (%s)"),
- strerror(errno));
- return 1;
- }
- len = xsize_t(st.st_size);
- path = xmallocz(len);
-
- read_result = read_in_full(fd, path, len);
- if (read_result < 0) {
- strbuf_addf(reason, _("unable to read gitdir file (%s)"),
- strerror(errno));
- close(fd);
- free(path);
- return 1;
- }
- close(fd);
-
- if (read_result != len) {
- strbuf_addf(reason,
- _("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"),
- (uintmax_t)len, (uintmax_t)read_result);
- free(path);
- return 1;
- }
- while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
- len--;
- if (!len) {
- strbuf_addstr(reason, _("invalid gitdir file"));
- free(path);
- return 1;
- }
- path[len] = '\0';
- if (!file_exists(path)) {
- if (stat(git_path("worktrees/%s/index", id), &st) ||
- st.st_mtime <= expire) {
- strbuf_addstr(reason, _("gitdir file points to non-existent location"));
- free(path);
- return 1;
- } else {
- *wtpath = path;
- return 0;
- }
- }
- *wtpath = path;
- return 0;
-}
-
static void prune_worktree(const char *id, const char *reason)
{
if (show_only || verbose)
@@ -195,7 +122,7 @@ static void prune_worktrees(void)
if (is_dot_or_dotdot(d->d_name))
continue;
strbuf_reset(&reason);
- if (should_prune_worktree(d->d_name, &reason, &path))
+ if (should_prune_worktree(d->d_name, &reason, &path, expire))
prune_worktree(d->d_name, reason.buf);
else if (path)
string_list_append(&kept, path)->util = xstrdup(d->d_name);
diff --git a/worktree.c b/worktree.c
index 821b233479..8ae019af79 100644
--- a/worktree.c
+++ b/worktree.c
@@ -741,3 +741,71 @@ void repair_worktree_at_path(const char *path,
strbuf_release(&realdotgit);
strbuf_release(&dotgit);
}
+
+int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, timestamp_t expire)
+{
+ struct stat st;
+ char *path;
+ int fd;
+ size_t len;
+ ssize_t read_result;
+
+ *wtpath = NULL;
+ if (!is_directory(git_path("worktrees/%s", id))) {
+ strbuf_addstr(reason, _("not a valid directory"));
+ return 1;
+ }
+ if (file_exists(git_path("worktrees/%s/locked", id)))
+ return 0;
+ if (stat(git_path("worktrees/%s/gitdir", id), &st)) {
+ strbuf_addstr(reason, _("gitdir file does not exist"));
+ return 1;
+ }
+ fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY);
+ if (fd < 0) {
+ strbuf_addf(reason, _("unable to read gitdir file (%s)"),
+ strerror(errno));
+ return 1;
+ }
+ len = xsize_t(st.st_size);
+ path = xmallocz(len);
+
+ read_result = read_in_full(fd, path, len);
+ if (read_result < 0) {
+ strbuf_addf(reason, _("unable to read gitdir file (%s)"),
+ strerror(errno));
+ close(fd);
+ free(path);
+ return 1;
+ }
+ close(fd);
+
+ if (read_result != len) {
+ strbuf_addf(reason,
+ _("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"),
+ (uintmax_t)len, (uintmax_t)read_result);
+ free(path);
+ return 1;
+ }
+ while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
+ len--;
+ if (!len) {
+ strbuf_addstr(reason, _("invalid gitdir file"));
+ free(path);
+ return 1;
+ }
+ path[len] = '\0';
+ if (!file_exists(path)) {
+ if (stat(git_path("worktrees/%s/index", id), &st) ||
+ st.st_mtime <= expire) {
+ strbuf_addstr(reason, _("gitdir file points to non-existent location"));
+ free(path);
+ return 1;
+ } else {
+ *wtpath = path;
+ return 0;
+ }
+ }
+ *wtpath = path;
+ return 0;
+}
diff --git a/worktree.h b/worktree.h
index f38e6fd5a2..818e1491c7 100644
--- a/worktree.h
+++ b/worktree.h
@@ -73,6 +73,20 @@ int is_main_worktree(const struct worktree *wt);
*/
const char *worktree_lock_reason(struct worktree *wt);
+/*
+ * Return true if worktree entry should be pruned, along with the reason for
+ * pruning. Otherwise, return false and the worktree's path in `wtpath`, or
+ * NULL if it cannot be determined. Caller is responsible for freeing
+ * returned path.
+ *
+ * `expire` defines a grace period to prune the worktree when its path
+ * does not exist.
+ */
+int should_prune_worktree(const char *id,
+ struct strbuf *reason,
+ char **wtpath,
+ timestamp_t expire);
+
#define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)
/*
--
2.30.0.372.gbc7e965391
^ permalink raw reply related [flat|nested] 98+ messages in thread* [PATCH v2 2/6] worktree: teach worktree to lazy-load "prunable" reason
2021-01-17 23:42 ` [PATCH v2 0/6] " Rafael Silva
2021-01-17 23:42 ` [PATCH v2 1/6] worktree: libify should_prune_worktree() Rafael Silva
@ 2021-01-17 23:42 ` Rafael Silva
2021-01-18 2:57 ` Eric Sunshine
2021-01-17 23:42 ` [PATCH v2 3/6] worktree: teach worktree_lock_reason() to gently handle main worktree Rafael Silva
` (5 subsequent siblings)
7 siblings, 1 reply; 98+ messages in thread
From: Rafael Silva @ 2021-01-17 23:42 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Rafael Silva
Add worktree_prune_reason() to allow a caller to discover whether a
worktree is prunable and the reason that it is, much like
worktree_lock_reason() indicates whether a worktree is locked and the
reason for the lock. As with worktree_lock_reason(), retrieve the
prunable reason lazily and cache it in the `worktree` structure.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
worktree.c | 20 ++++++++++++++++++++
worktree.h | 9 +++++++++
2 files changed, 29 insertions(+)
diff --git a/worktree.c b/worktree.c
index 8ae019af79..474ed46562 100644
--- a/worktree.c
+++ b/worktree.c
@@ -15,6 +15,7 @@ void free_worktrees(struct worktree **worktrees)
free(worktrees[i]->id);
free(worktrees[i]->head_ref);
free(worktrees[i]->lock_reason);
+ free(worktrees[i]->prune_reason);
free(worktrees[i]);
}
free (worktrees);
@@ -245,6 +246,25 @@ const char *worktree_lock_reason(struct worktree *wt)
return wt->lock_reason;
}
+const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire)
+{
+ struct strbuf reason = STRBUF_INIT;
+ char *path;
+
+ if (is_main_worktree(wt))
+ return NULL;
+ if (wt->prune_reason_valid)
+ return wt->prune_reason;
+
+ if (should_prune_worktree(wt->id, &reason, &path, expire))
+ wt->prune_reason = strbuf_detach(&reason, NULL);
+ wt->prune_reason_valid = 1;
+
+ strbuf_release(&reason);
+ free(path);
+ return wt->prune_reason;
+}
+
/* convenient wrapper to deal with NULL strbuf */
static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...)
{
diff --git a/worktree.h b/worktree.h
index 818e1491c7..8b7c408132 100644
--- a/worktree.h
+++ b/worktree.h
@@ -11,11 +11,13 @@ struct worktree {
char *id;
char *head_ref; /* NULL if HEAD is broken or detached */
char *lock_reason; /* private - use worktree_lock_reason */
+ char *prune_reason; /* private - use worktree_prune_reason */
struct object_id head_oid;
int is_detached;
int is_bare;
int is_current;
int lock_reason_valid; /* private */
+ int prune_reason_valid; /* private */
};
/*
@@ -73,6 +75,13 @@ int is_main_worktree(const struct worktree *wt);
*/
const char *worktree_lock_reason(struct worktree *wt);
+/*
+ * Return the reason string if the given worktree should be pruned, otherwise
+ * NULL if it should not be pruned. `expire` defines a grace period to prune
+ * the worktree when its path does not exist.
+ */
+const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire);
+
/*
* Return true if worktree entry should be pruned, along with the reason for
* pruning. Otherwise, return false and the worktree's path in `wtpath`, or
--
2.30.0.372.gbc7e965391
^ permalink raw reply related [flat|nested] 98+ messages in thread* Re: [PATCH v2 2/6] worktree: teach worktree to lazy-load "prunable" reason
2021-01-17 23:42 ` [PATCH v2 2/6] worktree: teach worktree to lazy-load "prunable" reason Rafael Silva
@ 2021-01-18 2:57 ` Eric Sunshine
2021-01-19 7:57 ` Rafael Silva
0 siblings, 1 reply; 98+ messages in thread
From: Eric Sunshine @ 2021-01-18 2:57 UTC (permalink / raw)
To: Rafael Silva; +Cc: Git List, Phillip Wood
On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> Add worktree_prune_reason() to allow a caller to discover whether a
> worktree is prunable and the reason that it is, much like
> worktree_lock_reason() indicates whether a worktree is locked and the
> reason for the lock. As with worktree_lock_reason(), retrieve the
> prunable reason lazily and cache it in the `worktree` structure.
>
> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
> ---
> diff --git a/worktree.c b/worktree.c
> @@ -245,6 +246,25 @@ const char *worktree_lock_reason(struct worktree *wt)
> +const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire)
> +{
> + struct strbuf reason = STRBUF_INIT;
> + char *path;
The `path` variable is uninitialized...
> + if (should_prune_worktree(wt->id, &reason, &path, expire))
> + wt->prune_reason = strbuf_detach(&reason, NULL);
...but the very first thing should_prune_worktree() does is
unconditionally set it to NULL, so it's either NULL or an allocated
string regardless of the value returned by should_prune_worktree()...
> + strbuf_release(&reason);
> + free(path);
...which makes the behavior of `free(path)` deterministic. Good.
I'm on the fence about whether or not we should initialize `path` to NULL:
char *path = NULL;
just to make it easier for the next person to reason about it without
having to dig into the implementation of should_prune_worktree(). This
is a really minor point, though, not worth a re-roll.
^ permalink raw reply [flat|nested] 98+ messages in thread* Re: [PATCH v2 2/6] worktree: teach worktree to lazy-load "prunable" reason
2021-01-18 2:57 ` Eric Sunshine
@ 2021-01-19 7:57 ` Rafael Silva
0 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-19 7:57 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Phillip Wood
Eric Sunshine writes:
> On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> Add worktree_prune_reason() to allow a caller to discover whether a
>> worktree is prunable and the reason that it is, much like
>> worktree_lock_reason() indicates whether a worktree is locked and the
>> reason for the lock. As with worktree_lock_reason(), retrieve the
>> prunable reason lazily and cache it in the `worktree` structure.
>>
>> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
>> ---
>> diff --git a/worktree.c b/worktree.c
>> @@ -245,6 +246,25 @@ const char *worktree_lock_reason(struct worktree *wt)
>> +const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire)
>> +{
>> + struct strbuf reason = STRBUF_INIT;
>> + char *path;
>
> The `path` variable is uninitialized...
>
>> + if (should_prune_worktree(wt->id, &reason, &path, expire))
>> + wt->prune_reason = strbuf_detach(&reason, NULL);
>
> ...but the very first thing should_prune_worktree() does is
> unconditionally set it to NULL, so it's either NULL or an allocated
> string regardless of the value returned by should_prune_worktree()...
>
>> + strbuf_release(&reason);
>> + free(path);
>
> ...which makes the behavior of `free(path)` deterministic. Good.
>
> I'm on the fence about whether or not we should initialize `path` to NULL:
>
> char *path = NULL;
>
> just to make it easier for the next person to reason about it without
> having to dig into the implementation of should_prune_worktree(). This
> is a really minor point, though, not worth a re-roll.
This is a good point and totally agreed. I'm going to include this
change in the next revision.
--
Thanks
Rafael
^ permalink raw reply [flat|nested] 98+ messages in thread
* [PATCH v2 3/6] worktree: teach worktree_lock_reason() to gently handle main worktree
2021-01-17 23:42 ` [PATCH v2 0/6] " Rafael Silva
2021-01-17 23:42 ` [PATCH v2 1/6] worktree: libify should_prune_worktree() Rafael Silva
2021-01-17 23:42 ` [PATCH v2 2/6] worktree: teach worktree to lazy-load "prunable" reason Rafael Silva
@ 2021-01-17 23:42 ` Rafael Silva
2021-01-17 23:42 ` [PATCH v2 4/6] worktree: teach `list --porcelain` to annotate locked worktree Rafael Silva
` (4 subsequent siblings)
7 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-17 23:42 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Rafael Silva
worktree_lock_reason() aborts with an assertion failure when called on
the main worktree since locking the main worktree is nonsensical. Not
only is this behaviour undocumented, thus callers might not even be aware
that the call could potentially crash the program, but it also forces
clients to be extra careful:
if (!is_main_worktree(wt) && worktree_locked_reason(...))
...
Since we know that locking makes no sense in the context of the main
worktree, we can simpliy return false for the main worktree, thus making
client code less complex by eliminating the need for the callers to have
inside knowledge about the implementation:
if (worktree_lock_reason(...))
...
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
worktree.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/worktree.c b/worktree.c
index 474ed46562..39495b261b 100644
--- a/worktree.c
+++ b/worktree.c
@@ -225,7 +225,8 @@ int is_main_worktree(const struct worktree *wt)
const char *worktree_lock_reason(struct worktree *wt)
{
- assert(!is_main_worktree(wt));
+ if (is_main_worktree(wt))
+ return NULL;
if (!wt->lock_reason_valid) {
struct strbuf path = STRBUF_INIT;
--
2.30.0.372.gbc7e965391
^ permalink raw reply related [flat|nested] 98+ messages in thread* [PATCH v2 4/6] worktree: teach `list --porcelain` to annotate locked worktree
2021-01-17 23:42 ` [PATCH v2 0/6] " Rafael Silva
` (2 preceding siblings ...)
2021-01-17 23:42 ` [PATCH v2 3/6] worktree: teach worktree_lock_reason() to gently handle main worktree Rafael Silva
@ 2021-01-17 23:42 ` Rafael Silva
2021-01-18 3:55 ` Eric Sunshine
2021-01-17 23:42 ` [PATCH v2 5/6] worktree: teach `list` to annotate prunable worktree Rafael Silva
` (3 subsequent siblings)
7 siblings, 1 reply; 98+ messages in thread
From: Rafael Silva @ 2021-01-17 23:42 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Rafael Silva
Commit c57b3367be (worktree: teach `list` to annotate locked worktree,
2020-10-11) taught "git worktree list" to annotate locked worktrees by
appending "locked" text to its output, however, this is not listed in
the --porcelain format.
Teach "list --porcelain" to do the same and add a "locked" attribute
followed by its reason, thus making both default and porcelain format
consistent. If the locked reason is not available then only "locked"
is shown.
The output of the "git worktree list --porcelain" becomes like so:
$ git worktree list --porcelain
...
worktree /path/to/locked
HEAD 123abcdea123abcd123acbd123acbda123abcd12
detached
locked
worktree /path/to/locked-with-reason
HEAD abc123abc123abc123abc123abc123abc123abc1
detached
locked reason why it is locked
...
The locked reason is quoted to prevent newline characters (i.e: LF or
CRLF) breaking the --porcelain format as each attribute is shown per
line. For example:
$ git worktree list --porcelain
...
locked worktree's path mounted in\nremovable device
...
Furthermore, let's update the documentation to state that some
attributes in the porcelain format might be listed alone or together
with its value depending whether the value is available or not. Thus
documenting the case of the new "locked" attribute.
Additionally, c57b3367be (worktree: teach `list` to annotate locked
worktree, 2020-10-11) introduced a new test to ensure locked worktrees
are listed with "locked" annotation. However, the test does not clean up
after itself as "git worktree prune" is not going to remove the locked
worktree in the first place. This not only leaves the test in an unclean
state it also potentially breaks following tests that relies on the
"git worktree list" output. Let's fix that by unlocking the worktree
before the "prune" command.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
Documentation/git-worktree.txt | 16 ++++++++++++++--
builtin/worktree.c | 13 +++++++++++++
t/t2402-worktree-list.sh | 32 ++++++++++++++++++++++++++++++++
3 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 02a706c4c0..d352a002f2 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -377,8 +377,10 @@ Porcelain Format
The porcelain format has a line per attribute. Attributes are listed with a
label and value separated by a single space. Boolean attributes (like `bare`
and `detached`) are listed as a label only, and are present only
-if the value is true. The first attribute of a working tree is always
-`worktree`, an empty line indicates the end of the record. For example:
+if the value is true. Some attributes (like `locked`) can be listed as a label
+only or with a value depending whether a reason is available. The first
+attribute of a working tree is always `worktree`, an empty line indicates the
+end of the record. For example:
------------
$ git worktree list --porcelain
@@ -393,6 +395,16 @@ worktree /path/to/other-linked-worktree
HEAD 1234abc1234abc1234abc1234abc1234abc1234a
detached
+worktree /path/to/linked-worktree-locked
+HEAD 5678abc5678abc5678abc5678abc5678abc5678c
+branch refs/heads/locked
+locked
+
+worktree /path/to/linked-worktree-locked-with-reason
+HEAD 3456def3456def3456def3456def3456def3456b
+branch refs/heads/locked-with-reason
+locked reason why is locked
+
------------
EXAMPLES
diff --git a/builtin/worktree.c b/builtin/worktree.c
index dd886d5029..37ae277352 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -12,6 +12,7 @@
#include "submodule.h"
#include "utf8.h"
#include "worktree.h"
+#include "quote.h"
static const char * const worktree_usage[] = {
N_("git worktree add [<options>] <path> [<commit-ish>]"),
@@ -569,6 +570,8 @@ static int add(int ac, const char **av, const char *prefix)
static void show_worktree_porcelain(struct worktree *wt)
{
+ const char *reason;
+
printf("worktree %s\n", wt->path);
if (wt->is_bare)
printf("bare\n");
@@ -579,6 +582,16 @@ static void show_worktree_porcelain(struct worktree *wt)
else if (wt->head_ref)
printf("branch %s\n", wt->head_ref);
}
+
+ reason = worktree_lock_reason(wt);
+ if (reason && *reason) {
+ struct strbuf sb = STRBUF_INIT;
+ quote_c_style(reason, &sb, NULL, CQUOTE_NODQ);
+ printf("locked %s\n", sb.buf);
+ strbuf_release(&sb);
+ } else if (reason)
+ printf("locked\n");
+
printf("\n");
}
diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index 795ddca2e4..c522190feb 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh
@@ -66,11 +66,43 @@ test_expect_success '"list" all worktrees with locked annotation' '
git worktree add --detach locked master &&
git worktree add --detach unlocked master &&
git worktree lock locked &&
+ test_when_finished "git worktree unlock locked" &&
git worktree list >out &&
grep "/locked *[0-9a-f].* locked$" out &&
! grep "/unlocked *[0-9a-f].* locked$" out
'
+test_expect_success '"list" all worktrees --porcelain with locked' '
+ test_when_finished "rm -rf locked1 locked2 unlocked out actual expect && git worktree prune" &&
+ echo "locked" >expect &&
+ echo "locked with reason" >>expect &&
+ git worktree add --detach locked1 &&
+ git worktree add --detach locked2 &&
+ git worktree add --detach unlocked &&
+ git worktree lock locked1 &&
+ git worktree lock locked2 --reason "with reason" &&
+ test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" &&
+ git worktree list --porcelain >out &&
+ grep "^locked" out >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '"list" all worktrees --porcelain with locked reason newline escaped' '
+ test_when_finished "rm -rf locked_lf locked_crlf out actual expect && git worktree prune" &&
+ printf "locked locked\\\\r\\\\nreason\n" >expect &&
+ printf "locked locked\\\\nreason\n" >>expect &&
+ git worktree add --detach locked_lf &&
+ git worktree add --detach locked_crlf &&
+ printf "locked\nreason\n\n" >reason_lf &&
+ printf "locked\r\nreason\n\n" >reason_crlf &&
+ git worktree lock locked_lf --reason "$(cat reason_lf)" &&
+ git worktree lock locked_crlf --reason "$(cat reason_crlf)" &&
+ test_when_finished "git worktree unlock locked_lf && git worktree unlock locked_crlf" &&
+ git worktree list --porcelain >out &&
+ grep "^locked" out >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'bare repo setup' '
git init --bare bare1 &&
echo "data" >file1 &&
--
2.30.0.372.gbc7e965391
^ permalink raw reply related [flat|nested] 98+ messages in thread* Re: [PATCH v2 4/6] worktree: teach `list --porcelain` to annotate locked worktree
2021-01-17 23:42 ` [PATCH v2 4/6] worktree: teach `list --porcelain` to annotate locked worktree Rafael Silva
@ 2021-01-18 3:55 ` Eric Sunshine
2021-01-19 8:20 ` Rafael Silva
0 siblings, 1 reply; 98+ messages in thread
From: Eric Sunshine @ 2021-01-18 3:55 UTC (permalink / raw)
To: Rafael Silva; +Cc: Git List, Phillip Wood
On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> Commit c57b3367be (worktree: teach `list` to annotate locked worktree,
> 2020-10-11) taught "git worktree list" to annotate locked worktrees by
> appending "locked" text to its output, however, this is not listed in
> the --porcelain format.
>
> Teach "list --porcelain" to do the same and add a "locked" attribute
> followed by its reason, thus making both default and porcelain format
> consistent. If the locked reason is not available then only "locked"
> is shown.
>
> The output of the "git worktree list --porcelain" becomes like so:
>
> $ git worktree list --porcelain
> ...
> worktree /path/to/locked
> HEAD 123abcdea123abcd123acbd123acbda123abcd12
> detached
> locked
>
> worktree /path/to/locked-with-reason
> HEAD abc123abc123abc123abc123abc123abc123abc1
> detached
> locked reason why it is locked
> ...
All good.
> The locked reason is quoted to prevent newline characters (i.e: LF or
> CRLF) breaking the --porcelain format as each attribute is shown per
> line. For example:
>
> $ git worktree list --porcelain
> ...
> locked worktree's path mounted in\nremovable device
> ...
I have a bit to say about this below.
> Additionally, c57b3367be (worktree: teach `list` to annotate locked
> worktree, 2020-10-11) introduced a new test to ensure locked worktrees
> are listed with "locked" annotation. However, the test does not clean up
> after itself as "git worktree prune" is not going to remove the locked
> worktree in the first place. This not only leaves the test in an unclean
> state it also potentially breaks following tests that relies on the
> "git worktree list" output. Let's fix that by unlocking the worktree
> before the "prune" command.
The actual code change to fix this bug is about as minimal as it gets,
but the explanation you've written here is lengthy enough and nicely
self-contained that it suggests splitting it off to its own patch. And
since you can re-use this paragraph almost verbatim as the commit
message, it shouldn't require much work to do so. On the other hand,
it is itself not necessarily worth a re-roll, but if you do re-roll,
perhaps it's worth considering.
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -377,8 +377,10 @@ Porcelain Format
> The porcelain format has a line per attribute. Attributes are listed with a
> label and value separated by a single space. Boolean attributes (like `bare`
> and `detached`) are listed as a label only, and are present only
> -if the value is true. The first attribute of a working tree is always
> -`worktree`, an empty line indicates the end of the record. For example:
> +if the value is true. Some attributes (like `locked`) can be listed as a label
> +only or with a value depending whether a reason is available. The first
Perhaps:
s/depending whether/depending upon whether/
> +attribute of a working tree is always `worktree`, an empty line indicates the
> +end of the record. For example:
> @@ -393,6 +395,16 @@ worktree /path/to/other-linked-worktree
> +worktree /path/to/linked-worktree-locked
> +HEAD 5678abc5678abc5678abc5678abc5678abc5678c
> +branch refs/heads/locked
> +locked
> +
> +worktree /path/to/linked-worktree-locked-with-reason
> +HEAD 3456def3456def3456def3456def3456def3456b
> +branch refs/heads/locked-with-reason
> +locked reason why is locked
I was momentarily confused by the branch named `locked` with the
`locked` attribute in the first new stanza. Perhaps take a hint from
the second new stanza and call the first one `locked-no-reason`:
worktree /path/to/linked-worktree-locked-no-reason
HEAD 5678abc5678abc5678abc5678abc5678abc5678c
branch refs/heads/locked-no-reason
locked
Again, though, not worth a re-roll.
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -579,6 +582,16 @@ static void show_worktree_porcelain(struct worktree *wt)
> + reason = worktree_lock_reason(wt);
> + if (reason && *reason) {
> + struct strbuf sb = STRBUF_INIT;
> + quote_c_style(reason, &sb, NULL, CQUOTE_NODQ);
> + printf("locked %s\n", sb.buf);
> + strbuf_release(&sb);
> + } else if (reason)
> + printf("locked\n");
This needs a change, and it's totally my fault that it does. In my
previous review, I mentioned that if the lock reason contains special
characters, we want those special characters escaped and the reason
quoted, but _only_ if it contains special characters. However, I then
incorrectly said to call quote_c_style() with CQUOTE_NODQ to achieve
that behavior. In fact, CQUOTE_NODQ gives us the wrong behavior since
it avoids quoting the string which, as Phillip pointed out, makes it
impossible to distinguish between a string which just happens to
contain the two-character sequence '\' and 'n', and an escaped newline
"\n". So, the above should really be:
quote_c_style(reason, &sb, NULL, 0);
The example in the commit message should be adjusted to account for
this change, as well:
In porcelain mode, if the lock reason contains special characters
such as newlines, they are escaped with backslashes and the entire
reason is enclosed in double quotes. For example:
$ git worktree list --porcelain
...
locked "worktree's path mounted in\nremovable device"
...
And, of course, the new test will need a slight adjustment.
> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
> @@ -66,11 +66,43 @@ test_expect_success '"list" all worktrees with locked annotation' '
> +test_expect_success '"list" all worktrees --porcelain with locked' '
> + test_when_finished "rm -rf locked1 locked2 unlocked out actual expect && git worktree prune" &&
> + echo "locked" >expect &&
> + echo "locked with reason" >>expect &&
> + git worktree add --detach locked1 &&
> + git worktree add --detach locked2 &&
> + git worktree add --detach unlocked &&
> + git worktree lock locked1 &&
> + git worktree lock locked2 --reason "with reason" &&
> + test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" &&
> + git worktree list --porcelain >out &&
> + grep "^locked" out >actual &&
> + test_cmp expect actual
> +'
So, the purpose of the `unlocked` worktree in this test is to prove
that it didn't accidentally get annotated with `locked`? (Since, if it
did get annotated, then `actual` would contain too many lines and not
match `expect`.) Is that correct?
> +test_expect_success '"list" all worktrees --porcelain with locked reason newline escaped' '
> + test_when_finished "rm -rf locked_lf locked_crlf out actual expect && git worktree prune" &&
> + printf "locked locked\\\\r\\\\nreason\n" >expect &&
> + printf "locked locked\\\\nreason\n" >>expect &&
> + git worktree add --detach locked_lf &&
> + git worktree add --detach locked_crlf &&
> + printf "locked\nreason\n\n" >reason_lf &&
> + printf "locked\r\nreason\n\n" >reason_crlf &&
The trailing "\n\n" is unneeded. Due to the way `$(...)` expansion
works (dropping trailing whitespace), you'll get the same successful
test result with:
printf "locked\nreason\n" >reason_lf &&
printf "locked\r\nreason\n" >reason_crlf &&
and even with:
printf "locked\nreason" >reason_lf &&
printf "locked\r\nreason" >reason_crlf &&
> + git worktree lock locked_lf --reason "$(cat reason_lf)" &&
> + git worktree lock locked_crlf --reason "$(cat reason_crlf)" &&
You could also just embed the `printf`'s here rather than using these
temporary files.
git worktree lock --reason $(printf "...") <path> &&
Or, if we care only about testing LF, and not about CRLF, even this would work:
git worktree lock --reason "reason with
newline" <path> &&
but that gets a bit ugly.
Anyhow, all the line terminator commentary about this test is a matter
of personal taste, probably not worth a re-roll or even changing.
^ permalink raw reply [flat|nested] 98+ messages in thread* Re: [PATCH v2 4/6] worktree: teach `list --porcelain` to annotate locked worktree
2021-01-18 3:55 ` Eric Sunshine
@ 2021-01-19 8:20 ` Rafael Silva
2021-01-19 17:16 ` Eric Sunshine
0 siblings, 1 reply; 98+ messages in thread
From: Rafael Silva @ 2021-01-19 8:20 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Phillip Wood
Thanks for the review.
Eric Sunshine writes:
> On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> Additionally, c57b3367be (worktree: teach `list` to annotate locked
>> worktree, 2020-10-11) introduced a new test to ensure locked worktrees
>> are listed with "locked" annotation. However, the test does not clean up
>> after itself as "git worktree prune" is not going to remove the locked
>> worktree in the first place. This not only leaves the test in an unclean
>> state it also potentially breaks following tests that relies on the
>> "git worktree list" output. Let's fix that by unlocking the worktree
>> before the "prune" command.
>
> The actual code change to fix this bug is about as minimal as it gets,
> but the explanation you've written here is lengthy enough and nicely
> self-contained that it suggests splitting it off to its own patch. And
> since you can re-use this paragraph almost verbatim as the commit
> message, it shouldn't require much work to do so. On the other hand,
> it is itself not necessarily worth a re-roll, but if you do re-roll,
> perhaps it's worth considering.
>
make sense, I'll re-roll with this change on its own patch. I actually
thought about splitting it off during as well, but I wasn't sure whether
this was a good idea. Now that you mentioned here I guess it sounds a
like reasonable change for the next version.
>> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
>> @@ -377,8 +377,10 @@ Porcelain Format
>> The porcelain format has a line per attribute. Attributes are listed with a
>> label and value separated by a single space. Boolean attributes (like `bare`
>> and `detached`) are listed as a label only, and are present only
>> -if the value is true. The first attribute of a working tree is always
>> -`worktree`, an empty line indicates the end of the record. For example:
>> +if the value is true. Some attributes (like `locked`) can be listed as a label
>> +only or with a value depending whether a reason is available. The first
>
> Perhaps:
> s/depending whether/depending upon whether/
>
Yeah, I think that's sounds bit better.
>> +attribute of a working tree is always `worktree`, an empty line indicates the
>> +end of the record. For example:
>> @@ -393,6 +395,16 @@ worktree /path/to/other-linked-worktree
>> +worktree /path/to/linked-worktree-locked
>> +HEAD 5678abc5678abc5678abc5678abc5678abc5678c
>> +branch refs/heads/locked
>> +locked
>> +
>> +worktree /path/to/linked-worktree-locked-with-reason
>> +HEAD 3456def3456def3456def3456def3456def3456b
>> +branch refs/heads/locked-with-reason
>> +locked reason why is locked
>
> I was momentarily confused by the branch named `locked` with the
> `locked` attribute in the first new stanza. Perhaps take a hint from
> the second new stanza and call the first one `locked-no-reason`:
>
> worktree /path/to/linked-worktree-locked-no-reason
> HEAD 5678abc5678abc5678abc5678abc5678abc5678c
> branch refs/heads/locked-no-reason
> locked
>
> Again, though, not worth a re-roll.
>
This seems like a nice touch. will include this in the next revision.
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -579,6 +582,16 @@ static void show_worktree_porcelain(struct worktree *wt)
>> + reason = worktree_lock_reason(wt);
>> + if (reason && *reason) {
>> + struct strbuf sb = STRBUF_INIT;
>> + quote_c_style(reason, &sb, NULL, CQUOTE_NODQ);
>> + printf("locked %s\n", sb.buf);
>> + strbuf_release(&sb);
>> + } else if (reason)
>> + printf("locked\n");
>
> This needs a change, and it's totally my fault that it does. In my
> previous review, I mentioned that if the lock reason contains special
> characters, we want those special characters escaped and the reason
> quoted, but _only_ if it contains special characters. However, I then
> incorrectly said to call quote_c_style() with CQUOTE_NODQ to achieve
> that behavior. In fact, CQUOTE_NODQ gives us the wrong behavior since
> it avoids quoting the string which, as Phillip pointed out, makes it
> impossible to distinguish between a string which just happens to
> contain the two-character sequence '\' and 'n', and an escaped newline
> "\n". So, the above should really be:
>
> quote_c_style(reason, &sb, NULL, 0);
>
> The example in the commit message should be adjusted to account for
> this change, as well:
>
> In porcelain mode, if the lock reason contains special characters
> such as newlines, they are escaped with backslashes and the entire
> reason is enclosed in double quotes. For example:
>
> $ git worktree list --porcelain
> ...
> locked "worktree's path mounted in\nremovable device"
> ...
>
> And, of course, the new test will need a slight adjustment.
>
Alright, I believe I've got the whole picture now and sorry for the
confusion. You and Phillip clearly stated in the review cycle that the
reason should be quoted because of the aforementioned reasons and I
dropped when I was working on this version.
I will re-roll and change this in the next revision.
>> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
>> @@ -66,11 +66,43 @@ test_expect_success '"list" all worktrees with locked annotation' '
>> +test_expect_success '"list" all worktrees --porcelain with locked' '
>> + test_when_finished "rm -rf locked1 locked2 unlocked out actual expect && git worktree prune" &&
>> + echo "locked" >expect &&
>> + echo "locked with reason" >>expect &&
>> + git worktree add --detach locked1 &&
>> + git worktree add --detach locked2 &&
>> + git worktree add --detach unlocked &&
>> + git worktree lock locked1 &&
>> + git worktree lock locked2 --reason "with reason" &&
>> + test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" &&
>> + git worktree list --porcelain >out &&
>> + grep "^locked" out >actual &&
>> + test_cmp expect actual
>> +'
>
> So, the purpose of the `unlocked` worktree in this test is to prove
> that it didn't accidentally get annotated with `locked`? (Since, if it
> did get annotated, then `actual` would contain too many lines and not
> match `expect`.) Is that correct?
>
Yes, this is what I intended to check when adding the `unlocked`
worktree. I'm considering how to make this more explicit so it's clear
for readers why the `unlocked` worktree exists in this test.
>> +test_expect_success '"list" all worktrees --porcelain with locked reason newline escaped' '
>> + test_when_finished "rm -rf locked_lf locked_crlf out actual expect && git worktree prune" &&
>> + printf "locked locked\\\\r\\\\nreason\n" >expect &&
>> + printf "locked locked\\\\nreason\n" >>expect &&
>> + git worktree add --detach locked_lf &&
>> + git worktree add --detach locked_crlf &&
>> + printf "locked\nreason\n\n" >reason_lf &&
>> + printf "locked\r\nreason\n\n" >reason_crlf &&
>
> The trailing "\n\n" is unneeded. Due to the way `$(...)` expansion
> works (dropping trailing whitespace), you'll get the same successful
> test result with:
>
> printf "locked\nreason\n" >reason_lf &&
> printf "locked\r\nreason\n" >reason_crlf &&
>
> and even with:
>
> printf "locked\nreason" >reason_lf &&
> printf "locked\r\nreason" >reason_crlf &&
>
>> + git worktree lock locked_lf --reason "$(cat reason_lf)" &&
>> + git worktree lock locked_crlf --reason "$(cat reason_crlf)" &&
>
> You could also just embed the `printf`'s here rather than using these
> temporary files.
>
> git worktree lock --reason $(printf "...") <path> &&
>
Having the `printf` together with the $(...) expansion seems like the
good simplification for this test. will include in the next revision.
> Or, if we care only about testing LF, and not about CRLF, even this would work:
>
> git worktree lock --reason "reason with
> newline" <path> &&
>
> but that gets a bit ugly.
>
> Anyhow, all the line terminator commentary about this test is a matter
> of personal taste, probably not worth a re-roll or even changing.
--
Thanks
Rafael
^ permalink raw reply [flat|nested] 98+ messages in thread* Re: [PATCH v2 4/6] worktree: teach `list --porcelain` to annotate locked worktree
2021-01-19 8:20 ` Rafael Silva
@ 2021-01-19 17:16 ` Eric Sunshine
0 siblings, 0 replies; 98+ messages in thread
From: Eric Sunshine @ 2021-01-19 17:16 UTC (permalink / raw)
To: Rafael Silva; +Cc: Git List, Phillip Wood
On Tue, Jan 19, 2021 at 3:20 AM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> Eric Sunshine writes:
> > On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
> > <rafaeloliveira.cs@gmail.com> wrote:
> >> + quote_c_style(reason, &sb, NULL, CQUOTE_NODQ);
> >
> > This needs a change, and it's totally my fault that it does. In my
> > previous review, I mentioned that if the lock reason contains special
> > characters, we want those special characters escaped and the reason
> > quoted, but _only_ if it contains special characters. However, I then
> > incorrectly said to call quote_c_style() with CQUOTE_NODQ to achieve
> > that behavior. In fact, CQUOTE_NODQ gives us the wrong behavior since
> > it avoids quoting the string which, as Phillip pointed out, makes it
> > impossible to distinguish between a string which just happens to
> > contain the two-character sequence '\' and 'n', and an escaped newline
> > "\n". So, the above should really be:
> >
> Alright, I believe I've got the whole picture now and sorry for the
> confusion. You and Phillip clearly stated in the review cycle that the
> reason should be quoted because of the aforementioned reasons and I
> dropped when I was working on this version.
It was my fault by confusing you with the misleading mention of CQUOTE_NODQ.
> >> + git worktree add --detach locked1 &&
> >> + git worktree add --detach locked2 &&
> >> + git worktree add --detach unlocked &&
> >
> > So, the purpose of the `unlocked` worktree in this test is to prove
> > that it didn't accidentally get annotated with `locked`? (Since, if it
> > did get annotated, then `actual` would contain too many lines and not
> > match `expect`.) Is that correct?
>
> Yes, this is what I intended to check when adding the `unlocked`
> worktree. I'm considering how to make this more explicit so it's clear
> for readers why the `unlocked` worktree exists in this test.
A simple in-code comment should suffice, I would think:
git worktree add --detach locked1 &&
git worktree add --detach locked2 &&
# unlocked worktree should not be annotated "locked"
git worktree add --detach unlocked &&
^ permalink raw reply [flat|nested] 98+ messages in thread
* [PATCH v2 5/6] worktree: teach `list` to annotate prunable worktree
2021-01-17 23:42 ` [PATCH v2 0/6] " Rafael Silva
` (3 preceding siblings ...)
2021-01-17 23:42 ` [PATCH v2 4/6] worktree: teach `list --porcelain` to annotate locked worktree Rafael Silva
@ 2021-01-17 23:42 ` Rafael Silva
2021-01-18 4:45 ` Eric Sunshine
2021-01-17 23:42 ` [PATCH v2 6/6] worktree: teach `list` verbose mode Rafael Silva
` (2 subsequent siblings)
7 siblings, 1 reply; 98+ messages in thread
From: Rafael Silva @ 2021-01-17 23:42 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Rafael Silva
The "git worktree list" command shows the absolute path to the worktree,
the commit that is checked out, the name of the branch, and a "locked"
annotation if the worktree is locked, however, it does not indicate
whether the worktree is prunable.
The "prune" command will remove a worktree if it is prunable unless
`--dry-run` option is specified. This could lead to a worktree being
removed without the user realizing before it is too late, in case the
user forgets to pass --dry-run for instance. If the "list" command shows
which worktree is prunable, the user could verify before running
"git worktree prune" and hopefully prevents the working tree to be
removed "accidentally" on the worse case scenario.
Let's teach "git worktree list" to show when a worktree is a prunable
candidate for both default and porcelain format.
In the default format a "prunable" text is appended:
$ git worktree list
/path/to/main aba123 [main]
/path/to/linked 123abc [branch-a]
/path/to/prunable ace127 (detached HEAD) prunable
In the --porcelain format a prunable label is added followed by
its reason:
$ git worktree list --porcelain
...
worktree /path/to/prunable
HEAD abc1234abc1234abc1234abc1234abc1234abc12
detached
prunable gitdir file points to non-existent location
...
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
Documentation/git-worktree.txt | 26 ++++++++++++++++++++++++--
builtin/worktree.c | 15 ++++++++++++++-
t/t2402-worktree-list.sh | 32 ++++++++++++++++++++++++++++++++
3 files changed, 70 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index d352a002f2..3d8c14dbdf 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -97,8 +97,9 @@ list::
List details of each working tree. The main working tree is listed first,
followed by each of the linked working trees. The output details include
whether the working tree is bare, the revision currently checked out, the
-branch currently checked out (or "detached HEAD" if none), and "locked" if
-the worktree is locked.
+branch currently checked out (or "detached HEAD" if none), "locked" if
+the worktree is locked, "prunable" if the worktree can be pruned by `prune`
+command.
lock::
@@ -234,6 +235,9 @@ This can also be set up as the default behaviour by using the
--expire <time>::
With `prune`, only expire unused working trees older than `<time>`.
++
+With `list`, annotate missing working trees as prunable if they are
+older than `<mtime>`.
--reason <string>::
With `lock`, an explanation why the working tree is locked.
@@ -372,6 +376,19 @@ $ git worktree list
/path/to/other-linked-worktree 1234abc (detached HEAD)
------------
+The command also shows annotations for each working tree, according to its state.
+These annotations are:
+
+ * `locked`, if the working tree is locked.
+ * `prunable`, if the working tree can be pruned via `git worktree prune`.
+
+------------
+$ git worktree list
+/path/to/linked-worktree abcd1234 [master]
+/path/to/locked-worktreee acbd5678 (brancha) locked
+/path/to/prunable-worktree 5678abc (detached HEAD) prunable
+------------
+
Porcelain Format
~~~~~~~~~~~~~~~~
The porcelain format has a line per attribute. Attributes are listed with a
@@ -405,6 +422,11 @@ HEAD 3456def3456def3456def3456def3456def3456b
branch refs/heads/locked-with-reason
locked reason why is locked
+worktree /path/to/linked-worktree-prunable
+HEAD 1233def1234def1234def1234def1234def1234b
+detached
+prunable gitdir file points to non-existent location
+
------------
EXAMPLES
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 37ae277352..fb82d7bb64 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -592,6 +592,10 @@ static void show_worktree_porcelain(struct worktree *wt)
} else if (reason)
printf("locked\n");
+ reason = worktree_prune_reason(wt, expire);
+ if (reason && *reason)
+ printf("prunable %s\n", reason);
+
printf("\n");
}
@@ -600,6 +604,7 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
struct strbuf sb = STRBUF_INIT;
int cur_path_len = strlen(wt->path);
int path_adj = cur_path_len - utf8_strwidth(wt->path);
+ const char *reason;
strbuf_addf(&sb, "%-*s ", 1 + path_maxlen + path_adj, wt->path);
if (wt->is_bare)
@@ -617,9 +622,14 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
strbuf_addstr(&sb, "(error)");
}
- if (!is_main_worktree(wt) && worktree_lock_reason(wt))
+ reason = worktree_lock_reason(wt);
+ if (reason)
strbuf_addstr(&sb, " locked");
+ reason = worktree_prune_reason(wt, expire);
+ if (reason)
+ strbuf_addstr(&sb, " prunable");
+
printf("%s\n", sb.buf);
strbuf_release(&sb);
}
@@ -663,9 +673,12 @@ static int list(int ac, const char **av, const char *prefix)
struct option options[] = {
OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
+ OPT_EXPIRY_DATE(0, "expire", &expire,
+ N_("add 'prunable' annotation to worktrees older than <time>")),
OPT_END()
};
+ expire = TIME_MAX;
ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
if (ac)
usage_with_options(worktree_usage, options);
diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index c522190feb..e9b410b69e 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh
@@ -103,6 +103,38 @@ test_expect_success '"list" all worktrees --porcelain with locked reason newline
test_cmp expect actual
'
+test_expect_success '"list" all worktrees with prunable annotation' '
+ test_when_finished "rm -rf prunable unprunable out && git worktree prune" &&
+ git worktree add --detach prunable &&
+ git worktree add --detach unprunable &&
+ rm -rf prunable &&
+ git worktree list >out &&
+ grep "/prunable *[0-9a-f].* prunable$" out &&
+ ! grep "/unprunable *[0-9a-f].* prunable$"
+'
+
+test_expect_success '"list" all worktrees --porcelain with prunable' '
+ test_when_finished "rm -rf prunable out && git worktree prune" &&
+ git worktree add --detach prunable &&
+ rm -rf prunable &&
+ git worktree list --porcelain >out &&
+ sed -n "/^worktree .*\/prunable$/,/^$/p" <out >only_prunable &&
+ test_i18ngrep "^prunable gitdir file points to non-existent location$" only_prunable
+'
+
+test_expect_success '"list" all worktrees with prunable consistent with "prune"' '
+ test_when_finished "rm -rf prunable unprunable out && git worktree prune" &&
+ git worktree add --detach prunable &&
+ git worktree add --detach unprunable &&
+ rm -rf prunable &&
+ git worktree list >out &&
+ grep "/prunable *[0-9a-f].* prunable$" out &&
+ ! grep "/unprunable *[0-9a-f].* unprunable$" out &&
+ git worktree prune --verbose >out &&
+ test_i18ngrep "^Removing worktrees/prunable" out &&
+ test_i18ngrep ! "^Removing worktrees/unprunable" out
+'
+
test_expect_success 'bare repo setup' '
git init --bare bare1 &&
echo "data" >file1 &&
--
2.30.0.372.gbc7e965391
^ permalink raw reply related [flat|nested] 98+ messages in thread* Re: [PATCH v2 5/6] worktree: teach `list` to annotate prunable worktree
2021-01-17 23:42 ` [PATCH v2 5/6] worktree: teach `list` to annotate prunable worktree Rafael Silva
@ 2021-01-18 4:45 ` Eric Sunshine
2021-01-19 10:26 ` Rafael Silva
0 siblings, 1 reply; 98+ messages in thread
From: Eric Sunshine @ 2021-01-18 4:45 UTC (permalink / raw)
To: Rafael Silva; +Cc: Git List, Phillip Wood
On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> The "git worktree list" command shows the absolute path to the worktree,
> the commit that is checked out, the name of the branch, and a "locked"
> annotation if the worktree is locked, however, it does not indicate
> whether the worktree is prunable.
> [...]
> Let's teach "git worktree list" to show when a worktree is a prunable
> candidate for both default and porcelain format.
Good.
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -234,6 +235,9 @@ This can also be set up as the default behaviour by using the
> --expire <time>::
> With `prune`, only expire unused working trees older than `<time>`.
> ++
> +With `list`, annotate missing working trees as prunable if they are
> +older than `<mtime>`.
s/mtime/time/
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -592,6 +592,10 @@ static void show_worktree_porcelain(struct worktree *wt)
> + reason = worktree_prune_reason(wt, expire);
> + if (reason && *reason)
> + printf("prunable %s\n", reason);
I lean toward not including `*reason` in the condition here. While one
may argue that `*reason` is future-proofing the condition, we know
today that worktree_prune_reason() will only ever return NULL or a
non-empty string. So, having `*reason` in the condition is misleading
and confuses readers into thinking that worktree_prune_reason() could
return an empty string or that perhaps it did in the past. And
studying the history of this file or even this commit won't help them
understand why the author included `*reason` in the condition.
> @@ -617,9 +622,14 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
> - if (!is_main_worktree(wt) && worktree_lock_reason(wt))
> + reason = worktree_lock_reason(wt);
> + if (reason)
> strbuf_addstr(&sb, " locked");
Although I understand what happened here because I read the entire
series, for anyone reading this patch in the future or even someone
reading this patch in isolation, the disappearance of
is_main_worktree() from the condition is mysterious. They won't know
if its removal was an accident or intentional, or if the change
introduces a bug. Therefore, it would be better to drop
is_main_worktree() from this conditional in patch [3/6], which is the
patch which makes it safe to call worktree_lock_reason() even for the
main worktree. Thus, in [3/6], this would change from:
if (!is_main_worktree(wt) && worktree_lock_reason(wt))
to:
if (worktree_lock_reason(wt))
and then in this patch [5/6], it becomes:
reason = worktree_lock_reason(wt);
if (reason)
> + reason = worktree_prune_reason(wt, expire);
> + if (reason)
> + strbuf_addstr(&sb, " prunable");
Looking at this also makes me wonder if patches [5/6] and [6/6] should
be swapped since it's not clear to the reader why you're adding the
`reason` variable in this patch when the same could be accomplished
more simply:
if (worktree_lock_reason(wt))
strbuf_addstr(&sb, " locked");
if (worktree_prune_reason(wt, expire))
strbuf_addstr(&sb, " prunable");
If you swap the patches and add --verbose mode first which requires
this new `reason` variable, then the mystery goes away, and the use of
`reason` is obvious when `prunable` annotation is added in the
subsequent patch.
Having said that, I'm not trying to make extra work for you by
expecting patch perfection. Sometimes it's fine to craft a patch in
such a way that it makes subsequent patches simpler, even if it looks
slightly odd in the current patch, and I haven't read [6/6] yet, so
whatever opinion I express here is based only upon what I've read up
to this point.
^ permalink raw reply [flat|nested] 98+ messages in thread* Re: [PATCH v2 5/6] worktree: teach `list` to annotate prunable worktree
2021-01-18 4:45 ` Eric Sunshine
@ 2021-01-19 10:26 ` Rafael Silva
2021-01-19 17:23 ` Eric Sunshine
0 siblings, 1 reply; 98+ messages in thread
From: Rafael Silva @ 2021-01-19 10:26 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Phillip Wood
Eric Sunshine writes:
> On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
>> @@ -234,6 +235,9 @@ This can also be set up as the default behaviour by using the
>> --expire <time>::
>> With `prune`, only expire unused working trees older than `<time>`.
>> ++
>> +With `list`, annotate missing working trees as prunable if they are
>> +older than `<mtime>`.
>
> s/mtime/time/
>
Oops. thanks for catching that. Will fix it in the next revision.
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -592,6 +592,10 @@ static void show_worktree_porcelain(struct worktree *wt)
>> + reason = worktree_prune_reason(wt, expire);
>> + if (reason && *reason)
>> + printf("prunable %s\n", reason);
>
> I lean toward not including `*reason` in the condition here. While one
> may argue that `*reason` is future-proofing the condition, we know
> today that worktree_prune_reason() will only ever return NULL or a
> non-empty string. So, having `*reason` in the condition is misleading
> and confuses readers into thinking that worktree_prune_reason() could
> return an empty string or that perhaps it did in the past. And
> studying the history of this file or even this commit won't help them
> understand why the author included `*reason` in the condition.
>
Fair enough. The `*reason` condition was indeed added to be safe and
future-proofing and, as you pointed it out, we know that currently
the worktree_prune_reason() returns a non-empty string when its true
and when the code reaches the `*reason` condition in
"if (reason && *reason)" this will always evaluates to true.
Agreed that removing this part of the condition will make the code
clearer and easier to followed. So I will drop this in the next
revision.
>> @@ -617,9 +622,14 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
>> - if (!is_main_worktree(wt) && worktree_lock_reason(wt))
>> + reason = worktree_lock_reason(wt);
>> + if (reason)
>> strbuf_addstr(&sb, " locked");
>
> Although I understand what happened here because I read the entire
> series, for anyone reading this patch in the future or even someone
> reading this patch in isolation, the disappearance of
> is_main_worktree() from the condition is mysterious. They won't know
> if its removal was an accident or intentional, or if the change
> introduces a bug. Therefore, it would be better to drop
> is_main_worktree() from this conditional in patch [3/6], which is the
> patch which makes it safe to call worktree_lock_reason() even for the
> main worktree. Thus, in [3/6], this would change from:
>
> if (!is_main_worktree(wt) && worktree_lock_reason(wt))
>
> to:
>
> if (worktree_lock_reason(wt))
>
> and then in this patch [5/6], it becomes:
>
> reason = worktree_lock_reason(wt);
> if (reason)
>
That's a good point, and even worse this is not mentioned in my commit
message at all. It make sense to move this change into [3/6] where the
API is changed and all the reason is explained in the commit message.
>> + reason = worktree_prune_reason(wt, expire);
>> + if (reason)
>> + strbuf_addstr(&sb, " prunable");
>
> Looking at this also makes me wonder if patches [5/6] and [6/6] should
> be swapped since it's not clear to the reader why you're adding the
> `reason` variable in this patch when the same could be accomplished
> more simply:
>
> if (worktree_lock_reason(wt))
> strbuf_addstr(&sb, " locked");
> if (worktree_prune_reason(wt, expire))
> strbuf_addstr(&sb, " prunable");
>
> If you swap the patches and add --verbose mode first which requires
> this new `reason` variable, then the mystery goes away, and the use of
> `reason` is obvious when `prunable` annotation is added in the
> subsequent patch.
>
> Having said that, I'm not trying to make extra work for you by
> expecting patch perfection. Sometimes it's fine to craft a patch in
> such a way that it makes subsequent patches simpler, even if it looks
> slightly odd in the current patch, and I haven't read [6/6] yet, so
> whatever opinion I express here is based only upon what I've read up
> to this point.
That's a good point. I'm inclined to leave the [5/6] with the following:
if (worktree_prune_reason(wt, expire))
strbuf_addstr(&sb, " prunable");
And move up the changes that includes the `reason` into the [5/6]
patches that introduces the --verbose option. This line seems easier to
follow when the reader is looking on this patch alone and only care
about a reason when the --verbose comes into play in the next patch
[6/6].
Although your suggestion about changing the patch also sounds reasonable
and I'll take into consideration when I re-roll this series.
Btw, I don't mind spending extra work on and I'm all forward
with the changes so we it would be easier to understand not only now where
all the patches are being reviewed together but in the future when
someone is looking at the history of the project, specially for
debugging/bisecting reasons.
Thanks for the insightful comments.
--
Thanks
Rafael
^ permalink raw reply [flat|nested] 98+ messages in thread* Re: [PATCH v2 5/6] worktree: teach `list` to annotate prunable worktree
2021-01-19 10:26 ` Rafael Silva
@ 2021-01-19 17:23 ` Eric Sunshine
0 siblings, 0 replies; 98+ messages in thread
From: Eric Sunshine @ 2021-01-19 17:23 UTC (permalink / raw)
To: Rafael Silva; +Cc: Git List, Phillip Wood
On Tue, Jan 19, 2021 at 5:26 AM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> Eric Sunshine writes:
> > On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
> > <rafaeloliveira.cs@gmail.com> wrote:
> >> + reason = worktree_prune_reason(wt, expire);
> >> + if (reason)
> >> + strbuf_addstr(&sb, " prunable");
> >
> > Looking at this also makes me wonder if patches [5/6] and [6/6] should
> > be swapped since it's not clear to the reader why you're adding the
> > `reason` variable in this patch when the same could be accomplished
> > more simply:
> >
> > if (worktree_prune_reason(wt, expire))
> > strbuf_addstr(&sb, " prunable");
> >
> > If you swap the patches and add --verbose mode first which requires
> > this new `reason` variable, then the mystery goes away, and the use of
> > `reason` is obvious when `prunable` annotation is added in the
> > subsequent patch.
>
> That's a good point. I'm inclined to leave the [5/6] with the following:
>
> if (worktree_prune_reason(wt, expire))
> strbuf_addstr(&sb, " prunable");
>
> And move up the changes that includes the `reason` into the [5/6]
> patches that introduces the --verbose option. This line seems easier to
> follow when the reader is looking on this patch alone and only care
> about a reason when the --verbose comes into play in the next patch
> [6/6].
I considered this, as well, but figured that it would make patch [6/6]
even noiser, and that swapping the patches would keep the noise level
down. But, I haven't actually tried it myself, so I could be wrong
about the assumption, and maybe the noisiness wouldn't be a problem in
practice or would be so stylized that it wouldn't matter.
^ permalink raw reply [flat|nested] 98+ messages in thread
* [PATCH v2 6/6] worktree: teach `list` verbose mode
2021-01-17 23:42 ` [PATCH v2 0/6] " Rafael Silva
` (4 preceding siblings ...)
2021-01-17 23:42 ` [PATCH v2 5/6] worktree: teach `list` to annotate prunable worktree Rafael Silva
@ 2021-01-17 23:42 ` Rafael Silva
2021-01-18 5:15 ` Eric Sunshine
2021-01-18 5:33 ` [PATCH v2 0/6] teach `worktree list` verbose mode and prunable annotations Eric Sunshine
2021-01-19 21:27 ` [PATCH v3 0/7] " Rafael Silva
7 siblings, 1 reply; 98+ messages in thread
From: Rafael Silva @ 2021-01-17 23:42 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Rafael Silva
"git worktree list" annotates each worktree according to its state such
as "prunable" or "locked", however it is not immediately obvious why
these worktrees are being annotated. For prunable worktrees a reason
is available that is returned by should_prune_worktree() and for locked
worktrees a reason might be available provided by the user via `lock`
command.
Let's teach "git worktree list" to output the reason why the worktrees
are being annotated. The reason is a text that can take virtually any
size and appending the text on the default columned format will make it
difficult to extend the command with other annotations and not fit nicely
on the screen. In order to address this shortcoming the annotation is
then moved to the next line indented followed by the reason, if the
reason is not available the annotation stays on the same line as the
worktree itself.
The output of "git worktree list" with verbose becomes like so:
$ git worktree list --verbose
...
/path/to/locked acb124 [branch-a] locked
/path/to/locked-with-reason acc125 [branch-b]
locked: worktree with a locked reason
/path/to/prunable-reason ace127 [branch-d]
prunable: gitdir file points to non-existent location
...
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
Documentation/git-worktree.txt | 20 ++++++++++++++++++++
builtin/worktree.c | 11 +++++++++--
t/t2402-worktree-list.sh | 27 +++++++++++++++++++++++++++
3 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 3d8c14dbdf..d34bf121f3 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -232,6 +232,8 @@ This can also be set up as the default behaviour by using the
-v::
--verbose::
With `prune`, report all removals.
++
+With `list`, output additional information about worktrees (see below).
--expire <time>::
With `prune`, only expire unused working trees older than `<time>`.
@@ -389,6 +391,24 @@ $ git worktree list
/path/to/prunable-worktree 5678abc (detached HEAD) prunable
------------
+For these annotations, a reason might also be available and this can be
+seen using the verbose mode. The annotation is then moved to the next line
+indented followed by the additional information.
+
+------------
+$ git worktree list --verbose
+/path/to/linked-worktree abcd1234 [master]
+/path/to/locked-worktreee acbd5678 (brancha)
+ locked: working tree path is mounted on a removable device
+/path/to/locked-no-reason abcd578 (detached HEAD) locked
+/path/to/prunable-worktree 5678abc (detached HEAD)
+ prunable: gitdir file points to non-existent location
+------------
+
+Note that the annotation is moved to the next line if the additional
+information is available, otherwise it stays on the same line as the
+working tree itself.
+
Porcelain Format
~~~~~~~~~~~~~~~~
The porcelain format has a line per attribute. Attributes are listed with a
diff --git a/builtin/worktree.c b/builtin/worktree.c
index fb82d7bb64..a59cbfa0d2 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -623,11 +623,15 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
}
reason = worktree_lock_reason(wt);
- if (reason)
+ if (verbose && reason && *reason)
+ strbuf_addf(&sb, "\n\tlocked: %s", reason);
+ else if (reason)
strbuf_addstr(&sb, " locked");
reason = worktree_prune_reason(wt, expire);
- if (reason)
+ if (verbose && reason && *reason)
+ strbuf_addf(&sb, "\n\tprunable: %s", reason);
+ else if (reason)
strbuf_addstr(&sb, " prunable");
printf("%s\n", sb.buf);
@@ -673,6 +677,7 @@ static int list(int ac, const char **av, const char *prefix)
struct option options[] = {
OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
+ OPT__VERBOSE(&verbose, N_("show extended annotations and reasons, if available")),
OPT_EXPIRY_DATE(0, "expire", &expire,
N_("add 'prunable' annotation to worktrees older than <time>")),
OPT_END()
@@ -682,6 +687,8 @@ static int list(int ac, const char **av, const char *prefix)
ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
if (ac)
usage_with_options(worktree_usage, options);
+ else if (verbose && porcelain)
+ die(_("--verbose and --porcelain are mutually exclusive"));
else {
struct worktree **worktrees = get_worktrees();
int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index e9b410b69e..4de37f896c 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh
@@ -135,6 +135,33 @@ test_expect_success '"list" all worktrees with prunable consistent with "prune"'
test_i18ngrep ! "^Removing worktrees/unprunable" out
'
+test_expect_success '"list" --verbose and --porcelain mutually exclusive' '
+ test_must_fail git worktree list --verbose --porcelain
+'
+
+test_expect_success '"list" all worktrees --verbose with locked' '
+ test_when_finished "rm -rf locked out actual expect && git worktree prune" &&
+ git worktree add locked --detach &&
+ git worktree lock locked --reason "with reason" &&
+ test_when_finished "git worktree unlock locked" &&
+ echo "$(git -C locked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
+ printf "\tlocked: with reason\n" >>expect &&
+ git worktree list --verbose >out &&
+ sed -n "s/ */ /g;/\/locked *[0-9a-f].*$/,/locked: .*$/p" <out >actual &&
+ test_cmp actual expect
+'
+
+test_expect_success '"list" all worktrees --verbose with prunable' '
+ test_when_finished "rm -rf prunable out actual expect && git worktree prune" &&
+ git worktree add prunable --detach &&
+ echo "$(git -C prunable rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
+ printf "\tprunable: gitdir file points to non-existent location\n" >>expect &&
+ rm -rf prunable &&
+ git worktree list --verbose >out &&
+ sed -n "s/ */ /g;/\/prunable *[0-9a-f].*$/,/prunable: .*$/p" <out >actual &&
+ test_i18ncmp actual expect
+'
+
test_expect_success 'bare repo setup' '
git init --bare bare1 &&
echo "data" >file1 &&
--
2.30.0.372.gbc7e965391
^ permalink raw reply related [flat|nested] 98+ messages in thread* Re: [PATCH v2 6/6] worktree: teach `list` verbose mode
2021-01-17 23:42 ` [PATCH v2 6/6] worktree: teach `list` verbose mode Rafael Silva
@ 2021-01-18 5:15 ` Eric Sunshine
2021-01-18 19:40 ` Eric Sunshine
0 siblings, 1 reply; 98+ messages in thread
From: Eric Sunshine @ 2021-01-18 5:15 UTC (permalink / raw)
To: Rafael Silva; +Cc: Git List, Phillip Wood
On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> "git worktree list" annotates each worktree according to its state such
> as "prunable" or "locked", however it is not immediately obvious why
> these worktrees are being annotated. For prunable worktrees a reason
> is available that is returned by should_prune_worktree() and for locked
> worktrees a reason might be available provided by the user via `lock`
> command.
>
> Let's teach "git worktree list" to output the reason why the worktrees
> are being annotated. The reason is a text that can take virtually any
> size and appending the text on the default columned format will make it
> difficult to extend the command with other annotations and not fit nicely
> on the screen. In order to address this shortcoming the annotation is
> then moved to the next line indented followed by the reason, if the
> reason is not available the annotation stays on the same line as the
> worktree itself.
If you're re-rolling, let's mention the new `--verbose` option
somewhere in the commit message since that is the focus of this patch.
The second paragraph would be a good place:
Let's teach "git worktree list" a --verbose mode which
outputs the reason...
Also, the final sentence is a bit difficult to follow due to the comma
before "if the reason is not available". If you make the "if the
reason is not available" a separate sentence, it becomes simple to
understand.
> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
> @@ -135,6 +135,33 @@ test_expect_success '"list" all worktrees with prunable consistent with "prune"'
> +test_expect_success '"list" all worktrees --verbose with locked' '
> + test_when_finished "rm -rf locked out actual expect && git worktree prune" &&
> + git worktree add locked --detach &&
> + git worktree lock locked --reason "with reason" &&
> + test_when_finished "git worktree unlock locked" &&
> + echo "$(git -C locked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
> + printf "\tlocked: with reason\n" >>expect &&
> + git worktree list --verbose >out &&
> + sed -n "s/ */ /g;/\/locked *[0-9a-f].*$/,/locked: .*$/p" <out >actual &&
> + test_cmp actual expect
> +'
At first, I wondered if we would also want this test to have a
locked-no-reason worktree to ensure that its `locked` annotation stays
on the same line as the worktree, but that's not needed because that
case is already covered by the existing test. Fine.
> +test_expect_success '"list" all worktrees --verbose with prunable' '
> + test_when_finished "rm -rf prunable out actual expect && git worktree prune" &&
> + git worktree add prunable --detach &&
> + echo "$(git -C prunable rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
> + printf "\tprunable: gitdir file points to non-existent location\n" >>expect &&
> + rm -rf prunable &&
> + git worktree list --verbose >out &&
> + sed -n "s/ */ /g;/\/prunable *[0-9a-f].*$/,/prunable: .*$/p" <out >actual &&
> + test_i18ncmp actual expect
> +'
An alternative would be to have a single test of --verbose which
includes a locked-no-reason worktree, a locked-with-reason worktree,
and a prunable worktree. However, that's a very minor and subjective
point and certainly not worth a re-roll or changing unless you think
it's a nice simplification.
^ permalink raw reply [flat|nested] 98+ messages in thread* Re: [PATCH v2 6/6] worktree: teach `list` verbose mode
2021-01-18 5:15 ` Eric Sunshine
@ 2021-01-18 19:40 ` Eric Sunshine
0 siblings, 0 replies; 98+ messages in thread
From: Eric Sunshine @ 2021-01-18 19:40 UTC (permalink / raw)
To: Rafael Silva; +Cc: Git List, Phillip Wood
On Mon, Jan 18, 2021 at 12:15 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
> > +test_expect_success '"list" all worktrees --verbose with locked' '
> > + test_when_finished "rm -rf locked out actual expect && git worktree prune" &&
> > + git worktree add locked --detach &&
> > + git worktree lock locked --reason "with reason" &&
> > + test_when_finished "git worktree unlock locked" &&
> > + echo "$(git -C locked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
> > + printf "\tlocked: with reason\n" >>expect &&
> > + git worktree list --verbose >out &&
> > + sed -n "s/ */ /g;/\/locked *[0-9a-f].*$/,/locked: .*$/p" <out >actual &&
> > + test_cmp actual expect
> > +'
>
> At first, I wondered if we would also want this test to have a
> locked-no-reason worktree to ensure that its `locked` annotation stays
> on the same line as the worktree, but that's not needed because that
> case is already covered by the existing test. Fine.
Erm, I think what I said is wrong. There is no existing test to show
that `locked` without a reason stays on the same line as the worktree
in --verbose mode. So having a locked-no-reason worktree in this test
could be beneficial.
^ permalink raw reply [flat|nested] 98+ messages in thread
* Re: [PATCH v2 0/6] teach `worktree list` verbose mode and prunable annotations
2021-01-17 23:42 ` [PATCH v2 0/6] " Rafael Silva
` (5 preceding siblings ...)
2021-01-17 23:42 ` [PATCH v2 6/6] worktree: teach `list` verbose mode Rafael Silva
@ 2021-01-18 5:33 ` Eric Sunshine
2021-01-19 16:44 ` Rafael Silva
2021-01-19 21:27 ` [PATCH v3 0/7] " Rafael Silva
7 siblings, 1 reply; 98+ messages in thread
From: Eric Sunshine @ 2021-01-18 5:33 UTC (permalink / raw)
To: Rafael Silva; +Cc: Git List, Phillip Wood
On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> This patch series addresses some of these changes by teaching
> `worktree list` to show "prunable" annotation, adding verbose mode and
> extending the --porcelain format with prunable and locked annotation as
> follow up from [1]. Additionally, it addresses one shortcoming for porcelain
> format to escape any newline characters (LF or CRLF) for the lock reason
> to prevent breaking the format that is mentioned in [4] and [1] during the
> review cycle.
Thanks for continuing to work on this. I'm pleased to see these
enhancements coming together so nicely.
> The patch series is organizes as:
The new organization is a nice improvement over v1.
As mentioned in my review of [5/6], there may be some value in
swapping [5/6] and [6/6] to make it easier for readers to digest the
changes, but it's not a hard requirement.
To avoid being mysterious, there's also a change in [5/6] which
probably belongs in [3/6], as explained in my review of [5/6].
My review of patch [4/6] suggests optionally splitting out a bug fix
into its own patch, but that's a very minor issue. Use your best
judgment whether or not to do so.
> 4. The fourth patch adds the "locked" attribute for the porcelain format
> in order to make both default and --porcelain format consistent.
This patch does need a re-roll, and it's entirely my fault. In my
review of the previous round, I said that if the lock reason contained
special characters, we'd want to escape those characters and quote the
string, but then I gave you a code suggestion which did the opposite
of that. My [4/6] review goes into more detail.
Aside from the one important fix in [4/6], and the possible minor
re-organizations suggested above, all my other review comments were
very minor and probably subjective, thus nothing to sweat over.
Nicely done.
[1]: https://lore.kernel.org/git/CAPig+cSH6PKt8YvDJhMBvzvePYLqbf70uVV8TERoMj+kfxJRHQ@mail.gmail.com/
^ permalink raw reply [flat|nested] 98+ messages in thread* Re: [PATCH v2 0/6] teach `worktree list` verbose mode and prunable annotations
2021-01-18 5:33 ` [PATCH v2 0/6] teach `worktree list` verbose mode and prunable annotations Eric Sunshine
@ 2021-01-19 16:44 ` Rafael Silva
0 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-19 16:44 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Phillip Wood
Eric Sunshine writes:
> On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> This patch series addresses some of these changes by teaching
>> `worktree list` to show "prunable" annotation, adding verbose mode and
>> extending the --porcelain format with prunable and locked annotation as
>> follow up from [1]. Additionally, it addresses one shortcoming for porcelain
>> format to escape any newline characters (LF or CRLF) for the lock reason
>> to prevent breaking the format that is mentioned in [4] and [1] during the
>> review cycle.
>
> Thanks for continuing to work on this. I'm pleased to see these
> enhancements coming together so nicely.
>
>> The patch series is organizes as:
>
> The new organization is a nice improvement over v1.
>
> As mentioned in my review of [5/6], there may be some value in
> swapping [5/6] and [6/6] to make it easier for readers to digest the
> changes, but it's not a hard requirement.
>
> To avoid being mysterious, there's also a change in [5/6] which
> probably belongs in [3/6], as explained in my review of [5/6].
>
> My review of patch [4/6] suggests optionally splitting out a bug fix
> into its own patch, but that's a very minor issue. Use your best
> judgment whether or not to do so.
>
>> 4. The fourth patch adds the "locked" attribute for the porcelain format
>> in order to make both default and --porcelain format consistent.
>
> This patch does need a re-roll, and it's entirely my fault. In my
> review of the previous round, I said that if the lock reason contained
> special characters, we'd want to escape those characters and quote the
> string, but then I gave you a code suggestion which did the opposite
> of that. My [4/6] review goes into more detail.
>
I kindly disagree and I actually think is my fault :).
> Aside from the one important fix in [4/6], and the possible minor
> re-organizations suggested above, all my other review comments were
> very minor and probably subjective, thus nothing to sweat over.
>
> Nicely done.
>
> [1]: https://lore.kernel.org/git/CAPig+cSH6PKt8YvDJhMBvzvePYLqbf70uVV8TERoMj+kfxJRHQ@mail.gmail.com/
Thank you for reviewing this series and all the helpful suggestions. I
will send another revision taking all of them into account.
--
Thanks
Rafael
^ permalink raw reply [flat|nested] 98+ messages in thread
* [PATCH v3 0/7] teach `worktree list` verbose mode and prunable annotations
2021-01-17 23:42 ` [PATCH v2 0/6] " Rafael Silva
` (6 preceding siblings ...)
2021-01-18 5:33 ` [PATCH v2 0/6] teach `worktree list` verbose mode and prunable annotations Eric Sunshine
@ 2021-01-19 21:27 ` Rafael Silva
2021-01-19 21:27 ` [PATCH v3 1/7] worktree: libify should_prune_worktree() Rafael Silva
` (8 more replies)
7 siblings, 9 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-19 21:27 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Rafael Silva
In c57b3367be (worktree: teach `list` to annotate locked worktree,
2020-10-11) we taught `git worktree list` to annotate working tree that
is locked by appending "locked" text in order to signalize to the user
that a working tree is locked. During the review, there was some
discussion about additional annotations and information that `list`
command could provide to the user that has long been envisioned and
mentioned in [2], [3] and [4].
This patch series addresses some of these changes by teaching
`worktree list` to show "prunable" annotation, adding verbose mode and
extending the --porcelain format with prunable and locked annotation as
follow up from [1]. Additionally, it addresses one shortcoming for porcelain
format to escape any newline characters (LF or CRLF) for the lock reason
to prevent breaking the format that is mentioned in [4] and [1] during the
review cycle.
The patch series is organizes as:
1. The first patch moves the should_prune_worktree() machinery to the top-level
worktree.c exposing the function as general API, that will be reference
by should_prune_worktree() wrapper implemented on the second patch.
The original idea was to not only move should_prune_worktree() but also
refactor to accept a "struct worktree" and load the information directly,
which allows simplify the `prune` command by reusing get_worktrees().
However this seems to also require refactoring get_worktrees() itself
to return "non-valid" working trees that can/should be pruned. This is
also mentioned in [5]. Having the wrapper function makes it easier to add
the prunable annotation without touching the get_worktrees() and the
other worktree sub commands. The refactoring can be addressed in a
future patch, if this turns out to be good idea. One possible approach
is to teach get_worktrees() to take additional flags that will tell
whether to return only valid or all worktrees in GIT_DIR/worktrees/
directory and address its own possible shortcoming.
2. Introduces the worktree_prune_reason() to discovery whether a worktree
is prunable along with two new fields in the `worktree` structure. The
function mimics the workree_lock_reason() API.
3. The third patch changes worktree_lock_reason() to be more gentle for
the main working tree to simply returning NULL instead of aborting the
program via assert() macro. This allow us to simplify the code that
checks if the working tree is locked for default and porcelain format.
This changes is also mentioned in [6].
4. Fix t2402 added in [1] to ensure the locked worktree is properly cleaned up.
5. The fourth patch adds the "locked" attribute for the porcelain format
in order to make both default and --porcelain format consistent.
6. The fifth patch introduces "prunable" annotation for both default
and --porcelain format.
7. The sixth patch introduces verbose mode to expand the `list` default
format and show each annotation reason when its available.
Series is built on top of 66e871b664 (The third batch, 2021-01-15).
[1] https://lore.kernel.org/git/20200928154953.30396-1-rafaeloliveira.cs@gmail.com/
[2] https://lore.kernel.org/git/CAPig+cQF6V8HNdMX5AZbmz3_w2WhSfA4SFfNhQqxXBqPXTZL+w@mail.gmail.com/
[3] https://lore.kernel.org/git/CAPig+cSGXqJuaZPhUhOVX5X=LMrjVfv8ye_6ncMUbyKox1i7QA@mail.gmail.com/
[4] https://lore.kernel.org/git/CAPig+cTitWCs5vB=0iXuUyEY22c0gvjXvY1ZtTT90s74ydhE=A@mail.gmail.com/
[5] https://lore.kernel.org/git/CACsJy8ChM99n6skQCv-GmFiod19mnwwH4j-6R+cfZSiVFAxjgA@mail.gmail.com/
[6] https://lore.kernel.org/git/xmqq8sctlgzx.fsf@gitster.c.googlers.com/
Changes in v3
=============
v3 is 1-patch bigger than v2 and it includes the following changes:
* Dropped CQUOTE_NODQ flag in the the locked reason to return a string
enclosed with double quotes if the text reason contains especial
characters, like newlines.
* fix in t2402 to ensure the locked worktree is properly cleaned up,
is move to its own patch.
* In worktree_prune_reason(), the `path` variable is initialize
with NULL to make the code easier to follow.
* The removal of `is_main_worktree()` before `worktree_lock_reason()`
is moved to the patch that actually changes the API to be more gentle
with the main worktree instead of refactoring the code in the next
patches that adds the annotations.
* Drop the `*reason` in `(reason && *reason)` as we know that
worktree_prune_reason() is either going to return a non-empty string
or NULL which makes the code easier to follow.
* The --verbose test for the locked annotation now tests that "locked"
annotation stays on the same line when there is no locked reason.
* Small documentation updates and make the "git worktree --verbose"
example a little consistent with the worktree path.
Changes in v2
=============
v2 changes considerably from v1 taking into account several comments
suggested by Eric Sunshine and Phillip Wood (thanks guys for the
insightful comments and patience reviewing my patches :) ).
* The biggest change are the way the series is organized. In v1 all the
code was implemented in the fourth patch, all the tests and documentation
updates was added in sixth and seventh patch respectfully, in v2 each
patch introduces the annotations and verbose mode together with theirs
respective test and documentation updates.
* Several rewrite of the commit messages
* In v1 the fifth patch was introducing a new function to escape newline
characters for the "locked" attribute. However, we already have this
feature from the quote.h API. So the new function is dropped and the
changes are squashed into v2's fourth patch.
* The new prunable annotation and locked annotation that was introduced
by [1] was refactor to not poke around the worktree private fields.
* Refactoring of the worktree_prune_reason() to cache the prune_reason
and refactor to early return the use cases following the more common
pattern used on Git's codebase.
* Few documentation rewrites, most notably the `--verbose` and `--expire`
doc sentences for the list command are moved to its own line to clearly
separate the description from the others commands instead of continuing
on the same paragraph.
* The `git unlock <worktree>` used in the test's cleanup is moved to after
we know the `git worktree locked` is executed successfully. This ensures
the unlock doesn't error in the cleanup stage which will make it harder
to debug the tests.
Rafael Silva (7):
worktree: libify should_prune_worktree()
worktree: teach worktree to lazy-load "prunable" reason
worktree: teach worktree_lock_reason() to gently handle main worktree
t2402: ensure locked worktree is properly cleaned up
worktree: teach `list --porcelain` to annotate locked worktree
worktree: teach `list` to annotate prunable worktree
worktree: teach `list` verbose mode
Documentation/git-worktree.txt | 62 +++++++++++++++++--
builtin/worktree.c | 110 +++++++++++----------------------
builtin/worktree.cc | 0
t/t2402-worktree-list.sh | 93 ++++++++++++++++++++++++++++
worktree.c | 91 ++++++++++++++++++++++++++-
worktree.h | 23 +++++++
6 files changed, 299 insertions(+), 80 deletions(-)
create mode 100644 builtin/worktree.cc
Range-diff against v2:
-: ---------- > 1: 66cd83ba42 worktree: libify should_prune_worktree()
1: 9d47fcb4a4 ! 2: 81f4824028 worktree: teach worktree to lazy-load "prunable" reason
@@ worktree.c: const char *worktree_lock_reason(struct worktree *wt)
+const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire)
+{
+ struct strbuf reason = STRBUF_INIT;
-+ char *path;
++ char *path = NULL;
+
+ if (is_main_worktree(wt))
+ return NULL;
2: ac7c375bac ! 3: d0dbf0b709 worktree: teach worktree_lock_reason() to gently handle main worktree
@@ Commit message
worktree_lock_reason() aborts with an assertion failure when called on
the main worktree since locking the main worktree is nonsensical. Not
- only is this behaviour undocumented, thus callers might not even be aware
+ only is this behavior undocumented, thus callers might not even be aware
that the call could potentially crash the program, but it also forces
clients to be extra careful:
@@ Commit message
...
Since we know that locking makes no sense in the context of the main
- worktree, we can simpliy return false for the main worktree, thus making
+ worktree, we can simply return false for the main worktree, thus making
client code less complex by eliminating the need for the callers to have
inside knowledge about the implementation:
@@ Commit message
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
+ ## builtin/worktree.c ##
+@@ builtin/worktree.c: static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
+ strbuf_addstr(&sb, "(error)");
+ }
+
+- if (!is_main_worktree(wt) && worktree_lock_reason(wt))
++ if (worktree_lock_reason(wt))
+ strbuf_addstr(&sb, " locked");
+
+ printf("%s\n", sb.buf);
+
## worktree.c ##
@@ worktree.c: int is_main_worktree(const struct worktree *wt)
-: ---------- > 4: c6f8a3e9cd t2402: ensure locked worktree is properly cleaned up
3: ec1eb5a9f8 ! 5: 86c5253288 worktree: teach `list --porcelain` to annotate locked worktree
@@ Commit message
locked reason why it is locked
...
- The locked reason is quoted to prevent newline characters (i.e: LF or
- CRLF) breaking the --porcelain format as each attribute is shown per
- line. For example:
+ In porcelain mode, if the lock reason contains special characters
+ such as newlines, they are escaped with backslashes and the entire
+ reason is enclosed in double quotes. For example:
$ git worktree list --porcelain
...
- locked worktree's path mounted in\nremovable device
+ locked "worktree's path mounted in\nremovable device"
...
Furthermore, let's update the documentation to state that some
@@ Commit message
with its value depending whether the value is available or not. Thus
documenting the case of the new "locked" attribute.
- Additionally, c57b3367be (worktree: teach `list` to annotate locked
- worktree, 2020-10-11) introduced a new test to ensure locked worktrees
- are listed with "locked" annotation. However, the test does not clean up
- after itself as "git worktree prune" is not going to remove the locked
- worktree in the first place. This not only leaves the test in an unclean
- state it also potentially breaks following tests that relies on the
- "git worktree list" output. Let's fix that by unlocking the worktree
- before the "prune" command.
-
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
@@ Documentation/git-worktree.txt: Porcelain Format
-if the value is true. The first attribute of a working tree is always
-`worktree`, an empty line indicates the end of the record. For example:
+if the value is true. Some attributes (like `locked`) can be listed as a label
-+only or with a value depending whether a reason is available. The first
++only or with a value depending upon whether a reason is available. The first
+attribute of a working tree is always `worktree`, an empty line indicates the
+end of the record. For example:
@@ Documentation/git-worktree.txt: worktree /path/to/other-linked-worktree
HEAD 1234abc1234abc1234abc1234abc1234abc1234a
detached
-+worktree /path/to/linked-worktree-locked
++worktree /path/to/linked-worktree-locked-no-reason
+HEAD 5678abc5678abc5678abc5678abc5678abc5678c
-+branch refs/heads/locked
++branch refs/heads/locked-no-reason
+locked
+
+worktree /path/to/linked-worktree-locked-with-reason
@@ builtin/worktree.c: static void show_worktree_porcelain(struct worktree *wt)
+ reason = worktree_lock_reason(wt);
+ if (reason && *reason) {
+ struct strbuf sb = STRBUF_INIT;
-+ quote_c_style(reason, &sb, NULL, CQUOTE_NODQ);
++ quote_c_style(reason, &sb, NULL, 0);
+ printf("locked %s\n", sb.buf);
+ strbuf_release(&sb);
+ } else if (reason)
@@ builtin/worktree.c: static void show_worktree_porcelain(struct worktree *wt)
## t/t2402-worktree-list.sh ##
@@ t/t2402-worktree-list.sh: test_expect_success '"list" all worktrees with locked annotation' '
- git worktree add --detach locked master &&
- git worktree add --detach unlocked master &&
- git worktree lock locked &&
-+ test_when_finished "git worktree unlock locked" &&
- git worktree list >out &&
- grep "/locked *[0-9a-f].* locked$" out &&
! grep "/unlocked *[0-9a-f].* locked$" out
'
@@ t/t2402-worktree-list.sh: test_expect_success '"list" all worktrees with locked
+ echo "locked with reason" >>expect &&
+ git worktree add --detach locked1 &&
+ git worktree add --detach locked2 &&
++ # unlocked worktree should not be annotated with "locked"
+ git worktree add --detach unlocked &&
+ git worktree lock locked1 &&
+ git worktree lock locked2 --reason "with reason" &&
@@ t/t2402-worktree-list.sh: test_expect_success '"list" all worktrees with locked
+
+test_expect_success '"list" all worktrees --porcelain with locked reason newline escaped' '
+ test_when_finished "rm -rf locked_lf locked_crlf out actual expect && git worktree prune" &&
-+ printf "locked locked\\\\r\\\\nreason\n" >expect &&
-+ printf "locked locked\\\\nreason\n" >>expect &&
++ printf "locked \"locked\\\\r\\\\nreason\"\n" >expect &&
++ printf "locked \"locked\\\\nreason\"\n" >>expect &&
+ git worktree add --detach locked_lf &&
+ git worktree add --detach locked_crlf &&
-+ printf "locked\nreason\n\n" >reason_lf &&
-+ printf "locked\r\nreason\n\n" >reason_crlf &&
-+ git worktree lock locked_lf --reason "$(cat reason_lf)" &&
-+ git worktree lock locked_crlf --reason "$(cat reason_crlf)" &&
++ git worktree lock locked_lf --reason "$(printf "locked\nreason")" &&
++ git worktree lock locked_crlf --reason "$(printf "locked\r\nreason")" &&
+ test_when_finished "git worktree unlock locked_lf && git worktree unlock locked_crlf" &&
+ git worktree list --porcelain >out &&
+ grep "^locked" out >actual &&
4: f937df6460 ! 6: be814326ee worktree: teach `list` to annotate prunable worktree
@@ Documentation/git-worktree.txt: This can also be set up as the default behaviour
With `prune`, only expire unused working trees older than `<time>`.
++
+With `list`, annotate missing working trees as prunable if they are
-+older than `<mtime>`.
++older than `<time>`.
--reason <string>::
With `lock`, an explanation why the working tree is locked.
@@ Documentation/git-worktree.txt: $ git worktree list
+
+------------
+$ git worktree list
-+/path/to/linked-worktree abcd1234 [master]
-+/path/to/locked-worktreee acbd5678 (brancha) locked
-+/path/to/prunable-worktree 5678abc (detached HEAD) prunable
++/path/to/linked-worktree abcd1234 [master]
++/path/to/locked-worktreee acbd5678 (brancha) locked
++/path/to/prunable-worktree 5678abc (detached HEAD) prunable
+------------
+
Porcelain Format
@@ builtin/worktree.c: static void show_worktree_porcelain(struct worktree *wt)
printf("locked\n");
+ reason = worktree_prune_reason(wt, expire);
-+ if (reason && *reason)
++ if (reason)
+ printf("prunable %s\n", reason);
+
printf("\n");
}
@@ builtin/worktree.c: static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
- struct strbuf sb = STRBUF_INIT;
- int cur_path_len = strlen(wt->path);
- int path_adj = cur_path_len - utf8_strwidth(wt->path);
-+ const char *reason;
-
- strbuf_addf(&sb, "%-*s ", 1 + path_maxlen + path_adj, wt->path);
- if (wt->is_bare)
-@@ builtin/worktree.c: static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
- strbuf_addstr(&sb, "(error)");
- }
-
-- if (!is_main_worktree(wt) && worktree_lock_reason(wt))
-+ reason = worktree_lock_reason(wt);
-+ if (reason)
+ if (worktree_lock_reason(wt))
strbuf_addstr(&sb, " locked");
-+ reason = worktree_prune_reason(wt, expire);
-+ if (reason)
++ if (worktree_prune_reason(wt, expire))
+ strbuf_addstr(&sb, " prunable");
+
printf("%s\n", sb.buf);
@@ builtin/worktree.c: static int list(int ac, const char **av, const char *prefix)
if (ac)
usage_with_options(worktree_usage, options);
+ ## builtin/worktree.cc (new) ##
+
## t/t2402-worktree-list.sh ##
@@ t/t2402-worktree-list.sh: test_expect_success '"list" all worktrees --porcelain with locked reason newline
test_cmp expect actual
5: 0b6ab6dddb ! 7: b0688f142d worktree: teach `list` verbose mode
@@ Commit message
worktrees a reason might be available provided by the user via `lock`
command.
- Let's teach "git worktree list" to output the reason why the worktrees
- are being annotated. The reason is a text that can take virtually any
- size and appending the text on the default columned format will make it
- difficult to extend the command with other annotations and not fit nicely
- on the screen. In order to address this shortcoming the annotation is
- then moved to the next line indented followed by the reason, if the
- reason is not available the annotation stays on the same line as the
- worktree itself.
+ Let's teach "git worktree list" a --verbose mode that outputs the reason
+ why the worktrees are being annotated. The reason is a text that can take
+ virtually any size and appending the text on the default columned format
+ will make it difficult to extend the command with other annotations and
+ not fit nicely on the screen. In order to address this shortcoming the
+ annotation is then moved to the next line indented followed by the reason
+ If the reason is not available the annotation stays on the same line as
+ the worktree itself.
The output of "git worktree list" with verbose becomes like so:
$ git worktree list --verbose
...
- /path/to/locked acb124 [branch-a] locked
- /path/to/locked-with-reason acc125 [branch-b]
+ /path/to/locked-no-reason acb124 [branch-a] locked
+ /path/to/locked-with-reason acc125 [branch-b]
locked: worktree with a locked reason
- /path/to/prunable-reason ace127 [branch-d]
+ /path/to/prunable-reason ace127 [branch-d]
prunable: gitdir file points to non-existent location
...
@@ Documentation/git-worktree.txt: This can also be set up as the default behaviour
--expire <time>::
With `prune`, only expire unused working trees older than `<time>`.
@@ Documentation/git-worktree.txt: $ git worktree list
- /path/to/prunable-worktree 5678abc (detached HEAD) prunable
+ /path/to/prunable-worktree 5678abc (detached HEAD) prunable
------------
+For these annotations, a reason might also be available and this can be
@@ Documentation/git-worktree.txt: $ git worktree list
+
+------------
+$ git worktree list --verbose
-+/path/to/linked-worktree abcd1234 [master]
-+/path/to/locked-worktreee acbd5678 (brancha)
-+ locked: working tree path is mounted on a removable device
-+/path/to/locked-no-reason abcd578 (detached HEAD) locked
-+/path/to/prunable-worktree 5678abc (detached HEAD)
++/path/to/linked-worktree abcd1234 [master]
++/path/to/locked-worktree-no-reason abcd5678 (detached HEAD) locked
++/path/to/locked-worktree-with-reason 1234abcd (brancha)
++ locked: working tree path is mounted on a portable device
++/path/to/prunable-worktree 5678abc1 (detached HEAD)
+ prunable: gitdir file points to non-existent location
+------------
+
@@ Documentation/git-worktree.txt: $ git worktree list
## builtin/worktree.c ##
@@ builtin/worktree.c: static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
+ struct strbuf sb = STRBUF_INIT;
+ int cur_path_len = strlen(wt->path);
+ int path_adj = cur_path_len - utf8_strwidth(wt->path);
++ const char *reason;
+
+ strbuf_addf(&sb, "%-*s ", 1 + path_maxlen + path_adj, wt->path);
+ if (wt->is_bare)
+@@ builtin/worktree.c: static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
+ strbuf_addstr(&sb, "(error)");
}
- reason = worktree_lock_reason(wt);
-- if (reason)
+- if (worktree_lock_reason(wt))
++ reason = worktree_lock_reason(wt);
+ if (verbose && reason && *reason)
+ strbuf_addf(&sb, "\n\tlocked: %s", reason);
+ else if (reason)
strbuf_addstr(&sb, " locked");
- reason = worktree_prune_reason(wt, expire);
-- if (reason)
-+ if (verbose && reason && *reason)
+- if (worktree_prune_reason(wt, expire))
++ reason = worktree_prune_reason(wt, expire);
++ if (verbose && reason)
+ strbuf_addf(&sb, "\n\tprunable: %s", reason);
+ else if (reason)
strbuf_addstr(&sb, " prunable");
@@ t/t2402-worktree-list.sh: test_expect_success '"list" all worktrees with prunabl
+'
+
+test_expect_success '"list" all worktrees --verbose with locked' '
-+ test_when_finished "rm -rf locked out actual expect && git worktree prune" &&
-+ git worktree add locked --detach &&
-+ git worktree lock locked --reason "with reason" &&
-+ test_when_finished "git worktree unlock locked" &&
-+ echo "$(git -C locked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
++ test_when_finished "rm -rf locked1 locked2 out actual expect && git worktree prune" &&
++ git worktree add locked1 --detach &&
++ git worktree add locked2 --detach &&
++ git worktree lock locked1 &&
++ git worktree lock locked2 --reason "with reason" &&
++ test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" &&
++ echo "$(git -C locked2 rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
+ printf "\tlocked: with reason\n" >>expect &&
+ git worktree list --verbose >out &&
-+ sed -n "s/ */ /g;/\/locked *[0-9a-f].*$/,/locked: .*$/p" <out >actual &&
++ grep "/locked1 *[0-9a-f].* locked$" out &&
++ sed -n "s/ */ /g;/\/locked2 *[0-9a-f].*$/,/locked: .*$/p" <out >actual &&
+ test_cmp actual expect
+'
+
--
2.30.0.421.g32f838e276
^ permalink raw reply [flat|nested] 98+ messages in thread* [PATCH v3 1/7] worktree: libify should_prune_worktree()
2021-01-19 21:27 ` [PATCH v3 0/7] " Rafael Silva
@ 2021-01-19 21:27 ` Rafael Silva
2021-01-19 21:27 ` [PATCH v3 2/7] worktree: teach worktree to lazy-load "prunable" reason Rafael Silva
` (7 subsequent siblings)
8 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-19 21:27 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Rafael Silva
As part of teaching "git worktree list" to annotate worktree that is a
candidate for pruning, let's move should_prune_worktree() from
builtin/worktree.c to worktree.c in order to make part of the worktree
public API.
should_prune_worktree() knows how to select the given worktree for
pruning based on an expiration date, however the expiration value is
stored in a static file-scope variable and it is not local to the
function. In order to move the function, teach should_prune_worktree()
to take the expiration date as an argument and document the new
parameter that is not immediately obvious.
Also, change the function comment to clearly state that the worktree's
path is returned in `wtpath` argument.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
builtin/worktree.c | 75 +---------------------------------------------
worktree.c | 68 +++++++++++++++++++++++++++++++++++++++++
worktree.h | 14 +++++++++
3 files changed, 83 insertions(+), 74 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 71287b2da6..dd886d5029 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -67,79 +67,6 @@ static void delete_worktrees_dir_if_empty(void)
rmdir(git_path("worktrees")); /* ignore failed removal */
}
-/*
- * Return true if worktree entry should be pruned, along with the reason for
- * pruning. Otherwise, return false and the worktree's path, or NULL if it
- * cannot be determined. Caller is responsible for freeing returned path.
- */
-static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)
-{
- struct stat st;
- char *path;
- int fd;
- size_t len;
- ssize_t read_result;
-
- *wtpath = NULL;
- if (!is_directory(git_path("worktrees/%s", id))) {
- strbuf_addstr(reason, _("not a valid directory"));
- return 1;
- }
- if (file_exists(git_path("worktrees/%s/locked", id)))
- return 0;
- if (stat(git_path("worktrees/%s/gitdir", id), &st)) {
- strbuf_addstr(reason, _("gitdir file does not exist"));
- return 1;
- }
- fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY);
- if (fd < 0) {
- strbuf_addf(reason, _("unable to read gitdir file (%s)"),
- strerror(errno));
- return 1;
- }
- len = xsize_t(st.st_size);
- path = xmallocz(len);
-
- read_result = read_in_full(fd, path, len);
- if (read_result < 0) {
- strbuf_addf(reason, _("unable to read gitdir file (%s)"),
- strerror(errno));
- close(fd);
- free(path);
- return 1;
- }
- close(fd);
-
- if (read_result != len) {
- strbuf_addf(reason,
- _("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"),
- (uintmax_t)len, (uintmax_t)read_result);
- free(path);
- return 1;
- }
- while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
- len--;
- if (!len) {
- strbuf_addstr(reason, _("invalid gitdir file"));
- free(path);
- return 1;
- }
- path[len] = '\0';
- if (!file_exists(path)) {
- if (stat(git_path("worktrees/%s/index", id), &st) ||
- st.st_mtime <= expire) {
- strbuf_addstr(reason, _("gitdir file points to non-existent location"));
- free(path);
- return 1;
- } else {
- *wtpath = path;
- return 0;
- }
- }
- *wtpath = path;
- return 0;
-}
-
static void prune_worktree(const char *id, const char *reason)
{
if (show_only || verbose)
@@ -195,7 +122,7 @@ static void prune_worktrees(void)
if (is_dot_or_dotdot(d->d_name))
continue;
strbuf_reset(&reason);
- if (should_prune_worktree(d->d_name, &reason, &path))
+ if (should_prune_worktree(d->d_name, &reason, &path, expire))
prune_worktree(d->d_name, reason.buf);
else if (path)
string_list_append(&kept, path)->util = xstrdup(d->d_name);
diff --git a/worktree.c b/worktree.c
index 821b233479..8ae019af79 100644
--- a/worktree.c
+++ b/worktree.c
@@ -741,3 +741,71 @@ void repair_worktree_at_path(const char *path,
strbuf_release(&realdotgit);
strbuf_release(&dotgit);
}
+
+int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, timestamp_t expire)
+{
+ struct stat st;
+ char *path;
+ int fd;
+ size_t len;
+ ssize_t read_result;
+
+ *wtpath = NULL;
+ if (!is_directory(git_path("worktrees/%s", id))) {
+ strbuf_addstr(reason, _("not a valid directory"));
+ return 1;
+ }
+ if (file_exists(git_path("worktrees/%s/locked", id)))
+ return 0;
+ if (stat(git_path("worktrees/%s/gitdir", id), &st)) {
+ strbuf_addstr(reason, _("gitdir file does not exist"));
+ return 1;
+ }
+ fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY);
+ if (fd < 0) {
+ strbuf_addf(reason, _("unable to read gitdir file (%s)"),
+ strerror(errno));
+ return 1;
+ }
+ len = xsize_t(st.st_size);
+ path = xmallocz(len);
+
+ read_result = read_in_full(fd, path, len);
+ if (read_result < 0) {
+ strbuf_addf(reason, _("unable to read gitdir file (%s)"),
+ strerror(errno));
+ close(fd);
+ free(path);
+ return 1;
+ }
+ close(fd);
+
+ if (read_result != len) {
+ strbuf_addf(reason,
+ _("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"),
+ (uintmax_t)len, (uintmax_t)read_result);
+ free(path);
+ return 1;
+ }
+ while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
+ len--;
+ if (!len) {
+ strbuf_addstr(reason, _("invalid gitdir file"));
+ free(path);
+ return 1;
+ }
+ path[len] = '\0';
+ if (!file_exists(path)) {
+ if (stat(git_path("worktrees/%s/index", id), &st) ||
+ st.st_mtime <= expire) {
+ strbuf_addstr(reason, _("gitdir file points to non-existent location"));
+ free(path);
+ return 1;
+ } else {
+ *wtpath = path;
+ return 0;
+ }
+ }
+ *wtpath = path;
+ return 0;
+}
diff --git a/worktree.h b/worktree.h
index f38e6fd5a2..818e1491c7 100644
--- a/worktree.h
+++ b/worktree.h
@@ -73,6 +73,20 @@ int is_main_worktree(const struct worktree *wt);
*/
const char *worktree_lock_reason(struct worktree *wt);
+/*
+ * Return true if worktree entry should be pruned, along with the reason for
+ * pruning. Otherwise, return false and the worktree's path in `wtpath`, or
+ * NULL if it cannot be determined. Caller is responsible for freeing
+ * returned path.
+ *
+ * `expire` defines a grace period to prune the worktree when its path
+ * does not exist.
+ */
+int should_prune_worktree(const char *id,
+ struct strbuf *reason,
+ char **wtpath,
+ timestamp_t expire);
+
#define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)
/*
--
2.30.0.421.g32f838e276
^ permalink raw reply related [flat|nested] 98+ messages in thread* [PATCH v3 2/7] worktree: teach worktree to lazy-load "prunable" reason
2021-01-19 21:27 ` [PATCH v3 0/7] " Rafael Silva
2021-01-19 21:27 ` [PATCH v3 1/7] worktree: libify should_prune_worktree() Rafael Silva
@ 2021-01-19 21:27 ` Rafael Silva
2021-01-19 21:27 ` [PATCH v3 3/7] worktree: teach worktree_lock_reason() to gently handle main worktree Rafael Silva
` (6 subsequent siblings)
8 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-19 21:27 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Rafael Silva
Add worktree_prune_reason() to allow a caller to discover whether a
worktree is prunable and the reason that it is, much like
worktree_lock_reason() indicates whether a worktree is locked and the
reason for the lock. As with worktree_lock_reason(), retrieve the
prunable reason lazily and cache it in the `worktree` structure.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
worktree.c | 20 ++++++++++++++++++++
worktree.h | 9 +++++++++
2 files changed, 29 insertions(+)
diff --git a/worktree.c b/worktree.c
index 8ae019af79..fb3e286996 100644
--- a/worktree.c
+++ b/worktree.c
@@ -15,6 +15,7 @@ void free_worktrees(struct worktree **worktrees)
free(worktrees[i]->id);
free(worktrees[i]->head_ref);
free(worktrees[i]->lock_reason);
+ free(worktrees[i]->prune_reason);
free(worktrees[i]);
}
free (worktrees);
@@ -245,6 +246,25 @@ const char *worktree_lock_reason(struct worktree *wt)
return wt->lock_reason;
}
+const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire)
+{
+ struct strbuf reason = STRBUF_INIT;
+ char *path = NULL;
+
+ if (is_main_worktree(wt))
+ return NULL;
+ if (wt->prune_reason_valid)
+ return wt->prune_reason;
+
+ if (should_prune_worktree(wt->id, &reason, &path, expire))
+ wt->prune_reason = strbuf_detach(&reason, NULL);
+ wt->prune_reason_valid = 1;
+
+ strbuf_release(&reason);
+ free(path);
+ return wt->prune_reason;
+}
+
/* convenient wrapper to deal with NULL strbuf */
static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...)
{
diff --git a/worktree.h b/worktree.h
index 818e1491c7..8b7c408132 100644
--- a/worktree.h
+++ b/worktree.h
@@ -11,11 +11,13 @@ struct worktree {
char *id;
char *head_ref; /* NULL if HEAD is broken or detached */
char *lock_reason; /* private - use worktree_lock_reason */
+ char *prune_reason; /* private - use worktree_prune_reason */
struct object_id head_oid;
int is_detached;
int is_bare;
int is_current;
int lock_reason_valid; /* private */
+ int prune_reason_valid; /* private */
};
/*
@@ -73,6 +75,13 @@ int is_main_worktree(const struct worktree *wt);
*/
const char *worktree_lock_reason(struct worktree *wt);
+/*
+ * Return the reason string if the given worktree should be pruned, otherwise
+ * NULL if it should not be pruned. `expire` defines a grace period to prune
+ * the worktree when its path does not exist.
+ */
+const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire);
+
/*
* Return true if worktree entry should be pruned, along with the reason for
* pruning. Otherwise, return false and the worktree's path in `wtpath`, or
--
2.30.0.421.g32f838e276
^ permalink raw reply related [flat|nested] 98+ messages in thread* [PATCH v3 3/7] worktree: teach worktree_lock_reason() to gently handle main worktree
2021-01-19 21:27 ` [PATCH v3 0/7] " Rafael Silva
2021-01-19 21:27 ` [PATCH v3 1/7] worktree: libify should_prune_worktree() Rafael Silva
2021-01-19 21:27 ` [PATCH v3 2/7] worktree: teach worktree to lazy-load "prunable" reason Rafael Silva
@ 2021-01-19 21:27 ` Rafael Silva
2021-01-19 21:27 ` [PATCH v3 4/7] t2402: ensure locked worktree is properly cleaned up Rafael Silva
` (5 subsequent siblings)
8 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-19 21:27 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Rafael Silva
worktree_lock_reason() aborts with an assertion failure when called on
the main worktree since locking the main worktree is nonsensical. Not
only is this behavior undocumented, thus callers might not even be aware
that the call could potentially crash the program, but it also forces
clients to be extra careful:
if (!is_main_worktree(wt) && worktree_locked_reason(...))
...
Since we know that locking makes no sense in the context of the main
worktree, we can simply return false for the main worktree, thus making
client code less complex by eliminating the need for the callers to have
inside knowledge about the implementation:
if (worktree_lock_reason(...))
...
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
builtin/worktree.c | 2 +-
worktree.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index dd886d5029..df90a5acca 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -604,7 +604,7 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
strbuf_addstr(&sb, "(error)");
}
- if (!is_main_worktree(wt) && worktree_lock_reason(wt))
+ if (worktree_lock_reason(wt))
strbuf_addstr(&sb, " locked");
printf("%s\n", sb.buf);
diff --git a/worktree.c b/worktree.c
index fb3e286996..e00858540e 100644
--- a/worktree.c
+++ b/worktree.c
@@ -225,7 +225,8 @@ int is_main_worktree(const struct worktree *wt)
const char *worktree_lock_reason(struct worktree *wt)
{
- assert(!is_main_worktree(wt));
+ if (is_main_worktree(wt))
+ return NULL;
if (!wt->lock_reason_valid) {
struct strbuf path = STRBUF_INIT;
--
2.30.0.421.g32f838e276
^ permalink raw reply related [flat|nested] 98+ messages in thread* [PATCH v3 4/7] t2402: ensure locked worktree is properly cleaned up
2021-01-19 21:27 ` [PATCH v3 0/7] " Rafael Silva
` (2 preceding siblings ...)
2021-01-19 21:27 ` [PATCH v3 3/7] worktree: teach worktree_lock_reason() to gently handle main worktree Rafael Silva
@ 2021-01-19 21:27 ` Rafael Silva
2021-01-24 7:50 ` Eric Sunshine
2021-01-19 21:27 ` [PATCH v3 5/7] worktree: teach `list --porcelain` to annotate locked worktree Rafael Silva
` (4 subsequent siblings)
8 siblings, 1 reply; 98+ messages in thread
From: Rafael Silva @ 2021-01-19 21:27 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Rafael Silva
In c57b3367be (worktree: teach `list` to annotate locked worktree,
2020-10-11) introduced a new test to ensure locked worktrees are listed
with "locked" annotation. However, the test does not clean up after
itself as "git worktree prune" is not going to remove the locked worktree
in the first place. This not only leaves the test in an unclean state it
also potentially breaks following tests that relies on the
"git worktree list" output.
Let's fix that by unlocking the worktree before the "prune" command.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
t/t2402-worktree-list.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index 795ddca2e4..1866ea09f6 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh
@@ -66,6 +66,7 @@ test_expect_success '"list" all worktrees with locked annotation' '
git worktree add --detach locked master &&
git worktree add --detach unlocked master &&
git worktree lock locked &&
+ test_when_finished "git worktree unlock locked" &&
git worktree list >out &&
grep "/locked *[0-9a-f].* locked$" out &&
! grep "/unlocked *[0-9a-f].* locked$" out
--
2.30.0.421.g32f838e276
^ permalink raw reply related [flat|nested] 98+ messages in thread* Re: [PATCH v3 4/7] t2402: ensure locked worktree is properly cleaned up
2021-01-19 21:27 ` [PATCH v3 4/7] t2402: ensure locked worktree is properly cleaned up Rafael Silva
@ 2021-01-24 7:50 ` Eric Sunshine
2021-01-24 10:19 ` Rafael Silva
0 siblings, 1 reply; 98+ messages in thread
From: Eric Sunshine @ 2021-01-24 7:50 UTC (permalink / raw)
To: Rafael Silva; +Cc: Git List, Phillip Wood
On Tue, Jan 19, 2021 at 4:28 PM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> In c57b3367be (worktree: teach `list` to annotate locked worktree,
> 2020-10-11) introduced a new test to ensure locked worktrees are listed
> with "locked" annotation. However, the test does not clean up after
> itself as "git worktree prune" is not going to remove the locked worktree
> in the first place. This not only leaves the test in an unclean state it
> also potentially breaks following tests that relies on the
> "git worktree list" output.
A couple grammos:
1) Drop "In" from the start of the first sentence.
2) s/relies/rely/
But please do not re-roll just for this. It's good enough as-is.
The patch itself is fine.
> Let's fix that by unlocking the worktree before the "prune" command.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
^ permalink raw reply [flat|nested] 98+ messages in thread
* Re: [PATCH v3 4/7] t2402: ensure locked worktree is properly cleaned up
2021-01-24 7:50 ` Eric Sunshine
@ 2021-01-24 10:19 ` Rafael Silva
0 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-24 10:19 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Phillip Wood
Eric Sunshine writes:
> On Tue, Jan 19, 2021 at 4:28 PM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> In c57b3367be (worktree: teach `list` to annotate locked worktree,
>> 2020-10-11) introduced a new test to ensure locked worktrees are listed
>> with "locked" annotation. However, the test does not clean up after
>> itself as "git worktree prune" is not going to remove the locked worktree
>> in the first place. This not only leaves the test in an unclean state it
>> also potentially breaks following tests that relies on the
>> "git worktree list" output.
>
> A couple grammos:
>
> 1) Drop "In" from the start of the first sentence.
> 2) s/relies/rely/
>
> But please do not re-roll just for this. It's good enough as-is.
>
> The patch itself is fine.
>
Nice catch. I'll be re-rolling the series with the documentation addition
suggested by Phillip and the test fixes suggested by you. So, I'll
include these changes as well.
--
Thanks
Rafael
^ permalink raw reply [flat|nested] 98+ messages in thread
* [PATCH v3 5/7] worktree: teach `list --porcelain` to annotate locked worktree
2021-01-19 21:27 ` [PATCH v3 0/7] " Rafael Silva
` (3 preceding siblings ...)
2021-01-19 21:27 ` [PATCH v3 4/7] t2402: ensure locked worktree is properly cleaned up Rafael Silva
@ 2021-01-19 21:27 ` Rafael Silva
2021-01-20 11:00 ` Phillip Wood
2021-01-24 8:10 ` Eric Sunshine
2021-01-19 21:27 ` [PATCH v3 6/7] worktree: teach `list` to annotate prunable worktree Rafael Silva
` (3 subsequent siblings)
8 siblings, 2 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-19 21:27 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Rafael Silva
Commit c57b3367be (worktree: teach `list` to annotate locked worktree,
2020-10-11) taught "git worktree list" to annotate locked worktrees by
appending "locked" text to its output, however, this is not listed in
the --porcelain format.
Teach "list --porcelain" to do the same and add a "locked" attribute
followed by its reason, thus making both default and porcelain format
consistent. If the locked reason is not available then only "locked"
is shown.
The output of the "git worktree list --porcelain" becomes like so:
$ git worktree list --porcelain
...
worktree /path/to/locked
HEAD 123abcdea123abcd123acbd123acbda123abcd12
detached
locked
worktree /path/to/locked-with-reason
HEAD abc123abc123abc123abc123abc123abc123abc1
detached
locked reason why it is locked
...
In porcelain mode, if the lock reason contains special characters
such as newlines, they are escaped with backslashes and the entire
reason is enclosed in double quotes. For example:
$ git worktree list --porcelain
...
locked "worktree's path mounted in\nremovable device"
...
Furthermore, let's update the documentation to state that some
attributes in the porcelain format might be listed alone or together
with its value depending whether the value is available or not. Thus
documenting the case of the new "locked" attribute.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
Documentation/git-worktree.txt | 16 ++++++++++++++--
builtin/worktree.c | 13 +++++++++++++
t/t2402-worktree-list.sh | 30 ++++++++++++++++++++++++++++++
3 files changed, 57 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 02a706c4c0..7cb8124f28 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -377,8 +377,10 @@ Porcelain Format
The porcelain format has a line per attribute. Attributes are listed with a
label and value separated by a single space. Boolean attributes (like `bare`
and `detached`) are listed as a label only, and are present only
-if the value is true. The first attribute of a working tree is always
-`worktree`, an empty line indicates the end of the record. For example:
+if the value is true. Some attributes (like `locked`) can be listed as a label
+only or with a value depending upon whether a reason is available. The first
+attribute of a working tree is always `worktree`, an empty line indicates the
+end of the record. For example:
------------
$ git worktree list --porcelain
@@ -393,6 +395,16 @@ worktree /path/to/other-linked-worktree
HEAD 1234abc1234abc1234abc1234abc1234abc1234a
detached
+worktree /path/to/linked-worktree-locked-no-reason
+HEAD 5678abc5678abc5678abc5678abc5678abc5678c
+branch refs/heads/locked-no-reason
+locked
+
+worktree /path/to/linked-worktree-locked-with-reason
+HEAD 3456def3456def3456def3456def3456def3456b
+branch refs/heads/locked-with-reason
+locked reason why is locked
+
------------
EXAMPLES
diff --git a/builtin/worktree.c b/builtin/worktree.c
index df90a5acca..98177f91d4 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -12,6 +12,7 @@
#include "submodule.h"
#include "utf8.h"
#include "worktree.h"
+#include "quote.h"
static const char * const worktree_usage[] = {
N_("git worktree add [<options>] <path> [<commit-ish>]"),
@@ -569,6 +570,8 @@ static int add(int ac, const char **av, const char *prefix)
static void show_worktree_porcelain(struct worktree *wt)
{
+ const char *reason;
+
printf("worktree %s\n", wt->path);
if (wt->is_bare)
printf("bare\n");
@@ -579,6 +582,16 @@ static void show_worktree_porcelain(struct worktree *wt)
else if (wt->head_ref)
printf("branch %s\n", wt->head_ref);
}
+
+ reason = worktree_lock_reason(wt);
+ if (reason && *reason) {
+ struct strbuf sb = STRBUF_INIT;
+ quote_c_style(reason, &sb, NULL, 0);
+ printf("locked %s\n", sb.buf);
+ strbuf_release(&sb);
+ } else if (reason)
+ printf("locked\n");
+
printf("\n");
}
diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index 1866ea09f6..1fe53c3309 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh
@@ -72,6 +72,36 @@ test_expect_success '"list" all worktrees with locked annotation' '
! grep "/unlocked *[0-9a-f].* locked$" out
'
+test_expect_success '"list" all worktrees --porcelain with locked' '
+ test_when_finished "rm -rf locked1 locked2 unlocked out actual expect && git worktree prune" &&
+ echo "locked" >expect &&
+ echo "locked with reason" >>expect &&
+ git worktree add --detach locked1 &&
+ git worktree add --detach locked2 &&
+ # unlocked worktree should not be annotated with "locked"
+ git worktree add --detach unlocked &&
+ git worktree lock locked1 &&
+ git worktree lock locked2 --reason "with reason" &&
+ test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" &&
+ git worktree list --porcelain >out &&
+ grep "^locked" out >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '"list" all worktrees --porcelain with locked reason newline escaped' '
+ test_when_finished "rm -rf locked_lf locked_crlf out actual expect && git worktree prune" &&
+ printf "locked \"locked\\\\r\\\\nreason\"\n" >expect &&
+ printf "locked \"locked\\\\nreason\"\n" >>expect &&
+ git worktree add --detach locked_lf &&
+ git worktree add --detach locked_crlf &&
+ git worktree lock locked_lf --reason "$(printf "locked\nreason")" &&
+ git worktree lock locked_crlf --reason "$(printf "locked\r\nreason")" &&
+ test_when_finished "git worktree unlock locked_lf && git worktree unlock locked_crlf" &&
+ git worktree list --porcelain >out &&
+ grep "^locked" out >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'bare repo setup' '
git init --bare bare1 &&
echo "data" >file1 &&
--
2.30.0.421.g32f838e276
^ permalink raw reply related [flat|nested] 98+ messages in thread* Re: [PATCH v3 5/7] worktree: teach `list --porcelain` to annotate locked worktree
2021-01-19 21:27 ` [PATCH v3 5/7] worktree: teach `list --porcelain` to annotate locked worktree Rafael Silva
@ 2021-01-20 11:00 ` Phillip Wood
2021-01-21 3:18 ` Junio C Hamano
` (2 more replies)
2021-01-24 8:10 ` Eric Sunshine
1 sibling, 3 replies; 98+ messages in thread
From: Phillip Wood @ 2021-01-20 11:00 UTC (permalink / raw)
To: Rafael Silva, git; +Cc: Eric Sunshine
Hi Rafael
Thanks for reworking this to use c_quote_path(). I have a couple of
comments below.
On 19/01/2021 21:27, Rafael Silva wrote:
> Commit c57b3367be (worktree: teach `list` to annotate locked worktree,
> 2020-10-11) taught "git worktree list" to annotate locked worktrees by
> appending "locked" text to its output, however, this is not listed in
> the --porcelain format.
>
> Teach "list --porcelain" to do the same and add a "locked" attribute
> followed by its reason, thus making both default and porcelain format
> consistent. If the locked reason is not available then only "locked"
> is shown.
>
> The output of the "git worktree list --porcelain" becomes like so:
>
> $ git worktree list --porcelain
> ...
> worktree /path/to/locked
> HEAD 123abcdea123abcd123acbd123acbda123abcd12
> detached
> locked
>
> worktree /path/to/locked-with-reason
> HEAD abc123abc123abc123abc123abc123abc123abc1
> detached
> locked reason why it is locked
> ...
>
> In porcelain mode, if the lock reason contains special characters
> such as newlines, they are escaped with backslashes and the entire
> reason is enclosed in double quotes. For example:
>
> $ git worktree list --porcelain
> ...
> locked "worktree's path mounted in\nremovable device"
> ...
>
> Furthermore, let's update the documentation to state that some
> attributes in the porcelain format might be listed alone or together
> with its value depending whether the value is available or not. Thus
> documenting the case of the new "locked" attribute.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
> ---
> Documentation/git-worktree.txt | 16 ++++++++++++++--
> builtin/worktree.c | 13 +++++++++++++
> t/t2402-worktree-list.sh | 30 ++++++++++++++++++++++++++++++
> 3 files changed, 57 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 02a706c4c0..7cb8124f28 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -377,8 +377,10 @@ Porcelain Format
> The porcelain format has a line per attribute. Attributes are listed with a
> label and value separated by a single space. Boolean attributes (like `bare`
> and `detached`) are listed as a label only, and are present only
> -if the value is true. The first attribute of a working tree is always
> -`worktree`, an empty line indicates the end of the record. For example:
> +if the value is true. Some attributes (like `locked`) can be listed as a label
> +only or with a value depending upon whether a reason is available. The first
> +attribute of a working tree is always `worktree`, an empty line indicates the
> +end of the record. For example:
I think it would be helpful to document that the reasons are quoted
according core.quotePath.
I'm not sure if it is worth changing this but I wonder if it would be
easier to parse the output if the names of attributes with optional
reasons were always followed by a space even when there is no reason,
otherwise the code that parses the output has to check for the name
followed by a space or newline. A script that only cares if the worktree
is locked can parse the output with
l.starts_with("locked ")
rather than having to do
l.starts_with("locked ") || l == "locked\n"
Best Wishes
Phillip
> ------------
> $ git worktree list --porcelain
> @@ -393,6 +395,16 @@ worktree /path/to/other-linked-worktree
> HEAD 1234abc1234abc1234abc1234abc1234abc1234a
> detached
>
> +worktree /path/to/linked-worktree-locked-no-reason
> +HEAD 5678abc5678abc5678abc5678abc5678abc5678c
> +branch refs/heads/locked-no-reason
> +locked
> +
> +worktree /path/to/linked-worktree-locked-with-reason
> +HEAD 3456def3456def3456def3456def3456def3456b
> +branch refs/heads/locked-with-reason
> +locked reason why is locked
> +
> ------------
>
> EXAMPLES
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index df90a5acca..98177f91d4 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -12,6 +12,7 @@
> #include "submodule.h"
> #include "utf8.h"
> #include "worktree.h"
> +#include "quote.h"
>
> static const char * const worktree_usage[] = {
> N_("git worktree add [<options>] <path> [<commit-ish>]"),
> @@ -569,6 +570,8 @@ static int add(int ac, const char **av, const char *prefix)
>
> static void show_worktree_porcelain(struct worktree *wt)
> {
> + const char *reason;
> +
> printf("worktree %s\n", wt->path);
> if (wt->is_bare)
> printf("bare\n");
> @@ -579,6 +582,16 @@ static void show_worktree_porcelain(struct worktree *wt)
> else if (wt->head_ref)
> printf("branch %s\n", wt->head_ref);
> }
> +
> + reason = worktree_lock_reason(wt);
> + if (reason && *reason) {
> + struct strbuf sb = STRBUF_INIT;
> + quote_c_style(reason, &sb, NULL, 0);
> + printf("locked %s\n", sb.buf);
> + strbuf_release(&sb);
> + } else if (reason)
> + printf("locked\n");
> +
> printf("\n");
> }
>
> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
> index 1866ea09f6..1fe53c3309 100755
> --- a/t/t2402-worktree-list.sh
> +++ b/t/t2402-worktree-list.sh
> @@ -72,6 +72,36 @@ test_expect_success '"list" all worktrees with locked annotation' '
> ! grep "/unlocked *[0-9a-f].* locked$" out
> '
>
> +test_expect_success '"list" all worktrees --porcelain with locked' '
> + test_when_finished "rm -rf locked1 locked2 unlocked out actual expect && git worktree prune" &&
> + echo "locked" >expect &&
> + echo "locked with reason" >>expect &&
> + git worktree add --detach locked1 &&
> + git worktree add --detach locked2 &&
> + # unlocked worktree should not be annotated with "locked"
> + git worktree add --detach unlocked &&
> + git worktree lock locked1 &&
> + git worktree lock locked2 --reason "with reason" &&
> + test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" &&
> + git worktree list --porcelain >out &&
> + grep "^locked" out >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success '"list" all worktrees --porcelain with locked reason newline escaped' '
> + test_when_finished "rm -rf locked_lf locked_crlf out actual expect && git worktree prune" &&
> + printf "locked \"locked\\\\r\\\\nreason\"\n" >expect &&
> + printf "locked \"locked\\\\nreason\"\n" >>expect &&
> + git worktree add --detach locked_lf &&
> + git worktree add --detach locked_crlf &&
> + git worktree lock locked_lf --reason "$(printf "locked\nreason")" &&
> + git worktree lock locked_crlf --reason "$(printf "locked\r\nreason")" &&
> + test_when_finished "git worktree unlock locked_lf && git worktree unlock locked_crlf" &&
> + git worktree list --porcelain >out &&
> + grep "^locked" out >actual &&
> + test_cmp expect actual
> +'
> +
> test_expect_success 'bare repo setup' '
> git init --bare bare1 &&
> echo "data" >file1 &&
>
^ permalink raw reply [flat|nested] 98+ messages in thread* Re: [PATCH v3 5/7] worktree: teach `list --porcelain` to annotate locked worktree
2021-01-20 11:00 ` Phillip Wood
@ 2021-01-21 3:18 ` Junio C Hamano
2021-01-21 15:25 ` Rafael Silva
2021-01-24 8:24 ` Eric Sunshine
2 siblings, 0 replies; 98+ messages in thread
From: Junio C Hamano @ 2021-01-21 3:18 UTC (permalink / raw)
To: Phillip Wood; +Cc: Rafael Silva, git, Eric Sunshine
Phillip Wood <phillip.wood123@gmail.com> writes:
> l.starts_with("locked ")
> rather than having to do
> l.starts_with("locked ") || l == "locked\n"
l.regexp_matches("^locked[\s\n]")?
Jokes aside, isn't the "space separated list" meant to be used more
like:
attrs = l.splitAtEach(" ")
if "locked" in attrs:
... yeah it is locked ...
if "broken" in attrs:
... ouch, it is broken ...
so I am not sure if having always a trailing whitespace is a good
idea to begin with (the last element may become an empty string if
the splitting is done naively).
^ permalink raw reply [flat|nested] 98+ messages in thread* Re: [PATCH v3 5/7] worktree: teach `list --porcelain` to annotate locked worktree
2021-01-20 11:00 ` Phillip Wood
2021-01-21 3:18 ` Junio C Hamano
@ 2021-01-21 15:25 ` Rafael Silva
2021-01-24 8:24 ` Eric Sunshine
2 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-21 15:25 UTC (permalink / raw)
To: phillip.wood; +Cc: git, Eric Sunshine
Hi Phillip,
Phillip Wood writes:
> Hi Rafael
>
> Thanks for reworking this to use c_quote_path(). I have a couple of
> comments below.
>
Thanks for reviewing this patch.
> On 19/01/2021 21:27, Rafael Silva wrote:
>> Commit c57b3367be (worktree: teach `list` to annotate locked worktree,
>> 2020-10-11) taught "git worktree list" to annotate locked worktrees by
>> appending "locked" text to its output, however, this is not listed in
>> the --porcelain format.
>> Teach "list --porcelain" to do the same and add a "locked" attribute
>> followed by its reason, thus making both default and porcelain format
>> consistent. If the locked reason is not available then only "locked"
>> is shown.
>> The output of the "git worktree list --porcelain" becomes like so:
>> $ git worktree list --porcelain
>> ...
>> worktree /path/to/locked
>> HEAD 123abcdea123abcd123acbd123acbda123abcd12
>> detached
>> locked
>> worktree /path/to/locked-with-reason
>> HEAD abc123abc123abc123abc123abc123abc123abc1
>> detached
>> locked reason why it is locked
>> ...
>> In porcelain mode, if the lock reason contains special characters
>> such as newlines, they are escaped with backslashes and the entire
>> reason is enclosed in double quotes. For example:
>> $ git worktree list --porcelain
>> ...
>> locked "worktree's path mounted in\nremovable device"
>> ...
>> Furthermore, let's update the documentation to state that some
>> attributes in the porcelain format might be listed alone or together
>> with its value depending whether the value is available or not. Thus
>> documenting the case of the new "locked" attribute.
>> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
>> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
>> ---
>> Documentation/git-worktree.txt | 16 ++++++++++++++--
>> builtin/worktree.c | 13 +++++++++++++
>> t/t2402-worktree-list.sh | 30 ++++++++++++++++++++++++++++++
>> 3 files changed, 57 insertions(+), 2 deletions(-)
>> diff --git a/Documentation/git-worktree.txt
>> b/Documentation/git-worktree.txt
>> index 02a706c4c0..7cb8124f28 100644
>> --- a/Documentation/git-worktree.txt
>> +++ b/Documentation/git-worktree.txt
>> @@ -377,8 +377,10 @@ Porcelain Format
>> The porcelain format has a line per attribute. Attributes are listed with a
>> label and value separated by a single space. Boolean attributes (like `bare`
>> and `detached`) are listed as a label only, and are present only
>> -if the value is true. The first attribute of a working tree is always
>> -`worktree`, an empty line indicates the end of the record. For example:
>> +if the value is true. Some attributes (like `locked`) can be listed as a label
>> +only or with a value depending upon whether a reason is available. The first
>> +attribute of a working tree is always `worktree`, an empty line indicates the
>> +end of the record. For example:
>
> I think it would be helpful to document that the reasons are quoted
> according core.quotePath.
>
Good point. I'll include this addition on the next revision.
--
Thanks
Rafael
^ permalink raw reply [flat|nested] 98+ messages in thread
* Re: [PATCH v3 5/7] worktree: teach `list --porcelain` to annotate locked worktree
2021-01-20 11:00 ` Phillip Wood
2021-01-21 3:18 ` Junio C Hamano
2021-01-21 15:25 ` Rafael Silva
@ 2021-01-24 8:24 ` Eric Sunshine
2 siblings, 0 replies; 98+ messages in thread
From: Eric Sunshine @ 2021-01-24 8:24 UTC (permalink / raw)
To: Phillip Wood; +Cc: Rafael Silva, Git List
On Wed, Jan 20, 2021 at 6:00 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 19/01/2021 21:27, Rafael Silva wrote:
> > The porcelain format has a line per attribute. Attributes are listed with a
> > label and value separated by a single space. Boolean attributes (like `bare`
> > and `detached`) are listed as a label only, and are present only
> > +if the value is true. Some attributes (like `locked`) can be listed as a label
> > +only or with a value depending upon whether a reason is available. The first
> > +attribute of a working tree is always `worktree`, an empty line indicates the
> > +end of the record. For example:
>
> I think it would be helpful to document that the reasons are quoted
> according core.quotePath.
Good idea.
> I'm not sure if it is worth changing this but I wonder if it would be
> easier to parse the output if the names of attributes with optional
> reasons were always followed by a space even when there is no reason,
> otherwise the code that parses the output has to check for the name
> followed by a space or newline. A script that only cares if the worktree
> is locked can parse the output with
> l.starts_with("locked ")
> rather than having to do
> l.starts_with("locked ") || l == "locked\n"
I see where you're coming from with this suggestion, though my
knee-jerk reaction is that it would be undesirable. Even after mulling
it over for a few days, I still haven't managed to convince myself
that it would be a good idea. There are a couple reasons (at least)
for my negative reaction. The primary reason is that the trailing
space is "invisible", and as such could end up being as confusing as
it is helpful for the simple parsing case (taking into consideration
that people often don't consult documentation). The second reason is
that we're already expecting clients to be able to parse C-style
quoting/escaping of the reason, so asking them to also distinguish
between a single token `locked` and a `locked reason-for-lock` seems
like very, very minor extra complexity. (It also just feels a bit
sloppy to have that trailing space, but that's a quite minor concern.)
^ permalink raw reply [flat|nested] 98+ messages in thread
* Re: [PATCH v3 5/7] worktree: teach `list --porcelain` to annotate locked worktree
2021-01-19 21:27 ` [PATCH v3 5/7] worktree: teach `list --porcelain` to annotate locked worktree Rafael Silva
2021-01-20 11:00 ` Phillip Wood
@ 2021-01-24 8:10 ` Eric Sunshine
2021-01-24 10:20 ` Rafael Silva
1 sibling, 1 reply; 98+ messages in thread
From: Eric Sunshine @ 2021-01-24 8:10 UTC (permalink / raw)
To: Rafael Silva; +Cc: Git List, Phillip Wood
On Tue, Jan 19, 2021 at 4:28 PM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> [...]
> Teach "list --porcelain" to do the same and add a "locked" attribute
> followed by its reason, thus making both default and porcelain format
> consistent. If the locked reason is not available then only "locked"
> is shown.
> [...]
> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
> ---
> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
> @@ -72,6 +72,36 @@ test_expect_success '"list" all worktrees with locked annotation' '
> +test_expect_success '"list" all worktrees --porcelain with locked' '
> + test_when_finished "rm -rf locked1 locked2 unlocked out actual expect && git worktree prune" &&
> + echo "locked" >expect &&
> + echo "locked with reason" >>expect &&
> + git worktree add --detach locked1 &&
> + git worktree add --detach locked2 &&
> + # unlocked worktree should not be annotated with "locked"
> + git worktree add --detach unlocked &&
> + git worktree lock locked1 &&
> + git worktree lock locked2 --reason "with reason" &&
> + test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" &&
There's a minor problem here. If the second `git worktree lock`
command fails, test_when_finished() will never be invoked, which means
that the first lock won't get cleaned up, thus the worktree won't get
pruned. To fix, you'd want:
git worktree lock locked1 &&
test_when_finished "git worktree unlock locked1" &&
git worktree lock locked2 --reason "with reason" &&
test_when_finished "git worktree unlock locked2" &&
> + git worktree list --porcelain >out &&
> + grep "^locked" out >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success '"list" all worktrees --porcelain with locked reason newline escaped' '
> + test_when_finished "rm -rf locked_lf locked_crlf out actual expect && git worktree prune" &&
> + printf "locked \"locked\\\\r\\\\nreason\"\n" >expect &&
> + printf "locked \"locked\\\\nreason\"\n" >>expect &&
> + git worktree add --detach locked_lf &&
> + git worktree add --detach locked_crlf &&
> + git worktree lock locked_lf --reason "$(printf "locked\nreason")" &&
> + git worktree lock locked_crlf --reason "$(printf "locked\r\nreason")" &&
> + test_when_finished "git worktree unlock locked_lf && git worktree unlock locked_crlf" &&
Same issue as above.
> + git worktree list --porcelain >out &&
> + grep "^locked" out >actual &&
> + test_cmp expect actual
> +'
^ permalink raw reply [flat|nested] 98+ messages in thread* Re: [PATCH v3 5/7] worktree: teach `list --porcelain` to annotate locked worktree
2021-01-24 8:10 ` Eric Sunshine
@ 2021-01-24 10:20 ` Rafael Silva
0 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-24 10:20 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Phillip Wood
Eric Sunshine writes:
> On Tue, Jan 19, 2021 at 4:28 PM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> [...]
>> Teach "list --porcelain" to do the same and add a "locked" attribute
>> followed by its reason, thus making both default and porcelain format
>> consistent. If the locked reason is not available then only "locked"
>> is shown.
>> [...]
>> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
>> ---
>> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
>> @@ -72,6 +72,36 @@ test_expect_success '"list" all worktrees with locked annotation' '
>> +test_expect_success '"list" all worktrees --porcelain with locked' '
>> + test_when_finished "rm -rf locked1 locked2 unlocked out actual expect && git worktree prune" &&
>> + echo "locked" >expect &&
>> + echo "locked with reason" >>expect &&
>> + git worktree add --detach locked1 &&
>> + git worktree add --detach locked2 &&
>> + # unlocked worktree should not be annotated with "locked"
>> + git worktree add --detach unlocked &&
>> + git worktree lock locked1 &&
>> + git worktree lock locked2 --reason "with reason" &&
>> + test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" &&
>
> There's a minor problem here. If the second `git worktree lock`
> command fails, test_when_finished() will never be invoked, which means
> that the first lock won't get cleaned up, thus the worktree won't get
> pruned. To fix, you'd want:
>
> git worktree lock locked1 &&
> test_when_finished "git worktree unlock locked1" &&
> git worktree lock locked2 --reason "with reason" &&
> test_when_finished "git worktree unlock locked2" &&
>
Excellent point. This case didn't occur to me when I was working on v3, I
will make this change in the next revision.
>> + git worktree list --porcelain >out &&
>> + grep "^locked" out >actual &&
>> + test_cmp expect actual
>> +'
>> +
>> +test_expect_success '"list" all worktrees --porcelain with locked reason newline escaped' '
>> + test_when_finished "rm -rf locked_lf locked_crlf out actual expect && git worktree prune" &&
>> + printf "locked \"locked\\\\r\\\\nreason\"\n" >expect &&
>> + printf "locked \"locked\\\\nreason\"\n" >>expect &&
>> + git worktree add --detach locked_lf &&
>> + git worktree add --detach locked_crlf &&
>> + git worktree lock locked_lf --reason "$(printf "locked\nreason")" &&
>> + git worktree lock locked_crlf --reason "$(printf "locked\r\nreason")" &&
>> + test_when_finished "git worktree unlock locked_lf && git worktree unlock locked_crlf" &&
>
> Same issue as above.
>
>> + git worktree list --porcelain >out &&
>> + grep "^locked" out >actual &&
>> + test_cmp expect actual
>> +'
--
Thanks
Rafael
^ permalink raw reply [flat|nested] 98+ messages in thread
* [PATCH v3 6/7] worktree: teach `list` to annotate prunable worktree
2021-01-19 21:27 ` [PATCH v3 0/7] " Rafael Silva
` (4 preceding siblings ...)
2021-01-19 21:27 ` [PATCH v3 5/7] worktree: teach `list --porcelain` to annotate locked worktree Rafael Silva
@ 2021-01-19 21:27 ` Rafael Silva
2021-01-21 3:28 ` Junio C Hamano
2021-01-19 21:27 ` [PATCH v3 7/7] worktree: teach `list` verbose mode Rafael Silva
` (2 subsequent siblings)
8 siblings, 1 reply; 98+ messages in thread
From: Rafael Silva @ 2021-01-19 21:27 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Rafael Silva
The "git worktree list" command shows the absolute path to the worktree,
the commit that is checked out, the name of the branch, and a "locked"
annotation if the worktree is locked, however, it does not indicate
whether the worktree is prunable.
The "prune" command will remove a worktree if it is prunable unless
`--dry-run` option is specified. This could lead to a worktree being
removed without the user realizing before it is too late, in case the
user forgets to pass --dry-run for instance. If the "list" command shows
which worktree is prunable, the user could verify before running
"git worktree prune" and hopefully prevents the working tree to be
removed "accidentally" on the worse case scenario.
Let's teach "git worktree list" to show when a worktree is a prunable
candidate for both default and porcelain format.
In the default format a "prunable" text is appended:
$ git worktree list
/path/to/main aba123 [main]
/path/to/linked 123abc [branch-a]
/path/to/prunable ace127 (detached HEAD) prunable
In the --porcelain format a prunable label is added followed by
its reason:
$ git worktree list --porcelain
...
worktree /path/to/prunable
HEAD abc1234abc1234abc1234abc1234abc1234abc12
detached
prunable gitdir file points to non-existent location
...
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
Documentation/git-worktree.txt | 26 ++++++++++++++++++++++++--
builtin/worktree.c | 10 ++++++++++
builtin/worktree.cc | 0
t/t2402-worktree-list.sh | 32 ++++++++++++++++++++++++++++++++
4 files changed, 66 insertions(+), 2 deletions(-)
create mode 100644 builtin/worktree.cc
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 7cb8124f28..e0dd291e30 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -97,8 +97,9 @@ list::
List details of each working tree. The main working tree is listed first,
followed by each of the linked working trees. The output details include
whether the working tree is bare, the revision currently checked out, the
-branch currently checked out (or "detached HEAD" if none), and "locked" if
-the worktree is locked.
+branch currently checked out (or "detached HEAD" if none), "locked" if
+the worktree is locked, "prunable" if the worktree can be pruned by `prune`
+command.
lock::
@@ -234,6 +235,9 @@ This can also be set up as the default behaviour by using the
--expire <time>::
With `prune`, only expire unused working trees older than `<time>`.
++
+With `list`, annotate missing working trees as prunable if they are
+older than `<time>`.
--reason <string>::
With `lock`, an explanation why the working tree is locked.
@@ -372,6 +376,19 @@ $ git worktree list
/path/to/other-linked-worktree 1234abc (detached HEAD)
------------
+The command also shows annotations for each working tree, according to its state.
+These annotations are:
+
+ * `locked`, if the working tree is locked.
+ * `prunable`, if the working tree can be pruned via `git worktree prune`.
+
+------------
+$ git worktree list
+/path/to/linked-worktree abcd1234 [master]
+/path/to/locked-worktreee acbd5678 (brancha) locked
+/path/to/prunable-worktree 5678abc (detached HEAD) prunable
+------------
+
Porcelain Format
~~~~~~~~~~~~~~~~
The porcelain format has a line per attribute. Attributes are listed with a
@@ -405,6 +422,11 @@ HEAD 3456def3456def3456def3456def3456def3456b
branch refs/heads/locked-with-reason
locked reason why is locked
+worktree /path/to/linked-worktree-prunable
+HEAD 1233def1234def1234def1234def1234def1234b
+detached
+prunable gitdir file points to non-existent location
+
------------
EXAMPLES
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 98177f91d4..20944c266e 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -592,6 +592,10 @@ static void show_worktree_porcelain(struct worktree *wt)
} else if (reason)
printf("locked\n");
+ reason = worktree_prune_reason(wt, expire);
+ if (reason)
+ printf("prunable %s\n", reason);
+
printf("\n");
}
@@ -620,6 +624,9 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
if (worktree_lock_reason(wt))
strbuf_addstr(&sb, " locked");
+ if (worktree_prune_reason(wt, expire))
+ strbuf_addstr(&sb, " prunable");
+
printf("%s\n", sb.buf);
strbuf_release(&sb);
}
@@ -663,9 +670,12 @@ static int list(int ac, const char **av, const char *prefix)
struct option options[] = {
OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
+ OPT_EXPIRY_DATE(0, "expire", &expire,
+ N_("add 'prunable' annotation to worktrees older than <time>")),
OPT_END()
};
+ expire = TIME_MAX;
ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
if (ac)
usage_with_options(worktree_usage, options);
diff --git a/builtin/worktree.cc b/builtin/worktree.cc
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index 1fe53c3309..9e342d6c66 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh
@@ -102,6 +102,38 @@ test_expect_success '"list" all worktrees --porcelain with locked reason newline
test_cmp expect actual
'
+test_expect_success '"list" all worktrees with prunable annotation' '
+ test_when_finished "rm -rf prunable unprunable out && git worktree prune" &&
+ git worktree add --detach prunable &&
+ git worktree add --detach unprunable &&
+ rm -rf prunable &&
+ git worktree list >out &&
+ grep "/prunable *[0-9a-f].* prunable$" out &&
+ ! grep "/unprunable *[0-9a-f].* prunable$"
+'
+
+test_expect_success '"list" all worktrees --porcelain with prunable' '
+ test_when_finished "rm -rf prunable out && git worktree prune" &&
+ git worktree add --detach prunable &&
+ rm -rf prunable &&
+ git worktree list --porcelain >out &&
+ sed -n "/^worktree .*\/prunable$/,/^$/p" <out >only_prunable &&
+ test_i18ngrep "^prunable gitdir file points to non-existent location$" only_prunable
+'
+
+test_expect_success '"list" all worktrees with prunable consistent with "prune"' '
+ test_when_finished "rm -rf prunable unprunable out && git worktree prune" &&
+ git worktree add --detach prunable &&
+ git worktree add --detach unprunable &&
+ rm -rf prunable &&
+ git worktree list >out &&
+ grep "/prunable *[0-9a-f].* prunable$" out &&
+ ! grep "/unprunable *[0-9a-f].* unprunable$" out &&
+ git worktree prune --verbose >out &&
+ test_i18ngrep "^Removing worktrees/prunable" out &&
+ test_i18ngrep ! "^Removing worktrees/unprunable" out
+'
+
test_expect_success 'bare repo setup' '
git init --bare bare1 &&
echo "data" >file1 &&
--
2.30.0.421.g32f838e276
^ permalink raw reply related [flat|nested] 98+ messages in thread* Re: [PATCH v3 6/7] worktree: teach `list` to annotate prunable worktree
2021-01-19 21:27 ` [PATCH v3 6/7] worktree: teach `list` to annotate prunable worktree Rafael Silva
@ 2021-01-21 3:28 ` Junio C Hamano
2021-01-21 15:09 ` Rafael Silva
0 siblings, 1 reply; 98+ messages in thread
From: Junio C Hamano @ 2021-01-21 3:28 UTC (permalink / raw)
To: Rafael Silva; +Cc: git, Eric Sunshine, Phillip Wood
Rafael Silva <rafaeloliveira.cs@gmail.com> writes:
> Documentation/git-worktree.txt | 26 ++++++++++++++++++++++++--
> builtin/worktree.c | 10 ++++++++++
> builtin/worktree.cc | 0
> t/t2402-worktree-list.sh | 32 ++++++++++++++++++++++++++++++++
> 4 files changed, 66 insertions(+), 2 deletions(-)
> create mode 100644 builtin/worktree.cc
What's the h*ck is that .cc file doing ;-)
^ permalink raw reply [flat|nested] 98+ messages in thread
* Re: [PATCH v3 6/7] worktree: teach `list` to annotate prunable worktree
2021-01-21 3:28 ` Junio C Hamano
@ 2021-01-21 15:09 ` Rafael Silva
2021-01-21 22:18 ` Junio C Hamano
0 siblings, 1 reply; 98+ messages in thread
From: Rafael Silva @ 2021-01-21 15:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Eric Sunshine, Phillip Wood
Junio C Hamano writes:
> Rafael Silva <rafaeloliveira.cs@gmail.com> writes:
>
>> Documentation/git-worktree.txt | 26 ++++++++++++++++++++++++--
>> builtin/worktree.c | 10 ++++++++++
>> builtin/worktree.cc | 0
>> t/t2402-worktree-list.sh | 32 ++++++++++++++++++++++++++++++++
>> 4 files changed, 66 insertions(+), 2 deletions(-)
>> create mode 100644 builtin/worktree.cc
>
> What's the h*ck is that .cc file doing ;-)
Oops. I accidentally created and committed this file.
Re-rolling ...
Side note (joke): I wasn't trying to add C++ into Git codebase :)
--
Thanks
Rafael
^ permalink raw reply [flat|nested] 98+ messages in thread
* Re: [PATCH v3 6/7] worktree: teach `list` to annotate prunable worktree
2021-01-21 15:09 ` Rafael Silva
@ 2021-01-21 22:18 ` Junio C Hamano
0 siblings, 0 replies; 98+ messages in thread
From: Junio C Hamano @ 2021-01-21 22:18 UTC (permalink / raw)
To: Rafael Silva; +Cc: git, Eric Sunshine, Phillip Wood
Rafael Silva <rafaeloliveira.cs@gmail.com> writes:
> Junio C Hamano writes:
>
>> Rafael Silva <rafaeloliveira.cs@gmail.com> writes:
>>
>>> Documentation/git-worktree.txt | 26 ++++++++++++++++++++++++--
>>> builtin/worktree.c | 10 ++++++++++
>>> builtin/worktree.cc | 0
>>> t/t2402-worktree-list.sh | 32 ++++++++++++++++++++++++++++++++
>>> 4 files changed, 66 insertions(+), 2 deletions(-)
>>> create mode 100644 builtin/worktree.cc
>>
>> What's the h*ck is that .cc file doing ;-)
>
> Oops. I accidentally created and committed this file.
>
> Re-rolling ...
>
> Side note (joke): I wasn't trying to add C++ into Git codebase :)
Yeah, I know ;-) I've already removed the empty file in my copy,
but it seems you are getting other useful input, so it probably is a
good idea to remove it on your end, too.
Thanks.
^ permalink raw reply [flat|nested] 98+ messages in thread
* [PATCH v3 7/7] worktree: teach `list` verbose mode
2021-01-19 21:27 ` [PATCH v3 0/7] " Rafael Silva
` (5 preceding siblings ...)
2021-01-19 21:27 ` [PATCH v3 6/7] worktree: teach `list` to annotate prunable worktree Rafael Silva
@ 2021-01-19 21:27 ` Rafael Silva
2021-01-24 8:42 ` Eric Sunshine
2021-01-24 8:51 ` [PATCH v3 0/7] teach `worktree list` verbose mode and prunable annotations Eric Sunshine
2021-01-27 8:03 ` Rafael Silva
8 siblings, 1 reply; 98+ messages in thread
From: Rafael Silva @ 2021-01-19 21:27 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Rafael Silva
"git worktree list" annotates each worktree according to its state such
as "prunable" or "locked", however it is not immediately obvious why
these worktrees are being annotated. For prunable worktrees a reason
is available that is returned by should_prune_worktree() and for locked
worktrees a reason might be available provided by the user via `lock`
command.
Let's teach "git worktree list" a --verbose mode that outputs the reason
why the worktrees are being annotated. The reason is a text that can take
virtually any size and appending the text on the default columned format
will make it difficult to extend the command with other annotations and
not fit nicely on the screen. In order to address this shortcoming the
annotation is then moved to the next line indented followed by the reason
If the reason is not available the annotation stays on the same line as
the worktree itself.
The output of "git worktree list" with verbose becomes like so:
$ git worktree list --verbose
...
/path/to/locked-no-reason acb124 [branch-a] locked
/path/to/locked-with-reason acc125 [branch-b]
locked: worktree with a locked reason
/path/to/prunable-reason ace127 [branch-d]
prunable: gitdir file points to non-existent location
...
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
Documentation/git-worktree.txt | 20 ++++++++++++++++++++
builtin/worktree.c | 14 ++++++++++++--
t/t2402-worktree-list.sh | 30 ++++++++++++++++++++++++++++++
3 files changed, 62 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index e0dd291e30..27f79ce638 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -232,6 +232,8 @@ This can also be set up as the default behaviour by using the
-v::
--verbose::
With `prune`, report all removals.
++
+With `list`, output additional information about worktrees (see below).
--expire <time>::
With `prune`, only expire unused working trees older than `<time>`.
@@ -389,6 +391,24 @@ $ git worktree list
/path/to/prunable-worktree 5678abc (detached HEAD) prunable
------------
+For these annotations, a reason might also be available and this can be
+seen using the verbose mode. The annotation is then moved to the next line
+indented followed by the additional information.
+
+------------
+$ git worktree list --verbose
+/path/to/linked-worktree abcd1234 [master]
+/path/to/locked-worktree-no-reason abcd5678 (detached HEAD) locked
+/path/to/locked-worktree-with-reason 1234abcd (brancha)
+ locked: working tree path is mounted on a portable device
+/path/to/prunable-worktree 5678abc1 (detached HEAD)
+ prunable: gitdir file points to non-existent location
+------------
+
+Note that the annotation is moved to the next line if the additional
+information is available, otherwise it stays on the same line as the
+working tree itself.
+
Porcelain Format
~~~~~~~~~~~~~~~~
The porcelain format has a line per attribute. Attributes are listed with a
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 20944c266e..1cd5c2016e 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -604,6 +604,7 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
struct strbuf sb = STRBUF_INIT;
int cur_path_len = strlen(wt->path);
int path_adj = cur_path_len - utf8_strwidth(wt->path);
+ const char *reason;
strbuf_addf(&sb, "%-*s ", 1 + path_maxlen + path_adj, wt->path);
if (wt->is_bare)
@@ -621,10 +622,16 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
strbuf_addstr(&sb, "(error)");
}
- if (worktree_lock_reason(wt))
+ reason = worktree_lock_reason(wt);
+ if (verbose && reason && *reason)
+ strbuf_addf(&sb, "\n\tlocked: %s", reason);
+ else if (reason)
strbuf_addstr(&sb, " locked");
- if (worktree_prune_reason(wt, expire))
+ reason = worktree_prune_reason(wt, expire);
+ if (verbose && reason)
+ strbuf_addf(&sb, "\n\tprunable: %s", reason);
+ else if (reason)
strbuf_addstr(&sb, " prunable");
printf("%s\n", sb.buf);
@@ -670,6 +677,7 @@ static int list(int ac, const char **av, const char *prefix)
struct option options[] = {
OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
+ OPT__VERBOSE(&verbose, N_("show extended annotations and reasons, if available")),
OPT_EXPIRY_DATE(0, "expire", &expire,
N_("add 'prunable' annotation to worktrees older than <time>")),
OPT_END()
@@ -679,6 +687,8 @@ static int list(int ac, const char **av, const char *prefix)
ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
if (ac)
usage_with_options(worktree_usage, options);
+ else if (verbose && porcelain)
+ die(_("--verbose and --porcelain are mutually exclusive"));
else {
struct worktree **worktrees = get_worktrees();
int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index 9e342d6c66..6346cd38a8 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh
@@ -134,6 +134,36 @@ test_expect_success '"list" all worktrees with prunable consistent with "prune"'
test_i18ngrep ! "^Removing worktrees/unprunable" out
'
+test_expect_success '"list" --verbose and --porcelain mutually exclusive' '
+ test_must_fail git worktree list --verbose --porcelain
+'
+
+test_expect_success '"list" all worktrees --verbose with locked' '
+ test_when_finished "rm -rf locked1 locked2 out actual expect && git worktree prune" &&
+ git worktree add locked1 --detach &&
+ git worktree add locked2 --detach &&
+ git worktree lock locked1 &&
+ git worktree lock locked2 --reason "with reason" &&
+ test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" &&
+ echo "$(git -C locked2 rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
+ printf "\tlocked: with reason\n" >>expect &&
+ git worktree list --verbose >out &&
+ grep "/locked1 *[0-9a-f].* locked$" out &&
+ sed -n "s/ */ /g;/\/locked2 *[0-9a-f].*$/,/locked: .*$/p" <out >actual &&
+ test_cmp actual expect
+'
+
+test_expect_success '"list" all worktrees --verbose with prunable' '
+ test_when_finished "rm -rf prunable out actual expect && git worktree prune" &&
+ git worktree add prunable --detach &&
+ echo "$(git -C prunable rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
+ printf "\tprunable: gitdir file points to non-existent location\n" >>expect &&
+ rm -rf prunable &&
+ git worktree list --verbose >out &&
+ sed -n "s/ */ /g;/\/prunable *[0-9a-f].*$/,/prunable: .*$/p" <out >actual &&
+ test_i18ncmp actual expect
+'
+
test_expect_success 'bare repo setup' '
git init --bare bare1 &&
echo "data" >file1 &&
--
2.30.0.421.g32f838e276
^ permalink raw reply related [flat|nested] 98+ messages in thread* Re: [PATCH v3 7/7] worktree: teach `list` verbose mode
2021-01-19 21:27 ` [PATCH v3 7/7] worktree: teach `list` verbose mode Rafael Silva
@ 2021-01-24 8:42 ` Eric Sunshine
2021-01-24 10:21 ` Rafael Silva
0 siblings, 1 reply; 98+ messages in thread
From: Eric Sunshine @ 2021-01-24 8:42 UTC (permalink / raw)
To: Rafael Silva; +Cc: Git List, Phillip Wood
On Tue, Jan 19, 2021 at 4:28 PM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> [...]
> Let's teach "git worktree list" a --verbose mode that outputs the reason
> why the worktrees are being annotated. The reason is a text that can take
> virtually any size and appending the text on the default columned format
> will make it difficult to extend the command with other annotations and
> not fit nicely on the screen. In order to address this shortcoming the
> annotation is then moved to the next line indented followed by the reason
> If the reason is not available the annotation stays on the same line as
> the worktree itself.
> [...]
> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
> ---
> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
> @@ -134,6 +134,36 @@ test_expect_success '"list" all worktrees with prunable consistent with "prune"'
> +test_expect_success '"list" all worktrees --verbose with locked' '
> + test_when_finished "rm -rf locked1 locked2 out actual expect && git worktree prune" &&
> + git worktree add locked1 --detach &&
> + git worktree add locked2 --detach &&
> + git worktree lock locked1 &&
> + git worktree lock locked2 --reason "with reason" &&
> + test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" &&
Same minor problem here as mentioned in my review of [5/7]: If locking
of the second worktree fails then test_when_finished() won't get
invoked, so the first worktree won't get unlocked, thus won't be
pruned. To fix:
git worktree lock locked1 &&
test_when_finished "git worktree unlock locked1" &&
git worktree lock locked2 --reason "with reason" &&
test_when_finished "git worktree unlock locked2" &&
> + echo "$(git -C locked2 rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
> + printf "\tlocked: with reason\n" >>expect &&
> + git worktree list --verbose >out &&
> + grep "/locked1 *[0-9a-f].* locked$" out &&
> + sed -n "s/ */ /g;/\/locked2 *[0-9a-f].*$/,/locked: .*$/p" <out >actual &&
> + test_cmp actual expect
> +'
^ permalink raw reply [flat|nested] 98+ messages in thread* Re: [PATCH v3 7/7] worktree: teach `list` verbose mode
2021-01-24 8:42 ` Eric Sunshine
@ 2021-01-24 10:21 ` Rafael Silva
0 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-24 10:21 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Phillip Wood
Eric Sunshine writes:
> On Tue, Jan 19, 2021 at 4:28 PM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> [...]
>> Let's teach "git worktree list" a --verbose mode that outputs the reason
>> why the worktrees are being annotated. The reason is a text that can take
>> virtually any size and appending the text on the default columned format
>> will make it difficult to extend the command with other annotations and
>> not fit nicely on the screen. In order to address this shortcoming the
>> annotation is then moved to the next line indented followed by the reason
>> If the reason is not available the annotation stays on the same line as
>> the worktree itself.
>> [...]
>> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
>> ---
>> diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
>> @@ -134,6 +134,36 @@ test_expect_success '"list" all worktrees with prunable consistent with "prune"'
>> +test_expect_success '"list" all worktrees --verbose with locked' '
>> + test_when_finished "rm -rf locked1 locked2 out actual expect && git worktree prune" &&
>> + git worktree add locked1 --detach &&
>> + git worktree add locked2 --detach &&
>> + git worktree lock locked1 &&
>> + git worktree lock locked2 --reason "with reason" &&
>> + test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" &&
>
> Same minor problem here as mentioned in my review of [5/7]: If locking
> of the second worktree fails then test_when_finished() won't get
> invoked, so the first worktree won't get unlocked, thus won't be
> pruned. To fix:
>
> git worktree lock locked1 &&
> test_when_finished "git worktree unlock locked1" &&
> git worktree lock locked2 --reason "with reason" &&
> test_when_finished "git worktree unlock locked2" &&
>
Make sense. Thank you.
--
Thanks
Rafael
^ permalink raw reply [flat|nested] 98+ messages in thread
* Re: [PATCH v3 0/7] teach `worktree list` verbose mode and prunable annotations
2021-01-19 21:27 ` [PATCH v3 0/7] " Rafael Silva
` (6 preceding siblings ...)
2021-01-19 21:27 ` [PATCH v3 7/7] worktree: teach `list` verbose mode Rafael Silva
@ 2021-01-24 8:51 ` Eric Sunshine
2021-01-27 8:08 ` Rafael Silva
2021-01-27 8:03 ` Rafael Silva
8 siblings, 1 reply; 98+ messages in thread
From: Eric Sunshine @ 2021-01-24 8:51 UTC (permalink / raw)
To: Rafael Silva; +Cc: Git List, Phillip Wood
On Tue, Jan 19, 2021 at 4:28 PM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> Changes in v3
> =============
> v3 is 1-patch bigger than v2 and it includes the following changes:
Thanks. I gave v3 a complete read-through, and it is just about perfect.
Aside from a very minor problem with the tests in [5/7] and [7/7] and
the documentation enhancement (`core.quotePath`) suggested by Phillip,
this is really, really close. (In fact, I would be happy to accept v3
as-is; those minor changes could easily be done on top.)
I look forward to giving v4 my Reviewed-by:.
> Range-diff against v2:
> -: ---------- > 1: 66cd83ba42 worktree: libify should_prune_worktree()
> 1: 9d47fcb4a4 ! 2: 81f4824028 worktree: teach worktree to lazy-load "prunable" reason
The range-diff seems slightly off. The "libify
should_prune_worktree()" patch existed in v1 and v2, but the
range-diff suggests that it's new with v3. You probably just need a
minor adjustment to your `git format-patch --range-diff` invocation.
^ permalink raw reply [flat|nested] 98+ messages in thread* Re: [PATCH v3 0/7] teach `worktree list` verbose mode and prunable annotations
2021-01-24 8:51 ` [PATCH v3 0/7] teach `worktree list` verbose mode and prunable annotations Eric Sunshine
@ 2021-01-27 8:08 ` Rafael Silva
0 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-27 8:08 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Phillip Wood
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Tue, Jan 19, 2021 at 4:28 PM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> Changes in v3
>> =============
>> v3 is 1-patch bigger than v2 and it includes the following changes:
>
> Thanks. I gave v3 a complete read-through, and it is just about perfect.
>
> Aside from a very minor problem with the tests in [5/7] and [7/7] and
> the documentation enhancement (`core.quotePath`) suggested by Phillip,
> this is really, really close. (In fact, I would be happy to accept v3
> as-is; those minor changes could easily be done on top.)
>
> I look forward to giving v4 my Reviewed-by:.
>
Thanks again for reviewing the series. I've just sent the v4 and I'm
looking forward to your review.
>> Range-diff against v2:
>> -: ---------- > 1: 66cd83ba42 worktree: libify should_prune_worktree()
>> 1: 9d47fcb4a4 ! 2: 81f4824028 worktree: teach worktree to lazy-load "prunable" reason
>
> The range-diff seems slightly off. The "libify
> should_prune_worktree()" patch existed in v1 and v2, but the
> range-diff suggests that it's new with v3. You probably just need a
> minor adjustment to your `git format-patch --range-diff` invocation.
Thanks for pointing this out, I didn't notice this from the v3 range-diff.
--
Thanks
Rafael
^ permalink raw reply [flat|nested] 98+ messages in thread
* [PATCH v3 0/7] teach `worktree list` verbose mode and prunable annotations
2021-01-19 21:27 ` [PATCH v3 0/7] " Rafael Silva
` (7 preceding siblings ...)
2021-01-24 8:51 ` [PATCH v3 0/7] teach `worktree list` verbose mode and prunable annotations Eric Sunshine
@ 2021-01-27 8:03 ` Rafael Silva
2021-01-27 8:03 ` [PATCH v4 1/7] worktree: libify should_prune_worktree() Rafael Silva
` (7 more replies)
8 siblings, 8 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-27 8:03 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Junio C Hamano, Rafael Silva
In c57b3367be (worktree: teach `list` to annotate locked worktree,
2020-10-11) we taught `git worktree list` to annotate working trees that
is locked by appending "locked" text in order to signalize to the user
that a working tree is locked. During the review, there was some
discussion about additional annotations and information that `list`
command could provide to the user that has long been envisioned and
mentioned in [2], [3] and [4].
This patch series addresses some of these changes by teaching
`worktree list` to show "prunable" annotation, adding verbose mode and
extending the --porcelain format with prunable and locked annotation as
follow up from [1]. Additionally, it addresses one shortcoming for porcelain
format to escape any newline characters (LF or CRLF) and quote the lock
reason to prevent breaking the format that is mentioned in [4] and [1] during
the review cycle.
This patch series includes:
1. The first patch moves the should_prune_worktree() machinery to the top-level
worktree.c exposing the function as general API, that will be reference
by should_prune_worktree() wrapper implemented on the second patch.
The original idea was to not only move should_prune_worktree() but also
refactor to accept a "struct worktree" and load the information directly,
which allows simplify the `prune` command by reusing get_worktrees().
However this seems to also require refactoring get_worktrees() itself
to return "non-valid" working trees that can/should be pruned. This is
also mentioned in [5]. Having the wrapper function makes it easier to add
the prunable annotation without touching the get_worktrees() and the
other worktree sub commands. The refactoring can be addressed in a
future patch, if this turns out to be good idea. One possible approach
is to teach get_worktrees() to take additional flags that will tell
whether to return only valid or all worktrees in GIT_DIR/worktrees/
directory and address its own possible shortcoming.
2. Introduces the worktree_prune_reason() to discovery whether a worktree
is prunable along with two new fields in the `worktree` structure. The
function mimics the workree_lock_reason() API.
3. The third patch changes worktree_lock_reason() to be more gentle for
the main working tree to simply returning NULL instead of aborting the
program via assert() macro. This allow us to simplify the code that
checks if the working tree is locked for default and porcelain format.
This changes is also mentioned in [6].
4. Fix t2402 added in [1] to ensure the locked worktree is properly cleaned up.
5. The fourth patch adds the "locked" attribute for the porcelain format
in order to make both default and --porcelain format consistent.
6. The fifth patch introduces "prunable" annotation for both default
and --porcelain format.
7. The sixth patch introduces verbose mode to expand the `list` default
format and show each annotation reason when its available.
Series is built on top of 66e871b664 (The third batch, 2021-01-15).
[1] https://lore.kernel.org/git/20200928154953.30396-1-rafaeloliveira.cs@gmail.com/
[2] https://lore.kernel.org/git/CAPig+cQF6V8HNdMX5AZbmz3_w2WhSfA4SFfNhQqxXBqPXTZL+w@mail.gmail.com/
[3] https://lore.kernel.org/git/CAPig+cSGXqJuaZPhUhOVX5X=LMrjVfv8ye_6ncMUbyKox1i7QA@mail.gmail.com/
[4] https://lore.kernel.org/git/CAPig+cTitWCs5vB=0iXuUyEY22c0gvjXvY1ZtTT90s74ydhE=A@mail.gmail.com/
[5] https://lore.kernel.org/git/CACsJy8ChM99n6skQCv-GmFiod19mnwwH4j-6R+cfZSiVFAxjgA@mail.gmail.com/
[6] https://lore.kernel.org/git/xmqq8sctlgzx.fsf@gitster.c.googlers.com/
Changes in v4
=============
Thanks Eric Sunshine and Phillip Wood for the review and suggestions.
* Added documentation to explain that the lock reason is quoted with
the same rules as described for `core.quotePath`.
* The `worktree unlock` issued in the test cleanup is splitted and
executed after each `worktree lock` to ensure the unlock is only
done after we know each locked command was successful.
* Fix a couple of grammos in the [4/7] commit message.
Changes in v3
=============
v3 is 1-patch bigger than v2 and it includes the changes:
* Dropped CQUOTE_NODQ flag in the the locked reason to return a string
enclosed with double quotes if the text reason contains especial
characters, like newlines.
* fix in t2402 to ensure the locked worktree is properly cleaned up,
is move to its own patch.
* In worktree_prune_reason(), the `path` variable is initialize
with NULL to make the code easier to follow.
* The removal of `is_main_worktree()` before `worktree_lock_reason()`
is moved to the patch that actually changes the API to be more gentle
with the main worktree instead of refactoring the code in the next
patches that adds the annotations.
* Drop the `*reason` in `(reason && *reason)` as we know that
worktree_prune_reason() is either going to return a non-empty string
or NULL which makes the code easier to follow.
* The --verbose test for the locked annotation now tests that "locked"
annotation stays on the same line when there is no locked reason.
* Small documentation updates and make the "git worktree --verbose"
example a little consistent with the worktree path.
Changes in v2
=============
v2 changes considerably from v1 taking into account several comments
suggested by Eric Sunshine and Phillip Wood (thanks guys for the
insightful comments and patience reviewing my patches :) ).
* The biggest change are the way the series is organized. In v1 all the
code was implemented in the fourth patch, all the tests and documentation
updates was added in sixth and seventh patch respectfully, in v2 each
patch introduces the annotations and verbose mode together with theirs
respective test and documentation updates.
* Several rewrite of the commit messages
* In v1 the fifth patch was introducing a new function to escape newline
characters for the "locked" attribute. However, we already have this
feature from the quote.h API. So the new function is dropped and the
changes are squashed into v2's fourth patch.
* The new prunable annotation and locked annotation that was introduced
by [1] was refactor to not poke around the worktree private fields.
* Refactoring of the worktree_prune_reason() to cache the prune_reason
and refactor to early return the use cases following the more common
pattern used on Git's codebase.
* Few documentation rewrites, most notably the `--verbose` and `--expire`
doc sentences for the list command are moved to its own line to clearly
separate the description from the others commands instead of continuing
on the same paragraph.
* The `git unlock <worktree>` used in the test's cleanup is moved to after
we know the `git worktree locked` is executed successfully. This ensures
the unlock doesn't error in the cleanup stage which will make it harder
to debug the tests.
Rafael Silva (7):
worktree: libify should_prune_worktree()
worktree: teach worktree to lazy-load "prunable" reason
worktree: teach worktree_lock_reason() to gently handle main worktree
t2402: ensure locked worktree is properly cleaned up
worktree: teach `list --porcelain` to annotate locked worktree
worktree: teach `list` to annotate prunable worktree
worktree: teach `list` verbose mode
Documentation/git-worktree.txt | 74 ++++++++++++++++++++--
builtin/worktree.c | 110 +++++++++++----------------------
t/t2402-worktree-list.sh | 96 ++++++++++++++++++++++++++++
worktree.c | 91 ++++++++++++++++++++++++++-
worktree.h | 23 +++++++
5 files changed, 314 insertions(+), 80 deletions(-)
Range-diff against v3:
1: fc4252c129 = 1: 66cd83ba42 worktree: libify should_prune_worktree()
2: 664d84155b = 2: 81f4824028 worktree: teach worktree to lazy-load "prunable" reason
3: cf334da2f0 = 3: c62ecf60d6 worktree: teach worktree_lock_reason() to gently handle main worktree
4: ecb11793d9 ! 4: d2ea467927 t2402: ensure locked worktree is properly cleaned up
@@ Metadata
## Commit message ##
t2402: ensure locked worktree is properly cleaned up
- In c57b3367be (worktree: teach `list` to annotate locked worktree,
+ c57b3367be (worktree: teach `list` to annotate locked worktree,
2020-10-11) introduced a new test to ensure locked worktrees are listed
with "locked" annotation. However, the test does not clean up after
itself as "git worktree prune" is not going to remove the locked worktree
in the first place. This not only leaves the test in an unclean state it
- also potentially breaks following tests that relies on the
+ also potentially breaks following tests that rely on the
"git worktree list" output.
Let's fix that by unlocking the worktree before the "prune" command.
5: 1fb68562f6 ! 5: 1b7661a0b3 worktree: teach `list --porcelain` to annotate locked worktree
@@ Commit message
with its value depending whether the value is available or not. Thus
documenting the case of the new "locked" attribute.
+ Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
@@ Documentation/git-worktree.txt: worktree /path/to/other-linked-worktree
+branch refs/heads/locked-with-reason
+locked reason why is locked
+
++------------
++
++If the lock reason contains "unusual" characters such as newline, they
++are escaped and the entire reason is quoted as explained for the
++configuration variable `core.quotePath` (see linkgit:git-config[1]).
++For Example:
++
++------------
++$ git worktree list --porcelain
++...
++locked "reason\nwhy is locked"
++...
------------
EXAMPLES
@@ t/t2402-worktree-list.sh: test_expect_success '"list" all worktrees with locked
+ # unlocked worktree should not be annotated with "locked"
+ git worktree add --detach unlocked &&
+ git worktree lock locked1 &&
++ test_when_finished "git worktree unlock locked1" &&
+ git worktree lock locked2 --reason "with reason" &&
-+ test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" &&
++ test_when_finished "git worktree unlock locked2" &&
+ git worktree list --porcelain >out &&
+ grep "^locked" out >actual &&
+ test_cmp expect actual
@@ t/t2402-worktree-list.sh: test_expect_success '"list" all worktrees with locked
+ git worktree add --detach locked_lf &&
+ git worktree add --detach locked_crlf &&
+ git worktree lock locked_lf --reason "$(printf "locked\nreason")" &&
++ test_when_finished "git worktree unlock locked_lf" &&
+ git worktree lock locked_crlf --reason "$(printf "locked\r\nreason")" &&
-+ test_when_finished "git worktree unlock locked_lf && git worktree unlock locked_crlf" &&
++ test_when_finished "git worktree unlock locked_crlf" &&
+ git worktree list --porcelain >out &&
+ grep "^locked" out >actual &&
+ test_cmp expect actual
6: 834584f826 = 6: c29ad7ffb1 worktree: teach `list` to annotate prunable worktree
7: 35ad456fbb ! 7: 7938950d04 worktree: teach `list` verbose mode
@@ t/t2402-worktree-list.sh: test_expect_success '"list" all worktrees with prunabl
+ git worktree add locked1 --detach &&
+ git worktree add locked2 --detach &&
+ git worktree lock locked1 &&
++ test_when_finished "git worktree unlock locked1" &&
+ git worktree lock locked2 --reason "with reason" &&
-+ test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" &&
++ test_when_finished "git worktree unlock locked2" &&
+ echo "$(git -C locked2 rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
+ printf "\tlocked: with reason\n" >>expect &&
+ git worktree list --verbose >out &&
--
2.30.0.373.geade8fefe8
^ permalink raw reply [flat|nested] 98+ messages in thread* [PATCH v4 1/7] worktree: libify should_prune_worktree()
2021-01-27 8:03 ` Rafael Silva
@ 2021-01-27 8:03 ` Rafael Silva
2021-01-27 8:03 ` [PATCH v4 2/7] worktree: teach worktree to lazy-load "prunable" reason Rafael Silva
` (6 subsequent siblings)
7 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-27 8:03 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Junio C Hamano, Rafael Silva
As part of teaching "git worktree list" to annotate worktree that is a
candidate for pruning, let's move should_prune_worktree() from
builtin/worktree.c to worktree.c in order to make part of the worktree
public API.
should_prune_worktree() knows how to select the given worktree for
pruning based on an expiration date, however the expiration value is
stored in a static file-scope variable and it is not local to the
function. In order to move the function, teach should_prune_worktree()
to take the expiration date as an argument and document the new
parameter that is not immediately obvious.
Also, change the function comment to clearly state that the worktree's
path is returned in `wtpath` argument.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
builtin/worktree.c | 75 +---------------------------------------------
worktree.c | 68 +++++++++++++++++++++++++++++++++++++++++
worktree.h | 14 +++++++++
3 files changed, 83 insertions(+), 74 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 71287b2da6..dd886d5029 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -67,79 +67,6 @@ static void delete_worktrees_dir_if_empty(void)
rmdir(git_path("worktrees")); /* ignore failed removal */
}
-/*
- * Return true if worktree entry should be pruned, along with the reason for
- * pruning. Otherwise, return false and the worktree's path, or NULL if it
- * cannot be determined. Caller is responsible for freeing returned path.
- */
-static int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath)
-{
- struct stat st;
- char *path;
- int fd;
- size_t len;
- ssize_t read_result;
-
- *wtpath = NULL;
- if (!is_directory(git_path("worktrees/%s", id))) {
- strbuf_addstr(reason, _("not a valid directory"));
- return 1;
- }
- if (file_exists(git_path("worktrees/%s/locked", id)))
- return 0;
- if (stat(git_path("worktrees/%s/gitdir", id), &st)) {
- strbuf_addstr(reason, _("gitdir file does not exist"));
- return 1;
- }
- fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY);
- if (fd < 0) {
- strbuf_addf(reason, _("unable to read gitdir file (%s)"),
- strerror(errno));
- return 1;
- }
- len = xsize_t(st.st_size);
- path = xmallocz(len);
-
- read_result = read_in_full(fd, path, len);
- if (read_result < 0) {
- strbuf_addf(reason, _("unable to read gitdir file (%s)"),
- strerror(errno));
- close(fd);
- free(path);
- return 1;
- }
- close(fd);
-
- if (read_result != len) {
- strbuf_addf(reason,
- _("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"),
- (uintmax_t)len, (uintmax_t)read_result);
- free(path);
- return 1;
- }
- while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
- len--;
- if (!len) {
- strbuf_addstr(reason, _("invalid gitdir file"));
- free(path);
- return 1;
- }
- path[len] = '\0';
- if (!file_exists(path)) {
- if (stat(git_path("worktrees/%s/index", id), &st) ||
- st.st_mtime <= expire) {
- strbuf_addstr(reason, _("gitdir file points to non-existent location"));
- free(path);
- return 1;
- } else {
- *wtpath = path;
- return 0;
- }
- }
- *wtpath = path;
- return 0;
-}
-
static void prune_worktree(const char *id, const char *reason)
{
if (show_only || verbose)
@@ -195,7 +122,7 @@ static void prune_worktrees(void)
if (is_dot_or_dotdot(d->d_name))
continue;
strbuf_reset(&reason);
- if (should_prune_worktree(d->d_name, &reason, &path))
+ if (should_prune_worktree(d->d_name, &reason, &path, expire))
prune_worktree(d->d_name, reason.buf);
else if (path)
string_list_append(&kept, path)->util = xstrdup(d->d_name);
diff --git a/worktree.c b/worktree.c
index 821b233479..8ae019af79 100644
--- a/worktree.c
+++ b/worktree.c
@@ -741,3 +741,71 @@ void repair_worktree_at_path(const char *path,
strbuf_release(&realdotgit);
strbuf_release(&dotgit);
}
+
+int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, timestamp_t expire)
+{
+ struct stat st;
+ char *path;
+ int fd;
+ size_t len;
+ ssize_t read_result;
+
+ *wtpath = NULL;
+ if (!is_directory(git_path("worktrees/%s", id))) {
+ strbuf_addstr(reason, _("not a valid directory"));
+ return 1;
+ }
+ if (file_exists(git_path("worktrees/%s/locked", id)))
+ return 0;
+ if (stat(git_path("worktrees/%s/gitdir", id), &st)) {
+ strbuf_addstr(reason, _("gitdir file does not exist"));
+ return 1;
+ }
+ fd = open(git_path("worktrees/%s/gitdir", id), O_RDONLY);
+ if (fd < 0) {
+ strbuf_addf(reason, _("unable to read gitdir file (%s)"),
+ strerror(errno));
+ return 1;
+ }
+ len = xsize_t(st.st_size);
+ path = xmallocz(len);
+
+ read_result = read_in_full(fd, path, len);
+ if (read_result < 0) {
+ strbuf_addf(reason, _("unable to read gitdir file (%s)"),
+ strerror(errno));
+ close(fd);
+ free(path);
+ return 1;
+ }
+ close(fd);
+
+ if (read_result != len) {
+ strbuf_addf(reason,
+ _("short read (expected %"PRIuMAX" bytes, read %"PRIuMAX")"),
+ (uintmax_t)len, (uintmax_t)read_result);
+ free(path);
+ return 1;
+ }
+ while (len && (path[len - 1] == '\n' || path[len - 1] == '\r'))
+ len--;
+ if (!len) {
+ strbuf_addstr(reason, _("invalid gitdir file"));
+ free(path);
+ return 1;
+ }
+ path[len] = '\0';
+ if (!file_exists(path)) {
+ if (stat(git_path("worktrees/%s/index", id), &st) ||
+ st.st_mtime <= expire) {
+ strbuf_addstr(reason, _("gitdir file points to non-existent location"));
+ free(path);
+ return 1;
+ } else {
+ *wtpath = path;
+ return 0;
+ }
+ }
+ *wtpath = path;
+ return 0;
+}
diff --git a/worktree.h b/worktree.h
index f38e6fd5a2..818e1491c7 100644
--- a/worktree.h
+++ b/worktree.h
@@ -73,6 +73,20 @@ int is_main_worktree(const struct worktree *wt);
*/
const char *worktree_lock_reason(struct worktree *wt);
+/*
+ * Return true if worktree entry should be pruned, along with the reason for
+ * pruning. Otherwise, return false and the worktree's path in `wtpath`, or
+ * NULL if it cannot be determined. Caller is responsible for freeing
+ * returned path.
+ *
+ * `expire` defines a grace period to prune the worktree when its path
+ * does not exist.
+ */
+int should_prune_worktree(const char *id,
+ struct strbuf *reason,
+ char **wtpath,
+ timestamp_t expire);
+
#define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)
/*
--
2.30.0.373.geade8fefe8
^ permalink raw reply related [flat|nested] 98+ messages in thread* [PATCH v4 2/7] worktree: teach worktree to lazy-load "prunable" reason
2021-01-27 8:03 ` Rafael Silva
2021-01-27 8:03 ` [PATCH v4 1/7] worktree: libify should_prune_worktree() Rafael Silva
@ 2021-01-27 8:03 ` Rafael Silva
2021-01-27 8:03 ` [PATCH v4 3/7] worktree: teach worktree_lock_reason() to gently handle main worktree Rafael Silva
` (5 subsequent siblings)
7 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-27 8:03 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Junio C Hamano, Rafael Silva
Add worktree_prune_reason() to allow a caller to discover whether a
worktree is prunable and the reason that it is, much like
worktree_lock_reason() indicates whether a worktree is locked and the
reason for the lock. As with worktree_lock_reason(), retrieve the
prunable reason lazily and cache it in the `worktree` structure.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
worktree.c | 20 ++++++++++++++++++++
worktree.h | 9 +++++++++
2 files changed, 29 insertions(+)
diff --git a/worktree.c b/worktree.c
index 8ae019af79..fb3e286996 100644
--- a/worktree.c
+++ b/worktree.c
@@ -15,6 +15,7 @@ void free_worktrees(struct worktree **worktrees)
free(worktrees[i]->id);
free(worktrees[i]->head_ref);
free(worktrees[i]->lock_reason);
+ free(worktrees[i]->prune_reason);
free(worktrees[i]);
}
free (worktrees);
@@ -245,6 +246,25 @@ const char *worktree_lock_reason(struct worktree *wt)
return wt->lock_reason;
}
+const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire)
+{
+ struct strbuf reason = STRBUF_INIT;
+ char *path = NULL;
+
+ if (is_main_worktree(wt))
+ return NULL;
+ if (wt->prune_reason_valid)
+ return wt->prune_reason;
+
+ if (should_prune_worktree(wt->id, &reason, &path, expire))
+ wt->prune_reason = strbuf_detach(&reason, NULL);
+ wt->prune_reason_valid = 1;
+
+ strbuf_release(&reason);
+ free(path);
+ return wt->prune_reason;
+}
+
/* convenient wrapper to deal with NULL strbuf */
static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...)
{
diff --git a/worktree.h b/worktree.h
index 818e1491c7..8b7c408132 100644
--- a/worktree.h
+++ b/worktree.h
@@ -11,11 +11,13 @@ struct worktree {
char *id;
char *head_ref; /* NULL if HEAD is broken or detached */
char *lock_reason; /* private - use worktree_lock_reason */
+ char *prune_reason; /* private - use worktree_prune_reason */
struct object_id head_oid;
int is_detached;
int is_bare;
int is_current;
int lock_reason_valid; /* private */
+ int prune_reason_valid; /* private */
};
/*
@@ -73,6 +75,13 @@ int is_main_worktree(const struct worktree *wt);
*/
const char *worktree_lock_reason(struct worktree *wt);
+/*
+ * Return the reason string if the given worktree should be pruned, otherwise
+ * NULL if it should not be pruned. `expire` defines a grace period to prune
+ * the worktree when its path does not exist.
+ */
+const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire);
+
/*
* Return true if worktree entry should be pruned, along with the reason for
* pruning. Otherwise, return false and the worktree's path in `wtpath`, or
--
2.30.0.373.geade8fefe8
^ permalink raw reply related [flat|nested] 98+ messages in thread* [PATCH v4 3/7] worktree: teach worktree_lock_reason() to gently handle main worktree
2021-01-27 8:03 ` Rafael Silva
2021-01-27 8:03 ` [PATCH v4 1/7] worktree: libify should_prune_worktree() Rafael Silva
2021-01-27 8:03 ` [PATCH v4 2/7] worktree: teach worktree to lazy-load "prunable" reason Rafael Silva
@ 2021-01-27 8:03 ` Rafael Silva
2021-01-27 8:03 ` [PATCH v4 4/7] t2402: ensure locked worktree is properly cleaned up Rafael Silva
` (4 subsequent siblings)
7 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-27 8:03 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Junio C Hamano, Rafael Silva
worktree_lock_reason() aborts with an assertion failure when called on
the main worktree since locking the main worktree is nonsensical. Not
only is this behavior undocumented, thus callers might not even be aware
that the call could potentially crash the program, but it also forces
clients to be extra careful:
if (!is_main_worktree(wt) && worktree_locked_reason(...))
...
Since we know that locking makes no sense in the context of the main
worktree, we can simply return false for the main worktree, thus making
client code less complex by eliminating the need for the callers to have
inside knowledge about the implementation:
if (worktree_lock_reason(...))
...
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
builtin/worktree.c | 2 +-
worktree.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index dd886d5029..df90a5acca 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -604,7 +604,7 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
strbuf_addstr(&sb, "(error)");
}
- if (!is_main_worktree(wt) && worktree_lock_reason(wt))
+ if (worktree_lock_reason(wt))
strbuf_addstr(&sb, " locked");
printf("%s\n", sb.buf);
diff --git a/worktree.c b/worktree.c
index fb3e286996..e00858540e 100644
--- a/worktree.c
+++ b/worktree.c
@@ -225,7 +225,8 @@ int is_main_worktree(const struct worktree *wt)
const char *worktree_lock_reason(struct worktree *wt)
{
- assert(!is_main_worktree(wt));
+ if (is_main_worktree(wt))
+ return NULL;
if (!wt->lock_reason_valid) {
struct strbuf path = STRBUF_INIT;
--
2.30.0.373.geade8fefe8
^ permalink raw reply related [flat|nested] 98+ messages in thread* [PATCH v4 4/7] t2402: ensure locked worktree is properly cleaned up
2021-01-27 8:03 ` Rafael Silva
` (2 preceding siblings ...)
2021-01-27 8:03 ` [PATCH v4 3/7] worktree: teach worktree_lock_reason() to gently handle main worktree Rafael Silva
@ 2021-01-27 8:03 ` Rafael Silva
2021-01-27 8:03 ` [PATCH v4 5/7] worktree: teach `list --porcelain` to annotate locked worktree Rafael Silva
` (3 subsequent siblings)
7 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-27 8:03 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Junio C Hamano, Rafael Silva
c57b3367be (worktree: teach `list` to annotate locked worktree,
2020-10-11) introduced a new test to ensure locked worktrees are listed
with "locked" annotation. However, the test does not clean up after
itself as "git worktree prune" is not going to remove the locked worktree
in the first place. This not only leaves the test in an unclean state it
also potentially breaks following tests that rely on the
"git worktree list" output.
Let's fix that by unlocking the worktree before the "prune" command.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
t/t2402-worktree-list.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index 795ddca2e4..1866ea09f6 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh
@@ -66,6 +66,7 @@ test_expect_success '"list" all worktrees with locked annotation' '
git worktree add --detach locked master &&
git worktree add --detach unlocked master &&
git worktree lock locked &&
+ test_when_finished "git worktree unlock locked" &&
git worktree list >out &&
grep "/locked *[0-9a-f].* locked$" out &&
! grep "/unlocked *[0-9a-f].* locked$" out
--
2.30.0.373.geade8fefe8
^ permalink raw reply related [flat|nested] 98+ messages in thread* [PATCH v4 5/7] worktree: teach `list --porcelain` to annotate locked worktree
2021-01-27 8:03 ` Rafael Silva
` (3 preceding siblings ...)
2021-01-27 8:03 ` [PATCH v4 4/7] t2402: ensure locked worktree is properly cleaned up Rafael Silva
@ 2021-01-27 8:03 ` Rafael Silva
2021-01-27 8:03 ` [PATCH v4 6/7] worktree: teach `list` to annotate prunable worktree Rafael Silva
` (2 subsequent siblings)
7 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-27 8:03 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Junio C Hamano, Rafael Silva
Commit c57b3367be (worktree: teach `list` to annotate locked worktree,
2020-10-11) taught "git worktree list" to annotate locked worktrees by
appending "locked" text to its output, however, this is not listed in
the --porcelain format.
Teach "list --porcelain" to do the same and add a "locked" attribute
followed by its reason, thus making both default and porcelain format
consistent. If the locked reason is not available then only "locked"
is shown.
The output of the "git worktree list --porcelain" becomes like so:
$ git worktree list --porcelain
...
worktree /path/to/locked
HEAD 123abcdea123abcd123acbd123acbda123abcd12
detached
locked
worktree /path/to/locked-with-reason
HEAD abc123abc123abc123abc123abc123abc123abc1
detached
locked reason why it is locked
...
In porcelain mode, if the lock reason contains special characters
such as newlines, they are escaped with backslashes and the entire
reason is enclosed in double quotes. For example:
$ git worktree list --porcelain
...
locked "worktree's path mounted in\nremovable device"
...
Furthermore, let's update the documentation to state that some
attributes in the porcelain format might be listed alone or together
with its value depending whether the value is available or not. Thus
documenting the case of the new "locked" attribute.
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
Documentation/git-worktree.txt | 28 ++++++++++++++++++++++++++--
builtin/worktree.c | 13 +++++++++++++
t/t2402-worktree-list.sh | 32 ++++++++++++++++++++++++++++++++
3 files changed, 71 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 02a706c4c0..426e9b4f76 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -377,8 +377,10 @@ Porcelain Format
The porcelain format has a line per attribute. Attributes are listed with a
label and value separated by a single space. Boolean attributes (like `bare`
and `detached`) are listed as a label only, and are present only
-if the value is true. The first attribute of a working tree is always
-`worktree`, an empty line indicates the end of the record. For example:
+if the value is true. Some attributes (like `locked`) can be listed as a label
+only or with a value depending upon whether a reason is available. The first
+attribute of a working tree is always `worktree`, an empty line indicates the
+end of the record. For example:
------------
$ git worktree list --porcelain
@@ -393,6 +395,28 @@ worktree /path/to/other-linked-worktree
HEAD 1234abc1234abc1234abc1234abc1234abc1234a
detached
+worktree /path/to/linked-worktree-locked-no-reason
+HEAD 5678abc5678abc5678abc5678abc5678abc5678c
+branch refs/heads/locked-no-reason
+locked
+
+worktree /path/to/linked-worktree-locked-with-reason
+HEAD 3456def3456def3456def3456def3456def3456b
+branch refs/heads/locked-with-reason
+locked reason why is locked
+
+------------
+
+If the lock reason contains "unusual" characters such as newline, they
+are escaped and the entire reason is quoted as explained for the
+configuration variable `core.quotePath` (see linkgit:git-config[1]).
+For Example:
+
+------------
+$ git worktree list --porcelain
+...
+locked "reason\nwhy is locked"
+...
------------
EXAMPLES
diff --git a/builtin/worktree.c b/builtin/worktree.c
index df90a5acca..98177f91d4 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -12,6 +12,7 @@
#include "submodule.h"
#include "utf8.h"
#include "worktree.h"
+#include "quote.h"
static const char * const worktree_usage[] = {
N_("git worktree add [<options>] <path> [<commit-ish>]"),
@@ -569,6 +570,8 @@ static int add(int ac, const char **av, const char *prefix)
static void show_worktree_porcelain(struct worktree *wt)
{
+ const char *reason;
+
printf("worktree %s\n", wt->path);
if (wt->is_bare)
printf("bare\n");
@@ -579,6 +582,16 @@ static void show_worktree_porcelain(struct worktree *wt)
else if (wt->head_ref)
printf("branch %s\n", wt->head_ref);
}
+
+ reason = worktree_lock_reason(wt);
+ if (reason && *reason) {
+ struct strbuf sb = STRBUF_INIT;
+ quote_c_style(reason, &sb, NULL, 0);
+ printf("locked %s\n", sb.buf);
+ strbuf_release(&sb);
+ } else if (reason)
+ printf("locked\n");
+
printf("\n");
}
diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index 1866ea09f6..39596e4aa9 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh
@@ -72,6 +72,38 @@ test_expect_success '"list" all worktrees with locked annotation' '
! grep "/unlocked *[0-9a-f].* locked$" out
'
+test_expect_success '"list" all worktrees --porcelain with locked' '
+ test_when_finished "rm -rf locked1 locked2 unlocked out actual expect && git worktree prune" &&
+ echo "locked" >expect &&
+ echo "locked with reason" >>expect &&
+ git worktree add --detach locked1 &&
+ git worktree add --detach locked2 &&
+ # unlocked worktree should not be annotated with "locked"
+ git worktree add --detach unlocked &&
+ git worktree lock locked1 &&
+ test_when_finished "git worktree unlock locked1" &&
+ git worktree lock locked2 --reason "with reason" &&
+ test_when_finished "git worktree unlock locked2" &&
+ git worktree list --porcelain >out &&
+ grep "^locked" out >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '"list" all worktrees --porcelain with locked reason newline escaped' '
+ test_when_finished "rm -rf locked_lf locked_crlf out actual expect && git worktree prune" &&
+ printf "locked \"locked\\\\r\\\\nreason\"\n" >expect &&
+ printf "locked \"locked\\\\nreason\"\n" >>expect &&
+ git worktree add --detach locked_lf &&
+ git worktree add --detach locked_crlf &&
+ git worktree lock locked_lf --reason "$(printf "locked\nreason")" &&
+ test_when_finished "git worktree unlock locked_lf" &&
+ git worktree lock locked_crlf --reason "$(printf "locked\r\nreason")" &&
+ test_when_finished "git worktree unlock locked_crlf" &&
+ git worktree list --porcelain >out &&
+ grep "^locked" out >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'bare repo setup' '
git init --bare bare1 &&
echo "data" >file1 &&
--
2.30.0.373.geade8fefe8
^ permalink raw reply related [flat|nested] 98+ messages in thread* [PATCH v4 6/7] worktree: teach `list` to annotate prunable worktree
2021-01-27 8:03 ` Rafael Silva
` (4 preceding siblings ...)
2021-01-27 8:03 ` [PATCH v4 5/7] worktree: teach `list --porcelain` to annotate locked worktree Rafael Silva
@ 2021-01-27 8:03 ` Rafael Silva
2021-01-27 8:03 ` [PATCH v4 7/7] worktree: teach `list` verbose mode Rafael Silva
2021-01-30 7:04 ` [PATCH v3 0/7] teach `worktree list` verbose mode and prunable annotations Eric Sunshine
7 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-27 8:03 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Junio C Hamano, Rafael Silva
The "git worktree list" command shows the absolute path to the worktree,
the commit that is checked out, the name of the branch, and a "locked"
annotation if the worktree is locked, however, it does not indicate
whether the worktree is prunable.
The "prune" command will remove a worktree if it is prunable unless
`--dry-run` option is specified. This could lead to a worktree being
removed without the user realizing before it is too late, in case the
user forgets to pass --dry-run for instance. If the "list" command shows
which worktree is prunable, the user could verify before running
"git worktree prune" and hopefully prevents the working tree to be
removed "accidentally" on the worse case scenario.
Let's teach "git worktree list" to show when a worktree is a prunable
candidate for both default and porcelain format.
In the default format a "prunable" text is appended:
$ git worktree list
/path/to/main aba123 [main]
/path/to/linked 123abc [branch-a]
/path/to/prunable ace127 (detached HEAD) prunable
In the --porcelain format a prunable label is added followed by
its reason:
$ git worktree list --porcelain
...
worktree /path/to/prunable
HEAD abc1234abc1234abc1234abc1234abc1234abc12
detached
prunable gitdir file points to non-existent location
...
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
Documentation/git-worktree.txt | 26 ++++++++++++++++++++++++--
builtin/worktree.c | 10 ++++++++++
t/t2402-worktree-list.sh | 32 ++++++++++++++++++++++++++++++++
3 files changed, 66 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 426e9b4f76..240c3fd76b 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -97,8 +97,9 @@ list::
List details of each working tree. The main working tree is listed first,
followed by each of the linked working trees. The output details include
whether the working tree is bare, the revision currently checked out, the
-branch currently checked out (or "detached HEAD" if none), and "locked" if
-the worktree is locked.
+branch currently checked out (or "detached HEAD" if none), "locked" if
+the worktree is locked, "prunable" if the worktree can be pruned by `prune`
+command.
lock::
@@ -234,6 +235,9 @@ This can also be set up as the default behaviour by using the
--expire <time>::
With `prune`, only expire unused working trees older than `<time>`.
++
+With `list`, annotate missing working trees as prunable if they are
+older than `<time>`.
--reason <string>::
With `lock`, an explanation why the working tree is locked.
@@ -372,6 +376,19 @@ $ git worktree list
/path/to/other-linked-worktree 1234abc (detached HEAD)
------------
+The command also shows annotations for each working tree, according to its state.
+These annotations are:
+
+ * `locked`, if the working tree is locked.
+ * `prunable`, if the working tree can be pruned via `git worktree prune`.
+
+------------
+$ git worktree list
+/path/to/linked-worktree abcd1234 [master]
+/path/to/locked-worktreee acbd5678 (brancha) locked
+/path/to/prunable-worktree 5678abc (detached HEAD) prunable
+------------
+
Porcelain Format
~~~~~~~~~~~~~~~~
The porcelain format has a line per attribute. Attributes are listed with a
@@ -405,6 +422,11 @@ HEAD 3456def3456def3456def3456def3456def3456b
branch refs/heads/locked-with-reason
locked reason why is locked
+worktree /path/to/linked-worktree-prunable
+HEAD 1233def1234def1234def1234def1234def1234b
+detached
+prunable gitdir file points to non-existent location
+
------------
If the lock reason contains "unusual" characters such as newline, they
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 98177f91d4..20944c266e 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -592,6 +592,10 @@ static void show_worktree_porcelain(struct worktree *wt)
} else if (reason)
printf("locked\n");
+ reason = worktree_prune_reason(wt, expire);
+ if (reason)
+ printf("prunable %s\n", reason);
+
printf("\n");
}
@@ -620,6 +624,9 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
if (worktree_lock_reason(wt))
strbuf_addstr(&sb, " locked");
+ if (worktree_prune_reason(wt, expire))
+ strbuf_addstr(&sb, " prunable");
+
printf("%s\n", sb.buf);
strbuf_release(&sb);
}
@@ -663,9 +670,12 @@ static int list(int ac, const char **av, const char *prefix)
struct option options[] = {
OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
+ OPT_EXPIRY_DATE(0, "expire", &expire,
+ N_("add 'prunable' annotation to worktrees older than <time>")),
OPT_END()
};
+ expire = TIME_MAX;
ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
if (ac)
usage_with_options(worktree_usage, options);
diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index 39596e4aa9..69342b7280 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh
@@ -104,6 +104,38 @@ test_expect_success '"list" all worktrees --porcelain with locked reason newline
test_cmp expect actual
'
+test_expect_success '"list" all worktrees with prunable annotation' '
+ test_when_finished "rm -rf prunable unprunable out && git worktree prune" &&
+ git worktree add --detach prunable &&
+ git worktree add --detach unprunable &&
+ rm -rf prunable &&
+ git worktree list >out &&
+ grep "/prunable *[0-9a-f].* prunable$" out &&
+ ! grep "/unprunable *[0-9a-f].* prunable$"
+'
+
+test_expect_success '"list" all worktrees --porcelain with prunable' '
+ test_when_finished "rm -rf prunable out && git worktree prune" &&
+ git worktree add --detach prunable &&
+ rm -rf prunable &&
+ git worktree list --porcelain >out &&
+ sed -n "/^worktree .*\/prunable$/,/^$/p" <out >only_prunable &&
+ test_i18ngrep "^prunable gitdir file points to non-existent location$" only_prunable
+'
+
+test_expect_success '"list" all worktrees with prunable consistent with "prune"' '
+ test_when_finished "rm -rf prunable unprunable out && git worktree prune" &&
+ git worktree add --detach prunable &&
+ git worktree add --detach unprunable &&
+ rm -rf prunable &&
+ git worktree list >out &&
+ grep "/prunable *[0-9a-f].* prunable$" out &&
+ ! grep "/unprunable *[0-9a-f].* unprunable$" out &&
+ git worktree prune --verbose >out &&
+ test_i18ngrep "^Removing worktrees/prunable" out &&
+ test_i18ngrep ! "^Removing worktrees/unprunable" out
+'
+
test_expect_success 'bare repo setup' '
git init --bare bare1 &&
echo "data" >file1 &&
--
2.30.0.373.geade8fefe8
^ permalink raw reply related [flat|nested] 98+ messages in thread* [PATCH v4 7/7] worktree: teach `list` verbose mode
2021-01-27 8:03 ` Rafael Silva
` (5 preceding siblings ...)
2021-01-27 8:03 ` [PATCH v4 6/7] worktree: teach `list` to annotate prunable worktree Rafael Silva
@ 2021-01-27 8:03 ` Rafael Silva
2021-01-30 7:04 ` [PATCH v3 0/7] teach `worktree list` verbose mode and prunable annotations Eric Sunshine
7 siblings, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-27 8:03 UTC (permalink / raw)
To: git; +Cc: Eric Sunshine, Phillip Wood, Junio C Hamano, Rafael Silva
"git worktree list" annotates each worktree according to its state such
as "prunable" or "locked", however it is not immediately obvious why
these worktrees are being annotated. For prunable worktrees a reason
is available that is returned by should_prune_worktree() and for locked
worktrees a reason might be available provided by the user via `lock`
command.
Let's teach "git worktree list" a --verbose mode that outputs the reason
why the worktrees are being annotated. The reason is a text that can take
virtually any size and appending the text on the default columned format
will make it difficult to extend the command with other annotations and
not fit nicely on the screen. In order to address this shortcoming the
annotation is then moved to the next line indented followed by the reason
If the reason is not available the annotation stays on the same line as
the worktree itself.
The output of "git worktree list" with verbose becomes like so:
$ git worktree list --verbose
...
/path/to/locked-no-reason acb124 [branch-a] locked
/path/to/locked-with-reason acc125 [branch-b]
locked: worktree with a locked reason
/path/to/prunable-reason ace127 [branch-d]
prunable: gitdir file points to non-existent location
...
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
---
Documentation/git-worktree.txt | 20 ++++++++++++++++++++
builtin/worktree.c | 14 ++++++++++++--
t/t2402-worktree-list.sh | 31 +++++++++++++++++++++++++++++++
3 files changed, 63 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 240c3fd76b..f1bb1fa5f5 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -232,6 +232,8 @@ This can also be set up as the default behaviour by using the
-v::
--verbose::
With `prune`, report all removals.
++
+With `list`, output additional information about worktrees (see below).
--expire <time>::
With `prune`, only expire unused working trees older than `<time>`.
@@ -389,6 +391,24 @@ $ git worktree list
/path/to/prunable-worktree 5678abc (detached HEAD) prunable
------------
+For these annotations, a reason might also be available and this can be
+seen using the verbose mode. The annotation is then moved to the next line
+indented followed by the additional information.
+
+------------
+$ git worktree list --verbose
+/path/to/linked-worktree abcd1234 [master]
+/path/to/locked-worktree-no-reason abcd5678 (detached HEAD) locked
+/path/to/locked-worktree-with-reason 1234abcd (brancha)
+ locked: working tree path is mounted on a portable device
+/path/to/prunable-worktree 5678abc1 (detached HEAD)
+ prunable: gitdir file points to non-existent location
+------------
+
+Note that the annotation is moved to the next line if the additional
+information is available, otherwise it stays on the same line as the
+working tree itself.
+
Porcelain Format
~~~~~~~~~~~~~~~~
The porcelain format has a line per attribute. Attributes are listed with a
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 20944c266e..1cd5c2016e 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -604,6 +604,7 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
struct strbuf sb = STRBUF_INIT;
int cur_path_len = strlen(wt->path);
int path_adj = cur_path_len - utf8_strwidth(wt->path);
+ const char *reason;
strbuf_addf(&sb, "%-*s ", 1 + path_maxlen + path_adj, wt->path);
if (wt->is_bare)
@@ -621,10 +622,16 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
strbuf_addstr(&sb, "(error)");
}
- if (worktree_lock_reason(wt))
+ reason = worktree_lock_reason(wt);
+ if (verbose && reason && *reason)
+ strbuf_addf(&sb, "\n\tlocked: %s", reason);
+ else if (reason)
strbuf_addstr(&sb, " locked");
- if (worktree_prune_reason(wt, expire))
+ reason = worktree_prune_reason(wt, expire);
+ if (verbose && reason)
+ strbuf_addf(&sb, "\n\tprunable: %s", reason);
+ else if (reason)
strbuf_addstr(&sb, " prunable");
printf("%s\n", sb.buf);
@@ -670,6 +677,7 @@ static int list(int ac, const char **av, const char *prefix)
struct option options[] = {
OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
+ OPT__VERBOSE(&verbose, N_("show extended annotations and reasons, if available")),
OPT_EXPIRY_DATE(0, "expire", &expire,
N_("add 'prunable' annotation to worktrees older than <time>")),
OPT_END()
@@ -679,6 +687,8 @@ static int list(int ac, const char **av, const char *prefix)
ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
if (ac)
usage_with_options(worktree_usage, options);
+ else if (verbose && porcelain)
+ die(_("--verbose and --porcelain are mutually exclusive"));
else {
struct worktree **worktrees = get_worktrees();
int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;
diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index 69342b7280..39a339e636 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh
@@ -136,6 +136,37 @@ test_expect_success '"list" all worktrees with prunable consistent with "prune"'
test_i18ngrep ! "^Removing worktrees/unprunable" out
'
+test_expect_success '"list" --verbose and --porcelain mutually exclusive' '
+ test_must_fail git worktree list --verbose --porcelain
+'
+
+test_expect_success '"list" all worktrees --verbose with locked' '
+ test_when_finished "rm -rf locked1 locked2 out actual expect && git worktree prune" &&
+ git worktree add locked1 --detach &&
+ git worktree add locked2 --detach &&
+ git worktree lock locked1 &&
+ test_when_finished "git worktree unlock locked1" &&
+ git worktree lock locked2 --reason "with reason" &&
+ test_when_finished "git worktree unlock locked2" &&
+ echo "$(git -C locked2 rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
+ printf "\tlocked: with reason\n" >>expect &&
+ git worktree list --verbose >out &&
+ grep "/locked1 *[0-9a-f].* locked$" out &&
+ sed -n "s/ */ /g;/\/locked2 *[0-9a-f].*$/,/locked: .*$/p" <out >actual &&
+ test_cmp actual expect
+'
+
+test_expect_success '"list" all worktrees --verbose with prunable' '
+ test_when_finished "rm -rf prunable out actual expect && git worktree prune" &&
+ git worktree add prunable --detach &&
+ echo "$(git -C prunable rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
+ printf "\tprunable: gitdir file points to non-existent location\n" >>expect &&
+ rm -rf prunable &&
+ git worktree list --verbose >out &&
+ sed -n "s/ */ /g;/\/prunable *[0-9a-f].*$/,/prunable: .*$/p" <out >actual &&
+ test_i18ncmp actual expect
+'
+
test_expect_success 'bare repo setup' '
git init --bare bare1 &&
echo "data" >file1 &&
--
2.30.0.373.geade8fefe8
^ permalink raw reply related [flat|nested] 98+ messages in thread* Re: [PATCH v3 0/7] teach `worktree list` verbose mode and prunable annotations
2021-01-27 8:03 ` Rafael Silva
` (6 preceding siblings ...)
2021-01-27 8:03 ` [PATCH v4 7/7] worktree: teach `list` verbose mode Rafael Silva
@ 2021-01-30 7:04 ` Eric Sunshine
2021-01-30 9:42 ` Rafael Silva
2021-01-30 17:50 ` Junio C Hamano
7 siblings, 2 replies; 98+ messages in thread
From: Eric Sunshine @ 2021-01-30 7:04 UTC (permalink / raw)
To: Rafael Silva; +Cc: Git List, Phillip Wood, Junio C Hamano
On Wed, Jan 27, 2021 at 3:03 AM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> Changes in v4
> * Added documentation to explain that the lock reason is quoted with
> the same rules as described for `core.quotePath`.
> * The `worktree unlock` issued in the test cleanup is splitted and
> executed after each `worktree lock` to ensure the unlock is only
> done after we know each locked command was successful.
> * Fix a couple of grammos in the [4/7] commit message.
I just gave v4 a complete read-through and it looks great. All review
comments on previous rounds have been addressed, and I didn't find
anything important to comment on in v4. Very nicely done.
Please consider this entire series:
Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
Junio: Perhaps the summary of this series in the merge message can be
refined a bit, as it only talks about verbose mode, but this series
does a bit more than that. Heres a proposal:
`git worktree list` now annotates worktrees as prunable, shows
locked and prunable attributes in --porcelain mode, and gained
a --verbose option.
^ permalink raw reply [flat|nested] 98+ messages in thread* Re: [PATCH v3 0/7] teach `worktree list` verbose mode and prunable annotations
2021-01-30 7:04 ` [PATCH v3 0/7] teach `worktree list` verbose mode and prunable annotations Eric Sunshine
@ 2021-01-30 9:42 ` Rafael Silva
2021-01-30 17:50 ` Junio C Hamano
1 sibling, 0 replies; 98+ messages in thread
From: Rafael Silva @ 2021-01-30 9:42 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List, Phillip Wood, Junio C Hamano
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Wed, Jan 27, 2021 at 3:03 AM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> Changes in v4
>> * Added documentation to explain that the lock reason is quoted with
>> the same rules as described for `core.quotePath`.
>> * The `worktree unlock` issued in the test cleanup is splitted and
>> executed after each `worktree lock` to ensure the unlock is only
>> done after we know each locked command was successful.
>> * Fix a couple of grammos in the [4/7] commit message.
>
> I just gave v4 a complete read-through and it looks great. All review
> comments on previous rounds have been addressed, and I didn't find
> anything important to comment on in v4. Very nicely done.
>
> Please consider this entire series:
>
> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
>
Thanks for the review and for all the insightful comments.
> Junio: Perhaps the summary of this series in the merge message can be
> refined a bit, as it only talks about verbose mode, but this series
> does a bit more than that. Heres a proposal:
>
> `git worktree list` now annotates worktrees as prunable, shows
> locked and prunable attributes in --porcelain mode, and gained
> a --verbose option.
I believe this message summarize nicely all the changes implemented by
this series.
--
Thanks
Rafael
^ permalink raw reply [flat|nested] 98+ messages in thread
* Re: [PATCH v3 0/7] teach `worktree list` verbose mode and prunable annotations
2021-01-30 7:04 ` [PATCH v3 0/7] teach `worktree list` verbose mode and prunable annotations Eric Sunshine
2021-01-30 9:42 ` Rafael Silva
@ 2021-01-30 17:50 ` Junio C Hamano
1 sibling, 0 replies; 98+ messages in thread
From: Junio C Hamano @ 2021-01-30 17:50 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Rafael Silva, Git List, Phillip Wood
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Wed, Jan 27, 2021 at 3:03 AM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> Changes in v4
>> * Added documentation to explain that the lock reason is quoted with
>> the same rules as described for `core.quotePath`.
>> * The `worktree unlock` issued in the test cleanup is splitted and
>> executed after each `worktree lock` to ensure the unlock is only
>> done after we know each locked command was successful.
>> * Fix a couple of grammos in the [4/7] commit message.
>
> I just gave v4 a complete read-through and it looks great. All review
> comments on previous rounds have been addressed, and I didn't find
> anything important to comment on in v4. Very nicely done.
>
> Please consider this entire series:
>
> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
>
> Junio: Perhaps the summary of this series in the merge message can be
> refined a bit, as it only talks about verbose mode, but this series
> does a bit more than that. Heres a proposal:
>
> `git worktree list` now annotates worktrees as prunable, shows
> locked and prunable attributes in --porcelain mode, and gained
> a --verbose option.
Nicely done. Thanks for excellent contribution, both of you.
^ permalink raw reply [flat|nested] 98+ messages in thread