config.c fixes, take 2

config.c fixes, take 2

[Kristian Høgsberg]

Hi,

Here's a follow-up series that allocates the lock_file on the heap.
Also, git_config_rename_section() did an extra close too so I added
a fix for that in this series.

cheers,
Kristian

[PATCH] Fix config lockfile handling.

[Kristian Høgsberg]

When we commit or roll back the lock file the fd is automatically closed,
so don't do that again.

Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
 config.c |   17 +++--------------
 1 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/config.c b/config.c
index 49d2b42..2e52b17 100644
--- a/config.c
+++ b/config.c
@@ -751,7 +751,7 @@ int git_config_set_multivar(const char* key, const char* value,
 	const char* value_regex, int multi_replace)
 {
 	int i, dot;
-	int fd = -1, in_fd;
+	int fd, in_fd;
 	int ret;
 	char* config_filename;
 	struct lock_file *lock = NULL;
@@ -955,26 +955,15 @@ int git_config_set_multivar(const char* key, const char* value,
 		munmap(contents, contents_sz);
 	}
 
-	if (close(fd) || commit_lock_file(lock) < 0) {
+	if (commit_lock_file(lock) < 0) {
 		fprintf(stderr, "Cannot commit config file!\n");
 		ret = 4;
 		goto out_free;
 	}
 
-	/* fd is closed, so don't try to close it below. */
-	fd = -1;
-	/*
-	 * lock is committed, so don't try to roll it back below.
-	 * NOTE: Since lockfile.c keeps a linked list of all created
-	 * lock_file structures, it isn't safe to free(lock).  It's
-	 * better to just leave it hanging around.
-	 */
-	lock = NULL;
 	ret = 0;
 
 out_free:
-	if (0 <= fd)
-		close(fd);
 	if (lock)
 		rollback_lock_file(lock);
 	free(config_filename);
@@ -1072,7 +1061,7 @@ int git_config_rename_section(const char *old_name, const char *new_name)
 	}
 	fclose(config_file);
  unlock_and_out:
-	if (close(out_fd) || commit_lock_file(lock) < 0)
+	if (commit_lock_file(lock) < 0)
 			ret = error("Cannot commit config file!");
  out:
 	free(config_filename);
-- 
1.5.3.4

[PATCH] Use a strbuf for building up section header and key/value pair strings.

[Kristian Høgsberg]

Avoids horrible 1-byte write(2) calls and cleans up the logic a bit.

Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
 config.c |   91 ++++++++++++++++++++++++++------------------------------------
 1 files changed, 38 insertions(+), 53 deletions(-)

diff --git a/config.c b/config.c
index 2e52b17..4c8e15d 100644
--- a/config.c
+++ b/config.c
@@ -610,46 +610,36 @@ static int write_error(void)
 
 static int store_write_section(int fd, const char* key)
 {
-	const char *dot = strchr(key, '.');
-	int len1 = store.baselen, len2 = -1;
+	const char *dot;
+	int i, success;
+	struct strbuf sb;
 
-	dot = strchr(key, '.');
+	strbuf_init(&sb, 0);
+	dot = memchr(key, '.', store.baselen);
 	if (dot) {
-		int dotlen = dot - key;
-		if (dotlen < len1) {
-			len2 = len1 - dotlen - 1;
-			len1 = dotlen;
+		strbuf_addf(&sb, "[%.*s \"", dot - key, key);
+		for (i = dot - key + 1; i < store.baselen; i++) {
+			if (key[i] == '"')
+				strbuf_addch(&sb, '\\');
+			strbuf_addch(&sb, key[i]);
 		}
+		strbuf_addstr(&sb, "\"]\n");
+	} else {
+		strbuf_addf(&sb, "[%.*s]\n", store.baselen, key);
 	}
 
-	if (write_in_full(fd, "[", 1) != 1 ||
-	    write_in_full(fd, key, len1) != len1)
-		return 0;
-	if (len2 >= 0) {
-		if (write_in_full(fd, " \"", 2) != 2)
-			return 0;
-		while (--len2 >= 0) {
-			unsigned char c = *++dot;
-			if (c == '"')
-				if (write_in_full(fd, "\\", 1) != 1)
-					return 0;
-			if (write_in_full(fd, &c, 1) != 1)
-				return 0;
-		}
-		if (write_in_full(fd, "\"", 1) != 1)
-			return 0;
-	}
-	if (write_in_full(fd, "]\n", 2) != 2)
-		return 0;
+	success = write_in_full(fd, sb.buf, sb.len) == sb.len;
+	strbuf_release(&sb);
 
-	return 1;
+	return success;
 }
 
 static int store_write_pair(int fd, const char* key, const char* value)
 {
-	int i;
-	int length = strlen(key+store.baselen+1);
-	int quote = 0;
+	int i, success;
+	int length = strlen(key + store.baselen + 1);
+	const char *quote = "";
+	struct strbuf sb;
 
 	/*
 	 * Check to see if the value needs to be surrounded with a dq pair.
@@ -659,43 +649,38 @@ static int store_write_pair(int fd, const char* key, const char* value)
 	 * configuration parser.
 	 */
 	if (value[0] == ' ')
-		quote = 1;
+		quote = "\"";
 	for (i = 0; value[i]; i++)
 		if (value[i] == ';' || value[i] == '#')
-			quote = 1;
-	if (i && value[i-1] == ' ')
-		quote = 1;
+			quote = "\"";
+	if (i && value[i - 1] == ' ')
+		quote = "\"";
+
+	strbuf_init(&sb, 0);
+	strbuf_addf(&sb, "\t%.*s = %s", 
+		    length, key + store.baselen + 1, quote);
 
-	if (write_in_full(fd, "\t", 1) != 1 ||
-	    write_in_full(fd, key+store.baselen+1, length) != length ||
-	    write_in_full(fd, " = ", 3) != 3)
-		return 0;
-	if (quote && write_in_full(fd, "\"", 1) != 1)
-		return 0;
 	for (i = 0; value[i]; i++)
 		switch (value[i]) {
 		case '\n':
-			if (write_in_full(fd, "\\n", 2) != 2)
-				return 0;
+			strbuf_addstr(&sb, "\\n");
 			break;
 		case '\t':
-			if (write_in_full(fd, "\\t", 2) != 2)
-				return 0;
+			strbuf_addstr(&sb, "\\t");
 			break;
 		case '"':
 		case '\\':
-			if (write_in_full(fd, "\\", 1) != 1)
-				return 0;
+			strbuf_addch(&sb, '\\');
 		default:
-			if (write_in_full(fd, value+i, 1) != 1)
-				return 0;
+			strbuf_addch(&sb, value[i]);
 			break;
 		}
-	if (quote && write_in_full(fd, "\"", 1) != 1)
-		return 0;
-	if (write_in_full(fd, "\n", 1) != 1)
-		return 0;
-	return 1;
+	strbuf_addf(&sb, "%s\n", quote);
+
+	success = write_in_full(fd, sb.buf, sb.len) == sb.len;
+	strbuf_release(&sb);
+
+	return success;
 }
 
 static ssize_t find_beginning_of_line(const char* contents, size_t size,
-- 
1.5.3.4

Re: [PATCH] Fix config lockfile handling.

[Junio C Hamano]

Kristian Høgsberg <krh@redhat.com> writes:

> When we commit or roll back the lock file the fd is automatically closed,
> so don't do that again.

With your change, we do not check the return status from close(2)
anymore, which means that we may have run out of diskspace without
noticing and renamed the incomplete file into the real place.  Oops?

At least the original code wouldn't have had that problem.

The right fix in the longer term would be to check the return value from
the close(2) in commit_lock_file(), but it currently does not check on
purpose, because the callers may have already closed the fd.

Re: [PATCH] Fix config lockfile handling.

[Kristian Høgsberg]

On Fri, 2007-12-14 at 13:57 -0800, Junio C Hamano wrote:
> Kristian Høgsberg <krh@redhat.com> writes:
> 
> > When we commit or roll back the lock file the fd is automatically closed,
> > so don't do that again.
> 
> With your change, we do not check the return status from close(2)
> anymore, which means that we may have run out of diskspace without
> noticing and renamed the incomplete file into the real place.  Oops?

You're right of course.  Ok, so lets just stick with the current config
file handling for 1.5.4, it's not seriously broken.

Kristian