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

Johannes Schindelin [off-list ref] writes:

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.

Hi Junio,

On 2015-06-22 19:37, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

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.

quoted

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

quoted

+			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