bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Štěpán Němec]

'gnus-shorten-url' (used by 'gnus-summary-browse-url') ignored
fragment identifiers and didn't check substring bounds, in some cases
leading to runtime errors, e.g.:

  (gnus-shorten-url "https://some.url.with/path/and#also_a_long_target" 40)
  ;; => Lisp error: (args-out-of-range "/path/and" -18 nil)

This commit makes it account for #fragments and fixes faulty string
computation, reusing existing helper function.

* lisp/vc/ediff-init.el
(ediff-truncate-string-left): Rename to 'string-truncate-left'.
* lisp/emacs-lisp/subr-x.el (string-truncate-left): Move here.
* lisp/vc/ediff-mult.el (ediff-meta-insert-file-info1) (ediff-draw-dir-diffs):
'ediff-draw-dir-diffs' renamed to 'string-truncate-left'.
* lisp/gnus/gnus-sum.el (gnus-shorten-url): Fix args-out-of-range
error, don't drop #fragments, use 'string-truncate-left'.
---
 lisp/emacs-lisp/subr-x.el |  8 ++++++++
 lisp/gnus/gnus-sum.el     | 14 +++++++-------
 lisp/vc/ediff-init.el     | 10 ----------
 lisp/vc/ediff-mult.el     | 10 ++++++----
 4 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 044c9aada0..baf20131cc 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -236,6 +236,14 @@ string-trim
 TRIM-LEFT and TRIM-RIGHT default to \"[ \\t\\n\\r]+\"."
   (string-trim-left (string-trim-right string trim-right) trim-left))
 
+(defsubst string-truncate-left (string length)
+  "Truncate STRING to LENGTH, replacing initial surplus with \"...\"."
+  (let ((strlen (length string)))
+    (if (<= strlen length)
+	string
+      (setq length (max 0 (- length 3)))
+      (concat "..." (substring string (max 0 (- strlen 1 length)))))))
+
 (defsubst string-blank-p (string)
   "Check whether STRING is either empty or only whitespace.
 The following characters count as whitespace here: space, tab, newline and
diff --git a/lisp/gnus/gnus-sum.el b/lisp/gnus/gnus-sum.el
index a47e657623..6f367692dd 100644
--- a/lisp/gnus/gnus-sum.el
+++ b/lisp/gnus/gnus-sum.el
@@ -9494,15 +9494,15 @@ gnus-collect-urls
     (delete-dups urls)))
 
 (defun gnus-shorten-url (url max)
-  "Return an excerpt from URL."
+  "Return an excerpt from URL not exceeding MAX characters."
   (if (<= (length url) max)
       url
-    (let ((parsed (url-generic-parse-url url)))
-      (concat (url-host parsed)
-	      "..."
-	      (substring (url-filename parsed)
-			 (- (length (url-filename parsed))
-			    (max (- max (length (url-host parsed))) 0)))))))
+    (let* ((parsed (url-generic-parse-url url))
+           (host (url-host parsed))
+           (rest (concat (url-filename parsed)
+                         (when-let ((target (url-target parsed)))
+                           (concat "#" target)))))
+      (concat host (string-truncate-left rest (- max (length host)))))))
 
 (defun gnus-summary-browse-url (&optional external)
   "Scan the current article body for links, and offer to browse them.
diff --git a/lisp/vc/ediff-init.el b/lisp/vc/ediff-init.el
index cbd8c0d322..c7498064dc 100644
--- a/lisp/vc/ediff-init.el
+++ b/lisp/vc/ediff-init.el
@@ -1524,16 +1524,6 @@ ediff-strip-last-dir
 	(setq dir (substring dir 0 pos)))
     (ediff-abbreviate-file-name (file-name-directory dir))))
 
-(defun ediff-truncate-string-left (str newlen)
-  ;; leave space for ... on the left
-  (let ((len (length str))
-	substr)
-    (if (<= len newlen)
-	str
-      (setq newlen (max 0 (- newlen 3)))
-      (setq substr (substring str (max 0 (- len 1 newlen))))
-      (concat "..." substr))))
-
 (defsubst ediff-nonempty-string-p (string)
   (and (stringp string) (not (string= string ""))))
 
