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

[Tassilo Horn <tsdh@gnu.org>]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

Hi Basil,

I'll incorporate your suggestions later.

>> +           (url (pcase backend
>> +                  ('Git (string-trim
>
> This needs (eval-when-compile (require 'subr-x)).

Or simply remove the "p" from "pcase", I guess.

>> +                         (shell-command-to-string
>> +                          "git ls-remote --get-url"))))))
>
> Doesn't VC provide a robust way to get output from Git?

vc-git--call maybe?

>> +      (cl-flet ((maybe-set (url-rx bug-rx bug-url-fmt)
>> +                           (when (string-match url-rx url)
>> +                             (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:"
>                                 ^^^
> Nit: Can this be a shy group?

Yes.  Is is generally better to use shy groups if we aren't going to do
anything with the groups anyway?

>>  ;;;###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,
>
> NO-ERROR seems to be a no-op in this patch.

Indeed.  Obviously it should have done what the docstring says.

> Instead of changing the function's arglist, would it be any worse to
> do the following?
>
>   (ignore-errors (vc-responsible-backend ...))

IMHO, that it errors if no backend is found is the actual error but we
cannot change that anymore.  And to me "what backend would this file
use" is a very common question.  Even vc.el itself encodes that (dolist
(backend vc-handled-backends) ...) form again in
`vc-backend-for-registration' in order not to trigger the error
semantics of `vc-responsible-backend'.

But that's not important to me.  I can also leave it as it is.

Bye,
Tassilo

PS: I like your style of suggesting improvements using questions which
all have a positive answer.  Did you visit a lecture of Stefan? ;-)