mbox series

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

Message ID 20210119212739.77882-1-rafaeloliveira.cs@gmail.com (mailing list archive)
Headers show
Series teach `worktree list` verbose mode and prunable annotations | expand

Message

Rafael Silva Jan. 19, 2021, 9:27 p.m. UTC 
In c57b3367be (worktree: teach `list` to annotate locked worktree,
2020-10-11) we taught `git worktree list` to annotate working tree that
is locked by appending "locked" text in order to signalize to the user
that a working tree is locked.  During the review, there was some
discussion about additional annotations and information that `list`
command could provide to the user that has long been envisioned and
mentioned in [2], [3] and [4].

This patch series addresses some of these changes by teaching
`worktree list` to show "prunable" annotation, adding verbose mode and
extending the --porcelain format with prunable and locked annotation as
follow up from [1]. Additionally, it addresses one shortcoming for porcelain
format to escape any newline characters (LF or CRLF) for the lock reason
to prevent breaking the format that is mentioned in [4] and [1] during the
review cycle.

The patch series is organizes as:

1. The first patch moves the should_prune_worktree() machinery to the top-level
   worktree.c exposing the function as general API, that will be reference
   by should_prune_worktree() wrapper implemented on the second patch.

   The original idea was to not only move should_prune_worktree() but also
   refactor to accept a "struct worktree" and load the information directly,
   which allows simplify the `prune` command by reusing get_worktrees().
   However this seems to also require refactoring get_worktrees() itself
   to return "non-valid" working trees that can/should be pruned. This is
   also mentioned in [5]. Having the wrapper function makes it easier to add
   the prunable annotation without touching the get_worktrees() and the
   other worktree sub commands. The refactoring can be addressed in a
   future patch, if this turns out to be good idea. One possible approach
   is to teach get_worktrees() to take additional flags that will tell
   whether to return only valid or all worktrees in GIT_DIR/worktrees/
   directory and address its own possible shortcoming.

2. Introduces the worktree_prune_reason() to discovery whether a worktree
   is prunable along with two new fields in the `worktree` structure. The
   function mimics the workree_lock_reason() API.

3. The third patch changes worktree_lock_reason() to be more gentle for
   the main working tree to simply returning NULL instead of aborting the
   program via assert() macro. This allow us to simplify the code that
   checks if the working tree is locked for default and porcelain format.
   This changes is also mentioned in [6].

4. Fix t2402 added in [1] to ensure the locked worktree is properly cleaned up.

5. The fourth patch adds the "locked" attribute for the porcelain format
   in order to make both default and --porcelain format consistent.

6. The fifth patch introduces "prunable" annotation for both default
   and --porcelain format.

7. The sixth patch introduces verbose mode to expand the `list` default
   format and show each annotation reason when its available.

Series is built on top of 66e871b664 (The third batch, 2021-01-15).

[1] https://lore.kernel.org/git/20200928154953.30396-1-rafaeloliveira.cs@gmail.com/
[2] https://lore.kernel.org/git/CAPig+cQF6V8HNdMX5AZbmz3_w2WhSfA4SFfNhQqxXBqPXTZL+w@mail.gmail.com/
[3] https://lore.kernel.org/git/CAPig+cSGXqJuaZPhUhOVX5X=LMrjVfv8ye_6ncMUbyKox1i7QA@mail.gmail.com/
[4] https://lore.kernel.org/git/CAPig+cTitWCs5vB=0iXuUyEY22c0gvjXvY1ZtTT90s74ydhE=A@mail.gmail.com/
[5] https://lore.kernel.org/git/CACsJy8ChM99n6skQCv-GmFiod19mnwwH4j-6R+cfZSiVFAxjgA@mail.gmail.com/
[6] https://lore.kernel.org/git/xmqq8sctlgzx.fsf@gitster.c.googlers.com/

Changes in v3
=============

v3 is 1-patch bigger than v2 and it includes the following changes:

 * Dropped CQUOTE_NODQ flag in the the locked reason to return a string
   enclosed with double quotes if the text reason contains especial
   characters, like newlines.

 * fix in t2402 to ensure the locked worktree is properly cleaned up,
   is move to its own patch.

 * In worktree_prune_reason(), the `path` variable is initialize
   with NULL to make the code easier to follow.

 * The removal of `is_main_worktree()` before `worktree_lock_reason()`
   is moved to the patch that actually changes the API to be more gentle
   with the main worktree instead of refactoring the code in the next
   patches that adds the annotations.

 * Drop the `*reason` in `(reason && *reason)` as we know that
   worktree_prune_reason() is either going to return a non-empty string
   or NULL which makes the code easier to follow.

 * The --verbose test for the locked annotation now tests that "locked"
   annotation stays on the same line when there is no locked reason.

 * Small documentation updates and make the "git worktree --verbose"
   example a little consistent with the worktree path.

Changes in v2
=============

