[PATCH] Added a test for fetching remote tags when there is not tags.

[PATCH] Added a test for fetching remote tags when there is not tags.

[Väinö Järvelä]

When a user runs "git fetch -t", git crashes when it doesn't find any
tags on the remote repository.

Signed-off-by: Väinö Järvelä <v@pp.inet.fi>
---
 t/t5510-fetch.sh |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 73a4e3c..40ebf2e 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -67,6 +67,18 @@ test_expect_success "fetch test for-merge" '
 	cut -f -2 .git/FETCH_HEAD >actual &&
 	diff expected actual'
 
+test_expect_success 'fetch tags when there is no tags' '
+
+    cd "$D" &&
+
+    mkdir notags &&
+    cd notags &&
+    git init &&
+
+    git fetch -t ..
+
+'
+
 test_expect_success 'fetch following tags' '
 
 	cd "$D" &&
-- 
1.5.3.4.1156.g5407-dirty

[PATCH] Fixed crash in fetching remote tags when there is not tags.

[Väinö Järvelä]

Signed-off-by: Väinö Järvelä <v@pp.inet.fi>
---
 remote.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/remote.c b/remote.c
index e7d735b..5e92378 100644
--- a/remote.c
+++ b/remote.c
@@ -537,6 +537,8 @@ static int count_refspec_match(const char *pattern,
 
 static void tail_link_ref(struct ref *ref, struct ref ***tail)
 {
+	if (!ref) return;
+	
 	**tail = ref;
 	while (ref->next)
 		ref = ref->next;
-- 
1.5.3.4.1156.g5407-dirty

Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.

[Väinö Järvelä]

Hi,

These patches should have been numbered, sorry, The test patch was  
supposed be number 1 of 2.

The patches were done on top of next. The fix should be checked, as  
it's mostly a quick fix to get it to work, probably not the correct  
way to fix that bug, or is it?

--
Väinö

On Oct 9, 2007, at 11:51, Väinö Järvelä wrote:

> Signed-off-by: Väinö Järvelä <v@pp.inet.fi>
> ---
>  remote.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index e7d735b..5e92378 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -537,6 +537,8 @@ static int count_refspec_match(const char  
> *pattern,
>
>  static void tail_link_ref(struct ref *ref, struct ref ***tail)
>  {
> +	if (!ref) return;
> +	
>  	**tail = ref;
>  	while (ref->next)
>  		ref = ref->next;
> -- 
> 1.5.3.4.1156.g5407-dirty
>
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.

[Alex Riesen]

Väinö Järvelä, Tue, Oct 09, 2007 12:52:01 +0200:
> Hi,
> 
> These patches should have been numbered, sorry, The test patch was  
> supposed be number 1 of 2.
> 
> The patches were done on top of next. The fix should be checked, as  
> it's mostly a quick fix to get it to work, probably not the correct  
> way to fix that bug, or is it?

Your test does not crash on my system and your fix is obviously bogus.
Just take a look at the only call site of the tail_link_ref: ret
cannot be NULL. alloc_ref will crash in memset, if this were the case.

If you can reproduce the crash reliably, try compiling git with -g and
run it in your test with valgrind or gdb (assuming these are available
to you).

Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.

[Jeff King]

On Tue, Oct 09, 2007 at 08:20:43PM +0200, Alex Riesen wrote:

> Your test does not crash on my system and your fix is obviously bogus.
> Just take a look at the only call site of the tail_link_ref: ret
> cannot be NULL. alloc_ref will crash in memset, if this were the case.

His work is almost certainly on top of next, which crashes reliably with
the test and has an additional call site for tail_link_ref. Aside from
some trailing whitespace in the patch, I think his fix is reasonable.

-Peff

Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.

[Alex Riesen]

Jeff King, Wed, Oct 10, 2007 07:10:35 +0200:
> On Tue, Oct 09, 2007 at 08:20:43PM +0200, Alex Riesen wrote:
> 
> > Your test does not crash on my system and your fix is obviously bogus.
> > Just take a look at the only call site of the tail_link_ref: ret
> > cannot be NULL. alloc_ref will crash in memset, if this were the case.
> 
> His work is almost certainly on top of next, which crashes reliably with
> the test and has an additional call site for tail_link_ref. Aside from
> some trailing whitespace in the patch, I think his fix is reasonable.

Ach, I see.

Still, I'd suggest move the test into the caller, firstly because it
is the only place that special. Also, I can't think of a proper reason
to add a NULL ref to a reflist, and so the crashing tail_link_ref will
help us find the callers which use tail_link_ref incorrectly
(illogically too).

As the result of patter expansion can be NULL (empty pattern, as it
seems), lets just check for it. I parked the patch below locally.

diff --git a/remote.c b/remote.c
index 5e92378..58d63ed 100644
--- a/remote.c
+++ b/remote.c
@@ -884,7 +884,8 @@ int get_fetch_map(struct ref *remote_refs,
 			    rm->peer_ref->name);
 	}
 
-	tail_link_ref(ref_map, tail);
+	if (ref_map)
+		tail_link_ref(ref_map, tail);
 
 	return 0;
 }

Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.

[Jeff King]

On Wed, Oct 10, 2007 at 11:27:35PM +0200, Alex Riesen wrote:

> Still, I'd suggest move the test into the caller, firstly because it
> is the only place that special. Also, I can't think of a proper reason

Yes, I agree with that. I came very close to suggesting it in my other
mail, but then realized I had never even looked at the surrounding code,
and I ought not to be making assessments of how that data structure
should be used.  But now there are two of us. :)

-Peff

Re: [PATCH] Fixed crash in fetching remote tags when there is not tags.

[Shawn O. Pearce]

Alex Riesen <raa.lkml@gmail.com> wrote:
> Still, I'd suggest move the test into the caller, firstly because it
> is the only place that special. Also, I can't think of a proper reason
> to add a NULL ref to a reflist, and so the crashing tail_link_ref will
> help us find the callers which use tail_link_ref incorrectly
> (illogically too).
> 
> As the result of patter expansion can be NULL (empty pattern, as it
> seems), lets just check for it. I parked the patch below locally.
> 
> diff --git a/remote.c b/remote.c
> index 5e92378..58d63ed 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -884,7 +884,8 @@ int get_fetch_map(struct ref *remote_refs,
>  			    rm->peer_ref->name);
>  	}
>  
> -	tail_link_ref(ref_map, tail);
> +	if (ref_map)
> +		tail_link_ref(ref_map, tail);
>  
>  	return 0;
>  }

