git-subtree completions

git-subtree completions

[Jordan Klassen]

[-- Attachment #1: Type: text/plain, Size: 416 bytes --]

Hey all, after a long time of sitting on a closed PR to zsh-completions, I
ported it over to zsh here: https://github.com/zsh-users/zsh/pull/8

I need feedback on it, and if it's better to submit a patch via `git
send-email` let me know. In particular, is there an easy way to test this
without actually compiling zsh? How do I fix my `THIS_IS_A_HACK` issue? And
code review in general.

Thanks!
-- 
~Jordan Klassen

Re: git-subtree completions

[Bart Schaefer]

On Wed, May 11, 2016 at 10:57 PM, Jordan Klassen <jordan@klassen.me.uk> wrote:
>
> I need feedback on it, and if it's better to submit a patch via `git
> send-email` let me know.

Generally speaking, yes, you should send a patch to zsh-workers.

> In particular, is there an easy way to test this
> without actually compiling zsh?

Since the diff is to a script file, yes.  You just need to change the
$fpath array to list the directory containing your edited file ahead
of the directory containing the un-edited copy, before the first time
you attempt to complete after "git".

> How do I fix my `THIS_IS_A_HACK` issue?

I'm not sure what that's meant to accomplish?  As far as I can tell
using a pattern that won't match anything should be the same as
leaving out the -A entirely.

> And code review in general.

There are others better qualified than me to critique changes to the
git completion ...

Re: git-subtree completions

[Oliver Kiddle]

Jordan Klassen wrote:
> How do I fix my `THIS_IS_A_HACK` issue?

Use -A "-*" and change this line:
    '*:: :->option-or-argument' && ret=0
to:
    '*::: := ->option-or-argument' && ret=0

> And code review in general.

Otherwise, it looks good. I'd suggest using _default instead of _nothing
for an unknown subcommand. _nothing might be more accurate at the moment
but falling back to default completion is better if a new subcommand is
added to git-subtree and the completion isn't updated. I'm aware that
the same comment could apply elsewhere in _git.

The descriptions for subcommands also could do with adjusting.
References to things like <commit> and <repository> make less sense in
the absense of the rest of the help information these have been copied
from.

Coming back to the THIS_IS_A_HACK issue, I think there is arguably a bug
in _arguments. The documentation states that with two
colons, "the words special array and the CURRENT special parameter are
modified to refer only to the normal arguments". I would expect the
command to be a "normal argument" in this context but -P coming after it
appears to affect this.
  For: git subtree add <tab> we get words=( add '' ) CURRENT=2
  For: git subtree add -P <tab> we get words=( '' ) CURRENT=1
_arguments in the option-or-argument state needs
words=( anything '' ) CURRENT=2
Arguably, the -P is covered by the *: action and should be included too.

The relevant code for this is the following two lines in computil.c:
    if ((restr = (arg->type == CAA_RARGS)))
	      restrict_range(ca_laststate.optbeg, ca_laststate.argend);

restrict_range appears to restrict the range based on a start and end
index so is not equipped to remove options interspersed with normal
arguments. So my question for -workers folks is how do we make sense of
struct castate to correct this? I think we need something like
ca_laststate.args along with compwords from ca_laststate.argbeg onwards.

Oliver

Re: git-subtree completions

[Daniel Shahaf]

I see this has been committed now; reviewing the committed patch:

> +++ b/Completion/Unix/Command/_git
> @@ -1747,6 +1747,91 @@ 
> +(( $+functions[_git-subtree] )) ||
> +_git-subtree () {
> +    (option-or-argument)
> +      curcontext=${curcontext%:*}-$line[1]:
> +      case $line[1] in
> +        (add)
> +          _arguments \
> +            '(-q --quiet)'{-q,--quiet}'[suppress progress output]' \
> +            '(-m --message)'{-m,--message}'[use the given message as the commit message for the merge commit]' \

-m takes an argument, but this spec doesn't reflect that.  I assume it should be

               '(-m --message)'{-m+,--message=}'[use the given message as the commit message for the merge commit]:message' \
	                        ^^^ ^^^^^^^^^^                                                                    ^^^^^^^^
(three changes)

> +            '(-P --prefix)'{-P,--prefix=}'[the path to the subtree in the repository to manipulate]: :_directories' \

s/-P,/-P+,/

> +            '--squash[import only a single commit from the subproject]' \
> +            ': :__git_any_repositories_or_references' \
> +            ':: :__git_ref_specs' && ret=0

__git_ref_specs doesn't seem appropriate here:
.
    % git subtree add -P foo ../other-repository/ master:target
    'master:target' does not look like a ref
.
I think what is wanted here is "names of refs in the remote repository",
same as the before-the-colon part of 'git fetch' refspecs.

(And by the way, __git_ref_specs is deprecated in master; new code
should use one of the two __git_ref_specs_* functions directly.)

Cheers,

Daniel

Re: git-subtree completions

[Daniel Shahaf]

Daniel Shahaf wrote on Thu, Jul 07, 2016 at 02:00:10 +0000:
> I see this has been committed now; reviewing the committed patch:
> 
> > +++ b/Completion/Unix/Command/_git
> > @@ -1747,6 +1747,91 @@ 
> > +            '(-m --message)'{-m,--message}'[use the given message as the commit message for the merge commit]' \
> 
> -m takes an argument, but this spec doesn't reflect that.  I assume it should be
> 
>                '(-m --message)'{-m+,--message=}'[use the given message as the commit message for the merge commit]:message' \
> 	                        ^^^ ^^^^^^^^^^                                                                    ^^^^^^^^
> (three changes)
> 
> > +            '(-P --prefix)'{-P,--prefix=}'[the path to the subtree in the repository to manipulate]: :_directories' \
> 
> s/-P,/-P+,/

I'll push a patch for these two in a minute (once I have the X-Seq of
this).

> > +            '--squash[import only a single commit from the subproject]' \
> > +            ': :__git_any_repositories_or_references' \
> > +            ':: :__git_ref_specs' && ret=0
> 
> __git_ref_specs doesn't seem appropriate here:
> .
>     % git subtree add -P foo ../other-repository/ master:target
>     'master:target' does not look like a ref

This issue is outstanding.

Cheers,

Daniel