v2 changes considerably from v1 taking into account several comments
suggested by Eric Sunshine and Phillip Wood (thanks guys for the
insightful comments and patience reviewing my patches :) ).

 * The biggest change are the way the series is organized. In v1 all the
   code was implemented in the fourth patch, all the tests and documentation
   updates was added in sixth and seventh patch respectfully, in v2 each
   patch introduces the annotations and verbose mode together with theirs
   respective test and documentation updates.

 * Several rewrite of the commit messages

 * In v1 the fifth patch was introducing a new function to escape newline
   characters for the "locked" attribute. However, we already have this
   feature from the quote.h API. So the new function is dropped and the
   changes are squashed into v2's fourth patch.

 * The new prunable annotation and locked annotation that was introduced
   by [1] was refactor to not poke around the worktree private fields.

 * Refactoring of the worktree_prune_reason() to cache the prune_reason
   and refactor to early return the use cases following the more common
   pattern used on Git's codebase.

 * Few documentation rewrites, most notably the `--verbose` and `--expire`
   doc sentences for the list command are moved to its own line to clearly
   separate the description from the others commands instead of continuing 
   on the same paragraph.

 * The `git unlock <worktree>` used in the test's cleanup is moved to after
   we know the `git worktree locked` is executed successfully. This ensures
   the unlock doesn't error in the cleanup stage which will make it harder
   to debug the tests.

Rafael Silva (7):
  worktree: libify should_prune_worktree()
  worktree: teach worktree to lazy-load "prunable" reason
  worktree: teach worktree_lock_reason() to gently handle main worktree
  t2402: ensure locked worktree is properly cleaned up
  worktree: teach `list --porcelain` to annotate locked worktree
  worktree: teach `list` to annotate prunable worktree
  worktree: teach `list` verbose mode

 Documentation/git-worktree.txt |  62 +++++++++++++++++--
 builtin/worktree.c             | 110 +++++++++++----------------------
 builtin/worktree.cc            |   0
 t/t2402-worktree-list.sh       |  93 ++++++++++++++++++++++++++++
 worktree.c                     |  91 ++++++++++++++++++++++++++-
 worktree.h                     |  23 +++++++
 6 files changed, 299 insertions(+), 80 deletions(-)
 create mode 100644 builtin/worktree.cc

Range-diff against v2:
-:  ---------- > 1:  66cd83ba42 worktree: libify should_prune_worktree()
1:  9d47fcb4a4 ! 2:  81f4824028 worktree: teach worktree to lazy-load "prunable" reason
    @@ worktree.c: const char *worktree_lock_reason(struct worktree *wt)
     +const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire)
     +{
     +	struct strbuf reason = STRBUF_INIT;
    -+	char *path;
    ++	char *path = NULL;
     +
     +	if (is_main_worktree(wt))
     +		return NULL;
2:  ac7c375bac ! 3:  d0dbf0b709 worktree: teach worktree_lock_reason() to gently handle main worktree
    @@ Commit message
     
         worktree_lock_reason() aborts with an assertion failure when called on
         the main worktree since locking the main worktree is nonsensical. Not
    -    only is this behaviour undocumented, thus callers might not even be aware
    +    only is this behavior undocumented, thus callers might not even be aware
         that the call could potentially crash the program, but it also forces
         clients to be extra careful:
     
    @@ Commit message
                 ...
     
         Since we know that locking makes no sense in the context of the main
    -    worktree, we can simpliy return false for the main worktree, thus making
    +    worktree, we can simply return false for the main worktree, thus making
         client code less complex by eliminating the need for the callers to have
         inside knowledge about the implementation:
     
    @@ Commit message
         Helped-by: Eric Sunshine <sunshine@sunshineco.com>
         Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
     
    + ## builtin/worktree.c ##
    +@@ builtin/worktree.c: static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
    + 			strbuf_addstr(&sb, "(error)");
    + 	}
    + 
    +-	if (!is_main_worktree(wt) && worktree_lock_reason(wt))
    ++	if (worktree_lock_reason(wt))
    + 		strbuf_addstr(&sb, " locked");
    + 
    + 	printf("%s\n", sb.buf);
    +
      ## worktree.c ##
     @@ worktree.c: int is_main_worktree(const struct worktree *wt)
      
