[5/6] completion: list existing working trees for 'git worktree' subcommands
diff mbox series

Message ID 20191017173501.3198-6-szeder.dev@gmail.com
State New
Headers show
Series
  • completion: improve completion for 'git worktree'
Related show

Commit Message

SZEDER Gábor Oct. 17, 2019, 5:35 p.m. UTC
Complete the paths of existing working trees for 'git worktree's
'move', 'remove', 'lock', and 'unlock' subcommands.

Note that 'git worktree list --porcelain' shows absolute paths, so for
simplicity's sake we'll complete full absolute paths as well (as
opposed to turning them into relative paths by finding common leading
directories between $PWD and the working tree's path and removing
them, risking trouble with symbolic links or Windows drive letters; or
completing them one path component at a time).

Never list the path of the main working tree, as it cannot be moved,
removed, locked, or unlocked.

Arguably 'git worktree unlock <TAB>' should only complete locked
working trees, but 'git worktree list --porcelain' doesn't indicate
which working trees are locked.  So for now it will complete the paths
of all existing working trees, including non-locked ones as well.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 28 +++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

Comments

Eric Sunshine Oct. 17, 2019, 6:08 p.m. UTC | #1
On Thu, Oct 17, 2019 at 1:35 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> Complete the paths of existing working trees for 'git worktree's
> 'move', 'remove', 'lock', and 'unlock' subcommands.
> [...]
> Arguably 'git worktree unlock <TAB>' should only complete locked
> working trees, but 'git worktree list --porcelain' doesn't indicate
> which working trees are locked.  So for now it will complete the paths
> of all existing working trees, including non-locked ones as well.

It is a long-standing To-Do[1] for "git worktree list [--porcelain]"
to indicate whether a worktree is locked, prunable, etc. Looking at
the implementation of builtin/worktree.c:show_worktree_porcelain(), it
should be easy enough to add. (Adding it for the non-porcelain case
would perhaps require more thinking and design since we might want a
--verbose option to trigger the extra information.)

[1]: https://public-inbox.org/git/CAPig+cTTrv2C7JLu1dr4+N8xo+7YQ+deiwLDA835wBGD6fhS1g@mail.gmail.com/

> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> @@ -2981,10 +2981,21 @@ _git_whatchanged ()
> +__git_complete_worktree_paths ()
> +{
> +       local IFS=$'\n'
> +       __gitcomp_nl "$(git worktree list --porcelain |
> +               sed -n -e '2,$ s/^worktree //p')"
> +}

I know that the commit message talks about it, but it might deserve an
in-code comment here (or a function-level comment) explaining why the
first line of "git worktree list --porcelain" output is thrown away
since it's not necessarily obvious to the casual reader.
SZEDER Gábor Oct. 18, 2019, 3 p.m. UTC | #2
On Thu, Oct 17, 2019 at 02:08:12PM -0400, Eric Sunshine wrote:
> On Thu, Oct 17, 2019 at 1:35 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > Complete the paths of existing working trees for 'git worktree's
> > 'move', 'remove', 'lock', and 'unlock' subcommands.
> > [...]
> > Arguably 'git worktree unlock <TAB>' should only complete locked
> > working trees, but 'git worktree list --porcelain' doesn't indicate
> > which working trees are locked.  So for now it will complete the paths
> > of all existing working trees, including non-locked ones as well.
> 
> It is a long-standing To-Do[1] for "git worktree list [--porcelain]"
> to indicate whether a worktree is locked, prunable, etc. Looking at
> the implementation of builtin/worktree.c:show_worktree_porcelain(), it
> should be easy enough to add.

I didn't look at the implementation, but only at the docs, which says:

  --porcelain
      With list, output in an easy-to-parse format for scripts. This
      format will remain stable across Git versions and regardless of
      user configuration. See below for details.

I'm not sure whether introducing a new boolean attribute (i.e. a line
containing only "locked") would still be considered acceptable, or
would count as changing the format.  I can imagine that a too strict
parser would barf upon encountering the unrecognized "locked"
attribute; but yeah, no sensible parser should be that strict IMO.

Furthermore, I'm not sure what to do with the reason for locking.  In
general I would think that it makes sense to display the reason in an
easy-to-parse format as well.  However, doing so will inherently make
the format less easy to parse, because the reason could span multiple
lines, so without some sort of encoding/escaping it would violate the
"a line per attribute" format.

I would say that this is beyong the scope of this patch series :)

> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > @@ -2981,10 +2981,21 @@ _git_whatchanged ()
> > +__git_complete_worktree_paths ()
> > +{
> > +       local IFS=$'\n'
> > +       __gitcomp_nl "$(git worktree list --porcelain |
> > +               sed -n -e '2,$ s/^worktree //p')"
> > +}
> 
> I know that the commit message talks about it, but it might deserve an
> in-code comment

OK.
Eric Sunshine Oct. 18, 2019, 8:45 p.m. UTC | #3
On Fri, Oct 18, 2019 at 11:00 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Thu, Oct 17, 2019 at 02:08:12PM -0400, Eric Sunshine wrote:
> > It is a long-standing To-Do[1] for "git worktree list [--porcelain]"
> > to indicate whether a worktree is locked, prunable, etc. Looking at
> > the implementation of builtin/worktree.c:show_worktree_porcelain(), it
> > should be easy enough to add.
>
> I didn't look at the implementation, but only at the docs, which says:
>
>   --porcelain
>       With list, output in an easy-to-parse format for scripts. This
>       format will remain stable across Git versions and regardless of
>       user configuration. See below for details.
>
> I'm not sure whether introducing a new boolean attribute (i.e. a line
> containing only "locked") would still be considered acceptable, or
> would count as changing the format.  I can imagine that a too strict
> parser would barf upon encountering the unrecognized "locked"
> attribute; but yeah, no sensible parser should be that strict IMO.

The stanza-based format of the porcelain output was chosen with the
specific intention of being easy to parse _and_ to allow extension
such as the introduction of new attributes. It is unfortunate that the
documentation you quoted, as well as the description of the porcelain
format itself, does a poor job of (or utterly fails at) conveying that
intention. (Had I been around to review the later iteration(s) of the
series which introduced the porcelain format, I likely would have
pointed out these shortcomings, however, Real Life had other ideas,
and I didn't manage to review it until weeks after the series had made
it into an actual release.)

I'm not convinced, though, that we're locked into exactly the few
attributes shown only in an example in the porcelain section of the
documentation. That documentation is so woefully underspecified --
indeed, it doesn't even talk about what attributes are available, but
only gives a single example showing some of the (possible) attributes
-- that it effectively makes no promises about what will or will not
appear in a stanza. (The only thing is says strongly is that stanzas
will be separated by a blank line -- despite the intention all along
being that each new stanza would start with a "worktree" attributes,
and that blank lines, if output, were to be ignored.)

So, I am of the opinion that we are not prevented from adding new
attributes, such as "locked" and "prunable".

> Furthermore, I'm not sure what to do with the reason for locking.  In
> general I would think that it makes sense to display the reason in an
> easy-to-parse format as well.  However, doing so will inherently make
> the format less easy to parse, because the reason could span multiple
> lines, so without some sort of encoding/escaping it would violate the
> "a line per attribute" format.

Yep, I believe my thinking at the time was that the lock reason and
the prunable reason would be escaped if needed. So, for instance:

    worktree /blah
    branch refs/heads/blah
    locked Sneaker-net removable storage\nNot always mounted

> I would say that this is beyong the scope of this patch series :)

Oh, I wasn't suggesting that this series do anything of the sort.
Instead, I was merely responding to the point in the commit message
that the porcelain format was lacking information about the lock, and
to say that it could (eventually) be solved by extending the output.

Patch
diff mbox series

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 643272eb2f..4eb13b06d6 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2981,10 +2981,21 @@  _git_whatchanged ()
 	_git_log
 }
 
+__git_complete_worktree_paths ()
+{
+	local IFS=$'\n'
+	__gitcomp_nl "$(git worktree list --porcelain |
+		sed -n -e '2,$ s/^worktree //p')"
+}
+
 _git_worktree ()
 {
 	local subcommands="add list lock move prune remove unlock"
-	local subcommand="$(__git_find_on_cmdline "$subcommands")"
+	local subcommand subcommand_idx
+
+	subcommand="$(__git_find_on_cmdline --show-idx "$subcommands")"
+	subcommand_idx="${subcommand% *}"
+	subcommand="${subcommand#* }"
 
 	case "$subcommand,$cur" in
 	,*)
@@ -2993,6 +3004,21 @@  _git_worktree ()
 	*,--*)
 		__gitcomp_builtin worktree_$subcommand
 		;;
+	lock,*|remove,*|unlock,*)
+		__git_complete_worktree_paths
+		;;
+	move,*)
+		if [ $cword -eq $((subcommand_idx+1)) ]; then
+			# The first parameter must be an existing working
+			# tree to be moved.
+			__git_complete_worktree_paths
+		else
+			# The second parameter is the destination: it could
+			# be any path, so don't list anything, but let Bash
+			# fall back to filename completion.
+			:
+		fi
+		;;
 	esac
 }