diff --git a/lisp/vc/ediff-mult.el b/lisp/vc/ediff-mult.el
index fee87e8352..5dcb42eb64 100644
--- a/lisp/vc/ediff-mult.el
+++ b/lisp/vc/ediff-mult.el
@@ -113,6 +113,8 @@ ediff-mult
 (require 'ediff-wind)
 (require 'ediff-util)
 
+(eval-when-compile
+  (require 'subr-x))                    ; string-truncate-left
 
 ;; meta-buffer
 (ediff-defvar-local ediff-meta-buffer nil "")
@@ -1172,7 +1174,7 @@ ediff-meta-insert-file-info1
 	  ;; abbreviate the file name, if file exists
 	  (if (and (not (stringp fname)) (< file-size -1))
 	      "-------"		; file doesn't exist
-	    (ediff-truncate-string-left
+	    (string-truncate-left
 	     (ediff-abbreviate-file-name fname)
 	     max-filename-width)))))))
 
@@ -1266,7 +1268,7 @@ ediff-draw-dir-diffs
 	(if (= (mod membership-code ediff-membership-code1) 0) ; dir1
 	    (let ((beg (point)))
 	      (insert (format "%-27s"
-			      (ediff-truncate-string-left
+			      (string-truncate-left
 			       (ediff-abbreviate-file-name
 				(if (file-directory-p (concat dir1 file))
 				    (file-name-as-directory file)
@@ -1281,7 +1283,7 @@ ediff-draw-dir-diffs
 	(if (= (mod membership-code ediff-membership-code2) 0) ; dir2
 	    (let ((beg (point)))
 	      (insert (format "%-26s"
-			      (ediff-truncate-string-left
+			      (string-truncate-left
 			       (ediff-abbreviate-file-name
 				(if (file-directory-p (concat dir2 file))
 				    (file-name-as-directory file)
@@ -1295,7 +1297,7 @@ ediff-draw-dir-diffs
 	    (if (= (mod membership-code ediff-membership-code3) 0) ; dir3
 		(let ((beg (point)))
 		  (insert (format " %-25s"
-				  (ediff-truncate-string-left
+				  (string-truncate-left
 				   (ediff-abbreviate-file-name
 				    (if (file-directory-p (concat dir3 file))
 					(file-name-as-directory file)
-- 
2.25.1

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Lars Ingebrigtsen]

Štěpán Němec <stepnem@gmail.com> writes:

> 'gnus-shorten-url' (used by 'gnus-summary-browse-url') ignored
> fragment identifiers and didn't check substring bounds, in some cases
> leading to runtime errors, e.g.:
>
>   (gnus-shorten-url "https://some.url.with/path/and#also_a_long_target" 40)
>   ;; => Lisp error: (args-out-of-range "/path/and" -18 nil)
>
> This commit makes it account for #fragments and fixes faulty string
> computation, reusing existing helper function.

Looks like a good fix to me, but is there a reason this is a defsubst
instead of a defun?

+(defsubst string-truncate-left (string length)

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

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Štěpán Němec]

On Sat, 14 Mar 2020 12:41:34 +0100
Lars Ingebrigtsen wrote:

> Looks like a good fix to me, but is there a reason this is a defsubst
> instead of a defun?
>
> +(defsubst string-truncate-left (string length)

subr-x is supposed to be loaded at compile time (see the file's
Commentary), so, with the single exception of `replace-region-contents'
(I wonder how that happened...), everything in there is either a macro
or a defsubst.

I would personally welcome lifting that limitation (again, what about
that one exception which needs to load it at run time, anyway?) as IME
it causes confusion with some users and package developers, too, but I'm
not sure that's an option.

-- 
Štěpán

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Lars Ingebrigtsen]

Štěpán Němec <stepnem@gmail.com> writes:

> subr-x is supposed to be loaded at compile time (see the file's
> Commentary), so, with the single exception of `replace-region-contents'
> (I wonder how that happened...), everything in there is either a macro
> or a defsubst.

The commentary is:

;; Less commonly used functions that complement basic APIs, often implemented in
;; C code (like hash-tables and strings), and are not eligible for inclusion
;; in subr.el.

;; Do not document these functions in the lispref.
;; https://lists.gnu.org/r/emacs-devel/2014-01/msg01006.html

;; NB If you want to use this library, it's almost always correct to use:
;; (eval-when-compile (require 'subr-x))

So it's just stuff that's not supposed to be in subr.el but is otherwise
subr-ey?  I don't know why it's become almost all substs and macros, but
it seems odd to decide to make things that look like a perfectly good
defun into a defsubst just because we're sticking it in subr-x.

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

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Štěpán Němec]

On Sat, 14 Mar 2020 19:03:24 +0100
Lars Ingebrigtsen wrote:

> I don't know why it's become almost all substs and macros, but it
> seems odd to decide to make things that look like a perfectly good
> defun into a defsubst just because we're sticking it in subr-x.

Indeed, but the same could be said about all the other string functions
already in there, so I can't think of any other reason for them being
defsubsts than avoiding runtime loading of the library, and the
commentary seems to kinda imply that, too, doesn't it?

-- 
Štěpán

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sat, 14 Mar 2020 19:03:24 +0100
> Cc: 39980@debbugs.gnu.org
> 
> I don't know why it's become almost all substs and macros, but it
> seems odd to decide to make things that look like a perfectly good
> defun into a defsubst just because we're sticking it in subr-x.

I tend to agree, FWIW.

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Štěpán Němec]

[-- Attachment #1: Type: text/plain, Size: 340 bytes --]


Considering the opinions expressed here and in
https://lists.gnu.org/archive/html/emacs-devel/2020-03/msg00541.html I
have reverted to defun and autoload the function instead to prevent
having to load subr-x at runtime whenever gnus-sum is loaded, as it is
currently only used there by `gnus-summary-browse-url'.

Revised patch attached.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnus-shorten-url-Improve-and-avoid-args-out-of-range.patch --]
[-- Type: text/x-patch, Size: 5688 bytes --]

From b6a2daee2e5be0c4aec6d13fe55b6d9df343a07a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0t=C4=9Bp=C3=A1n=20N=C4=9Bmec?= <stepnem@gmail.com>
Date: Sat, 7 Mar 2020 18:26:44 +0100
Subject: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

'gnus-shorten-url' (used by 'gnus-summary-browse-url') ignored
fragment identifiers and didn't check substring bounds, in some cases
leading to runtime errors, e.g.:

  (gnus-shorten-url "https://some.url.with/path/and#also_a_long_target" 40)
  ;; => Lisp error: (args-out-of-range "/path/and" -18 nil)

This commit makes it account for #fragments and fixes faulty string
computation, reusing existing helper function.  (bug#39980)

* lisp/vc/ediff-init.el
(ediff-truncate-string-left): Rename to 'string-truncate-left'.
* lisp/emacs-lisp/subr-x.el (string-truncate-left): Move here.
* lisp/vc/ediff-mult.el (ediff-meta-insert-file-info1) (ediff-draw-dir-diffs):
'ediff-draw-dir-diffs' renamed to 'string-truncate-left'.
* lisp/gnus/gnus-sum.el (gnus-shorten-url): Fix args-out-of-range
error, don't drop #fragments, use 'string-truncate-left'.
---
 lisp/emacs-lisp/subr-x.el |  9 +++++++++
 lisp/gnus/gnus-sum.el     | 14 +++++++-------
 lisp/vc/ediff-init.el     | 10 ----------
 lisp/vc/ediff-mult.el     |  9 ++++-----
 4 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/lisp/emacs-lisp/subr-x.el b/lisp/emacs-lisp/subr-x.el
index 044c9aada0..9f96ac50d1 100644
--- a/lisp/emacs-lisp/subr-x.el
+++ b/lisp/emacs-lisp/subr-x.el
@@ -236,6 +236,15 @@ string-trim
 TRIM-LEFT and TRIM-RIGHT default to \"[ \\t\\n\\r]+\"."
   (string-trim-left (string-trim-right string trim-right) trim-left))
 
+;;;###autoload
+(defun string-truncate-left (string length)
+  "Truncate STRING to LENGTH, replacing initial surplus with \"...\"."
+  (let ((strlen (length string)))
+    (if (<= strlen length)
+	string
+      (setq length (max 0 (- length 3)))
+      (concat "..." (substring string (max 0 (- strlen 1 length)))))))
+
 (defsubst string-blank-p (string)
   "Check whether STRING is either empty or only whitespace.
 The following characters count as whitespace here: space, tab, newline and
diff --git a/lisp/gnus/gnus-sum.el b/lisp/gnus/gnus-sum.el
index a47e657623..6f367692dd 100644
--- a/lisp/gnus/gnus-sum.el
+++ b/lisp/gnus/gnus-sum.el
@@ -9494,15 +9494,15 @@ gnus-collect-urls
     (delete-dups urls)))
 
 (defun gnus-shorten-url (url max)
-  "Return an excerpt from URL."
+  "Return an excerpt from URL not exceeding MAX characters."
   (if (<= (length url) max)
       url
-    (let ((parsed (url-generic-parse-url url)))
-      (concat (url-host parsed)
-	      "..."
-	      (substring (url-filename parsed)
-			 (- (length (url-filename parsed))
-			    (max (- max (length (url-host parsed))) 0)))))))
+    (let* ((parsed (url-generic-parse-url url))
+           (host (url-host parsed))
+           (rest (concat (url-filename parsed)
+                         (when-let ((target (url-target parsed)))
+                           (concat "#" target)))))
+      (concat host (string-truncate-left rest (- max (length host)))))))
 
 (defun gnus-summary-browse-url (&optional external)
   "Scan the current article body for links, and offer to browse them.
diff --git a/lisp/vc/ediff-init.el b/lisp/vc/ediff-init.el
index e59d4b57b5..da6509b7cb 100644
--- a/lisp/vc/ediff-init.el
+++ b/lisp/vc/ediff-init.el
@@ -1510,16 +1510,6 @@ ediff-strip-last-dir
 	(setq dir (substring dir 0 pos)))
     (ediff-abbreviate-file-name (file-name-directory dir))))
 
-(defun ediff-truncate-string-left (str newlen)
-  ;; leave space for ... on the left
-  (let ((len (length str))
-	substr)
-    (if (<= len newlen)
-	str
-      (setq newlen (max 0 (- newlen 3)))
-      (setq substr (substring str (max 0 (- len 1 newlen))))
-      (concat "..." substr))))
-
 (defsubst ediff-nonempty-string-p (string)
   (and (stringp string) (not (string= string ""))))
 
diff --git a/lisp/vc/ediff-mult.el b/lisp/vc/ediff-mult.el
index fee87e8352..2b1b07927f 100644
--- a/lisp/vc/ediff-mult.el
+++ b/lisp/vc/ediff-mult.el
@@ -113,7 +113,6 @@ ediff-mult
 (require 'ediff-wind)
 (require 'ediff-util)
 
-
 ;; meta-buffer
 (ediff-defvar-local ediff-meta-buffer nil "")
 (ediff-defvar-local ediff-parent-meta-buffer nil "")
@@ -1172,7 +1171,7 @@ ediff-meta-insert-file-info1
 	  ;; abbreviate the file name, if file exists
 	  (if (and (not (stringp fname)) (< file-size -1))
 	      "-------"		; file doesn't exist
-	    (ediff-truncate-string-left
+	    (string-truncate-left
 	     (ediff-abbreviate-file-name fname)
 	     max-filename-width)))))))
 
@@ -1266,7 +1265,7 @@ ediff-draw-dir-diffs
 	(if (= (mod membership-code ediff-membership-code1) 0) ; dir1
 	    (let ((beg (point)))
 	      (insert (format "%-27s"
-			      (ediff-truncate-string-left
+			      (string-truncate-left
 			       (ediff-abbreviate-file-name
 				(if (file-directory-p (concat dir1 file))
 				    (file-name-as-directory file)
@@ -1281,7 +1280,7 @@ ediff-draw-dir-diffs
 	(if (= (mod membership-code ediff-membership-code2) 0) ; dir2
 	    (let ((beg (point)))
 	      (insert (format "%-26s"
-			      (ediff-truncate-string-left
+			      (string-truncate-left
 			       (ediff-abbreviate-file-name
 				(if (file-directory-p (concat dir2 file))
 				    (file-name-as-directory file)
@@ -1295,7 +1294,7 @@ ediff-draw-dir-diffs
 	    (if (= (mod membership-code ediff-membership-code3) 0) ; dir3
 		(let ((beg (point)))
 		  (insert (format " %-25s"
-				  (ediff-truncate-string-left
+				  (string-truncate-left
 				   (ediff-abbreviate-file-name
 				    (if (file-directory-p (concat dir3 file))
 					(file-name-as-directory file)
-- 
2.26.0

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Lars Ingebrigtsen]

Štěpán Němec <stepnem@gmail.com> writes:

> Considering the opinions expressed here and in
> https://lists.gnu.org/archive/html/emacs-devel/2020-03/msg00541.html I
> have reverted to defun and autoload the function instead to prevent
> having to load subr-x at runtime whenever gnus-sum is loaded, as it is
> currently only used there by `gnus-summary-browse-url'.
>
> Revised patch attached.

Looks good to me.

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

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Štěpán Němec]

On Thu, 02 Apr 2020 13:01:35 +0200
Lars Ingebrigtsen wrote:

> Štěpán Němec <stepnem@gmail.com> writes:
>
>> Considering the opinions expressed here and in
>> https://lists.gnu.org/archive/html/emacs-devel/2020-03/msg00541.html I
>> have reverted to defun and autoload the function instead to prevent
>> having to load subr-x at runtime whenever gnus-sum is loaded, as it is
>> currently only used there by `gnus-summary-browse-url'.
>>
>> Revised patch attached.
>
> Looks good to me.

This can go to emacs-27, right? `gnus-summary-browse-url' is new in
Emacs 27, it would seem a shame to ship it with this bug.

-- 
Štěpán

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Eli Zaretskii]

> From: Štěpán Němec <stepnem@gmail.com>
> Cc: 39980@debbugs.gnu.org, Lars Ingebrigtsen <larsi@gnus.org>
> Date: Sun, 12 Apr 2020 11:53:47 +0200
> 
> On Thu, 02 Apr 2020 13:01:35 +0200
> Lars Ingebrigtsen wrote:
> 
> > Štěpán Němec <stepnem@gmail.com> writes:
> >
> >> Considering the opinions expressed here and in
> >> https://lists.gnu.org/archive/html/emacs-devel/2020-03/msg00541.html I
> >> have reverted to defun and autoload the function instead to prevent
> >> having to load subr-x at runtime whenever gnus-sum is loaded, as it is
> >> currently only used there by `gnus-summary-browse-url'.
> >>
> >> Revised patch attached.
> >
> > Looks good to me.
> 
> This can go to emacs-27, right? `gnus-summary-browse-url' is new in
> Emacs 27, it would seem a shame to ship it with this bug.

OK for the change in gnus-summary-browse-url, but let's please leave
out of emacs-27 changes that aren't necessarily needed to fix the
original problem; cleanups are for master.  On emacs-27, let's just
fix the original problem locally.

Thanks.

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Štěpán Němec]

On Sun, 12 Apr 2020 13:33:37 +0300
Eli Zaretskii wrote:

>> From: Štěpán Němec <stepnem@gmail.com>
>> Cc: 39980@debbugs.gnu.org, Lars Ingebrigtsen <larsi@gnus.org>
>> Date: Sun, 12 Apr 2020 11:53:47 +0200
>> 
>> On Thu, 02 Apr 2020 13:01:35 +0200
>> Lars Ingebrigtsen wrote:
>> 
>> > Štěpán Němec <stepnem@gmail.com> writes:
>> >
>> >> Considering the opinions expressed here and in
>> >> https://lists.gnu.org/archive/html/emacs-devel/2020-03/msg00541.html I
>> >> have reverted to defun and autoload the function instead to prevent
>> >> having to load subr-x at runtime whenever gnus-sum is loaded, as it is
>> >> currently only used there by `gnus-summary-browse-url'.
>> >>
>> >> Revised patch attached.
>> >
>> > Looks good to me.
>> 
>> This can go to emacs-27, right? `gnus-summary-browse-url' is new in
>> Emacs 27, it would seem a shame to ship it with this bug.
>
> OK for the change in gnus-summary-browse-url, but let's please leave
> out of emacs-27 changes that aren't necessarily needed to fix the
> original problem; cleanups are for master.

Understood.

> On emacs-27, let's just fix the original problem locally.

The revised patch is upthread
(<https://lists.gnu.org/archive/html/bug-gnu-emacs/2020-03/msg01100.html>)

There are no changes in the patch that are not necessary for fixing the
problem. The ediff changes (only adjusting callers) are necessitated by
moving the helper function to subr-x.el, so that Gnus (or anything else)
can use it, too.

OK?

-- 
Štěpán

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Eli Zaretskii]

> From: Štěpán Němec <stepnem@gmail.com>
> Cc: larsi@gnus.org,  39980@debbugs.gnu.org
> Date: Sun, 12 Apr 2020 12:49:33 +0200
> 
> > OK for the change in gnus-summary-browse-url, but let's please leave
> > out of emacs-27 changes that aren't necessarily needed to fix the
> > original problem; cleanups are for master.
> 
> Understood.
> 
> > On emacs-27, let's just fix the original problem locally.
> 
> The revised patch is upthread
> (<https://lists.gnu.org/archive/html/bug-gnu-emacs/2020-03/msg01100.html>)

That's the one I was talking about: it includes changes that aren't
needed to fix the original bug.  Those additions make the change
cleaner, but we are way past cleanup time on the release branch.

> There are no changes in the patch that are not necessary for fixing the
> problem. The ediff changes (only adjusting callers) are necessitated by
> moving the helper function to subr-x.el, so that Gnus (or anything else)
> can use it, too.

The changes in subr-x and in ediff are unnecessary.  Let's make
changes only in gnus-summary-browse-url, OK?  The full changeset can
go to master.

Btw, this part of the commit log is inaccurate:

  * lisp/vc/ediff-init.el
  (ediff-truncate-string-left): Rename to 'string-truncate-left'.

And this one names the wrong function:

  * lisp/vc/ediff-mult.el (ediff-meta-insert-file-info1) (ediff-draw-dir-diffs):
  'ediff-draw-dir-diffs' renamed to 'string-truncate-left'.

And finally, the format of the last entry is slightly off the mark.
It should look like this:

  * lisp/vc/ediff-mult.el (ediff-meta-insert-file-info1)
  (ediff-draw-dir-diffs):

That is, the second parentheses pair should be on a new line.

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Štěpán Němec]

On Sun, 12 Apr 2020 14:05:40 +0300
Eli Zaretskii wrote:

>> The revised patch is upthread
>> (<https://lists.gnu.org/archive/html/bug-gnu-emacs/2020-03/msg01100.html>)
>
> That's the one I was talking about: it includes changes that aren't
> needed to fix the original bug.  Those additions make the change
> cleaner, but we are way past cleanup time on the release branch.
>
>> There are no changes in the patch that are not necessary for fixing the
>> problem. The ediff changes (only adjusting callers) are necessitated by
>> moving the helper function to subr-x.el, so that Gnus (or anything else)
>> can use it, too.
>
> The changes in subr-x and in ediff are unnecessary.  Let's make
> changes only in gnus-summary-browse-url, OK?  The full changeset can
> go to master.

You mean instead of reusing an existing function, fix `gnus-shorten-url'
without using it? I can only see disadvantages here: having to maintain
two separate versions, reinventing the wheel (and possibly introducing
bugs that way, which is how all this started)... are you sure you
couldn't be persuaded otherwise? Surely just renaming a function and
adjusting callers should be safer for the release branch, too?

> Btw, this part of the commit log is inaccurate:
>
>   * lisp/vc/ediff-init.el
>   (ediff-truncate-string-left): Rename to 'string-truncate-left'.
>
> And this one names the wrong function:
>
>   * lisp/vc/ediff-mult.el (ediff-meta-insert-file-info1) (ediff-draw-dir-diffs):
>   'ediff-draw-dir-diffs' renamed to 'string-truncate-left'.
>
> And finally, the format of the last entry is slightly off the mark.
> It should look like this:
>
>   * lisp/vc/ediff-mult.el (ediff-meta-insert-file-info1)
>   (ediff-draw-dir-diffs):
>
> That is, the second parentheses pair should be on a new line.

Thank you, how about:

* lisp/vc/ediff-init.el (ediff-truncate-string-left): Rename to
'string-truncate-left' and move...
* lisp/emacs-lisp/subr-x.el (string-truncate-left): ...here.
* lisp/vc/ediff-mult.el (ediff-meta-insert-file-info1)
(ediff-draw-dir-diffs): Adjust callers.
* lisp/gnus/gnus-sum.el (gnus-shorten-url): Fix args-out-of-range
error, don't drop #fragments, use 'string-truncate-left'.

Or even compacting the first three items to:

* lisp/vc/ediff-init.el (ediff-truncate-string-left): Rename to
'string-truncate-left' and move...
* lisp/emacs-lisp/subr-x.el (string-truncate-left): ...here.  All callers
changed.

as suggested in https://www.gnu.org/prep/standards/standards.html#Simple-Changes

-- 
Štěpán

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Eli Zaretskii]

> From: Štěpán Němec <stepnem@gmail.com>
> Cc: larsi@gnus.org,  39980@debbugs.gnu.org
> Date: Sun, 12 Apr 2020 13:47:20 +0200
> 
> >> There are no changes in the patch that are not necessary for fixing the
> >> problem. The ediff changes (only adjusting callers) are necessitated by
> >> moving the helper function to subr-x.el, so that Gnus (or anything else)
> >> can use it, too.
> >
> > The changes in subr-x and in ediff are unnecessary.  Let's make
> > changes only in gnus-summary-browse-url, OK?  The full changeset can
> > go to master.
> 
> You mean instead of reusing an existing function, fix `gnus-shorten-url'
> without using it?

Almost: the "existing function" doesn't really exist, you introduced
it with the same changeset, right?

> I can only see disadvantages here: having to maintain
> two separate versions, reinventing the wheel (and possibly introducing
> bugs that way, which is how all this started)... are you sure you
> couldn't be persuaded otherwise? Surely just renaming a function and
> adjusting callers should be safer for the release branch, too?

It isn't safe enough for my taste, sorry.  It affects every package
that uses subr-x, and it affects Ediff.  I'd like to avoid that for
the release branch, however low risk that is.  Solving it just in
gnus-shorten-url makes the risk of breaking anything else exactly
zero on the release branch.

The change on the release branch should be marked by "don't merge to
master", and the original changeset pushed to master instead.

> * lisp/vc/ediff-init.el (ediff-truncate-string-left): Rename to
> 'string-truncate-left' and move...
> * lisp/emacs-lisp/subr-x.el (string-truncate-left): ...here.
> * lisp/vc/ediff-mult.el (ediff-meta-insert-file-info1)
> (ediff-draw-dir-diffs): Adjust callers.
> * lisp/gnus/gnus-sum.el (gnus-shorten-url): Fix args-out-of-range
> error, don't drop #fragments, use 'string-truncate-left'.
> 
> Or even compacting the first three items to:
> 
> * lisp/vc/ediff-init.el (ediff-truncate-string-left): Rename to
> 'string-truncate-left' and move...
> * lisp/emacs-lisp/subr-x.el (string-truncate-left): ...here.  All callers
> changed.

Both variants of the log message are fine.

Thanks.

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Štěpán Němec]

On Sun, 12 Apr 2020 16:38:57 +0300
Eli Zaretskii wrote:

>> You mean instead of reusing an existing function, fix `gnus-shorten-url'
>> without using it?
>
> Almost: the "existing function" doesn't really exist, you introduced
> it with the same changeset, right?

No, it's the (hopefully) tried and true `ediff-truncate-string-left',
only renamed and moved to subr-x to be generally available. The changes
are cosmetic. It would even be possible (in emacs-27, to avoid moving
anything), to use `ediff-truncate-string-left' unchanged, but that would
require gnus-sum to (require 'ediff-init), which would be crazy. Or make
`ediff-truncate-string-left' a defsubst instead of defun and
(eval-when-compile (require 'ediff-init)) in gnus-sum. That should
actually work :-P

> Both variants of the log message are fine.

Thanks.

-- 
Štěpán

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Eli Zaretskii]

> From: Štěpán Němec <stepnem@gmail.com>
> Cc: larsi@gnus.org,  39980@debbugs.gnu.org
> Date: Sun, 12 Apr 2020 16:13:23 +0200
> 
> On Sun, 12 Apr 2020 16:38:57 +0300
> Eli Zaretskii wrote:
> 
> >> You mean instead of reusing an existing function, fix `gnus-shorten-url'
> >> without using it?
> >
> > Almost: the "existing function" doesn't really exist, you introduced
> > it with the same changeset, right?
> 
> No, it's the (hopefully) tried and true `ediff-truncate-string-left',
> only renamed and moved to subr-x to be generally available.

That's exactly what I'd like to avoid on the release branch.  It's
fine for master, of course.


> The changes are cosmetic. It would even be possible (in emacs-27, to
> avoid moving anything), to use `ediff-truncate-string-left'
> unchanged, but that would require gnus-sum to (require 'ediff-init),
> which would be crazy.

It would be crazy and also somewhat risky.

Let's just come up with a local change in gnus-sum, for the emacs-27
branch, okay?

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Štěpán Němec]

[-- Attachment #1: Type: text/plain, Size: 334 bytes --]

On Sun, 12 Apr 2020 17:35:31 +0300
Eli Zaretskii wrote:

> Let's just come up with a local change in gnus-sum, for the emacs-27
> branch, okay?

OK. I think the pat(c)h of least resistence would be as follows
(everything just the same as for master, but with the helper function
localized to gnus-sum and no changes to other files):


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnus-shorten-url-Improve-and-avoid-args-out-of-range.patch --]
[-- Type: text/x-patch, Size: 2525 bytes --]

From 9fd1f20078220c17f9e954b556c6b770ca70961a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C5=A0t=C4=9Bp=C3=A1n=20N=C4=9Bmec?= <stepnem@gmail.com>
Date: Sun, 12 Apr 2020 19:57:59 +0200
Subject: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

'gnus-shorten-url' (used by 'gnus-summary-browse-url') ignored
fragment identifiers and didn't check substring bounds, in some cases
leading to runtime errors, e.g.:

  (gnus-shorten-url "https://some.url.with/path/and#also_a_long_target" 40)
  ;; => Lisp error: (args-out-of-range "/path/and" -18 nil)

This commit makes it account for #fragments and fixes faulty string
computation.  (bug#39980)

; Do not merge to master, where the helper is put to subr-x.el.

* lisp/gnus/gnus-sum.el (gnus--string-truncate-left): New helper
function (copied from 'ediff-truncate-string-left').
(gnus-shorten-url): Use it and don't drop #fragments.
---
 lisp/gnus/gnus-sum.el | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/lisp/gnus/gnus-sum.el b/lisp/gnus/gnus-sum.el
index a40e563e75..9b11d5878d 100644
--- a/lisp/gnus/gnus-sum.el
+++ b/lisp/gnus/gnus-sum.el
@@ -9493,16 +9493,26 @@ gnus-collect-urls
       (push primary urls))
     (delete-dups urls)))
 
+;; cf. `ediff-truncate-string-left', to become `string-truncate-left'
+;; in Emacs 28
+(defun gnus--string-truncate-left (string length)
+  "Truncate STRING to LENGTH, replacing initial surplus with \"...\"."
+  (let ((strlen (length string)))
+    (if (<= strlen length)
+	string
+      (setq length (max 0 (- length 3)))
+      (concat "..." (substring string (max 0 (- strlen 1 length)))))))
+
 (defun gnus-shorten-url (url max)
-  "Return an excerpt from URL."
+  "Return an excerpt from URL not exceeding MAX characters."
   (if (<= (length url) max)
       url
-    (let ((parsed (url-generic-parse-url url)))
-      (concat (url-host parsed)
-	      "..."
-	      (substring (url-filename parsed)
-			 (- (length (url-filename parsed))
-			    (max (- max (length (url-host parsed))) 0)))))))
+    (let* ((parsed (url-generic-parse-url url))
+           (host (url-host parsed))
+           (rest (concat (url-filename parsed)
+                         (when-let ((target (url-target parsed)))
+                           (concat "#" target)))))
+      (concat host (gnus--string-truncate-left rest (- max (length host)))))))
 
 (defun gnus-summary-browse-url (&optional external)
   "Scan the current article body for links, and offer to browse them.
-- 
2.26.0


[-- Attachment #3: Type: text/plain, Size: 18 bytes --]


-- 
Štěpán

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Eli Zaretskii]

> From: Štěpán Němec <stepnem@gmail.com>
> Cc: larsi@gnus.org,  39980@debbugs.gnu.org
> Date: Sun, 12 Apr 2020 22:02:18 +0200
> 
> OK. I think the pat(c)h of least resistence would be as follows
> (everything just the same as for master, but with the helper function
> localized to gnus-sum and no changes to other files):

Thanks, this is OK for emacs-27.

> ; Do not merge to master, where the helper is put to subr-x.el.

No need to precede this with a semi-colon, I think.

bug#39980: [PATCH] gnus-shorten-url: Improve and avoid args-out-of-range error

[Štěpán Němec]

On Mon, 13 Apr 2020 07:26:45 +0300
Eli Zaretskii wrote:

> Thanks, this is OK for emacs-27.

Pushed to emacs-27 as

2020-04-12T19:57:59+02:00!stepnem@gmail.com
81d07da788 (gnus-shorten-url: Improve and avoid args-out-of-range error)
https://git.sv.gnu.org/cgit/emacs.git/commit/?id=81d07da788

and master as

2020-03-07T18:26:44+01:00!stepnem@gmail.com
188bd80a90 (gnus-shorten-url: Improve and avoid args-out-of-range error)
https://git.sv.gnu.org/cgit/emacs.git/commit/?id=188bd80a90

>> ; Do not merge to master, where the helper is put to subr-x.el.
>
> No need to precede this with a semi-colon, I think.

I saw it done in some other commit messages and thought it was a good
way to distinguish this as a meta instruction, as opposed to the commit
message proper. I have removed it.

Thanks,

  Štěpán