bug#57673: [PATCH] Parse --help messages for pcomplete

[Augusto Stoffel <arstoffel@gmail.com>]

Hi Stefan,

thanks for the detailed comments.  I was also curious, on a more high
level, about your opinion on parsing the --help messages.  It's a bit of
a heuristic thing and it will probably lead to some false positives here
and there.

On Thu,  8 Sep 2022 at 16:49, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

>> +;;; Commentary:
>> +
>
> I don't fully agree with you here.

I thought this was pretty uncontroversial, but I don't feel too strongly
about it.

>> +(defun pcmpl-git--tracked-file-predicate ()
>> +  "Return a predicate function determining if a file is tracked by git."
>
> I'd capitalize "git".

Noted.

>> +  (when-let ((files (ignore-errors
>> +                      (process-lines vc-git-program "ls-files")))
>
> Is it normal&common for `ls-files` to return an error?  If not, then
> maybe we should use `with-demoted-errors` so that the users are made
> aware of an error if it occurs.

I think that being outside of a repo is the main error situation.  But
in this particular spot I'd rather fail silently, because the
consequence of an error is rather innocuous -- we'll lack a predicate
function and therefore show some extra files completions.

>> +(defun pcmpl-git--remote-refs (remote)
>> +  "List the locally known git revisions from REMOTE.
>> +If REMOTE is nil, return the list of remotes."
>> +  (if (null remote)
>
> AFAICT you could just as well have two separate functions here since all
> callers either always provide a nil arg or never provide a nil arg.

That's true.  I was trying to save a symbol name.

>> +      (ignore-errors
>> +        (process-lines vc-git-program "remote"))
>> +    (delq nil
>> +          (mapcar
>> +           (let ((re (concat (regexp-quote remote) "/\\(.*\\)")))
>> +             (lambda (s) (when (string-match re s) (match-string 1 s))))
>> +           (vc-git-revision-table nil)))))
>
> I think the `re` needs to be anchored to avoid false positives.

Good point.

>> +;;;###autoload
>> +(defun pcomplete/git ()
>> +  "Completion for the `git' command."
>> +  (let ((subcmds (pcomplete-parse-help "git help -a"
>> +                                       :margin "^\\( +\\)[a-z]"
>> +                                       :argument "[-a-z]+")))
>> +    (while (not (member (pcomplete-arg 1) subcmds))
>> +      (pcomplete-here (append subcmds
>> +                              (pcomplete-parse-help "git help"
>> +                                                    :margin "\\(\\[\\)-"
>> +                                                    :separator " | "
>> +                                                    :description "\\`"))))
>
> I don't quite understand this `while` loop.  IIUC this is to handle the
> case where `--foo` args are passed before the `subcmd`, but if we want
> to support that use case, don't we need to change the code so it doesn't
> hardcode the `1` in (pcomplete-arg 1)?

The thing here is that each call to `pcomplete-here' shifts the args.
So `1' in this context means “the previous arg”.  In English, the above
form means: offer global switches and subcommand names as completion
candidates until the previous arg is a subcommand.

(We could additionally allow filenames, since some switches take a file
argument.)

> Maybe we should follow a scheme similar to that used in `pcomplete-cvs`,
> tho it'd probably require a new replacement for `pcomplete-opt`

I don't understand what you mean exactly here, but note that pcomplete
allows entering, for instance

    git --no-pager -C commit --amend --long ./m4/acl.m4

without completely typing any of these words.

>> +    (let ((subcmd (pcomplete-arg 1)))
>> +      (while (pcase subcmd
>> +               ((guard (pcomplete-match "\\`-" 0))
>> +                (pcomplete-here
>> +                 (pcmpl-git--expand-flags
>> +                  (pcomplete-parse-help (format "git help %s" subcmd)
>> +                                        :argument "-+\\(?:\\[no-\\]\\)?[a-z-]+=?"))))
>
> `subcmd` may contain funny chars like `&`, so we should
> `shell-quote-argument` it (tho see further down).

True, we shouldn't use a shell.  But also note that a funny char will
not appear in an element of `subcmds', since our :argument parameter is
"[-a-z]+".

>> +               ;; Complete tracked files
>> +               ((or "mv" "rm" "restore" "grep" "status" "commit")
>> +                (pcomplete-here
>> +                 (pcomplete-entries nil (pcmpl-git--tracked-file-predicate))))
>
> Regarding `git commit`:
> - I never specify files on `git commit` without using `--` first.

Well, `--' is among the completion candidates.  Is that what you want?

> - I often appreciate the completion of `--amend`.

You can complete switches after `git commit'.  This is taken care of by
the (guard (pcomplete-match "\\`-" 0)) case of the pcase.

> - It probably makes sense to only complete files that have been modified.

So, this is brings up an important point.

I'm advocating for a “worse is better” method where we try to have a
more or less comprehensive list of completion candidates but we don't
try to be too precise about the context in which each thing can appear.

If we do try to be precise, we might end up with 3592 lines of code,
which is the length of /usr/share/bash-completion/git on my machine.

What do you think?

>> +(cl-defun pcomplete-parse-help (command
>
> AFAICT, this "command" never uses any functionality of the shell, so we
> should be using `call-process` rather than `call-process-shell-command`,
> since the use of a shell here only brings trouble (it's less efficient
> and it forces us to `shell-quote-arguments` to try and avoid
> pathological errors in corner cases).

Yes, passing the output of `format' straight to a shell is of course not
acceptable for the final version of this patch.