quotearg and trigraphs [was: quotearg improvements]

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Eric Blake on 2/13/2008 8:57 PM:
| Because the quotearg module _already_ did trigraph quoting (try ls
| --quoting-style=c for an example).  The question is whether the new
| c_maybe style (or if we come up with a better name for it), designed for
| use in unambiguous error message output, should continue using that
| trigraph code or ditch it.  I think the consensus is to ditch it by
| default, although it might still be worth leaving the option in the code
| to provide it (quotearg, as a module, is useful for more than just error
| messages).

Independently of whether c_maybe_quoting_style makes sense as the
recommended style for error messages, what do people think about this
patch?  I'll install in a couple of days unless I get a review sooner.

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFHs8h884KuGfSFAYARAhWRAKDPSwR83ubRJP4Dl+rELXtpQPz1sQCeKO0H
yDwuBN9J8Jcg+vNbkO+1wQQ=
=j+h2
-----END PGP SIGNATURE-----

From aa0f572b80248a7e8fcbefba840de03b069f9701 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Wed, 13 Feb 2008 21:46:03 -0700
Subject: [PATCH] Avoid trigraph quoting in default output.

* lib/quotearg.h (enum quoting_flags): Add QA_SPLIT_TRIGRAPHS.
* lib/quotearg.c (quotearg_buffer_restyled): Don't quote trigraphs
unless explicitly requested.
* tests/test-quotearg.c (flag_results, main): Add additional tests.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog             |    8 +++++++
 lib/quotearg.c        |    3 +-
 lib/quotearg.h        |   29 ++++++++++++++----------
 tests/test-quotearg.c |   56 +++++++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 77 insertions(+), 19 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index a7ef8fb..e2f0b0d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2008-02-13  Eric Blake  <address@hidden>
+
+       Avoid trigraph quoting in default output.
+       * lib/quotearg.h (enum quoting_flags): Add QA_SPLIT_TRIGRAPHS.
+       * lib/quotearg.c (quotearg_buffer_restyled): Don't quote trigraphs
+       unless explicitly requested.
+       * tests/test-quotearg.c (flag_results, main): Add additional tests.
+
 2008-02-13  Lasse Collin  <address@hidden>
 
        Don't rely on signed integer overflowing to negative value.
