Alex VandiverI submitted a patch to the testfile on the 8th; what follows is a pass at solving the underlying bug as well. - Alex
threads / patch / 20221
patch, 2 partsSection renaming can lose content
Subject: [PATCH 0/2] Section renaming can lose content
Alex VandiverAlex Vandiver[PATCH 1/2] Make section_name_match start on '[', and return the length on success
Signed-off-by: Alex Vandiver <alex@chmrr.net> --- config.c | 19 ++++++++++++++++--- 1 files changed, 16 insertions(+), 3 deletions(-)
Show changes to config.c +16 −3
diff --git a/config.c b/config.c index 04380bb..7d6f6f6 100644 --- a/config.c +++ b/config.c @@ -1194,7 +1194,9 @@ write_err_out: static int section_name_match (const char *buf, const char *name) { int i = 0, j = 0, dot = 0; - for (; buf[i] && buf[i] != ']'; i++) { + if (buf[i] != '[') + return 0; + for (i = 1; buf[i] && buf[i] != ']'; i++) { if (!dot && isspace(buf[i])) { dot = 1; if (name[j++] != '.') @@ -1215,7 +1217,17 @@ static int section_name_match (const char *buf, const char *name) if (buf[i] != name[j++]) break; } - return (buf[i] == ']' && name[j] == 0); + if (buf[i] == ']' && name[j] == 0) { + /* + * We match, now just find the right length offset by + * gobbling up any whitespace after it, as well + */ + i++; + for (; buf[i] && isspace(buf[i]); i++) + ; /* do nothing */ + return i; + } + return 0; } /* if new_name == NULL, the section is removed instead */ @@ -1249,7 +1261,8 @@ int git_config_rename_section(const char *old_name, const char *new_name) ; /* do nothing */ if (buf[i] == '[') { /* it's a section */ - if (section_name_match (&buf[i+1], old_name)) { + int offset = section_name_match (&buf[i], old_name); + if (offset > 0) { ret++; if (new_name == NULL) { remove = 1;
-- 1.6.3.3.473.gb74fc4.dirty
Alex Vandiver[PATCH 2/2] After renaming a section, print any trailing variable definitions
Signed-off-by: Alex Vandiver <alex@chmrr.net> --- config.c | 22 +++++++++++++++++++--- t/t1300-repo-config.sh | 22 ++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-)
Show changes to 2 files +41 −3
config.c, t/t1300-repo-config.sh
diff --git a/config.c b/config.c index 7d6f6f6..3a2965e 100644 --- a/config.c +++ b/config.c @@ -1257,6 +1257,7 @@ int git_config_rename_section(const char *old_name, const char *new_name) while (fgets(buf, sizeof(buf), config_file)) { int i; int length; + char* output = buf; for (i = 0; buf[i] && isspace(buf[i]); i++) ; /* do nothing */ if (buf[i] == '[') { @@ -1273,14 +1274,29 @@ int git_config_rename_section(const char *old_name, const char *new_name) ret = write_error(lock->filename); goto out; } - continue; + /* + * We wrote out the new section, with + * a newline, now skip the old + * section's length + */ + output += offset + i; + if (strlen(output) > 0) { + /* + * More content means there's + * a declaration to put on the + * next line; indent with a + * tab + */ + output -= 1; + output[0] = '\t'; + } } remove = 0; } if (remove) continue; - length = strlen(buf); - if (write_in_full(out_fd, buf, length) != length) { + length = strlen(output); + if (write_in_full(out_fd, output, length) != length) { ret = write_error(lock->filename); goto out; } diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh index 43ea283..8c43dcd 100755 --- a/t/t1300-repo-config.sh +++ b/t/t1300-repo-config.sh @@ -460,6 +460,28 @@ EOF test_expect_success "rename succeeded" "test_cmp expect .git/config" cat >> .git/config << EOF +[branch "vier"] z = 1 +EOF + +test_expect_success "rename a section with a var on the same line" \ + 'git config --rename-section branch.vier branch.zwei' + +cat > expect << EOF +# Hallo + #Bello +[branch "zwei"] + x = 1 +[branch "zwei"] + y = 1 +[branch "drei"] +weird +[branch "zwei"] + z = 1 +EOF + +test_expect_success "rename succeeded" "test_cmp expect .git/config" + +cat >> .git/config << EOF [branch "zwei"] a = 1 [branch "vier"] EOF
-- 1.6.3.3.473.gb74fc4.dirty
Nanako ShiraishiRe: [PATCH 2/2] After renaming a section, print any trailing variable definitions
Quoting Alex Vandiver <alex@chmrr.net>
Show 10 quoted lines
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh > index 43ea283..8c43dcd 100755 > --- a/t/t1300-repo-config.sh > +++ b/t/t1300-repo-config.sh > @@ -460,6 +460,28 @@ EOF > test_expect_success "rename succeeded" "test_cmp expect .git/config" > > cat >> .git/config << EOF > +[branch "vier"] z = 1 > +EOF
Isn't this a syntax error?
Documentation/config.txt says this.
Subsection names are case sensitive and can contain any characters
except newline (doublequote `"` and backslash have to be escaped as
`\"` and `\\`, respectively). Section headers cannot span multiple
lines. Variables may belong directly to a section or to a given
subsection. You can have `[section]` if you have `[section
"subsection"]`, but you don't need to. There is also a case insensitive alternative `[section.subsection]`
syntax. In this syntax, subsection names follow the same restrictions
as for section names. All the other lines are recognized as setting variables, in the form
'name = value'. If there is no equal sign on the line, the entire
line ...I read "All the other lines" to mean that the section headers and variable definitions are supposed to be on different lines.
-- Nanako Shiraishi http://ivory.ap.teacup.com/nanako3/
Alex VandiverRe: [PATCH 2/2] After renaming a section, print any trailing variable definitions
At Fri Jul 24 18:11:05 -0400 2009, Nanako Shiraishi wrote:
Show 13 quoted lines
> Quoting Alex Vandiver <alex@chmrr.net> > > diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh > > index 43ea283..8c43dcd 100755 > > --- a/t/t1300-repo-config.sh > > +++ b/t/t1300-repo-config.sh > > @@ -460,6 +460,28 @@ EOF > > test_expect_success "rename succeeded" "test_cmp expect .git/config" > > > > cat >> .git/config << EOF > > +[branch "vier"] z = 1 > > +EOF > > Isn't this a syntax error?
Nope -- at least, not according to both the code, and the tests (search or noNewline in t/t1300-repo-config.sh).
Though I also note that the documentation disagrees with the code in the following case:
Each variable must belong to some section, which means that there
must be a section header before the first setting of a variable.$ cat >bogus foo = 42
$ git config --file bogus --list foo=42
$ git config --file bogus --get foo 42
- Alex
-- Networking -- only one letter away from not working
Alex Vandiver[PATCH] Make git config fail on variables with no section, as documented
Documentation/config.txt claims:
Each variable must belong to some section, which means that there
must be a section header before the first setting of a variable.However, the parsing code did not enforce this. This change makes it a syntax error to defined a variable before the first section header.
Signed-off-by: Alex Vandiver <alex@chmrr.net> --- config.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
Show changes to config.c +2 −0
diff --git a/config.c b/config.c index 1b3823d..447ad00 100644 --- a/config.c +++ b/config.c @@ -248,6 +248,8 @@ static int git_parse_file(config_fn_t fn, void *data) } if (!isalpha(c)) break; + if (baselen == 0) + break; var[baselen] = tolower(c); if (get_value(fn, data, var, baselen+1) < 0) break;
-- 1.6.3.3.473.gb74fc4.dirty
Johannes SchindelinRe: [PATCH] Make git config fail on variables with no section, as documented
Hi,
On Sun, 26 Jul 2009, Alex Vandiver wrote:
Show 7 quoted lines
> Documentation/config.txt claims: > > Each variable must belong to some section, which means that there > must be a section header before the first setting of a variable. > > However, the parsing code did not enforce this. This change makes it > a syntax error to defined a variable before the first section header.
Is there any downside in allowing this?
Ciao, Dscho
Alex VandiverRe: [PATCH] Make git config fail on variables with no section, as documented
At Sun Jul 26 12:49:28 -0400 2009, Johannes Schindelin wrote:
> Is there any downside in allowing this?
Not explicitly. However, there are no legal current uses of it, and allowing it might encourage extensions to use the top-level config namespace. It also has the odd property that it _must_ be at the top of a configuration file -- unlike all other configuration options, you can never return to the section to add more variable definitions later. - Alex
-- Networking -- only one letter away from not working
Johannes SchindelinRe: [PATCH] Make git config fail on variables with no section, as documented
Hi,
On Sun, 26 Jul 2009, Alex Vandiver wrote:
Show 6 quoted lines
> At Sun Jul 26 12:49:28 -0400 2009, Johannes Schindelin wrote: > > Is there any downside in allowing this? > > Not explicitly. However, there are no legal current uses of it, and > allowing it might encourage extensions to use the top-level config > namespace.
So?
> It also has the odd property that it _must_ be at the top of a > configuration file -- unlike all other configuration options, you can > never return to the section to add more variable definitions later.
Yes, that is a special property that you might actually want in some contexts.
That, together with the fact that "git config -f <file>" was meant _explicitely_ to allow 3rd party porcelains having their own config files without having to implement their own "git config" lets me suspect that we'd rather want the current behavior.
Ciao, Dscho
Alex VandiverRe: [PATCH] Make git config fail on variables with no section, as documented
At Sun Jul 26 15:48:20 -0400 2009, Johannes Schindelin wrote:
> That, together with the fact that "git config -f <file>" was meant > _explicitely_ to allow 3rd party porcelains having their own config files > without having to implement their own "git config" lets me suspect that > we'd rather want the current behavior.
*shrug* Note I'm not wholly convinced of the rationale myself -- I'm merely trying to reconcile the documentation and reality. I'll also note that you can't, at current, set or unset such values from the command-line.
Looking back at the blame for the documentation, it looks like e136f33 in 2007 is what added the claim that section-less variables weren't acceptable. The `git repo-config` of the time parsed them just fine, however.
If you wish to correct the documentation instead, I can send in a doc patch. Fixing --set and --unset to work with section-less variables will take a bit more work, however. - Alex
-- Networking -- only one letter away from not working
Junio C HamanoRe: [PATCH 2/2] After renaming a section, print any trailing variable definitions
Nanako Shiraishi <nanako3@lavabit.com> writes:
Show 16 quoted lines
> Quoting Alex Vandiver <alex@chmrr.net> > >> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh >> index 43ea283..8c43dcd 100755 >> --- a/t/t1300-repo-config.sh >> +++ b/t/t1300-repo-config.sh >> @@ -460,6 +460,28 @@ EOF >> test_expect_success "rename succeeded" "test_cmp expect .git/config" >> >> cat >> .git/config << EOF >> +[branch "vier"] z = 1 >> +EOF > > Isn't this a syntax error? > > Documentation/config.txt says this.
Even if it were, I think it would be nice to allow it. I'll have to re-read Alex's patch, but I thought it was sane. Perhaps we can update the documentation, mildly hinting that it is allowed without encouraging it too strongly, as I think it is a good style to have these on separate lines.
Dscho? Have any suggestions/comments on the patch?
Nanako Shiraishi(unknown)
Subject: [PATCH] Documentation/config.txt: a variable can be defined on the section header line
Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com> ---
Quoting Junio C Hamano <gitster@pobox.com>:
> Nanako Shiraishi <nanako3@lavabit.com> writes: > >> Isn't this a syntax error? >> >> Documentation/config.txt says this. > > Even if it were, I think it would be nice to allow it. I'll have to > re-read Alex's patch, but I thought it was sane. Perhaps we can update > the documentation, mildly hinting that it is allowed without encouraging > it too strongly, as I think it is a good style to have these on separate > lines. How about this small update to the documentation, then?
Documentation/config.txt | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
Show changes to Documentation/config.txt +2 −1
diff --git a/Documentation/config.txt b/Documentation/config.txt index 6857d2f..c6f09f8 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -49,7 +49,8 @@ There is also a case insensitive alternative `[section.subsection]` syntax. In this syntax, subsection names follow the same restrictions as for section names. -All the other lines are recognized as setting variables, in the form +All the other lines (and the remainder of the line after the section +header) are recognized as setting variables, in the form 'name = value'. If there is no equal sign on the line, the entire line is taken as 'name' and the variable is recognized as boolean "true". The variable names are case-insensitive and only alphanumeric
-- Nanako Shiraishi http://ivory.ap.teacup.com/nanako3/
Johannes SchindelinRe: [PATCH 2/2] After renaming a section, print any trailing variable definitions
Hi,
On Fri, 24 Jul 2009, Junio C Hamano wrote:
> Dscho? Have any suggestions/comments on the patch?
Thanks for making me aware of the patches. I just sent out a reply.
Ciao, Dscho
Johannes SchindelinRe: [PATCH 1/2] Make section_name_match start on '[', and return the length on success
Hi,
On Fri, 24 Jul 2009, Alex Vandiver wrote:
Show 11 quoted lines
> diff --git a/config.c b/config.c
> index 04380bb..7d6f6f6 100644
> --- a/config.c
> +++ b/config.c
> @@ -1194,7 +1194,9 @@ write_err_out:
> static int section_name_match (const char *buf, const char *name)
> {
> int i = 0, j = 0, dot = 0;
> - for (; buf[i] && buf[i] != ']'; i++) {
> + if (buf[i] != '[')
> + return 0;Is this not unnecessary, given that we only call that function when we know that buf[0] == '[':
Show 10 quoted lines
> @@ -1249,7 +1261,8 @@ int git_config_rename_section(const char *old_name, const char *new_name)
> ; /* do nothing */
> if (buf[i] == '[') {
> /* it's a section */
> - if (section_name_match (&buf[i+1], old_name)) {
> + int offset = section_name_match (&buf[i], old_name);
> + if (offset > 0) {
> ret++;
> if (new_name == NULL) {
> remove = 1;I was a bit surprised that "offset" is not used further in your patch, but I saw that 2/2 uses it.
So except for the unnecessary test, I like your patches (read: ACK).
Ciao, Dscho
Alex VandiverRe: [PATCH 1/2] Make section_name_match start on '[', and return the length on success
At Sat Jul 25 10:09:56 -0400 2009, Johannes Schindelin wrote:
> Is this not unnecessary, given that we only call that function when we > know that buf[0] == '[':
Yes. However, given that I had changed the calling convention for the code, (it used to be passed the string starting just _inside_ the '['), I wanted to make the new calling convention clearer, and catch any places that were using the old convention.
I'm happy to submit a new version without it, if you wish.
> I was a bit surprised that "offset" is not used further in your patch, but > I saw that 2/2 uses it.
Yeah, this hunk should probably have gone in 2/2 instead. - Alex
-- Networking -- only one letter away from not working
Junio C HamanoRe: [PATCH 1/2] Make section_name_match start on '[', and return the length on success
Alex Vandiver <alex@chmrr.net> writes:
Show 8 quoted lines
> At Sat Jul 25 10:09:56 -0400 2009, Johannes Schindelin wrote: >> Is this not unnecessary, given that we only call that function when we >> know that buf[0] == '[': > > Yes. However, given that I had changed the calling convention for the > code, (it used to be passed the string starting just _inside_ the > '['), I wanted to make the new calling convention clearer, and catch > any places that were using the old convention.
It's Ok. I do not think this is performance critical part of the system, and I'd feel safer with a bit of defensive programming like this, especially because "git config" that writes (reader is Ok) has traditionally been one of the most fragile part of the system.
Show 6 quoted lines
> I'm happy to submit a new version without it, if you wish. > >> I was a bit surprised that "offset" is not used further in your patch, but >> I saw that 2/2 uses it. > > Yeah, this hunk should probably have gone in 2/2 instead.
I actually thought about suggesting to squash these two patches into one, as the change in [1/2] only makes sense in the context of the [2/2], but decided against it.
I haven't applied (actually, I didn't even notice until this morning) the small documentation update from Nana. Have any comments on that one?
Johannes SchindelinRe: [PATCH 1/2] Make section_name_match start on '[', and return the length on success
Hi,
On Sat, 25 Jul 2009, Alex Vandiver wrote:
Show 15 quoted lines
> At Sat Jul 25 10:09:56 -0400 2009, Johannes Schindelin wrote: > > Is this not unnecessary, given that we only call that function when we > > know that buf[0] == '[': > > Yes. However, given that I had changed the calling convention for the > code, (it used to be passed the string starting just _inside_ the > '['), I wanted to make the new calling convention clearer, and catch > any places that were using the old convention. > > I'm happy to submit a new version without it, if you wish. > > > I was a bit surprised that "offset" is not used further in your patch, but > > I saw that 2/2 uses it. > > Yeah, this hunk should probably have gone in 2/2 instead.
No need to change anything, Junio already applied your patches.
Ciao, Dscho