[bug-diffutils] bug#20062: bug#20062: bug#20062: bug#20062: [PATCH] diff: add support for --color

[Giuseppe Scrivano]

Hi Paul,

thanks for the review:

Paul Eggert <address@hidden> writes:

> Gisle Vanem wrote:
>> No. He added a 'reset_color_context (true);' inside
>> 'signal_handler()'.
>
> Ah, sorry, I was looking at the original edition of the patch.
>
> I just now looked at <http://bugs.gnu.org/20062#101> and still see
> some problems.  Mainly, it doesn't handle SIGTSTP properly.  With
> SIGTSTP an application is supposed to reset the terminal and then
> resignal itself with SIGSTOP before really stopping.  When restarting
> the application then needs to restore the terminal to the current
> state, if the current state is not the default.  This is nearly
> impossible to do correctly with the proposed design.  I suggest
> looking at how GNU ls does it: the signal handler merely sets a flag,
> and the application checks occasionally at safe places whether a
> signal has arrived.

IIRC we agreed on this design so that we react immediately to signals
without blocking (now the most we can block is to write the reset
sequence, but we can't cut this), blocking signals was done in an older
version of the patch.

I've tried to handle SIGTSTP with this addition and everything seems to
work as expected, I'll test it more before propose a new version of the
patch.

Do you see any issue with it?

Regards,
Giuseppe


diff --git a/src/util.c b/src/util.c
index 0f8f7a8..7f11318 100644
--- a/src/util.c
+++ b/src/util.c
@@ -28,6 +28,8 @@
 
 char const pr_program[] = PR_PROGRAM;
 
+static const char *last_context_str;
+
 /* Queue up one-line messages to be printed at the end,
    when -l is specified.  Each message is recorded with a 'struct msg'.  */
 
@@ -145,8 +147,35 @@ print_message_queue (void)
 }
 
 static void
+safe_write_from_signal (const char *str, size_t len)
+{
+  size_t written = 0;
+  while (written < len)
+    {
+      int ret = write (STDOUT_FILENO, str + written,
+                       len - written);
+      if (ret < 0)
+        return;
+      written += ret;
+    }
+}
+
+static void
 signal_handler (int sig)
 {
+  if (sig == SIGCONT)
+    {
+      if (last_context_str)
+        safe_write_from_signal (last_context_str, strlen (last_context_str));
+      return;
+    }
+  if (sig == SIGTSTP)
+    {
+      reset_color_context (true);
+      raise (SIGSTOP);
+      return;
+    }
+
   reset_color_context (true);
 
   /* Restore the default handler, and report the signal again.  */
@@ -163,6 +192,7 @@ install_signal_handlers (void)
 #endif
   int const sig[] =
     {
+      SIGCONT,
       SIGTSTP,
       SIGALRM, SIGHUP, SIGINT, SIGPIPE, SIGQUIT, SIGTERM,
 #ifdef SIGPOLL
@@ -796,28 +826,40 @@ void
 set_header_color_context (void)
 {
   if (colors_enabled)
-    fprintf (outfile, "\x1B[1;39m");
+    {
+      last_context_str = "\x1B[1;39m";
+      fprintf (outfile, "%s", last_context_str);
+    }
 }
 
 void
 set_line_numbers_color_context (void)
 {
   if (colors_enabled)
-    fprintf (outfile, "\x1B[36m");
+    {
+      last_context_str = "\x1B[36m";
+      fprintf (outfile, "%s", last_context_str);
+    }
 }
 
 void
 set_add_color_context (void)
 {
   if (colors_enabled)
-    fprintf (outfile, "\x1B[32m");
+    {
+      last_context_str = "\x1B[32m";
+      fprintf (outfile, "%s", last_context_str);
+    }
 }
 
 void
 set_delete_color_context (void)
 {
   if (colors_enabled)
-    fprintf (outfile, "\x1B[31m");
+    {
+      last_context_str = "\x1B[31m";
+      fprintf (outfile, "%s", last_context_str);
+    }
 }
 
 void
@@ -827,19 +869,12 @@ reset_color_context (bool from_signal)
   if (! colors_enabled)
     return;
 
-  if (! from_signal)
-    fputs (reset_sequence, outfile);
+  if (from_signal)
+    safe_write_from_signal (reset_sequence, sizeof reset_sequence - 1);
   else
     {
-      size_t written = 0;
-      while (written < sizeof reset_sequence - 1)
-        {
-          int ret = write (STDOUT_FILENO, reset_sequence + written,
-                           sizeof reset_sequence - 1 - written);
-          if (ret < 0)
-            return;
-          written += ret;
-        }
+      fputs (reset_sequence, outfile);
+      last_context_str = NULL;
     }
 }