{"thread":{"id":"31962","subject":"[PATCH] parse_dirstat_params(): use string_list to split comma-separated string","startedAt":"2012-10-28T16:50:54Z","lastAt":"2012-10-31T15:48:07Z","messageCount":4,"participants":["Michael Haggerty","Matt Kraai","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"202058","messageId":"1351443054-10472-1-git-send-email-mhagger@alum.mit.edu","threadId":"31962","inReplyTo":null,"subject":"[PATCH] parse_dirstat_params(): use string_list to split comma-separated string","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-10-28T16:50:54Z","receivedAt":"2012-10-28T16:50:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Use string_list_split_in_place() to split the comma-separated\nparameters string.  This simplifies the code and also fixes a bug: the\nold code made calls like\n\n    memcmp(p, \"lines\", p_len)\n\nwhich needn't work if p_len is different than the length of the\nconstant string (and could illegally access memory if p_len is larger\nthan the length of the constant string).\n\nWhen p_len was less than the length of the constant string, the old\ncode would have allowed some abbreviations to be accepted (e.g., \"cha\"\nfor \"changes\") but this seems to have been a bug rather than a\nfeature, because (1) it is not documented; (2) no attempt was made to\nhandle ambiguous abbreviations, like \"c\" for \"changes\" vs\n\"cumulative\".\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n diff.c | 40 +++++++++++++++++++++-------------------\n 1 file changed, 21 insertions(+), 19 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 35d3f07..a9cc8a9 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -15,6 +15,7 @@\n #include \"sigchain.h\"\n #include \"submodule.h\"\n #include \"ll-merge.h\"\n+#include \"string-list.h\"\n \n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -68,26 +69,30 @@ static int parse_diff_color_slot(const char *var, int ofs)\n \treturn -1;\n }\n \n-static int parse_dirstat_params(struct diff_options *options, const char *params,\n+static int parse_dirstat_params(struct diff_options *options, const char *params_string,\n \t\t\t\tstruct strbuf *errmsg)\n {\n-\tconst char *p = params;\n-\tint p_len, ret = 0;\n+\tchar *params_copy = xstrdup(params_string);\n+\tstruct string_list params = STRING_LIST_INIT_NODUP;\n+\tint ret = 0;\n+\tint i;\n \n-\twhile (*p) {\n-\t\tp_len = strchrnul(p, ',') - p;\n-\t\tif (!memcmp(p, \"changes\", p_len)) {\n+\tif (*params_copy)\n+\t\tstring_list_split_in_place(&params, params_copy, ',', -1);\n+\tfor (i = 0; i < params.nr; i++) {\n+\t\tconst char *p = params.items[i].string;\n+\t\tif (!strcmp(p, \"changes\")) {\n \t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_LINE);\n \t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_FILE);\n-\t\t} else if (!memcmp(p, \"lines\", p_len)) {\n+\t\t} else if (!strcmp(p, \"lines\")) {\n \t\t\tDIFF_OPT_SET(options, DIRSTAT_BY_LINE);\n \t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_FILE);\n-\t\t} else if (!memcmp(p, \"files\", p_len)) {\n+\t\t} else if (!strcmp(p, \"files\")) {\n \t\t\tDIFF_OPT_CLR(options, DIRSTAT_BY_LINE);\n \t\t\tDIFF_OPT_SET(options, DIRSTAT_BY_FILE);\n-\t\t} else if (!memcmp(p, \"noncumulative\", p_len)) {\n+\t\t} else if (!strcmp(p, \"noncumulative\")) {\n \t\t\tDIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);\n-\t\t} else if (!memcmp(p, \"cumulative\", p_len)) {\n+\t\t} else if (!strcmp(p, \"cumulative\")) {\n \t\t\tDIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);\n \t\t} else if (isdigit(*p)) {\n \t\t\tchar *end;\n@@ -99,24 +104,21 @@ static int parse_dirstat_params(struct diff_options *options, const char *params\n \t\t\t\twhile (isdigit(*++end))\n \t\t\t\t\t; /* nothing */\n \t\t\t}\n-\t\t\tif (end - p == p_len)\n+\t\t\tif (!*end)\n \t\t\t\toptions->dirstat_permille = permille;\n \t\t\telse {\n-\t\t\t\tstrbuf_addf(errmsg, _(\"  Failed to parse dirstat cut-off percentage '%.*s'\\n\"),\n-\t\t\t\t\t    p_len, p);\n+\t\t\t\tstrbuf_addf(errmsg, _(\"  Failed to parse dirstat cut-off percentage '%s'\\n\"),\n+\t\t\t\t\t    p);\n \t\t\t\tret++;\n \t\t\t}\n \t\t} else {\n-\t\t\tstrbuf_addf(errmsg, _(\"  Unknown dirstat parameter '%.*s'\\n\"),\n-\t\t\t\t    p_len, p);\n+\t\t\tstrbuf_addf(errmsg, _(\"  Unknown dirstat parameter '%s'\\n\"), p);\n \t\t\tret++;\n \t\t}\n \n-\t\tp += p_len;\n-\n-\t\tif (*p)\n-\t\t\tp++; /* more parameters, swallow separator */\n \t}\n+\tstring_list_clear(&params, 0);\n+\tfree(params_copy);\n \treturn ret;\n }\n \n-- \n1.8.0\n"},{"id":"202220","messageId":"loom.20121030T193428-242@post.gmane.org","threadId":"31962","inReplyTo":"1351443054-10472-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH] parse_dirstat_params(): use string_list to split comma-separated string","fromName":"Matt Kraai","fromEmail":"kraai@ftbfs.org","sentAt":"2012-10-30T18:43:51Z","receivedAt":"2012-10-30T18:43:51Z","isPatch":true,"sender":{"key":"kraai@ftbfs.org","avatar":null},"body":"Michael Haggerty <mhagger <at> alum.mit.edu> writes:\n...\n> -static int parse_dirstat_params(struct diff_options *options, const char ...\n> +static int parse_dirstat_params(struct diff_options *options, const char ...\n>  \t\t\t\tstruct strbuf *errmsg)\n>  {\n> -\tconst char *p = params;\n> -\tint p_len, ret = 0;\n> +\tchar *params_copy = xstrdup(params_string);\n> +\tstruct string_list params = STRING_LIST_INIT_NODUP;\n> +\tint ret = 0;\n> +\tint i;\n> \n> -\twhile (*p) {\n> -\t\tp_len = strchrnul(p, ',') - p;\n> -\t\tif (!memcmp(p, \"changes\", p_len)) {\n> +\tif (*params_copy)\n\nparams_copy is set to the value returned by xstrdup, which cannot be NULL.\nThis check can be removed and if params_string can be NULL, it should be\nchecked before being passed to xstrdup.\n"},{"id":"202312","messageId":"20121031140636.GA24291@sigill.intra.peff.net","threadId":"31962","inReplyTo":"loom.20121030T193428-242@post.gmane.org","subject":"Re: [PATCH] parse_dirstat_params(): use string_list to split comma-separated string","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-10-31T14:06:36Z","receivedAt":"2012-10-31T14:06:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 30, 2012 at 06:43:51PM +0000, Matt Kraai wrote:\n\n> Michael Haggerty <mhagger <at> alum.mit.edu> writes:\n> ...\n> > -static int parse_dirstat_params(struct diff_options *options, const char ...\n> > +static int parse_dirstat_params(struct diff_options *options, const char ...\n> >  \t\t\t\tstruct strbuf *errmsg)\n> >  {\n> > -\tconst char *p = params;\n> > -\tint p_len, ret = 0;\n> > +\tchar *params_copy = xstrdup(params_string);\n> > +\tstruct string_list params = STRING_LIST_INIT_NODUP;\n> > +\tint ret = 0;\n> > +\tint i;\n> > \n> > -\twhile (*p) {\n> > -\t\tp_len = strchrnul(p, ',') - p;\n> > -\t\tif (!memcmp(p, \"changes\", p_len)) {\n> > +\tif (*params_copy)\n> \n> params_copy is set to the value returned by xstrdup, which cannot be NULL.\n> This check can be removed and if params_string can be NULL, it should be\n> checked before being passed to xstrdup.\n\nIf you are referring to the last line, isn't it checking whether the\nstring is empty, not NULL?\n\n-Peff\n"},{"id":"202332","messageId":"20121031154807.GA19483@ftbfs.org","threadId":"31962","inReplyTo":"20121031140636.GA24291@sigill.intra.peff.net","subject":"Re: [PATCH] parse_dirstat_params(): use string_list to split comma-separated string","fromName":"Matt Kraai","fromEmail":"kraai@ftbfs.org","sentAt":"2012-10-31T15:48:07Z","receivedAt":"2012-10-31T15:48:07Z","isPatch":true,"sender":{"key":"kraai@ftbfs.org","avatar":null},"body":"On Wed, Oct 31, 2012 at 10:06:36AM -0400, Jeff King wrote:\n> On Tue, Oct 30, 2012 at 06:43:51PM +0000, Matt Kraai wrote:\n> \n> > Michael Haggerty <mhagger <at> alum.mit.edu> writes:\n> > > +\tif (*params_copy)\n> > \n> > params_copy is set to the value returned by xstrdup, which cannot be NULL.\n> > This check can be removed and if params_string can be NULL, it should be\n> > checked before being passed to xstrdup.\n> \n> If you are referring to the last line, isn't it checking whether the\n> string is empty, not NULL?\n\nOops, you're right.  Sorry for misreading that.\n\n-- \nMatt Kraai\nhttps://ftbfs.org/kraai\n"}]}