[PATCH 0/7] teach `worktree list` verbose mode and prunable annotations

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 address 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 address one shortcoming for porcelain
format to escape any newline characters (LF and CRLF) for the lock reason
to prevent breaking format mentioned in [4] and [1] during the review
cycle.

I have tried to organise in a way the series is easy to review, the
patches are organised in seven patches.

The first 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 "struct worktree" and load the information directly,
which can 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, if any.

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].

The forth patch is the one that teaches `list` command to show prunable
annotation, for both default and porcelain format, and adds the verbose
option. These changes are also done for the "locked" annotation that was
not done on in [1]. Additionally, `list` learns to fail when both
--verbose and --porcelain format as used together.

The fifth patch adds worktree_escape_reason() that accepts a (char *)
text and returned the text with any LF or CRLF escaped. The caller is
responsible to freeing the escaped text. This is used by the locked
annotation in porcelain format. Currently, this is defined within
builtin/worktree.c as I was not sure whether libfying the function as
part of this series is a good idea. At this time it seems more sensible
to leave the code internally and libfying later once we are confident
about the implementation and whether it can be used in other part of the
code base but I'm open for suggestion.

The sixth and seventh patches adds tests and update the git-worktree.txt
documentation accordingly.

[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/

Series is built on top of 71ca53e8125e36efbda17293c50027d31681a41f, currently
poiting to master and v2.30.0 tag.

Rafael Silva (7):
  worktree: move should_prune_worktree() to worktree.c
  worktree: implement worktree_prune_reason() wrapper
  worktree: teach worktree_lock_reason() to gently handle main worktree
  worktree: teach `list` prunable annotation and verbose
  worktree: `list` escape lock reason in --porcelain
  worktree: add tests for `list` verbose and annotations
  worktree: document `list` verbose and prunable annotations

 Documentation/git-worktree.txt |  59 ++++++++++++++-
 builtin/worktree.c             | 130 ++++++++++++++-------------------
 t/t2402-worktree-list.sh       |  94 +++++++++++++++++++++++-
 worktree.c                     |  96 +++++++++++++++++++++++-
 worktree.h                     |  17 +++++
 5 files changed, 313 insertions(+), 83 deletions(-)

-- 
2.30.0.391.g469bf2a980

The main worktree should not be locked and the worktree_lock_reason() API
is aware of this fact and avoids running the check code for the main
worktree. This checks is done via assert() macro, Therefore the caller
needs to ensure the function is never called, usually by additional code.

We can handle that case more gently by just returning false for the main
worktree and not bother checking if the "locked" file exists. This will
allowed further simplification from the caller as they will not need to
ensure the main worktree is never passed to the API.

Teach worktree_lock_reason() to be more gently and just return false for
the main working tree.

Signed-off-by: Rafael Silva <redacted>
---
 worktree.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/worktree.c b/worktree.c
index ee14db3ab5..78be83ba16 100644
--- a/worktree.c
+++ b/worktree.c

@@ -225,9 +225,7 @@ int is_main_worktree(const struct worktree *wt)
 
 const char *worktree_lock_reason(struct worktree *wt)
 {
-	assert(!is_main_worktree(wt));
-
-	if (!wt->lock_reason_valid) {
+	if (!is_main_worktree(wt) && !wt->lock_reason_valid) {
 		struct strbuf path = STRBUF_INIT;
 
 		strbuf_addstr(&path, worktree_git_path(wt, "locked"));

-- 
2.30.0.391.g469bf2a980

Add tests for "git worktree list" verbose mode, prunable and locked
annotations for both default and porcelain format and ensure the
"prunable" annotation is consistent with what "git worktree prune"
command will eventually remove. Additionally, add one test case to
ensure any newline characters are escaped from locked reason for the
porcelain format to prevent breaking the format.

The 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 remove the worktree
as the "git worktree prune" is not going to remove any locked worktrees.
Let's fix that by unlocking the worktree before the "prune" command.

Signed-off-by: Rafael Silva <redacted>
---
 t/t2402-worktree-list.sh | 94 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 93 insertions(+), 1 deletion(-)

diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index 795ddca2e4..6c4da438b3 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh

@@ -62,7 +62,9 @@ test_expect_success '"list" all worktrees --porcelain' '
 '
 
 test_expect_success '"list" all worktrees with locked annotation' '
-	test_when_finished "rm -rf locked unlocked out && git worktree prune" &&
+	test_when_finished "rm -rf locked unlocked out &&
+		git worktree unlock locked &&
+		git worktree prune" &&
 	git worktree add --detach locked master &&
 	git worktree add --detach unlocked master &&
 	git worktree lock locked &&

@@ -71,6 +73,96 @@ test_expect_success '"list" all worktrees with locked annotation' '
 	! grep "/unlocked  *[0-9a-f].* locked$" out
 '
 
+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 with prunable consistent with "prune"' '
+	test_when_finished "rm -rf prunable out && git worktree prune" &&
+	git worktree add --detach prunable &&
+	rm -rf prunable &&
+	git worktree list >out &&
+	grep "/prunable  *[0-9a-f].* prunable$" out &&
+	git worktree prune --verbose >out &&
+	test_i18ngrep "^Removing worktrees/prunable" out
+'
+
+test_expect_success '"list" all worktrees --porcelain with locked' '
+	test_when_finished "rm -rf locked1 locked2 out actual expect &&
+		git worktree unlock locked1 &&
+		git worktree unlock locked2 &&
+		git worktree prune" &&
+	echo "locked" >expect &&
+	echo "locked with reason" >>expect &&
+	git worktree add locked1 --detach &&
+	git worktree add locked2 --detach &&
+	git worktree lock locked1 &&
+	git worktree lock locked2 --reason "with reason" &&
+	git worktree list --porcelain >out &&
+	grep "^locked" out >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '"list" all worktrees --porcelain with newline escaped locked reason' '
+	test_when_finished "rm -rf locked_lf locked_crlf reason_lf reason_crlf out actual expect reason &&
+		git worktree unlock locked_lf &&
+		git worktree unlock locked_crlf &&
+		git worktree prune" &&
+	printf "locked locked\\\\r\\\\nreason\n" >expect &&
+	printf "locked locked\\\\nreason\n" >>expect &&
+	git worktree add locked_lf --detach &&
+	git worktree add locked_crlf --detach &&
+	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 list --porcelain >out &&
+	grep "^locked" out >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '"list" all worktrees --porcelain with prunable' '
+	test_when_finished "rm -rf prunable list out && git worktree prune" &&
+	git worktree add --detach prunable &&
+	rm -rf prunable &&
+	git worktree list --porcelain >out &&
+	test_i18ngrep "^prunable gitdir file points to non-existent location$" out
+'
+
+test_expect_success '"list" all worktrees --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 unlock locked &&
+		git worktree prune" &&
+	git worktree add locked --detach &&
+	git worktree lock locked --reason "with reason" &&
+	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.391.g469bf2a980

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. It is not clear whether a worktree,
if any, is prunable. The "prune" command will remove a worktree in case
is a prunable candidate unless --dry-run option is specified. This could
lead to a worktree being removed without the user realizing before is to
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 "accidently" on the worse case scenario.

Let's teach "git worktree list" to show when a worktree is prunable by
appending "prunable" text to its output by default and show a prunable
label for the porcelain format followed by the reason, if the avaiable.
While at it, let's do the same for the "locked" annotation.

Also, the worktree_prune_reason() stores the reason why git is selecting
the worktree to be pruned, so let's leverage that and also display this
information to the user. However, the reason is human readable text that
can take virtually any size which might make harder to extend the "list"
command with additional annotations and not fit nicely on the screen.

In order to address this shortcoming, let's teach "git worktree list" to
take a verbose option that will output the prune reason on the next line
indented, if the reason is available, otherwise the annotation is kept on
the same line. While at it, let's do the same for the "locked"
annotation.

The output of "git worktree list" with verbose becomes like so:

    $ git worktree list --verbose
    /path/to/main	      aba123 [main]
    /path/to/locked           acb124 [branch-a] locked
    /path/to/locked-reason    acc125 [branch-b]
        locked: worktree with locked reason
    /path/to/prunable         acd126 [branch-c] prunable
    /path/to/prunable-reason  ace127 [branch-d]
        prunable: gitdir file points to non-existent location

And the output of the "git worktree list --porcelain" becomes like so:

    $ git worktree list --porcelain
    worktree /path/to/main
    HEAD abc1234abc1234abc1234abc1234abc1234abc12
    branch refs/heads/main

    worktree /path/to/linked
    HEAD 123abc456def789abc1234def5678abc9123abce
    branch refs/heads/linked

    worktree /path/to/locked
    HEAD 123abcdea123abcd123acbd123acbda123abcd12
    detached
    locked lock reason

    worktree /path/to/prunable
    HEAD def1234def1234def1234def1234def1234def1a
    detached
    prunable prunable reason

Signed-off-by: Rafael Silva <redacted>
---
 builtin/worktree.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index eeb3ffaed0..dedd4089e5 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c

@@ -578,6 +578,20 @@ static void show_worktree_porcelain(struct worktree *wt)
 			printf("detached\n");
 		else if (wt->head_ref)
 			printf("branch %s\n", wt->head_ref);
+
+		if (worktree_lock_reason(wt)) {
+			if (*wt->lock_reason)
+				printf("locked %s\n", wt->lock_reason);
+			else
+				printf("locked\n");
+		}
+
+		if (worktree_prune_reason(wt, expire)) {
+			if (*wt->prune_reason)
+				printf("prunable %s\n", wt->prune_reason);
+			else
+				printf("prunable\n");
+		}
 	}
 	printf("\n");
 }

@@ -604,8 +618,19 @@ 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))
-		strbuf_addstr(&sb, " locked");
+	if (worktree_lock_reason(wt)) {
+		if (verbose && *wt->lock_reason)
+			strbuf_addf(&sb, "\n\tlocked: %s", wt->lock_reason);
+		else
+			strbuf_addstr(&sb, " locked");
+	}
+
+	if (worktree_prune_reason(wt, expire)) {
+		if (verbose && *wt->prune_reason)
+			strbuf_addf(&sb, "\n\tprunable: %s", wt->prune_reason);
+		else
+			strbuf_addstr(&sb, " prunable");
+	}
 
 	printf("%s\n", sb.buf);
 	strbuf_release(&sb);

