Re: [RFC PATCH 0/2] teach `worktree list` to mark locked worktrees

[Rafael Silva <rafaeloliveira.cs@gmail.com>]

Thank you for all the feedback, I will work on a another
patch that will address all the feedback provided on this series,
including having the changes into a single patch.

On Mon, Sep 28, 2020 at 02:19:53PM -0700, Junio C Hamano wrote:
> Rafael Silva <rafaeloliveira.cs@gmail.com> writes:
> 
> > This patch series introduces a new information on the git `worktree list`
> > command output, to mark when a worktree is locked with a (locked) text mark.
> >
> > The intent is to improve the user experience to earlier sinalize that a linked
> > worktree is locked, instead of realising later when attempting to remove it
> > with `remove` command as it happened to me twice :)
> 
> Change with a good intention, it seems.
> 
> > The patches are divided into two parts. First part introduces
> > the new marker to the worktree list command and small documentation
> > change. And the second adds one test case into t2402 to test
> > if the (locked) text will be properly set for a locked worktree, and
> > not mistankely set to a unlocked or master worktree.
> 
> Probably they belong together in a single patch (I am saying this
> after only seeing the above five lines, without reading either of
> these two patches, so there may be some things in them that makes it
> make sense to have them separate).
> 

Yes, I believe it make sense to have them in a single patch.

> > This is the output of the worktree list with locked marker:
> >
> >   $ git worktree list
> >   /repo/to/main                abc123 [master]
> >   /path/to/unlocked-worktree1  456def [brancha]
> >   /path/to/locked-worktree     123abc (detached HEAD) (locked)
> 
> Looks OK to me
> 
> > This patches are marked with RFC mainly due to:
> >
> >   - This will change the default behaviour of the worktree list, I am
> >     not sure whether will be better to make this tuned via a config
> >     and/or a git parameter. (assuming this change is a good idea ;) )
> 
> The default output is meant for human consumption (scripts that want
> to read from the command and expect stable output would be using the
> "--porcelain" option).
> 
> The ideal case is a new output is universally useful for everybody,
> in which case we can just change it without any new configuration or
> command line option.

Thanks for the explanation, will keep that in mind.

> 
> >   - Perhaps the `(locked)` marker is not the best suitable way to output
> >     this information and we might need to come with a better way.
> 
> It looks good enough to me.  I am not qualified to have a strong
> opinion on this part, as I do not use the command all that often.
> 
>     $ git shortlog --no-merges --since=18.months builtin/worktree.c
> 
> tells me that Eric Sunshine (CC'ed) may be a good source of wisdom
> on this command.
> 

Thank you, I will keep Eric Sunshine in the loop as well.

> >   - I am a new contributor to the code base, still learning a lot of git
> >     internals data structure and commands. Likely this patch will require
> >     updates.
> 
> Welcome.

Thank you.

> 
> > Rafael Silva (2):
> >   teach `list` to mark locked worktree
> >   t2402: add test to locked linked worktree marker
> >
> >  Documentation/git-worktree.txt |  5 +++--
> >  builtin/worktree.c             |  6 +++++-
> >  t/t2402-worktree-list.sh       | 13 +++++++++++++
> >  3 files changed, 21 insertions(+), 3 deletions(-)