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

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

[Giuseppe Scrivano]

[-- Attachment #1: Type: text/plain, Size: 190 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Print-the-usage-string-on-stdout-instead-of-stderr.patch --]
[-- Type: text/x-diff, Size: 3267 bytes --]

>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(-)

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

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

[Michael J Gruber]

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.

> 
> The small patch I have attached changes -h to print on stdout.
> 
> Thanks,
> Giuseppe
> 

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

[Miles Bader]

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_.

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

[Giuseppe Scrivano]

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

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

[Michael J Gruber]

Miles Bader venit, vidit, dixit 17.05.2010 14:07:
> 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

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

[Michael J Gruber]

Giuseppe Scrivano venit, vidit, dixit 17.05.2010 14:40:
> 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

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

[Miles Bader]

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.

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

[Giuseppe Scrivano]

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

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

[Tay Ray Chuan]

On Mon, May 17, 2010 at 10:07 PM, Giuseppe Scrivano <gscrivano@gnu.org> wrote:
> 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

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

[Giuseppe Scrivano]

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.

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
-- 
1.7.1

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

[Michael J Gruber]

Giuseppe Scrivano venit, vidit, dixit 17.05.2010 18:02:
> 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!

> 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

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

[Giuseppe Scrivano]

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

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

[Michael J Gruber]

Giuseppe Scrivano venit, vidit, dixit 24.05.2010 22:51:
> 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

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

[Giuseppe Scrivano]

Hello,

Michael J Gruber <git@drmicha.warpmail.net> writes:

> 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(-)

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