Re: [PATCH 1/3] transport-helper: emit progress and verbosity options after asking for capabilities

[Junio C Hamano <gitster@pobox.com>]

Mike Hommey <mh@glandium.org> 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 <mh@glandium.org>
> ---

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);