threads / patch / 2551

patchFix git_config_set() for mean cases

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

## tl;dr

7 messages between Nov 16, 2005 and Nov 17, 2005. Diffs are folded; open one to read it.

replies: 6people: 5as markdown or json

Johannes Schindelin· Nov 16, 2005, 09:47 UTC · lore

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

Show changes to 2 files +144 −10

config.c, t/t1300-config-set.sh

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
Junio C Hamano· Nov 16, 2005, 20:21 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 6 quoted lines
> 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.

H. Peter Anvin· Nov 16, 2005, 20:52 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 15 quoted lines
> 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
Johannes Schindelin· Nov 16, 2005, 23:19 UTC · re: Junio C Hamano · lore

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

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

Randal L. Schwartz· Nov 17, 2005, 01:40 UTC · re: Johannes Schindelin · lore

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

>>>>> "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!
Matthias Urlichs· Nov 16, 2005, 23:26 UTC · re: Junio C Hamano · lore

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

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
Johannes Schindelin· Nov 17, 2005, 21:10 UTC · lore

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

Hi,
[taking the discussion back to public]
On Thu, 17 Nov 2005, Junio C Hamano wrote:
Show 8 quoted lines
> 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.
Show 10 quoted lines
> 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.

Show 6 quoted lines
>    - 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.
Show 6 quoted lines
>    - _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

← back to recent threads