{"thread":{"id":"51234","subject":"[RFC/PATCH 0/5] Fix fetch regression with transport helpers","startedAt":"2019-06-04T02:13:34Z","lastAt":"2019-06-07T05:32:41Z","messageCount":18,"participants":["Felipe Contreras","Jeff King","Junio C Hamano","Johannes Schindelin","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"376634","messageId":"20190604021330.16130-1-felipe.contreras@gmail.com","threadId":"51234","inReplyTo":null,"subject":"[RFC/PATCH 0/5] Fix fetch regression with transport helpers","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2019-06-04T02:13:25Z","receivedAt":"2019-06-04T02:13:34Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"I noticed a regression with running tests for git-remote-hg; can't seem to be\nable to fetch tags.\n\nProbably all remote helpers that use the import method are affected, if not\nall.\n\nThe following patches are meant to test for the issue, fix it, and get some\ncleanups.\n\nI'm not exactly sure the solution is the one we want, but hopefull it gives an\nidea as to what is needed.\n\n\nFelipe Contreras (5):\n  t5801 (remote-helpers): cleanup refspec stuff\n  t5801 (remote-helpers): add test to fetch tags\n  fetch: trivial cleanup\n  fetch: make the code more understandable\n  fetch: fix regression with transport helpers\n\n builtin/fetch.c            | 28 ++++++++++++++++++----------\n t/t5801-remote-helpers.sh  | 18 ++++++++++++++----\n t/t5801/git-remote-testgit | 22 +++++++++++++---------\n 3 files changed, 45 insertions(+), 23 deletions(-)\n\n-- \n2.21.0\n\n"},{"id":"376635","messageId":"20190604021330.16130-2-felipe.contreras@gmail.com","threadId":"51234","inReplyTo":"20190604021330.16130-1-felipe.contreras@gmail.com","subject":"[RFC/PATCH 1/5] t5801 (remote-helpers): cleanup refspec stuff","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2019-06-04T02:13:26Z","receivedAt":"2019-06-04T02:13:35Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"The code is much simpler this way, specially thanks to:\n\n  git fast-export --refspec\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n t/t5801-remote-helpers.sh  |  8 ++++----\n t/t5801/git-remote-testgit | 11 ++++-------\n 2 files changed, 8 insertions(+), 11 deletions(-)\n\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex d04f8007e0..48bed7c2fe 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -126,7 +126,7 @@ test_expect_success 'forced push' '\n '\n \n test_expect_success 'cloning without refspec' '\n-\tGIT_REMOTE_TESTGIT_REFSPEC=\"\" \\\n+\tGIT_REMOTE_TESTGIT_NOREFSPEC=1 \\\n \tgit clone \"testgit::${PWD}/server\" local2 2>error &&\n \ttest_i18ngrep \"this remote helper should implement refspec capability\" error &&\n \tcompare_refs local2 HEAD server HEAD\n@@ -135,7 +135,7 @@ test_expect_success 'cloning without refspec' '\n test_expect_success 'pulling without refspecs' '\n \t(cd local2 &&\n \tgit reset --hard &&\n-\tGIT_REMOTE_TESTGIT_REFSPEC=\"\" git pull 2>../error) &&\n+\tGIT_REMOTE_TESTGIT_NOREFSPEC=1 git pull 2>../error) &&\n \ttest_i18ngrep \"this remote helper should implement refspec capability\" error &&\n \tcompare_refs local2 HEAD server HEAD\n '\n@@ -145,8 +145,8 @@ test_expect_success 'pushing without refspecs' '\n \t(cd local2 &&\n \techo content >>file &&\n \tgit commit -a -m ten &&\n-\tGIT_REMOTE_TESTGIT_REFSPEC=\"\" &&\n-\texport GIT_REMOTE_TESTGIT_REFSPEC &&\n+\tGIT_REMOTE_TESTGIT_NOREFSPEC=1 &&\n+\texport GIT_REMOTE_TESTGIT_NOREFSPEC &&\n \ttest_must_fail git push 2>../error) &&\n \ttest_i18ngrep \"remote-helper doesn.t support push; refspec needed\" error\n '\ndiff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit\nindex 752c763eb6..f2b551dfaf 100755\n--- a/t/t5801/git-remote-testgit\n+++ b/t/t5801/git-remote-testgit\n@@ -11,13 +11,10 @@ fi\n url=$2\n \n dir=\"$GIT_DIR/testgit/$alias\"\n-prefix=\"refs/testgit/$alias\"\n \n-default_refspec=\"refs/heads/*:${prefix}/heads/*\"\n+refspec=\"refs/heads/*:refs/testgit/$alias/heads/*\"\n \n-refspec=\"${GIT_REMOTE_TESTGIT_REFSPEC-$default_refspec}\"\n-\n-test -z \"$refspec\" && prefix=\"refs\"\n+test -n \"$GIT_REMOTE_TESTGIT_NOREFSPEC\" && refspec=\"\"\n \n GIT_DIR=\"$url/.git\"\n export GIT_DIR\n@@ -81,10 +78,10 @@ do\n \n \t\techo \"feature done\"\n \t\tgit fast-export \\\n+\t\t\t${refspec:+\"--refspec=$refspec\"} \\\n \t\t\t${testgitmarks:+\"--import-marks=$testgitmarks\"} \\\n \t\t\t${testgitmarks:+\"--export-marks=$testgitmarks\"} \\\n-\t\t\t$refs |\n-\t\tsed -e \"s#refs/heads/#${prefix}/heads/#g\"\n+\t\t\t$refs\n \t\techo \"done\"\n \t\t;;\n \texport)\n-- \n2.21.0\n\n"},{"id":"376636","messageId":"20190604021330.16130-3-felipe.contreras@gmail.com","threadId":"51234","inReplyTo":"20190604021330.16130-1-felipe.contreras@gmail.com","subject":"[RFC/PATCH 2/5] t5801 (remote-helpers): add test to fetch tags","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2019-06-04T02:13:27Z","receivedAt":"2019-06-04T02:13:37Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This used to work, but commit e198b3a740 broke it.\n\n  e198b3a740 (fetch: replace string-list used as a look-up table with a hashmap)\n\nProbably all remote helpers that use the import method are affected, but\nwe didn't catch the issue.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n t/t5801-remote-helpers.sh  | 10 ++++++++++\n t/t5801/git-remote-testgit | 17 ++++++++++++-----\n 2 files changed, 22 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 48bed7c2fe..238774bc17 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -303,4 +303,14 @@ test_expect_success 'fetch url' '\n \tcompare_refs server HEAD local FETCH_HEAD\n '\n \n+test_expect_failure 'fetch tag' '\n+\t(cd server &&\n+\t git tag v1.0\n+\t) &&\n+\t(cd local &&\n+\t git fetch\n+\t) &&\n+\tcompare_refs local v1.0 server v1.0\n+'\n+\n test_done\ndiff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit\nindex f2b551dfaf..6b9f0b5dc7 100755\n--- a/t/t5801/git-remote-testgit\n+++ b/t/t5801/git-remote-testgit\n@@ -12,9 +12,14 @@ url=$2\n \n dir=\"$GIT_DIR/testgit/$alias\"\n \n-refspec=\"refs/heads/*:refs/testgit/$alias/heads/*\"\n+h_refspec=\"refs/heads/*:refs/testgit/$alias/heads/*\"\n+t_refspec=\"refs/tags/*:refs/testgit/$alias/tags/*\"\n \n-test -n \"$GIT_REMOTE_TESTGIT_NOREFSPEC\" && refspec=\"\"\n+if test -n \"$GIT_REMOTE_TESTGIT_NOREFSPEC\"\n+then\n+\th_refspec=\"\"\n+\tt_refspec=\"\"\n+fi\n \n GIT_DIR=\"$url/.git\"\n export GIT_DIR\n@@ -37,7 +42,8 @@ do\n \tcapabilities)\n \t\techo 'import'\n \t\techo 'export'\n-\t\ttest -n \"$refspec\" && echo \"refspec $refspec\"\n+\t\ttest -n \"$h_refspec\" && echo \"refspec $h_refspec\"\n+\t\ttest -n \"$t_refspec\" && echo \"refspec $t_refspec\"\n \t\tif test -n \"$gitmarks\"\n \t\tthen\n \t\t\techo \"*import-marks $gitmarks\"\n@@ -49,7 +55,7 @@ do\n \t\techo\n \t\t;;\n \tlist)\n-\t\tgit for-each-ref --format='? %(refname)' 'refs/heads/'\n+\t\tgit for-each-ref --format='? %(refname)' 'refs/heads/' 'refs/tags/'\n \t\thead=$(git symbolic-ref HEAD)\n \t\techo \"@$head HEAD\"\n \t\techo\n@@ -78,7 +84,8 @@ do\n \n \t\techo \"feature done\"\n \t\tgit fast-export \\\n-\t\t\t${refspec:+\"--refspec=$refspec\"} \\\n+\t\t\t${h_refspec:+\"--refspec=$h_refspec\"} \\\n+\t\t\t${t_refspec:+\"--refspec=$t_refspec\"} \\\n \t\t\t${testgitmarks:+\"--import-marks=$testgitmarks\"} \\\n \t\t\t${testgitmarks:+\"--export-marks=$testgitmarks\"} \\\n \t\t\t$refs\n-- \n2.21.0\n\n"},{"id":"376637","messageId":"20190604021330.16130-4-felipe.contreras@gmail.com","threadId":"51234","inReplyTo":"20190604021330.16130-1-felipe.contreras@gmail.com","subject":"[RFC/PATCH 3/5] fetch: trivial cleanup","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2019-06-04T02:13:28Z","receivedAt":"2019-06-04T02:13:38Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Create a helper function to clear an item. The way items are cleared has\nchanged, and will change again soon.\n\nNo functional changes.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n builtin/fetch.c | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 4ba63d5ac6..8af5e319f1 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -287,6 +287,11 @@ static int refname_hash_exists(struct hashmap *map, const char *refname)\n \treturn !!hashmap_get_from_hash(map, strhash(refname), refname);\n }\n \n+static void clear_item(struct refname_hash_entry *item)\n+{\n+\toidclr(&item->oid);\n+}\n+\n static void find_non_local_tags(const struct ref *refs,\n \t\t\t\tstruct ref **head,\n \t\t\t\tstruct ref ***tail)\n@@ -319,7 +324,7 @@ static void find_non_local_tags(const struct ref *refs,\n \t\t\t    !will_fetch(head, ref->old_oid.hash) &&\n \t\t\t    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&\n \t\t\t    !will_fetch(head, item->oid.hash))\n-\t\t\t\toidclr(&item->oid);\n+\t\t\t\tclear_item(item);\n \t\t\titem = NULL;\n \t\t\tcontinue;\n \t\t}\n@@ -333,7 +338,7 @@ static void find_non_local_tags(const struct ref *refs,\n \t\tif (item &&\n \t\t    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&\n \t\t    !will_fetch(head, item->oid.hash))\n-\t\t\toidclr(&item->oid);\n+\t\t\tclear_item(item);\n \n \t\titem = NULL;\n \n@@ -354,7 +359,7 @@ static void find_non_local_tags(const struct ref *refs,\n \tif (item &&\n \t    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&\n \t    !will_fetch(head, item->oid.hash))\n-\t\toidclr(&item->oid);\n+\t\tclear_item(item);\n \n \t/*\n \t * For all the tags in the remote_refs_list,\n-- \n2.21.0\n\n"},{"id":"376638","messageId":"20190604021330.16130-6-felipe.contreras@gmail.com","threadId":"51234","inReplyTo":"20190604021330.16130-1-felipe.contreras@gmail.com","subject":"[RFC/PATCH 5/5] fetch: fix regression with transport helpers","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2019-06-04T02:13:30Z","receivedAt":"2019-06-04T02:13:41Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Commit e198b3a740 changed the behavior of fetch with regards to tags.\nBefore, null oids where not ignored, now they are, regardless of whether\nthe refs have been explicitly cleared or not.\n\n  e198b3a740 (fetch: replace string-list used as a look-up table with a hashmap)\n\nWhen using a transport helper the oids can certainly be null. So now\ntags are ignored and fetching them is impossible.\n\nThis patch fixes that by having a specific flag that is set only when we\nexplicitly want to ignore the refs, restoring the original behavior.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n builtin/fetch.c           | 5 +++--\n t/t5801-remote-helpers.sh | 2 +-\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 9dc551551e..f2be50a4a3 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -239,6 +239,7 @@ static int will_fetch(struct ref **head, const unsigned char *sha1)\n struct refname_hash_entry {\n \tstruct hashmap_entry ent; /* must be the first member */\n \tstruct object_id oid;\n+\tint ignore;\n \tchar refname[FLEX_ARRAY];\n };\n \n@@ -289,7 +290,7 @@ static int refname_hash_exists(struct hashmap *map, const char *refname)\n \n static void clear_item(struct refname_hash_entry *item)\n {\n-\toidclr(&item->oid);\n+\titem->ignore = 1;\n }\n \n static void find_non_local_tags(const struct ref *refs,\n@@ -374,7 +375,7 @@ static void find_non_local_tags(const struct ref *refs,\n \t\t\tBUG(\"unseen remote ref?\");\n \n \t\t/* Unless we have already decided to ignore this item... */\n-\t\tif (is_null_oid(&item->oid))\n+\t\tif (item->ignore)\n \t\t\tcontinue;\n \n \t\trm = alloc_ref(item->refname);\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 238774bc17..2d6c4a281e 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -303,7 +303,7 @@ test_expect_success 'fetch url' '\n \tcompare_refs server HEAD local FETCH_HEAD\n '\n \n-test_expect_failure 'fetch tag' '\n+test_expect_success 'fetch tag' '\n \t(cd server &&\n \t git tag v1.0\n \t) &&\n-- \n2.21.0\n\n"},{"id":"376639","messageId":"20190604021330.16130-5-felipe.contreras@gmail.com","threadId":"51234","inReplyTo":"20190604021330.16130-1-felipe.contreras@gmail.com","subject":"[RFC/PATCH 4/5] fetch: make the code more understandable","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2019-06-04T02:13:29Z","receivedAt":"2019-06-04T02:13:42Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"The comment makes it seem as if the condition is the other way around.\nThe exception is when the oid is null, so check for that.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n builtin/fetch.c | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 8af5e319f1..9dc551551e 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -367,19 +367,21 @@ static void find_non_local_tags(const struct ref *refs,\n \t */\n \tfor_each_string_list_item(remote_ref_item, &remote_refs_list) {\n \t\tconst char *refname = remote_ref_item->string;\n+\t\tstruct ref *rm;\n \n \t\titem = hashmap_get_from_hash(&remote_refs, strhash(refname), refname);\n \t\tif (!item)\n \t\t\tBUG(\"unseen remote ref?\");\n \n \t\t/* Unless we have already decided to ignore this item... */\n-\t\tif (!is_null_oid(&item->oid)) {\n-\t\t\tstruct ref *rm = alloc_ref(item->refname);\n-\t\t\trm->peer_ref = alloc_ref(item->refname);\n-\t\t\toidcpy(&rm->old_oid, &item->oid);\n-\t\t\t**tail = rm;\n-\t\t\t*tail = &rm->next;\n-\t\t}\n+\t\tif (is_null_oid(&item->oid))\n+\t\t\tcontinue;\n+\n+\t\trm = alloc_ref(item->refname);\n+\t\trm->peer_ref = alloc_ref(item->refname);\n+\t\toidcpy(&rm->old_oid, &item->oid);\n+\t\t**tail = rm;\n+\t\t*tail = &rm->next;\n \t}\n \thashmap_free(&remote_refs, 1);\n \tstring_list_clear(&remote_refs_list, 0);\n-- \n2.21.0\n\n"},{"id":"376660","messageId":"20190604141619.GA10598@sigill.intra.peff.net","threadId":"51234","inReplyTo":"20190604021330.16130-2-felipe.contreras@gmail.com","subject":"Re: [RFC/PATCH 1/5] t5801 (remote-helpers): cleanup refspec stuff","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-04T14:16:19Z","receivedAt":"2019-06-04T14:16:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 03, 2019 at 09:13:26PM -0500, Felipe Contreras wrote:\n\n> diff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit\n> index 752c763eb6..f2b551dfaf 100755\n> --- a/t/t5801/git-remote-testgit\n> +++ b/t/t5801/git-remote-testgit\n> @@ -11,13 +11,10 @@ fi\n>  url=$2\n>  \n>  dir=\"$GIT_DIR/testgit/$alias\"\n> -prefix=\"refs/testgit/$alias\"\n>  \n> -default_refspec=\"refs/heads/*:${prefix}/heads/*\"\n> +refspec=\"refs/heads/*:refs/testgit/$alias/heads/*\"\n>  \n> -refspec=\"${GIT_REMOTE_TESTGIT_REFSPEC-$default_refspec}\"\n> -\n> -test -z \"$refspec\" && prefix=\"refs\"\n> +test -n \"$GIT_REMOTE_TESTGIT_NOREFSPEC\" && refspec=\"\"\n\nSo this simplifies the feature by just allowing two refspecs: the\ndefault one, or none at all. And that works because all of the existing\ncallers wanted or the other. Makes sense.\n\n> @@ -81,10 +78,10 @@ do\n>  \n>  \t\techo \"feature done\"\n>  \t\tgit fast-export \\\n> +\t\t\t${refspec:+\"--refspec=$refspec\"} \\\n>  \t\t\t${testgitmarks:+\"--import-marks=$testgitmarks\"} \\\n>  \t\t\t${testgitmarks:+\"--export-marks=$testgitmarks\"} \\\n> -\t\t\t$refs |\n> -\t\tsed -e \"s#refs/heads/#${prefix}/heads/#g\"\n> +\t\t\t$refs\n\nThis second hunk puzzled me for a minute. By using --refspec, we can\navoid doing the mapping with sed here, which is simpler and more robust.\nGood.\n\nOne caveat for anybody else testing this: when I initially applied this\npatch, some tests failed! The problem turned out to be a leftover\ngit-remote-testgit in my build dir from prior to 5afb2ce4cd\n(remote-testgit: move it into the support directory for t5801,\n2019-04-12).\n\nSo the problem isn't related to this patch (it's only noticeable here\nbecause this is the first change to remote-testgit since it moved). I\nwonder if we could make the test script more robust here. I think it's\ntricky because the build dir is added to the $PATH internally by Git\nitself (since it's the exec-path), so nothing do in the test script can\noverride that. Perhaps it's worth just renaming the script as part of\nthe move. +cc Dscho\n\n-Peff\n"},{"id":"376661","messageId":"20190604142233.GB10598@sigill.intra.peff.net","threadId":"51234","inReplyTo":"20190604021330.16130-3-felipe.contreras@gmail.com","subject":"Re: [RFC/PATCH 2/5] t5801 (remote-helpers): add test to fetch tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-04T14:22:33Z","receivedAt":"2019-06-04T14:22:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 03, 2019 at 09:13:27PM -0500, Felipe Contreras wrote:\n\n> This used to work, but commit e198b3a740 broke it.\n> \n>   e198b3a740 (fetch: replace string-list used as a look-up table with a hashmap)\n> \n> Probably all remote helpers that use the import method are affected, but\n> we didn't catch the issue.\n\nThanks for beefing up the tests. It's rather unfortunate that we missed\nsuch a severe regression.\n\n> +test_expect_failure 'fetch tag' '\n> +\t(cd server &&\n> +\t git tag v1.0\n> +\t) &&\n> +\t(cd local &&\n> +\t git fetch\n> +\t) &&\n> +\tcompare_refs local v1.0 server v1.0\n> +'\n\nThis single-commands might be more readable using \"git -C\", but this\nmatches the existing style in the test script, so I'm OK with it either\nway.\n\n> diff --git a/t/t5801/git-remote-testgit b/t/t5801/git-remote-testgit\n> index f2b551dfaf..6b9f0b5dc7 100755\n> --- a/t/t5801/git-remote-testgit\n> +++ b/t/t5801/git-remote-testgit\n> @@ -12,9 +12,14 @@ url=$2\n>  \n>  dir=\"$GIT_DIR/testgit/$alias\"\n>  \n> -refspec=\"refs/heads/*:refs/testgit/$alias/heads/*\"\n> +h_refspec=\"refs/heads/*:refs/testgit/$alias/heads/*\"\n> +t_refspec=\"refs/tags/*:refs/testgit/$alias/tags/*\"\n>  \n> -test -n \"$GIT_REMOTE_TESTGIT_NOREFSPEC\" && refspec=\"\"\n> +if test -n \"$GIT_REMOTE_TESTGIT_NOREFSPEC\"\n> +then\n> +\th_refspec=\"\"\n> +\tt_refspec=\"\"\n> +fi\n\nAnd the simplification from the prior step pays off. Looks good.\n\nThe rest of it seems pretty sensible (modulo the caveat that I don't know\nremote-testgit well enough to detect anything subtle).\n\n-Peff\n"},{"id":"376662","messageId":"20190604143240.GC10598@sigill.intra.peff.net","threadId":"51234","inReplyTo":"20190604021330.16130-6-felipe.contreras@gmail.com","subject":"Re: [RFC/PATCH 5/5] fetch: fix regression with transport helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-04T14:32:40Z","receivedAt":"2019-06-04T14:32:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 03, 2019 at 09:13:30PM -0500, Felipe Contreras wrote:\n\n> Commit e198b3a740 changed the behavior of fetch with regards to tags.\n> Before, null oids where not ignored, now they are, regardless of whether\n> the refs have been explicitly cleared or not.\n> \n>   e198b3a740 (fetch: replace string-list used as a look-up table with a hashmap)\n> \n> When using a transport helper the oids can certainly be null. So now\n> tags are ignored and fetching them is impossible.\n> \n> This patch fixes that by having a specific flag that is set only when we\n> explicitly want to ignore the refs, restoring the original behavior.\n\nMakes sense. Prior to e198b3a740, we indicated \"ignored\" by setting the\noid (in item->util) to NULL. But since it's no longer a pointer after\nthat commit, we can't do so.\n\nSo the obvious alternative to this is going back to having it as a\npointer. I think I prefer your solution, though, since it keeps the\nmemory management a bit simpler, and more clearly expresses the intent.\n\nIf we add any new code that has to similarly ignore an item in the hash,\nit will have to remember to use clear_item() rather than oidclr(). But I\ndon't think that's a big risk.\n\n>  builtin/fetch.c           | 5 +++--\n>  t/t5801-remote-helpers.sh | 2 +-\n>  2 files changed, 4 insertions(+), 3 deletions(-)\n\nThe code change itself looks obviously correct.\n\n-Peff\n"},{"id":"376664","messageId":"20190604143553.GD10598@sigill.intra.peff.net","threadId":"51234","inReplyTo":"20190604021330.16130-1-felipe.contreras@gmail.com","subject":"Re: [RFC/PATCH 0/5] Fix fetch regression with transport helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-04T14:35:53Z","receivedAt":"2019-06-04T14:35:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 03, 2019 at 09:13:25PM -0500, Felipe Contreras wrote:\n\n> I noticed a regression with running tests for git-remote-hg; can't seem to be\n> able to fetch tags.\n> \n> Probably all remote helpers that use the import method are affected, if not\n> all.\n> \n> The following patches are meant to test for the issue, fix it, and get some\n> cleanups.\n> \n> I'm not exactly sure the solution is the one we want, but hopefull it gives an\n> idea as to what is needed.\n\nIt looks good to me. Thanks for fixing this.\n\nThe breakage is in v2.20, so I don't think this needs to be rushed into\nv2.22 (which is already at -rc3). But it should probably go to 'maint'\nsooner rather than later.\n\n-Peff\n"},{"id":"376684","messageId":"CAMP44s3-8uaTn-1fUKqGLX8weMaK4yjvfdxcw8TF3JZfuKYGUQ@mail.gmail.com","threadId":"51234","inReplyTo":"20190604143553.GD10598@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 0/5] Fix fetch regression with transport helpers","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2019-06-04T19:15:32Z","receivedAt":"2019-06-04T19:15:48Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Tue, Jun 4, 2019 at 9:35 AM Jeff King <peff@peff.net> wrote:\n>\n> On Mon, Jun 03, 2019 at 09:13:25PM -0500, Felipe Contreras wrote:\n\n> > I'm not exactly sure the solution is the one we want, but hopefull it gives an\n> > idea as to what is needed.\n>\n> It looks good to me. Thanks for fixing this.\n>\n> The breakage is in v2.20, so I don't think this needs to be rushed into\n> v2.22 (which is already at -rc3). But it should probably go to 'maint'\n> sooner rather than later.\n\nIndeed, it's probably not so urgent since people have not even been\ncomplaining (that I know of). However, I think it's clear that there's\na regression and the fix is simple, so maybe just apply the obvious\nfix, and leave the rest of the patches for later?\n\nI'm attaching only the fix, just to temp the idea of applying that\ntiny thing into v2.2.\n\n-- \nFelipe Contreras\n\n\nFrom a59903e9828a4f265647014ca87fa3be9016a225 Mon Sep 17 00:00:00 2001\nFrom: Felipe Contreras <felipe.contreras@gmail.com>\nDate: Tue, 4 Jun 2019 14:10:43 -0500\nSubject: [PATCH] fetch: fix regression with transport helpers\n\nCommit e198b3a740 changed the behavior of fetch with regards to tags.\nBefore, null oids where not ignored, now they are, regardless of whether\nthe refs have been explicitly cleared or not.\n\n  e198b3a740 (fetch: replace string-list used as a look-up table with a hashmap)\n\nWhen using a transport helper the oids can certainly be null. So now\ntags are ignored and fetching them is impossible.\n\nThis patch fixes that by having a specific flag that is set only when we\nexplicitly want to ignore the refs, restoring the original behavior.\n\nSigned-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n---\n builtin/fetch.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 4ba63d5ac6..f4962b93d6 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -239,6 +239,7 @@ static int will_fetch(struct ref **head, const unsigned char *sha1)\n struct refname_hash_entry {\n \tstruct hashmap_entry ent; /* must be the first member */\n \tstruct object_id oid;\n+\tint ignore;\n \tchar refname[FLEX_ARRAY];\n };\n \n@@ -319,7 +320,7 @@ static void find_non_local_tags(const struct ref *refs,\n \t\t\t    !will_fetch(head, ref->old_oid.hash) &&\n \t\t\t    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&\n \t\t\t    !will_fetch(head, item->oid.hash))\n-\t\t\t\toidclr(&item->oid);\n+\t\t\t\titem->ignore = 1;\n \t\t\titem = NULL;\n \t\t\tcontinue;\n \t\t}\n@@ -333,7 +334,7 @@ static void find_non_local_tags(const struct ref *refs,\n \t\tif (item &&\n \t\t    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&\n \t\t    !will_fetch(head, item->oid.hash))\n-\t\t\toidclr(&item->oid);\n+\t\t\titem->ignore = 1;\n \n \t\titem = NULL;\n \n@@ -354,7 +355,7 @@ static void find_non_local_tags(const struct ref *refs,\n \tif (item &&\n \t    !has_object_file_with_flags(&item->oid, OBJECT_INFO_QUICK) &&\n \t    !will_fetch(head, item->oid.hash))\n-\t\toidclr(&item->oid);\n+\t\titem->ignore = 1;\n \n \t/*\n \t * For all the tags in the remote_refs_list,\n@@ -368,7 +369,7 @@ static void find_non_local_tags(const struct ref *refs,\n \t\t\tBUG(\"unseen remote ref?\");\n \n \t\t/* Unless we have already decided to ignore this item... */\n-\t\tif (!is_null_oid(&item->oid)) {\n+\t\tif (!item->ignore) {\n \t\t\tstruct ref *rm = alloc_ref(item->refname);\n \t\t\trm->peer_ref = alloc_ref(item->refname);\n \t\t\toidcpy(&rm->old_oid, &item->oid);\n-- \n2.21.0\n\n"},{"id":"376685","messageId":"xmqqd0jttb1b.fsf@gitster-ct.c.googlers.com","threadId":"51234","inReplyTo":"20190604021330.16130-6-felipe.contreras@gmail.com","subject":"Re: [RFC/PATCH 5/5] fetch: fix regression with transport helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-04T19:17:52Z","receivedAt":"2019-06-04T19:18:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras <felipe.contreras@gmail.com> writes:\n\n> Commit e198b3a740 changed the behavior of fetch with regards to tags.\n> Before, null oids where not ignored, now they are, regardless of whether\n> the refs have been explicitly cleared or not.\n>\n>   e198b3a740 (fetch: replace string-list used as a look-up table with a hashmap)\n>\n> When using a transport helper the oids can certainly be null. So now\n> tags are ignored and fetching them is impossible.\n>\n> This patch fixes that by having a specific flag that is set only when we\n> explicitly want to ignore the refs, restoring the original behavior.\n>\n> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>\n> ---\n>  builtin/fetch.c           | 5 +++--\n>  t/t5801-remote-helpers.sh | 2 +-\n>  2 files changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 9dc551551e..f2be50a4a3 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -239,6 +239,7 @@ static int will_fetch(struct ref **head, const unsigned char *sha1)\n>  struct refname_hash_entry {\n>  \tstruct hashmap_entry ent; /* must be the first member */\n>  \tstruct object_id oid;\n> +\tint ignore;\n>  \tchar refname[FLEX_ARRAY];\n>  };\n>  \n> @@ -289,7 +290,7 @@ static int refname_hash_exists(struct hashmap *map, const char *refname)\n>  \n>  static void clear_item(struct refname_hash_entry *item)\n>  {\n> -\toidclr(&item->oid);\n> +\titem->ignore = 1;\n>  }\n>  \n>  static void find_non_local_tags(const struct ref *refs,\n> @@ -374,7 +375,7 @@ static void find_non_local_tags(const struct ref *refs,\n>  \t\t\tBUG(\"unseen remote ref?\");\n>  \n>  \t\t/* Unless we have already decided to ignore this item... */\n> -\t\tif (is_null_oid(&item->oid))\n> +\t\tif (item->ignore)\n>  \t\t\tcontinue;\n\nYeah, we should have added a bit like this to preserve the\ndistinction between a NULL pointer for \"struct object_id\" and oid\nthat has 0{40} in the old code, which was lost in the conversion.\n\nThanks for spotting and fixing.\n"},{"id":"376706","messageId":"nycvar.QRO.7.76.6.1906051005060.1775@tvgsbejvaqbjf.bet","threadId":"51234","inReplyTo":"20190604021330.16130-1-felipe.contreras@gmail.com","subject":"Re: [RFC/PATCH 0/5] Fix fetch regression with transport helpers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-05T08:12:12Z","receivedAt":"2019-06-05T08:12:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"On Mon, 3 Jun 2019, Felipe Contreras wrote:\n\n> Felipe Contreras (5):\n>   t5801 (remote-helpers): cleanup refspec stuff\n>   t5801 (remote-helpers): add test to fetch tags\n>   fetch: trivial cleanup\n>   fetch: make the code more understandable\n>   fetch: fix regression with transport helpers\n>\n>  builtin/fetch.c            | 28 ++++++++++++++++++----------\n>  t/t5801-remote-helpers.sh  | 18 ++++++++++++++----\n>  t/t5801/git-remote-testgit | 22 +++++++++++++---------\n>  3 files changed, 45 insertions(+), 23 deletions(-)\n\nThis fails on macOS, in t5601, both in our osx-clang and osx-gcc jobs, as\nwell as in the StaticAnalysis job. For details, see\nhttps://dev.azure.com/gitgitgadget/git/_build/results?buildId=10206\n\nThe t5601 failure looks like this:\n\n-- snip --\nchecking prerequisite: CASE_INSENSITIVE_FS\n\nmkdir -p \"$TRASH_DIRECTORY/prereq-test-dir\" &&\n(\n\tcd \"$TRASH_DIRECTORY/prereq-test-dir\" &&\n\techo good >CamelCase &&\n\techo bad >camelcase &&\n\ttest \"$(cat CamelCase)\" != good\n\n)\n++ mkdir -p '/Users/vsts/agent/2.150.3/work/1/s/t/trash\ndirectory.t5601-clone/prereq-test-dir'\n++ cd '/Users/vsts/agent/2.150.3/work/1/s/t/trash\ndirectory.t5601-clone/prereq-test-dir'\n++ echo good\n++ echo bad\n+++ cat CamelCase\n++ test bad '!=' good\nprerequisite CASE_INSENSITIVE_FS ok\nexpecting success:\n\tgrep X icasefs/warning &&\n\tgrep x icasefs/warning &&\n\ttest_i18ngrep \"the following paths have collided\" icasefs/warning\n\n++ grep X icasefs/warning\nerror: last command exited with $?=1\nnot ok 99 - colliding file detection\n-- snap --\n\nMy guess is that your changes remove something that was expected before,\nand is still expected, and that this was only tested on Linux, and only on\na file system with case-sensitive file names.\n\nThe StaticAnalysis job (which is admittedly a misnomer) points out another\nfew valid issues, but that is probably because Junio applied this patch\nseries on top of a very old commit. I, at least, could not spot any file\nin the Coccinelle report that was touched by this here patch series.\n\nCiao,\nJohannes\n"},{"id":"376709","messageId":"20190605112713.GA14027@sigill.intra.peff.net","threadId":"51234","inReplyTo":"nycvar.QRO.7.76.6.1906051005060.1775@tvgsbejvaqbjf.bet","subject":"Re: [RFC/PATCH 0/5] Fix fetch regression with transport helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-05T11:27:14Z","receivedAt":"2019-06-05T11:27:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 05, 2019 at 10:12:12AM +0200, Johannes Schindelin wrote:\n\n> This fails on macOS, in t5601, both in our osx-clang and osx-gcc jobs, as\n> well as in the StaticAnalysis job. For details, see\n> https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10206\n\nHmm. I'm having a hard time seeing why (and I can't seem to reproduce it\nlocally on a case-insensitive HFS+ filesystem under Linux).\n\nIn particular, if the problem is here:\n\n> expecting success:\n> \tgrep X icasefs/warning &&\n> \tgrep x icasefs/warning &&\n> \ttest_i18ngrep \"the following paths have collided\" icasefs/warning\n> \n> ++ grep X icasefs/warning\n> error: last command exited with $?=1\n> not ok 99 - colliding file detection\n\nthen that implies it has to do with the checkout phase, which Felipe's\npatch doesn't touch. Later in the log we see the actual file contents\n(I'm confused as to how this gets here; it looks like debugging bits\nthat were added after the main script?):\n\n  2019-06-05T07:58:37.7961890Z Cloning into 'bogus'...\n  2019-06-05T07:58:37.7962430Z done.\n  2019-06-05T07:58:37.7963360Z warning: the following paths have collided (e.g. case-sensitive paths\n  2019-06-05T07:58:37.7964300Z on a case-insensitive filesystem) and only one from the same\n  2019-06-05T07:58:37.7964880Z colliding group is in the working tree:\n  2019-06-05T07:58:37.7965290Z \n  2019-06-05T07:58:37.7966250Z   'x'\n\nwhereas a succeeding test expects us to mention both 'x' and 'X'.\n\nSo we _did_ find the collision, but somehow 'X' was not reported.\nLooking at the code, I'm not even sure how that could happen. Given that\nthis process does involve looking at stat data, it makes me wonder if\nthere could be some raciness involved. But again, I'm scratching my head\nas to how exactly, and I couldn't reproduce it under load or with some\ncarefully inserted sleep() calls.\n\nAnd it looks like it did reproduce twice on Azure.\n\nCan somebody who has osx locally reproduce this? Or is there a way to\ninteractively access the Azure environment to dig further?\n\n> My guess is that your changes remove something that was expected before,\n> and is still expected, and that this was only tested on Linux, and only on\n> a file system with case-sensitive file names.\n\nIt sounds like you're suggesting that changes to the test script subtly\naffected the later state. Which is indeed a common culprit. But the\nchanges in Felipe's series were all to t5801, not the failing t5601. Am\nI misunderstanding what you mean?\n\n-Peff\n"},{"id":"376711","messageId":"CACsJy8DKWoerME5BykVmihyX2eX10YTO0BNyVc7MGjwp_Shg2Q@mail.gmail.com","threadId":"51234","inReplyTo":"20190605112713.GA14027@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 0/5] Fix fetch regression with transport helpers","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-06-05T12:22:11Z","receivedAt":"2019-06-05T12:22:40Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jun 5, 2019 at 6:27 PM Jeff King <peff@peff.net> wrote:\n>\n> On Wed, Jun 05, 2019 at 10:12:12AM +0200, Johannes Schindelin wrote:\n>\n> > This fails on macOS, in t5601, both in our osx-clang and osx-gcc jobs, as\n> > well as in the StaticAnalysis job. For details, see\n> > https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10206\n>\n> Hmm. I'm having a hard time seeing why (and I can't seem to reproduce it\n> locally on a case-insensitive HFS+ filesystem under Linux).\n>\n> In particular, if the problem is here:\n>\n> > expecting success:\n> >       grep X icasefs/warning &&\n> >       grep x icasefs/warning &&\n> >       test_i18ngrep \"the following paths have collided\" icasefs/warning\n> >\n> > ++ grep X icasefs/warning\n> > error: last command exited with $?=1\n> > not ok 99 - colliding file detection\n>\n> then that implies it has to do with the checkout phase, which Felipe's\n> patch doesn't touch. Later in the log we see the actual file contents\n> (I'm confused as to how this gets here; it looks like debugging bits\n> that were added after the main script?):\n>\n>   2019-06-05T07:58:37.7961890Z Cloning into 'bogus'...\n>   2019-06-05T07:58:37.7962430Z done.\n>   2019-06-05T07:58:37.7963360Z warning: the following paths have collided (e.g. case-sensitive paths\n>   2019-06-05T07:58:37.7964300Z on a case-insensitive filesystem) and only one from the same\n>   2019-06-05T07:58:37.7964880Z colliding group is in the working tree:\n>   2019-06-05T07:58:37.7965290Z\n>   2019-06-05T07:58:37.7966250Z   'x'\n>\n> whereas a succeeding test expects us to mention both 'x' and 'X'.\n>\n> So we _did_ find the collision, but somehow 'X' was not reported.\n> Looking at the code, I'm not even sure how that could happen. Given that\n> this process does involve looking at stat data, it makes me wonder if\n\nIt does use stat data in mark_colliding_entries() if core.checkStat is\nfalse. I think on MacOS it's actually true.\n\nI vaguely recall seeing just one 'x' once. I think last time I had a\nproblem with truncating st_ino, but that should be fixed in e66ceca94b\n(clone: fix colliding file detection on APFS, 2018-11-20). So no idea\nhow this happens again.\n\n> there could be some raciness involved. But again, I'm scratching my head\n> as to how exactly, and I couldn't reproduce it under load or with some\n> carefully inserted sleep() calls.\n-- \nDuy\n"},{"id":"376749","messageId":"nycvar.QRO.7.76.6.1906061505310.42@tvgsbejvaqbjf.bet","threadId":"51234","inReplyTo":"CACsJy8DKWoerME5BykVmihyX2eX10YTO0BNyVc7MGjwp_Shg2Q@mail.gmail.com","subject":"Re: [RFC/PATCH 0/5] Fix fetch regression with transport helpers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-06T13:07:02Z","receivedAt":"2019-06-06T13:07:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Duy,\n\nOn Wed, 5 Jun 2019, Duy Nguyen wrote:\n\n> On Wed, Jun 5, 2019 at 6:27 PM Jeff King <peff@peff.net> wrote:\n> >\n> > On Wed, Jun 05, 2019 at 10:12:12AM +0200, Johannes Schindelin wrote:\n> >\n> > > This fails on macOS, in t5601, both in our osx-clang and osx-gcc jobs, as\n> > > well as in the StaticAnalysis job. For details, see\n> > > https://dev.azure.com/gitgitgadget/git/_build/results?buildId=10206\n> >\n> > Hmm. I'm having a hard time seeing why (and I can't seem to reproduce it\n> > locally on a case-insensitive HFS+ filesystem under Linux).\n> >\n> > In particular, if the problem is here:\n> >\n> > > expecting success:\n> > >       grep X icasefs/warning &&\n> > >       grep x icasefs/warning &&\n> > >       test_i18ngrep \"the following paths have collided\" icasefs/warning\n> > >\n> > > ++ grep X icasefs/warning\n> > > error: last command exited with $?=1\n> > > not ok 99 - colliding file detection\n> >\n> > then that implies it has to do with the checkout phase, which Felipe's\n> > patch doesn't touch. Later in the log we see the actual file contents\n> > (I'm confused as to how this gets here; it looks like debugging bits\n> > that were added after the main script?):\n> >\n> >   2019-06-05T07:58:37.7961890Z Cloning into 'bogus'...\n> >   2019-06-05T07:58:37.7962430Z done.\n> >   2019-06-05T07:58:37.7963360Z warning: the following paths have collided (e.g. case-sensitive paths\n> >   2019-06-05T07:58:37.7964300Z on a case-insensitive filesystem) and only one from the same\n> >   2019-06-05T07:58:37.7964880Z colliding group is in the working tree:\n> >   2019-06-05T07:58:37.7965290Z\n> >   2019-06-05T07:58:37.7966250Z   'x'\n> >\n> > whereas a succeeding test expects us to mention both 'x' and 'X'.\n> >\n> > So we _did_ find the collision, but somehow 'X' was not reported.\n> > Looking at the code, I'm not even sure how that could happen. Given that\n> > this process does involve looking at stat data, it makes me wonder if\n>\n> It does use stat data in mark_colliding_entries() if core.checkStat is\n> false. I think on MacOS it's actually true.\n>\n> I vaguely recall seeing just one 'x' once. I think last time I had a\n> problem with truncating st_ino, but that should be fixed in e66ceca94b\n> (clone: fix colliding file detection on APFS, 2018-11-20). So no idea\n> how this happens again.\n\nGood catch. I think the reason it happens again is simply that Junio\npicked a base commit that is older than the commit you referenced.\n\nPoint in favor: Junio merged these here patches into `pu` and those\ntest failures (as well as the StaticAnalysis issues) are gone.\n\nThanks,\nJohannes\n\n>\n> > there could be some raciness involved. But again, I'm scratching my head\n> > as to how exactly, and I couldn't reproduce it under load or with some\n> > carefully inserted sleep() calls.\n> --\n> Duy\n>\n"},{"id":"376769","messageId":"xmqqr286r8ts.fsf@gitster-ct.c.googlers.com","threadId":"51234","inReplyTo":"nycvar.QRO.7.76.6.1906061505310.42@tvgsbejvaqbjf.bet","subject":"Re: [RFC/PATCH 0/5] Fix fetch regression with transport helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-06T16:13:03Z","receivedAt":"2019-06-06T16:13:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> I vaguely recall seeing just one 'x' once. I think last time I had a\n>> problem with truncating st_ino, but that should be fixed in e66ceca94b\n>> (clone: fix colliding file detection on APFS, 2018-11-20). So no idea\n>> how this happens again.\n>\n> Good catch. I think the reason it happens again is simply that Junio\n> picked a base commit that is older than the commit you referenced.\n\nYeah, that is because the patch specifically targetted a single\ncommit as culprit, so naturally the topic that introduced that\ncommit was the place to be \"fixed\" ;-)\n\nI was wondering if the base commit _before_ the fixes, i.e. e198b3a7\n(\"fetch: replace string-list used as a look-up table with a\nhashmap\", 2018-09-25), failed the same test you saw problems with.\nIt does predate e66ceca9 (\"clone: fix colliding file detection on\nAPFS\", 2018-11-20), so my current theory is that it was broken\nalready adn these patches that fixed a breakge had nothing to do\nwith the t5601 tests failing.\n\n"},{"id":"376810","messageId":"nycvar.QRO.7.76.6.1906070722300.789@QRFXGBC-DHN364S.ybpnyqbznva","threadId":"51234","inReplyTo":"xmqqr286r8ts.fsf@gitster-ct.c.googlers.com","subject":"CI builds on GitGitGadget, was Re: [RFC/PATCH 0/5] Fix fetch regression with transport helpers","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-06-07T05:32:36Z","receivedAt":"2019-06-07T05:32:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 6 Jun 2019, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n>\n> >> I vaguely recall seeing just one 'x' once. I think last time I had a\n> >> problem with truncating st_ino, but that should be fixed in e66ceca94b\n> >> (clone: fix colliding file detection on APFS, 2018-11-20). So no idea\n> >> how this happens again.\n> >\n> > Good catch. I think the reason it happens again is simply that Junio\n> > picked a base commit that is older than the commit you referenced.\n>\n> Yeah, that is because the patch specifically targetted a single\n> commit as culprit, so naturally the topic that introduced that\n> commit was the place to be \"fixed\" ;-)\n\nYes, that matches my impression.\n\nI should maybe describe a bit better what *is* tested on GitGitGadget when\nit runs the build & test suite for the individual branches (as opposed to\nthe integration branches maint, master, next & pu): the Azure Pipeline\nobviously *cannot* be defined in `azure-pipelines.yml`, as many of those\nbranches do not even have that file.\n\nOne of the features I like most about Azure Pipelines (yes, I really like\nPipelines, they save me from so much work by enabling me to automate a\n*lot* in Git for Windows' maintenance, such as building and packaging\nquite a few of the components such as OpenSSH, cURL, etc... but I digress)\nis that it offers *both* to define the builds via a file that is committed\n*and also* in a definition that is maintained outside of the source code.\n\nSo what I did was to port azure-pipelines.yml from the former to the\nlatter, and *that* is run on those individual branches.\n\nAs we noticed here, this opens the door for running into regressions that\nhave been long fixed, just not in the tested branch.\n\nSide note: many projects that want to rely on the confidence instilled by\nautomated testing therefore change their workflow to a more \"topic\nbranches are based on master, or on the release branches' tips\" one. I am\nnot saying that you, Junio, should switch to such a workflow because you\nare clearly comfortable with the current one. I mention this mainly for\nthe benefit of readers who might wonder what options they have in their\nown projects to deal with this.\n\nWhat I usually do is a hack: this \"manual\" job definition tries to\ncherry-pick all kinds of known fixes to known regressions, and this\nAPFS-name-collision one is just not one of them. When I find the time\n(hopefully next week, probably not), I shall try to take care of that.\n\n> I was wondering if the base commit _before_ the fixes, i.e. e198b3a7\n> (\"fetch: replace string-list used as a look-up table with a\n> hashmap\", 2018-09-25), failed the same test you saw problems with.\n> It does predate e66ceca9 (\"clone: fix colliding file detection on\n> APFS\", 2018-11-20), so my current theory is that it was broken\n> already adn these patches that fixed a breakge had nothing to do\n> with the t5601 tests failing.\n\nYes, I agree.\n\nOne might ask why I even bother with testing the individual branches. The\nreason is that bisecting breakages in `pu` is a *nightmare*. Most of those\nbreakages are present already in the individual branches, though, so if I\ncan catch the breakages there, I have a much easier time keeping the CI\nbuilds green.\n\nCiao,\nDscho\n"}]}