[v2,0/6] teach `worktree list` verbose mode and prunable annotations

Message

Rafael Silva Jan. 17, 2021, 11:42 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. The fourth patch adds the "locked" attribute for the porcelain format
   in order to make both default and --porcelain format consistent.

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

6. 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/[e-mail]/
[2] https://lore.kernel.org/git/[e-mail]/
[3] https://lore.kernel.org/git/CAPig+cSGXqJuaZPhUhOVX5X=[e-mail]/
[4] https://lore.kernel.org/git/CAPig+cTitWCs5vB=0iXuUyEY22c0gvjXvY1ZtTT90s74ydhE=[e-mail]/
[5] https://lore.kernel.org/git/[e-mail]/
[6] https://lore.kernel.org/git/[e-mail]/

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 (6):
  worktree: libify should_prune_worktree()
  worktree: teach worktree to lazy-load "prunable" reason
  worktree: teach worktree_lock_reason() to gently handle main worktree
  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 +++++++++++----------------------
 t/t2402-worktree-list.sh       |  91 +++++++++++++++++++++++++++
 worktree.c                     |  91 ++++++++++++++++++++++++++-
 worktree.h                     |  23 +++++++
 5 files changed, 297 insertions(+), 80 deletions(-)

Range-diff against v1:
1:  9d3fff52b4 ! 1:  e4df49d882 worktree: move should_prune_worktree() to worktree.c
    @@ Metadata
     Author: Rafael Silva <[e-mail]>
     
      ## Commit message ##
    -    worktree: move should_prune_worktree() to worktree.c
    +    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.
     
    -    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 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.
     
    -    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.
    +    Also, change the function comment to clearly state that the worktree's
    +    path is returned in `wtpath` argument.
     
    +    Helped-by: Eric Sunshine <[e-mail]>
         Signed-off-by: Rafael Silva <[e-mail]>
     
      ## builtin/worktree.c ##
    @@ worktree.c: void repair_worktree_at_path(const char *path,
      	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;
    @@ worktree.h: 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.
    ++ * pruning. Otherwise, return false and the worktree's path in `wtpath`, or
    ++ * NULL if it cannot be determined. Caller is responsible for freeing
    ++ * returned path.
    ++ *
    ++ * `expire` defines a grace period to prune the worktree when its path
    ++ * does not exist.
     + */
     +int should_prune_worktree(const char *id,
     +			  struct strbuf *reason,
2:  80044e0ba9 ! 2:  d8db43fec8 worktree: implement worktree_prune_reason() wrapper
    @@ Metadata
     Author: Rafael Silva <[e-mail]>
     
      ## Commit message ##
    -    worktree: implement worktree_prune_reason() wrapper
    +    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.
    -
    -    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.
    +    Add worktree_prune_reason() to allow a caller to discover whether a
    +    worktree is prunable and the reason that it is, much like
    +    worktree_lock_reason() indicates whether a worktree is locked and the
    +    reason for the lock. As with worktree_lock_reason(), retrieve the
    +    prunable reason lazily and cache it in the `worktree` structure.
     
    +    Helped-by: Eric Sunshine <[e-mail]>
         Signed-off-by: Rafael Silva <[e-mail]>
     
      ## worktree.c ##
    @@ worktree.c: 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;
    ++	struct strbuf reason = STRBUF_INIT;
    ++	char *path;
     +
    -+		if (should_prune_worktree(wt->id, &reason, &path, expire))
    -+			wt->prune_reason = strbuf_detach(&reason, NULL);
    -+		else
    -+			wt->prune_reason = NULL;
    ++	if (is_main_worktree(wt))
    ++		return NULL;
    ++	if (wt->prune_reason_valid)
    ++		return wt->prune_reason;
     +
    -+		free(path);
    -+		strbuf_release(&reason);
    -+	}
    ++	if (should_prune_worktree(wt->id, &reason, &path, expire))
    ++		wt->prune_reason = strbuf_detach(&reason, NULL);
    ++	wt->prune_reason_valid = 1;
     +
    ++	strbuf_release(&reason);
    ++	free(path);
     +	return wt->prune_reason;
     +}
     +
    @@ worktree.h: struct worktree {
      	struct object_id head_oid;
      	int is_detached;
      	int is_bare;
    + 	int is_current;
    + 	int lock_reason_valid; /* private */
    ++	int prune_reason_valid; /* private */
    + };
    + 
    + /*
     @@ worktree.h: 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.
    ++ * Return the reason string if the given worktree should be pruned, otherwise
    ++ * NULL if it should not be pruned. `expire` defines a grace period to prune
    ++ * the worktree when its path does not exist.
     + */
     +const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire);
     +
      /*
       * Return true if worktree entry should be pruned, along with the reason for
    -  * pruning. Otherwise, return false and the worktree's path, or NULL if it
    +  * pruning. Otherwise, return false and the worktree's path in `wtpath`, or
3:  fa2297714b < -:  ---------- worktree: teach worktree_lock_reason() to gently handle main worktree
4:  bcc2e5d745 < -:  ---------- worktree: teach `list` prunable annotation and verbose
5:  588051ed5f < -:  ---------- worktree: `list` escape lock reason in --porcelain
6:  cbff8c993a < -:  ---------- worktree: add tests for `list` verbose and annotations
7:  9c5893d2b9 < -:  ---------- worktree: document `list` verbose and prunable annotations
-:  ---------- > 3:  5b8963292f worktree: teach worktree_lock_reason() to gently handle main worktree
-:  ---------- > 4:  6bdd46b9bc worktree: teach `list --porcelain` to annotate locked worktree
-:  ---------- > 5:  bbf6c53ecc worktree: teach `list` to annotate prunable worktree
-:  ---------- > 6:  acc0bf22cd worktree: teach `list` verbose mode

Comments

Eric Sunshine Jan. 18, 2021, 5:33 a.m. UTC | #1

On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
<[e-mail]> wrote:
> This patch series addresses some of these changes by teaching
> `worktree list` to show "prunable" annotation, adding verbose mode and
> extending the --porcelain format with prunable and locked annotation as
> follow up from [1]. Additionally, it addresses one shortcoming for porcelain
> format to escape any newline characters (LF or CRLF) for the lock reason
> to prevent breaking the format that is mentioned in [4] and [1] during the
> review cycle.

Thanks for continuing to work on this. I'm pleased to see these
enhancements coming together so nicely.

> The patch series is organizes as:

The new organization is a nice improvement over v1.

As mentioned in my review of [5/6], there may be some value in
swapping [5/6] and [6/6] to make it easier for readers to digest the
changes, but it's not a hard requirement.

To avoid being mysterious, there's also a change in [5/6] which
probably belongs in [3/6], as explained in my review of [5/6].

My review of patch [4/6] suggests optionally splitting out a bug fix
into its own patch, but that's a very minor issue. Use your best
judgment whether or not to do so.

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

This patch does need a re-roll, and it's entirely my fault. In my
review of the previous round, I said that if the lock reason contained
special characters, we'd want to escape those characters and quote the
string, but then I gave you a code suggestion which did the opposite
of that. My [4/6] review goes into more detail.

Aside from the one important fix in [4/6], and the possible minor
re-organizations suggested above, all my other review comments were
very minor and probably subjective, thus nothing to sweat over.

Nicely done.

[1]: https://lore.kernel.org/git/[e-mail]/

Rafael Silva Jan. 19, 2021, 4:44 p.m. UTC | #2

Eric Sunshine writes:

> On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
> <[e-mail]> wrote:
>> This patch series addresses some of these changes by teaching
>> `worktree list` to show "prunable" annotation, adding verbose mode and
>> extending the --porcelain format with prunable and locked annotation as
>> follow up from [1]. Additionally, it addresses one shortcoming for porcelain
>> format to escape any newline characters (LF or CRLF) for the lock reason
>> to prevent breaking the format that is mentioned in [4] and [1] during the
>> review cycle.
>
> Thanks for continuing to work on this. I'm pleased to see these
> enhancements coming together so nicely.
>
>> The patch series is organizes as:
>
> The new organization is a nice improvement over v1.
>
> As mentioned in my review of [5/6], there may be some value in
> swapping [5/6] and [6/6] to make it easier for readers to digest the
> changes, but it's not a hard requirement.
>
> To avoid being mysterious, there's also a change in [5/6] which
> probably belongs in [3/6], as explained in my review of [5/6].
>
> My review of patch [4/6] suggests optionally splitting out a bug fix
> into its own patch, but that's a very minor issue. Use your best
> judgment whether or not to do so.
>
>> 4. The fourth patch adds the "locked" attribute for the porcelain format
>>    in order to make both default and --porcelain format consistent.
>
> This patch does need a re-roll, and it's entirely my fault. In my
> review of the previous round, I said that if the lock reason contained
> special characters, we'd want to escape those characters and quote the
> string, but then I gave you a code suggestion which did the opposite
> of that. My [4/6] review goes into more detail.
>

I kindly disagree and I actually think is my fault :).

> Aside from the one important fix in [4/6], and the possible minor
> re-organizations suggested above, all my other review comments were
> very minor and probably subjective, thus nothing to sweat over.
>
> Nicely done.
>
> [1]: https://lore.kernel.org/git/[e-mail]/

Thank you for reviewing this series and all the helpful suggestions. I
will send another revision taking all of them into account.

--
Thanks
Rafael