@@ -650,12 +675,18 @@ 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_("show working trees that is candidate to be pruned 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);
+	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;

-- 
2.30.0.391.g469bf2a980

Update the documentation with "git worktree list" verbose mode, prunable
and locked annotations for the default and porcelain format as part of
teaching the command to show prunable working trees and verbose mode.

Signed-off-by: Rafael Silva <redacted>
---
 Documentation/git-worktree.txt | 59 ++++++++++++++++++++++++++++++++--
 1 file changed, 56 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index af06128cc9..8ce579e2e8 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::
 

@@ -226,9 +227,12 @@ 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 for working trees.
 
 --expire <time>::
 	With `prune`, only expire unused working trees older than `<time>`.
+	With `list`, annotate unused working trees older than `<time>` as prunable
+	candidates that will be remove by `prune` command if the same option is used.
 
 --reason <string>::
 	With `lock`, an explanation why the working tree is locked.

@@ -367,13 +371,48 @@ $ 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 any working tree is locked
+ * "prunable", if any 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
+------------
+
+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 only moved to the next line only if the
+additional text is available, otherwise the text is kept on the same.
+
 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:
+`worktree`, an empty line indicates the end of the record.
++
+In case any of the working trees are locked or is a candidate for pruning
+(See DESCRIPTION above) the labels "locked" and "prunable" is also shown
+followed by a reason, if available, otherwise only the labels are listed.
+For example:
 
 ------------
 $ git worktree list --porcelain

@@ -388,6 +427,20 @@ 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
+
+worktree /path/to/linked-worktree-prunable
+HEAD 1233def1234def1234def1234def1234def1234b
+detached
+prunable gitdir file points to non-existent location
 ------------
 
 EXAMPLES

-- 
2.30.0.391.g469bf2a980

"git worktree list" porcelain format shows one attribute per line, each
attribute is listed with a label and value separated by a single space.
If any worktree is locked, a "locked" attribute is listed followed by the
reason, if available, otherwise only "locked" is shown.

In case the lock reason contains newline characters (i.e: LF or CRLF)
this will cause the format to break as each line should correspond to
one attribute. This leads to unwanted behavior, specially as the
porcelain is intended to be machine-readable. To address this shortcoming
let's teach "git worktree list" to escape any newline character before
outputting the locked reason for porcelain format.

Signed-off-by: Rafael Silva <redacted>
---
 builtin/worktree.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index dedd4089e5..9156ccd67e 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c

@@ -567,6 +567,24 @@ static int add(int ac, const char **av, const char *prefix)
 	return add_worktree(path, branch, &opts);
 }
 
+static char *worktree_escape_reason(char *reason)
+{
+	struct strbuf escaped = STRBUF_INIT;
+	char *r;
+
+	for (r = reason; *r; r++) {
+		if (*r == '\r' && *(r + 1) && *(r + 1) == '\n') {
+			strbuf_addstr(&escaped, "\\r\\n");
+			r++;
+		} else if (*r == '\n')
+			strbuf_addstr(&escaped, "\\n");
+		else
+			strbuf_addch(&escaped, *r);
+	}
+
+	return strbuf_detach(&escaped, NULL);
+}
+
 static void show_worktree_porcelain(struct worktree *wt)
 {
 	printf("worktree %s\n", wt->path);

@@ -580,9 +598,11 @@ static void show_worktree_porcelain(struct worktree *wt)
 			printf("branch %s\n", wt->head_ref);
 
 		if (worktree_lock_reason(wt)) {
-			if (*wt->lock_reason)
-				printf("locked %s\n", wt->lock_reason);
-			else
+			if (*wt->lock_reason) {
+				char *reason = worktree_escape_reason(wt->lock_reason);
+				printf("locked %s\n", reason);
+				free(reason);
+			} else
 				printf("locked\n");
 		}
 

-- 
2.30.0.391.g469bf2a980

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.

This function will be used by another API function, implemented in the
next patch that will accept a pointer to "worktree" structure directly
making it easier to implement the "prunable" annotations.

should_prune_worktree() knows how to select the given worktree for pruning
based on an expiration date, however the expiration value is stored on a
global 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.

Signed-off-by: Rafael Silva <redacted>
---
 builtin/worktree.c | 75 +---------------------------------------------
 worktree.c         | 73 ++++++++++++++++++++++++++++++++++++++++++++
 worktree.h         | 10 +++++++
 3 files changed, 84 insertions(+), 74 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 197fd24a55..eeb3ffaed0 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 f84ceae87d..5764b0dc7c 100644
--- a/worktree.c
+++ b/worktree.c

@@ -700,3 +700,76 @@ void repair_worktree_at_path(const char *path,
 	strbuf_release(&realdotgit);
 	strbuf_release(&dotgit);
 }
+
+/*
+ * 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.
+ */
+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..e5f4320725 100644
--- a/worktree.h
+++ b/worktree.h

@@ -73,6 +73,16 @@ 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, or NULL if it
+ * cannot be determined. Caller is responsible for freeing returned path.
+ */
+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.391.g469bf2a980

The should_prune_worktree() machinery is used by the "prune" command to
identify whether a worktree is a candidate for pruning. This function
however, is not prepared to work directly with "struct worktree" and
refactoring is required not only on the function itself, but also also
changing get_worktrees() to return non-valid worktrees and address the
changes in all "worktree" sub commands.

Instead let's implement worktree_prune_reason() that accepts
"struct worktree" and uses should_prune_worktree() and returns whether
the given worktree is a candidate for pruning. As the "list" sub command
already uses a list of "struct worktree", this allow to simply check if
the working tree prunable by passing the structure directly without the
others parameters.

Also, let's add prune_reason field to the worktree structure that will
store the reason why the worktree can be pruned that is returned by
should_prune_worktree() when such reason is available.

Signed-off-by: Rafael Silva <redacted>
---
 worktree.c | 19 +++++++++++++++++++
 worktree.h |  7 +++++++
 2 files changed, 26 insertions(+)

diff --git a/worktree.c b/worktree.c
index 5764b0dc7c..ee14db3ab5 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,24 @@ const char *worktree_lock_reason(struct worktree *wt)
 	return wt->lock_reason;
 }
 
