From 6e17fb3409d90f496769a5af1646a65e6770625b Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 3 Jun 2019 21:13:26 -0500 Subject: [PATCH 1/5] t5801 (remote-helpers): cleanup refspec stuff The code is much simpler this way, specially thanks to: git fast-export --refspec Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- git-remote-testgit.sh | 11 ++++------- t/t5801-remote-helpers.sh | 8 ++++---- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/git-remote-testgit.sh b/git-remote-testgit.sh index 752c763eb6..f2b551dfaf 100755 --- a/git-remote-testgit.sh +++ b/git-remote-testgit.sh @@ -11,13 +11,10 @@ fi url=$2 dir="$GIT_DIR/testgit/$alias" -prefix="refs/testgit/$alias" -default_refspec="refs/heads/*:${prefix}/heads/*" +refspec="refs/heads/*:refs/testgit/$alias/heads/*" -refspec="${GIT_REMOTE_TESTGIT_REFSPEC-$default_refspec}" - -test -z "$refspec" && prefix="refs" +test -n "$GIT_REMOTE_TESTGIT_NOREFSPEC" && refspec="" GIT_DIR="$url/.git" export GIT_DIR @@ -81,10 +78,10 @@ do echo "feature done" git fast-export \ + ${refspec:+"--refspec=$refspec"} \ ${testgitmarks:+"--import-marks=$testgitmarks"} \ ${testgitmarks:+"--export-marks=$testgitmarks"} \ - $refs | - sed -e "s#refs/heads/#${prefix}/heads/#g" + $refs echo "done" ;; export) diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index aaaa722cca..9d1a514d18 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -124,7 +124,7 @@ test_expect_success 'forced push' ' ' test_expect_success 'cloning without refspec' ' - GIT_REMOTE_TESTGIT_REFSPEC="" \ + GIT_REMOTE_TESTGIT_NOREFSPEC=1 \ git clone "testgit::${PWD}/server" local2 2>error && test_i18ngrep "this remote helper should implement refspec capability" error && compare_refs local2 HEAD server HEAD @@ -133,7 +133,7 @@ test_expect_success 'cloning without refspec' ' test_expect_success 'pulling without refspecs' ' (cd local2 && git reset --hard && - GIT_REMOTE_TESTGIT_REFSPEC="" git pull 2>../error) && + GIT_REMOTE_TESTGIT_NOREFSPEC=1 git pull 2>../error) && test_i18ngrep "this remote helper should implement refspec capability" error && compare_refs local2 HEAD server HEAD ' @@ -143,8 +143,8 @@ test_expect_success 'pushing without refspecs' ' (cd local2 && echo content >>file && git commit -a -m ten && - GIT_REMOTE_TESTGIT_REFSPEC="" && - export GIT_REMOTE_TESTGIT_REFSPEC && + GIT_REMOTE_TESTGIT_NOREFSPEC=1 && + export GIT_REMOTE_TESTGIT_NOREFSPEC && test_must_fail git push 2>../error) && test_i18ngrep "remote-helper doesn.t support push; refspec needed" error ' From 8144f09ccde4df49cdb807a7ce9e6cb14a09bd2e Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 3 Jun 2019 21:13:27 -0500 Subject: [PATCH 2/5] t5801 (remote-helpers): add test to fetch tags This used to work, but commit e198b3a740 broke it. e198b3a740 (fetch: replace string-list used as a look-up table with a hashmap) Probably all remote helpers that use the import method are affected, but we didn't catch the issue. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- git-remote-testgit.sh | 17 ++++++++++++----- t/t5801-remote-helpers.sh | 10 ++++++++++ 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/git-remote-testgit.sh b/git-remote-testgit.sh index f2b551dfaf..6b9f0b5dc7 100755 --- a/git-remote-testgit.sh +++ b/git-remote-testgit.sh @@ -12,9 +12,14 @@ url=$2 dir="$GIT_DIR/testgit/$alias" -refspec="refs/heads/*:refs/testgit/$alias/heads/*" +h_refspec="refs/heads/*:refs/testgit/$alias/heads/*" +t_refspec="refs/tags/*:refs/testgit/$alias/tags/*" -test -n "$GIT_REMOTE_TESTGIT_NOREFSPEC" && refspec="" +if test -n "$GIT_REMOTE_TESTGIT_NOREFSPEC" +then + h_refspec="" + t_refspec="" +fi GIT_DIR="$url/.git" export GIT_DIR @@ -37,7 +42,8 @@ do capabilities) echo 'import' echo 'export' - test -n "$refspec" && echo "refspec $refspec" + test -n "$h_refspec" && echo "refspec $h_refspec" + test -n "$t_refspec" && echo "refspec $t_refspec" if test -n "$gitmarks" then echo "*import-marks $gitmarks" @@ -49,7 +55,7 @@ do echo ;; list) - git for-each-ref --format='? %(refname)' 'refs/heads/' + git for-each-ref --format='? %(refname)' 'refs/heads/' 'refs/tags/' head=$(git symbolic-ref HEAD) echo "@$head HEAD" echo @@ -78,7 +84,8 @@ do echo "feature done" git fast-export \ - ${refspec:+"--refspec=$refspec"} \ + ${h_refspec:+"--refspec=$h_refspec"} \ + ${t_refspec:+"--refspec=$t_refspec"} \ ${testgitmarks:+"--import-marks=$testgitmarks"} \ ${testgitmarks:+"--export-marks=$testgitmarks"} \ $refs diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index 9d1a514d18..4138354e00 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -301,4 +301,14 @@ test_expect_success 'fetch url' ' compare_refs server HEAD local FETCH_HEAD ' +test_expect_failure 'fetch tag' ' + (cd server && + git tag v1.0 + ) && + (cd local && + git fetch + ) && + compare_refs local v1.0 server v1.0 +' + test_done From a8363b57193ea3e881367ff06035d4a2fe021d59 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 3 Jun 2019 21:13:28 -0500 Subject: [PATCH 3/5] fetch: trivial cleanup Create a helper function to clear an item. The way items are cleared has changed, and will change again soon. No functional changes. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/fetch.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index aee1d9bf21..547b25d206 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -285,6 +285,11 @@ static int refname_hash_exists(struct hashmap *map, const char *refname) return !!hashmap_get_from_hash(map, strhash(refname), refname); } +static void clear_item(struct refname_hash_entry *item) +{ + oidclr(&item->oid); +} + static void find_non_local_tags(const struct ref *refs, struct ref **head, struct ref ***tail) @@ -318,7 +323,7 @@ static void find_non_local_tags(const struct ref *refs, !has_sha1_file_with_flags(item->oid.hash, OBJECT_INFO_QUICK) && !will_fetch(head, item->oid.hash)) - oidclr(&item->oid); + clear_item(item); item = NULL; continue; } @@ -332,7 +337,7 @@ static void find_non_local_tags(const struct ref *refs, if (item && !has_sha1_file_with_flags(item->oid.hash, OBJECT_INFO_QUICK) && !will_fetch(head, item->oid.hash)) - oidclr(&item->oid); + clear_item(item); item = NULL; @@ -353,7 +358,7 @@ static void find_non_local_tags(const struct ref *refs, if (item && !has_sha1_file_with_flags(item->oid.hash, OBJECT_INFO_QUICK) && !will_fetch(head, item->oid.hash)) - oidclr(&item->oid); + clear_item(item); /* * For all the tags in the remote_refs_list, From 9528b80b1a269540e12c95346f0c4b06a27dc37c Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 3 Jun 2019 21:13:29 -0500 Subject: [PATCH 4/5] fetch: make the code more understandable The comment makes it seem as if the condition is the other way around. The exception is when the oid is null, so check for that. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/fetch.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 547b25d206..0bf8fa7030 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -366,19 +366,21 @@ static void find_non_local_tags(const struct ref *refs, */ for_each_string_list_item(remote_ref_item, &remote_refs_list) { const char *refname = remote_ref_item->string; + struct ref *rm; item = hashmap_get_from_hash(&remote_refs, strhash(refname), refname); if (!item) BUG("unseen remote ref?"); /* Unless we have already decided to ignore this item... */ - if (!is_null_oid(&item->oid)) { - struct ref *rm = alloc_ref(item->refname); - rm->peer_ref = alloc_ref(item->refname); - oidcpy(&rm->old_oid, &item->oid); - **tail = rm; - *tail = &rm->next; - } + if (is_null_oid(&item->oid)) + continue; + + rm = alloc_ref(item->refname); + rm->peer_ref = alloc_ref(item->refname); + oidcpy(&rm->old_oid, &item->oid); + **tail = rm; + *tail = &rm->next; } hashmap_free(&remote_refs, 1); string_list_clear(&remote_refs_list, 0); From f80d922355302abf07bf0cf9a2135c2eaa61f502 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 3 Jun 2019 21:13:30 -0500 Subject: [PATCH 5/5] fetch: fix regression with transport helpers Commit e198b3a740 changed the behavior of fetch with regards to tags. Before, null oids where not ignored, now they are, regardless of whether the refs have been explicitly cleared or not. e198b3a740 (fetch: replace string-list used as a look-up table with a hashmap) When using a transport helper the oids can certainly be null. So now tags are ignored and fetching them is impossible. This patch fixes that by having a specific flag that is set only when we explicitly want to ignore the refs, restoring the original behavior. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/fetch.c | 5 +++-- t/t5801-remote-helpers.sh | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 0bf8fa7030..e485d429c9 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -237,6 +237,7 @@ static int will_fetch(struct ref **head, const unsigned char *sha1) struct refname_hash_entry { struct hashmap_entry ent; /* must be the first member */ struct object_id oid; + int ignore; char refname[FLEX_ARRAY]; }; @@ -287,7 +288,7 @@ static int refname_hash_exists(struct hashmap *map, const char *refname) static void clear_item(struct refname_hash_entry *item) { - oidclr(&item->oid); + item->ignore = 1; } static void find_non_local_tags(const struct ref *refs, @@ -373,7 +374,7 @@ static void find_non_local_tags(const struct ref *refs, BUG("unseen remote ref?"); /* Unless we have already decided to ignore this item... */ - if (is_null_oid(&item->oid)) + if (item->ignore) continue; rm = alloc_ref(item->refname); diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index 4138354e00..1829ef29c7 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -301,7 +301,7 @@ test_expect_success 'fetch url' ' compare_refs server HEAD local FETCH_HEAD ' -test_expect_failure 'fetch tag' ' +test_expect_success 'fetch tag' ' (cd server && git tag v1.0 ) &&