Re: [PATCH v7 04/19] fsck: Offer a function to demote fsck errors to warnings
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 22, 2015, 17:37 UTC
- Message-ID
- <xmqq616fiqz7.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <08ab723892b4412431b88df8764f506f0abbb0a6.1434986506.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 49 quoted lines
> 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.
Show 17 quoted lines
> +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++)Show 11 quoted lines
> + 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.