threads / patch / 32018

patch, 2 partsAnother minor cleanup involving string_lists

Subject: [PATCH 0/2] Another minor cleanup involving string_lists

## tl;dr

4 messages between Nov 5, 2012 and Nov 8, 2012. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Michael Haggerty· Nov 5, 2012, 08:41 UTC · lore

Nothing really earthshattering here. But it's funny how every time I look closely at a site where I think string_lists could be used, I find problems with the old code. In this case is_absolute_path() is called with an argument that is not a null-terminated string, which is incorrect (though harmless because the function only looks at the first two bytes of the string).

Another peculiarity of the (old and new) code is that it rejects "comments" even in paths taken from the colon-separated environment variable GIT_ALTERNATE_OBJECT_DIRECTORIES. The fix would be to change link_alt_odb_entries() to take a string_list and let the callers strip out comments when appropriate. But it didn't seem worth the extra code.

Michael Haggerty (2):
  link_alt_odb_entries(): use string_list_split_in_place()
  link_alt_odb_entries(): take (char *, len) rather than two pointers

 sha1_file.c | 50 ++++++++++++++++++++++----------------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

-- 
1.8.0

Michael Haggerty· Nov 5, 2012, 08:41 UTC · re: Michael Haggerty · lore

[PATCH 1/2] link_alt_odb_entries(): use string_list_split_in_place()

Change link_alt_odb_entry() to take a NUL-terminated string instead of (char *, len). Use string_list_split_in_place() rather than inline code in link_alt_odb_entries().

This approach saves some code and also avoids the (probably harmless) error of passing a non-NUL-terminated string to is_absolute_path().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 sha1_file.c | 42 ++++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 24 deletions(-)

Show changes to sha1_file.c +18 −24

diff --git a/sha1_file.c b/sha1_file.c
index 9152974..c352413 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -7,6 +7,7 @@
  * creation etc.
  */
 #include "cache.h"
+#include "string-list.h"
 #include "delta.h"
 #include "pack.h"
 #include "blob.h"
@@ -246,7 +247,7 @@ static int git_open_noatime(const char *name);
  * SHA1, an extra slash for the first level indirection, and the
  * terminating NUL.
  */
-static int link_alt_odb_entry(const char * entry, int len, const char * relative_base, int depth)
+static int link_alt_odb_entry(const char *entry, const char *relative_base, int depth)
 {
 	const char *objdir = get_object_directory();
 	struct alternate_object_database *ent;
@@ -258,7 +259,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative
 		strbuf_addstr(&pathbuf, real_path(relative_base));
 		strbuf_addch(&pathbuf, '/');
 	}
-	strbuf_add(&pathbuf, entry, len);
+	strbuf_addstr(&pathbuf, entry);
 
 	normalize_path_copy(pathbuf.buf, pathbuf.buf);
 
