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

From

Junio C Hamano <gitster@pobox.com>

Date

Jun 23, 2014, 18:50 UTC

Message-ID

<xmqq1tufcvfl.fsf@gitster.dls.corp.google.com>

In-Reply-To

<20140618194417.GD22622@sigill.intra.peff.net>

Jeff King <peff@peff.net> 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.