| Message ID | pull.1681.v2.git.1709824540636.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] config: add --comment option to add a comment | expand |
"Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Ralph Seichter <github@seichter.de> > > Introduce the ability to append comments to modifications > made using git-config. Example usage: > > git config --comment "changed via script" \ > --add safe.directory /home/alice/repo.git > > based on the proposed patch, the output produced is: > > [safe] > directory = /home/alice/repo.git #changed via script For readability, you probably would want to have a SP before the given string, i.e., variable = "value" # message comes here > * Motivation: > > The use case which triggered my submitting this patch is These two lines should not be needed. It is customary to just state that the users need to be able to do X and adding feature Y is one way to allow that, without such preamble. > my need to distinguish between config entries made using > automation and entries made by a human. Automation can > add comments containing a URL pointing to explanations > for the change made, avoiding questions from users as to > why their config file was changed by a third party. > > * Design considerations and implementation details: > > The implementation ensures that a # character is always > prepended to the provided comment string, and that the It is unclear what happens when a user gives comment that already has the comment introducer, e.g., --comment="# this is comment". > comment text is appended as a suffix to the changed key- > value-pair in the same line of text. Multiline comments > are deliberately not supported, because their usefulness "not supported" can take many shapes, ranging from "producing a random result and may corrupt the resulting configuration file" and "the second and subsequent lines are silently ignored", to "results in an error, stops the command before doing anything". > does not justifiy the possible problems they pose when > it comes to removing ML comments later. "justify" > > * Target audience: > > Regarding the intended consumers of the comments made: The above two lines say the same thing and are unnecessary, especially before a sentence that begins with "They are aimed at". > They are aimed at humans who inspect or change their Git > config using a pager or editor. Comments are not meant > to be read or displayed by git-config at a later time. > > Signed-off-by: Ralph Seichter <github@seichter.de> > --- > config: add --comment option to add a comment > > config: add --comment option to add a comment > > Introduce the ability to append comments to modifications made using > ... > displayed by git-config at a later time. No need to repeat a wall of text below the three-dash line if they do not give additional information on top of what is already in the proposed commit log message. > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt > index dff39093b5e..ee8cd251b24 100644 > --- a/Documentation/git-config.txt > +++ b/Documentation/git-config.txt > @@ -9,9 +9,9 @@ git-config - Get and set repository or global options > SYNOPSIS > -------- > [verse] > -'git config' [<file-option>] [--type=<type>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]] > -'git config' [<file-option>] [--type=<type>] --add <name> <value> > -'git config' [<file-option>] [--type=<type>] [--fixed-value] --replace-all <name> <value> [<value-pattern>] > +'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]] > +'git config' [<file-option>] [--type=<type>] [--comment=<value>] --add <name> <value> > +'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] --replace-all <name> <value> [<value-pattern>] > 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get <name> [<value-pattern>] > 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all <name> [<value-pattern>] > 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp <name-regex> [<value-pattern>] There is Patrick's proposal to revamp the UI of this command, https://lore.kernel.org/git/cover.1709724089.git.ps@pks.im/ which may make the above simpler (e.g., there won't be two lines that talks about "set" commands, with and without "--add" option, for example). This topic may want to at least wait for the conclusion of that design discussion, and possibly its implementation. > @@ -87,6 +87,10 @@ OPTIONS > values. This is the same as providing '^$' as the `value-pattern` > in `--replace-all`. > > +--comment:: > + Append a comment to new or modified lines. A '#' character > + will be automatically prepended to the value. Other options that take mandatory parameter are are described with their parameter on the item heading line. It may help to somehow mention that the "appending" is done at the end on the same line. Perhaps something along this line. --comment <message>:: Append the _<message>_ at the end of the new or modified lines with the value. A comment introducer ` # ` is prepended to _<message>_ if it does not already have one. The command errors out if given a _<message>_ that spans multiple lines. > @@ -797,6 +799,11 @@ int cmd_config(int argc, const char **argv, const char *prefix) > usage_builtin_config(); > } > > + if (comment && !(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) { This overly long line is both visually annoying and harder to maintain. If you can design a solution that makes it easier to read a future patch that adds support of the comment option to more actions (or removes it from an action), that would be great. Otherwise, do at least: if (comment && !(actions & (...)) { > + error(_("--comment is only applicable to add/set/replace operations")); > + usage_builtin_config(); > + } > + If I were doing this, I'd probably add this new block after the fixed-value thing, as it is bad manners to add new code _before_ existing ones, as if your new invention were somehow more important than all the others, when the order does not matter. If there is a valid technical reason (e.g., the new code modifies the state of the program that affects the beahviour of the later and existing parts of the code), the above comment of course does not apply. > /* check usage of --fixed-value */ > if (fixed_value) { > int allowed_usage = 0; > @@ -880,7 +887,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) > check_write(); > check_argc(argc, 2, 2); > value = normalize_value(argv[0], argv[1], &default_kvi); > - ret = git_config_set_in_file_gently(given_config_source.file, argv[0], value); > + ret = git_config_set_in_file_gently(given_config_source.file, argv[0], comment, value); It is somewhat annoying that we have to see so much code churn to add this new parameter X-<. I notice that we are sending "comment" to the underlying machinery without doing ANY sanity checking (like, "ah, we got a message without the '#' prefix, so we should add '# ' in front of it before asking the API to add that comment string to the value line"). We may also want to have a code that says: Yikes, this message has LF in it, but the underlying machinery does not check for it and end up corrupting the configuration file by creating [section] var = value #comment that spans on two lines when given LF=' ' git config --comment="comment${LF}that spans..." section.var value or something. The underlying machinery should be updated to die() when given such a message instead of silently corrupting the resulting file, but the front-end that receives the end-user input should check for obviously problematic payload before bothering the API with it. > - strbuf_addf(&sb, "%s\n", quote); > + if (comment) > + strbuf_addf(&sb, "%s #%s\n", quote, comment); > + else > + strbuf_addf(&sb, "%s\n", quote); > diff --git a/config.h b/config.h > index 5dba984f770..a85a8271696 100644 > --- a/config.h > +++ b/config.h > @@ -290,7 +290,7 @@ int git_config_pathname(const char **, const char *, const char *); > > int git_config_expiry_date(timestamp_t *, const char *, const char *); > int git_config_color(char *, const char *, const char *); > -int git_config_set_in_file_gently(const char *, const char *, const char *); The original was probably OK without naming these parameters of the same type, as it is (arguably) obvious to have the filename first, because that is what differenciates the "in-file" variant. And then key followed by value, because that is the usual "assignment" order people would naturally expect. But it was already on the borderline. The idea is that we do not have to (but still can) name the parameters in our function declarations when it is obvious from their types what they are. Addition of this comment thing will push us way over the line. The same comment applies to many of the function declarations touched by this patch. If I were doing this patch, I'd have at least one clean-up patch that updates this line to read: int git_config_set_in_file_gently(const char *filename, const char *key, const char *value); And then write a separate patch to add the "--comment" feature. I however have some suspicion that we might move away from "listing many random parameters" style to "having only the essential parameters, together with a single pointer to a structure that holds the optional bits" style, especially when the UI revamp Patrick proposed comes. In the case of config API, <key, value> is the essential bit in the write direction, and value-pattern restriction and the bit that says key is an regexp would be in "the optional bits" group. This <comment> thing will also be in the latter class. > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > index 31c38786870..daddaedd23c 100755 > --- a/t/t1300-config.sh > +++ b/t/t1300-config.sh > @@ -69,13 +69,18 @@ test_expect_success 'replace with non-match (actually matching)' ' > > cat > expect << EOF > [section] > - penguin = very blue > Movie = BadPhysics > UPPERCASE = true > - penguin = kingpin > + penguin = gentoo #Pygoscelis papua > + disposition = peckish #find fish > [Sections] > WhatEver = Second > EOF > +test_expect_success 'append comments' ' > + git config --replace-all --comment="Pygoscelis papua" section.penguin gentoo && > + git config --comment="find fish" section.disposition peckish && > + test_cmp expect .git/config > +' Add test for at least two more cases and show what should happen. --comment="# the user already has the pound" --comment="this is being ${LF} naughty to break configuration" Thanks.
On 2024-03-11 13:55, Junio C Hamano wrote: > "Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: Ralph Seichter <github@seichter.de> >> >> Introduce the ability to append comments to modifications >> made using git-config. Example usage: >> >> git config --comment "changed via script" \ >> --add safe.directory /home/alice/repo.git >> >> based on the proposed patch, the output produced is: >> >> [safe] >> directory = /home/alice/repo.git #changed via script > > For readability, you probably would want to have a SP before the > given string, i.e., > > variable = "value" # message comes here Let me interject... Perhaps also a tab character before the "# comment", instead of a space character. That would result in even better readability.
On Monday, March 11, 2024 12:17 PM, Dragan Simic wrote: >Subject: Re: [PATCH v2] config: add --comment option to add a comment > >On 2024-03-11 13:55, Junio C Hamano wrote: >> "Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >>> From: Ralph Seichter <github@seichter.de> >>> >>> Introduce the ability to append comments to modifications made using >>> git-config. Example usage: >>> >>> git config --comment "changed via script" \ >>> --add safe.directory /home/alice/repo.git >>> >>> based on the proposed patch, the output produced is: >>> >>> [safe] >>> directory = /home/alice/repo.git #changed via script >> >> For readability, you probably would want to have a SP before the given >> string, i.e., >> >> variable = "value" # message comes here > >Let me interject... Perhaps also a tab character before the "# comment", instead of a space character. That would result in even better >readability. Does adding a tab following data change the parse semantics of .gitconfig? My naïve understanding is that .gitconfig follows a basic rule of leading tab within a section, followed by text. Is there a formal syntax description of what valid input is? The value does not need to be quoted, so what does the following actually resolve to: (TAB)variable = value(TAB)# comment. Does variable mean value or value(TAB)? Obviously TABS should be correctly be interpreted as whitespace to be ignored. However, what about: (TAB)variable = value(TAB)s(TAB) # comment. Does that mean value(TAB)s, value(TAB)s(TAB), value s, value s(TAB), values? The definition according to git-config is "The syntax is fairly flexible and permissive; whitespaces are mostly ignored. The # and ; characters begin comments to the end of line, blank lines are ignored." "Mostly" does not make me comfortable that this is formally allowed or disallowed or ignored. I would suggest that this change needs to formalize the grammar on that documentation page for clarity.
On 2024-03-11 17:48, rsbecker@nexbridge.com wrote: > On Monday, March 11, 2024 12:17 PM, Dragan Simic wrote: >> Subject: Re: [PATCH v2] config: add --comment option to add a comment >> >> On 2024-03-11 13:55, Junio C Hamano wrote: >>> "Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes: >>> >>>> From: Ralph Seichter <github@seichter.de> >>>> >>>> Introduce the ability to append comments to modifications made using >>>> git-config. Example usage: >>>> >>>> git config --comment "changed via script" \ >>>> --add safe.directory /home/alice/repo.git >>>> >>>> based on the proposed patch, the output produced is: >>>> >>>> [safe] >>>> directory = /home/alice/repo.git #changed via script >>> >>> For readability, you probably would want to have a SP before the >>> given >>> string, i.e., >>> >>> variable = "value" # message comes here >> >> Let me interject... Perhaps also a tab character before the "# >> comment", > instead of a space character. That would result in even better >> readability. > > Does adding a tab following data change the parse semantics of > .gitconfig? > My naïve understanding is that .gitconfig follows a basic rule of > leading > tab within a section, followed by text. Is there a formal syntax > description > of what valid input is? The value does not need to be quoted, so what > does > the following actually resolve to: > > (TAB)variable = value(TAB)# comment. > > Does variable mean value or value(TAB)? Obviously TABS should be > correctly > be interpreted as whitespace to be ignored. However, what about: > > (TAB)variable = value(TAB)s(TAB) # comment. > > Does that mean value(TAB)s, value(TAB)s(TAB), value s, value s(TAB), > values? It should mean "value(TAB)s", according to git-config(1). > The definition according to git-config is > > "The syntax is fairly flexible and permissive; whitespaces are mostly > ignored. The # and ; characters begin comments to the end of line, > blank > lines are ignored." I believe these two quotations from git-config(1) should make it more clear: A line that defines a value can be continued to the next line by ending it with a \; the backslash and the end-of-line are stripped. Leading whitespaces after name =, the remainder of the line after the first comment character # or ;, and trailing whitespaces of the line are discarded unless they are enclosed in double quotes. Internal whitespaces within the value are retained verbatim. The following escape sequences (beside \" and \\) are recognized: \n for newline character (NL), \t for horizontal tabulation (HT, TAB) and \b for backspace (BS). Other char escape sequences (including octal escape sequences) are invalid. To me, all that indicates that trailing tab characters are stripped, but... > "Mostly" does not make me comfortable that this is formally allowed or > disallowed or ignored. I would suggest that this change needs to > formalize > the grammar on that documentation page for clarity. ... I do agree that it should be clarified further in the man page.
Dragan Simic <dsimic@manjaro.org> writes: > On 2024-03-11 13:55, Junio C Hamano wrote: >> "Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >>> From: Ralph Seichter <github@seichter.de> >>> Introduce the ability to append comments to modifications >>> made using git-config. Example usage: >>> git config --comment "changed via script" \ >>> --add safe.directory /home/alice/repo.git >>> based on the proposed patch, the output produced is: >>> [safe] >>> directory = /home/alice/repo.git #changed via script >> For readability, you probably would want to have a SP before the >> given string, i.e., >> variable = "value" # message comes here > > Let me interject... Perhaps also a tab character before the "# > comment", > instead of a space character. That would result in even better > readability. Depends on your screen width ;-) If you were trying to tell me that SP or no SP is merely a personal preference with the comment, I think you succeeded in doing so. Thanks.
On 2024-03-11 18:29, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: >> On 2024-03-11 13:55, Junio C Hamano wrote: >>> "Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes: >>> >>>> From: Ralph Seichter <github@seichter.de> >>>> Introduce the ability to append comments to modifications >>>> made using git-config. Example usage: >>>> git config --comment "changed via script" \ >>>> --add safe.directory /home/alice/repo.git >>>> based on the proposed patch, the output produced is: >>>> [safe] >>>> directory = /home/alice/repo.git #changed via script >>> For readability, you probably would want to have a SP before the >>> given string, i.e., >>> variable = "value" # message comes here >> >> Let me interject... Perhaps also a tab character before the "# >> comment", >> instead of a space character. That would result in even better >> readability. > > Depends on your screen width ;-) Ah, screens are pretty wide these days. :) > If you were trying to tell me that SP or no SP is merely a personal > preference with the comment, I think you succeeded in doing so. Huh, that wasn't my intention. IMHO, a space character between "#" and the actual comment is pretty much mandatory.
On 2024-03-11 18:00, Dragan Simic wrote: > On 2024-03-11 17:48, rsbecker@nexbridge.com wrote: >> On Monday, March 11, 2024 12:17 PM, Dragan Simic wrote: >>> Subject: Re: [PATCH v2] config: add --comment option to add a comment >>> >>> On 2024-03-11 13:55, Junio C Hamano wrote: >>>> "Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes: >>>> >>>>> From: Ralph Seichter <github@seichter.de> >>>>> >>>>> Introduce the ability to append comments to modifications made >>>>> using >>>>> git-config. Example usage: >>>>> >>>>> git config --comment "changed via script" \ >>>>> --add safe.directory /home/alice/repo.git >>>>> >>>>> based on the proposed patch, the output produced is: >>>>> >>>>> [safe] >>>>> directory = /home/alice/repo.git #changed via script >>>> >>>> For readability, you probably would want to have a SP before the >>>> given >>>> string, i.e., >>>> >>>> variable = "value" # message comes here >>> >>> Let me interject... Perhaps also a tab character before the "# >>> comment", >> instead of a space character. That would result in even better >>> readability. >> >> Does adding a tab following data change the parse semantics of >> .gitconfig? >> My naïve understanding is that .gitconfig follows a basic rule of >> leading >> tab within a section, followed by text. Is there a formal syntax >> description >> of what valid input is? The value does not need to be quoted, so what >> does >> the following actually resolve to: >> >> (TAB)variable = value(TAB)# comment. >> >> Does variable mean value or value(TAB)? Obviously TABS should be >> correctly >> be interpreted as whitespace to be ignored. However, what about: >> >> (TAB)variable = value(TAB)s(TAB) # comment. >> >> Does that mean value(TAB)s, value(TAB)s(TAB), value s, value s(TAB), >> values? > > It should mean "value(TAB)s", according to git-config(1). Hmm, after having a look at config.c and doing some testing, git-config(1) seems to be wrong when it says that "internal whitespaces within the value are retained verbatim". Here's an example... I placed the following into ~/.gitconfig: [test] blah = huh uh There's a tab character between "huh" and "uh". Though, running git config --get test.blah produces huh uh which contains a space character, not a tab. That's expected according to config.c, but not according to git-config(1). Should we correct the wording in git-config(1) accordingly? >> The definition according to git-config is >> >> "The syntax is fairly flexible and permissive; whitespaces are mostly >> ignored. The # and ; characters begin comments to the end of line, >> blank >> lines are ignored." > > I believe these two quotations from git-config(1) should make it more > clear: > > A line that defines a value can be continued to the next line by > ending > it with a \; the backslash and the end-of-line are stripped. > Leading > whitespaces after name =, the remainder of the line after the first > comment character # or ;, and trailing whitespaces of the line are > discarded unless they are enclosed in double quotes. Internal > whitespaces within the value are retained verbatim. > > The following escape sequences (beside \" and \\) are recognized: > \n > for newline character (NL), \t for horizontal tabulation (HT, TAB) > and > \b for backspace (BS). Other char escape sequences (including octal > escape sequences) are invalid. > > To me, all that indicates that trailing tab characters are stripped, > but... After some testing, I can confirm that any tabs (or spaces, or mixes) between the value and the "#" character do get ignored. >> "Mostly" does not make me comfortable that this is formally allowed or >> disallowed or ignored. I would suggest that this change needs to >> formalize >> the grammar on that documentation page for clarity. > > ... I do agree that it should be clarified further in the man page.
Preface: I am not replying to every part of Junio's email this time around. However, that does not imply I am ignoring them, I simply do not currently have access to the source code. * Junio C Hamano: > For readability, you probably would want to have a SP before the > given string, i.e., > > variable = "value" # message comes here If the user wants a whitespace after the #, they can add it in the comment using quoting, e.g. --comment " message comes here". I don't think it is necessary to enforce the extra SP, because it is not syntactically required. Besides, readability is quite subjective. >> The implementation ensures that a # character is always >> prepended to the provided comment string, and that the > > It is unclear what happens when a user gives comment that already > has the comment introducer, e.g., --comment="# this is comment". Unclear? ;-) I wrote "a # character is always prepended to the provided comment string", and that is what happens. The current implementation is meant to be safe, not fancy. > No need to repeat a wall of text below the three-dash line if they > do not give additional information on top of what is already in the > proposed commit log message. That's GitGitGadget's doing. Unfortunately, it does not want to let me remove my pull request description on the GitHub website. I already tried to fiddle with it, but no success yet; GitHub keeps restoring my latest description text. > This topic may want to at least wait for the conclusion of that > design discussion, and possibly its implementation. I don't know enough about the timespans involved to be sure, but that sounds like it could take a long time? > I notice that we are sending "comment" to the underlying machinery > without doing ANY sanity checking (like, "ah, we got a message > without the '#' prefix, so we should add '# ' in front of it before > asking the API to add that comment string to the value line"). As I wrote before, the # is prepended unconditionally. LF is a more interesting question. I haven't yet tried actively introducing a linefeed. Does strbuf_addf() have any related filtering logic? -Ralph
* Dragan Simic: > Let me interject... Perhaps also a tab character before the "# > comment", instead of a space character. That would result in even > better readability. I'd rather not open /that/ can of worms. Tabs versus spaces, tab size, and so forth, are too much matters of personal taste for me to want to spend any time discussing. -Ralph
On 2024-03-11 19:23, Ralph Seichter wrote: > * Dragan Simic: > >> Let me interject... Perhaps also a tab character before the "# >> comment", instead of a space character. That would result in even >> better readability. > > I'd rather not open /that/ can of worms. Tabs versus spaces, tab size, > and so forth, are too much matters of personal taste for me to want to > spend any time discussing. I see, but please note that everyone should be prepared to spend some time discussing even seemingly unrelated things when contributing to a major open-source project. I mean, perhaps the whole thing with the tab characters may not be the right example, but I just wanted to point out that the more major an open-source project is, the more discussion is often required.
On 2024-03-11 19:16, Ralph Seichter wrote: > * Junio C Hamano: > >> For readability, you probably would want to have a SP before the >> given string, i.e., >> >> variable = "value" # message comes here > > If the user wants a whitespace after the #, they can add it in the > comment using quoting, e.g. --comment " message comes here". I don't > think it is necessary to enforce the extra SP, because it is not > syntactically required. Besides, readability is quite subjective. Perhaps that should be documented, so the users know what to expect and how to ensure extra spacing, if they desire so. >>> The implementation ensures that a # character is always >>> prepended to the provided comment string, and that the >> >> It is unclear what happens when a user gives comment that already >> has the comment introducer, e.g., --comment="# this is comment". > > Unclear? ;-) I wrote "a # character is always prepended to the > provided comment string", and that is what happens. The current > implementation is meant to be safe, not fancy. Perhaps that should also be documented, to avoid the "##" sequences from occurring unexpectedly.
* Dragan Simic: > I mean, perhaps the whole thing with the tab characters may not be > the right example, but I just wanted to point out that the more > major an open-source project is, the more discussion is often > required. Oh, I have no qualms discussing things, but over the last 40+ years, nothing good has ever come from debating the pros and cons of tabs and spaces. At least that's my personal experience. -Ralph
On 2024-03-11 19:57, Ralph Seichter wrote: > * Dragan Simic: > >> I mean, perhaps the whole thing with the tab characters may not be >> the right example, but I just wanted to point out that the more >> major an open-source project is, the more discussion is often >> required. > > Oh, I have no qualms discussing things, but over the last 40+ years, > nothing good has ever come from debating the pros and cons of tabs and > spaces. At least that's my personal experience. There are pros and cons to both spaces and tabs, so there's hardly anything that can be concluded about either option being better or worse than the other. They're both good and bad at the same time. However, I think there should be some way to allow the users to choose the kind of spacing between the automatically added comments and their respective values. Yes, readability may be subjective, but I think that the users should be allowed some kind of choice.
* Dragan Simic: > Perhaps that should be documented, so the users know what to expect > and how to ensure extra spacing, if they desire so. Yup, better to be explicit than implicit. By the way: When it comes to my proposed patch, I am having small difficulties finding a balance between documenting things thoroughly and being accused of producing walls of text. ;-) -Ralph
On Monday, March 11, 2024 2:57 PM, Ralph Seichter wrote: >Subject: Re: [PATCH v2] config: add --comment option to add a comment > >* Dragan Simic: > > > I mean, perhaps the whole thing with the tab characters may not be > the right example, but I just wanted to point out that the >more > major an open-source project is, the more discussion is often > required. > >Oh, I have no qualms discussing things, but over the last 40+ years, nothing good has ever come from debating the pros and cons of >tabs and spaces. At least that's my personal experience. My take would be that all whitespace is ignored, but if you want a tab or other character in a value, be explicit about it: variable = "value\ts" # comment where all whitespace and comments are dropped from the parse to be functionally equivalent to variable=value(TAB)s when processing. Implicit quoting arbitrary whitespace can be interpreted in an ambiguous way. That's basically what the C parser does. --Randall
Dragan Simic <dsimic@manjaro.org> writes: >>> Let me interject... Perhaps also a tab character before the "# >>> comment", >>> instead of a space character. That would result in even better >>> readability. >> Depends on your screen width ;-) > > Ah, screens are pretty wide these days. :) > >> If you were trying to tell me that SP or no SP is merely a personal >> preference with the comment, I think you succeeded in doing so. > > Huh, that wasn't my intention. IMHO, a space character between "#" > and the actual comment is pretty much mandatory. Ah, OK, you were talking about the gap after the value before the "#" that introduces the comment, but I somehow mistook it as a comment about the whitespace after '#'. The gap after the value, I do not have a strong opinion either way between SP and HT, except that I agree there should be something there for readability. Given that other places where we do insert comments, like in the log message editor during "git commit -e", we always give a single space after the comment character, I tend to agree that a space after '#' is pretty much mandatory. It is a non starter to tell users that they should add their own SP at the beginning if they want to use such a common style, i.e. git commit --comment=' here is my message' ;# BAD With a simple rule like "Unless your message begins with '#', the message is prepended by '# ' (pound, followed by a SP), but when your message begins with '#', the string is used as is", those who want to use their own style can use whatever style they want, e.g. git commit --comment='#I do not want SP there' git commit --comment='#^II want a HT there instead' and that would be a much more preferrable design, i.e. making the common things easy, while leaving unusual things possible. Thanks.
Dragan Simic <dsimic@manjaro.org> writes: > However, I think there should be some way to allow the users to choose > the kind of spacing between the automatically added comments and their > respective values. Yes, readability may be subjective, but I think > that the users should be allowed some kind of choice. As to the whitespace _before_ the '#', we can just pick one and leave the choice to users' editor, which they have to resort to anyway for even trivial things like fixing typos. I am fine to defer the choice between HT and SP to Ralph as the person in the driver seat, As to the whitespace _after_ the '#', I would say we have obligation to the users to be consistent with other codepaths that we already write our own comments. "# " is the only sensible choice of the default there, and we can allow other styles as I outlined in a separate message if we wanted to.
On 2024-03-11 20:54, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: > >>>> Let me interject... Perhaps also a tab character before the "# >>>> comment", >>>> instead of a space character. That would result in even better >>>> readability. >>> Depends on your screen width ;-) >> >> Ah, screens are pretty wide these days. :) >> >>> If you were trying to tell me that SP or no SP is merely a personal >>> preference with the comment, I think you succeeded in doing so. >> >> Huh, that wasn't my intention. IMHO, a space character between "#" >> and the actual comment is pretty much mandatory. > > Ah, OK, you were talking about the gap after the value before the > "#" that introduces the comment, but I somehow mistook it as a > comment about the whitespace after '#'. Yes, that's what I was talking about. I'm sorry if the way I wrote it initially wasn't clear enough. > The gap after the value, I do not have a strong opinion either way > between SP and HT, except that I agree there should be something > there for readability. I'd vote for a space character after "#", because that's pretty much the de facto standard. I don't remember seeing tabs used there. > Given that other places where we do insert comments, like in the log > message editor during "git commit -e", we always give a single space > after the comment character, I tend to agree that a space after '#' > is pretty much mandatory. It is a non starter to tell users that > they should add their own SP at the beginning if they want to use > such a common style, i.e. > > git commit --comment=' here is my message' ;# BAD I'd agree with that. Requiring the users to include a leading space would make things inconistent. > With a simple rule like "Unless your message begins with '#', the > message is prepended by '# ' (pound, followed by a SP), but when > your message begins with '#', the string is used as is", those who > want to use their own style can use whatever style they want, e.g. > > git commit --comment='#I do not want SP there' > git commit --comment='#^II want a HT there instead' > > and that would be a much more preferrable design, i.e. making the > common things easy, while leaving unusual things possible. Agreed. That would be nice.
On 2024-03-11 20:17, rsbecker@nexbridge.com wrote: > My take would be that all whitespace is ignored, but if you want a tab > or other character in a value, be explicit about it: > > variable = "value\ts" # comment > > where all whitespace and comments are dropped from the parse to be > functionally equivalent to > > variable=value(TAB)s > > when processing. Implicit quoting arbitrary whitespace can be > interpreted in an ambiguous way. That's basically what the C parser > does. Oh, I fully agree, but I'm afraid that would be a breaking change at this point. Perhaps numerous configuration values in the field already use the current (mis)behavior of the value parser in config.c. I think the only right thing to do at this point is to document it properly and correctly in the git-config(1) manual page. A bit later I'll prepare and send a patch that does it.
On 2024-03-11 22:31, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: > >> However, I think there should be some way to allow the users to choose >> the kind of spacing between the automatically added comments and their >> respective values. Yes, readability may be subjective, but I think >> that the users should be allowed some kind of choice. > > As to the whitespace _before_ the '#', we can just pick one and > leave the choice to users' editor, which they have to resort to > anyway for even trivial things like fixing typos. I am fine to > defer the choice between HT and SP to Ralph as the person in the > driver seat, I'd vote for tabs before hashes (i.e. "name(SP)=(SP)value(HT)#(SP)comment"), because that's what I've seen used in numerous codebases. Thus, that should introduce some kind of additional consistency. > As to the whitespace _after_ the '#', I would say we have obligation > to the users to be consistent with other codepaths that we already > write our own comments. "# " is the only sensible choice of the > default there, and we can allow other styles as I outlined in a > separate message if we wanted to. I'd agree with that, as I already noted in an earlier reply.
* Ralph Seichter: > LF is a more interesting question. I haven't yet tried actively > introducing a linefeed. Does strbuf_addf() have any related > filtering logic? I have now tried several times to inject a LF into a comment string, and did not manage to break the resulting config file. For example, ./git config --comment "foo\012bar" --add section.key value leads to this entry: [section] key = value #foo\012bar Variable expansion with LF="\012" and --comment "foo${LF}bar" did not introduce a linefeed either. Have you guys perhaps managed to create an invalid config file? -Ralph
On Mon, Mar 11, 2024 at 11:22 PM Ralph Seichter <github@seichter.de> wrote: > I have now tried several times to inject a LF into a comment string ... > Variable expansion with LF="\012" ... You must use a literal line feed, e.g.: LF=' ' (a single quoted newline) or, in a shell that supports this syntax: LF=$'\012' (note $ and single quote: double quotes here do not work) to get the line-feed into the shell variable. You can also capture the output of a program that emits the line-feed, but these are the direct assignment methods. For instance: $ LF=$'\012' $ echo "foo${LF}bar" foo bar $ Lacking such capabilities, it's easy enough to use the printf shell command to produce special characters as output (which you can then capture, but various shells may also have Special Rules about such captured output, so the direct assignment method is better, in my opinion). Chris
* Chris Torek: > You must use a literal line feed, e.g.: > > LF=' > ' Of course, silly me. Easily done in a shell script. I was too focused on trying it in an interactive shell. Thanks, Chris. Do you perhaps know of a function in the Git code base which could be used to sanitise strings? It is quite a lot of code to sift through if one is unfamiliar with it, so I'll gladly take hints. -Ralph
Ralph Seichter <github@seichter.de> writes: > * Chris Torek: > >> You must use a literal line feed, e.g.: >> >> LF=' >> ' > > Of course, silly me. Easily done in a shell script. I was too focused on > trying it in an interactive shell. Thanks, Chris. ;-) I think I've already given you that in my first message that mentioned multi-line input. In the test suite, we already have that $LF defined so your tests can use it without defining it yourself. > Do you perhaps know of a function in the Git code base which could be > used to sanitise strings? It is quite a lot of code to sift through if > one is unfamiliar with it, so I'll gladly take hints. Don't silently butcher end-user input; instead do something like this to make it clear that you do not support it. if (strchr(comment_message, '\n')) die(_("multi-line comment not supported: '%s'"), comment_message);
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index dff39093b5e..ee8cd251b24 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -9,9 +9,9 @@ git-config - Get and set repository or global options SYNOPSIS -------- [verse] -'git config' [<file-option>] [--type=<type>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]] -'git config' [<file-option>] [--type=<type>] --add <name> <value> -'git config' [<file-option>] [--type=<type>] [--fixed-value] --replace-all <name> <value> [<value-pattern>] +'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]] +'git config' [<file-option>] [--type=<type>] [--comment=<value>] --add <name> <value> +'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] --replace-all <name> <value> [<value-pattern>] 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get <name> [<value-pattern>] 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all <name> [<value-pattern>] 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp <name-regex> [<value-pattern>] @@ -87,6 +87,10 @@ OPTIONS values. This is the same as providing '^$' as the `value-pattern` in `--replace-all`. +--comment:: + Append a comment to new or modified lines. A '#' character + will be automatically prepended to the value. + --get:: Get the value for a given key (optionally filtered by a regex matching the value). Returns error code 1 if the key was not diff --git a/builtin/config.c b/builtin/config.c index b55bfae7d66..2aab3c0baf3 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -44,6 +44,7 @@ static struct config_options config_options; static int show_origin; static int show_scope; static int fixed_value; +static const char *comment; #define ACTION_GET (1<<0) #define ACTION_GET_ALL (1<<1) @@ -173,6 +174,7 @@ static struct option builtin_config_options[] = { OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")), OPT_BOOL(0, "show-scope", &show_scope, N_("show scope of config (worktree, local, global, system, command)")), OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")), + OPT_STRING(0, "comment", &comment, N_("value"), N_("human-readable comment string (# will be prepended automatically)")), OPT_END(), }; @@ -797,6 +799,11 @@ int cmd_config(int argc, const char **argv, const char *prefix) usage_builtin_config(); } + if (comment && !(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) { + error(_("--comment is only applicable to add/set/replace operations")); + usage_builtin_config(); + } + /* check usage of --fixed-value */ if (fixed_value) { int allowed_usage = 0; @@ -880,7 +887,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) check_write(); check_argc(argc, 2, 2); value = normalize_value(argv[0], argv[1], &default_kvi); - ret = git_config_set_in_file_gently(given_config_source.file, argv[0], value); + ret = git_config_set_in_file_gently(given_config_source.file, argv[0], comment, value); if (ret == CONFIG_NOTHING_SET) error(_("cannot overwrite multiple values with a single value\n" " Use a regexp, --add or --replace-all to change %s."), argv[0]); @@ -891,7 +898,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) value = normalize_value(argv[0], argv[1], &default_kvi); ret = git_config_set_multivar_in_file_gently(given_config_source.file, argv[0], value, argv[2], - flags); + comment, flags); } else if (actions == ACTION_ADD) { check_write(); @@ -900,7 +907,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) ret = git_config_set_multivar_in_file_gently(given_config_source.file, argv[0], value, CONFIG_REGEX_NONE, - flags); + comment, flags); } else if (actions == ACTION_REPLACE_ALL) { check_write(); @@ -908,7 +915,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) value = normalize_value(argv[0], argv[1], &default_kvi); ret = git_config_set_multivar_in_file_gently(given_config_source.file, argv[0], value, argv[2], - flags | CONFIG_FLAGS_MULTI_REPLACE); + comment, flags | CONFIG_FLAGS_MULTI_REPLACE); } else if (actions == ACTION_GET) { check_argc(argc, 1, 2); @@ -936,17 +943,17 @@ int cmd_config(int argc, const char **argv, const char *prefix) if (argc == 2) return git_config_set_multivar_in_file_gently(given_config_source.file, argv[0], NULL, argv[1], - flags); + NULL, flags); else return git_config_set_in_file_gently(given_config_source.file, - argv[0], NULL); + argv[0], NULL, NULL); } else if (actions == ACTION_UNSET_ALL) { check_write(); check_argc(argc, 1, 2); return git_config_set_multivar_in_file_gently(given_config_source.file, argv[0], NULL, argv[1], - flags | CONFIG_FLAGS_MULTI_REPLACE); + NULL, flags | CONFIG_FLAGS_MULTI_REPLACE); } else if (actions == ACTION_RENAME_SECTION) { check_write(); diff --git a/builtin/gc.c b/builtin/gc.c index cb80ced6cb5..342907f7bdb 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1553,7 +1553,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) die(_("$HOME not set")); rc = git_config_set_multivar_in_file_gently( config_file, "maintenance.repo", maintpath, - CONFIG_REGEX_NONE, 0); + CONFIG_REGEX_NONE, NULL, 0); free(global_config_file); if (rc) @@ -1620,7 +1620,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi if (!config_file) die(_("$HOME not set")); rc = git_config_set_multivar_in_file_gently( - config_file, key, NULL, maintpath, + config_file, key, NULL, maintpath, NULL, CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE); free(global_config_file); diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index fda50f2af1e..e4e18adb575 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1283,7 +1283,7 @@ static void sync_submodule(const char *path, const char *prefix, submodule_to_gitdir(&sb, path); strbuf_addstr(&sb, "/config"); - if (git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url)) + if (git_config_set_in_file_gently(sb.buf, remote_key, NULL, sub_origin_url)) die(_("failed to update remote for submodule '%s'"), path); diff --git a/builtin/worktree.c b/builtin/worktree.c index 9c76b62b02d..a20cc8820e5 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -365,12 +365,12 @@ static void copy_filtered_worktree_config(const char *worktree_git_dir) if (!git_configset_get_bool(&cs, "core.bare", &bare) && bare && git_config_set_multivar_in_file_gently( - to_file, "core.bare", NULL, "true", 0)) + to_file, "core.bare", NULL, "true", NULL, 0)) error(_("failed to unset '%s' in '%s'"), "core.bare", to_file); if (!git_configset_get(&cs, "core.worktree") && git_config_set_in_file_gently(to_file, - "core.worktree", NULL)) + "core.worktree", NULL, NULL)) error(_("failed to unset '%s' in '%s'"), "core.worktree", to_file); diff --git a/config.c b/config.c index 3cfeb3d8bd9..a22594eabd9 100644 --- a/config.c +++ b/config.c @@ -3001,6 +3001,7 @@ static ssize_t write_section(int fd, const char *key, } static ssize_t write_pair(int fd, const char *key, const char *value, + const char *comment, const struct config_store_data *store) { int i; @@ -3041,7 +3042,10 @@ static ssize_t write_pair(int fd, const char *key, const char *value, strbuf_addch(&sb, value[i]); break; } - strbuf_addf(&sb, "%s\n", quote); + if (comment) + strbuf_addf(&sb, "%s #%s\n", quote, comment); + else + strbuf_addf(&sb, "%s\n", quote); ret = write_in_full(fd, sb.buf, sb.len); strbuf_release(&sb); @@ -3130,9 +3134,9 @@ static void maybe_remove_section(struct config_store_data *store, } int git_config_set_in_file_gently(const char *config_filename, - const char *key, const char *value) + const char *key, const char *comment, const char *value) { - return git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, 0); + return git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, comment, 0); } void git_config_set_in_file(const char *config_filename, @@ -3153,7 +3157,7 @@ int repo_config_set_worktree_gently(struct repository *r, if (r->repository_format_worktree_config) { char *file = repo_git_path(r, "config.worktree"); int ret = git_config_set_multivar_in_file_gently( - file, key, value, NULL, 0); + file, key, value, NULL, NULL, 0); free(file); return ret; } @@ -3195,6 +3199,7 @@ void git_config_set(const char *key, const char *value) int git_config_set_multivar_in_file_gently(const char *config_filename, const char *key, const char *value, const char *value_pattern, + const char *comment, unsigned flags) { int fd = -1, in_fd = -1; @@ -3245,7 +3250,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, free(store.key); store.key = xstrdup(key); if (write_section(fd, key, &store) < 0 || - write_pair(fd, key, value, &store) < 0) + write_pair(fd, key, value, comment, &store) < 0) goto write_err_out; } else { struct stat st; @@ -3399,7 +3404,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, if (write_section(fd, key, &store) < 0) goto write_err_out; } - if (write_pair(fd, key, value, &store) < 0) + if (write_pair(fd, key, value, comment, &store) < 0) goto write_err_out; } @@ -3444,7 +3449,7 @@ void git_config_set_multivar_in_file(const char *config_filename, const char *value_pattern, unsigned flags) { if (!git_config_set_multivar_in_file_gently(config_filename, key, value, - value_pattern, flags)) + value_pattern, NULL, flags)) return; if (value) die(_("could not set '%s' to '%s'"), key, value); @@ -3467,7 +3472,7 @@ int repo_config_set_multivar_gently(struct repository *r, const char *key, int res = git_config_set_multivar_in_file_gently(file, key, value, value_pattern, - flags); + NULL, flags); free(file); return res; } diff --git a/config.h b/config.h index 5dba984f770..a85a8271696 100644 --- a/config.h +++ b/config.h @@ -290,7 +290,7 @@ int git_config_pathname(const char **, const char *, const char *); int git_config_expiry_date(timestamp_t *, const char *, const char *); int git_config_color(char *, const char *, const char *); -int git_config_set_in_file_gently(const char *, const char *, const char *); +int git_config_set_in_file_gently(const char *, const char *, const char *, const char *); /** * write config values to a specific config file, takes a key/value pair as @@ -336,7 +336,7 @@ int git_config_parse_key(const char *, char **, size_t *); int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned); 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 *, unsigned); +int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, const char *, unsigned); /** * takes four parameters: diff --git a/sequencer.c b/sequencer.c index f49a871ac06..4c91ca5a844 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3460,54 +3460,54 @@ static int save_opts(struct replay_opts *opts) if (opts->no_commit) res |= git_config_set_in_file_gently(opts_file, - "options.no-commit", "true"); + "options.no-commit", NULL, "true"); if (opts->edit >= 0) - res |= git_config_set_in_file_gently(opts_file, "options.edit", + res |= git_config_set_in_file_gently(opts_file, "options.edit", NULL, opts->edit ? "true" : "false"); if (opts->allow_empty) res |= git_config_set_in_file_gently(opts_file, - "options.allow-empty", "true"); + "options.allow-empty", NULL, "true"); if (opts->allow_empty_message) res |= git_config_set_in_file_gently(opts_file, - "options.allow-empty-message", "true"); + "options.allow-empty-message", NULL, "true"); if (opts->keep_redundant_commits) res |= git_config_set_in_file_gently(opts_file, - "options.keep-redundant-commits", "true"); + "options.keep-redundant-commits", NULL, "true"); if (opts->signoff) res |= git_config_set_in_file_gently(opts_file, - "options.signoff", "true"); + "options.signoff", NULL, "true"); if (opts->record_origin) res |= git_config_set_in_file_gently(opts_file, - "options.record-origin", "true"); + "options.record-origin", NULL, "true"); if (opts->allow_ff) res |= git_config_set_in_file_gently(opts_file, - "options.allow-ff", "true"); + "options.allow-ff", NULL, "true"); if (opts->mainline) { struct strbuf buf = STRBUF_INIT; strbuf_addf(&buf, "%d", opts->mainline); res |= git_config_set_in_file_gently(opts_file, - "options.mainline", buf.buf); + "options.mainline", NULL, buf.buf); strbuf_release(&buf); } if (opts->strategy) res |= git_config_set_in_file_gently(opts_file, - "options.strategy", opts->strategy); + "options.strategy", NULL, opts->strategy); if (opts->gpg_sign) res |= git_config_set_in_file_gently(opts_file, - "options.gpg-sign", opts->gpg_sign); + "options.gpg-sign", NULL, opts->gpg_sign); for (size_t i = 0; i < opts->xopts.nr; i++) res |= git_config_set_multivar_in_file_gently(opts_file, "options.strategy-option", - opts->xopts.v[i], "^$", 0); + opts->xopts.v[i], "^$", NULL, 0); if (opts->allow_rerere_auto) res |= git_config_set_in_file_gently(opts_file, - "options.allow-rerere-auto", + "options.allow-rerere-auto", NULL, opts->allow_rerere_auto == RERERE_AUTOUPDATE ? "true" : "false"); if (opts->explicit_cleanup) res |= git_config_set_in_file_gently(opts_file, - "options.default-msg-cleanup", + "options.default-msg-cleanup", NULL, describe_cleanup_mode(opts->default_msg_cleanup)); return res; } diff --git a/submodule-config.c b/submodule-config.c index 54130f6a385..11428b4adad 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -978,7 +978,7 @@ int config_set_in_gitmodules_file_gently(const char *key, const char *value) { int ret; - ret = git_config_set_in_file_gently(GITMODULES_FILE, key, value); + ret = git_config_set_in_file_gently(GITMODULES_FILE, key, NULL, value); if (ret < 0) /* Maybe the user already did that, don't error out here */ warning(_("Could not update .gitmodules entry %s"), key); diff --git a/submodule.c b/submodule.c index 213da79f661..86630932d09 100644 --- a/submodule.c +++ b/submodule.c @@ -2052,7 +2052,7 @@ void submodule_unset_core_worktree(const struct submodule *sub) submodule_name_to_gitdir(&config_path, the_repository, sub->name); strbuf_addstr(&config_path, "/config"); - if (git_config_set_in_file_gently(config_path.buf, "core.worktree", NULL)) + if (git_config_set_in_file_gently(config_path.buf, "core.worktree", NULL, NULL)) warning(_("Could not unset core.worktree setting in submodule '%s'"), sub->path); diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 31c38786870..daddaedd23c 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -69,13 +69,18 @@ test_expect_success 'replace with non-match (actually matching)' ' cat > expect << EOF [section] - penguin = very blue Movie = BadPhysics UPPERCASE = true - penguin = kingpin + penguin = gentoo #Pygoscelis papua + disposition = peckish #find fish [Sections] WhatEver = Second EOF +test_expect_success 'append comments' ' + git config --replace-all --comment="Pygoscelis papua" section.penguin gentoo && + git config --comment="find fish" section.disposition peckish && + test_cmp expect .git/config +' test_expect_success 'non-match result' 'test_cmp expect .git/config' diff --git a/worktree.c b/worktree.c index b02a05a74a3..cf5eea8c931 100644 --- a/worktree.c +++ b/worktree.c @@ -807,9 +807,9 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, static int move_config_setting(const char *key, const char *value, const char *from_file, const char *to_file) { - if (git_config_set_in_file_gently(to_file, key, value)) + if (git_config_set_in_file_gently(to_file, key, NULL, value)) return error(_("unable to set %s in '%s'"), key, to_file); - if (git_config_set_in_file_gently(from_file, key, NULL)) + if (git_config_set_in_file_gently(from_file, key, NULL, NULL)) return error(_("unable to unset %s in '%s'"), key, from_file); return 0; }