Re: [PATCH v2 16/16] config: allow multi-byte core.commentChar

From

Jeff King <peff@peff.net>

Date

Mar 16, 2024, 05:50 UTC

Message-ID

<20240316055013.GA32145@coredump.intra.peff.net>

In-Reply-To

<xmqqr0gbtsdz.fsf@gitster.g>

On Fri, Mar 15, 2024 at 08:40:56AM -0700, Junio C Hamano wrote:

> > That's assuming we don't want to go the commentString route, which would
> > require a bit more re-working of the patch. I'm also open to a more
> > clever or pretty multi-byte example if we have one. ;)
> 
> Adding core.commentString can be done long after the dust settles
> and I would expect that most of the changes in the patch would not
> have to be updated.  The parts that use comment_line_str variable do
> not have to change, the documentation needs "core.commentString is a
> synonym for core.commentChar, the latter of which is understood by
> older versions of Git (but they may use only the first byte of the
> string)" or something, but other than that, the existing text after
> this patch does not have to be updated.  If we add a proper synonym
> support to the config machinery, that would be a sizable project,
> but otherwise it would be just another "if (!strcmp()) var = val".

diff --git a/config.c b/config.c
index 92c752ed9f..13fb922bf5 100644
--- a/config.c
+++ b/config.c
@@ -1560,7 +1560,8 @@ static int git_default_core_config(const char *var, const char *value,
 	if (!strcmp(var, "core.editor"))
 		return git_config_string(&editor_program, var, value);
 
-	if (!strcmp(var, "core.commentchar")) {
+	if (!strcmp(var, "core.commentchar") ||
+	    !strcmp(var, "core.commentstring")) {
 		if (!value)
 			return config_error_nonbool(var);
 		else if (!strcasecmp(value, "auto"))

(the real work of course being in docs and tests).

If we wanted to distinguish them more (say, core.commentChar remains
as-is but core.commentString allows strings and takes precedence), then
we'd need to do it now to avoid flip-flopping between versions. I don't
see a huge benefit in restricting commentChar though.

> Stepping back a bit, one thing that we do need to mention in this
> round is what happens when you use multi-byte sequence and have it
> accessed by existing versions of Git.  "use only the first byte" I
> wrote above came out of thin air without experimenting or reading
> the code, but something like that ought to be part of the "Note
> that" paragraph above.
> 
> 	(default '#'). Note that this variable can be a string like
> 	`//` or `⁑⁕⁑`; it doesn't have to be a single ASCII character.
> 	Also note that older versions of Git used only the first byte
> 	(not necessarily a character) of the value of this variable,
> 	so you may want to be careful if you plan to use versions of
> 	Git older than 2.45.

The current code barfs for anything larger than a byte:

  $ git.v2.44.0 -c core.commentchar=foo stripspace -s
  error: core.commentChar should only be one ASCII character
  fatal: unable to parse 'core.commentchar' from command-line config

I'm mixed on these sorts of version-specific notes in the documentation.
For people who aren't mixing versions, the history is useless noise
(whose value decreases as time goes on and 2.45 becomes "old" itself).
For people who do use older versions, they'd quickly get an error like
the one above.

So I dunno. I'm not strictly opposed, but if this is something we think
is worth warning about, then that implies to me that it is worth
providing a more ergonomic solution like core.commentString.

-Peff