{"thread":{"id":"59199","subject":"[PATCH] pack-objects: use strcspn(3) in name_cmp_len()","startedAt":"2023-02-05T10:42:38Z","lastAt":"2023-02-06T22:35:31Z","messageCount":3,"participants":["René Scharfe","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"471519","messageId":"7315487c-c97c-b8a2-d3b2-4fbf642495dd@web.de","threadId":"59199","inReplyTo":null,"subject":"[PATCH] pack-objects: use strcspn(3) in name_cmp_len()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2023-02-05T10:42:27Z","receivedAt":"2023-02-05T10:42:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Call strcspn(3) to find the length of a string terminated by NUL, NL or\nslash instead of open-coding it.  Adopt its return type, size_t, to\nsupport strings of arbitrary length.  Use that type in callers as well\nfor variables and function parameters that receive the return value.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nFormatted with --function-context for easier review.\n\n builtin/pack-objects.c | 13 +++++--------\n 1 file changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex cabace4abb..536edc3a48 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1708,50 +1708,47 @@ static void pbase_tree_put(struct pbase_tree_cache *cache)\n \tfree(cache);\n }\n\n-static int name_cmp_len(const char *name)\n+static size_t name_cmp_len(const char *name)\n {\n-\tint i;\n-\tfor (i = 0; name[i] && name[i] != '\\n' && name[i] != '/'; i++)\n-\t\t;\n-\treturn i;\n+\treturn strcspn(name, \"\\n/\");\n }\n\n static void add_pbase_object(struct tree_desc *tree,\n \t\t\t     const char *name,\n-\t\t\t     int cmplen,\n+\t\t\t     size_t cmplen,\n \t\t\t     const char *fullname)\n {\n \tstruct name_entry entry;\n \tint cmp;\n\n \twhile (tree_entry(tree,&entry)) {\n \t\tif (S_ISGITLINK(entry.mode))\n \t\t\tcontinue;\n \t\tcmp = tree_entry_len(&entry) != cmplen ? 1 :\n \t\t      memcmp(name, entry.path, cmplen);\n \t\tif (cmp > 0)\n \t\t\tcontinue;\n \t\tif (cmp < 0)\n \t\t\treturn;\n \t\tif (name[cmplen] != '/') {\n \t\t\tadd_object_entry(&entry.oid,\n \t\t\t\t\t object_type(entry.mode),\n \t\t\t\t\t fullname, 1);\n \t\t\treturn;\n \t\t}\n \t\tif (S_ISDIR(entry.mode)) {\n \t\t\tstruct tree_desc sub;\n \t\t\tstruct pbase_tree_cache *tree;\n \t\t\tconst char *down = name+cmplen+1;\n-\t\t\tint downlen = name_cmp_len(down);\n+\t\t\tsize_t downlen = name_cmp_len(down);\n\n \t\t\ttree = pbase_tree_get(&entry.oid);\n \t\t\tif (!tree)\n \t\t\t\treturn;\n \t\t\tinit_tree_desc(&sub, tree->tree_data, tree->tree_size);\n\n \t\t\tadd_pbase_object(&sub, down, downlen, fullname);\n \t\t\tpbase_tree_put(tree);\n \t\t}\n \t}\n }\n@@ -1795,21 +1792,21 @@ static int check_pbase_path(unsigned hash)\n static void add_preferred_base_object(const char *name)\n {\n \tstruct pbase_tree *it;\n-\tint cmplen;\n+\tsize_t cmplen;\n \tunsigned hash = pack_name_hash(name);\n\n \tif (!num_preferred_base || check_pbase_path(hash))\n \t\treturn;\n\n \tcmplen = name_cmp_len(name);\n \tfor (it = pbase_tree; it; it = it->next) {\n \t\tif (cmplen == 0) {\n \t\t\tadd_object_entry(&it->pcache.oid, OBJ_TREE, NULL, 1);\n \t\t}\n \t\telse {\n \t\t\tstruct tree_desc tree;\n \t\t\tinit_tree_desc(&tree, it->pcache.tree_data, it->pcache.tree_size);\n \t\t\tadd_pbase_object(&tree, name, cmplen, name);\n \t\t}\n \t}\n }\n--\n2.39.1\n"},{"id":"471542","messageId":"230205.86357j95wj.gmgdl@evledraar.gmail.com","threadId":"59199","inReplyTo":"7315487c-c97c-b8a2-d3b2-4fbf642495dd@web.de","subject":"Re: [PATCH] pack-objects: use strcspn(3) in name_cmp_len()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-02-05T20:59:10Z","receivedAt":"2023-02-05T21:08:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Feb 05 2023, René Scharfe wrote:\n\n> -static int name_cmp_len(const char *name)\n> +static size_t name_cmp_len(const char *name)\n>  {\n> -\tint i;\n> -\tfor (i = 0; name[i] && name[i] != '\\n' && name[i] != '/'; i++)\n> -\t\t;\n> -\treturn i;\n> +\treturn strcspn(name, \"\\n/\");\n>  }\n\nI wonder if this name_cmp_len() is worth keeping at all. If all we're\ndoing is wrapping strcspn() (which b.t.w, seem to be less \"open-coding\"\nand just that it wasn't known to the original author in 5d4a6003354\n(Make git-pack-objects a builtin, 2006-08-03)), then just inlining that\nin the two name_cmp_len() invocations would be better, or maybe:\n\n\tstrcspn(..., object_reject);\n\nWhere we previously declared \"object_reject\" (or same better name for\nsuch a variable) to be \"\\n/\".\n\n>  static void add_pbase_object(struct tree_desc *tree,\n>  \t\t\t     const char *name,\n> -\t\t\t     int cmplen,\n> +\t\t\t     size_t cmplen,\n>  \t\t\t     const char *fullname)\n>  {\n>  \tstruct name_entry entry;\n>  \tint cmp;\n>\n>  \twhile (tree_entry(tree,&entry)) {\n>  \t\tif (S_ISGITLINK(entry.mode))\n>  \t\t\tcontinue;\n>  \t\tcmp = tree_entry_len(&entry) != cmplen ? 1 :\n>  \t\t      memcmp(name, entry.path, cmplen);\n\nThe return of tree_entry_len() is \"int\", so that's a new implicit cast,\nbut that seems OK in this case (and the \"namelen\" is another thing we\nshould convert to \"size_t\").\n"},{"id":"471621","messageId":"xmqqr0v22zio.fsf@gitster.g","threadId":"59199","inReplyTo":"230205.86357j95wj.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] pack-objects: use strcspn(3) in name_cmp_len()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-06T22:35:27Z","receivedAt":"2023-02-06T22:35:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> I wonder if this name_cmp_len() is worth keeping at all. If all we're\n> doing is wrapping strcspn() (which b.t.w, seem to be less \"open-coding\"\n> and just that it wasn't known to the original author in 5d4a6003354\n> (Make git-pack-objects a builtin, 2006-08-03)), then just inlining that\n> in the two name_cmp_len() invocations would be better, or maybe:\n\nEven if the stop candidate bytes were a constant, or if there were\nonly a single callsite, I am not sure if it is a good idea, simply\nbecause with this\n\n> \tstrcspn(..., object_reject);\n\nor with a literal \"\\n/\" to make it easier to see where in the string\nwe are stopping, it is hard without named function to tell what\nlength we are computing.\n\nThe function being file-scope static, decent compilers hopefully\nwould inline the calls by two callers.\n"}]}