diff --git a/lib/quotearg.c b/lib/quotearg.c
index 7d0c91d..c599acd 100644
--- a/lib/quotearg.c
+++ b/lib/quotearg.c
@@ -337,7 +337,8 @@ quotearg_buffer_restyled (char *buffer, size_t buffersize,
              break;
 
            case c_quoting_style:
-             if (i + 2 < argsize && arg[i + 1] == '?')
+             if ((flags & QA_SPLIT_TRIGRAPHS)
+                 && i + 2 < argsize && arg[i + 1] == '?')
                switch (arg[i + 2])
                  {
                  case '!': case '\'':
diff --git a/lib/quotearg.h b/lib/quotearg.h
index 9f81771..078d9b6 100644
--- a/lib/quotearg.h
+++ b/lib/quotearg.h
@@ -75,14 +75,15 @@ enum quoting_style
 
     /* Quote names as for a C language string (ls --quoting-style=c).
        Behaves like c_maybe_quoting_style if QA_ELIDE_OUTER_QUOTES is
-       in effect.
+       in effect.  Split into consecutive strings if
+       QA_SPLIT_TRIGRAPHS.
 
        quotearg_buffer:
-       "\"simple\"", "\"\\0 \\t\\n'\\\"\\033?\"\"?/\\\\\"", "\"a:b\""
+       "\"simple\"", "\"\\0 \\t\\n'\\\"\\033??/\\\\\"", "\"a:b\""
        quotearg:
-       "\"simple\"", "\"\\0 \\t\\n'\\\"\\033?\"\"?/\\\\\"", "\"a:b\""
+       "\"simple\"", "\"\\0 \\t\\n'\\\"\\033??/\\\\\"", "\"a:b\""
        quotearg_colon:
-       "\"simple\"", "\"\\0 \\t\\n'\\\"\\033?\"\"?/\\\\\"", "\"a\\:b\""
+       "\"simple\"", "\"\\0 \\t\\n'\\\"\\033??/\\\\\"", "\"a\\:b\""
     */
     c_quoting_style,
 
@@ -90,17 +91,16 @@ enum quoting_style
        characters if no quoted characters are encountered.
 
        quotearg_buffer:
-       "simple", "\"\\0 \\t\\n'\\\"\\033?\"\"?/\\\\\"", "a:b"
+       "simple", "\"\\0 \\t\\n'\\\"\\033??/\\\\\"", "a:b"
        quotearg:
-       "simple", "\"\\0 \\t\\n'\\\"\\033?\"\"?/\\\\\"", "a:b"
+       "simple", "\"\\0 \\t\\n'\\\"\\033??/\\\\\"", "a:b"
        quotearg_colon:
-       "simple", "\"\\0 \\t\\n'\\\"\\033?\"\"?/\\\\\"", "\"a:b\""
+       "simple", "\"\\0 \\t\\n'\\\"\\033??/\\\\\"", "\"a:b\""
     */
     c_maybe_quoting_style,
 
     /* Like c_quoting_style except always omit the surrounding
-       double-quote characters and don't worry about trigraphs (ls
-       --quoting-style=escape).
+       double-quote characters (ls --quoting-style=escape).
 
        quotearg_buffer:
        "simple", "\\0 \\t\\n'\"\\033??/\\\\", "a:b"
@@ -136,8 +136,7 @@ enum quoting_style
     locale_quoting_style,
 
     /* Like c_quoting_style except use quotation marks appropriate for
-       the locale and don't worry about trigraphs (ls
-       --quoting-style=clocale).
+       the locale (ls --quoting-style=clocale).
 
        LC_MESSAGES=C
        quotearg_buffer:
@@ -171,7 +170,13 @@ enum quoting_flags
 
     /* Omit the surrounding quote characters if no escaped characters
        are encountered.  */
-    QA_ELIDE_OUTER_QUOTES = 0x02
+    QA_ELIDE_OUTER_QUOTES = 0x02,
+
+    /* In the c_quoting_style and c_maybe_quoting_style, split ANSI
+       trigraph sequences into concatenated strings (for example,
+       "?""?/" rather than "??/", which could be confused with
+       "\\").  */
+    QA_SPLIT_TRIGRAPHS = 0x04
   };
 
 /* For now, --quoting-style=literal is the default, but this may change.  */
diff --git a/tests/test-quotearg.c b/tests/test-quotearg.c
index c6db70e..51f5ce7 100644
--- a/tests/test-quotearg.c
+++ b/tests/test-quotearg.c
@@ -90,18 +90,18 @@ static struct result_groups results_g[] = {
 
   /* c_quoting_style */
   { { "\"\"", "\"\\0001\\0\"", 9, "\"simple\"",
-      "\" \\t\\n'\\\"\\033?\"\"?/\\\\\"", "\"a:b\"" },
+      "\" \\t\\n'\\\"\\033?""?/\\\\\"", "\"a:b\"" },
     { "\"\"", "\"\\0001\\0\"", 9, "\"simple\"",
-      "\" \\t\\n'\\\"\\033?\"\"?/\\\\\"", "\"a:b\"" },
+      "\" \\t\\n'\\\"\\033?""?/\\\\\"", "\"a:b\"" },
     { "\"\"", "\"\\0001\\0\"", 9, "\"simple\"",
-      "\" \\t\\n'\\\"\\033?\"\"?/\\\\\"", "\"a\\:b\"" } },
+      "\" \\t\\n'\\\"\\033?""?/\\\\\"", "\"a\\:b\"" } },
 
   /* c_maybe_quoting_style */