+const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire)
+{
+	if (!is_main_worktree(wt)) {
+		char *path;
+		struct strbuf reason = STRBUF_INIT;
+
+		if (should_prune_worktree(wt->id, &reason, &path, expire))
+			wt->prune_reason = strbuf_detach(&reason, NULL);
+		else
+			wt->prune_reason = NULL;
+
+		free(path);
+		strbuf_release(&reason);
+	}
+
+	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 e5f4320725..24ded0f3c6 100644
--- a/worktree.h
+++ b/worktree.h

@@ -11,6 +11,7 @@ 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;

@@ -73,6 +74,12 @@ 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
+ * or NULL otherwise.
+ */
+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, or NULL if it

-- 
2.30.0.391.g469bf2a980

Hi Rafael

Thanks for working on this

On 04/01/2021 16:21, Rafael Silva wrote:

"git worktree list" porcelain format shows one attribute per line, each
attribute is listed with a label and value separated by a single space.
If any worktree is locked, a "locked" attribute is listed followed by the
reason, if available, otherwise only "locked" is shown.

In case the lock reason contains newline characters (i.e: LF or CRLF)
this will cause the format to break as each line should correspond to
one attribute. This leads to unwanted behavior, specially as the
porcelain is intended to be machine-readable. To address this shortcoming
let's teach "git worktree list" to escape any newline character before
outputting the locked reason for porcelain format.

As the porcelain output format cannot handle worktree paths that contain newlines either I think it would be better to add a `-z` flag which uses NUL termination as we have for many other commands (diff, ls-files etc). This would fix the problem forever without having to worry about quoting each time a new field is added.

If you really need to quote the lock reason then please use one of the path quoting routines (probably quote_c_style() or write_name_quoted()) in quote.c rather than rolling your own - the code below gives the same output for a string that contains the two characters `\` followed by `n` as it does for the single character `\n`. People are (hopefully) used to dequoting our path quoting and there are routines out there to do it (we have one git Git.pm) using a function in quote.c will allow them to use those routines here.

Best Wishes

Phillip

From: Phillip Wood <redacted>

Add a -z option to be used in conjunction with --porcelain that gives
NUL-terminated output. This enables 'worktree list --porcelain' to
handle worktree paths that contain newlines.

Signed-off-by: Phillip Wood <redacted>
---
I found an old patch that added NUL termination. I've rebased it
but the new test fails as there seems to be another worktree thats
been added since I wrote this, anyway I thought it might be a useful
start for adding a `-z` option.

 Documentation/git-worktree.txt | 13 +++++++++---
 builtin/worktree.c             | 36 ++++++++++++++++++++++------------
 t/t2402-worktree-list.sh       | 22 +++++++++++++++++++++
 3 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index af06128cc9..a07b593cc3 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt

@@ -10,7 +10,7 @@ SYNOPSIS
 --------
 [verse]
 'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b <new-branch>] <path> [<commit-ish>]
-'git worktree list' [--porcelain]
+'git worktree list' [--porcelain [-z]]
 'git worktree lock' [--reason <string>] <worktree>
 'git worktree move' <worktree> <new-path>
 'git worktree prune' [-n] [-v] [--expire <expire>]

@@ -217,7 +217,13 @@ This can also be set up as the default behaviour by using the
 --porcelain::
 	With `list`, output in an easy-to-parse format for scripts.
 	This format will remain stable across Git versions and regardless of user
-	configuration.  See below for details.
+	configuration.  It is recommended to combine this with `-z`.
+	See below for details.
+
+-z::
+	When `--porcelain` is specified with `list` terminate each line with a
+	NUL rather than a newline. This makes it possible to parse the output
+	when a worktree path contains a newline character.
 
 -q::
 --quiet::

@@ -369,7 +375,8 @@ $ git worktree list
 
 Porcelain Format
 ~~~~~~~~~~~~~~~~
-The porcelain format has a line per attribute.  Attributes are listed with a
+The porcelain format has a line per attribute.  If `-z` is given then the lines
+are terminated with NUL rather than a newline.  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

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 197fd24a55..0cd331873c 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c

@@ -640,19 +640,25 @@ static int add(int ac, const char **av, const char *prefix)
 	return add_worktree(path, branch, &opts);
 }
 
-static void show_worktree_porcelain(struct worktree *wt)
+static void show_worktree_porcelain(struct worktree *wt, int line_terminator)
 {
-	printf("worktree %s\n", wt->path);
-	if (wt->is_bare)
-		printf("bare\n");
-	else {
-		printf("HEAD %s\n", oid_to_hex(&wt->head_oid));
-		if (wt->is_detached)
-			printf("detached\n");
-		else if (wt->head_ref)
-			printf("branch %s\n", wt->head_ref);
+	printf("worktree %s", wt->path);
+	fputc(line_terminator, stdout);
+	if (wt->is_bare) {
+		printf("bare");
+		fputc(line_terminator, stdout);
+	} else {
+		printf("HEAD %s", oid_to_hex(&wt->head_oid));
+		fputc(line_terminator, stdout);
+		if (wt->is_detached) {
+			printf("detached");
+			fputc(line_terminator, stdout);
+		} else if (wt->head_ref) {
+			printf("branch %s", wt->head_ref);
+			fputc(line_terminator, stdout);
+		}
 	}
-	printf("\n");
+	fputc(line_terminator, stdout);
 }
 
 static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)

@@ -720,15 +726,20 @@ static void pathsort(struct worktree **wt)
 static int list(int ac, const char **av, const char *prefix)
 {
 	int porcelain = 0;
+	int line_terminator = '\n';
 
 	struct option options[] = {
 		OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
+		OPT_SET_INT('z', NULL, &line_terminator,
+			    N_("fields are separated with NUL character"), '\0'),
 		OPT_END()
 	};
 
 	ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
 	if (ac)
 		usage_with_options(worktree_usage, options);
+	else if (!line_terminator && !porcelain)
+		die(_("'-z' requires '--porcelain'"));
 	else {
 		struct worktree **worktrees = get_worktrees();
 		int path_maxlen = 0, abbrev = DEFAULT_ABBREV, i;

@@ -741,7 +752,8 @@ static int list(int ac, const char **av, const char *prefix)
 
 		for (i = 0; worktrees[i]; i++) {
 			if (porcelain)
-				show_worktree_porcelain(worktrees[i]);
+				show_worktree_porcelain(worktrees[i],
+							line_terminator);
 			else
 				show_worktree(worktrees[i], path_maxlen, abbrev);
 		}

diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh
index 795ddca2e4..acd9f8ab84 100755
--- a/t/t2402-worktree-list.sh
+++ b/t/t2402-worktree-list.sh

@@ -71,6 +71,28 @@ test_expect_success '"list" all worktrees with locked annotation' '
 	! grep "/unlocked  *[0-9a-f].* locked$" out
 '
 
+test_expect_success '"list" all worktrees --porcelain -z' '
+	test_when_finished "rm -rf here _actual actual expect &&
+				git worktree prune" &&
+	printf "worktree %sQHEAD %sQbranch %sQQ" \
+		"$(git rev-parse --show-toplevel)" \
+		"$(git rev-parse HEAD)" \
+		"$(git symbolic-ref HEAD)" >expect &&
+	git worktree add --detach here master &&
+	printf "worktree %sQHEAD %sQdetachedQQ" \
+		"$(git -C here rev-parse --show-toplevel)" \
+		"$(git rev-parse HEAD)" >>expect &&
+	git worktree list --porcelain -z >_actual &&
+	cat _actual | tr "\0" Q >actual	&&
+	test_cmp expect actual
+'
+
+test_expect_success '"list" -z fails without --porcelain' '
+	test_when_finished "rm -rf here && git worktree prune" &&
+	git worktree add --detach here master &&
+	test_must_fail git worktree list -z
+'
+
 test_expect_success 'bare repo setup' '
 	git init --bare bare1 &&
 	echo "data" >file1 &&

-- 
2.30.0

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

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 address 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 address one shortcoming for porcelain
format to escape any newline characters (LF and CRLF) for the lock reason
to prevent breaking format mentioned in [4] and [1] during the review
cycle.

Thank you for working on this. I'm happy to see these long-envisioned
enhancements finally taking shape. Before even reviewing the patches,
I decided to apply them and play with the new features, and I'm very
pleased to see that they behave exactly as I had envisioned all those
years ago.

Very nicely done.

I'll review the patches when I finish responding to this cover letter.

The first 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 "struct worktree" and load the information directly,
which can 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, if any.

I haven't looked at the patches yet, so I can't respond to this right now.

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].

Sounds good.

The forth patch is the one that teaches `list` command to show prunable
annotation, for both default and porcelain format, and adds the verbose
option. These changes are also done for the "locked" annotation that was
not done on in [1]. Additionally, `list` learns to fail when both
--verbose and --porcelain format as used together.

My knee-jerk response to disallowing --verbose and --porcelain
together was that it seemed unnecessarily harsh, but upon reflection,
I think it's the correct thing to do. After all, porcelain output,
even though extensible and flexible, should be predictable, so it
doesn't make sense for options such as --verbose to affect it.

The fifth patch adds worktree_escape_reason() that accepts a (char *)
text and returned the text with any LF or CRLF escaped. The caller is
responsible to freeing the escaped text. This is used by the locked
annotation in porcelain format. Currently, this is defined within
builtin/worktree.c as I was not sure whether libfying the function as
part of this series is a good idea. At this time it seems more sensible
to leave the code internally and libfying later once we are confident
about the implementation and whether it can be used in other part of the
code base but I'm open for suggestion.

Perhaps I misunderstand, but I had envisioned employing one of the
codebase's existing quoting/escaping functions rather than crafting a
new one from scratch. However, I'll reserve judgment until I actually
read the patch.

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

worktree: move should_prune_worktree() to worktree.c

The subject is a bit confusing since this function already lives in
`worktree.c` (albeit within the `builtin` directory). An alternate way
to word this might be:

    worktree: libify and publish should_prune_worktree()

or just:

    worktree: libify should_prune_worktree()

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.

Good.

This function will be used by another API function, implemented in the
next patch that will accept a pointer to "worktree" structure directly
making it easier to implement the "prunable" annotations.

Okay, though I'm not sure this paragraph adds value since the reader
understands implicitly from the first paragraph that this is the
reason this patch is libifying the function. (I'd probably drop this
paragraph if I was composing the message.)

should_prune_worktree() knows how to select the given worktree for pruning
based on an expiration date, however the expiration value is stored on a
global 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.

Rather than "on a global variable", it would be more accurate to say
"static file-scope variable".

The function documentation which was moved here (worktree.c) from
builtin/worktree.c is duplicated in the header worktree.h. Having it
in the header is good. Having it here in the source file doesn't
necessarily hurt, but I worry about it becoming out-of-sync with the
copy in the header. For that reason, it might make sense to keep it
only in the header.

All of the above review comments are very minor and several of them
are subjective, thus not necessarily worth a re-roll.

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

A few more comments...

It would be good to update the documentation to explain what `expire`
is since it's not necessarily obvious. The documentation could also be
tweaked to say that the worktree's path is returned in `wtpath` rather
than saying only that it is returned. If you choose to make these
changes, they should be probably done in a separate patch from the
patch which moves the code. This is a very minor issue, not
necessarily worth a re-roll.

Now that this is a public function, it might make sense for `wtpath`
to be optional; if the caller is not interested in it, then NULL would
be passed in for this argument. The implementation of
should_prune_worktree() would need to be updated to check that
`wtpath` is not NULL before assigning to it. This change, too, would
be done in a separate patch. However, this is just a "would be nice to
have" item, not at all required, and I don't think it should be added
to this series since it's not needed by anything in this series, thus
would just be noise (wasting your time and reviewer time). It's just
something we can keep in mind for the future.

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

worktree: implement worktree_prune_reason() wrapper

We might be able to give the reader more useful information in the
subject by explaining a bit more the goal of this patch. Perhaps
something like this:

    worktree: teach worktree to lazy-load "prunable" reason

The should_prune_worktree() machinery is used by the "prune" command to
identify whether a worktree is a candidate for pruning. This function
however, is not prepared to work directly with "struct worktree" and
refactoring is required not only on the function itself, but also also
changing get_worktrees() to return non-valid worktrees and address the
changes in all "worktree" sub commands.

Instead let's implement worktree_prune_reason() that accepts
"struct worktree" and uses should_prune_worktree() and returns whether
the given worktree is a candidate for pruning. As the "list" sub command
already uses a list of "struct worktree", this allow to simply check if
the working tree prunable by passing the structure directly without the
others parameters.

Everything through "not prepared to work directly with `struct
worktree`" makes sense when explaining why you are adding this wrapper
function, however, most of what follows is describing an aborted idea
about how you originally intended to implement this. The bit about
get_worktrees() not being able to return non-valid worktrees is
certainly an important limitation in the overall scheme of things, but
doesn't really help to sell the change made by this patch, and it
probably confuses the reader who didn't also read the cover-letter,
especially since this patch does nothing to help that situation.

It also isn't really necessary to talk about `git worktree list` at
this stage since this patch stands on its own by fleshing out the API
without having to cite a specific client.

Also, let's add prune_reason field to the worktree structure that will
store the reason why the worktree can be pruned that is returned by
should_prune_worktree() when such reason is available.

In my opinion, this is the real reason this patch exists, thus should
be the focus of the commit message. All the description above this
paragraph can likely be dropped.

Taking the above comments into account, perhaps the entire commit
message could be collapsed to something like this:

    worktree: teach worktree to lazy-load "prunable" reason

    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.

Remembering to free the prune-reason. Good.

A couple observations...

I realize you patterned this after worktree_lock_reason(), however, it
is more common in this codebase to return early from the function for
conditions such as `is_main_worktree(wt)` which don't require any
additional processing. One reason is that doing so allows us to lose
an indentation level. Another is that it is easier to reason about the
rest of the function if we get the simple cases out of the way early,
such that we don't have to think about them again while reading the
remainder of the code.

If I'm not mistaken, the intention here was to cache `prune_reason`
for reuse, however, this function just overwrites it each time it's
called for a particular worktree, thus providing no caching and
leaking the previously-retrieved reason as well. To fix this, I think
you need to add a private `prune_reason_valid` member to `struct
worktree` (similar to `lock_reason_valid`) and check it before calling
should_prune_worktree().

Taking the above comments into account, perhaps it should be written like this:

    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;
    free(path);
    strbuf_release(&reason);

As noted above, we also probably need a new `prune_reason_valid`
member, similar to the existing `lock_reason_valid`.

The documentation should also talk about `expire` since its purpose
and meaning is unclear.

Nit: I realize that you patterned this description after the one for
worktree_lock_reason(), but it's a bit unclear. Perhaps rephrasing it
like this would help:

    Return the reason the worktree should be pruned, otherwise
    NULL if it should not be pruned.

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

The main worktree should not be locked and the worktree_lock_reason() API
is aware of this fact and avoids running the check code for the main
worktree. This checks is done via assert() macro, Therefore the caller

s/Therefore/therefore/

needs to ensure the function is never called, usually by additional code.

We can handle that case more gently by just returning false for the main
worktree and not bother checking if the "locked" file exists. This will
allowed further simplification from the caller as they will not need to
ensure the main worktree is never passed to the API.

Teach worktree_lock_reason() to be more gently and just return false for
the main working tree.

The situation is even a bit worse since the main worktree restriction
isn't even documented. Here's a possible rewrite of the commit message
which addresses that point too:

    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
    callers to have inside knowledge about the implementation:

        if (worktree_locked_reason(...))
            ...

As mentioned in my review of patch [2/7], this would be more idiomatic
and easier to reason about if the function returns early for the main
worktree case, thus freeing the reader from having to think about that
case for the remainder of the code. So:

    if (is_main_worktree(wt))
        return NULL;
    if (!wt->lock_reason_valid) {
        ...

Subjective, and not necessarily worth a re-roll, though.

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] 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. It is not clear whether a worktree,
if any, is prunable.

Maybe this could just say...

    ... "locked" annotation if the worktree is locked,
    however, it does not indicate whether it is prunable.

The "prune" command will remove a worktree in case
is a prunable candidate unless --dry-run option is specified. This could

s/case is/case it is/

Or better:

    ... will remove a worktree if it is prunable unless...

lead to a worktree being removed without the user realizing before is to
late, in case the user forgets to pass --dry-run for instance.

s/before is/before it is/
s/to/too/

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 "accidently" on the worse case scenario.

Let's teach "git worktree list" to show when a worktree is prunable by
appending "prunable" text to its output by default and show a prunable
label for the porcelain format followed by the reason, if the avaiable.
While at it, let's do the same for the "locked" annotation.

s/avaiable/available/

Also, the worktree_prune_reason() stores the reason why git is selecting
the worktree to be pruned, so let's leverage that and also display this
information to the user. However, the reason is human readable text that
can take virtually any size which might make harder to extend the "list"
command with additional annotations and not fit nicely on the screen.

In order to address this shortcoming, let's teach "git worktree list" to
take a verbose option that will output the prune reason on the next line
indented, if the reason is available, otherwise the annotation is kept on
the same line. While at it, let's do the same for the "locked"
annotation.

This is a lot of changes for one patch to be making, and it's hard for
a reader to digest all those changes from the commit message. I think
I counted four distinct changes being made here:

1. extend porcelain to include lock line (with optional reason)
2. add prunable annotation to normal list output
3. add prune line (with optional reason) to porcelain output
4. extend normal list output with --verbose to include reasons

The patch itself is not overly large or complicated, so perhaps
combining all these changes together is reasonable, although I'm quite
tempted to ask for them to be separated into at least four patches
(probably in the order shown above). A benefit of splitting them into
distinct patches is that you can add targeted documentation and test
updates to each individual patch rather than saving all the
documentation and test updates for a single (large) patch at the end
of the series. This helps reviewers reason about the changes more
easily since they get to see how each change impacts the documentation
and tests rather than having to wait for a patch late in the series to
make all the documentation and test updates, at which point the
reviewers may have forgotten details of the earlier patches.

(I've come back here after reading and reviewing the patch itself, and
I'm on the fence as to whether to suggest splitting this into multiple
patches. The changes in this patch are easy enough to understand and
digest, so I'm not convinced it makes sense to ask you to do all the
extra work of splitting it into smaller pieces. On the other hand, it
might be nice to have each documentation and test update done in the
patch which makes each particular change. So, I don't have a good
answer. Use your best judgment and do the amount of work you feel is
appropriate.)

The output of "git worktree list" with verbose becomes like so:

    $ git worktree list --verbose
    /path/to/main             aba123 [main]
    /path/to/locked           acb124 [branch-a] locked
    /path/to/locked-reason    acc125 [branch-b]
        locked: worktree with locked reason
    /path/to/prunable         acd126 [branch-c] prunable
    /path/to/prunable-reason  ace127 [branch-d]
        prunable: gitdir file points to non-existent location

One "weird" aesthetic issue is that if the lock reason contains
newlines, the subsequent lines of the lock reason are not indented.
However, this is a very minor point and I don't think this patch
series should worry about it. We can think about how to address it
some time in the future if someone ever complains about it.

A couple observations...

As a consumer of `struct worktree`, builtin/worktree.c should not be
poking at or accessing the structure's private fields `prune_reason`
and `lock_reason`; instead it should be retrieving those values via
worktree_prune_reason() and worktree_lock_reason() which are part of
the public API.

If a worktree is prunable, then worktree_prune_reason() will
unconditionally return a (non-empty) string; if it is not prunable,
then it will unconditionally return NULL. This means that the
`printf("prunable\n")` case is dead-code; it will never be reached.
This differs from worktree_lock_reason() which can return an empty
string to indicate a locked worktree for which no reason has been
specified.

Taking the above observations into account, a reasonable rewrite would be:

    const char *reason;

    reason = worktree_lock_reason(wt);
    if (reason && *reason)
        printf("locked %s\n", reason);
    else if (reason)
        printf("locked\n");

    reason = worktree_prune_reason(wt, expire);
    if (reason)
        printf("prunable %s\n", reason);

Same observations here about using public API rather than touching
private `struct worktree` fields, and about the final `else` case
being dead code.

Perhaps:

    "add 'prunable' annotation to worktrees older than <time>"

Good, this mirrors how prune() initializes this variable.

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

"git worktree list" porcelain format shows one attribute per line, each
attribute is listed with a label and value separated by a single space.
If any worktree is locked, a "locked" attribute is listed followed by the
reason, if available, otherwise only "locked" is shown.

In case the lock reason contains newline characters (i.e: LF or CRLF)
this will cause the format to break as each line should correspond to
one attribute. This leads to unwanted behavior, specially as the
porcelain is intended to be machine-readable. To address this shortcoming
let's teach "git worktree list" to escape any newline character before
outputting the locked reason for porcelain format.

s/specially/especially/

The `*(r +1)` in the middle of the condition is redundant. The same
case is already handled by the `*(r + 1) == '\n'` which follows it
since EOL ('\0') won't match '\n'.

As Phillip already mentioned upstream, we can use one of the functions
from quote.c rather than rolling out own here. quote_c_style(), as
Phillip suggested, is almost certainly the best choice for a couple
reasons. First, when called with CQUOTE_NODQ, it adds quotes around
the string if there are any characters which need to be escaped, but
doesn't quote the string if it contains no special characters. This
makes the output more nicely readable in the typical case when there
are no special characters, and makes it possible to distinguish the
case Phillip pointed out between literal two characters "\n" in a
string and '\n' representing a newline. Second, consumers of our
porcelain are already used to consuming strings produced by
quote_c_style().

It would be preferable to fold this change directly into the preceding
patch, thus eliminating this patch altogether. That way this patch
series doesn't introduce a state in which the behavior is knowingly
"broken", but instead is correct from the start. Also, since you can
use the existing quote_c_style(), the change made by this patch
becomes tiny, thus is easily folded into the earlier patch.

On Tue, Jan 5, 2021 at 5:29 AM Phillip Wood [off-list ref] wrote:

On 04/01/2021 16:21, Rafael Silva wrote:

quoted

In case the lock reason contains newline characters (i.e: LF or CRLF)
this will cause the format to break as each line should correspond to
one attribute. This leads to unwanted behavior, specially as the
porcelain is intended to be machine-readable. To address this shortcoming
let's teach "git worktree list" to escape any newline character before
outputting the locked reason for porcelain format.

As the porcelain output format cannot handle worktree paths that contain
newlines either I think it would be better to add a `-z` flag which uses
NUL termination as we have for many other commands (diff, ls-files etc).
This would fix the problem forever without having to worry about quoting
each time a new field is added.

Adding a `-z` flag makes sense, but that doesn't mean the existing
`--porcelain` is meaningless without it. I see `-z` as a good
complement to `--porcelain`.

Yes, we should fix the problem that the existing porcelain output
doesn't escape the path, but that can be done as a separate bug fix;
it doesn't need to be rolled into this series.

If you really need to quote the lock reason then please use one of the
path quoting routines (probably quote_c_style() or write_name_quoted())
in quote.c rather than rolling your own - the code below gives the same
output for a string that contains the two characters `\` followed by `n`
as it does for the single character `\n`. People are (hopefully) used to
dequoting our path quoting and there are routines out there to do it (we
have one git Git.pm) using a function in quote.c will allow them to use
those routines here.

I was going to suggest the same. Thanks.

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

You might need to be a bit more careful here. If the test fails before
the worktree is locked, then the `git worktree unlock` in the cleanup
code will return an error, which will make the code executed by
test_when_finished() fail, as well, which makes it harder to debug
problems. Moving the `unlock` cleanup after you know the lock
succeeded will address this issue:

    test_when_finished "rm -rf locked unlocked out && git worktree prune" &&
    git worktree add --detach locked master &&
    git worktree add --detach unlocked master &&
    git worktree lock locked &&
    test_when_finished "git worktree unlock locked" &&
    ...

Same comment applies to other new tests added by this patch which lock
worktrees.

To be trustworthy, doesn't this test also need to have an unprunable
worktree, and test that `git worktree list` doesn't annotate it as
"prunable" _and_ that `git worktree prune` didn't prune it? Otherwise,
we really don't know that the two commands agree about what is and is
not prunable -- we only know they agree that this particular worktree
was prunable.

Nit: It's not a big deal, but we don't normally bother cleaning up
every junk file in tests, such as `out`, `actual`, `expect` if those
files aren't going to be a problem for subsequent tests. We are
explicitly cleaning up the worktrees in these tests because this
script is all about testing worktree behavior, and some random
leftover worktree could be a problem for other tests. I don't care
strongly one way or the other, but I worry a tiny bit that the list of
files being cleaned up could become outdated as changes are made to
the tests later...

... for instance, the file `list` being cleaned up in this test is not
even created by this test.

Nit: This test title could probably be shortened to:

    '"list' --verbose and --porcelain mutually exclusive' '

Finally, I haven't tried it myself, but I was wondering if it is
possible to make a test which shows both "locked" and "prunable"
annotations for the same worktree. Would that be possible by
specifying a particular value for `--expire`? If it's too much work,
don't worry about it.

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

This leaves the reader wondering what additional information is output
for `list`. Perhaps a small tweak will help:

    With `list`, output additional information about worktrees (see below).

Perhaps just minimal:

    With `list`, annotate missing worktrees as prunable if they
    are older than `<time>`.

s/any/the/g

We might want to use backticks around these annotations rather than
double quotes, and we certainly do want to use backticks around the
`git worktree prune` command to ensure it is styled consistently with
other commands in this document.

+Note that, the annotation is only moved to the next line only if the
+additional text is available, otherwise the text is kept on the same.

Drop the comma between "that, the". Also, too many "only"s in this
sentence. You can actually drop both of them and the sentence will
still read fine:

   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 worktree itself.

or something.

s/(See/(see/
s/is also/are also/

Let's also use backticks rather than double quotes around `locked` and
`prunable` to ensure consistent formatting with the other porcelain
labels `bare` and `detached` which are already in backticks.

Also, the unnecessary `+` line (seen as `++` in the diff) makes this
render incorrectly. It renders as:

    +In case any...

To fix it, just leave the line blank between paragraphs.

(If possible, install `asciidoc` and `xmlto` and then run `make html`
to render the documentation yourself, and open
Documentation/git-worktree.html in your browser to check the output.)

On Tue, Jan 5, 2021 at 6:02 AM Phillip Wood [off-list ref] wrote:

Add a -z option to be used in conjunction with --porcelain that gives
NUL-terminated output. This enables 'worktree list --porcelain' to
handle worktree paths that contain newlines.

Adding a -z mode makes a lot of sense. This, along with a fix to call
quote_c_style() on paths in normal (not `-z`) porcelain mode, can
easily be done after Rafael's series lands. Or they could be done
before his series, though that might make a lot of extra work for him.

Signed-off-by: Phillip Wood <redacted>
---
I found an old patch that added NUL termination. I've rebased it
but the new test fails as there seems to be another worktree thats
been added since I wrote this, anyway I thought it might be a useful
start for adding a `-z` option.

The test failure is fallout from a "bug" in a test added by Rafael's
earlier series[1] which was not caught by the reviewer[2]. In fact,
his current series has a drive-by fix[3] for this bug in patch [6/7].
Applying that fix (or the refinement[4] I suggested in my review)
makes your test work.

[1]: https://lore.kernel.org/git/20201011101152.5291-2-rafaeloliveira.cs@gmail.com/
[2]: https://lore.kernel.org/git/CAPig+cS8hvX4GCsnfLBnQ4Q_AkUad=bw7rjVcaOqSEqcLZvx8w@mail.gmail.com/
[3]: https://lore.kernel.org/git/20210104162128.95281-7-rafaeloliveira.cs@gmail.com/
[4]: https://lore.kernel.org/git/CAPig+cRysXpK0e1xXOuVd+EtkeyTk8FR6MUrL=Bg3W4ve8jdNA@mail.gmail.com/

If we fix normal-mode porcelain to call quote_c_style(), then we can
drop the last sentence or refine it to say something along the lines
of it being easier to deal with paths with embedded newlines than in
normal porcelain mode.

Perhaps this could all be done a bit more concisely with printf()
alone rather than combining it with fputc(). For instance:

    printf("worktree %s%c", wt->path, line_terminator);

and so on.

-       printf("\n");
+       fputc(line_terminator, stdout);

This use of fputc() makes sense.

"fields" sounds a little odd. "lines" might make more sense. "records"
might be even better and matches the wording of some other Git
commands accepting `-z`.

+       else if (!line_terminator && !porcelain)
+               die(_("'-z' requires '--porcelain'"));

Other error messages in this file don't quote command-line options, so:

    die(_("-z requires --porcelain"));

would be more consistent.

Or just use nul_to_q():

    nul_to_q <_actual >actual &&

(And there's no need to `cat` the file.)

I don't think there's any need for this test to create (and cleanup) a
worktree. So, the entire test could collapse to:

    test_expect_success '"list" -z fails without --porcelain' '
        test_must_fail git worktree list -z
    '

On Wed, Jan 6, 2021 at 4:39 AM Eric Sunshine [off-list ref] wrote:

Finally, I haven't tried it myself, but I was wondering if it is
possible to make a test which shows both "locked" and "prunable"
annotations for the same worktree. Would that be possible by
specifying a particular value for `--expire`? If it's too much work,
don't worry about it.

Ignore this stupid question/suggestion. A locked worktree will never
be considered prunable. Don't know what I was thinking when I asked
it.

On Wed, Jan 6, 2021 at 1:55 AM Eric Sunshine [off-list ref] wrote:

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

quoted

+/*
+ * 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.
+ */

It would be good to update the documentation to explain what `expire`
is since it's not necessarily obvious. The documentation could also be
tweaked to say that the worktree's path is returned in `wtpath` rather
than saying only that it is returned. If you choose to make these
changes, they should be probably done in a separate patch from the
patch which moves the code. This is a very minor issue, not
necessarily worth a re-roll.

On second thought, adding a patch just to make a minor update to the
function comment is probably overkill. It should be okay to do it in
this patch. Mention the change in the commit to alert reviewers.

Eric Sunshine writes:

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

quoted

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 address 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 address one shortcoming for porcelain
format to escape any newline characters (LF and CRLF) for the lock reason
to prevent breaking format mentioned in [4] and [1] during the review
cycle.

Thank you for working on this. I'm happy to see these long-envisioned
enhancements finally taking shape. Before even reviewing the patches,
I decided to apply them and play with the new features, and I'm very
pleased to see that they behave exactly as I had envisioned all those
years ago.

Very nicely done.

Thank you. I'm glad to hear the patches are aligned with what you
envisioned.

I'll review the patches when I finish responding to this cover letter.

Thank you for reviewing and applying the patches, really appreciate it.

quoted

The fifth patch adds worktree_escape_reason() that accepts a (char *)
text and returned the text with any LF or CRLF escaped. The caller is
responsible to freeing the escaped text. This is used by the locked
annotation in porcelain format. Currently, this is defined within
builtin/worktree.c as I was not sure whether libfying the function as
part of this series is a good idea. At this time it seems more sensible
to leave the code internally and libfying later once we are confident
about the implementation and whether it can be used in other part of the
code base but I'm open for suggestion.

Perhaps I misunderstand, but I had envisioned employing one of the
codebase's existing quoting/escaping functions rather than crafting a
new one from scratch. However, I'll reserve judgment until I actually
read the patch.

Agreed. It make sense to reuse one of the already implemented functions
from the code base. for some reason I was not able to find it. I believe
this was cleared out in one of the patches replies by you and Phillip Wood.

I will remove this and reuse one of the existing function on the next
revision.

-- 
Thanks
Rafael

Thank you for the detailed review.

Eric Sunshine writes:

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

quoted

worktree: move should_prune_worktree() to worktree.c

The subject is a bit confusing since this function already lives in
`worktree.c` (albeit within the `builtin` directory). An alternate way
to word this might be:

    worktree: libify and publish should_prune_worktree()

or just:

    worktree: libify should_prune_worktree()

yes, I think these seems clearer. I will drop the current subject and
use one of these on the next revision.

quoted

This function will be used by another API function, implemented in the
next patch that will accept a pointer to "worktree" structure directly
making it easier to implement the "prunable" annotations.

Okay, though I'm not sure this paragraph adds value since the reader
understands implicitly from the first paragraph that this is the
reason this patch is libifying the function. (I'd probably drop this
paragraph if I was composing the message.)

Make sense.

quoted

should_prune_worktree() knows how to select the given worktree for pruning
based on an expiration date, however the expiration value is stored on a
global 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.

Rather than "on a global variable", it would be more accurate to say
"static file-scope variable".

Good point. I will definitely change that to be more accurate on the next
revision.   

quoted

diff --git a/worktree.c b/worktree.c

@@ -700,3 +700,76 @@ void repair_worktree_at_path(const char *path,
+/*
+ * 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.
+ */
+int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, timestamp_t expire)

The function documentation which was moved here (worktree.c) from
builtin/worktree.c is duplicated in the header worktree.h. Having it
in the header is good. Having it here in the source file doesn't
necessarily hurt, but I worry about it becoming out-of-sync with the
copy in the header. For that reason, it might make sense to keep it
only in the header.

All of the above review comments are very minor and several of them
are subjective, thus not necessarily worth a re-roll.

Make sense as well. 

-- 
Thanks
Rafael

Eric Sunshine writes:

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

quoted

diff --git a/worktree.h b/worktree.h

@@ -73,6 +73,16 @@ int is_main_worktree(const 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, or NULL if it
+ * cannot be determined. Caller is responsible for freeing returned path.
+ */
+int should_prune_worktree(const char *id,
+                         struct strbuf *reason,
+                         char **wtpath,
+                         timestamp_t expire);

A few more comments...

It would be good to update the documentation to explain what `expire`
is since it's not necessarily obvious. The documentation could also be
tweaked to say that the worktree's path is returned in `wtpath` rather
than saying only that it is returned. If you choose to make these
changes, they should be probably done in a separate patch from the
patch which moves the code. This is a very minor issue, not
necessarily worth a re-roll.

Good idea. I will include these changes on the next revision. specially
the documentation about the `expire` field.

Now that this is a public function, it might make sense for `wtpath`
to be optional; if the caller is not interested in it, then NULL would
be passed in for this argument. The implementation of
should_prune_worktree() would need to be updated to check that
`wtpath` is not NULL before assigning to it. This change, too, would
be done in a separate patch. However, this is just a "would be nice to
have" item, not at all required, and I don't think it should be added
to this series since it's not needed by anything in this series, thus
would just be noise (wasting your time and reviewer time). It's just
something we can keep in mind for the future.

Agreed. These seems like reasonable changes and might not be good fit
for this patches but definitely something that we can follow up after it.

-- 
Thanks
Rafael

Eric Sunshine writes:

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

quoted

worktree: implement worktree_prune_reason() wrapper

We might be able to give the reader more useful information in the
subject by explaining a bit more the goal of this patch. Perhaps
something like this:

    worktree: teach worktree to lazy-load "prunable" reason

yeah, that's sounds better. will change it on the next revision.

quoted

The should_prune_worktree() machinery is used by the "prune" command to
identify whether a worktree is a candidate for pruning. This function
however, is not prepared to work directly with "struct worktree" and
refactoring is required not only on the function itself, but also also
changing get_worktrees() to return non-valid worktrees and address the
changes in all "worktree" sub commands.

Instead let's implement worktree_prune_reason() that accepts
"struct worktree" and uses should_prune_worktree() and returns whether
the given worktree is a candidate for pruning. As the "list" sub command
already uses a list of "struct worktree", this allow to simply check if
the working tree prunable by passing the structure directly without the
others parameters.

Everything through "not prepared to work directly with `struct
worktree`" makes sense when explaining why you are adding this wrapper
function, however, most of what follows is describing an aborted idea
about how you originally intended to implement this. The bit about
get_worktrees() not being able to return non-valid worktrees is
certainly an important limitation in the overall scheme of things, but
doesn't really help to sell the change made by this patch, and it
probably confuses the reader who didn't also read the cover-letter,
especially since this patch does nothing to help that situation.

It also isn't really necessary to talk about `git worktree list` at
this stage since this patch stands on its own by fleshing out the API
without having to cite a specific client.

Make sense.

quoted

Also, let's add prune_reason field to the worktree structure that will
store the reason why the worktree can be pruned that is returned by
should_prune_worktree() when such reason is available.

In my opinion, this is the real reason this patch exists, thus should
be the focus of the commit message. All the description above this
paragraph can likely be dropped.

Taking the above comments into account, perhaps the entire commit
message could be collapsed to something like this:

    worktree: teach worktree to lazy-load "prunable" reason

    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.

Interesting point. Rewording the commit message like this seems better,
and as you mentioned this patch can stands on its own and can be more
clear about what the commit is introducing to the code instead of all
the previous message trying to explain why this exists.

Thank you for suggesting this commit message. I will revise and change on
the next revision.

quoted

diff --git a/worktree.c b/worktree.c

@@ -15,6 +15,7 @@ void free_worktrees(struct worktree **worktrees)
                free(worktrees[i]->lock_reason);
+               free(worktrees[i]->prune_reason);
                free(worktrees[i]);

Remembering to free the prune-reason. Good.

quoted

@@ -245,6 +246,24 @@ const char *worktree_lock_reason(struct worktree *wt)
+const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire)
+{
+       if (!is_main_worktree(wt)) {
+               char *path;
+               struct strbuf reason = STRBUF_INIT;
+
+               if (should_prune_worktree(wt->id, &reason, &path, expire))
+                       wt->prune_reason = strbuf_detach(&reason, NULL);
+               else
+                       wt->prune_reason = NULL;
+
+               free(path);
+               strbuf_release(&reason);
+       }
+
+       return wt->prune_reason;
+}

A couple observations...

I realize you patterned this after worktree_lock_reason(), however, it
is more common in this codebase to return early from the function for
conditions such as `is_main_worktree(wt)` which don't require any
additional processing. One reason is that doing so allows us to lose
an indentation level. Another is that it is easier to reason about the
rest of the function if we get the simple cases out of the way early,
such that we don't have to think about them again while reading the
remainder of the code.

If I'm not mistaken, the intention here was to cache `prune_reason`
for reuse, however, this function just overwrites it each time it's
called for a particular worktree, thus providing no caching and
leaking the previously-retrieved reason as well. To fix this, I think
you need to add a private `prune_reason_valid` member to `struct
worktree` (similar to `lock_reason_valid`) and check it before calling
should_prune_worktree().

Good point. I totally missed the caching aspect of the implementation
and I greed about getting the simple case out earlier in order to make it
simple to reason about it.

Taking the above comments into account, perhaps it should be written like this:

    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;
    free(path);
    strbuf_release(&reason);

Thanks. I will add this suggestion in the next revision. 

quoted

diff --git a/worktree.h b/worktree.h

@@ -11,6 +11,7 @@ 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 */

As noted above, we also probably need a new `prune_reason_valid`
member, similar to the existing `lock_reason_valid`.

Indeed. will add on the next revision.

quoted

@@ -73,6 +74,12 @@ int is_main_worktree(const struct worktree *wt);
+/*
+ * Return the reason string if the given worktree should be pruned
+ * or NULL otherwise.
+ */
+const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire);

The documentation should also talk about `expire` since its purpose
and meaning is unclear.

Good point. I missed adding the message here for the `expire` parameter,
will add it. 

Nit: I realize that you patterned this description after the one for
worktree_lock_reason(), but it's a bit unclear. Perhaps rephrasing it
like this would help:

    Return the reason the worktree should be pruned, otherwise
    NULL if it should not be pruned.

Also make sense to rephrase like this.


Thank you for this review will address all this changes on the v2.

-- 
Thanks
Rafael

Eric Sunshine writes:

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

quoted

The main worktree should not be locked and the worktree_lock_reason() API
is aware of this fact and avoids running the check code for the main
worktree. This checks is done via assert() macro, Therefore the caller

s/Therefore/therefore/

Nice catch. thanks.

quoted

needs to ensure the function is never called, usually by additional code.

We can handle that case more gently by just returning false for the main
worktree and not bother checking if the "locked" file exists. This will
allowed further simplification from the caller as they will not need to
ensure the main worktree is never passed to the API.

Teach worktree_lock_reason() to be more gently and just return false for
the main working tree.

The situation is even a bit worse since the main worktree restriction
isn't even documented. Here's a possible rewrite of the commit message
which addresses that point too:

    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
    callers to have inside knowledge about the implementation:

        if (worktree_locked_reason(...))
            ...

Yes, this is a nicer commit message to explain the current situation and
having the code example in there makes even more clearer. Thanks for the
suggestion will definitely make this change on the next revision.

quoted

Signed-off-by: Rafael Silva <redacted>
---

diff --git a/worktree.c b/worktree.c

@@ -225,9 +225,7 @@ int is_main_worktree(const struct worktree *wt)
 const char *worktree_lock_reason(struct worktree *wt)
 {
-       assert(!is_main_worktree(wt));
-
-       if (!wt->lock_reason_valid) {
+       if (!is_main_worktree(wt) && !wt->lock_reason_valid) {
                struct strbuf path = STRBUF_INIT;

As mentioned in my review of patch [2/7], this would be more idiomatic
and easier to reason about if the function returns early for the main
worktree case, thus freeing the reader from having to think about that
case for the remainder of the code. So:

    if (is_main_worktree(wt))
        return NULL;
    if (!wt->lock_reason_valid) {
        ...

Subjective, and not necessarily worth a re-roll, though.

I'm inclined to change here on the next revision as I will do the same
for the patch [2/7] with the worktree_prune_reason() and use the same
pattern for both functions.

-- 
Thanks
Rafael

Eric Sunshine writes:

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

quoted

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. It is not clear whether a worktree,
if any, is prunable.

Maybe this could just say...

    ... "locked" annotation if the worktree is locked,
    however, it does not indicate whether it is prunable.

Make sense.

quoted

The "prune" command will remove a worktree in case
is a prunable candidate unless --dry-run option is specified. This could

s/case is/case it is/

Or better:

    ... will remove a worktree if it is prunable unless...

Yeah, it seems better.

quoted

lead to a worktree being removed without the user realizing before is to
late, in case the user forgets to pass --dry-run for instance.

s/before is/before it is/
s/to/too/

Nice catch, thanks.

quoted

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 "accidently" on the worse case scenario.

Let's teach "git worktree list" to show when a worktree is prunable by
appending "prunable" text to its output by default and show a prunable
label for the porcelain format followed by the reason, if the avaiable.
While at it, let's do the same for the "locked" annotation.

s/avaiable/available/

Another good catch, thanks. 

quoted

Also, the worktree_prune_reason() stores the reason why git is selecting
the worktree to be pruned, so let's leverage that and also display this
information to the user. However, the reason is human readable text that
can take virtually any size which might make harder to extend the "list"
command with additional annotations and not fit nicely on the screen.

In order to address this shortcoming, let's teach "git worktree list" to
take a verbose option that will output the prune reason on the next line
indented, if the reason is available, otherwise the annotation is kept on
the same line. While at it, let's do the same for the "locked"
annotation.

This is a lot of changes for one patch to be making, and it's hard for
a reader to digest all those changes from the commit message. I think
I counted four distinct changes being made here:

1. extend porcelain to include lock line (with optional reason)
2. add prunable annotation to normal list output
3. add prune line (with optional reason) to porcelain output
4. extend normal list output with --verbose to include reasons

The patch itself is not overly large or complicated, so perhaps
combining all these changes together is reasonable, although I'm quite
tempted to ask for them to be separated into at least four patches
(probably in the order shown above). A benefit of splitting them into
distinct patches is that you can add targeted documentation and test
updates to each individual patch rather than saving all the
documentation and test updates for a single (large) patch at the end
of the series. This helps reviewers reason about the changes more
easily since they get to see how each change impacts the documentation
and tests rather than having to wait for a patch late in the series to
make all the documentation and test updates, at which point the
reviewers may have forgotten details of the earlier patches.

(I've come back here after reading and reviewing the patch itself, and
I'm on the fence as to whether to suggest splitting this into multiple
patches. The changes in this patch are easy enough to understand and
digest, so I'm not convinced it makes sense to ask you to do all the
extra work of splitting it into smaller pieces. On the other hand, it
might be nice to have each documentation and test update done in the
patch which makes each particular change. So, I don't have a good
answer. Use your best judgment and do the amount of work you feel is
appropriate.)

Interesting point. During the creation of this series it was not clear
to me how to properly split them in a way that was easier to review it.

I'm inclined to split the series in the way that you are suggesting,
specially about having the tests and documentation together. I believe
this also make sense to reason about in the future when looking over the
commit history of the project without needing to go back and forth
to understand the documentation vs implementation vs testing.

quoted

The output of "git worktree list" with verbose becomes like so:

    $ git worktree list --verbose
    /path/to/main             aba123 [main]
    /path/to/locked           acb124 [branch-a] locked
    /path/to/locked-reason    acc125 [branch-b]
        locked: worktree with locked reason
    /path/to/prunable         acd126 [branch-c] prunable
    /path/to/prunable-reason  ace127 [branch-d]
        prunable: gitdir file points to non-existent location

One "weird" aesthetic issue is that if the lock reason contains
newlines, the subsequent lines of the lock reason are not indented.
However, this is a very minor point and I don't think this patch
series should worry about it. We can think about how to address it
some time in the future if someone ever complains about it.

Yes, that is something that was scratching my head and it was not clear
whether something that we should roll out with this patch. But I agree,
that we can address this time in the future.

quoted

Signed-off-by: Rafael Silva <redacted>
---
 builtin/worktree.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index eeb3ffaed0..dedd4089e5 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c

@@ -578,6 +578,20 @@ static void show_worktree_porcelain(struct worktree *wt)
+               if (worktree_lock_reason(wt)) {
+                       if (*wt->lock_reason)
+                               printf("locked %s\n", wt->lock_reason);
+                       else
+                               printf("locked\n");
+               }
+
+               if (worktree_prune_reason(wt, expire)) {
+                       if (*wt->prune_reason)
+                               printf("prunable %s\n", wt->prune_reason);
+                       else
+                               printf("prunable\n");
+               }

A couple observations...

As a consumer of `struct worktree`, builtin/worktree.c should not be
poking at or accessing the structure's private fields `prune_reason`
and `lock_reason`; instead it should be retrieving those values via
worktree_prune_reason() and worktree_lock_reason() which are part of
the public API.

Make sense.

If a worktree is prunable, then worktree_prune_reason() will
unconditionally return a (non-empty) string; if it is not prunable,
then it will unconditionally return NULL. This means that the
`printf("prunable\n")` case is dead-code; it will never be reached.
This differs from worktree_lock_reason() which can return an empty
string to indicate a locked worktree for which no reason has been
specified.

Great point. I will remove the dead-code from the implementation as that
state is never going to be reached given the current implementation of
worktree_prune_reason().

Thanks.

Taking the above observations into account, a reasonable rewrite would be:

    const char *reason;

    reason = worktree_lock_reason(wt);
    if (reason && *reason)
        printf("locked %s\n", reason);
    else if (reason)
        printf("locked\n");

    reason = worktree_prune_reason(wt, expire);
    if (reason)
        printf("prunable %s\n", reason);

Yeah, this suggested snippet seems like sensible rewrite and much easier
to reason about.

quoted

@@ -604,8 +618,19 @@ 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))
-               strbuf_addstr(&sb, " locked");
+       if (worktree_lock_reason(wt)) {
+               if (verbose && *wt->lock_reason)
+                       strbuf_addf(&sb, "\n\tlocked: %s", wt->lock_reason);
+               else
+                       strbuf_addstr(&sb, " locked");
+       }
+
+       if (worktree_prune_reason(wt, expire)) {
+               if (verbose && *wt->prune_reason)
+                       strbuf_addf(&sb, "\n\tprunable: %s", wt->prune_reason);
+               else
+                       strbuf_addstr(&sb, " prunable");
+       }

Same observations here about using public API rather than touching
private `struct worktree` fields, and about the final `else` case
being dead code.

Make sense.

quoted

@@ -650,12 +675,18 @@ static int list(int ac, const char **av, const char *prefix)
                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_("show working trees that is candidate to be pruned older than <time>")),

Perhaps:

    "add 'prunable' annotation to worktrees older than <time>"

Make sense.

quoted

                OPT_END()
        };

+       expire = TIME_MAX;

Good, this mirrors how prune() initializes this variable.

-- 
Thanks
Rafael

Hi Phillip,

Thank you for reviewing the patch and the suggestions.

Phillip Wood writes:

On 04/01/2021 16:21, Rafael Silva wrote:

quoted

"git worktree list" porcelain format shows one attribute per line, each
attribute is listed with a label and value separated by a single space.
If any worktree is locked, a "locked" attribute is listed followed by the
reason, if available, otherwise only "locked" is shown.
In case the lock reason contains newline characters (i.e: LF or
CRLF)
this will cause the format to break as each line should correspond to
one attribute. This leads to unwanted behavior, specially as the
porcelain is intended to be machine-readable. To address this shortcoming
let's teach "git worktree list" to escape any newline character before
outputting the locked reason for porcelain format.

As the porcelain output format cannot handle worktree paths that
contain newlines either I think it would be better to add a `-z` flag
which uses NUL termination as we have for many other commands (diff,
ls-files etc). This would fix the problem forever without having to
worry about quoting each time a new field is added.

This is a good point and it seems to be good addition for the porcelain format.

Perhaps it make more sense to apply as separate patch though, something
that Eric Sunshine also mentioned on this message reply.

If you really need to quote the lock reason then please use one of the
path quoting routines (probably quote_c_style() or
write_name_quoted()) in quote.c rather than rolling your own - the
code below gives the same output for a string that contains the two
characters `\` followed by `n` as it does for the single character
`\n`. People are (hopefully) used to dequoting our path quoting and
there are routines out there to do it (we have one git Git.pm) using a
function in quote.c will allow them to use those routines here.

Best Wishes

Phillip

That's great, thank you for pointing this out to me. I will definitely
change this into the next revision by dropping the function written from
scratch on this patch and reuse one of the existing functions.

For what is worth, I just added the function because I didn't know about
the existing ones. So, there is no reason to add the new function.

-- 
Thanks
Rafael

Eric Sunshine writes:

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

quoted

Add tests for "git worktree list" verbose mode, prunable and locked
annotations for both default and porcelain format and ensure the
"prunable" annotation is consistent with what "git worktree prune"
command will eventually remove. Additionally, add one test case to
ensure any newline characters are escaped from locked reason for the
porcelain format to prevent breaking the format.

The 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 remove the worktree
as the "git worktree prune" is not going to remove any locked worktrees.
Let's fix that by unlocking the worktree before the "prune" command.

Signed-off-by: Rafael Silva <redacted>
---

diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh

@@ -62,7 +62,9 @@ test_expect_success '"list" all worktrees --porcelain' '
 test_expect_success '"list" all worktrees with locked annotation' '
-       test_when_finished "rm -rf locked unlocked out && git worktree prune" &&
+       test_when_finished "rm -rf locked unlocked out &&
+               git worktree unlock locked &&
+               git worktree prune" &&
        git worktree add --detach locked master &&
        git worktree add --detach unlocked master &&
        git worktree lock locked &&

You might need to be a bit more careful here. If the test fails before
the worktree is locked, then the `git worktree unlock` in the cleanup
code will return an error, which will make the code executed by
test_when_finished() fail, as well, which makes it harder to debug
problems. Moving the `unlock` cleanup after you know the lock
succeeded will address this issue:

    test_when_finished "rm -rf locked unlocked out && git worktree prune" &&
    git worktree add --detach locked master &&
    git worktree add --detach unlocked master &&
    git worktree lock locked &&
    test_when_finished "git worktree unlock locked" &&
    ...

Same comment applies to other new tests added by this patch which lock
worktrees.

This is good point. will change on the next revision.

quoted

@@ -71,6 +73,96 @@ test_expect_success '"list" all worktrees with locked annotation' '
+test_expect_success '"list" all worktrees with prunable consistent with "prune"' '
+       test_when_finished "rm -rf prunable out && git worktree prune" &&
+       git worktree add --detach prunable &&
+       rm -rf prunable &&
+       git worktree list >out &&
+       grep "/prunable  *[0-9a-f].* prunable$" out &&
+       git worktree prune --verbose >out &&
+       test_i18ngrep "^Removing worktrees/prunable" out
+'

To be trustworthy, doesn't this test also need to have an unprunable
worktree, and test that `git worktree list` doesn't annotate it as
"prunable" _and_ that `git worktree prune` didn't prune it? Otherwise,
we really don't know that the two commands agree about what is and is
not prunable -- we only know they agree that this particular worktree
was prunable.

You're right this test is missing the case the "unprunable case". Will
add into the next revision.

quoted

+test_expect_success '"list" all worktrees --porcelain with newline escaped locked reason' '
+       test_when_finished "rm -rf locked_lf locked_crlf reason_lf reason_crlf out actual expect reason &&
+               git worktree unlock locked_lf &&
+               git worktree unlock locked_crlf &&
+               git worktree prune" &&

Nit: It's not a big deal, but we don't normally bother cleaning up
every junk file in tests, such as `out`, `actual`, `expect` if those
files aren't going to be a problem for subsequent tests. We are
explicitly cleaning up the worktrees in these tests because this
script is all about testing worktree behavior, and some random
leftover worktree could be a problem for other tests. I don't care
strongly one way or the other, but I worry a tiny bit that the list of
files being cleaned up could become outdated as changes are made to
the tests later...

Make sense, and ...

quoted

+test_expect_success '"list" all worktrees --porcelain with prunable' '
+       test_when_finished "rm -rf prunable list out && git worktree prune" &&
+       git worktree add --detach prunable &&
+       rm -rf prunable &&
+       git worktree list --porcelain >out &&
+       test_i18ngrep "^prunable gitdir file points to non-existent location$" out
+'

... for instance, the file `list` being cleaned up in this test is not
even created by this test.

This is exactly the case of updating the test and forgetting to
remove from the cleanup part because it was not used anymore. I'm also
inclined to make this changes on the next revision.

-- 
Thanks
Rafael

Eric Sunshine writes:

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

quoted

Update the documentation with "git worktree list" verbose mode, prunable
and locked annotations for the default and porcelain format as part of
teaching the command to show prunable working trees and verbose mode.

Signed-off-by: Rafael Silva <redacted>
---

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt

@@ -226,9 +227,12 @@ 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 for working trees.

This leaves the reader wondering what additional information is output
for `list`. Perhaps a small tweak will help:

    With `list`, output additional information about worktrees (see below).

Make sense.

quoted

 --expire <time>::
        With `prune`, only expire unused working trees older than `<time>`.
+       With `list`, annotate unused working trees older than `<time>` as prunable
+       candidates that will be remove by `prune` command if the same option is used.

Perhaps just minimal:

    With `list`, annotate missing worktrees as prunable if they
    are older than `<time>`.

Sounds reasonable.

quoted

@@ -367,13 +371,48 @@ $ git worktree list
+The command also shows annotations for each working tree, according to its state.
+These annotations are:
+
+ * "locked", if any working tree is locked
+ * "prunable", if any working tree can be pruned via "git worktree prune".

s/any/the/g

We might want to use backticks around these annotations rather than
double quotes, and we certainly do want to use backticks around the
`git worktree prune` command to ensure it is styled consistently with
other commands in this document.

Yes, good catch. It make sense to have backticks here.

quoted

+Note that, the annotation is only moved to the next line only if the
+additional text is available, otherwise the text is kept on the same.

Drop the comma between "that, the". Also, too many "only"s in this
sentence. You can actually drop both of them and the sentence will
still read fine:

   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 worktree itself.

or something.

Thanks for the alternative suggestion. It reads better like this will
add change the patch message to something more close to your suggestion.

quoted

 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:
+`worktree`, an empty line indicates the end of the record.
++
+In case any of the working trees are locked or is a candidate for pruning
+(See DESCRIPTION above) the labels "locked" and "prunable" is also shown
+followed by a reason, if available, otherwise only the labels are listed.
+For example:

s/(See/(see/
s/is also/are also/

Let's also use backticks rather than double quotes around `locked` and
`prunable` to ensure consistent formatting with the other porcelain
labels `bare` and `detached` which are already in backticks.

Nice catch. 

Also, the unnecessary `+` line (seen as `++` in the diff) makes this
render incorrectly. It renders as:

    +In case any...

To fix it, just leave the line blank between paragraphs.

(If possible, install `asciidoc` and `xmlto` and then run `make html`
to render the documentation yourself, and open
Documentation/git-worktree.html in your browser to check the output.)

Sure thing. thanks for the suggestion. I should have rendered the
documentation before sending. Apologize for that.

-- 
Thanks
Rafael

On Fri, Jan 8, 2021 at 2:38 AM Rafael Silva [off-list ref] wrote:

Eric Sunshine writes:

quoted

On Mon, Jan 4, 2021 at 11:22 AM Rafael Silva
[off-list ref] wrote:

quoted

The fifth patch adds worktree_escape_reason() that accepts a (char *)
text and returned the text with any LF or CRLF escaped. [...]

Perhaps I misunderstand, but I had envisioned employing one of the
codebase's existing quoting/escaping functions rather than crafting a
new one from scratch. However, I'll reserve judgment until I actually
read the patch.

Agreed. It make sense to reuse one of the already implemented functions
from the code base. for some reason I was not able to find it. I believe
this was cleared out in one of the patches replies by you and Phillip Wood.

No need to apologize. It's a big project and it can be difficult to
discover existing utility functions. Fortunately, reviewers can often
point out useful alternatives.

Hi Eric & Rafael

On 07/01/2021 03:34, Eric Sunshine wrote:

On Tue, Jan 5, 2021 at 6:02 AM Phillip Wood [off-list ref] wrote:

quoted

Add a -z option to be used in conjunction with --porcelain that gives
NUL-terminated output. This enables 'worktree list --porcelain' to
handle worktree paths that contain newlines.

Adding a -z mode makes a lot of sense. This, along with a fix to call
quote_c_style() on paths in normal (not `-z`) porcelain mode,

I'm concerned that starting to quote paths will break backwards compatibility. Even if we restricted the quoting to just those paths that contain '\n' there is no way to distinguish between a quoted path and one that begins and ends with ". This is the reason I prefer to add `-z` instead of taking Rafael's patch to quote the lock reason as that patch still leaves the output of `git worktree list --porcelain` ambiguous and it cannot be fixed without breaking existing users. A counter argument to all this is that there are thousands of users on file systems that cannot have newlines in paths and Rafael's patch is definitely a net win for them.

can
easily be done after Rafael's series lands. Or they could be done
before his series, though that might make a lot of extra work for him.

I would definitely be easiest to wait for Rafael's series to land if we want to add `-z` separately

quoted

Signed-off-by: Phillip Wood <redacted>
---
I found an old patch that added NUL termination. I've rebased it
but the new test fails as there seems to be another worktree thats
been added since I wrote this, anyway I thought it might be a useful
start for adding a `-z` option.

The test failure is fallout from a "bug" in a test added by Rafael's
earlier series[1] which was not caught by the reviewer[2]. In fact,
his current series has a drive-by fix[3] for this bug in patch [6/7].
Applying that fix (or the refinement[4] I suggested in my review)
makes your test work.

Thanks ever so much for taking the time to track down the cause of the test failure.

Best Wishes

Phillip

[1]: https://lore.kernel.org/git/20201011101152.5291-2-rafaeloliveira.cs@gmail.com/
[2]: https://lore.kernel.org/git/CAPig+cS8hvX4GCsnfLBnQ4Q_AkUad=bw7rjVcaOqSEqcLZvx8w@mail.gmail.com/
[3]: https://lore.kernel.org/git/20210104162128.95281-7-rafaeloliveira.cs@gmail.com/
[4]: https://lore.kernel.org/git/CAPig+cRysXpK0e1xXOuVd+EtkeyTk8FR6MUrL=Bg3W4ve8jdNA@mail.gmail.com/

quoted

diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt

@@ -217,7 +217,13 @@ This can also be set up as the default behaviour by using the
+-z::
+       When `--porcelain` is specified with `list` terminate each line with a
+       NUL rather than a newline. This makes it possible to parse the output
+       when a worktree path contains a newline character.

If we fix normal-mode porcelain to call quote_c_style(), then we can
drop the last sentence or refine it to say something along the lines
of it being easier to deal with paths with embedded newlines than in
normal porcelain mode.

quoted

diff --git a/builtin/worktree.c b/builtin/worktree.c

@@ -640,19 +640,25 @@ static int add(int ac, const char **av, const char *prefix)
+static void show_worktree_porcelain(struct worktree *wt, int line_terminator)
  {
+       printf("worktree %s", wt->path);
+       fputc(line_terminator, stdout);
+       if (wt->is_bare) {
+               printf("bare");
+               fputc(line_terminator, stdout);
+       } else {
+               printf("HEAD %s", oid_to_hex(&wt->head_oid));
+               fputc(line_terminator, stdout);
+               if (wt->is_detached) {
+                       printf("detached");
+                       fputc(line_terminator, stdout);
+               } else if (wt->head_ref) {
+                       printf("branch %s", wt->head_ref);
+                       fputc(line_terminator, stdout);
+               }

Perhaps this could all be done a bit more concisely with printf()
alone rather than combining it with fputc(). For instance:

     printf("worktree %s%c", wt->path, line_terminator);

and so on.

quoted

-       printf("\n");
+       fputc(line_terminator, stdout);

This use of fputc() makes sense.

quoted

@@ -720,15 +726,20 @@ static void pathsort(struct worktree **wt)
  static int list(int ac, const char **av, const char *prefix)
  {
+               OPT_SET_INT('z', NULL, &line_terminator,
+                           N_("fields are separated with NUL character"), '\0'),

"fields" sounds a little odd. "lines" might make more sense. "records"
might be even better and matches the wording of some other Git
commands accepting `-z`.

quoted

+       else if (!line_terminator && !porcelain)
+               die(_("'-z' requires '--porcelain'"));

Other error messages in this file don't quote command-line options, so:

     die(_("-z requires --porcelain"));

would be more consistent.

quoted

diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh

@@ -71,6 +71,28 @@ test_expect_success '"list" all worktrees with locked annotation' '
+test_expect_success '"list" all worktrees --porcelain -z' '
+       test_when_finished "rm -rf here _actual actual expect &&
+                               git worktree prune" &&
+       printf "worktree %sQHEAD %sQbranch %sQQ" \
+               "$(git rev-parse --show-toplevel)" \
+               "$(git rev-parse HEAD)" \
+               "$(git symbolic-ref HEAD)" >expect &&
+       git worktree add --detach here master &&
+       printf "worktree %sQHEAD %sQdetachedQQ" \
+               "$(git -C here rev-parse --show-toplevel)" \
+               "$(git rev-parse HEAD)" >>expect &&
+       git worktree list --porcelain -z >_actual &&
+       cat _actual | tr "\0" Q >actual &&

Or just use nul_to_q():

     nul_to_q <_actual >actual &&

(And there's no need to `cat` the file.)

quoted

+       test_cmp expect actual
+'
+
+test_expect_success '"list" -z fails without --porcelain' '
+       test_when_finished "rm -rf here && git worktree prune" &&
+       git worktree add --detach here master &&
+       test_must_fail git worktree list -z
+'

I don't think there's any need for this test to create (and cleanup) a
worktree. So, the entire test could collapse to:

     test_expect_success '"list" -z fails without --porcelain' '
         test_must_fail git worktree list -z
     '

On Fri, Jan 8, 2021 at 5:33 AM Phillip Wood [off-list ref] wrote:

On 07/01/2021 03:34, Eric Sunshine wrote:

quoted

On Tue, Jan 5, 2021 at 6:02 AM Phillip Wood [off-list ref] wrote:

quoted

Add a -z option to be used in conjunction with --porcelain that gives
NUL-terminated output. This enables 'worktree list --porcelain' to
handle worktree paths that contain newlines.

Adding a -z mode makes a lot of sense. This, along with a fix to call
quote_c_style() on paths in normal (not `-z`) porcelain mode,

I'm concerned that starting to quote paths will break backwards
compatibility. Even if we restricted the quoting to just those paths
that contain '\n' there is no way to distinguish between a quoted path
and one that begins and ends with ".

Backward compatibility is a valid concern, though I haven't managed to
convince myself that it would matter much in this case. In one sense,
the failure of the porcelain format to properly quote/escape paths
when needed can be viewed as an outright bug and, although we value
backward compatibility, we also value correctness, and such bug fixes
have been accepted in the past. Especially in a case such as this, it
seems exceedingly unlikely that fixing the bug would be harmful or
break existing tooling (though, of course that possibility always
exists, even if remotely so).

I can imagine ways in which tooling might be engineered to work around
the shortcoming that `git worktree list --porcelain` doesn't properly
handle newlines embedded in paths, but such tooling would almost
certainly be so fragile anyhow that it would break as we add more keys
to the extensible porcelain format. Coupled with the fact that
newlines embedded in paths are so exceedingly unlikely, it's hard to
imagine that fixing this bug would have a big impact on existing
tooling.

The case you mention about a path which happens to have a double-quote
as its first character does concern me a bit more since existing
tooling wouldn't have had to jump through hoops, or indeed do anything
special, with such paths, unlike the embedded newline case. But then,
too, it's pretty hard to imagine this coming up much, if at all, in
practice. That's not to say that I can't imagine a path, in general,
beginning with a quote, but keeping in mind that we're talking only
about worktree paths, it seems exceedingly unlikely.

My gut feeling (for what it's worth) is that worktree paths containing
embedded newlines (or other special characters) or beginning with a
double-quote is so unlikely to come in in actual practice that viewing
this as a bug fix is probably a reasonable approach, whereas some
other approach -- such as introducing porcelain-v2 or creating a new
porcelain key, say "worktreepath" which supersedes "worktree" (without
retiring "worktree") -- may be overkill.

None of the above is an argument against a complementary `-z` mode,
which I think is a very good idea.

This is the reason I prefer to add
`-z` instead of taking Rafael's patch to quote the lock reason as that
patch still leaves the output of `git worktree list --porcelain`
ambiguous and it cannot be fixed without breaking existing users. A
counter argument to all this is that there are thousands of users on
file systems that cannot have newlines in paths and Rafael's patch is
definitely a net win for them.

Rafael's patch is quoting only the lock-reason, not the worktree path,
so I think it's orthogonal to this discussion. Also, his patch is
introducing `lock` as a new attribute in porcelain output, not
modifying behavior of an existing `lock` attribute.

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 [off-list ref]
Signed-off-by: Rafael Silva <redacted>
---
 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)