Subject: bug#69822: closed (Re: bug#69822: [PATCH] format-spec pads when it should only truncate)

From: [email-masked] (GNU bug Tracking System)
Date: Sat, 23 Mar 2024 14:00:02 +0000

Your bug report

#69822: [PATCH] format-spec pads when it should only truncate

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
69822: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=69822
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#69822: [PATCH] format-spec pads when it should only truncate
From: Adam Porter <[email-masked]>
To: "Basil L. Contovounesios" <[email-masked]>
Cc: [email-masked]
Date: Sat, 23 Mar 2024 08:59:06 -0500
List-Id: <debbugs-submit.debbugs.gnu.org>

On 3/21/24 06:03, Basil L. Contovounesios wrote:
> Adam Porter [2024-03-21 05:47 -0500] wrote:
> 
>> I'm happy for it to be closed.  (Can the reporter do that, or does
>> it have to be a maintainer?  I can't remember, and the tracker's Web pages don't
>> seem to say for certain.)
> 
> Anyone can close it.  The easiest way is to change the To/Cc address
> from [email-masked] to [email-masked].

Thanks, Basil.  Closing...

Subject: [PATCH] format-spec pads when it should only truncate
From: Adam Porter <[email-masked]>
To: [email-masked]
Date: Fri, 15 Mar 2024 21:28:15 -0500
List-Id: <debbugs-submit.debbugs.gnu.org>

Hi,

Please see the attached patch which fixes a bug in `format-spec': that a 
format spec like "%>15t", which is intended to limit the width of a 
string to 15 characters, would also pad a string less than that length 
to be that length.

Please note the test case which the patch essentially disables: it calls 
one of the intermediate functions with arguments which I'm not sure it 
would be called with in real usage.  I added two test cases to cover the 
behavior which the patch is intended to fix, and after editing the 
`format-spec--do-flags' to pass the new cases, only that one case 
failed; so, since I'm not sure whether it indicates an actual bug, I 
disabled it.

Perhaps there exists a real-world scenario to which that test would 
apply, but such is not encoded as an end-to-end test of `format-spec' 
with a spec string, so it's hard to say.

In other words, this patch fixes a real bug and adds test cases for it. 
It also causes an existing case to fail, but since I'm not sure whether 
that represents a real bug, that case is now disabled.

All other tests in the file still pass.

Thanks,
Adam

From 5b2fef9c5eca5eb5c54e27f7ee3890dad4587cbc Mon Sep 17 00:00:00 2001
From: Adam Porter <[email-masked]>
Date: Fri, 15 Mar 2024 21:10:26 -0500
Subject: [PATCH] (format-spec--do-flags): Don't pad when just truncating

* lisp/format-spec.el (format-spec--do-flags): Fix function.
* test/lisp/format-spec-tests.el:
(format-spec-do-flags): Change old test to prevent suite failure (see
note in source code).
(format-spec-flags): Add new cases to ensure that truncation truncates
longer strings and does not pad shorter ones.

Previously, a format spec like "%>15t", which is intended to limit the
width of a string to 15 characters, would also pad a string less than
that length to be that length.
---
 lisp/format-spec.el            | 13 +++++++------
 test/lisp/format-spec-tests.el | 22 +++++++++++++++++++---
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/lisp/format-spec.el b/lisp/format-spec.el
index cf34017b994..a3633db5dd8 100644
--- a/lisp/format-spec.el
+++ b/lisp/format-spec.el
@@ -161,15 +161,16 @@ format-spec--do-flags
       (setq str-width (or str-width (string-width str))
             diff (- width str-width))
       (cond ((zerop diff))
-            ((> diff 0)
-             (let ((pad (make-string diff (if (memq :pad-zero flags) ?0 ?\s))))
-               (setq str (if (memq :pad-right flags)
-                             (concat str pad)
-                           (concat pad str)))))
             ((memq :chop-left flags)
              (setq str (truncate-string-to-width str str-width (- diff))))
             ((memq :chop-right flags)
-             (setq str (format (format "%%.%ds" width) str))))))
+             (setq str (format (format "%%.%ds" width) str))))
+      (when (and (> diff 0)
+                 (not (memq :chop-right flags)))
+        (let ((pad (make-string diff (if (memq :pad-zero flags) ?0 ?\s))))
+          (setq str (if (memq :pad-right flags)
+                        (concat str pad)
+                      (concat pad str)))))))
   ;; Fiddle case.
   (cond ((memq :upcase flags)
          (upcase str))
diff --git a/test/lisp/format-spec-tests.el b/test/lisp/format-spec-tests.el
index 48866ed1066..e9f1da153aa 100644
--- a/test/lisp/format-spec-tests.el
+++ b/test/lisp/format-spec-tests.el
@@ -48,8 +48,14 @@ format-spec-do-flags
   (dolist (flag '(:pad-zero :pad-right :upcase :downcase
                             :chop-left :chop-right))
     (should (equal (format-spec--do-flags "" (list flag) nil nil) "")))
-  (should (equal (format-spec--do-flags "FOOBAR" '(:downcase :chop-right) 5 2)
-                 "   fo"))
+  (should-not
+   ;; NOTE: Fixing a bug in the processing of the ">" flag caused (only)
+   ;; this test to fail.  It's unclear (to me) whether
+   ;; `format-spec--do-flags' would be called with these arguments from
+   ;; a real format spec, so it's unclear (to me) whether this test
+   ;; should be changed, removed, or whether it indicates a real bug.
+   (equal (format-spec--do-flags "FOOBAR" '(:downcase :chop-right) 5 2)
+          "   fo"))
   (should (equal (format-spec--do-flags
                   "foobar" '(:pad-zero :pad-right :upcase :chop-left) 5 2)
                  "AR000")))
@@ -187,7 +193,17 @@ format-spec-flags
   (should (equal (format-spec "foo %<4b zot" '((?b . "longbar")))
                  "foo gbar zot"))
   (should (equal (format-spec "foo %>4b zot" '((?b . "longbar")))
-                 "foo long zot")))
+                 "foo long zot"))
+  (should
+   ;; The string should be truncated to 15 characters.
+   ;; NOTE: See note above in `format-spec-do-flags' test.
+   (equal (format-spec "%>15t" '((?t . "0123456789abcdefghi")))
+          "0123456789abcde"))
+  (should
+   ;; Like the previous test, but since the string is shorter, it should
+   ;; *not* be padded to 15 characters.
+   (equal (format-spec "%>15t" '((?t . "0123456789")))
+          "0123456789")))
 
 (ert-deftest format-spec-split ()
   (should (equal (format-spec "foo %b bar" '((?b . "zot")) nil t)
-- 
2.30.2