{"thread":{"id":"30705","subject":"[RFC 1/4] Implement a basic remote helper vor svn in C.","startedAt":"2012-06-04T17:20:51Z","lastAt":"2012-08-12T20:10:36Z","messageCount":57,"participants":["Florian Achleitner","David Michael Barr","Jeff King","Johannes Sixt","Jonathan Nieder","Steven Michalske","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"192825","messageId":"1338830455-3091-1-git-send-email-florian.achleitner.2.6.31@gmail.com","threadId":"30705","inReplyTo":null,"subject":"[RFC 0/4]","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-06-04T17:20:51Z","receivedAt":"2012-06-04T17:20:51Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"Series of patches creating a very basic remote helper in C.\n\n[RFC 1/4] Implement a basic remote helper vor svn in C.\n[RFC 2/4] Integrate remote-svn into svn-fe/Makefile.\n[RFC 3/4] Add svndump_init_fd to allow reading dumps from arbitrary\n[RFC 4/4] Add cat-blob report pipe from fast-import to\n"},{"id":"192821","messageId":"1338830455-3091-2-git-send-email-florian.achleitner.2.6.31@gmail.com","threadId":"30705","inReplyTo":"1338830455-3091-1-git-send-email-florian.achleitner.2.6.31@gmail.com","subject":"[RFC 1/4] Implement a basic remote helper vor svn in C.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-06-04T17:20:52Z","receivedAt":"2012-06-04T17:20:52Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"Experimental implementation.\nInspired by the existing shell script at divanorama/remote-svn-alpha.\nIt doesn't use marks or notes yet, always imports the full history.\n\nsvnrdump is started as a subprocess while svn-fe's functions\nare called directly.\n\nSigned-off-by: Florian Achleitner \n---\n contrib/svn-fe/remote-svn.c | 188 +++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 188 insertions(+)\n create mode 100644 contrib/svn-fe/remote-svn.c\n\ndiff --git a/contrib/svn-fe/remote-svn.c b/contrib/svn-fe/remote-svn.c\nnew file mode 100644\nindex 0000000..ac09cc5\n--- /dev/null\n+++ b/contrib/svn-fe/remote-svn.c\n@@ -0,0 +1,188 @@\n+\n+#include \n+#include \n+#include \n+#include \"cache.h\"\n+#include \"remote.h\"\n+#include \"strbuf.h\"\n+#include \"url.h\"\n+#include \"exec_cmd.h\"\n+#include \"run-command.h\"\n+#include \"svndump.h\"\n+\n+static int debug = 0;\n+\n+static inline void printd(const char* fmt, ...)\n+{\n+\tif(debug) {\n+\t\tva_list vargs;\n+\t\tva_start(vargs, fmt);\n+\t\tfprintf(stderr, \"rhsvn debug: \");\n+\t\tvfprintf(stderr, fmt, vargs);\n+\t\tfprintf(stderr, \"\\n\");\n+\t\tva_end(vargs);\n+\t}\n+}\n+\n+static struct remote* remote;\n+static const char* url;\n+const char* private_refs = \"refs/remote-svn/\";\t\t/* + remote->name. */\n+const char* remote_ref = \"refs/heads/master\";\n+\n+enum cmd_result cmd_capabilities(struct strbuf* line);\n+enum cmd_result cmd_import(struct strbuf* line);\n+enum cmd_result cmd_list(struct strbuf* line);\n+\n+enum cmd_result { SUCCESS, NOT_HANDLED, ERROR };\n+typedef enum cmd_result (*command)(struct strbuf*);\n+\n+const command command_list[] = {\n+\t\tcmd_capabilities, cmd_import, cmd_list, NULL\n+};\n+\n+enum cmd_result cmd_capabilities(struct strbuf* line)\n+{\n+\tif(strcmp(line->buf, \"capabilities\"))\n+\t\treturn NOT_HANDLED;\n+\n+\tprintf(\"import\\n\");\n+\tprintf(\"\\n\");\n+\tfflush(stdout);\n+\treturn SUCCESS;\n+}\n+\n+enum cmd_result cmd_import(struct strbuf* line)\n+{\n+\tconst char* revs = \"-r0:HEAD\";\n+\tint code;\n+\tstruct child_process svndump_proc = {\n+\t\t\t.argv = NULL,\t\t/* comes later .. */\n+\t\t\t/* we want a pipe to the child's stdout, but stdin, stderr inherited.\n+\t\t\t The user can be asked for e.g. a password */\n+\t\t\t.in = 0, .out = -1, .err = 0,\n+\t\t\t.no_stdin = 0, .no_stdout = 0, .no_stderr = 0,\n+\t\t\t.git_cmd = 0,\n+\t\t\t.silent_exec_failure = 0,\n+\t\t\t.stdout_to_stderr = 0,\n+\t\t\t.use_shell = 0,\n+\t\t\t.clean_on_exit = 0,\n+\t\t\t.preexec_cb = NULL,\n+\t\t\t.env = NULL,\n+\t\t\t.dir = NULL\n+\t};\n+\n+\tif(prefixcmp(line->buf, \"import\"))\n+\t\treturn NOT_HANDLED;\n+\n+\tsvndump_proc.argv = xcalloc(5, sizeof(char*));\n+\tsvndump_proc.argv[0] = \"svnrdump\";\n+\tsvndump_proc.argv[1] = \"dump\";\n+\tsvndump_proc.argv[2] = url;\n+\tsvndump_proc.argv[3] = revs;\n+\n+\tcode = start_command(&svndump_proc);\n+\tif(code)\n+\t\tdie(\"Unable to start %s, code %d\", svndump_proc.argv[0], code);\n+\n+\tsvndump_init_fd(svndump_proc.out);\n+\tsvndump_read(url);\n+\tsvndump_deinit();\n+\tsvndump_reset();\n+\n+\tclose(svndump_proc.out);\n+\n+\tcode = finish_command(&svndump_proc);\n+\tif(code)\n+\t\twarning(\"Something went wrong with termination of %s, code %d\", svndump_proc.argv[0], code);\n+\tfree(svndump_proc.argv);\n+\n+\tprintf(\"done\\n\");\n+\treturn SUCCESS;\n+\n+\n+\n+}\n+\n+enum cmd_result cmd_list(struct strbuf* line)\n+{\n+\tif(strcmp(line->buf, \"list\"))\n+\t\treturn NOT_HANDLED;\n+\n+\tprintf(\"? HEAD\\n\");\n+\tprintf(\"? %s\\n\", remote_ref);\n+\tprintf(\"\\n\");\n+\tfflush(stdout);\n+\treturn SUCCESS;\n+}\n+\n+enum cmd_result do_command(struct strbuf* line)\n+{\n+\tconst command* p = command_list;\n+\tenum cmd_result ret;\n+\tprintd(\"command line '%s'\", line->buf);\n+\twhile(*p) {\n+\t\tret = (*p)(line);\n+\t\tif(ret != NOT_HANDLED)\n+\t\t\treturn ret;\n+\t\tp++;\n+\t}\n+\twarning(\"Unknown command '%s'\\n\", line->buf);\n+\treturn ret;\n+}\n+\n+int main(int argc, const char **argv)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint nongit;\n+\n+\tif (getenv(\"GIT_TRANSPORT_HELPER_DEBUG\"))\n+\t\tdebug = 1;\n+\n+\tgit_extract_argv0_path(argv[0]);\n+\tsetup_git_directory_gently(&nongit);\n+\tif (argc < 2) {\n+\t\tfprintf(stderr, \"Remote needed\\n\");\n+\t\treturn 1;\n+\t}\n+\n+\tremote = remote_get(argv[1]);\n+\tif (argc == 3) {\n+\t\tend_url_with_slash(&buf, argv[2]);\n+\t} else if (argc == 2) {\n+\t\tend_url_with_slash(&buf, remote->url[0]);\n+\t} else {\n+\t\twarning(\"Excess arguments!\");\n+\t}\n+\n+\turl = strbuf_detach(&buf, NULL);\n+\n+\tprintd(\"remote-svn starting with url %s\", url);\n+\n+\t/* build private ref namespace path for this svn remote. */\n+\tstrbuf_init(&buf, 0);\n+\tstrbuf_addstr(&buf, private_refs);\n+\tstrbuf_addstr(&buf, remote->name);\n+\tstrbuf_addch(&buf, '/');\n+\tprivate_refs = strbuf_detach(&buf, NULL);\n+\n+\twhile(1) {\n+\t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF) {\n+\t\t\tif (ferror(stdin))\n+\t\t\t\tfprintf(stderr, \"Error reading command stream\\n\");\n+\t\t\telse\n+\t\t\t\tfprintf(stderr, \"Unexpected end of command stream\\n\");\n+\t\t\treturn 1;\n+\t\t}\n+\t\t/* an empty line terminates the command stream */\n+\t\tif(buf.len == 0)\n+\t\t\tbreak;\n+\n+\t\tdo_command(&buf);\n+\t\tstrbuf_reset(&buf);\n+\t}\n+\n+\tstrbuf_release(&buf);\n+\tfree((void*)url);\n+\tfree((void*)private_refs);\n+\treturn 0;\n+}\n-- \n1.7.9.5\n"},{"id":"192824","messageId":"1338830455-3091-3-git-send-email-florian.achleitner.2.6.31@gmail.com","threadId":"30705","inReplyTo":"1338830455-3091-2-git-send-email-florian.achleitner.2.6.31@gmail.com","subject":"[RFC 2/4] Integrate remote-svn into svn-fe/Makefile.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-06-04T17:20:53Z","receivedAt":"2012-06-04T17:20:53Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"Requires some sha.h to be used and the libraries\nto be linked, this is currently hardcoded.\n\nSigned-off-by: Florian Achleitner \n---\n contrib/svn-fe/Makefile | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/contrib/svn-fe/Makefile b/contrib/svn-fe/Makefile\nindex 360d8da..253324f 100644\n--- a/contrib/svn-fe/Makefile\n+++ b/contrib/svn-fe/Makefile\n@@ -1,14 +1,14 @@\n-all:: svn-fe$X\n+all:: svn-fe$X remote-svn$X\n \n CC = gcc\n RM = rm -f\n MV = mv\n \n-CFLAGS = -g -O2 -Wall\n+CFLAGS = -g -O2 -Wall -DSHA1_HEADER=''\n LDFLAGS =\n ALL_CFLAGS = $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n-EXTLIBS =\n+EXTLIBS = -lssl -lcrypto -lpthread ../../xdiff/lib.a\n \n GIT_LIB = ../../libgit.a\n VCSSVN_LIB = ../../vcs-svn/lib.a\n@@ -37,8 +37,12 @@ svn-fe$X: svn-fe.o $(VCSSVN_LIB) $(GIT_LIB)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ svn-fe.o \\\n \t\t$(ALL_LDFLAGS) $(LIBS)\n \n-svn-fe.o: svn-fe.c ../../vcs-svn/svndump.h\n-\t$(QUIET_CC)$(CC) -I../../vcs-svn -o $*.o -c $(ALL_CFLAGS) $<\n+remote-svn$X: remote-svn.o $(VCSSVN_LIB) $(GIT_LIB)\n+\t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ remote-svn.o \\\n+\t\t$(ALL_LDFLAGS) $(LIBS)\n+\t\t\n+%.o: %.c ../../vcs-svn/svndump.h\n+\t$(QUIET_CC)$(CC) -I../../vcs-svn -I../../ -o $*.o -c $(ALL_CFLAGS) $<\n \n svn-fe.html: svn-fe.txt\n \t$(QUIET_SUBDIR0)../../Documentation $(QUIET_SUBDIR1) \\\n-- \n1.7.9.5\n"},{"id":"192822","messageId":"1338830455-3091-4-git-send-email-florian.achleitner.2.6.31@gmail.com","threadId":"30705","inReplyTo":"1338830455-3091-3-git-send-email-florian.achleitner.2.6.31@gmail.com","subject":"[RFC 3/4] Add svndump_init_fd to allow reading dumps from arbitrary FDs.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-06-04T17:20:54Z","receivedAt":"2012-06-04T17:20:54Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"The existing function only allowed reading from a filename or\nfrom stdin.\n\nSigned-off-by: Florian Achleitner \n---\n vcs-svn/svndump.c | 20 +++++++++++++++++---\n vcs-svn/svndump.h | 1 +\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c\nindex 0899790..2f0089f 100644\n--- a/vcs-svn/svndump.c\n+++ b/vcs-svn/svndump.c\n@@ -465,10 +465,8 @@ void svndump_read(const char *url)\n \t\tend_revision();\n }\n \n-int svndump_init(const char *filename)\n+static void init()\n {\n-\tif (buffer_init(&input, filename))\n-\t\treturn error(\"cannot open %s: %s\", filename, strerror(errno));\n \tfast_export_init(REPORT_FILENO);\n \tstrbuf_init(&dump_ctx.uuid, 4096);\n \tstrbuf_init(&dump_ctx.url, 4096);\n@@ -479,6 +477,22 @@ int svndump_init(const char *filename)\n \treset_dump_ctx(NULL);\n \treset_rev_ctx(0);\n \treset_node_ctx(NULL);\n+\treturn;\n+}\n+\n+int svndump_init(const char *filename)\n+{\n+\tif (buffer_init(&input, filename))\n+\t\treturn error(\"cannot open %s: %s\", filename, strerror(errno));\n+\tinit();\n+\treturn 0;\n+}\n+\n+int svndump_init_fd(int in_fd)\n+{\n+\tif(buffer_fdinit(&input, in_fd))\n+\t\treturn error(\"cannot open fd %d: %s\", in_fd, strerror(errno));\n+\tinit();\n \treturn 0;\n }\n \ndiff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h\nindex df9ceb0..24e7beb 100644\n--- a/vcs-svn/svndump.h\n+++ b/vcs-svn/svndump.h\n@@ -2,6 +2,7 @@\n #define SVNDUMP_H_\n \n int svndump_init(const char *filename);\n+int svndump_init_fd(int in_fd);\n void svndump_read(const char *url);\n void svndump_deinit(void);\n void svndump_reset(void);\n-- \n1.7.9.5\n"},{"id":"192823","messageId":"1338830455-3091-5-git-send-email-florian.achleitner.2.6.31@gmail.com","threadId":"30705","inReplyTo":"1338830455-3091-4-git-send-email-florian.achleitner.2.6.31@gmail.com","subject":"[RFC 4/4] Add cat-blob report pipe from fast-import to remote-helper.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-06-04T17:20:55Z","receivedAt":"2012-06-04T17:20:55Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"On invocation of a helper a new pipe is opened.\nTo close the other end after fork, the prexec_cb feature\nof the run_command api is used.\nIf the helper is not used with fast-import later the pipe\nis unused.\nThe FD is passed to the remote-helper via it's environment,\nhelpers that don't use fast-import can simply ignore it.\nfast-import has an argv for that.\n\nSigned-off-by: Florian Achleitner \n---\n transport-helper.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++\n vcs-svn/svndump.c | 9 ++++++++-\n 2 files changed, 56 insertions(+), 1 deletion(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 61c928f..b438040 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -17,6 +17,7 @@ struct helper_data {\n \tconst char *name;\n \tstruct child_process *helper;\n \tFILE *out;\n+\tint fast_import_backchannel_pipe[2];\n \tunsigned fetch : 1,\n \t\timport : 1,\n \t\texport : 1,\n@@ -98,19 +99,30 @@ static void do_take_over(struct transport *transport)\n \tfree(data);\n }\n \n+static int fd_to_close;\n+void close_fd_prexec_cb(void)\n+{\n+\tif(debug)\n+\t\tfprintf(stderr, \"close_fd_prexec_cb closing %d\\n\", fd_to_close);\n+\tclose(fd_to_close);\n+}\n+\n static struct child_process *get_helper(struct transport *transport)\n {\n \tstruct helper_data *data = transport->data;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf envbuf = STRBUF_INIT;\n \tstruct child_process *helper;\n \tconst char **refspecs = NULL;\n \tint refspec_nr = 0;\n \tint refspec_alloc = 0;\n \tint duped;\n \tint code;\n+\tint err;\n \tchar git_dir_buf[sizeof(GIT_DIR_ENVIRONMENT) + PATH_MAX + 1];\n \tconst char *helper_env[] = {\n \t\tgit_dir_buf,\n+\t\tNULL,\t/* placeholder */\n \t\tNULL\n \t};\n \n@@ -133,6 +145,24 @@ static struct child_process *get_helper(struct transport *transport)\n \tsnprintf(git_dir_buf, sizeof(git_dir_buf), \"%s=%s\", GIT_DIR_ENVIRONMENT, get_git_dir());\n \thelper->env = helper_env;\n \n+\n+\t/* create an additional pipe from fast-import to the helper */\n+\terr = pipe(data->fast_import_backchannel_pipe);\n+\tif(err)\n+\t\tdie(\"cannot create fast_import_backchannel_pipe: %s\", strerror(errno));\n+\n+\tif(debug)\n+\t\tfprintf(stderr, \"Remote helper created fast_import_backchannel_pipe { %d, %d }\\n\",\n+\t\t\t\tdata->fast_import_backchannel_pipe[0], data->fast_import_backchannel_pipe[1]);\n+\n+\tstrbuf_addf(&envbuf, \"GIT_REPORT_FILENO=%d\", data->fast_import_backchannel_pipe[0]);\n+\thelper_env[1] = strbuf_detach(&envbuf, NULL);\n+\n+\t/* after the fork, we need to close the write end in the helper */\n+\tfd_to_close = data->fast_import_backchannel_pipe[1];\n+\t/* the prexec callback is run just before exec */\n+\thelper->preexec_cb = close_fd_prexec_cb;\n+\n \tcode = start_command(helper);\n \tif (code < 0 && errno == ENOENT)\n \t\tdie(\"Unable to find remote helper for '%s'\", data->name);\n@@ -141,6 +171,7 @@ static struct child_process *get_helper(struct transport *transport)\n \n \tdata->helper = helper;\n \tdata->no_disconnect_req = 0;\n+\tfree((void*)helper_env[1]);\n \n \t/*\n \t * Open the output as FILE* so strbuf_getline() can be used.\n@@ -237,6 +268,10 @@ static int disconnect_helper(struct transport *transport)\n \t\t\txwrite(data->helper->in, \"\\n\", 1);\n \t\t\tsigchain_pop(SIGPIPE);\n \t\t}\n+\t\t/* close the pipe, it is still open if it wasn't used for fast-import. */\n+\t\tclose(data->fast_import_backchannel_pipe[0]);\n+\t\tclose(data->fast_import_backchannel_pipe[1]);\n+\n \t\tclose(data->helper->in);\n \t\tclose(data->helper->out);\n \t\tfclose(data->out);\n@@ -376,13 +411,20 @@ static int fetch_with_fetch(struct transport *transport,\n static int get_importer(struct transport *transport, struct child_process *fastimport)\n {\n \tstruct child_process *helper = get_helper(transport);\n+\tstruct helper_data *data = transport->data;\n+\tstruct strbuf buf = STRBUF_INIT;\n \tmemset(fastimport, 0, sizeof(*fastimport));\n \tfastimport->in = helper->out;\n \tfastimport->argv = xcalloc(5, sizeof(*fastimport->argv));\n \tfastimport->argv[0] = \"fast-import\";\n \tfastimport->argv[1] = \"--quiet\";\n+\tstrbuf_addf(&buf, \"--cat-blob-fd=%d\", data->fast_import_backchannel_pipe[1]);\n+\tfastimport->argv[2] = strbuf_detach(&buf, NULL);\n \n \tfastimport->git_cmd = 1;\n+\n+\tfd_to_close = data->fast_import_backchannel_pipe[0];\n+\tfastimport->preexec_cb = close_fd_prexec_cb;\n \treturn start_command(fastimport);\n }\n \n@@ -427,6 +469,11 @@ static int fetch_with_import(struct transport *transport,\n \tif (get_importer(transport, &fastimport))\n \t\tdie(\"Couldn't run fast-import\");\n \n+\n+\t/* in the parent process we close both pipe ends. */\n+\tclose(data->fast_import_backchannel_pipe[0]);\n+\tclose(data->fast_import_backchannel_pipe[1]);\n+\n \tfor (i = 0; i < nr_heads; i++) {\n \t\tposn = to_fetch[i];\n \t\tif (posn->status & REF_STATUS_UPTODATE)\n@@ -441,6 +488,7 @@ static int fetch_with_import(struct transport *transport,\n \n \tif (finish_command(&fastimport))\n \t\tdie(\"Error while running fast-import\");\n+\tfree((void*)fastimport.argv[2]);\n \tfree(fastimport.argv);\n \tfastimport.argv = NULL;\n \ndiff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c\nindex 2f0089f..b1fe03f 100644\n--- a/vcs-svn/svndump.c\n+++ b/vcs-svn/svndump.c\n@@ -467,7 +467,14 @@ void svndump_read(const char *url)\n \n static void init()\n {\n-\tfast_export_init(REPORT_FILENO);\n+\tint report_fd;\n+\tchar* back_fd_env = getenv(\"GIT_REPORT_FILENO\");\n+\tif(!back_fd_env || sscanf(back_fd_env, \"%d\", &report_fd) != 1) {\n+\t\twarning(\"Cannot get cat-blob fd from environment, using default!\");\n+\t\treport_fd = REPORT_FILENO;\n+\t}\n+\n+\tfast_export_init(report_fd);\n \tstrbuf_init(&dump_ctx.uuid, 4096);\n \tstrbuf_init(&dump_ctx.url, 4096);\n \tstrbuf_init(&rev_ctx.log, 4096);\n-- \n1.7.9.5\n"},{"id":"192863","messageId":"CAFfmPPO3Yvbs0EzMQa2z13ERi6Ocjv3W=8FYaAGPt739bD_qbw@mail.gmail.com","threadId":"30705","inReplyTo":"1338830455-3091-4-git-send-email-florian.achleitner.2.6.31@gmail.com","subject":"Re: [RFC 3/4] Add svndump_init_fd to allow reading dumps from arbitrary FDs.","fromName":"David Michael Barr","fromEmail":"davidbarr@google.com","sentAt":"2012-06-05T01:21:05Z","receivedAt":"2012-06-05T01:21:05Z","isPatch":false,"sender":{"key":"davidbarr@google.com","avatar":"https://avatars.githubusercontent.com/u/220594?v=4"},"body":"On Tue, Jun 5, 2012 at 3:20 AM, Florian Achleitner\n wrote:\n> The existing function only allowed reading from a filename or\n> from stdin.\n>\n> Signed-off-by: Florian Achleitner \n> ---\n>  vcs-svn/svndump.c |   20 +++++++++++++++++---\n>  vcs-svn/svndump.h |    1 +\n>  2 files changed, 18 insertions(+), 3 deletions(-)\n>\n> diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c\n> index 0899790..2f0089f 100644\n> --- a/vcs-svn/svndump.c\n> +++ b/vcs-svn/svndump.c\n> @@ -465,10 +465,8 @@ void svndump_read(const char *url)\n>                end_revision();\n>  }\n>\n> -int svndump_init(const char *filename)\n> +static void init()\n>  {\n> -       if (buffer_init(&input, filename))\n> -               return error(\"cannot open %s: %s\", filename, strerror(errno));\n>        fast_export_init(REPORT_FILENO);\n>        strbuf_init(&dump_ctx.uuid, 4096);\n>        strbuf_init(&dump_ctx.url, 4096);\n> @@ -479,6 +477,22 @@ int svndump_init(const char *filename)\n>        reset_dump_ctx(NULL);\n>        reset_rev_ctx(0);\n>        reset_node_ctx(NULL);\n> +       return;\n> +}\n> +\n> +int svndump_init(const char *filename)\n> +{\n> +       if (buffer_init(&input, filename))\n> +               return error(\"cannot open %s: %s\", filename, strerror(errno));\n\nNote: filename is allowed to be NULL here.\nThis is a bug in the existing code that you just moved.\n\nI suggest moving error printing into buffer_init().\nThis way the basis for the message is clearer.\n\nFor bonus points, we should split buffer_init().\nPlain buffer_init() should use stdin.\nThe new buff_init_path() should take a filename.\n\n> +       init();\n> +       return 0;\n> +}\n> +\n> +int svndump_init_fd(int in_fd)\n> +{\n> +       if(buffer_fdinit(&input, in_fd))\n> +               return error(\"cannot open fd %d: %s\", in_fd, strerror(errno));\n> +       init();\n>        return 0;\n>  }\n>\n> diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h\n> index df9ceb0..24e7beb 100644\n> --- a/vcs-svn/svndump.h\n> +++ b/vcs-svn/svndump.h\n> @@ -2,6 +2,7 @@\n>  #define SVNDUMP_H_\n>\n>  int svndump_init(const char *filename);\n> +int svndump_init_fd(int in_fd);\n>  void svndump_read(const char *url);\n>  void svndump_deinit(void);\n>  void svndump_reset(void);\n> --\n> 1.7.9.5\n\nOtherwise, I like the direction of this patch.\n\n--\nDavid Barr\n"},{"id":"192864","messageId":"CAFfmPPMLWdVBGBzdXV4QD7tbmgykWmB3OpQmWQ0BAbQiGt7fQg@mail.gmail.com","threadId":"30705","inReplyTo":"1338830455-3091-5-git-send-email-florian.achleitner.2.6.31@gmail.com","subject":"Re: [RFC 4/4] Add cat-blob report pipe from fast-import to remote-helper.","fromName":"David Michael Barr","fromEmail":"davidbarr@google.com","sentAt":"2012-06-05T01:33:16Z","receivedAt":"2012-06-05T01:33:16Z","isPatch":false,"sender":{"key":"davidbarr@google.com","avatar":"https://avatars.githubusercontent.com/u/220594?v=4"},"body":"On Tue, Jun 5, 2012 at 3:20 AM, Florian Achleitner\n wrote:\n> On invocation of a helper a new pipe is opened.\n> To close the other end after fork, the prexec_cb feature\n> of the run_command api is used.\n> If the helper is not used with fast-import later the pipe\n> is unused.\n> The FD is passed to the remote-helper via it's environment,\n> helpers that don't use fast-import can simply ignore it.\n> fast-import has an argv for that.\n>\n> Signed-off-by: Florian Achleitner \n> ---\n>  transport-helper.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++\n>  vcs-svn/svndump.c  |    9 ++++++++-\n>  2 files changed, 56 insertions(+), 1 deletion(-)\n>\n> diff --git a/transport-helper.c b/transport-helper.c\n> index 61c928f..b438040 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -17,6 +17,7 @@ struct helper_data {\n>        const char *name;\n>        struct child_process *helper;\n>        FILE *out;\n> +       int fast_import_backchannel_pipe[2];\n>        unsigned fetch : 1,\n>                import : 1,\n>                export : 1,\n> @@ -98,19 +99,30 @@ static void do_take_over(struct transport *transport)\n>        free(data);\n>  }\n>\n> +static int fd_to_close;\n> +void close_fd_prexec_cb(void)\n> +{\n> +       if(debug)\n> +               fprintf(stderr, \"close_fd_prexec_cb closing %d\\n\", fd_to_close);\n> +       close(fd_to_close);\n> +}\n> +\n>  static struct child_process *get_helper(struct transport *transport)\n>  {\n>        struct helper_data *data = transport->data;\n>        struct strbuf buf = STRBUF_INIT;\n> +       struct strbuf envbuf = STRBUF_INIT;\n>        struct child_process *helper;\n>        const char **refspecs = NULL;\n>        int refspec_nr = 0;\n>        int refspec_alloc = 0;\n>        int duped;\n>        int code;\n> +       int err;\n>        char git_dir_buf[sizeof(GIT_DIR_ENVIRONMENT) + PATH_MAX + 1];\n>        const char *helper_env[] = {\n>                git_dir_buf,\n> +               NULL,   /* placeholder */\n>                NULL\n>        };\n>\n> @@ -133,6 +145,24 @@ static struct child_process *get_helper(struct transport *transport)\n>        snprintf(git_dir_buf, sizeof(git_dir_buf), \"%s=%s\", GIT_DIR_ENVIRONMENT, get_git_dir());\n>        helper->env = helper_env;\n>\n> +\n> +       /* create an additional pipe from fast-import to the helper */\n> +       err = pipe(data->fast_import_backchannel_pipe);\n> +       if(err)\n> +               die(\"cannot create fast_import_backchannel_pipe: %s\", strerror(errno));\n> +\n> +       if(debug)\n> +               fprintf(stderr, \"Remote helper created fast_import_backchannel_pipe { %d, %d }\\n\",\n> +                               data->fast_import_backchannel_pipe[0], data->fast_import_backchannel_pipe[1]);\n> +\n> +       strbuf_addf(&envbuf, \"GIT_REPORT_FILENO=%d\", data->fast_import_backchannel_pipe[0]);\n> +       helper_env[1] = strbuf_detach(&envbuf, NULL);\n> +\n> +       /* after the fork, we need to close the write end in the helper */\n> +       fd_to_close = data->fast_import_backchannel_pipe[1];\n> +       /* the prexec callback is run just before exec */\n> +       helper->preexec_cb = close_fd_prexec_cb;\n> +\n>        code = start_command(helper);\n>        if (code < 0 && errno == ENOENT)\n>                die(\"Unable to find remote helper for '%s'\", data->name);\n> @@ -141,6 +171,7 @@ static struct child_process *get_helper(struct transport *transport)\n>\n>        data->helper = helper;\n>        data->no_disconnect_req = 0;\n> +       free((void*)helper_env[1]);\n>\n>        /*\n>         * Open the output as FILE* so strbuf_getline() can be used.\n> @@ -237,6 +268,10 @@ static int disconnect_helper(struct transport *transport)\n>                        xwrite(data->helper->in, \"\\n\", 1);\n>                        sigchain_pop(SIGPIPE);\n>                }\n> +               /* close the pipe, it is still open if it wasn't used for fast-import. */\n> +               close(data->fast_import_backchannel_pipe[0]);\n> +               close(data->fast_import_backchannel_pipe[1]);\n> +\n>                close(data->helper->in);\n>                close(data->helper->out);\n>                fclose(data->out);\n> @@ -376,13 +411,20 @@ static int fetch_with_fetch(struct transport *transport,\n>  static int get_importer(struct transport *transport, struct child_process *fastimport)\n>  {\n>        struct child_process *helper = get_helper(transport);\n> +       struct helper_data *data = transport->data;\n> +       struct strbuf buf = STRBUF_INIT;\n>        memset(fastimport, 0, sizeof(*fastimport));\n>        fastimport->in = helper->out;\n>        fastimport->argv = xcalloc(5, sizeof(*fastimport->argv));\n>        fastimport->argv[0] = \"fast-import\";\n>        fastimport->argv[1] = \"--quiet\";\n> +       strbuf_addf(&buf, \"--cat-blob-fd=%d\", data->fast_import_backchannel_pipe[1]);\n> +       fastimport->argv[2] = strbuf_detach(&buf, NULL);\n>\n>        fastimport->git_cmd = 1;\n> +\n> +       fd_to_close = data->fast_import_backchannel_pipe[0];\n> +       fastimport->preexec_cb = close_fd_prexec_cb;\n>        return start_command(fastimport);\n>  }\n>\n> @@ -427,6 +469,11 @@ static int fetch_with_import(struct transport *transport,\n>        if (get_importer(transport, &fastimport))\n>                die(\"Couldn't run fast-import\");\n>\n> +\n> +       /* in the parent process we close both pipe ends. */\n> +       close(data->fast_import_backchannel_pipe[0]);\n> +       close(data->fast_import_backchannel_pipe[1]);\n> +\n>        for (i = 0; i < nr_heads; i++) {\n>                posn = to_fetch[i];\n>                if (posn->status & REF_STATUS_UPTODATE)\n> @@ -441,6 +488,7 @@ static int fetch_with_import(struct transport *transport,\n>\n>        if (finish_command(&fastimport))\n>                die(\"Error while running fast-import\");\n> +       free((void*)fastimport.argv[2]);\n>        free(fastimport.argv);\n>        fastimport.argv = NULL;\n>\n> diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c\n> index 2f0089f..b1fe03f 100644\n> --- a/vcs-svn/svndump.c\n> +++ b/vcs-svn/svndump.c\n> @@ -467,7 +467,14 @@ void svndump_read(const char *url)\n>\n>  static void init()\n>  {\n> -       fast_export_init(REPORT_FILENO);\n> +       int report_fd;\n> +       char* back_fd_env = getenv(\"GIT_REPORT_FILENO\");\n> +       if(!back_fd_env || sscanf(back_fd_env, \"%d\", &report_fd) != 1) {\n> +               warning(\"Cannot get cat-blob fd from environment, using default!\");\n> +               report_fd = REPORT_FILENO;\n> +       }\n> +\n> +       fast_export_init(report_fd);\n>        strbuf_init(&dump_ctx.uuid, 4096);\n>        strbuf_init(&dump_ctx.url, 4096);\n>        strbuf_init(&rev_ctx.log, 4096);\n> --\n> 1.7.9.5\n\n+cc Sverre Rabbelier and Jeff King, as they have been active in\ntransport-helper.c.\n\n--\nDavid Barr\n"},{"id":"192870","messageId":"20120605065628.GA25809@sigill.intra.peff.net","threadId":"30705","inReplyTo":"1338830455-3091-5-git-send-email-florian.achleitner.2.6.31@gmail.com","subject":"Re: [RFC 4/4] Add cat-blob report pipe from fast-import to remote-helper.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-05T06:56:28Z","receivedAt":"2012-06-05T06:56:28Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jun 04, 2012 at 07:20:55PM +0200, Florian Achleitner wrote:\n\n> On invocation of a helper a new pipe is opened.\n> To close the other end after fork, the prexec_cb feature\n> of the run_command api is used.\n> If the helper is not used with fast-import later the pipe\n> is unused.\n> The FD is passed to the remote-helper via it's environment,\n> helpers that don't use fast-import can simply ignore it.\n> fast-import has an argv for that.\n\nI don't keep up on fast-import development, so I have no clue how useful\nthis extra pipe is, or whether this patch is a good idea overall. But a\nfew comments on the transport.c half of things:\n\n> +static int fd_to_close;\n> +void close_fd_prexec_cb(void)\n> +{\n> +\tif(debug)\n> +\t\tfprintf(stderr, \"close_fd_prexec_cb closing %d\\n\", fd_to_close);\n> +\tclose(fd_to_close);\n> +}\n\nNote that preexec_cb does not work at all on Windows, as it assumes a\nforking model (rather than a spawn, which leaves no room to execute\narbitrary code in the child). If all you want to do is open an extra\npipe, then probably run-command should be extended to handle this\n(though I have no idea how complex that would be for the Windows side of\nthings, it is at least _possible_, as opposed to preexec_cb, which will\nnever be possible).\n\n> @@ -376,13 +411,20 @@ static int fetch_with_fetch(struct transport *transport,\n> static int get_importer(struct transport *transport, struct child_process *fastimport)\n> {\n> \tstruct child_process *helper = get_helper(transport);\n> +\tstruct helper_data *data = transport->data;\n> +\tstruct strbuf buf = STRBUF_INIT;\n> \tmemset(fastimport, 0, sizeof(*fastimport));\n> \tfastimport->in = helper->out;\n> \tfastimport->argv = xcalloc(5, sizeof(*fastimport->argv));\n> \tfastimport->argv[0] = \"fast-import\";\n> \tfastimport->argv[1] = \"--quiet\";\n> +\tstrbuf_addf(&buf, \"--cat-blob-fd=%d\", data->fast_import_backchannel_pipe[1]);\n> +\tfastimport->argv[2] = strbuf_detach(&buf, NULL);\n\nConsider converting this to use argv_array. You can drop the magic\nnumbers, and \"argv_array_pushf\" handles the strbuf bits for you\nautomatically.\n\nAnd this grossness can go away:\n\n> @@ -441,6 +488,7 @@ static int fetch_with_import(struct transport *transport,\n> \n> \tif (finish_command(&fastimport))\n> \t\tdie(\"Error while running fast-import\");\n> +\tfree((void*)fastimport.argv[2]);\n> \tfree(fastimport.argv);\n> \tfastimport.argv = NULL;\n\n(you'd instead want to free everything; it would probably make sense to\nadd an argv_array_free_detached() function to do so).\n\n> @@ -427,6 +469,11 @@ static int fetch_with_import(struct transport *transport,\n> \tif (get_importer(transport, &fastimport))\n> \t\tdie(\"Couldn't run fast-import\");\n> \n> +\n> +\t/* in the parent process we close both pipe ends. */\n> +\tclose(data->fast_import_backchannel_pipe[0]);\n> +\tclose(data->fast_import_backchannel_pipe[1]);\n\nI'm confused. We close both ends? Who is actually reading and writing to\nthis pipe, then?\n\n-Peff\n"},{"id":"192872","messageId":"CAFfmPPP1koMnYBFbgHt0MGr77okjL5OdAh-TMxFTevj+mDbOZQ@mail.gmail.com","threadId":"30705","inReplyTo":"20120605065628.GA25809@sigill.intra.peff.net","subject":"Re: [RFC 4/4] Add cat-blob report pipe from fast-import to remote-helper.","fromName":"David Michael Barr","fromEmail":"davidbarr@google.com","sentAt":"2012-06-05T07:07:53Z","receivedAt":"2012-06-05T07:07:53Z","isPatch":false,"sender":{"key":"davidbarr@google.com","avatar":"https://avatars.githubusercontent.com/u/220594?v=4"},"body":"On Tue, Jun 5, 2012 at 4:56 PM, Jeff King wrote:\n> On Mon, Jun 04, 2012 at 07:20:55PM +0200, Florian Achleitner wrote:\n>> @@ -427,6 +469,11 @@ static int fetch_with_import(struct transport *transport,\n>>       if (get_importer(transport, &fastimport))\n>>               die(\"Couldn't run fast-import\");\n>>\n>> +\n>> +     /* in the parent process we close both pipe ends. */\n>> +     close(data->fast_import_backchannel_pipe[0]);\n>> +     close(data->fast_import_backchannel_pipe[1]);\n>\n> I'm confused. We close both ends? Who is actually reading and writing to\n> this pipe, then?\n\nOne child, git-fast-import writes to one end.\nThe other child, git-remote-* reads from the other end.\n\n--\nDavid Barr\n"},{"id":"192880","messageId":"20120605081402.GF25809@sigill.intra.peff.net","threadId":"30705","inReplyTo":"CAFfmPPP1koMnYBFbgHt0MGr77okjL5OdAh-TMxFTevj+mDbOZQ@mail.gmail.com","subject":"Re: [RFC 4/4] Add cat-blob report pipe from fast-import to remote-helper.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-05T08:14:02Z","receivedAt":"2012-06-05T08:14:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 05, 2012 at 05:07:53PM +1000, David Michael Barr wrote:\n\n> On Tue, Jun 5, 2012 at 4:56 PM, Jeff King wrote:\n> > On Mon, Jun 04, 2012 at 07:20:55PM +0200, Florian Achleitner wrote:\n> >> @@ -427,6 +469,11 @@ static int fetch_with_import(struct transport *transport,\n> >>       if (get_importer(transport, &fastimport))\n> >>               die(\"Couldn't run fast-import\");\n> >>\n> >> +\n> >> +     /* in the parent process we close both pipe ends. */\n> >> +     close(data->fast_import_backchannel_pipe[0]);\n> >> +     close(data->fast_import_backchannel_pipe[1]);\n> >\n> > I'm confused. We close both ends? Who is actually reading and writing to\n> > this pipe, then?\n> \n> One child, git-fast-import writes to one end.\n> The other child, git-remote-* reads from the other end.\n\nAh, thanks. I missed where the write end was going, but now I see it.\nOverall, the point of the patch makes sense to me (it would have been\nnice if the commit message described the rationale a bit more\ncompletely).\n\nIs there a reason that the patch unconditionally creates the pipe in\nget_helper? I.e., isn't it specific to the get_importer code path? It\nfeels a little hacky to have it infect the other code paths.\n\n-Peff\n"},{"id":"192881","messageId":"4FCDC894.7000905@viscovery.net","threadId":"30705","inReplyTo":"20120605065628.GA25809@sigill.intra.peff.net","subject":"Re: [RFC 4/4] Add cat-blob report pipe from fast-import to remote-helper.","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-06-05T08:51:32Z","receivedAt":"2012-06-05T08:51:32Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 6/5/2012 8:56, schrieb Jeff King:\n> On Mon, Jun 04, 2012 at 07:20:55PM +0200, Florian Achleitner wrote:\n>> +static int fd_to_close;\n>> +void close_fd_prexec_cb(void)\n>> +{\n>> +\tif(debug)\n>> +\t\tfprintf(stderr, \"close_fd_prexec_cb closing %d\\n\", fd_to_close);\n>> +\tclose(fd_to_close);\n>> +}\n> \n> Note that preexec_cb does not work at all on Windows, as it assumes a\n> forking model (rather than a spawn, which leaves no room to execute\n> arbitrary code in the child). If all you want to do is open an extra\n> pipe, then probably run-command should be extended to handle this\n> (though I have no idea how complex that would be for the Windows side of\n> things, it is at least _possible_, as opposed to preexec_cb, which will\n> never be possible).\n\nThe lack of support for preexec_cb on Windows is actually not the problem\nin this case. Our emulation of pipe() actually creates file handles that\nare not inherited by child processes. (For the standard channels 0,1,2 we\nrely on that dup() creates duplicates that *can* be inherited; so they\nstill work.)\n\nThe first problem with the new infrastructure in this patch is that dup()\nis not called anywhere after pipe(). To solve this, we would have to\nextend run-command in some way to allow passing along arbitrary pipes and\nhandles.\n\nThe second problem is more severe and is at the lowest level of our\ninfrastructure: We set up our child processes so that they know only about\nfile descriptors other than 0,1,2 to the child process. Even if the first\nproblem were solved, the child process does not receive sufficient\ninformation to know that there are open file descriptors other than 0,1,2.\nThere is a facility to pass along this information from the parent to the\nchild, but we simply do not implement it.\n\nIOW: Everything that uses --cat-blob-fd or a similar facility cannot work\non Windows without considerable additional effort.\n\n-- Hannes\n"},{"id":"192884","messageId":"20120605090702.GA27376@sigill.intra.peff.net","threadId":"30705","inReplyTo":"4FCDC894.7000905@viscovery.net","subject":"Re: [RFC 4/4] Add cat-blob report pipe from fast-import to remote-helper.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-05T09:07:02Z","receivedAt":"2012-06-05T09:07:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 05, 2012 at 10:51:32AM +0200, Johannes Sixt wrote:\n\n> > Note that preexec_cb does not work at all on Windows, as it assumes a\n> [... a very nice explanation of the pipe issues ...]\n> \n> IOW: Everything that uses --cat-blob-fd or a similar facility cannot work\n> on Windows without considerable additional effort.\n\nThanks, Johannes, that makes sense to me.\n\nFlorian, does that mean that making the svn helper start to use\n--cat-blob-fd at all is a potential regression for Windows? The\nfast-import documentation says that the cat-blob output will go to\nstdout now. Does it even work at all now? I don't really know or\nunderstand all of the reasons for cat-blob-fd to exist in the first\nplace.\n\nI expect one answer might be \"well, the svn remote helper does not work\nat all on Windows already, so there's no regression\". But this affects\n_all_ fast-import calls that git's transport-helper makes. Are there\nother ones that use import, and would they be affected by this? \n\nFor that matter, isn't this a backwards-incompatible change for other\nthird-party helpers? Won't they need to respect the new\nGIT_REPORT_FILENO environment variable? Do we need the helper to specify\n\"yes, I am ready to handle cat-blob-fd\" in its capabilities list?\n\n-Peff\n"},{"id":"192885","messageId":"4FCDCCCE.5060201@viscovery.net","threadId":"30705","inReplyTo":"4FCDC894.7000905@viscovery.net","subject":"Re: [RFC 4/4] Add cat-blob report pipe from fast-import to remote-helper.","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-06-05T09:09:34Z","receivedAt":"2012-06-05T09:09:34Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 6/5/2012 10:51, schrieb Johannes Sixt:\n> The second problem is more severe and is at the lowest level of our\n> infrastructure: We set up our child processes so that they know only about\n> file descriptors other than 0,1,2 to the child process.\n\nThat should read:\n\nWe set up our child processes so that they know only about file\ndescriptors 0,1,2.\n\n> Even if the first\n> problem were solved, the child process does not receive sufficient\n> information to know that there are open file descriptors other than 0,1,2.\n> There is a facility to pass along this information from the parent to the\n> child, but we simply do not implement it.\n"},{"id":"192956","messageId":"5801019.gWQEmI8V81@flobuntu","threadId":"30705","inReplyTo":"20120605081402.GF25809@sigill.intra.peff.net","subject":"Re: [RFC 4/4] Add cat-blob report pipe from fast-import to remote-helper.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-06-05T22:16:25Z","receivedAt":"2012-06-05T22:16:25Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"On Tuesday 05 June 2012 04:14:02 Jeff King wrote:\n> Is there a reason that the patch unconditionally creates the pipe in\n> get_helper? I.e., isn't it specific to the get_importer code path? It\n> feels a little hacky to have it infect the other code paths.\n\nI agree, it's a bit hacky. For me as a newbee, it was just a way to make fast-\nimport have the pipe it needs. I didn't know about the history of the \npreexec_cb as a fix for a bug in less.\n\nThe pipe is created unconditionally, because at the fork-time of the remote-\nhelper it is not known whether the import command will be used later together \nwith fast-import, or not. (and later, there's no way, I think).\nHelpers that don't use the pipe could simply ignore it.\n\n--\nFlorian Achleitner\n"},{"id":"192957","messageId":"1920266.XH2GkcxZYT@flobuntu","threadId":"30705","inReplyTo":"20120605090702.GA27376@sigill.intra.peff.net","subject":"Re: [RFC 4/4] Add cat-blob report pipe from fast-import to remote-helper.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-06-05T22:17:27Z","receivedAt":"2012-06-05T22:17:27Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"On Tuesday 05 June 2012 05:07:02 Jeff King wrote:\n> Florian, does that mean that making the svn helper start to use\n> --cat-blob-fd at all is a potential regression for Windows? \n\nActually I don't know. I'm don't know a lot about Windows support in git at \nall. \nI created the pipe, because the current vcs-svn/* code requires it for \nimporting svn dumps. I can't explain yet why it requires it.\n\n> The\n> fast-import documentation says that the cat-blob output will go to\n> stdout now. Does it even work at all now? \n\nstdout is connected to the parent, while the new pipe connects it's two \nchilds..\nIt works.\n\n> I don't really know or\n> understand all of the reasons for cat-blob-fd to exist in the first\n> place.\n\nGood point.\n\n> \n> I expect one answer might be \"well, the svn remote helper does not work\n> at all on Windows already, so there's no regression\". But this affects\n> all fast-import calls that git's transport-helper makes. Are there\n> other ones that use import, and would they be affected by this? \n> \n> For that matter, isn't this a backwards-incompatible change for other\n> third-party helpers? Won't they need to respect the new\n> GIT_REPORT_FILENO environment variable? Do we need the helper to specify\n> \"yes, I am ready to handle cat-blob-fd\" in its capabilities list?\n\nI think whenever the remote helper uses some specific commands of fast-import \nit needs the cat-blob-fd to read feedback, but I haven't digged into that \nyet..\n\n--\nFlorian Achleitner\n"},{"id":"192991","messageId":"20120606134320.GC2597@sigill.intra.peff.net","threadId":"30705","inReplyTo":"5801019.gWQEmI8V81@flobuntu","subject":"Re: [RFC 4/4] Add cat-blob report pipe from fast-import to remote-helper.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-06T13:43:21Z","receivedAt":"2012-06-06T13:43:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 06, 2012 at 12:16:25AM +0200, Florian Achleitner wrote:\n\n> On Tuesday 05 June 2012 04:14:02 Jeff King wrote:\n> > Is there a reason that the patch unconditionally creates the pipe in\n> > get_helper? I.e., isn't it specific to the get_importer code path? It\n> > feels a little hacky to have it infect the other code paths.\n> \n> I agree, it's a bit hacky. For me as a newbee, it was just a way to make fast-\n> import have the pipe it needs. I didn't know about the history of the \n> preexec_cb as a fix for a bug in less.\n> \n> The pipe is created unconditionally, because at the fork-time of the remote-\n> helper it is not known whether the import command will be used later together \n> with fast-import, or not. (and later, there's no way, I think).\n> Helpers that don't use the pipe could simply ignore it.\n\nGood point. I think we really are stuck with doing it in every case,\nunless we want to turn to something that can be opened after the fact\n(like a fifo).\n\n-Peff\n"},{"id":"193014","messageId":"1455162.h3ffdVdFSZ@flobuntu","threadId":"30705","inReplyTo":"20120606134320.GC2597@sigill.intra.peff.net","subject":"Re: [RFC 4/4] Add cat-blob report pipe from fast-import to remote-helper.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-06-06T21:04:18Z","receivedAt":"2012-06-06T21:04:18Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"On Wednesday 06 June 2012 09:43:21 you wrote:\n> Good point. I think we really are stuck with doing it in every case,\n> unless we want to turn to something that can be opened after the fact\n> (like a fifo).\n\nA fifo could be clever. There's also something similar on windows, maybe we can \nuse named pipes there too.\n\n> \n> -Peff\n\n-- Flo\n"},{"id":"194425","messageId":"1374057.qfvOg1c6C6@flobuntu","threadId":"30705","inReplyTo":"1338830455-3091-1-git-send-email-florian.achleitner.2.6.31@gmail.com","subject":"[RFC 0/4 v2]","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-06-29T07:49:16Z","receivedAt":"2012-06-29T07:49:16Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"This new version uses a fifo instead of a pipe and addresses other issues \ndiscussed in this thread.\nTo pass the name of the fifo to fast-import, it gets a new cmd-line-arg.\nIt no longer requires the prexec_cb and should be more portable, as there \nexist pipes on windows too.\n\n\n\nOn Monday 04 June 2012 19:20:51 Florian Achleitner wrote:\n> Series of patches creating a very basic remote helper in C.\n> \n> [RFC 1/4] Implement a basic remote helper vor svn in C.\n> [RFC 2/4] Integrate remote-svn into svn-fe/Makefile.\n> [RFC 3/4] Add svndump_init_fd to allow reading dumps from arbitrary\n> [RFC 4/4] Add cat-blob report pipe from fast-import to\n"},{"id":"194429","messageId":"23122876.7xH9dZiP4M@flobuntu","threadId":"30705","inReplyTo":"1374057.qfvOg1c6C6@flobuntu","subject":"[RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-06-29T07:54:51Z","receivedAt":"2012-06-29T07:54:51Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"Experimental implementation.\nInspired by the existing shell script at divanorama/remote-svn-alpha.\nIt doesn't use marks or notes yet, always imports the full history.\n\nsvnrdump is started as a subprocess while svn-fe's functions\nare called directly.\n\nSigned-off-by: Florian Achleitner \n---\ndiff: Use fifo instead of pipe: Retrieve the name of the pipe from env and open it\nfor svndump.\n\n contrib/svn-fe/remote-svn.c | 207 +++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 207 insertions(+)\n create mode 100644 contrib/svn-fe/remote-svn.c\n\ndiff --git a/contrib/svn-fe/remote-svn.c b/contrib/svn-fe/remote-svn.c\nnew file mode 100644\nindex 0000000..5ec7fbb\n--- /dev/null\n+++ b/contrib/svn-fe/remote-svn.c\n@@ -0,0 +1,207 @@\n+\n+#include \n+#include \n+#include \n+#include \"cache.h\"\n+#include \"remote.h\"\n+#include \"strbuf.h\"\n+#include \"url.h\"\n+#include \"exec_cmd.h\"\n+#include \"run-command.h\"\n+#include \"svndump.h\"\n+\n+static int debug = 0;\n+\n+static inline void printd(const char* fmt, ...)\n+{\n+\tif(debug) {\n+\t\tva_list vargs;\n+\t\tva_start(vargs, fmt);\n+\t\tfprintf(stderr, \"rhsvn debug: \");\n+\t\tvfprintf(stderr, fmt, vargs);\n+\t\tfprintf(stderr, \"\\n\");\n+\t\tva_end(vargs);\n+\t}\n+}\n+\n+static struct remote* remote;\n+static const char* url;\n+const char* private_refs = \"refs/remote-svn/\";\t\t/* + remote->name. */\n+const char* remote_ref = \"refs/heads/master\";\n+\n+enum cmd_result cmd_capabilities(struct strbuf* line);\n+enum cmd_result cmd_import(struct strbuf* line);\n+enum cmd_result cmd_list(struct strbuf* line);\n+\n+enum cmd_result { SUCCESS, NOT_HANDLED, ERROR };\n+typedef enum cmd_result (*command)(struct strbuf*);\n+\n+const command command_list[] = {\n+\t\tcmd_capabilities, cmd_import, cmd_list, NULL\n+};\n+\n+enum cmd_result cmd_capabilities(struct strbuf* line)\n+{\n+\tif(strcmp(line->buf, \"capabilities\"))\n+\t\treturn NOT_HANDLED;\n+\n+\tprintf(\"import\\n\");\n+\tprintf(\"\\n\");\n+\tfflush(stdout);\n+\treturn SUCCESS;\n+}\n+\n+enum cmd_result cmd_import(struct strbuf* line)\n+{\n+\tconst char* revs = \"-r0:HEAD\";\n+\tint code, report_fd;\n+\tchar* back_pipe_env;\n+\tstruct child_process svndump_proc = {\n+\t\t\t.argv = NULL,\t\t/* comes later .. */\n+\t\t\t/* we want a pipe to the child's stdout, but stdin, stderr inherited.\n+\t\t\t The user can be asked for e.g. a password */\n+\t\t\t.in = 0, .out = -1, .err = 0,\n+\t\t\t.no_stdin = 0, .no_stdout = 0, .no_stderr = 0,\n+\t\t\t.git_cmd = 0,\n+\t\t\t.silent_exec_failure = 0,\n+\t\t\t.stdout_to_stderr = 0,\n+\t\t\t.use_shell = 0,\n+\t\t\t.clean_on_exit = 0,\n+\t\t\t.preexec_cb = NULL,\n+\t\t\t.env = NULL,\n+\t\t\t.dir = NULL\n+\t};\n+\n+\tif(prefixcmp(line->buf, \"import\"))\n+\t\treturn NOT_HANDLED;\n+\n+\tback_pipe_env = getenv(\"GIT_REPORT_FIFO\");\n+\tif(!back_pipe_env) {\n+\t\tdie(\"Cannot get cat-blob-pipe from environment!\");\n+\t}\n+\n+\t/* opening a fifo for usually reading blocks until a writer has opened it too.\n+\t * Therefore, we open with RDWR.\n+\t */\n+\treport_fd = open(back_pipe_env, O_RDWR);\n+\tif(report_fd < 0) {\n+\t\tdie(\"Unable to open fast-import back-pipe! %s\", strerror(errno));\n+\t}\n+\n+\tprintd(\"Opened fast-import back-pipe %s for reading.\", back_pipe_env);\n+\n+\tsvndump_proc.argv = xcalloc(5, sizeof(char*));\n+\tsvndump_proc.argv[0] = \"svnrdump\";\n+\tsvndump_proc.argv[1] = \"dump\";\n+\tsvndump_proc.argv[2] = url;\n+\tsvndump_proc.argv[3] = revs;\n+\n+\tcode = start_command(&svndump_proc);\n+\tif(code)\n+\t\tdie(\"Unable to start %s, code %d\", svndump_proc.argv[0], code);\n+\n+\n+\n+\tsvndump_init_fd(svndump_proc.out, report_fd);\n+\tsvndump_read(url);\n+\tsvndump_deinit();\n+\tsvndump_reset();\n+\n+\tclose(svndump_proc.out);\n+\tclose(report_fd);\n+\n+\tcode = finish_command(&svndump_proc);\n+\tif(code)\n+\t\twarning(\"Something went wrong with termination of %s, code %d\", svndump_proc.argv[0], code);\n+\tfree(svndump_proc.argv);\n+\n+\tprintf(\"done\\n\");\n+\treturn SUCCESS;\n+\n+\n+\n+}\n+\n+enum cmd_result cmd_list(struct strbuf* line)\n+{\n+\tif(strcmp(line->buf, \"list\"))\n+\t\treturn NOT_HANDLED;\n+\n+\tprintf(\"? HEAD\\n\");\n+\tprintf(\"? %s\\n\", remote_ref);\n+\tprintf(\"\\n\");\n+\tfflush(stdout);\n+\treturn SUCCESS;\n+}\n+\n+enum cmd_result do_command(struct strbuf* line)\n+{\n+\tconst command* p = command_list;\n+\tenum cmd_result ret;\n+\tprintd(\"command line '%s'\", line->buf);\n+\twhile(*p) {\n+\t\tret = (*p)(line);\n+\t\tif(ret != NOT_HANDLED)\n+\t\t\treturn ret;\n+\t\tp++;\n+\t}\n+\twarning(\"Unknown command '%s'\\n\", line->buf);\n+\treturn ret;\n+}\n+\n+int main(int argc, const char **argv)\n+{\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint nongit;\n+\n+\tif (getenv(\"GIT_TRANSPORT_HELPER_DEBUG\"))\n+\t\tdebug = 1;\n+\n+\tgit_extract_argv0_path(argv[0]);\n+\tsetup_git_directory_gently(&nongit);\n+\tif (argc < 2) {\n+\t\tfprintf(stderr, \"Remote needed\\n\");\n+\t\treturn 1;\n+\t}\n+\n+\tremote = remote_get(argv[1]);\n+\tif (argc == 3) {\n+\t\tend_url_with_slash(&buf, argv[2]);\n+\t} else if (argc == 2) {\n+\t\tend_url_with_slash(&buf, remote->url[0]);\n+\t} else {\n+\t\twarning(\"Excess arguments!\");\n+\t}\n+\n+\turl = strbuf_detach(&buf, NULL);\n+\n+\tprintd(\"remote-svn starting with url %s\", url);\n+\n+\t/* build private ref namespace path for this svn remote. */\n+\tstrbuf_init(&buf, 0);\n+\tstrbuf_addstr(&buf, private_refs);\n+\tstrbuf_addstr(&buf, remote->name);\n+\tstrbuf_addch(&buf, '/');\n+\tprivate_refs = strbuf_detach(&buf, NULL);\n+\n+\twhile(1) {\n+\t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF) {\n+\t\t\tif (ferror(stdin))\n+\t\t\t\tfprintf(stderr, \"Error reading command stream\\n\");\n+\t\t\telse\n+\t\t\t\tfprintf(stderr, \"Unexpected end of command stream\\n\");\n+\t\t\treturn 1;\n+\t\t}\n+\t\t/* an empty line terminates the command stream */\n+\t\tif(buf.len == 0)\n+\t\t\tbreak;\n+\n+\t\tdo_command(&buf);\n+\t\tstrbuf_reset(&buf);\n+\t}\n+\n+\tstrbuf_release(&buf);\n+\tfree((void*)url);\n+\tfree((void*)private_refs);\n+\treturn 0;\n+}\n-- \n1.7.9.5\n"},{"id":"194428","messageId":"2647822.XIsT94IYqY@flobuntu","threadId":"30705","inReplyTo":"1374057.qfvOg1c6C6@flobuntu","subject":"[RFC 2/4 v2] Integrate remote-svn into svn-fe/Makefile.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-06-29T07:58:56Z","receivedAt":"2012-06-29T07:58:56Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"Requires some sha.h to be used and the libraries\nto be linked, this is currently hardcoded.\n\nSigned-off-by: Florian Achleitner \n---\ndiff: add -Wdeclaration-after-statement to CFLAGS\n\n contrib/svn-fe/Makefile | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/contrib/svn-fe/Makefile b/contrib/svn-fe/Makefile\nindex 360d8da..49b91e6 100644\n--- a/contrib/svn-fe/Makefile\n+++ b/contrib/svn-fe/Makefile\n@@ -1,14 +1,14 @@\n-all:: svn-fe$X\n+all:: svn-fe$X remote-svn$X\n \n CC = gcc\n RM = rm -f\n MV = mv\n \n-CFLAGS = -g -O2 -Wall\n+CFLAGS = -g -O2 -Wall -DSHA1_HEADER='' -Wdeclaration-after-statement\n LDFLAGS =\n ALL_CFLAGS = $(CFLAGS)\n ALL_LDFLAGS = $(LDFLAGS)\n-EXTLIBS =\n+EXTLIBS = -lssl -lcrypto -lpthread ../../xdiff/lib.a\n \n GIT_LIB = ../../libgit.a\n VCSSVN_LIB = ../../vcs-svn/lib.a\n@@ -37,8 +37,12 @@ svn-fe$X: svn-fe.o $(VCSSVN_LIB) $(GIT_LIB)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ svn-fe.o \\\n \t\t$(ALL_LDFLAGS) $(LIBS)\n \n-svn-fe.o: svn-fe.c ../../vcs-svn/svndump.h\n-\t$(QUIET_CC)$(CC) -I../../vcs-svn -o $*.o -c $(ALL_CFLAGS) $<\n+remote-svn$X: remote-svn.o $(VCSSVN_LIB) $(GIT_LIB)\n+\t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ remote-svn.o \\\n+\t\t$(ALL_LDFLAGS) $(LIBS)\n+\t\t\n+%.o: %.c ../../vcs-svn/svndump.h\n+\t$(QUIET_CC)$(CC) -I../../vcs-svn -I../../ -o $*.o -c $(ALL_CFLAGS) $<\n \n svn-fe.html: svn-fe.txt\n \t$(QUIET_SUBDIR0)../../Documentation $(QUIET_SUBDIR1) \\\n-- \n1.7.9.5\n"},{"id":"194427","messageId":"1790879.5n53DiMT9G@flobuntu","threadId":"30705","inReplyTo":"1374057.qfvOg1c6C6@flobuntu","subject":"[RFC 3/4 v2] Add svndump_init_fd to allow reading dumps from arbitrary FDs.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-06-29T07:59:23Z","receivedAt":"2012-06-29T07:59:23Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"The existing function only allowed reading from a filename or\nfrom stdin. Allow passing of a FD and an additional FD for\nthe back report pipe. This allows us to retrieve the name of\nthe pipe in the caller.\n\nFixes the filename could be NULL bug.\n\nSigned-off-by: Florian Achleitner \n---\n vcs-svn/svndump.c | 22 ++++++++++++++++++----\n vcs-svn/svndump.h | 1 +\n 2 files changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c\nindex 0899790..eb76bf8 100644\n--- a/vcs-svn/svndump.c\n+++ b/vcs-svn/svndump.c\n@@ -465,11 +465,9 @@ void svndump_read(const char *url)\n \t\tend_revision();\n }\n \n-int svndump_init(const char *filename)\n+static void init(int report_fd)\n {\n-\tif (buffer_init(&input, filename))\n-\t\treturn error(\"cannot open %s: %s\", filename, strerror(errno));\n-\tfast_export_init(REPORT_FILENO);\n+\tfast_export_init(report_fd);\n \tstrbuf_init(&dump_ctx.uuid, 4096);\n \tstrbuf_init(&dump_ctx.url, 4096);\n \tstrbuf_init(&rev_ctx.log, 4096);\n@@ -479,6 +477,22 @@ int svndump_init(const char *filename)\n \treset_dump_ctx(NULL);\n \treset_rev_ctx(0);\n \treset_node_ctx(NULL);\n+\treturn;\n+}\n+\n+int svndump_init(const char *filename)\n+{\n+\tif (buffer_init(&input, filename))\n+\t\treturn error(\"cannot open %s: %s\", filename ? filename : \"NULL\", strerror(errno));\n+\tinit(REPORT_FILENO);\n+\treturn 0;\n+}\n+\n+int svndump_init_fd(int in_fd, int back_fd)\n+{\n+\tif(buffer_fdinit(&input, in_fd))\n+\t\treturn error(\"cannot open fd %d: %s\", in_fd, strerror(errno));\n+\tinit(back_fd);\n \treturn 0;\n }\n \ndiff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h\nindex df9ceb0..acb5b47 100644\n--- a/vcs-svn/svndump.h\n+++ b/vcs-svn/svndump.h\n@@ -2,6 +2,7 @@\n #define SVNDUMP_H_\n \n int svndump_init(const char *filename);\n+int svndump_init_fd(int in_fd, int back_fd);\n void svndump_read(const char *url);\n void svndump_deinit(void);\n void svndump_reset(void);\n-- \n1.7.9.5\n"},{"id":"194426","messageId":"1415957.ivnctqiWQE@flobuntu","threadId":"30705","inReplyTo":"1374057.qfvOg1c6C6@flobuntu","subject":"[RFC 4/4 v2] Add cat-blob report fifo from fast-import to remote-helper.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-06-29T08:00:22Z","receivedAt":"2012-06-29T08:00:22Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"For some fast-import commands (e.g. cat-blob) an answer-channel\nis required. For this purpose a fifo (aka named pipe) (mkfifo)\nis created (.git/fast-import-report-fifo) by the transport-helper\nwhen fetch via import is requested. The remote-helper and\nfast-import open the ends of the pipe.\n\nThe filename of the fifo is passed to the remote-helper via\nit's environment, helpers that don't use fast-import can\nsimply ignore it.\nAdd a new command line option --cat-blob-pipe to fast-import,\nfor this purpose.\n\nUse argv_arrays in get_helper and get_importer.\n\nSigned-off-by: Florian Achleitner \n---\n fast-import.c | 15 ++++++++++++\n transport-helper.c | 64 ++++++++++++++++++++++++++++++++++++++++------------\n 2 files changed, 64 insertions(+), 15 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex eed97c8..44cb124 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3180,6 +3180,16 @@ static void option_cat_blob_fd(const char *fd)\n \tcat_blob_fd = (int) n;\n }\n \n+static void option_cat_blob_pipe(const char *name)\n+{\n+\tint report_fd = open(name, O_WRONLY);\n+\twarning(\"Opened pipe %s.\", name);\n+\tif(report_fd < 0) {\n+\t\tdie(\"Unable to open fast-import back-pipe! %s\", strerror(errno));\n+\t}\n+\tcat_blob_fd = report_fd;\n+}\n+\n static void option_export_pack_edges(const char *edges)\n {\n \tif (pack_edges)\n@@ -3337,6 +3347,11 @@ static void parse_argv(void)\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif(!prefixcmp(a + 2, \"cat-blob-pipe=\")) {\n+\t\t\toption_cat_blob_pipe(a + 2 + strlen(\"cat-blob-pipe=\"));\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tdie(\"unknown option %s\", a);\n \t}\n \tif (i != global_argc)\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 61c928f..616db91 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -10,6 +10,7 @@\n #include \"string-list.h\"\n #include \"thread-utils.h\"\n #include \"sigchain.h\"\n+#include \"argv-array.h\"\n \n static int debug;\n \n@@ -17,6 +18,7 @@ struct helper_data {\n \tconst char *name;\n \tstruct child_process *helper;\n \tFILE *out;\n+\tchar *report_fifo;\n \tunsigned fetch : 1,\n \t\timport : 1,\n \t\texport : 1,\n@@ -101,6 +103,7 @@ static void do_take_over(struct transport *transport)\n static struct child_process *get_helper(struct transport *transport)\n {\n \tstruct helper_data *data = transport->data;\n+\tstruct argv_array argv = ARGV_ARRAY_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct child_process *helper;\n \tconst char **refspecs = NULL;\n@@ -111,6 +114,7 @@ static struct child_process *get_helper(struct transport *transport)\n \tchar git_dir_buf[sizeof(GIT_DIR_ENVIRONMENT) + PATH_MAX + 1];\n \tconst char *helper_env[] = {\n \t\tgit_dir_buf,\n+\t\tNULL,\t/* placeholder */\n \t\tNULL\n \t};\n \n@@ -122,17 +126,23 @@ static struct child_process *get_helper(struct transport *transport)\n \thelper->in = -1;\n \thelper->out = -1;\n \thelper->err = 0;\n-\thelper->argv = xcalloc(4, sizeof(*helper->argv));\n-\tstrbuf_addf(&buf, \"git-remote-%s\", data->name);\n-\thelper->argv[0] = strbuf_detach(&buf, NULL);\n-\thelper->argv[1] = transport->remote->name;\n-\thelper->argv[2] = remove_ext_force(transport->url);\n+\targv_array_pushf(&argv, \"git-remote-%s\", data->name);\n+\targv_array_push(&argv, transport->remote->name);\n+\targv_array_push(&argv, remove_ext_force(transport->url));\n+\thelper->argv = argv.argv;\n \thelper->git_cmd = 0;\n \thelper->silent_exec_failure = 1;\n \n \tsnprintf(git_dir_buf, sizeof(git_dir_buf), \"%s=%s\", GIT_DIR_ENVIRONMENT, get_git_dir());\n \thelper->env = helper_env;\n \n+\tstrbuf_init(&buf, 0);\n+\tstrbuf_addf(&buf, \"%s/fast-import-report-fifo\", get_git_dir());\n+\tdata->report_fifo = strbuf_detach(&buf, NULL);\n+\tstrbuf_init(&buf, 0);\n+\tstrbuf_addf(&buf, \"GIT_REPORT_FIFO=%s\", data->report_fifo);\n+\thelper_env[1] = strbuf_detach(&buf, NULL);\n+\n \tcode = start_command(helper);\n \tif (code < 0 && errno == ENOENT)\n \t\tdie(\"Unable to find remote helper for '%s'\", data->name);\n@@ -141,6 +151,8 @@ static struct child_process *get_helper(struct transport *transport)\n \n \tdata->helper = helper;\n \tdata->no_disconnect_req = 0;\n+\tfree((void*) helper_env[1]);\n+\targv_array_clear(&argv);\n \n \t/*\n \t * Open the output as FILE* so strbuf_getline() can be used.\n@@ -237,13 +249,13 @@ static int disconnect_helper(struct transport *transport)\n \t\t\txwrite(data->helper->in, \"\\n\", 1);\n \t\t\tsigchain_pop(SIGPIPE);\n \t\t}\n+\n \t\tclose(data->helper->in);\n \t\tclose(data->helper->out);\n \t\tfclose(data->out);\n \t\tres = finish_command(data->helper);\n-\t\tfree((char *)data->helper->argv[0]);\n-\t\tfree(data->helper->argv);\n \t\tfree(data->helper);\n+\t\tfree(data->report_fifo);\n \t\tdata->helper = NULL;\n \t}\n \treturn res;\n@@ -373,16 +385,18 @@ static int fetch_with_fetch(struct transport *transport,\n \treturn 0;\n }\n \n-static int get_importer(struct transport *transport, struct child_process *fastimport)\n+static int get_importer(struct transport *transport, struct child_process *fastimport, struct argv_array *argv)\n {\n \tstruct child_process *helper = get_helper(transport);\n+\tstruct helper_data *data = transport->data;\n \tmemset(fastimport, 0, sizeof(*fastimport));\n \tfastimport->in = helper->out;\n-\tfastimport->argv = xcalloc(5, sizeof(*fastimport->argv));\n-\tfastimport->argv[0] = \"fast-import\";\n-\tfastimport->argv[1] = \"--quiet\";\n-\n+\targv_array_push(argv, \"fast-import\");\n+\targv_array_push(argv, \"--quiet\");\n+\targv_array_pushf(argv, \"--cat-blob-pipe=%s\", data->report_fifo);\n+\tfastimport->argv = argv->argv;\n \tfastimport->git_cmd = 1;\n+\n \treturn start_command(fastimport);\n }\n \n@@ -421,10 +435,30 @@ static int fetch_with_import(struct transport *transport,\n \tint i;\n \tstruct ref *posn;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct argv_array importer_argv = ARGV_ARRAY_INIT;\n+\tstruct stat fifostat;\n+\n+\t/* create a fifo for back-reporting of fast-import to the remote helper,\n+\t * if it doesn't exist. */\n+\tif(!stat(data->report_fifo, &fifostat)) {\n+\t\tif(S_ISFIFO(fifostat.st_mode)) {\t\t/* exists and is fifo, unlink and recreate, to be sure that permissions are ok.. */\n+\t\t\tif (debug)\n+\t\t\t\tfprintf(stderr, \"Debug: Remote helper: Unlinked existing fifo.\\n\");\n+\t\t\tif(unlink(data->report_fifo))\n+\t\t\t\tdie_errno(\"Couldn't unlink fifo %s\", data->report_fifo);\n+\t\t}\n+\t\telse\n+\t\t\tdie(\"Fifo %s used by some other file.\", data->report_fifo);\n+\t}\n+\tif(mkfifo(data->report_fifo, 0660))\n+\t\tdie_errno(\"Couldn't create fifo %s\", data->report_fifo);\n+\tif (debug)\n+\t\tfprintf(stderr, \"Debug: Remote helper: Mkfifo %s\\n\", data->report_fifo);\n+\n \n \tget_helper(transport);\n \n-\tif (get_importer(transport, &fastimport))\n+\tif (get_importer(transport, &fastimport, &importer_argv))\n \t\tdie(\"Couldn't run fast-import\");\n \n \tfor (i = 0; i < nr_heads; i++) {\n@@ -441,8 +475,8 @@ static int fetch_with_import(struct transport *transport,\n \n \tif (finish_command(&fastimport))\n \t\tdie(\"Error while running fast-import\");\n-\tfree(fastimport.argv);\n-\tfastimport.argv = NULL;\n+\n+\targv_array_clear(&importer_argv);\n \n \tfor (i = 0; i < nr_heads; i++) {\n \t\tchar *private;\n-- \n1.7.9.5\n"},{"id":"194466","messageId":"20120702110741.GA3527@burratino","threadId":"30705","inReplyTo":"23122876.7xH9dZiP4M@flobuntu","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-02T11:07:41Z","receivedAt":"2012-07-02T11:07:41Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nFlorian Achleitner wrote:\n\n> Experimental implementation.\n\nOk, so this adds a new program named \"remote-svn\". How do I build it?\nWhat does it do? Will it make my life better?\n\n[...]\n> diff: Use fifo instead of pipe: Retrieve the name of the pipe from env and open it\n> for svndump.\n\nI'd prefer to avoid this if possible, since it means having to decide\nwhere the pipe goes on the filesystem. Can you summarize the\ndiscussion in the commit message so future readers understand why\nwe're doing it?\n\n[...]\n> --- /dev/null\n> +++ b/contrib/svn-fe/remote-svn.c\n> @@ -0,0 +1,207 @@\n> +\n> +#include \n> +#include \n> +#include \n\ngit-compat-util.h (or some header that includes it) must be the first\nheader included so the appropriate feature test macros can be defined.\nSee Documentation/CodingGuidelines for more on that.\n\n> +#include \"cache.h\"\n> +#include \"remote.h\"\n> +#include \"strbuf.h\"\n> +#include \"url.h\"\n> +#include \"exec_cmd.h\"\n> +#include \"run-command.h\"\n> +#include \"svndump.h\"\n> +\n> +static int debug = 0;\n\nSmall nit: please drop the redundant \"= 0\" here. Or:\n\n> +\n> +static inline void printd(const char* fmt, ...)\n> +{\n> +\tif(debug) {\n> +\t\tva_list vargs;\n> +\t\tva_start(vargs, fmt);\n> +\t\tfprintf(stderr, \"rhsvn debug: \");\n> +\t\tvfprintf(stderr, fmt, vargs);\n> +\t\tfprintf(stderr, \"\\n\");\n> +\t\tva_end(vargs);\n> +\t}\n> +}\n\nWhy not use trace_printf and avoid the complication?\n\n[...]\n\n> +\n> +static struct remote* remote;\n> +static const char* url;\n> +const char* private_refs = \"refs/remote-svn/\";\t\t/* + remote->name. */\n> +const char* remote_ref = \"refs/heads/master\";\n\nStyle: '*' attaches to the variable name, to avoid making declarations\nlike\n\n\tchar *p, c;\n\nconfusing.\n\n> +\n> +enum cmd_result cmd_capabilities(struct strbuf* line);\n> +enum cmd_result cmd_import(struct strbuf* line);\n> +enum cmd_result cmd_list(struct strbuf* line);\n\nWhat's a cmd_result? '*' sticks to variable name.\n\n> +\n> +enum cmd_result { SUCCESS, NOT_HANDLED, ERROR };\n\nOh, that's what a cmd_result is. :) Why not define the type before\nusing it to avoid keeping the reader in suspense?\n\nWhat does each result represent? If this is a convention like\n\n 1: handled\n 0: not handled\n -1: error, callee takes care of printing the error message\n\nthen please document it in a comment near the caller so the reader can\nunderstand what is happening without too much confusion. Given such a\ncomment, does the enum add clarity?\n\n> +typedef enum cmd_result (*command)(struct strbuf*);\n\nWhen I first read this, I wonder what is being commanded. Are these\ncommands passed on the remote helper's standard input, commands passed\non its output, or commands run at some point in the process? What is\nthe effect and return value of associated function? Does the function\nalways return some success/failure value, or does it sometimes exit?\n\nMaybe a more specific type name would be clearer?\n\n[...]\n> +\n> +const command command_list[] = {\n> +\t\tcmd_capabilities, cmd_import, cmd_list, NULL\n> +};\n\nFirst association is to functions like cmd_fetch() which implement git\nsubcommands. So I thought these were going to implement subcommands\nlike \"git remote-svn capabilities\", \"git remote-svn import\" and would\nuse the same cmd_foo(argc, argv, prefix) calling convention that git\nsubcommands do. Maybe a different naming convention could avoid\nconfusion.\n\n[...]\n> +enum cmd_result cmd_capabilities(struct strbuf* line)\n> +{\n> +\tif(strcmp(line->buf, \"capabilities\"))\n> +\t\treturn NOT_HANDLED;\n\nStyle: missing SP after keyword.\n\n> +\n> +\tprintf(\"import\\n\");\n> +\tprintf(\"\\n\");\n> +\tfflush(stdout);\n> +\treturn SUCCESS;\n> +}\n\nWhy the multiple printf? Is the flush needed?\n\n[...]\n> +\n> +enum cmd_result cmd_import(struct strbuf* line)\n> +{\n> +\tconst char* revs = \"-r0:HEAD\";\n\nStyle: * goes with ... (I won't point out the rest of these.\n\n> +\tint code, report_fd;\n> +\tchar* back_pipe_env;\n> +\tstruct child_process svndump_proc = {\n> +\t\t\t.argv = NULL,\t\t/* comes later .. */\n\nI don't understand this comment.\n\n> +\t\t\t/* we want a pipe to the child's stdout, but stdin, stderr inherited.\n> +\t\t\t The user can be asked for e.g. a password */\n> +\t\t\t.in = 0, .out = -1, .err = 0,\n\nStyle: comments in git are spelled like this:\n\n\t\t\t/*\n\t\t\t * Here I put a sentence or two explaining some\n\t\t\t * relevant design decision or fact about the world\n\t\t\t * that will provide useful context for\n\t\t\t * understanding the following code.\n\t\t\t */\n> +\t\t\t.no_stdin = 0, .no_stdout = 0, .no_stderr = 0,\n\nI couldn't parse the above comment, so I'm skipping it for now.\n\n[...]\n> +\t\t\t.git_cmd = 0,\n> +\t\t\t.silent_exec_failure = 0,\n> +\t\t\t.stdout_to_stderr = 0,\n> +\t\t\t.use_shell = 0,\n> +\t\t\t.clean_on_exit = 0,\n> +\t\t\t.preexec_cb = NULL,\n> +\t\t\t.env = NULL,\n> +\t\t\t.dir = NULL\n\nStyle: C99-style initializers are (unfortunately) not supported in\nsome compilers we want to support.\n\nNo need to initialize all fields --- any trailing unlisted fields\nare automatically initialized to zero.\n\n> +\t};\n> +\n> +\tif(prefixcmp(line->buf, \"import\"))\n\nStyle: missing SP after keyword (I won't point out the rest of these).\n\n> +\t\treturn NOT_HANDLED;\n> +\n> +\tback_pipe_env = getenv(\"GIT_REPORT_FIFO\");\n> +\tif(!back_pipe_env) {\n> +\t\tdie(\"Cannot get cat-blob-pipe from environment!\");\n> +\t}\n\nDoes this mean that expected usage is something like\n\n\tGIT_REPORT_FIFO=/tmp/foo/bar git clone svn::foo/bar/baz\n\n? And if I don't do that, I get\n\n\tfatal: Cannot get cat-blob-pipe from environment!\n\nand am somehow supposed to understand what to do?\n\n> +\n> +\t/* opening a fifo for usually reading blocks until a writer has opened it too.\n> +\t * Therefore, we open with RDWR.\n> +\t */\n> +\treport_fd = open(back_pipe_env, O_RDWR);\n> +\tif(report_fd < 0) {\n> +\t\tdie(\"Unable to open fast-import back-pipe! %s\", strerror(errno));\n> +\t}\n\nIs this necessary? Why shouldn't we fork the writer first and wait\nfor it here?\n\n> +\n> +\tprintd(\"Opened fast-import back-pipe %s for reading.\", back_pipe_env);\n> +\n> +\tsvndump_proc.argv = xcalloc(5, sizeof(char*));\n> +\tsvndump_proc.argv[0] = \"svnrdump\";\n> +\tsvndump_proc.argv[1] = \"dump\";\n> +\tsvndump_proc.argv[2] = url;\n> +\tsvndump_proc.argv[3] = revs;\n\nStyle: could simplify by using struct argv_array.\n\n> +\n> +\tcode = start_command(&svndump_proc);\n> +\tif(code)\n> +\t\tdie(\"Unable to start %s, code %d\", svndump_proc.argv[0], code);\n\nstart_command() is supposed to have printed a message already when it\nfails, unless errno == ENOENT and silent_exec_failure was set.\n\n> +\n> +\n> +\n\nStyle: looks like some stray carriage returns snuck in.\n\n> +\tsvndump_init_fd(svndump_proc.out, report_fd);\n> +\tsvndump_read(url);\n> +\tsvndump_deinit();\n> +\tsvndump_reset();\n\nNot your fault: this API looks a little overcomplicated.\n\n> +\n> +\tclose(svndump_proc.out);\n\nImportant? Wouldn't finish_command do this?\n\n> +\tclose(report_fd);\n\nWhat is the purpose of this step?\n\n> +\n> +\tcode = finish_command(&svndump_proc);\n> +\tif(code)\n> +\t\twarning(\"Something went wrong with termination of %s, code %d\", svndump_proc.argv[0], code);\n\nfinish_command() is supposed to print a message when it fails.\n\n> +\tfree(svndump_proc.argv);\n> +\n> +\tprintf(\"done\\n\");\n> +\treturn SUCCESS;\n\nSuccess even if it failed?\n\n> +\n> +\n> +\n\nBlank lines seem to have snuck in.\n\n> +}\n> +\n> +enum cmd_result cmd_list(struct strbuf* line)\n> +{\n> +\tif(strcmp(line->buf, \"list\"))\n> +\t\treturn NOT_HANDLED;\n> +\n> +\tprintf(\"? HEAD\\n\");\n> +\tprintf(\"? %s\\n\", remote_ref);\n\nWhy is this variable?\n\n> +\tprintf(\"\\n\");\n> +\tfflush(stdout);\n\nWhy the flush?\n\n> +\treturn SUCCESS;\n> +}\n> +\n> +enum cmd_result do_command(struct strbuf* line)\n> +{\n> +\tconst command* p = command_list;\n> +\tenum cmd_result ret;\n> +\tprintd(\"command line '%s'\", line->buf);\n> +\twhile(*p) {\n> +\t\tret = (*p)(line);\n> +\t\tif(ret != NOT_HANDLED)\n> +\t\t\treturn ret;\n> +\t\tp++;\n> +\t}\n\nIf possible, matching commands by name (like git.c does) would make\nthe behavior easier to predict.\n\n[...]\n> +\tif (argc < 2) {\n> +\t\tfprintf(stderr, \"Remote needed\\n\");\n> +\t\treturn 1;\n> +\t}\n\nusage() can be used to write a clearer error message.\n\n[...]\n> +\n> +\tremote = remote_get(argv[1]);\n> +\tif (argc == 3) {\n> +\t\tend_url_with_slash(&buf, argv[2]);\n> +\t} else if (argc == 2) {\n> +\t\tend_url_with_slash(&buf, remote->url[0]);\n> +\t} else {\n> +\t\twarning(\"Excess arguments!\");\n> +\t}\n\nStyle: no need for these braces. usage() could be used to make it\nclearer to the user what she can do next.\n\n[...]\n> +\t/* build private ref namespace path for this svn remote. */\n> +\tstrbuf_init(&buf, 0);\n> +\tstrbuf_addstr(&buf, private_refs);\n> +\tstrbuf_addstr(&buf, remote->name);\n> +\tstrbuf_addch(&buf, '/');\n> +\tprivate_refs = strbuf_detach(&buf, NULL);\n\nWhat is a private ref namespace path? An example would make the comment\nclearer.\n\n> +\n> +\twhile(1) {\n> +\t\tif (strbuf_getline(&buf, stdin, '\\n') == EOF) {\n> +\t\t\tif (ferror(stdin))\n> +\t\t\t\tfprintf(stderr, \"Error reading command stream\\n\");\n\nerrno will be meaningful; the message can be made clearer by using it.\n\nMaybe this could use error() or die().\n\n[...]\n> +\tfree((void*)url);\n> +\tfree((void*)private_refs);\n\nWon't this crash?\n\nIt would also be nice to add a test case to the t/ directory to make others\nchanging this code do not accidentally break your new functionality.\n\nHope that helps,\nJonathan\n"},{"id":"194689","messageId":"20120706003023.GA15387@burratino","threadId":"30705","inReplyTo":"20120702110741.GA3527@burratino","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-06T00:30:24Z","receivedAt":"2012-07-06T00:30:24Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Florian Achleitner wrote:\n\n>> Experimental implementation.\n>\n> Ok, so this adds a new program named \"remote-svn\". How do I build it?\n> What does it do? Will it make my life better?\n[...]\n\nI forgot to say: thanks for working on this! I hope my comments are\nnot demoralizing. They are meant in the opposite vein --- if I\nexpected you to be a one-time contributor then I would just take what\nis useful from the patch and let it be, but I would be happy to see\nmore changes from you in the future so I gave some hints to explain\nhow.\n\nThe next step is to work with the list to figure out what a second\nversion of the patch should do, and then to send that with something\nlike \"RFC/PATCH v2\" in the subject line to clarify that it supersedes\nthis one.\n\nIf you have any questions about the review or the codebase in general,\nplease don't hesitate to ask. Especially, if you get stuck on\nsomething and documentation is unhelpful, please do complain. :)\n\nHope that helps,\nJonathan\n"},{"id":"194716","messageId":"7497704.pUs9jgNDKQ@flobuntu","threadId":"30705","inReplyTo":"20120706003023.GA15387@burratino","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-07-06T10:39:02Z","receivedAt":"2012-07-06T10:39:02Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"Hi Jonathan!\n\nThanks for your review! I will come up with a new version later after finishing \nsome other feature.. \nNow I've done all exams (last 2 yesterday :) ) and submissions and commit \nmyself to git and gsoc much more.\n\nOn Thursday 05 July 2012 19:30:24 Jonathan Nieder wrote:\n> Jonathan Nieder wrote:\n> > Florian Achleitner wrote:\n> >> Experimental implementation.\n> > \n> > Ok, so this adds a new program named \"remote-svn\". How do I build it?\n> > What does it do? Will it make my life better?\n> \n> [...]\n\nI need to emphasize that it is in no way something complete and truely useful.\nIt's a starting point for the rest of my project. It will change.\nYour review will help to make it a good basis!\n\n> \n> I forgot to say: thanks for working on this! I hope my comments are\n> not demoralizing. They are meant in the opposite vein --- if I\n> expected you to be a one-time contributor then I would just take what\n> is useful from the patch and let it be, but I would be happy to see\n> more changes from you in the future so I gave some hints to explain\n> how.\n> \n> The next step is to work with the list to figure out what a second\n> version of the patch should do, and then to send that with something\n> like \"RFC/PATCH v2\" in the subject line to clarify that it supersedes\n> this one.\n\nI will come back to that soon..\n\n> \n> If you have any questions about the review or the codebase in general,\n> please don't hesitate to ask. Especially, if you get stuck on\n> something and documentation is unhelpful, please do complain. :)\n> \n> Hope that helps,\n> Jonathan\n\nFlorian\n"},{"id":"195400","messageId":"2448876.O3MA5kWbuX@flobuntu","threadId":"30705","inReplyTo":"1415957.ivnctqiWQE@flobuntu","subject":"Re: [RFC 4/4 v3] Add cat-blob report fifo from fast-import to remote-helper.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-07-21T12:45:20Z","receivedAt":"2012-07-21T12:45:20Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"For some fast-import commands (e.g. cat-blob) an answer-channel\nis required. For this purpose a fifo (aka named pipe) (mkfifo)\nis created (.git/fast-import-report-fifo) by the transport-helper\nwhen fetch via import is requested. The remote-helper and\nfast-import open the ends of the pipe.\n\nThe filename of the fifo is passed to the remote-helper via\nit's environment, helpers that don't use fast-import can\nsimply ignore it.\nAdd a new command line option --cat-blob-pipe to fast-import,\nfor this purpose.\n\nUse argv_arrays in get_helper and get_importer.\n\nOpening the pipe with O_RDWR prevents blocking open calls on both ends.\n\nSigned-off-by: Florian Achleitner \n---\ndiff: Opening the pipe with O_RDWR prevents blocking open calls on both ends.\n fast-import.c | 15 ++++++++++++\n transport-helper.c | 64 ++++++++++++++++++++++++++++++++++++++++------------\n 2 files changed, 64 insertions(+), 15 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex eed97c8..65a9341 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3180,6 +3180,16 @@ static void option_cat_blob_fd(const char *fd)\n \tcat_blob_fd = (int) n;\n }\n \n+static void option_cat_blob_pipe(const char *name)\n+{\n+\tint report_fd = open(name, O_RDWR);\n+\twarning(\"Opened pipe %s.\", name);\n+\tif(report_fd < 0) {\n+\t\tdie(\"Unable to open fast-import back-pipe! %s\", strerror(errno));\n+\t}\n+\tcat_blob_fd = report_fd;\n+}\n+\n static void option_export_pack_edges(const char *edges)\n {\n \tif (pack_edges)\n@@ -3337,6 +3347,11 @@ static void parse_argv(void)\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif(!prefixcmp(a + 2, \"cat-blob-pipe=\")) {\n+\t\t\toption_cat_blob_pipe(a + 2 + strlen(\"cat-blob-pipe=\"));\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tdie(\"unknown option %s\", a);\n \t}\n \tif (i != global_argc)\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 61c928f..616db91 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -10,6 +10,7 @@\n #include \"string-list.h\"\n #include \"thread-utils.h\"\n #include \"sigchain.h\"\n+#include \"argv-array.h\"\n \n static int debug;\n \n@@ -17,6 +18,7 @@ struct helper_data {\n \tconst char *name;\n \tstruct child_process *helper;\n \tFILE *out;\n+\tchar *report_fifo;\n \tunsigned fetch : 1,\n \t\timport : 1,\n \t\texport : 1,\n@@ -101,6 +103,7 @@ static void do_take_over(struct transport *transport)\n static struct child_process *get_helper(struct transport *transport)\n {\n \tstruct helper_data *data = transport->data;\n+\tstruct argv_array argv = ARGV_ARRAY_INIT;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct child_process *helper;\n \tconst char **refspecs = NULL;\n@@ -111,6 +114,7 @@ static struct child_process *get_helper(struct transport *transport)\n \tchar git_dir_buf[sizeof(GIT_DIR_ENVIRONMENT) + PATH_MAX + 1];\n \tconst char *helper_env[] = {\n \t\tgit_dir_buf,\n+\t\tNULL,\t/* placeholder */\n \t\tNULL\n \t};\n \n@@ -122,17 +126,23 @@ static struct child_process *get_helper(struct transport *transport)\n \thelper->in = -1;\n \thelper->out = -1;\n \thelper->err = 0;\n-\thelper->argv = xcalloc(4, sizeof(*helper->argv));\n-\tstrbuf_addf(&buf, \"git-remote-%s\", data->name);\n-\thelper->argv[0] = strbuf_detach(&buf, NULL);\n-\thelper->argv[1] = transport->remote->name;\n-\thelper->argv[2] = remove_ext_force(transport->url);\n+\targv_array_pushf(&argv, \"git-remote-%s\", data->name);\n+\targv_array_push(&argv, transport->remote->name);\n+\targv_array_push(&argv, remove_ext_force(transport->url));\n+\thelper->argv = argv.argv;\n \thelper->git_cmd = 0;\n \thelper->silent_exec_failure = 1;\n \n \tsnprintf(git_dir_buf, sizeof(git_dir_buf), \"%s=%s\", GIT_DIR_ENVIRONMENT, get_git_dir());\n \thelper->env = helper_env;\n \n+\tstrbuf_init(&buf, 0);\n+\tstrbuf_addf(&buf, \"%s/fast-import-report-fifo\", get_git_dir());\n+\tdata->report_fifo = strbuf_detach(&buf, NULL);\n+\tstrbuf_init(&buf, 0);\n+\tstrbuf_addf(&buf, \"GIT_REPORT_FIFO=%s\", data->report_fifo);\n+\thelper_env[1] = strbuf_detach(&buf, NULL);\n+\n \tcode = start_command(helper);\n \tif (code < 0 && errno == ENOENT)\n \t\tdie(\"Unable to find remote helper for '%s'\", data->name);\n@@ -141,6 +151,8 @@ static struct child_process *get_helper(struct transport *transport)\n \n \tdata->helper = helper;\n \tdata->no_disconnect_req = 0;\n+\tfree((void*) helper_env[1]);\n+\targv_array_clear(&argv);\n \n \t/*\n \t * Open the output as FILE* so strbuf_getline() can be used.\n@@ -237,13 +249,13 @@ static int disconnect_helper(struct transport *transport)\n \t\t\txwrite(data->helper->in, \"\\n\", 1);\n \t\t\tsigchain_pop(SIGPIPE);\n \t\t}\n+\n \t\tclose(data->helper->in);\n \t\tclose(data->helper->out);\n \t\tfclose(data->out);\n \t\tres = finish_command(data->helper);\n-\t\tfree((char *)data->helper->argv[0]);\n-\t\tfree(data->helper->argv);\n \t\tfree(data->helper);\n+\t\tfree(data->report_fifo);\n \t\tdata->helper = NULL;\n \t}\n \treturn res;\n@@ -373,16 +385,18 @@ static int fetch_with_fetch(struct transport *transport,\n \treturn 0;\n }\n \n-static int get_importer(struct transport *transport, struct child_process *fastimport)\n+static int get_importer(struct transport *transport, struct child_process *fastimport, struct argv_array *argv)\n {\n \tstruct child_process *helper = get_helper(transport);\n+\tstruct helper_data *data = transport->data;\n \tmemset(fastimport, 0, sizeof(*fastimport));\n \tfastimport->in = helper->out;\n-\tfastimport->argv = xcalloc(5, sizeof(*fastimport->argv));\n-\tfastimport->argv[0] = \"fast-import\";\n-\tfastimport->argv[1] = \"--quiet\";\n-\n+\targv_array_push(argv, \"fast-import\");\n+\targv_array_push(argv, \"--quiet\");\n+\targv_array_pushf(argv, \"--cat-blob-pipe=%s\", data->report_fifo);\n+\tfastimport->argv = argv->argv;\n \tfastimport->git_cmd = 1;\n+\n \treturn start_command(fastimport);\n }\n \n@@ -421,10 +435,30 @@ static int fetch_with_import(struct transport *transport,\n \tint i;\n \tstruct ref *posn;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct argv_array importer_argv = ARGV_ARRAY_INIT;\n+\tstruct stat fifostat;\n+\n+\t/* create a fifo for back-reporting of fast-import to the remote helper,\n+\t * if it doesn't exist. */\n+\tif(!stat(data->report_fifo, &fifostat)) {\n+\t\tif(S_ISFIFO(fifostat.st_mode)) {\t\t/* exists and is fifo, unlink and recreate, to be sure that permissions are ok.. */\n+\t\t\tif (debug)\n+\t\t\t\tfprintf(stderr, \"Debug: Remote helper: Unlinked existing fifo.\\n\");\n+\t\t\tif(unlink(data->report_fifo))\n+\t\t\t\tdie_errno(\"Couldn't unlink fifo %s\", data->report_fifo);\n+\t\t}\n+\t\telse\n+\t\t\tdie(\"Fifo %s used by some other file.\", data->report_fifo);\n+\t}\n+\tif(mkfifo(data->report_fifo, 0660))\n+\t\tdie_errno(\"Couldn't create fifo %s\", data->report_fifo);\n+\tif (debug)\n+\t\tfprintf(stderr, \"Debug: Remote helper: Mkfifo %s\\n\", data->report_fifo);\n+\n \n \tget_helper(transport);\n \n-\tif (get_importer(transport, &fastimport))\n+\tif (get_importer(transport, &fastimport, &importer_argv))\n \t\tdie(\"Couldn't run fast-import\");\n \n \tfor (i = 0; i < nr_heads; i++) {\n@@ -441,8 +475,8 @@ static int fetch_with_import(struct transport *transport,\n \n \tif (finish_command(&fastimport))\n \t\tdie(\"Error while running fast-import\");\n-\tfree(fastimport.argv);\n-\tfastimport.argv = NULL;\n+\n+\targv_array_clear(&importer_argv);\n \n \tfor (i = 0; i < nr_heads; i++) {\n \t\tchar *private;\n-- \n1.7.9.5\n"},{"id":"195404","messageId":"20120721144834.GB19860@burratino","threadId":"30705","inReplyTo":"2448876.O3MA5kWbuX@flobuntu","subject":"Re: [RFC 4/4 v3] Add cat-blob report fifo from fast-import to remote-helper.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-21T14:48:34Z","receivedAt":"2012-07-21T14:48:34Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nFlorian Achleitner wrote:\n\n> [Subject: Re: [RFC 4/4 v3] Add cat-blob report fifo from fast-import to\n> remote-helper.]\n\nIs this on top of patches 1, 2, and 3 from v2 of the series?\n\n*checks* Looks like it doesn't overlap with any of the files from\nthose patches, so I don't have to understand them first. Phew. My\nsuggestion for next time would be to submit patches that can be\nunderstood on their own independently instead of as part of a series.\n\n> For some fast-import commands (e.g. cat-blob) an answer-channel\n> is required. For this purpose a fifo (aka named pipe) (mkfifo)\n> is created (.git/fast-import-report-fifo) by the transport-helper\n> when fetch via import is requested. The remote-helper and\n> fast-import open the ends of the pipe.\n\nMotivation described! But it's odd --- it seems like this is\ndoing at least two things:\n\n 1) adding to the fast-import interface\n 2) using the new fast-import feature in some in-tree callers\n\nThose really want to be separate patches. That way, the fast-import\nchange can be studied by other implementers of the fast-import\ninterface (hg-fast-import, bzr-fast-import). As a side-benefit, it\ngives an easy check that any changes to fast-import were at least\nroughly backward-compatible (\"did all the in-tree users still work?\").\n\nI'll focus on the new fast-import change below, since it's the most\nimportant part.\n\n> The filename of the fifo is passed to the remote-helper via\n> it's environment, helpers that don't use fast-import can\n> simply ignore it.\n\nMy first impression is that I'd rather there be a command to request\nthe filename instead of using the environment for the first time,\nsince when debugging people would already be monitoring the command\nstream and responses.\n\n> Add a new command line option --cat-blob-pipe to fast-import,\n> for this purpose.\n\nThis is completely redundant next to --cat-blob-fd, right? That's\nreally problematic --- adding new interfaces means new code and\ngratuitous incompatibility with all existing fast-import backends,\nwith no benefit in return.\n\nI imagine that there was some portability reason you were thinking\nabout, but the above doesn't mention it at all. Future readers\nscratching their heads at the changelog can't read your mind! Please\nplease please explain what you're trying to do.\n\nSince if we're lucky fixing that could mean not having to change\nfast-import at all, I'm stopping here.\n\nAnother quick thought: any finished patch adding a new fast-import\nfeature should also include\n\n - documentation in the manpage (Documentation/fast-import.txt)\n - testcases to make sure your careful work does not get broken\n by later changes (somewhere in t/*fast-import*.sh)\n\nBut don't worry too much about that now --- sending incomplete patches\nfor review before then to make sure the direction is sane is a very\ngood idea, as long as they are marked as such (as you've already done\nby marking this as RFC).\n\nTo sum up: I think we should just stick to pipes --- why all this fifo\ncomplication?\n\nHope that helps,\nJonathan\n"},{"id":"195405","messageId":"3246520.u1PcGtbf0N@flobuntu","threadId":"30705","inReplyTo":"20120721144834.GB19860@burratino","subject":"Re: [RFC 4/4 v3] Add cat-blob report fifo from fast-import to remote-helper.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-07-21T15:24:45Z","receivedAt":"2012-07-21T15:24:45Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"On Saturday 21 July 2012 09:48:34 Jonathan Nieder wrote:\n> To sum up: I think we should just stick to pipes --- why all this fifo\n> complication?\n\nPeople didn't like pipe variant (prexec_cb not being compatible to windows' \nprocess creation model), so I learned about fifos and implemented a (basic) fifo \nvariant. *shrug*\n\n-- \nFlorian\n"},{"id":"195406","messageId":"20120721154437.GC19860@burratino","threadId":"30705","inReplyTo":"3246520.u1PcGtbf0N@flobuntu","subject":"Re: [RFC 4/4 v3] Add cat-blob report fifo from fast-import to remote-helper.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-21T15:44:37Z","receivedAt":"2012-07-21T15:44:37Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Florian Achleitner wrote:\n> On Saturday 21 July 2012 09:48:34 Jonathan Nieder wrote:\n\n>> To sum up: I think we should just stick to pipes --- why all this fifo\n>> complication?\n>\n> People didn't like pipe variant (prexec_cb not being compatible to windows' \n> process creation model), so I learned about fifos and implemented a (basic) fifo \n> variant. *shrug*\n\nOk, can you elaborate on that? What does it mean that preexec_cb is\nnot compatible to windows' process creation model? Don't the people\nof the future working on this code deserve to know about that, too, so\nthey don't break it?\n\nCome on --- I'm not asking these questions just to make your life\ndifficult. Please make it easy to understand your code changes and to\nkeep them maintained.\n"},{"id":"195416","messageId":"20120721155814.GD19860@burratino","threadId":"30705","inReplyTo":"3246520.u1PcGtbf0N@flobuntu","subject":"Re: [RFC 4/4 v3] Add cat-blob report fifo from fast-import to remote-helper.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-21T15:58:15Z","receivedAt":"2012-07-21T15:58:15Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(adding msysgit list to cc for a Windows question)\nHi,\n\n(regarding bidirectional communication between git and fast-import for\nremote helpers)\nFlorian Achleitner wrote:\n\n> People didn't like pipe variant (prexec_cb not being compatible to windows' \n> process creation model), so I learned about fifos and implemented a (basic) fifo \n> variant. *shrug*\n\nIs this meant as a summary of [1]? Hannes wrote:\n\n| The second problem is more severe and is at the lowest level of our\n| infrastructure: We set up our child processes so that they know only about\n| file descriptors other than 0,1,2 to the child process. Even if the first\n| problem were solved, the child process does not receive sufficient\n| information to know that there are open file descriptors other than 0,1,2.\n| There is a facility to pass along this information from the parent to the\n| child, but we simply do not implement it.\n\nIt sounds to me like the pipe model would work fine on Windows, and it\nwould just require some porting work (out of scope for this summer of\ncode project). Am I misunderstanding?\n\nThanks,\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/199216\n"},{"id":"195457","messageId":"5489458.8D23shS0RV@flomedio","threadId":"30705","inReplyTo":"20120721154437.GC19860@burratino","subject":"Re: [RFC 4/4 v3] Add cat-blob report fifo from fast-import to remote-helper.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-07-22T21:03:13Z","receivedAt":"2012-07-22T21:03:13Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"On Saturday 21 July 2012 10:44:37 Jonathan Nieder wrote:\n> Florian Achleitner wrote:\n> > On Saturday 21 July 2012 09:48:34 Jonathan Nieder wrote:\n> >> To sum up: I think we should just stick to pipes --- why all this fifo\n> >> complication?\n> > \n> > People didn't like pipe variant (prexec_cb not being compatible to\n> > windows'\n> > process creation model), so I learned about fifos and implemented a\n> > (basic) fifo variant. *shrug*\n> \n> Ok, can you elaborate on that? What does it mean that preexec_cb is\n> not compatible to windows' process creation model? Don't the people\n> of the future working on this code deserve to know about that, too, so\n> they don't break it?\n\nLet's discuss how to describe the solution after we decide which of the two \nvariants we choose.\n\nSummarizing the earlier discussion (in this thread as replies to version 1 of \nthe patch) more verbosely:\nI used the prexec_cb feature of start_command which allows to install a \ncallback function that is called just before exec.\nIt's purpose is to close the other pipe end in each of the processes that \ninherited both ends after fork.\nThe pipe is created in transport-helper.c, because it forks fast-import as \nwell as the remote-helper, which inherit the pipe's file descriptors.\n\nOn Windows, processses are not forked but spawned from new and therefore can't \ninherit pipe file descriptors. So we had the idea to use a fifo, which can be \nopened after the creation of the process. Also there are named pipes on \nWindows which are similar to fifos (though I've never used them).\n\nMostly out of curiosity I played around with fifos and implemented this \nadditional cat-blob-pipe feature, like a proof-of-concept.\n\nThe first version used the pipes because this feature already exists.\n\n> \n> Come on --- I'm not asking these questions just to make your life\n> difficult. Please make it easy to understand your code changes and to\n> keep them maintained.\n\nThe reason why I resent the patch in version 3 these days is that I the \nproblem of blocking open calls leading to potential deadlocks.\nOf course I should tell future maintainers better what its all about, if we \nreally want that feature.\n\nHope that helps,\nFlorian\n"},{"id":"195460","messageId":"20120722212424.GA680@burratino","threadId":"30705","inReplyTo":"5489458.8D23shS0RV@flomedio","subject":"Re: [RFC 4/4 v3] Add cat-blob report fifo from fast-import to remote-helper.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-22T21:24:24Z","receivedAt":"2012-07-22T21:24:24Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nFlorian Achleitner wrote:\n\n> On Windows, processses are not forked but spawned from new and therefore can't \n> inherit pipe file descriptors.\n\nOk, this is what I didn't understand (and still don't understand).\n\nYes, on Windows processes are spawned instead of forked. But does\nthat mean they can't inherit pipes? How do pipelines in the shell\nwork at all, then?\n\nI thought what Hannes said was rather that the current C runtime\nused by Git for Windows doesn't take care of inherited file\ndescriptors, period, other than 0, 1, and 2. This has nothing to do\nwith the fork/spawn distinction. So while --cat-blob-fd or some other\nmechanism could be made to work on Windows in the future, *relying* on\n--cat-blob-fd on Windows today is a no-go, since it rely on file\ndescriptors >2 and break the remote helper infrastructure completely\nthere.\n\nThat does not force us into a corner at all. We have multiple\noptions. Here is what I propose:\n\nRemote helpers declare using a capability (e.g., \"bidi-import\") that\nthey would like a bidirectional communication channel with\nfast-import. Git tells the remote helper that its request has been\ngranted by using a different command (e.g., \"bidi-import\") instead of\n\"import\" to start the import process.\n\nThe responses from fast-import come on stdin (file descriptor 0), so\nin principle it should be possible to implement this interface on\nWindows. The interface is portable, even if the initial\nimplementation isn't.\n\nExisting remote helpers using the \"import\" capability would be\nunaffected and would work as before.\n\nOn Windows, Git would not take advantage of the bidi-import capability\nfor now. Windows support is an added complication and we have enough\nto do this summer.\n\nThen interested people using Windows would be able to experiment using\nwhatever mechanism they please (CRT support for inherited file\ndescriptors >2, fifos, sockets, some other Windows-specific thing) to\nimplement the bidi-import capability. Once that is implemented, they\nautomatically get support for remote helpers that rely on the\nbidirectional communication functionality.\n\nWhat do you think?\n\nThanks for explaining,\nJonathan\n"},{"id":"195840","messageId":"44779150.xA3SZNmQ1h@flomedio","threadId":"30705","inReplyTo":"20120702110741.GA3527@burratino","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-07-26T08:31:57Z","receivedAt":"2012-07-26T08:31:57Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"Hi!\n\nMost of this review went into the new version.. \nFor the remaining points, some comments follow.\n\nOn Monday 02 July 2012 06:07:41 Jonathan Nieder wrote:\n> Hi,\n> \n> Florian Achleitner wrote:\n\n> \n> > --- /dev/null\n> > +++ b/contrib/svn-fe/remote-svn.c\n> > @@ -0,0 +1,207 @@\n> > +\n> > +#include \n> > +#include \n> > +#include \n> \n> git-compat-util.h (or some header that includes it) must be the first\n> header included so the appropriate feature test macros can be defined.\n> See Documentation/CodingGuidelines for more on that.\n\ncheck.\n\n> \n> > +#include \"cache.h\"\n> > +#include \"remote.h\"\n> > +#include \"strbuf.h\"\n> > +#include \"url.h\"\n> > +#include \"exec_cmd.h\"\n> > +#include \"run-command.h\"\n> > +#include \"svndump.h\"\n> > +\n> > +static int debug = 0;\n> \n> Small nit: please drop the redundant \"= 0\" here. Or:\n\ncheck.\n\n> > +\n> > +static inline void printd(const char* fmt, ...)\n> > +{\n> > +\tif(debug) {\n> > +\t\tva_list vargs;\n> > +\t\tva_start(vargs, fmt);\n> > +\t\tfprintf(stderr, \"rhsvn debug: \");\n> > +\t\tvfprintf(stderr, fmt, vargs);\n> > +\t\tfprintf(stderr, \"\\n\");\n> > +\t\tva_end(vargs);\n> > +\t}\n> > +}\n> \n> Why not use trace_printf and avoid the complication?\n\nHm.. I tried. It wasn't exactly what I wanted. When I use trace_printf, it's \nactivated together with all other traces. I can use trace_vprintf and specify \na key, but I would always have to print the header \"rhsvn debug: \" and the key \nby hand. So I could replace vfprintf in this function by trace_vprintf to do \nthat. But then there's not much simplification. (?)\n\n\n> > +\n> > +enum cmd_result cmd_capabilities(struct strbuf* line);\n> > +enum cmd_result cmd_import(struct strbuf* line);\n> > +enum cmd_result cmd_list(struct strbuf* line);\n> \n> What's a cmd_result? '*' sticks to variable name.\n> \n> > +\n> > +enum cmd_result { SUCCESS, NOT_HANDLED, ERROR };\n> \n> Oh, that's what a cmd_result is. :) Why not define the type before\n> using it to avoid keeping the reader in suspense?\n> \n> What does each result represent? If this is a convention like\n> \n> 1: handled\n> 0: not handled\n> -1: error, callee takes care of printing the error message\n> \n> then please document it in a comment near the caller so the reader can\n> understand what is happening without too much confusion. Given such a\n> comment, does the enum add clarity?\n\nHm.. the enum now has SUCCESS, NOT_HANDLED, TERMINATE.\nIt gives the numbers a name, thats it.\n\n> \n> > +typedef enum cmd_result (*command)(struct strbuf*);\n> \n> When I first read this, I wonder what is being commanded. Are these\n> commands passed on the remote helper's standard input, commands passed\n> on its output, or commands run at some point in the process? What is\n> the effect and return value of associated function? Does the function\n> always return some success/failure value, or does it sometimes exit?\n> \n> Maybe a more specific type name would be clearer?\n\nI renamed it to input_command_handler. Unfortunately the remote-helper spec \ncalls what is sent to the helper a 'command'.\n\n> \n> [...]\n> \n> > +\n> > +const command command_list[] = {\n> > +\t\tcmd_capabilities, cmd_import, cmd_list, NULL\n> > +};\n> \n> First association is to functions like cmd_fetch() which implement git\n> subcommands. So I thought these were going to implement subcommands\n> like \"git remote-svn capabilities\", \"git remote-svn import\" and would\n> use the same cmd_foo(argc, argv, prefix) calling convention that git\n> subcommands do. Maybe a different naming convention could avoid\n> confusion.\n\nOk.. same as above, they are kind of commands. Of course I can change the \nnames. For me it's not too confusing, because I don't know the git subcommands \nconvention very well. You can choose a name.\n\n> \n> [...]\n> \n> > +enum cmd_result cmd_capabilities(struct strbuf* line)\n> > +{\n> > +\tif(strcmp(line->buf, \"capabilities\"))\n> > +\t\treturn NOT_HANDLED;\n> \n> Style: missing SP after keyword.\n> \n> > +\n> > +\tprintf(\"import\\n\");\n> > +\tprintf(\"\\n\");\n> > +\tfflush(stdout);\n> > +\treturn SUCCESS;\n> > +}\n> \n> Why the multiple printf? Is the flush needed?\n\nExcess printf gone.\nFlush is needed. Otherwise it doesn't flush and the other end waits forever.\nDon't know exactly why. Some pipe-buffer ..\n\n> > +\n> > +\t/* opening a fifo for usually reading blocks until a writer has opened\n> > it too. +\t * Therefore, we open with RDWR.\n> > +\t */\n> > +\treport_fd = open(back_pipe_env, O_RDWR);\n> > +\tif(report_fd < 0) {\n> > +\t\tdie(\"Unable to open fast-import back-pipe! %s\", strerror(errno));\n> > +\t}\n> \n> Is this necessary? Why shouldn't we fork the writer first and wait\n> for it here?\n\nYes, necessary. Blocking on this open call prevents fast-import as well as the \nremote helper from reading and writing on their normal command streams.\nThis leads to deadlocks.\n\nE.g. If there's have nothing to import, the helper sends only 'done' to fast-\nimport and quits. That might happen before fast-import opened this pipe.\nThen it waits forever because the reader has already closed it.\n\n\n> > +\n> > +\tcode = start_command(&svndump_proc);\n> > +\tif(code)\n> > +\t\tdie(\"Unable to start %s, code %d\", svndump_proc.argv[0], code);\n> \n> start_command() is supposed to have printed a message already when it\n> fails, unless errno == ENOENT and silent_exec_failure was set.\n> \n\nYes, but it doesn't die, right?\n\n> > +\n> > +\tclose(svndump_proc.out);\n> \n> Important? Wouldn't finish_command do this?\n> \n\nAs far as I understood it, it doesn't close extra created pipes. Probably I \njust didn't find it in the code ..\n\n> > +\tclose(report_fd);\n> \n> What is the purpose of this step?\n\nClose the back-report pipe end of the remote-helper.\n\n> \n> > +\n> > +\tcode = finish_command(&svndump_proc);\n> > +\tif(code)\n> > +\t\twarning(\"Something went wrong with termination of %s, code %d\",\n> > svndump_proc.argv[0], code);\n> finish_command() is supposed to print a message when it fails.\n\nI changed the message text. It should tell us if svnrdump exited with non-\nzero.\n\n> \n> > +\tfree(svndump_proc.argv);\n> > +\n> > +\tprintf(\"done\\n\");\n> > +\treturn SUCCESS;\n> \n> Success even if it failed?\n\nOn fatal errors it dies.\n\n> > +enum cmd_result do_command(struct strbuf* line)\n> > +{\n> > +\tconst command* p = command_list;\n> > +\tenum cmd_result ret;\n> > +\tprintd(\"command line '%s'\", line->buf);\n> > +\twhile(*p) {\n> > +\t\tret = (*p)(line);\n> > +\t\tif(ret != NOT_HANDLED)\n> > +\t\t\treturn ret;\n> > +\t\tp++;\n> > +\t}\n> \n> If possible, matching commands by name (like git.c does) would make\n> the behavior easier to predict.\n> \n\nThere is some usecase for this. The intention was, that command handlers \nshould be able to process more than one 'name'. E.g. an import batch is \nterminated by a newline. This newline is handled by the import handler if it \nis a batch. (This feature wasn't implemented in the version reviewed here.)\n\nSo I decided to let the handler functions tell if they handle this line.\n\n> [...]\n> \n> > +\tif (argc < 2) {\n> > +\t\tfprintf(stderr, \"Remote needed\\n\");\n> > +\t\treturn 1;\n> > +\t}\n> \n> usage() can be used to write a clearer error message.\n\n> [...]\n> \n> > +\tfree((void*)url);\n> > +\tfree((void*)private_refs);\n> \n> Won't this crash?\n\nCrash? It frees detached strbuf buffers.\n\n> \n> It would also be nice to add a test case to the t/ directory to make others\n> changing this code do not accidentally break your new functionality.\n\ncheck.\n\n> \n> Hope that helps,\n> Jonathan\n\nIt helped ;)\n\nthx, Florian\n"},{"id":"195842","messageId":"20120726090842.GA4999@burratino","threadId":"30705","inReplyTo":"44779150.xA3SZNmQ1h@flomedio","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-26T09:08:42Z","receivedAt":"2012-07-26T09:08:42Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Florian Achleitner wrote:\n\n> Most of this review went into the new version.. \n> For the remaining points, some comments follow.\n\nThanks for this.\n\n> On Monday 02 July 2012 06:07:41 Jonathan Nieder wrote:\n\n[...]\n>>> +\n>>> +static inline void printd(const char* fmt, ...)\n[...]\n>> Why not use trace_printf and avoid the complication?\n>\n> Hm.. I tried. It wasn't exactly what I wanted. When I use trace_printf, it's \n> activated together with all other traces. I can use trace_vprintf and specify \n> a key, but I would always have to print the header \"rhsvn debug: \" and the key \n> by hand. So I could replace vfprintf in this function by trace_vprintf to do \n> that. But then there's not much simplification. (?)\n\nHmm. There's no trace_printf_with_key() but that's presumably because\nno one has needed it. If it existed, you could use\n\n\t#define printd(msg) trace_printf_with_key(\"GIT_TRACE_REMOTE_SVN\", \"%s\", msg)\n\nBut now that I check, I don't see how the current printd() calls would\nbe useful to other people. Why announce these moments and not others?\nThey're just temporary debugging cruft, right?\n\nFor that, plain trace_printf() works great.\n\n[...]\n>>> +\n>>> +enum cmd_result { SUCCESS, NOT_HANDLED, ERROR };\n[...]\n> Hm.. the enum now has SUCCESS, NOT_HANDLED, TERMINATE.\n\nMuch nicer.\n\nI think this tristate return value could be avoided entirely because...\n[continued at (*) below]\n\n[...]\n>>> +\n>>> +\tprintf(\"import\\n\");\n>>> +\tprintf(\"\\n\");\n>>> +\tfflush(stdout);\n>>> +\treturn SUCCESS;\n>>> +}\n>>\n>> Why the multiple printf? Is the flush needed?\n>\n> Excess printf gone.\n> Flush is needed. Otherwise it doesn't flush and the other end waits forever.\n\nAh, fast-import is ready, remote helper is ready, no one initiates\npumping of data between them. Maybe the purpose of the flush would\nbe more obvious if it were moved to the caller.\n\n[...]\n>>> +\t/* opening a fifo for usually reading blocks until a writer has opened\n>>> it too. +\t * Therefore, we open with RDWR.\n>>> +\t */\n>>> +\treport_fd = open(back_pipe_env, O_RDWR);\n>>> +\tif(report_fd < 0) {\n>>> +\t\tdie(\"Unable to open fast-import back-pipe! %s\", strerror(errno));\n>>> +\t}\n>>\n>> Is this necessary? Why shouldn't we fork the writer first and wait\n>> for it here?\n>\n> Yes, necessary.\n\nOh, dear. I hope not. E.g., Cygwin doesn't support opening fifos\nRDWR (out of scope for the gsoc project, but still).\n\n[...]\n> E.g. If there's have nothing to import, the helper sends only 'done' to fast-\n> import and quits.\n\nWon't the writer open the pipe and wait for us to open our end before\ndoing that?\n\n[...]\n>>> +\n>>> +\tcode = start_command(&svndump_proc);\n>>> +\tif(code)\n>>> +\t\tdie(\"Unable to start %s, code %d\", svndump_proc.argv[0], code);\n>>\n>> start_command() is supposed to have printed a message already when it\n>> fails, unless errno == ENOENT and silent_exec_failure was set.\n>\n> Yes, but it doesn't die, right?\n\nYou can exit without writing a message with exit(), e.g. like so:\n\n\tif (code)\n\t\texit(code);\n\nor like so:\n\n\tif (code)\n\t\texit(128);\n\n[...]\n>>> +\n>>> +\tclose(svndump_proc.out);\n>>\n>> Important? Wouldn't finish_command do this?\n>\n> As far as I understood it, it doesn't close extra created pipes. Probably I \n> just didn't find it in the code ..\n\nSo this is to work around a bug in the run-command interface?\n\n[...]\n>>> +\tclose(report_fd);\n>>\n>> What is the purpose of this step?\n>\n> Close the back-report pipe end of the remote-helper.\n\nThat's just repeating the question. :) Perhaps it's supposed to\ntrigger some action on the other end of the pipe? It would just be\nuseful to add a comment documenting why one shouldn't remove this\nclose() call, or else will probably end up removing it and needlessly\nsuffering.\n\n[...]\n>>> +\n>>> +\tcode = finish_command(&svndump_proc);\n>>> +\tif(code)\n>>> +\t\twarning(\"Something went wrong with termination of %s, code %d\",\n>>> svndump_proc.argv[0], code);\n>> finish_command() is supposed to print a message when it fails.\n>\n> I changed the message text. It should tell us if svnrdump exited with non-\n> zero.\n\nI'd suggest looking at other finish_command() callers for examples.\n\n[...]\n>>> +enum cmd_result do_command(struct strbuf* line)\n>>> +{\n>>> +\tconst command* p = command_list;\n>>> +\tenum cmd_result ret;\n>>> +\tprintd(\"command line '%s'\", line->buf);\n>>> +\twhile(*p) {\n>>> +\t\tret = (*p)(line);\n>>> +\t\tif(ret != NOT_HANDLED)\n>>> +\t\t\treturn ret;\n>>> +\t\tp++;\n>>> +\t}\n>>\n>> If possible, matching commands by name (like git.c does) would make\n>> the behavior easier to predict.\n>\n> There is some usecase for this. The intention was, that command handlers \n> should be able to process more than one 'name'. E.g. an import batch is \n> terminated by a newline. This newline is handled by the import handler if it \n> is a batch. (This feature wasn't implemented in the version reviewed here.)\n>\n> So I decided to let the handler functions tell if they handle this line.\n\n[continued from (*) above]\n... it isn't needed at the moment.\n\nSee http://c2.com/xp/YouArentGonnaNeedIt.html\n\n[...]\n>>> +\tfree((void*)url);\n>>> +\tfree((void*)private_refs);\n>>\n>> Won't this crash?\n>\n> Crash? It frees detached strbuf buffers.\n\nI see \"url = argv[2];\" a few lines above, but it looks like the variable\nwas reused for two different purposes. :(\n\nI'm not sure why you detach url, by the way. If the goal is to free it,\nwhy not use strbuf_release()?\n\nThanks,\nJonathan\n"},{"id":"195843","messageId":"5D2201A1-A945-4189-89BD-4B379EEEF045@gmail.com","threadId":"30705","inReplyTo":"20120702110741.GA3527@burratino","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Steven Michalske","fromEmail":"smichalske@gmail.com","sentAt":"2012-07-26T09:45:46Z","receivedAt":"2012-07-26T09:45:46Z","isPatch":false,"sender":{"key":"smichalske@gmail.com","avatar":"https://gravatar.com/avatar/721f27456adc9ac84f3bb235f021a70015abb9e09222ae8622fc5579c6a203c1?d=mp&s=160"},"body":"\nOn Jul 2, 2012, at 4:07 AM, Jonathan Nieder wrote:\n\n> [...]\n>> diff: Use fifo instead of pipe: Retrieve the name of the pipe from env and open it\n>> for svndump.\n> \n> I'd prefer to avoid this if possible, since it means having to decide\n> where the pipe goes on the filesystem. Can you summarize the\n> discussion in the commit message so future readers understand why\n> we're doing it?\n\nCrazy thought here but would a socket not be a bad choice here?\n\nImagine being able to ssh tunnel into the SVN server and run the helper with filesystem access to the SVN repo.\n\nAkin to the pushy project use case.\nhttp://packages.python.org/pushy/\n\nSSH into the machine, copy the required components to the machine, and use the RPC.\nNothing needed but SSH and python. In this case SSH, SVN, and the helper would be needed.\n\nThis also would work just fine with the local host too.\n\nSteve\n\nNote: Resent, Sorry it was signed, and rejected before."},{"id":"195846","messageId":"20120726114039.GA6712@burratino","threadId":"30705","inReplyTo":"358E6F1E-8BAD-4F82-B270-0233AB86EF66@gmail.com","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-26T11:40:39Z","receivedAt":"2012-07-26T11:40:39Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Steven Michalske wrote:\n> On Jul 2, 2012, at 4:07 AM, Jonathan Nieder wrote:\n\n>> [...]\n>>> diff: Use fifo instead of pipe: Retrieve the name of the pipe from env and open it\n>>> for svndump.\n>>\n>> I'd prefer to avoid this if possible, since it means having to decide\n>> where the pipe goes on the filesystem. Can you summarize the\n>> discussion in the commit message so future readers understand why\n>> we're doing it?\n>\n> Crazy thought here but would a socket not be a bad choice here?\n\nNot crazy --- it was already mentioned. It could probably allow using\n--cat-blob-fd even on the platforms that don't inherit file\ndescriptors >2, though it wuld take some tweaking. Though I still\nthink the way forward is to keep using plain pipes internally for now\nand to make the bidirectional communication optional, since it\nwouldn't close any doors to whatever is most convenient on each\nplatform. Hopefully I'll hear more from Florian about this in time.\n\n> Imagine being able to ssh tunnel into the SVN server and run the helper with\n> filesystem access to the SVN repo.\n\nWe're talking about what communicates between the SVN dump parser the\nversion control system-specific backend (git fast-import) that reads\nthe converted result, so that particular socket wouldn't help much.\n\nJonathan\n"},{"id":"195859","messageId":"1486896.KW3TvzfC56@flomedio","threadId":"30705","inReplyTo":"20120726114039.GA6712@burratino","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner2.6.31@gmail.com","sentAt":"2012-07-26T14:28:33Z","receivedAt":"2012-07-26T14:28:33Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"On Thursday 26 July 2012 06:40:39 Jonathan Nieder wrote:\n> Steven Michalske wrote:\n> > On Jul 2, 2012, at 4:07 AM, Jonathan Nieder wrote:\n> >> [...]\n> >> \n> >>> diff: Use fifo instead of pipe: Retrieve the name of the pipe from env\n> >>> and open it for svndump.\n> >> \n> >> I'd prefer to avoid this if possible, since it means having to decide\n> >> where the pipe goes on the filesystem. Can you summarize the\n> >> discussion in the commit message so future readers understand why\n> >> we're doing it?\n> > \n> > Crazy thought here but would a socket not be a bad choice here?\n> \n> Not crazy --- it was already mentioned. It could probably allow using\n> --cat-blob-fd even on the platforms that don't inherit file\n> descriptors >2, though it wuld take some tweaking. Though I still\n> think the way forward is to keep using plain pipes internally for now\n> and to make the bidirectional communication optional, since it\n> wouldn't close any doors to whatever is most convenient on each\n> platform. Hopefully I'll hear more from Florian about this in time.\n\nWould you like to see a new pipe patch?\n\n> \n> > Imagine being able to ssh tunnel into the SVN server and run the helper\n> > with filesystem access to the SVN repo.\n> \n> We're talking about what communicates between the SVN dump parser the\n> version control system-specific backend (git fast-import) that reads\n> the converted result, so that particular socket wouldn't help much.\n>\n\nYes .. the network part is already handled quite well by svnrdump.\n \n"},{"id":"195860","messageId":"20120726145426.GB3058@burratino","threadId":"30705","inReplyTo":"1486896.KW3TvzfC56@flomedio","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-26T14:54:26Z","receivedAt":"2012-07-26T14:54:26Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(cc-ing Ram since he's also knowledgeable about remote-helper protocol)\nFlorian Achleitner wrote:\n> On Thursday 26 July 2012 06:40:39 Jonathan Nieder wrote:\n\n>> Though I still\n>> think the way forward is to keep using plain pipes internally for now\n>> and to make the bidirectional communication optional, since it\n>> wouldn't close any doors to whatever is most convenient on each\n>> platform. Hopefully I'll hear more from Florian about this in time.\n>\n> Would you like to see a new pipe patch?\n\nSince the svn remote helper relies on this, it seems worth working on,\nyeah. As for how to spend your time (and whether to beg someone else\nto work on it instead :)): I'm not sure what's on your plate or where\nyou are with respect to the original plan for the summer at the\nmoment, so it would be hard for me to give useful advice about how to\nbalance things.\n\nWhat did you think of the suggestion of adding a new bidi-import\ncapability and command to the remote helper protocol? I think this\nwould be clean and avoid causing a regression on Windows, but it's\neasily possible I am missing something fundamental.\n\nThanks,\nJonathan\n"},{"id":"195862","messageId":"1609414.ugUML9Yn73@flomedio","threadId":"30705","inReplyTo":"20120726090842.GA4999@burratino","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-07-26T16:16:31Z","receivedAt":"2012-07-26T16:16:31Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"On Thursday 26 July 2012 04:08:42 Jonathan Nieder wrote:\n> Florian Achleitner wrote:\n\n> > On Monday 02 July 2012 06:07:41 Jonathan Nieder wrote:\n> [...]\n> \n> >>> +\n> >>> +static inline void printd(const char* fmt, ...)\n> \n> [...]\n> \n> >> Why not use trace_printf and avoid the complication?\n> > \n> > Hm.. I tried. It wasn't exactly what I wanted. When I use trace_printf,\n> > it's activated together with all other traces. I can use trace_vprintf\n> > and specify a key, but I would always have to print the header \"rhsvn\n> > debug: \" and the key by hand. So I could replace vfprintf in this\n> > function by trace_vprintf to do that. But then there's not much\n> > simplification. (?)\n> \n> Hmm. There's no trace_printf_with_key() but that's presumably because\n> no one has needed it. If it existed, you could use\n> \n> \t#define printd(msg) trace_printf_with_key(\"GIT_TRACE_REMOTE_SVN\", \"%s\",\n> msg)\n> \n> But now that I check, I don't see how the current printd() calls would\n> be useful to other people. Why announce these moments and not others?\n> They're just temporary debugging cruft, right?\n> \n> For that, plain trace_printf() works great.\n\nYes, it's for debugging only, I could just delete it all. It's inspired by \ntransport-helper.c. The env var GIT_TRANSPORT_HELPER_DEBUG enables it. While \ntransport-helper has a lot of if (debug) fprintf(..), I encapsulated it in \nprintd.\nSo I should kick printd out?\n\n> >>> +\n> >>> +\tprintf(\"import\\n\");\n> >>> +\tprintf(\"\\n\");\n> >>> +\tfflush(stdout);\n> >>> +\treturn SUCCESS;\n> >>> +}\n> >> \n> >> Why the multiple printf? Is the flush needed?\n> > \n> > Excess printf gone.\n> > Flush is needed. Otherwise it doesn't flush and the other end waits\n> > forever.\n> Ah, fast-import is ready, remote helper is ready, no one initiates\n> pumping of data between them. Maybe the purpose of the flush would\n> be more obvious if it were moved to the caller.\n\nAcutally this goes to the git parent process (not fast-import), waiting for a \nreply to the command. I think I have to call flush on this side of the pipe. \nCan you flush it from the reader? This wouldn't have the desired effect, it \ndrops buffered data.\n\n> [...]\n> \n> >>> +\t/* opening a fifo for usually reading blocks until a writer has opened\n> >>> it too. +\t * Therefore, we open with RDWR.\n> >>> +\t */\n> >>> +\treport_fd = open(back_pipe_env, O_RDWR);\n> >>> +\tif(report_fd < 0) {\n> >>> +\t\tdie(\"Unable to open fast-import back-pipe! %s\", strerror(errno));\n> >>> +\t}\n> >> \n> >> Is this necessary? Why shouldn't we fork the writer first and wait\n> >> for it here?\n> > \n> > Yes, necessary.\n> \n> Oh, dear. I hope not. E.g., Cygwin doesn't support opening fifos\n> RDWR (out of scope for the gsoc project, but still).\n\nI believe it can be solved using RDONLY and WRONLY too. Probably we solve it \nby not using the fifo at all.\nCurrently the blocking comes from the fact, that fast-import doesn't parse \nit's command line at startup. It rather reads an input line first and decides \nwhether to parse the argv after reading the first input line or at the end of \nthe input. (don't know why)\nremote-svn opens the pipe before sending the first command to fast-import and \nblocks on the open, while fast-import waits for input --> deadlock.\nwith remote-svn: RDWR, fast-import: WRONLY, this works.\n\nOther scenario: Nothing to import, remote-svn only sends 'done' and closes the \npipe again. After fast-import reads the first line it parses it's command line \nand tries to open the fifo which is already closed on the other side --> \nblocks.\nThis is solved by using RDWR on both sides.\n\nIf we change the points where the pipes are openend and closed, this could be \ncircumvented.\n\n> \n> [...]\n> \n> > E.g. If there's have nothing to import, the helper sends only 'done' to\n> > fast- import and quits.\n> \n> Won't the writer open the pipe and wait for us to open our end before\n> doing that?\n> \n> [...]\n> \n> >>> +\n> >>> +\tcode = start_command(&svndump_proc);\n> >>> +\tif(code)\n> >>> +\t\tdie(\"Unable to start %s, code %d\", svndump_proc.argv[0], code);\n> >> \n> >> start_command() is supposed to have printed a message already when it\n> >> fails, unless errno == ENOENT and silent_exec_failure was set.\n> > \n> > Yes, but it doesn't die, right?\n> \n> You can exit without writing a message with exit(), e.g. like so:\n> \n> \tif (code)\n> \t\texit(code);\n> \n> or like so:\n> \n> \tif (code)\n> \t\texit(128);\n\nok, why not..\n\n> \n> [...]\n> \n> >>> +\n> >>> +\tclose(svndump_proc.out);\n> >> \n> >> Important? Wouldn't finish_command do this?\n> > \n> > As far as I understood it, it doesn't close extra created pipes. Probably\n> > I\n> > just didn't find it in the code ..\n> \n> So this is to work around a bug in the run-command interface?\n\nGood question. \n\n> \n> [...]\n> \n> >>> +\tclose(report_fd);\n> >> \n> >> What is the purpose of this step?\n> > \n> > Close the back-report pipe end of the remote-helper.\n> \n> That's just repeating the question. :) Perhaps it's supposed to\n> trigger some action on the other end of the pipe? It would just be\n> useful to add a comment documenting why one shouldn't remove this\n> close() call, or else will probably end up removing it and needlessly\n> suffering.\n\nIt's just closing files I've openend before. I usually do that, when i no \nlonger need them. Isn't that common?\nI believe it makes no difference if you only call import once. On subsequent \nimports the pipe is still open when it tries to open it again, that could be a \nproblem. It would have to statically store report_fd, but what for??\n\n> \n> [...]\n> \n> >>> +\n> >>> +\tcode = finish_command(&svndump_proc);\n> >>> +\tif(code)\n> >>> +\t\twarning(\"Something went wrong with termination of %s, code %d\",\n> >>> svndump_proc.argv[0], code);\n> >> \n> >> finish_command() is supposed to print a message when it fails.\n> > \n> > I changed the message text. It should tell us if svnrdump exited with non-\n> > zero.\n> \n> I'd suggest looking at other finish_command() callers for examples.\n> \n> [...]\n> \n> >>> +enum cmd_result do_command(struct strbuf* line)\n> >>> +{\n> >>> +\tconst command* p = command_list;\n> >>> +\tenum cmd_result ret;\n> >>> +\tprintd(\"command line '%s'\", line->buf);\n> >>> +\twhile(*p) {\n> >>> +\t\tret = (*p)(line);\n> >>> +\t\tif(ret != NOT_HANDLED)\n> >>> +\t\t\treturn ret;\n> >>> +\t\tp++;\n> >>> +\t}\n> >> \n> >> If possible, matching commands by name (like git.c does) would make\n> >> the behavior easier to predict.\n> > \n> > There is some usecase for this. The intention was, that command handlers\n> > should be able to process more than one 'name'. E.g. an import batch is\n> > terminated by a newline. This newline is handled by the import handler if\n> > it is a batch. (This feature wasn't implemented in the version reviewed\n> > here.)\n> > \n> > So I decided to let the handler functions tell if they handle this line.\n> \n> [continued from (*) above]\n> ... it isn't needed at the moment.\n> \n> See http://c2.com/xp/YouArentGonnaNeedIt.html\n\nHm.. all three values are used, they're not for the future but for now.\nOf course it could be done somehow else.\n\n> \n> [...]\n> \n> >>> +\tfree((void*)url);\n> >>> +\tfree((void*)private_refs);\n> >> \n> >> Won't this crash?\n> > \n> > Crash? It frees detached strbuf buffers.\n> \n> I see \"url = argv[2];\" a few lines above, but it looks like the variable\n> was reused for two different purposes. :(\n\nOoops. That was introduced by splitting out \"[RFC 07/16] Allow reading svn \ndumps from files via file:// urls.\" It makes url malloced unconditionally. Will \nfix.\n> \n> I'm not sure why you detach url, by the way. If the goal is to free it,\n> why not use strbuf_release()?\n\nI used the strbuf because of it's nice formatting functions. But on the other \nhand, e.g. url_decode returns a char *, so I stored it in this type.\nIt doesn't make a big difference if I use three strbufs or one and detach and \nfree the char * at the end. In fact, char * will consume some bytes less than \na strbuf ;)\nAnother question is, if calling free just before exit makes sense anyways.\n(But you learn it in programming courses, and it pleases valgrind :D )\n\n> \n> Thanks,\n> Jonathan\n\nFlorian\n"},{"id":"195872","messageId":"7vlii68m7k.fsf@alter.siamese.dyndns.org","threadId":"30705","inReplyTo":"20120726090842.GA4999@burratino","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-26T17:29:51Z","receivedAt":"2012-07-26T17:29:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> [...]\n>>>> +\n>>>> +enum cmd_result { SUCCESS, NOT_HANDLED, ERROR };\n> [...]\n>> Hm.. the enum now has SUCCESS, NOT_HANDLED, TERMINATE.\n>\n> Much nicer.\n>\n> I think this tristate return value could be avoided entirely because...\n> ... it isn't needed at the moment.\n\nI am not sure what you mean by that.\n\nThe command dispatcher loop in [Patch v2 1/16] seems to call every\npossible input handler with the first line of the input and expect\nthem to answer \"This is not for me\", so NOT_HANDLED is needed.\n\nAn alternative dispatcher could be written in such a way that the\ndispatcher inspects the first line and decide what to call, and in\nsuch a scheme, you do not need NOT_HANDLED. My intuition tells me\nthat such an arrangement is in general a better organization.\n\nLooking at what cmd_import() does, however, I think the approach the\npatch takes might make sense for this application. Unlike other\nhandlers like \"capabilities\" that do not want to handle anything\nother than \"capabilities\", it wants to handle two:\n\n - \"import\" that starts an import batch;\n - \"\" (an empty line), but only when an import batch is in effect.\n\nA centralized dispatcher that does not use NOT_HANDLED could be\nwritten for such an input stream, but then the state information\n(i.e. \"are we in an import batch?\") needs to be global, which may or\nmay not be desirable (I haven't thought things through on this).\n\nIn any case, if you are going to use dispatching based on\nNOT_HANDLED, the result may have to be (at least) quadri-state. In\naddition to \"I am done successfully, please go back and dispatch\nanother command\" (SUCCESS), \"This is not for me\" (NOT_HANDLED), and\n\"I am done successfully, and there is no need to dispatch and\nprocess another command further\" (TERMINATE), you may want to be\nable to say \"This was for me, but I found an error\" (ERROR).\n\nOf course, if the dispatch loop has to be rewritten so that a\ncentral dispatcher decides what to call, individual input handlers\ndo not need to say NOT_HANDLED nor TERMINATE, as the central\ndispatcher should keep track of the overall state of the system, and\nthe usual \"0 on success, negative on error\" may be sufficient.\n\nOne thing I wondered was how an input \"capability\" (or \"list\")\nshould be handled after \"import\" was issued (hence batch_active\nbecomes true). The dispatcher loop in the patch based on\nNOT_HANDLED convention will happily call cmd_capabilities(), which\ndoes not have any notion of the batch_active state (because it is a\nfunction scope static inside cmd_import()), and will say \"Ah, that\nis mine, and let me do my thing.\" If we want to diagnose such an\ninput stream as an error, the dispatch loop needs to become aware of\nthe overall state of the system _anyway_, so that may be an argument\nagainst the NOT_HANDLED based dispatch system the patch series uses.\n"},{"id":"195932","messageId":"9398008.kKbpiTCtsb@flomedio","threadId":"30705","inReplyTo":"20120726145426.GB3058@burratino","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-07-27T07:23:42Z","receivedAt":"2012-07-27T07:23:42Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"On Thursday 26 July 2012 09:54:26 Jonathan Nieder wrote:\n> \n> Since the svn remote helper relies on this, it seems worth working on,\n> yeah. As for how to spend your time (and whether to beg someone else\n> to work on it instead :)): I'm not sure what's on your plate or where\n> you are with respect to the original plan for the summer at the\n> moment, so it would be hard for me to give useful advice about how to\n> balance things.\n\nBtw, the pipe version did already exist before I started, it was added with \nthe cat-blob command and already used by Dmitry's remote-svn-alpha.\nI didn't search for design discussions in the past ..\n\n> \n> What did you think of the suggestion of adding a new bidi-import\n> capability and command to the remote helper protocol? I think this\n> would be clean and avoid causing a regression on Windows, but it's\n> easily possible I am missing something fundamental.\n\nI don't have much overview over this topic besides the part I'm working on, \nlike other users of fast-import. \nThe bidi-import capability/command would have the advantage, that we don't \nhave to bother with the pipe/fifo at all, if the remote-helper doesn't use it.\n\nWhen I implemented the two variants I had the idea to pass it to the 'option' \ncommand, that fast-import already has. Anyways, specifying cat-blob-fd is not \nallowed via the 'option' command (see Documentation and 85c62395).\nIt wouldn't make too much sense, because the file descriptor must be set up by \nthe parent.\n\nBut for the fifo, it would, probably. The backward channel is only used by the \ncommands 'cat-blob' and 'ls' of fast-import. If a remote helper wants to use \nthem, it would could make fast-import open the pipe by sending an 'option' \ncommand with the fifo filename, otherwise it defaults to stdout (like now) and \nis rather useless.\nThis would take the fifo setup out of transport-helper. The remote-helper would \nhave to create it, if it needs it.\n\nApropos stdout. That leads to another idea. You already suggested that it \nwould be easiest to only use FDs 0..2. Currently stdout and stderr of fast-\nimport go to the shell. We could connect stdout to the remote-helper and don't \nneed the additional channel at all.\n(Probably there's a good reason why they haven't done that ..)\nMaybe this requires many changes to fast-import and breaks existing frontends.\n\n--\nFlorian\n"},{"id":"195993","messageId":"20120728065439.GB4739@burratino","threadId":"30705","inReplyTo":"9398008.kKbpiTCtsb@flomedio","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-28T06:54:39Z","receivedAt":"2012-07-28T06:54:39Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nSome quick details.\n\nFlorian Achleitner wrote:\n\n> Anyways, specifying cat-blob-fd is not \n> allowed via the 'option' command (see Documentation and 85c62395).\n> It wouldn't make too much sense, because the file descriptor must be set up by \n> the parent.\n>\n> But for the fifo, it would, probably.\n\nMore precisely, requiring that the cat-blob fd go on the command line\ninstead of in the stream matches a model where fast-import's three\nstandard streams are abstract:\n\n - its input, using the INPUT FORMAT described in git-fast-import(1)\n - its standard output, which echoes \"progress\" commands\n - its backflow stream, where responses to \"cat-blob\" and \"ls\" commands go\n\nThe stream format is not necessarily pinned to a Unix model where\ninput and output are based on filesystems and file descriptors. We\ncan imagine a fast-import backend that runs on a remote server and the\ntransport used for these streams is sockets; for fast-import frontends\nto be usable in such a context, the streams they produce must not rely\non particular fd numbers, nor on pathnames (except for mark state\nsaved to relative paths with --relative-marks) representing anything\nin particular.\n\nThis goes just as much for a fifo set up on the filesystem where the\nfast-import backend runs as for an inherited file descriptor. In the\ncurrent model, such backend-specific details of setup go on the\ncommand line.\n\n> The backward channel is only used by the \n> commands 'cat-blob' and 'ls' of fast-import. If a remote helper wants to use \n> them, it would could make fast-import open the pipe by sending an 'option' \n> command with the fifo filename, otherwise it defaults to stdout (like now) and \n> is rather useless.\n\nI'm getting confused by terminology again. Let's see if I have the cast\nof characters straight:\n\n - the fast-import backend (e.g., \"git fast-import\" or \"hg-fastimport\")\n - the fast-import frontend (e.g., \"git fast-export\" or svn-fe)\n - git's generic foreign VCS support plumbing, also known as\n transport-helper.c\n - the remote helper (e.g., \"git remote-svn\" or \"git remote-testgit\")\n\nWhy would the fast-import backend ever need to open a pipe? If I want\nit to write backflow to a fifo, I can use\n\n\tmkfifo my-favorite-fifo\n\tgit fast-import --cat-blob-fd=3 3>my-favorite-fifo\n\nIf I want it to write backflow to a pipe, I can use (using ksh syntax)\n\n\tcat |&\n\tgit fast-import --cat-blob-fd=3 3>&p\n\n> This would take the fifo setup out of transport-helper. The remote-helper would \n> have to create it, if it needs it.\n\nWe can imagine transport-helper.c learning the name of a fifo set up by\nthe remote helper by sending it the \"capabilities\" command:\n\n\tgit> capabilities\n\thelper> option\n\thelper> import\n\thelper> cat-blob-file my-favorite-fifo\n\thelper> refspec refs/heads/*:refs/helper/remotename/*\n\thelper>\n\ntransport-helper.c could then use that information to invoke\nfast-import appropriately:\n\n\tgit fast-import --cat-blob-fd=3 3>my-favorite-fifo\n\nBut this seems like pushing complication onto the remote helper; since\nthere is expected to be one remote helper per foreign VCS,\nimplementing the appropriate logic correctly once and for all in\ntransport-helper.c for all interested remote helpers to take advantage\nof seems to me like a better policy.\n\n> Apropos stdout. That leads to another idea. You already suggested that it \n> would be easiest to only use FDs 0..2. Currently stdout and stderr of fast-\n> import go to the shell. We could connect stdout to the remote-helper and don't \n> need the additional channel at all.\n\nThe complication that makes this strategy not so easy is \"progress\"\ncommands in the fast-import input stream. (Incidentally, it would be\nnice to teach transport-helper.c to display specially formatted\n\"progress\" commands using a progress bar some day.)\n\nHoping that clarifies,\nJonathan\n"},{"id":"195994","messageId":"20120728070030.GC4739@burratino","threadId":"30705","inReplyTo":"1609414.ugUML9Yn73@flomedio","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-28T07:00:31Z","receivedAt":"2012-07-28T07:00:31Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Florian Achleitner wrote:\n\n> So I should kick printd out?\n\nI think so, yes.\n\n\"git log -SGIT_TRANSPORT_HELPER_DEBUG transport-helper.c\" tells me\nthat that option was added to make the transport-helper machinery make\nnoise to make it obvious at what stage a remote helper has deadlocked.\n\nGIT_TRANSPORT_HELPER_DEBUG already takes care of that, so there would\nnot be need for an imitation of that in remote-svn, unless I am\nmissing something (and even if I am missing something, it seems\ncomplicated enough to be worth moving to another patch where it can be\nexplained more easily).\n\n[...]\n>>>>> +\n>>>>> +\tprintf(\"import\\n\");\n>>>>> +\tprintf(\"\\n\");\n>>>>> +\tfflush(stdout);\n>>>>> +\treturn SUCCESS;\n>>>>> +}\n[...]\n>> Maybe the purpose of the flush would\n>> be more obvious if it were moved to the caller.\n>\n> Acutally this goes to the git parent process (not fast-import), waiting for a \n> reply to the command. I think I have to call flush on this side of the pipe. \n> Can you flush it from the reader? This wouldn't have the desired effect, it \n> drops buffered data.\n\n*slaps head* This is the \"capabilities\" command, and it needs to\nflush because the reader needs to know what commands it's allowed to\nuse next before it starts using them. My brain turned off and I\nthought you were emitting an \"import\" command rather than advertising\nthat you support it for some reason.\n\nAnd 'printf(\"\\n\")' was a separate printf because that way, patches\nlike\n\n\t \tprintf(\"import\\n\");\n\t+\tprintf(\"bidi-import\\n\");\n\t \tprintf(\"\\n\");\n\t \tfflush(stdout);\n\nbecome simpler.\n\nI'm tempted to suggest a structure like\n\n\t\tconst char * const capabilities[] = {\"import\"};\n\t\tint i;\n\n\t\tfor (i = 0; i < ARRAY_SIZE(capabilities); i++)\n\t\t\tputs(capabilities[i]);\n\t\tputs(\"\");\t/* blank line */\n\n\t\tfflush(stdout);\n\nbut your original code was fine, too.\n\n[...]\n>>>>> +\t/* opening a fifo for usually reading blocks until a writer has opened\n>>>>> it too. +\t * Therefore, we open with RDWR.\n>>>>> +\t */\n>>>>> +\treport_fd = open(back_pipe_env, O_RDWR);\n>>>>> +\tif(report_fd < 0) {\n>>>>> +\t\tdie(\"Unable to open fast-import back-pipe! %s\", strerror(errno));\n[...]\n> I believe it can be solved using RDONLY and WRONLY too. Probably we solve it \n> by not using the fifo at all.\n> Currently the blocking comes from the fact, that fast-import doesn't parse \n> it's command line at startup. It rather reads an input line first and decides \n> whether to parse the argv after reading the first input line or at the end of \n> the input. (don't know why)\n> remote-svn opens the pipe before sending the first command to fast-import and \n> blocks on the open, while fast-import waits for input --> deadlock.\n\nThanks for explaining. Now we've discussed a few different approproaches,\nnone of which is perfect.\n\na. use --cat-blob-fd, no FIFO\n\n Doing this unconditionally would break platforms that don't support\n --cat-blob-fd=(descriptor >2), like Windows, so we'd have to:\n\n * Make it conditional --- only do it (1) we are not on Windows and\n (2) the remote helper requests backflow by advertising the\n import-bidi capability.\n\n * Let the remote helper know what's going on by using\n \"import-bidi\" instead of \"import\" in the command stream to\n initiate the import.\n\nb. use envvars to pass around FIFO path\n\n This complicates the fast-import interface and makes debugging hard.\n It would be nice to avoid this if we can, but in case we can't, it's\n nice to have the option available.\n\nc. transport-helper.c uses FIFO behind the scenes.\n\n Like (a), except it would require a fast-import tweak (boo) and\n would work on Windows (yea)\n\nd. use --cat-blob-fd with FIFO\n\n Early scripted remote-svn prototypes did this to fulfill \"fetch\"\n requests.\n\n It has no advantage over \"use --cat-blob-fd, no FIFO\" except being\n easier to implement as a shell script. I'm listing this just for\n comparison; since (a) looks better in every way, I don't see any\n reason to pursue this one.\n\nSince avoiding deadlocks with bidirectional communication is always a\nlittle subtle, it would be nice for this to be implemented once in\ntransport-helper.c rather than each remote helper author having to\nreimplement it again. As a result, my knee-jerk ranking is a > c >\nb > d.\n\nSane?\nJonathan\n"},{"id":"196107","messageId":"3225988.4e4jhmQGr7@flomedio","threadId":"30705","inReplyTo":"20120728070030.GC4739@burratino","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-07-30T08:12:06Z","receivedAt":"2012-07-30T08:12:06Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"On Saturday 28 July 2012 02:00:31 Jonathan Nieder wrote:\n> Thanks for explaining. Now we've discussed a few different approproaches,\n> none of which is perfect.\n> \n> a. use --cat-blob-fd, no FIFO\n> \n> Doing this unconditionally would break platforms that don't support\n> --cat-blob-fd=(descriptor >2), like Windows, so we'd have to:\n> \n> * Make it conditional --- only do it (1) we are not on Windows and\n> (2) the remote helper requests backflow by advertising the\n> import-bidi capability.\n> \n> * Let the remote helper know what's going on by using\n> \"import-bidi\" instead of \"import\" in the command stream to\n> initiate the import.\n\nGenerally I like your prefered solution.\nI think there's one problem:\nThe pipe needs to be created before the fork, so that the fd can be inherited. \nThere is no way of creating it if the remote-helper advertises a capability, \nbecause it is already forked then. This would work with fifos, though.\n\nWe could:\n- add a capability: bidi-import. \n- make transport-helper create a fifo if the helper advertises it.\n- add a command for remote-helpers, like 'bidi-import ' that makes \nthe remote helper open the fifo at and use it.\n- fast-import is forked after the helper, so we do already know if there will \nbe a back-pipe. If yes, open it in transport-helper and pass the fd as command \nline argument cat-blob-fd. \n\n--> fast-import wouldn't need to be changed, but we'd use a fifo, and we get \nrid of the env-vars.\n(I guess it could work on windows too).\n\nWhat do you think?\n\n> \n> b. use envvars to pass around FIFO path\n> \n> This complicates the fast-import interface and makes debugging hard.\n> It would be nice to avoid this if we can, but in case we can't, it's\n> nice to have the option available.\n> \n> c. transport-helper.c uses FIFO behind the scenes.\n> \n> Like (a), except it would require a fast-import tweak (boo) and\n> would work on Windows (yea)\n> \n> d. use --cat-blob-fd with FIFO\n> \n> Early scripted remote-svn prototypes did this to fulfill \"fetch\"\n> requests.\n> \n> It has no advantage over \"use --cat-blob-fd, no FIFO\" except being\n> easier to implement as a shell script. I'm listing this just for\n> comparison; since (a) looks better in every way, I don't see any\n> reason to pursue this one.\n> \n> Since avoiding deadlocks with bidirectional communication is always a\n> little subtle, it would be nice for this to be implemented once in\n> transport-helper.c rather than each remote helper author having to\n> reimplement it again. As a result, my knee-jerk ranking is a > c >\n> b > d.\n> \n> Sane?\n> Jonathan\n"},{"id":"196109","messageId":"2324454.rWiRnly2JJ@flomedio","threadId":"30705","inReplyTo":"7vlii68m7k.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-07-30T08:12:29Z","receivedAt":"2012-07-30T08:12:29Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"On Thursday 26 July 2012 10:29:51 Junio C Hamano wrote:\n> Of course, if the dispatch loop has to be rewritten so that a\n> central dispatcher decides what to call, individual input handlers\n> do not need to say NOT_HANDLED nor TERMINATE, as the central\n> dispatcher should keep track of the overall state of the system, and\n> the usual \"0 on success, negative on error\" may be sufficient.\n> \n> One thing I wondered was how an input \"capability\" (or \"list\")\n> should be handled after \"import\" was issued (hence batch_active\n> becomes true). The dispatcher loop in the patch based on\n> NOT_HANDLED convention will happily call cmd_capabilities(), which\n> does not have any notion of the batch_active state (because it is a\n> function scope static inside cmd_import()), and will say \"Ah, that\n> is mine, and let me do my thing.\" If we want to diagnose such an\n> input stream as an error, the dispatch loop needs to become aware of\n> the overall state of the system _anyway_, so that may be an argument\n> against the NOT_HANDLED based dispatch system the patch series uses.\n\nThat's a good point. The current implementation allows other commands to \nappear during import batches. This shouldn't be possible according to the \nprotocol, I think. But it doesn't do harm. Solving it will require a global \nstate and go towards a global displatcher.\n"},{"id":"196110","messageId":"20120730082951.GA7702@burratino","threadId":"30705","inReplyTo":"3225988.4e4jhmQGr7@flomedio","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-30T08:29:52Z","receivedAt":"2012-07-30T08:29:52Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Florian Achleitner wrote:\n> On Saturday 28 July 2012 02:00:31 Jonathan Nieder wrote:\n\n>> a. use --cat-blob-fd, no FIFO\n[...]\n>> * Make it conditional --- only do it (1) we are not on Windows and\n>> (2) the remote helper requests backflow by advertising the\n>> import-bidi capability.\n>>\n>> * Let the remote helper know what's going on by using\n>> \"import-bidi\" instead of \"import\" in the command stream to\n>> initiate the import.\n>\n> Generally I like your prefered solution.\n> I think there's one problem:\n> The pipe needs to be created before the fork, so that the fd can be inherited. \n\nThe relevant pipe already exists at that point: the remote helper's\nstdin.\n\nIn other words, it could work like this (just like the existing demo\ncode, except adding a conditional based on the \"capabilities\"\nresponse):\n\n\t0. transport-helper.c invokes the remote helper. This requires\n\t a pipe used to send commands to the remote helper\n\t (helper->in) and a pipe used to receive responses from the\n\t remote helper (helper->out)\n\n\t1. transport-helper.c sends the \"capabilities\" command to decide\n\t what to do. The remote helper replies that it would like\n\t some feedback from fast-import.\n\n\t2. transport-helper.c forks and execs git fast-import with input\n\t redirected from helper->out and the cat-blob fd redirected\n\t to helper->in\n\n\t3. transport-helper.c tells the remote helper to start the\n\t import\n\n\t4. wait for fast-import to exit\n"},{"id":"196125","messageId":"19477122.a5lMBqWgns@flomedio","threadId":"30705","inReplyTo":"20120730082951.GA7702@burratino","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-07-30T13:55:14Z","receivedAt":"2012-07-30T13:55:14Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"On Monday 30 July 2012 03:29:52 Jonathan Nieder wrote:\n> > Generally I like your prefered solution.\n> > I think there's one problem:\n> > The pipe needs to be created before the fork, so that the fd can be\n> > inherited. \n> The relevant pipe already exists at that point: the remote helper's\n> stdin.\n> \n> In other words, it could work like this (just like the existing demo\n> code, except adding a conditional based on the \"capabilities\"\n> response):\n> \n> 0. transport-helper.c invokes the remote helper. This requires\n> a pipe used to send commands to the remote helper\n> (helper->in) and a pipe used to receive responses from the\n> remote helper (helper->out)\n> \n> 1. transport-helper.c sends the \"capabilities\" command to decide\n> what to do. The remote helper replies that it would like\n> some feedback from fast-import.\n> \n> 2. transport-helper.c forks and execs git fast-import with input\n> redirected from helper->out and the cat-blob fd redirected\n> to helper->in\n\nfast-import writes to the helpers stdin..\n\n> 3. transport-helper.c tells the remote helper to start the\n> import\n\ntransport-helper writes commands to the helper's stdin.\n\n> \n> 4. wait for fast-import to exit\n\nHm .. that would mean, that both fast-import and git (transport-helper) would \nwrite to the remote-helper's stdin, right?\n"},{"id":"196160","messageId":"20120730165502.GB8515@burratino","threadId":"30705","inReplyTo":"19477122.a5lMBqWgns@flomedio","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-30T16:55:02Z","receivedAt":"2012-07-30T16:55:02Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Florian Achleitner wrote:\n\n> Hm .. that would mean, that both fast-import and git (transport-helper) would \n> write to the remote-helper's stdin, right?\n\nYes, first git writes the list of refs to import, and then fast-import\nwrites feedback during the import. Is that a problem?\n"},{"id":"196255","messageId":"2351904.F5IazNUWoD@flomedio","threadId":"30705","inReplyTo":"20120730165502.GB8515@burratino","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-07-31T19:31:16Z","receivedAt":"2012-07-31T19:31:16Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"On Monday 30 July 2012 11:55:02 Jonathan Nieder wrote:\n> Florian Achleitner wrote:\n> > Hm .. that would mean, that both fast-import and git (transport-helper)\n> > would write to the remote-helper's stdin, right?\n> \n> Yes, first git writes the list of refs to import, and then fast-import\n> writes feedback during the import. Is that a problem?\n\nI haven't tried that yet, nor do I remember anything where I've already seen \ntwo processes writing to the same pipe.\nAt least it sounds cumbersome to me. Processes' lifetimes overlap, so buffering \nand flushing could mix data.\nWe have to use it for both purposes interchangably because there can be more \nthan one import command to the remote-helper, of course.\n\nWill try that in test-program..\n"},{"id":"196268","messageId":"CAFzf2XzC4Y1AhBV4BU5zZ411f=oVzoOyNA=e1L2eZd3bjyEgjQ@mail.gmail.com","threadId":"30705","inReplyTo":"2351904.F5IazNUWoD@flomedio","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-31T22:43:57Z","receivedAt":"2012-07-31T22:43:57Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Florian Achleitner wrote:\n\n> I haven't tried that yet, nor do I remember anything where I've already seen\n> two processes writing to the same pipe.\n\nIt's a perfectly normal and well supported thing to do.\n\n[...]\n> Will try that in test-program..\n\nThanks.\n\nGood luck,\nJonathan\n"},{"id":"196288","messageId":"1447696.eZjtSkvvWp@flomedio","threadId":"30705","inReplyTo":"CAFzf2XzC4Y1AhBV4BU5zZ411f=oVzoOyNA=e1L2eZd3bjyEgjQ@mail.gmail.com","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-08-01T08:25:40Z","receivedAt":"2012-08-01T08:25:40Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"On Tuesday 31 July 2012 15:43:57 Jonathan Nieder wrote:\n> Florian Achleitner wrote:\n> > I haven't tried that yet, nor do I remember anything where I've already\n> > seen two processes writing to the same pipe.\n> \n> It's a perfectly normal and well supported thing to do.\n\nI played around with a little testprogram. It generally works.\nI'm still not convinced that this doesn't cause more problems than it can \nsolve.\nThe standard defines that write calls to pipe fds are atomic, i.e. data is not \ninterleaved with data from other processes, if the data is less than PIPE_BUF \n[1].\nWe would need some kind of locking/synchronization to make it work for sure, \nwhile I believe it will work most of the time.\n\nCurrently it runs like this:\ntransport-helper.c writes one or more 'import ' lines, we don't know in \nadvance how many and how long they are. Then it waits for fast-import to \nfinish.\n\nWhen the first line arrives at the remote-helper, it starts importing one line \nat a time, leaving the remaining lines in the pipe.\nFor importing it requires the data from fast-import, which would be mixed with \nimport lines or queued at the end of them.\n\n[1] \nhttp://pubs.opengroup.org/onlinepubs/009695399/functions/write.html\n"},{"id":"196300","messageId":"20120801194247.GE24357@copier","threadId":"30705","inReplyTo":"1447696.eZjtSkvvWp@flomedio","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-08-01T19:42:48Z","receivedAt":"2012-08-01T19:42:48Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi again,\n\nFlorian Achleitner wrote:\n\n> When the first line arrives at the remote-helper, it starts importing one line \n> at a time, leaving the remaining lines in the pipe.\n> For importing it requires the data from fast-import, which would be mixed with \n> import lines or queued at the end of them.\n\nOh, good catch.\n\nThe way it's supposed to work is that in a bidi-import, the remote\nhelper reads in the entire list of refs to be imported and only once\nthe newline indicating that that list is over arrives starts writing\nits fast-import stream. We could make this more obvious by not\nspawning fast-import until immediately before writing that newline.\n\nThis needs to be clearly documented in the git-remote-helpers(1) page\nif the bidi-import command is introduced.\n\nIf a remote helper writes commands for fast-import before that newline\ncomes, that is a bug in the remote helper, plain and simple. It might\nbe fun to diagnose this problem:\n\n\tstatic void pipe_drained_or_die(int fd, const char *msg)\n\t{\n\t\tchar buf[1];\n\t\tint flags = fcntl(fd, F_GETFL);\n\t\tif (flags < 0)\n\t\t\tdie_errno(\"cannot get pipe flags\");\n\t\tif (fcntl(fd, F_SETFL, flags | O_NONBLOCK))\n\t\t\tdie_errno(\"cannot set up non-blocking pipe read\");\n\t\tif (read(fd, buf, 1) > 0)\n\t\t\tdie(\"%s\", msg);\n\t\tif (fcntl(fd, F_SETFL, flags))\n\t\t\tdie_errno(\"cannot restore pipe flags\");\n\t}\n\t...\n\n\tfor (i = 0; i < nr_heads; i++) {\n\t\twrite \"import %s\\n\", to_fetch[i]->name;\n\t}\n\n\tif (getenv(\"GIT_REMOTE_HELPERS_SLOW_SANITY_CHECK\"))\n\t\tsleep(1);\n\n\tpipe_drained_or_die(\"unexpected output from remote helper before fast-import launch\");\n\n\tif (get_importer(transport, &fastimport))\n\t\tdie(\"couldn't run fast-import\");\n\twrite_constant(data->helper->in, \"\\n\");\n"},{"id":"196880","messageId":"1636924.tANzCnKezB@flobuntu","threadId":"30705","inReplyTo":"20120801194247.GE24357@copier","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-08-12T10:06:59Z","receivedAt":"2012-08-12T10:06:59Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"Hi,\n\nback to the pipe-topic.\n\nOn Wednesday 01 August 2012 12:42:48 Jonathan Nieder wrote:\n> Hi again,\n> \n> Florian Achleitner wrote:\n> > When the first line arrives at the remote-helper, it starts importing one\n> > line at a time, leaving the remaining lines in the pipe.\n> > For importing it requires the data from fast-import, which would be mixed\n> > with import lines or queued at the end of them.\n> \n> Oh, good catch.\n> \n> The way it's supposed to work is that in a bidi-import, the remote\n> helper reads in the entire list of refs to be imported and only once\n> the newline indicating that that list is over arrives starts writing\n> its fast-import stream. We could make this more obvious by not\n> spawning fast-import until immediately before writing that newline.\n> \n> This needs to be clearly documented in the git-remote-helpers(1) page\n> if the bidi-import command is introduced.\n> \n> If a remote helper writes commands for fast-import before that newline\n> comes, that is a bug in the remote helper, plain and simple. It might\n> be fun to diagnose this problem:\n\nThis would require all existing remote helpers that use 'import' to be ported \nto the new concept, right? Probably there is no other..\n\n> \n> \tstatic void pipe_drained_or_die(int fd, const char *msg)\n> \t{\n> \t\tchar buf[1];\n> \t\tint flags = fcntl(fd, F_GETFL);\n> \t\tif (flags < 0)\n> \t\t\tdie_errno(\"cannot get pipe flags\");\n> \t\tif (fcntl(fd, F_SETFL, flags | O_NONBLOCK))\n> \t\t\tdie_errno(\"cannot set up non-blocking pipe read\");\n> \t\tif (read(fd, buf, 1) > 0)\n> \t\t\tdie(\"%s\", msg);\n> \t\tif (fcntl(fd, F_SETFL, flags))\n> \t\t\tdie_errno(\"cannot restore pipe flags\");\n> \t}\n> \t...\n> \n> \tfor (i = 0; i < nr_heads; i++) {\n> \t\twrite \"import %s\\n\", to_fetch[i]->name;\n> \t}\n> \n> \tif (getenv(\"GIT_REMOTE_HELPERS_SLOW_SANITY_CHECK\"))\n> \t\tsleep(1);\n> \n> \tpipe_drained_or_die(\"unexpected output from remote helper before\n> fast-import launch\");\n> \n> \tif (get_importer(transport, &fastimport))\n> \t\tdie(\"couldn't run fast-import\");\n> \twrite_constant(data->helper->in, \"\\n\");\n\nI still don't believe that sharing the input pipe of the remote helper is \nworth the hazzle.\nIt still requires an additional pipe to be setup, the one from fast-import to \nthe remote-helper, sharing one FD at the remote helper.\nIt still requires more than just stdin, stdout, stderr.\n\nI would suggest to use a fifo. It can be openend independently, after forking \nand on windows they have named pipes with similar semantics, so I think this \ncould be easily ported. \nI would suggest the following changes:\n- add a capability to the remote helper 'bidi-import', or 'bidi-pipe'. This \nsignals that the remote helper requires data from fast-import.\n\n- add a command 'bidi-import', or 'bidi-pipe' that is tells the remote helper \nwhich filename the fifo is at, so that it can open it and read it when it \nhandles 'import' commands.\n\n- transport-helper.c creates the fifo on demand, i.e. on seeing the capability, \nin the gitdir or in /tmp.\n\n- fast-import gets the name of the fifo as a command-line argument. The \nalternative would be to add a command, but that's not allowed, because it \nchanges the stream semantics.\nAnother alternative would be to use the existing --cat-pipe-fd argument. But \nthat requires to open the fifo before execing fast-import and makes us \ndependent on the posix model of forking and inheriting file descriptors, while \nopening a fifo in fast-import would not.\n"},{"id":"196885","messageId":"20120812161258.GA3829@mannheim-rule.local","threadId":"30705","inReplyTo":"1636924.tANzCnKezB@flobuntu","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-08-12T16:12:58Z","receivedAt":"2012-08-12T16:12:58Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi again,\n\nFlorian Achleitner wrote:\n\n> back to the pipe-topic.\n\nOk, thanks.\n\n[...]\n>> The way it's supposed to work is that in a bidi-import, the remote\n>> helper reads in the entire list of refs to be imported and only once\n>> the newline indicating that that list is over arrives starts writing\n>> its fast-import stream.\n[...]\n> This would require all existing remote helpers that use 'import' to be ported \n> to the new concept, right? Probably there is no other..\n\nYou mean all existing remote helpers that use 'bidi-import', right?\nThere are none.\n\n[...]\n> I still don't believe that sharing the input pipe of the remote helper is \n> worth the hazzle.\n> It still requires an additional pipe to be setup, the one from fast-import to \n> the remote-helper, sharing one FD at the remote helper.\n\nIf I understand correctly, you misunderstood how sharing the input\npipe works. Have you tried it?\n\nIt does not involve setting up an additional pipe. Standard input for\nthe remote helper is already a pipe. That pipe is what allows\ntransport-helper.c to communicate with the remote helper. Letting\nfast-import share that pipe involves passing that file descriptor to\ngit fast-import. No additional pipe() calls.\n\nDo you mean that it would be too much work to implement? This\nexplanation just doesn't make sense to me, given that the version\nusing pipe() *already* *exists* and is *tested*.\n\nI get the feeling I am missing something very basic. I would welcome\ninput from others that shows what I am missing.\n\n[...]\n> Another alternative would be to use the existing --cat-pipe-fd argument. But \n> that requires to open the fifo before execing fast-import and makes us \n> dependent on the posix model of forking and inheriting file descriptors, while \n> opening a fifo in fast-import would not.\n\nI'm getting kind of frustrated with this conversation going nowhere. Here\nis a compromise to explain why. Suppose:\n\n- fast-import learns a --cat-blob-file parameter, which tells it to open a\n file to write responses to \"cat-blob\" and \"ls\" commands to instead of\n using an inherited file descriptor\n\n- transport-helper.c sets up a named pipe and passes it using --cat-blob-file.\n\n- transport-helper.c reads from that named pipe and copies everything it sees\n to the remote helper's standard input, until fast-import exits.\n\nThis would:\n\n - allow us to continue to use a very simple protocol for communicating\n with the remote helper, where commands and fast-import backflow both\n come on standard input\n\n - work fine on both Windows and Unix\n\nMeanwhile it would:\n\n - be 100% functionally equivalent to the solution where fast-import\n writes directly to the remote helper's standard input. Two programs\n can have the same pipe open for writing at the same time for a few\n seconds and that is *perfectly fine*. On Unix and on Windows.\n\n On Windows the only complication with the pipe()-based is that we haven't\n wired up the low-level logic to pass file descriptors other than\n stdin, stdout, stderr to child processes; and if I have understood\n earlier messages correctly, the operating system *does* have a\n concept of that and this is just a todo item in msys\n implementation.\n\n - be more complicated than the code that already exists for this\n stuff.\n\nSo while I presented this as a compromise, I don't see the point.\n\nIs your goal portability, a dislike of the interface, some\nimplementation detail I have missed, or something else? Could you\nexplain the problem as concisely but clearly as possible (perhaps\nusing an example) so that others like Sverre, Peff, or David can help\nthink through it and to explain it in a way that dim people like me\nunderstand what's going on?\n\nPuzzled,\nJonathan\n"},{"id":"196888","messageId":"20120812193640.GA4065@mannheim-rule.local","threadId":"30705","inReplyTo":"1636924.tANzCnKezB@flobuntu","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-08-12T19:36:40Z","receivedAt":"2012-08-12T19:36:40Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi again,\n\nFlorian Achleitner wrote:\n\n> Another alternative would be to use the existing --cat-pipe-fd argument. But \n> that requires to open the fifo before execing fast-import and makes us \n> dependent on the posix model of forking and inheriting file descriptors\n\nLet me elaborate on this, which I think is the real point (though I\ncould easily be wrong).\n\nProbably instead of just sending feedback I should have been sending\nsuggested patches to compare. You do not work for me and your time is\nnot my time, and I would not want to have the responsibility of\ndictating how your code works, anyway. Generally speaking, as long as\ncode is useful and not hurting maintainability, the best thing I can\ndo is to make it easy to incorporate. That has side benefits, too,\nlike giving an example of what it is like to have your code\nincorporated and creating momentum. Small mistakes can be fixed\nlater.\n\nUnfortunately here we are talking about two interfaces that need to\nbe stable. Mistakes stay --- once there is a UI wart in interfaces\npeople are using, we are stuck continuing to support it.\n\nTo make life even more complicated, there are two interfaces involved\nhere:\n\n - the remote-helper protocol, which I think it is very important\n to keep sane and stable. Despite the remote-helper protocol\n existing for a long time, it hasn't seen a lot of adoption\n outside git yet, and complicating the protocol will not help\n that.\n\n I am worried about what it would mean to add an additional\n stream on top of the standard input and output used in the current\n remote-helper protocol. Receiving responses to fast-import\n commands on stdin seems very simple. Maybe I am wrong?\n\n - the fast-import interface, which is also important but I'm not\n as worried about. Adding a new command-line parameter means\n importers that call fast-import can unnecessarily depend on\n newer versions of git, so it's still something to be avoided.\n\nGiven a particular interface, there may be multiple choices of how to\nimplement it. For example, on Unix the remote-helper protocol could\nbe fulfilled by letting fast-import inherit the file descriptor\nhelper->in, while on Windows it could for example be fulfilled by\nusing DuplicateHandle() to transmit the pipe handle before or after\nfast-import has already started running.\n\nThe implementation strategy can easily be changed later. What is\nimportant is that the interface be pleasant and *possible* to\nimplement sanely.\n\nHoping that clarifies a little,\nJonathan\n"},{"id":"196889","messageId":"2007117.uOeClQJdrW@flobuntu","threadId":"30705","inReplyTo":"20120812161258.GA3829@mannheim-rule.local","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Florian Achleitner","fromEmail":"florian.achleitner.2.6.31@gmail.com","sentAt":"2012-08-12T19:39:35Z","receivedAt":"2012-08-12T19:39:35Z","isPatch":false,"sender":{"key":"florian.achleitner.2.6.31@gmail.com","avatar":"https://avatars.githubusercontent.com/u/880777?v=4"},"body":"On Sunday 12 August 2012 09:12:58 Jonathan Nieder wrote:\n> Hi again,\n> \n> Florian Achleitner wrote:\n> > back to the pipe-topic.\n> \n> Ok, thanks.\n> \n> [...]\n> \n> >> The way it's supposed to work is that in a bidi-import, the remote\n> >> helper reads in the entire list of refs to be imported and only once\n> >> the newline indicating that that list is over arrives starts writing\n> >> its fast-import stream.\n> \n> [...]\n> \n> > This would require all existing remote helpers that use 'import' to be\n> > ported to the new concept, right? Probably there is no other..\n> \n> You mean all existing remote helpers that use 'bidi-import', right?\n> There are none.\n\nOk, it would not affect the existing import command.\n> \n> [...]\n> \n> > I still don't believe that sharing the input pipe of the remote helper is\n> > worth the hazzle.\n> > It still requires an additional pipe to be setup, the one from fast-import\n> > to the remote-helper, sharing one FD at the remote helper.\n> \n> If I understand correctly, you misunderstood how sharing the input\n> pipe works. Have you tried it?\n\nYes wrote a test program, sharing works, that's not the problem.\n\n> \n> It does not involve setting up an additional pipe. Standard input for\n> the remote helper is already a pipe. That pipe is what allows\n> transport-helper.c to communicate with the remote helper. Letting\n> fast-import share that pipe involves passing that file descriptor to\n> git fast-import. No additional pipe() calls.\n> \n> Do you mean that it would be too much work to implement? This\n> explanation just doesn't make sense to me, given that the version\n> using pipe() *already* *exists* and is *tested*.\n\nYes, that was the first version I wrote, and remote-svn-alpha uses.\n\n> \n> I get the feeling I am missing something very basic. I would welcome\n> input from others that shows what I am missing.\n> \n\nThis is how I see it, probably it's all wrong:\nI thought the main problem is, that we don't want processes to have *more than \nthree pipes attached*, i.e. stdout, stdin, stderr, because existing APIs don't \nallow it.\nWhen we share stdin of the remote helper, we achieve this goal for this one \nprocess, but fast-import still has an additional pipe:\nstdout --> shell;\nstderr --> shell; \nstdin <-- remote-helper; \nadditional_pipe --> remote-helper.\n\nThat's what I wanted to say: We still have more than three pipes on fast-\nimport.\nAnd we need to transfer that fourth file descriptor by inheritance and it's \nnumber as a command line argument. \nSo if we make the remote-helper have only three pipes by double-using stdin, \nbut fast-import still has four pipes, what problem does it solve?\n\nUsing fifos would remove the requirement to inherit more than three pipes. \nThat's my point.\n\n[..]\n> \n> Meanwhile it would:\n> \n> - be 100% functionally equivalent to the solution where fast-import\n> writes directly to the remote helper's standard input. Two programs\n> can have the same pipe open for writing at the same time for a few\n> seconds and that is *perfectly fine*. On Unix and on Windows.\n> \n> On Windows the only complication with the pipe()-based is that we\n> haven't wired up the low-level logic to pass file descriptors other than\n> stdin, stdout, stderr to child processes; and if I have understood earlier\n> messages correctly, the operating system *does* have a\n> concept of that and this is just a todo item in msys\n> implementation.\n\nI digged into MSDN and it seems it's not a problem at all on the windows api \nlayer. Pipe handles can be inherited. [1]\nIf the low-level logic once supports passing more than 3 fds, it will work on \nfast-import as well as remote-helper.\n\n> \n> - be more complicated than the code that already exists for this\n> stuff.\n> \n> So while I presented this as a compromise, I don't see the point.\n> \n> Is your goal portability, a dislike of the interface, some\n> implementation detail I have missed, or something else? Could you\n> explain the problem as concisely but clearly as possible (perhaps\n> using an example) so that others like Sverre, Peff, or David can help\n> think through it and to explain it in a way that dim people like me\n> understand what's going on?\n\nIt all started as portability-only discussion. On Linux, my first version would \nhave worked. It created an additional pipe before forking using pipe(). Runs \ngreat, it did it like remote-svn-alpha.sh.\n\nI wouldn't have started to produce something else or start a discussion on my \nown. But I was told, it's not good because of portability. This is the root of \nthis endless story. (you already know the thread, I think). Since weeks nobody \nof them is interested in that except you and me.\n \nSo if we accept having more than three pipes on a process, we have no more \nproblem.\nWe can dig out that first version, as well as write the one proposed by you.\nWhile your version saves some trouble by not requiring an additional pipe() \ncall and not requiring the prexec_cb, but adding a little complexity with the \nre-using of stdin.\n\nCurrently I have the implemented the original pipe version, the original fifo \nversion, the fifo version described a mail ago. I'm going to implement the \nstdin-sharing version now..\n\n[1] http://msdn.microsoft.com/en-\nus/library/windows/desktop/aa365782(v=vs.85).aspx\n\n> \n> Puzzled,\n> Jonathan\n\nPiped,\nFlorian ;)\n"},{"id":"196891","messageId":"20120812201036.GB4065@mannheim-rule.local","threadId":"30705","inReplyTo":"2007117.uOeClQJdrW@flobuntu","subject":"Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-08-12T20:10:36Z","receivedAt":"2012-08-12T20:10:36Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Florian Achleitner wrote:\n\n> This is how I see it, probably it's all wrong:\n> I thought the main problem is, that we don't want processes to have *more than\n> three pipes attached*, i.e. stdout, stdin, stderr, because existing APIs don't\n> allow it.\n\nOh, that makes sense. Thanks for explaining, and sorry to have been so\ndense.\n\nAt the Windows API level, Set/GetStdHandle() is only advertised to\nhandle stdin, stdout, and stderr, so on Windows there would indeed\nneed to be some magic to communicate the inherited HANDLE value to\nfast-import.\n\nBut I am confident in the Windows porters, and if a fast-import\ninterface change ends up being needed, I think we can deal with it\nwhen the moment comes and it wouldn't necessitate changing the remote\nhelper interface.\n\nYou also mentioned before that passing fast-import responses to the\nremote helper stdin means that the remote helper has to slurp up the\nwhole list of refs to import before starting to talk to fast-import.\nThat was good practice already anyway, but it is a real pitfall and a\nreal downside to the single-input approach. Thanks for bringing it\nup.\n\nJonathan\n"}]}