Re: [PATCH] parse_dirstat_params(): use string_list to split comma-separated string

2012-10-31 Thread Matt Kraai
On Wed, Oct 31, 2012 at 10:06:36AM -0400, Jeff King wrote:
> On Tue, Oct 30, 2012 at 06:43:51PM +, Matt Kraai wrote:
> 
> > Michael Haggerty  alum.mit.edu> writes:
> > > + if (*params_copy)
> > 
> > params_copy is set to the value returned by xstrdup, which cannot be NULL.
> > This check can be removed and if params_string can be NULL, it should be
> > checked before being passed to xstrdup.
> 
> If you are referring to the last line, isn't it checking whether the
> string is empty, not NULL?

Oops, you're right.  Sorry for misreading that.

-- 
Matt Kraai
https://ftbfs.org/kraai
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] parse_dirstat_params(): use string_list to split comma-separated string

2012-10-31 Thread Jeff King
On Tue, Oct 30, 2012 at 06:43:51PM +, Matt Kraai wrote:

> Michael Haggerty  alum.mit.edu> writes:
> ...
> > -static int parse_dirstat_params(struct diff_options *options, const char 
> > ...
> > +static int parse_dirstat_params(struct diff_options *options, const char 
> > ...
> > struct strbuf *errmsg)
> >  {
> > -   const char *p = params;
> > -   int p_len, ret = 0;
> > +   char *params_copy = xstrdup(params_string);
> > +   struct string_list params = STRING_LIST_INIT_NODUP;
> > +   int ret = 0;
> > +   int i;
> > 
> > -   while (*p) {
> > -   p_len = strchrnul(p, ',') - p;
> > -   if (!memcmp(p, "changes", p_len)) {
> > +   if (*params_copy)
> 
> params_copy is set to the value returned by xstrdup, which cannot be NULL.
> This check can be removed and if params_string can be NULL, it should be
> checked before being passed to xstrdup.

If you are referring to the last line, isn't it checking whether the
string is empty, not NULL?

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] parse_dirstat_params(): use string_list to split comma-separated string

2012-10-30 Thread Matt Kraai
Michael Haggerty  alum.mit.edu> writes:
...
> -static int parse_dirstat_params(struct diff_options *options, const char ...
> +static int parse_dirstat_params(struct diff_options *options, const char ...
>   struct strbuf *errmsg)
>  {
> - const char *p = params;
> - int p_len, ret = 0;
> + char *params_copy = xstrdup(params_string);
> + struct string_list params = STRING_LIST_INIT_NODUP;
> + int ret = 0;
> + int i;
> 
> - while (*p) {
> - p_len = strchrnul(p, ',') - p;
> - if (!memcmp(p, "changes", p_len)) {
> + if (*params_copy)

params_copy is set to the value returned by xstrdup, which cannot be NULL.
This check can be removed and if params_string can be NULL, it should be
checked before being passed to xstrdup.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] parse_dirstat_params(): use string_list to split comma-separated string

2012-10-28 Thread Michael Haggerty
Use string_list_split_in_place() to split the comma-separated
parameters string.  This simplifies the code and also fixes a bug: the
old code made calls like

memcmp(p, "lines", p_len)

which needn't work if p_len is different than the length of the
constant string (and could illegally access memory if p_len is larger
than the length of the constant string).

When p_len was less than the length of the constant string, the old
code would have allowed some abbreviations to be accepted (e.g., "cha"
for "changes") but this seems to have been a bug rather than a
feature, because (1) it is not documented; (2) no attempt was made to
handle ambiguous abbreviations, like "c" for "changes" vs
"cumulative".

Signed-off-by: Michael Haggerty 
---
 diff.c | 40 +---
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/diff.c b/diff.c
index 35d3f07..a9cc8a9 100644
--- a/diff.c
+++ b/diff.c
@@ -15,6 +15,7 @@
 #include "sigchain.h"
 #include "submodule.h"
 #include "ll-merge.h"
+#include "string-list.h"
 
 #ifdef NO_FAST_WORKING_DIRECTORY
 #define FAST_WORKING_DIRECTORY 0
@@ -68,26 +69,30 @@ static int parse_diff_color_slot(const char *var, int ofs)
return -1;
 }
 
-static int parse_dirstat_params(struct diff_options *options, const char 
*params,
+static int parse_dirstat_params(struct diff_options *options, const char 
*params_string,
struct strbuf *errmsg)
 {
-   const char *p = params;
-   int p_len, ret = 0;
+   char *params_copy = xstrdup(params_string);
+   struct string_list params = STRING_LIST_INIT_NODUP;
+   int ret = 0;
+   int i;
 
-   while (*p) {
-   p_len = strchrnul(p, ',') - p;
-   if (!memcmp(p, "changes", p_len)) {
+   if (*params_copy)
+   string_list_split_in_place(¶ms, params_copy, ',', -1);
+   for (i = 0; i < params.nr; i++) {
+   const char *p = params.items[i].string;
+   if (!strcmp(p, "changes")) {
DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
-   } else if (!memcmp(p, "lines", p_len)) {
+   } else if (!strcmp(p, "lines")) {
DIFF_OPT_SET(options, DIRSTAT_BY_LINE);
DIFF_OPT_CLR(options, DIRSTAT_BY_FILE);
-   } else if (!memcmp(p, "files", p_len)) {
+   } else if (!strcmp(p, "files")) {
DIFF_OPT_CLR(options, DIRSTAT_BY_LINE);
DIFF_OPT_SET(options, DIRSTAT_BY_FILE);
-   } else if (!memcmp(p, "noncumulative", p_len)) {
+   } else if (!strcmp(p, "noncumulative")) {
DIFF_OPT_CLR(options, DIRSTAT_CUMULATIVE);
-   } else if (!memcmp(p, "cumulative", p_len)) {
+   } else if (!strcmp(p, "cumulative")) {
DIFF_OPT_SET(options, DIRSTAT_CUMULATIVE);
} else if (isdigit(*p)) {
char *end;
@@ -99,24 +104,21 @@ static int parse_dirstat_params(struct diff_options 
*options, const char *params
while (isdigit(*++end))
; /* nothing */
}
-   if (end - p == p_len)
+   if (!*end)
options->dirstat_permille = permille;
else {
-   strbuf_addf(errmsg, _("  Failed to parse 
dirstat cut-off percentage '%.*s'\n"),
-   p_len, p);
+   strbuf_addf(errmsg, _("  Failed to parse 
dirstat cut-off percentage '%s'\n"),
+   p);
ret++;
}
} else {
-   strbuf_addf(errmsg, _("  Unknown dirstat parameter 
'%.*s'\n"),
-   p_len, p);
+   strbuf_addf(errmsg, _("  Unknown dirstat parameter 
'%s'\n"), p);
ret++;
}
 
-   p += p_len;
-
-   if (*p)
-   p++; /* more parameters, swallow separator */
}
+   string_list_clear(¶ms, 0);
+   free(params_copy);
return ret;
 }
 
-- 
1.8.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html