v2 of --stderr=FILE patches

v2 of --stderr=FILE patches

[Tomi Ollila]

This is version 2 of id:1369332362-4719-1-git-send-email-tomi.ollila@iki.fi

 NEWS               |  6 ++++++
 man/man1/notmuch.1 |  7 +++++++
 notmuch-client.h   |  1 +
 notmuch.c          | 32 ++++++++++++++++++++++++++++++++
 test/help-test     |  9 +++++++++
 5 files changed, 55 insertions(+)

diffdiff of the changes since v1:

diff --git a/notmuch.c b/notmuch.c
index 77b5282..654a568 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -251,7 +251,8 @@ notmuch_command (notmuch_config_t *config,
     return 0;
 }
 
-static int redirect_stderr (const char * stderr_file)
+static int
+redirect_stderr (const char * stderr_file)
 {
     if (strcmp (stderr_file, "-") == 0) {
 	if (dup2 (STDOUT_FILENO, STDERR_FILENO) < 0) {
diff --git a/test/help-test b/test/help-test
index 8b77931..bd0111c 100755
--- a/test/help-test
+++ b/test/help-test
@@ -15,7 +15,6 @@ test_expect_equal "$(cat stderr)" "
 Sorry, % is not a known command. There's not much I can do to help."
 
 test_begin_subtest "notmuch --stderr=- help %"
-notmuch --stderr=stderr help %
 test_expect_equal "$(notmuch --stderr=- help %)" "
 Sorry, % is not a known command. There's not much I can do to help."

[PATCH v2 1/4] cli: add global option --stderr=FILE

[Tomi Ollila]

With this option all writes to stderr are redirected to the spesified
FILE (or to stdout on case FILE is '-'). This is immediately useful
in emacs interface as some of its exec intefaces do not provide
separation of stdout and stderr.
---
 notmuch-client.h |  1 +
 notmuch.c        | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/notmuch-client.h b/notmuch-client.h
index 45749a6..4a3c7ac 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -54,6 +54,7 @@ typedef GMimeCipherContext notmuch_crypto_context_t;
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <unistd.h>
+#include <fcntl.h>
 #include <dirent.h>
 #include <errno.h>
 #include <signal.h>
diff --git a/notmuch.c b/notmuch.c
index f51a84f..654a568 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -251,6 +251,32 @@ notmuch_command (notmuch_config_t *config,
     return 0;
 }
 
+static int
+redirect_stderr (const char * stderr_file)
+{
+    if (strcmp (stderr_file, "-") == 0) {
+	if (dup2 (STDOUT_FILENO, STDERR_FILENO) < 0) {
+	    perror ("dup2");
+	    return 1;
+	}
+    } else {
+	int fd = open (stderr_file, O_WRONLY|O_CREAT|O_APPEND, 0644);
+	if (fd < 0) {
+	    fprintf (stderr, "Error: Cannot redirect stderr to '%s': %s\n",
+		     stderr_file, strerror (errno));
+	    return 1;
+	}
+	if (fd != STDERR_FILENO) {
+	    if (dup2 (fd, STDERR_FILENO) < 0) {
+		perror ("dup2");
+		return 1;
+	    }
+	    close (fd);
+	}
+    }
+    return 0;
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -259,6 +285,7 @@ main (int argc, char *argv[])
     const char *command_name = NULL;
     command_t *command;
     char *config_file_name = NULL;
+    char *stderr_file = NULL;
     notmuch_config_t *config;
     notmuch_bool_t print_help=FALSE, print_version=FALSE;
     int opt_index;
@@ -268,6 +295,7 @@ main (int argc, char *argv[])
 	{ NOTMUCH_OPT_BOOLEAN, &print_help, "help", 'h', 0 },
 	{ NOTMUCH_OPT_BOOLEAN, &print_version, "version", 'v', 0 },
 	{ NOTMUCH_OPT_STRING, &config_file_name, "config", 'c', 0 },
+	{ NOTMUCH_OPT_STRING, &stderr_file, "stderr", '\0', 0 },
 	{ 0, 0, 0, 0, 0 }
     };
 
@@ -287,6 +315,10 @@ main (int argc, char *argv[])
 	return 1;
     }
 
+    if (stderr_file && redirect_stderr (stderr_file) != 0) {
+	/* error already printed */
+	return 1;
+    }
     if (print_help)
 	return notmuch_help_command (NULL, argc - 1, &argv[1]);
 
-- 
1.8.0

Re: [PATCH v2 1/4] cli: add global option --stderr=FILE

[Austin Clements]

Quoth Tomi Ollila on May 26 at 11:45 am:
> With this option all writes to stderr are redirected to the spesified
> FILE (or to stdout on case FILE is '-'). This is immediately useful
> in emacs interface as some of its exec intefaces do not provide
> separation of stdout and stderr.
> ---
>  notmuch-client.h |  1 +
>  notmuch.c        | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 45749a6..4a3c7ac 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -54,6 +54,7 @@ typedef GMimeCipherContext notmuch_crypto_context_t;
>  #include <sys/stat.h>
>  #include <sys/time.h>
>  #include <unistd.h>
> +#include <fcntl.h>
>  #include <dirent.h>
>  #include <errno.h>
>  #include <signal.h>
> diff --git a/notmuch.c b/notmuch.c
> index f51a84f..654a568 100644
> --- a/notmuch.c
> +++ b/notmuch.c
> @@ -251,6 +251,32 @@ notmuch_command (notmuch_config_t *config,
>      return 0;
>  }
>  
> +static int
> +redirect_stderr (const char * stderr_file)
> +{
> +    if (strcmp (stderr_file, "-") == 0) {
> +	if (dup2 (STDOUT_FILENO, STDERR_FILENO) < 0) {
> +	    perror ("dup2");
> +	    return 1;
> +	}
> +    } else {
> +	int fd = open (stderr_file, O_WRONLY|O_CREAT|O_APPEND, 0644);

I think this should include O_TRUNC; otherwise it's too error-prone to
use programmatically.  The permissions should be 0666 (if the user's
umask says things should be group or world writable, it's not our job
to disagree).

> +	if (fd < 0) {
> +	    fprintf (stderr, "Error: Cannot redirect stderr to '%s': %s\n",
> +		     stderr_file, strerror (errno));
> +	    return 1;
> +	}
> +	if (fd != STDERR_FILENO) {
> +	    if (dup2 (fd, STDERR_FILENO) < 0) {
> +		perror ("dup2");
> +		return 1;
> +	    }
> +	    close (fd);
> +	}
> +    }
> +    return 0;
> +}
> +
>  int
>  main (int argc, char *argv[])
>  {
> @@ -259,6 +285,7 @@ main (int argc, char *argv[])
>      const char *command_name = NULL;
>      command_t *command;
>      char *config_file_name = NULL;
> +    char *stderr_file = NULL;
>      notmuch_config_t *config;
>      notmuch_bool_t print_help=FALSE, print_version=FALSE;
>      int opt_index;
> @@ -268,6 +295,7 @@ main (int argc, char *argv[])
>  	{ NOTMUCH_OPT_BOOLEAN, &print_help, "help", 'h', 0 },
>  	{ NOTMUCH_OPT_BOOLEAN, &print_version, "version", 'v', 0 },
>  	{ NOTMUCH_OPT_STRING, &config_file_name, "config", 'c', 0 },
> +	{ NOTMUCH_OPT_STRING, &stderr_file, "stderr", '\0', 0 },
>  	{ 0, 0, 0, 0, 0 }
>      };
>  
> @@ -287,6 +315,10 @@ main (int argc, char *argv[])
>  	return 1;
>      }
>  
> +    if (stderr_file && redirect_stderr (stderr_file) != 0) {
> +	/* error already printed */
> +	return 1;
> +    }
>      if (print_help)
>  	return notmuch_help_command (NULL, argc - 1, &argv[1]);
>  

Re: [PATCH v2 1/4] cli: add global option --stderr=FILE

[Tomi Ollila]

On Tue, May 28 2013, Austin Clements <amdragon@MIT.EDU> wrote:

> Quoth Tomi Ollila on May 26 at 11:45 am:
>> With this option all writes to stderr are redirected to the spesified
>> FILE (or to stdout on case FILE is '-'). This is immediately useful
>> in emacs interface as some of its exec intefaces do not provide
>> separation of stdout and stderr.
>> ---
>>  notmuch-client.h |  1 +
>>  notmuch.c        | 32 ++++++++++++++++++++++++++++++++
>>  2 files changed, 33 insertions(+)
>> 
>> diff --git a/notmuch-client.h b/notmuch-client.h
>> index 45749a6..4a3c7ac 100644
>> --- a/notmuch-client.h
>> +++ b/notmuch-client.h
>> @@ -54,6 +54,7 @@ typedef GMimeCipherContext notmuch_crypto_context_t;
>>  #include <sys/stat.h>
>>  #include <sys/time.h>
>>  #include <unistd.h>
>> +#include <fcntl.h>
>>  #include <dirent.h>
>>  #include <errno.h>
>>  #include <signal.h>
>> diff --git a/notmuch.c b/notmuch.c
>> index f51a84f..654a568 100644
>> --- a/notmuch.c
>> +++ b/notmuch.c
>> @@ -251,6 +251,32 @@ notmuch_command (notmuch_config_t *config,
>>      return 0;
>>  }
>>  
>> +static int
>> +redirect_stderr (const char * stderr_file)
>> +{
>> +    if (strcmp (stderr_file, "-") == 0) {
>> +	if (dup2 (STDOUT_FILENO, STDERR_FILENO) < 0) {
>> +	    perror ("dup2");
>> +	    return 1;
>> +	}
>> +    } else {
>> +	int fd = open (stderr_file, O_WRONLY|O_CREAT|O_APPEND, 0644);
>
> I think this should include O_TRUNC; otherwise it's too error-prone to
> use programmatically. 

You're right! I thought this one night and was supposed check that it is
O_TRUNC there -- forgot and obviously it wasn't ;/

> The permissions should be 0666 (if the user's
> umask says things should be group or world writable, it's not our job
> to disagree).

I agree that 0644 is incorrect -- but IMHO it should be 0600 -- when the
data is written to /tmp world-readable file gave others chance to read
potentially sensitive information from that file...

I'll ask this (also) in IRC -- if people generally agree the bits should
be 0666 I'll do that change instead (even personally opposing ATM).

I'll also do the man and NEWS chances suggested for v3.

Thanks for the review (this also goes to Jani & Mark).

Tomi

>> +	if (fd < 0) {
>> +	    fprintf (stderr, "Error: Cannot redirect stderr to '%s': %s\n",
>> +		     stderr_file, strerror (errno));
>> +	    return 1;
>> +	}
>> +	if (fd != STDERR_FILENO) {
>> +	    if (dup2 (fd, STDERR_FILENO) < 0) {
>> +		perror ("dup2");
>> +		return 1;
>> +	    }
>> +	    close (fd);
>> +	}
>> +    }
>> +    return 0;
>> +}
>> +
>>  int
>>  main (int argc, char *argv[])
>>  {
>> @@ -259,6 +285,7 @@ main (int argc, char *argv[])
>>      const char *command_name = NULL;
>>      command_t *command;
>>      char *config_file_name = NULL;
>> +    char *stderr_file = NULL;
>>      notmuch_config_t *config;
>>      notmuch_bool_t print_help=FALSE, print_version=FALSE;
>>      int opt_index;
>> @@ -268,6 +295,7 @@ main (int argc, char *argv[])
>>  	{ NOTMUCH_OPT_BOOLEAN, &print_help, "help", 'h', 0 },
>>  	{ NOTMUCH_OPT_BOOLEAN, &print_version, "version", 'v', 0 },
>>  	{ NOTMUCH_OPT_STRING, &config_file_name, "config", 'c', 0 },
>> +	{ NOTMUCH_OPT_STRING, &stderr_file, "stderr", '\0', 0 },
>>  	{ 0, 0, 0, 0, 0 }
>>      };
>>  
>> @@ -287,6 +315,10 @@ main (int argc, char *argv[])
>>  	return 1;
>>      }
>>  
>> +    if (stderr_file && redirect_stderr (stderr_file) != 0) {
>> +	/* error already printed */
>> +	return 1;
>> +    }
>>      if (print_help)
>>  	return notmuch_help_command (NULL, argc - 1, &argv[1]);
>>  
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH v2 2/4] test: added --stderr=FILE tests

