threads / patch / 38557

patch, 3 partstransport-helper: emit progress and verbosity options after asking for capabilities

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

## tl;dr

11 messages between Feb 12, 2015 and Feb 14, 2015. Diffs are folded; open one to read it.

replies: 10people: 2as markdown or json

Mike Hommey· Feb 12, 2015, 10:09 UTC · lore

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(-)
Show changes to transport-helper.c +3 −2
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· Feb 12, 2015, 10:10 UTC · re: Mike Hommey · lore

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

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(-)
Show changes to transport-helper.c +10 −10
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· Feb 12, 2015, 10:10 UTC · re: Mike Hommey · lore

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

Signed-off-by: Mike Hommey <mh@glandium.org>
---
 transport-helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to transport-helper.c +1 −1
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· Feb 12, 2015, 10:20 UTC · re: Mike Hommey · lore

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

On Thu, Feb 12, 2015 at 07:10:01PM +0900, Mike Hommey wrote:
Show 13 quoted lines
> 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
Junio C Hamano· Feb 12, 2015, 20:34 UTC · re: Mike Hommey · lore

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

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.
Junio C Hamano· Feb 12, 2015, 20:28 UTC · re: Mike Hommey · lore

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

Mike Hommey <mh@glandium.org> writes:
Show 6 quoted lines
> 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.
Show 39 quoted lines
>  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);
Mike Hommey· Feb 13, 2015, 05:24 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to transport-helper.c +3 −2
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· Feb 13, 2015, 05:24 UTC · re: Mike Hommey · lore

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

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(-)
Show changes to transport-helper.c +10 −10
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· Feb 13, 2015, 19:36 UTC · re: Mike Hommey · lore

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

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?

Mike Hommey· Feb 13, 2015, 22:14 UTC · re: Junio C Hamano · lore

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

On Fri, Feb 13, 2015 at 11:36:24AM -0800, Junio C Hamano wrote:
Show 11 quoted lines
> 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
Junio C Hamano· Feb 14, 2015, 08:12 UTC · re: Mike Hommey · lore

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

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.

← back to recent threads