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

-

[ message continues ]

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

-

[ message continues ]

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ö


-

[ message continues ]

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

-

[ message continues ]

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
-

[ message continues ]

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

[ message continues ]

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
-

[ message continues ]

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

[ message continues ]

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


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

-

[ message continues ]

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

-- 
larsh
-

[ message continues ]