From: Florian Achleitner <florian.achleitner.2.6.31@gmail•com>
To: Junio C Hamano <gitster@pobox•com>
Cc: Florian Achleitner <florian.achleitner.2.6.31@gmail•com>,
git@vger•kernel.org, David Michael Barr <davidbarr@google•com>,
Jonathan Nieder <jrnieder@gmail•com>
Subject: Re: [PATCH/RFC v3 04/16] Connect fast-import to the remote-helper via pipe, adding 'bidi-import' capability.
Date: Wed, 15 Aug 2012 14:00:33 +0200 [thread overview]
Message-ID: <2444647.P1AdWcSsQk@flomedio> (raw)
In-Reply-To: <7v4no59phn.fsf@alter.siamese.dyndns.org>
On Tuesday 14 August 2012 13:40:20 Junio C Hamano wrote:
> Florian Achleitner <florian.achleitner.2.6.31@gmail•com> writes:
> > 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/;
>
> > 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.
>
> > 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 ;-)
>
> > 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???
Sorry, legacy from previous versions, will be deleted.
>
> > + argv_array_clear(&argv);
>
> See below.
>
> > /*
> >
> > * 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]);
Yes, sounds reasonable. The present of immedate clearing has the advantage
that I don't have to store the struct argv_array, as struct child_process only
has a member for const char **argv.
I'll improve postpone the free until the command finishes.
next prev parent reply other threads:[~2012-08-15 12:00 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-08-14 19:13 [PATCH/RFC v3 00/16] GSOC remote-svn Florian Achleitner
2012-08-14 19:13 ` [PATCH/RFC v3 01/16] Implement a remote helper for svn in C Florian Achleitner
2012-08-14 19:13 ` [PATCH/RFC v3 02/16] Integrate remote-svn into svn-fe/Makefile Florian Achleitner
2012-08-14 19:13 ` [PATCH/RFC v3 03/16] Add svndump_init_fd to allow reading dumps from arbitrary FDs Florian Achleitner
2012-08-14 19:13 ` [PATCH/RFC v3 04/16] Connect fast-import to the remote-helper via pipe, adding 'bidi-import' capability Florian Achleitner
2012-08-14 19:13 ` [PATCH/RFC v3 05/16] Add documentation for the 'bidi-import' capability of remote-helpers Florian Achleitner
2012-08-14 19:13 ` [PATCH/RFC v3 06/16] remote-svn, vcs-svn: Enable fetching to private refs Florian Achleitner
2012-08-14 19:13 ` [PATCH/RFC v3 07/16] Add a symlink 'git-remote-svn' in base dir Florian Achleitner
2012-08-14 19:13 ` [PATCH/RFC v3 08/16] Allow reading svn dumps from files via file:// urls Florian Achleitner
2012-08-14 19:13 ` [PATCH/RFC v3 09/16] vcs-svn: add fast_export_note to create notes Florian Achleitner
2012-08-14 19:13 ` [PATCH/RFC v3 10/16] Create a note for every imported commit containing svn metadata Florian Achleitner
2012-08-14 19:13 ` [PATCH/RFC v3 11/16] When debug==1, start fast-import with "--stats" instead of "--quiet" Florian Achleitner
2012-08-14 19:13 ` [PATCH/RFC v3 12/16] remote-svn: add incremental import Florian Achleitner
2012-08-14 19:13 ` [PATCH/RFC v3 13/16] Add a svnrdump-simulator replaying a dump file for testing Florian Achleitner
2012-08-14 19:13 ` [PATCH/RFC v3 14/16] transport-helper: add import|export-marks to fast-import command line Florian Achleitner
2012-08-14 19:13 ` [PATCH/RFC v3 15/16] remote-svn: add marks-file regeneration Florian Achleitner
2012-08-14 19:13 ` [PATCH/RFC v3 16/16] Add a test script for remote-svn Florian Achleitner
2012-08-15 11:46 ` Florian Achleitner
2012-08-15 19:52 ` [PATCH/RFC v3 14/16] transport-helper: add import|export-marks to fast-import command line Junio C Hamano
2012-08-15 20:20 ` Florian Achleitner
2012-08-15 21:06 ` Florian Achleitner
2012-08-15 19:50 ` [PATCH/RFC v3 11/16] When debug==1, start fast-import with "--stats" instead of "--quiet" Junio C Hamano
2012-08-15 19:49 ` [PATCH/RFC v3 10/16] Create a note for every imported commit containing svn metadata Junio C Hamano
2012-08-15 20:10 ` Florian Achleitner
2012-08-14 20:43 ` [PATCH/RFC v3 07/16] Add a symlink 'git-remote-svn' in base dir Junio C Hamano
2012-08-14 20:46 ` Junio C Hamano
2012-08-15 9:20 ` Florian Achleitner
2012-08-14 20:40 ` [PATCH/RFC v3 04/16] Connect fast-import to the remote-helper via pipe, adding 'bidi-import' capability Junio C Hamano
2012-08-15 12:00 ` Florian Achleitner [this message]
2012-08-15 17:41 ` Junio C Hamano
2012-08-14 20:15 ` [PATCH/RFC v3 03/16] Add svndump_init_fd to allow reading dumps from arbitrary FDs Junio C Hamano
2012-08-14 20:14 ` [PATCH/RFC v3 02/16] Integrate remote-svn into svn-fe/Makefile Junio C Hamano
2012-08-15 8:54 ` Florian Achleitner
2012-08-14 20:07 ` [PATCH/RFC v3 01/16] Implement a remote helper for svn in C Junio C Hamano
2012-08-15 12:00 ` Florian Achleitner
2012-08-14 23:59 ` [PATCH/RFC v3 00/16] GSOC remote-svn David Michael Barr
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2444647.P1AdWcSsQk@flomedio \
--to=florian.achleitner.2.6.31@gmail$(echo .)com \
--cc=davidbarr@google$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=jrnieder@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox