{"thread":{"id":"36935","subject":"[PATCH v2 0/3] add strnncmp() function","startedAt":"2014-06-17T07:34:36Z","lastAt":"2014-06-18T10:33:31Z","messageCount":15,"participants":["Jeremiah Mahler","Torsten Bögershausen","Erik Faye-Lund","Jonathan Nieder","Junio C Hamano","Ondřej Bílka"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"244380","messageId":"cover.1402990051.git.jmmahler@gmail.com","threadId":"36935","inReplyTo":null,"subject":"[PATCH v2 0/3] add strnncmp() function","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-17T07:34:36Z","receivedAt":"2014-06-17T07:34:36Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Add a strnncmp() function which behaves like strncmp() except it takes\nthe length of both strings instead of just one.\n\nThen simplify tree-walk.c and unpack-trees.c using this new function.\nReplace all occurrences of name_compare() with strnncmp(). Remove\nname_compare(), which they both had identical copies of.\n\nVersion 2 includes suggestions from Jonathan Neider [1]:\n\n - Fix the logic which caused the new strnncmp() to behave differently\n\tfrom the old version. Now it is identical to strncmp().\n\n - Improve description of strnncmp().\n\nAlso, strnncmp() was switched from using memcmp() to strncmp()\ninternally to make it clear that this is meant for strings, not\ngeneral buffers.\n\n[1]: http://marc.info/?l=git&m=140294981320743&w=2\n\nJeremiah Mahler (3):\n add strnncmp() function\n tree-walk: simplify via strnncmp()\n unpack-trees: simplify via strnncmp()\n\n strbuf.c | 9 +++++++++\n strbuf.h | 2 ++\n tree-walk.c | 16 +++-------------\n unpack-trees.c | 13 +------------\n 4 files changed, 15 insertions(+), 25 deletions(-)\n\n-- \n2.0.0.695.g38ee9a9\n"},{"id":"244381","messageId":"50de63f47ded2337adcd8bce151190fb99b38d64.1402990051.git.jmmahler@gmail.com","threadId":"36935","inReplyTo":"cover.1402990051.git.jmmahler@gmail.com","subject":"[PATCH v2 1/3] add strnncmp() function","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-17T07:34:37Z","receivedAt":"2014-06-17T07:34:37Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Add a strnncmp() function which behaves like strncmp() except it takes\nthe length of both strings instead of just one. It behaves the same as\nstrncmp() up to the minimum common length between the strings. When the\nstrings are identical up to this minimum common length, the length\ndifference is returned.\n\nSigned-off-by: Jeremiah Mahler \n---\n strbuf.c | 9 +++++++++\n strbuf.h | 2 ++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex ac62982..4eb7954 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -600,3 +600,12 @@ char *xstrdup_tolower(const char *string)\n \tresult[i] = '\\0';\n \treturn result;\n }\n+\n+int strnncmp(const char *a, int len_a, const char *b, int len_b)\n+{\n+\tint min_len = (len_a < len_b) ? len_a : len_b;\n+\tint cmp = strncmp(a, b, min_len);\n+\tif (cmp)\n+\t\treturn cmp;\n+\treturn (len_a - len_b);\n+}\ndiff --git a/strbuf.h b/strbuf.h\nindex e9ad03e..88af9bf 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -187,4 +187,6 @@ extern int fprintf_ln(FILE *fp, const char *fmt, ...);\n \n char *xstrdup_tolower(const char *);\n \n+extern int strnncmp(const char *a, int len_a, const char *b, int len_b);\n+\n #endif /* STRBUF_H */\n-- \n2.0.0.695.g38ee9a9\n"},{"id":"244382","messageId":"dc427309f0c47c87f4783a3197aa6508f611bcb9.1402990051.git.jmmahler@gmail.com","threadId":"36935","inReplyTo":"cover.1402990051.git.jmmahler@gmail.com","subject":"[PATCH v2 2/3] tree-walk: simplify via strnncmp()","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-17T07:34:38Z","receivedAt":"2014-06-17T07:34:38Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Simplify tree-walk.c using the strnncmp() function and remove the\nname_compare() function.\n\nSigned-off-by: Jeremiah Mahler \n---\n tree-walk.c | 16 +++-------------\n 1 file changed, 3 insertions(+), 13 deletions(-)\n\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 4dc86c7..efbd3b7 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -144,16 +144,6 @@ struct tree_desc_x {\n \tstruct tree_desc_skip *skip;\n };\n \n-static int name_compare(const char *a, int a_len,\n-\t\t\tconst char *b, int b_len)\n-{\n-\tint len = (a_len < b_len) ? a_len : b_len;\n-\tint cmp = memcmp(a, b, len);\n-\tif (cmp)\n-\t\treturn cmp;\n-\treturn (a_len - b_len);\n-}\n-\n static int check_entry_match(const char *a, int a_len, const char *b, int b_len)\n {\n \t/*\n@@ -174,7 +164,7 @@ static int check_entry_match(const char *a, int a_len, const char *b, int b_len)\n \t * scanning further.\n \t */\n \n-\tint cmp = name_compare(a, a_len, b, b_len);\n+\tint cmp = strnncmp(a, a_len, b, b_len);\n \n \t/* Most common case first -- reading sync'd trees */\n \tif (!cmp)\n@@ -369,7 +359,7 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \t\t\t\tfirst_len = len;\n \t\t\t\tcontinue;\n \t\t\t}\n-\t\t\tif (name_compare(e->path, len, first, first_len) < 0) {\n+\t\t\tif (strnncmp(e->path, len, first, first_len) < 0) {\n \t\t\t\tfirst = e->path;\n \t\t\t\tfirst_len = len;\n \t\t\t}\n@@ -383,7 +373,7 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \t\t\t\tif (!e->path)\n \t\t\t\t\tcontinue;\n \t\t\t\tlen = tree_entry_len(e);\n-\t\t\t\tif (name_compare(e->path, len, first, first_len))\n+\t\t\t\tif (strnncmp(e->path, len, first, first_len))\n \t\t\t\t\tentry_clear(e);\n \t\t\t}\n \t\t}\n-- \n2.0.0.695.g38ee9a9\n"},{"id":"244383","messageId":"38ee9a9fac6aa861d18d6f756f48dc9df1ac19b1.1402990051.git.jmmahler@gmail.com","threadId":"36935","inReplyTo":"cover.1402990051.git.jmmahler@gmail.com","subject":"[PATCH v2 3/3] unpack-trees: simplify via strnncmp()","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-17T07:34:39Z","receivedAt":"2014-06-17T07:34:39Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Simplify unpack-trees.c using the strnncmp() function and remove the\nname_compare() function.\n\nSigned-off-by: Jeremiah Mahler \n---\n unpack-trees.c | 13 +------------\n 1 file changed, 1 insertion(+), 12 deletions(-)\n\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 4a9cdf2..9a71b5a 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -629,17 +629,6 @@ static int unpack_failed(struct unpack_trees_options *o, const char *message)\n \treturn -1;\n }\n \n-/* NEEDSWORK: give this a better name and share with tree-walk.c */\n-static int name_compare(const char *a, int a_len,\n-\t\t\tconst char *b, int b_len)\n-{\n-\tint len = (a_len < b_len) ? a_len : b_len;\n-\tint cmp = memcmp(a, b, len);\n-\tif (cmp)\n-\t\treturn cmp;\n-\treturn (a_len - b_len);\n-}\n-\n /*\n * The tree traversal is looking at name p. If we have a matching entry,\n * return it. If name p is a directory in the index, do not return\n@@ -678,7 +667,7 @@ static int find_cache_pos(struct traverse_info *info,\n \t\t\tce_len = ce_slash - ce_name;\n \t\telse\n \t\t\tce_len = ce_namelen(ce) - pfxlen;\n-\t\tcmp = name_compare(p->path, p_len, ce_name, ce_len);\n+\t\tcmp = strnncmp(p->path, p_len, ce_name, ce_len);\n \t\t/*\n \t\t * Exact match; if we have a directory we need to\n \t\t * delay returning it.\n-- \n2.0.0.695.g38ee9a9\n"},{"id":"244384","messageId":"539FFAF2.3070002@web.de","threadId":"36935","inReplyTo":"50de63f47ded2337adcd8bce151190fb99b38d64.1402990051.git.jmmahler@gmail.com","subject":"Re: [PATCH v2 1/3] add strnncmp() function","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-06-17T08:23:14Z","receivedAt":"2014-06-17T08:23:14Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-06-17 09.34, Jeremiah Mahler wrote:\n> Add a strnncmp() function which behaves like strncmp() except it takes\n> the length of both strings instead of just one. It behaves the same as\n> strncmp() up to the minimum common length between the strings. When the\nminimum common length? Isn'n t that 0?\nUsing the word \"common\", I think we could call it \"common length\".\n(And more places below)\n\n> strings are identical up to this minimum common length, the length\n> difference is returned.\n> \n> Signed-off-by: Jeremiah Mahler \n> ---\n> strbuf.c | 9 +++++++++\n> strbuf.h | 2 ++\n> 2 files changed, 11 insertions(+)\n> \n> diff --git a/strbuf.c b/strbuf.c\n> index ac62982..4eb7954 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -600,3 +600,12 @@ char *xstrdup_tolower(const char *string)\n> \tresult[i] = '\\0';\n> \treturn result;\n> }\n> +\nstrncmp uses size_t, not int:\nint strncmp(const char *s1, const char *s2, size_t n);\n\nIs there a special reason to allow negative string length?\nSome call sites use int when calling strncmp() or others,\nthat is one thing.\nBut when writing a generic strnncmp() function, I think\nit should use size_t, unless negative values have a meaning and\nare handled in the code.\n\n\n> +int strnncmp(const char *a, int len_a, const char *b, int len_b)\n> +{\n> +\tint min_len = (len_a < len_b) ? len_a : len_b;\n> +\tint cmp = strncmp(a, b, min_len);\n\n> +\tif (cmp)\n> +\t\treturn cmp;\n> +\treturn (len_a - len_b);\n> +}\n"},{"id":"244387","messageId":"CABPQNSZ7Vhn1Pz4j0R5twg+P-UzOG6xfw7fNqp0JO_Sh5t3CiA@mail.gmail.com","threadId":"36935","inReplyTo":"50de63f47ded2337adcd8bce151190fb99b38d64.1402990051.git.jmmahler@gmail.com","subject":"Re: [PATCH v2 1/3] add strnncmp() function","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2014-06-17T09:09:59Z","receivedAt":"2014-06-17T09:09:59Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Jun 17, 2014 at 9:34 AM, Jeremiah Mahler wrote:\n> Add a strnncmp() function which behaves like strncmp() except it takes\n> the length of both strings instead of just one. It behaves the same as\n> strncmp() up to the minimum common length between the strings. When the\n> strings are identical up to this minimum common length, the length\n> difference is returned.\n>\n> Signed-off-by: Jeremiah Mahler \n> ---\n> strbuf.c | 9 +++++++++\n> strbuf.h | 2 ++\n> 2 files changed, 11 insertions(+)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index ac62982..4eb7954 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -600,3 +600,12 @@ char *xstrdup_tolower(const char *string)\n> result[i] = '\\0';\n> return result;\n> }\n> +\n> +int strnncmp(const char *a, int len_a, const char *b, int len_b)\n> +{\n> + int min_len = (len_a < len_b) ? len_a : len_b;\n> + int cmp = strncmp(a, b, min_len);\n> + if (cmp)\n> + return cmp;\n> + return (len_a - len_b);\n> +}\n\nUsing a name that sounds like it's from the stdlib makes me cringe a\nlittle bit. Names that start with \"str\" reserved for stdlib[1][2], but\nwe already ignore this for strbuf (and perhaps some other functions).\nHowever, in this case it doesn't seem *that* unlikely that we might\ncollide with some stdlib-extensions.\n\n[1]: http://pubs.opengroup.org/onlinepubs/007904975/functions/xsh_chap02_02.html#tag_02_02_02\n[2]: http://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html\n"},{"id":"244393","messageId":"53A02195.8080202@web.de","threadId":"36935","inReplyTo":"cover.1402990051.git.jmmahler@gmail.com","subject":"Re: [PATCH v2 0/3] add strnncmp() function","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-06-17T11:08:05Z","receivedAt":"2014-06-17T11:08:05Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2014-06-17 09.34, Jeremiah Mahler wrote:\n> Add a strnncmp() function which behaves like strncmp() except it takes\n> the length of both strings instead of just one.\n> \n> Then simplify tree-walk.c and unpack-trees.c using this new function.\n> Replace all occurrences of name_compare() with strnncmp(). Remove\n> name_compare(), which they both had identical copies of.\n> \n> Version 2 includes suggestions from Jonathan Neider [1]:\n> \n> - Fix the logic which caused the new strnncmp() to behave differently\n> \tfrom the old version. Now it is identical to strncmp().\n> \n> - Improve description of strnncmp().\n> \n> Also, strnncmp() was switched from using memcmp() to strncmp()\n> internally to make it clear that this is meant for strings, not\n> general buffers.\nI don't think this is a good change, for 2 reasons:\n- It changes the semantics of existing code, which should be carefully\n reviewed, documented and may be put into a seperate commit.\n- Looking into the code for memcmp() and strncmp() in libc,\n I can see that memcmp() is written in 13 lines of assembler,\n (on a 386 system) with a fast\n repz cmpsb %es:(%edi),%ds:(%esi)\n working as the core engine.\n \n strncmp() uses 83 lines of assembler, because after each comparison\n the code needs to check of the '\\0' in both strings.\n- I can't see a reason to replace efficient code with less efficient code,\n so moving the old function \"as is\" into a include file, and declare\n it \"static inline\" could be the first step.\n\n Having code inline may open the door for the compiler to decide,\n \"Oh, I know exactly what memcmp() does, so I through in a handfull\n of lines assembly code, instead of calling memcmp() from libc\".\n\n\nAnd another thing:\n What does cache_name_compare(name, namelen, ce->name, len))\n in name-hash.c do?\n Isn't that the same function ?\n\nI like strnncmp() better than \ncache_name_compare() or name_compare(),\nbut I agree with Erik here that strnncmp() has the potential to\nbecome a name clash some day, so that git_strnncmp() may be better.\n\nThanks for the effort, cleaning up is needed.\n\n\n \n"},{"id":"244400","messageId":"20140617154841.GA5162@hudson.localdomain","threadId":"36935","inReplyTo":"539FFAF2.3070002@web.de","subject":"Re: [PATCH v2 1/3] add strnncmp() function","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-17T15:48:41Z","receivedAt":"2014-06-17T15:48:41Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Torsten,\n\nOn Tue, Jun 17, 2014 at 10:23:14AM +0200, Torsten Bögershausen wrote:\n> On 2014-06-17 09.34, Jeremiah Mahler wrote:\n> > Add a strnncmp() function which behaves like strncmp() except it takes\n> > the length of both strings instead of just one. It behaves the same as\n> > strncmp() up to the minimum common length between the strings. When the\n> minimum common length? Isn'n t that 0?\n> Using the word \"common\", I think we could call it \"common length\".\n> (And more places below)\n> \nYes, \"minimum\" doesn't make sense. \"common length\" sounds better.\n\n> > strings are identical up to this minimum common length, the length\n> > difference is returned.\n> > \n> > Signed-off-by: Jeremiah Mahler \n> > ---\n> > strbuf.c | 9 +++++++++\n> > strbuf.h | 2 ++\n> > 2 files changed, 11 insertions(+)\n> > \n> > diff --git a/strbuf.c b/strbuf.c\n> > index ac62982..4eb7954 100644\n> > --- a/strbuf.c\n> > +++ b/strbuf.c\n> > @@ -600,3 +600,12 @@ char *xstrdup_tolower(const char *string)\n> > \tresult[i] = '\\0';\n> > \treturn result;\n> > }\n> > +\n> strncmp uses size_t, not int:\n> int strncmp(const char *s1, const char *s2, size_t n);\n> \n> Is there a special reason to allow negative string length?\n> Some call sites use int when calling strncmp() or others,\n> that is one thing.\n> But when writing a generic strnncmp() function, I think\n> it should use size_t, unless negative values have a meaning and\n> are handled in the code.\n> \nDon't need negatives, size_t is more appropriate. Fixed.\n\n> \n> > +int strnncmp(const char *a, int len_a, const char *b, int len_b)\n> > +{\n> > +\tint min_len = (len_a < len_b) ? len_a : len_b;\n> > +\tint cmp = strncmp(a, b, min_len);\n> \n> > +\tif (cmp)\n> > +\t\treturn cmp;\n> > +\treturn (len_a - len_b);\n> > +}\n\nThanks,\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"244401","messageId":"20140617154926.GB5162@hudson.localdomain","threadId":"36935","inReplyTo":"CABPQNSZ7Vhn1Pz4j0R5twg+P-UzOG6xfw7fNqp0JO_Sh5t3CiA@mail.gmail.com","subject":"Re: [PATCH v2 1/3] add strnncmp() function","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-17T15:49:26Z","receivedAt":"2014-06-17T15:49:26Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Erik,\n\nOn Tue, Jun 17, 2014 at 11:09:59AM +0200, Erik Faye-Lund wrote:\n> On Tue, Jun 17, 2014 at 9:34 AM, Jeremiah Mahler wrote:\n> > Add a strnncmp() function which behaves like strncmp() except it takes\n> > the length of both strings instead of just one. It behaves the same as\n> > strncmp() up to the minimum common length between the strings. When the\n> > strings are identical up to this minimum common length, the length\n> > difference is returned.\n> >\n> > Signed-off-by: Jeremiah Mahler \n> > ---\n> > strbuf.c | 9 +++++++++\n> > strbuf.h | 2 ++\n> > 2 files changed, 11 insertions(+)\n> >\n> > diff --git a/strbuf.c b/strbuf.c\n> > index ac62982..4eb7954 100644\n> > --- a/strbuf.c\n> > +++ b/strbuf.c\n> > @@ -600,3 +600,12 @@ char *xstrdup_tolower(const char *string)\n> > result[i] = '\\0';\n> > return result;\n> > }\n> > +\n> > +int strnncmp(const char *a, int len_a, const char *b, int len_b)\n> > +{\n> > + int min_len = (len_a < len_b) ? len_a : len_b;\n> > + int cmp = strncmp(a, b, min_len);\n> > + if (cmp)\n> > + return cmp;\n> > + return (len_a - len_b);\n> > +}\n> \n> Using a name that sounds like it's from the stdlib makes me cringe a\n> little bit. Names that start with \"str\" reserved for stdlib[1][2], but\n> we already ignore this for strbuf (and perhaps some other functions).\n> However, in this case it doesn't seem *that* unlikely that we might\n> collide with some stdlib-extensions.\n> \n> [1]: http://pubs.opengroup.org/onlinepubs/007904975/functions/xsh_chap02_02.html#tag_02_02_02\n> [2]: http://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html\n\nI chose strnncmp() to try and emphasize its similarity to strncmp().\nBut you have a good point about potential name conflicts. That could be\na problem. I will change the name.\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"244402","messageId":"20140617154953.GC5162@hudson.localdomain","threadId":"36935","inReplyTo":"53A02195.8080202@web.de","subject":"Re: [PATCH v2 0/3] add strnncmp() function","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-17T15:49:53Z","receivedAt":"2014-06-17T15:49:53Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Torsten,\n\nOn Tue, Jun 17, 2014 at 01:08:05PM +0200, Torsten Bögershausen wrote:\n> On 2014-06-17 09.34, Jeremiah Mahler wrote:\n> > Add a strnncmp() function which behaves like strncmp() except it takes\n> > the length of both strings instead of just one.\n> > \n> > Then simplify tree-walk.c and unpack-trees.c using this new function.\n> > Replace all occurrences of name_compare() with strnncmp(). Remove\n> > name_compare(), which they both had identical copies of.\n> > \n> > Version 2 includes suggestions from Jonathan Neider [1]:\n> > \n> > - Fix the logic which caused the new strnncmp() to behave differently\n> > \tfrom the old version. Now it is identical to strncmp().\n> > \n> > - Improve description of strnncmp().\n> > \n> > Also, strnncmp() was switched from using memcmp() to strncmp()\n> > internally to make it clear that this is meant for strings, not\n> > general buffers.\n> I don't think this is a good change, for 2 reasons:\n> - It changes the semantics of existing code, which should be carefully\n> reviewed, documented and may be put into a seperate commit.\n> - Looking into the code for memcmp() and strncmp() in libc,\n> I can see that memcmp() is written in 13 lines of assembler,\n> (on a 386 system) with a fast\n> repz cmpsb %es:(%edi),%ds:(%esi)\n> working as the core engine.\n> \n> strncmp() uses 83 lines of assembler, because after each comparison\n> the code needs to check of the '\\0' in both strings.\n> - I can't see a reason to replace efficient code with less efficient code,\n> so moving the old function \"as is\" into a include file, and declare\n> it \"static inline\" could be the first step.\n> \n> Having code inline may open the door for the compiler to decide,\n> \"Oh, I know exactly what memcmp() does, so I through in a handfull\n> of lines assembly code, instead of calling memcmp() from libc\".\n> \nThanks for explaining the benefits of memcmp() over strcmp(), I will\nswitch it back.\n\nThe only case I can imagine where it would make a difference is when\nthere is a '\\0' in the middle of the string. But that would be an\nunlikely case since it probably meant the lengths were mis-calculated.\n\n> \n> And another thing:\n> What does cache_name_compare(name, namelen, ce->name, len))\n> in name-hash.c do?\n> Isn't that the same function ?\n> \ncache_name_compare() is the same except it returns -1, +1 instead of -N,\n+N. However, none of the cases where name_compare() is used need the\nmagnitude so this function could be used.\n\n> I like strnncmp() better than \n> cache_name_compare() or name_compare(),\n> but I agree with Erik here that strnncmp() has the potential to\n> become a name clash some day, so that git_strnncmp() may be better.\n> \nAgreed.\n\n> Thanks for the effort, cleaning up is needed.\n> \n\nThanks for the feedback :-)\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"244462","messageId":"20140617174817.GQ8557@google.com","threadId":"36935","inReplyTo":"20140617154953.GC5162@hudson.localdomain","subject":"Re: [PATCH v2 0/3] add strnncmp() function","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-06-17T17:48:17Z","receivedAt":"2014-06-17T17:48:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":">> On 2014-06-17 09.34, Jeremiah Mahler wrote:\n\n>>> Also, strnncmp() was switched from using memcmp() to strncmp()\n>>> internally to make it clear that this is meant for strings, not\n>>> general buffers.\n\nWhy shouldn't I want to use this helper on arbitrary data? One of the\nadvantages of other helpers in git that take a pointer and a length\n(e.g., the strbuf library) are that they are 8-bit clean and can work\non binary data when it's useful.\n\nThanks,\nJonathan\n"},{"id":"244463","messageId":"xmqqd2e7mneh.fsf@gitster.dls.corp.google.com","threadId":"36935","inReplyTo":"50de63f47ded2337adcd8bce151190fb99b38d64.1402990051.git.jmmahler@gmail.com","subject":"Re: [PATCH v2 1/3] add strnncmp() function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-17T17:55:18Z","receivedAt":"2014-06-17T17:55:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeremiah Mahler writes:\n\n> Add a strnncmp() function which behaves like strncmp() except it takes\n> the length of both strings instead of just one. It behaves the same as\n> strncmp() up to the minimum common length between the strings. When the\n> strings are identical up to this minimum common length, the length\n> difference is returned.\n>\n> Signed-off-by: Jeremiah Mahler \n> ---\n> strbuf.c | 9 +++++++++\n> strbuf.h | 2 ++\n> 2 files changed, 11 insertions(+)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index ac62982..4eb7954 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -600,3 +600,12 @@ char *xstrdup_tolower(const char *string)\n> \tresult[i] = '\\0';\n> \treturn result;\n> }\n> +\n> +int strnncmp(const char *a, int len_a, const char *b, int len_b)\n> +{\n> +\tint min_len = (len_a < len_b) ? len_a : len_b;\n> +\tint cmp = strncmp(a, b, min_len);\n> +\tif (cmp)\n> +\t\treturn cmp;\n> +\treturn (len_a - len_b);\n> +}\n\nI am not sure if the interface into this function conceptually makes\nmuch sense. strncmp(entry, string, 14) was invented as the way to\nsee if a NUL terminated \"string\" matches with the contents in an\narray of char \"entry\" that is up to 14 bytes long, and because the\n\"entry\" was allowed to fill full 14-byte space without terminated\nwith a NUL, the maximum possible length is specified separately, but\na NUL termination in \"entry\", if exists, is still honored. Is there\nany case where such a pair of \"maximum N bytes but could be shorter\"\nstrings are compared, especially with different N's defined per\nstring, in our codebase (or in other people's project for that\nmatter)?\n\nFurther, I do think that the interface into this function and its\nimplementation are inappropriate for implementing the name_compare()\nfunction in tree-walk.c and unpack-trees.c. These functions are\ndesigned to take counted strings; in a tuple they take,\n\"a_len\" is the only thing that determines the length of string \"a\".\nThere is no room for a NUL termination inside \"a\" come into play to\nmake \"a\" shorter than \"a_len\".\n\nIn other words, \"Two NUL-terminated strings can be compared with\nstrcmp(a, b), but we use counted strings in many places in our\ncodebase, and compare_counted_strings(a, a_len, b, b_len) function\nwould help us, so let's add one and use it in name_compare()\" may\nmake good sense, but if we were to do so, I do not think strncmp()\nwould be involved in its implementation.\n"},{"id":"244471","messageId":"20140617190912.GA23557@hudson.localdomain","threadId":"36935","inReplyTo":"20140617174817.GQ8557@google.com","subject":"Re: [PATCH v2 0/3] add strnncmp() function","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-17T19:09:12Z","receivedAt":"2014-06-17T19:09:12Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Jonathan,\n\nOn Tue, Jun 17, 2014 at 10:48:17AM -0700, Jonathan Nieder wrote:\n> >> On 2014-06-17 09.34, Jeremiah Mahler wrote:\n> \n> >>> Also, strnncmp() was switched from using memcmp() to strncmp()\n> >>> internally to make it clear that this is meant for strings, not\n> >>> general buffers.\n> \n> Why shouldn't I want to use this helper on arbitrary data? One of the\n> advantages of other helpers in git that take a pointer and a length\n> (e.g., the strbuf library) are that they are 8-bit clean and can work\n> on binary data when it's useful.\n> \n> Thanks,\n> Jonathan\n\nYes, along with the performance of strncmp() being worse than memcmp(),\nand Junios explanation of \"counted strings\", I think this was a bad idea.\nI will switch back to the memcmp() version.\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"244472","messageId":"20140617192727.GB23557@hudson.localdomain","threadId":"36935","inReplyTo":"xmqqd2e7mneh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 1/3] add strnncmp() function","fromName":"Jeremiah Mahler","fromEmail":"jmmahler@gmail.com","sentAt":"2014-06-17T19:27:27Z","receivedAt":"2014-06-17T19:27:27Z","isPatch":true,"sender":{"key":"jmmahler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/154028?v=4"},"body":"Junio,\n\nOn Tue, Jun 17, 2014 at 10:55:18AM -0700, Junio C Hamano wrote:\n> Jeremiah Mahler writes:\n> \n> > Add a strnncmp() function which behaves like strncmp() except it takes\n> > the length of both strings instead of just one. It behaves the same as\n> > strncmp() up to the minimum common length between the strings. When the\n> > strings are identical up to this minimum common length, the length\n> > difference is returned.\n> >\n> > Signed-off-by: Jeremiah Mahler \n> > ---\n> > strbuf.c | 9 +++++++++\n> > strbuf.h | 2 ++\n> > 2 files changed, 11 insertions(+)\n> >\n> > diff --git a/strbuf.c b/strbuf.c\n> > index ac62982..4eb7954 100644\n> > --- a/strbuf.c\n> > +++ b/strbuf.c\n> > @@ -600,3 +600,12 @@ char *xstrdup_tolower(const char *string)\n> > \tresult[i] = '\\0';\n> > \treturn result;\n> > }\n> > +\n> > +int strnncmp(const char *a, int len_a, const char *b, int len_b)\n> > +{\n> > +\tint min_len = (len_a < len_b) ? len_a : len_b;\n> > +\tint cmp = strncmp(a, b, min_len);\n> > +\tif (cmp)\n> > +\t\treturn cmp;\n> > +\treturn (len_a - len_b);\n> > +}\n> \n> I am not sure if the interface into this function conceptually makes\n> much sense. strncmp(entry, string, 14) was invented as the way to\n> see if a NUL terminated \"string\" matches with the contents in an\n> array of char \"entry\" that is up to 14 bytes long, and because the\n> \"entry\" was allowed to fill full 14-byte space without terminated\n> with a NUL, the maximum possible length is specified separately, but\n> a NUL termination in \"entry\", if exists, is still honored. Is there\n> any case where such a pair of \"maximum N bytes but could be shorter\"\n> strings are compared, especially with different N's defined per\n> string, in our codebase (or in other people's project for that\n> matter)?\n> \n> Further, I do think that the interface into this function and its\n> implementation are inappropriate for implementing the name_compare()\n> function in tree-walk.c and unpack-trees.c. These functions are\n> designed to take counted strings; in a tuple they take,\n> \"a_len\" is the only thing that determines the length of string \"a\".\n> There is no room for a NUL termination inside \"a\" come into play to\n> make \"a\" shorter than \"a_len\".\n> \n> In other words, \"Two NUL-terminated strings can be compared with\n> strcmp(a, b), but we use counted strings in many places in our\n> codebase, and compare_counted_strings(a, a_len, b, b_len) function\n> would help us, so let's add one and use it in name_compare()\" may\n> make good sense, but if we were to do so, I do not think strncmp()\n> would be involved in its implementation.\n> \n> \nThe concept of a counted string clears up some of the confusion I was\nhaving. Thanks for that explanation.\n\n-- \nJeremiah Mahler\njmmahler@gmail.com\nhttp://github.com/jmahler\n"},{"id":"244508","messageId":"20140618103331.GA12445@domone.podge","threadId":"36935","inReplyTo":"53A02195.8080202@web.de","subject":"Re: [PATCH v2 0/3] add strnncmp() function","fromName":"Ondřej Bílka","fromEmail":"neleai@seznam.cz","sentAt":"2014-06-18T10:33:31Z","receivedAt":"2014-06-18T10:33:31Z","isPatch":true,"sender":{"key":"neleai@seznam.cz","avatar":"https://avatars.githubusercontent.com/u/48067?v=4"},"body":"On Tue, Jun 17, 2014 at 01:08:05PM +0200, Torsten Bögershausen wrote:\n> On 2014-06-17 09.34, Jeremiah Mahler wrote:\n> > Add a strnncmp() function which behaves like strncmp() except it takes\n> > the length of both strings instead of just one.\n> > \n> > Then simplify tree-walk.c and unpack-trees.c using this new function.\n> > Replace all occurrences of name_compare() with strnncmp(). Remove\n> > name_compare(), which they both had identical copies of.\n> > \n> > Version 2 includes suggestions from Jonathan Neider [1]:\n> > \n> > - Fix the logic which caused the new strnncmp() to behave differently\n> > \tfrom the old version. Now it is identical to strncmp().\n> > \n> > - Improve description of strnncmp().\n> > \n> > Also, strnncmp() was switched from using memcmp() to strncmp()\n> > internally to make it clear that this is meant for strings, not\n> > general buffers.\n> I don't think this is a good change, for 2 reasons:\n> - It changes the semantics of existing code, which should be carefully\n> reviewed, documented and may be put into a seperate commit.\n> - Looking into the code for memcmp() and strncmp() in libc,\n> I can see that memcmp() is written in 13 lines of assembler,\n> (on a 386 system) with a fast\n> repz cmpsb %es:(%edi),%ds:(%esi)\n> working as the core engine.\n> \n> strncmp() uses 83 lines of assembler, because after each comparison\n> the code needs to check of the '\\0' in both strings.\n> - I can't see a reason to replace efficient code with less efficient code,\n> so moving the old function \"as is\" into a include file, and declare\n> it \"static inline\" could be the first step.\n> \nThat is not true, a rep cmpsb was fast for 486 but is relatively slow\nfor newer processors. For performance a correct answer is to measure it than do \nblind guess. Are these strings null terminated or is giving a size just\na hint? If it is a hint then a plain strcmp could be faster (this\ndepends on implementation). A reason is that for implementations that\ncheck more bytes at once it is easier to combine a terminating null mask with \ndifference than trying to first find which of first 16 bytes are different and \nthen compare if it is within size.\n"}]}