Am 20.12.2013 00:50, schrieb Duy Nguyen:
> On Fri, Dec 20, 2013 at 6:32 AM, René Scharfe <l....@web.de> wrote:
>> 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)) {

---
 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     1
diff --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
-- 
1.8.5.2
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to