[PATCH] parse-options: print "fatal:" before usage_msg_opt()

[PATCH] parse-options: print "fatal:" before usage_msg_opt()

[Jeff King]

Programs may use usage_msg_opt() to print a brief message
followed by the program usage, and then exit. The message
isn't prefixed at all, though, so it doesn't match our usual
error output and is easy to overlook:

    $ git clone 1 2 3
    Too many arguments.

    usage: git clone [<options>] [--] <repo> [<dir>]

    -v, --verbose         be more verbose
    -q, --quiet           be more quiet
    --progress            force progress reporting
    -n, --no-checkout     don't create a checkout
    --bare                create a bare repository
    [...and so on for another 31 lines...]

It looks especially bad when the message starts with an
option, like:

    $ git replace -e
    -e needs exactly one argument

    usage: git replace [-f] <object> <replacement>
       or: git replace [-f] --edit <object>
    [...etc...]

Let's put our usual "fatal:" prefix in front of it.

Signed-off-by: Jeff King <peff@peff.net>
---
Some of the message in git-clone could stand to be rewritten to match
our usual style, too (no capitals, no trailing period), but that's
obviously out of scope for this patch. I don't think this change makes
them look any worse.

 parse-options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/parse-options.c b/parse-options.c
index 312a85dbd..4fbe924a5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -661,7 +661,7 @@ void NORETURN usage_msg_opt(const char *msg,
 		   const char * const *usagestr,
 		   const struct option *options)
 {
-	fprintf(stderr, "%s\n\n", msg);
+	fprintf(stderr, "fatal: %s\n\n", msg);
 	usage_with_options(usagestr, options);
 }
 
-- 
2.11.0.341.g202cd3142

Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Programs may use usage_msg_opt() to print a brief message
> followed by the program usage, and then exit. The message
> isn't prefixed at all, though, so it doesn't match our usual
> error output and is easy to overlook:
>
>     $ git clone 1 2 3
>     Too many arguments.
>
>     usage: git clone [<options>] [--] <repo> [<dir>]
>
>     -v, --verbose         be more verbose
>     -q, --quiet           be more quiet
>     --progress            force progress reporting
>     -n, --no-checkout     don't create a checkout
>     --bare                create a bare repository
>     [...and so on for another 31 lines...]
>
> It looks especially bad when the message starts with an
> option, like:
>
>     $ git replace -e
>     -e needs exactly one argument
>
>     usage: git replace [-f] <object> <replacement>
>        or: git replace [-f] --edit <object>
>     [...etc...]
>
> Let's put our usual "fatal:" prefix in front of it.

I briefly wondered if any caller uses this in a non-fatal situation,
but usage_with_options() always dies, so this looks like the right
thing to do.  Thanks.

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Some of the message in git-clone could stand to be rewritten to match
> our usual style, too (no capitals, no trailing period), but that's
> obviously out of scope for this patch. I don't think this change makes
> them look any worse.
>
>  parse-options.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 312a85dbd..4fbe924a5 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -661,7 +661,7 @@ void NORETURN usage_msg_opt(const char *msg,
>  		   const char * const *usagestr,
>  		   const struct option *options)
>  {
> -	fprintf(stderr, "%s\n\n", msg);
> +	fprintf(stderr, "fatal: %s\n\n", msg);
>  	usage_with_options(usagestr, options);
>  }

Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()

[Duy Nguyen]

On Wed, Dec 14, 2016 at 10:10:10AM -0500, Jeff King wrote:
> diff --git a/parse-options.c b/parse-options.c
> index 312a85dbd..4fbe924a5 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -661,7 +661,7 @@ void NORETURN usage_msg_opt(const char *msg,
>  		   const char * const *usagestr,
>  		   const struct option *options)
>  {
> -	fprintf(stderr, "%s\n\n", msg);
> +	fprintf(stderr, "fatal: %s\n\n", msg);

Your commit message does not make clear if you want this "fatal" to be
grep-able (by scripts) or not. If not, please _() the string.  Maybe
this to reduce work for translators

	/* TRANSLATORS: this is the prefix before usage error */
	fprintf(stderr, "%s %s\n\n", _("fatal:"), msg);

>  	usage_with_options(usagestr, options);
>  }
>  
> -- 
> 2.11.0.341.g202cd3142

Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()

[Jeff King]

On Mon, Dec 19, 2016 at 07:07:19PM +0700, Duy Nguyen wrote:

