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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 21, 2015, 21:59 UTC
Message-ID: <CAPig+cQ9Rema_n1xBJQNSv9i75sGT=RWu1AheWJFTBMa89DyFA@mail.gmail.com>
In-Reply-To: <20150821224918.Horde.edB9u314lsP17FLUzwFsQA1@webmail.informatik.kit.edu>

On Fri, Aug 21, 2015 at 4:49 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:

Show 15 quoted lines

> 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.

Show 7 quoted lines

>>>> +_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)...

Show 9 quoted lines

>>>> +       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).

Show 18 quoted lines

>>>> +                       ;;
>>>> +               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.

Show 10 quoted lines

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