-  { { "", "\"\\0001\\0\"", 9, "simple", "\" \\t\\n'\\\"\\033?\"\"?/\\\\\"",
+  { { "", "\"\\0001\\0\"", 9, "simple", "\" \\t\\n'\\\"\\033?""?/\\\\\"",
       "a:b" },
-    { "", "\"\\0001\\0\"", 9, "simple", "\" \\t\\n'\\\"\\033?\"\"?/\\\\\"",
+    { "", "\"\\0001\\0\"", 9, "simple", "\" \\t\\n'\\\"\\033?""?/\\\\\"",
       "a:b" },
-    { "", "\"\\0001\\0\"", 9, "simple", "\" \\t\\n'\\\"\\033?\"\"?/\\\\\"",
+    { "", "\"\\0001\\0\"", 9, "simple", "\" \\t\\n'\\\"\\033?""?/\\\\\"",
       "\"a:b\"" } },
 
   /* escape_quoting_style */
@@ -126,6 +126,29 @@ static struct result_groups results_g[] = {
       "\" \\t\\n'\\\"\\033?""?/\\\\\"", "\"a\\:b\"" } }
 };
 
+static struct result_groups flag_results[] = {
+  /* literal_quoting_style and QA_ELIDE_NULL_BYTES */
+  { { "", "1", 1, "simple", " \t\n'\"\033?""?/\\", "a:b" },
+    { "", "1", 1, "simple", " \t\n'\"\033?""?/\\", "a:b" },
+    { "", "1", 1, "simple", " \t\n'\"\033?""?/\\", "a:b" } },
+
+  /* c_quoting_style and QA_ELIDE_OUTER_QUOTES */
+  { { "", "\"\\0001\\0\"", 9, "simple", "\" \\t\\n'\\\"\\033?""?/\\\\\"",
+      "a:b" },
+    { "", "\"\\0001\\0\"", 9, "simple", "\" \\t\\n'\\\"\\033?""?/\\\\\"",
+      "a:b" },
+    { "", "\"\\0001\\0\"", 9, "simple", "\" \\t\\n'\\\"\\033?""?/\\\\\"",
+      "\"a:b\"" } },
+
+  /* c_quoting_style and QA_SPLIT_TRIGRAPHS */
+  { { "\"\"", "\"\\0001\\0\"", 9, "\"simple\"",
+      "\" \\t\\n'\\\"\\033?\"\"?/\\\\\"", "\"a:b\"" },
+    { "\"\"", "\"\\0001\\0\"", 9, "\"simple\"",
+      "\" \\t\\n'\\\"\\033?\"\"?/\\\\\"", "\"a:b\"" },
+    { "\"\"", "\"\\0001\\0\"", 9, "\"simple\"",
+      "\" \\t\\n'\\\"\\033?\"\"?/\\\\\"", "\"a\\:b\"" } }
+};
+
 #if ENABLE_NLS
 static struct result_groups locale_results[] = {
   /* locale_quoting_style */
@@ -261,6 +284,27 @@ main ()
       compare_strings (use_quotearg_colon, &results_g[i].group3);
     }
 
+  set_quoting_style (NULL, literal_quoting_style);
+  ASSERT (set_quoting_flags (NULL, QA_ELIDE_NULL_BYTES) == 0);
+  compare_strings (use_quotearg_buffer, &flag_results[0].group1);
+  compare_strings (use_quotearg, &flag_results[0].group2);
+  compare_strings (use_quotearg_colon, &flag_results[0].group3);
+
+  set_quoting_style (NULL, c_quoting_style);
+  ASSERT (set_quoting_flags (NULL, QA_ELIDE_OUTER_QUOTES)
+         == QA_ELIDE_NULL_BYTES);
+  compare_strings (use_quotearg_buffer, &flag_results[1].group1);
+  compare_strings (use_quotearg, &flag_results[1].group2);
+  compare_strings (use_quotearg_colon, &flag_results[1].group3);
+
+  ASSERT (set_quoting_flags (NULL, QA_SPLIT_TRIGRAPHS)
+         == QA_ELIDE_OUTER_QUOTES);
+  compare_strings (use_quotearg_buffer, &flag_results[2].group1);
+  compare_strings (use_quotearg, &flag_results[2].group2);
+  compare_strings (use_quotearg_colon, &flag_results[2].group3);
+
+  ASSERT (set_quoting_flags (NULL, 0) == QA_SPLIT_TRIGRAPHS);
+
 #if ENABLE_NLS
   /* Rather than change locales, and require a .gmo file with
      translations for "`" and "'" that match our expectations, we
-- 
1.5.4