On Tue, Jan 19, 2021 at 5:26 AM Rafael Silva <rafaeloliveira.cs@xxxxxxxxx> wrote: > Eric Sunshine writes: > > On Sun, Jan 17, 2021 at 6:43 PM Rafael Silva > > <rafaeloliveira.cs@xxxxxxxxx> 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.
