[PATCH] Fix git_config_set() for mean cases

[PATCH] Fix git_config_set() for mean cases

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5883 bytes --]


There were problems when the keys´ case didn´t match, and also when the
section was in the same line as the key.

This patch also adds a test case, so you see that it works now.

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

---

	I shoulda started by writing those tests. This patch is on top
	of my earlier patch, which worked only in the nice cases. It 
	now needs git-config-set, though.

 config.c              |   42 ++++++++++++++----
 t/t1300-config-set.sh |  112 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 144 insertions(+), 10 deletions(-)
 create mode 100644 t/t1300-config-set.sh

applies-to: ae3c1e28a1df80086033825ac68fdf4c6c835eee
e326b690c85a648ffcbbc63e69333a7100a07892
diff --git a/config.c b/config.c
index 3b883b2..9315351 100644
--- a/config.c
+++ b/config.c
@@ -255,7 +255,7 @@ int git_config(config_fn_t fn)
 
 static struct {
 	int baselen;
-	const char* key;
+	char* key;
 	const char* value;
 	off_t offset;
 	enum { START, SECTION_SEEN, SECTION_END_SEEN, KEY_SEEN } state;
@@ -270,10 +270,10 @@ static int store_aux(const char* key, co
 			break;
 		case SECTION_SEEN:
 			if (strncmp(key, store.key, store.baselen+1)) {
-				store.offset = ftell(config_file);
 				store.state = SECTION_END_SEEN;
 				break;
-			}
+			} else
+				store.offset = ftell(config_file);
 		case SECTION_END_SEEN:
 		case START:
 			if (!strcmp(key, store.key)) {
@@ -326,18 +326,21 @@ int git_config_set(const char* key, cons
 		return 2;
 	}
 
+	store.key = strdup(key);
 	for (i = 0; key[i]; i++)
 		if (i != store.baselen && !isalpha(key[i])) {
 			fprintf(stderr, "invalid key: %s\n", key);
+			free(store.key);
 			return 1;
-		}
+		} else
+			store.key[i] = tolower(key[i]);
 
-	store.key = key;
 	store.value = value;
 
 	fd = open(lock_file, O_WRONLY | O_CREAT | O_EXCL, 0666);
 	if (fd < 0) {
 		fprintf(stderr, "could not lock config file\n");
+		free(store.key);
 		return -1;
 	}
 
@@ -349,6 +352,9 @@ int git_config_set(const char* key, cons
 			"#\n"
 			"\n";
 
+		free(store.key);
+		store.key = (char*)key;
+
 		write(fd, contents, sizeof(contents)-1);
 		store_write_section(fd);
 		store_write_pair(fd);
@@ -362,25 +368,41 @@ int git_config_set(const char* key, cons
 
 		if (git_config(store_aux)) {
 			fprintf(stderr, "invalid config file\n");
+			free(store.key);
 			return 3;
 		}
 
+		free(store.key);
+		store.key = (char*)key;
+
 		in_fd = open(config_file, O_RDONLY, 0666);
 		contents = mmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, in_fd, 0);
 		close(in_fd);
 
 		if (store.offset == 0)
-			offset = st.st_size;
-		else {
+			store.offset = offset = st.st_size;
+		else if (store.state != KEY_SEEN) {
+			offset = store.offset;
+		} else {
+			int equal_offset = st.st_size,
+				bracket_offset = st.st_size;
 			for (offset = store.offset-2; offset > 0 
-					&& contents[offset] != '\n'; offset--);
-			offset++;
+					&& contents[offset] != '\n'; offset--)
+				switch (contents[offset]) {
+					case '=': equal_offset = offset; break;
+					case ']': bracket_offset = offset; break;
+				}
+			if (bracket_offset < equal_offset) {
+				contents[bracket_offset+1] = '\n';
+				offset = bracket_offset+2;
+			} else
+				offset++;
 		}
 		write(fd, contents, offset);
 		if (store.state == START)
 			store_write_section(fd);
 		store_write_pair(fd);
-		if (store.offset > offset)
+		if (store.offset < st.st_size)
 			write(fd, contents + store.offset,
 				st.st_size - store.offset);
 		
diff --git a/t/t1300-config-set.sh b/t/t1300-config-set.sh
new file mode 100644
index 0000000..48d4ff1
--- /dev/null
+++ b/t/t1300-config-set.sh
@@ -0,0 +1,112 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Johannes Schindelin
+#
+
+test_description='Test git-config-set in different settings'
+
+. ./test-lib.sh
+
+test -f .git/config && rm .git/config
+
+git-config-set core.penguin "little blue"
+
+cat > expect.1 << EOF
+#
+# This is the config file
+#
+
+[core]
+	penguin = little blue
+EOF
+
+test_expect_success 'initial' 'cmp .git/config expect.1'
+
+git-config-set Core.Movie BadPhysics
+
+cat > expect.2 << EOF
+#
+# This is the config file
+#
+
+[core]
+	penguin = little blue
+	Movie = BadPhysics
+EOF
+
+test_expect_success 'mixed case' 'cmp .git/config expect.2'
+
+git-config-set Cores.WhatEver Second
+
+cat > expect.3 << EOF
+#
+# This is the config file
+#
+
+[core]
+	penguin = little blue
+	Movie = BadPhysics
+[Cores]
+	WhatEver = Second
+EOF
+
+test_expect_success 'similar section' 'cmp .git/config expect.3'
+
+git-config-set CORE.UPPERCASE true
+
+cat > expect.4 << EOF
+#
+# This is the config file
+#
+
+[core]
+	penguin = little blue
+	Movie = BadPhysics
+	UPPERCASE = true
+[Cores]
+	WhatEver = Second
+EOF
+
+test_expect_success 'similar section' 'cmp .git/config expect.4'
+
+cat > .git/config << EOF
+[beta] ; silly comment # another comment
+noIndent= sillyValue ; ´nother silly comment
+
+# empty line
+		; comment
+		haha   ="beta" # last silly comment
+[nextSection] noNewline = ouch
+EOF
+
+git-config-set beta.haha alpha
+
+cat > expect.5 << EOF
+[beta] ; silly comment # another comment
+noIndent= sillyValue ; ´nother silly comment
+
+# empty line
+		; comment
+	haha = alpha
+[nextSection] noNewline = ouch
+EOF
+
+test_expect_success 'really mean test' 'cmp .git/config expect.5'
+
+git-config-set nextsection.nonewline wow
+
+cat > expect.6 << EOF
+[beta] ; silly comment # another comment
+noIndent= sillyValue ; ´nother silly comment
+
+# empty line
+		; comment
+	haha = alpha
+[nextSection]
+	nonewline = wow
+EOF
+
+test_expect_success 'really really mean test' 'cmp .git/config expect.6'
+
+test_done
+
---
0.99.9.GIT

Re: [PATCH] Fix git_config_set() for mean cases

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> There were problems when the keys´ case didn´t match, and also when the
> section was in the same line as the key.
>
> This patch also adds a test case, so you see that it works now.
>
> Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

Why do you spell apostrophe with 0xb4 not ASCII 0x27?  Not that
it matters because I'll apply it with -u flag to git-am to
convert it to UTF-8 in the log message, but I am just curious.

Re: [PATCH] Fix git_config_set() for mean cases

[H. Peter Anvin]

Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> 
>>There were problems when the keys´ case didn´t match, and also when the
>>section was in the same line as the key.
>>
>>This patch also adds a test case, so you see that it works now.
>>
>>Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> 
> 
> Why do you spell apostrophe with 0xb4 not ASCII 0x27?  Not that
> it matters because I'll apply it with -u flag to git-am to
> convert it to UTF-8 in the log message, but I am just curious.
> 

Not to mention the fact that it's just plain wrong -- U+00B4 is accute 
accent, not apostrophe.

	-hpa

Re: [PATCH] Fix git_config_set() for mean cases

[Johannes Schindelin]

Hi,

On Wed, 16 Nov 2005, Junio C Hamano wrote:

> Why do you spell apostrophe with 0xb4 not ASCII 0x27?  Not that
> it matters because I'll apply it with -u flag to git-am to
> convert it to UTF-8 in the log message, but I am just curious.

Efficiency. I often do the patches on my iBook w/ German keyboard. The 
apostrophe 0xb4 (upward accent) is just beside my back space key, i.e. in 
a favorite location for my right little finger.

OTOH, 0x27 (the real apostrophe) is just beside the Enter key, which is 
the only key Apple made too small. Way too small. So I try to avoid that 
key, since I hit it by accident already too often. Furthermore, on the 
German keyboard you have to hold Shift down for the apostrophe, while you 
do not for 0xb4.

Besides, the character is there, so why not use it? ;-)

Ciao,
Dscho

Re: [PATCH] Fix git_config_set() for mean cases

[Matthias Urlichs]

Hi, Junio C Hamano wrote:

> Why do you spell apostrophe with 0xb4 not ASCII 0x27?

Because on German keyboards these two characters are right next to each
other, I'd suspect...

-- 
Matthias Urlichs   |   {M:U} IT Design @ m-u-it.de   |  smurf@smurf.noris.de
Disclaimer: The quote was selected randomly. Really. | http://smurf.noris.de
 - -
"A horrible little boy came up to me and said, `You know in your book
The Martian Chronicles?'  I said, `Yes?'  He said, `You know where you
talk about Deimos rising in the East?'  I said, `Yes?'  He said `No.'
-- So I hit him."
		-- attributed to Ray Bradbury

Re: [PATCH] Fix git_config_set() for mean cases

[Randal L. Schwartz]

>>>>> "Johannes" == Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Johannes> Besides, the character is there, so why not use it? ;-)

