GNU bug report logs - #51316
29.0.50; Should we match the final ".git" in bug-reference autosetup?

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 51316 in the body.
You can then email your comments to 51316 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: miha <at> kamnitnik.top
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.50; Should we match the final ".git" in bug-reference autosetup?
Date: Thu, 21 Oct 2021 14:18:32 +0200

[Message part 1 (text/plain, inline)]

If function 'bug-reference--build-forge-setup-entry':

> `(,(concat "[/@]" host-domain "[/:]\\([.A-Za-z0-9_/-]+\\)\\.git")
This should be "(regexp-quote host-domain)".

Also, it would be nice if the final "\\.git" wasn't mandatory.  I often
git clone a website url as displayed in a web browser
("https://gitlab.com/rstocker/emacs-bluetooth" for example) without
appending ".git".  Git has no problem fetching from such an url (tested
with github, gitlab and gitea), but bug-reference autosetup machinery
fails to detect it as a valid url.

Unfortunately, we can't simply change the final .git into
"\\(?:\\.git\\)?" because regexp greediness would then swallow it into
the first match group.  Instead, something like this could work
(concat
 "[/@]" (regexp-quote host-domain)
 "\\(?:"
 "\\(?1:[.A-Za-z0-9_/-]+\\)\\.git\\|"
 "\\(?1:[.A-Za-z0-9_/-]+\\)"
 "\\)")

Thanks and best regards.

In GNU Emacs 29.0.50 (build 5, x86_64-pc-linux-gnu, GTK+ Version
3.24.30, cairo version 1.17.4)

[signature.asc (application/pgp-signature, inline)]

Message #8 received at 51316 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: miha <at> kamnitnik.top
Cc: Tassilo Horn <tsdh <at> gnu.org>, 51316 <at> debbugs.gnu.org
Subject: Re: bug#51316: 29.0.50; Should we match the final ".git" in
 bug-reference autosetup?
Date: Fri, 22 Oct 2021 16:59:54 +0200

miha <at> kamnitnik.top writes:

> If function 'bug-reference--build-forge-setup-entry':
>
>> `(,(concat "[/@]" host-domain "[/:]\\([.A-Za-z0-9_/-]+\\)\\.git")
> This should be "(regexp-quote host-domain)".

This is now fixed in Emacs 28.

> Also, it would be nice if the final "\\.git" wasn't mandatory.  I often
> git clone a website url as displayed in a web browser
> ("https://gitlab.com/rstocker/emacs-bluetooth" for example) without
> appending ".git".  Git has no problem fetching from such an url (tested
> with github, gitlab and gitea), but bug-reference autosetup machinery
> fails to detect it as a valid url.
>
> Unfortunately, we can't simply change the final .git into
> "\\(?:\\.git\\)?" because regexp greediness would then swallow it into
> the first match group.

This would work, though:

"[/:]\\([.A-Za-z0-9_/-]+?\\)\\(?:\\.git\\)?\\'"

But requires that the string doesn't have anything after the .git,
whereas it's currently more sloppy.  I'm not sure whether that's by
intent or not.  (So I'm adding Tassilo to the CCs.)

This is a feature request, in any case, so it should go to Emacs 29, I
think.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #15 received at 51316 <at> debbugs.gnu.org (full text, mbox):

From: Tassilo Horn <tsdh <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: miha <at> kamnitnik.top, 51316 <at> debbugs.gnu.org
Subject: Re: bug#51316: 29.0.50; Should we match the final ".git" in
 bug-reference autosetup?
Date: Fri, 22 Oct 2021 22:45:23 +0200

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

>> If function 'bug-reference--build-forge-setup-entry':
>>
>>> `(,(concat "[/@]" host-domain "[/:]\\([.A-Za-z0-9_/-]+\\)\\.git")
>> This should be "(regexp-quote host-domain)".
>
> This is now fixed in Emacs 28.

Thanks.

>> Also, it would be nice if the final "\\.git" wasn't mandatory.  I
>> often git clone a website url as displayed in a web browser
>> ("https://gitlab.com/rstocker/emacs-bluetooth" for example) without
>> appending ".git".  Git has no problem fetching from such an url
>> (tested with github, gitlab and gitea), but bug-reference autosetup
>> machinery fails to detect it as a valid url.

Oh, right, that seems to work just fine.  I only checked the URLs you
get with the "copy to clipboard" buttons the forges provide.

>> Unfortunately, we can't simply change the final .git into
>> "\\(?:\\.git\\)?" because regexp greediness would then swallow it
>> into the first match group.
>
> This would work, though:
>
> "[/:]\\([.A-Za-z0-9_/-]+?\\)\\(?:\\.git\\)?\\'"

Indeed.

> But requires that the string doesn't have anything after the .git,
> whereas it's currently more sloppy.  I'm not sure whether that's by
> intent or not.  (So I'm adding Tassilo to the CCs.)

No, in my experience there cannot be anything after ".git".  At least
it's the last part of the filename and I doubt you can have query
parameters like https://forge.org/user/project.git?foo=bar in a git url.

> This is a feature request, in any case, so it should go to Emacs 29, I
> think.

I would kindly ask to reconsider.  This complete bug-reference
auto-setup thingy is new in emacs 28, the forge setup code is even just
a month old, and using your improved regexp doesn't seem risky at all
and might provide a much better user experience to possibly a lot of
users, my vote would be to fix this in emacs-28.

Bye,
Tassilo

Message #18 received at 51316 <at> debbugs.gnu.org (full text, mbox):

From: <miha <at> kamnitnik.top>
To: Tassilo Horn <tsdh <at> gnu.org>, Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 51316 <at> debbugs.gnu.org
Subject: Re: bug#51316: 29.0.50; Should we match the final ".git" in
 bug-reference autosetup?
Date: Fri, 22 Oct 2021 23:42:44 +0200

[Message part 1 (text/plain, inline)]

Tassilo Horn <tsdh <at> gnu.org> writes:

> Lars Ingebrigtsen <larsi <at> gnus.org> writes:
>
>> But requires that the string doesn't have anything after the .git,
>> whereas it's currently more sloppy.  I'm not sure whether that's by
>> intent or not.  (So I'm adding Tassilo to the CCs.)
>
> No, in my experience there cannot be anything after ".git".  At least
> it's the last part of the filename and I doubt you can have query
> parameters like https://forge.org/user/project.git?foo=bar in a git url.
>
I tried "git clone https://gitlab.com/rstocker/emacs-bluetooth.git/" and
it worked, so we should probably allow ".git/" with a final slash as
well.

And then I also tried "git clone
https://gitlab.com/rstocker/emacs-bluetooth.git//" and it also worked,
so I guess any number of slashes are allowed after the final ".git".
Therefore I propose something like this:

"[/:]\\([.A-Za-z0-9_/-]+?\\)\\(?:\\.git/*\\)?\\'"

Thanks, best regards.

[signature.asc (application/pgp-signature, inline)]

Message #21 received at 51316 <at> debbugs.gnu.org (full text, mbox):

From: Tassilo Horn <thorn+gnu <at> fastmail.fm>
To: miha <at> kamnitnik.top
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, 51316 <at> debbugs.gnu.org
Subject: Re: bug#51316: 29.0.50; Should we match the final ".git" in
 bug-reference autosetup?
Date: Sat, 23 Oct 2021 11:12:38 +0200

<miha <at> kamnitnik.top> writes:

>>> But requires that the string doesn't have anything after the .git,
>>> whereas it's currently more sloppy.  I'm not sure whether that's by
>>> intent or not.  (So I'm adding Tassilo to the CCs.)
>>
>> No, in my experience there cannot be anything after ".git".  At least
>> it's the last part of the filename and I doubt you can have query
>> parameters like https://forge.org/user/project.git?foo=bar in a git
>> url.
>>
> I tried "git clone https://gitlab.com/rstocker/emacs-bluetooth.git/"
> and it worked, so we should probably allow ".git/" with a final slash
> as well.

Ah, good catch, that's obviously correct since .git it is a directory.

> And then I also tried "git clone
> https://gitlab.com/rstocker/emacs-bluetooth.git//" and it also worked,
> so I guess any number of slashes are allowed after the final ".git".

I've also tried, and it seems the maximum number of trailing slashes is
two.  Those all work:

  https://github.com/djcb/mu
  https://github.com/djcb/mu/
  https://github.com/djcb/mu//
  https://github.com/djcb/mu.git
  https://github.com/djcb/mu.git/
  https://github.com/djcb/mu.git//

but any more / gives me "Not found".  Well, and I guess even the two-/
will most probably never occur in real-life when considering where the
URL comes from.  I mean, those are usually copy-and-pasted from the
forge's special "clone me" button or from a browser's URL bar.

> Therefore I propose something like this:
>
> "[/:]\\([.A-Za-z0-9_/-]+?\\)\\(?:\\.git/*\\)?\\'"

With the reasoning above, I'd suggest using

  "[/:]\\([.A-Za-z0-9_/-]+?\\)\\(?:\\.git/?\\)?\\'"

Bye,
Tassilo

Message #24 received at 51316 <at> debbugs.gnu.org (full text, mbox):

From: Gregory Heytings <gregory <at> heytings.org>
To: Tassilo Horn <thorn+gnu <at> fastmail.fm>
Cc: Lars Ingebrigtsen <larsi <at> gnus.org>, miha <at> kamnitnik.top,
 51316 <at> debbugs.gnu.org
Subject: Re: bug#51316: 29.0.50; Should we match the final ".git" in
 bug-reference autosetup?
Date: Sat, 23 Oct 2021 12:58:55 +0000

>
> I've also tried, and it seems the maximum number of trailing slashes is 
> two.  Those all work:
>
> https://github.com/djcb/mu
> https://github.com/djcb/mu/
> https://github.com/djcb/mu//
> https://github.com/djcb/mu.git
> https://github.com/djcb/mu.git/
> https://github.com/djcb/mu.git//
>
> but any more / gives me "Not found".  Well, and I guess even the two-/ 
> will most probably never occur in real-life when considering where the 
> URL comes from.  I mean, those are usually copy-and-pasted from the 
> forge's special "clone me" button or from a browser's URL bar.
>

Allowing more trailing slashes is a convenience that some but not all Git 
hosts offer.  On Gitlab an (unlimited?) number of trailing slashes are 
allowed, on Github it's two, on Savannah it's one.  The safest solution is 
probably to allow either a trailing ".git" or a trailing ".git/".

Message #27 received at 51316 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Tassilo Horn <tsdh <at> gnu.org>
Cc: miha <at> kamnitnik.top, 51316 <at> debbugs.gnu.org
Subject: Re: bug#51316: 29.0.50; Should we match the final ".git" in
 bug-reference autosetup?
Date: Sun, 24 Oct 2021 14:17:15 +0200

Tassilo Horn <tsdh <at> gnu.org> writes:

>> This is a feature request, in any case, so it should go to Emacs 29, I
>> think.
>
> I would kindly ask to reconsider.  This complete bug-reference
> auto-setup thingy is new in emacs 28, the forge setup code is even just
> a month old, and using your improved regexp doesn't seem risky at all
> and might provide a much better user experience to possibly a lot of
> users, my vote would be to fix this in emacs-28.

But it's not a bug fix, and twiddling this may lead to introducing bugs.
(All new features are nice to have.)

Tassilo Horn <thorn+gnu <at> fastmail.fm> writes:

> With the reasoning above, I'd suggest using
>
>   "[/:]\\([.A-Za-z0-9_/-]+?\\)\\(?:\\.git/?\\)?\\'"

Doesn't match "https://github.com/emacs-mirror/emacs/", which is what this
bug report was originally about, sort of.  So I've now added a test for
this and altered the regexp to pass the test (on the trunk).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #32 received at 51316 <at> debbugs.gnu.org (full text, mbox):

From: <miha <at> kamnitnik.top>
To: Lars Ingebrigtsen <larsi <at> gnus.org>, Tassilo Horn <tsdh <at> gnu.org>
Cc: 51316 <at> debbugs.gnu.org
Subject: Re: bug#51316: 29.0.50; Should we match the final ".git" in
 bug-reference autosetup?
Date: Tue, 26 Oct 2021 11:01:03 +0200

[Message part 1 (text/plain, inline)]

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Tassilo Horn <tsdh <at> gnu.org> writes:
>
>> With the reasoning above, I'd suggest using
>>
>>   "[/:]\\([.A-Za-z0-9_/-]+?\\)\\(?:\\.git/?\\)?\\'"
>
> Doesn't match "https://github.com/emacs-mirror/emacs/", which is what this
> bug report was originally about, sort of.  So I've now added a test for
> this and altered the regexp to pass the test (on the trunk).

Thanks. It works now for github URLs, but it should probably be used for
gitlab and gitea URLs as well, patch attached.

[0001-Allow-matching-non-.git-gitlab-and-gitea-URLs-in-bug.patch (text/x-patch, inline)]

From d75c466580a5a1d6242d77d1820f6d7aa0ec4895 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miha=20Rihtar=C5=A1i=C4=8D?= <miha <at> kamnitnik.top>
Date: Tue, 26 Oct 2021 10:54:54 +0200
Subject: [PATCH] Allow matching non-.git gitlab and gitea URLs in
 bug-reference

* lisp/progmodes/bug-reference.el
(bug-reference--build-forge-setup-entry): Allow matching non-.git
gitlab and gitea URLs, with and without slashes (bug#51316).
---
 lisp/progmodes/bug-reference.el            |  4 +-
 test/lisp/progmodes/bug-reference-tests.el | 74 ++++++++++++++++++++--
 2 files changed, 72 insertions(+), 6 deletions(-)

diff --git a/lisp/progmodes/bug-reference.el b/lisp/progmodes/bug-reference.el
index 993d670917..d7092a37d4 100644
--- a/lisp/progmodes/bug-reference.el
+++ b/lisp/progmodes/bug-reference.el
@@ -287,7 +287,7 @@ bug-reference--build-forge-setup-entry
 (cl-defmethod bug-reference--build-forge-setup-entry
   (host-domain (_forge-type (eql 'gitlab)) protocol)
   `(,(concat "[/@]" (regexp-quote host-domain)
-             "[/:]\\([.A-Za-z0-9_/-]+\\)\\.git")
+             "[/:]\\([.A-Za-z0-9_/-]+?\\)\\(?:\\.git\\)?/?\\'")
     "\\(\\([.A-Za-z0-9_/-]+\\)?\\([#!]\\)\\([0-9]+\\)\\)\\>"
     ,(lambda (groups)
        (let ((ns-project (nth 1 groups)))
@@ -304,7 +304,7 @@ bug-reference--build-forge-setup-entry
 (cl-defmethod bug-reference--build-forge-setup-entry
   (host-domain (_forge-type (eql 'gitea)) protocol)
   `(,(concat "[/@]" (regexp-quote host-domain)
-             "[/:]\\([.A-Za-z0-9_/-]+\\)\\.git")
+             "[/:]\\([.A-Za-z0-9_/-]+?\\)\\(?:\\.git\\)?/?\\'")
     "\\(\\([.A-Za-z0-9_/-]+\\)?\\(?:#\\)\\([0-9]+\\)\\)\\>"
     ,(lambda (groups)
        (let ((ns-project (nth 1 groups)))
diff --git a/test/lisp/progmodes/bug-reference-tests.el b/test/lisp/progmodes/bug-reference-tests.el
index 7a355509a1..7a3ab5fbda 100644
--- a/test/lisp/progmodes/bug-reference-tests.el
+++ b/test/lisp/progmodes/bug-reference-tests.el
@@ -26,12 +26,26 @@
 (require 'bug-reference)
 (require 'ert)
 
-(defun test--get-github-entry (protocol)
+(defun test--get-github-entry (url)
   (and (string-match
 	(car (bug-reference--build-forge-setup-entry
-              "github.com" 'github protocol))
-        protocol)
-       (match-string 1 protocol)))
+              "github.com" 'github "https"))
+        url)
+       (match-string 1 url)))
+
+(defun test--get-gitlab-entry (url)
+  (and (string-match
+	(car (bug-reference--build-forge-setup-entry
+              "gitlab.com" 'gitlab "https"))
+        url)
+       (match-string 1 url)))
+
+(defun test--get-gitea-entry (url)
+  (and (string-match
+	(car (bug-reference--build-forge-setup-entry
+              "gitea.com" 'gitea "https"))
+        url)
+       (match-string 1 url)))
 
 (ert-deftest test-github-entry ()
   (should
@@ -59,4 +73,56 @@ test-github-entry
     (test--get-github-entry "https://github.com/magit/magit/")
     "magit/magit")))
 
+(ert-deftest test-gitlab-entry ()
+  (should
+   (equal
+    (test--get-gitlab-entry "git <at> gitlab.com:larsmagne/csid.git")
+    "larsmagne/csid"))
+  (should
+   (equal
+    (test--get-gitlab-entry "git <at> gitlab.com:larsmagne/csid")
+    "larsmagne/csid"))
+  (should
+   (equal
+    (test--get-gitlab-entry "https://gitlab.com/magit/magit.git")
+    "magit/magit"))
+  (should
+   (equal
+    (test--get-gitlab-entry "https://gitlab.com/magit/magit.git/")
+    "magit/magit"))
+  (should
+   (equal
+    (test--get-gitlab-entry "https://gitlab.com/magit/magit")
+    "magit/magit"))
+  (should
+   (equal
+    (test--get-gitlab-entry "https://gitlab.com/magit/magit/")
+    "magit/magit")))
+
+(ert-deftest test-gitea-entry ()
+  (should
+   (equal
+    (test--get-gitea-entry "git <at> gitea.com:larsmagne/csid.git")
+    "larsmagne/csid"))
+  (should
+   (equal
+    (test--get-gitea-entry "git <at> gitea.com:larsmagne/csid")
+    "larsmagne/csid"))
+  (should
+   (equal
+    (test--get-gitea-entry "https://gitea.com/magit/magit.git")
+    "magit/magit"))
+  (should
+   (equal
+    (test--get-gitea-entry "https://gitea.com/magit/magit.git/")
+    "magit/magit"))
+  (should
+   (equal
+    (test--get-gitea-entry "https://gitea.com/magit/magit")
+    "magit/magit"))
+  (should
+   (equal
+    (test--get-gitea-entry "https://gitea.com/magit/magit/")
+    "magit/magit")))
+
 ;;; bug-reference-tests.el ends here
-- 
2.33.0

[signature.asc (application/pgp-signature, inline)]

Message #35 received at 51316 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: <miha <at> kamnitnik.top>
Cc: 51316 <at> debbugs.gnu.org, Tassilo Horn <tsdh <at> gnu.org>
Subject: Re: bug#51316: 29.0.50; Should we match the final ".git" in
 bug-reference autosetup?
Date: Wed, 27 Oct 2021 15:02:44 +0200

<miha <at> kamnitnik.top> writes:

> Thanks. It works now for github URLs, but it should probably be used for
> gitlab and gitea URLs as well, patch attached.

Thanks; applied to Emacs 29.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.