[Tomi Ollila]

--stderr=FILE tests were added to test/help-test as it is the one
doing most global option testing. Also, it was simplest to test
this new option using `notmuch help` command.
---
 test/help-test | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/test/help-test b/test/help-test
index f7df725..bd0111c 100755
--- a/test/help-test
+++ b/test/help-test
@@ -9,4 +9,13 @@ test_expect_success 'notmuch help' 'notmuch help'
 test_expect_success 'notmuch help tag' 'notmuch help tag'
 test_expect_success 'notmuch --version' 'notmuch --version'
 
+test_begin_subtest "notmuch --stderr=stderr help %"
+notmuch --stderr=stderr help %
+test_expect_equal "$(cat stderr)" "
+Sorry, % is not a known command. There's not much I can do to help."
+
+test_begin_subtest "notmuch --stderr=- help %"
+test_expect_equal "$(notmuch --stderr=- help %)" "
+Sorry, % is not a known command. There's not much I can do to help."
+
 test_done
-- 
1.8.0

[PATCH v2 3/4] man: documented --stderr=FILE in notmuch.1 manual page

[Tomi Ollila]

---
 man/man1/notmuch.1 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/man/man1/notmuch.1 b/man/man1/notmuch.1
index 033cc10..fbd575a 100644
--- a/man/man1/notmuch.1
+++ b/man/man1/notmuch.1
@@ -76,7 +76,14 @@ Print the installed version of notmuch, and exit.
 
 Specify the configuration file to use. This overrides any
 configuration file specified by ${NOTMUCH_CONFIG}.
+.RE
+
+.RS 4
+.TP 4
+.B \-\-stderr=FILE
 
+Redirect all writes to stderr to the specified file.
+If the file name is a plain '-', stderr is written to stdout.
 .RE
 
 .SH COMMANDS
-- 
1.8.0

Re: [PATCH v2 3/4] man: documented --stderr=FILE in notmuch.1 manual page

[Austin Clements]

Quoth Tomi Ollila on May 26 at 11:45 am:
> ---
>  man/man1/notmuch.1 | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/man/man1/notmuch.1 b/man/man1/notmuch.1
> index 033cc10..fbd575a 100644
> --- a/man/man1/notmuch.1
> +++ b/man/man1/notmuch.1
> @@ -76,7 +76,14 @@ Print the installed version of notmuch, and exit.
>  
>  Specify the configuration file to use. This overrides any
>  configuration file specified by ${NOTMUCH_CONFIG}.
> +.RE
> +
> +.RS 4
> +.TP 4
> +.B \-\-stderr=FILE
>  
> +Redirect all writes to stderr to the specified file.
> +If the file name is a plain '-', stderr is written to stdout.

The second sentence could be just "If FILE is '-', stderr is
redirected to stdout.".  Same goes for the NEWS.

>  .RE
>  
>  .SH COMMANDS

