{"thread":{"id":"33529","subject":"[PATCH 0/6] transport-helper: some clarifications and a fix","startedAt":"2013-04-18T00:05:18Z","lastAt":"2013-04-18T18:56:47Z","messageCount":14,"participants":["Felipe Contreras","Sverre Rabbelier","Thomas Rast","Stefano Lattarini"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"214655","messageId":"1366243524-18202-1-git-send-email-felipe.contreras@gmail.com","threadId":"33529","inReplyTo":null,"subject":"[PATCH 0/6] transport-helper: some clarifications and a fix","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T00:05:18Z","receivedAt":"2013-04-18T00:05:18Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nIt seems the workings of transport-helper are anything but clear, so let's try\nto clarify them a bit, and after that, hopefully it would become clearer why\nthe last patch is actually a good fix.\n\nFelipe Contreras (6):\n transport-helper: clarify *:* refspec\n transport-helper: update refspec documentation\n transport-helper: clarify pushing without refspecs\n transport-helper: warn when refspec is not used\n transport-helper: trivial code shuffle\n transport-helper: update remote helper namespace\n\n Documentation/gitremote-helpers.txt | 12 ++++++------\n t/t5801-remote-helpers.sh | 39 ++++++++++++++++++-------------------\n transport-helper.c | 37 +++++++++++++++++++++++------------\n 3 files changed, 50 insertions(+), 38 deletions(-)\n\n-- \n1.8.2.1.679.g509521a\n"},{"id":"214656","messageId":"1366243524-18202-2-git-send-email-felipe.contreras@gmail.com","threadId":"33529","inReplyTo":"1366243524-18202-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 1/6] transport-helper: clarify *:* refspec","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T00:05:19Z","receivedAt":"2013-04-18T00:05:19Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"The *:* refspec doesn't work, and never has, clarify the code and\ndocumentation to reflect that. This in effect reverts commit 9e7673e\n(gitremote-helpers(1): clarify refspec behaviour).\n\nSigned-off-by: Felipe Contreras \n---\n Documentation/gitremote-helpers.txt | 4 ++--\n t/t5801-remote-helpers.sh | 15 ---------------\n transport-helper.c | 2 +-\n 3 files changed, 3 insertions(+), 18 deletions(-)\n\ndiff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\nindex f506031..0c91aba 100644\n--- a/Documentation/gitremote-helpers.txt\n+++ b/Documentation/gitremote-helpers.txt\n@@ -174,8 +174,8 @@ ref.\n This capability can be advertised multiple times. The first\n applicable refspec takes precedence. The left-hand of refspecs\n advertised with this capability must cover all refs reported by\n-the list command. If a helper does not need a specific 'refspec'\n-capability then it should advertise `refspec *:*`.\n+the list command. If no 'refspec' capability is advertised,\n+there is an implied `refspec *:*`.\n \n 'bidi-import'::\n \tThis modifies the 'import' capability.\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex f387027..cd1873c 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -120,21 +120,6 @@ test_expect_failure 'pushing without refspecs' '\n \tcompare_refs local2 HEAD server HEAD\n '\n \n-test_expect_success 'pulling with straight refspec' '\n-\t(cd local2 &&\n-\tGIT_REMOTE_TESTGIT_REFSPEC=\"*:*\" git pull) &&\n-\tcompare_refs local2 HEAD server HEAD\n-'\n-\n-test_expect_failure 'pushing with straight refspec' '\n-\ttest_when_finished \"(cd local2 && git reset --hard origin)\" &&\n-\t(cd local2 &&\n-\techo content >>file &&\n-\tgit commit -a -m eleven &&\n-\tGIT_REMOTE_TESTGIT_REFSPEC=\"*:*\" git push) &&\n-\tcompare_refs local2 HEAD server HEAD\n-'\n-\n test_expect_success 'pulling without marks' '\n \t(cd local2 &&\n \tGIT_REMOTE_TESTGIT_NO_MARKS=1 git pull) &&\ndiff --git a/transport-helper.c b/transport-helper.c\nindex dcd8d97..cea787c 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -469,7 +469,7 @@ static int fetch_with_import(struct transport *transport,\n \t * were fetching.\n \t *\n \t * (If no \"refspec\" capability was specified, for historical\n-\t * reasons we default to *:*.)\n+\t * reasons we default to the equivalent of *:*.)\n \t *\n \t * Store the result in to_fetch[i].old_sha1. Callers such\n \t * as \"git fetch\" can use the value to write feedback to the\n-- \n1.8.2.1.679.g509521a\n"},{"id":"214657","messageId":"1366243524-18202-3-git-send-email-felipe.contreras@gmail.com","threadId":"33529","inReplyTo":"1366243524-18202-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 2/6] transport-helper: update refspec documentation","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T00:05:20Z","receivedAt":"2013-04-18T00:05:20Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"The refspec capability is not only used by 'import', also by 'export',\nand it's recommend in both.\n\nSigned-off-by: Felipe Contreras \n---\n Documentation/gitremote-helpers.txt | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\nindex 0c91aba..ba7240c 100644\n--- a/Documentation/gitremote-helpers.txt\n+++ b/Documentation/gitremote-helpers.txt\n@@ -159,11 +159,11 @@ Miscellaneous capabilities\n \tcarried out.\n \n 'refspec' ::\n-\tThis modifies the 'import' capability, allowing the produced\n-\tfast-import stream to modify refs in a private namespace\n-\tinstead of writing to refs/heads or refs/remotes directly.\n-\tIt is recommended that all importers providing the 'import'\n-\tcapability use this.\n+\tFor remote helpers that implement 'import' or 'export', this capability\n+\tallows the refs to be constrained to a private namespace, instead of\n+\twriting to refs/heads or refs/remotes directly.\n+\tIt is recommended that all importers providing the 'import' or 'export'\n+\tcapabilities use this.\n +\n A helper advertising the capability\n `refspec refs/heads/*:refs/svn/origin/branches/*`\n-- \n1.8.2.1.679.g509521a\n"},{"id":"214658","messageId":"1366243524-18202-4-git-send-email-felipe.contreras@gmail.com","threadId":"33529","inReplyTo":"1366243524-18202-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 3/6] transport-helper: clarify pushing without refspecs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T00:05:21Z","receivedAt":"2013-04-18T00:05:21Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"This has never worked, since it's inception the code simply skips all\nthe refs, essentially telling fast-export to do nothing.\n\nLet's at least tell the user what's going on.\n\nSigned-off-by: Felipe Contreras \n---\n Documentation/gitremote-helpers.txt | 4 ++--\n t/t5801-remote-helpers.sh | 6 +++---\n transport-helper.c | 5 +++--\n 3 files changed, 8 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\nindex ba7240c..4d26e37 100644\n--- a/Documentation/gitremote-helpers.txt\n+++ b/Documentation/gitremote-helpers.txt\n@@ -162,8 +162,8 @@ Miscellaneous capabilities\n \tFor remote helpers that implement 'import' or 'export', this capability\n \tallows the refs to be constrained to a private namespace, instead of\n \twriting to refs/heads or refs/remotes directly.\n-\tIt is recommended that all importers providing the 'import' or 'export'\n-\tcapabilities use this.\n+\tIt is recommended that all importers providing the 'import'\n+\tcapability use this. It's mandatory for 'export'.\n +\n A helper advertising the capability\n `refspec refs/heads/*:refs/svn/origin/branches/*`\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex cd1873c..3eeb309 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -111,13 +111,13 @@ test_expect_success 'pulling without refspecs' '\n \tcompare_refs local2 HEAD server HEAD\n '\n \n-test_expect_failure 'pushing without refspecs' '\n+test_expect_success 'pushing without refspecs' '\n \ttest_when_finished \"(cd local2 && git reset --hard origin)\" &&\n \t(cd local2 &&\n \techo content >>file &&\n \tgit commit -a -m ten &&\n-\tGIT_REMOTE_TESTGIT_REFSPEC=\"\" git push) &&\n-\tcompare_refs local2 HEAD server HEAD\n+\tGIT_REMOTE_TESTGIT_REFSPEC=\"\" test_must_fail git push 2> ../error) &&\n+\tgrep \"remote-helper doesn.t support push; refspec needed\" error\n '\n \n test_expect_success 'pulling without marks' '\ndiff --git a/transport-helper.c b/transport-helper.c\nindex cea787c..4d98567 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -785,6 +785,9 @@ static int push_refs_with_export(struct transport *transport,\n \tstruct string_list revlist_args = STRING_LIST_INIT_NODUP;\n \tstruct strbuf buf = STRBUF_INIT;\n \n+\tif (!data->refspecs)\n+\t\tdie(\"remote-helper doesn't support push; refspec needed\");\n+\n \thelper = get_helper(transport);\n \n \twrite_constant(helper->in, \"export\\n\");\n@@ -795,8 +798,6 @@ static int push_refs_with_export(struct transport *transport,\n \t\tchar *private;\n \t\tunsigned char sha1[20];\n \n-\t\tif (!data->refspecs)\n-\t\t\tcontinue;\n \t\tprivate = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);\n \t\tif (private && !get_sha1(private, sha1)) {\n \t\t\tstrbuf_addf(&buf, \"^%s\", private);\n-- \n1.8.2.1.679.g509521a\n"},{"id":"214659","messageId":"1366243524-18202-5-git-send-email-felipe.contreras@gmail.com","threadId":"33529","inReplyTo":"1366243524-18202-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 4/6] transport-helper: warn when refspec is not used","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T00:05:22Z","receivedAt":"2013-04-18T00:05:22Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"For the modes that need it. In the future we should probably error out,\ninstead of providing half-assed support.\n\nThe reason we want to do this is because if it's not present, the remote\nhelper might be updating refs/heads/*, or refs/remotes/origin/*,\ndirectly, and in the process fetch will get confused trying to update\nrefs that are already updated, or older than what they should be. We\nshouldn't be messing with the rest of git.\n\nSigned-off-by: Felipe Contreras \n---\n t/t5801-remote-helpers.sh | 6 ++++--\n transport-helper.c | 2 ++\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 3eeb309..1bb7529 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -100,14 +100,16 @@ test_expect_failure 'push new branch with old:new refspec' '\n \n test_expect_success 'cloning without refspec' '\n \tGIT_REMOTE_TESTGIT_REFSPEC=\"\" \\\n-\tgit clone \"testgit::${PWD}/server\" local2 &&\n+\tgit clone \"testgit::${PWD}/server\" local2 2> error &&\n+\tgrep \"This remote helper should implement refspec capability\" error &&\n \tcompare_refs local2 HEAD server HEAD\n '\n \n test_expect_success 'pulling without refspecs' '\n \t(cd local2 &&\n \tgit reset --hard &&\n-\tGIT_REMOTE_TESTGIT_REFSPEC=\"\" git pull) &&\n+\tGIT_REMOTE_TESTGIT_REFSPEC=\"\" git pull 2> ../error) &&\n+\tgrep \"This remote helper should implement refspec capability\" error &&\n \tcompare_refs local2 HEAD server HEAD\n '\n \ndiff --git a/transport-helper.c b/transport-helper.c\nindex 4d98567..573eaf7 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -215,6 +215,8 @@ static struct child_process *get_helper(struct transport *transport)\n \t\t\tfree((char *)refspecs[i]);\n \t\t}\n \t\tfree(refspecs);\n+\t} else if (data->import || data->bidi_import || data->export) {\n+\t\twarning(\"This remote helper should implement refspec capability.\");\n \t}\n \tstrbuf_release(&buf);\n \tif (debug)\n-- \n1.8.2.1.679.g509521a\n"},{"id":"214660","messageId":"1366243524-18202-6-git-send-email-felipe.contreras@gmail.com","threadId":"33529","inReplyTo":"1366243524-18202-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 5/6] transport-helper: trivial code shuffle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T00:05:23Z","receivedAt":"2013-04-18T00:05:23Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Just shuffle the die() part to make it more explicit, and cleanup the\ncode-style.\n\nSigned-off-by: Felipe Contreras \n---\n transport-helper.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 573eaf7..9d31f2d 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -800,6 +800,9 @@ static int push_refs_with_export(struct transport *transport,\n \t\tchar *private;\n \t\tunsigned char sha1[20];\n \n+\t\tif (ref->deletion)\n+\t\t\tdie(\"remote-helpers do not support ref deletion\");\n+\n \t\tprivate = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);\n \t\tif (private && !get_sha1(private, sha1)) {\n \t\t\tstrbuf_addf(&buf, \"^%s\", private);\n@@ -807,13 +810,8 @@ static int push_refs_with_export(struct transport *transport,\n \t\t}\n \t\tfree(private);\n \n-\t\tif (ref->deletion) {\n-\t\t\tdie(\"remote-helpers do not support ref deletion\");\n-\t\t}\n-\n \t\tif (ref->peer_ref)\n \t\t\tstring_list_append(&revlist_args, ref->peer_ref->name);\n-\n \t}\n \n \tif (get_exporter(transport, &exporter, &revlist_args))\n-- \n1.8.2.1.679.g509521a\n"},{"id":"214661","messageId":"1366243524-18202-7-git-send-email-felipe.contreras@gmail.com","threadId":"33529","inReplyTo":"1366243524-18202-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH 6/6] transport-helper: update remote helper namespace","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T00:05:24Z","receivedAt":"2013-04-18T00:05:24Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"When pushing, the remote namespace is updated correctly\n(e.g. refs/origin/master), but not the remote helper's\n(e.g. refs/testgit/origin/master), which currently is only updated while\nfetching.\n\nSince the remote namespace is used to tell fast-export which commits to\navoid (because they were already imported/exported), it makes sense to\nhave them in sync so they don't get generated twice. If the remote\nhelper was implemented properly, they would be ignored, if not, they\nprobably would end up repeated (probably).\n\nSigned-off-by: Felipe Contreras \n---\n t/t5801-remote-helpers.sh | 12 ++++++++++++\n transport-helper.c | 20 ++++++++++++++++----\n 2 files changed, 28 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 1bb7529..097691c 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -153,4 +153,16 @@ test_expect_success 'push ref with existing object' '\n \tcompare_refs local dup server dup\n '\n \n+test_expect_success 'push update refs' '\n+\t(cd local &&\n+\tgit checkout -b update master &&\n+\techo update >>file &&\n+\tgit commit -a -m update &&\n+\tgit push origin update\n+\tgit rev-parse --verify testgit/origin/heads/update >expect &&\n+\tgit rev-parse --verify remotes/origin/update >actual\n+\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 9d31f2d..414d6c8 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -11,6 +11,7 @@\n #include \"thread-utils.h\"\n #include \"sigchain.h\"\n #include \"argv-array.h\"\n+#include \"refs.h\"\n \n static int debug;\n \n@@ -620,7 +621,7 @@ static int fetch(struct transport *transport,\n \treturn -1;\n }\n \n-static void push_update_ref_status(struct strbuf *buf,\n+static int push_update_ref_status(struct strbuf *buf,\n \t\t\t\t struct ref **ref,\n \t\t\t\t struct ref *remote_refs)\n {\n@@ -686,7 +687,7 @@ static void push_update_ref_status(struct strbuf *buf,\n \t\t*ref = find_ref_by_name(remote_refs, refname);\n \tif (!*ref) {\n \t\twarning(\"helper reported unexpected status of %s\", refname);\n-\t\treturn;\n+\t\treturn 1;\n \t}\n \n \tif ((*ref)->status != REF_STATUS_NONE) {\n@@ -695,11 +696,12 @@ static void push_update_ref_status(struct strbuf *buf,\n \t\t * status reported by the remote helper if the latter is 'no match'.\n \t\t */\n \t\tif (status == REF_STATUS_NONE)\n-\t\t\treturn;\n+\t\t\treturn 1;\n \t}\n \n \t(*ref)->status = status;\n \t(*ref)->remote_status = msg;\n+\treturn 0;\n }\n \n static void push_update_refs_status(struct helper_data *data,\n@@ -708,11 +710,21 @@ static void push_update_refs_status(struct helper_data *data,\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct ref *ref = remote_refs;\n \tfor (;;) {\n+\t\tchar *private;\n+\n \t\trecvline(data, &buf);\n \t\tif (!buf.len)\n \t\t\tbreak;\n \n-\t\tpush_update_ref_status(&buf, &ref, remote_refs);\n+\t\tif (push_update_ref_status(&buf, &ref, remote_refs))\n+\t\t\tcontinue;\n+\n+\t\t/* propagate back the update to the remote namespace */\n+\t\tprivate = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);\n+\t\tif (!private)\n+\t\t\tcontinue;\n+\t\tupdate_ref(\"update by helper\", private, ref->new_sha1, NULL, 0, 0);\n+\t\tfree(private);\n \t}\n \tstrbuf_release(&buf);\n }\n-- \n1.8.2.1.679.g509521a\n"},{"id":"214663","messageId":"CAGdFq_j-Z-HCxLQwYEK3esuipUGLy6kNqZzSH+CQxQbkiAR6Cg@mail.gmail.com","threadId":"33529","inReplyTo":"1366243524-18202-4-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 3/6] transport-helper: clarify pushing without refspecs","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2013-04-18T00:14:44Z","receivedAt":"2013-04-18T00:14:44Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"On Wed, Apr 17, 2013 at 5:05 PM, Felipe Contreras\n wrote:\n> This has never worked, since it's inception the code simply skips all\n> the refs, essentially telling fast-export to do nothing.\n\nMakes sense.\n\n--\nCheers,\n\nSverre Rabbelier\n"},{"id":"214669","messageId":"CAMP44s05XMWO=HTDj-tBiEpXozJk7Q9e3+9d0d0U5bseGWTyzg@mail.gmail.com","threadId":"33529","inReplyTo":"1366243524-18202-7-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 6/6] transport-helper: update remote helper namespace","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T04:14:15Z","receivedAt":"2013-04-18T04:14:15Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Apr 17, 2013 at 7:05 PM, Felipe Contreras\n wrote:\n\n> --- a/t/t5801-remote-helpers.sh\n> +++ b/t/t5801-remote-helpers.sh\n> @@ -153,4 +153,16 @@ test_expect_success 'push ref with existing object' '\n> compare_refs local dup server dup\n> '\n>\n> +test_expect_success 'push update refs' '\n> + (cd local &&\n> + git checkout -b update master &&\n> + echo update >>file &&\n> + git commit -a -m update &&\n> + git push origin update\n> + git rev-parse --verify testgit/origin/heads/update >expect &&\n> + git rev-parse --verify remotes/origin/update >actual\n> + test_cmp expect actual\n\nSlightly confusing and a bit buggy:\n\n- git rev-parse --verify testgit/origin/heads/update >expect &&\n- git rev-parse --verify remotes/origin/update >actual\n+ git rev-parse --verify remotes/origin/update >expect &&\n+ git rev-parse --verify testgit/origin/heads/update >actual &&\n\n> static void push_update_refs_status(struct helper_data *data,\n> @@ -708,11 +710,21 @@ static void push_update_refs_status(struct helper_data *data,\n> struct strbuf buf = STRBUF_INIT;\n> struct ref *ref = remote_refs;\n> for (;;) {\n> + char *private;\n> +\n> recvline(data, &buf);\n> if (!buf.len)\n> break;\n>\n> - push_update_ref_status(&buf, &ref, remote_refs);\n> + if (push_update_ref_status(&buf, &ref, remote_refs))\n> + continue;\n\nActually, since this function is also used by push_with_push:\n\n if (!data->refspecs)\n continue;\n\nI had it in my previous series but removed it.\n\n\nI'll reroll.\n\n-- \nFelipe Contreras\n"},{"id":"214684","messageId":"87li8gxpq2.fsf@linux-k42r.v.cablecom.net","threadId":"33529","inReplyTo":"1366243524-18202-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 1/6] transport-helper: clarify *:* refspec","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-04-18T07:28:05Z","receivedAt":"2013-04-18T07:28:05Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Felipe Contreras writes:\n\n> The *:* refspec doesn't work, and never has, clarify the code and\n> documentation to reflect that. This in effect reverts commit 9e7673e\n> (gitremote-helpers(1): clarify refspec behaviour).\n[...]\n> -test_expect_success 'pulling with straight refspec' '\n> -\t(cd local2 &&\n> -\tGIT_REMOTE_TESTGIT_REFSPEC=\"*:*\" git pull) &&\n> -\tcompare_refs local2 HEAD server HEAD\n> -'\n> -\n> -test_expect_failure 'pushing with straight refspec' '\n> -\ttest_when_finished \"(cd local2 && git reset --hard origin)\" &&\n> -\t(cd local2 &&\n> -\techo content >>file &&\n> -\tgit commit -a -m eleven &&\n> -\tGIT_REMOTE_TESTGIT_REFSPEC=\"*:*\" git push) &&\n> -\tcompare_refs local2 HEAD server HEAD\n> -'\n\nSo what's wrong with the tests? Do they fail to test what they claim\n(how?), test something that wasn't reasonable to begin with, or\nsomething entirely different?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"214692","messageId":"CAMP44s0c-c3pPW8t9p9qabjv46gSeE6y4p6STPeV+kqB77xOJA@mail.gmail.com","threadId":"33529","inReplyTo":"87li8gxpq2.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH 1/6] transport-helper: clarify *:* refspec","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T08:18:43Z","receivedAt":"2013-04-18T08:18:43Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Apr 18, 2013 at 2:28 AM, Thomas Rast wrote:\n> Felipe Contreras writes:\n>\n>> The *:* refspec doesn't work, and never has, clarify the code and\n>> documentation to reflect that. This in effect reverts commit 9e7673e\n>> (gitremote-helpers(1): clarify refspec behaviour).\n> [...]\n>> -test_expect_success 'pulling with straight refspec' '\n>> - (cd local2 &&\n>> - GIT_REMOTE_TESTGIT_REFSPEC=\"*:*\" git pull) &&\n>> - compare_refs local2 HEAD server HEAD\n>> -'\n>> -\n>> -test_expect_failure 'pushing with straight refspec' '\n>> - test_when_finished \"(cd local2 && git reset --hard origin)\" &&\n>> - (cd local2 &&\n>> - echo content >>file &&\n>> - git commit -a -m eleven &&\n>> - GIT_REMOTE_TESTGIT_REFSPEC=\"*:*\" git push) &&\n>> - compare_refs local2 HEAD server HEAD\n>> -'\n>\n> So what's wrong with the tests? Do they fail to test what they claim\n> (how?), test something that wasn't reasonable to begin with, or\n> something entirely different?\n\nLook at the code comment, and look at the now updated documentation\nthat assumes that *:* was reasonable. Given the available information,\nit would be reasonable to assume that *:* did work, but it didn't\nwork, and it's not really possible to fix it, even if we wanted to, it\nwould be a hack. It's better to accept that fact and stop worrying too\nmuch about what would be the best way to do the wrong thing.\n\n-- \nFelipe Contreras\n"},{"id":"214724","messageId":"87ppxsvwq7.fsf@linux-k42r.v.cablecom.net","threadId":"33529","inReplyTo":"CAMP44s0c-c3pPW8t9p9qabjv46gSeE6y4p6STPeV+kqB77xOJA@mail.gmail.com","subject":"Re: [PATCH 1/6] transport-helper: clarify *:* refspec","fromName":"Thomas Rast","fromEmail":"trast@inf.ethz.ch","sentAt":"2013-04-18T12:39:44Z","receivedAt":"2013-04-18T12:39:44Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Felipe Contreras writes:\n\n> On Thu, Apr 18, 2013 at 2:28 AM, Thomas Rast wrote:\n>> Felipe Contreras writes:\n>>\n>>> The *:* refspec doesn't work, and never has, clarify the code and\n>>> documentation to reflect that. This in effect reverts commit 9e7673e\n>>> (gitremote-helpers(1): clarify refspec behaviour).\n>> [...]\n>>> -test_expect_success 'pulling with straight refspec' '\n>>> - (cd local2 &&\n>>> - GIT_REMOTE_TESTGIT_REFSPEC=\"*:*\" git pull) &&\n>>> - compare_refs local2 HEAD server HEAD\n>>> -'\n>>> -\n>>> -test_expect_failure 'pushing with straight refspec' '\n>>> - test_when_finished \"(cd local2 && git reset --hard origin)\" &&\n>>> - (cd local2 &&\n>>> - echo content >>file &&\n>>> - git commit -a -m eleven &&\n>>> - GIT_REMOTE_TESTGIT_REFSPEC=\"*:*\" git push) &&\n>>> - compare_refs local2 HEAD server HEAD\n>>> -'\n>>\n>> So what's wrong with the tests? Do they fail to test what they claim\n>> (how?), test something that wasn't reasonable to begin with, or\n>> something entirely different?\n>\n> Look at the code comment, and look at the now updated documentation\n> that assumes that *:* was reasonable. Given the available information,\n> it would be reasonable to assume that *:* did work, but it didn't\n> work, and it's not really possible to fix it, even if we wanted to, it\n> would be a hack. It's better to accept that fact and stop worrying too\n> much about what would be the best way to do the wrong thing.\n\nOk, you say that the *failing* test set an expectation that is\nunrealistic, so let's drop it.\n\nBut then what about the successful test? Does it actually work (and by\nremoving the test, you are saying that we don't care if we subsequently\nbreak that (mis)feature)? Or did it test the wrong thing?\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"214725","messageId":"CAMP44s2p1udJ=YuXBoYJ42iMiB68nU8t_GBGMdsMjTOEsjrZuQ@mail.gmail.com","threadId":"33529","inReplyTo":"87ppxsvwq7.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH 1/6] transport-helper: clarify *:* refspec","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T13:06:00Z","receivedAt":"2013-04-18T13:06:00Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Apr 18, 2013 at 7:39 AM, Thomas Rast wrote:\n> Felipe Contreras writes:\n>\n>> On Thu, Apr 18, 2013 at 2:28 AM, Thomas Rast wrote:\n>>> Felipe Contreras writes:\n>>>\n>>>> The *:* refspec doesn't work, and never has, clarify the code and\n>>>> documentation to reflect that. This in effect reverts commit 9e7673e\n>>>> (gitremote-helpers(1): clarify refspec behaviour).\n>>> [...]\n>>>> -test_expect_success 'pulling with straight refspec' '\n>>>> - (cd local2 &&\n>>>> - GIT_REMOTE_TESTGIT_REFSPEC=\"*:*\" git pull) &&\n>>>> - compare_refs local2 HEAD server HEAD\n>>>> -'\n>>>> -\n>>>> -test_expect_failure 'pushing with straight refspec' '\n>>>> - test_when_finished \"(cd local2 && git reset --hard origin)\" &&\n>>>> - (cd local2 &&\n>>>> - echo content >>file &&\n>>>> - git commit -a -m eleven &&\n>>>> - GIT_REMOTE_TESTGIT_REFSPEC=\"*:*\" git push) &&\n>>>> - compare_refs local2 HEAD server HEAD\n>>>> -'\n>>>\n>>> So what's wrong with the tests? Do they fail to test what they claim\n>>> (how?), test something that wasn't reasonable to begin with, or\n>>> something entirely different?\n>>\n>> Look at the code comment, and look at the now updated documentation\n>> that assumes that *:* was reasonable. Given the available information,\n>> it would be reasonable to assume that *:* did work, but it didn't\n>> work, and it's not really possible to fix it, even if we wanted to, it\n>> would be a hack. It's better to accept that fact and stop worrying too\n>> much about what would be the best way to do the wrong thing.\n>\n> Ok, you say that the *failing* test set an expectation that is\n> unrealistic, so let's drop it.\n>\n> But then what about the successful test? Does it actually work (and by\n> removing the test, you are saying that we don't care if we subsequently\n> break that (mis)feature)? Or did it test the wrong thing?\n\nYeah, it works, in the sense that peeing in a bottle is a solution; it\nmight work, but it's not recommendable. So, if suddenly working,\nfrankly I don't care. I added those tests, and I don't think they are\nneeded. In a not too distant future it should not be permitted to\n\"work\"; we don't want developers to shoot themselves in the foot, and\nheir users too.\n\n-- \nFelipe Contreras\n"},{"id":"214750","messageId":"517041EF.5060000@gmail.com","threadId":"33529","inReplyTo":"1366243524-18202-4-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH 3/6] transport-helper: clarify pushing without refspecs","fromName":"Stefano Lattarini","fromEmail":"stefano.lattarini@gmail.com","sentAt":"2013-04-18T18:56:47Z","receivedAt":"2013-04-18T18:56:47Z","isPatch":true,"sender":{"key":"stefano.lattarini@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1429199?v=4"},"body":"On 04/18/2013 02:05 AM, Felipe Contreras wrote:\n> This has never worked, since it's inception the code simply skips all\n>\ns/it's/its/ (sorry for nitpicking)\n\n> the refs, essentially telling fast-export to do nothing.\n> \n> Let's at least tell the user what's going on.\n> \n> [SNIP]\n>\n\nRegards,\n Stefano\n"}]}