# [PATCH 1/3] transport-helper: emit progress and verbosity options after asking for capabilities 11 messages from 2015-02-12 to 2015-02-14. Participants: Mike Hommey, Junio C Hamano. Thread: https://gitlist.dev/t/38557 ## Mike Hommey, 2015-02-12 10:09 Subject: [PATCH 1/3] transport-helper: emit progress and verbosity options after asking for capabilities Message-ID: <1423735801-11108-1-git-send-email-mh@glandium.org> URL: https://gitlist.dev/e/1423735801-11108-1-git-send-email-mh%40glandium.org ``` Currently, the progress and verbosity options are only emitted for the fetch and push commands, but they should also be emitted for other commands, such as import or export, and, why not, even list. Signed-off-by: Mike Hommey --- transport-helper.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/transport-helper.c b/transport-helper.c index 0224687..23a741c 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -98,6 +98,8 @@ static void do_take_over(struct transport *transport) free(data); } +static void standard_options(struct transport *t); + static struct child_process *get_helper(struct transport *transport) { struct helper_data *data = transport->data; @@ -212,6 +214,7 @@ static struct child_process *get_helper(struct transport *transport) strbuf_release(&buf); if (debug) fprintf(stderr, "Debug: Capabilities complete.\n"); + standard_options(transport); return data->helper; } @@ -339,7 +342,6 @@ static int fetch_with_fetch(struct transport *transport, int i; struct strbuf buf = STRBUF_INIT; - standard_options(transport); if (data->check_connectivity && data->transport_options.check_self_contained_and_connected) set_helper_option(transport, "check-connectivity", "true"); @@ -824,7 +826,6 @@ static int push_refs_with_push(struct transport *transport, return 0; } - standard_options(transport); for_each_string_list_item(cas_option, &cas_options) set_helper_option(transport, "cas", cas_option->string); -- 2.3.0.1.g55ac403.dirty ``` ## Mike Hommey, 2015-02-12 10:10 Subject: [PATCH 2/3] transport-helper: emit check-connectivity, cloning, and update-shallow options for import Message-ID: <1423735801-11108-2-git-send-email-mh@glandium.org> URL: https://gitlist.dev/e/1423735801-11108-2-git-send-email-mh%40glandium.org In-Reply-To: <1423735801-11108-1-git-send-email-mh@glandium.org> ``` The check-connectivity, cloning, and update-shallow options are currently only emitted for the fetch command but should also be emitted for the import command. Signed-off-by: Mike Hommey --- transport-helper.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/transport-helper.c b/transport-helper.c index 23a741c..c3868e4 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -342,16 +342,6 @@ static int fetch_with_fetch(struct transport *transport, int i; struct strbuf buf = STRBUF_INIT; - if (data->check_connectivity && - data->transport_options.check_self_contained_and_connected) - set_helper_option(transport, "check-connectivity", "true"); - - if (transport->cloning) - set_helper_option(transport, "cloning", "true"); - - if (data->transport_options.update_shallow) - set_helper_option(transport, "update-shallow", "true"); - for (i = 0; i < nr_heads; i++) { const struct ref *posn = to_fetch[i]; if (posn->status & REF_STATUS_UPTODATE) @@ -622,6 +612,16 @@ static int fetch(struct transport *transport, if (!count) return 0; + if (data->check_connectivity && + data->transport_options.check_self_contained_and_connected) + set_helper_option(transport, "check-connectivity", "true"); + + if (transport->cloning) + set_helper_option(transport, "cloning", "true"); + + if (data->transport_options.update_shallow) + set_helper_option(transport, "update-shallow", "true"); + if (data->fetch) return fetch_with_fetch(transport, nr_heads, to_fetch); -- 2.3.0.1.g55ac403.dirty ``` ## Mike Hommey, 2015-02-12 10:10 Subject: [PATCH 3/3] transport-helper: fix typo in error message when --signed is not supported Message-ID: <1423735801-11108-3-git-send-email-mh@glandium.org> URL: https://gitlist.dev/e/1423735801-11108-3-git-send-email-mh%40glandium.org In-Reply-To: <1423735801-11108-1-git-send-email-mh@glandium.org> ``` Signed-off-by: Mike Hommey --- transport-helper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/transport-helper.c b/transport-helper.c index c3868e4..b72fd4a 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -861,7 +861,7 @@ static int push_refs_with_export(struct transport *transport, die("helper %s does not support dry-run", data->name); } else if (flags & TRANSPORT_PUSH_CERT) { if (set_helper_option(transport, TRANS_OPT_PUSH_CERT, "true") != 0) - die("helper %s does not support dry-run", data->name); + die("helper %s does not support --signed", data->name); } if (flags & TRANSPORT_PUSH_FORCE) { -- 2.3.0.1.g55ac403.dirty ``` ## Mike Hommey, 2015-02-12 10:20 Subject: Re: [PATCH 3/3] transport-helper: fix typo in error message when --signed is not supported Message-ID: <20150212102048.GA25303@glandium.org> URL: https://gitlist.dev/e/20150212102048.GA25303%40glandium.org In-Reply-To: <1423735801-11108-3-git-send-email-mh@glandium.org> ``` On Thu, Feb 12, 2015 at 07:10:01PM +0900, Mike Hommey wrote: > Signed-off-by: Mike Hommey > --- > transport-helper.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/transport-helper.c b/transport-helper.c > index c3868e4..b72fd4a 100644 > --- a/transport-helper.c > +++ b/transport-helper.c > @@ -861,7 +861,7 @@ static int push_refs_with_export(struct transport *transport, > die("helper %s does not support dry-run", data->name); > } else if (flags & TRANSPORT_PUSH_CERT) { > if (set_helper_option(transport, TRANS_OPT_PUSH_CERT, "true") != 0) Note that this TRANS_OPT_PUSH_CERT option is not documented in Documentation/gitremote-helpers.txt. Mike ``` ## Junio C Hamano, 2015-02-12 20:28 Subject: Re: [PATCH 1/3] transport-helper: emit progress and verbosity options after asking for capabilities Message-ID: URL: https://gitlist.dev/e/xmqqlhk2ri1j.fsf%40gitster.dls.corp.google.com In-Reply-To: <1423735801-11108-1-git-send-email-mh@glandium.org> ``` Mike Hommey writes: > Currently, the progress and verbosity options are only emitted for the fetch > and push commands, but they should also be emitted for other commands, such as > import or export, and, why not, even list. > > Signed-off-by: Mike Hommey > --- I had a hard time understanding what you are trying to achieve. The word "emit" may be techinically correct from the point of view of this program, but wouldn't there be a better word to describe what is being achieved by this program "emit"ting the additional output lines? Perhaps "ask the helper to enable progress and verbosity capabilities" or somesuch? They "should also be emitted" may also want to be justified in a better way, by describing what _bad_ things the current code that lacks this patch is doing as a bug. Perhaps like "The current transport-helper.c code does not tell a remote helper that uses the import and export interface to enable progress and verbosity capabilities, even if the helper supports them." Is there a downside of doing this? I do not think of any offhand. Thanks. > transport-helper.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/transport-helper.c b/transport-helper.c > index 0224687..23a741c 100644 > --- a/transport-helper.c > +++ b/transport-helper.c > @@ -98,6 +98,8 @@ static void do_take_over(struct transport *transport) > free(data); > } > > +static void standard_options(struct transport *t); > + > static struct child_process *get_helper(struct transport *transport) > { > struct helper_data *data = transport->data; > @@ -212,6 +214,7 @@ static struct child_process *get_helper(struct transport *transport) > strbuf_release(&buf); > if (debug) > fprintf(stderr, "Debug: Capabilities complete.\n"); > + standard_options(transport); > return data->helper; > } > > @@ -339,7 +342,6 @@ static int fetch_with_fetch(struct transport *transport, > int i; > struct strbuf buf = STRBUF_INIT; > > - standard_options(transport); > if (data->check_connectivity && > data->transport_options.check_self_contained_and_connected) > set_helper_option(transport, "check-connectivity", "true"); > @@ -824,7 +826,6 @@ static int push_refs_with_push(struct transport *transport, > return 0; > } > > - standard_options(transport); > for_each_string_list_item(cas_option, &cas_options) > set_helper_option(transport, "cas", cas_option->string); ``` ## Junio C Hamano, 2015-02-12 20:34 Subject: Re: [PATCH 3/3] transport-helper: fix typo in error message when --signed is not supported Message-ID: URL: https://gitlist.dev/e/xmqqh9uqrhqs.fsf%40gitster.dls.corp.google.com In-Reply-To: <1423735801-11108-3-git-send-email-mh@glandium.org> ``` Good eyes. This one is a straight-forward bugfix for 0ea47f9d (signed push: teach smart-HTTP to pass "git push --signed" around, 2014-09-15); will queue directly to jc/push-cert branch and will merge to maintenance tracks independent from the other two. Thanks. ``` ## Mike Hommey, 2015-02-13 05:24 Subject: [PATCH 1/2] transport-helper: ask the helper to set progress and verbosity options after asking for its capabilities Message-ID: <1423805086-807-1-git-send-email-mh@glandium.org> URL: https://gitlist.dev/e/1423805086-807-1-git-send-email-mh%40glandium.org In-Reply-To: ``` Currently, a remote helper is only told about the progress and verbosity options for the 'fetch' and 'push' commands. This means a remote helper that implements 'import' and 'export' can never know the user requested progress or verbosity (or lack thereof) through the command line. Telling the remote helper about those options after asking for its capabilities ensures it can act accordingly for all commands. Signed-off-by: Mike Hommey --- transport-helper.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/transport-helper.c b/transport-helper.c index 0224687..23a741c 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -98,6 +98,8 @@ static void do_take_over(struct transport *transport) free(data); } +static void standard_options(struct transport *t); + static struct child_process *get_helper(struct transport *transport) { struct helper_data *data = transport->data; @@ -212,6 +214,7 @@ static struct child_process *get_helper(struct transport *transport) strbuf_release(&buf); if (debug) fprintf(stderr, "Debug: Capabilities complete.\n"); + standard_options(transport); return data->helper; } @@ -339,7 +342,6 @@ static int fetch_with_fetch(struct transport *transport, int i; struct strbuf buf = STRBUF_INIT; - standard_options(transport); if (data->check_connectivity && data->transport_options.check_self_contained_and_connected) set_helper_option(transport, "check-connectivity", "true"); @@ -824,7 +826,6 @@ static int push_refs_with_push(struct transport *transport, return 0; } - standard_options(transport); for_each_string_list_item(cas_option, &cas_options) set_helper_option(transport, "cas", cas_option->string); -- 2.3.0.3.g5a196f5 ``` ## Mike Hommey, 2015-02-13 05:24 Subject: [PATCH 2/2] transport-helper: ask the helper to set the same options for import as for fetch Message-ID: <1423805086-807-2-git-send-email-mh@glandium.org> URL: https://gitlist.dev/e/1423805086-807-2-git-send-email-mh%40glandium.org In-Reply-To: <1423805086-807-1-git-send-email-mh@glandium.org> ``` A remote helper is currently only told about the 'check-connectivity', 'cloning', and 'update-shallow' options when it supports the 'fetch' command, but not when it supports 'import' instead. This is especially important for the 'cloning' option, because it means a remote helper that only supports 'import' can't distinguish between a clone and a pull besides doing some assumptions from the git directory state. Signed-off-by: Mike Hommey --- transport-helper.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/transport-helper.c b/transport-helper.c index 23a741c..c3868e4 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -342,16 +342,6 @@ static int fetch_with_fetch(struct transport *transport, int i; struct strbuf buf = STRBUF_INIT; - if (data->check_connectivity && - data->transport_options.check_self_contained_and_connected) - set_helper_option(transport, "check-connectivity", "true"); - - if (transport->cloning) - set_helper_option(transport, "cloning", "true"); - - if (data->transport_options.update_shallow) - set_helper_option(transport, "update-shallow", "true"); - for (i = 0; i < nr_heads; i++) { const struct ref *posn = to_fetch[i]; if (posn->status & REF_STATUS_UPTODATE) @@ -622,6 +612,16 @@ static int fetch(struct transport *transport, if (!count) return 0; + if (data->check_connectivity && + data->transport_options.check_self_contained_and_connected) + set_helper_option(transport, "check-connectivity", "true"); + + if (transport->cloning) + set_helper_option(transport, "cloning", "true"); + + if (data->transport_options.update_shallow) + set_helper_option(transport, "update-shallow", "true"); + if (data->fetch) return fetch_with_fetch(transport, nr_heads, to_fetch); -- 2.3.0.3.g5a196f5 ``` ## Junio C Hamano, 2015-02-13 19:36 Subject: Re: [PATCH 2/2] transport-helper: ask the helper to set the same options for import as for fetch Message-ID: URL: https://gitlist.dev/e/xmqqwq3lmwnb.fsf%40gitster.dls.corp.google.com In-Reply-To: <1423805086-807-2-git-send-email-mh@glandium.org> ``` Mike Hommey writes: > A remote helper is currently only told about the 'check-connectivity', > 'cloning', and 'update-shallow' options when it supports the 'fetch' > command, but not when it supports 'import' instead. Sounds sensible. Does the same issue exist for export vs push or do they happen to be coded to pass similar enough set of options already by copied and pasted code? ``` ## Mike Hommey, 2015-02-13 22:14 Subject: Re: [PATCH 2/2] transport-helper: ask the helper to set the same options for import as for fetch Message-ID: <20150213221450.GA24285@glandium.org> URL: https://gitlist.dev/e/20150213221450.GA24285%40glandium.org In-Reply-To: ``` On Fri, Feb 13, 2015 at 11:36:24AM -0800, Junio C Hamano wrote: > Mike Hommey writes: > > > A remote helper is currently only told about the 'check-connectivity', > > 'cloning', and 'update-shallow' options when it supports the 'fetch' > > command, but not when it supports 'import' instead. > > Sounds sensible. > > Does the same issue exist for export vs push or do they happen to be > coded to pass similar enough set of options already by copied and > pasted code? The issue exists: - export is given dry-run, pushcert and force. - push is given cas, dry-run and pushcert. (note: cas and pushcert are both not documented in gitremote-helpers.txt) Force is actually not necessary for push, because the push syntax itself includes the force instruction in the refspec given as argument. I haven't looked exactly what cas does and if it makes sense for export. (FWIW, I'm using push and import at the moment, so it's not a direct issue for me ; I don't support cas anyways) Mike ``` ## Junio C Hamano, 2015-02-14 08:12 Subject: Re: [PATCH 2/2] transport-helper: ask the helper to set the same options for import as for fetch Message-ID: URL: https://gitlist.dev/e/xmqqoaowlxnc.fsf%40gitster.dls.corp.google.com In-Reply-To: <20150213221450.GA24285@glandium.org> ``` Mike Hommey writes: > I haven't looked exactly what cas does and if it makes sense for export. > (FWIW, I'm using push and import at the moment, so it's not a direct > issue for me ; I don't support cas anyways) The question primarily came from curiosity to gauge how much potential work remains in the area (of course, people are welcome to share the curiosity and get motivated to fill the gaps as discovered). In other words, I didn't mean to say "complete other methods in transport helper while at it; otherwise your patches are incomplete and unacceptable". It is better to concentrate what you use whose desired behaviour you are sure about, and these two patches do exactly that. Thanks. ```