From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.ciao.gmane.io!not-for-mail From: Stefan Monnier Newsgroups: gmane.emacs.devel Subject: Re: RFC: Automatic setup for bug-reference-mode Date: Sun, 14 Jun 2020 10:22:08 -0400 Message-ID: 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="70487"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) To: emacs-devel@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sun Jun 14 16:23:02 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 1jkTXJ-000I9u-On for ged-emacs-devel@m.gmane-mx.org; Sun, 14 Jun 2020 16:23:01 +0200 Original-Received: from localhost ([::1]:45506 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jkTXH-0001OC-H3 for ged-emacs-devel@m.gmane-mx.org; Sun, 14 Jun 2020 10:22:59 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:47762) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jkTWb-0000xe-Qw for emacs-devel@gnu.org; Sun, 14 Jun 2020 10:22:17 -0400 Original-Received: from mailscanner.iro.umontreal.ca ([132.204.25.50]:28912) by eggs.gnu.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jkTWY-0003rd-Dj for emacs-devel@gnu.org; Sun, 14 Jun 2020 10:22:17 -0400 Original-Received: from pmg1.iro.umontreal.ca (localhost.localdomain [127.0.0.1]) by pmg1.iro.umontreal.ca (Proxmox) with ESMTP id 5AF061002FC; Sun, 14 Jun 2020 10:22:12 -0400 (EDT) Original-Received: from mail01.iro.umontreal.ca (unknown [172.31.2.1]) by pmg1.iro.umontreal.ca (Proxmox) with ESMTP id 0AC3F1002D8; Sun, 14 Jun 2020 10:22:10 -0400 (EDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=iro.umontreal.ca; s=mail; t=1592144530; bh=4Gylds2Y99TKFGxGMppsItaH3kQ6Gd7fDn0FwsscY1I=; h=From:To:Subject:References:Date:In-Reply-To:From; b=CV2uQ7ovOT+Y7bgdf7P4SHNJQueuWC8pexOmvpxtUJ5Auiqe0oNXu6ej9alfQNpBA AtgueAwhm3ZvRaKxvyKmPwAho79gdi+jS26non8LTpZWoQR2GiGUJqgYcK+wA/lO4S PmDHc/EWOnPjjqXsxH7yV48AcG41jJOsM72u/ROyFh9gF+jTvd7owUWRGMccl7S0NZ jho2yem8nOxVsfwYXVO3Y/FITGWcmGyPNYSJZloXpgC5tliTPw3dKyk4eKDc8sEEbW 5mZAR20LqxS53bRknblEZZr4wxgGgXq/CARAvWXUiCz5kCIXN6stJEWWQq2RC/sSQn gBpM6RyU0Wuvw== Original-Received: from alfajor (unknown [216.154.55.41]) by mail01.iro.umontreal.ca (Postfix) with ESMTPSA id 78AA21202E2; Sun, 14 Jun 2020 10:22:09 -0400 (EDT) In-Reply-To: <87r1uihtsu.fsf@gnu.org> (Tassilo Horn's message of "Sun, 14 Jun 2020 11:37:37 +0200") Received-SPF: pass client-ip=132.204.25.50; envelope-from=monnier@iro.umontreal.ca; helo=mailscanner.iro.umontreal.ca X-detected-operating-system: by eggs.gnu.org: First seen = 2020/06/14 09:49:08 X-ACL-Warn: Detected OS = Linux 2.2.x-3.x [generic] X-Spam_score_int: -42 X-Spam_score: -4.3 X-Spam_bar: ---- X-Spam_report: (-4.3 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, RCVD_IN_DNSWL_MED=-2.3, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=_AUTOLEARN X-Spam_action: no action 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:252231 Archived-At: > +(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? > + (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`. > + (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 :-( > + (setq bug-reference-bug-regexp bug-rx) > + (setq bug-reference-url-format > + (if (functionp bug-url-fmt) > + (funcall bug-url-fmt) > + bug-url-fmt))))) > + (when (and url > + ;; If there's a space in the url, it's propably an > + ;; error message. > + (not (string-match-p "[[:space:]]" url))) > + (or > + ;; GNU projects on savannah. FIXME: Only a fraction of > + ;; them uses debbugs. > + (maybe-set "git\\.\\(sv\\|savannah\\)\\.gnu\\.org:" > + "\\([Bb]ug ?#?\\)\\([0-9]+\\(?:#[0-9]+\\)?\\)" > + "https://debbugs.gnu.org/%s") > + ;; GitHub projects. Here #17 may refer to either an issue > + ;; or a pull request but visiting the issue/17 web page > + ;; will automatically redirect to the pull/17 page if 17 is > + ;; a PR. Explicit user/project#17 links to possibly > + ;; different projects are also supported. > + (maybe-set > + "[/@]github.com[/:]\\([.A-Za-z0-9_/-]+\\)\\.git" > + "\\([.A-Za-z0-9_/-]+\\)?\\(?:#\\)\\([0-9]+\\)" > + (lambda () > + (let ((ns-project (match-string 1 url))) > + (lambda () > + (concat "https://github.com/" > + (or > + ;; Explicit user/proj#18 link. > + (match-string 1) > + ns-project) > + "/issues/" > + (match-string 2)))))) > + ;; 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 ()`? 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. > +(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. > +;;;###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? > @@ -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`. > 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 ;-) Stefan