threads / rfc / 23825

RFC patchPrint the usage string on stdout instead of stderr.

Subject: [RFC][PATCH] Print the usage string on stdout instead of stderr.

## tl;dr

14 messages between May 17, 2010 and May 25, 2010. Diffs are folded; open one to read it.

replies: 13people: 4as markdown or json

Giuseppe Scrivano· May 17, 2010, 09:48 UTC · lore
Hello,

I have noticed that the -h flag uses stderr to print the usage string, is there any reason for it?

The small patch I have attached changes -h to print on stdout.

Thanks, Giuseppe

>From 4a5c4e4470dae11e22ff233b34a10b6a912fcd3e Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <gscrivano@gnu.org>
Date: Mon, 17 May 2010 11:31:09 +0200
Subject: [PATCH] Print the usage string on stdout instead of stderr.
Signed-off-by: Giuseppe Scrivano <gscrivano@gnu.org>
---
 parse-options.c |   36 +++++++++++++++++-------------------
 1 files changed, 17 insertions(+), 19 deletions(-)
Show changes to parse-options.c +17 −19
diff --git a/parse-options.c b/parse-options.c
index 8546d85..9adaf44 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -479,7 +479,7 @@ static int usage_argh(const struct option *opts)
 			s = literal ? "[%s]" : "[<%s>]";
 	else
 		s = literal ? " %s" : " <%s>";
-	return fprintf(stderr, s, opts->argh ? opts->argh : "...");
+	return printf(s, opts->argh ? opts->argh : "...");
 }
 
 #define USAGE_OPTS_WIDTH 24
