Re: [PATCH] better diagnostics for seq

[Jim Meyering]

Steven Schubiger <address@hidden> wrote:
> Attached is a revised patch that should take "appropriately" care of
> your suggestions. I ran make check and all tests passed. Furthermore,
> I checked coreutils.texi, but there seems to be no relevant documentation
> for seq with regard to diagnostics (as expected). FYI, make distcheck
> ungracefully exits with "fuzzy patch".
>
> Steven Schubiger
>
> diff --git a/ChangeLog-2008 b/ChangeLog-2008
> index aac9feb..df88058 100644
> --- a/ChangeLog-2008
> +++ b/ChangeLog-2008
> @@ -1,3 +1,13 @@
> +2008-02-18  Steven Schubiger  <address@hidden>
> +
> +     seq: give better diagnostics for invalid formats.
> +     * src/seq.c: (validate_format): New function.
> +     (main): Use it.
> +     * tests/misc/seq: Test for expected diagnostics with
> +     invalid formats.
> +     * NEWS: Mention this change.
> +     * TODO: Remove this item.

Thanks.

BTW, I still do require ChangeLog entries, but not in any file.
Now, they end up in the git commit log, and eventually in a generated
ChangeLog that is created in the release tarball at "make dist" time.

The "fuzzy patch" error means the c99-to-c89.diff file needs
to be updated.  Currently, that has to be resolved manually
by running "make patch-check REGEN_PATCH=yes" and then copying
new-diff to src/c99-to-c89.diff.  I've just done that.

I made a few changes to your patch, and added a new change set
to update src/c99-to-c89.diff, below.  I'll push this tomorrow.

-----------------------------------
>From f0e9eb150c4f97211de4ebd609091e2cef88898e Mon Sep 17 00:00:00 2001
From: Steven Schubiger <address@hidden>
Date: Mon, 18 Feb 2008 22:39:22 +0100
Subject: [PATCH] seq: give better diagnostics for invalid formats.

* src/seq.c: (validate_format): New function.
(main): Use it.
* tests/misc/seq (fmt-d, fmt-e): Test for expected diagnostics with
invalid formats.
* NEWS: Mention this change.
* TODO: Remove this item.
[jm: src/seq.c: make diagnostics more consistent
 tests/misc/seq (fmt-eos1): adjust the expected diagnostic ]

Signed-off-by: Jim Meyering <address@hidden>
---
 NEWS           |    2 ++
 TODO           |    2 --
 src/seq.c      |   28 ++++++++++++++++++++++++++++
 tests/misc/seq |    6 +++++-
 4 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/NEWS b/NEWS
index bc1b47d..d5d6737 100644
--- a/NEWS
+++ b/NEWS
@@ -20,6 +20,8 @@ GNU coreutils NEWS                                    -*- 
outline -*-

   ls --color no longer outputs unnecessary escape sequences

+  seq gives better diagnostics for invalid formats.
+
 ** Consistency

   mkdir and split now write --verbose output to stdout, not stderr.
diff --git a/TODO b/TODO
index 8c6b6fc..3f4d26c 100644
--- a/TODO
+++ b/TODO
@@ -61,8 +61,6 @@ printf: consider adapting builtins/printf.def from bash

 df: add `--total' option, suggested here http://bugs.debian.org/186007

-seq: give better diagnostics for invalid formats:
-   e.g. no or too many % directives
 seq: consider allowing format string to contain no %-directives

 tail: don't use xlseek; it *exits*.
diff --git a/src/seq.c b/src/seq.c
index b073fd1..c7e8cb9 100644
--- a/src/seq.c
+++ b/src/seq.c
@@ -177,6 +177,33 @@ scan_arg (const char *arg)
   return ret;
 }

+/* Validate the format, FMT.  Print a diagnostic and exit
+   if there is not exactly one %-directive.  */
+
+static void
+validate_format (char const *fmt)
+{
+  unsigned int n_directives = 0;
+  char const *p;
+
+  for (p = fmt; *p; p++)
+    {
+      if (p[0] == '%' && p[1] != '%' && p[1] != '\0')
+       {
+         ++n_directives;
+         ++p;
+       }
+    }
+  if (! n_directives)
+    {
+      error (0, 0, _("no %% directive in format string %s"), quote (fmt));
+      usage (EXIT_FAILURE);
+    }
+  else if (1 < n_directives)
+    error (EXIT_FAILURE, 0, _("too many %% directives in format string %s"),
+          quote (fmt));
+}
+
 /* If FORMAT is a valid printf format for a double argument, return
    its long double equivalent, possibly allocated from dynamic
    storage, and store into *LAYOUT a description of the output layout;
@@ -405,6 +432,7 @@ main (int argc, char **argv)

   if (format_str)
     {
+      validate_format (format_str);
       char const *f = long_double_format (format_str, &layout);
       if (! f)
        {
diff --git a/tests/misc/seq b/tests/misc/seq
index 1a153a3..f48289b 100755
--- a/tests/misc/seq
+++ b/tests/misc/seq
@@ -87,10 +87,14 @@ my @Tests =
    # "seq: memory exhausted".  In coreutils-6.0..6.8, it would mistakenly
    # succeed and print a blank line.
    ['fmt-eos1', qw(-f % 1), {EXIT => 1},
-    {ERR => "seq: invalid format string: `%'\n" . $try_help }],
+    {ERR => "seq: no % directive in format string `%'\n" . $try_help }],
    ['fmt-eos2', qw(-f %g% 1), {EXIT => 1},
     {ERR => "seq: invalid format string: `%g%'\n" . $try_help }],

