| Message ID | 20210117234244.95106-6-rafaeloliveira.cs@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | teach `worktree list` verbose mode and prunable annotations | expand |
On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva <rafaeloliveira.cs@gmail.com> wrote: > The "git worktree list" command shows the absolute path to the worktree, > the commit that is checked out, the name of the branch, and a "locked" > annotation if the worktree is locked, however, it does not indicate > whether the worktree is prunable. > [...] > Let's teach "git worktree list" to show when a worktree is a prunable > candidate for both default and porcelain format. Good. > diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt > @@ -234,6 +235,9 @@ This can also be set up as the default behaviour by using the > --expire <time>:: > With `prune`, only expire unused working trees older than `<time>`. > ++ > +With `list`, annotate missing working trees as prunable if they are > +older than `<mtime>`. s/mtime/time/ > diff --git a/builtin/worktree.c b/builtin/worktree.c > @@ -592,6 +592,10 @@ static void show_worktree_porcelain(struct worktree *wt) > + reason = worktree_prune_reason(wt, expire); > + if (reason && *reason) > + printf("prunable %s\n", reason); I lean toward not including `*reason` in the condition here. While one may argue that `*reason` is future-proofing the condition, we know today that worktree_prune_reason() will only ever return NULL or a non-empty string. So, having `*reason` in the condition is misleading and confuses readers into thinking that worktree_prune_reason() could return an empty string or that perhaps it did in the past. And studying the history of this file or even this commit won't help them understand why the author included `*reason` in the condition. > @@ -617,9 +622,14 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len) > - if (!is_main_worktree(wt) && worktree_lock_reason(wt)) > + reason = worktree_lock_reason(wt); > + if (reason) > strbuf_addstr(&sb, " locked"); Although I understand what happened here because I read the entire series, for anyone reading this patch in the future or even someone reading this patch in isolation, the disappearance of is_main_worktree() from the condition is mysterious. They won't know if its removal was an accident or intentional, or if the change introduces a bug. Therefore, it would be better to drop is_main_worktree() from this conditional in patch [3/6], which is the patch which makes it safe to call worktree_lock_reason() even for the main worktree. Thus, in [3/6], this would change from: if (!is_main_worktree(wt) && worktree_lock_reason(wt)) to: if (worktree_lock_reason(wt)) and then in this patch [5/6], it becomes: reason = worktree_lock_reason(wt); if (reason) > + reason = worktree_prune_reason(wt, expire); > + if (reason) > + strbuf_addstr(&sb, " prunable"); Looking at this also makes me wonder if patches [5/6] and [6/6] should be swapped since it's not clear to the reader why you're adding the `reason` variable in this patch when the same could be accomplished more simply: if (worktree_lock_reason(wt)) strbuf_addstr(&sb, " locked"); if (worktree_prune_reason(wt, expire)) strbuf_addstr(&sb, " prunable"); If you swap the patches and add --verbose mode first which requires this new `reason` variable, then the mystery goes away, and the use of `reason` is obvious when `prunable` annotation is added in the subsequent patch. Having said that, I'm not trying to make extra work for you by expecting patch perfection. Sometimes it's fine to craft a patch in such a way that it makes subsequent patches simpler, even if it looks slightly odd in the current patch, and I haven't read [6/6] yet, so whatever opinion I express here is based only upon what I've read up to this point.
Eric Sunshine writes: > On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva > <rafaeloliveira.cs@gmail.com> wrote: >> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt >> @@ -234,6 +235,9 @@ This can also be set up as the default behaviour by using the >> --expire <time>:: >> With `prune`, only expire unused working trees older than `<time>`. >> ++ >> +With `list`, annotate missing working trees as prunable if they are >> +older than `<mtime>`. > > s/mtime/time/ > Oops. thanks for catching that. Will fix it in the next revision. >> diff --git a/builtin/worktree.c b/builtin/worktree.c >> @@ -592,6 +592,10 @@ static void show_worktree_porcelain(struct worktree *wt) >> + reason = worktree_prune_reason(wt, expire); >> + if (reason && *reason) >> + printf("prunable %s\n", reason); > > I lean toward not including `*reason` in the condition here. While one > may argue that `*reason` is future-proofing the condition, we know > today that worktree_prune_reason() will only ever return NULL or a > non-empty string. So, having `*reason` in the condition is misleading > and confuses readers into thinking that worktree_prune_reason() could > return an empty string or that perhaps it did in the past. And > studying the history of this file or even this commit won't help them > understand why the author included `*reason` in the condition. > Fair enough. The `*reason` condition was indeed added to be safe and future-proofing and, as you pointed it out, we know that currently the worktree_prune_reason() returns a non-empty string when its true and when the code reaches the `*reason` condition in "if (reason && *reason)" this will always evaluates to true. Agreed that removing this part of the condition will make the code clearer and easier to followed. So I will drop this in the next revision. >> @@ -617,9 +622,14 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len) >> - if (!is_main_worktree(wt) && worktree_lock_reason(wt)) >> + reason = worktree_lock_reason(wt); >> + if (reason) >> strbuf_addstr(&sb, " locked"); > > Although I understand what happened here because I read the entire > series, for anyone reading this patch in the future or even someone > reading this patch in isolation, the disappearance of > is_main_worktree() from the condition is mysterious. They won't know > if its removal was an accident or intentional, or if the change > introduces a bug. Therefore, it would be better to drop > is_main_worktree() from this conditional in patch [3/6], which is the > patch which makes it safe to call worktree_lock_reason() even for the > main worktree. Thus, in [3/6], this would change from: > > if (!is_main_worktree(wt) && worktree_lock_reason(wt)) > > to: > > if (worktree_lock_reason(wt)) > > and then in this patch [5/6], it becomes: > > reason = worktree_lock_reason(wt); > if (reason) > That's a good point, and even worse this is not mentioned in my commit message at all. It make sense to move this change into [3/6] where the API is changed and all the reason is explained in the commit message. >> + reason = worktree_prune_reason(wt, expire); >> + if (reason) >> + strbuf_addstr(&sb, " prunable"); > > Looking at this also makes me wonder if patches [5/6] and [6/6] should > be swapped since it's not clear to the reader why you're adding the > `reason` variable in this patch when the same could be accomplished > more simply: > > if (worktree_lock_reason(wt)) > strbuf_addstr(&sb, " locked"); > if (worktree_prune_reason(wt, expire)) > strbuf_addstr(&sb, " prunable"); > > If you swap the patches and add --verbose mode first which requires > this new `reason` variable, then the mystery goes away, and the use of > `reason` is obvious when `prunable` annotation is added in the > subsequent patch. > > Having said that, I'm not trying to make extra work for you by > expecting patch perfection. Sometimes it's fine to craft a patch in > such a way that it makes subsequent patches simpler, even if it looks > slightly odd in the current patch, and I haven't read [6/6] yet, so > whatever opinion I express here is based only upon what I've read up > to this point. That's a good point. I'm inclined to leave the [5/6] with the following: if (worktree_prune_reason(wt, expire)) strbuf_addstr(&sb, " prunable"); And move up the changes that includes the `reason` into the [5/6] patches that introduces the --verbose option. This line seems easier to follow when the reader is looking on this patch alone and only care about a reason when the --verbose comes into play in the next patch [6/6]. Although your suggestion about changing the patch also sounds reasonable and I'll take into consideration when I re-roll this series. Btw, I don't mind spending extra work on and I'm all forward with the changes so we it would be easier to understand not only now where all the patches are being reviewed together but in the future when someone is looking at the history of the project, specially for debugging/bisecting reasons. Thanks for the insightful comments.
On Tue, Jan 19, 2021 at 5:26 AM Rafael Silva <rafaeloliveira.cs@gmail.com> wrote: > Eric Sunshine writes: > > On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva > > <rafaeloliveira.cs@gmail.com> wrote: > >> + reason = worktree_prune_reason(wt, expire); > >> + if (reason) > >> + strbuf_addstr(&sb, " prunable"); > > > > Looking at this also makes me wonder if patches [5/6] and [6/6] should > > be swapped since it's not clear to the reader why you're adding the > > `reason` variable in this patch when the same could be accomplished > > more simply: > > > > if (worktree_prune_reason(wt, expire)) > > strbuf_addstr(&sb, " prunable"); > > > > If you swap the patches and add --verbose mode first which requires > > this new `reason` variable, then the mystery goes away, and the use of > > `reason` is obvious when `prunable` annotation is added in the > > subsequent patch. > > That's a good point. I'm inclined to leave the [5/6] with the following: > > if (worktree_prune_reason(wt, expire)) > strbuf_addstr(&sb, " prunable"); > > And move up the changes that includes the `reason` into the [5/6] > patches that introduces the --verbose option. This line seems easier to > follow when the reader is looking on this patch alone and only care > about a reason when the --verbose comes into play in the next patch > [6/6]. I considered this, as well, but figured that it would make patch [6/6] even noiser, and that swapping the patches would keep the noise level down. But, I haven't actually tried it myself, so I could be wrong about the assumption, and maybe the noisiness wouldn't be a problem in practice or would be so stylized that it wouldn't matter.
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt index d352a002f2..3d8c14dbdf 100644 --- a/Documentation/git-worktree.txt +++ b/Documentation/git-worktree.txt @@ -97,8 +97,9 @@ list:: List details of each working tree. The main working tree is listed first, followed by each of the linked working trees. The output details include whether the working tree is bare, the revision currently checked out, the -branch currently checked out (or "detached HEAD" if none), and "locked" if -the worktree is locked. +branch currently checked out (or "detached HEAD" if none), "locked" if +the worktree is locked, "prunable" if the worktree can be pruned by `prune` +command. lock:: @@ -234,6 +235,9 @@ This can also be set up as the default behaviour by using the --expire <time>:: With `prune`, only expire unused working trees older than `<time>`. ++ +With `list`, annotate missing working trees as prunable if they are +older than `<mtime>`. --reason <string>:: With `lock`, an explanation why the working tree is locked. @@ -372,6 +376,19 @@ $ git worktree list /path/to/other-linked-worktree 1234abc (detached HEAD) ------------ +The command also shows annotations for each working tree, according to its state. +These annotations are: + + * `locked`, if the working tree is locked. + * `prunable`, if the working tree can be pruned via `git worktree prune`. + +------------ +$ git worktree list +/path/to/linked-worktree abcd1234 [master] +/path/to/locked-worktreee acbd5678 (brancha) locked +/path/to/prunable-worktree 5678abc (detached HEAD) prunable +------------ + Porcelain Format ~~~~~~~~~~~~~~~~ The porcelain format has a line per attribute. Attributes are listed with a @@ -405,6 +422,11 @@ HEAD 3456def3456def3456def3456def3456def3456b branch refs/heads/locked-with-reason locked reason why is locked +worktree /path/to/linked-worktree-prunable +HEAD 1233def1234def1234def1234def1234def1234b +detached +prunable gitdir file points to non-existent location + ------------ EXAMPLES diff --git a/builtin/worktree.c b/builtin/worktree.c index 37ae277352..fb82d7bb64 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -592,6 +592,10 @@ static void show_worktree_porcelain(struct worktree *wt) } else if (reason) printf("locked\n"); + reason = worktree_prune_reason(wt, expire); + if (reason && *reason) + printf("prunable %s\n", reason); + printf("\n"); } @@ -600,6 +604,7 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len) struct strbuf sb = STRBUF_INIT; int cur_path_len = strlen(wt->path); int path_adj = cur_path_len - utf8_strwidth(wt->path); + const char *reason; strbuf_addf(&sb, "%-*s ", 1 + path_maxlen + path_adj, wt->path); if (wt->is_bare) @@ -617,9 +622,14 @@ static void show_worktree(struct worktree *wt, int path_maxlen, int abbrev_len) strbuf_addstr(&sb, "(error)"); } - if (!is_main_worktree(wt) && worktree_lock_reason(wt)) + reason = worktree_lock_reason(wt); + if (reason) strbuf_addstr(&sb, " locked"); + reason = worktree_prune_reason(wt, expire); + if (reason) + strbuf_addstr(&sb, " prunable"); + printf("%s\n", sb.buf); strbuf_release(&sb); } @@ -663,9 +673,12 @@ static int list(int ac, const char **av, const char *prefix) struct option options[] = { OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")), + OPT_EXPIRY_DATE(0, "expire", &expire, + N_("add 'prunable' annotation to worktrees older than <time>")), OPT_END() }; + expire = TIME_MAX; ac = parse_options(ac, av, prefix, options, worktree_usage, 0); if (ac) usage_with_options(worktree_usage, options); diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh index c522190feb..e9b410b69e 100755 --- a/t/t2402-worktree-list.sh +++ b/t/t2402-worktree-list.sh @@ -103,6 +103,38 @@ test_expect_success '"list" all worktrees --porcelain with locked reason newline test_cmp expect actual ' +test_expect_success '"list" all worktrees with prunable annotation' ' + test_when_finished "rm -rf prunable unprunable out && git worktree prune" && + git worktree add --detach prunable && + git worktree add --detach unprunable && + rm -rf prunable && + git worktree list >out && + grep "/prunable *[0-9a-f].* prunable$" out && + ! grep "/unprunable *[0-9a-f].* prunable$" +' + +test_expect_success '"list" all worktrees --porcelain with prunable' ' + test_when_finished "rm -rf prunable out && git worktree prune" && + git worktree add --detach prunable && + rm -rf prunable && + git worktree list --porcelain >out && + sed -n "/^worktree .*\/prunable$/,/^$/p" <out >only_prunable && + test_i18ngrep "^prunable gitdir file points to non-existent location$" only_prunable +' + +test_expect_success '"list" all worktrees with prunable consistent with "prune"' ' + test_when_finished "rm -rf prunable unprunable out && git worktree prune" && + git worktree add --detach prunable && + git worktree add --detach unprunable && + rm -rf prunable && + git worktree list >out && + grep "/prunable *[0-9a-f].* prunable$" out && + ! grep "/unprunable *[0-9a-f].* unprunable$" out && + git worktree prune --verbose >out && + test_i18ngrep "^Removing worktrees/prunable" out && + test_i18ngrep ! "^Removing worktrees/unprunable" out +' + test_expect_success 'bare repo setup' ' git init --bare bare1 && echo "data" >file1 &&
The "git worktree list" command shows the absolute path to the worktree, the commit that is checked out, the name of the branch, and a "locked" annotation if the worktree is locked, however, it does not indicate whether the worktree is prunable. The "prune" command will remove a worktree if it is prunable unless `--dry-run` option is specified. This could lead to a worktree being removed without the user realizing before it is too late, in case the user forgets to pass --dry-run for instance. If the "list" command shows which worktree is prunable, the user could verify before running "git worktree prune" and hopefully prevents the working tree to be removed "accidentally" on the worse case scenario. Let's teach "git worktree list" to show when a worktree is a prunable candidate for both default and porcelain format. In the default format a "prunable" text is appended: $ git worktree list /path/to/main aba123 [main] /path/to/linked 123abc [branch-a] /path/to/prunable ace127 (detached HEAD) prunable In the --porcelain format a prunable label is added followed by its reason: $ git worktree list --porcelain ... worktree /path/to/prunable HEAD abc1234abc1234abc1234abc1234abc1234abc12 detached prunable gitdir file points to non-existent location ... Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> --- Documentation/git-worktree.txt | 26 ++++++++++++++++++++++++-- builtin/worktree.c | 15 ++++++++++++++- t/t2402-worktree-list.sh | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 3 deletions(-)