It reminds me of when people typed "l" instead of "1" because typewriters
didn't have a "1".

In that manner, it's just plain wrong.

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Re: [PATCH] Fix git_config_set() for mean cases

[Johannes Schindelin]

Hi,

[taking the discussion back to public]

On Thu, 17 Nov 2005, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Also, I felt that a function like git_config_set() was lacking for too 
> > long from git. Maybe git-init-db does not need that flag, but I guess that 
> > we'd want to write config variables comfortably from C at some stage.
> 
> Yes.  But I hesitate to throw in an interface that dismisses the
> multivalued key problem without thinking things through first.

Okay, I gave it some thought. See below for my points.

> BTW, I think this discussion should be thrown back to the
> public.  It went private by accident.

Hereby done.

> Mind rehashing the discussion so far in the open?
> 
> The main points I can think of are:
> 
>  - configuration file format and semantics cleanups (methinks
>    it is premature to talk about implementation and interface
>    before discussing this).
> 
>    - allowing sectionless variables seems to be a bug.  disallow
>      it.

 Agree.

>    - section names are alnum (i.e. they can start with a digit)
>      while variable names are alpha+alnum.  is this what we
>      want?  otherwise how should the rule be changed?

It seems natural to make variables start with alpha, since people are so 
used to it. It would not hurt the parsing to make them possibly start with 
a digit, but I think it's okay to let it be as it is.

