Re: [PATCH 00/12] Hard coded string length cleanup
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:59:29
René Scharfe [off-list ref] writes:
Am 20.12.2013 00:50, schrieb Duy Nguyen:quoted
On Fri, Dec 20, 2013 at 6:32 AM, René Scharfe [off-list ref] wrote:quoted
Seeing that skip_prefix_defval is mostly used in the form skip_prefix_defval(foo, prefix, foo) I wonder if it makes sense to first change skip_prefix to return the full string instead of NULL if the prefix is not matched. Would the resulting function cover most use cases? And would it still be easily usable?That was skip_prefix_gently() that I forgot to replace in a commit message, before I turned it into _defval variant. The reason for _defval is it could be use to chain expression together without adding temporary variables, e.g. - if (starts_with(line->buf, ">From") && isspace(line->buf[5])) { + if (isspace(*skip_prefix_defval(line->buf, ">From", "NOSPACE"))) { Without _defval, one would need to do if ((p = skip_prefix(..)) && isspace(*p)). I'm not entirely sure this is a good thing though as it could make it a bit harder to read.That usage is quite rare compared to occurrences of skip_prefix_defval(foo, prefix, foo), no? Adding a temporary variable for them wouldn't be that bad if we can simplify the API to a single function -- if that one is usable, that is. On the other hand, we could add a special function for that example and we'd already have three users in the tree (patch below). I think that's too narrow a use case for a library function, though. Doing the following instead in the three cases doesn't seem to be too bad: rest = skip_prefix(line->buf, ">From"); if (rest != line->buf && isspace(*rest)) {
Yeah, I personally feel that the "NOSPACE" hack is a bit too ugly to live in a code meant to be maintained for a longer term. The above with a "rest" variable, whose assignment is outside if () condition, is so far the easiest to read, at least to me. I am not convinced if skip-prefix-and-space is even a good abstraction of anything; it feels a bit too specialized. Thanks.
quoted hunk
--- builtin/apply.c | 2 +- builtin/mailinfo.c | 4 ++-- git-compat-util.h | 1 + strbuf.c | 9 +++++++++ 4 files changed, 13 insertions(+), 3 deletions(-)diff --git a/builtin/apply.c b/builtin/apply.c index b0d0986..b96befd 100644 --- a/builtin/apply.c +++ b/builtin/apply.c@@ -433,7 +433,7 @@ static unsigned long linelen(const char *buffer, unsigned long size) static int is_dev_null(const char *str) { - return !memcmp("/dev/null", str, 9) && isspace(str[9]); + return skip_prefix_and_space(str, "/dev/null") != str; } #define TERM_SPACE 1diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c index 2c3cd8e..2575989 100644 --- a/builtin/mailinfo.c +++ b/builtin/mailinfo.c@@ -328,11 +328,11 @@ static int check_header(const struct strbuf *line, } /* for inbody stuff */ - if (starts_with(line->buf, ">From") && isspace(line->buf[5])) { + if (skip_prefix_and_space(line->buf, ">From") != line->buf) { ret = 1; /* Should this return 0? */ goto check_header_out; } - if (starts_with(line->buf, "[PATCH]") && isspace(line->buf[7])) { + if (skip_prefix_and_space(line->buf, "[PATCH]") != line->buf) { for (i = 0; header[i]; i++) { if (!memcmp("Subject", header[i], 7)) { handle_header(&hdr_data[i], line);diff --git a/git-compat-util.h b/git-compat-util.h index dcb92c4..a083918 100644 --- a/git-compat-util.h +++ b/git-compat-util.h@@ -355,6 +355,7 @@ extern int prefixcmp(const char *str, const char *prefix); extern int ends_with(const char *str, const char *suffix); extern int suffixcmp(const char *str, const char *suffix); extern const char *skip_prefix(const char *str, const char *prefix); +extern const char *skip_prefix_and_space(const char *str, const char *prefix); #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)diff --git a/strbuf.c b/strbuf.c index 222df13..768331f 100644 --- a/strbuf.c +++ b/strbuf.c@@ -47,6 +47,15 @@ const char *skip_prefix(const char *str, const char *prefix) return str; } +const char *skip_prefix_and_space(const char *str, const char *prefix) +{ + const char *p = skip_prefix(str, prefix); + if (((p != str) || !*prefix) && isspace(*p)) + return p + 1; + else + return str; +} + /* * Used as the default ->buf value, so that people can always assume * buf is non NULL and ->buf is NUL terminated even for a freshly