Re: [PATCH 04/16] refactor skip_prefix to return a boolean

From: Jeff King <peff@peff.net>
Date: Jun 20, 2014, 02:08 UTC
Message-ID: <20140620020824.GA31908@sigill.intra.peff.net>
In-Reply-To: <CAPig+cTKgrYx1hcO-hV3ZT2sm1rR17bWJC0ktZzFQB=TMr6Xuw@mail.gmail.com>

On Thu, Jun 19, 2014 at 09:59:39PM -0400, Eric Sunshine wrote:

Show 17 quoted lines

> > diff --git a/git-compat-util.h b/git-compat-util.h
> > index b6f03b3..556c839 100644
> > --- a/git-compat-util.h
> > +++ b/git-compat-util.h
> > @@ -349,13 +349,31 @@ extern void set_die_is_recursing_routine(int (*routine)(void));
> >  extern int starts_with(const char *str, const char *prefix);
> >  extern int ends_with(const char *str, const char *suffix);
> >
> > -static inline const char *skip_prefix(const char *str, const char *prefix)
> > +/*
> > + * If "str" begins with "prefix", return 1. If out is non-NULL,
> > + * it it set to str + strlen(prefix) (i.e., the prefix is skipped).
> 
> The documentation claims that 'out' can be NULL, however, the code
> does not respect this. NULL 'out' seems rather pointless (unless you
> want an alias for starts_with()), so presumably the documentation is
> incorrect.

Thanks for catching. My original version (before I sent to the list) did allow for a conditional NULL out, but I realized there was exactly one caller who wanted this, and that they would be better off using starts_with. I added patch 3 ("avoid using skip_prefix as a boolean") and stripped the conditional from skip_prefix, but forgot to update the comment.

I think we'd just want to squash the patch below (I also took the opportunity to fix a typo and clarify the text a bit):

diff --git a/git-compat-util.h b/git-compat-util.h
index 556c839..1187e1a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -350,8 +350,9 @@ extern int starts_with(const char *str, const char *prefix);
 extern int ends_with(const char *str, const char *suffix);
 
 /*
- * If "str" begins with "prefix", return 1. If out is non-NULL,
- * it it set to str + strlen(prefix) (i.e., the prefix is skipped).
+ * If the string "str" begins with the string found in "prefix", return 1.
+ * The "out" parameter is set to "str + strlen(prefix)" (i.e., to the point in
+ * the string right after the prefix).
  *
  * Otherwise, returns 0 and out is left untouched.
  *

-Peff