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

[Stefan Monnier]

Augusto Stoffel [2022-09-08 23:53:02] wrote:
> 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.

I'm all for using `--help`.

I think any problems should be pushed upstream: the commands should be
self-describing so if `--help` can't be used reliably, they should
provide something else that can.

>>> +;;; Commentary:
>>> +
>> I don't fully agree with you here.
> I thought this was pretty uncontroversial, but I don't feel too strongly
> about it.

[ OK, you got me at my own game: I can't tell if you're pushing the
  joke further or if you're serious :-)  ]

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

Fair, enough, tho being outside of a repo should be rather unusual, no
(unless Emacs's dir tracker has gone hooftracks, admittedly).
In any case, you might want to add a brief comment about why you decided
to use `ignore-errors`.

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

Oh, right it's not the absolute position, now I get it, thanks.
Looks good, then.

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

No, I'm just not sure we should be completing file names before a "--"
was given.  Maybe it's OK, tho.

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

Indeed, I missed that.

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

Fair enough.  Especially since being more precise risks missing some
files which can legitimately appear.


        Stefan