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

[Stefan Monnier]

> +;;; Commentary:
> +

I don't fully agree with you here.

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

I'd capitalize "git".

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

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

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

> +;;;###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)?

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

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

> +               ;; 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.
- I often appreciate the completion of `--amend`.
- It probably makes sense to only complete files that have been modified.

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


        Stefan