All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] parse-options: print "fatal:" before usage_msg_opt()
@ 2016-12-14 15:10 Jeff King
  2016-12-14 17:57 ` Junio C Hamano
  2016-12-19 12:07 ` Duy Nguyen
  0 siblings, 2 replies; 8+ messages in thread
From: Jeff King @ 2016-12-14 15:10 UTC (permalink / raw)
  To: git

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()
  2016-12-14 15:10 [PATCH] parse-options: print "fatal:" before usage_msg_opt() Jeff King
@ 2016-12-14 17:57 ` Junio C Hamano
  2016-12-19 12:07 ` Duy Nguyen
  1 sibling, 0 replies; 8+ messages in thread
From: Junio C Hamano @ 2016-12-14 17:57 UTC (permalink / raw)
  To: Jeff King; +Cc: git

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);
>  }

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()
  2016-12-14 15:10 [PATCH] parse-options: print "fatal:" before usage_msg_opt() Jeff King
  2016-12-14 17:57 ` Junio C Hamano
@ 2016-12-19 12:07 ` Duy Nguyen
  2016-12-19 13:41   ` Jeff King
  1 sibling, 1 reply; 8+ messages in thread
From: Duy Nguyen @ 2016-12-19 12:07 UTC (permalink / raw)
  To: Jeff King; +Cc: git

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()
  2016-12-19 12:07 ` Duy Nguyen
@ 2016-12-19 13:41   ` Jeff King
  2016-12-19 13:53     ` Duy Nguyen
  0 siblings, 1 reply; 8+ messages in thread
From: Jeff King @ 2016-12-19 13:41 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: git

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()
  2016-12-19 13:41   ` Jeff King
@ 2016-12-19 13:53     ` Duy Nguyen
  2016-12-19 14:05       ` Jeff King
  0 siblings, 1 reply; 8+ messages in thread
From: Duy Nguyen @ 2016-12-19 13:53 UTC (permalink / raw)
  To: Jeff King; +Cc: Git Mailing List

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()
  2016-12-19 13:53     ` Duy Nguyen
@ 2016-12-19 14:05       ` Jeff King
  2016-12-19 14:30         ` Duy Nguyen
  0 siblings, 1 reply; 8+ messages in thread
From: Jeff King @ 2016-12-19 14:05 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Git Mailing List

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()
  2016-12-19 14:05       ` Jeff King
@ 2016-12-19 14:30         ` Duy Nguyen
  2016-12-19 14:32           ` Jeff King
  0 siblings, 1 reply; 8+ messages in thread
From: Duy Nguyen @ 2016-12-19 14:30 UTC (permalink / raw)
  To: Jeff King; +Cc: Git Mailing List

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()
  2016-12-19 14:30         ` Duy Nguyen
@ 2016-12-19 14:32           ` Jeff King
  0 siblings, 0 replies; 8+ messages in thread
From: Jeff King @ 2016-12-19 14:32 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Git Mailing List

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-12-19 15:03 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-14 15:10 [PATCH] parse-options: print "fatal:" before usage_msg_opt() Jeff King
2016-12-14 17:57 ` Junio C Hamano
2016-12-19 12:07 ` Duy Nguyen
2016-12-19 13:41   ` Jeff King
2016-12-19 13:53     ` Duy Nguyen
2016-12-19 14:05       ` Jeff King
2016-12-19 14:30         ` Duy Nguyen
2016-12-19 14:32           ` Jeff King

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.