* bug#51316: 29.0.50; Should we match the final ".git" in bug-reference autosetup?
@ 2021-10-21 12:18 miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-10-22 14:59 ` Lars Ingebrigtsen
0 siblings, 1 reply; 9+ messages in thread
From: miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-10-21 12:18 UTC (permalink / raw)
To: 51316
[-- Attachment #1: Type: text/plain, Size: 984 bytes --]
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)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#51316: 29.0.50; Should we match the final ".git" in bug-reference autosetup?
2021-10-21 12:18 bug#51316: 29.0.50; Should we match the final ".git" in bug-reference autosetup? miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-10-22 14:59 ` Lars Ingebrigtsen
2021-10-22 20:45 ` Tassilo Horn
0 siblings, 1 reply; 9+ messages in thread
From: Lars Ingebrigtsen @ 2021-10-22 14:59 UTC (permalink / raw)
To: miha; +Cc: 51316, Tassilo Horn
miha@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#51316: 29.0.50; Should we match the final ".git" in bug-reference autosetup?
2021-10-22 14:59 ` Lars Ingebrigtsen
@ 2021-10-22 20:45 ` Tassilo Horn
2021-10-22 21:42 ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-10-24 12:17 ` Lars Ingebrigtsen
0 siblings, 2 replies; 9+ messages in thread
From: Tassilo Horn @ 2021-10-22 20:45 UTC (permalink / raw)
To: Lars Ingebrigtsen; +Cc: miha, 51316
Lars Ingebrigtsen <larsi@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#51316: 29.0.50; Should we match the final ".git" in bug-reference autosetup?
2021-10-22 20:45 ` Tassilo Horn
@ 2021-10-22 21:42 ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-10-23 9:12 ` Tassilo Horn
2021-10-24 12:17 ` Lars Ingebrigtsen
1 sibling, 1 reply; 9+ messages in thread
From: miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-10-22 21:42 UTC (permalink / raw)
To: Tassilo Horn, Lars Ingebrigtsen; +Cc: 51316
[-- Attachment #1: Type: text/plain, Size: 936 bytes --]
Tassilo Horn <tsdh@gnu.org> writes:
> Lars Ingebrigtsen <larsi@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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#51316: 29.0.50; Should we match the final ".git" in bug-reference autosetup?
2021-10-22 21:42 ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-10-23 9:12 ` Tassilo Horn
2021-10-23 12:58 ` Gregory Heytings
0 siblings, 1 reply; 9+ messages in thread
From: Tassilo Horn @ 2021-10-23 9:12 UTC (permalink / raw)
To: miha; +Cc: Lars Ingebrigtsen, 51316
<miha@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#51316: 29.0.50; Should we match the final ".git" in bug-reference autosetup?
2021-10-23 9:12 ` Tassilo Horn
@ 2021-10-23 12:58 ` Gregory Heytings
0 siblings, 0 replies; 9+ messages in thread
From: Gregory Heytings @ 2021-10-23 12:58 UTC (permalink / raw)
To: Tassilo Horn; +Cc: Lars Ingebrigtsen, miha, 51316
>
> 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/".
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#51316: 29.0.50; Should we match the final ".git" in bug-reference autosetup?
2021-10-22 20:45 ` Tassilo Horn
2021-10-22 21:42 ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-10-24 12:17 ` Lars Ingebrigtsen
2021-10-26 9:01 ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
1 sibling, 1 reply; 9+ messages in thread
From: Lars Ingebrigtsen @ 2021-10-24 12:17 UTC (permalink / raw)
To: Tassilo Horn; +Cc: miha, 51316
Tassilo Horn <tsdh@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@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
^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#51316: 29.0.50; Should we match the final ".git" in bug-reference autosetup?
2021-10-24 12:17 ` Lars Ingebrigtsen
@ 2021-10-26 9:01 ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-10-27 13:02 ` Lars Ingebrigtsen
0 siblings, 1 reply; 9+ messages in thread
From: miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2021-10-26 9:01 UTC (permalink / raw)
To: Lars Ingebrigtsen, Tassilo Horn; +Cc: 51316
[-- Attachment #1.1: Type: text/plain, Size: 526 bytes --]
Lars Ingebrigtsen <larsi@gnus.org> writes:
> Tassilo Horn <tsdh@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.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Allow-matching-non-.git-gitlab-and-gitea-URLs-in-bug.patch --]
[-- Type: text/x-patch, Size: 4463 bytes --]
From d75c466580a5a1d6242d77d1820f6d7aa0ec4895 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miha=20Rihtar=C5=A1i=C4=8D?= <miha@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@gitlab.com:larsmagne/csid.git")
+ "larsmagne/csid"))
+ (should
+ (equal
+ (test--get-gitlab-entry "git@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@gitea.com:larsmagne/csid.git")
+ "larsmagne/csid"))
+ (should
+ (equal
+ (test--get-gitea-entry "git@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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* bug#51316: 29.0.50; Should we match the final ".git" in bug-reference autosetup?
2021-10-26 9:01 ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2021-10-27 13:02 ` Lars Ingebrigtsen
0 siblings, 0 replies; 9+ messages in thread
From: Lars Ingebrigtsen @ 2021-10-27 13:02 UTC (permalink / raw)
To: miha; +Cc: Tassilo Horn, 51316
<miha@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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-10-27 13:02 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-21 12:18 bug#51316: 29.0.50; Should we match the final ".git" in bug-reference autosetup? miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-10-22 14:59 ` Lars Ingebrigtsen
2021-10-22 20:45 ` Tassilo Horn
2021-10-22 21:42 ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-10-23 9:12 ` Tassilo Horn
2021-10-23 12:58 ` Gregory Heytings
2021-10-24 12:17 ` Lars Ingebrigtsen
2021-10-26 9:01 ` miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors
2021-10-27 13:02 ` Lars Ingebrigtsen
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).