| Message ID | xmqqy39xwtfs.fsf_-_@gitster-ct.c.googlers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Re* [PATCH v3 1/1] protocol: advertise multiple supported versions | expand |
On Mon, Nov 12, 2018 at 7:24 PM Junio C Hamano <gitster@pobox.com> wrote: > > Stefan Beller <sbeller@google.com> writes: > > >> + if (have_advertised_versions_already) > >> + BUG(_("attempting to register an allowed protocol version after advertisement")); > > > > If this is a real BUG (due to wrong program flow) instead of bad user input, > > we would not want to burden the translators with this message. > > > > If it is a message that the user is intended to see upon bad input > > we'd rather go with die(_("translatable text here")); > > Yeah, good suggestion. > > Perhaps we should spell it out in docstrings for BUG/die with the > above rationale. A weatherbaloon patch follows. "Nobody reads documentation any more, we have too much of it." [1] [1] c.f. https://quoteinvestigator.com/2014/08/29/too-crowded/ I would have hoped to have a .cocci patch in the bad style category, i.e. disallowing the _() function inside the context of BUG(). That said, I like the patch below. Thanks for writing it. > git-compat-util.h | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/git-compat-util.h b/git-compat-util.h > index 96a3f86d8e..a1cf68cbbc 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -420,7 +420,16 @@ static inline char *git_find_last_dir_sep(const char *path) > > struct strbuf; > > -/* General helper functions */ > +/* > + * General helper functions > + * > + * Note that these are to produce end-user facing messages, and most > + * of them should be marked for translation (the exception is > + * messages generated to be sent over the wire---as we currently do not > + * have a mechanism to notice and honor locale settings used on the > + * other end, leave them untranslated. We should *not* send messages > + * that are localized for our end). > + */ > extern void vreportf(const char *prefix, const char *err, va_list params); > extern NORETURN void usage(const char *err); > extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2))); > @@ -1142,6 +1151,17 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size, > /* usage.c: only to be used for testing BUG() implementation (see test-tool) */ > extern int BUG_exit_code; > > +/* > + * BUG(fmt, ...) is to be used when the problem of reaching that point > + * in code can only be caused by a program error. To abort with a > + * message to report impossible-to-continue condition due to external > + * factors like end-user input, environment settings, data it was fed > + * over the wire, use die(_(fmt),...). > + * > + * Note that the message from BUG() should not be marked for > + * translation while the message from die() is in general end-user > + * facing and should be marked for translation. > + */ > #ifdef HAVE_VARIADIC_MACROS > __attribute__((format (printf, 3, 4))) NORETURN > void BUG_fl(const char *file, int line, const char *fmt, ...);
Stefan Beller <sbeller@google.com> writes: > "Nobody reads documentation any more, we have too much of it." Maybe, but spelling it out and writing it down, you can point at it (or hope that other people point at it without making a bad or incorrect rephrase). > I would have hoped to have a .cocci patch in the bad style category, > i.e. disallowing the _() function inside the context of BUG(). > > That said, I like the patch below. Thanks for writing it. That was a mere weather-baloon, and too early to thank about. Hopefully people can pick nits and come to a final version that can be applied. I have a few things in it that I think may be debatable. I am not sure if we should talk about being careful not to send localized strings over the wire in that comment, for example.
diff --git a/git-compat-util.h b/git-compat-util.h index 96a3f86d8e..a1cf68cbbc 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -420,7 +420,16 @@ static inline char *git_find_last_dir_sep(const char *path) struct strbuf; -/* General helper functions */ +/* + * General helper functions + * + * Note that these are to produce end-user facing messages, and most + * of them should be marked for translation (the exception is + * messages generated to be sent over the wire---as we currently do not + * have a mechanism to notice and honor locale settings used on the + * other end, leave them untranslated. We should *not* send messages + * that are localized for our end). + */ extern void vreportf(const char *prefix, const char *err, va_list params); extern NORETURN void usage(const char *err); extern NORETURN void usagef(const char *err, ...) __attribute__((format (printf, 1, 2))); @@ -1142,6 +1151,17 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size, /* usage.c: only to be used for testing BUG() implementation (see test-tool) */ extern int BUG_exit_code; +/* + * BUG(fmt, ...) is to be used when the problem of reaching that point + * in code can only be caused by a program error. To abort with a + * message to report impossible-to-continue condition due to external + * factors like end-user input, environment settings, data it was fed + * over the wire, use die(_(fmt),...). + * + * Note that the message from BUG() should not be marked for + * translation while the message from die() is in general end-user + * facing and should be marked for translation. + */ #ifdef HAVE_VARIADIC_MACROS __attribute__((format (printf, 3, 4))) NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...);