[PATCH 0/2] further fixes of check_ref_format() users

[PATCH 0/2] further fixes of check_ref_format() users

[Junio C Hamano]

There were a few overtight "check_ref_format()" that was exposed
with a natural "mirror" refspec "refs/*:refs/*".

Junio C Hamano (2):
  fetch/push: allow refs/*:refs/*
  get_fetch_map(): tighten checks on dest refs

 builtin/fetch-pack.c   |  2 +-
 builtin/receive-pack.c |  2 +-
 connect.c              |  8 ++------
 remote.c               |  6 +++---
 t/t5516-fetch-push.sh  | 33 +++++++++++++++++++++++++++++++++
 5 files changed, 40 insertions(+), 11 deletions(-)

-- 
1.7.12.92.g7963261

[PATCH 1/2] fetch/push: allow refs/*:refs/*

[Junio C Hamano]

There are a handful of places where we call check_refname_format() on a
substring after "refs/" of a refname we are going to use, and filter out a
valid match with "refs/stash" with such a pathspec.  Not sending a stash
may arguably be a feature (as stash is inherently a local workflow
element), but the code in the transport layer is oblivious to this
filtering performed by the lower layer of the code, and complains that the
other side did not send all the objects that needs to complete refs/stash
at the end, even though the code will not write refs/stash out anyway, and
making the whole command fail.

This is an attempt to "fix" it by using check_refname_format() on the
whole "refs/....." string and allowing refs/stash to be also copied.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/fetch-pack.c   |  2 +-
 builtin/receive-pack.c |  2 +-
 connect.c              |  8 ++------
 remote.c               |  2 +-
 t/t5516-fetch-push.sh  | 33 +++++++++++++++++++++++++++++++++
 5 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 6207ecd..a3e3fa3 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -546,7 +546,7 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
 	for (ref = *refs; ref; ref = next) {
 		next = ref->next;
 		if (!memcmp(ref->name, "refs/", 5) &&
-		    check_refname_format(ref->name + 5, 0))
+		    check_refname_format(ref->name, 0))
 			; /* trash */
 		else if (args.fetch_all &&
 			 (!args.depth || prefixcmp(ref->name, "refs/tags/") )) {
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 7ec68a1..1935b80 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -401,7 +401,7 @@ static const char *update(struct command *cmd)
 	struct ref_lock *lock;
 
 	/* only refs/... are allowed */
-	if (prefixcmp(name, "refs/") || check_refname_format(name + 5, 0)) {
+	if (prefixcmp(name, "refs/") || check_refname_format(name, 0)) {
 		rp_error("refusing to create funny ref '%s' remotely", name);
 		return "funny refname";
 	}
diff --git a/connect.c b/connect.c
index c8d0ea5..b30639c 100644
--- a/connect.c
+++ b/connect.c
@@ -17,20 +17,16 @@ static int check_ref(const char *name, int len, unsigned int flags)
 	if (len < 5 || memcmp(name, "refs/", 5))
 		return 0;
 
-	/* Skip the "refs/" part */
-	name += 5;
-	len -= 5;
-
 	/* REF_NORMAL means that we don't want the magic fake tag refs */
 	if ((flags & REF_NORMAL) && check_refname_format(name, 0))
 		return 0;
 
 	/* REF_HEADS means that we want regular branch heads */
-	if ((flags & REF_HEADS) && !memcmp(name, "heads/", 6))
+	if ((flags & REF_HEADS) && !memcmp(name, "refs/heads/", 11))
 		return 1;
 
 	/* REF_TAGS means that we want tags */
-	if ((flags & REF_TAGS) && !memcmp(name, "tags/", 5))
+	if ((flags & REF_TAGS) && !memcmp(name, "refs/tags/", 10))
 		return 1;
 
 	/* All type bits clear means that we are ok with anything */
diff --git a/remote.c b/remote.c
index e2ef991..eacd8ad 100644
--- a/remote.c
+++ b/remote.c
@@ -1595,7 +1595,7 @@ static int one_local_ref(const char *refname, const unsigned char *sha1, int fla
 	int len;
 
 	/* we already know it starts with refs/ to get here */
-	if (check_refname_format(refname + 5, 0))
+	if (check_refname_format(refname, 0))
 		return 0;
 
 	len = strlen(refname) + 1;
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b69cf57..3fdfaea 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -929,6 +929,39 @@ test_expect_success 'push into aliased refs (inconsistent)' '
 	)
 '
 
+test_expect_success 'push all hierarchies with stash' '
+	mk_empty &&
+	git stash clear &&
+	git reset --hard &&
+	echo >>path1 &&
+	git stash save "Tweak path1" &&
+	git push testrepo "refs/*:refs/*" &&
+	# without fix to connect.c::check_ref(), the second push
+	# would fail.
+	git push testrepo refs/stash &&
+	git ls-remote . >expect &&
+	git ls-remote testrepo >actual &&
+	test_cmp actual expect
+'
+
+test_expect_success 'fetch all hierarchies with stash' '
+	mk_empty &&
+	git stash clear &&
+	git reset --hard &&
+	echo >>path1 &&
+	git stash save "Tweak path1" &&
+	(
+		cd testrepo &&
+		git commit --allow-empty -m initial &&
+		git checkout HEAD^0 &&
+		git fetch .. "+refs/*:refs/*" &&
+		git checkout master
+	) &&
+	git ls-remote . >expect &&
+	git ls-remote testrepo >actual &&
+	test_cmp actual expect
+'
+
 test_expect_success 'push --porcelain' '
 	mk_empty &&
 	echo >.git/foo  "To testrepo" &&
-- 
1.7.12.92.g7963261

Re: [PATCH 1/2] fetch/push: allow refs/*:refs/*

[Michael Haggerty]

On 08/20/2012 07:39 PM, Junio C Hamano wrote:
> There are a handful of places where we call check_refname_format() on a
> substring after "refs/" of a refname we are going to use, and filter out a
> valid match with "refs/stash" with such a pathspec.  Not sending a stash
> may arguably be a feature (as stash is inherently a local workflow
> element), but the code in the transport layer is oblivious to this
> filtering performed by the lower layer of the code, and complains that the
> other side did not send all the objects that needs to complete refs/stash
> at the end, even though the code will not write refs/stash out anyway, and
> making the whole command fail.
> 
> This is an attempt to "fix" it by using check_refname_format() on the
> whole "refs/....." string and allowing refs/stash to be also copied.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/fetch-pack.c   |  2 +-
>  builtin/receive-pack.c |  2 +-
>  connect.c              |  8 ++------
>  remote.c               |  2 +-
>  t/t5516-fetch-push.sh  | 33 +++++++++++++++++++++++++++++++++
>  5 files changed, 38 insertions(+), 9 deletions(-)
> 
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 6207ecd..a3e3fa3 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -546,7 +546,7 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
>  	for (ref = *refs; ref; ref = next) {
>  		next = ref->next;
>  		if (!memcmp(ref->name, "refs/", 5) &&
> -		    check_refname_format(ref->name + 5, 0))
> +		    check_refname_format(ref->name, 0))
>  			; /* trash */
>  		else if (args.fetch_all &&
>  			 (!args.depth || prefixcmp(ref->name, "refs/tags/") )) {

I understand that you didn't introduce this code, but it seems like a
suspicious combination of conditions:

    if ((ref->name starts with "refs/")
        and (ref->name has invalid format))

It would seem more plausible to want

    if ((ref->name starts with "refs/")
        and (ref->name has *valid* format))

or its logical inverse

    if ((ref->name does not start with "refs/")
        or (ref->name has invalid format))

If you know the purpose of the code here, maybe you could give it a
quick sanity check.  (I believe the test was originally introduced by
Dscho in 1baaae5e1f9ac09aafe11b5aa4daa69bae68173b.)

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 7ec68a1..1935b80 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -401,7 +401,7 @@ static const char *update(struct command *cmd)
>  	struct ref_lock *lock;
>  
>  	/* only refs/... are allowed */
> -	if (prefixcmp(name, "refs/") || check_refname_format(name + 5, 0)) {
> +	if (prefixcmp(name, "refs/") || check_refname_format(name, 0)) {
>  		rp_error("refusing to create funny ref '%s' remotely", name);
>  		return "funny refname";
>  	}
> diff --git a/connect.c b/connect.c
> index c8d0ea5..b30639c 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -17,20 +17,16 @@ static int check_ref(const char *name, int len, unsigned int flags)
>  	if (len < 5 || memcmp(name, "refs/", 5))
>  		return 0;
>  
> -	/* Skip the "refs/" part */
> -	name += 5;
> -	len -= 5;
> -
>  	/* REF_NORMAL means that we don't want the magic fake tag refs */
>  	if ((flags & REF_NORMAL) && check_refname_format(name, 0))
>  		return 0;
>  
>  	/* REF_HEADS means that we want regular branch heads */
> -	if ((flags & REF_HEADS) && !memcmp(name, "heads/", 6))
> +	if ((flags & REF_HEADS) && !memcmp(name, "refs/heads/", 11))
>  		return 1;
>  
>  	/* REF_TAGS means that we want tags */
> -	if ((flags & REF_TAGS) && !memcmp(name, "tags/", 5))
> +	if ((flags & REF_TAGS) && !memcmp(name, "refs/tags/", 10))
>  		return 1;
>  
>  	/* All type bits clear means that we are ok with anything */
> diff --git a/remote.c b/remote.c
> index e2ef991..eacd8ad 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1595,7 +1595,7 @@ static int one_local_ref(const char *refname, const unsigned char *sha1, int fla
>  	int len;
>  
>  	/* we already know it starts with refs/ to get here */
> -	if (check_refname_format(refname + 5, 0))
> +	if (check_refname_format(refname, 0))
>  		return 0;
>  
>  	len = strlen(refname) + 1;
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index b69cf57..3fdfaea 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -929,6 +929,39 @@ test_expect_success 'push into aliased refs (inconsistent)' '
>  	)
>  '
>  
> +test_expect_success 'push all hierarchies with stash' '
> +	mk_empty &&
> +	git stash clear &&
> +	git reset --hard &&
> +	echo >>path1 &&
> +	git stash save "Tweak path1" &&
> +	git push testrepo "refs/*:refs/*" &&
> +	# without fix to connect.c::check_ref(), the second push
> +	# would fail.
> +	git push testrepo refs/stash &&
> +	git ls-remote . >expect &&
> +	git ls-remote testrepo >actual &&
> +	test_cmp actual expect
> +'
> +
> +test_expect_success 'fetch all hierarchies with stash' '
> +	mk_empty &&
> +	git stash clear &&
> +	git reset --hard &&
> +	echo >>path1 &&
> +	git stash save "Tweak path1" &&
> +	(
> +		cd testrepo &&
> +		git commit --allow-empty -m initial &&
> +		git checkout HEAD^0 &&
> +		git fetch .. "+refs/*:refs/*" &&
> +		git checkout master
> +	) &&
> +	git ls-remote . >expect &&
> +	git ls-remote testrepo >actual &&
> +	test_cmp actual expect
> +'
> +
>  test_expect_success 'push --porcelain' '
>  	mk_empty &&
>  	echo >.git/foo  "To testrepo" &&
> 

Otherwise I see nothing wrong with your changes.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 1/2] fetch/push: allow refs/*:refs/*

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

>> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
>> index 6207ecd..a3e3fa3 100644
>> --- a/builtin/fetch-pack.c
>> +++ b/builtin/fetch-pack.c
>> @@ -546,7 +546,7 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
>>  	for (ref = *refs; ref; ref = next) {
>>  		next = ref->next;
>>  		if (!memcmp(ref->name, "refs/", 5) &&
>> -		    check_refname_format(ref->name + 5, 0))
>> +		    check_refname_format(ref->name, 0))
>>  			; /* trash */
>>  		else if (args.fetch_all &&
>>  			 (!args.depth || prefixcmp(ref->name, "refs/tags/") )) {
>
> I understand that you didn't introduce this code, but it seems like a
> suspicious combination of conditions:
>
>     if ((ref->name starts with "refs/")
>         and (ref->name has invalid format))

This protects us from getting contaminated by bogus ref under refs/
when running "fetch refs/heads/*:refs/remotes/origin/*" no?

The remote side can also throw phony "I have this object, too, but
not at a particular ref---this entry is only to let you know I have
it, so that we can negotiate minimal transfer better" entries that
are labelled with strings that do not begin with "refs/" and do not
pass check_refname_format() (and because they are not refs, they do
not have to pass the test) at us, and we do not want to filter them
out in this function.  But we do not want anything that is malformed
under "refs/".

Re: [PATCH 1/2] fetch/push: allow refs/*:refs/*

[Michael Haggerty]

On 08/21/2012 07:37 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>>> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
>>> index 6207ecd..a3e3fa3 100644
>>> --- a/builtin/fetch-pack.c
>>> +++ b/builtin/fetch-pack.c
>>> @@ -546,7 +546,7 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)
>>>  	for (ref = *refs; ref; ref = next) {
>>>  		next = ref->next;
>>>  		if (!memcmp(ref->name, "refs/", 5) &&
>>> -		    check_refname_format(ref->name + 5, 0))
>>> +		    check_refname_format(ref->name, 0))
>>>  			; /* trash */
>>>  		else if (args.fetch_all &&
>>>  			 (!args.depth || prefixcmp(ref->name, "refs/tags/") )) {
>>
>> I understand that you didn't introduce this code, but it seems like a
>> suspicious combination of conditions:
>>
>>     if ((ref->name starts with "refs/")
>>         and (ref->name has invalid format))
> 
> This protects us from getting contaminated by bogus ref under refs/
> when running "fetch refs/heads/*:refs/remotes/origin/*" no?
> 
> The remote side can also throw phony "I have this object, too, but
> not at a particular ref---this entry is only to let you know I have
> it, so that we can negotiate minimal transfer better" entries that
> are labelled with strings that do not begin with "refs/" and do not
> pass check_refname_format() (and because they are not refs, they do
> not have to pass the test) at us, and we do not want to filter them
> out in this function.  But we do not want anything that is malformed
> under "refs/".

Thanks for the explanation.  I'm trying to dig some more into this so
that I can add some documentation, because this area of the code is
rather obscure.

Here is the loop being discussed, in full (from builtin/fetch-pack.c,
filter_refs()):

> 	for (ref = *refs; ref; ref = next) {
> 		next = ref->next;
> 		if (!memcmp(ref->name, "refs/", 5) &&
> 		    check_refname_format(ref->name, 0))
> 			; /* trash */
> 		else if (args.fetch_all &&
> 			 (!args.depth || prefixcmp(ref->name, "refs/tags/") )) {
> 			*newtail = ref;
> 			ref->next = NULL;
> 			newtail = &ref->next;
> 			continue;
> 		}
> 		else {
> 			int i;
> 			for (i = 0; i < nr_match; i++) {
> 				if (!strcmp(ref->name, match[i])) {
> 					match[i][0] = '\0';
> 					return_refs[i] = ref;
> 					break;
> 				}
> 			}
> 			if (i < nr_match)
> 				continue; /* we will link it later */
> 		}
> 		free(ref);
> 	}

Empirically (determined by instrumenting the code and running the git
test suite):

* The first branch of the if statement is only executed for ref->name of
the form "refs/tags/foo^{}" for various "foo".

* The second branch of the if is *never* executed.

* The third branch is invoked for various reference names under "refs/"
(including oddballs like "refs/for/refs/heads/master", "refs/stash",
"refs/replace/<SHA1>"), and also for "HEAD".

This doesn't quite agree with your explanation, because the phony refs
(at least in this dataset) *do* start with "refs/" and they *are* trashed.

I'll continue to try to figure out this area.  I already found an
apparent memory leak...

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 1/2] fetch/push: allow refs/*:refs/*

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

>> The remote side can also throw phony "I have this object, too, but
>> not at a particular ref---this entry is only to let you know I have
>> it, so that we can negotiate minimal transfer better" entries that
>> are labelled with strings that do not begin with "refs/" and do not
>> pass check_refname_format() (and because they are not refs, they do
>> not have to pass the test) at us, and we do not want to filter them
>> out in this function.  But we do not want anything that is malformed
>> under "refs/".
>
> Thanks for the explanation.  I'm trying to dig some more into this so
> that I can add some documentation, because this area of the code is
> rather obscure.
>
> Here is the loop being discussed, in full (from builtin/fetch-pack.c,
> filter_refs()):
> ...
> Empirically (determined by instrumenting the code and running the git
> test suite):
>
> * The first branch of the if statement is only executed for ref->name of
> the form "refs/tags/foo^{}" for various "foo".

We do not want "fetch --mirror" and "refs/*:refs/*" to add a tag
whose name is "foo^{}" to us.

> * The second branch of the if is *never* executed.

I am not familiar with (nor particularly interested in) the "args.depth"
code, so I have no comment on this part offhand.

> * The third branch is invoked for various reference names under "refs/"
> (including oddballs like "refs/for/refs/heads/master", "refs/stash",
> "refs/replace/<SHA1>"), and also for "HEAD".
>
> This doesn't quite agree with your explanation, because the phony refs
> (at least in this dataset) *do* start with "refs/" and they *are* trashed.

Try fetching from a repository that has an alternate, and you would
see those ".have" phoney refs.

But yes, they are trashed as well, as they are not likely to match,
so you are right; the ".have" entries are red-herring (they have
already been used before the caller calls this function for their
sole purpose of marking the other side has).

Re: [PATCH 1/2] fetch/push: allow refs/*:refs/*

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> ...
>> * The second branch of the if is *never* executed.
> 		else if (args.fetch_all &&
> 			 (!args.depth || prefixcmp(ref->name, "refs/tags/") )) {
> 			*newtail = ref;
> 			ref->next = NULL;
> 			newtail = &ref->next;
> 			continue;
> 		}
>
> I am not familiar with (nor particularly interested in) the "args.depth"
> code, so I have no comment on this part offhand.

This is "fetch --all" case.  I think the original intent was to
fetch all branches (hence exclusion of tags) so the prefixcmp() may
not be correct (!prefixcmp() with "refs/heads" would be more kosher)
these days.  I am not sure what !args.depth check wants to do here
(that is, I can see it wants to defeat "fetch --all" and instead
make it go through the usual refspec matching codepath, but I do not
know why we thought that is a good thing to do here).

[PATCH 2/2] get_fetch_map(): tighten checks on dest refs

[Junio C Hamano]

The code to check the refname we store the fetched result locally did not
bother checking the first 5 bytes of it, presumably assuming that it
always begin with "refs/".  For a fetch refspec (or the result of applying
wildcard on one), we always want the RHS to map to something inside
"refs/" hierarchy, so let's spell that rule out in a more explicit way.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 remote.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index eacd8ad..0f2b1db 100644
--- a/remote.c
+++ b/remote.c
@@ -1402,8 +1402,8 @@ int get_fetch_map(const struct ref *remote_refs,
 
 	for (rmp = &ref_map; *rmp; ) {
 		if ((*rmp)->peer_ref) {
-			if (check_refname_format((*rmp)->peer_ref->name + 5,
-				REFNAME_ALLOW_ONELEVEL)) {
+			if (prefixcmp((*rmp)->peer_ref->name, "refs/") ||
+			    check_refname_format((*rmp)->peer_ref->name, 0)) {
 				struct ref *ignore = *rmp;
 				error("* Ignoring funny ref '%s' locally",
 				      (*rmp)->peer_ref->name);
-- 
1.7.12.92.g7963261