[PATCH 0/2] Section renaming can lose content

[PATCH 0/2] Section renaming can lose content

[Alex Vandiver]

I submitted a patch to the testfile on the 8th; what follows is a pass
at solving the underlying bug as well.
 - Alex

[PATCH 1/2] Make section_name_match start on '[', and return the length on success

[Alex Vandiver]

Signed-off-by: Alex Vandiver <alex@chmrr.net>
---
 config.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

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

[PATCH 2/2] After renaming a section, print any trailing variable definitions

[Alex Vandiver]

Signed-off-by: Alex Vandiver <alex@chmrr.net>
---
 config.c               |   22 +++++++++++++++++++---
 t/t1300-repo-config.sh |   22 ++++++++++++++++++++++
 2 files changed, 41 insertions(+), 3 deletions(-)

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

Re: [PATCH 2/2] After renaming a section, print any trailing variable definitions

[Nanako Shiraishi]

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.

    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/

Re: [PATCH 2/2] After renaming a section, print any trailing variable definitions

[Alex Vandiver]

At Fri Jul 24 18:11:05 -0400 2009, Nanako Shiraishi wrote:
> 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

[PATCH] Make git config fail on variables with no section, as documented

[Alex Vandiver]

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(-)

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

Re: [PATCH] Make git config fail on variables with no section, as documented

[Johannes Schindelin]

Hi,

On Sun, 26 Jul 2009, Alex Vandiver wrote:

> 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

Re: [PATCH] Make git config fail on variables with no section, as documented

[Alex Vandiver]

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

Re: [PATCH] Make git config fail on variables with no section, as documented

[Johannes Schindelin]

Hi,

On Sun, 26 Jul 2009, Alex Vandiver wrote:

> 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

Re: [PATCH] Make git config fail on variables with no section, as documented

[Alex Vandiver]

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

Re: [PATCH 2/2] After renaming a section, print any trailing variable definitions

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> 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?

(unknown),

[Nanako Shiraishi]

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(-)

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/

Re: [PATCH 2/2] After renaming a section, print any trailing variable definitions

[Johannes Schindelin]

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

Re: [PATCH 1/2] Make section_name_match start on '[', and return the length on success

[Johannes Schindelin]

Hi,

On Fri, 24 Jul 2009, Alex Vandiver wrote:

> 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] == '[':

> @@ -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

Re: [PATCH 1/2] Make section_name_match start on '[', and return the length on success

[Alex Vandiver]

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

Re: [PATCH 1/2] Make section_name_match start on '[', and return the length on success

[Junio C Hamano]

Alex Vandiver <alex@chmrr.net> writes:

> 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.

> 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?

Re: [PATCH 1/2] Make section_name_match start on '[', and return the length on success

[Johannes Schindelin]

Hi,

On Sat, 25 Jul 2009, Alex Vandiver wrote:

> 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