{"thread":{"id":"38557","subject":"[PATCH 1/3] transport-helper: emit progress and verbosity options after asking for capabilities","startedAt":"2015-02-12T10:09:59Z","lastAt":"2015-02-14T08:12:23Z","messageCount":11,"participants":["Mike Hommey","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"255964","messageId":"1423735801-11108-1-git-send-email-mh@glandium.org","threadId":"38557","inReplyTo":null,"subject":"[PATCH 1/3] transport-helper: emit progress and verbosity options after asking for capabilities","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-02-12T10:09:59Z","receivedAt":"2015-02-12T10:09:59Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Currently, the progress and verbosity options are only emitted for the fetch\nand push commands, but they should also be emitted for other commands, such as\nimport or export, and, why not, even list.\n\nSigned-off-by: Mike Hommey \n---\n transport-helper.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 0224687..23a741c 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -98,6 +98,8 @@ static void do_take_over(struct transport *transport)\n \tfree(data);\n }\n \n+static void standard_options(struct transport *t);\n+\n static struct child_process *get_helper(struct transport *transport)\n {\n \tstruct helper_data *data = transport->data;\n@@ -212,6 +214,7 @@ static struct child_process *get_helper(struct transport *transport)\n \tstrbuf_release(&buf);\n \tif (debug)\n \t\tfprintf(stderr, \"Debug: Capabilities complete.\\n\");\n+\tstandard_options(transport);\n \treturn data->helper;\n }\n \n@@ -339,7 +342,6 @@ static int fetch_with_fetch(struct transport *transport,\n \tint i;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tstandard_options(transport);\n \tif (data->check_connectivity &&\n \t data->transport_options.check_self_contained_and_connected)\n \t\tset_helper_option(transport, \"check-connectivity\", \"true\");\n@@ -824,7 +826,6 @@ static int push_refs_with_push(struct transport *transport,\n \t\treturn 0;\n \t}\n \n-\tstandard_options(transport);\n \tfor_each_string_list_item(cas_option, &cas_options)\n \t\tset_helper_option(transport, \"cas\", cas_option->string);\n \n-- \n2.3.0.1.g55ac403.dirty\n"},{"id":"255966","messageId":"1423735801-11108-2-git-send-email-mh@glandium.org","threadId":"38557","inReplyTo":"1423735801-11108-1-git-send-email-mh@glandium.org","subject":"[PATCH 2/3] transport-helper: emit check-connectivity, cloning, and update-shallow options for import","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-02-12T10:10:00Z","receivedAt":"2015-02-12T10:10:00Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"The check-connectivity, cloning, and update-shallow options are currently only\nemitted for the fetch command but should also be emitted for the import\ncommand.\n\nSigned-off-by: Mike Hommey \n---\n transport-helper.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 23a741c..c3868e4 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -342,16 +342,6 @@ static int fetch_with_fetch(struct transport *transport,\n \tint i;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tif (data->check_connectivity &&\n-\t data->transport_options.check_self_contained_and_connected)\n-\t\tset_helper_option(transport, \"check-connectivity\", \"true\");\n-\n-\tif (transport->cloning)\n-\t\tset_helper_option(transport, \"cloning\", \"true\");\n-\n-\tif (data->transport_options.update_shallow)\n-\t\tset_helper_option(transport, \"update-shallow\", \"true\");\n-\n \tfor (i = 0; i < nr_heads; i++) {\n \t\tconst struct ref *posn = to_fetch[i];\n \t\tif (posn->status & REF_STATUS_UPTODATE)\n@@ -622,6 +612,16 @@ static int fetch(struct transport *transport,\n \tif (!count)\n \t\treturn 0;\n \n+\tif (data->check_connectivity &&\n+\t data->transport_options.check_self_contained_and_connected)\n+\t\tset_helper_option(transport, \"check-connectivity\", \"true\");\n+\n+\tif (transport->cloning)\n+\t\tset_helper_option(transport, \"cloning\", \"true\");\n+\n+\tif (data->transport_options.update_shallow)\n+\t\tset_helper_option(transport, \"update-shallow\", \"true\");\n+\n \tif (data->fetch)\n \t\treturn fetch_with_fetch(transport, nr_heads, to_fetch);\n \n-- \n2.3.0.1.g55ac403.dirty\n"},{"id":"255965","messageId":"1423735801-11108-3-git-send-email-mh@glandium.org","threadId":"38557","inReplyTo":"1423735801-11108-1-git-send-email-mh@glandium.org","subject":"[PATCH 3/3] transport-helper: fix typo in error message when --signed is not supported","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-02-12T10:10:01Z","receivedAt":"2015-02-12T10:10:01Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Signed-off-by: Mike Hommey \n---\n transport-helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex c3868e4..b72fd4a 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -861,7 +861,7 @@ static int push_refs_with_export(struct transport *transport,\n \t\t\tdie(\"helper %s does not support dry-run\", data->name);\n \t} else if (flags & TRANSPORT_PUSH_CERT) {\n \t\tif (set_helper_option(transport, TRANS_OPT_PUSH_CERT, \"true\") != 0)\n-\t\t\tdie(\"helper %s does not support dry-run\", data->name);\n+\t\t\tdie(\"helper %s does not support --signed\", data->name);\n \t}\n \n \tif (flags & TRANSPORT_PUSH_FORCE) {\n-- \n2.3.0.1.g55ac403.dirty\n"},{"id":"255967","messageId":"20150212102048.GA25303@glandium.org","threadId":"38557","inReplyTo":"1423735801-11108-3-git-send-email-mh@glandium.org","subject":"Re: [PATCH 3/3] transport-helper: fix typo in error message when --signed is not supported","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-02-12T10:20:48Z","receivedAt":"2015-02-12T10:20:48Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Thu, Feb 12, 2015 at 07:10:01PM +0900, Mike Hommey wrote:\n> Signed-off-by: Mike Hommey \n> ---\n> transport-helper.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/transport-helper.c b/transport-helper.c\n> index c3868e4..b72fd4a 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -861,7 +861,7 @@ static int push_refs_with_export(struct transport *transport,\n> \t\t\tdie(\"helper %s does not support dry-run\", data->name);\n> \t} else if (flags & TRANSPORT_PUSH_CERT) {\n> \t\tif (set_helper_option(transport, TRANS_OPT_PUSH_CERT, \"true\") != 0)\n\nNote that this TRANS_OPT_PUSH_CERT option is not documented in\nDocumentation/gitremote-helpers.txt.\n\nMike\n"},{"id":"256017","messageId":"xmqqlhk2ri1j.fsf@gitster.dls.corp.google.com","threadId":"38557","inReplyTo":"1423735801-11108-1-git-send-email-mh@glandium.org","subject":"Re: [PATCH 1/3] transport-helper: emit progress and verbosity options after asking for capabilities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-12T20:28:24Z","receivedAt":"2015-02-12T20:28:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> Currently, the progress and verbosity options are only emitted for the fetch\n> and push commands, but they should also be emitted for other commands, such as\n> import or export, and, why not, even list.\n>\n> Signed-off-by: Mike Hommey \n> ---\n\nI had a hard time understanding what you are trying to achieve. The\nword \"emit\" may be techinically correct from the point of view of\nthis program, but wouldn't there be a better word to describe what\nis being achieved by this program \"emit\"ting the additional output\nlines?\n\nPerhaps \"ask the helper to enable progress and verbosity capabilities\"\nor somesuch?\n\nThey \"should also be emitted\" may also want to be justified in a\nbetter way, by describing what _bad_ things the current code that\nlacks this patch is doing as a bug.\n\nPerhaps like \"The current transport-helper.c code does not tell a\nremote helper that uses the import and export interface to enable\nprogress and verbosity capabilities, even if the helper supports\nthem.\"\n\nIs there a downside of doing this? I do not think of any offhand.\n\nThanks.\n\n> transport-helper.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 0224687..23a741c 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -98,6 +98,8 @@ static void do_take_over(struct transport *transport)\n> \tfree(data);\n> }\n> \n> +static void standard_options(struct transport *t);\n> +\n> static struct child_process *get_helper(struct transport *transport)\n> {\n> \tstruct helper_data *data = transport->data;\n> @@ -212,6 +214,7 @@ static struct child_process *get_helper(struct transport *transport)\n> \tstrbuf_release(&buf);\n> \tif (debug)\n> \t\tfprintf(stderr, \"Debug: Capabilities complete.\\n\");\n> +\tstandard_options(transport);\n> \treturn data->helper;\n> }\n> \n> @@ -339,7 +342,6 @@ static int fetch_with_fetch(struct transport *transport,\n> \tint i;\n> \tstruct strbuf buf = STRBUF_INIT;\n> \n> -\tstandard_options(transport);\n> \tif (data->check_connectivity &&\n> \t data->transport_options.check_self_contained_and_connected)\n> \t\tset_helper_option(transport, \"check-connectivity\", \"true\");\n> @@ -824,7 +826,6 @@ static int push_refs_with_push(struct transport *transport,\n> \t\treturn 0;\n> \t}\n> \n> -\tstandard_options(transport);\n> \tfor_each_string_list_item(cas_option, &cas_options)\n> \t\tset_helper_option(transport, \"cas\", cas_option->string);\n"},{"id":"256019","messageId":"xmqqh9uqrhqs.fsf@gitster.dls.corp.google.com","threadId":"38557","inReplyTo":"1423735801-11108-3-git-send-email-mh@glandium.org","subject":"Re: [PATCH 3/3] transport-helper: fix typo in error message when --signed is not supported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-12T20:34:51Z","receivedAt":"2015-02-12T20:34:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Good eyes.\n\nThis one is a straight-forward bugfix for 0ea47f9d (signed push:\nteach smart-HTTP to pass \"git push --signed\" around, 2014-09-15);\nwill queue directly to jc/push-cert branch and will merge to\nmaintenance tracks independent from the other two.\n\nThanks.\n"},{"id":"256049","messageId":"1423805086-807-1-git-send-email-mh@glandium.org","threadId":"38557","inReplyTo":"xmqqlhk2ri1j.fsf@gitster.dls.corp.google.com","subject":"[PATCH 1/2] transport-helper: ask the helper to set progress and verbosity options after asking for its capabilities","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-02-13T05:24:45Z","receivedAt":"2015-02-13T05:24:45Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Currently, a remote helper is only told about the progress and verbosity\noptions for the 'fetch' and 'push' commands. This means a remote helper\nthat implements 'import' and 'export' can never know the user requested\nprogress or verbosity (or lack thereof) through the command line.\n\nTelling the remote helper about those options after asking for its\ncapabilities ensures it can act accordingly for all commands.\n\nSigned-off-by: Mike Hommey \n---\n transport-helper.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 0224687..23a741c 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -98,6 +98,8 @@ static void do_take_over(struct transport *transport)\n \tfree(data);\n }\n \n+static void standard_options(struct transport *t);\n+\n static struct child_process *get_helper(struct transport *transport)\n {\n \tstruct helper_data *data = transport->data;\n@@ -212,6 +214,7 @@ static struct child_process *get_helper(struct transport *transport)\n \tstrbuf_release(&buf);\n \tif (debug)\n \t\tfprintf(stderr, \"Debug: Capabilities complete.\\n\");\n+\tstandard_options(transport);\n \treturn data->helper;\n }\n \n@@ -339,7 +342,6 @@ static int fetch_with_fetch(struct transport *transport,\n \tint i;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tstandard_options(transport);\n \tif (data->check_connectivity &&\n \t data->transport_options.check_self_contained_and_connected)\n \t\tset_helper_option(transport, \"check-connectivity\", \"true\");\n@@ -824,7 +826,6 @@ static int push_refs_with_push(struct transport *transport,\n \t\treturn 0;\n \t}\n \n-\tstandard_options(transport);\n \tfor_each_string_list_item(cas_option, &cas_options)\n \t\tset_helper_option(transport, \"cas\", cas_option->string);\n \n-- \n2.3.0.3.g5a196f5\n"},{"id":"256048","messageId":"1423805086-807-2-git-send-email-mh@glandium.org","threadId":"38557","inReplyTo":"1423805086-807-1-git-send-email-mh@glandium.org","subject":"[PATCH 2/2] transport-helper: ask the helper to set the same options for import as for fetch","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-02-13T05:24:46Z","receivedAt":"2015-02-13T05:24:46Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"A remote helper is currently only told about the 'check-connectivity',\n'cloning', and 'update-shallow' options when it supports the 'fetch'\ncommand, but not when it supports 'import' instead.\n\nThis is especially important for the 'cloning' option, because it\nmeans a remote helper that only supports 'import' can't distinguish\nbetween a clone and a pull besides doing some assumptions from the\ngit directory state.\n\nSigned-off-by: Mike Hommey \n---\n transport-helper.c | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 23a741c..c3868e4 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -342,16 +342,6 @@ static int fetch_with_fetch(struct transport *transport,\n \tint i;\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tif (data->check_connectivity &&\n-\t data->transport_options.check_self_contained_and_connected)\n-\t\tset_helper_option(transport, \"check-connectivity\", \"true\");\n-\n-\tif (transport->cloning)\n-\t\tset_helper_option(transport, \"cloning\", \"true\");\n-\n-\tif (data->transport_options.update_shallow)\n-\t\tset_helper_option(transport, \"update-shallow\", \"true\");\n-\n \tfor (i = 0; i < nr_heads; i++) {\n \t\tconst struct ref *posn = to_fetch[i];\n \t\tif (posn->status & REF_STATUS_UPTODATE)\n@@ -622,6 +612,16 @@ static int fetch(struct transport *transport,\n \tif (!count)\n \t\treturn 0;\n \n+\tif (data->check_connectivity &&\n+\t data->transport_options.check_self_contained_and_connected)\n+\t\tset_helper_option(transport, \"check-connectivity\", \"true\");\n+\n+\tif (transport->cloning)\n+\t\tset_helper_option(transport, \"cloning\", \"true\");\n+\n+\tif (data->transport_options.update_shallow)\n+\t\tset_helper_option(transport, \"update-shallow\", \"true\");\n+\n \tif (data->fetch)\n \t\treturn fetch_with_fetch(transport, nr_heads, to_fetch);\n \n-- \n2.3.0.3.g5a196f5\n"},{"id":"256076","messageId":"xmqqwq3lmwnb.fsf@gitster.dls.corp.google.com","threadId":"38557","inReplyTo":"1423805086-807-2-git-send-email-mh@glandium.org","subject":"Re: [PATCH 2/2] transport-helper: ask the helper to set the same options for import as for fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-13T19:36:24Z","receivedAt":"2015-02-13T19:36:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> A remote helper is currently only told about the 'check-connectivity',\n> 'cloning', and 'update-shallow' options when it supports the 'fetch'\n> command, but not when it supports 'import' instead.\n\nSounds sensible.\n\nDoes the same issue exist for export vs push or do they happen to be\ncoded to pass similar enough set of options already by copied and\npasted code?\n"},{"id":"256095","messageId":"20150213221450.GA24285@glandium.org","threadId":"38557","inReplyTo":"xmqqwq3lmwnb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] transport-helper: ask the helper to set the same options for import as for fetch","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2015-02-13T22:14:50Z","receivedAt":"2015-02-13T22:14:50Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Fri, Feb 13, 2015 at 11:36:24AM -0800, Junio C Hamano wrote:\n> Mike Hommey writes:\n> \n> > A remote helper is currently only told about the 'check-connectivity',\n> > 'cloning', and 'update-shallow' options when it supports the 'fetch'\n> > command, but not when it supports 'import' instead.\n> \n> Sounds sensible.\n> \n> Does the same issue exist for export vs push or do they happen to be\n> coded to pass similar enough set of options already by copied and\n> pasted code?\n\nThe issue exists:\n- export is given dry-run, pushcert and force.\n- push is given cas, dry-run and pushcert.\n\n(note: cas and pushcert are both not documented in\ngitremote-helpers.txt)\n\nForce is actually not necessary for push, because the push syntax itself\nincludes the force instruction in the refspec given as argument.\n\nI haven't looked exactly what cas does and if it makes sense for export.\n(FWIW, I'm using push and import at the moment, so it's not a direct\nissue for me ; I don't support cas anyways)\n\nMike\n"},{"id":"256100","messageId":"xmqqoaowlxnc.fsf@gitster.dls.corp.google.com","threadId":"38557","inReplyTo":"20150213221450.GA24285@glandium.org","subject":"Re: [PATCH 2/2] transport-helper: ask the helper to set the same options for import as for fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-14T08:12:23Z","receivedAt":"2015-02-14T08:12:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> I haven't looked exactly what cas does and if it makes sense for export.\n> (FWIW, I'm using push and import at the moment, so it's not a direct\n> issue for me ; I don't support cas anyways)\n\nThe question primarily came from curiosity to gauge how much\npotential work remains in the area (of course, people are welcome to\nshare the curiosity and get motivated to fill the gaps as\ndiscovered).\n\nIn other words, I didn't mean to say \"complete other methods in\ntransport helper while at it; otherwise your patches are incomplete\nand unacceptable\". It is better to concentrate what you use whose\ndesired behaviour you are sure about, and these two patches do\nexactly that.\n\nThanks.\n"}]}