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

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

[Mike Hommey]

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

[PATCH 2/3] transport-helper: emit check-connectivity, cloning, and update-shallow options for import

[Mike Hommey]

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

[PATCH 3/3] transport-helper: fix typo in error message when --signed is not supported

[Mike Hommey]

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 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

Re: [PATCH 3/3] transport-helper: fix typo in error message when --signed is not supported

[Mike Hommey]

On Thu, Feb 12, 2015 at 07:10:01PM +0900, Mike Hommey wrote:
> Signed-off-by: Mike Hommey <mh@glandium.org>
> ---
>  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

Re: [PATCH 3/3] transport-helper: fix typo in error message when --signed is not supported

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

[PATCH 1/2] transport-helper: ask the helper to set progress and verbosity options after asking for its capabilities

[Mike Hommey]

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

[PATCH 2/2] transport-helper: ask the helper to set the same options for import as for fetch

[Mike Hommey]

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

Re: [PATCH 2/2] transport-helper: ask the helper to set the same options for import as for fetch

[Junio C Hamano]

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

Re: [PATCH 2/2] transport-helper: ask the helper to set the same options for import as for fetch

[Mike Hommey]

On Fri, Feb 13, 2015 at 11:36:24AM -0800, Junio C Hamano wrote:
> Mike Hommey <mh@glandium.org> 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

Re: [PATCH 2/2] transport-helper: ask the helper to set the same options for import as for fetch

[Junio C Hamano]

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