git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH/RFC v3 04/16] Connect fast-import to the remote-helper via pipe, adding 'bidi-import' capability.

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 14, 2012, 20:40 UTC
Message-ID
<7v4no59phn.fsf@alter.siamese.dyndns.org>
In-Reply-To
<1344971598-8213-5-git-send-email-florian.achleitner.2.6.31@gmail.com>
Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:
Show 5 quoted lines
> The fast-import commands 'cat-blob' and 'ls' can be used by remote-helpers
> to retrieve information about blobs and trees that already exist in
> fast-import's memory. This requires a channel from fast-import to the
> remote-helper.
> remote-helpers that use this features shall advertise the new 'bidi-import'
s/this fea/these fea/
> capability so signal that they require the communication channel.
s/so sig/to sig/, I think.
> When forking fast-import in transport-helper.c connect it to a dup of
> the remote-helper's stdin-pipe. The additional file descriptor is passed
> to fast-import via it's command line (--cat-blob-fd).
s/via it's/via its/;
Show 8 quoted lines
> It follows that git and fast-import are connected to the remote-helpers's
> stdin.
> Because git can send multiple commands to the remote-helper on it's stdin,
> it is required that helpers that advertise 'bidi-import' buffer all input
> commands until the batch of 'import' commands is ended by a newline
> before sending data to fast-import.
> This is to prevent mixing commands and fast-import responses on the
> helper's stdin.

Please have a blank line each between paragraphs; a solid block of text is very hard to follow.

Show 46 quoted lines
> Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> ---
>  transport-helper.c |   45 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index cfe0988..257274b 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -10,6 +10,7 @@
>  #include "string-list.h"
>  #include "thread-utils.h"
>  #include "sigchain.h"
> +#include "argv-array.h"
>  
>  static int debug;
>  
> @@ -19,6 +20,7 @@ struct helper_data {
>  	FILE *out;
>  	unsigned fetch : 1,
>  		import : 1,
> +		bidi_import : 1,
>  		export : 1,
>  		option : 1,
>  		push : 1,
> @@ -101,6 +103,7 @@ static void do_take_over(struct transport *transport)
>  static struct child_process *get_helper(struct transport *transport)
>  {
>  	struct helper_data *data = transport->data;
> +	struct argv_array argv = ARGV_ARRAY_INIT;
>  	struct strbuf buf = STRBUF_INIT;
>  	struct child_process *helper;
>  	const char **refspecs = NULL;
> @@ -122,11 +125,10 @@ static struct child_process *get_helper(struct transport *transport)
>  	helper->in = -1;
>  	helper->out = -1;
>  	helper->err = 0;
> -	helper->argv = xcalloc(4, sizeof(*helper->argv));
> -	strbuf_addf(&buf, "git-remote-%s", data->name);
> -	helper->argv[0] = strbuf_detach(&buf, NULL);
> -	helper->argv[1] = transport->remote->name;
> -	helper->argv[2] = remove_ext_force(transport->url);
> +	argv_array_pushf(&argv, "git-remote-%s", data->name);
> +	argv_array_push(&argv, transport->remote->name);
> +	argv_array_push(&argv, remove_ext_force(transport->url));
> +	helper->argv = argv.argv;
Much nicer than before thanks to argv_array ;-)
Show 8 quoted lines
>  	helper->git_cmd = 0;
>  	helper->silent_exec_failure = 1;
>  
> @@ -141,6 +143,8 @@ static struct child_process *get_helper(struct transport *transport)
>  
>  	data->helper = helper;
>  	data->no_disconnect_req = 0;
> +	free((void*) helper_env[1]);
What is this free() for???
> +	argv_array_clear(&argv);
See below.
Show 65 quoted lines
>  	/*
>  	 * Open the output as FILE* so strbuf_getline() can be used.
> @@ -178,6 +182,8 @@ static struct child_process *get_helper(struct transport *transport)
>  			data->push = 1;
>  		else if (!strcmp(capname, "import"))
>  			data->import = 1;
> +		else if (!strcmp(capname, "bidi-import"))
> +			data->bidi_import = 1;
>  		else if (!strcmp(capname, "export"))
>  			data->export = 1;
>  		else if (!data->refspecs && !prefixcmp(capname, "refspec ")) {
> @@ -241,8 +247,6 @@ static int disconnect_helper(struct transport *transport)
>  		close(data->helper->out);
>  		fclose(data->out);
>  		res = finish_command(data->helper);
> -		free((char *)data->helper->argv[0]);
> -		free(data->helper->argv);
>  		free(data->helper);
>  		data->helper = NULL;
>  	}
> @@ -376,14 +380,24 @@ static int fetch_with_fetch(struct transport *transport,
>  static int get_importer(struct transport *transport, struct child_process *fastimport)
>  {
>  	struct child_process *helper = get_helper(transport);
> +	struct helper_data *data = transport->data;
> +	struct argv_array argv = ARGV_ARRAY_INIT;
> +	int cat_blob_fd, code;
>  	memset(fastimport, 0, sizeof(*fastimport));
>  	fastimport->in = helper->out;
> -	fastimport->argv = xcalloc(5, sizeof(*fastimport->argv));
> -	fastimport->argv[0] = "fast-import";
> -	fastimport->argv[1] = "--quiet";
> +	argv_array_push(&argv, "fast-import");
> +	argv_array_push(&argv, "--quiet");
>  
> +	if (data->bidi_import) {
> +		cat_blob_fd = xdup(helper->in);
> +		argv_array_pushf(&argv, "--cat-blob-fd=%d", cat_blob_fd);
> +	}
> +	fastimport->argv = argv.argv;
>  	fastimport->git_cmd = 1;
> -	return start_command(fastimport);
> +
> +	code = start_command(fastimport);
> +	argv_array_clear(&argv);
> +	return code;
>  }
>  
>  static int get_exporter(struct transport *transport,
> @@ -438,11 +452,16 @@ static int fetch_with_import(struct transport *transport,
>  	}
>  
>  	write_constant(data->helper->in, "\n");
> +	/*
> +	 * remote-helpers that advertise the bidi-import capability are required to
> +	 * buffer the complete batch of import commands until this newline before
> +	 * sending data to fast-import.
> +	 * These helpers read back data from fast-import on their stdin, which could
> +	 * be mixed with import commands, otherwise.
> +	 */
>  
>  	if (finish_command(&fastimport))
>  		die("Error while running fast-import");
> -	free(fastimport.argv);
> -	fastimport.argv = NULL;

The updated code frees argv[] immediately after start_command() returns, and it may happen to be safe to do so with the current implementation of start_command() and friends, but I think it is a bad taste to free argv[] (or env[] for that matter) before calling finish_command(). These pieces of memory are still pointed by the child_process structure, and users of the structure may want to use contents of them (especially, argv[0]) for reporting errors and various other purposes, e.g.

	child = get_helper();
        trace("started %s\n", child->argv[0]);
	if (finish_command(child))
        	return error("failed to cleanly finish %s", child->argv[0]);
Previous: Florian AchleitnerNext: Florian Achleitner
Message 28 of 36 in “GSOC remote-svn”
  1. 00/16 GSOC remote-svnFlorian Achleitner, Aug 14, 2012
  2. 01/16 Implement a remote helper for svn in C.Florian Achleitner, Aug 14, 2012
  3. 02/16 Integrate remote-svn into svn-fe/Makefile.Florian Achleitner, Aug 14, 2012
  4. 03/16 Add svndump_init_fd to allow reading dumps from arbitrary FDs.Florian Achleitner, Aug 14, 2012
  5. 04/16 Connect fast-import to the remote-helper via pipe, adding 'bidi-import' capability.Florian Achleitner, Aug 14, 2012
  6. 05/16 Add documentation for the 'bidi-import' capability of remote-helpers.Florian Achleitner, Aug 14, 2012
  7. 06/16 remote-svn, vcs-svn: Enable fetching to private refs.Florian Achleitner, Aug 14, 2012
  8. 07/16 Add a symlink 'git-remote-svn' in base dir.Florian Achleitner, Aug 14, 2012
  9. 08/16 Allow reading svn dumps from files via file:// urls.Florian Achleitner, Aug 14, 2012
  10. 09/16 vcs-svn: add fast_export_note to create notesFlorian Achleitner, Aug 14, 2012
  11. 10/16 Create a note for every imported commit containing svn metadata.Florian Achleitner, Aug 14, 2012
  12. 11/16 When debug==1, start fast-import with "--stats" instead of "--quiet".Florian Achleitner, Aug 14, 2012
  13. 12/16 remote-svn: add incremental import.Florian Achleitner, Aug 14, 2012
  14. 13/16 Add a svnrdump-simulator replaying a dump file for testing.Florian Achleitner, Aug 14, 2012
  15. 14/16 transport-helper: add import|export-marks to fast-import command line.Florian Achleitner, Aug 14, 2012
  16. 15/16 remote-svn: add marks-file regeneration.Florian Achleitner, Aug 14, 2012
  17. 16/16 Add a test script for remote-svn.Florian Achleitner, Aug 14, 2012
  18. Florian AchleitnerAug 15, 2012
  19. Junio C HamanoAug 15, 2012
  20. Florian AchleitnerAug 15, 2012
  21. Florian AchleitnerAug 15, 2012
  22. Junio C HamanoAug 15, 2012
  23. Junio C HamanoAug 15, 2012
  24. Florian AchleitnerAug 15, 2012
  25. Junio C HamanoAug 14, 2012
  26. Junio C HamanoAug 14, 2012
  27. Florian AchleitnerAug 15, 2012
  28. Junio C HamanoAug 14, 2012
  29. Florian AchleitnerAug 15, 2012
  30. Junio C HamanoAug 15, 2012
  31. Junio C HamanoAug 14, 2012
  32. Junio C HamanoAug 14, 2012
  33. Florian AchleitnerAug 15, 2012
  34. Junio C HamanoAug 14, 2012
  35. Florian AchleitnerAug 15, 2012
  36. David Michael BarrAug 14, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.