threads / patch / 29297

patchFix incorrect ref namespace check

Subject: [PATCH] Fix incorrect ref namespace check

## tl;dr

5 messages between Jan 5, 2012 and Jan 11, 2012. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Nguyễn Thái Ngọc Duy· Jan 5, 2012, 12:35 UTC · lore

The reason why the trailing slash is needed is obvious. refs/stash is not a namespace, but a single ref. Do full string compare on it.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/fetch.c  |    2 +-
 builtin/remote.c |    2 +-
 log-tree.c       |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

Show changes to 3 files +3 −3

builtin/fetch.c, builtin/remote.c, log-tree.c

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 33ad3aa..daa68d2 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -573,7 +573,7 @@ static void find_non_local_tags(struct transport *transport,
 
 	for_each_ref(add_existing, &existing_refs);
 	for (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {
-		if (prefixcmp(ref->name, "refs/tags"))
+		if (prefixcmp(ref->name, "refs/tags/"))
 			continue;
 
 		/*
diff --git a/builtin/remote.c b/builtin/remote.c
index 583eec9..f54a89a 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -534,7 +534,7 @@ static int add_branch_for_removal(const char *refname,
 	}
 
 	/* don't delete non-remote-tracking refs */
-	if (prefixcmp(refname, "refs/remotes")) {
+	if (prefixcmp(refname, "refs/remotes/")) {
 		/* advise user how to delete local branches */
 		if (!prefixcmp(refname, "refs/heads/"))
 			string_list_append(branches->skipped,
diff --git a/log-tree.c b/log-tree.c
index 319bd31..9a88fcc 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -119,7 +119,7 @@ static int add_ref_decoration(const char *refname, const unsigned char *sha1, in
 		type = DECORATION_REF_REMOTE;
 	else if (!prefixcmp(refname, "refs/tags/"))
 		type = DECORATION_REF_TAG;
-	else if (!prefixcmp(refname, "refs/stash"))
+	else if (!strcmp(refname, "refs/stash"))
 		type = DECORATION_REF_STASH;
 	else if (!prefixcmp(refname, "HEAD"))
 		type = DECORATION_REF_HEAD;

-- 
1.7.8.36.g69ee2

Nguyễn Thái Ngọc Duy· Jan 5, 2012, 12:39 UTC · re: Nguyễn Thái Ngọc Duy · lore

The reason why the trailing slash is needed is obvious. refs/stash and HEAD are not namespace, but complete refs. Do full string compare on them.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I missed prefixcmp(..., "HEAD") right below prefixcmp(..., "refs/stash")

 builtin/fetch.c  |    2 +-
 builtin/remote.c |    2 +-
 log-tree.c       |    4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

Show changes to 3 files +4 −4

builtin/fetch.c, builtin/remote.c, log-tree.c

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 33ad3aa..daa68d2 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -573,7 +573,7 @@ static void find_non_local_tags(struct transport *transport,
 
 	for_each_ref(add_existing, &existing_refs);
 	for (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {
-		if (prefixcmp(ref->name, "refs/tags"))
+		if (prefixcmp(ref->name, "refs/tags/"))
 			continue;
 
 		/*
diff --git a/builtin/remote.c b/builtin/remote.c
index 583eec9..f54a89a 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -534,7 +534,7 @@ static int add_branch_for_removal(const char *refname,
 	}
 
 	/* don't delete non-remote-tracking refs */
-	if (prefixcmp(refname, "refs/remotes")) {
+	if (prefixcmp(refname, "refs/remotes/")) {
 		/* advise user how to delete local branches */
 		if (!prefixcmp(refname, "refs/heads/"))
 			string_list_append(branches->skipped,
diff --git a/log-tree.c b/log-tree.c
index 319bd31..535b905 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -119,9 +119,9 @@ static int add_ref_decoration(const char *refname, const unsigned char *sha1, in
 		type = DECORATION_REF_REMOTE;
 	else if (!prefixcmp(refname, "refs/tags/"))
 		type = DECORATION_REF_TAG;
-	else if (!prefixcmp(refname, "refs/stash"))
+	else if (!strcmp(refname, "refs/stash"))
 		type = DECORATION_REF_STASH;
-	else if (!prefixcmp(refname, "HEAD"))
+	else if (!strcmp(refname, "HEAD"))
 		type = DECORATION_REF_HEAD;
 
 	if (!cb_data || *(int *)cb_data == DECORATE_SHORT_REFS)

-- 
1.7.8.36.g69ee2

Junio C Hamano· Jan 5, 2012, 16:23 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH] Fix incorrect ref namespace check

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 6 quoted lines

> The reason why the trailing slash is needed is obvious. refs/stash and
> HEAD are not namespace, but complete refs. Do full string compare on them.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  I missed prefixcmp(..., "HEAD") right below prefixcmp(..., "refs/stash")

As Michael has been actively showing interest in cleaning up the area, he should have been CC'ed, I would think.

Show 48 quoted lines

>
>  builtin/fetch.c  |    2 +-
>  builtin/remote.c |    2 +-
>  log-tree.c       |    4 ++--
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 33ad3aa..daa68d2 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -573,7 +573,7 @@ static void find_non_local_tags(struct transport *transport,
>  
>  	for_each_ref(add_existing, &existing_refs);
>  	for (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {
> -		if (prefixcmp(ref->name, "refs/tags"))
> +		if (prefixcmp(ref->name, "refs/tags/"))
>  			continue;
>  
>  		/*
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 583eec9..f54a89a 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -534,7 +534,7 @@ static int add_branch_for_removal(const char *refname,
>  	}
>  
>  	/* don't delete non-remote-tracking refs */
> -	if (prefixcmp(refname, "refs/remotes")) {
> +	if (prefixcmp(refname, "refs/remotes/")) {
>  		/* advise user how to delete local branches */
>  		if (!prefixcmp(refname, "refs/heads/"))
>  			string_list_append(branches->skipped,
> diff --git a/log-tree.c b/log-tree.c
> index 319bd31..535b905 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -119,9 +119,9 @@ static int add_ref_decoration(const char *refname, const unsigned char *sha1, in
>  		type = DECORATION_REF_REMOTE;
>  	else if (!prefixcmp(refname, "refs/tags/"))
>  		type = DECORATION_REF_TAG;
> -	else if (!prefixcmp(refname, "refs/stash"))
> +	else if (!strcmp(refname, "refs/stash"))
>  		type = DECORATION_REF_STASH;
> -	else if (!prefixcmp(refname, "HEAD"))
> +	else if (!strcmp(refname, "HEAD"))
>  		type = DECORATION_REF_HEAD;
>  
>  	if (!cb_data || *(int *)cb_data == DECORATE_SHORT_REFS)

Nguyen Thai Ngoc Duy· Jan 11, 2012, 01:54 UTC · re: Junio C Hamano · lore

Re: [PATCH] Fix incorrect ref namespace check

Any comments Mike?

2012/1/5 Junio C Hamano <gitster@pobox.com>:

Show 60 quoted lines

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> The reason why the trailing slash is needed is obvious. refs/stash and
>> HEAD are not namespace, but complete refs. Do full string compare on them.
>>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  I missed prefixcmp(..., "HEAD") right below prefixcmp(..., "refs/stash")
>
> As Michael has been actively showing interest in cleaning up the area, he
> should have been CC'ed, I would think.
>
>>
>>  builtin/fetch.c  |    2 +-
>>  builtin/remote.c |    2 +-
>>  log-tree.c       |    4 ++--
>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 33ad3aa..daa68d2 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -573,7 +573,7 @@ static void find_non_local_tags(struct transport *transport,
>>
>>       for_each_ref(add_existing, &existing_refs);
>>       for (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {
>> -             if (prefixcmp(ref->name, "refs/tags"))
>> +             if (prefixcmp(ref->name, "refs/tags/"))
>>                       continue;
>>
>>               /*
>> diff --git a/builtin/remote.c b/builtin/remote.c
>> index 583eec9..f54a89a 100644
>> --- a/builtin/remote.c
>> +++ b/builtin/remote.c
>> @@ -534,7 +534,7 @@ static int add_branch_for_removal(const char *refname,
>>       }
>>
>>       /* don't delete non-remote-tracking refs */
>> -     if (prefixcmp(refname, "refs/remotes")) {
>> +     if (prefixcmp(refname, "refs/remotes/")) {
>>               /* advise user how to delete local branches */
>>               if (!prefixcmp(refname, "refs/heads/"))
>>                       string_list_append(branches->skipped,
>> diff --git a/log-tree.c b/log-tree.c
>> index 319bd31..535b905 100644
>> --- a/log-tree.c
>> +++ b/log-tree.c
>> @@ -119,9 +119,9 @@ static int add_ref_decoration(const char *refname, const unsigned char *sha1, in
>>               type = DECORATION_REF_REMOTE;
>>       else if (!prefixcmp(refname, "refs/tags/"))
>>               type = DECORATION_REF_TAG;
>> -     else if (!prefixcmp(refname, "refs/stash"))
>> +     else if (!strcmp(refname, "refs/stash"))
>>               type = DECORATION_REF_STASH;
>> -     else if (!prefixcmp(refname, "HEAD"))
>> +     else if (!strcmp(refname, "HEAD"))
>>               type = DECORATION_REF_HEAD;
>>
>>       if (!cb_data || *(int *)cb_data == DECORATE_SHORT_REFS)

-- 
Duy

Michael Haggerty· Jan 11, 2012, 04:49 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH] Fix incorrect ref namespace check

On 01/11/2012 02:54 AM, Nguyen Thai Ngoc Duy wrote:

> Any comments Mike?

Looks fine to me.

Theoretically, we could add an API for iterating over references
*excluding* a particular namespace, which (after hierarchical refs is
accepted) could be implemented a bit more efficiently than iterating
over all references and then excluding those in the unwanted namespace.
 But I think it would be far more trouble than it's worth.

Michael

Show 64 quoted lines

> 2012/1/5 Junio C Hamano <gitster@pobox.com>:
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>> The reason why the trailing slash is needed is obvious. refs/stash and
>>> HEAD are not namespace, but complete refs. Do full string compare on them.
>>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>>> ---
>>>  I missed prefixcmp(..., "HEAD") right below prefixcmp(..., "refs/stash")
>>
>> As Michael has been actively showing interest in cleaning up the area, he
>> should have been CC'ed, I would think.
>>
>>>
>>>  builtin/fetch.c  |    2 +-
>>>  builtin/remote.c |    2 +-
>>>  log-tree.c       |    4 ++--
>>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>>> index 33ad3aa..daa68d2 100644
>>> --- a/builtin/fetch.c
>>> +++ b/builtin/fetch.c
>>> @@ -573,7 +573,7 @@ static void find_non_local_tags(struct transport *transport,
>>>
>>>       for_each_ref(add_existing, &existing_refs);
>>>       for (ref = transport_get_remote_refs(transport); ref; ref = ref->next) {
>>> -             if (prefixcmp(ref->name, "refs/tags"))
>>> +             if (prefixcmp(ref->name, "refs/tags/"))
>>>                       continue;
>>>
>>>               /*
>>> diff --git a/builtin/remote.c b/builtin/remote.c
>>> index 583eec9..f54a89a 100644
>>> --- a/builtin/remote.c
>>> +++ b/builtin/remote.c
>>> @@ -534,7 +534,7 @@ static int add_branch_for_removal(const char *refname,
>>>       }
>>>
>>>       /* don't delete non-remote-tracking refs */
>>> -     if (prefixcmp(refname, "refs/remotes")) {
>>> +     if (prefixcmp(refname, "refs/remotes/")) {
>>>               /* advise user how to delete local branches */
>>>               if (!prefixcmp(refname, "refs/heads/"))
>>>                       string_list_append(branches->skipped,
>>> diff --git a/log-tree.c b/log-tree.c
>>> index 319bd31..535b905 100644
>>> --- a/log-tree.c
>>> +++ b/log-tree.c
>>> @@ -119,9 +119,9 @@ static int add_ref_decoration(const char *refname, const unsigned char *sha1, in
>>>               type = DECORATION_REF_REMOTE;
>>>       else if (!prefixcmp(refname, "refs/tags/"))
>>>               type = DECORATION_REF_TAG;
>>> -     else if (!prefixcmp(refname, "refs/stash"))
>>> +     else if (!strcmp(refname, "refs/stash"))
>>>               type = DECORATION_REF_STASH;
>>> -     else if (!prefixcmp(refname, "HEAD"))
>>> +     else if (!strcmp(refname, "HEAD"))
>>>               type = DECORATION_REF_HEAD;
>>>
>>>       if (!cb_data || *(int *)cb_data == DECORATE_SHORT_REFS)
> 
> 
>

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