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