-:  ---------- > 4:  c6f8a3e9cd t2402: ensure locked worktree is properly cleaned up
3:  ec1eb5a9f8 ! 5:  86c5253288 worktree: teach `list --porcelain` to annotate locked worktree
    @@ Commit message
             locked reason why it is locked
             ...
     
    -    The locked reason is quoted to prevent newline characters (i.e: LF or
    -    CRLF) breaking the --porcelain format as each attribute is shown per
    -    line. For example:
    +    In porcelain mode, if the lock reason contains special characters
    +    such as newlines, they are escaped with backslashes and the entire
    +    reason is enclosed in double quotes. For example:
     
            $ git worktree list --porcelain
            ...
    -       locked worktree's path mounted in\nremovable device
    +       locked "worktree's path mounted in\nremovable device"
            ...
     
         Furthermore, let's update the documentation to state that some
    @@ Commit message
         with its value depending whether the value is available or not. Thus
         documenting the case of the new "locked" attribute.
     
    -    Additionally, c57b3367be (worktree: teach `list` to annotate locked
    -    worktree, 2020-10-11) introduced a new test to ensure locked worktrees
    -    are listed with "locked" annotation. However, the test does not clean up
    -    after itself as "git worktree prune" is not going to remove the locked
    -    worktree in the first place. This not only leaves the test in an unclean
    -    state it also potentially breaks following tests that relies on the
    -    "git worktree list" output. Let's fix that by unlocking the worktree
    -    before the "prune" command.
    -
         Helped-by: Eric Sunshine <sunshine@sunshineco.com>
         Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
     
    @@ Documentation/git-worktree.txt: Porcelain Format
     -if the value is true.  The first attribute of a working tree is always
     -`worktree`, an empty line indicates the end of the record.  For example:
     +if the value is true.  Some attributes (like `locked`) can be listed as a label
    -+only or with a value depending whether a reason is available.  The first
    ++only or with a value depending upon whether a reason is available.  The first
     +attribute of a working tree is always `worktree`, an empty line indicates the
     +end of the record.  For example:
      
    @@ Documentation/git-worktree.txt: worktree /path/to/other-linked-worktree
      HEAD 1234abc1234abc1234abc1234abc1234abc1234a
      detached
      
    -+worktree /path/to/linked-worktree-locked
    ++worktree /path/to/linked-worktree-locked-no-reason
     +HEAD 5678abc5678abc5678abc5678abc5678abc5678c
    -+branch refs/heads/locked
    ++branch refs/heads/locked-no-reason
     +locked
     +
     +worktree /path/to/linked-worktree-locked-with-reason
    @@ builtin/worktree.c: static void show_worktree_porcelain(struct worktree *wt)
     +	reason = worktree_lock_reason(wt);
     +	if (reason && *reason) {
     +		struct strbuf sb = STRBUF_INIT;
    -+		quote_c_style(reason, &sb, NULL, CQUOTE_NODQ);
    ++		quote_c_style(reason, &sb, NULL, 0);
     +		printf("locked %s\n", sb.buf);
     +		strbuf_release(&sb);
     +	} else if (reason)
    @@ builtin/worktree.c: static void show_worktree_porcelain(struct worktree *wt)
     
      ## t/t2402-worktree-list.sh ##
     @@ t/t2402-worktree-list.sh: test_expect_success '"list" all worktrees with locked annotation' '
    - 	git worktree add --detach locked master &&
    - 	git worktree add --detach unlocked master &&
    - 	git worktree lock locked &&
    -+	test_when_finished "git worktree unlock locked" &&
    - 	git worktree list >out &&
    - 	grep "/locked  *[0-9a-f].* locked$" out &&
      	! grep "/unlocked  *[0-9a-f].* locked$" out
      '
      
    @@ t/t2402-worktree-list.sh: test_expect_success '"list" all worktrees with locked
     +	echo "locked with reason" >>expect &&
     +	git worktree add --detach locked1 &&
     +	git worktree add --detach locked2 &&
    ++	# unlocked worktree should not be annotated with "locked"
     +	git worktree add --detach unlocked &&
     +	git worktree lock locked1 &&
     +	git worktree lock locked2 --reason "with reason" &&
    @@ t/t2402-worktree-list.sh: test_expect_success '"list" all worktrees with locked
     +
     +test_expect_success '"list" all worktrees --porcelain with locked reason newline escaped' '
     +	test_when_finished "rm -rf locked_lf locked_crlf out actual expect && git worktree prune" &&
    -+	printf "locked locked\\\\r\\\\nreason\n" >expect &&
    -+	printf "locked locked\\\\nreason\n" >>expect &&
    ++	printf "locked \"locked\\\\r\\\\nreason\"\n" >expect &&
    ++	printf "locked \"locked\\\\nreason\"\n" >>expect &&
     +	git worktree add --detach locked_lf &&
     +	git worktree add --detach locked_crlf &&
    -+	printf "locked\nreason\n\n" >reason_lf &&
    -+	printf "locked\r\nreason\n\n" >reason_crlf &&
    -+	git worktree lock locked_lf --reason "$(cat reason_lf)" &&
    -+	git worktree lock locked_crlf --reason "$(cat reason_crlf)" &&
    ++	git worktree lock locked_lf --reason "$(printf "locked\nreason")" &&
    ++	git worktree lock locked_crlf --reason "$(printf "locked\r\nreason")" &&
     +	test_when_finished "git worktree unlock locked_lf && git worktree unlock locked_crlf" &&
     +	git worktree list --porcelain >out &&
     +	grep "^locked" out >actual &&
