Re: [PATCH 04/16] refactor skip_prefix to return a boolean
On Mon, Jun 23, 2014 at 11:50:06AM -0700, Junio C Hamano wrote:
> I was re-reading this and noticed another possible bug.
>
> builtin/clone.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index b12989d..df659dd 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -696,7 +696,7 @@ static void write_refspec_config(const char*
> src_ref_prefix,
> if (option_mirror || !option_bare) {
> if (option_single_branch && !option_mirror) {
> if (option_branch) {
> - if (strstr(our_head_points_at->name,
> "refs/tags/"))
> + if (starts_with(our_head_points_at->name,
> "refs/tags/"))
> strbuf_addf(&value, "+%s:%s",
> our_head_points_at->name,
> our_head_points_at->name);
> else
>
> Because the pattern is not anchored to the left with a slash, it is
> clear that the original cannot even claim that it was trying to
> munge "foo/refs/tags/" as well.
Yeah, the strstr seems very wrong there. Even with the "/", why would
you want to match "refs/heads/refs/tags/"?
> Which means this is trivially correct, but at the same time I wonder
> what it means for our-head to point at a ref in refs/tags/ hierarchy.
I think it is for "git clone --branch=v1.0". We create a refspec pulling
v1.0 to its local tag in that case (as opposed to to something in
"refs/remotes/origin/"). So I really think this does want to be
starts_with.
-Peff
--
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
Re: [PATCH 04/16] refactor skip_prefix to return a boolean
Jeff King writes:
> diff --git a/builtin/clone.c b/builtin/clone.c
> index b12989d..a5b2d9d 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -703,9 +703,12 @@ static void write_refspec_config(const char*
> src_ref_prefix,
> strbuf_addf(&value, "+%s:%s%s",
> our_head_points_at->name,
> branch_top->buf, option_branch);
> } else if (remote_head_points_at) {
> + const char *head = remote_head_points_at->name;
> + if (!skip_prefix(head, "refs/heads/", &head))
> + die("BUG: remote HEAD points at
> non-head?");
> +
> strbuf_addf(&value, "+%s:%s%s",
> remote_head_points_at->name,
> - branch_top->buf,
> -
> skip_prefix(remote_head_points_at->name, "refs/heads/"));
> + branch_top->buf, head);
> }
> /*
>* otherwise, the next "git fetch" will
I was re-reading this and noticed another possible bug.
builtin/clone.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index b12989d..df659dd 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -696,7 +696,7 @@ static void write_refspec_config(const char* src_ref_prefix,
if (option_mirror || !option_bare) {
if (option_single_branch && !option_mirror) {
if (option_branch) {
- if (strstr(our_head_points_at->name,
"refs/tags/"))
+ if (starts_with(our_head_points_at->name,
"refs/tags/"))
strbuf_addf(&value, "+%s:%s",
our_head_points_at->name,
our_head_points_at->name);
else
Because the pattern is not anchored to the left with a slash, it is
clear that the original cannot even claim that it was trying to
munge "foo/refs/tags/" as well.
Which means this is trivially correct, but at the same time I wonder
what it means for our-head to point at a ref in refs/tags/ hierarchy.
--
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
Re: [PATCH 04/16] refactor skip_prefix to return a boolean
On Thu, Jun 19, 2014 at 10:30:36PM -0400, Eric Sunshine wrote: > > 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. > > For consistency with the preceding paragraph: > > Otherwise, return 0 and leave "out" untouched. Heh, I even noticed the verb tense mismatch but thought "nah, nobody will care". I see I was wrong. :) Here is the full squashable patch against v1, in case Junio picks it up from here (or it will be in my re-roll if necessary). diff --git a/git-compat-util.h b/git-compat-util.h index 556c839..d29e1df 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -350,10 +350,11 @@ 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. + * Otherwise, return 0 and leave "out" untouched. * * Examples: * -- 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
Re: [PATCH 04/16] refactor skip_prefix to return a boolean
On Thu, Jun 19, 2014 at 10:08 PM, Jeff King wrote:
> On Thu, Jun 19, 2014 at 09:59:39PM -0400, Eric Sunshine wrote:
>
>> > 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.
For consistency with the preceding paragraph:
Otherwise, return 0 and leave "out" untouched.
> *
>
> -Peff
--
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
Re: [PATCH 04/16] refactor skip_prefix to return a boolean
On Thu, Jun 19, 2014 at 09:59:39PM -0400, Eric Sunshine wrote:
> > 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
--
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
Re: [PATCH 04/16] refactor skip_prefix to return a boolean
On Wed, Jun 18, 2014 at 3:44 PM, Jeff King wrote:
> The skip_prefix function returns a pointer to the content
> past the prefix, or NULL if the prefix was not found. While
> this is nice and simple, in practice it makes it hard to use
> for two reasons:
>
> 1. When you want to conditionally skip or keep the string
> as-is, you have to introduce a second temporary
> variable. For example:
>
>tmp = skip_prefix(buf, "foo");
>if (tmp)
>buf = tmp;
>
> 2. It is verbose to check the outcome in a conditional, as
> you need extra parentheses to silence compiler
> warnings. For example:
>
>if ((cp = skip_prefix(buf, "foo"))
>/* do something with cp */
>
> Both of these make it harder to use for long if-chains, and
> we tend to use starts_with instead. However, the first line
> of "do something" is often to then skip forward in buf past
> the prefix, either using a magic constant or with an extra
> strlen (which is generally computed at compile time, but
> means we are repeating ourselves).
>
> This patch refactors skip_prefix to return a simple boolean,
> and to provide the pointer value as an out-parameter. If the
> prefix is not found, the out-parameter is untouched. This
> lets you write:
>
> if (skip_prefix(arg, "foo ", &arg))
> do_foo(arg);
> else if (skip_prefix(arg, "bar ", &arg))
> do_bar(arg);
>
> Signed-off-by: Jeff King
> ---
> 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.
> + *
> + * Otherwise, returns 0 and out is left untouched.
> + *
> + * Examples:
> + *
> + * [extract branch name, fail if not a branch]
> + * if (!skip_prefix(ref, "refs/heads/", &branch)
> + * return -1;
> + *
> + * [skip prefix if present, otherwise use whole string]
> + * skip_prefix(name, "refs/heads/", &name);
> + */
> +static inline int skip_prefix(const char *str, const char *prefix,
> + const char **out)
> {
> do {
> - if (!*prefix)
> - return str;
> + if (!*prefix) {
> + *out = str;
> + return 1;
> + }
> } while (*str++ == *prefix++);
> - return NULL;
> + return 0;
> }
>
> #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
--
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
[PATCH 04/16] refactor skip_prefix to return a boolean
The skip_prefix function returns a pointer to the content
past the prefix, or NULL if the prefix was not found. While
this is nice and simple, in practice it makes it hard to use
for two reasons:
1. When you want to conditionally skip or keep the string
as-is, you have to introduce a second temporary
variable. For example:
tmp = skip_prefix(buf, "foo");
if (tmp)
buf = tmp;
2. It is verbose to check the outcome in a conditional, as
you need extra parentheses to silence compiler
warnings. For example:
if ((cp = skip_prefix(buf, "foo"))
/* do something with cp */
Both of these make it harder to use for long if-chains, and
we tend to use starts_with instead. However, the first line
of "do something" is often to then skip forward in buf past
the prefix, either using a magic constant or with an extra
strlen (which is generally computed at compile time, but
means we are repeating ourselves).
This patch refactors skip_prefix to return a simple boolean,
and to provide the pointer value as an out-parameter. If the
prefix is not found, the out-parameter is untouched. This
lets you write:
if (skip_prefix(arg, "foo ", &arg))
do_foo(arg);
else if (skip_prefix(arg, "bar ", &arg))
do_bar(arg);
Signed-off-by: Jeff King
---
The diffstat is misleading. We actually save lines, except that I added
documentation for the function. :)
advice.c | 5 -
branch.c | 4 ++--
builtin/branch.c | 6 +++---
builtin/clone.c| 11 +++
builtin/commit.c | 5 ++---
builtin/fmt-merge-msg.c| 2 +-
builtin/push.c | 7 +++
builtin/remote.c | 4 +---
column.c | 5 +++--
commit.c | 6 ++
config.c | 3 +--
credential-cache--daemon.c | 6 ++
credential.c | 3 +--
fsck.c | 14 +-
git-compat-util.h | 26 ++
parse-options.c| 16 +---
transport.c| 4 +++-
urlmatch.c | 3 +--
18 files changed, 72 insertions(+), 58 deletions(-)
diff --git a/advice.c b/advice.c
index c50ebdf..9b42033 100644
--- a/advice.c
+++ b/advice.c
@@ -61,9 +61,12 @@ void advise(const char *advice, ...)
int git_default_advice_config(const char *var, const char *value)
{
- const char *k = skip_prefix(var, "advice.");
+ const char *k;
int i;
+ if (!skip_prefix(var, "advice.", &k))
+ return 0;
+
for (i = 0; i < ARRAY_SIZE(advice_config); i++) {
if (strcmp(k, advice_config[i].name))
continue;
diff --git a/branch.c b/branch.c
index 660097b..46e8aa8 100644
--- a/branch.c
+++ b/branch.c
@@ -50,11 +50,11 @@ static int should_setup_rebase(const char *origin)
void install_branch_config(int flag, const char *local, const char *origin,
const char *remote)
{
- const char *shortname = skip_prefix(remote, "refs/heads/");
+ const char *shortname = NULL;
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
- if (shortname
+ if (skip_prefix(remote, "refs/heads/", &shortname)
&& !strcmp(local, shortname)
&& !origin) {
warning(_("Not setting branch %s as its own upstream."),
diff --git a/builtin/branch.c b/builtin/branch.c
index 652b1d2..0591b22 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -294,13 +294,13 @@ static char *resolve_symref(const char *src, const char
*prefix)
{
unsigned char sha1[20];
int flag;
- const char *dst, *cp;
+ const char *dst;
dst = resolve_ref_unsafe(src, sha1, 0, &flag);
if (!(dst && (flag & REF_ISSYMREF)))
return NULL;
- if (prefix && (cp = skip_prefix(dst, prefix)))
- dst = cp;
+ if (prefix)
+ skip_prefix(dst, prefix, &dst);
return xstrdup(dst);
}
diff --git a/builtin/clone.c b/builtin/clone.c
index b12989d..a5b2d9d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -584,11 +584,11 @@ static void update_remote_refs(const struct ref *refs,
static void update_head(const struct ref *our, const struct ref *remote,
const char *msg)
{
- if (our && starts_with(our->name, "refs/heads/")) {
+ const char *head;
+ if (our && skip_prefix(our->name, "refs/heads/", &head)) {
/* Local default branch link */
create_symref("HEAD", our->name, NULL);
if (!option_bare) {
- const char *head = skip_prefix(our->name,
"refs/heads/");
update_ref(msg, "HEAD", our->old_sha1, NULL, 0,
UPDATE_REFS_DIE_ON_ERR);
install_branch_config(0, head, option_origin,
our->nam