+   ['fmt-d',   qw(-f "" 1), {EXIT => 1},
+    {ERR => "seq: no % directive in format string `'\n" . $try_help }],
+   ['fmt-e',   qw(-f %g%g 1), {EXIT => 1},
+    {ERR => "seq: too many % directives in format string `%g%g'\n"}],
   );

 # Append a newline to each entry in the OUT array.
--
1.5.4.2.124.gc4db3


>From 8ebdb95fb939e9710e985c504b62afa4a05c1e63 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 18 Feb 2008 23:52:26 +0100
Subject: [PATCH] * src/c99-to-c89.diff: Adjust seq.c offsets.  Accommodate a 
new C99-ism.


Signed-off-by: Jim Meyering <address@hidden>
---
 src/c99-to-c89.diff |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/src/c99-to-c89.diff b/src/c99-to-c89.diff
index 9dfa1e8..79ac4e6 100644
--- a/src/c99-to-c89.diff
+++ b/src/c99-to-c89.diff
@@ -108,7 +108,7 @@ diff -upr src/seq.c src/seq.c
      }

    return ret;
-@@ -311,6 +313,7 @@ get_default_format (operand first, opera
+@@ -338,6 +340,7 @@ get_default_format (operand first, opera
          size_t last_width = last.width + (prec - last.precision);
          if (last.precision && prec == 0)
            last_width--;  /* don't include space for '.' */
@@ -116,7 +116,7 @@ diff -upr src/seq.c src/seq.c
          size_t width = MAX (first_width, last_width);
          if (width <= INT_MAX)
            {
-@@ -318,6 +321,7 @@ get_default_format (operand first, opera
+@@ -345,6 +348,7 @@ get_default_format (operand first, opera
              sprintf (format_buf, "%%0%d.%dLf", w, prec);
              return format_buf;
            }
@@ -124,6 +124,22 @@ diff -upr src/seq.c src/seq.c
        }
        else
        {
+@@ -433,6 +437,7 @@ main (int argc, char **argv)
+   if (format_str)
+     {
+       validate_format (format_str);
++      {
+       char const *f = long_double_format (format_str, &layout);
+       if (! f)
+       {
+@@ -440,6 +445,7 @@ main (int argc, char **argv)
+         usage (EXIT_FAILURE);
+       }
+       format_str = f;
++      }
+     }
+
+   last = scan_arg (argv[optind++]);
 diff -upr src/shred.c src/shred.c
 --- src/shred.c        2007-07-23 12:56:20.000000000 +0200
 +++ src/shred.c        2007-07-23 13:03:12.000000000 +0200
@@ -136,3 +152,5 @@ diff -upr src/shred.c src/shred.c
                  if (errnum == EIO && 0 <= size && (soff | SECTOR_MASK) < lim)
                    {
                      size_t soff1 = (soff | SECTOR_MASK) + 1;
+--- src/seq.c  2008-02-18 22:53:29.000000000 +0100
++++ src-c89/seq.c      2008-02-18 23:16:35.000000000 +0100
--
1.5.4.2.124.gc4db3