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

Johannes Schindelin [off-list ref] writes:

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;

Hi Junio,

On 2015-06-19 21:26, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

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

It is actually not `prefixcmp()` because it requires that `match` has precisely `len` bytes, while `prefixcmp()` or `starts_with()` would not care as long as the first `strlen(match)` bytes of `string` match `match`.

Maybe `fixed_length_strcmp()` would be an appropriate name, but it is pretty long and *still* does not convey exactly what this function is about.

Also please note that this is a `static inline` function, i.e. its definition does not bleed outside of this file.

So I hope that you are satisfied with simply adding

    /** compares a counted string to a NUL-terminated one. */

as comment above the `substrcmp()` function?

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().

Hmm. I really do not like that kind of thinking, i.e. having to duplicate, then modify data to be able to call the API, only to have to modify the data back afterwards, and eventually having to unallocate the data in all code paths. That feels just very inelegant to me.

I agree that it cannot be helped, sometimes, when you *have* to do exactly such mucking with data on a copy, be it to avoid additional complexity or poor performance. But I simply do not see the need for that here.

Ciao,
Dscho

Hi Junio,

On 2015-06-19 21:26, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

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

Turns out that the diffstat says it saves 10 lines. So I changed it according to your suggestion. Part of v7.

Ciao,
Dscho