[RFC PATCH] revert: Removed top level --stderr= option

[RFC PATCH] revert: Removed top level --stderr= option

[Tomi Ollila]

While looked good on paper, its attempted use caused confusion, complexity,
and potential for information leak when passed through wrapper scripts.
For slimmer code and to lessen demand for maintenance/support the set of
commits which added top level --stderr= option is now reverted.
---

Change was easy, commit message hard. Opinions? Revert is easiest to do now.
Also, if someone comes with a novel idea how to utilize --stderr option
please tell it now -- I'd be most eager to hear it :D


This change was done the following way:

$ git checkout -b rvrt b9020448bd
$ git reset --hard HEAD^^^^
$ git reset b9020448bd
$ git commit -a
$ git diff HEAD~5..HEAD

(last one to reveal HEAD~5 & HEAD have identical trees).

Question why:
id:20130521195549.6550.53636@thinkbox.jade-hamburg.de

Good reason why not:
id:1369934016-22308-1-git-send-email-amdragon@mit.edu


 NEWS               |  5 -----
 man/man1/notmuch.1 |  7 -------
 notmuch-client.h   |  1 -
 notmuch.c          | 32 --------------------------------
 test/help-test     |  9 ---------
 5 files changed, 54 deletions(-)

diff --git a/NEWS b/NEWS
index 80abd97..a7f2ec6 100644
--- a/NEWS
+++ b/NEWS
@@ -35,11 +35,6 @@ 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 FILE is '-', stderr is redirected to stdout.
-
 Deprecated commands "part" and "search-tags" are removed.
 
 Bash command-line completion
diff --git a/man/man1/notmuch.1 b/man/man1/notmuch.1
index f5ca0ad..033cc10 100644
--- a/man/man1/notmuch.1
+++ b/man/man1/notmuch.1
@@ -76,14 +76,7 @@ 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 FILE is '-', stderr is redirected to stdout.
 .RE
 
 .SH COMMANDS
diff --git a/notmuch-client.h b/notmuch-client.h
index 4a3c7ac..45749a6 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -54,7 +54,6 @@ 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 15e90c8..f51a84f 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -251,32 +251,6 @@ 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_TRUNC, 0666);
-	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[])
 {
@@ -285,7 +259,6 @@ 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;
@@ -295,7 +268,6 @@ 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 }
     };
 
@@ -315,10 +287,6 @@ 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]);
 
diff --git a/test/help-test b/test/help-test
index bd0111c..f7df725 100755
--- a/test/help-test
+++ b/test/help-test
@@ -9,13 +9,4 @@ 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

Re: [RFC PATCH] revert: Removed top level --stderr= option

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> While looked good on paper, its attempted use caused confusion, complexity,
> and potential for information leak when passed through wrapper scripts.
> For slimmer code and to lessen demand for maintenance/support the set of
> commits which added top level --stderr= option is now reverted.
> ---

I don't mind reverting. If people feel the feature is independently
useful, that's fine too. 

I guess that isn't much help, decision-wise.

d

Re: [RFC PATCH] revert: Removed top level --stderr= option

[Jani Nikula]

On Fri, 31 May 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> While looked good on paper, its attempted use caused confusion, complexity,
> and potential for information leak when passed through wrapper scripts.
> For slimmer code and to lessen demand for maintenance/support the set of
> commits which added top level --stderr= option is now reverted.
> ---
>
> Change was easy, commit message hard. Opinions? Revert is easiest to do now.
> Also, if someone comes with a novel idea how to utilize --stderr option
> please tell it now -- I'd be most eager to hear it :D
>
>
> This change was done the following way:
>
> $ git checkout -b rvrt b9020448bd
> $ git reset --hard HEAD^^^^
> $ git reset b9020448bd
> $ git commit -a
> $ git diff HEAD~5..HEAD

Protip ;)

$ git revert -n b9020448bd^^^^..b9020448bd
$ git commit

BR,
Jani.

>
> (last one to reveal HEAD~5 & HEAD have identical trees).
>
> Question why:
> id:20130521195549.6550.53636@thinkbox.jade-hamburg.de
>
> Good reason why not:
> id:1369934016-22308-1-git-send-email-amdragon@mit.edu
>
>
>  NEWS               |  5 -----
>  man/man1/notmuch.1 |  7 -------
>  notmuch-client.h   |  1 -
>  notmuch.c          | 32 --------------------------------
>  test/help-test     |  9 ---------
>  5 files changed, 54 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 80abd97..a7f2ec6 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -35,11 +35,6 @@ 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 FILE is '-', stderr is redirected to stdout.
> -
>  Deprecated commands "part" and "search-tags" are removed.
>  
>  Bash command-line completion
> diff --git a/man/man1/notmuch.1 b/man/man1/notmuch.1
> index f5ca0ad..033cc10 100644
> --- a/man/man1/notmuch.1
> +++ b/man/man1/notmuch.1
> @@ -76,14 +76,7 @@ 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 FILE is '-', stderr is redirected to stdout.
>  .RE
>  
>  .SH COMMANDS
> diff --git a/notmuch-client.h b/notmuch-client.h
> index 4a3c7ac..45749a6 100644
> --- a/notmuch-client.h
> +++ b/notmuch-client.h
> @@ -54,7 +54,6 @@ 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 15e90c8..f51a84f 100644
> --- a/notmuch.c
> +++ b/notmuch.c
> @@ -251,32 +251,6 @@ 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_TRUNC, 0666);
> -	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[])
>  {
> @@ -285,7 +259,6 @@ 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;
> @@ -295,7 +268,6 @@ 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 }
>      };
>  
> @@ -315,10 +287,6 @@ 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]);
>  
> diff --git a/test/help-test b/test/help-test
> index bd0111c..f7df725 100755
> --- a/test/help-test
> +++ b/test/help-test
> @@ -9,13 +9,4 @@ 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
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [RFC PATCH] revert: Removed top level --stderr= option

[Austin Clements]

LGTM.  While I thought --stderr was a fine solution originally, now that
we've stumbled across problems that make it difficult to use correctly
and robustly, I think Tomi's right that we should probably revert it.

On Fri, 31 May 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> While looked good on paper, its attempted use caused confusion, complexity,
> and potential for information leak when passed through wrapper scripts.
> For slimmer code and to lessen demand for maintenance/support the set of
> commits which added top level --stderr= option is now reverted.
> ---
>
> Change was easy, commit message hard. Opinions? Revert is easiest to do now.
> Also, if someone comes with a novel idea how to utilize --stderr option
> please tell it now -- I'd be most eager to hear it :D
>
>
> This change was done the following way:
>
> $ git checkout -b rvrt b9020448bd
> $ git reset --hard HEAD^^^^
> $ git reset b9020448bd
> $ git commit -a
> $ git diff HEAD~5..HEAD
>
> (last one to reveal HEAD~5 & HEAD have identical trees).
>
> Question why:
> id:20130521195549.6550.53636@thinkbox.jade-hamburg.de
>
> Good reason why not:
> id:1369934016-22308-1-git-send-email-amdragon@mit.edu

Re: [RFC PATCH] revert: Removed top level --stderr= option

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> While looked good on paper, its attempted use caused confusion, complexity,
> and potential for information leak when passed through wrapper scripts.
> For slimmer code and to lessen demand for maintenance/support the set of
> commits which added top level --stderr= option is now reverted.

pushed.

d