Re: [RFC PATCH 0/2] teach `worktree list` to mark locked worktrees
- From
Rafael Silva <rafaeloliveira.cs@gmail.com>
- Date
- Sep 29, 2020, 21:35 UTC
- Message-ID
- <20200929213529.GA1336@contrib-buster.localdomain>
- In-Reply-To
- <xmqqft71lhty.fsf@gitster.c.googlers.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:
Show 22 quoted lines
> 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.
Show 22 quoted lines
> > 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.
Show 12 quoted lines
> > > - 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.
Show 5 quoted lines
> > - 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.
Show 9 quoted lines
> > > 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(-)