Series LGTM other than my two comments.

[PATCH v2 4/4] NEWS: added information about new --stderr=FILE global option

[Tomi Ollila]

---
 NEWS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/NEWS b/NEWS
index 8545913..c04f6bb 100644
--- a/NEWS
+++ b/NEWS
@@ -18,6 +18,12 @@ Bash command-line completion
   `notmuch config`. The new completion support depends on the
   bash-completion package.
 
+New global option `--stderr=FILE`
+
+  With this option all writes to stderr are redirected to the
+  specified file. If file name is a plain '-', stderr is written
+  to stdout.
+
 Vim Front-End
 -------------
 
-- 
1.8.0

[PATCH v2 4b/4] NEWS: added information about new --stderr=FILE top level option

[Tomi Ollila]

---
 NEWS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/NEWS b/NEWS
index a7f2ec6..990b038 100644
--- a/NEWS
+++ b/NEWS
@@ -35,6 +35,12 @@ Top level option to specify configuration file
   It's now possible to specify the configuration file to use on the
   command line using the `notmuch --config=FILE` option.
 
+Top level option to redirect writes to stderr
+
+  With `notmuch --stderr=FILE` all writes to stderr are redirected to
+  the specified file. If the file name is a plain '-', stderr is
+  written to stdout.
+
 Deprecated commands "part" and "search-tags" are removed.
 
 Bash command-line completion
-- 
1.8.1.4