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

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ä <redacted>
---
 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

Signed-off-by: Väinö Järvelä <redacted>
---
 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

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:

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).

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

Jeff King, Wed, Oct 10, 2007 07:10:35 +0200:

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

quoted

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;
 }

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

Alex Riesen [off-list ref] wrote:

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 [off-list ref] so he can put it into the patch queue.

-- 
Shawn.

Originally-by: Väinö Järvelä [off-list ref]
Signed-off-by: Alex Riesen <redacted>
---
 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 [off-list ref] 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

On 10/12/07, Alex Riesen [off-list ref] wrote:

Shawn O. Pearce, Fri, Oct 12, 2007 06:07:45 +0200:

quoted

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 [off-list ref] 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