Re: Re* [PATCH v3 1/1] protocol: advertise multiple supported versions

From: Stefan Beller <sbeller@google.com>
Date: Nov 13, 2018, 19:21 UTC
Message-ID: <CAGZ79kYv_a8-6dppo3XcNb8wc0RuTSJeOj71Q+3FmEZguBBUiA@mail.gmail.com>
In-Reply-To: <xmqqy39xwtfs.fsf_-_@gitster-ct.c.googlers.com>

On Mon, Nov 12, 2018 at 7:24 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

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

Show 43 quoted lines

>  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, ...);