From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Augusto Stoffel Newsgroups: gmane.emacs.bugs Subject: bug#57673: [PATCH] Parse --help messages for pcomplete Date: Thu, 08 Sep 2022 23:53:02 +0200 Message-ID: <87sfl1leip.fsf@gmail.com> References: <87zgfagqfs.fsf@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="29276"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/29.0.50 (gnu/linux) Cc: 57673@debbugs.gnu.org To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Sep 08 23:54:13 2022 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oWPTQ-0007On-Dd for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 08 Sep 2022 23:54:12 +0200 Original-Received: from localhost ([::1]:35122 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1oWPTP-0000zm-BF for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 08 Sep 2022 17:54:11 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:49554) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1oWPTH-0000zU-4a for bug-gnu-emacs@gnu.org; Thu, 08 Sep 2022 17:54:03 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:43474) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1oWPTG-0005dX-C2 for bug-gnu-emacs@gnu.org; Thu, 08 Sep 2022 17:54:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1oWPTG-0007nb-7t for bug-gnu-emacs@gnu.org; Thu, 08 Sep 2022 17:54:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Augusto Stoffel Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 08 Sep 2022 21:54:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 57673 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 57673-submit@debbugs.gnu.org id=B57673.166267399429915 (code B ref 57673); Thu, 08 Sep 2022 21:54:02 +0000 Original-Received: (at 57673) by debbugs.gnu.org; 8 Sep 2022 21:53:14 +0000 Original-Received: from localhost ([127.0.0.1]:60404 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oWPST-0007mP-Gb for submit@debbugs.gnu.org; Thu, 08 Sep 2022 17:53:13 -0400 Original-Received: from mail-ej1-f42.google.com ([209.85.218.42]:47066) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1oWPSR-0007mC-1n for 57673@debbugs.gnu.org; Thu, 08 Sep 2022 17:53:12 -0400 Original-Received: by mail-ej1-f42.google.com with SMTP id bj12so41123292ejb.13 for <57673@debbugs.gnu.org>; Thu, 08 Sep 2022 14:53:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:mime-version:user-agent:message-id:date :references:in-reply-to:subject:cc:to:from:from:to:cc:subject:date; bh=3JDLeWOZCWvx4nafNZ5wRwnoGado9EzlG8B3slI00Us=; b=frevH4vXTCi6Zw+NdfOl6YbMszAXsp+snfQRT1XaS8i+l5iiQcFhAiN9ICpEoYhl98 9/BBxLxFkaYqElDrT+xDK+vNnQUyeiGaRBcNdsJAcH0nvXdzuO7TdhydBr1jAkW7BWti +J3p6QwXsRuM61iVhwp5z4bHClthVQBon1jxaloHXuyLEXIA6JDnHq7m+4IjnyFXuuLW 2RErC0gc1Peb3kVyNlfjSKVSKS0itv2bJoXIoB2/lbQo6iNtANLASiiY9cP2ksRAEwqc GxlBauu110o7F63jjym68Vhv3d7o6vXjjhuuUh6dRkj1AaD9pcURCipZrXfQ403p8+/t fwUg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:user-agent:message-id:date :references:in-reply-to:subject:cc:to:from:x-gm-message-state:from :to:cc:subject:date; bh=3JDLeWOZCWvx4nafNZ5wRwnoGado9EzlG8B3slI00Us=; b=HYNuK97ORAZUGecdRuo4gMP6xNTdR2+jtFt4ULYZirY6oYmI2ToApR95sj+lHZ1KKL W/HYqbTlcrLTRV4tQ5RJHNO/GDF1teectw2eyCiENWVEj1pjjPzWprABnigUurv1p1Yw IzqJF9tcW6IpfT/MApW8jPZuqaeGj2byiks0WzTTL4qL13BjWZVhEJyp89aLVdZ0K5Fo Dlbmd4XbOdijxlS2f1AXeEfIt2L28VmDE535VMj1CHEtJhGmPheYFJq6IRm44CETObzN cEiqo287T7r7z3bASbOU8xic9aP0SL0ZXAf//dVhYIfy7wotJh/cAVUCVUtZ2w1esKbD RF0Q== X-Gm-Message-State: ACgBeo0RfWZ3E3tRjUHoeDmxu+IQqt5PZcbCHHxjNqIfcLUGCbbYxbaC mrEGUQCkzuB9/G+UeAcDIODVMsunQUs= X-Google-Smtp-Source: AA6agR4UGtNOSwzTdpKzVmnTVD4kA7GNZIPYLJWd7lpisT2W56M6tmXdosge9CnxKKqCeE37+ohyKg== X-Received: by 2002:a17:907:868b:b0:741:8f8c:6ab5 with SMTP id qa11-20020a170907868b00b007418f8c6ab5mr7434717ejc.194.1662673984352; Thu, 08 Sep 2022 14:53:04 -0700 (PDT) Original-Received: from ars3 ([2a02:8109:8ac0:56d0::157b]) by smtp.gmail.com with ESMTPSA id g21-20020a170906199500b0073d9a0d0cbcsm38270ejd.72.2022.09.08.14.53.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 08 Sep 2022 14:53:03 -0700 (PDT) In-Reply-To: (Stefan Monnier's message of "Thu, 08 Sep 2022 16:49:03 -0400") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:241945 Archived-At: 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 wro= te: >> +;;; 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 =E2=80=9Cthe previous arg=E2=80=9D. In Englis= h, 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-]+=3D?")))) > > `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-predic= ate)))) > > 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 =E2=80=9Cworse is better=E2=80=9D 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.