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

[Basil L. Contovounesios]

Tassilo Horn <tsdh@gnu.org> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>>> +           (url (pcase backend
>>> +                  ('Git (string-trim
>>
>> This needs (eval-when-compile (require 'subr-x)).
>
> Or simply remove the "p" from "pcase", I guess.

I was referring to your use of string-trim.

>>> +                         (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?

Perhaps; I'm not that familiar with VC.

>>> +      (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?

Yes, for the negligible potential performance gain, so that there's one
less group number taken, and more importantly so the reader isn't left
wondering why we're capturing this.

Personally, I often just go with what rx does, either by writing rx
forms directly or by manually copying its expansion into the program, as
that way I'm less likely to make a mistake.

>>>  ;;;###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.

FWIW I'm happy with the optional argument you suggest.

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

No, but I wish. ;)

Thanks,

-- 
Basil