Re: [RFC/PATCH] contrib: teach completion about git-worktree options and arguments

[Eric Sunshine <sunshine@sunshineco.com>]

On Fri, Aug 21, 2015 at 4:49 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> Quoting Junio C Hamano <gitster@pobox.com>:
>> Eric Sunshine <sunshine@sunshineco.com> writes:
>>> On Thu, Jul 23, 2015 at 4:49 PM, Eric Sunshine <sunshine@sunshineco.com>
>>> wrote:
>>>> Complete subcommands 'add' and 'prune', as well as their respective
>>>> options --force, --detach, --dry-run, --verbose, and --expire. Also
>>>> complete 'refname' in "git worktree add [-b <newbranch>] <path>
>>>> <refname>".
>>>
>>> Ping[1]?
>>
>> Ping indeed?
>
> Yeah, right, sorry.  Non-subscribed occasional gmane-reader here amidst
> job hunting, so thanks for the ping.  And the re-ping...

Thanks for the review.

>>>> +_git_worktree ()
>>>> +{
>>>> +       local subcommands='add prune'
>>>> +       local subcommand="$(__git_find_on_cmdline "$subcommands")"
>>>> +       local c=2 n=0 refpos=2
>
> A more descriptive variable name for 'n' would be great.

Indeed. I was planning on resubmitting with better variable names
(even if I didn't get any feedback)...

>>>> +       if [ -z "$subcommand" ]; then
>>>> +               __gitcomp "$subcommands"
>>>> +       else
>>>> +               case "$subcommand,$cur" in
>>>> +               add,--*)
>>>> +                       __gitcomp "--force --detach"
>
> We usually don't offer '--force', because that option must be
> handled with care.

As a person who never uses git-completion (or git-prompt), I wasn't
aware of that, so thanks for the heads-up. I only installed completion
(and prompt) when I decided to work on this (and I don't think I've
used tab-completion since then).

>>>> +                       ;;
>>>> +               add,*)
>>>> +                       while [ $c -lt $cword ]; do
>>>> +                               case "${words[c]}" in
>>>> +                               --*) ;;
>>>> +                               -[bB]) ((refpos++)) ;;
>>>> +                               *) ((n++)) ;;
>>>> +                               esac
>>>> +                               ((c++))
>>>> +                       done
>>>> +                       if [ $n -eq $refpos ]; then
>
> I suppose here you wanted to calculate where (i.e. at which word on
> the command line) we should offer refs and fall back to bash builtin
> filename completion otherwise.  It works well in the common cases,
> but:
>
>   - it doesn't offer refs after -b or -B

That was intentional since this is a new branch name, so it didn't
seem sensible to offer completion of existing refs. On the other hand,
it doesn't make much since to offer pathname completion either, but I
didn't see a better alternative. On reflection, I suppose ref
completion can make sense if the new branch name is going to be
similar to an existing one.

>   - it gets fooled by options to the git command, e.g. 'git
>     --git-dir=.git worktree add <TAB>' offers refs instead of files,
>     'git --git-dir=.git worktree add ../some/path <TAB>' offers
>     refs, etc.

This shortcoming could be addressed by computing relative to the
subcommand-index as your version does, correct?

I don't have enough background with the (git-specific) completion
facility to be able to judge if this patch and approach has merit and
ought to be pursued further, or if it would be better to drop in favor
of (some version of) your patch. I don't care strongly, and am fine
with dropping this patch if it's approach is suboptimal.

>>>> +                               __gitcomp_nl "$(__git_refs)"
>>>> +                       fi
>>>> +                       ;;
>>>> +               prune,--*)
>>>> +                       __gitcomp "--dry-run --verbose --expire"
>>>> +                       ;;
>>>> +               esac
>>>> +       fi
>>>> +}
>>>> +