Re: [PATCH v2 5/6] worktree: teach `list` to annotate prunable worktree

On Tue, Jan 19, 2021 at 5:26 AM Rafael Silva
[off-list ref] wrote:

Eric Sunshine writes:

quoted

On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva
[off-list ref] wrote:

quoted

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