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

From: Junio C Hamano <[email-masked]>
Date: Fri, 19 Jun 2015 12:26:11 -0700

Johannes Schindelin <[email-masked]> writes:

> diff --git a/fsck.c b/fsck.c
> index da5717c..8c3caff 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -103,13 +103,85 @@ static int fsck_msg_type(enum fsck_msg_id msg_id,
>  {
>  	int msg_type;
>  
> -	msg_type = msg_id_info[msg_id].msg_type;
> -	if (options->strict && msg_type == FSCK_WARN)
> -		msg_type = FSCK_ERROR;
> +	assert(msg_id >= 0 && msg_id < FSCK_MSG_MAX);
> +
> +	if (options->msg_type)
> +		msg_type = options->msg_type[msg_id];
> +	else {
> +		msg_type = msg_id_info[msg_id].msg_type;
> +		if (options->strict && msg_type == FSCK_WARN)
> +			msg_type = FSCK_ERROR;
> +	}
>  
>  	return msg_type;
>  }

Nice.

> +static inline int substrcmp(const char *string, int len, const char *match)
> +{
> +	int match_len = strlen(match);
> +	if (match_len != len)
> +		return -1;
> +	return memcmp(string, match, len);
> +}

What this does looks suspiciously like starts_with(), but its name
"substrcmp()" does not give any hint that this is about the beginnig
part of "string"; if anything, it gives a wrong hint that it may be
any substring.  prefixcmp() might be a better name but that was the
old name for !starts_with() so we cannot use it here.  It is a
mouthful, but starts_with_counted() may be.

But the whole thing may be moot.

If we take the "why not upcase the end-user string upfront"
suggestion from the previous review, fsck_set_msg_types() would have
an upcased copy of the end-user string that it can muck with; it can
turn "badfoo=error,poorbar=..." into "BADFOO=error,POORBAR=..."
that is stored in its own writable memory (possibly a strbuf), and
at that point it can afford to NUL-terminate BADFOO=error after
finding where one specification ends with strcspn() before calling
fsck_set_msg_type(), which in turn calls parse_msg_type().

So all parse_msg_type() needs to do is just !strcmp().

> +
> +static int parse_msg_type(const char *str, int len)
> +{
> +	if (len < 0)
> +		len = strlen(str);
> +
> +	if (!substrcmp(str, len, "error"))
> +		return FSCK_ERROR;
> +	else if (!substrcmp(str, len, "warn"))
> +		return FSCK_WARN;

> +	else
> +		die("Unknown fsck message type: '%.*s'",
> +				len, str);
> +}
> +
> +void fsck_set_msg_type(struct fsck_options *options,
> +		const char *msg_id, int msg_id_len,
> +		const char *msg_type, int msg_type_len)
> +{
> +	int id = parse_msg_id(msg_id, msg_id_len), type;
> +
> +	if (id < 0)
> +		die("Unhandled message id: %.*s", msg_id_len, msg_id);
> +	type = parse_msg_type(msg_type, msg_type_len);
> +
> +	if (!options->msg_type) {
> +		int i;
> +		int *msg_type = xmalloc(sizeof(int) * FSCK_MSG_MAX);
> +		for (i = 0; i < FSCK_MSG_MAX; i++)
> +			msg_type[i] = fsck_msg_type(i, options);
> +		options->msg_type = msg_type;
> +	}
> +
> +	options->msg_type[id] = type;
> +}
> +
> +void fsck_set_msg_types(struct fsck_options *options, const char *values)
> +{
> +	while (*values) {
> +		int len = strcspn(values, " ,|"), equal;
> +
> +		if (!len) {
> +			values++;
> +			continue;
> +		}
> +
> +		for (equal = 0; equal < len; equal++)
> +			if (values[equal] == '=' || values[equal] == ':')
> +				break;
> +
> +		if (equal == len)
> +			die("Missing '=': '%.*s'", len, values);
> +
> +		fsck_set_msg_type(options, values, equal,
> +				values + equal + 1, len - equal - 1);
> +		values += len;
> +	}
> +}
> +
>  __attribute__((format (printf, 4, 5)))
>  static int report(struct fsck_options *options, struct object *object,
>  	enum fsck_msg_id id, const char *fmt, ...)
> @@ -599,6 +671,10 @@ int fsck_object(struct object *obj, void *data, unsigned long size,
>  
>  int fsck_error_function(struct object *obj, int msg_type, const char *message)
>  {
> +	if (msg_type == FSCK_WARN) {
> +		warning("object %s: %s", sha1_to_hex(obj->sha1), message);
> +		return 0;
> +	}
>  	error("object %s: %s", sha1_to_hex(obj->sha1), message);
>  	return 1;
>  }
> diff --git a/fsck.h b/fsck.h
> index f6f268a..edb4540 100644
> --- a/fsck.h
> +++ b/fsck.h
> @@ -6,6 +6,11 @@
>  
>  struct fsck_options;
>  
> +void fsck_set_msg_type(struct fsck_options *options,
> +		const char *msg_id, int msg_id_len,
> +		const char *msg_type, int msg_type_len);
> +void fsck_set_msg_types(struct fsck_options *options, const char *values);
> +
>  /*
>   * callback function for fsck_walk
>   * type is the expected type of the object or OBJ_ANY
> @@ -25,10 +30,11 @@ struct fsck_options {
>  	fsck_walk_func walk;
>  	fsck_error error_func;
>  	unsigned strict:1;
> +	int *msg_type;
>  };
>  
> -#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0 }
> -#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1 }
> +#define FSCK_OPTIONS_DEFAULT { NULL, fsck_error_function, 0, NULL }
> +#define FSCK_OPTIONS_STRICT { NULL, fsck_error_function, 1, NULL }
>  
>  /* descend in all linked child objects
>   * the return value is: