Re: [PATCH v2 15/20] transport-helper: use the new done feature where possible

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Sverre Rabbelier wrote:

> In other words, use fast-export --use-done-feature to add a 'done'
> command at the end of streams passed to remote helpers' "import"
> commands, and teach the remote helpers implementing "export" to use
> the 'done' command in turn when producing their streams.

Sounds like a reasonable thing to do.  One puzzle:

[...]
> +++ b/transport-helper.c
> @@ -375,8 +375,9 @@ static int get_exporter(struct transport *transport,
>  	/* we need to duplicate helper->in because we want to use it after
>  	 * fastexport is done with it. */
>  	fastexport->out = dup(helper->in);
> -	fastexport->argv = xcalloc(4 + revlist_args->nr, sizeof(*fastexport->argv));
> +	fastexport->argv = xcalloc(5 + revlist_args->nr, sizeof(*fastexport->argv));
>  	fastexport->argv[argc++] = "fast-export";
> +	fastexport->argv[argc++] = "--use-done-feature";
>  	if (export_marks)
>  		fastexport->argv[argc++] = export_marks;
>  	if (import_marks)
> @@ -412,11 +413,8 @@ static int fetch_with_import(struct transport *transport,
>  		sendline(data, &buf);
>  		strbuf_reset(&buf);
>  	}
> -	if (disconnect_helper(transport))
> -		die("Error while disconnecting helper");
>  	if (finish_command(&fastimport))
>  		die("Error while running fast-import");
> -

What is this change about?  Is the plan to allow other commands after
a fetch_with_import?  Sounds reasonable; I think it should be
advertised in the log message, though.

When does the disconnect_helper call happen (to avoid leaks)?  Ah, in
release_helper; phew.

The disconnect_helper call writes the blank line that terminates the
list of "import %s" commands to start the import, so there would need
to be a

	strbuf_reset(&buf);
	strbuf_addf(&buf, "\n");
	sendline(data, &buf);

in its place.