threads / rfc / 51234

RFC patch, 5 partsFix fetch regression with transport helpers

Subject: [RFC/PATCH 0/5] Fix fetch regression with transport helpers

## tl;dr

18 messages between Jun 4, 2019 and Jun 7, 2019. Diffs are folded; open one to read it.

replies: 17people: 5as markdown or json

Felipe Contreras· Jun 4, 2019, 02:13 UTC · lore

I noticed a regression with running tests for git-remote-hg; can't seem to be able to fetch tags.

Probably all remote helpers that use the import method are affected, if not all.

The following patches are meant to test for the issue, fix it, and get some cleanups.

I'm not exactly sure the solution is the one we want, but hopefull it gives an idea as to what is needed.

Felipe Contreras (5):
  t5801 (remote-helpers): cleanup refspec stuff
  t5801 (remote-helpers): add test to fetch tags
  fetch: trivial cleanup
  fetch: make the code more understandable
  fetch: fix regression with transport helpers
 builtin/fetch.c            | 28 ++++++++++++++++++----------
 t/t5801-remote-helpers.sh  | 18 ++++++++++++++----
 t/t5801/git-remote-testgit | 22 +++++++++++++---------
 3 files changed, 45 insertions(+), 23 deletions(-)
-- 
2.21.0
Felipe Contreras· Jun 4, 2019, 02:13 UTC · re: Felipe Contreras · lore

[RFC/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 <felipe.contreras@gmail.com>
---
 t/t5801-remote-helpers.sh  |  8 ++++----
 t/t5801/git-remote-testgit | 11 ++++-------
 2 files changed, 8 insertions(+), 11 deletions(-)
Show changes to 2 files +8 −11

t/t5801-remote-helpers.sh, t/t5801/git-remote-testgit

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index d04f8007e0..48bed7c2fe 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -126,7 +126,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
@@ -135,7 +135,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
 '
@@ -145,8 +145,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
 '
diff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit
index 752c763eb6..f2b551dfaf 100755
--- a/t/t5801/git-remote-testgit
+++ b/t/t5801/git-remote-testgit
@@ -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)
-- 
2.21.0
Jeff King· Jun 4, 2019, 14:16 UTC · re: Felipe Contreras · lore

Re: [RFC/PATCH 1/5] t5801 (remote-helpers): cleanup refspec stuff

On Mon, Jun 03, 2019 at 09:13:26PM -0500, Felipe Contreras wrote:
Show 17 quoted lines
> diff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit
> index 752c763eb6..f2b551dfaf 100755
> --- a/t/t5801/git-remote-testgit
> +++ b/t/t5801/git-remote-testgit
> @@ -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=""

So this simplifies the feature by just allowing two refspecs: the default one, or none at all. And that works because all of the existing callers wanted or the other. Makes sense.

Show 10 quoted lines
> @@ -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

This second hunk puzzled me for a minute. By using --refspec, we can avoid doing the mapping with sed here, which is simpler and more robust. Good.

One caveat for anybody else testing this: when I initially applied this patch, some tests failed! The problem turned out to be a leftover git-remote-testgit in my build dir from prior to 5afb2ce4cd (remote-testgit: move it into the support directory for t5801, 2019-04-12).

So the problem isn't related to this patch (it's only noticeable here because this is the first change to remote-testgit since it moved). I wonder if we could make the test script more robust here. I think it's tricky because the build dir is added to the $PATH internally by Git itself (since it's the exec-path), so nothing do in the test script can override that. Perhaps it's worth just renaming the script as part of the move. +cc Dscho

-Peff
Felipe Contreras· Jun 4, 2019, 02:13 UTC · re: Felipe Contreras · lore