I disagree with Alex's argument above, because the result of the
pattern expansion can be NULL due to a pattern that matched nothing
from the remote.  This can easily happen if the user originally
configured "remote.$name.fetch = +refs/heads/subdir/*:..." and
then the remote deletes all branches from refs/heads/subdir at some
point in the future.

I think the above patch is the only thing to do here.  Perhaps Alex
can write up a formal patch and send it to back to the list and CC
Lars Hjemli <hjemli@gmail.com> so he can put it into the patch queue.

-- 
Shawn.

[PATCH] Fix a crash in ls-remote when refspec expands into nothing

[Alex Riesen]

Originally-by: Väinö Järvelä <v@pp.inet.fi>
Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 remote.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Shawn O. Pearce, Fri, Oct 12, 2007 06:07:45 +0200:
> I think the above patch is the only thing to do here.  Perhaps Alex
> can write up a formal patch and send it to back to the list and CC
> Lars Hjemli <hjemli@gmail.com> so he can put it into the patch queue.

here you go

diff --git a/remote.c b/remote.c
index e7d735b..a25f66d 100644
--- a/remote.c
+++ b/remote.c
@@ -882,7 +882,8 @@ int get_fetch_map(struct ref *remote_refs,
 			    rm->peer_ref->name);
 	}
 
-	tail_link_ref(ref_map, tail);
+	if (ref_map)
+		tail_link_ref(ref_map, tail);
 
 	return 0;
 }
-- 
1.5.3.4.232.ga843

Re: [PATCH] Fix a crash in ls-remote when refspec expands into nothing

[Lars Hjemli]

On 10/12/07, Alex Riesen <raa.lkml@gmail.com> wrote:
> Shawn O. Pearce, Fri, Oct 12, 2007 06:07:45 +0200:
> > I think the above patch is the only thing to do here.  Perhaps Alex
> > can write up a formal patch and send it to back to the list and CC
> > Lars Hjemli <hjemli@gmail.com> so he can put it into the patch queue.
>
> here you go
>

Thanks. I've replaced the tip of q/vj/fetch-t (in
git://hjemli.net/pub/git/git) with this patch.

-- 
larsh