{"thread":{"id":"37017","subject":"[PATCH 1/2] sha1_file: replace PATH_MAX buffer wirh strbuf in prepare_packed_git_one()","startedAt":"2014-06-28T14:47:55Z","lastAt":"2014-07-02T17:41:12Z","messageCount":27,"participants":["René Scharfe","Duy Nguyen","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"245125","messageId":"53AED59B.1020209@web.de","threadId":"37017","inReplyTo":null,"subject":"[PATCH 1/2] sha1_file: replace PATH_MAX buffer wirh strbuf in prepare_packed_git_one()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-06-28T14:47:55Z","receivedAt":"2014-06-28T14:47:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Instead of using strbuf to create a message string in case a path is\ntoo long for our fixed-size buffer, replace that buffer with a strbuf\nand thus get rid of the limitation.\n\nSigned-off-by: Rene Scharfe \n---\n sha1_file.c | 41 +++++++++++++++--------------------------\n 1 file changed, 15 insertions(+), 26 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 34d527f..0c3cada 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1177,48 +1177,36 @@ static void report_pack_garbage(struct string_list *list)\n \n static void prepare_packed_git_one(char *objdir, int local)\n {\n-\t/* Ensure that this buffer is large enough so that we can\n-\t append \"/pack/\" without clobbering the stack even if\n-\t strlen(objdir) were PATH_MAX. */\n-\tchar path[PATH_MAX + 1 + 4 + 1 + 1];\n-\tint len;\n+\tstruct strbuf path = STRBUF_INIT;\n+\tsize_t dirnamelen;\n \tDIR *dir;\n \tstruct dirent *de;\n \tstruct string_list garbage = STRING_LIST_INIT_DUP;\n \n-\tsprintf(path, \"%s/pack\", objdir);\n-\tlen = strlen(path);\n-\tdir = opendir(path);\n+\tstrbuf_addstr(&path, objdir);\n+\tstrbuf_addstr(&path, \"/pack\");\n+\tdir = opendir(path.buf);\n \tif (!dir) {\n \t\tif (errno != ENOENT)\n \t\t\terror(\"unable to open object pack directory: %s: %s\",\n-\t\t\t path, strerror(errno));\n+\t\t\t path.buf, strerror(errno));\n \t\treturn;\n \t}\n-\tpath[len++] = '/';\n+\tstrbuf_addch(&path, '/');\n+\tdirnamelen = path.len;\n \twhile ((de = readdir(dir)) != NULL) {\n-\t\tint namelen = strlen(de->d_name);\n \t\tstruct packed_git *p;\n \n-\t\tif (len + namelen + 1 > sizeof(path)) {\n-\t\t\tif (report_garbage) {\n-\t\t\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\t\t\tstrbuf_addf(&sb, \"%.*s/%s\", len - 1, path, de->d_name);\n-\t\t\t\treport_garbage(\"path too long\", sb.buf);\n-\t\t\t\tstrbuf_release(&sb);\n-\t\t\t}\n-\t\t\tcontinue;\n-\t\t}\n-\n \t\tif (is_dot_or_dotdot(de->d_name))\n \t\t\tcontinue;\n \n-\t\tstrcpy(path + len, de->d_name);\n+\t\tstrbuf_setlen(&path, dirnamelen);\n+\t\tstrbuf_addstr(&path, de->d_name);\n \n \t\tif (has_extension(de->d_name, \".idx\")) {\n \t\t\t/* Don't reopen a pack we already have. */\n \t\t\tfor (p = packed_git; p; p = p->next) {\n-\t\t\t\tif (!memcmp(path, p->pack_name, len + namelen - 4))\n+\t\t\t\tif (!memcmp(path.buf, p->pack_name, path.len - 4))\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t\tif (p == NULL &&\n@@ -1226,7 +1214,7 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\t\t * See if it really is a valid .idx file with\n \t\t\t * corresponding .pack file that we can map.\n \t\t\t */\n-\t\t\t (p = add_packed_git(path, len + namelen, local)) != NULL)\n+\t\t\t (p = add_packed_git(path.buf, path.len, local)) != NULL)\n \t\t\t\tinstall_packed_git(p);\n \t\t}\n \n@@ -1237,13 +1225,14 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\t has_extension(de->d_name, \".pack\") ||\n \t\t has_extension(de->d_name, \".bitmap\") ||\n \t\t has_extension(de->d_name, \".keep\"))\n-\t\t\tstring_list_append(&garbage, path);\n+\t\t\tstring_list_append(&garbage, path.buf);\n \t\telse\n-\t\t\treport_garbage(\"garbage found\", path);\n+\t\t\treport_garbage(\"garbage found\", path.buf);\n \t}\n \tclosedir(dir);\n \treport_pack_garbage(&garbage);\n \tstring_list_clear(&garbage, 0);\n+\tstrbuf_release(&path);\n }\n \n static int sort_pack(const void *a_, const void *b_)\n-- \n2.0.0\n"},{"id":"245126","messageId":"53AED8B1.4070701@web.de","threadId":"37017","inReplyTo":"53AED59B.1020209@web.de","subject":"[PATCH 2/4] sha1_file: use strncmp for string comparison","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-06-28T15:01:05Z","receivedAt":"2014-06-28T15:01:05Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Avoid overrunning the existing pack name (p->pack_name, a C string) in\nthe case that the new path is longer by using strncmp instead of memcmp\nfor comparing. While at it, replace the magic constant 4 with a\nstrlen call to document its meaning.\n\nSigned-off-by: Rene Scharfe \n---\n sha1_file.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 0c3cada..72b8fcb 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1206,7 +1206,8 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\tif (has_extension(de->d_name, \".idx\")) {\n \t\t\t/* Don't reopen a pack we already have. */\n \t\t\tfor (p = packed_git; p; p = p->next) {\n-\t\t\t\tif (!memcmp(path.buf, p->pack_name, path.len - 4))\n+\t\t\t\tif (!strncmp(path.buf, p->pack_name,\n+\t\t\t\t\t path.len - strlen(\".idx\")))\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t\tif (p == NULL &&\n-- \n2.0.0\n"},{"id":"245144","messageId":"CACsJy8B_DAjLRcMe4ys2LGkLOcKuW-PL_WNHyFB8Ry3Uv38LCw@mail.gmail.com","threadId":"37017","inReplyTo":"53AED59B.1020209@web.de","subject":"Re: [PATCH 1/2] sha1_file: replace PATH_MAX buffer wirh strbuf in prepare_packed_git_one()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-06-29T01:21:22Z","receivedAt":"2014-06-29T01:21:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jun 28, 2014 at 9:47 PM, René Scharfe wrote:\n> - sprintf(path, \"%s/pack\", objdir);\n> - len = strlen(path);\n> - dir = opendir(path);\n> + strbuf_addstr(&path, objdir);\n> + strbuf_addstr(&path, \"/pack\");\n> + dir = opendir(path.buf);\n> if (!dir) {\n> if (errno != ENOENT)\n> error(\"unable to open object pack directory: %s: %s\",\n> - path, strerror(errno));\n> + path.buf, strerror(errno));\n> return;\n\nMemory leak. The rest looks good.\n-- \nDuy\n"},{"id":"245146","messageId":"53AFA775.1090900@web.de","threadId":"37017","inReplyTo":"CACsJy8B_DAjLRcMe4ys2LGkLOcKuW-PL_WNHyFB8Ry3Uv38LCw@mail.gmail.com","subject":"[PATCH v2 1/2] sha1_file: replace PATH_MAX buffer wirh strbuf in, prepare_packed_git_one()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-06-29T05:43:17Z","receivedAt":"2014-06-29T05:43:17Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Instead of using strbuf to create a message string in case a path is\ntoo long for our fixed-size buffer, replace that buffer with a strbuf\nand thus get rid of the limitation.\n\nHelped-by: Duy Nguyen \nSigned-off-by: Rene Scharfe \n---\n@Duy: Thanks for catching the missing strbuf_release in the opendir\nerror case.\n\n sha1_file.c | 42 ++++++++++++++++--------------------------\n 1 file changed, 16 insertions(+), 26 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 34d527f..394fa45 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1177,48 +1177,37 @@ static void report_pack_garbage(struct string_list *list)\n \n static void prepare_packed_git_one(char *objdir, int local)\n {\n-\t/* Ensure that this buffer is large enough so that we can\n-\t append \"/pack/\" without clobbering the stack even if\n-\t strlen(objdir) were PATH_MAX. */\n-\tchar path[PATH_MAX + 1 + 4 + 1 + 1];\n-\tint len;\n+\tstruct strbuf path = STRBUF_INIT;\n+\tsize_t dirnamelen;\n \tDIR *dir;\n \tstruct dirent *de;\n \tstruct string_list garbage = STRING_LIST_INIT_DUP;\n \n-\tsprintf(path, \"%s/pack\", objdir);\n-\tlen = strlen(path);\n-\tdir = opendir(path);\n+\tstrbuf_addstr(&path, objdir);\n+\tstrbuf_addstr(&path, \"/pack\");\n+\tdir = opendir(path.buf);\n \tif (!dir) {\n \t\tif (errno != ENOENT)\n \t\t\terror(\"unable to open object pack directory: %s: %s\",\n-\t\t\t path, strerror(errno));\n+\t\t\t path.buf, strerror(errno));\n+\t\tstrbuf_release(&path);\n \t\treturn;\n \t}\n-\tpath[len++] = '/';\n+\tstrbuf_addch(&path, '/');\n+\tdirnamelen = path.len;\n \twhile ((de = readdir(dir)) != NULL) {\n-\t\tint namelen = strlen(de->d_name);\n \t\tstruct packed_git *p;\n \n-\t\tif (len + namelen + 1 > sizeof(path)) {\n-\t\t\tif (report_garbage) {\n-\t\t\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\t\t\tstrbuf_addf(&sb, \"%.*s/%s\", len - 1, path, de->d_name);\n-\t\t\t\treport_garbage(\"path too long\", sb.buf);\n-\t\t\t\tstrbuf_release(&sb);\n-\t\t\t}\n-\t\t\tcontinue;\n-\t\t}\n-\n \t\tif (is_dot_or_dotdot(de->d_name))\n \t\t\tcontinue;\n \n-\t\tstrcpy(path + len, de->d_name);\n+\t\tstrbuf_setlen(&path, dirnamelen);\n+\t\tstrbuf_addstr(&path, de->d_name);\n \n \t\tif (has_extension(de->d_name, \".idx\")) {\n \t\t\t/* Don't reopen a pack we already have. */\n \t\t\tfor (p = packed_git; p; p = p->next) {\n-\t\t\t\tif (!memcmp(path, p->pack_name, len + namelen - 4))\n+\t\t\t\tif (!memcmp(path.buf, p->pack_name, path.len - 4))\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t\tif (p == NULL &&\n@@ -1226,7 +1215,7 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\t\t * See if it really is a valid .idx file with\n \t\t\t * corresponding .pack file that we can map.\n \t\t\t */\n-\t\t\t (p = add_packed_git(path, len + namelen, local)) != NULL)\n+\t\t\t (p = add_packed_git(path.buf, path.len, local)) != NULL)\n \t\t\t\tinstall_packed_git(p);\n \t\t}\n \n@@ -1237,13 +1226,14 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\t has_extension(de->d_name, \".pack\") ||\n \t\t has_extension(de->d_name, \".bitmap\") ||\n \t\t has_extension(de->d_name, \".keep\"))\n-\t\t\tstring_list_append(&garbage, path);\n+\t\t\tstring_list_append(&garbage, path.buf);\n \t\telse\n-\t\t\treport_garbage(\"garbage found\", path);\n+\t\t\treport_garbage(\"garbage found\", path.buf);\n \t}\n \tclosedir(dir);\n \treport_pack_garbage(&garbage);\n \tstring_list_clear(&garbage, 0);\n+\tstrbuf_release(&path);\n }\n \n static int sort_pack(const void *a_, const void *b_)\n-- \n2.0.0\n"},{"id":"245147","messageId":"53AFAA89.6050200@web.de","threadId":"37017","inReplyTo":"53AFA775.1090900@web.de","subject":"[PATCH v2 2/2] sha1_file: use strncmp for string comparison","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-06-29T05:56:25Z","receivedAt":"2014-06-29T05:56:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Avoid overrunning the existing pack name (p->pack_name, a C string) in\nthe case that the new path is longer by using strncmp instead of memcmp\nfor comparing. While at it, replace the magic constant 4 with a\nstrlen call to document its meaning.\n\nSigned-off-by: Rene Scharfe \n---\nNo changes from intial round.\n\n sha1_file.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 394fa45..8adab14 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1207,7 +1207,8 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\tif (has_extension(de->d_name, \".idx\")) {\n \t\t\t/* Don't reopen a pack we already have. */\n \t\t\tfor (p = packed_git; p; p = p->next) {\n-\t\t\t\tif (!memcmp(path.buf, p->pack_name, path.len - 4))\n+\t\t\t\tif (!strncmp(path.buf, p->pack_name,\n+\t\t\t\t\t path.len - strlen(\".idx\")))\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t\tif (p == NULL &&\n-- \n2.0.0\n"},{"id":"245182","messageId":"20140630132423.GA14799@sigill.intra.peff.net","threadId":"37017","inReplyTo":"53AFA775.1090900@web.de","subject":"Re: [PATCH v2 1/2] sha1_file: replace PATH_MAX buffer wirh strbuf in, prepare_packed_git_one()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-30T13:24:24Z","receivedAt":"2014-06-30T13:24:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 29, 2014 at 07:43:17AM +0200, René Scharfe wrote:\n\n> Instead of using strbuf to create a message string in case a path is\n> too long for our fixed-size buffer, replace that buffer with a strbuf\n> and thus get rid of the limitation.\n\nYay. Safer, and the end result is much more readable.\n\n-Peff\n"},{"id":"245184","messageId":"20140630134317.GB14799@sigill.intra.peff.net","threadId":"37017","inReplyTo":"53AFAA89.6050200@web.de","subject":"Re: [PATCH v2 2/2] sha1_file: use strncmp for string comparison","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-30T13:43:17Z","receivedAt":"2014-06-30T13:43:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 29, 2014 at 07:56:25AM +0200, René Scharfe wrote:\n\n> Avoid overrunning the existing pack name (p->pack_name, a C string) in\n> the case that the new path is longer by using strncmp instead of memcmp\n> for comparing. While at it, replace the magic constant 4 with a\n> strlen call to document its meaning.\n\nAn unwritten assumption with this code is that we have \"foo.idx\" and we\nwant to make sure that we are matching \"foo.pack\" from the existing pack\nlist. Both before and after your patch, I think we would match\n\"foobar.pack\". It's probably not a big deal, as we don't expect random\njunk in the pack directory, but I wonder if it would be better to be\nexplicit, like:\n\n /*\n * like ends_with, but return the truncated size of str via\n * the \"len\" parameter.\n */\n int strip_suffix(const char *str, const char *suffix, size_t *len)\n {\n\tsize_t suflen = strlen(suffix);\n\t*len = strlen(str);\n\n\tif (len < suflen)\n\t\treturn 0;\n\telse if (strcmp(str + len - suflen, suffix))\n\t\treturn 0;\n\telse {\n\t\t*len -= suflen;\n\t\treturn 1;\n\t}\n }\n\n int idx_matches_pack(const char *idx, const char *pack)[\n {\n\tsize_t idx_len, pack_len;\n\tif (!strip_suffix(idx, \".idx\", &idx_len) ||\n\t !strip_suffix(pack, \".pack\", &pack_len))\n\t\treturn 0;\n\tif (idx_len != pack_len)\n\t\treturn 0;\n\treturn !memcmp(idx, pack, idx_len);\n }\n\nYou'd perhaps want to split idx_matches_pack across the loop below (our\nidx is actually a strbuf, so you can reuse path->len to avoid a strlen,\nand you do not have to verify idx_len each time through the loop).\n\nI think strip_suffix would have other uses, too. It's a more featureful\nversion of ends_with, as skip_prefix is to starts_with. E.g.,\nbuiltin/remote.c:config_read_branches could use it to avoid some magic\nnumbers.\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 394fa45..8adab14 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1207,7 +1207,8 @@ static void prepare_packed_git_one(char *objdir, int local)\n> \t\tif (has_extension(de->d_name, \".idx\")) {\n> \t\t\t/* Don't reopen a pack we already have. */\n\nIf we don't follow my suggestion above, we still have this\nhas_extension. This is a reimplementation of ends_with, isn't it? We can\nprobably drop it and just use ends_with.\n\n-Peff\n"},{"id":"245185","messageId":"CACsJy8A5Dq6MP8kH7R3POSjXoJjYvWF695fKj1YgTdscF5xYxA@mail.gmail.com","threadId":"37017","inReplyTo":"20140630134317.GB14799@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] sha1_file: use strncmp for string comparison","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-06-30T13:59:53Z","receivedAt":"2014-06-30T13:59:53Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jun 30, 2014 at 8:43 PM, Jeff King wrote:\n>> diff --git a/sha1_file.c b/sha1_file.c\n>> index 394fa45..8adab14 100644\n>> --- a/sha1_file.c\n>> +++ b/sha1_file.c\n>> @@ -1207,7 +1207,8 @@ static void prepare_packed_git_one(char *objdir, int local)\n>> if (has_extension(de->d_name, \".idx\")) {\n>> /* Don't reopen a pack we already have. */\n>\n> If we don't follow my suggestion above, we still have this\n> has_extension. This is a reimplementation of ends_with, isn't it? We can\n> probably drop it and just use ends_with.\n\nThis calls for another patch if we just want to kill has_extension()\nin favor of ends_with(). There are 12 call sites of it.\n-- \nDuy\n"},{"id":"245186","messageId":"20140630142232.GC14799@sigill.intra.peff.net","threadId":"37017","inReplyTo":"CACsJy8A5Dq6MP8kH7R3POSjXoJjYvWF695fKj1YgTdscF5xYxA@mail.gmail.com","subject":"Re: [PATCH v2 2/2] sha1_file: use strncmp for string comparison","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-30T14:22:32Z","receivedAt":"2014-06-30T14:22:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 30, 2014 at 08:59:53PM +0700, Duy Nguyen wrote:\n\n> On Mon, Jun 30, 2014 at 8:43 PM, Jeff King wrote:\n> >> diff --git a/sha1_file.c b/sha1_file.c\n> >> index 394fa45..8adab14 100644\n> >> --- a/sha1_file.c\n> >> +++ b/sha1_file.c\n> >> @@ -1207,7 +1207,8 @@ static void prepare_packed_git_one(char *objdir, int local)\n> >> if (has_extension(de->d_name, \".idx\")) {\n> >> /* Don't reopen a pack we already have. */\n> >\n> > If we don't follow my suggestion above, we still have this\n> > has_extension. This is a reimplementation of ends_with, isn't it? We can\n> > probably drop it and just use ends_with.\n> \n> This calls for another patch if we just want to kill has_extension()\n> in favor of ends_with(). There are 12 call sites of it.\n\nYes. Some of those would want to become ends_with, and some would\nactually want to become strip_suffix. I'm working up a series now.\n\n-Peff\n"},{"id":"245193","messageId":"53B191E6.7010509@web.de","threadId":"37017","inReplyTo":"20140630134317.GB14799@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] sha1_file: use strncmp for string comparison","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-06-30T16:35:50Z","receivedAt":"2014-06-30T16:35:50Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.06.2014 15:43, schrieb Jeff King:\n> On Sun, Jun 29, 2014 at 07:56:25AM +0200, René Scharfe wrote:\n> \n>> Avoid overrunning the existing pack name (p->pack_name, a C string) in\n>> the case that the new path is longer by using strncmp instead of memcmp\n>> for comparing. While at it, replace the magic constant 4 with a\n>> strlen call to document its meaning.\n> \n> An unwritten assumption with this code is that we have \"foo.idx\" and we\n> want to make sure that we are matching \"foo.pack\" from the existing pack\n> list. Both before and after your patch, I think we would match\n> \"foobar.pack\".\n\nYes, indeed.\n\n> It's probably not a big deal, as we don't expect random\n> junk in the pack directory, but I wonder if it would be better to be\n> explicit, like:\n\n\n\nHere's a simpler approach:\n\n-- >8 --\nSubject: [PATCH v2 3/2] sha1_file: more precise packname matching\n\nConsider the full length of the already loaded pack names when checking\nfor duplicates.\n\nSigned-off-by: Rene Scharfe \n---\n sha1_file.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b7ad6c1..a13fbbd 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1206,10 +1206,12 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\tstrbuf_addstr(&path, de->d_name);\n \n \t\tif (has_extension(de->d_name, \".idx\")) {\n+\t\t\tsize_t len = path.len - strlen(\".idx\");\n+\n \t\t\t/* Don't reopen a pack we already have. */\n \t\t\tfor (p = packed_git; p; p = p->next) {\n-\t\t\t\tif (!strncmp(path.buf, p->pack_name,\n-\t\t\t\t\t path.len - strlen(\".idx\")))\n+\t\t\t\tif (!strncmp(p->pack_name, path.buf, len) &&\n+\t\t\t\t !strcmp(p->pack_name + len, \".pack\"))\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t\tif (p == NULL &&\n-- \n2.0.0\n"},{"id":"245194","messageId":"53B193B7.6020407@web.de","threadId":"37017","inReplyTo":"20140630142232.GC14799@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] sha1_file: use strncmp for string comparison","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-06-30T16:43:35Z","receivedAt":"2014-06-30T16:43:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.06.2014 16:22, schrieb Jeff King:\n> On Mon, Jun 30, 2014 at 08:59:53PM +0700, Duy Nguyen wrote:\n>\n>> On Mon, Jun 30, 2014 at 8:43 PM, Jeff King wrote:\n>>>> diff --git a/sha1_file.c b/sha1_file.c\n>>>> index 394fa45..8adab14 100644\n>>>> --- a/sha1_file.c\n>>>> +++ b/sha1_file.c\n>>>> @@ -1207,7 +1207,8 @@ static void prepare_packed_git_one(char *objdir, int local)\n>>>> if (has_extension(de->d_name, \".idx\")) {\n>>>> /* Don't reopen a pack we already have. */\n>>>\n>>> If we don't follow my suggestion above, we still have this\n>>> has_extension. This is a reimplementation of ends_with, isn't it? We can\n>>> probably drop it and just use ends_with.\n>>\n>> This calls for another patch if we just want to kill has_extension()\n>> in favor of ends_with(). There are 12 call sites of it.\n>\n> Yes. Some of those would want to become ends_with, and some would\n> actually want to become strip_suffix. I'm working up a series now.\n\nNB: has_extension is almost the same as ends_with, but it also checks if \nthe string is longer than just the extension:\n\n\tends_with(\"x\", \"x\") => 1\n\thas_extension(\"x\", \"x\") => 0\n\nRené\n"},{"id":"245195","messageId":"20140630164517.GA11906@sigill.intra.peff.net","threadId":"37017","inReplyTo":"53B193B7.6020407@web.de","subject":"Re: [PATCH v2 2/2] sha1_file: use strncmp for string comparison","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-30T16:45:17Z","receivedAt":"2014-06-30T16:45:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 30, 2014 at 06:43:35PM +0200, René Scharfe wrote:\n\n> Am 30.06.2014 16:22, schrieb Jeff King:\n> >On Mon, Jun 30, 2014 at 08:59:53PM +0700, Duy Nguyen wrote:\n> >\n> >>On Mon, Jun 30, 2014 at 8:43 PM, Jeff King wrote:\n> >>>>diff --git a/sha1_file.c b/sha1_file.c\n> >>>>index 394fa45..8adab14 100644\n> >>>>--- a/sha1_file.c\n> >>>>+++ b/sha1_file.c\n> >>>>@@ -1207,7 +1207,8 @@ static void prepare_packed_git_one(char *objdir, int local)\n> >>>> if (has_extension(de->d_name, \".idx\")) {\n> >>>> /* Don't reopen a pack we already have. */\n> >>>\n> >>>If we don't follow my suggestion above, we still have this\n> >>>has_extension. This is a reimplementation of ends_with, isn't it? We can\n> >>>probably drop it and just use ends_with.\n> >>\n> >>This calls for another patch if we just want to kill has_extension()\n> >>in favor of ends_with(). There are 12 call sites of it.\n> >\n> >Yes. Some of those would want to become ends_with, and some would\n> >actually want to become strip_suffix. I'm working up a series now.\n> \n> NB: has_extension is almost the same as ends_with, but it also checks if the\n> string is longer than just the extension:\n> \n> \tends_with(\"x\", \"x\") => 1\n> \thas_extension(\"x\", \"x\") => 0\n\nThanks, I didn't notice that. I don't think the distinction is\nimportant in any callers, but I'll double check.\n\n-Peff\n"},{"id":"245196","messageId":"20140630164600.GA7641@sigill.intra.peff.net","threadId":"37017","inReplyTo":"53B191E6.7010509@web.de","subject":"Re: [PATCH v2 2/2] sha1_file: use strncmp for string comparison","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-30T16:46:00Z","receivedAt":"2014-06-30T16:46:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 30, 2014 at 06:35:50PM +0200, René Scharfe wrote:\n\n> > It's probably not a big deal, as we don't expect random\n> > junk in the pack directory, but I wonder if it would be better to be\n> > explicit, like:\n> \n> \n> \n> Here's a simpler approach:\n\nI agree that solves the problem. However, I'm about to post an\nalternative series that also replaces has_extension with strip_suffix,\nwhich I think ends up a bit nicer.\n\n-Peff\n"},{"id":"245197","messageId":"20140630165526.GA15690@sigill.intra.peff.net","threadId":"37017","inReplyTo":"20140630134317.GB14799@sigill.intra.peff.net","subject":"[PATCH 0/9] add strip_suffix as an alternative to ends_with","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-30T16:55:27Z","receivedAt":"2014-06-30T16:55:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a series to do for ends_with what the recent skip_prefix series\ndid for starts_with. Namely: drop some magic numbers and repeated string\nliterals, and hopefully make things more readable.\n\nThe first patch is René's patch 1/2, with the leak fix from Duy and typo\nfixes in the commit message from me. The rest are new, and replace the\nchanges to prepare_packed_git_one done elsewhere in the thread.\n\n [1/9]: sha1_file: replace PATH_MAX buffer with strbuf in prepare_packed_git_one()\n [2/9]: add strip_suffix function\n [3/9]: implement ends_with via strip_suffix\n [4/9]: replace has_extension with ends_with\n [5/9]: use strip_suffix instead of ends_with in simple cases\n [6/9]: index-pack: use strip_suffix to avoid magic numbers\n [7/9]: strbuf: implement strbuf_strip_suffix\n [8/9]: verify-pack: use strbuf_strip_suffix\n [9/9]: prepare_packed_git_one: refactor duplicate-pack check\n\n-Peff\n"},{"id":"245198","messageId":"20140630165551.GA16637@sigill.intra.peff.net","threadId":"37017","inReplyTo":"20140630165526.GA15690@sigill.intra.peff.net","subject":"[PATCH 1/9] sha1_file: replace PATH_MAX buffer with strbuf in prepare_packed_git_one()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-30T16:55:52Z","receivedAt":"2014-06-30T16:55:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: René Scharfe \n\nInstead of using strbuf to create a message string in case a path is\ntoo long for our fixed-size buffer, replace that buffer with a strbuf\nand thus get rid of the limitation.\n\nHelped-by: Duy Nguyen \nSigned-off-by: Rene Scharfe \nSigned-off-by: Jeff King \n---\n sha1_file.c | 42 ++++++++++++++++--------------------------\n 1 file changed, 16 insertions(+), 26 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 34d527f..394fa45 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1177,48 +1177,37 @@ static void report_pack_garbage(struct string_list *list)\n \n static void prepare_packed_git_one(char *objdir, int local)\n {\n-\t/* Ensure that this buffer is large enough so that we can\n-\t append \"/pack/\" without clobbering the stack even if\n-\t strlen(objdir) were PATH_MAX. */\n-\tchar path[PATH_MAX + 1 + 4 + 1 + 1];\n-\tint len;\n+\tstruct strbuf path = STRBUF_INIT;\n+\tsize_t dirnamelen;\n \tDIR *dir;\n \tstruct dirent *de;\n \tstruct string_list garbage = STRING_LIST_INIT_DUP;\n \n-\tsprintf(path, \"%s/pack\", objdir);\n-\tlen = strlen(path);\n-\tdir = opendir(path);\n+\tstrbuf_addstr(&path, objdir);\n+\tstrbuf_addstr(&path, \"/pack\");\n+\tdir = opendir(path.buf);\n \tif (!dir) {\n \t\tif (errno != ENOENT)\n \t\t\terror(\"unable to open object pack directory: %s: %s\",\n-\t\t\t path, strerror(errno));\n+\t\t\t path.buf, strerror(errno));\n+\t\tstrbuf_release(&path);\n \t\treturn;\n \t}\n-\tpath[len++] = '/';\n+\tstrbuf_addch(&path, '/');\n+\tdirnamelen = path.len;\n \twhile ((de = readdir(dir)) != NULL) {\n-\t\tint namelen = strlen(de->d_name);\n \t\tstruct packed_git *p;\n \n-\t\tif (len + namelen + 1 > sizeof(path)) {\n-\t\t\tif (report_garbage) {\n-\t\t\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\t\t\tstrbuf_addf(&sb, \"%.*s/%s\", len - 1, path, de->d_name);\n-\t\t\t\treport_garbage(\"path too long\", sb.buf);\n-\t\t\t\tstrbuf_release(&sb);\n-\t\t\t}\n-\t\t\tcontinue;\n-\t\t}\n-\n \t\tif (is_dot_or_dotdot(de->d_name))\n \t\t\tcontinue;\n \n-\t\tstrcpy(path + len, de->d_name);\n+\t\tstrbuf_setlen(&path, dirnamelen);\n+\t\tstrbuf_addstr(&path, de->d_name);\n \n \t\tif (has_extension(de->d_name, \".idx\")) {\n \t\t\t/* Don't reopen a pack we already have. */\n \t\t\tfor (p = packed_git; p; p = p->next) {\n-\t\t\t\tif (!memcmp(path, p->pack_name, len + namelen - 4))\n+\t\t\t\tif (!memcmp(path.buf, p->pack_name, path.len - 4))\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t\tif (p == NULL &&\n@@ -1226,7 +1215,7 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\t\t * See if it really is a valid .idx file with\n \t\t\t * corresponding .pack file that we can map.\n \t\t\t */\n-\t\t\t (p = add_packed_git(path, len + namelen, local)) != NULL)\n+\t\t\t (p = add_packed_git(path.buf, path.len, local)) != NULL)\n \t\t\t\tinstall_packed_git(p);\n \t\t}\n \n@@ -1237,13 +1226,14 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\t has_extension(de->d_name, \".pack\") ||\n \t\t has_extension(de->d_name, \".bitmap\") ||\n \t\t has_extension(de->d_name, \".keep\"))\n-\t\t\tstring_list_append(&garbage, path);\n+\t\t\tstring_list_append(&garbage, path.buf);\n \t\telse\n-\t\t\treport_garbage(\"garbage found\", path);\n+\t\t\treport_garbage(\"garbage found\", path.buf);\n \t}\n \tclosedir(dir);\n \treport_pack_garbage(&garbage);\n \tstring_list_clear(&garbage, 0);\n+\tstrbuf_release(&path);\n }\n \n static int sort_pack(const void *a_, const void *b_)\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245199","messageId":"20140630165751.GB16637@sigill.intra.peff.net","threadId":"37017","inReplyTo":"20140630165526.GA15690@sigill.intra.peff.net","subject":"[PATCH 2/9] add strip_suffix function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-30T16:57:51Z","receivedAt":"2014-06-30T16:57:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Many callers of ends_with want to not only find out whether\na string has a suffix, but want to also strip it off. Doing\nthat separately has two minor problems:\n\n 1. We often run over the string twice (once to find\n the suffix, and then once more to find its length to\n subtract the suffix length).\n\n 2. We have to specify the suffix length again, which means\n either a magic number, or repeating ourselves with\n strlen(\"suffix\").\n\nJust as we have skip_prefix to avoid these cases with\nstarts_with, we can add a strip_suffix to avoid them with\nends_with.\n\nNote that we add two forms of strip_suffix here: one that\ntakes a string, with the resulting length as an\nout-parameter; and one that takes a pointer/length pair, and\nreuses the length as an out-parameter. The latter is more\nefficient when the caller already has the length (e.g., when\nusing strbufs), but it can be easy to confuse the two, as\nthey take the same number and types of parameters.\n\nFor that reason, the \"mem\" form puts its length parameter\nnext to the buffer (since they are a pair), and the string\nform puts it at the end (since it is an out-parameter). The\ncompiler can notice when you get the order wrong, which\nshould help prevent writing one when you meant the other.\n\nSigned-off-by: Jeff King \n---\nI hope the word \"strip\" is OK, as it does not actually NUL-terminate\n(doing so would make it unusable for many cases). Between the comment\nbelow and the \"const\" in the parameter, I think it should be pretty\nclear that it does not touch the string. And I could not think of a\nbetter word.\n\n git-compat-util.h | 27 +++++++++++++++++++++++++++\n 1 file changed, 27 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex b6f03b3..d044c42 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -358,6 +358,33 @@ static inline const char *skip_prefix(const char *str, const char *prefix)\n \treturn NULL;\n }\n \n+/*\n+ * If buf ends with suffix, return 1 and subtract the length of the suffix\n+ * from *len. Otherwise, return 0 and leave *len untouched.\n+ */\n+static inline int strip_suffix_mem(const char *buf, size_t *len,\n+\t\t\t\t const char *suffix)\n+{\n+\tsize_t suflen = strlen(suffix);\n+\tif (*len < suflen || memcmp(buf + (*len - suflen), suffix, suflen))\n+\t\treturn 0;\n+\t*len -= suflen;\n+\treturn 1;\n+}\n+\n+/*\n+ * If str ends with suffix, return 1 and set *len to the size of the string\n+ * without the suffix. Otherwise, return 0 and set *len to the size of the\n+ * string.\n+ *\n+ * Note that we do _not_ NUL-terminate str to the new length.\n+ */\n+static inline int strip_suffix(const char *str, const char *suffix, size_t *len)\n+{\n+\t*len = strlen(str);\n+\treturn strip_suffix_mem(str, len, suffix);\n+}\n+\n #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)\n \n #ifndef PROT_READ\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245200","messageId":"20140630165808.GC16637@sigill.intra.peff.net","threadId":"37017","inReplyTo":"20140630165526.GA15690@sigill.intra.peff.net","subject":"[PATCH 3/9] implement ends_with via strip_suffix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-30T16:58:08Z","receivedAt":"2014-06-30T16:58:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The ends_with function is essentially a simplified version\nof strip_suffix, in which we throw away the stripped length.\nImplementing it as an inline on top of strip_suffix has two\nadvantages:\n\n 1. We save a bit of duplicated code.\n\n 2. The suffix is typically a string literal, and we call\n strlen on it. By making the function inline, many\n compilers can replace the strlen call with a constant.\n\nSigned-off-by: Jeff King \n---\n git-compat-util.h | 7 ++++++-\n strbuf.c | 9 ---------\n 2 files changed, 6 insertions(+), 10 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex d044c42..4cfde49 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -347,7 +347,6 @@ extern void set_error_routine(void (*routine)(const char *err, va_list params));\n extern void set_die_is_recursing_routine(int (*routine)(void));\n \n extern int starts_with(const char *str, const char *prefix);\n-extern int ends_with(const char *str, const char *suffix);\n \n static inline const char *skip_prefix(const char *str, const char *prefix)\n {\n@@ -385,6 +384,12 @@ static inline int strip_suffix(const char *str, const char *suffix, size_t *len)\n \treturn strip_suffix_mem(str, len, suffix);\n }\n \n+static inline int ends_with(const char *str, const char *suffix)\n+{\n+\tsize_t len;\n+\treturn strip_suffix(str, suffix, &len);\n+}\n+\n #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)\n \n #ifndef PROT_READ\ndiff --git a/strbuf.c b/strbuf.c\nindex ac62982..99dbeba 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -11,15 +11,6 @@ int starts_with(const char *str, const char *prefix)\n \t\t\treturn 0;\n }\n \n-int ends_with(const char *str, const char *suffix)\n-{\n-\tint len = strlen(str), suflen = strlen(suffix);\n-\tif (len < suflen)\n-\t\treturn 0;\n-\telse\n-\t\treturn !strcmp(str + len - suflen, suffix);\n-}\n-\n /*\n * Used as the default ->buf value, so that people can always assume\n * buf is non NULL and ->buf is NUL terminated even for a freshly\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245201","messageId":"20140630165825.GD16637@sigill.intra.peff.net","threadId":"37017","inReplyTo":"20140630165526.GA15690@sigill.intra.peff.net","subject":"[PATCH 4/9] replace has_extension with ends_with","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-30T16:58:25Z","receivedAt":"2014-06-30T16:58:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These two are almost the same function, with the exception\nthat has_extension only matches if there is content before\nthe suffix. So ends_with(\".exe\", \".exe\") is true, but\nhas_extension would not be.\n\nThis distinction does not matter to any of the callers,\nthough, and we can just replace uses of has_extension with\nends_with. We prefer the \"ends_with\" name because it is more\ngeneric, and there is nothing about the function that\nrequires it to be used for file extensions.\n\nSigned-off-by: Jeff King \n---\n builtin/index-pack.c | 4 ++--\n builtin/verify-pack.c | 4 ++--\n git-compat-util.h | 7 -------\n help.c | 2 +-\n refs.c | 4 ++--\n sha1_file.c | 10 +++++-----\n 6 files changed, 12 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 18f57de..46376b6 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1603,7 +1603,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"--fix-thin cannot be used without --stdin\"));\n \tif (!index_name && pack_name) {\n \t\tint len = strlen(pack_name);\n-\t\tif (!has_extension(pack_name, \".pack\"))\n+\t\tif (!ends_with(pack_name, \".pack\"))\n \t\t\tdie(_(\"packfile name '%s' does not end with '.pack'\"),\n \t\t\t pack_name);\n \t\tindex_name_buf = xmalloc(len);\n@@ -1613,7 +1613,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t}\n \tif (keep_msg && !keep_name && pack_name) {\n \t\tint len = strlen(pack_name);\n-\t\tif (!has_extension(pack_name, \".pack\"))\n+\t\tif (!ends_with(pack_name, \".pack\"))\n \t\t\tdie(_(\"packfile name '%s' does not end with '.pack'\"),\n \t\t\t pack_name);\n \t\tkeep_name_buf = xmalloc(len);\ndiff --git a/builtin/verify-pack.c b/builtin/verify-pack.c\nindex 66cd2df..2fd29ce 100644\n--- a/builtin/verify-pack.c\n+++ b/builtin/verify-pack.c\n@@ -27,9 +27,9 @@ static int verify_one_pack(const char *path, unsigned int flags)\n \t * normalize these forms to \"foo.pack\" for \"index-pack --verify\".\n \t */\n \tstrbuf_addstr(&arg, path);\n-\tif (has_extension(arg.buf, \".idx\"))\n+\tif (ends_with(arg.buf, \".idx\"))\n \t\tstrbuf_splice(&arg, arg.len - 3, 3, \"pack\", 4);\n-\telse if (!has_extension(arg.buf, \".pack\"))\n+\telse if (!ends_with(arg.buf, \".pack\"))\n \t\tstrbuf_add(&arg, \".pack\", 5);\n \targv[2] = arg.buf;\n \ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 4cfde49..8cf79ae 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -594,13 +594,6 @@ static inline size_t xsize_t(off_t len)\n \treturn (size_t)len;\n }\n \n-static inline int has_extension(const char *filename, const char *ext)\n-{\n-\tsize_t len = strlen(filename);\n-\tsize_t extlen = strlen(ext);\n-\treturn len > extlen && !memcmp(filename + len - extlen, ext, extlen);\n-}\n-\n /* in ctype.c, for kwset users */\n extern const char tolower_trans_tbl[256];\n \ndiff --git a/help.c b/help.c\nindex b266b09..372728f 100644\n--- a/help.c\n+++ b/help.c\n@@ -156,7 +156,7 @@ static void list_commands_in_dir(struct cmdnames *cmds,\n \t\t\tcontinue;\n \n \t\tentlen = strlen(de->d_name) - prefix_len;\n-\t\tif (has_extension(de->d_name, \".exe\"))\n+\t\tif (ends_with(de->d_name, \".exe\"))\n \t\t\tentlen -= 4;\n \n \t\tadd_cmdname(cmds, de->d_name + prefix_len, entlen);\ndiff --git a/refs.c b/refs.c\nindex dc45774..e31adfd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1162,7 +1162,7 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)\n \n \t\tif (de->d_name[0] == '.')\n \t\t\tcontinue;\n-\t\tif (has_extension(de->d_name, \".lock\"))\n+\t\tif (ends_with(de->d_name, \".lock\"))\n \t\t\tcontinue;\n \t\tstrbuf_addstr(&refname, de->d_name);\n \t\trefdir = *refs->name\n@@ -3233,7 +3233,7 @@ static int do_for_each_reflog(struct strbuf *name, each_ref_fn fn, void *cb_data\n \n \t\tif (de->d_name[0] == '.')\n \t\t\tcontinue;\n-\t\tif (has_extension(de->d_name, \".lock\"))\n+\t\tif (ends_with(de->d_name, \".lock\"))\n \t\t\tcontinue;\n \t\tstrbuf_addstr(name, de->d_name);\n \t\tif (stat(git_path(\"logs/%s\", name->buf), &st) < 0) {\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 394fa45..93b794f 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1204,7 +1204,7 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\tstrbuf_setlen(&path, dirnamelen);\n \t\tstrbuf_addstr(&path, de->d_name);\n \n-\t\tif (has_extension(de->d_name, \".idx\")) {\n+\t\tif (ends_with(de->d_name, \".idx\")) {\n \t\t\t/* Don't reopen a pack we already have. */\n \t\t\tfor (p = packed_git; p; p = p->next) {\n \t\t\t\tif (!memcmp(path.buf, p->pack_name, path.len - 4))\n@@ -1222,10 +1222,10 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\tif (!report_garbage)\n \t\t\tcontinue;\n \n-\t\tif (has_extension(de->d_name, \".idx\") ||\n-\t\t has_extension(de->d_name, \".pack\") ||\n-\t\t has_extension(de->d_name, \".bitmap\") ||\n-\t\t has_extension(de->d_name, \".keep\"))\n+\t\tif (ends_with(de->d_name, \".idx\") ||\n+\t\t ends_with(de->d_name, \".pack\") ||\n+\t\t ends_with(de->d_name, \".bitmap\") ||\n+\t\t ends_with(de->d_name, \".keep\"))\n \t\t\tstring_list_append(&garbage, path.buf);\n \t\telse\n \t\t\treport_garbage(\"garbage found\", path.buf);\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245202","messageId":"20140630165850.GE16637@sigill.intra.peff.net","threadId":"37017","inReplyTo":"20140630165526.GA15690@sigill.intra.peff.net","subject":"[PATCH 5/9] use strip_suffix instead of ends_with in simple cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-30T16:58:51Z","receivedAt":"2014-06-30T16:58:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When stripping a suffix like:\n\n if (ends_with(str, \"foo\"))\n\tbuf = xmemdupz(str, strlen(str) - 3);\n\nwe can instead use strip_suffix to avoid the constant 3,\nwhich must match the literal \"foo\" (we sometimes use\nstrlen(\"foo\") instead, but that means we are repeating\nourselves). The example above becomes:\n\n if (strip_suffix(str, \"foo\", &len))\n\tbuf = xmemdupz(str, len);\n\nThis also saves a strlen(), since we calculate the string\nlength when detecting the suffix.\n\nNote that in some cases we also switch from xstrndup to\nxmemdupz, which saves a further strlen call.\n\nSigned-off-by: Jeff King \n---\n builtin/remote.c | 13 +++++++------\n builtin/repack.c | 5 ++---\n connected.c | 6 +++---\n help.c | 5 ++---\n 4 files changed, 14 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex c9102e8..7c2999e 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -265,16 +265,17 @@ static int config_read_branches(const char *key, const char *value, void *cb)\n \t\tstruct string_list_item *item;\n \t\tstruct branch_info *info;\n \t\tenum { REMOTE, MERGE, REBASE } type;\n+\t\tsize_t key_len;\n \n \t\tkey += 7;\n-\t\tif (ends_with(key, \".remote\")) {\n-\t\t\tname = xstrndup(key, strlen(key) - 7);\n+\t\tif (strip_suffix(key, \".remote\", &key_len)) {\n+\t\t\tname = xmemdupz(key, key_len);\n \t\t\ttype = REMOTE;\n-\t\t} else if (ends_with(key, \".merge\")) {\n-\t\t\tname = xstrndup(key, strlen(key) - 6);\n+\t\t} else if (strip_suffix(key, \".merge\", &key_len)) {\n+\t\t\tname = xmemdupz(key, key_len);\n \t\t\ttype = MERGE;\n-\t\t} else if (ends_with(key, \".rebase\")) {\n-\t\t\tname = xstrndup(key, strlen(key) - 7);\n+\t\t} else if (strip_suffix(key, \".rebase\", &key_len)) {\n+\t\t\tname = xmemdupz(key, key_len);\n \t\t\ttype = REBASE;\n \t\t} else\n \t\t\treturn 0;\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex ff2216a..a77e743 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -83,16 +83,15 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list)\n \tDIR *dir;\n \tstruct dirent *e;\n \tchar *fname;\n-\tsize_t len;\n \n \tif (!(dir = opendir(packdir)))\n \t\treturn;\n \n \twhile ((e = readdir(dir)) != NULL) {\n-\t\tif (!ends_with(e->d_name, \".pack\"))\n+\t\tsize_t len;\n+\t\tif (!strip_suffix(e->d_name, \".pack\", &len))\n \t\t\tcontinue;\n \n-\t\tlen = strlen(e->d_name) - strlen(\".pack\");\n \t\tfname = xmemdupz(e->d_name, len);\n \n \t\tif (!file_exists(mkpath(\"%s/%s.keep\", packdir, fname)))\ndiff --git a/connected.c b/connected.c\nindex be0253e..dae9c99 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -31,6 +31,7 @@ static int check_everything_connected_real(sha1_iterate_fn fn,\n \tunsigned char sha1[20];\n \tint err = 0, ac = 0;\n \tstruct packed_git *new_pack = NULL;\n+\tsize_t base_len;\n \n \tif (fn(cb_data, sha1))\n \t\treturn err;\n@@ -38,10 +39,9 @@ static int check_everything_connected_real(sha1_iterate_fn fn,\n \tif (transport && transport->smart_options &&\n \t transport->smart_options->self_contained_and_connected &&\n \t transport->pack_lockfile &&\n-\t ends_with(transport->pack_lockfile, \".keep\")) {\n+\t strip_suffix(transport->pack_lockfile, \".keep\", &base_len)) {\n \t\tstruct strbuf idx_file = STRBUF_INIT;\n-\t\tstrbuf_addstr(&idx_file, transport->pack_lockfile);\n-\t\tstrbuf_setlen(&idx_file, idx_file.len - 5); /* \".keep\" */\n+\t\tstrbuf_add(&idx_file, transport->pack_lockfile, base_len);\n \t\tstrbuf_addstr(&idx_file, \".idx\");\n \t\tnew_pack = add_packed_git(idx_file.buf, idx_file.len, 1);\n \t\tstrbuf_release(&idx_file);\ndiff --git a/help.c b/help.c\nindex 372728f..97567c4 100644\n--- a/help.c\n+++ b/help.c\n@@ -145,7 +145,7 @@ static void list_commands_in_dir(struct cmdnames *cmds,\n \tlen = buf.len;\n \n \twhile ((de = readdir(dir)) != NULL) {\n-\t\tint entlen;\n+\t\tsize_t entlen;\n \n \t\tif (!starts_with(de->d_name, prefix))\n \t\t\tcontinue;\n@@ -156,8 +156,7 @@ static void list_commands_in_dir(struct cmdnames *cmds,\n \t\t\tcontinue;\n \n \t\tentlen = strlen(de->d_name) - prefix_len;\n-\t\tif (ends_with(de->d_name, \".exe\"))\n-\t\t\tentlen -= 4;\n+\t\tstrip_suffix(de->d_name, \".exe\", &entlen);\n \n \t\tadd_cmdname(cmds, de->d_name + prefix_len, entlen);\n \t}\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245203","messageId":"20140630165910.GF16637@sigill.intra.peff.net","threadId":"37017","inReplyTo":"20140630165526.GA15690@sigill.intra.peff.net","subject":"[PATCH 6/9] index-pack: use strip_suffix to avoid magic numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-30T16:59:10Z","receivedAt":"2014-06-30T16:59:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We also switch to using strbufs, which lets us avoid the\npotentially dangerous combination of a manual malloc\nfollowed by a strcpy.\n\nSigned-off-by: Jeff King \n---\n builtin/index-pack.c | 29 ++++++++++++++---------------\n 1 file changed, 14 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 46376b6..d4b77fd 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1505,7 +1505,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tconst char *curr_index;\n \tconst char *index_name = NULL, *pack_name = NULL;\n \tconst char *keep_name = NULL, *keep_msg = NULL;\n-\tchar *index_name_buf = NULL, *keep_name_buf = NULL;\n+\tstruct strbuf index_name_buf = STRBUF_INIT,\n+\t\t keep_name_buf = STRBUF_INIT;\n \tstruct pack_idx_entry **idx_objects;\n \tstruct pack_idx_option opts;\n \tunsigned char pack_sha1[20];\n@@ -1602,24 +1603,22 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tif (fix_thin_pack && !from_stdin)\n \t\tdie(_(\"--fix-thin cannot be used without --stdin\"));\n \tif (!index_name && pack_name) {\n-\t\tint len = strlen(pack_name);\n-\t\tif (!ends_with(pack_name, \".pack\"))\n+\t\tsize_t len;\n+\t\tif (!strip_suffix(pack_name, \".pack\", &len))\n \t\t\tdie(_(\"packfile name '%s' does not end with '.pack'\"),\n \t\t\t pack_name);\n-\t\tindex_name_buf = xmalloc(len);\n-\t\tmemcpy(index_name_buf, pack_name, len - 5);\n-\t\tstrcpy(index_name_buf + len - 5, \".idx\");\n-\t\tindex_name = index_name_buf;\n+\t\tstrbuf_add(&index_name_buf, pack_name, len);\n+\t\tstrbuf_addstr(&index_name_buf, \".idx\");\n+\t\tindex_name = index_name_buf.buf;\n \t}\n \tif (keep_msg && !keep_name && pack_name) {\n-\t\tint len = strlen(pack_name);\n-\t\tif (!ends_with(pack_name, \".pack\"))\n+\t\tsize_t len;\n+\t\tif (!strip_suffix(pack_name, \".pack\", &len))\n \t\t\tdie(_(\"packfile name '%s' does not end with '.pack'\"),\n \t\t\t pack_name);\n-\t\tkeep_name_buf = xmalloc(len);\n-\t\tmemcpy(keep_name_buf, pack_name, len - 5);\n-\t\tstrcpy(keep_name_buf + len - 5, \".keep\");\n-\t\tkeep_name = keep_name_buf;\n+\t\tstrbuf_add(&keep_name_buf, pack_name, len);\n+\t\tstrbuf_addstr(&keep_name_buf, \".idx\");\n+\t\tkeep_name = keep_name_buf.buf;\n \t}\n \tif (verify) {\n \t\tif (!index_name)\n@@ -1667,8 +1666,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \telse\n \t\tclose(input_fd);\n \tfree(objects);\n-\tfree(index_name_buf);\n-\tfree(keep_name_buf);\n+\tstrbuf_release(&index_name_buf);\n+\tstrbuf_release(&keep_name_buf);\n \tif (pack_name == NULL)\n \t\tfree((void *) curr_pack);\n \tif (index_name == NULL)\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245204","messageId":"20140630170151.GG16637@sigill.intra.peff.net","threadId":"37017","inReplyTo":"20140630165526.GA15690@sigill.intra.peff.net","subject":"[PATCH 7/9] strbuf: implement strbuf_strip_suffix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-30T17:01:51Z","receivedAt":"2014-06-30T17:01:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"You can almost get away with just calling \"strip_suffix_mem\"\non a strbuf's buf and len fields. But we also need to move\nthe NUL-terminator to satisfy strbuf's invariants. Let's\nprovide a convenience wrapper that handles this.\n\nSigned-off-by: Jeff King \n---\nI called strbuf_setlen here because it seemed sensible to use that as an\nopaque building block, but we are violating the invariant here for a\nmoment by setting sb->len for a moment. I think that is fine, as this is\na strbuf function, and can violate the invariant for a moment if it\nwants to.\n\nBut if we care, we can keep a separate \"size_t len\" variable, and\nstrbuf_setlen to that.\n\nOr we can make it less opaque, and replace the setlen with\n\"sb->buf[sb->len] = 0\".\n\n strbuf.h | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex e9ad03e..ec11742 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -49,6 +49,15 @@ extern int strbuf_reencode(struct strbuf *sb, const char *from, const char *to);\n extern void strbuf_tolower(struct strbuf *sb);\n extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);\n \n+static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n+{\n+\tif (strip_suffix_mem(sb->buf, &sb->len, suffix)) {\n+\t\tstrbuf_setlen(sb, sb->len);\n+\t\treturn 1;\n+\t} else\n+\t\treturn 0;\n+}\n+\n /*\n * Split str (of length slen) at the specified terminator character.\n * Return a null-terminated array of pointers to strbuf objects\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245205","messageId":"20140630170205.GH16637@sigill.intra.peff.net","threadId":"37017","inReplyTo":"20140630165526.GA15690@sigill.intra.peff.net","subject":"[PATCH 8/9] verify-pack: use strbuf_strip_suffix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-30T17:02:05Z","receivedAt":"2014-06-30T17:02:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In this code, we try to convert both \"foo.idx\" and \"foo\"\ninto \"foo.pack\". By stripping the suffix, we can avoid a\nconfusing use of strbuf_splice, and make it clear that both\ncases are adding \".pack\" to the end.\n\nSigned-off-by: Jeff King \n---\n builtin/verify-pack.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/verify-pack.c b/builtin/verify-pack.c\nindex 2fd29ce..972579f 100644\n--- a/builtin/verify-pack.c\n+++ b/builtin/verify-pack.c\n@@ -27,10 +27,9 @@ static int verify_one_pack(const char *path, unsigned int flags)\n \t * normalize these forms to \"foo.pack\" for \"index-pack --verify\".\n \t */\n \tstrbuf_addstr(&arg, path);\n-\tif (ends_with(arg.buf, \".idx\"))\n-\t\tstrbuf_splice(&arg, arg.len - 3, 3, \"pack\", 4);\n-\telse if (!ends_with(arg.buf, \".pack\"))\n-\t\tstrbuf_add(&arg, \".pack\", 5);\n+\tif (strbuf_strip_suffix(&arg, \".idx\") ||\n+\t !ends_with(arg.buf, \".pack\"))\n+\t\tstrbuf_addstr(&arg, \".pack\");\n \targv[2] = arg.buf;\n \n \tmemset(&index_pack, 0, sizeof(index_pack));\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245206","messageId":"20140630170403.GI16637@sigill.intra.peff.net","threadId":"37017","inReplyTo":"20140630165526.GA15690@sigill.intra.peff.net","subject":"[PATCH 9/9] prepare_packed_git_one: refactor duplicate-pack check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-30T17:04:03Z","receivedAt":"2014-06-30T17:04:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are reloading the list of packs, we check whether a\nparticular pack has been loaded. This is slightly tricky,\nbecause we load packs based on the presence of their \".idx\"\nfiles, but record the name of the matching \".pack\" file.\nTherefore we want to compare their bases.\n\nThe existing code stripped off \".idx\" from a file we found,\nthen compared that whole base length to strings containing\nthe \".pack\" version. This meant we could end up comparing\nbytes past what the \".pack\" string contained, if the \".idx\"\nfile name was much longer.\n\nIn practice, it worked OK because memcmp would end up seeing\na difference in the two strings and would return early\nbefore hitting the full length. However, memcmp may\nsometimes read extra bytes past a difference (e.g., because\nit is comparing 64-bit words), or is even free to compare in\nreverse order.\n\nFurthermore, our memcmp made no guarantees that we matched\nthe whole pack name, up to \".pack\". So \"foo.idx\" would match\n\"foo-bar.pack\", which is wrong (but does not typically\nhappen, because our pack names have a fixed size).\n\nWe can fix both issues, avoid magic numbers, and document\nthat we expect to compare against a string with \".pack\" by\nusing strip_suffix.\n\nSigned-off-by: Jeff King \n---\nThis is a teeny bit less efficient than it could be, because we are\nverifying our assumption at run-time that each pack name ends in\n\".pack\". I'd venture to say if we cared about efficiency here, the low\nhanging fruit would be to avoid the O(n^2) loop to find duplicate pack\nnames in the first place.\n\n sha1_file.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 93b794f..129a4c5 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1197,6 +1197,7 @@ static void prepare_packed_git_one(char *objdir, int local)\n \tdirnamelen = path.len;\n \twhile ((de = readdir(dir)) != NULL) {\n \t\tstruct packed_git *p;\n+\t\tsize_t base_len;\n \n \t\tif (is_dot_or_dotdot(de->d_name))\n \t\t\tcontinue;\n@@ -1204,10 +1205,14 @@ static void prepare_packed_git_one(char *objdir, int local)\n \t\tstrbuf_setlen(&path, dirnamelen);\n \t\tstrbuf_addstr(&path, de->d_name);\n \n-\t\tif (ends_with(de->d_name, \".idx\")) {\n+\t\tbase_len = path.len;\n+\t\tif (strip_suffix_mem(path.buf, &base_len, \".idx\")) {\n \t\t\t/* Don't reopen a pack we already have. */\n \t\t\tfor (p = packed_git; p; p = p->next) {\n-\t\t\t\tif (!memcmp(path.buf, p->pack_name, path.len - 4))\n+\t\t\t\tsize_t len;\n+\t\t\t\tif (strip_suffix(p->pack_name, \".pack\", &len) &&\n+\t\t\t\t len == base_len &&\n+\t\t\t\t !memcmp(p->pack_name, path.buf, len))\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t\tif (p == NULL &&\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"245220","messageId":"CAPc5daW_WBBwKaXvmY7Htd1eL3NWiUTDQK-y=2M=_NWuozxSEg@mail.gmail.com","threadId":"37017","inReplyTo":"20140630165526.GA15690@sigill.intra.peff.net","subject":"Re: [PATCH 0/9] add strip_suffix as an alternative to ends_with","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-01T01:00:37Z","receivedAt":"2014-07-01T01:00:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Mon, Jun 30, 2014 at 9:55 AM, Jeff King wrote:\n> Here's a series to do for ends_with what the recent skip_prefix series\n> did for starts_with. Namely: drop some magic numbers and repeated string\n> literals, and hopefully make things more readable.\n>\n> The first patch is René's patch 1/2, with the leak fix from Duy and typo\n> fixes in the commit message from me. The rest are new, and replace the\n> changes to prepare_packed_git_one done elsewhere in the thread.\n>\n> [1/9]: sha1_file: replace PATH_MAX buffer with strbuf in prepare_packed_git_one()\n> [2/9]: add strip_suffix function\n> [3/9]: implement ends_with via strip_suffix\n> [4/9]: replace has_extension with ends_with\n> [5/9]: use strip_suffix instead of ends_with in simple cases\n> [6/9]: index-pack: use strip_suffix to avoid magic numbers\n> [7/9]: strbuf: implement strbuf_strip_suffix\n> [8/9]: verify-pack: use strbuf_strip_suffix\n> [9/9]: prepare_packed_git_one: refactor duplicate-pack check\n\nLooked very sensible. Thanks all ;-)\n"},{"id":"245301","messageId":"xmqqlhsbiwmz.fsf@gitster.dls.corp.google.com","threadId":"37017","inReplyTo":"20140630165751.GB16637@sigill.intra.peff.net","subject":"Re: [PATCH 2/9] add strip_suffix function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-02T15:54:44Z","receivedAt":"2014-07-02T15:54:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> For that reason, the \"mem\" form puts its length parameter\n> next to the buffer (since they are a pair), and the string\n> form puts it at the end (since it is an out-parameter). The\n> compiler can notice when you get the order wrong, which\n> should help prevent writing one when you meant the other.\n\nVery sensible consideration. I like commits that careful thinking\nbehind them shows through them.\n\n> Signed-off-by: Jeff King \n> ---\n> I hope the word \"strip\" is OK, as it does not actually NUL-terminate\n> (doing so would make it unusable for many cases). Between the comment\n> below and the \"const\" in the parameter, I think it should be pretty\n> clear that it does not touch the string. And I could not think of a\n> better word.\n\nAll other words I can think of offhand, trim, chomp, etc., hint\nshortening of the input string, and by definition shortening of a\nstring implies NUL-termination.\n\nThe \"mem\" variant deals with a counted string, however, so its\nshortening implies NUL-termination a lot less [*1*] and should be\nfine.\n\nIf we want to avoid implying NUL-termination, the only way to do so\nwould be to use wording that does not hint shortening. At least for\nthe C-string variant, which is measuring the length of the basename\npart (i.e. `basename $str $suffix`) without touching anything else,\ne.g. basename_length(\"hello.c\", \".c\", &len), but at the same time\nyou want to make it a boolean to signal if the string ends with the\nsuffix, so perhaps has_suffix(\"hello.c\", \".c\", &len)?\n\n[Footnote]\n\n *1* ... but not entirely, because we often NUL-terminate even\n counted strings (the buffer returned from read_sha1_file() and\n the payload of strbuf are two examples).\n\n> git-compat-util.h | 27 +++++++++++++++++++++++++++\n> 1 file changed, 27 insertions(+)\n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index b6f03b3..d044c42 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -358,6 +358,33 @@ static inline const char *skip_prefix(const char *str, const char *prefix)\n> \treturn NULL;\n> }\n> \n> +/*\n> + * If buf ends with suffix, return 1 and subtract the length of the suffix\n> + * from *len. Otherwise, return 0 and leave *len untouched.\n> + */\n> +static inline int strip_suffix_mem(const char *buf, size_t *len,\n> +\t\t\t\t const char *suffix)\n> +{\n> +\tsize_t suflen = strlen(suffix);\n> +\tif (*len < suflen || memcmp(buf + (*len - suflen), suffix, suflen))\n> +\t\treturn 0;\n> +\t*len -= suflen;\n> +\treturn 1;\n> +}\n> +\n> +/*\n> + * If str ends with suffix, return 1 and set *len to the size of the string\n> + * without the suffix. Otherwise, return 0 and set *len to the size of the\n> + * string.\n> + *\n> + * Note that we do _not_ NUL-terminate str to the new length.\n> + */\n> +static inline int strip_suffix(const char *str, const char *suffix, size_t *len)\n> +{\n> +\t*len = strlen(str);\n> +\treturn strip_suffix_mem(str, len, suffix);\n> +}\n> +\n> #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)\n> \n> #ifndef PROT_READ\n"},{"id":"245307","messageId":"20140702163832.GC14404@sigill.intra.peff.net","threadId":"37017","inReplyTo":"xmqqlhsbiwmz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/9] add strip_suffix function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-02T16:38:32Z","receivedAt":"2014-07-02T16:38:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 02, 2014 at 08:54:44AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > For that reason, the \"mem\" form puts its length parameter\n> > next to the buffer (since they are a pair), and the string\n> > form puts it at the end (since it is an out-parameter). The\n> > compiler can notice when you get the order wrong, which\n> > should help prevent writing one when you meant the other.\n> \n> Very sensible consideration. I like commits that careful thinking\n> behind them shows through them.\n\nI would like to take credit for advanced thinking, but I actually did\nwhat felt natural, and only noticed the \"compiler will tell you when you\nare wrong\" effect when I got it wrong while writing a later patch in the\nseries. :)\n\n> If we want to avoid implying NUL-termination, the only way to do so\n> would be to use wording that does not hint shortening. At least for\n> the C-string variant, which is measuring the length of the basename\n> part (i.e. `basename $str $suffix`) without touching anything else,\n> e.g. basename_length(\"hello.c\", \".c\", &len), but at the same time\n> you want to make it a boolean to signal if the string ends with the\n> suffix, so perhaps has_suffix(\"hello.c\", \".c\", &len)?\n\nI think that invites some confusion with \"ends_with\", which is the same\nthing (but just does not take the \"len\" parameter). We could just add\nthis feature to ends_with, and ask callers who do not care to pass NULL,\nbut that makes those call sites uglier.\n\nHaving had a day to mull it over, and having read your email, I think I\nstill prefer strip_suffix.\n\n-Peff\n"},{"id":"245316","messageId":"xmqqwqbvhd53.fsf@gitster.dls.corp.google.com","threadId":"37017","inReplyTo":"20140702163832.GC14404@sigill.intra.peff.net","subject":"Re: [PATCH 2/9] add strip_suffix function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-02T17:41:12Z","receivedAt":"2014-07-02T17:41:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Having had a day to mull it over, and having read your email, I think I\n> still prefer strip_suffix.\n\nThat's OK. I was only trying to help you explore different avenues,\nwithout having strong preference myself either way.\n"}]}