>    - multivalued variables.  it is nice to have such, and is
>      easier to read by humans (methinks).  it is not how other
>      systems do .ini format, and the tools can live with single
>      value and parse it into list of values (methinks that is
>      what you are saying).  which way are we going?  does the
>      order matter in such list valued variable in either case?

To recap:

        [proxy]
                command="ssh" for "ssh://kernel.org/"
                command="proxy-command" for kernel.org
                command="myprotocol-command" for "my://"

It should be possible to set only the command for kernel.org while leaving 
the others. I actually implemented a function

	git_config_set_multivar(const char* name, const char* value,
		const char* value_regex);

which you can call like this:

	git_config_set_multivar(
		"proxy.command",
		"\"rsh\" for kernel.org",
		"for kernel.org$");

to change just the second command. You can use that function also to 
unset (i.e. remove) an entry:

	git_config_set_multivar(
		"proxy.command",
		NULL,
		"for \"ssh://kernel.org\"$");

>  - having a way for scripts to query and update the config file
>    programatically is a good thing, both from C level and script
>    level (i do not think there is a disagreement between us on
>    this point).

There is no disagreement.

>    - _if_ we do multivalued variables, what is the interface to
>      append, replace, and remove one of the values?  (we may be
>      able to get away by only supporting "replace the whole
>      thing", in which case the program can do git_config_get()
>      to grab the list, manipulate the list and feed the updated
>      list to git_config_set()).

See above. For clarities sake, I left "git_config_set(name,value)" as 
shortcut to "git_config_set_multivar(name,value,NULL)", i.e. calling the 
multivar version with value_regex==NULL actually reverts to singlevar 
version.

I'll send out the patches in a few minutes (have to get them into shape 
first).

Ciao,
Dscho