{"thread":{"id":"33530","subject":"[PATCH v2 0/6] transport-helper: some clarifications and a fix","startedAt":"2013-04-18T04:14:27Z","lastAt":"2013-04-19T03:41:17Z","messageCount":26,"participants":["Felipe Contreras","John Keeping","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":6},"messages":[{"id":"214670","messageId":"1366258473-12841-1-git-send-email-felipe.contreras@gmail.com","threadId":"33530","inReplyTo":null,"subject":"[PATCH v2 0/6] transport-helper: some clarifications and a fix","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T04:14:27Z","receivedAt":"2013-04-18T04:14:27Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Hi,\n\nSame as before, but with a small bug fix, and minor test improvements.\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 | 40 ++++++++++++++++++++++++++-----------\n 3 files changed, 53 insertions(+), 38 deletions(-)\n\n-- \n1.8.2.1.679.g509521a\n"},{"id":"214671","messageId":"1366258473-12841-2-git-send-email-felipe.contreras@gmail.com","threadId":"33530","inReplyTo":"1366258473-12841-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 1/6] transport-helper: clarify *:* refspec","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T04:14:28Z","receivedAt":"2013-04-18T04:14:28Z","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":"214673","messageId":"1366258473-12841-3-git-send-email-felipe.contreras@gmail.com","threadId":"33530","inReplyTo":"1366258473-12841-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 2/6] transport-helper: update refspec documentation","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T04:14:29Z","receivedAt":"2013-04-18T04:14:29Z","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":"214672","messageId":"1366258473-12841-4-git-send-email-felipe.contreras@gmail.com","threadId":"33530","inReplyTo":"1366258473-12841-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 3/6] transport-helper: clarify pushing without refspecs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T04:14:30Z","receivedAt":"2013-04-18T04:14:30Z","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":"214674","messageId":"1366258473-12841-5-git-send-email-felipe.contreras@gmail.com","threadId":"33530","inReplyTo":"1366258473-12841-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 4/6] transport-helper: warn when refspec is not used","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T04:14:31Z","receivedAt":"2013-04-18T04:14:31Z","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":"214675","messageId":"1366258473-12841-6-git-send-email-felipe.contreras@gmail.com","threadId":"33530","inReplyTo":"1366258473-12841-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 5/6] transport-helper: trivial code shuffle","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T04:14:32Z","receivedAt":"2013-04-18T04:14:32Z","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":"214676","messageId":"1366258473-12841-7-git-send-email-felipe.contreras@gmail.com","threadId":"33530","inReplyTo":"1366258473-12841-1-git-send-email-felipe.contreras@gmail.com","subject":"[PATCH v2 6/6] transport-helper: update remote helper namespace","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T04:14:33Z","receivedAt":"2013-04-18T04:14:33Z","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 | 23 +++++++++++++++++++----\n 2 files changed, 31 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex 1bb7529..5466969 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 remotes/origin/update >expect &&\n+\tgit rev-parse --verify testgit/origin/heads/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..da16393 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,24 @@ 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\tif (!data->refspecs)\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":"214693","messageId":"20130418082441.GR2278@serenity.lan","threadId":"33530","inReplyTo":"1366258473-12841-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 1/6] transport-helper: clarify *:* refspec","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-04-18T08:24:41Z","receivedAt":"2013-04-18T08:24:41Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Apr 17, 2013 at 11:14:28PM -0500, Felipe Contreras wrote:\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\nIn what way doesn't it work? If I specify that refspec then I do get\noutput that appears sensible.\n\n> Signed-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> \n> diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\n> index 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\nThis is wrong. As your later patch makes clearer, there is no implied\nrefspec for push - it only works for fetch. I found the wording you've\nreverted to extremely misleading. How about something like this:\n\n For historical reasons, 'import' treats the absence of a 'refspec'\n line as equivalent to `refspec *:*`; remote helpers should always\n specify an explicit refspec.\n\n?\n\n> 'bidi-import'::\n> \tThis modifies the 'import' capability.\n> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\n> index 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) &&\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 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> -- \n> 1.8.2.1.679.g509521a\n"},{"id":"214699","messageId":"CAMP44s0cJwHSJrWKkWAwKSv8OE=OqVRdKEtBNdAvQ7YAE5-JKw@mail.gmail.com","threadId":"33530","inReplyTo":"20130418082441.GR2278@serenity.lan","subject":"Re: [PATCH v2 1/6] transport-helper: clarify *:* refspec","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T09:27:57Z","receivedAt":"2013-04-18T09:27:57Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Apr 18, 2013 at 3:24 AM, John Keeping wrote:\n> On Wed, Apr 17, 2013 at 11:14:28PM -0500, Felipe Contreras wrote:\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> In what way doesn't it work? If I specify that refspec then I do get\n> output that appears sensible.\n\n% git checkout origin/master\n% make -C t t5801-remote-helpers.sh\n\nnot ok 15 - pushing with straight refspec # TODO known breakage\n\nIt fails for me here.\n\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>>\n>> diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\n>> index 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> This is wrong.\n\nIt has been like that since v1.7.0.\n\n> As your later patch makes clearer, there is no implied\n> refspec for push - it only works for fetch. I found the wording you've\n> reverted to extremely misleading. How about something like this:\n>\n> For historical reasons, 'import' treats the absence of a 'refspec'\n> line as equivalent to `refspec *:*`; remote helpers should always\n> specify an explicit refspec.\n\nMaybe my version was \"misleading\" because it didn't mention it was for\nhistorical reasons, but yours is plain wrong; remote helpers might not\nbe using 'import' or 'export' at all, so not all remote helpers should\nalways specify an explicit refspec. And if the previous text \"It is\nrecommended that all importers providing the 'import' capability use\nthis. It's mandatory for 'export'.\" does not convey the idea these\nremote helpers should always specify an explicit refspec, I don't know\nwhat it does.\n\n-- \nFelipe Contreras\n"},{"id":"214701","messageId":"20130418094531.GU2278@serenity.lan","threadId":"33530","inReplyTo":"CAMP44s0cJwHSJrWKkWAwKSv8OE=OqVRdKEtBNdAvQ7YAE5-JKw@mail.gmail.com","subject":"Re: [PATCH v2 1/6] transport-helper: clarify *:* refspec","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-04-18T09:45:31Z","receivedAt":"2013-04-18T09:45:31Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Apr 18, 2013 at 04:27:57AM -0500, Felipe Contreras wrote:\n> On Thu, Apr 18, 2013 at 3:24 AM, John Keeping wrote:\n> > On Wed, Apr 17, 2013 at 11:14:28PM -0500, Felipe Contreras wrote:\n> >> diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\n> >> index 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> > This is wrong.\n> \n> It has been like that since v1.7.0.\n\nThat doesn't mean it's right. I suspect that it was originally correct,\nbut then 'export' was added at which point this became stale.\n\n> > As your later patch makes clearer, there is no implied\n> > refspec for push - it only works for fetch. I found the wording you've\n> > reverted to extremely misleading. How about something like this:\n> >\n> > For historical reasons, 'import' treats the absence of a 'refspec'\n> > line as equivalent to `refspec *:*`; remote helpers should always\n> > specify an explicit refspec.\n> \n> Maybe my version was \"misleading\" because it didn't mention it was for\n> historical reasons, but yours is plain wrong; remote helpers might not\n> be using 'import' or 'export' at all, so not all remote helpers should\n> always specify an explicit refspec. And if the previous text \"It is\n> recommended that all importers providing the 'import' capability use\n> this. It's mandatory for 'export'.\" does not convey the idea these\n> remote helpers should always specify an explicit refspec, I don't know\n> what it does.\n\nI didn't say mine was correct, but there was a reason that I wanted to\nchange the existing text. Just going back to what was there before is\nnot a good way to make things better.\n\nIn my copy of pu I don't see the text \"It's mandatory for 'export'\", it\njust stops after \"It is recommended that all importers providing the\n'import' capability use this\". That paragraph also starts with \"This\nmodifies the 'import' capability\" making no mention of export.\n\nPerhaps we need a slightly more extensive re-write of the documentation\nfor refspec to make it very clear where it applies and when it is\nneeded.\n"},{"id":"214705","messageId":"CAMP44s35tmmgNd-vJQnqG56avSPY2NOBR6iR7eNtpfemXsfPJA@mail.gmail.com","threadId":"33530","inReplyTo":"20130418094531.GU2278@serenity.lan","subject":"Re: [PATCH v2 1/6] transport-helper: clarify *:* refspec","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T10:02:11Z","receivedAt":"2013-04-18T10:02:11Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Apr 18, 2013 at 4:45 AM, John Keeping wrote:\n> On Thu, Apr 18, 2013 at 04:27:57AM -0500, Felipe Contreras wrote:\n\n>> Maybe my version was \"misleading\" because it didn't mention it was for\n>> historical reasons, but yours is plain wrong; remote helpers might not\n>> be using 'import' or 'export' at all, so not all remote helpers should\n>> always specify an explicit refspec. And if the previous text \"It is\n>> recommended that all importers providing the 'import' capability use\n>> this. It's mandatory for 'export'.\" does not convey the idea these\n>> remote helpers should always specify an explicit refspec, I don't know\n>> what it does.\n>\n> I didn't say mine was correct, but there was a reason that I wanted to\n> change the existing text. Just going back to what was there before is\n> not a good way to make things better.\n\nAnd just because it was that way before doesn't mean it was worst.\nYour patch essentially switched from \"it is implied\", to \"it should be\nexplicit\", which is wrong. Going back to the previous text is the\nsimplest change that restores a reasonable explanation. The next\npatches in this series deal with the rest of the issues in this text.\n\n> In my copy of pu I don't see the text \"It's mandatory for 'export'\", it\n> just stops after \"It is recommended that all importers providing the\n> 'import' capability use this\". That paragraph also starts with \"This\n> modifies the 'import' capability\" making no mention of export.\n\nThis is patch 1 of 6, keep going.\n\n-- \nFelipe Contreras\n"},{"id":"214707","messageId":"20130418100610.GV2278@serenity.lan","threadId":"33530","inReplyTo":"CAMP44s35tmmgNd-vJQnqG56avSPY2NOBR6iR7eNtpfemXsfPJA@mail.gmail.com","subject":"Re: [PATCH v2 1/6] transport-helper: clarify *:* refspec","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-04-18T10:06:11Z","receivedAt":"2013-04-18T10:06:11Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Apr 18, 2013 at 05:02:11AM -0500, Felipe Contreras wrote:\n> On Thu, Apr 18, 2013 at 4:45 AM, John Keeping wrote:\n> > In my copy of pu I don't see the text \"It's mandatory for 'export'\", it\n> > just stops after \"It is recommended that all importers providing the\n> > 'import' capability use this\". That paragraph also starts with \"This\n> > modifies the 'import' capability\" making no mention of export.\n> \n> This is patch 1 of 6, keep going.\n\nAh, I see now. I had read all of the patches initially, but then\nfocused on this one for discussion.\n\nThe end result after this series does look good, but I find it slightly\nstrange to take a step backwards on the way.\n"},{"id":"214708","messageId":"20130418101133.GW2278@serenity.lan","threadId":"33530","inReplyTo":"1366258473-12841-4-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-04-18T10:11:33Z","receivedAt":"2013-04-18T10:11:33Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Apr 17, 2013 at 11:14:30PM -0500, Felipe Contreras 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> \n> Let's at least tell the user what's going on.\n> \n> Signed-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> \n> diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\n> index 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\ns/It's/It is/\n\n> +\n> A helper advertising the capability\n> `refspec refs/heads/*:refs/svn/origin/branches/*`\n> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\n> index 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' '\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 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\nI think the \"refspec needed\" text is likely to be confusing if an\nend-user ever sees this message. I'm not sure how we can provide useful\nfeedback for both remote helper authors and end-users though.\n\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> -- \n> 1.8.2.1.679.g509521a\n"},{"id":"214709","messageId":"CAMP44s33WLE6xJ_bFJBX2z0hdkALqCKpq7Ve+8hZw2URWUPz_w@mail.gmail.com","threadId":"33530","inReplyTo":"20130418101133.GW2278@serenity.lan","subject":"Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T10:14:18Z","receivedAt":"2013-04-18T10:14:18Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Apr 18, 2013 at 5:11 AM, John Keeping wrote:\n> On Wed, Apr 17, 2013 at 11:14:30PM -0500, Felipe Contreras 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>>\n>> Let's at least tell the user what's going on.\n>>\n>> Signed-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>>\n>> diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\n>> index ba7240c..4d26e37 100644\n>> --- a/Documentation/gitremote-helpers.txt\n>> +++ b/Documentation/gitremote-helpers.txt\n>> @@ -162,8 +162,8 @@ Miscellaneous capabilities\n>> For remote helpers that implement 'import' or 'export', this capability\n>> allows the refs to be constrained to a private namespace, instead of\n>> writing to refs/heads or refs/remotes directly.\n>> - It is recommended that all importers providing the 'import' or 'export'\n>> - capabilities use this.\n>> + It is recommended that all importers providing the 'import'\n>> + capability use this. It's mandatory for 'export'.\n>\n> s/It's/It is/\n\nWhat's the difference?\n\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>> struct string_list revlist_args = STRING_LIST_INIT_NODUP;\n>> struct strbuf buf = STRBUF_INIT;\n>>\n>> + if (!data->refspecs)\n>> + die(\"remote-helper doesn't support push; refspec needed\");\n>\n> I think the \"refspec needed\" text is likely to be confusing if an\n> end-user ever sees this message. I'm not sure how we can provide useful\n> feedback for both remote helper authors and end-users though.\n\nIt doesn't have to be. They'll google it, or they'll post a bug report\nwith this warning verbatim, or they'll just ignore it. I don't think\nthere's much more we can do to help in either of these cases.\n\n-- \nFelipe Contreras\n"},{"id":"214711","messageId":"20130418102428.GX2278@serenity.lan","threadId":"33530","inReplyTo":"CAMP44s33WLE6xJ_bFJBX2z0hdkALqCKpq7Ve+8hZw2URWUPz_w@mail.gmail.com","subject":"Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-04-18T10:24:29Z","receivedAt":"2013-04-18T10:24:29Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Apr 18, 2013 at 05:14:18AM -0500, Felipe Contreras wrote:\n> On Thu, Apr 18, 2013 at 5:11 AM, John Keeping wrote:\n> > On Wed, Apr 17, 2013 at 11:14:30PM -0500, Felipe Contreras 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> >>\n> >> Let's at least tell the user what's going on.\n> >>\n> >> Signed-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> >>\n> >> diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\n> >> index ba7240c..4d26e37 100644\n> >> --- a/Documentation/gitremote-helpers.txt\n> >> +++ b/Documentation/gitremote-helpers.txt\n> >> @@ -162,8 +162,8 @@ Miscellaneous capabilities\n> >> For remote helpers that implement 'import' or 'export', this capability\n> >> allows the refs to be constrained to a private namespace, instead of\n> >> writing to refs/heads or refs/remotes directly.\n> >> - It is recommended that all importers providing the 'import' or 'export'\n> >> - capabilities use this.\n> >> + It is recommended that all importers providing the 'import'\n> >> + capability use this. It's mandatory for 'export'.\n> >\n> > s/It's/It is/\n> \n> What's the difference?\n\n\"It's\" is considered informal, while we do adopt that tone on the\nmailing list and sometimes in the documentation, in general I think we\nshould avoid contractions in the formal documentation.\n\nIn this case it is particularly jarring because it immediately follows a\nsentence where we use the full \"It is\" form.\n"},{"id":"214740","messageId":"7vhaj37nr5.fsf@alter.siamese.dyndns.org","threadId":"33530","inReplyTo":"1366258473-12841-2-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 1/6] transport-helper: clarify *:* refspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-18T17:27:26Z","receivedAt":"2013-04-18T17:27:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?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> 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\nI presume\n\n s/.$/, but `*:*` does not work so do not use it./\n\nis implied?\n\n> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\n> index 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\nThis one made me raise my eyebrows, first.\n\nThe only reason this test \"passes\" is because this particular\n\"pull\", due to what local2 and server happens to have before this\ntest runs, is an \"Already up-to-date\" and a no-op. You are removing\nthis because it is not really testing anything useful, and if you\nchange it to test something real, e.g. by rewinding local2, it will\nreveal the breakage.\n\nAm I reading you correctly?\n\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 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"},{"id":"214741","messageId":"7va9ov7npq.fsf@alter.siamese.dyndns.org","threadId":"33530","inReplyTo":"1366258473-12841-4-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-18T17:28:17Z","receivedAt":"2013-04-18T17:28:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\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\nGood description.\n\n>\n> Let's at least tell the user what's going on.\n>\n> Signed-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>\n> diff --git a/Documentation/gitremote-helpers.txt b/Documentation/gitremote-helpers.txt\n> index 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\nAs [1/6] said \"*:*\" does not work and has never worked, and\nespecially on the push side the patch below makes it clear it was a\nglorified no-op, it may be better to say a bit more than \"use\nthis. It's mandatory\". It is not like any value makes sense, right?\n\nPerhaps the documentation will be further updated in a later step in\nthe series to clarify it. Let's read on til the end of the series...\n\n> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\n> index 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\nI somehow like this change that turns a _failure into a _success by\nexpecting test_must_fail, especially because it is clear why the\ntested \"push\" should fail when you look at the rest of the patch ;-)\n\nFun.\n\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 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"},{"id":"214742","messageId":"7v38un7nnp.fsf@alter.siamese.dyndns.org","threadId":"33530","inReplyTo":"20130418101133.GW2278@serenity.lan","subject":"Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-18T17:29:30Z","receivedAt":"2013-04-18T17:29:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n>> +\tIt is recommended that all importers providing the 'import'\n>> +\tcapability use this. It's mandatory for 'export'.\n>\n> s/It's/It is/\n\nI personally do not care _too_ deeply either way, but I agree our\ndocumentation tends to use the latter more and being consistent\nwould be good.\n\n>> diff --git a/transport-helper.c b/transport-helper.c\n>> index 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> I think the \"refspec needed\" text is likely to be confusing if an\n> end-user ever sees this message. I'm not sure how we can provide useful\n> feedback for both remote helper authors and end-users though.\n\nThis \"refspecs\" only come via the helper and not directly from the\nend user, no?\n\nIf that is the case, I do not think \"confusing\" is much of an issue.\nNot _(\"localizing\") is also the right thing to do. We may want to\nsay \"BUG: \" at front to clarify it is not the end-user's fault, but\na problem they may want to report. If we at this point know what\nhelper attempted export without giving refspecs, it may help to show\nit here, so that developers will know with what helper the user\nhad problems with.\n"},{"id":"214743","messageId":"7vvc7j6917.fsf@alter.siamese.dyndns.org","threadId":"33530","inReplyTo":"1366258473-12841-5-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 4/6] transport-helper: warn when refspec is not used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-18T17:30:44Z","receivedAt":"2013-04-18T17:30:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> For the modes that need it. In the future we should probably error out,\n> instead of providing half-assed support.\n>\n> The reason we want to do this is because if it's not present, the remote\n> helper might be updating refs/heads/*, or refs/remotes/origin/*,\n> directly, and in the process fetch will get confused trying to update\n> refs that are already updated, or older than what they should be. We\n> shouldn't be messing with the rest of git.\n\nSo that answers my question in the response to an earlier one in\nthis series. We expect the ref updates to be done by the fetch or\npush that drives the helper, and do not want the helper to interfere\nwith its ref updates.\n\nSo it is not just 'refspec' _allows_ the refs to be constrained to a\nprivate namespace, like the earlier updates made the documentation\nsay; it _is_ mandatory to use refspecs to constrain them to avoid\ntouching refs/heads and refs/remotes namespace.\n\nAm I reading you correctly?\n\nAssuming I am, the patch in this message looks reasonable.\n\nIt makes the documentation updates a few patches ago look a bit\nwanting, though.\n\nThanks.\n\n> Signed-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>\n> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\n> index 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> \n> diff --git a/transport-helper.c b/transport-helper.c\n> index 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"},{"id":"214744","messageId":"7vobdb690u.fsf@alter.siamese.dyndns.org","threadId":"33530","inReplyTo":"1366258473-12841-7-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 6/6] transport-helper: update remote helper namespace","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-18T17:30:57Z","receivedAt":"2013-04-18T17:30:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Felipe Contreras writes:\n\n> 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\n> fetching.\n>\n> Since the remote namespace is used to tell fast-export which commits to\n> avoid (because they were already imported/exported), it makes sense to\n> have them in sync so they don't get generated twice. If the remote\n> helper was implemented properly, they would be ignored, if not, they\n> probably would end up repeated (probably).\n>\n> Signed-off-by: Felipe Contreras \n> ---\n\nThis one looks suspiciously familiar ;-).\n\nI'll drop the last one I queued and pushed out a few days ago and\nreplace it with these 6 patches.\n"},{"id":"214749","messageId":"20130418183540.GB2278@serenity.lan","threadId":"33530","inReplyTo":"7v38un7nnp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-04-18T18:35:40Z","receivedAt":"2013-04-18T18:35:40Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Apr 18, 2013 at 10:29:30AM -0700, Junio C Hamano wrote:\n> John Keeping writes:\n> >> diff --git a/transport-helper.c b/transport-helper.c\n> >> index 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> > I think the \"refspec needed\" text is likely to be confusing if an\n> > end-user ever sees this message. I'm not sure how we can provide useful\n> > feedback for both remote helper authors and end-users though.\n> \n> This \"refspecs\" only come via the helper and not directly from the\n> end user, no?\n> \n> If that is the case, I do not think \"confusing\" is much of an issue.\n> Not _(\"localizing\") is also the right thing to do. We may want to\n> say \"BUG: \" at front to clarify it is not the end-user's fault, but\n> a problem they may want to report. If we at this point know what\n> helper attempted export without giving refspecs, it may help to show\n> it here, so that developers will know with what helper the user\n> had problems with.\n\nI like this idea. Perhaps we should say \"Bug in remote helper; refspec\nneeded with export\", so that it clearly indicates to both end-users and\nremote helper authors that the error is in the remote helper.\n"},{"id":"214751","messageId":"CAMP44s1NRMbk2DpgyWqnyEKV+WkxAg1Gjn3UZQKMopJUDBZ4jw@mail.gmail.com","threadId":"33530","inReplyTo":"7vhaj37nr5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/6] transport-helper: clarify *:* refspec","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T19:07:28Z","receivedAt":"2013-04-18T19:07:28Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Apr 18, 2013 at 12:27 PM, Junio C Hamano 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>> 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> I presume\n>\n> s/.$/, but `*:*` does not work so do not use it./\n>\n> is implied?\n\nI believe\n\n s/.$/, but you should specify an apropriate one as described above./\n\nWould be better.\n\n>> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\n>> index 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>> compare_refs local2 HEAD server HEAD\n>> '\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> This one made me raise my eyebrows, first.\n>\n> The only reason this test \"passes\" is because this particular\n> \"pull\", due to what local2 and server happens to have before this\n> test runs, is an \"Already up-to-date\" and a no-op. You are removing\n> this because it is not really testing anything useful, and if you\n> change it to test something real, e.g. by rewinding local2, it will\n> reveal the breakage.\n>\n> Am I reading you correctly?\n\nNot really. This particular fetch does work, and it's tricky to\nexplain all the reasons why, even if you update the server ref before\nfetching. The reason it's written this way is that it comes from from\na branch of mine that has a hack to make the push above works, and\nsince the transport-helper's push doesn't update the ref in this\nversion, the test did actually do something and was working.\n\nI am removing the test because we don't care if it works or not,\nremote-helpers should not be doing that. The fact that the test wasn't\nactually doing anything is icing on the cake.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"214752","messageId":"CAMP44s3329nhkpekg4szbYZBuKUNmVScx1uFWaNm+Xg_0=s86w@mail.gmail.com","threadId":"33530","inReplyTo":"7vvc7j6917.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 4/6] transport-helper: warn when refspec is not used","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-18T19:12:09Z","receivedAt":"2013-04-18T19:12:09Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Apr 18, 2013 at 12:30 PM, Junio C Hamano wrote:\n> Felipe Contreras writes:\n>\n>> For the modes that need it. In the future we should probably error out,\n>> instead of providing half-assed support.\n>>\n>> The reason we want to do this is because if it's not present, the remote\n>> helper might be updating refs/heads/*, or refs/remotes/origin/*,\n>> directly, and in the process fetch will get confused trying to update\n>> refs that are already updated, or older than what they should be. We\n>> shouldn't be messing with the rest of git.\n>\n> So that answers my question in the response to an earlier one in\n> this series. We expect the ref updates to be done by the fetch or\n> push that drives the helper, and do not want the helper to interfere\n> with its ref updates.\n>\n> So it is not just 'refspec' _allows_ the refs to be constrained to a\n> private namespace, like the earlier updates made the documentation\n> say; it _is_ mandatory to use refspecs to constrain them to avoid\n> touching refs/heads and refs/remotes namespace.\n\nYeah, it was not stated that it was mandatory, but I think it's in\neverybody's best interest that it is.\n\n-- \nFelipe Contreras\n"},{"id":"214789","messageId":"CAPig+cRp7J+HYBA=2OL-CdA3NiQFjuUMZEjE+i+SnNrrPBAZ6Q@mail.gmail.com","threadId":"33530","inReplyTo":"1366258473-12841-4-git-send-email-felipe.contreras@gmail.com","subject":"Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-04-19T00:27:45Z","receivedAt":"2013-04-19T00:27:45Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Apr 18, 2013 at 12:14 AM, Felipe Contreras\n wrote:\n> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\n> index 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> compare_refs local2 HEAD server HEAD\n> '\n>\n> -test_expect_failure 'pushing without refspecs' '\n> +test_expect_success 'pushing without refspecs' '\n> test_when_finished \"(cd local2 && git reset --hard origin)\" &&\n> (cd local2 &&\n> echo content >>file &&\n> git commit -a -m ten &&\n> - GIT_REMOTE_TESTGIT_REFSPEC=\"\" git push) &&\n> - compare_refs local2 HEAD server HEAD\n> + GIT_REMOTE_TESTGIT_REFSPEC=\"\" test_must_fail git push 2> ../error) &&\n> + grep \"remote-helper doesn.t support push; refspec needed\" error\n\nIs \"doesn.t\" intentional? It certainly works by accident in grep, but\ndid you mean s/doesn.t/doesn't/ ?\n\n> '\n>\n> test_expect_success 'pulling without marks' '\n"},{"id":"214790","messageId":"CAMP44s0bQ588w91aX9Df-89L+2Ax4X0caB+Uh-C-_B4=JX=G8A@mail.gmail.com","threadId":"33530","inReplyTo":"CAPig+cRp7J+HYBA=2OL-CdA3NiQFjuUMZEjE+i+SnNrrPBAZ6Q@mail.gmail.com","subject":"Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2013-04-19T00:30:34Z","receivedAt":"2013-04-19T00:30:34Z","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:27 PM, Eric Sunshine wrote:\n> On Thu, Apr 18, 2013 at 12:14 AM, Felipe Contreras\n> wrote:\n>> diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\n>> index 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>> compare_refs local2 HEAD server HEAD\n>> '\n>>\n>> -test_expect_failure 'pushing without refspecs' '\n>> +test_expect_success 'pushing without refspecs' '\n>> test_when_finished \"(cd local2 && git reset --hard origin)\" &&\n>> (cd local2 &&\n>> echo content >>file &&\n>> git commit -a -m ten &&\n>> - GIT_REMOTE_TESTGIT_REFSPEC=\"\" git push) &&\n>> - compare_refs local2 HEAD server HEAD\n>> + GIT_REMOTE_TESTGIT_REFSPEC=\"\" test_must_fail git push 2> ../error) &&\n>> + grep \"remote-helper doesn.t support push; refspec needed\" error\n>\n> Is \"doesn.t\" intentional? It certainly works by accident in grep, but\n> did you mean s/doesn.t/doesn't/ ?\n\nIn all git test cases we are already inside single quotes; can't do that.\n\n-- \nFelipe Contreras\n"},{"id":"214793","messageId":"7v4nf34276.fsf@alter.siamese.dyndns.org","threadId":"33530","inReplyTo":"CAPig+cRp7J+HYBA=2OL-CdA3NiQFjuUMZEjE+i+SnNrrPBAZ6Q@mail.gmail.com","subject":"Re: [PATCH v2 3/6] transport-helper: clarify pushing without refspecs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-19T03:41:17Z","receivedAt":"2013-04-19T03:41:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> + grep \"remote-helper doesn.t support push; refspec needed\" error\n>\n> Is \"doesn.t\" intentional? It certainly works by accident in grep, but\n> did you mean s/doesn.t/doesn't/ ?\n\nThe pattern matching the expected string is not by accident, but by\ndesign.\n\nIt of course can be made more strict to reject \"doesnot\" and require\n\"doesn't\" by doing something like this:\n\n grep \"remote-helper doesn'\\''t support push; refspec needed\" error\n\nbut at some point, it simply stops being worth it to tighten the\npattern.\n\nFor that matter, it could be as loose as\n\n\tgrep \"support push; refspec needed\" error\n\nif you know the string is unique enough.\n"}]}