4:  f937df6460 ! 6:  be814326ee worktree: teach `list` to annotate prunable worktree
    @@ Documentation/git-worktree.txt: This can also be set up as the default behaviour
      	With `prune`, only expire unused working trees older than `<time>`.
     ++
     +With `list`, annotate missing working trees as prunable if they are
    -+older than `<mtime>`.
    ++older than `<time>`.
      
      --reason <string>::
      	With `lock`, an explanation why the working tree is locked.
    @@ Documentation/git-worktree.txt: $ git worktree list
     +
     +------------
     +$ git worktree list
    -+/path/to/linked-worktree        abcd1234 [master]
    -+/path/to/locked-worktreee       acbd5678 (brancha) locked
    -+/path/to/prunable-worktree      5678abc  (detached HEAD) prunable
    ++/path/to/linked-worktree    abcd1234 [master]
    ++/path/to/locked-worktreee   acbd5678 (brancha) locked
    ++/path/to/prunable-worktree  5678abc  (detached HEAD) prunable
     +------------
     +
      Porcelain Format
    @@ builtin/worktree.c: static void show_worktree_porcelain(struct worktree *wt)
      		printf("locked\n");
      
     +	reason = worktree_prune_reason(wt, expire);
    -+	if (reason && *reason)
    ++	if (reason)
     +		printf("prunable %s\n", reason);
     +
      	printf("\n");
      }
      
     @@ builtin/worktree.c: static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
    - 	struct strbuf sb = STRBUF_INIT;
    - 	int cur_path_len = strlen(wt->path);
    - 	int path_adj = cur_path_len - utf8_strwidth(wt->path);
    -+	const char *reason;
    - 
    - 	strbuf_addf(&sb, "%-*s ", 1 + path_maxlen + path_adj, wt->path);
    - 	if (wt->is_bare)
    -@@ builtin/worktree.c: static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
    - 			strbuf_addstr(&sb, "(error)");
    - 	}
    - 
    --	if (!is_main_worktree(wt) && worktree_lock_reason(wt))
    -+	reason = worktree_lock_reason(wt);
    -+	if (reason)
    + 	if (worktree_lock_reason(wt))
      		strbuf_addstr(&sb, " locked");
      
    -+	reason = worktree_prune_reason(wt, expire);
    -+	if (reason)
    ++	if (worktree_prune_reason(wt, expire))
     +		strbuf_addstr(&sb, " prunable");
     +
      	printf("%s\n", sb.buf);
    @@ builtin/worktree.c: static int list(int ac, const char **av, const char *prefix)
      	if (ac)
      		usage_with_options(worktree_usage, options);
     
    + ## builtin/worktree.cc (new) ##
    +
      ## t/t2402-worktree-list.sh ##
     @@ t/t2402-worktree-list.sh: test_expect_success '"list" all worktrees --porcelain with locked reason newline
      	test_cmp expect actual
