Re: RFC: Automatic setup for bug-reference-mode

[Tassilo Horn <tsdh@gnu.org>]

Stefan Monnier <monnier@iro.umontreal.ca> 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