| Message ID | xmqq4jd7p1wf.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 31399a6b6166cf76cc533bc9915878211607ed80 |
| Headers | show |
| Series | [v3] config: add --comment option to add a comment | expand |
Junio C Hamano <gitster@pobox.com> writes:
> Extending the previous step, this allows ...
If I am not mistaken, this topic, originating at
https://lore.kernel.org/git/pull.1681.v3.git.1710280020508.gitgitgadget@gmail.com/
is expecting responses from you. Can you unblock it to let us move
forward?
Thanks.
* Junio C. Hamano:
> Can you unblock [this topic] to let us move forward?
I don't see any obvious way to alter the pull request's state, or where
a response of mine might be inserted to move things along. I also have
not found anything related in the GitGitGadget docs. Thus, I'll try to
issue another /submit command in the hope that this might affect the
process.
-Ralph
Ralph Seichter <github@seichter.de> writes: >> Can you unblock [this topic] to let us move forward? > > I don't see any obvious way to alter the pull request's state, or where > a response of mine might be inserted to move things along. The easiest would have been for you to say "Yup, the two additional patches queued on top of mine seem to make it better. Let me review them in detail", followed by "Yeah, they looked good to me", and after that we can just merge the topic with three patches down to 'next' and later to 'master'. Of course, if you do not agree with the two follow-up patches, you can point out issues in them and argue why they are bad idea. That would take more cycles, of course.
* Junio C. Hamano: > The easiest would have been for you to say "Yup, the two additional > patches queued on top of mine seem to make it better. [...] Would it? Well, you wrote the following two weeks ago, among other things: >> I thought we already discussed this and unconditional "#comment" has >> already been declared a non starter. This unilateral decision of yours, and the following prolonged debate about spaces/tabs (which I clearly stated I consider a waste of time) left me with the impression that what I think doesn't matter much anyway. Also, it seems to me that this whole subject has already been blown far out of proportion. If this exercise leads to the feature I was proposing, you guys can do whatever, and I won't slow things down by expressing my opinions, or by continuing to formulate my own commit messages. So much time has already been spent. -Ralph
Hello Ralph, On 2024-03-27 01:27, Ralph Seichter wrote: > * Junio C. Hamano: > >> The easiest would have been for you to say "Yup, the two additional >> patches queued on top of mine seem to make it better. [...] > > Would it? Well, you wrote the following two weeks ago, among other > things: > >>> I thought we already discussed this and unconditional "#comment" has >>> already been declared a non starter. > > This unilateral decision of yours, and the following prolonged debate > about spaces/tabs (which I clearly stated I consider a waste of time) > left me with the impression that what I think doesn't matter much > anyway. Also, it seems to me that this whole subject has already been > blown far out of proportion. If this exercise leads to the feature I > was > proposing, you guys can do whatever, and I won't slow things down by > expressing my opinions, or by continuing to formulate my own commit > messages. So much time has already been spent. Well, is getting patches accepted to git hard and often challenging? Absolutely. Do additional patches and discussions sprout all over the place? They obviously do. But, does a high-profile open-source project deserve such an approach? Without doubt. In fact, pretty much every good software project should employ such an approach, but that unfortunately isn't always the case. That's how I see it.
Ralph Seichter <github@seichter.de> writes: >>> I thought we already discussed this and unconditional "#comment" has >>> already been declared a non starter. > > This unilateral decision of yours, and the following prolonged debate > about spaces/tabs (which I clearly stated I consider a waste of time) > left me with the impression that what I think doesn't matter much > anyway. You can call it unilateral or whatever you like, but there are project principles, e.g., "try to keep things consistent", and "making unusual things possible is fine, but it shouldn't make the default thing harder", and they are not negotiable. When it comes to quality of the end product, it is true that your considering it a waste of time does not matter. It is effort needed to polish your topic to an acceptable level. Either we do so with the help of reviewer input, or alternatively we could drop the topic. In any case, I'll keep the three patches separate and mark the topic for 'next'. Thanks.
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index af374ee2e0..e4f2e07926 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -89,9 +89,15 @@ OPTIONS --comment <message>:: Append a comment at the end of new or modified lines. - Unless _<message>_ begins with "#", a string "# " (hash - followed by a space) is prepended to it. The _<message>_ must not - contain linefeed characters (no multi-line comments are permitted). + + If _<message>_ begins with one or more whitespaces followed + by "#", it is used as-is. If it begins with "#", a space is + prepended before it is used. Otherwise, a string " # " (a + space followed by a hash followed by a space) is prepended + to it. And the resulting string is placed immediately after + the value defined for the variable. The _<message>_ must + not contain linefeed characters (no multi-line comments are + permitted). --get:: Get the value for a given key (optionally filtered by a regex diff --git a/builtin/config.c b/builtin/config.c index e859a659f4..0015620dde 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -841,12 +841,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) flags |= CONFIG_FLAGS_FIXED_VALUE; } - if (comment) { - if (strchr(comment, '\n')) - die(_("no multi-line comment allowed: '%s'"), comment); - if (comment[0] != '#') - comment = xstrfmt("# %s", comment); - } + comment = git_config_prepare_comment_string(comment); if (actions & PAGING_ACTIONS) setup_auto_pager("config", 1); diff --git a/config.c b/config.c index 15019cb9a5..f1d4263a68 100644 --- a/config.c +++ b/config.c @@ -3044,7 +3044,7 @@ static ssize_t write_pair(int fd, const char *key, const char *value, } if (comment) - strbuf_addf(&sb, "%s %s\n", quote, comment); + strbuf_addf(&sb, "%s%s\n", quote, comment); else strbuf_addf(&sb, "%s\n", quote); @@ -3172,6 +3172,62 @@ void git_config_set(const char *key, const char *value) trace2_cmd_set_config(key, value); } +/* + * The ownership rule is that the caller will own the string + * if it receives a piece of memory different from what it passed + * as the parameter. + */ +const char *git_config_prepare_comment_string(const char *comment) +{ + size_t leading_blanks; + + if (!comment) + return NULL; + + if (strchr(comment, '\n')) + die(_("no multi-line comment allowed: '%s'"), comment); + + /* + * If it begins with one or more leading whitespace characters + * followed by '#", the comment string is used as-is. + * + * If it begins with '#', a SP is inserted between the comment + * and the value the comment is about. + * + * Otherwise, the value is followed by a SP followed by '#' + * followed by SP and then the comment string comes. + */ + + leading_blanks = strspn(comment, " \t"); + if (leading_blanks && comment[leading_blanks] == '#') + ; /* use it as-is */ + else if (comment[0] == '#') + comment = xstrfmt(" %s", comment); + else + comment = xstrfmt(" # %s", comment); + + return comment; +} + +static void validate_comment_string(const char *comment) +{ + size_t leading_blanks; + + if (!comment) + return; + /* + * The front-end must have massaged the comment string + * properly before calling us. + */ + if (strchr(comment, '\n')) + BUG("multi-line comments are not permitted: '%s'", comment); + + leading_blanks = strspn(comment, " \t"); + if (!leading_blanks || comment[leading_blanks] != '#') + BUG("comment must begin with one or more SP followed by '#': '%s'", + comment); +} + /* * If value==NULL, unset in (remove from) config, * if value_pattern!=NULL, disregard key/value pairs where value does not match. @@ -3211,16 +3267,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, size_t contents_sz; struct config_store_data store = CONFIG_STORE_INIT; - if (comment) { - /* - * The front-end must have massaged the comment string - * properly before calling us. - */ - if (strchr(comment, '\n')) - BUG("multi-line comments are not permitted: '%s'", comment); - if (comment[0] != '#') - BUG("comment should begin with '#': '%s'", comment); - } + validate_comment_string(comment); /* parse-key returns negative; flip the sign to feed exit(3) */ ret = 0 - git_config_parse_key(key, &store.key, &store.baselen); diff --git a/config.h b/config.h index a85a827169..f4966e3749 100644 --- a/config.h +++ b/config.h @@ -338,6 +338,8 @@ void git_config_set_multivar(const char *, const char *, const char *, unsigned) int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned); int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, const char *, unsigned); +const char *git_config_prepare_comment_string(const char *); + /** * takes four parameters: * diff --git a/t/t1300-config.sh b/t/t1300-config.sh index d5dfb45877..cc050b3c20 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -74,6 +74,8 @@ cat > expect << EOF penguin = gentoo # Pygoscelis papua disposition = peckish # find fish foo = bar #abc + spsp = value # and comment + htsp = value # and comment [Sections] WhatEver = Second EOF @@ -82,6 +84,10 @@ test_expect_success 'append comments' ' git config --replace-all --comment="Pygoscelis papua" section.penguin gentoo && git config --comment="find fish" section.disposition peckish && git config --comment="#abc" section.foo bar && + + git config --comment="and comment" section.spsp value && + git config --comment=" # and comment" section.htsp value && + test_cmp expect .git/config '
Extending the previous step, this allows the whitespace placed after the value before the "# comment message" to be tweaked by tweaking the preprocessing rule to: * If the given comment string begins with one or more whitespace characters followed by '#', it is passed intact. * If the given comment string begins with '#', a Space is prepended. * Otherwise, " # " (Space, '#', Space) is prefixed. * A string with LF in it cannot be used as a comment string. Unlike the previous step, which unconditionally added a space after the value before writing the "# comment string", because the above preprocessing already gives a whitespace before the '#', the resulting string is written immediately after copying the value. And the sanity checking rule becomes * comment string after the above massaging that comes into git_config_set_multivar_in_file_gently() must - begin with zero or more whitespace characters followed by '#'. - not have a LF in it. I personally think this is over-engineered, but since I thought things through anyway, here it is in the patch form. The logic to tweak end-user supplied comment string is encapsulated in a new helper function, git_config_prepare_comment_string(), so if new front-end callers would want to use the same massaging rules, it is easily reused. Unfortunately I do not think of a way to tweak the preprocessing rules further to optionally allow having no blank after the value, i.e. to produce [section] variable = value#comment (which is a valid way to say section.variable=value, by the way) without sacrificing the ergonomics for the more usual case, so this time I really stop here. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * This is [3/1] as the one attached to my review of the single patch is [2/1]. Documentation/git-config.txt | 12 +++++-- builtin/config.c | 7 +--- config.c | 69 ++++++++++++++++++++++++++++++------ config.h | 2 ++ t/t1300-config.sh | 6 ++++ 5 files changed, 76 insertions(+), 20 deletions(-)