5:  0b6ab6dddb ! 7:  b0688f142d worktree: teach `list` verbose mode
    @@ Commit message
         worktrees a reason might be available provided by the user via `lock`
         command.
     
    -    Let's teach "git worktree list" to output the reason why the worktrees
    -    are being annotated. The reason is a text that can take virtually any
    -    size and appending the text on the default columned format will make it
    -    difficult to extend the command with other annotations and not fit nicely
    -    on the screen. In order to address this shortcoming the annotation is
    -    then moved to the next line indented followed by the reason, if the
    -    reason is not available the annotation stays on the same line as the
    -    worktree itself.
    +    Let's teach "git worktree list" a --verbose mode that outputs the reason
    +    why the worktrees are being annotated. The reason is a text that can take
    +    virtually any size and appending the text on the default columned format
    +    will make it difficult to extend the command with other annotations and
    +    not fit nicely on the screen. In order to address this shortcoming the
    +    annotation is then moved to the next line indented followed by the reason
    +    If the reason is not available the annotation stays on the same line as
    +    the worktree itself.
     
         The output of "git worktree list" with verbose becomes like so:
     
             $ git worktree list --verbose
             ...
    -        /path/to/locked                acb124 [branch-a] locked
    -        /path/to/locked-with-reason    acc125 [branch-b]
    +        /path/to/locked-no-reason    acb124 [branch-a] locked
    +        /path/to/locked-with-reason  acc125 [branch-b]
                 locked: worktree with a locked reason
    -        /path/to/prunable-reason       ace127 [branch-d]
    +        /path/to/prunable-reason     ace127 [branch-d]
                 prunable: gitdir file points to non-existent location
             ...
     
    @@ Documentation/git-worktree.txt: This can also be set up as the default behaviour
      --expire <time>::
      	With `prune`, only expire unused working trees older than `<time>`.
     @@ Documentation/git-worktree.txt: $ git worktree list
    - /path/to/prunable-worktree      5678abc  (detached HEAD) prunable
    + /path/to/prunable-worktree  5678abc  (detached HEAD) prunable
      ------------
      
     +For these annotations, a reason might also be available and this can be
    @@ Documentation/git-worktree.txt: $ git worktree list
     +
     +------------
     +$ git worktree list --verbose
    -+/path/to/linked-worktree        abcd1234 [master]
    -+/path/to/locked-worktreee       acbd5678 (brancha)
    -+	locked: working tree path is mounted on a removable device
    -+/path/to/locked-no-reason       abcd578  (detached HEAD) locked
    -+/path/to/prunable-worktree      5678abc  (detached HEAD)
    ++/path/to/linked-worktree              abcd1234 [master]
    ++/path/to/locked-worktree-no-reason    abcd5678 (detached HEAD) locked
    ++/path/to/locked-worktree-with-reason  1234abcd (brancha)
    ++	locked: working tree path is mounted on a portable device
    ++/path/to/prunable-worktree            5678abc1 (detached HEAD)
     +	prunable: gitdir file points to non-existent location
     +------------
     +
    @@ Documentation/git-worktree.txt: $ git worktree list
     
      ## builtin/worktree.c ##
     @@ builtin/worktree.c: static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
    + 	struct strbuf sb = STRBUF_INIT;
    + 	int cur_path_len = strlen(wt->path);
    + 	int path_adj = cur_path_len - utf8_strwidth(wt->path);
    ++	const char *reason;
    + 
    + 	strbuf_addf(&sb, "%-*s ", 1 + path_maxlen + path_adj, wt->path);
    + 	if (wt->is_bare)
    +@@ builtin/worktree.c: static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len)
    + 			strbuf_addstr(&sb, "(error)");
      	}
      
    - 	reason = worktree_lock_reason(wt);
    --	if (reason)
    +-	if (worktree_lock_reason(wt))
    ++	reason = worktree_lock_reason(wt);
     +	if (verbose && reason && *reason)
     +		strbuf_addf(&sb, "\n\tlocked: %s", reason);
     +	else if (reason)
      		strbuf_addstr(&sb, " locked");
      
    - 	reason = worktree_prune_reason(wt, expire);
    --	if (reason)
    -+	if (verbose && reason && *reason)
    +-	if (worktree_prune_reason(wt, expire))
    ++	reason = worktree_prune_reason(wt, expire);
    ++	if (verbose && reason)
     +		strbuf_addf(&sb, "\n\tprunable: %s", reason);
     +	else if (reason)
      		strbuf_addstr(&sb, " prunable");
    @@ t/t2402-worktree-list.sh: test_expect_success '"list" all worktrees with prunabl
     +'
     +
     +test_expect_success '"list" all worktrees --verbose with locked' '
    -+	test_when_finished "rm -rf locked out actual expect && git worktree prune" &&
    -+	git worktree add locked --detach &&
    -+	git worktree lock locked --reason "with reason" &&
    -+	test_when_finished "git worktree unlock locked" &&
    -+	echo "$(git -C locked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
    ++	test_when_finished "rm -rf locked1 locked2 out actual expect && git worktree prune" &&
    ++	git worktree add locked1 --detach &&
    ++	git worktree add locked2 --detach &&
    ++	git worktree lock locked1 &&
    ++	git worktree lock locked2 --reason "with reason" &&
    ++	test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" &&
    ++	echo "$(git -C locked2 rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
     +	printf "\tlocked: with reason\n" >>expect &&
     +	git worktree list --verbose >out &&
    -+	sed -n "s/  */ /g;/\/locked  *[0-9a-f].*$/,/locked: .*$/p" <out >actual &&
    ++	grep "/locked1  *[0-9a-f].* locked$" out &&
    ++	sed -n "s/  */ /g;/\/locked2  *[0-9a-f].*$/,/locked: .*$/p" <out >actual &&
     +	test_cmp actual expect
     +'
     +

Comments

Eric Sunshine Jan. 24, 2021, 8:51 a.m. UTC | #1 
On Tue, Jan 19, 2021 at 4:28 PM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> Changes in v3
> =============
> v3 is 1-patch bigger than v2 and it includes the following changes:

Thanks. I gave v3 a complete read-through, and it is just about perfect.

Aside from a very minor problem with the tests in [5/7] and [7/7] and
the documentation enhancement (`core.quotePath`) suggested by Phillip,
this is really, really close. (In fact, I would be happy to accept v3
as-is; those minor changes could easily be done on top.)

I look forward to giving v4 my Reviewed-by:.

> Range-diff against v2:
> -:  ---------- > 1:  66cd83ba42 worktree: libify should_prune_worktree()
> 1:  9d47fcb4a4 ! 2:  81f4824028 worktree: teach worktree to lazy-load "prunable" reason

The range-diff seems slightly off. The "libify
should_prune_worktree()" patch existed in v1 and v2, but the
range-diff suggests that it's new with v3. You probably just need a
minor adjustment to your `git format-patch --range-diff` invocation.
Rafael Silva Jan. 27, 2021, 8:03 a.m. UTC | #2 
In c57b3367be (worktree: teach `list` to annotate locked worktree,
2020-10-11) we taught `git worktree list` to annotate working trees that
is locked by appending "locked" text in order to signalize to the user
that a working tree is locked.  During the review, there was some
discussion about additional annotations and information that `list`
command could provide to the user that has long been envisioned and
mentioned in [2], [3] and [4].

This patch series addresses some of these changes by teaching
`worktree list` to show "prunable" annotation, adding verbose mode and
extending the --porcelain format with prunable and locked annotation as
follow up from [1]. Additionally, it addresses one shortcoming for porcelain
format to escape any newline characters (LF or CRLF) and quote the lock
reason to prevent breaking the format that is mentioned in [4] and [1] during
the review cycle.

This patch series includes:

