{"thread":{"id":"39102","subject":"[PATCH] type_from_string_gently: make sure length matches","startedAt":"2015-04-17T14:52:48Z","lastAt":"2015-04-17T21:11:33Z","messageCount":4,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"259567","messageId":"20150417145247.GA2421@peff.net","threadId":"39102","inReplyTo":null,"subject":"[PATCH] type_from_string_gently: make sure length matches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-17T14:52:48Z","receivedAt":"2015-04-17T14:52:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When commit fe8e3b7 refactored type_from_string to allow\ninput that was not NUL-terminated, it switched to using\nstrncmp instead of strcmp. But this means we check only the\nfirst \"len\" bytes of the strings, and ignore any remaining\nbytes in the object_type_string. We should make sure that it\nis also \"len\" bytes, or else we would accept \"comm\" as\n\"commit\", and so forth.\n\nSigned-off-by: Jeff King \n---\nSince the strings we are matching are literals, we could also record\ntheir sizes in the object_type_strings array and check the length first\nbefore even calling strncmp. I doubt this is a performance hot-spot,\nthough.\n\nYou could also potentially just use strlen(object_type_strings[i]), but\nI'm not sure if compilers will optimize out the strlen in this case,\nsince it is in a loop.\n\n object.c | 3 ++-\n t/t1007-hash-object.sh | 8 ++++++++\n 2 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/object.c b/object.c\nindex 23d6c96..980ac5f 100644\n--- a/object.c\n+++ b/object.c\n@@ -41,7 +41,8 @@ int type_from_string_gently(const char *str, ssize_t len, int gentle)\n \t\tlen = strlen(str);\n \n \tfor (i = 1; i < ARRAY_SIZE(object_type_strings); i++)\n-\t\tif (!strncmp(str, object_type_strings[i], len))\n+\t\tif (!strncmp(str, object_type_strings[i], len) &&\n+\t\t object_type_strings[i][len] == '\\0')\n \t\t\treturn i;\n \n \tif (gentle)\ndiff --git a/t/t1007-hash-object.sh b/t/t1007-hash-object.sh\nindex f83df8e..ebb3a69 100755\n--- a/t/t1007-hash-object.sh\n+++ b/t/t1007-hash-object.sh\n@@ -201,4 +201,12 @@ test_expect_success 'corrupt tag' '\n \ttest_must_fail git hash-object -t tag --stdin writes:\n\n> When commit fe8e3b7 refactored type_from_string to allow\n> input that was not NUL-terminated, it switched to using\n> strncmp instead of strcmp. But this means we check only the\n> first \"len\" bytes of the strings, and ignore any remaining\n> bytes in the object_type_string. We should make sure that it\n> is also \"len\" bytes, or else we would accept \"comm\" as\n> \"commit\", and so forth.\n>\n> Signed-off-by: Jeff King \n> ---\n> Since the strings we are matching are literals, we could also record\n> their sizes in the object_type_strings array and check the length first\n> before even calling strncmp. I doubt this is a performance hot-spot,\n> though.\n>\n> You could also potentially just use strlen(object_type_strings[i]), but\n> I'm not sure if compilers will optimize out the strlen in this case,\n> since it is in a loop.\n\nThat thought crossed my mind while reading your patch. It could\neven make it go faster if we made object_type_strings into an array\nof counted strings (i.e. \"struct { const char *str; int len; }\")\nand then took advantage of the fact that we have lengths of both.\n\n\n object.c | 24 ++++++++++++++----------\n 1 file changed, 14 insertions(+), 10 deletions(-)\n\ndiff --git a/object.c b/object.c\nindex aedac24..51584ea 100644\n--- a/object.c\n+++ b/object.c\n@@ -18,19 +18,22 @@ struct object *get_indexed_object(unsigned int idx)\n \treturn obj_hash[idx];\n }\n \n-static const char *object_type_strings[] = {\n-\tNULL,\t\t/* OBJ_NONE = 0 */\n-\t\"commit\",\t/* OBJ_COMMIT = 1 */\n-\t\"tree\",\t\t/* OBJ_TREE = 2 */\n-\t\"blob\",\t\t/* OBJ_BLOB = 3 */\n-\t\"tag\",\t\t/* OBJ_TAG = 4 */\n+static struct {\n+\tconst char *str;\n+\tint len;\n+} object_type_name[] = {\n+\t{ NULL, 0 }, /* OBJ_NONE = 0 */\n+\t{ \"commit\", 6 }, /* OBJ_COMMIT = 1 */\n+\t{ \"tree\", 4 },\t /* OBJ_TREE = 2 */\n+\t{ \"blob\", 4 }, /* OBJ_BLOB = 3 */\n+\t{ \"tag\", 3 }, /* OBJ_TAG = 4 */\n };\n \n const char *typename(unsigned int type)\n {\n-\tif (type >= ARRAY_SIZE(object_type_strings))\n+\tif (type >= ARRAY_SIZE(object_type_name))\n \t\treturn NULL;\n-\treturn object_type_strings[type];\n+\treturn object_type_name[type].str;\n }\n \n int type_from_string_gently(const char *str, ssize_t len, int gentle)\n@@ -40,8 +43,9 @@ int type_from_string_gently(const char *str, ssize_t len, int gentle)\n \tif (len < 0)\n \t\tlen = strlen(str);\n \n-\tfor (i = 1; i < ARRAY_SIZE(object_type_strings); i++)\n-\t\tif (!strncmp(str, object_type_strings[i], len))\n+\tfor (i = 1; i < ARRAY_SIZE(object_type_name); i++)\n+\t\tif (object_type_name[i].len == len &&\n+\t\t\t !strncmp(str, object_type_name[i].str, len))\n \t\t\treturn i;\n \n \tif (gentle)\n"},{"id":"259599","messageId":"20150417210713.GA7403@peff.net","threadId":"39102","inReplyTo":"xmqqd232pjb0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] type_from_string_gently: make sure length matches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-17T21:07:13Z","receivedAt":"2015-04-17T21:07:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 17, 2015 at 01:54:27PM -0700, Junio C Hamano wrote:\n\n> > Since the strings we are matching are literals, we could also record\n> > their sizes in the object_type_strings array and check the length first\n> > before even calling strncmp. I doubt this is a performance hot-spot,\n> > though.\n> >\n> > You could also potentially just use strlen(object_type_strings[i]), but\n> > I'm not sure if compilers will optimize out the strlen in this case,\n> > since it is in a loop.\n> \n> That thought crossed my mind while reading your patch. It could\n> even make it go faster if we made object_type_strings into an array\n> of counted strings (i.e. \"struct { const char *str; int len; }\")\n> and then took advantage of the fact that we have lengths of both.\n\nRight, that was what I meant.\n\nI'd be surprised if it appreciably speeds things up, but I guess it is\nnot too complicated to do.\n\n> +static struct {\n> +\tconst char *str;\n> +\tint len;\n> +} object_type_name[] = {\n> +\t{ NULL, 0 }, /* OBJ_NONE = 0 */\n> +\t{ \"commit\", 6 }, /* OBJ_COMMIT = 1 */\n> +\t{ \"tree\", 4 },\t /* OBJ_TREE = 2 */\n> +\t{ \"blob\", 4 }, /* OBJ_BLOB = 3 */\n> +\t{ \"tag\", 3 }, /* OBJ_TAG = 4 */\n> };\n\nI had envisioned a macro like:\n\n #define SIZED_STRING(x) { (x), (sizeof(x) - 1) }\n\nthough perhaps that is overkill for such a short list (that we don't\neven expect to change).\n\n-Peff\n"},{"id":"259601","messageId":"xmqqzj66o3y2.fsf@gitster.dls.corp.google.com","threadId":"39102","inReplyTo":"20150417210713.GA7403@peff.net","subject":"Re: [PATCH] type_from_string_gently: make sure length matches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-17T21:11:33Z","receivedAt":"2015-04-17T21:11:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'd be surprised if it appreciably speeds things up, but I guess it is\n> not too complicated to do.\n>\n>> +static struct {\n>> +\tconst char *str;\n>> +\tint len;\n>> +} object_type_name[] = {\n>> +\t{ NULL, 0 }, /* OBJ_NONE = 0 */\n>> +\t{ \"commit\", 6 }, /* OBJ_COMMIT = 1 */\n>> +\t{ \"tree\", 4 },\t /* OBJ_TREE = 2 */\n>> +\t{ \"blob\", 4 }, /* OBJ_BLOB = 3 */\n>> +\t{ \"tag\", 3 }, /* OBJ_TAG = 4 */\n>> };\n>\n> I had envisioned a macro like:\n>\n> #define SIZED_STRING(x) { (x), (sizeof(x) - 1) }\n>\n> though perhaps that is overkill for such a short list (that we don't\n> even expect to change).\n\nSounds good (either way ;-)\n"}]}