[PATCH 0/2] Another minor cleanup involving string_lists

[PATCH 0/2] Another minor cleanup involving string_lists

[Michael Haggerty]

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

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

[Michael Haggerty]

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(-)

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

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

[Michael Haggerty]

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(-)

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

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

[Jeff King]

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

> 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.

> 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