1. The first patch moves the should_prune_worktree() machinery to the top-level
   worktree.c exposing the function as general API, that will be reference
   by should_prune_worktree() wrapper implemented on the second patch.

   The original idea was to not only move should_prune_worktree() but also
   refactor to accept a "struct worktree" and load the information directly,
   which allows simplify the `prune` command by reusing get_worktrees().
   However this seems to also require refactoring get_worktrees() itself
   to return "non-valid" working trees that can/should be pruned. This is
   also mentioned in [5]. Having the wrapper function makes it easier to add
   the prunable annotation without touching the get_worktrees() and the
   other worktree sub commands. The refactoring can be addressed in a
   future patch, if this turns out to be good idea. One possible approach
   is to teach get_worktrees() to take additional flags that will tell
   whether to return only valid or all worktrees in GIT_DIR/worktrees/
   directory and address its own possible shortcoming.

2. Introduces the worktree_prune_reason() to discovery whether a worktree
   is prunable along with two new fields in the `worktree` structure. The
   function mimics the workree_lock_reason() API.

3. The third patch changes worktree_lock_reason() to be more gentle for
   the main working tree to simply returning NULL instead of aborting the
   program via assert() macro. This allow us to simplify the code that
   checks if the working tree is locked for default and porcelain format.
   This changes is also mentioned in [6].

4. Fix t2402 added in [1] to ensure the locked worktree is properly cleaned up.

5. The fourth patch adds the "locked" attribute for the porcelain format
   in order to make both default and --porcelain format consistent.

6. The fifth patch introduces "prunable" annotation for both default
   and --porcelain format.

7. The sixth patch introduces verbose mode to expand the `list` default
   format and show each annotation reason when its available.

Series is built on top of 66e871b664 (The third batch, 2021-01-15).

[1] https://lore.kernel.org/git/20200928154953.30396-1-rafaeloliveira.cs@gmail.com/
[2] https://lore.kernel.org/git/CAPig+cQF6V8HNdMX5AZbmz3_w2WhSfA4SFfNhQqxXBqPXTZL+w@mail.gmail.com/
[3] https://lore.kernel.org/git/CAPig+cSGXqJuaZPhUhOVX5X=LMrjVfv8ye_6ncMUbyKox1i7QA@mail.gmail.com/
[4] https://lore.kernel.org/git/CAPig+cTitWCs5vB=0iXuUyEY22c0gvjXvY1ZtTT90s74ydhE=A@mail.gmail.com/
[5] https://lore.kernel.org/git/CACsJy8ChM99n6skQCv-GmFiod19mnwwH4j-6R+cfZSiVFAxjgA@mail.gmail.com/
[6] https://lore.kernel.org/git/xmqq8sctlgzx.fsf@gitster.c.googlers.com/

Changes in v4
=============

Thanks Eric Sunshine and Phillip Wood for the review and suggestions.

 * Added documentation to explain that the lock reason is quoted with
   the same rules as described for `core.quotePath`.

 * The `worktree unlock` issued in the test cleanup is splitted and
   executed after each `worktree lock` to ensure the unlock is only
   done after we know each locked command was successful.  

 * Fix a couple of grammos in the [4/7] commit message.

Changes in v3
=============

v3 is 1-patch bigger than v2 and it includes the changes:

 * Dropped CQUOTE_NODQ flag in the the locked reason to return a string
   enclosed with double quotes if the text reason contains especial
   characters, like newlines.

 * fix in t2402 to ensure the locked worktree is properly cleaned up,
   is move to its own patch.

 * In worktree_prune_reason(), the `path` variable is initialize
   with NULL to make the code easier to follow.

 * The removal of `is_main_worktree()` before `worktree_lock_reason()`
   is moved to the patch that actually changes the API to be more gentle
   with the main worktree instead of refactoring the code in the next
   patches that adds the annotations.

 * Drop the `*reason` in `(reason && *reason)` as we know that
   worktree_prune_reason() is either going to return a non-empty string
   or NULL which makes the code easier to follow.

 * The --verbose test for the locked annotation now tests that "locked"
   annotation stays on the same line when there is no locked reason.

 * Small documentation updates and make the "git worktree --verbose"
   example a little consistent with the worktree path.

Changes in v2
=============

v2 changes considerably from v1 taking into account several comments
suggested by Eric Sunshine and Phillip Wood (thanks guys for the
insightful comments and patience reviewing my patches :) ).

 * The biggest change are the way the series is organized. In v1 all the
   code was implemented in the fourth patch, all the tests and documentation
   updates was added in sixth and seventh patch respectfully, in v2 each
   patch introduces the annotations and verbose mode together with theirs
   respective test and documentation updates.

 * Several rewrite of the commit messages

 * In v1 the fifth patch was introducing a new function to escape newline
   characters for the "locked" attribute. However, we already have this
   feature from the quote.h API. So the new function is dropped and the
   changes are squashed into v2's fourth patch.

 * The new prunable annotation and locked annotation that was introduced
   by [1] was refactor to not poke around the worktree private fields.

 * Refactoring of the worktree_prune_reason() to cache the prune_reason
   and refactor to early return the use cases following the more common
   pattern used on Git's codebase.

 * Few documentation rewrites, most notably the `--verbose` and `--expire`
   doc sentences for the list command are moved to its own line to clearly
   separate the description from the others commands instead of continuing 
   on the same paragraph.

 * The `git unlock <worktree>` used in the test's cleanup is moved to after
   we know the `git worktree locked` is executed successfully. This ensures
   the unlock doesn't error in the cleanup stage which will make it harder
   to debug the tests.

