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

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

[Felipe Contreras]

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

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

[Felipe Contreras]

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

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

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

[Felipe Contreras]

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

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

[RFC/PATCH 3/5] fetch: trivial cleanup

[Felipe Contreras]

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

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

[RFC/PATCH 4/5] fetch: make the code more understandable

[Felipe Contreras]

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

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

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

[Felipe Contreras]

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

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

[Jeff King]

On Mon, Jun 03, 2019 at 09:13:26PM -0500, Felipe Contreras wrote:

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

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

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

[Jeff King]

On Mon, Jun 03, 2019 at 09:13:27PM -0500, Felipe Contreras wrote:

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

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

> 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

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

[Jeff King]

On Mon, Jun 03, 2019 at 09:13:30PM -0500, Felipe Contreras wrote:

> 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

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

[Jeff King]

On Mon, Jun 03, 2019 at 09:13:25PM -0500, Felipe Contreras wrote:

> 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

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

[Felipe Contreras]

[-- Attachment #1: Type: text/plain, Size: 845 bytes --]

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:

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

[-- Attachment #2: 0001-fetch-fix-regression-with-transport-helpers.patch --]
[-- Type: text/x-patch, Size: 2538 bytes --]

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

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

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

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

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

[Johannes Schindelin]

On Mon, 3 Jun 2019, Felipe Contreras wrote:

> 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

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

[Jeff King]

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

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

[Duy Nguyen]

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.

> 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

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

[Johannes Schindelin]

Hi Duy,

On Wed, 5 Jun 2019, Duy Nguyen wrote:

> 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

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

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

[Junio C Hamano]

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" ;-)

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.

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

[Johannes Schindelin]

Hi Junio,

On Thu, 6 Jun 2019, Junio C Hamano wrote:

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

> 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