Subject: Re: [PATCH v7 04/19] fsck: Offer a function to demote fsck errors to warnings

From: Junio C Hamano <[email-masked]>
Date: Mon, 22 Jun 2015 10:37:48 -0700

Johannes Schindelin <[email-masked]> writes:

> diff --git a/fsck.c b/fsck.c
> index 1a3f7ce..e81a342 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -64,30 +64,29 @@ enum fsck_msg_id {
>  #undef MSG_ID
>  
>  #define STR(x) #x
> -#define MSG_ID(id, msg_type) { STR(id), FSCK_##msg_type },
> +#define MSG_ID(id, msg_type) { STR(id), NULL, FSCK_##msg_type },
>  static struct {
>  	const char *id_string;
> +	const char *lowercased;
>  	int msg_type;
>  } msg_id_info[FSCK_MSG_MAX + 1] = {
>  	FOREACH_MSG_ID(MSG_ID)
> -	{ NULL, -1 }
> +	{ NULL, NULL, -1 }
>  };
>  #undef MSG_ID
>  
>  static int parse_msg_id(const char *text)
>  {
> -	static char **lowercased;
>  	int i;
>  
> -	if (!lowercased) {
> +	if (!msg_id_info[0].lowercased) {
>  		/* convert id_string to lower case, without underscores. */
> -		lowercased = xmalloc(FSCK_MSG_MAX * sizeof(*lowercased));
>  		for (i = 0; i < FSCK_MSG_MAX; i++) {
>  			const char *p = msg_id_info[i].id_string;
>  			int len = strlen(p);
>  			char *q = xmalloc(len);
>  
> -			lowercased[i] = q;
> +			msg_id_info[i].lowercased = q;
>  			while (*p)
>  				if (*p == '_')
>  					p++;
> @@ -98,7 +97,7 @@ static int parse_msg_id(const char *text)
>  	}
>  
>  	for (i = 0; i < FSCK_MSG_MAX; i++)
> -		if (!strcmp(text, lowercased[i]))
> +		if (!strcmp(text, msg_id_info[i].lowercased))
>  			return i;
>  
>  	return -1;

Heh, this was the first thing that came to my mind when I saw 03/19
that lazily prepares downcased version (which is good) but do so in
a separately allocated buffer (which is improved by this change) ;-)

IOW, I think all of the above should have been part of 03/19, not
"oops I belatedly realized that this way is better" fixup here.

The end result looks good, so let's keep reading.

> +void fsck_set_msg_types(struct fsck_options *options, const char *values)
> +{
> +	char *buf = xstrdup(values), *to_free = buf;
> +	int done = 0;
> +
> +	while (!done) {
> +		int len = strcspn(buf, " ,|"), equal;
> +
> +		done = !buf[len];
> +		if (!len) {
> +			buf++;
> +			continue;
> +		}
> +		buf[len] = '\0';
> +
> +		for (equal = 0; equal < len &&
> +				buf[equal] != '=' && buf[equal] != ':'; equal++)

Style.  I'd format this more like so:

		for (equal = 0;
                     equal < len && buf[equal] != '=' && buf[equal] != ':';
		     equal++)

> +			buf[equal] = tolower(buf[equal]);
> +		buf[equal] = '\0';
> +
> +		if (equal == len)
> +			die("Missing '=': '%s'", buf);
> +
> +		fsck_set_msg_type(options, buf, buf + equal + 1);
> +		buf += len + 1;
> +	}
> +	free(to_free);
> +}

Overall, the change is good (and it was good in v6, too), and I
think it has become simpler to follow the logic with the upfront
downcasing.

Thanks.