> On Wed, Dec 14, 2016 at 10:10:10AM -0500, Jeff King wrote:
> > diff --git a/parse-options.c b/parse-options.c
> > index 312a85dbd..4fbe924a5 100644
> > --- a/parse-options.c
> > +++ b/parse-options.c
> > @@ -661,7 +661,7 @@ void NORETURN usage_msg_opt(const char *msg,
> >  		   const char * const *usagestr,
> >  		   const struct option *options)
> >  {
> > -	fprintf(stderr, "%s\n\n", msg);
> > +	fprintf(stderr, "fatal: %s\n\n", msg);
> 
> Your commit message does not make clear if you want this "fatal" to be
> grep-able (by scripts) or not. If not, please _() the string.  Maybe
> this to reduce work for translators
> 
> 	/* TRANSLATORS: this is the prefix before usage error */
> 	fprintf(stderr, "%s %s\n\n", _("fatal:"), msg);

I don't think we translate any of our "fatal:", "error:", etc, do we?
It certainly doesn't look like it in usage.c.

-Peff

Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()

[Duy Nguyen]

On Mon, Dec 19, 2016 at 8:41 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Dec 19, 2016 at 07:07:19PM +0700, Duy Nguyen wrote:
>
>> On Wed, Dec 14, 2016 at 10:10:10AM -0500, Jeff King wrote:
>> > diff --git a/parse-options.c b/parse-options.c
>> > index 312a85dbd..4fbe924a5 100644
>> > --- a/parse-options.c
>> > +++ b/parse-options.c
>> > @@ -661,7 +661,7 @@ void NORETURN usage_msg_opt(const char *msg,
>> >                const char * const *usagestr,
>> >                const struct option *options)
>> >  {
>> > -   fprintf(stderr, "%s\n\n", msg);
>> > +   fprintf(stderr, "fatal: %s\n\n", msg);
>>
>> Your commit message does not make clear if you want this "fatal" to be
>> grep-able (by scripts) or not. If not, please _() the string.  Maybe
>> this to reduce work for translators
>>
>>       /* TRANSLATORS: this is the prefix before usage error */
>>       fprintf(stderr, "%s %s\n\n", _("fatal:"), msg);
>
> I don't think we translate any of our "fatal:", "error:", etc, do we?
> It certainly doesn't look like it in usage.c.

I know. But those existed before the l10n starts, some of those belong
to plumbing messages. This one is new.
-- 
Duy

Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()

[Jeff King]

On Mon, Dec 19, 2016 at 08:53:30PM +0700, Duy Nguyen wrote:

> >> Your commit message does not make clear if you want this "fatal" to be
> >> grep-able (by scripts) or not. If not, please _() the string.  Maybe
> >> this to reduce work for translators
> >>
> >>       /* TRANSLATORS: this is the prefix before usage error */
> >>       fprintf(stderr, "%s %s\n\n", _("fatal:"), msg);
> >
> > I don't think we translate any of our "fatal:", "error:", etc, do we?
> > It certainly doesn't look like it in usage.c.
> 
> I know. But those existed before the l10n starts, some of those belong
> to plumbing messages. This one is new.

We add lots of new messages which are themselves translated, and they
still get untranslated prefixes. It seems like consistency is more
important than translating this one spot. But then, I do not use a
translated git myself, so I would not see the difference either way.

-Peff

Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()

[Duy Nguyen]

On Mon, Dec 19, 2016 at 9:05 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Dec 19, 2016 at 08:53:30PM +0700, Duy Nguyen wrote:
>
>> >> Your commit message does not make clear if you want this "fatal" to be
>> >> grep-able (by scripts) or not. If not, please _() the string.  Maybe
>> >> this to reduce work for translators
>> >>
>> >>       /* TRANSLATORS: this is the prefix before usage error */
>> >>       fprintf(stderr, "%s %s\n\n", _("fatal:"), msg);
>> >
>> > I don't think we translate any of our "fatal:", "error:", etc, do we?
>> > It certainly doesn't look like it in usage.c.
>>
>> I know. But those existed before the l10n starts, some of those belong
>> to plumbing messages. This one is new.
>
> We add lots of new messages which are themselves translated, and they
> still get untranslated prefixes. It seems like consistency is more
> important than translating this one spot. But then, I do not use a
> translated git myself, so I would not see the difference either way.

I'm kinda used to the half English half Vietnamese messages after so
many years (not just git). I do like the prefix translated as well.
But I guess we could leave this a is for now. At least we know the
scope of this message and will have easier time i18n-izing it when we
do the same for die(), warning() and friends.
-- 
Duy

Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()

[Jeff King]

On Mon, Dec 19, 2016 at 09:30:09PM +0700, Duy Nguyen wrote:

> On Mon, Dec 19, 2016 at 9:05 PM, Jeff King <peff@peff.net> wrote:
> > On Mon, Dec 19, 2016 at 08:53:30PM +0700, Duy Nguyen wrote:
> >
> >> >> Your commit message does not make clear if you want this "fatal" to be
> >> >> grep-able (by scripts) or not. If not, please _() the string.  Maybe
> >> >> this to reduce work for translators
> >> >>
> >> >>       /* TRANSLATORS: this is the prefix before usage error */
> >> >>       fprintf(stderr, "%s %s\n\n", _("fatal:"), msg);
> >> >
> >> > I don't think we translate any of our "fatal:", "error:", etc, do we?
> >> > It certainly doesn't look like it in usage.c.
> >>
> >> I know. But those existed before the l10n starts, some of those belong
> >> to plumbing messages. This one is new.
> >
> > We add lots of new messages which are themselves translated, and they
> > still get untranslated prefixes. It seems like consistency is more
> > important than translating this one spot. But then, I do not use a
> > translated git myself, so I would not see the difference either way.
> 
> I'm kinda used to the half English half Vietnamese messages after so
> many years (not just git). I do like the prefix translated as well.
> But I guess we could leave this a is for now. At least we know the
> scope of this message and will have easier time i18n-izing it when we
> do the same for die(), warning() and friends.

Yeah. It's a big enough change that at the very least it should go into
its own patch. I don't have a strong opinion myself, except that if we
want to leave plumbing messages completely grep-able, we probably need
to distinguish between different calls to die(), etc. Which sounds kind
of nasty.

I dunno. I _thought_ nobody was supposed to be grepping stderr, even for
plumbing commands. But maybe that does not match reality.

-Peff