threads / patch / 7731

patchSimplify calling of CR/LF conversion routines

Subject: [PATCH] Simplify calling of CR/LF conversion routines

## tl;dr

4 messages between Apr 18, 2007 and Apr 22, 2007. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Alex Riesen· Apr 18, 2007, 22:28 UTC · lore

What is the point of having a return value if it is discarded afterwords anyway?

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
It passes the tests.
 builtin-apply.c |   18 +++++++-----------
 cache.h         |    4 ++--
 convert.c       |   46 +++++++++++++++++++++-------------------------
 diff.c          |    4 ++--
 entry.c         |    7 ++-----
 sha1_file.c     |    7 +++----
 6 files changed, 37 insertions(+), 49 deletions(-)
Show changes to 6 files +37 −49

builtin-apply.c, cache.h, convert.c, diff.c, entry.c, sha1_file.c

diff --git a/builtin-apply.c b/builtin-apply.c
index fd92ef7..ccd342c 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1475,8 +1475,8 @@ static int read_old_data(struct stat *st, const char *path, char **buf_p, unsign
 		}
 		close(fd);
 		nsize = got;
-		nbuf = buf;
-		if (convert_to_git(path, &nbuf, &nsize)) {
+		nbuf = convert_to_git(path, buf, &nsize);
+		if (nbuf) {
 			free(buf);
 			*buf_p = nbuf;
 			*alloc_p = nsize;
@@ -2355,9 +2355,8 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned
 
 static int try_create_file(const char *path, unsigned int mode, const char *buf, unsigned long size)
 {
-	int fd, converted;
+	int fd;
 	char *nbuf;
-	unsigned long nsize;
 
 	if (has_symlinks && S_ISLNK(mode))
 		/* Although buf:size is counted string, it also is NUL
@@ -2369,13 +2368,10 @@ static int try_create_file(const char *path, unsigned int mode, const char *buf,
 	if (fd < 0)
 		return -1;
 
-	nsize = size;
-	nbuf = (char *) buf;
-	converted = convert_to_working_tree(path, &nbuf, &nsize);
-	if (converted) {
+	nbuf = convert_to_working_tree(path, buf, &size);
+	if (nbuf)
 		buf = nbuf;
-		size = nsize;
-	}
+
 	while (size) {
 		int written = xwrite(fd, buf, size);
 		if (written < 0)
@@ -2387,7 +2383,7 @@ static int try_create_file(const char *path, unsigned int mode, const char *buf,
 	}
 	if (close(fd) < 0)
 		die("closing file %s: %s", path, strerror(errno));
-	if (converted)
+	if (nbuf)
 		free(nbuf);
 	return 0;
 }
diff --git a/cache.h b/cache.h
index 53c2341..67fd050 100644
--- a/cache.h
+++ b/cache.h
@@ -493,8 +493,8 @@ extern void trace_printf(const char *format, ...);
 extern void trace_argv_printf(const char **argv, int count, const char *format, ...);
 
 /* convert.c */
-extern int convert_to_git(const char *path, char **bufp, unsigned long *sizep);
-extern int convert_to_working_tree(const char *path, char **bufp, unsigned long *sizep);
+extern char *convert_to_git(const char *path, const char *bufp, unsigned long *sizep);
+extern char *convert_to_working_tree(const char *path, const char *bufp, unsigned long *sizep);
 
 /* match-trees.c */
 void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, int);
diff --git a/convert.c b/convert.c
index 898bfe3..b219c39 100644
--- a/convert.c
+++ b/convert.c
@@ -72,7 +72,7 @@ static int is_binary(unsigned long size, struct text_stat *stats)
 	return 0;
 }
 
-int convert_to_git(const char *path, char **bufp, unsigned long *sizep)
+char *convert_to_git(const char *path, const char *bufp, unsigned long *sizep)
 {
 	char *buffer, *nbuf;
 	unsigned long size, nsize;
@@ -84,18 +84,17 @@ int convert_to_git(const char *path, char **bufp, unsigned long *sizep)
 	 * stupid auto-CRLF one.
 	 */
 	if (!auto_crlf)
-		return 0;
+		return NULL;
 
 	size = *sizep;
 	if (!size)
-		return 0;
-	buffer = *bufp;
+		return NULL;
 
-	gather_stats(buffer, size, &stats);
+	gather_stats(bufp, size, &stats);
 
 	/* No CR? Nothing to convert, regardless. */
 	if (!stats.cr)
-		return 0;
+		return NULL;
 
 	/*
 	 * We're currently not going to even try to convert stuff
@@ -103,32 +102,31 @@ int convert_to_git(const char *path, char **bufp, unsigned long *sizep)
 	 * stuff?
 	 */
 	if (stats.cr != stats.crlf)
-		return 0;
+		return NULL;
 
 	/*
 	 * And add some heuristics for binary vs text, of course...
 	 */
 	if (is_binary(size, &stats))
-		return 0;
+		return NULL;
 
 	/*
 	 * Ok, allocate a new buffer, fill it in, and return true
 	 * to let the caller know that we switched buffers on it.
 	 */
 	nsize = size - stats.crlf;
-	nbuf = xmalloc(nsize);
-	*bufp = nbuf;
+	buffer = nbuf = xmalloc(nsize);
 	*sizep = nsize;
 	do {
-		unsigned char c = *buffer++;
+		unsigned char c = *bufp++;
 		if (c != '\r')
 			*nbuf++ = c;
 	} while (--size);
 
-	return 1;
+	return buffer;
 }
 
-int convert_to_working_tree(const char *path, char **bufp, unsigned long *sizep)
+char *convert_to_working_tree(const char *path, const char *bufp, unsigned long *sizep)
 {
 	char *buffer, *nbuf;
 	unsigned long size, nsize;
@@ -141,46 +139,44 @@ int convert_to_working_tree(const char *path, char **bufp, unsigned long *sizep)
 	 * stupid auto-CRLF one.
 	 */
 	if (auto_crlf <= 0)
-		return 0;
+		return NULL;
 
 	size = *sizep;
 	if (!size)
-		return 0;
-	buffer = *bufp;
+		return NULL;
 
-	gather_stats(buffer, size, &stats);
+	gather_stats(bufp, size, &stats);
 
 	/* No LF? Nothing to convert, regardless. */
 	if (!stats.lf)
-		return 0;
+		return NULL;
 
 	/* Was it already in CRLF format? */
 	if (stats.lf == stats.crlf)
-		return 0;
+		return NULL;
 
 	/* If we have any bare CR characters, we're not going to touch it */
 	if (stats.cr != stats.crlf)
-		return 0;
+		return NULL;
 
 	if (is_binary(size, &stats))
-		return 0;
+		return NULL;
 
 	/*
 	 * Ok, allocate a new buffer, fill it in, and return true
 	 * to let the caller know that we switched buffers on it.
 	 */
 	nsize = size + stats.lf - stats.crlf;
-	nbuf = xmalloc(nsize);
-	*bufp = nbuf;
+	buffer = nbuf = xmalloc(nsize);
 	*sizep = nsize;
 	last = 0;
 	do {
-		unsigned char c = *buffer++;
+		unsigned char c = *bufp++;
 		if (c == '\n' && last != '\r')
 			*nbuf++ = '\r';
 		*nbuf++ = c;
 		last = c;
 	} while (--size);
 
-	return 1;
+	return buffer;
 }
diff --git a/diff.c b/diff.c
index fbb79d7..ee72cc6 100644
--- a/diff.c
+++ b/diff.c
@@ -1461,9 +1461,9 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 		/*
 		 * Convert from working tree format to canonical git format
 		 */
-		buf = s->data;
 		size = s->size;
-		if (convert_to_git(s->path, &buf, &size)) {
+		buf = convert_to_git(s->path, s->data, &size);
+		if (buf) {
 			munmap(s->data, s->size);
 			s->should_munmap = 0;
 			s->data = buf;
diff --git a/entry.c b/entry.c
index d72f811..3771209 100644
--- a/entry.c
+++ b/entry.c
@@ -79,7 +79,6 @@ static int write_entry(struct cache_entry *ce, char *path, struct checkout *stat
 	}
 	switch (ntohl(ce->ce_mode) & S_IFMT) {
 		char *buf;
-		unsigned long nsize;
 
 	case S_IFREG:
 		if (to_tempfile) {
@@ -96,12 +95,10 @@ static int write_entry(struct cache_entry *ce, char *path, struct checkout *stat
 		/*
 		 * Convert from git internal format to working tree format
 		 */
-		buf = new;
-		nsize = size;
-		if (convert_to_working_tree(ce->name, &buf, &nsize)) {
+		buf = convert_to_working_tree(ce->name, new, &size);
+		if (buf) {
 			free(new);
 			new = buf;
-			size = nsize;
 		}
 
 		wrote = write_in_full(fd, new, size);
diff --git a/sha1_file.c b/sha1_file.c
index 4304fe9..1978d5f 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2277,10 +2277,9 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
 	 */
 	if ((type == OBJ_BLOB) && S_ISREG(st->st_mode)) {
 		unsigned long nsize = size;
-		char *nbuf = buf;
-		if (convert_to_git(path, &nbuf, &nsize)) {
-			if (size)
-				munmap(buf, size);
+		char *nbuf = convert_to_git(path, buf, &nsize);
+		if (nbuf) {
+			munmap(buf, size);
 			size = nsize;
 			buf = nbuf;
 			re_allocated = 1;
-- 
1.5.1.1.876.ge36f76
Alex Riesen· Apr 18, 2007, 23:03 UTC · re: Alex Riesen · lore

Re: [PATCH] Simplify calling of CR/LF conversion routines

Alex Riesen, Thu, Apr 19, 2007 00:28:27 +0200:
> What is the point of having a return value if it is discarded
> afterwords anyway?
> 
Ignore this. I'll rediff against the "next" branch
Martin Waitz· Apr 22, 2007, 13:45 UTC · re: Alex Riesen · lore

Re: [PATCH] Simplify calling of CR/LF conversion routines

hoi :)
For me (on glibc-2.5) the testsuite fails in t0020-crlf.sh, test 14:
*** glibc detected *** git: free(): invalid next size (fast): 0x081097b8 ***
======= Backtrace: =========
/lib/i686/cmov/libc.so.6[0x401ed843]
/lib/i686/cmov/libc.so.6(__libc_free+0x90)[0x401f0d00]
git[0x8096f66]
git[0x808ca97]
git[0x808cf52]
git[0x80715b2]
git[0x804a957]
git[0x804adac]
/lib/i686/cmov/libc.so.6(__libc_start_main+0xd8)[0x4019b878]
git[0x804a6d1]
======= Memory map: ========
08048000-080ca000 r-xp 00000000 fe:03 856305     /home/martin/src/git/git
080ca000-080cc000 rw-p 00082000 fe:03 856305     /home/martin/src/git/git
080cc000-0812a000 rw-p 080cc000 00:00 0          [heap]
40000000-4001c000 r-xp 00000000 fe:05 1505       /lib/ld-2.5.so
4001c000-4001e000 rw-p 0001b000 fe:05 1505       /lib/ld-2.5.so
4001e000-4001f000 r-xp 4001e000 00:00 0          [vdso]
4001f000-40021000 rw-p 4001f000 00:00 0
40021000-40022000 rw-p 00000000 fe:03 902199     /home/martin/src/git/t/trash/.git/index
40022000-4002c000 r-xp 00000000 fe:05 2937       /lib/libgcc_s.so.1
4002c000-4002d000 rw-p 00009000 fe:05 2937       /lib/libgcc_s.so.1
40035000-40048000 r-xp 00000000 fe:09 98501      /usr/lib/libz.so.1.2.3
40048000-40049000 rw-p 00012000 fe:09 98501      /usr/lib/libz.so.1.2.3
40049000-4016d000 r-xp 00000000 fe:09 114802     /usr/lib/i686/cmov/libcrypto.so.0.9.8
4016d000-40181000 rw-p 00123000 fe:09 114802     /usr/lib/i686/cmov/libcrypto.so.0.9.8
40181000-40186000 rw-p 40181000 00:00 0
40186000-402c1000 r-xp 00000000 fe:05 1983       /lib/i686/cmov/libc-2.5.so
402c1000-402c2000 r--p 0013b000 fe:05 1983       /lib/i686/cmov/libc-2.5.so
402c2000-402c4000 rw-p 0013c000 fe:05 1983       /lib/i686/cmov/libc-2.5.so
402c4000-402c7000 rw-p 402c4000 00:00 0
402c7000-402c9000 r-xp 00000000 fe:05 2194       /lib/i686/cmov/libdl-2.5.so
402c9000-402cb000 rw-p 00001000 fe:05 2194       /lib/i686/cmov/libdl-2.5.so
402cb000-402cc000 rw-p 402cb000 00:00 0
40300000-40321000 rw-p 40300000 00:00 0
40321000-40400000 ---p 40321000 00:00 0
bff3f000-bff54000 rw-p bff3f000 00:00 0          [stack]
* FAIL 14: .gitattributes says two and three are text

bisecting points to ac78e548, but I can't see any obvious problem with this commit. Do you have any idea?

-- 
Martin Waitz
Alex Riesen· Apr 22, 2007, 16:16 UTC · re: Martin Waitz · lore

Re: [PATCH] Simplify calling of CR/LF conversion routines

Martin Waitz, Sun, Apr 22, 2007 15:45:06 +0200:
Show 5 quoted lines
> hoi :)
> 
> For me (on glibc-2.5) the testsuite fails in t0020-crlf.sh, test 14:
> 
> *** glibc detected *** git: free(): invalid next size (fast): 0x081097b8 ***

Yep, already fixed. The patches attached (I forgot which one was the fix for this particular problem).

>From 001f0b86bc4142743eea242649fb188807e03c16 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Sun, 22 Apr 2007 16:03:49 +0200
Subject: [PATCH] Fix crash in t0020 (crlf conversion)

Reallocated wrong size. Noticed on Ubuntu 7.04 probably because it has some malloc diagnostics in libc: "git-read-tree --reset -u HEAD" aborted in the test. Valgrind sped up the debugging greatly: took me 10 minutes.

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 attr.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)
Show changes to attr.c +4 −2
diff --git a/attr.c b/attr.c
index 285e689..a071254 100644
--- a/attr.c
+++ b/attr.c
@@ -300,7 +300,8 @@ static struct attr_stack *read_attr_from_array(const char **list)
 		a = parse_attr_line(line, "[builtin]", ++lineno, 1);
 		if (!a)
 			continue;
-		res->attrs = xrealloc(res->attrs, res->num_matches + 1);
+		res->attrs = xrealloc(res->attrs,
+			sizeof(struct match_attr *) * (res->num_matches + 1));
 		res->attrs[res->num_matches++] = a;
 	}
 	return res;
@@ -324,7 +325,8 @@ static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
 		a = parse_attr_line(buf, path, ++lineno, macro_ok);
 		if (!a)
 			continue;
-		res->attrs = xrealloc(res->attrs, res->num_matches + 1);
+		res->attrs = xrealloc(res->attrs,
+			sizeof(struct match_attr *) * (res->num_matches + 1));
 		res->attrs[res->num_matches++] = a;
 	}
 	fclose(fp);
-- 
1.5.1.1.946.gdb75a



>From e306988bdabf5dec30c945167bfaa5f5f2c16835 Mon Sep 17 00:00:00 2001
From: Alex Riesen <raa.lkml@gmail.com>
Date: Sun, 22 Apr 2007 16:07:19 +0200
Subject: [PATCH] Fix a typo in crlf conversion code

Also, noticed by valgrind: the code caused a read out-of-bounds.
Some comments updated as well (they still reflected old calling
conventions).

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 convert.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/convert.c b/convert.c
index 37239ac..ad106ef 100644
--- a/convert.c
+++ b/convert.c
@@ -115,8 +115,8 @@ static char *crlf_to_git(const char *path, const char *src, unsigned long *sizep
 	}
 
 	/*
-	 * Ok, allocate a new buffer, fill it in, and return true
-	 * to let the caller know that we switched buffers on it.
+	 * Ok, allocate a new buffer, fill it in, and return it
+	 * to let the caller know that we switched buffers.
 	 */
 	nsize = size - stats.crlf;
 	buffer = xmalloc(nsize);
@@ -137,7 +137,7 @@ static char *crlf_to_git(const char *path, const char *src, unsigned long *sizep
 	} else {
 		do {
 			unsigned char c = *src++;
-			if (! (c == '\r' && (1 < size && *buffer == '\n')))
+			if (! (c == '\r' && (1 < size && *src == '\n')))
 				*dst++ = c;
 		} while (--size);
 	}
@@ -180,8 +180,8 @@ static char *crlf_to_worktree(const char *path, const char *src, unsigned long *
 	}
 
 	/*
-	 * Ok, allocate a new buffer, fill it in, and return true
-	 * to let the caller know that we switched buffers on it.
+	 * Ok, allocate a new buffer, fill it in, and return it
+	 * to let the caller know that we switched buffers.
 	 */
 	nsize = size + stats.lf - stats.crlf;
 	buffer = xmalloc(nsize);
-- 
1.5.1.1.946.gdb75a

← back to recent threads