@@ -319,7 +320,9 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative
 static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
 				 const char *relative_base, int depth)
 {
-	const char *cp, *last;
+	struct string_list entries = STRING_LIST_INIT_NODUP;
+	char *alt_copy;
+	int i;
 
 	if (depth > 5) {
 		error("%s: ignoring alternate object stores, nesting too deep.",
@@ -327,30 +330,21 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
 		return;
 	}
 
-	last = alt;
-	while (last < ep) {
-		cp = last;
-		if (cp < ep && *cp == '#') {
-			while (cp < ep && *cp != sep)
-				cp++;
-			last = cp + 1;
+	alt_copy = xmemdupz(alt, ep - alt);
+	string_list_split_in_place(&entries, alt_copy, sep, -1);
+	for (i = 0; i < entries.nr; i++) {
+		const char *entry = entries.items[i].string;
+		if (entry[0] == '\0' || entry[0] == '#')
 			continue;
+		if (!is_absolute_path(entry) && depth) {
+			error("%s: ignoring relative alternate object store %s",
+					relative_base, entry);
+		} else {
+			link_alt_odb_entry(entry, relative_base, depth);
 		}
-		while (cp < ep && *cp != sep)
-			cp++;
-		if (last != cp) {
-			if (!is_absolute_path(last) && depth) {
-				error("%s: ignoring relative alternate object store %s",
-						relative_base, last);
-			} else {
-				link_alt_odb_entry(last, cp - last,
-						relative_base, depth);
-			}
-		}
-		while (cp < ep && *cp == sep)
-			cp++;
-		last = cp;
 	}
+	string_list_clear(&entries, 0);
+	free(alt_copy);
 }
 
 void read_info_alternates(const char * relative_base, int depth)

-- 
1.8.0

Michael Haggerty· Nov 5, 2012, 08:41 UTC · re: Michael Haggerty · lore

[PATCH 2/2] link_alt_odb_entries(): take (char *, len) rather than two pointers

Change link_alt_odb_entries() to take the length of the "alt" parameter rather than a pointer to the end of the "alt" string. This is the more common calling convention and simplifies the code a tiny bit.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 sha1_file.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Show changes to sha1_file.c +5 −5

diff --git a/sha1_file.c b/sha1_file.c
index c352413..40b2329 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -317,7 +317,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base, int
 	return 0;
 }
 
-static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
+static void link_alt_odb_entries(const char *alt, int len, int sep,
 				 const char *relative_base, int depth)
 {
 	struct string_list entries = STRING_LIST_INIT_NODUP;
@@ -330,7 +330,7 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,
 		return;
 	}
 
-	alt_copy = xmemdupz(alt, ep - alt);
+	alt_copy = xmemdupz(alt, len);
 	string_list_split_in_place(&entries, alt_copy, sep, -1);
 	for (i = 0; i < entries.nr; i++) {
 		const char *entry = entries.items[i].string;
@@ -371,7 +371,7 @@ void read_info_alternates(const char * relative_base, int depth)
 	map = xmmap(NULL, mapsz, PROT_READ, MAP_PRIVATE, fd, 0);
 	close(fd);
 
-	link_alt_odb_entries(map, map + mapsz, '\n', relative_base, depth);
+	link_alt_odb_entries(map, mapsz, '\n', relative_base, depth);
 
 	munmap(map, mapsz);
 }
@@ -385,7 +385,7 @@ void add_to_alternates_file(const char *reference)
 	if (commit_lock_file(lock))
 		die("could not close alternates file");
 	if (alt_odb_tail)
-		link_alt_odb_entries(alt, alt + strlen(alt), '\n', NULL, 0);
+		link_alt_odb_entries(alt, strlen(alt), '\n', NULL, 0);
 }
 
 void foreach_alt_odb(alt_odb_fn fn, void *cb)
@@ -409,7 +409,7 @@ void prepare_alt_odb(void)
 	if (!alt) alt = "";
 
 	alt_odb_tail = &alt_odb_list;
-	link_alt_odb_entries(alt, alt + strlen(alt), PATH_SEP, NULL, 0);
+	link_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);
 
 	read_info_alternates(get_object_directory(), 0);
 }

-- 
1.8.0

Jeff King· Nov 8, 2012, 17:38 UTC · re: Michael Haggerty · lore

Re: [PATCH 0/2] Another minor cleanup involving string_lists

On Mon, Nov 05, 2012 at 09:41:21AM +0100, Michael Haggerty wrote:

Show 6 quoted lines

> Nothing really earthshattering here.  But it's funny how every time I
> look closely at a site where I think string_lists could be used, I
> find problems with the old code.  In this case is_absolute_path() is
> called with an argument that is not a null-terminated string, which is
> incorrect (though harmless because the function only looks at the
> first two bytes of the string).

Thanks, the new version is much easier on the eyes.

Show 6 quoted lines

> Another peculiarity of the (old and new) code is that it rejects
> "comments" even in paths taken from the colon-separated environment
> variable GIT_ALTERNATE_OBJECT_DIRECTORIES.  The fix would be to change
> link_alt_odb_entries() to take a string_list and let the callers strip
> out comments when appropriate.  But it didn't seem worth the extra
> code.

I don't think it's worth worrying about. Given that the entries must be absolute paths anyway, we do not even have to worry about an insane path starting with "#".

-Peff