{"thread":{"id":"32018","subject":"[PATCH 0/2] Another minor cleanup involving string_lists","startedAt":"2012-11-05T08:41:21Z","lastAt":"2012-11-08T17:38:35Z","messageCount":4,"participants":["Michael Haggerty","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"202535","messageId":"1352104883-21053-1-git-send-email-mhagger@alum.mit.edu","threadId":"32018","inReplyTo":null,"subject":"[PATCH 0/2] Another minor cleanup involving string_lists","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-11-05T08:41:21Z","receivedAt":"2012-11-05T08:41:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Nothing really earthshattering here.  But it's funny how every time I\nlook closely at a site where I think string_lists could be used, I\nfind problems with the old code.  In this case is_absolute_path() is\ncalled with an argument that is not a null-terminated string, which is\nincorrect (though harmless because the function only looks at the\nfirst two bytes of the string).\n\nAnother peculiarity of the (old and new) code is that it rejects\n\"comments\" even in paths taken from the colon-separated environment\nvariable GIT_ALTERNATE_OBJECT_DIRECTORIES.  The fix would be to change\nlink_alt_odb_entries() to take a string_list and let the callers strip\nout comments when appropriate.  But it didn't seem worth the extra\ncode.\n\nMichael Haggerty (2):\n  link_alt_odb_entries(): use string_list_split_in_place()\n  link_alt_odb_entries(): take (char *, len) rather than two pointers\n\n sha1_file.c | 50 ++++++++++++++++++++++----------------------------\n 1 file changed, 22 insertions(+), 28 deletions(-)\n\n-- \n1.8.0\n"},{"id":"202536","messageId":"1352104883-21053-2-git-send-email-mhagger@alum.mit.edu","threadId":"32018","inReplyTo":"1352104883-21053-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/2] link_alt_odb_entries(): use string_list_split_in_place()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-11-05T08:41:22Z","receivedAt":"2012-11-05T08:41:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change link_alt_odb_entry() to take a NUL-terminated string instead of\n(char *, len).  Use string_list_split_in_place() rather than inline\ncode in link_alt_odb_entries().\n\nThis approach saves some code and also avoids the (probably harmless)\nerror of passing a non-NUL-terminated string to is_absolute_path().\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n sha1_file.c | 42 ++++++++++++++++++------------------------\n 1 file changed, 18 insertions(+), 24 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 9152974..c352413 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -7,6 +7,7 @@\n  * creation etc.\n  */\n #include \"cache.h\"\n+#include \"string-list.h\"\n #include \"delta.h\"\n #include \"pack.h\"\n #include \"blob.h\"\n@@ -246,7 +247,7 @@ static int git_open_noatime(const char *name);\n  * SHA1, an extra slash for the first level indirection, and the\n  * terminating NUL.\n  */\n-static int link_alt_odb_entry(const char * entry, int len, const char * relative_base, int depth)\n+static int link_alt_odb_entry(const char *entry, const char *relative_base, int depth)\n {\n \tconst char *objdir = get_object_directory();\n \tstruct alternate_object_database *ent;\n@@ -258,7 +259,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative\n \t\tstrbuf_addstr(&pathbuf, real_path(relative_base));\n \t\tstrbuf_addch(&pathbuf, '/');\n \t}\n-\tstrbuf_add(&pathbuf, entry, len);\n+\tstrbuf_addstr(&pathbuf, entry);\n \n \tnormalize_path_copy(pathbuf.buf, pathbuf.buf);\n \n@@ -319,7 +320,9 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative\n static void link_alt_odb_entries(const char *alt, const char *ep, int sep,\n \t\t\t\t const char *relative_base, int depth)\n {\n-\tconst char *cp, *last;\n+\tstruct string_list entries = STRING_LIST_INIT_NODUP;\n+\tchar *alt_copy;\n+\tint i;\n \n \tif (depth > 5) {\n \t\terror(\"%s: ignoring alternate object stores, nesting too deep.\",\n@@ -327,30 +330,21 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,\n \t\treturn;\n \t}\n \n-\tlast = alt;\n-\twhile (last < ep) {\n-\t\tcp = last;\n-\t\tif (cp < ep && *cp == '#') {\n-\t\t\twhile (cp < ep && *cp != sep)\n-\t\t\t\tcp++;\n-\t\t\tlast = cp + 1;\n+\talt_copy = xmemdupz(alt, ep - alt);\n+\tstring_list_split_in_place(&entries, alt_copy, sep, -1);\n+\tfor (i = 0; i < entries.nr; i++) {\n+\t\tconst char *entry = entries.items[i].string;\n+\t\tif (entry[0] == '\\0' || entry[0] == '#')\n \t\t\tcontinue;\n+\t\tif (!is_absolute_path(entry) && depth) {\n+\t\t\terror(\"%s: ignoring relative alternate object store %s\",\n+\t\t\t\t\trelative_base, entry);\n+\t\t} else {\n+\t\t\tlink_alt_odb_entry(entry, relative_base, depth);\n \t\t}\n-\t\twhile (cp < ep && *cp != sep)\n-\t\t\tcp++;\n-\t\tif (last != cp) {\n-\t\t\tif (!is_absolute_path(last) && depth) {\n-\t\t\t\terror(\"%s: ignoring relative alternate object store %s\",\n-\t\t\t\t\t\trelative_base, last);\n-\t\t\t} else {\n-\t\t\t\tlink_alt_odb_entry(last, cp - last,\n-\t\t\t\t\t\trelative_base, depth);\n-\t\t\t}\n-\t\t}\n-\t\twhile (cp < ep && *cp == sep)\n-\t\t\tcp++;\n-\t\tlast = cp;\n \t}\n+\tstring_list_clear(&entries, 0);\n+\tfree(alt_copy);\n }\n \n void read_info_alternates(const char * relative_base, int depth)\n-- \n1.8.0\n"},{"id":"202537","messageId":"1352104883-21053-3-git-send-email-mhagger@alum.mit.edu","threadId":"32018","inReplyTo":"1352104883-21053-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/2] link_alt_odb_entries(): take (char *, len) rather than two pointers","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-11-05T08:41:23Z","receivedAt":"2012-11-05T08:41:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change link_alt_odb_entries() to take the length of the \"alt\"\nparameter rather than a pointer to the end of the \"alt\" string.  This\nis the more common calling convention and simplifies the code a tiny\nbit.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n sha1_file.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex c352413..40b2329 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -317,7 +317,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base, int\n \treturn 0;\n }\n \n-static void link_alt_odb_entries(const char *alt, const char *ep, int sep,\n+static void link_alt_odb_entries(const char *alt, int len, int sep,\n \t\t\t\t const char *relative_base, int depth)\n {\n \tstruct string_list entries = STRING_LIST_INIT_NODUP;\n@@ -330,7 +330,7 @@ static void link_alt_odb_entries(const char *alt, const char *ep, int sep,\n \t\treturn;\n \t}\n \n-\talt_copy = xmemdupz(alt, ep - alt);\n+\talt_copy = xmemdupz(alt, len);\n \tstring_list_split_in_place(&entries, alt_copy, sep, -1);\n \tfor (i = 0; i < entries.nr; i++) {\n \t\tconst char *entry = entries.items[i].string;\n@@ -371,7 +371,7 @@ void read_info_alternates(const char * relative_base, int depth)\n \tmap = xmmap(NULL, mapsz, PROT_READ, MAP_PRIVATE, fd, 0);\n \tclose(fd);\n \n-\tlink_alt_odb_entries(map, map + mapsz, '\\n', relative_base, depth);\n+\tlink_alt_odb_entries(map, mapsz, '\\n', relative_base, depth);\n \n \tmunmap(map, mapsz);\n }\n@@ -385,7 +385,7 @@ void add_to_alternates_file(const char *reference)\n \tif (commit_lock_file(lock))\n \t\tdie(\"could not close alternates file\");\n \tif (alt_odb_tail)\n-\t\tlink_alt_odb_entries(alt, alt + strlen(alt), '\\n', NULL, 0);\n+\t\tlink_alt_odb_entries(alt, strlen(alt), '\\n', NULL, 0);\n }\n \n void foreach_alt_odb(alt_odb_fn fn, void *cb)\n@@ -409,7 +409,7 @@ void prepare_alt_odb(void)\n \tif (!alt) alt = \"\";\n \n \talt_odb_tail = &alt_odb_list;\n-\tlink_alt_odb_entries(alt, alt + strlen(alt), PATH_SEP, NULL, 0);\n+\tlink_alt_odb_entries(alt, strlen(alt), PATH_SEP, NULL, 0);\n \n \tread_info_alternates(get_object_directory(), 0);\n }\n-- \n1.8.0\n"},{"id":"202656","messageId":"20121108173835.GH15560@sigill.intra.peff.net","threadId":"32018","inReplyTo":"1352104883-21053-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/2] Another minor cleanup involving string_lists","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-08T17:38:35Z","receivedAt":"2012-11-08T17:38:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 05, 2012 at 09:41:21AM +0100, Michael Haggerty wrote:\n\n> Nothing really earthshattering here.  But it's funny how every time I\n> look closely at a site where I think string_lists could be used, I\n> find problems with the old code.  In this case is_absolute_path() is\n> called with an argument that is not a null-terminated string, which is\n> incorrect (though harmless because the function only looks at the\n> first two bytes of the string).\n\nThanks, the new version is much easier on the eyes.\n\n> Another peculiarity of the (old and new) code is that it rejects\n> \"comments\" even in paths taken from the colon-separated environment\n> variable GIT_ALTERNATE_OBJECT_DIRECTORIES.  The fix would be to change\n> link_alt_odb_entries() to take a string_list and let the callers strip\n> out comments when appropriate.  But it didn't seem worth the extra\n> code.\n\nI don't think it's worth worrying about. Given that the entries must be\nabsolute paths anyway, we do not even have to worry about an insane path\nstarting with \"#\".\n\n-Peff\n"}]}