Re: [PATCH 17/16] config: add core.commentString

From: Jeff King <peff@peff.net>
Date: Mar 28, 2024, 09:47 UTC
Message-ID: <20240328094740.GA871147@coredump.intra.peff.net>
In-Reply-To: <xmqqa5mj3b6c.fsf@gitster.g>

On Wed, Mar 27, 2024 at 09:13:31AM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> > An alternative to using "$var cannot ..." in the error messages (if we
> > don't like the all-lowercase variable name) is to just say "comment
> > strings cannot ...". That vaguely covers both cases, and the message
> > printed by the config code itself does mention the actual variable name
> > that triggered the error.
> 
> OK, because the error() return from this function will trigger
> another die() in the caller, e.g.
> 
>     error: core.commentchar must have at least one character
>     fatal: bad config variable 'core.commentchar' in file '.git/config' at line 6
> 
> so we can afford to make the "error" side vague, except that the
> "fatal" one is also downcased already, so we are not really solving
> anything by making the message vague, I would think.  The posted
> patch as-is is prefectly fine.

Oh, right. For some reason I thought the die() message would have the variable as written by the user, but that obviously is not true. So I agree it would not even be an improvement (and the normalizing in my new error() message is something we've been living with all along anyway for other messages).

Show 9 quoted lines

> Side note:
>     I wonder if we would later want to somehow _merge_ these two
>     error messages, i.e. the lower-level will notice and record the
>     nature of the problem instead of calling error(), and the caller
>     will use the recorded information while composing the "fatal"
>     message to die with.  I actually do not know if it is a good
>     idea to begin with.  If we want to do it right, the "record"
>     part probably cannot be a simple "stringify into strbuf" that
>     will result in lego message that is harder for i18n folks.

Yeah, this is a general problem of accumulating errors. I had always assumed in cases like this that we could have some language-independent syntax like:

  die("%s:%d: error parsing '%s': %s",
      file, line_nr, var, err_from_callback);

It's certainly lego-like, but it avoids the worst lego cases where we're literally composing sentences. But as somebody who does not do translations, it's possible I'm just being optimistic. ;)

-Peff