@@ -491,47 +491,45 @@ static int usage_with_options_internal(const char * const *usagestr,
 	if (!usagestr)
 		return PARSE_OPT_HELP;
 
-	fprintf(stderr, "usage: %s\n", *usagestr++);
+	printf("usage: %s\n", *usagestr++);
 	while (*usagestr && **usagestr)
-		fprintf(stderr, "   or: %s\n", *usagestr++);
+		printf("   or: %s\n", *usagestr++);
 	while (*usagestr) {
-		fprintf(stderr, "%s%s\n",
-				**usagestr ? "    " : "",
-				*usagestr);
+		printf("%s%s\n",**usagestr ? "    " : "", *usagestr);
 		usagestr++;
 	}
 
 	if (opts->type != OPTION_GROUP)
-		fputc('\n', stderr);
+		fputc('\n', stdout);
 
 	for (; opts->type != OPTION_END; opts++) {
 		size_t pos;
 		int pad;
 
 		if (opts->type == OPTION_GROUP) {
-			fputc('\n', stderr);
+			fputc('\n', stdout);
 			if (*opts->help)
-				fprintf(stderr, "%s\n", opts->help);
+				printf("%s\n", opts->help);
 			continue;
 		}
 		if (!full && (opts->flags & PARSE_OPT_HIDDEN))
 			continue;
 
-		pos = fprintf(stderr, "    ");
+		pos = printf("    ");
 		if (opts->short_name && !(opts->flags & PARSE_OPT_NEGHELP)) {
 			if (opts->flags & PARSE_OPT_NODASH)
-				pos += fprintf(stderr, "%c", opts->short_name);
+				pos += printf("%c", opts->short_name);
 			else
-				pos += fprintf(stderr, "-%c", opts->short_name);
+				pos += printf("-%c", opts->short_name);
 		}
 		if (opts->long_name && opts->short_name)
-			pos += fprintf(stderr, ", ");
+			pos += printf(", ");
 		if (opts->long_name)
-			pos += fprintf(stderr, "--%s%s",
+			pos += printf("--%s%s",
 				(opts->flags & PARSE_OPT_NEGHELP) ?  "no-" : "",
 				opts->long_name);
 		if (opts->type == OPTION_NUMBER)
-			pos += fprintf(stderr, "-NUM");
+			pos += printf("-NUM");
 
 		if (!(opts->flags & PARSE_OPT_NOARG))
 			pos += usage_argh(opts);
@@ -539,12 +537,12 @@ static int usage_with_options_internal(const char * const *usagestr,
 		if (pos <= USAGE_OPTS_WIDTH)
 			pad = USAGE_OPTS_WIDTH - pos;
 		else {
-			fputc('\n', stderr);
+			fputc('\n', stdout);
 			pad = USAGE_OPTS_WIDTH;
 		}
-		fprintf(stderr, "%*s%s\n", pad + USAGE_GAP, "", opts->help);
+		printf("%*s%s\n", pad + USAGE_GAP, "", opts->help);
 	}
-	fputc('\n', stderr);
+	fputc('\n', stdout);
 
 	return PARSE_OPT_HELP;
 }
@@ -560,7 +558,7 @@ void usage_msg_opt(const char *msg,
 		   const char * const *usagestr,
 		   const struct option *options)
 {
-	fprintf(stderr, "%s\n\n", msg);
+	printf("%s\n\n", msg);
 	usage_with_options(usagestr, options);
 }
 
-- 
1.7.1
Michael J Gruber· May 17, 2010, 11:46 UTC · re: Giuseppe Scrivano · lore

Re: [RFC][PATCH] Print the usage string on stdout instead of stderr.

Giuseppe Scrivano venit, vidit, dixit 17.05.2010 11:48:
> Hello,
> 
> I have noticed that the -h flag uses stderr to print the usage string,
> is there any reason for it?

As a general rule, regular output goes to stdout and error reports to stderr.

Now, usage messages are displayed on specific request (-h) as well as when a command is used with wrong arguments. So the classification depends on the use case! But I reckon that even with '-h', usage strings are not exactly "regular output", so stderr looks more natural to me.

More importantly, callers expect error messages on stderr, such as usage with wrong arguments. I don't think scripts would call commands with '-h', and if they do they do so on purpose and can parse stderr, knowing there is no stdout in this case.

Michael
P.S.: I guess this means NACK from me FWIIW.
Show 6 quoted lines
> 
> The small patch I have attached changes -h to print on stdout.
> 
> Thanks,
> Giuseppe
> 
Miles Bader· May 17, 2010, 12:07 UTC · re: Michael J Gruber · lore

Re: [RFC][PATCH] Print the usage string on stdout instead of stderr.

Michael J Gruber <git@drmicha.warpmail.net> writes:
> Now, usage messages are displayed on specific request (-h) as well as
> when a command is used with wrong arguments. So the classification
> depends on the use case! But I reckon that even with '-h', usage strings
> are not exactly "regular output", so stderr looks more natural to me.

Usage info specifically requested by the user is not error output, it is the output of the command. It should be output to stdout, not stderr.

[Note that for GNU progs, this behavior is explicitly required by the GNU coding standards, and I think it's a pretty reasonable rule.]

-miles
-- 
o The existentialist, not having a pillow, goes everywhere with the book by
  Sullivan, _I am going to spit on your graves_.
Michael J Gruber· May 17, 2010, 13:30 UTC · re: Miles Bader · lore

Re: [RFC][PATCH] Print the usage string on stdout instead of stderr.

Miles Bader venit, vidit, dixit 17.05.2010 14:07:
Show 8 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>> Now, usage messages are displayed on specific request (-h) as well as
>> when a command is used with wrong arguments. So the classification
>> depends on the use case! But I reckon that even with '-h', usage strings
>> are not exactly "regular output", so stderr looks more natural to me.
> 
> Usage info specifically requested by the user is not error output, it is
> the output of the command.  It should be output to stdout, not stderr.

Well sure it is, just as I wrote. So do you suggest that the file handle should depend on the use case? Care to implement?

> [Note that for GNU progs, this behavior is explicitly required by the
> GNU coding standards, and I think it's a pretty reasonable rule.]
Fortunately, Git is not GNU software.
Michael
Miles Bader· May 17, 2010, 13:54 UTC · re: Michael J Gruber · lore

Re: [RFC][PATCH] Print the usage string on stdout instead of stderr.

On Mon, May 17, 2010 at 10:30 PM, Michael J Gruber <git@drmicha.warpmail.net> wrote:

>> [Note that for GNU progs, this behavior is explicitly required by the
>> GNU coding standards, and I think it's a pretty reasonable rule.]
>
> Fortunately, Git is not GNU software.

The point is not that git must follow these standards, but that it''s a reasonable guideline (as are many of the GNU standards), and it's generally useful if free software such as git is broadly consistent with other free software. For instance, git uses GNU-style long command-line option syntax, and this is a good thing.

-Miles
-- 
Do not taunt Happy Fun Ball.
Giuseppe Scrivano· May 17, 2010, 14:07 UTC · re: Michael J Gruber · lore

Re: [RFC][PATCH] Print the usage string on stdout instead of stderr.

Michael J Gruber <git@drmicha.warpmail.net> writes:
> Well sure it is, just as I wrote. So do you suggest that the file handle
> should depend on the use case? Care to implement?
Thanks for your comments.  I will fix my patch accordingly and resend it.

Cheers, Giuseppe

Tay Ray Chuan· May 17, 2010, 14:11 UTC · re: Giuseppe Scrivano · lore

Re: [RFC][PATCH] Print the usage string on stdout instead of stderr.

On Mon, May 17, 2010 at 10:07 PM, Giuseppe Scrivano <gscrivano@gnu.org> wrote:
Show 6 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> Well sure it is, just as I wrote. So do you suggest that the file handle
>> should depend on the use case? Care to implement?
>
> Thanks for your comments.  I will fix my patch accordingly and resend it.
While you're at it, please take a look at
  http://github.com/git/git/blob/master/Documentation/SubmittingPatches
for your next re-send.
-- 
Cheers,
Ray Chuan
Giuseppe Scrivano· May 17, 2010, 12:40 UTC · re: Michael J Gruber · lore

Re: [RFC][PATCH] Print the usage string on stdout instead of stderr.

Michael J Gruber <git@drmicha.warpmail.net> writes:
> More importantly, callers expect error messages on stderr, such as usage
> with wrong arguments. I don't think scripts would call commands with
> '-h', and if they do they do so on purpose and can parse stderr, knowing
> there is no stdout in this case.

It looks like a workaround to me. Anyway, if -h is left unchanged then, I think, --help should be adjusted as well when it doesn't use an external pager.

These two commands behave differently:

git status --help 2>/dev/null | cat - git status -h 2>/dev/null | cat -

IMO, what should be changed is -h to be uniform with --help, as the it is the expected output, not an error.

Cheers, Giuseppe

Michael J Gruber· May 17, 2010, 13:30 UTC · re: Giuseppe Scrivano · lore

Re: [RFC][PATCH] Print the usage string on stdout instead of stderr.

Giuseppe Scrivano venit, vidit, dixit 17.05.2010 14:40:
Show 15 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> More importantly, callers expect error messages on stderr, such as usage
>> with wrong arguments. I don't think scripts would call commands with
>> '-h', and if they do they do so on purpose and can parse stderr, knowing
>> there is no stdout in this case.
> 
> It looks like a workaround to me.  Anyway, if -h is left unchanged then,
> I think, --help should be adjusted as well when it doesn't use an
> external pager.
> 
> These two commands behave differently:
> 
> git status --help 2>/dev/null | cat -
> git status -h 2>/dev/null | cat -

They also do two very different things. The first one displays the man page, the second the usage string.

> IMO, what should be changed is -h to be uniform with --help, as the it
> is the expected output, not an error.
That is not what your patch does.

I'm not opposed to having -h output on stdout. I'm opposed to having usage error messages on stdout. Currently, both are on stderr, which is no problem (we're not bound by GNU rules here).

Your patch puts both on stdout, and that is a problem, not only for several test which could be adjusted, but also for scripters.

A patch which *really* only changes '-h' to use stdout would certainly not be objected. Actually, most calling sites are probably the "error case" so that you only need to make sure that the "-h" path get's to use a different output file descriptor.

Cheers, Michael

Giuseppe Scrivano· May 17, 2010, 16:02 UTC · re: Michael J Gruber · lore

Re: [RFC][PATCH] Print the usage string on stdout instead of stderr.

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 7 quoted lines
> Your patch puts both on stdout, and that is a problem, not only for
> several test which could be adjusted, but also for scripters.
>
> A patch which *really* only changes '-h' to use stdout would certainly
> not be objected. Actually, most calling sites are probably the "error
> case" so that you only need to make sure that the "-h" path get's to use
> a different output file descriptor.
I have changed my patches following your suggestions.
I have also fixed two tests.

Cheers, Giuseppe

>From 00100c61db30725011edf62e7e0e7bc6ac685cb0 Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <gscrivano@gnu.org>
Date: Mon, 17 May 2010 17:34:41 +0200
Subject: [PATCH] print the usage string on stdout instead of stderr

When -h is used, print usage messages on stdout. If a command is invoked with wrong arguments then print the usage messages on stderr.

Signed-off-by: Giuseppe Scrivano <gscrivano@gnu.org>
---
 parse-options.c               |   64 +++++++++++++++++++++--------------------
 t/t0040-parse-options.sh      |    8 +++--
 t/t1502-rev-parse-parseopt.sh |    6 ++--
 3 files changed, 41 insertions(+), 37 deletions(-)
Show changes to 3 files +41 −37

parse-options.c, t/t0040-parse-options.sh, t/t1502-rev-parse-parseopt.sh

diff --git a/parse-options.c b/parse-options.c
index 8546d85..c8aaf95 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -5,7 +5,7 @@
 #include "color.h"
 
 static int parse_options_usage(const char * const *usagestr,
-			       const struct option *opts);
+			       const struct option *opts, int err);
 
 #define OPT_SHORT 1
 #define OPT_UNSET 2
@@ -352,7 +352,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 }
 
 static int usage_with_options_internal(const char * const *,
-				       const struct option *, int);
+				       const struct option *, int, int);
 
 int parse_options_step(struct parse_opt_ctx_t *ctx,
 		       const struct option *options,
@@ -380,10 +380,10 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 		if (arg[1] != '-') {
 			ctx->opt = arg + 1;
 			if (internal_help && *ctx->opt == 'h')
-				return parse_options_usage(usagestr, options);
+				return parse_options_usage(usagestr, options, 0);
 			switch (parse_short_opt(ctx, options)) {
 			case -1:
-				return parse_options_usage(usagestr, options);
+				return parse_options_usage(usagestr, options, 1);
 			case -2:
 				goto unknown;
 			}
@@ -391,10 +391,10 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 				check_typos(arg + 1, options);
 			while (ctx->opt) {
 				if (internal_help && *ctx->opt == 'h')
-					return parse_options_usage(usagestr, options);
+					return parse_options_usage(usagestr, options, 0);
 				switch (parse_short_opt(ctx, options)) {
 				case -1:
-					return parse_options_usage(usagestr, options);
+					return parse_options_usage(usagestr, options, 1);
 				case -2:
 					/* fake a short option thing to hide the fact that we may have
 					 * started to parse aggregated stuff
@@ -418,12 +418,12 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 		}
 
 		if (internal_help && !strcmp(arg + 2, "help-all"))
-			return usage_with_options_internal(usagestr, options, 1);
+			return usage_with_options_internal(usagestr, options, 1, 0);
 		if (internal_help && !strcmp(arg + 2, "help"))
-			return parse_options_usage(usagestr, options);
+			return parse_options_usage(usagestr, options, 0);
 		switch (parse_long_opt(ctx, arg + 2, options)) {
 		case -1:
-			return parse_options_usage(usagestr, options);
+			return parse_options_usage(usagestr, options, 1);
 		case -2:
 			goto unknown;
 		}
@@ -468,7 +468,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
 	return parse_options_end(&ctx);
 }
 
-static int usage_argh(const struct option *opts)
+static int usage_argh(const struct option *opts, FILE *outfile)
 {
 	const char *s;
 	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
@@ -479,72 +479,74 @@ static int usage_argh(const struct option *opts)
 			s = literal ? "[%s]" : "[<%s>]";
 	else
 		s = literal ? " %s" : " <%s>";
-	return fprintf(stderr, s, opts->argh ? opts->argh : "...");
+	return fprintf(outfile, s, opts->argh ? opts->argh : "...");
 }
 
 #define USAGE_OPTS_WIDTH 24
 #define USAGE_GAP         2
 
 static int usage_with_options_internal(const char * const *usagestr,
-				const struct option *opts, int full)
+				const struct option *opts, int full, int err)
 {
+	FILE *outfile = err ? stderr : stdout;
+
 	if (!usagestr)
 		return PARSE_OPT_HELP;
 
-	fprintf(stderr, "usage: %s\n", *usagestr++);
+	fprintf(outfile, "usage: %s\n", *usagestr++);
 	while (*usagestr && **usagestr)
-		fprintf(stderr, "   or: %s\n", *usagestr++);
+		fprintf(outfile, "   or: %s\n", *usagestr++);
 	while (*usagestr) {
-		fprintf(stderr, "%s%s\n",
+		fprintf(outfile, "%s%s\n",
 				**usagestr ? "    " : "",
 				*usagestr);
 		usagestr++;
 	}
 
 	if (opts->type != OPTION_GROUP)
-		fputc('\n', stderr);
+		fputc('\n', outfile);
 
 	for (; opts->type != OPTION_END; opts++) {
 		size_t pos;
 		int pad;
 
 		if (opts->type == OPTION_GROUP) {
-			fputc('\n', stderr);
+			fputc('\n', outfile);
 			if (*opts->help)
-				fprintf(stderr, "%s\n", opts->help);
+				fprintf(outfile, "%s\n", opts->help);
 			continue;
 		}
 		if (!full && (opts->flags & PARSE_OPT_HIDDEN))
 			continue;
 
-		pos = fprintf(stderr, "    ");
+		pos = fprintf(outfile, "    ");
 		if (opts->short_name && !(opts->flags & PARSE_OPT_NEGHELP)) {
 			if (opts->flags & PARSE_OPT_NODASH)
-				pos += fprintf(stderr, "%c", opts->short_name);
+				pos += fprintf(outfile, "%c", opts->short_name);
 			else
-				pos += fprintf(stderr, "-%c", opts->short_name);
+				pos += fprintf(outfile, "-%c", opts->short_name);
 		}
 		if (opts->long_name && opts->short_name)
-			pos += fprintf(stderr, ", ");
+			pos += fprintf(outfile, ", ");
 		if (opts->long_name)
-			pos += fprintf(stderr, "--%s%s",
+			pos += fprintf(outfile, "--%s%s",
 				(opts->flags & PARSE_OPT_NEGHELP) ?  "no-" : "",
 				opts->long_name);
 		if (opts->type == OPTION_NUMBER)
-			pos += fprintf(stderr, "-NUM");
+			pos += fprintf(outfile, "-NUM");
 
 		if (!(opts->flags & PARSE_OPT_NOARG))
-			pos += usage_argh(opts);
+			pos += usage_argh(opts, outfile);
 
 		if (pos <= USAGE_OPTS_WIDTH)
 			pad = USAGE_OPTS_WIDTH - pos;
 		else {
-			fputc('\n', stderr);
+			fputc('\n', outfile);
 			pad = USAGE_OPTS_WIDTH;
 		}
-		fprintf(stderr, "%*s%s\n", pad + USAGE_GAP, "", opts->help);
+		fprintf(outfile, "%*s%s\n", pad + USAGE_GAP, "", opts->help);
 	}
-	fputc('\n', stderr);
+	fputc('\n', outfile);
 
 	return PARSE_OPT_HELP;
 }
@@ -552,7 +554,7 @@ static int usage_with_options_internal(const char * const *usagestr,
 void usage_with_options(const char * const *usagestr,
 			const struct option *opts)
 {
-	usage_with_options_internal(usagestr, opts, 0);
+	usage_with_options_internal(usagestr, opts, 0, 1);
 	exit(129);
 }
 
@@ -565,9 +567,9 @@ void usage_msg_opt(const char *msg,
 }
 
 static int parse_options_usage(const char * const *usagestr,
-			       const struct option *opts)
+			       const struct option *opts, int err)
 {
-	return usage_with_options_internal(usagestr, opts, 0);
+	return usage_with_options_internal(usagestr, opts, 0, err);
 }
 
 
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 3d450ed..2092450 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -7,7 +7,7 @@ test_description='our own option parser'
 
 . ./test-lib.sh
 
-cat > expect.err << EOF
+cat > expect << EOF
 usage: test-parse-options <options>
 
     -b, --boolean         get a boolean
@@ -46,10 +46,12 @@ EOF
 
 test_expect_success 'test help' '
 	test_must_fail test-parse-options -h > output 2> output.err &&
-	test ! -s output &&
-	test_cmp expect.err output.err
+	test ! -s output.err &&
+	test_cmp expect output
 '
 
+mv expect expect.err
+
 cat > expect << EOF
 boolean: 2
 integer: 1729
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index e504058..660487d 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -3,7 +3,7 @@
 test_description='test git rev-parse --parseopt'
 . ./test-lib.sh
 
-cat > expect.err <<EOF
+cat > expect <<EOF
 usage: some-command [options] <args>...
 
     some-command does foo and bar!
@@ -38,8 +38,8 @@ extra1    line above used to cause a segfault but no longer does
 EOF
 
 test_expect_success 'test --parseopt help output' '
-	git rev-parse --parseopt -- -h 2> output.err < optionspec
-	test_cmp expect.err output.err
+	git rev-parse --parseopt -- -h > output < optionspec
+	test_cmp expect output
 '
 
 cat > expect <<EOF
-- 
1.7.1
Michael J Gruber· May 18, 2010, 09:43 UTC · re: Giuseppe Scrivano · lore

Re: [RFC][PATCH] Print the usage string on stdout instead of stderr.

Giuseppe Scrivano venit, vidit, dixit 17.05.2010 18:02:
Show 11 quoted lines
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> Your patch puts both on stdout, and that is a problem, not only for
>> several test which could be adjusted, but also for scripters.
>>
>> A patch which *really* only changes '-h' to use stdout would certainly
>> not be objected. Actually, most calling sites are probably the "error
>> case" so that you only need to make sure that the "-h" path get's to use
>> a different output file descriptor.
> 
> I have changed my patches following your suggestions.
Thanks!
Show 261 quoted lines
> I have also fixed two tests.
> 
> Cheers,
> Giuseppe
> 
> 
> 
> From 00100c61db30725011edf62e7e0e7bc6ac685cb0 Mon Sep 17 00:00:00 2001
> From: Giuseppe Scrivano <gscrivano@gnu.org>
> Date: Mon, 17 May 2010 17:34:41 +0200
> Subject: [PATCH] print the usage string on stdout instead of stderr
> 
> When -h is used, print usage messages on stdout.  If a command is invoked with
> wrong arguments then print the usage messages on stderr.
> 
> Signed-off-by: Giuseppe Scrivano <gscrivano@gnu.org>
> ---
>  parse-options.c               |   64 +++++++++++++++++++++--------------------
>  t/t0040-parse-options.sh      |    8 +++--
>  t/t1502-rev-parse-parseopt.sh |    6 ++--
>  3 files changed, 41 insertions(+), 37 deletions(-)
> 
> diff --git a/parse-options.c b/parse-options.c
> index 8546d85..c8aaf95 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -5,7 +5,7 @@
>  #include "color.h"
>  
>  static int parse_options_usage(const char * const *usagestr,
> -			       const struct option *opts);
> +			       const struct option *opts, int err);
>  
>  #define OPT_SHORT 1
>  #define OPT_UNSET 2
> @@ -352,7 +352,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
>  }
>  
>  static int usage_with_options_internal(const char * const *,
> -				       const struct option *, int);
> +				       const struct option *, int, int);
>  
>  int parse_options_step(struct parse_opt_ctx_t *ctx,
>  		       const struct option *options,
> @@ -380,10 +380,10 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>  		if (arg[1] != '-') {
>  			ctx->opt = arg + 1;
>  			if (internal_help && *ctx->opt == 'h')
> -				return parse_options_usage(usagestr, options);
> +				return parse_options_usage(usagestr, options, 0);
>  			switch (parse_short_opt(ctx, options)) {
>  			case -1:
> -				return parse_options_usage(usagestr, options);
> +				return parse_options_usage(usagestr, options, 1);
>  			case -2:
>  				goto unknown;
>  			}
> @@ -391,10 +391,10 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>  				check_typos(arg + 1, options);
>  			while (ctx->opt) {
>  				if (internal_help && *ctx->opt == 'h')
> -					return parse_options_usage(usagestr, options);
> +					return parse_options_usage(usagestr, options, 0);
>  				switch (parse_short_opt(ctx, options)) {
>  				case -1:
> -					return parse_options_usage(usagestr, options);
> +					return parse_options_usage(usagestr, options, 1);
>  				case -2:
>  					/* fake a short option thing to hide the fact that we may have
>  					 * started to parse aggregated stuff
> @@ -418,12 +418,12 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>  		}
>  
>  		if (internal_help && !strcmp(arg + 2, "help-all"))
> -			return usage_with_options_internal(usagestr, options, 1);
> +			return usage_with_options_internal(usagestr, options, 1, 0);
>  		if (internal_help && !strcmp(arg + 2, "help"))
> -			return parse_options_usage(usagestr, options);
> +			return parse_options_usage(usagestr, options, 0);
>  		switch (parse_long_opt(ctx, arg + 2, options)) {
>  		case -1:
> -			return parse_options_usage(usagestr, options);
> +			return parse_options_usage(usagestr, options, 1);
>  		case -2:
>  			goto unknown;
>  		}
> @@ -468,7 +468,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
>  	return parse_options_end(&ctx);
>  }
>  
> -static int usage_argh(const struct option *opts)
> +static int usage_argh(const struct option *opts, FILE *outfile)
>  {
>  	const char *s;
>  	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
> @@ -479,72 +479,74 @@ static int usage_argh(const struct option *opts)
>  			s = literal ? "[%s]" : "[<%s>]";
>  	else
>  		s = literal ? " %s" : " <%s>";
> -	return fprintf(stderr, s, opts->argh ? opts->argh : "...");
> +	return fprintf(outfile, s, opts->argh ? opts->argh : "...");
>  }
>  
>  #define USAGE_OPTS_WIDTH 24
>  #define USAGE_GAP         2
>  
>  static int usage_with_options_internal(const char * const *usagestr,
> -				const struct option *opts, int full)
> +				const struct option *opts, int full, int err)
>  {
> +	FILE *outfile = err ? stderr : stdout;
> +
>  	if (!usagestr)
>  		return PARSE_OPT_HELP;
>  
> -	fprintf(stderr, "usage: %s\n", *usagestr++);
> +	fprintf(outfile, "usage: %s\n", *usagestr++);
>  	while (*usagestr && **usagestr)
> -		fprintf(stderr, "   or: %s\n", *usagestr++);
> +		fprintf(outfile, "   or: %s\n", *usagestr++);
>  	while (*usagestr) {
> -		fprintf(stderr, "%s%s\n",
> +		fprintf(outfile, "%s%s\n",
>  				**usagestr ? "    " : "",
>  				*usagestr);
>  		usagestr++;
>  	}
>  
>  	if (opts->type != OPTION_GROUP)
> -		fputc('\n', stderr);
> +		fputc('\n', outfile);
>  
>  	for (; opts->type != OPTION_END; opts++) {
>  		size_t pos;
>  		int pad;
>  
>  		if (opts->type == OPTION_GROUP) {
> -			fputc('\n', stderr);
> +			fputc('\n', outfile);
>  			if (*opts->help)
> -				fprintf(stderr, "%s\n", opts->help);
> +				fprintf(outfile, "%s\n", opts->help);
>  			continue;
>  		}
>  		if (!full && (opts->flags & PARSE_OPT_HIDDEN))
>  			continue;
>  
> -		pos = fprintf(stderr, "    ");
> +		pos = fprintf(outfile, "    ");
>  		if (opts->short_name && !(opts->flags & PARSE_OPT_NEGHELP)) {
>  			if (opts->flags & PARSE_OPT_NODASH)
> -				pos += fprintf(stderr, "%c", opts->short_name);
> +				pos += fprintf(outfile, "%c", opts->short_name);
>  			else
> -				pos += fprintf(stderr, "-%c", opts->short_name);
> +				pos += fprintf(outfile, "-%c", opts->short_name);
>  		}
>  		if (opts->long_name && opts->short_name)
> -			pos += fprintf(stderr, ", ");
> +			pos += fprintf(outfile, ", ");
>  		if (opts->long_name)
> -			pos += fprintf(stderr, "--%s%s",
> +			pos += fprintf(outfile, "--%s%s",
>  				(opts->flags & PARSE_OPT_NEGHELP) ?  "no-" : "",
>  				opts->long_name);
>  		if (opts->type == OPTION_NUMBER)
> -			pos += fprintf(stderr, "-NUM");
> +			pos += fprintf(outfile, "-NUM");
>  
>  		if (!(opts->flags & PARSE_OPT_NOARG))
> -			pos += usage_argh(opts);
> +			pos += usage_argh(opts, outfile);
>  
>  		if (pos <= USAGE_OPTS_WIDTH)
>  			pad = USAGE_OPTS_WIDTH - pos;
>  		else {
> -			fputc('\n', stderr);
> +			fputc('\n', outfile);
>  			pad = USAGE_OPTS_WIDTH;
>  		}
> -		fprintf(stderr, "%*s%s\n", pad + USAGE_GAP, "", opts->help);
> +		fprintf(outfile, "%*s%s\n", pad + USAGE_GAP, "", opts->help);
>  	}
> -	fputc('\n', stderr);
> +	fputc('\n', outfile);
>  
>  	return PARSE_OPT_HELP;
>  }
> @@ -552,7 +554,7 @@ static int usage_with_options_internal(const char * const *usagestr,
>  void usage_with_options(const char * const *usagestr,
>  			const struct option *opts)
>  {
> -	usage_with_options_internal(usagestr, opts, 0);
> +	usage_with_options_internal(usagestr, opts, 0, 1);
>  	exit(129);
>  }
>  
> @@ -565,9 +567,9 @@ void usage_msg_opt(const char *msg,
>  }
>  
>  static int parse_options_usage(const char * const *usagestr,
> -			       const struct option *opts)
> +			       const struct option *opts, int err)
>  {
> -	return usage_with_options_internal(usagestr, opts, 0);
> +	return usage_with_options_internal(usagestr, opts, 0, err);
>  }
>  
>  
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index 3d450ed..2092450 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -7,7 +7,7 @@ test_description='our own option parser'
>  
>  . ./test-lib.sh
>  
> -cat > expect.err << EOF
> +cat > expect << EOF
>  usage: test-parse-options <options>
>  
>      -b, --boolean         get a boolean
> @@ -46,10 +46,12 @@ EOF
>  
>  test_expect_success 'test help' '
>  	test_must_fail test-parse-options -h > output 2> output.err &&
> -	test ! -s output &&
> -	test_cmp expect.err output.err
> +	test ! -s output.err &&
> +	test_cmp expect output
>  '
>  
> +mv expect expect.err
> +
>  cat > expect << EOF
>  boolean: 2
>  integer: 1729
> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
> index e504058..660487d 100755
> --- a/t/t1502-rev-parse-parseopt.sh
> +++ b/t/t1502-rev-parse-parseopt.sh
> @@ -3,7 +3,7 @@
>  test_description='test git rev-parse --parseopt'
>  . ./test-lib.sh
>  
> -cat > expect.err <<EOF
> +cat > expect <<EOF
>  usage: some-command [options] <args>...
>  
>      some-command does foo and bar!
> @@ -38,8 +38,8 @@ extra1    line above used to cause a segfault but no longer does
>  EOF
>  
>  test_expect_success 'test --parseopt help output' '
> -	git rev-parse --parseopt -- -h 2> output.err < optionspec
> -	test_cmp expect.err output.err
> +	git rev-parse --parseopt -- -h > output < optionspec
> +	test_cmp expect output
>  '
>  
>  cat > expect <<EOF

I haven't checked whether this covers all code paths but other than that it looks OK to me, and the tests pass.

Cheers, Michael

Giuseppe Scrivano· May 24, 2010, 20:51 UTC · re: Michael J Gruber · lore

Re: [RFC][PATCH] Print the usage string on stdout instead of stderr.

Hello Michael,
Michael J Gruber <git@drmicha.warpmail.net> writes:
> I haven't checked whether this covers all code paths but other than that
> it looks OK to me, and the tests pass.
is the patch ready for inclusion?

Thanks, Giuseppe

Michael J Gruber· May 25, 2010, 06:46 UTC · re: Giuseppe Scrivano · lore

Re: [RFC][PATCH] Print the usage string on stdout instead of stderr.

Giuseppe Scrivano venit, vidit, dixit 24.05.2010 22:51:
Show 9 quoted lines
> Hello Michael,
> 
> 
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>> I haven't checked whether this covers all code paths but other than that
>> it looks OK to me, and the tests pass.
> 
> is the patch ready for inclusion?
You're the author, you should know ;)

Note that I can't include your patch. That's up to Junio, and he's been lagging back for good reasons (as you could read on the list) and is in the process of keeping up.

I gave you my partial ack'ed-by (see above) and a homework problem to answer: Have you checked whether this covers all code paths (all help/usage callers)? If yes then it's a good idea to resend v2 of your patch as a reply to this thread but with subject "[PATCH v2]..." as it is hard to find otherwise, and cc to Junio.

Cheers, Michael

Giuseppe Scrivano· May 25, 2010, 08:40 UTC · re: Michael J Gruber · lore

[PATCH v2] Print the usage string on stdout instead of stderr.

Hello,
Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 5 quoted lines
> I gave you my partial ack'ed-by (see above) and a homework problem to
> answer: Have you checked whether this covers all code paths (all
> help/usage callers)? If yes then it's a good idea to resend v2 of your
> patch as a reply to this thread but with subject "[PATCH v2]..." as it
> is hard to find otherwise, and cc to Junio.
Yes, I have just double-checked it; I hope I am not missing something.

Thanks, Giuseppe

>From 00100c61db30725011edf62e7e0e7bc6ac685cb0 Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <gscrivano@gnu.org>
Date: Mon, 17 May 2010 17:34:41 +0200
Subject: [PATCH] print the usage string on stdout instead of stderr

When -h is used, print usage messages on stdout. If a command is invoked with wrong arguments then print the usage messages on stderr.

Signed-off-by: Giuseppe Scrivano <gscrivano@gnu.org>
---
 parse-options.c               |   64 +++++++++++++++++++++--------------------
 t/t0040-parse-options.sh      |    8 +++--
 t/t1502-rev-parse-parseopt.sh |    6 ++--
 3 files changed, 41 insertions(+), 37 deletions(-)
Show changes to 3 files +41 −37

parse-options.c, t/t0040-parse-options.sh, t/t1502-rev-parse-parseopt.sh

diff --git a/parse-options.c b/parse-options.c
index 8546d85..c8aaf95 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -5,7 +5,7 @@
 #include "color.h"
 
 static int parse_options_usage(const char * const *usagestr,
-			       const struct option *opts);
+			       const struct option *opts, int err);
 
 #define OPT_SHORT 1
 #define OPT_UNSET 2
@@ -352,7 +352,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 }
 
 static int usage_with_options_internal(const char * const *,
-				       const struct option *, int);
+				       const struct option *, int, int);
 
 int parse_options_step(struct parse_opt_ctx_t *ctx,
 		       const struct option *options,
@@ -380,10 +380,10 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 		if (arg[1] != '-') {
 			ctx->opt = arg + 1;
 			if (internal_help && *ctx->opt == 'h')
-				return parse_options_usage(usagestr, options);
+				return parse_options_usage(usagestr, options, 0);
 			switch (parse_short_opt(ctx, options)) {
 			case -1:
-				return parse_options_usage(usagestr, options);
+				return parse_options_usage(usagestr, options, 1);
 			case -2:
 				goto unknown;
 			}
@@ -391,10 +391,10 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 				check_typos(arg + 1, options);
 			while (ctx->opt) {
 				if (internal_help && *ctx->opt == 'h')
-					return parse_options_usage(usagestr, options);
+					return parse_options_usage(usagestr, options, 0);
 				switch (parse_short_opt(ctx, options)) {
 				case -1:
-					return parse_options_usage(usagestr, options);
+					return parse_options_usage(usagestr, options, 1);
 				case -2:
 					/* fake a short option thing to hide the fact that we may have
 					 * started to parse aggregated stuff
@@ -418,12 +418,12 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 		}
 
 		if (internal_help && !strcmp(arg + 2, "help-all"))
-			return usage_with_options_internal(usagestr, options, 1);
+			return usage_with_options_internal(usagestr, options, 1, 0);
 		if (internal_help && !strcmp(arg + 2, "help"))
-			return parse_options_usage(usagestr, options);
+			return parse_options_usage(usagestr, options, 0);
 		switch (parse_long_opt(ctx, arg + 2, options)) {
 		case -1:
-			return parse_options_usage(usagestr, options);
+			return parse_options_usage(usagestr, options, 1);
 		case -2:
 			goto unknown;
 		}
@@ -468,7 +468,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
 	return parse_options_end(&ctx);
 }
 
-static int usage_argh(const struct option *opts)
+static int usage_argh(const struct option *opts, FILE *outfile)
 {
 	const char *s;
 	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
@@ -479,72 +479,74 @@ static int usage_argh(const struct option *opts)
 			s = literal ? "[%s]" : "[<%s>]";
 	else
 		s = literal ? " %s" : " <%s>";
-	return fprintf(stderr, s, opts->argh ? opts->argh : "...");
+	return fprintf(outfile, s, opts->argh ? opts->argh : "...");
 }
 
 #define USAGE_OPTS_WIDTH 24
 #define USAGE_GAP         2
 
 static int usage_with_options_internal(const char * const *usagestr,
-				const struct option *opts, int full)
+				const struct option *opts, int full, int err)
 {
+	FILE *outfile = err ? stderr : stdout;
+
 	if (!usagestr)
 		return PARSE_OPT_HELP;
 
-	fprintf(stderr, "usage: %s\n", *usagestr++);
+	fprintf(outfile, "usage: %s\n", *usagestr++);
 	while (*usagestr && **usagestr)
-		fprintf(stderr, "   or: %s\n", *usagestr++);
+		fprintf(outfile, "   or: %s\n", *usagestr++);
 	while (*usagestr) {
-		fprintf(stderr, "%s%s\n",
+		fprintf(outfile, "%s%s\n",
 				**usagestr ? "    " : "",
 				*usagestr);
 		usagestr++;
 	}
 
 	if (opts->type != OPTION_GROUP)
-		fputc('\n', stderr);
+		fputc('\n', outfile);
 
 	for (; opts->type != OPTION_END; opts++) {
 		size_t pos;
 		int pad;
 
 		if (opts->type == OPTION_GROUP) {
-			fputc('\n', stderr);
+			fputc('\n', outfile);
 			if (*opts->help)
-				fprintf(stderr, "%s\n", opts->help);
+				fprintf(outfile, "%s\n", opts->help);
 			continue;
 		}
 		if (!full && (opts->flags & PARSE_OPT_HIDDEN))
 			continue;
 
-		pos = fprintf(stderr, "    ");
+		pos = fprintf(outfile, "    ");
 		if (opts->short_name && !(opts->flags & PARSE_OPT_NEGHELP)) {
 			if (opts->flags & PARSE_OPT_NODASH)
-				pos += fprintf(stderr, "%c", opts->short_name);
+				pos += fprintf(outfile, "%c", opts->short_name);
 			else
-				pos += fprintf(stderr, "-%c", opts->short_name);
+				pos += fprintf(outfile, "-%c", opts->short_name);
 		}
 		if (opts->long_name && opts->short_name)
-			pos += fprintf(stderr, ", ");
+			pos += fprintf(outfile, ", ");
 		if (opts->long_name)
-			pos += fprintf(stderr, "--%s%s",
+			pos += fprintf(outfile, "--%s%s",
 				(opts->flags & PARSE_OPT_NEGHELP) ?  "no-" : "",
 				opts->long_name);
 		if (opts->type == OPTION_NUMBER)
-			pos += fprintf(stderr, "-NUM");
+			pos += fprintf(outfile, "-NUM");
 
 		if (!(opts->flags & PARSE_OPT_NOARG))
-			pos += usage_argh(opts);
+			pos += usage_argh(opts, outfile);
 
 		if (pos <= USAGE_OPTS_WIDTH)
 			pad = USAGE_OPTS_WIDTH - pos;
 		else {
-			fputc('\n', stderr);
+			fputc('\n', outfile);
 			pad = USAGE_OPTS_WIDTH;
 		}
-		fprintf(stderr, "%*s%s\n", pad + USAGE_GAP, "", opts->help);
+		fprintf(outfile, "%*s%s\n", pad + USAGE_GAP, "", opts->help);
 	}
-	fputc('\n', stderr);
+	fputc('\n', outfile);
 
 	return PARSE_OPT_HELP;
 }
@@ -552,7 +554,7 @@ static int usage_with_options_internal(const char * const *usagestr,
 void usage_with_options(const char * const *usagestr,
 			const struct option *opts)
 {
-	usage_with_options_internal(usagestr, opts, 0);
+	usage_with_options_internal(usagestr, opts, 0, 1);
 	exit(129);
 }
 
@@ -565,9 +567,9 @@ void usage_msg_opt(const char *msg,
 }
 
 static int parse_options_usage(const char * const *usagestr,
-			       const struct option *opts)
+			       const struct option *opts, int err)
 {
-	return usage_with_options_internal(usagestr, opts, 0);
+	return usage_with_options_internal(usagestr, opts, 0, err);
 }
 
 
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 3d450ed..2092450 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -7,7 +7,7 @@ test_description='our own option parser'
 
 . ./test-lib.sh
 
-cat > expect.err << EOF
+cat > expect << EOF
 usage: test-parse-options <options>
 
     -b, --boolean         get a boolean
@@ -46,10 +46,12 @@ EOF
 
 test_expect_success 'test help' '
 	test_must_fail test-parse-options -h > output 2> output.err &&
-	test ! -s output &&
-	test_cmp expect.err output.err
+	test ! -s output.err &&
+	test_cmp expect output
 '
 
+mv expect expect.err
+
 cat > expect << EOF
 boolean: 2
 integer: 1729
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index e504058..660487d 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -3,7 +3,7 @@
 test_description='test git rev-parse --parseopt'
 . ./test-lib.sh
 
-cat > expect.err <<EOF
+cat > expect <<EOF
 usage: some-command [options] <args>...
 
     some-command does foo and bar!
@@ -38,8 +38,8 @@ extra1    line above used to cause a segfault but no longer does
 EOF
 
 test_expect_success 'test --parseopt help output' '
-	git rev-parse --parseopt -- -h 2> output.err < optionspec
-	test_cmp expect.err output.err
+	git rev-parse --parseopt -- -h > output < optionspec
+	test_cmp expect output
 '
 
 cat > expect <<EOF
-- 
1.7.1

← back to recent threads