Rafael Silva (7):
  worktree: libify should_prune_worktree()
  worktree: teach worktree to lazy-load "prunable" reason
  worktree: teach worktree_lock_reason() to gently handle main worktree
  t2402: ensure locked worktree is properly cleaned up
  worktree: teach `list --porcelain` to annotate locked worktree
  worktree: teach `list` to annotate prunable worktree
  worktree: teach `list` verbose mode

 Documentation/git-worktree.txt |  74 ++++++++++++++++++++--
 builtin/worktree.c             | 110 +++++++++++----------------------
 t/t2402-worktree-list.sh       |  96 ++++++++++++++++++++++++++++
 worktree.c                     |  91 ++++++++++++++++++++++++++-
 worktree.h                     |  23 +++++++
 5 files changed, 314 insertions(+), 80 deletions(-)

Range-diff against v3:
1:  fc4252c129 = 1:  66cd83ba42 worktree: libify should_prune_worktree()
2:  664d84155b = 2:  81f4824028 worktree: teach worktree to lazy-load "prunable" reason
3:  cf334da2f0 = 3:  c62ecf60d6 worktree: teach worktree_lock_reason() to gently handle main worktree
4:  ecb11793d9 ! 4:  d2ea467927 t2402: ensure locked worktree is properly cleaned up
    @@ Metadata
      ## Commit message ##
         t2402: ensure locked worktree is properly cleaned up
     
    -    In c57b3367be (worktree: teach `list` to annotate locked worktree,
    +    c57b3367be (worktree: teach `list` to annotate locked worktree,
         2020-10-11) introduced a new test to ensure locked worktrees are listed
         with "locked" annotation. However, the test does not clean up after
         itself as "git worktree prune" is not going to remove the locked worktree
         in the first place. This not only leaves the test in an unclean state it
    -    also potentially breaks following tests that relies on the
    +    also potentially breaks following tests that rely on the
         "git worktree list" output.
     
         Let's fix that by unlocking the worktree before the "prune" command.
5:  1fb68562f6 ! 5:  1b7661a0b3 worktree: teach `list --porcelain` to annotate locked worktree
    @@ Commit message
         with its value depending whether the value is available or not. Thus
         documenting the case of the new "locked" attribute.
     
    +    Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
         Helped-by: Eric Sunshine <sunshine@sunshineco.com>
         Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
     
    @@ Documentation/git-worktree.txt: worktree /path/to/other-linked-worktree
     +branch refs/heads/locked-with-reason
     +locked reason why is locked
     +
    ++------------
    ++
    ++If the lock reason contains "unusual" characters such as newline, they
    ++are escaped and the entire reason is quoted as explained for the
    ++configuration variable `core.quotePath` (see linkgit:git-config[1]).
    ++For Example:
    ++
    ++------------
    ++$ git worktree list --porcelain
    ++...
    ++locked "reason\nwhy is locked"
    ++...
      ------------
      
      EXAMPLES
    @@ t/t2402-worktree-list.sh: test_expect_success '"list" all worktrees with locked
     +	# unlocked worktree should not be annotated with "locked"
     +	git worktree add --detach unlocked &&
     +	git worktree lock locked1 &&
    ++	test_when_finished "git worktree unlock locked1" &&
     +	git worktree lock locked2 --reason "with reason" &&
    -+	test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" &&
    ++	test_when_finished "git worktree unlock locked2" &&
     +	git worktree list --porcelain >out &&
     +	grep "^locked" out >actual &&
     +	test_cmp expect actual
    @@ t/t2402-worktree-list.sh: test_expect_success '"list" all worktrees with locked
     +	git worktree add --detach locked_lf &&
     +	git worktree add --detach locked_crlf &&
     +	git worktree lock locked_lf --reason "$(printf "locked\nreason")" &&
    ++	test_when_finished "git worktree unlock locked_lf" &&
     +	git worktree lock locked_crlf --reason "$(printf "locked\r\nreason")" &&
    -+	test_when_finished "git worktree unlock locked_lf && git worktree unlock locked_crlf" &&
    ++	test_when_finished "git worktree unlock locked_crlf" &&
     +	git worktree list --porcelain >out &&
     +	grep "^locked" out >actual &&
     +	test_cmp expect actual
6:  834584f826 = 6:  c29ad7ffb1 worktree: teach `list` to annotate prunable worktree
7:  35ad456fbb ! 7:  7938950d04 worktree: teach `list` verbose mode
    @@ t/t2402-worktree-list.sh: test_expect_success '"list" all worktrees with prunabl
     +	git worktree add locked1 --detach &&
     +	git worktree add locked2 --detach &&
     +	git worktree lock locked1 &&
    ++	test_when_finished "git worktree unlock locked1" &&
     +	git worktree lock locked2 --reason "with reason" &&
    -+	test_when_finished "git worktree unlock locked1 && git worktree unlock locked2" &&
    ++	test_when_finished "git worktree unlock locked2" &&
     +	echo "$(git -C locked2 rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >expect &&
     +	printf "\tlocked: with reason\n" >>expect &&
     +	git worktree list --verbose >out &&
Rafael Silva Jan. 27, 2021, 8:08 a.m. UTC | #3 
Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Jan 19, 2021 at 4:28 PM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> Changes in v3
>> =============
>> v3 is 1-patch bigger than v2 and it includes the following changes:
>
> Thanks. I gave v3 a complete read-through, and it is just about perfect.
>
> Aside from a very minor problem with the tests in [5/7] and [7/7] and
> the documentation enhancement (`core.quotePath`) suggested by Phillip,
> this is really, really close. (In fact, I would be happy to accept v3
> as-is; those minor changes could easily be done on top.)
>
> I look forward to giving v4 my Reviewed-by:.
>

Thanks again for reviewing the series. I've just sent the v4 and I'm
looking forward to your review.

>> Range-diff against v2:
>> -:  ---------- > 1:  66cd83ba42 worktree: libify should_prune_worktree()
>> 1:  9d47fcb4a4 ! 2:  81f4824028 worktree: teach worktree to lazy-load "prunable" reason
>
> The range-diff seems slightly off. The "libify
> should_prune_worktree()" patch existed in v1 and v2, but the
> range-diff suggests that it's new with v3. You probably just need a
> minor adjustment to your `git format-patch --range-diff` invocation.

Thanks for pointing this out, I didn't notice this from the v3 range-diff.
Eric Sunshine Jan. 30, 2021, 7:04 a.m. UTC | #4 
On Wed, Jan 27, 2021 at 3:03 AM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> Changes in v4
>  * Added documentation to explain that the lock reason is quoted with
>    the same rules as described for `core.quotePath`.
>  * The `worktree unlock` issued in the test cleanup is splitted and
>    executed after each `worktree lock` to ensure the unlock is only
>    done after we know each locked command was successful.
>  * Fix a couple of grammos in the [4/7] commit message.

I just gave v4 a complete read-through and it looks great. All review
comments on previous rounds have been addressed, and I didn't find
anything important to comment on in v4. Very nicely done.

Please consider this entire series:

    Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

Junio: Perhaps the summary of this series in the merge message can be
refined a bit, as it only talks about verbose mode, but this series
does a bit more than that. Heres a proposal:

    `git worktree list` now annotates worktrees as prunable, shows
    locked and prunable attributes in --porcelain mode, and gained
    a --verbose option.
Rafael Silva Jan. 30, 2021, 9:42 a.m. UTC | #5 
Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Jan 27, 2021 at 3:03 AM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> Changes in v4
>>  * Added documentation to explain that the lock reason is quoted with
>>    the same rules as described for `core.quotePath`.
>>  * The `worktree unlock` issued in the test cleanup is splitted and
>>    executed after each `worktree lock` to ensure the unlock is only
>>    done after we know each locked command was successful.
>>  * Fix a couple of grammos in the [4/7] commit message.
>
> I just gave v4 a complete read-through and it looks great. All review
> comments on previous rounds have been addressed, and I didn't find
> anything important to comment on in v4. Very nicely done.
>
> Please consider this entire series:
>
>     Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
>

Thanks for the review and for all the insightful comments.

> Junio: Perhaps the summary of this series in the merge message can be
> refined a bit, as it only talks about verbose mode, but this series
> does a bit more than that. Heres a proposal:
>
>     `git worktree list` now annotates worktrees as prunable, shows
>     locked and prunable attributes in --porcelain mode, and gained
>     a --verbose option.

I believe this message summarize nicely all the changes implemented by
this series.
Junio C Hamano Jan. 30, 2021, 5:50 p.m. UTC | #6 
Eric Sunshine <sunshine@sunshineco.com> writes:

> On Wed, Jan 27, 2021 at 3:03 AM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> Changes in v4
>>  * Added documentation to explain that the lock reason is quoted with
>>    the same rules as described for `core.quotePath`.
>>  * The `worktree unlock` issued in the test cleanup is splitted and
>>    executed after each `worktree lock` to ensure the unlock is only
>>    done after we know each locked command was successful.
>>  * Fix a couple of grammos in the [4/7] commit message.
>
> I just gave v4 a complete read-through and it looks great. All review
> comments on previous rounds have been addressed, and I didn't find
> anything important to comment on in v4. Very nicely done.
>
> Please consider this entire series:
>
>     Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>
>
> Junio: Perhaps the summary of this series in the merge message can be
> refined a bit, as it only talks about verbose mode, but this series
> does a bit more than that. Heres a proposal:
>
>     `git worktree list` now annotates worktrees as prunable, shows
>     locked and prunable attributes in --porcelain mode, and gained
>     a --verbose option.

Nicely done.  Thanks for excellent contribution, both of you.