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

[Johannes Schindelin <johannes.schindelin@gmx.de>]

Hi Junio,

On 2015-06-22 19:37, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> 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.

Gaaaah. Wrong commit fixed up. Sorry. Will be fixed in v8.

>> +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++)

Will be fixed.

>> +			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.

Yep, I agree. I did not expect that, but it was worth the effort to compare the two versions.

Ciao,
Dscho