[RFC/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 <felipe.contreras@gmail.com>
---
 t/t5801-remote-helpers.sh  | 10 ++++++++++
 t/t5801/git-remote-testgit | 17 ++++++++++++-----
 2 files changed, 22 insertions(+), 5 deletions(-)
Show changes to 2 files +22 −5

t/t5801-remote-helpers.sh, t/t5801/git-remote-testgit

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 48bed7c2fe..238774bc17 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -303,4 +303,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
diff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit
index f2b551dfaf..6b9f0b5dc7 100755
--- a/t/t5801/git-remote-testgit
+++ b/t/t5801/git-remote-testgit
@@ -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
-- 
2.21.0
Jeff King· Jun 4, 2019, 14:22 UTC · re: Felipe Contreras · lore

Re: [RFC/PATCH 2/5] t5801 (remote-helpers): add test to fetch tags

On Mon, Jun 03, 2019 at 09:13:27PM -0500, Felipe Contreras wrote:
Show 6 quoted lines
> 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.

Thanks for beefing up the tests. It's rather unfortunate that we missed such a severe regression.

Show 9 quoted lines
> +test_expect_failure 'fetch tag' '
> +	(cd server &&
> +	 git tag v1.0
> +	) &&
> +	(cd local &&
> +	 git fetch
> +	) &&
> +	compare_refs local v1.0 server v1.0
> +'

This single-commands might be more readable using "git -C", but this matches the existing style in the test script, so I'm OK with it either way.

Show 18 quoted lines
> diff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit
> index f2b551dfaf..6b9f0b5dc7 100755
> --- a/t/t5801/git-remote-testgit
> +++ b/t/t5801/git-remote-testgit
> @@ -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
And the simplification from the prior step pays off. Looks good.

The rest of it seems pretty sensible (modulo the caveat that I don't know remote-testgit well enough to detect anything subtle).

-Peff
Felipe Contreras· Jun 4, 2019, 02:13 UTC · re: Felipe Contreras · lore

[RFC/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 <felipe.contreras@gmail.com>
---
 builtin/fetch.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
Show changes to builtin/fetch.c +8 −3
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4ba63d5ac6..8af5e319f1 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -287,6 +287,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)
@@ -319,7 +324,7 @@ static void find_non_local_tags(const struct ref *refs,
 			    !will_fetch(head, ref->old_oid.hash) &&
 			    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&
 			    !will_fetch(head, item->oid.hash))
-				oidclr(&item->oid);
+				clear_item(item);
 			item = NULL;
 			continue;
 		}
@@ -333,7 +338,7 @@ static void find_non_local_tags(const struct ref *refs,
 		if (item &&
 		    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&
 		    !will_fetch(head, item->oid.hash))
-			oidclr(&item->oid);
+			clear_item(item);
 
 		item = NULL;
 
@@ -354,7 +359,7 @@ static void find_non_local_tags(const struct ref *refs,
 	if (item &&
 	    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&
 	    !will_fetch(head, item->oid.hash))
-		oidclr(&item->oid);
+		clear_item(item);
 
 	/*
 	 * For all the tags in the remote_refs_list,
-- 
2.21.0
Felipe Contreras· Jun 4, 2019, 02:13 UTC · re: Felipe Contreras · lore

[RFC/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 <felipe.contreras@gmail.com>
---
 builtin/fetch.c           | 5 +++--
 t/t5801-remote-helpers.sh | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)
Show changes to 2 files +4 −3

builtin/fetch.c, t/t5801-remote-helpers.sh

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 9dc551551e..f2be50a4a3 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -239,6 +239,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];
 };
 
@@ -289,7 +290,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,
@@ -374,7 +375,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 238774bc17..2d6c4a281e 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -303,7 +303,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
 	) &&
-- 
2.21.0
Jeff King· Jun 4, 2019, 14:32 UTC · re: Felipe Contreras · lore

Re: [RFC/PATCH 5/5] fetch: fix regression with transport helpers

On Mon, Jun 03, 2019 at 09:13:30PM -0500, Felipe Contreras wrote:
Show 11 quoted lines
> 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.

Makes sense. Prior to e198b3a740, we indicated "ignored" by setting the oid (in item->util) to NULL. But since it's no longer a pointer after that commit, we can't do so.

So the obvious alternative to this is going back to having it as a pointer. I think I prefer your solution, though, since it keeps the memory management a bit simpler, and more clearly expresses the intent.

If we add any new code that has to similarly ignore an item in the hash, it will have to remember to use clear_item() rather than oidclr(). But I don't think that's a big risk.

>  builtin/fetch.c           | 5 +++--
>  t/t5801-remote-helpers.sh | 2 +-
>  2 files changed, 4 insertions(+), 3 deletions(-)
The code change itself looks obviously correct.
-Peff
Junio C Hamano· Jun 4, 2019, 19:17 UTC · re: Felipe Contreras · lore

Re: [RFC/PATCH 5/5] fetch: fix regression with transport helpers

Felipe Contreras <felipe.contreras@gmail.com> writes:
Show 46 quoted lines
> 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 <felipe.contreras@gmail.com>
> ---
>  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 9dc551551e..f2be50a4a3 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -239,6 +239,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];
>  };
>  
> @@ -289,7 +290,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,
> @@ -374,7 +375,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;

Yeah, we should have added a bit like this to preserve the distinction between a NULL pointer for "struct object_id" and oid that has 0{40} in the old code, which was lost in the conversion.

Thanks for spotting and fixing.
Felipe Contreras· Jun 4, 2019, 02:13 UTC · re: Felipe Contreras · lore

[RFC/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 <felipe.contreras@gmail.com>
---
 builtin/fetch.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)
Show changes to builtin/fetch.c +9 −7
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8af5e319f1..9dc551551e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -367,19 +367,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);
-- 
2.21.0
Jeff King· Jun 4, 2019, 14:35 UTC · re: Felipe Contreras · lore

Re: [RFC/PATCH 0/5] Fix fetch regression with transport helpers

On Mon, Jun 03, 2019 at 09:13:25PM -0500, Felipe Contreras wrote:
Show 11 quoted lines
> I noticed a regression with running tests for git-remote-hg; can't seem to be
> able to fetch tags.
> 
> Probably all remote helpers that use the import method are affected, if not
> all.
> 
> The following patches are meant to test for the issue, fix it, and get some
> cleanups.
> 
> I'm not exactly sure the solution is the one we want, but hopefull it gives an
> idea as to what is needed.
It looks good to me. Thanks for fixing this.

The breakage is in v2.20, so I don't think this needs to be rushed into v2.22 (which is already at -rc3). But it should probably go to 'maint' sooner rather than later.

-Peff
Felipe Contreras· Jun 4, 2019, 19:15 UTC · re: Jeff King · lore

Re: [RFC/PATCH 0/5] Fix fetch regression with transport helpers

On Tue, Jun 4, 2019 at 9:35 AM Jeff King <peff@peff.net> wrote:
>
> On Mon, Jun 03, 2019 at 09:13:25PM -0500, Felipe Contreras wrote:
Show 8 quoted lines
> > I'm not exactly sure the solution is the one we want, but hopefull it gives an
> > idea as to what is needed.
>
> It looks good to me. Thanks for fixing this.
>
> The breakage is in v2.20, so I don't think this needs to be rushed into
> v2.22 (which is already at -rc3). But it should probably go to 'maint'
> sooner rather than later.

Indeed, it's probably not so urgent since people have not even been complaining (that I know of). However, I think it's clear that there's a regression and the fix is simple, so maybe just apply the obvious fix, and leave the rest of the patches for later?

I'm attaching only the fix, just to temp the idea of applying that tiny thing into v2.2.

-- 
Felipe Contreras


From a59903e9828a4f265647014ca87fa3be9016a225 Mon Sep 17 00:00:00 2001
From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Tue, 4 Jun 2019 14:10:43 -0500
Subject: [PATCH] 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 <felipe.contreras@gmail.com>
---
 builtin/fetch.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4ba63d5ac6..f4962b93d6 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -239,6 +239,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];
 };
 
@@ -319,7 +320,7 @@ static void find_non_local_tags(const struct ref *refs,
 			    !will_fetch(head, ref->old_oid.hash) &&
 			    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&
 			    !will_fetch(head, item->oid.hash))
-				oidclr(&item->oid);
+				item->ignore = 1;
 			item = NULL;
 			continue;
 		}
@@ -333,7 +334,7 @@ static void find_non_local_tags(const struct ref *refs,
 		if (item &&
 		    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&
 		    !will_fetch(head, item->oid.hash))
-			oidclr(&item->oid);
+			item->ignore = 1;
 
 		item = NULL;
 
@@ -354,7 +355,7 @@ static void find_non_local_tags(const struct ref *refs,
 	if (item &&
 	    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&
 	    !will_fetch(head, item->oid.hash))
-		oidclr(&item->oid);
+		item->ignore = 1;
 
 	/*
 	 * For all the tags in the remote_refs_list,
@@ -368,7 +369,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) {
 			struct ref *rm = alloc_ref(item->refname);
 			rm->peer_ref = alloc_ref(item->refname);
 			oidcpy(&rm->old_oid, &item->oid);
-- 
2.21.0
Johannes Schindelin· Jun 5, 2019, 08:12 UTC · re: Felipe Contreras · lore

Re: [RFC/PATCH 0/5] Fix fetch regression with transport helpers

On Mon, 3 Jun 2019, Felipe Contreras wrote:
Show 11 quoted lines
> Felipe Contreras (5):
>   t5801 (remote-helpers): cleanup refspec stuff
>   t5801 (remote-helpers): add test to fetch tags
>   fetch: trivial cleanup
>   fetch: make the code more understandable
>   fetch: fix regression with transport helpers
>
>  builtin/fetch.c            | 28 ++++++++++++++++++----------
>  t/t5801-remote-helpers.sh  | 18 ++++++++++++++----
>  t/t5801/git-remote-testgit | 22 +++++++++++++---------
>  3 files changed, 45 insertions(+), 23 deletions(-)

This fails on macOS, in t5601, both in our osx-clang and osx-gcc jobs, as well as in the StaticAnalysis job. For details, see https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10206

The t5601 failure looks like this:

-- snip -- checking prerequisite: CASE_INSENSITIVE_FS

mkdir -p "$TRASH_DIRECTORY/prereq-test-dir" &&
(
	cd "$TRASH_DIRECTORY/prereq-test-dir" &&
	echo good >CamelCase &&
	echo bad >camelcase &&
	test "$(cat CamelCase)" != good
)
++ mkdir -p '/Users/vsts/agent/2.150.3/work/1/s/t/trash
directory.t5601-clone/prereq-test-dir'
++ cd '/Users/vsts/agent/2.150.3/work/1/s/t/trash
directory.t5601-clone/prereq-test-dir'
++ echo good
++ echo bad
+++ cat CamelCase
++ test bad '!=' good
prerequisite CASE_INSENSITIVE_FS ok
expecting success:
	grep X icasefs/warning &&
	grep x icasefs/warning &&
	test_i18ngrep "the following paths have collided" icasefs/warning

++ grep X icasefs/warning error: last command exited with $?=1 not ok 99 - colliding file detection -- snap --

My guess is that your changes remove something that was expected before, and is still expected, and that this was only tested on Linux, and only on a file system with case-sensitive file names.

The StaticAnalysis job (which is admittedly a misnomer) points out another few valid issues, but that is probably because Junio applied this patch series on top of a very old commit. I, at least, could not spot any file in the Coccinelle report that was touched by this here patch series.

Ciao, Johannes

Jeff King· Jun 5, 2019, 11:27 UTC · re: Johannes Schindelin · lore

Re: [RFC/PATCH 0/5] Fix fetch regression with transport helpers

On Wed, Jun 05, 2019 at 10:12:12AM +0200, Johannes Schindelin wrote:
> This fails on macOS, in t5601, both in our osx-clang and osx-gcc jobs, as
> well as in the StaticAnalysis job. For details, see
> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10206

Hmm. I'm having a hard time seeing why (and I can't seem to reproduce it locally on a case-insensitive HFS+ filesystem under Linux).

In particular, if the problem is here:
Show 8 quoted lines
> expecting success:
> 	grep X icasefs/warning &&
> 	grep x icasefs/warning &&
> 	test_i18ngrep "the following paths have collided" icasefs/warning
> 
> ++ grep X icasefs/warning
> error: last command exited with $?=1
> not ok 99 - colliding file detection

then that implies it has to do with the checkout phase, which Felipe's patch doesn't touch. Later in the log we see the actual file contents (I'm confused as to how this gets here; it looks like debugging bits that were added after the main script?):

  2019-06-05T07:58:37.7961890Z Cloning into 'bogus'...
  2019-06-05T07:58:37.7962430Z done.
  2019-06-05T07:58:37.7963360Z warning: the following paths have collided (e.g. case-sensitive paths
  2019-06-05T07:58:37.7964300Z on a case-insensitive filesystem) and only one from the same
  2019-06-05T07:58:37.7964880Z colliding group is in the working tree:
  2019-06-05T07:58:37.7965290Z 
  2019-06-05T07:58:37.7966250Z   'x'
whereas a succeeding test expects us to mention both 'x' and 'X'.

So we _did_ find the collision, but somehow 'X' was not reported. Looking at the code, I'm not even sure how that could happen. Given that this process does involve looking at stat data, it makes me wonder if there could be some raciness involved. But again, I'm scratching my head as to how exactly, and I couldn't reproduce it under load or with some carefully inserted sleep() calls.

And it looks like it did reproduce twice on Azure.

Can somebody who has osx locally reproduce this? Or is there a way to interactively access the Azure environment to dig further?

> My guess is that your changes remove something that was expected before,
> and is still expected, and that this was only tested on Linux, and only on
> a file system with case-sensitive file names.

It sounds like you're suggesting that changes to the test script subtly affected the later state. Which is indeed a common culprit. But the changes in Felipe's series were all to t5801, not the failing t5601. Am I misunderstanding what you mean?

-Peff
Duy Nguyen· Jun 5, 2019, 12:22 UTC · re: Jeff King · lore

Re: [RFC/PATCH 0/5] Fix fetch regression with transport helpers

On Wed, Jun 5, 2019 at 6:27 PM Jeff King <peff@peff.net> wrote:
Show 39 quoted lines
>
> On Wed, Jun 05, 2019 at 10:12:12AM +0200, Johannes Schindelin wrote:
>
> > This fails on macOS, in t5601, both in our osx-clang and osx-gcc jobs, as
> > well as in the StaticAnalysis job. For details, see
> > https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10206
>
> Hmm. I'm having a hard time seeing why (and I can't seem to reproduce it
> locally on a case-insensitive HFS+ filesystem under Linux).
>
> In particular, if the problem is here:
>
> > expecting success:
> >       grep X icasefs/warning &&
> >       grep x icasefs/warning &&
> >       test_i18ngrep "the following paths have collided" icasefs/warning
> >
> > ++ grep X icasefs/warning
> > error: last command exited with $?=1
> > not ok 99 - colliding file detection
>
> then that implies it has to do with the checkout phase, which Felipe's
> patch doesn't touch. Later in the log we see the actual file contents
> (I'm confused as to how this gets here; it looks like debugging bits
> that were added after the main script?):
>
>   2019-06-05T07:58:37.7961890Z Cloning into 'bogus'...
>   2019-06-05T07:58:37.7962430Z done.
>   2019-06-05T07:58:37.7963360Z warning: the following paths have collided (e.g. case-sensitive paths
>   2019-06-05T07:58:37.7964300Z on a case-insensitive filesystem) and only one from the same
>   2019-06-05T07:58:37.7964880Z colliding group is in the working tree:
>   2019-06-05T07:58:37.7965290Z
>   2019-06-05T07:58:37.7966250Z   'x'
>
> whereas a succeeding test expects us to mention both 'x' and 'X'.
>
> So we _did_ find the collision, but somehow 'X' was not reported.
> Looking at the code, I'm not even sure how that could happen. Given that
> this process does involve looking at stat data, it makes me wonder if

It does use stat data in mark_colliding_entries() if core.checkStat is false. I think on MacOS it's actually true.

I vaguely recall seeing just one 'x' once. I think last time I had a problem with truncating st_ino, but that should be fixed in e66ceca94b (clone: fix colliding file detection on APFS, 2018-11-20). So no idea how this happens again.

> there could be some raciness involved. But again, I'm scratching my head
> as to how exactly, and I couldn't reproduce it under load or with some
> carefully inserted sleep() calls.
-- 
Duy
Johannes Schindelin· Jun 6, 2019, 13:07 UTC · re: Duy Nguyen · lore

Re: [RFC/PATCH 0/5] Fix fetch regression with transport helpers

Hi Duy,
On Wed, 5 Jun 2019, Duy Nguyen wrote:
Show 48 quoted lines
> On Wed, Jun 5, 2019 at 6:27 PM Jeff King <peff@peff.net> wrote:
> >
> > On Wed, Jun 05, 2019 at 10:12:12AM +0200, Johannes Schindelin wrote:
> >
> > > This fails on macOS, in t5601, both in our osx-clang and osx-gcc jobs, as
> > > well as in the StaticAnalysis job. For details, see
> > > https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10206
> >
> > Hmm. I'm having a hard time seeing why (and I can't seem to reproduce it
> > locally on a case-insensitive HFS+ filesystem under Linux).
> >
> > In particular, if the problem is here:
> >
> > > expecting success:
> > >       grep X icasefs/warning &&
> > >       grep x icasefs/warning &&
> > >       test_i18ngrep "the following paths have collided" icasefs/warning
> > >
> > > ++ grep X icasefs/warning
> > > error: last command exited with $?=1
> > > not ok 99 - colliding file detection
> >
> > then that implies it has to do with the checkout phase, which Felipe's
> > patch doesn't touch. Later in the log we see the actual file contents
> > (I'm confused as to how this gets here; it looks like debugging bits
> > that were added after the main script?):
> >
> >   2019-06-05T07:58:37.7961890Z Cloning into 'bogus'...
> >   2019-06-05T07:58:37.7962430Z done.
> >   2019-06-05T07:58:37.7963360Z warning: the following paths have collided (e.g. case-sensitive paths
> >   2019-06-05T07:58:37.7964300Z on a case-insensitive filesystem) and only one from the same
> >   2019-06-05T07:58:37.7964880Z colliding group is in the working tree:
> >   2019-06-05T07:58:37.7965290Z
> >   2019-06-05T07:58:37.7966250Z   'x'
> >
> > whereas a succeeding test expects us to mention both 'x' and 'X'.
> >
> > So we _did_ find the collision, but somehow 'X' was not reported.
> > Looking at the code, I'm not even sure how that could happen. Given that
> > this process does involve looking at stat data, it makes me wonder if
>
> It does use stat data in mark_colliding_entries() if core.checkStat is
> false. I think on MacOS it's actually true.
>
> I vaguely recall seeing just one 'x' once. I think last time I had a
> problem with truncating st_ino, but that should be fixed in e66ceca94b
> (clone: fix colliding file detection on APFS, 2018-11-20). So no idea
> how this happens again.

Good catch. I think the reason it happens again is simply that Junio picked a base commit that is older than the commit you referenced.

Point in favor: Junio merged these here patches into `pu` and those test failures (as well as the StaticAnalysis issues) are gone.

Thanks, Johannes

Show 7 quoted lines
>
> > there could be some raciness involved. But again, I'm scratching my head
> > as to how exactly, and I couldn't reproduce it under load or with some
> > carefully inserted sleep() calls.
> --
> Duy
>
Junio C Hamano· Jun 6, 2019, 16:13 UTC · re: Johannes Schindelin · lore

Re: [RFC/PATCH 0/5] Fix fetch regression with transport helpers

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 7 quoted lines
>> I vaguely recall seeing just one 'x' once. I think last time I had a
>> problem with truncating st_ino, but that should be fixed in e66ceca94b
>> (clone: fix colliding file detection on APFS, 2018-11-20). So no idea
>> how this happens again.
>
> Good catch. I think the reason it happens again is simply that Junio
> picked a base commit that is older than the commit you referenced.

Yeah, that is because the patch specifically targetted a single commit as culprit, so naturally the topic that introduced that commit was the place to be "fixed" ;-)

I was wondering if the base commit _before_ the fixes, i.e. e198b3a7 ("fetch: replace string-list used as a look-up table with a hashmap", 2018-09-25), failed the same test you saw problems with. It does predate e66ceca9 ("clone: fix colliding file detection on APFS", 2018-11-20), so my current theory is that it was broken already adn these patches that fixed a breakge had nothing to do with the t5601 tests failing.

Johannes Schindelin· Jun 7, 2019, 05:32 UTC · re: Junio C Hamano · lore

CI builds on GitGitGadget, was Re: [RFC/PATCH 0/5] Fix fetch regression with transport helpers

Hi Junio,
On Thu, 6 Jun 2019, Junio C Hamano wrote:
Show 13 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> I vaguely recall seeing just one 'x' once. I think last time I had a
> >> problem with truncating st_ino, but that should be fixed in e66ceca94b
> >> (clone: fix colliding file detection on APFS, 2018-11-20). So no idea
> >> how this happens again.
> >
> > Good catch. I think the reason it happens again is simply that Junio
> > picked a base commit that is older than the commit you referenced.
>
> Yeah, that is because the patch specifically targetted a single
> commit as culprit, so naturally the topic that introduced that
> commit was the place to be "fixed" ;-)
Yes, that matches my impression.

I should maybe describe a bit better what *is* tested on GitGitGadget when it runs the build & test suite for the individual branches (as opposed to the integration branches maint, master, next & pu): the Azure Pipeline obviously *cannot* be defined in `azure-pipelines.yml`, as many of those branches do not even have that file.

One of the features I like most about Azure Pipelines (yes, I really like Pipelines, they save me from so much work by enabling me to automate a *lot* in Git for Windows' maintenance, such as building and packaging quite a few of the components such as OpenSSH, cURL, etc... but I digress) is that it offers *both* to define the builds via a file that is committed *and also* in a definition that is maintained outside of the source code.

So what I did was to port azure-pipelines.yml from the former to the latter, and *that* is run on those individual branches.

As we noticed here, this opens the door for running into regressions that have been long fixed, just not in the tested branch.

Side note: many projects that want to rely on the confidence instilled by automated testing therefore change their workflow to a more "topic branches are based on master, or on the release branches' tips" one. I am not saying that you, Junio, should switch to such a workflow because you are clearly comfortable with the current one. I mention this mainly for the benefit of readers who might wonder what options they have in their own projects to deal with this.

What I usually do is a hack: this "manual" job definition tries to cherry-pick all kinds of known fixes to known regressions, and this APFS-name-collision one is just not one of them. When I find the time (hopefully next week, probably not), I shall try to take care of that.

Show 7 quoted lines
> I was wondering if the base commit _before_ the fixes, i.e. e198b3a7
> ("fetch: replace string-list used as a look-up table with a
> hashmap", 2018-09-25), failed the same test you saw problems with.
> It does predate e66ceca9 ("clone: fix colliding file detection on
> APFS", 2018-11-20), so my current theory is that it was broken
> already adn these patches that fixed a breakge had nothing to do
> with the t5601 tests failing.
Yes, I agree.

One might ask why I even bother with testing the individual branches. The reason is that bisecting breakages in `pu` is a *nightmare*. Most of those breakages are present already in the individual branches, though, so if I can catch the breakages there, I have a much easier time keeping the CI builds green.

Ciao, Dscho

← back to recent threads