From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.ciao.gmane.io!not-for-mail From: Tassilo Horn Newsgroups: gmane.emacs.devel Subject: Re: RFC: Automatic setup for bug-reference-mode Date: Sun, 14 Jun 2020 17:18:47 +0200 Message-ID: <87h7vd3cbs.fsf@gnu.org> References: <87r1uihtsu.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="ciao.gmane.io:159.69.161.202"; logging-data="41177"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sun Jun 14 17:19:44 2020 Return-path: Envelope-to: ged-emacs-devel@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 1jkUQC-000AZl-6A for ged-emacs-devel@m.gmane-mx.org; Sun, 14 Jun 2020 17:19:44 +0200 Original-Received: from localhost ([::1]:60508 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jkUQB-0007wi-8j for ged-emacs-devel@m.gmane-mx.org; Sun, 14 Jun 2020 11:19:43 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:51618) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jkUPL-0007Um-UD for emacs-devel@gnu.org; Sun, 14 Jun 2020 11:18:53 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:46873) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jkUPL-0008JT-JY; Sun, 14 Jun 2020 11:18:51 -0400 Original-Received: from auth1-smtp.messagingengine.com ([66.111.4.227]:51063) by fencepost.gnu.org with esmtpsa (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.82) (envelope-from ) id 1jkUPK-0007bw-MO; Sun, 14 Jun 2020 11:18:51 -0400 Original-Received: from compute7.internal (compute7.nyi.internal [10.202.2.47]) by mailauth.nyi.internal (Postfix) with ESMTP id 4B3ED27C0054; Sun, 14 Jun 2020 11:18:50 -0400 (EDT) Original-Received: from mailfrontend1 ([10.202.2.162]) by compute7.internal (MEProxy); Sun, 14 Jun 2020 11:18:50 -0400 X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduhedrudeiiedgledtucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufhfffgjkfgfgggtsehttdertddtredtnecuhfhrohhmpefvrghsshhi lhhoucfjohhrnhcuoehtshguhhesghhnuhdrohhrgheqnecuggftrfgrthhtvghrnhepke elgeefteefhfevueejieffieekvdetheeffeevhefggfdvvefgieetudeigefhnecuffho mhgrihhnpehgihhtlhgrsgdrtghomhdpghhnuhdrohhrghenucfkphepleefrddvfeeird dufeeirdefudenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhr ohhmpehthhhorhhnodhmvghsmhhtphgruhhthhhpvghrshhonhgrlhhithihqdekieejfe ekjeekgedqieefhedvleekqdhtshguhheppehgnhhurdhorhhgsehfrghsthhmrghilhdr fhhm X-ME-Proxy: Original-Received: from thinkpad-t440p (p5dec881f.dip0.t-ipconnect.de [93.236.136.31]) by mail.messagingengine.com (Postfix) with ESMTPA id 243F0328005D; Sun, 14 Jun 2020 11:18:48 -0400 (EDT) Mail-Followup-To: Stefan Monnier , emacs-devel@gnu.org In-Reply-To: (Stefan Monnier's message of "Sun, 14 Jun 2020 10:22:08 -0400") X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:252234 Archived-At: Stefan Monnier writes: Hi Stefan, >> +(defcustom bug-reference-setup-functions nil >> + "A list of function for setting up bug-reference mode. >> +A setup function should return non-nil if it set >> +`bug-reference-bug-regexp' and `bug-reference-url-format' >> +appropiately for the current buffer. The functions are called in >> +sequence stopping as soon as one signalled a successful setup. >> +They are only called if the two variables aren't set already, >> +e.g., by a local variables section. >> + >> +Also see `bug-reference-default-setup-functions'. >> + >> +The `bug-reference-setup-functions' take preference over >> +`bug-reference-default-setup-functions', i.e., they are >> +called before the latter." >> + :type '(list function) >> + :version "28.1" >> + :group 'bug-reference) > > The :group is redundant ;-) > > More importantly, I'm wondering what was your motivation for > introducing two hooks (`bug-reference-setup-functions` and > `bug-reference-default-setup-functions`). Maybe that should be > explained in a comment? The docstring of bug-reference-default-setup-functions says: Like `bug-reference-setup-functions' for packages to hook in. So the defcustom is for users and the default setup functions for packages to add their own functions. But given that no such setup function will be trivial, we can probably drop the defcustom... >> + (let* ((backend (vc-responsible-backend (buffer-file-name) t)) >> + (url (pcase backend >> + ('Git (string-trim >> + (shell-command-to-string >> + "git ls-remote --get-url")))))) > > This should be moved to a new VC function. > Along the way I expect some related problems will be fixed such as: > - Unneeded forking of a shell only to immediately fork git. > - Hardcoding "git" when we have `vc-git-command`. Sounds right. I'll do that first and then come back to bug-reference.el later... >> + (cl-flet ((maybe-set (url-rx bug-rx bug-url-fmt) >> + (when (string-match url-rx url) > > This is mis-indented. It's not your fault, but I recommend you > override the auto-indentation :-( That would be a hassle for aggressive-indent-mode users (like me). >> + ;; GitLab projects. Here #18 is an issue and !17 is a >> + ;; merge request. Explicit namespace/project#18 references >> + ;; to possibly different projects are also supported. >> + (maybe-set >> + "[/@]gitlab.com[/:]\\([.A-Za-z0-9_/-]+\\)\\.git" >> + "\\(?1:[.A-Za-z0-9_/-]+\\)?\\(?3:#\\|!\\)\\(?2:[0-9]+\\)" >> + (lambda () >> + (let ((ns-project (match-string 1 url))) >> + (lambda () >> + (concat "https://gitlab.com/" >> + (or (match-string 1) >> + ns-project) >> + "/-/" >> + (if (string= (match-string 3) "#") >> + "issues/" >> + "merge_requests/") >> + (match-string 2)))))))))))) > > Do we really need those functions returning functions? Wouldn't it > work just as well if we do just `(setq bug-reference-url-format > bug-url-fmt)` and drop the outer `(lambda ()`? I don't think so. The outer lambda is evaluated by maybe-set and extracts the default namespace/project part from the repository URL. That will be used as a fallback in the inner lambda which is evaluated when following a bug reference #17 whereas it'll be ignored when the namespace/project is already included in the bug reference, e.g., namespace/project#17. > More importantly, I think it would be even much nicer to make this > into a list of > > (URL-RX BUG-RX BUG-URL-FORMAT) > > So users can easily add their own entries for other > repository-repositories. I agree that would be nice. >> +(defun bug-reference-try-setup-from-gnus () >> + (when (and (memq major-mode '(gnus-summary-mode gnus-article-mode)) >> + (boundp 'gnus-newsgroup-name) >> + gnus-newsgroup-name) >> + (let ((debbugs-regexp >> + ;; TODO: Obviously there are more, so add them. >> + (regexp-opt '("emacs" "auctex" "reftex" >> + "-devel@gnu.org" "ding@gnus.org")))) >> + (when (or (string-match-p debbugs-regexp gnus-newsgroup-name) >> + (and >> + gnus-article-buffer >> + (with-current-buffer gnus-article-buffer >> + (let ((headers (mail-header-extract))) >> + (when headers >> + (or (string-match-p >> + debbugs-regexp >> + (or (mail-header 'from headers) "")) >> + (string-match-p >> + debbugs-regexp >> + (or (mail-header 'to headers) "")) >> + (string-match-p >> + debbugs-regexp >> + (or (mail-header 'cc headers) "")))))))) >> + (setq bug-reference-bug-regexp >> + "\\([Bb]ug ?#?\\)\\([0-9]+\\(?:#[0-9]+\\)?\\)") >> + (setq bug-reference-url-format >> + "https://debbugs.gnu.org/%s"))))) > > Same here: using a list to make it easy for users to add their own > mailing lists. Right. >> +;;;###autoload >> +(defvar bug-reference-default-setup-functions >> + (list #'bug-reference-try-setup-from-vc >> + #'bug-reference-try-setup-from-gnus) >> + "Like `bug-reference-setup-functions' for packages to hook in.") > > Why autoloaded? So that packages can add their function to that list. >> @@ -146,7 +281,7 @@ bug-reference-mode >> "" >> nil >> (if bug-reference-mode >> - (jit-lock-register #'bug-reference-fontify) >> + (bug-reference--init) >> (jit-lock-unregister #'bug-reference-fontify) >> (save-restriction >> (widen) > > FWIW, I'd rather keep the `jit-lock-register` call next to its > matching `jit-lock-unregister`. So if we move it to > `bug-reference--init`, we should probably move the > `jit-lock-unregister` to a matching `bug-reference--uninit`. Ok with me. >> modified lisp/vc/vc.el >> @@ -957,7 +957,7 @@ vc-backend-for-registration >> (throw 'found bk)))) >> >> ;;;###autoload >> -(defun vc-responsible-backend (file) >> +(defun vc-responsible-backend (file &optional no-error) >> "Return the name of a backend system that is responsible for FILE. >> >> If FILE is already registered, return the >> @@ -967,7 +967,10 @@ vc-responsible-backend >> >> Note that if FILE is a symbolic link, it will not be resolved -- >> the responsible backend system for the symbolic link itself will >> -be reported." >> +be reported. >> + >> +If NO-ERROR is nil, signal an error that no VC backend is >> +responsible for the given file." >> (or (and (not (file-directory-p file)) (vc-backend file)) >> (catch 'found >> ;; First try: find a responsible backend. If this is for registration, >> --8<---------------cut here---------------end--------------->8--- > > Looks like a spurious hunk got into your patch ;-) Yes, indeed. :-) Bye, Tassilo