From: Rafael Silva <rafaeloliveira.cs@gmail.com>
To: git@vger.kernel.org
Cc: Eric Sunshine <sunshine@sunshineco.com>,
Phillip Wood <phillip.wood123@gmail.com>,
Rafael Silva <rafaeloliveira.cs@gmail.com>
Subject: [PATCH v2 0/6] teach `worktree list` verbose mode and prunable annotations
Date: Mon, 18 Jan 2021 00:42:38 +0100 [thread overview]
Message-ID: <20210117234244.95106-1-rafaeloliveira.cs@gmail.com> (raw)
In-Reply-To: <20210104162128.95281-1-rafaeloliveira.cs@gmail.com>
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/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 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 <rafaeloliveira.cs@gmail.com>
## 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 <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
## 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 <rafaeloliveira.cs@gmail.com>
## 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 <sunshine@sunshineco.com>
Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com>
## 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
--
2.30.0.356.gd4bb5ad4ba
next prev parent reply other threads:[~2021-01-17 23:46 UTC|newest]
Thread overview: 88+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-04 16:21 [PATCH 0/7] teach `worktree list` verbose mode and prunable annotations Rafael Silva
2021-01-04 16:21 ` [PATCH 1/7] worktree: move should_prune_worktree() to worktree.c Rafael Silva
2021-01-06 5:58 ` Eric Sunshine
2021-01-08 7:40 ` Rafael Silva
2021-01-06 6:55 ` Eric Sunshine
2021-01-07 7:24 ` Eric Sunshine
2021-01-08 7:41 ` Rafael Silva
2021-01-04 16:21 ` [PATCH 2/7] worktree: implement worktree_prune_reason() wrapper Rafael Silva
2021-01-06 7:08 ` Eric Sunshine
2021-01-08 7:42 ` Rafael Silva
2021-01-04 16:21 ` [PATCH 3/7] worktree: teach worktree_lock_reason() to gently handle main worktree Rafael Silva
2021-01-06 7:29 ` Eric Sunshine
2021-01-08 7:43 ` Rafael Silva
2021-01-04 16:21 ` [PATCH 4/7] worktree: teach `list` prunable annotation and verbose Rafael Silva
2021-01-06 8:31 ` Eric Sunshine
2021-01-08 7:45 ` Rafael Silva
2021-01-04 16:21 ` [PATCH 5/7] worktree: `list` escape lock reason in --porcelain Rafael Silva
2021-01-05 10:29 ` Phillip Wood
2021-01-05 11:02 ` [PATCH] worktree: add -z option for list subcommand Phillip Wood
2021-01-07 3:34 ` Eric Sunshine
2021-01-08 10:33 ` Phillip Wood
2021-01-10 7:27 ` Eric Sunshine
2021-01-06 9:07 ` [PATCH 5/7] worktree: `list` escape lock reason in --porcelain Eric Sunshine
2021-01-08 7:47 ` Rafael Silva
2021-01-06 8:59 ` Eric Sunshine
2021-01-04 16:21 ` [PATCH 6/7] worktree: add tests for `list` verbose and annotations Rafael Silva
2021-01-06 9:39 ` Eric Sunshine
2021-01-07 4:09 ` Eric Sunshine
2021-01-08 7:49 ` Rafael Silva
2021-01-04 16:21 ` [PATCH 7/7] worktree: document `list` verbose and prunable annotations Rafael Silva
2021-01-06 9:57 ` Eric Sunshine
2021-01-08 7:49 ` Rafael Silva
2021-01-06 5:36 ` [PATCH 0/7] teach `worktree list` verbose mode " Eric Sunshine
2021-01-08 7:38 ` Rafael Silva
2021-01-08 8:19 ` Eric Sunshine
2021-01-17 23:42 ` Rafael Silva [this message]
2021-01-17 23:42 ` [PATCH v2 1/6] worktree: libify should_prune_worktree() Rafael Silva
2021-01-17 23:42 ` [PATCH v2 2/6] worktree: teach worktree to lazy-load "prunable" reason Rafael Silva
2021-01-18 2:57 ` Eric Sunshine
2021-01-19 7:57 ` Rafael Silva
2021-01-17 23:42 ` [PATCH v2 3/6] worktree: teach worktree_lock_reason() to gently handle main worktree Rafael Silva
2021-01-17 23:42 ` [PATCH v2 4/6] worktree: teach `list --porcelain` to annotate locked worktree Rafael Silva
2021-01-18 3:55 ` Eric Sunshine
2021-01-19 8:20 ` Rafael Silva
2021-01-19 17:16 ` Eric Sunshine
2021-01-17 23:42 ` [PATCH v2 5/6] worktree: teach `list` to annotate prunable worktree Rafael Silva
2021-01-18 4:45 ` Eric Sunshine
2021-01-19 10:26 ` Rafael Silva
2021-01-19 17:23 ` Eric Sunshine
2021-01-17 23:42 ` [PATCH v2 6/6] worktree: teach `list` verbose mode Rafael Silva
2021-01-18 5:15 ` Eric Sunshine
2021-01-18 19:40 ` Eric Sunshine
2021-01-18 5:33 ` [PATCH v2 0/6] teach `worktree list` verbose mode and prunable annotations Eric Sunshine
2021-01-19 16:44 ` Rafael Silva
2021-01-19 21:27 ` [PATCH v3 0/7] " Rafael Silva
2021-01-19 21:27 ` [PATCH v3 1/7] worktree: libify should_prune_worktree() Rafael Silva
2021-01-19 21:27 ` [PATCH v3 2/7] worktree: teach worktree to lazy-load "prunable" reason Rafael Silva
2021-01-19 21:27 ` [PATCH v3 3/7] worktree: teach worktree_lock_reason() to gently handle main worktree Rafael Silva
2021-01-19 21:27 ` [PATCH v3 4/7] t2402: ensure locked worktree is properly cleaned up Rafael Silva
2021-01-24 7:50 ` Eric Sunshine
2021-01-24 10:19 ` Rafael Silva
2021-01-19 21:27 ` [PATCH v3 5/7] worktree: teach `list --porcelain` to annotate locked worktree Rafael Silva
2021-01-20 11:00 ` Phillip Wood
2021-01-21 3:18 ` Junio C Hamano
2021-01-21 15:25 ` Rafael Silva
2021-01-24 8:24 ` Eric Sunshine
2021-01-24 8:10 ` Eric Sunshine
2021-01-24 10:20 ` Rafael Silva
2021-01-19 21:27 ` [PATCH v3 6/7] worktree: teach `list` to annotate prunable worktree Rafael Silva
2021-01-21 3:28 ` Junio C Hamano
2021-01-21 15:09 ` Rafael Silva
2021-01-21 22:18 ` Junio C Hamano
2021-01-19 21:27 ` [PATCH v3 7/7] worktree: teach `list` verbose mode Rafael Silva
2021-01-24 8:42 ` Eric Sunshine
2021-01-24 10:21 ` Rafael Silva
2021-01-24 8:51 ` [PATCH v3 0/7] teach `worktree list` verbose mode and prunable annotations Eric Sunshine
2021-01-27 8:08 ` Rafael Silva
2021-01-27 8:03 ` Rafael Silva
2021-01-27 8:03 ` [PATCH v4 1/7] worktree: libify should_prune_worktree() Rafael Silva
2021-01-27 8:03 ` [PATCH v4 2/7] worktree: teach worktree to lazy-load "prunable" reason Rafael Silva
2021-01-27 8:03 ` [PATCH v4 3/7] worktree: teach worktree_lock_reason() to gently handle main worktree Rafael Silva
2021-01-27 8:03 ` [PATCH v4 4/7] t2402: ensure locked worktree is properly cleaned up Rafael Silva
2021-01-27 8:03 ` [PATCH v4 5/7] worktree: teach `list --porcelain` to annotate locked worktree Rafael Silva
2021-01-27 8:03 ` [PATCH v4 6/7] worktree: teach `list` to annotate prunable worktree Rafael Silva
2021-01-27 8:03 ` [PATCH v4 7/7] worktree: teach `list` verbose mode Rafael Silva
2021-01-30 7:04 ` [PATCH v3 0/7] teach `worktree list` verbose mode and prunable annotations Eric Sunshine
2021-01-30 9:42 ` Rafael Silva
2021-01-30 17:50 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210117234244.95106-1-rafaeloliveira.cs@gmail.com \
--to=rafaeloliveira.cs@gmail.com \
--cc=git@vger.kernel.org \
--cc=phillip.wood123@gmail.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).