| Message ID | 20240327081922.GA830163@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9ccf3e9b22b6843892319b189fd7aed37c451420 |
| Headers | show |
| Series | allow multi-byte core.commentChar | expand |
Assuming the implementation continues as suggested, I'll mention here that I really like this note: On Wed, Mar 27, 2024 at 1:19 AM Jeff King <peff@peff.net> wrote: > +Note that these two variables are aliases of each other, and in modern > +versions of Git you are free to use a string (e.g., `//` or `⁑⁕⁑`) with > +`commentChar`. Versions of Git prior to v2.45.0 will ignore > +`commentString` but will reject a value of `commentChar` that consists > +of more than a single ASCII byte. If you plan to use your config with > +older and newer versions of Git, you may want to specify both: One of the big things I think is missing from existing Git documentation (and would, alas, be a huge effort to provide) is backwards-compatibility notes. People are often stuck with old versions of software, at least during initial bringup, for a variety of reasons, and such notes can be quite helpful. Examples of modern systems that have extensive notes include Python, where the documentation often says "new in 3.7" or whatever, and Go, where the automatically-built documentation notes which version of Go introduced some new function. I'm not exactly volunteering here for the heavy lifting though. :-) Chris
Jeff King <peff@peff.net> writes: > Note that you graduated kh/doc-commentchar-is-a-byte, which says "this > ASCII character" early in the description, which will be incorrect if my > series is merged. True. I could tweak this patch to force a conflict core.commentChar:: core.commentString:: Commands such as `commit` and `tag` that let you edit - messages consider a line that begins with this character + messages consider a line that begins with this string commented, and removes them after the editor returns (default '#'). and let the rerere database to remember the resolution (which will tweak "string" back to "character"). But I'll prepare a merge-fix before I forget, which is a cleaner approach. > 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. 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. $ git diff refs/merge-fix/jk/core-comment-string^! diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt index bd033ab100..bbe869c497 100644 --- a/Documentation/config/core.txt +++ b/Documentation/config/core.txt @@ -522,7 +522,7 @@ core.editor:: core.commentChar:: core.commentString:: Commands such as `commit` and `tag` that let you edit - messages consider a line that begins with this ASCII character + messages consider a line that begins with this character commented, and removes them after the editor returns (default '#'). +
On Wed, Mar 27, 2024 at 09:13:31AM -0700, Junio C Hamano wrote: > > 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). > 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
diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt index c86b8c8408..bbe869c497 100644 --- a/Documentation/config/core.txt +++ b/Documentation/config/core.txt @@ -520,15 +520,28 @@ core.editor:: `GIT_EDITOR` is not set. See linkgit:git-var[1]. core.commentChar:: +core.commentString:: Commands such as `commit` and `tag` that let you edit messages consider a line that begins with this character commented, and removes them after the editor returns - (default '#'). Note that this option can take values larger than - a byte (whether a single multi-byte character, or you - could even go wild with a multi-character sequence). + (default '#'). + If set to "auto", `git-commit` would select a character that is not the beginning character of any line in existing commit messages. ++ +Note that these two variables are aliases of each other, and in modern +versions of Git you are free to use a string (e.g., `//` or `⁑⁕⁑`) with +`commentChar`. Versions of Git prior to v2.45.0 will ignore +`commentString` but will reject a value of `commentChar` that consists +of more than a single ASCII byte. If you plan to use your config with +older and newer versions of Git, you may want to specify both: ++ + [core] + # single character for older versions + commentChar = "#" + # string for newer versions (which will override commentChar + # because it comes later in the file) + commentString = "//" core.filesRefLockTimeout:: The length of time, in milliseconds, to retry when trying to diff --git a/config.c b/config.c index 92c752ed9f..d12e0f34f1 100644 --- a/config.c +++ b/config.c @@ -1560,18 +1560,19 @@ 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")) auto_comment_line_char = 1; else if (value[0]) { if (strchr(value, '\n')) - return error(_("core.commentChar cannot contain newline")); + return error(_("%s cannot contain newline"), var); comment_line_str = xstrdup(value); auto_comment_line_char = 0; } else - return error(_("core.commentChar must have at least one character")); + return error(_("%s must have at least one character"), var); return 0; } diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh index a161faf702..f10f42ff1e 100755 --- a/t/t0030-stripspace.sh +++ b/t/t0030-stripspace.sh @@ -401,14 +401,19 @@ test_expect_success 'strip comments with changed comment char' ' test -z "$(echo "; comment" | git -c core.commentchar=";" stripspace -s)" ' +test_expect_success 'strip comments with changed comment string' ' + test ! -z "$(echo "// comment" | git -c core.commentchar=// stripspace)" && + test -z "$(echo "// comment" | git -c core.commentchar="//" stripspace -s)" +' + test_expect_success 'newline as commentchar is forbidden' ' test_must_fail git -c core.commentChar="$LF" stripspace -s 2>err && - grep "core.commentChar cannot contain newline" err + grep "core.commentchar cannot contain newline" err ' test_expect_success 'empty commentchar is forbidden' ' test_must_fail git -c core.commentchar= stripspace -s 2>err && - grep "core.commentChar must have at least one character" err + grep "core.commentchar must have at least one character" err ' test_expect_success '-c with single line' '