[PATCH/RFC v3 00/16] GSOC remote-svn

[PATCH/RFC v3 00/16] GSOC remote-svn

[Florian Achleitner]

Hi.

Version 3 of this series adds the 'bidi-import' capability, as suggested
Jonathan. 
Diff details are attached to the patches.
04 and 05 are completely new.

[PATCH/RFC v3 01/16] Implement a remote helper for svn in C.
[PATCH/RFC v3 02/16] Integrate remote-svn into svn-fe/Makefile.
[PATCH/RFC v3 03/16] Add svndump_init_fd to allow reading dumps from
[PATCH/RFC v3 04/16] Connect fast-import to the remote-helper via
[PATCH/RFC v3 05/16] Add documentation for the 'bidi-import'
[PATCH/RFC v3 06/16] remote-svn, vcs-svn: Enable fetching to private
[PATCH/RFC v3 07/16] Add a symlink 'git-remote-svn' in base dir.
[PATCH/RFC v3 08/16] Allow reading svn dumps from files via file://
[PATCH/RFC v3 09/16] vcs-svn: add fast_export_note to create notes
[PATCH/RFC v3 10/16] Create a note for every imported commit
[PATCH/RFC v3 11/16] When debug==1, start fast-import with "--stats"
[PATCH/RFC v3 12/16] remote-svn: add incremental import.
[PATCH/RFC v3 13/16] Add a svnrdump-simulator replaying a dump file
[PATCH/RFC v3 14/16] transport-helper: add import|export-marks to
[PATCH/RFC v3 15/16] remote-svn: add marks-file regeneration.
[PATCH/RFC v3 16/16] Add a test script for remote-svn.

[PATCH/RFC v3 01/16] Implement a remote helper for svn in C.

[Florian Achleitner]

Enable basic fetching from subversion repositories. When processing remote URLs
starting with svn::, git invokes this remote-helper.
It starts svnrdump to extract revisions from the subversion repository in the
'dump file format', and converts them to a git-fast-import stream using
the functions of vcs-svn/.

Imported refs are created in a private namespace at refs/svn/<remote-name/master.
The revision history is imported linearly (no branch detection) and completely,
i.e. from revision 0 to HEAD.

The 'bidi-import' capability is used. The remote-helper expects data from
fast-import on its stdin. It buffers a batch of 'import' command lines
in a string_list before starting to process them.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
diff:
- incorporate review
- remove redundant strbuf_init
- add 'bidi-import' to capabilities
- buffer all lines of a command batch in string_list

 contrib/svn-fe/remote-svn.c |  183 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 183 insertions(+)
 create mode 100644 contrib/svn-fe/remote-svn.c

diff --git a/contrib/svn-fe/remote-svn.c b/contrib/svn-fe/remote-svn.c
new file mode 100644
index 0000000..ce59344
--- /dev/null
+++ b/contrib/svn-fe/remote-svn.c
@@ -0,0 +1,183 @@
+
+#include "cache.h"
+#include "remote.h"
+#include "strbuf.h"
+#include "url.h"
+#include "exec_cmd.h"
+#include "run-command.h"
+#include "svndump.h"
+#include "notes.h"
+#include "argv-array.h"
+
+static const char *url;
+static const char *private_ref;
+static const char *remote_ref = "refs/heads/master";
+
+static int cmd_capabilities(const char *line);
+static int cmd_import(const char *line);
+static int cmd_list(const char *line);
+
+typedef int (*input_command_handler)(const char *);
+struct input_command_entry {
+	const char *name;
+	input_command_handler fct;
+	unsigned char batchable;	/* whether the command starts or is part of a batch */
+};
+
+static const struct input_command_entry input_command_list[] = {
+		{ "capabilities", cmd_capabilities, 0 },
+		{ "import", cmd_import, 1 },
+		{ "list", cmd_list, 0 },
+		{ NULL, NULL }
+};
+
+static int cmd_capabilities(const char *line) {
+	printf("import\n");
+	printf("bidi-import\n");
+	printf("refspec %s:%s\n\n", remote_ref, private_ref);
+	fflush(stdout);
+	return 0;
+}
+
+static void terminate_batch(void)
+{
+	/* terminate a current batch's fast-import stream */
+		printf("done\n");
+		fflush(stdout);
+}
+
+static int cmd_import(const char *line)
+{
+	int code;
+	int dumpin_fd;
+	unsigned int startrev = 0;
+	struct argv_array svndump_argv = ARGV_ARRAY_INIT;
+	struct child_process svndump_proc;
+
+	memset(&svndump_proc, 0, sizeof (struct child_process));
+	svndump_proc.out = -1;
+	argv_array_push(&svndump_argv, "svnrdump");
+	argv_array_push(&svndump_argv, "dump");
+	argv_array_push(&svndump_argv, url);
+	argv_array_pushf(&svndump_argv, "-r%u:HEAD", startrev);
+	svndump_proc.argv = svndump_argv.argv;
+
+	code = start_command(&svndump_proc);
+	if (code)
+		die("Unable to start %s, code %d", svndump_proc.argv[0], code);
+	dumpin_fd = svndump_proc.out;
+
+	code = start_command(&svndump_proc);
+	if (code)
+		die("Unable to start %s, code %d", svndump_proc.argv[0], code);
+	dumpin_fd = svndump_proc.out;
+
+	svndump_init_fd(dumpin_fd, STDIN_FILENO);
+	svndump_read(url, private_ref);
+	svndump_deinit();
+	svndump_reset();
+
+	close(dumpin_fd);
+	code = finish_command(&svndump_proc);
+	if (code)
+		warning("%s, returned %d", svndump_proc.argv[0], code);
+	argv_array_clear(&svndump_argv);
+
+	return 0;
+}
+
+static int cmd_list(const char *line)
+{
+	printf("? %s\n\n", remote_ref);
+	fflush(stdout);
+	return 0;
+}
+
+static int do_command(struct strbuf *line)
+{
+	const struct input_command_entry *p = input_command_list;
+	static struct string_list batchlines = STRING_LIST_INIT_DUP;
+	static const struct input_command_entry *batch_cmd;
+	/*
+	 * commands can be grouped together in a batch.
+	 * Batches are ended by \n. If no batch is active the program ends.
+	 * During a batch all lines are buffered and passed to the handler function
+	 * when the batch is terminated.
+	 */
+	if (line->len == 0) {
+		if (batch_cmd) {
+			struct string_list_item *item;
+			for_each_string_list_item(item, &batchlines)
+				batch_cmd->fct(item->string);
+			terminate_batch();
+			batch_cmd = NULL;
+			string_list_clear(&batchlines, 0);
+			return 0;	/* end of the batch, continue reading other commands. */
+		}
+		return 1;	/* end of command stream, quit */
+	}
+	if (batch_cmd) {
+		if (strcmp(batch_cmd->name, line->buf))
+			die("Active %s batch interrupted by %s", batch_cmd->name, line->buf);
+		/* buffer batch lines */
+		string_list_append(&batchlines, line->buf);
+		return 0;
+	}
+
+	for(p = input_command_list; p->name; p++) {
+		if (!prefixcmp(line->buf, p->name) &&
+				(strlen(p->name) == line->len || line->buf[strlen(p->name)] == ' ')) {
+			if (p->batchable) {
+				batch_cmd = p;
+				string_list_append(&batchlines, line->buf);
+				return 0;
+			}
+			return p->fct(line->buf);
+		}
+	}
+	warning("Unknown command '%s'\n", line->buf);
+	return 0;
+}
+
+int main(int argc, const char **argv)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int nongit;
+	static struct remote *remote;
+	const char *url_in;
+
+	git_extract_argv0_path(argv[0]);
+	setup_git_directory_gently(&nongit);
+	if (argc < 2 || argc > 3) {
+		usage("git-remote-svn <remote-name> [<url>]");
+		return 1;
+	}
+
+	remote = remote_get(argv[1]);
+	url_in = remote->url[0];
+	if (argc == 3)
+		url_in = argv[2];
+
+	end_url_with_slash(&buf, url_in);
+	url = strbuf_detach(&buf, NULL);
+
+	strbuf_addf(&buf, "refs/svn/%s/master", remote->name);
+	private_ref = strbuf_detach(&buf, NULL);
+
+	while(1) {
+		if (strbuf_getline(&buf, stdin, '\n') == EOF) {
+			if (ferror(stdin))
+				die("Error reading command stream");
+			else
+				die("Unexpected end of command stream");
+		}
+		if (do_command(&buf))
+			break;
+		strbuf_reset(&buf);
+	}
+
+	strbuf_release(&buf);
+	free((void*)url);
+	free((void*)private_ref);
+	return 0;
+}
-- 
1.7.9.5

[PATCH/RFC v3 02/16] Integrate remote-svn into svn-fe/Makefile.

[Florian Achleitner]

Requires some sha.h to be used and the libraries
to be linked, this is currently hardcoded.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 contrib/svn-fe/Makefile |   16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/contrib/svn-fe/Makefile b/contrib/svn-fe/Makefile
index 360d8da..8f0eec2 100644
--- a/contrib/svn-fe/Makefile
+++ b/contrib/svn-fe/Makefile
@@ -1,14 +1,14 @@
-all:: svn-fe$X
+all:: svn-fe$X remote-svn$X
 
 CC = gcc
 RM = rm -f
 MV = mv
 
-CFLAGS = -g -O2 -Wall
+CFLAGS = -g -O2 -Wall -DSHA1_HEADER='<openssl/sha.h>' -Wdeclaration-after-statement
 LDFLAGS =
 ALL_CFLAGS = $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
-EXTLIBS =
+EXTLIBS = -lssl -lcrypto -lpthread ../../xdiff/lib.a
 
 GIT_LIB = ../../libgit.a
 VCSSVN_LIB = ../../vcs-svn/lib.a
@@ -37,8 +37,12 @@ svn-fe$X: svn-fe.o $(VCSSVN_LIB) $(GIT_LIB)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ svn-fe.o \
 		$(ALL_LDFLAGS) $(LIBS)
 
-svn-fe.o: svn-fe.c ../../vcs-svn/svndump.h
-	$(QUIET_CC)$(CC) -I../../vcs-svn -o $*.o -c $(ALL_CFLAGS) $<
+remote-svn$X: remote-svn.o $(VCSSVN_LIB) $(GIT_LIB)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ remote-svn.o \
+		$(ALL_LDFLAGS) $(LIBS)
+		
+%.o: %.c ../../vcs-svn/svndump.h
+	$(QUIET_CC)$(CC) -I../../vcs-svn -I../../ -o $*.o -c $(ALL_CFLAGS) $<
 
 svn-fe.html: svn-fe.txt
 	$(QUIET_SUBDIR0)../../Documentation $(QUIET_SUBDIR1) \
@@ -58,6 +62,6 @@ svn-fe.1: svn-fe.txt
 	$(QUIET_SUBDIR0)../.. $(QUIET_SUBDIR1) libgit.a
 
 clean:
-	$(RM) svn-fe$X svn-fe.o svn-fe.html svn-fe.xml svn-fe.1
+	$(RM) svn-fe$X svn-fe.o svn-fe.html svn-fe.xml svn-fe.1 remote-svn.o
 
 .PHONY: all clean FORCE
-- 
1.7.9.5

[PATCH/RFC v3 03/16] Add svndump_init_fd to allow reading dumps from arbitrary FDs.

[Florian Achleitner]

The existing function only allows reading from a filename or
from stdin. Allow passing of a FD and an additional FD for
the back report pipe. This allows us to retrieve the name of
the pipe in the caller.

Fixes the filename could be NULL bug.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
- dup input file descriptor, because buffer_deinit closes the fd.
 vcs-svn/svndump.c |   22 ++++++++++++++++++----
 vcs-svn/svndump.h |    1 +
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 2b168ae..d81a078 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -468,11 +468,9 @@ void svndump_read(const char *url)
 		end_revision();
 }
 
-int svndump_init(const char *filename)
+static void init(int report_fd)
 {
-	if (buffer_init(&input, filename))
-		return error("cannot open %s: %s", filename, strerror(errno));
-	fast_export_init(REPORT_FILENO);
+	fast_export_init(report_fd);
 	strbuf_init(&dump_ctx.uuid, 4096);
 	strbuf_init(&dump_ctx.url, 4096);
 	strbuf_init(&rev_ctx.log, 4096);
@@ -482,6 +480,22 @@ int svndump_init(const char *filename)
 	reset_dump_ctx(NULL);
 	reset_rev_ctx(0);
 	reset_node_ctx(NULL);
+	return;
+}
+
+int svndump_init(const char *filename)
+{
+	if (buffer_init(&input, filename))
+		return error("cannot open %s: %s", filename ? filename : "NULL", strerror(errno));
+	init(REPORT_FILENO);
+	return 0;
+}
+
+int svndump_init_fd(int in_fd, int back_fd)
+{
+	if(buffer_fdinit(&input, xdup(in_fd)))
+		return error("cannot open fd %d: %s", in_fd, strerror(errno));
+	init(xdup(back_fd));
 	return 0;
 }
 
diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h
index df9ceb0..acb5b47 100644
--- a/vcs-svn/svndump.h
+++ b/vcs-svn/svndump.h
@@ -2,6 +2,7 @@
 #define SVNDUMP_H_
 
 int svndump_init(const char *filename);
+int svndump_init_fd(int in_fd, int back_fd);
 void svndump_read(const char *url);
 void svndump_deinit(void);
 void svndump_reset(void);
-- 
1.7.9.5

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

[Florian Achleitner]

The fast-import commands 'cat-blob' and 'ls' can be used by remote-helpers
to retrieve information about blobs and trees that already exist in
fast-import's memory. This requires a channel from fast-import to the
remote-helper.
remote-helpers that use this features shall advertise the new 'bidi-import'
capability so signal that they require the communication channel.
When forking fast-import in transport-helper.c connect it to a dup of
the remote-helper's stdin-pipe. The additional file descriptor is passed
to fast-import via it's command line (--cat-blob-fd).
It follows that git and fast-import are connected to the remote-helpers's
stdin.
Because git can send multiple commands to the remote-helper on it's stdin,
it is required that helpers that advertise 'bidi-import' buffer all input
commands until the batch of 'import' commands is ended by a newline
before sending data to fast-import.
This is to prevent mixing commands and fast-import responses on the
helper's stdin.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 transport-helper.c |   45 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 13 deletions(-)

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

[PATCH/RFC v3 05/16] Add documentation for the 'bidi-import' capability of remote-helpers.

[Florian Achleitner]

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 Documentation/git-remote-helpers.txt |   21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-remote-helpers.txt b/Documentation/git-remote-helpers.txt
index f5836e4..5faa48e 100644
--- a/Documentation/git-remote-helpers.txt
+++ b/Documentation/git-remote-helpers.txt
@@ -98,6 +98,20 @@ advertised with this capability must cover all refs reported by
 the list command.  If no 'refspec' capability is advertised,
 there is an implied `refspec *:*`.
 
+'bidi-import'::
+	The fast-import commands 'cat-blob' and 'ls' can be used by remote-helpers
+    to retrieve information about blobs and trees that already exist in
+    fast-import's memory. This requires a channel from fast-import to the
+    remote-helper.
+    If it is advertised in addition to "import", git establishes a pipe from
+	fast-import to the remote-helper's stdin.
+	It follows that git and fast-import are both connected to the
+	remote-helper's stdin. Because git can send multiple commands to
+	the remote-helper it is required that helpers that use 'bidi-import'
+	buffer all 'import' commands of a batch before sending data to fast-import.
+    This is to prevent mixing commands and fast-import responses on the
+    helper's stdin.
+
 Capabilities for Pushing
 ~~~~~~~~~~~~~~~~~~~~~~~~
 'connect'::
@@ -286,7 +300,12 @@ terminated with a blank line. For each batch of 'import', the remote
 helper should produce a fast-import stream terminated by a 'done'
 command.
 +
-Supported if the helper has the "import" capability.
+Note that if the 'bidi-import' capability is used the complete batch
+sequence has to be buffered before starting to send data to fast-import
+to prevent mixing of commands and fast-import responses on the helper's
+stdin.
++
+Supported if the helper has the 'import' capability.
 
 'connect' <service>::
 	Connects to given service. Standard input and standard output
-- 
1.7.9.5

[PATCH/RFC v3 06/16] remote-svn, vcs-svn: Enable fetching to private refs.

[Florian Achleitner]

The reference to update by the fast-import stream is hard-coded.
When fetching from a remote the remote-helper shall update refs
in a private namespace, i.e. a private subdir of refs/.
This namespace is defined by the 'refspec' capability, that the
remote-helper advertises as a reply to the 'capablilities' command.

Extend svndump and fast-export to allow passing the target ref.
Update svn-fe to be compatible.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
- fix hard-coded ref in test-svn-fe.c. Broke a testcase.

 contrib/svn-fe/svn-fe.c |    2 +-
 test-svn-fe.c           |    2 +-
 vcs-svn/fast_export.c   |    4 ++--
 vcs-svn/fast_export.h   |    2 +-
 vcs-svn/svndump.c       |   14 +++++++-------
 vcs-svn/svndump.h       |    2 +-
 6 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index 35db24f..c796cc0 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -10,7 +10,7 @@ int main(int argc, char **argv)
 {
 	if (svndump_init(NULL))
 		return 1;
-	svndump_read((argc > 1) ? argv[1] : NULL);
+	svndump_read((argc > 1) ? argv[1] : NULL, "refs/heads/master");
 	svndump_deinit();
 	svndump_reset();
 	return 0;
diff --git a/test-svn-fe.c b/test-svn-fe.c
index 83633a2..cb0d80f 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -40,7 +40,7 @@ int main(int argc, char *argv[])
 	if (argc == 2) {
 		if (svndump_init(argv[1]))
 			return 1;
-		svndump_read(NULL);
+		svndump_read(NULL, "refs/heads/master");
 		svndump_deinit();
 		svndump_reset();
 		return 0;
diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 1f04697..11f8f94 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -72,7 +72,7 @@ static char gitsvnline[MAX_GITSVN_LINE_LEN];
 void fast_export_begin_commit(uint32_t revision, const char *author,
 			const struct strbuf *log,
 			const char *uuid, const char *url,
-			unsigned long timestamp)
+			unsigned long timestamp, const char *local_ref)
 {
 	static const struct strbuf empty = STRBUF_INIT;
 	if (!log)
@@ -84,7 +84,7 @@ void fast_export_begin_commit(uint32_t revision, const char *author,
 	} else {
 		*gitsvnline = '\0';
 	}
-	printf("commit refs/heads/master\n");
+	printf("commit %s\n", local_ref);
 	printf("mark :%"PRIu32"\n", revision);
 	printf("committer %s <%s@%s> %ld +0000\n",
 		   *author ? author : "nobody",
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index 8823aca..17eb13b 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -11,7 +11,7 @@ void fast_export_delete(const char *path);
 void fast_export_modify(const char *path, uint32_t mode, const char *dataref);
 void fast_export_begin_commit(uint32_t revision, const char *author,
 			const struct strbuf *log, const char *uuid,
-			const char *url, unsigned long timestamp);
+			const char *url, unsigned long timestamp, const char *local_ref);
 void fast_export_end_commit(uint32_t revision);
 void fast_export_data(uint32_t mode, off_t len, struct line_buffer *input);
 void fast_export_blob_delta(uint32_t mode,
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index d81a078..288bb42 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -299,22 +299,22 @@ static void handle_node(void)
 				node_ctx.text_length, &input);
 }
 
-static void begin_revision(void)
+static void begin_revision(const char *remote_ref)
 {
 	if (!rev_ctx.revision)	/* revision 0 gets no git commit. */
 		return;
 	fast_export_begin_commit(rev_ctx.revision, rev_ctx.author.buf,
 		&rev_ctx.log, dump_ctx.uuid.buf, dump_ctx.url.buf,
-		rev_ctx.timestamp);
+		rev_ctx.timestamp, remote_ref);
 }
 
-static void end_revision(void)
+static void end_revision()
 {
 	if (rev_ctx.revision)
 		fast_export_end_commit(rev_ctx.revision);
 }
 
-void svndump_read(const char *url)
+void svndump_read(const char *url, const char *local_ref)
 {
 	char *val;
 	char *t;
@@ -353,7 +353,7 @@ void svndump_read(const char *url)
 			if (active_ctx == NODE_CTX)
 				handle_node();
 			if (active_ctx == REV_CTX)
-				begin_revision();
+				begin_revision(local_ref);
 			if (active_ctx != DUMP_CTX)
 				end_revision();
 			active_ctx = REV_CTX;
@@ -366,7 +366,7 @@ void svndump_read(const char *url)
 				if (active_ctx == NODE_CTX)
 					handle_node();
 				if (active_ctx == REV_CTX)
-					begin_revision();
+					begin_revision(local_ref);
 				active_ctx = NODE_CTX;
 				reset_node_ctx(val);
 				break;
@@ -463,7 +463,7 @@ void svndump_read(const char *url)
 	if (active_ctx == NODE_CTX)
 		handle_node();
 	if (active_ctx == REV_CTX)
-		begin_revision();
+		begin_revision(local_ref);
 	if (active_ctx != DUMP_CTX)
 		end_revision();
 }
diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h
index acb5b47..febeecb 100644
--- a/vcs-svn/svndump.h
+++ b/vcs-svn/svndump.h
@@ -3,7 +3,7 @@
 
 int svndump_init(const char *filename);
 int svndump_init_fd(int in_fd, int back_fd);
-void svndump_read(const char *url);
+void svndump_read(const char *url, const char *local_ref);
 void svndump_deinit(void);
 void svndump_reset(void);
 
-- 
1.7.9.5

[PATCH/RFC v3 07/16] Add a symlink 'git-remote-svn' in base dir.

[Florian Achleitner]

Allow execution of git-remote-svn even if the binary
currently is located in contrib/svn-fe/.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 git-remote-svn |    1 +
 1 file changed, 1 insertion(+)
 create mode 120000 git-remote-svn

diff --git a/git-remote-svn b/git-remote-svn
new file mode 120000
index 0000000..d3b1c07
--- /dev/null
+++ b/git-remote-svn
@@ -0,0 +1 @@
+contrib/svn-fe/remote-svn
\ No newline at end of file
-- 
1.7.9.5

[PATCH/RFC v3 08/16] Allow reading svn dumps from files via file:// urls.

[Florian Achleitner]

For testing as well as for importing large, already
available dumps, it's useful to bypass svnrdump and
replay the svndump from a file directly.

Add support for file:// urls in the remote url.
e.g. svn::file:///path/to/dump
When the remote helper finds an url starting with
file:// it tries to open that file instead of invoking svnrdump.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 contrib/svn-fe/remote-svn.c |   59 ++++++++++++++++++++++++++-----------------
 1 file changed, 36 insertions(+), 23 deletions(-)

diff --git a/contrib/svn-fe/remote-svn.c b/contrib/svn-fe/remote-svn.c
index ce59344..df1babc 100644
--- a/contrib/svn-fe/remote-svn.c
+++ b/contrib/svn-fe/remote-svn.c
@@ -10,6 +10,7 @@
 #include "argv-array.h"
 
 static const char *url;
+static int dump_from_file;
 static const char *private_ref;
 static const char *remote_ref = "refs/heads/master";
 
@@ -54,34 +55,39 @@ static int cmd_import(const char *line)
 	struct argv_array svndump_argv = ARGV_ARRAY_INIT;
 	struct child_process svndump_proc;
 
-	memset(&svndump_proc, 0, sizeof (struct child_process));
-	svndump_proc.out = -1;
-	argv_array_push(&svndump_argv, "svnrdump");
-	argv_array_push(&svndump_argv, "dump");
-	argv_array_push(&svndump_argv, url);
-	argv_array_pushf(&svndump_argv, "-r%u:HEAD", startrev);
-	svndump_proc.argv = svndump_argv.argv;
-
-	code = start_command(&svndump_proc);
-	if (code)
-		die("Unable to start %s, code %d", svndump_proc.argv[0], code);
-	dumpin_fd = svndump_proc.out;
-
-	code = start_command(&svndump_proc);
-	if (code)
-		die("Unable to start %s, code %d", svndump_proc.argv[0], code);
-	dumpin_fd = svndump_proc.out;
+	if(dump_from_file) {
+		dumpin_fd = open(url, O_RDONLY);
+		if(dumpin_fd < 0) {
+			die_errno("Couldn't open svn dump file %s.", url);
+		}
+	}
+	else {
+		memset(&svndump_proc, 0, sizeof (struct child_process));
+		svndump_proc.out = -1;
+		argv_array_push(&svndump_argv, "svnrdump");
+		argv_array_push(&svndump_argv, "dump");
+		argv_array_push(&svndump_argv, url);
+		argv_array_pushf(&svndump_argv, "-r%u:HEAD", startrev);
+		svndump_proc.argv = svndump_argv.argv;
+
+		code = start_command(&svndump_proc);
+		if (code)
+			die("Unable to start %s, code %d", svndump_proc.argv[0], code);
+		dumpin_fd = svndump_proc.out;
 
+	}
 	svndump_init_fd(dumpin_fd, STDIN_FILENO);
 	svndump_read(url, private_ref);
 	svndump_deinit();
 	svndump_reset();
 
 	close(dumpin_fd);
-	code = finish_command(&svndump_proc);
-	if (code)
-		warning("%s, returned %d", svndump_proc.argv[0], code);
-	argv_array_clear(&svndump_argv);
+	if(!dump_from_file) {
+		code = finish_command(&svndump_proc);
+		if (code)
+			warning("%s, returned %d", svndump_proc.argv[0], code);
+		argv_array_clear(&svndump_argv);
+	}
 
 	return 0;
 }
@@ -158,8 +164,15 @@ int main(int argc, const char **argv)
 	if (argc == 3)
 		url_in = argv[2];
 
-	end_url_with_slash(&buf, url_in);
-	url = strbuf_detach(&buf, NULL);
+	if (!prefixcmp(url_in, "file://")) {
+		dump_from_file = 1;
+		url = url_decode(url_in + sizeof("file://")-1);
+	}
+	else {
+		dump_from_file = 0;
+		end_url_with_slash(&buf, url_in);
+		url = strbuf_detach(&buf, NULL);
+	}
 
 	strbuf_addf(&buf, "refs/svn/%s/master", remote->name);
 	private_ref = strbuf_detach(&buf, NULL);
-- 
1.7.9.5

[PATCH/RFC v3 09/16] vcs-svn: add fast_export_note to create notes

[Florian Achleitner]

From: Dmitry Ivankov <divanorama@gmail.com>

fast_export lacked a method to writes notes to fast-import stream.
Add two new functions fast_export_note which is similar to
fast_export_modify. And also add fast_export_buf_to_data to be able
to write inline blobs that don't come from a line_buffer or from delta
application.

To be used like this:
fast_export_begin_commit("refs/notes/somenotes", ...)

fast_export_note("refs/heads/master", "inline")
fast_export_buf_to_data(&data)
or maybe
fast_export_note("refs/heads/master", sha1)

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 vcs-svn/fast_export.c |   12 ++++++++++++
 vcs-svn/fast_export.h |    2 ++
 2 files changed, 14 insertions(+)

diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 11f8f94..1ecae4b 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -68,6 +68,11 @@ void fast_export_modify(const char *path, uint32_t mode, const char *dataref)
 	putchar('\n');
 }
 
+void fast_export_note(const char *committish, const char *dataref)
+{
+	printf("N %s %s\n", dataref, committish);
+}
+
 static char gitsvnline[MAX_GITSVN_LINE_LEN];
 void fast_export_begin_commit(uint32_t revision, const char *author,
 			const struct strbuf *log,
@@ -222,6 +227,13 @@ static long apply_delta(off_t len, struct line_buffer *input,
 	return ret;
 }
 
+void fast_export_buf_to_data(const struct strbuf *data)
+{
+	printf("data %"PRIuMAX"\n", (uintmax_t)data->len);
+	fwrite(data->buf, data->len, 1, stdout);
+	fputc('\n', stdout);
+}
+
 void fast_export_data(uint32_t mode, off_t len, struct line_buffer *input)
 {
 	assert(len >= 0);
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index 17eb13b..9b32f1e 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -9,11 +9,13 @@ void fast_export_deinit(void);
 
 void fast_export_delete(const char *path);
 void fast_export_modify(const char *path, uint32_t mode, const char *dataref);
+void fast_export_note(const char *committish, const char *dataref);
 void fast_export_begin_commit(uint32_t revision, const char *author,
 			const struct strbuf *log, const char *uuid,
 			const char *url, unsigned long timestamp, const char *local_ref);
 void fast_export_end_commit(uint32_t revision);
 void fast_export_data(uint32_t mode, off_t len, struct line_buffer *input);
+void fast_export_buf_to_data(const struct strbuf *data);
 void fast_export_blob_delta(uint32_t mode,
 			uint32_t old_mode, const char *old_data,
 			off_t len, struct line_buffer *input);
-- 
1.7.9.5

[PATCH/RFC v3 10/16] Create a note for every imported commit containing svn metadata.

[Florian Achleitner]

To provide metadata from svn dumps for further processing, e.g.
branch detection, attach a note to each imported commit that
stores additional information.
The notes are currently hard-coded in refs/notes/svn/revs.
Currently the following lines from the svn dump are directly
accumulated in the note. This can be refined on purpose, of course.
- "Revision-number"
- "Node-path"
- "Node-kind"
- "Node-action"
- "Node-copyfrom-path"
- "Node-copyfrom-rev"

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 vcs-svn/fast_export.c |   13 +++++++++++++
 vcs-svn/fast_export.h |    2 ++
 vcs-svn/svndump.c     |   21 +++++++++++++++++++--
 3 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 1ecae4b..796dd1a 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -12,6 +12,7 @@
 #include "svndiff.h"
 #include "sliding_window.h"
 #include "line_buffer.h"
+#include "cache.h"
 
 #define MAX_GITSVN_LINE_LEN 4096
 
@@ -68,6 +69,18 @@ void fast_export_modify(const char *path, uint32_t mode, const char *dataref)
 	putchar('\n');
 }
 
+void fast_export_begin_note(uint32_t revision, const char *author,
+		const char *log, unsigned long timestamp)
+{
+	timestamp = 1341914616;
+	size_t loglen = strlen(log);
+	printf("commit refs/notes/svn/revs\n");
+	printf("committer %s <%s@%s> %ld +0000\n", author, author, "local", timestamp);
+	printf("data %"PRIuMAX"\n", loglen);
+	fwrite(log, loglen, 1, stdout);
+	fputc('\n', stdout);
+}
+
 void fast_export_note(const char *committish, const char *dataref)
 {
 	printf("N %s %s\n", dataref, committish);
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index 9b32f1e..c2f6f11 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -10,6 +10,8 @@ void fast_export_deinit(void);
 void fast_export_delete(const char *path);
 void fast_export_modify(const char *path, uint32_t mode, const char *dataref);
 void fast_export_note(const char *committish, const char *dataref);
+void fast_export_begin_note(uint32_t revision, const char *author,
+		const char *log, unsigned long timestamp);
 void fast_export_begin_commit(uint32_t revision, const char *author,
 			const struct strbuf *log, const char *uuid,
 			const char *url, unsigned long timestamp, const char *local_ref);
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 288bb42..cd65b51 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -48,7 +48,7 @@ static struct {
 static struct {
 	uint32_t revision;
 	unsigned long timestamp;
-	struct strbuf log, author;
+	struct strbuf log, author, note;
 } rev_ctx;
 
 static struct {
@@ -77,6 +77,7 @@ static void reset_rev_ctx(uint32_t revision)
 	rev_ctx.timestamp = 0;
 	strbuf_reset(&rev_ctx.log);
 	strbuf_reset(&rev_ctx.author);
+	strbuf_reset(&rev_ctx.note);
 }
 
 static void reset_dump_ctx(const char *url)
@@ -310,8 +311,15 @@ static void begin_revision(const char *remote_ref)
 
 static void end_revision()
 {
-	if (rev_ctx.revision)
+	struct strbuf mark = STRBUF_INIT;
+	if (rev_ctx.revision) {
 		fast_export_end_commit(rev_ctx.revision);
+		fast_export_begin_note(rev_ctx.revision, "remote-svn",
+				"Note created by remote-svn.", rev_ctx.timestamp);
+		strbuf_addf(&mark, ":%"PRIu32, rev_ctx.revision);
+		fast_export_note(mark.buf, "inline");
+		fast_export_buf_to_data(&rev_ctx.note);
+	}
 }
 
 void svndump_read(const char *url, const char *local_ref)
@@ -358,6 +366,7 @@ void svndump_read(const char *url, const char *local_ref)
 				end_revision();
 			active_ctx = REV_CTX;
 			reset_rev_ctx(atoi(val));
+			strbuf_addf(&rev_ctx.note, "%s\n", t);
 			break;
 		case sizeof("Node-path"):
 			if (constcmp(t, "Node-"))
@@ -369,10 +378,12 @@ void svndump_read(const char *url, const char *local_ref)
 					begin_revision(local_ref);
 				active_ctx = NODE_CTX;
 				reset_node_ctx(val);
+				strbuf_addf(&rev_ctx.note, "%s\n", t);
 				break;
 			}
 			if (constcmp(t + strlen("Node-"), "kind"))
 				continue;
+			strbuf_addf(&rev_ctx.note, "%s\n", t);
 			if (!strcmp(val, "dir"))
 				node_ctx.type = REPO_MODE_DIR;
 			else if (!strcmp(val, "file"))
@@ -383,6 +394,7 @@ void svndump_read(const char *url, const char *local_ref)
 		case sizeof("Node-action"):
 			if (constcmp(t, "Node-action"))
 				continue;
+			strbuf_addf(&rev_ctx.note, "%s\n", t);
 			if (!strcmp(val, "delete")) {
 				node_ctx.action = NODEACT_DELETE;
 			} else if (!strcmp(val, "add")) {
@@ -401,11 +413,13 @@ void svndump_read(const char *url, const char *local_ref)
 				continue;
 			strbuf_reset(&node_ctx.src);
 			strbuf_addstr(&node_ctx.src, val);
+			strbuf_addf(&rev_ctx.note, "%s\n", t);
 			break;
 		case sizeof("Node-copyfrom-rev"):
 			if (constcmp(t, "Node-copyfrom-rev"))
 				continue;
 			node_ctx.srcRev = atoi(val);
+			strbuf_addf(&rev_ctx.note, "%s\n", t);
 			break;
 		case sizeof("Text-content-length"):
 			if (constcmp(t, "Text") && constcmp(t, "Prop"))
@@ -475,6 +489,7 @@ static void init(int report_fd)
 	strbuf_init(&dump_ctx.url, 4096);
 	strbuf_init(&rev_ctx.log, 4096);
 	strbuf_init(&rev_ctx.author, 4096);
+	strbuf_init(&rev_ctx.note, 4096);
 	strbuf_init(&node_ctx.src, 4096);
 	strbuf_init(&node_ctx.dst, 4096);
 	reset_dump_ctx(NULL);
@@ -506,6 +521,8 @@ void svndump_deinit(void)
 	reset_rev_ctx(0);
 	reset_node_ctx(NULL);
 	strbuf_release(&rev_ctx.log);
+	strbuf_release(&rev_ctx.author);
+	strbuf_release(&rev_ctx.note);
 	strbuf_release(&node_ctx.src);
 	strbuf_release(&node_ctx.dst);
 	if (buffer_deinit(&input))
-- 
1.7.9.5

[PATCH/RFC v3 11/16] When debug==1, start fast-import with "--stats" instead of "--quiet".

[Florian Achleitner]

fast-import prints statistics that could be interesting to the
developer of remote helpers.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 transport-helper.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/transport-helper.c b/transport-helper.c
index 257274b..7fb52d4 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -386,7 +386,7 @@ static int get_importer(struct transport *transport, struct child_process *fasti
 	memset(fastimport, 0, sizeof(*fastimport));
 	fastimport->in = helper->out;
 	argv_array_push(&argv, "fast-import");
-	argv_array_push(&argv, "--quiet");
+	argv_array_push(&argv, debug ? "--stats" : "--quiet");
 
 	if (data->bidi_import) {
 		cat_blob_fd = xdup(helper->in);
-- 
1.7.9.5

[PATCH/RFC v3 12/16] remote-svn: add incremental import.

[Florian Achleitner]

Search for a note attached to the ref to update and read it's
'Revision-number:'-line. Start import from the next svn revision.

If there is no next revision in the svn repo, svnrdump terminates
with a message on stderr an non-zero return value. This looks a
little weird, but there is no other way to know whether there is
a new revision in the svn repo.

On the start of an incremental import, the parent of the first commit
in the fast-import stream is set to the branch name to update. All
following commits specify their parent by a mark number. Previous
mark files are currently not reused.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 contrib/svn-fe/remote-svn.c |   66 +++++++++++++++++++++++++++++++++++++++++--
 contrib/svn-fe/svn-fe.c     |    3 +-
 test-svn-fe.c               |    2 +-
 vcs-svn/fast_export.c       |   16 ++++++++---
 vcs-svn/fast_export.h       |    6 ++--
 vcs-svn/svndump.c           |   12 ++++----
 vcs-svn/svndump.h           |    2 +-
 7 files changed, 89 insertions(+), 18 deletions(-)

diff --git a/contrib/svn-fe/remote-svn.c b/contrib/svn-fe/remote-svn.c
index df1babc..d659a0e 100644
--- a/contrib/svn-fe/remote-svn.c
+++ b/contrib/svn-fe/remote-svn.c
@@ -13,6 +13,8 @@ static const char *url;
 static int dump_from_file;
 static const char *private_ref;
 static const char *remote_ref = "refs/heads/master";
+static const char *notes_ref;
+struct rev_note { unsigned int rev_nr; };
 
 static int cmd_capabilities(const char *line);
 static int cmd_import(const char *line);
@@ -47,14 +49,70 @@ static void terminate_batch(void)
 		fflush(stdout);
 }
 
+/* NOTE: 'ref' refers to a git reference, while 'rev' refers to a svn revision. */
+static char *read_ref_note(const unsigned char sha1[20]) {
+	const unsigned char *note_sha1;
+	char *msg = NULL;
+	unsigned long msglen;
+	enum object_type type;
+	init_notes(NULL, notes_ref, NULL, 0);
+	if(	(note_sha1 = get_note(NULL, sha1)) == NULL ||
+			!(msg = read_sha1_file(note_sha1, &type, &msglen)) ||
+			!msglen || type != OBJ_BLOB) {
+		free(msg);
+		return NULL;
+	}
+	free_notes(NULL);
+	return msg;
+}
+
+static int parse_rev_note(const char *msg, struct rev_note *res) {
+	const char *key, *value, *end;
+	size_t len;
+	while(*msg) {
+		end = strchr(msg, '\n');
+		len = end ? end - msg : strlen(msg);
+
+		key = "Revision-number: ";
+		if(!prefixcmp(msg, key)) {
+			long i;
+			value = msg + strlen(key);
+			i = atol(value);
+			if(i < 0 || i > UINT32_MAX)
+				return 1;
+			res->rev_nr = i;
+		}
+		msg += len + 1;
+	}
+	return 0;
+}
+
 static int cmd_import(const char *line)
 {
 	int code;
 	int dumpin_fd;
-	unsigned int startrev = 0;
+	char *note_msg;
+	unsigned char head_sha1[20];
+	unsigned int startrev;
 	struct argv_array svndump_argv = ARGV_ARRAY_INIT;
 	struct child_process svndump_proc;
 
+	if(read_ref(private_ref, head_sha1))
+		startrev = 0;
+	else {
+		note_msg = read_ref_note(head_sha1);
+		if(note_msg == NULL) {
+			warning("No note found for %s.", private_ref);
+			startrev = 0;
+		}
+		else {
+			struct rev_note note = { 0 };
+			parse_rev_note(note_msg, &note);
+			startrev = note.rev_nr + 1;
+			free(note_msg);
+		}
+	}
+
 	if(dump_from_file) {
 		dumpin_fd = open(url, O_RDONLY);
 		if(dumpin_fd < 0) {
@@ -77,7 +135,7 @@ static int cmd_import(const char *line)
 
 	}
 	svndump_init_fd(dumpin_fd, STDIN_FILENO);
-	svndump_read(url, private_ref);
+	svndump_read(url, private_ref, notes_ref);
 	svndump_deinit();
 	svndump_reset();
 
@@ -177,6 +235,9 @@ int main(int argc, const char **argv)
 	strbuf_addf(&buf, "refs/svn/%s/master", remote->name);
 	private_ref = strbuf_detach(&buf, NULL);
 
+	strbuf_addf(&buf, "refs/notes/%s/revs", remote->name);
+	notes_ref = strbuf_detach(&buf, NULL);
+
 	while(1) {
 		if (strbuf_getline(&buf, stdin, '\n') == EOF) {
 			if (ferror(stdin))
@@ -192,5 +253,6 @@ int main(int argc, const char **argv)
 	strbuf_release(&buf);
 	free((void*)url);
 	free((void*)private_ref);
+	free((void*)notes_ref);
 	return 0;
 }
diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index c796cc0..f363505 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -10,7 +10,8 @@ int main(int argc, char **argv)
 {
 	if (svndump_init(NULL))
 		return 1;
-	svndump_read((argc > 1) ? argv[1] : NULL, "refs/heads/master");
+	svndump_read((argc > 1) ? argv[1] : NULL, "refs/heads/master",
+			"refs/notes/svn/revs");
 	svndump_deinit();
 	svndump_reset();
 	return 0;
diff --git a/test-svn-fe.c b/test-svn-fe.c
index cb0d80f..0f2d9a4 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -40,7 +40,7 @@ int main(int argc, char *argv[])
 	if (argc == 2) {
 		if (svndump_init(argv[1]))
 			return 1;
-		svndump_read(NULL, "refs/heads/master");
+		svndump_read(NULL, "refs/heads/master", "refs/notes/svn/revs");
 		svndump_deinit();
 		svndump_reset();
 		return 0;
diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 796dd1a..32f71a1 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -70,14 +70,20 @@ void fast_export_modify(const char *path, uint32_t mode, const char *dataref)
 }
 
 void fast_export_begin_note(uint32_t revision, const char *author,
-		const char *log, unsigned long timestamp)
+		const char *log, unsigned long timestamp, const char *note_ref)
 {
+	static int firstnote = 1;
 	timestamp = 1341914616;
 	size_t loglen = strlen(log);
-	printf("commit refs/notes/svn/revs\n");
+	printf("commit %s\n", note_ref);
 	printf("committer %s <%s@%s> %ld +0000\n", author, author, "local", timestamp);
 	printf("data %"PRIuMAX"\n", loglen);
 	fwrite(log, loglen, 1, stdout);
+	if (firstnote) {
+		if (revision > 1)
+			printf("from %s^0", note_ref);
+		firstnote = 0;
+	}
 	fputc('\n', stdout);
 }
 
@@ -90,7 +96,7 @@ static char gitsvnline[MAX_GITSVN_LINE_LEN];
 void fast_export_begin_commit(uint32_t revision, const char *author,
 			const struct strbuf *log,
 			const char *uuid, const char *url,
-			unsigned long timestamp, const char *local_ref)
+			unsigned long timestamp, const char *local_ref, const char *parent)
 {
 	static const struct strbuf empty = STRBUF_INIT;
 	if (!log)
@@ -113,7 +119,9 @@ void fast_export_begin_commit(uint32_t revision, const char *author,
 	fwrite(log->buf, log->len, 1, stdout);
 	printf("%s\n", gitsvnline);
 	if (!first_commit_done) {
-		if (revision > 1)
+		if(parent)
+			printf("from %s^0\n", parent);
+		else if (revision > 1)
 			printf("from :%"PRIu32"\n", revision - 1);
 		first_commit_done = 1;
 	}
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index c2f6f11..5174aae 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -11,10 +11,10 @@ void fast_export_delete(const char *path);
 void fast_export_modify(const char *path, uint32_t mode, const char *dataref);
 void fast_export_note(const char *committish, const char *dataref);
 void fast_export_begin_note(uint32_t revision, const char *author,
-		const char *log, unsigned long timestamp);
+		const char *log, unsigned long timestamp, const char *note_ref);
 void fast_export_begin_commit(uint32_t revision, const char *author,
-			const struct strbuf *log, const char *uuid,
-			const char *url, unsigned long timestamp, const char *local_ref);
+			const struct strbuf *log, const char *uuid,const char *url,
+			unsigned long timestamp, const char *local_ref, const char *parent);
 void fast_export_end_commit(uint32_t revision);
 void fast_export_data(uint32_t mode, off_t len, struct line_buffer *input);
 void fast_export_buf_to_data(const struct strbuf *data);
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index cd65b51..e567e67 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -306,23 +306,23 @@ static void begin_revision(const char *remote_ref)
 		return;
 	fast_export_begin_commit(rev_ctx.revision, rev_ctx.author.buf,
 		&rev_ctx.log, dump_ctx.uuid.buf, dump_ctx.url.buf,
-		rev_ctx.timestamp, remote_ref);
+		rev_ctx.timestamp, remote_ref, rev_ctx.revision == 1 ? NULL : remote_ref);
 }
 
-static void end_revision()
+static void end_revision(const char *note_ref)
 {
 	struct strbuf mark = STRBUF_INIT;
 	if (rev_ctx.revision) {
 		fast_export_end_commit(rev_ctx.revision);
 		fast_export_begin_note(rev_ctx.revision, "remote-svn",
-				"Note created by remote-svn.", rev_ctx.timestamp);
+				"Note created by remote-svn.", rev_ctx.timestamp, note_ref);
 		strbuf_addf(&mark, ":%"PRIu32, rev_ctx.revision);
 		fast_export_note(mark.buf, "inline");
 		fast_export_buf_to_data(&rev_ctx.note);
 	}
 }
 
-void svndump_read(const char *url, const char *local_ref)
+void svndump_read(const char *url, const char *local_ref, const char *notes_ref)
 {
 	char *val;
 	char *t;
@@ -363,7 +363,7 @@ void svndump_read(const char *url, const char *local_ref)
 			if (active_ctx == REV_CTX)
 				begin_revision(local_ref);
 			if (active_ctx != DUMP_CTX)
-				end_revision();
+				end_revision(notes_ref);
 			active_ctx = REV_CTX;
 			reset_rev_ctx(atoi(val));
 			strbuf_addf(&rev_ctx.note, "%s\n", t);
@@ -479,7 +479,7 @@ void svndump_read(const char *url, const char *local_ref)
 	if (active_ctx == REV_CTX)
 		begin_revision(local_ref);
 	if (active_ctx != DUMP_CTX)
-		end_revision();
+		end_revision(notes_ref);
 }
 
 static void init(int report_fd)
diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h
index febeecb..b8eb129 100644
--- a/vcs-svn/svndump.h
+++ b/vcs-svn/svndump.h
@@ -3,7 +3,7 @@
 
 int svndump_init(const char *filename);
 int svndump_init_fd(int in_fd, int back_fd);
-void svndump_read(const char *url, const char *local_ref);
+void svndump_read(const char *url, const char *local_ref, const char *notes_ref);
 void svndump_deinit(void);
 void svndump_reset(void);
 
-- 
1.7.9.5

[PATCH/RFC v3 13/16] Add a svnrdump-simulator replaying a dump file for testing.

[Florian Achleitner]

To ease testing without depending on a reachable svn server, this
compact python script mimics parts of svnrdumps behaviour.
It requires the remote url to start with sim://.
Start and end revisions are evaluated.
If the requested revision doesn't exist, as it is the case with
incremental imports, if no new commit was added, it returns 1
(like svnrdump).
To allow using the same dump file for simulating multiple
incremental imports the highest revision can be limited by setting
the environment variable SVNRMAX to that value. This simulates the
situation where higher revs don't exist yet.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 contrib/svn-fe/svnrdump_sim.py |   53 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100755 contrib/svn-fe/svnrdump_sim.py

diff --git a/contrib/svn-fe/svnrdump_sim.py b/contrib/svn-fe/svnrdump_sim.py
new file mode 100755
index 0000000..ab4ccf1
--- /dev/null
+++ b/contrib/svn-fe/svnrdump_sim.py
@@ -0,0 +1,53 @@
+#!/usr/bin/python
+"""
+Simulates svnrdump by replaying an existing dump from a file, taking care
+of the specified revision range.
+To simulate incremental imports the environment variable SVNRMAX can be set
+to the highest revision that should be available.
+"""
+import sys, os
+
+
+def getrevlimit():
+	var = 'SVNRMAX'
+	if os.environ.has_key(var):
+		return os.environ[var]
+	return None
+	
+def writedump(url, lower, upper):
+	if url.startswith('sim://'):
+		filename = url[6:]
+		if filename[-1] == '/': filename = filename[:-1] #remove terminating slash
+	else:
+		raise ValueError('sim:// url required')
+	f = open(filename, 'r');
+	state = 'header'
+	wroterev = False
+	while(True):
+		l = f.readline()
+		if l == '': break
+		if state == 'header' and l.startswith('Revision-number: '):
+			state = 'prefix'
+		if state == 'prefix' and l == 'Revision-number: %s\n' % lower:
+			state = 'selection'
+		if not upper == 'HEAD' and state == 'selection' and l == 'Revision-number: %s\n' % upper:
+			break;
+
+		if state == 'header' or state == 'selection':
+			if state == 'selection': wroterev = True
+			sys.stdout.write(l)
+	return wroterev
+
+if __name__ == "__main__":
+	if not (len(sys.argv) in (3, 4, 5)):
+		print "usage: %s dump URL -rLOWER:UPPER"
+		sys.exit(1)
+	if not sys.argv[1] == 'dump': raise NotImplementedError('only "dump" is suppported.')
+	url = sys.argv[2]
+	r = ('0', 'HEAD')
+	if len(sys.argv) == 4 and sys.argv[3][0:2] == '-r':
+		r = sys.argv[3][2:].lstrip().split(':')
+	if not getrevlimit() is None: r[1] = getrevlimit()
+	if writedump(url, r[0], r[1]): ret = 0
+	else: ret = 1
+	sys.exit(ret)
-- 
1.7.9.5

[PATCH/RFC v3 14/16] transport-helper: add import|export-marks to fast-import command line.

[Florian Achleitner]

fast-import internally uses marks that refer to an object via its sha1.
Those marks are created during import to find previously created objects.
At exit the accumulated marks can be exported to a file and reloaded at
startup, so that the previous marks are available.
Add command line options to the fast-import command line to enable this.
The mark files are stored in info/fast-import/marks/<remote-name>.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 transport-helper.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/transport-helper.c b/transport-helper.c
index 7fb52d4..47db055 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -387,6 +387,9 @@ static int get_importer(struct transport *transport, struct child_process *fasti
 	fastimport->in = helper->out;
 	argv_array_push(&argv, "fast-import");
 	argv_array_push(&argv, debug ? "--stats" : "--quiet");
+	argv_array_push(&argv, "--relative-marks");
+	argv_array_pushf(&argv, "--import-marks-if-exists=marks/%s", transport->remote->name);
+	argv_array_pushf(&argv, "--export-marks=marks/%s", transport->remote->name);
 
 	if (data->bidi_import) {
 		cat_blob_fd = xdup(helper->in);
-- 
1.7.9.5

[PATCH/RFC v3 15/16] remote-svn: add marks-file regeneration.

[Florian Achleitner]

fast-import mark files are stored outside the object database and are therefore
not fetched and can be lost somehow else.
marks provide a svn revision --> git sha1 mapping, while the notes that are attached
to each commit when it is imported provide a git sha1 --> svn revision.

If the marks file is not available or not plausible, regenerate it by walking through
the notes tree.
, i.e.
The plausibility check tests if the highest revision in the marks file matches the
revision of the top ref. It doesn't ensure that the mark file is completely correct.
This could only be done with an effort equal to unconditional regeneration.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 contrib/svn-fe/remote-svn.c |   69 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)

diff --git a/contrib/svn-fe/remote-svn.c b/contrib/svn-fe/remote-svn.c
index d659a0e..94e5196 100644
--- a/contrib/svn-fe/remote-svn.c
+++ b/contrib/svn-fe/remote-svn.c
@@ -13,7 +13,7 @@ static const char *url;
 static int dump_from_file;
 static const char *private_ref;
 static const char *remote_ref = "refs/heads/master";
-static const char *notes_ref;
+static const char *notes_ref, *marksfilename;
 struct rev_note { unsigned int rev_nr; };
 
 static int cmd_capabilities(const char *line);
@@ -87,6 +87,68 @@ static int parse_rev_note(const char *msg, struct rev_note *res) {
 	return 0;
 }
 
+static int note2mark_cb(const unsigned char *object_sha1,
+		const unsigned char *note_sha1, char *note_path,
+		void *cb_data) {
+	FILE *file = (FILE *)cb_data;
+	char *msg;
+	unsigned long msglen;
+	enum object_type type;
+	struct rev_note note;
+	if (!(msg = read_sha1_file(note_sha1, &type, &msglen)) ||
+			!msglen || type != OBJ_BLOB) {
+		free(msg);
+		return 1;
+	}
+	if (parse_rev_note(msg, &note))
+		return 2;
+	if (fprintf(file, ":%d %s\n", note.rev_nr, sha1_to_hex(object_sha1)) < 1)
+		return 3;
+	return 0;
+}
+
+static void regenerate_marks() {
+	int ret;
+	FILE *marksfile;
+	marksfile = fopen(marksfilename, "w+");
+	if (!marksfile)
+		die_errno("Couldn't create mark file %s.", marksfilename);
+	ret = for_each_note(NULL, 0, note2mark_cb, marksfile);
+	if (ret)
+		die("Regeneration of marks failed, returned %d.", ret);
+	fclose(marksfile);
+}
+
+static void check_or_regenerate_marks(int latestrev) {
+	FILE *marksfile;
+	char *line = NULL;
+	size_t linelen = 0;
+	struct strbuf sb = STRBUF_INIT;
+	int found = 0;
+
+	if (latestrev < 1)
+		return;
+
+	init_notes(NULL, notes_ref, NULL, 0);
+	marksfile = fopen(marksfilename, "r");
+	if (!marksfile)
+		regenerate_marks(marksfile);
+	else {
+		strbuf_addf(&sb, ":%d ", latestrev);
+		while (getline(&line, &linelen, marksfile) != -1) {
+			if (!prefixcmp(line, sb.buf)) {
+				found++;
+				break;
+			}
+		}
+		fclose(marksfile);
+		if (!found)
+			regenerate_marks();
+	}
+	free_notes(NULL);
+	strbuf_release(&sb);
+}
+
 static int cmd_import(const char *line)
 {
 	int code;
@@ -112,6 +174,7 @@ static int cmd_import(const char *line)
 			free(note_msg);
 		}
 	}
+	check_or_regenerate_marks(startrev - 1);
 
 	if(dump_from_file) {
 		dumpin_fd = open(url, O_RDONLY);
@@ -238,6 +301,9 @@ int main(int argc, const char **argv)
 	strbuf_addf(&buf, "refs/notes/%s/revs", remote->name);
 	notes_ref = strbuf_detach(&buf, NULL);
 
+	strbuf_addf(&buf, "%s/info/fast-import/marks/%s", get_git_dir(), remote->name);
+	marksfilename = strbuf_detach(&buf, NULL);
+
 	while(1) {
 		if (strbuf_getline(&buf, stdin, '\n') == EOF) {
 			if (ferror(stdin))
@@ -254,5 +320,6 @@ int main(int argc, const char **argv)
 	free((void*)url);
 	free((void*)private_ref);
 	free((void*)notes_ref);
+	free((void*)marksfilename);
 	return 0;
 }
-- 
1.7.9.5

[PATCH/RFC v3 16/16] Add a test script for remote-svn.

[Florian Achleitner]

Use svnrdump_sim.py to emulate svnrdump without an svn server.
Tests fetching, incremental fetching, fetching from file://,
and the regeneration of fast-import's marks file.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 t/t9020-remote-svn.sh |   69 +++++++++++++++++++++++++++++++++++++++++++++++++
 transport-helper.c    |   15 ++++++-----
 2 files changed, 77 insertions(+), 7 deletions(-)
 create mode 100755 t/t9020-remote-svn.sh

diff --git a/t/t9020-remote-svn.sh b/t/t9020-remote-svn.sh
new file mode 100755
index 0000000..a0c6a21
--- /dev/null
+++ b/t/t9020-remote-svn.sh
@@ -0,0 +1,69 @@
+#!/bin/sh
+
+test_description='tests remote-svn'
+
+. ./test-lib.sh
+
+# We override svnrdump by placing a symlink to the svnrdump-emulator in .
+export PATH="$HOME:$PATH"
+ln -sf $GIT_BUILD_DIR/contrib/svn-fe/svnrdump_sim.py "$HOME/svnrdump"
+
+init_git () {
+	rm -fr .git &&
+	git init &&
+	#git remote add svnsim svn::sim:///$TEST_DIRECTORY/t9020/example.svnrdump
+	# let's reuse an exisiting dump file!?
+	git remote add svnsim svn::sim:///$TEST_DIRECTORY/t9154/svn.dump
+	git remote add svnfile svn::file:///$TEST_DIRECTORY/t9154/svn.dump
+}
+
+test_debug '
+	git --version
+	which git
+	which svnrdump
+'
+
+test_expect_success 'simple fetch' '
+	init_git &&
+	git fetch svnsim &&
+	test_cmp .git/refs/svn/svnsim/master .git/refs/remotes/svnsim/master  &&
+	cp .git/refs/remotes/svnsim/master master.good
+'
+
+test_debug '
+	cat .git/refs/svn/svnsim/master
+	cat .git/refs/remotes/svnsim/master
+'
+
+test_expect_success 'repeated fetch, nothing shall change' '
+	git fetch svnsim &&
+	test_cmp master.good .git/refs/remotes/svnsim/master
+'
+
+test_expect_success 'fetch from a file:// url gives the same result' '
+	git fetch svnfile 
+'
+
+test_expect_failure 'the sha1 differ because the git-svn-id line in the commit msg contains the url' '
+	test_cmp .git/refs/remotes/svnfile/master .git/refs/remotes/svnsim/master
+'
+
+test_expect_success 'mark-file regeneration' '
+	mv .git/info/fast-import/marks/svnsim .git/info/fast-import/marks/svnsim.old &&
+	git fetch svnsim &&
+	test_cmp .git/info/fast-import/marks/svnsim.old .git/info/fast-import/marks/svnsim
+'
+
+test_expect_success 'incremental imports must lead to the same head' '
+	export SVNRMAX=3 &&
+	init_git &&
+	git fetch svnsim &&
+	test_cmp .git/refs/svn/svnsim/master .git/refs/remotes/svnsim/master  &&
+	unset SVNRMAX &&
+	git fetch svnsim &&
+	test_cmp master.good .git/refs/remotes/svnsim/master
+'
+
+test_debug 'git branch -a'	
+
+test_done
diff --git a/transport-helper.c b/transport-helper.c
index 47db055..a363f2c 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -17,6 +17,7 @@ static int debug;
 struct helper_data {
 	const char *name;
 	struct child_process *helper;
+	struct argv_array argv;
 	FILE *out;
 	unsigned fetch : 1,
 		import : 1,
@@ -103,7 +104,6 @@ static void do_take_over(struct transport *transport)
 static struct child_process *get_helper(struct transport *transport)
 {
 	struct helper_data *data = transport->data;
-	struct argv_array argv = ARGV_ARRAY_INIT;
 	struct strbuf buf = STRBUF_INIT;
 	struct child_process *helper;
 	const char **refspecs = NULL;
@@ -125,10 +125,11 @@ static struct child_process *get_helper(struct transport *transport)
 	helper->in = -1;
 	helper->out = -1;
 	helper->err = 0;
-	argv_array_pushf(&argv, "git-remote-%s", data->name);
-	argv_array_push(&argv, transport->remote->name);
-	argv_array_push(&argv, remove_ext_force(transport->url));
-	helper->argv = argv.argv;
+	argv_array_init(&data->argv);
+	argv_array_pushf(&data->argv, "git-remote-%s", data->name);
+	argv_array_push(&data->argv, transport->remote->name);
+	argv_array_push(&data->argv, remove_ext_force(transport->url));
+	helper->argv = data->argv.argv;
 	helper->git_cmd = 0;
 	helper->silent_exec_failure = 1;
 
@@ -143,8 +144,6 @@ static struct child_process *get_helper(struct transport *transport)
 
 	data->helper = helper;
 	data->no_disconnect_req = 0;
-	free((void*) helper_env[1]);
-	argv_array_clear(&argv);
 
 	/*
 	 * Open the output as FILE* so strbuf_getline() can be used.
@@ -247,6 +246,8 @@ static int disconnect_helper(struct transport *transport)
 		close(data->helper->out);
 		fclose(data->out);
 		res = finish_command(data->helper);
+		free((void*) data->helper->env[1]);
+		argv_array_clear(&data->argv);
 		free(data->helper);
 		data->helper = NULL;
 	}
-- 
1.7.9.5

Re: [PATCH/RFC v3 01/16] Implement a remote helper for svn in C.

[Junio C Hamano]

Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:

> Enable basic fetching from subversion repositories. When processing remote URLs
> starting with svn::, git invokes this remote-helper.
> It starts svnrdump to extract revisions from the subversion repository in the
> 'dump file format', and converts them to a git-fast-import stream using
> the functions of vcs-svn/.

(nit) the above is a bit too wide, isn't it?

> Imported refs are created in a private namespace at refs/svn/<remote-name/master.

(nit) missing closing '>'?

> The revision history is imported linearly (no branch detection) and completely,
> i.e. from revision 0 to HEAD.
>
> The 'bidi-import' capability is used. The remote-helper expects data from
> fast-import on its stdin. It buffers a batch of 'import' command lines
> in a string_list before starting to process them.
>
> Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> ---
> diff:
> - incorporate review
> - remove redundant strbuf_init
> - add 'bidi-import' to capabilities
> - buffer all lines of a command batch in string_list
>
>  contrib/svn-fe/remote-svn.c |  183 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 183 insertions(+)
>  create mode 100644 contrib/svn-fe/remote-svn.c
>
> diff --git a/contrib/svn-fe/remote-svn.c b/contrib/svn-fe/remote-svn.c
> new file mode 100644
> index 0000000..ce59344
> --- /dev/null
> +++ b/contrib/svn-fe/remote-svn.c
> @@ -0,0 +1,183 @@
> +

Remove.

> +#include "cache.h"
> +#include "remote.h"
> +#include "strbuf.h"
> +#include "url.h"
> +#include "exec_cmd.h"
> +#include "run-command.h"
> +#include "svndump.h"
> +#include "notes.h"
> +#include "argv-array.h"
> +
> +static const char *url;
> +static const char *private_ref;
> +static const char *remote_ref = "refs/heads/master";

Just wondering; is this name "master" (or "refs/heads/" for that
matter) significant in any way when talking to a subversion remote?

> +static int cmd_capabilities(const char *line);
> +static int cmd_import(const char *line);
> +static int cmd_list(const char *line);
> +
> +typedef int (*input_command_handler)(const char *);
> +struct input_command_entry {
> +	const char *name;
> +	input_command_handler fct;
> +	unsigned char batchable;	/* whether the command starts or is part of a batch */
> +};
> +
> +static const struct input_command_entry input_command_list[] = {
> +		{ "capabilities", cmd_capabilities, 0 },

One level too deeply indented?

> +		{ "import", cmd_import, 1 },
> +		{ "list", cmd_list, 0 },
> +		{ NULL, NULL }
> +};
> +
> +static int cmd_capabilities(const char *line) {
> +	printf("import\n");
> +	printf("bidi-import\n");
> +	printf("refspec %s:%s\n\n", remote_ref, private_ref);
> +	fflush(stdout);
> +	return 0;
> +}
> +
> +static void terminate_batch(void)
> +{
> +	/* terminate a current batch's fast-import stream */
> +		printf("done\n");

Likewise.

> +		fflush(stdout);
> +}
> +
> +static int cmd_import(const char *line)
> +{
> +	int code;
> +	int dumpin_fd;
> +	unsigned int startrev = 0;
> +	struct argv_array svndump_argv = ARGV_ARRAY_INIT;
> +	struct child_process svndump_proc;
> +
> +	memset(&svndump_proc, 0, sizeof (struct child_process));

Please lose SP between sizeof and '('.

> +	svndump_proc.out = -1;
> +	argv_array_push(&svndump_argv, "svnrdump");
> +	argv_array_push(&svndump_argv, "dump");
> +	argv_array_push(&svndump_argv, url);
> +	argv_array_pushf(&svndump_argv, "-r%u:HEAD", startrev);
> +	svndump_proc.argv = svndump_argv.argv;

(just me making a mental note) We read from "svnrdump", which would
read (if it ever does) from the same stdin as ours and spits (if it
ever does) its errors to the same stderr as ours.

> +
> +	code = start_command(&svndump_proc);
> +	if (code)
> +		die("Unable to start %s, code %d", svndump_proc.argv[0], code);
> +	dumpin_fd = svndump_proc.out;
> +
> +	code = start_command(&svndump_proc);
> +	if (code)
> +		die("Unable to start %s, code %d", svndump_proc.argv[0], code);
> +	dumpin_fd = svndump_proc.out;

You start it twice without finishing the first invocation, or just a
double paste?

> +	svndump_init_fd(dumpin_fd, STDIN_FILENO);
> +	svndump_read(url, private_ref);
> +	svndump_deinit();
> +	svndump_reset();
> +
> +	close(dumpin_fd);

(my mental note) And at this point, we finished feeding whatever
comes out of "svnrdump" to svndump_read().

> +	code = finish_command(&svndump_proc);
> +	if (code)
> +		warning("%s, returned %d", svndump_proc.argv[0], code);
> +	argv_array_clear(&svndump_argv);
> +
> +	return 0;

Other than the "twice?" puzzle, this function looks straightforward.

> +}
> +
> +static int cmd_list(const char *line)
> +{
> +	printf("? %s\n\n", remote_ref);
> +	fflush(stdout);
> +	return 0;
> +}
> +
> +static int do_command(struct strbuf *line)
> +{
> +	const struct input_command_entry *p = input_command_list;
> +	static struct string_list batchlines = STRING_LIST_INIT_DUP;
> +	static const struct input_command_entry *batch_cmd;
> +	/*
> +	 * commands can be grouped together in a batch.
> +	 * Batches are ended by \n. If no batch is active the program ends.
> +	 * During a batch all lines are buffered and passed to the handler function
> +	 * when the batch is terminated.
> +	 */
> +	if (line->len == 0) {
> +		if (batch_cmd) {
> +			struct string_list_item *item;
> +			for_each_string_list_item(item, &batchlines)
> +				batch_cmd->fct(item->string);

(style) I think we tend to call these unnamed functions "fn" in our
codebase.

> +			terminate_batch();
> +			batch_cmd = NULL;
> +			string_list_clear(&batchlines, 0);
> +			return 0;	/* end of the batch, continue reading other commands. */
> +		}
> +		return 1;	/* end of command stream, quit */
> +	}
> +	if (batch_cmd) {
> +		if (strcmp(batch_cmd->name, line->buf))
> +			die("Active %s batch interrupted by %s", batch_cmd->name, line->buf);
> +		/* buffer batch lines */
> +		string_list_append(&batchlines, line->buf);
> +		return 0;
> +	}

A "batch-able" command, e.g. "import", will first cause the
batch_cmd to point at the command structure in this function, and
then the next and subsequent lines, as long as the input line is
exactly the same as the current batch_cmd->name, e.g. "import", is
appended into batchlines.

Would this mean that you can feed something like this:

	import foobar
        import
        import
        import

        another command

and buffer the four "import" lines in batchlines, and then on the
empty line, have the for-each-string-list-item loop to call
cmd_import() on "import foobar", "import", "import", then "import"
(literally, without anything other than "import" on the line).

How is that useful?  With that "if (strcmp(batch_cmd->name, line->buf))",
I cannot think of other valid input to make this "batch" mechanism
to trigger and do something useful.  Am I missing something?

> +
> +	for(p = input_command_list; p->name; p++) {

Have a SP between for and '('.

> +		if (!prefixcmp(line->buf, p->name) &&
> +				(strlen(p->name) == line->len || line->buf[strlen(p->name)] == ' ')) {

A line way too wide.

> +			if (p->batchable) {
> +				batch_cmd = p;
> +				string_list_append(&batchlines, line->buf);
> +				return 0;
> +			}
> +			return p->fct(line->buf);
> +		}

OK, so a command word on a line by itself, or a command word
followed by a SP (probably followed by its arguments) on a line
triggers a command lookup, and individual command implementation
parses the line.

> +	}
> +	warning("Unknown command '%s'\n", line->buf);
> +	return 0;

Why isn't this an error?

> +}
> +
> +int main(int argc, const char **argv)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	int nongit;
> +	static struct remote *remote;
> +	const char *url_in;
> +
> +	git_extract_argv0_path(argv[0]);
> +	setup_git_directory_gently(&nongit);
> +	if (argc < 2 || argc > 3) {
> +		usage("git-remote-svn <remote-name> [<url>]");
> +		return 1;
> +	}

If this is an importer, you would be importing _into_ a git
repository, no?  How can you not error out when you are not in one?
In other words, why &nongit with *_gently()?

> +	remote = remote_get(argv[1]);
> +	url_in = remote->url[0];
> +	if (argc == 3)
> +		url_in = argv[2];

Shouldn't it be more like this?

	url_in = (argc == 3) ? argv[2] : remote->url[0];

> +	end_url_with_slash(&buf, url_in);
> +	url = strbuf_detach(&buf, NULL);
> +
> +	strbuf_addf(&buf, "refs/svn/%s/master", remote->name);
> +	private_ref = strbuf_detach(&buf, NULL);
> +
> +	while(1) {
> +		if (strbuf_getline(&buf, stdin, '\n') == EOF) {
> +			if (ferror(stdin))
> +				die("Error reading command stream");
> +			else
> +				die("Unexpected end of command stream");
> +		}
> +		if (do_command(&buf))
> +			break;
> +		strbuf_reset(&buf);
> +	}
> +
> +	strbuf_release(&buf);
> +	free((void*)url);
> +	free((void*)private_ref);
> +	return 0;
> +}

Re: [PATCH/RFC v3 02/16] Integrate remote-svn into svn-fe/Makefile.

[Junio C Hamano]

Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:

> Requires some sha.h to be used and the libraries
> to be linked, this is currently hardcoded.
>
> Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> ---
>  contrib/svn-fe/Makefile |   16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/svn-fe/Makefile b/contrib/svn-fe/Makefile
> index 360d8da..8f0eec2 100644
> --- a/contrib/svn-fe/Makefile
> +++ b/contrib/svn-fe/Makefile
> @@ -1,14 +1,14 @@
> -all:: svn-fe$X
> +all:: svn-fe$X remote-svn$X
>  
>  CC = gcc
>  RM = rm -f
>  MV = mv
>  
> -CFLAGS = -g -O2 -Wall
> +CFLAGS = -g -O2 -Wall -DSHA1_HEADER='<openssl/sha.h>' -Wdeclaration-after-statement
>  LDFLAGS =
>  ALL_CFLAGS = $(CFLAGS)
>  ALL_LDFLAGS = $(LDFLAGS)
> -EXTLIBS =
> +EXTLIBS = -lssl -lcrypto -lpthread ../../xdiff/lib.a

I haven't looked carefully, but didn't we have to do a bit more
elaborate when linking with ssl/crypto in our main Makefile to be
portable across various vintages of OpenSSL libraries?

Does contrib/svn-fe/ already depend on OpenSSL by the way?  It needs
to be documented somewhere in the same directory.

If one builds the main Git binary with NO_OPENSSL, can this still be
built and linked?

What does this use xdiff/lib.a for?

The above are just mental notes; I didn't read the later patches in
the series that may already address these issues.

>  GIT_LIB = ../../libgit.a
>  VCSSVN_LIB = ../../vcs-svn/lib.a
> @@ -37,8 +37,12 @@ svn-fe$X: svn-fe.o $(VCSSVN_LIB) $(GIT_LIB)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ svn-fe.o \
>  		$(ALL_LDFLAGS) $(LIBS)
>  
> -svn-fe.o: svn-fe.c ../../vcs-svn/svndump.h
> -	$(QUIET_CC)$(CC) -I../../vcs-svn -o $*.o -c $(ALL_CFLAGS) $<
> +remote-svn$X: remote-svn.o $(VCSSVN_LIB) $(GIT_LIB)
> +	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ remote-svn.o \
> +		$(ALL_LDFLAGS) $(LIBS)
> +		
> +%.o: %.c ../../vcs-svn/svndump.h
> +	$(QUIET_CC)$(CC) -I../../vcs-svn -I../../ -o $*.o -c $(ALL_CFLAGS) $<
>  
>  svn-fe.html: svn-fe.txt
>  	$(QUIET_SUBDIR0)../../Documentation $(QUIET_SUBDIR1) \
> @@ -58,6 +62,6 @@ svn-fe.1: svn-fe.txt
>  	$(QUIET_SUBDIR0)../.. $(QUIET_SUBDIR1) libgit.a
>  
>  clean:
> -	$(RM) svn-fe$X svn-fe.o svn-fe.html svn-fe.xml svn-fe.1
> +	$(RM) svn-fe$X svn-fe.o svn-fe.html svn-fe.xml svn-fe.1 remote-svn.o
>  
>  .PHONY: all clean FORCE

Re: [PATCH/RFC v3 03/16] Add svndump_init_fd to allow reading dumps from arbitrary FDs.

[Junio C Hamano]

Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:

> The existing function only allows reading from a filename or
> from stdin. Allow passing of a FD and an additional FD for
> the back report pipe. This allows us to retrieve the name of
> the pipe in the caller.
>
> Fixes the filename could be NULL bug.

What bug?  Was this line meant to go in the log message?

> Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> ---
> - dup input file descriptor, because buffer_deinit closes the fd.
>  vcs-svn/svndump.c |   22 ++++++++++++++++++----
>  vcs-svn/svndump.h |    1 +
>  2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
> index 2b168ae..d81a078 100644
> --- a/vcs-svn/svndump.c
> +++ b/vcs-svn/svndump.c
> @@ -468,11 +468,9 @@ void svndump_read(const char *url)
>  		end_revision();
>  }
>  
> -int svndump_init(const char *filename)
> +static void init(int report_fd)
>  {
> -	if (buffer_init(&input, filename))
> -		return error("cannot open %s: %s", filename, strerror(errno));
> -	fast_export_init(REPORT_FILENO);
> +	fast_export_init(report_fd);
>  	strbuf_init(&dump_ctx.uuid, 4096);
>  	strbuf_init(&dump_ctx.url, 4096);
>  	strbuf_init(&rev_ctx.log, 4096);
> @@ -482,6 +480,22 @@ int svndump_init(const char *filename)
>  	reset_dump_ctx(NULL);
>  	reset_rev_ctx(0);
>  	reset_node_ctx(NULL);
> +	return;
> +}
> +
> +int svndump_init(const char *filename)
> +{
> +	if (buffer_init(&input, filename))
> +		return error("cannot open %s: %s", filename ? filename : "NULL", strerror(errno));
> +	init(REPORT_FILENO);
> +	return 0;
> +}
> +
> +int svndump_init_fd(int in_fd, int back_fd)
> +{
> +	if(buffer_fdinit(&input, xdup(in_fd)))
> +		return error("cannot open fd %d: %s", in_fd, strerror(errno));
> +	init(xdup(back_fd));
>  	return 0;
>  }
>  
> diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h
> index df9ceb0..acb5b47 100644
> --- a/vcs-svn/svndump.h
> +++ b/vcs-svn/svndump.h
> @@ -2,6 +2,7 @@
>  #define SVNDUMP_H_
>  
>  int svndump_init(const char *filename);
> +int svndump_init_fd(int in_fd, int back_fd);
>  void svndump_read(const char *url);
>  void svndump_deinit(void);
>  void svndump_reset(void);

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

[Junio C Hamano]

Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:

> The fast-import commands 'cat-blob' and 'ls' can be used by remote-helpers
> to retrieve information about blobs and trees that already exist in
> fast-import's memory. This requires a channel from fast-import to the
> remote-helper.
> remote-helpers that use this features shall advertise the new 'bidi-import'

s/this fea/these fea/

> capability so signal that they require the communication channel.

s/so sig/to sig/, I think.

> When forking fast-import in transport-helper.c connect it to a dup of
> the remote-helper's stdin-pipe. The additional file descriptor is passed
> to fast-import via it's command line (--cat-blob-fd).

s/via it's/via its/;

> It follows that git and fast-import are connected to the remote-helpers's
> stdin.
> Because git can send multiple commands to the remote-helper on it's stdin,
> it is required that helpers that advertise 'bidi-import' buffer all input
> commands until the batch of 'import' commands is ended by a newline
> before sending data to fast-import.
> This is to prevent mixing commands and fast-import responses on the
> helper's stdin.

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

> Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> ---
>  transport-helper.c |   45 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index cfe0988..257274b 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -10,6 +10,7 @@
>  #include "string-list.h"
>  #include "thread-utils.h"
>  #include "sigchain.h"
> +#include "argv-array.h"
>  
>  static int debug;
>  
> @@ -19,6 +20,7 @@ struct helper_data {
>  	FILE *out;
>  	unsigned fetch : 1,
>  		import : 1,
> +		bidi_import : 1,
>  		export : 1,
>  		option : 1,
>  		push : 1,
> @@ -101,6 +103,7 @@ static void do_take_over(struct transport *transport)
>  static struct child_process *get_helper(struct transport *transport)
>  {
>  	struct helper_data *data = transport->data;
> +	struct argv_array argv = ARGV_ARRAY_INIT;
>  	struct strbuf buf = STRBUF_INIT;
>  	struct child_process *helper;
>  	const char **refspecs = NULL;
> @@ -122,11 +125,10 @@ static struct child_process *get_helper(struct transport *transport)
>  	helper->in = -1;
>  	helper->out = -1;
>  	helper->err = 0;
> -	helper->argv = xcalloc(4, sizeof(*helper->argv));
> -	strbuf_addf(&buf, "git-remote-%s", data->name);
> -	helper->argv[0] = strbuf_detach(&buf, NULL);
> -	helper->argv[1] = transport->remote->name;
> -	helper->argv[2] = remove_ext_force(transport->url);
> +	argv_array_pushf(&argv, "git-remote-%s", data->name);
> +	argv_array_push(&argv, transport->remote->name);
> +	argv_array_push(&argv, remove_ext_force(transport->url));
> +	helper->argv = argv.argv;

Much nicer than before thanks to argv_array ;-)

>  	helper->git_cmd = 0;
>  	helper->silent_exec_failure = 1;
>  
> @@ -141,6 +143,8 @@ static struct child_process *get_helper(struct transport *transport)
>  
>  	data->helper = helper;
>  	data->no_disconnect_req = 0;
> +	free((void*) helper_env[1]);

What is this free() for???

> +	argv_array_clear(&argv);

See below.

>  	/*
>  	 * Open the output as FILE* so strbuf_getline() can be used.
> @@ -178,6 +182,8 @@ static struct child_process *get_helper(struct transport *transport)
>  			data->push = 1;
>  		else if (!strcmp(capname, "import"))
>  			data->import = 1;
> +		else if (!strcmp(capname, "bidi-import"))
> +			data->bidi_import = 1;
>  		else if (!strcmp(capname, "export"))
>  			data->export = 1;
>  		else if (!data->refspecs && !prefixcmp(capname, "refspec ")) {
> @@ -241,8 +247,6 @@ static int disconnect_helper(struct transport *transport)
>  		close(data->helper->out);
>  		fclose(data->out);
>  		res = finish_command(data->helper);
> -		free((char *)data->helper->argv[0]);
> -		free(data->helper->argv);
>  		free(data->helper);
>  		data->helper = NULL;
>  	}
> @@ -376,14 +380,24 @@ static int fetch_with_fetch(struct transport *transport,
>  static int get_importer(struct transport *transport, struct child_process *fastimport)
>  {
>  	struct child_process *helper = get_helper(transport);
> +	struct helper_data *data = transport->data;
> +	struct argv_array argv = ARGV_ARRAY_INIT;
> +	int cat_blob_fd, code;
>  	memset(fastimport, 0, sizeof(*fastimport));
>  	fastimport->in = helper->out;
> -	fastimport->argv = xcalloc(5, sizeof(*fastimport->argv));
> -	fastimport->argv[0] = "fast-import";
> -	fastimport->argv[1] = "--quiet";
> +	argv_array_push(&argv, "fast-import");
> +	argv_array_push(&argv, "--quiet");
>  
> +	if (data->bidi_import) {
> +		cat_blob_fd = xdup(helper->in);
> +		argv_array_pushf(&argv, "--cat-blob-fd=%d", cat_blob_fd);
> +	}
> +	fastimport->argv = argv.argv;
>  	fastimport->git_cmd = 1;
> -	return start_command(fastimport);
> +
> +	code = start_command(fastimport);
> +	argv_array_clear(&argv);
> +	return code;
>  }
>  
>  static int get_exporter(struct transport *transport,
> @@ -438,11 +452,16 @@ static int fetch_with_import(struct transport *transport,
>  	}
>  
>  	write_constant(data->helper->in, "\n");
> +	/*
> +	 * remote-helpers that advertise the bidi-import capability are required to
> +	 * buffer the complete batch of import commands until this newline before
> +	 * sending data to fast-import.
> +	 * These helpers read back data from fast-import on their stdin, which could
> +	 * be mixed with import commands, otherwise.
> +	 */
>  
>  	if (finish_command(&fastimport))
>  		die("Error while running fast-import");
> -	free(fastimport.argv);
> -	fastimport.argv = NULL;

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

	child = get_helper();

        trace("started %s\n", child->argv[0]);

	if (finish_command(child))
        	return error("failed to cleanly finish %s", child->argv[0]);

Re: [PATCH/RFC v3 07/16] Add a symlink 'git-remote-svn' in base dir.

[Junio C Hamano]

Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:

> Allow execution of git-remote-svn even if the binary
> currently is located in contrib/svn-fe/.
>
> Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> ---
>  git-remote-svn |    1 +
>  1 file changed, 1 insertion(+)
>  create mode 120000 git-remote-svn
>
> diff --git a/git-remote-svn b/git-remote-svn
> new file mode 120000
> index 0000000..d3b1c07
> --- /dev/null
> +++ b/git-remote-svn
> @@ -0,0 +1 @@
> +contrib/svn-fe/remote-svn
> \ No newline at end of file

Not paying enough attention to the patch you are sending?

Re: [PATCH/RFC v3 07/16] Add a symlink 'git-remote-svn' in base dir.

[Junio C Hamano]

Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:

> Allow execution of git-remote-svn even if the binary
> currently is located in contrib/svn-fe/.
>
> Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> ---
>  git-remote-svn |    1 +
>  1 file changed, 1 insertion(+)
>  create mode 120000 git-remote-svn
>
> diff --git a/git-remote-svn b/git-remote-svn
> new file mode 120000
> index 0000000..d3b1c07
> --- /dev/null
> +++ b/git-remote-svn
> @@ -0,0 +1 @@
> +contrib/svn-fe/remote-svn
> \ No newline at end of file

Please scratch my previous comment.  I thought you were adding an
entry to .gitignore or something.

I'd rather not to see such a symbolic link that points at a build
product in the source tree.  Making a symlink from the toplevel
Makefile _after_ we built it in contrib/svn-fe/ (and removing it
upon "make clean") is OK, though.

Re: [PATCH/RFC v3 00/16] GSOC remote-svn

[David Michael Barr]

On Wed, Aug 15, 2012 at 5:13 AM, Florian Achleitner
<florian.achleitner.2.6.31@gmail.com> wrote:
> Hi.
>
> Version 3 of this series adds the 'bidi-import' capability, as suggested
> Jonathan.
> Diff details are attached to the patches.
> 04 and 05 are completely new.
>
> [PATCH/RFC v3 01/16] Implement a remote helper for svn in C.
> [PATCH/RFC v3 02/16] Integrate remote-svn into svn-fe/Makefile.
> [PATCH/RFC v3 03/16] Add svndump_init_fd to allow reading dumps from
> [PATCH/RFC v3 04/16] Connect fast-import to the remote-helper via
> [PATCH/RFC v3 05/16] Add documentation for the 'bidi-import'
> [PATCH/RFC v3 06/16] remote-svn, vcs-svn: Enable fetching to private
> [PATCH/RFC v3 07/16] Add a symlink 'git-remote-svn' in base dir.
> [PATCH/RFC v3 08/16] Allow reading svn dumps from files via file://
> [PATCH/RFC v3 09/16] vcs-svn: add fast_export_note to create notes
> [PATCH/RFC v3 10/16] Create a note for every imported commit
> [PATCH/RFC v3 11/16] When debug==1, start fast-import with "--stats"
> [PATCH/RFC v3 12/16] remote-svn: add incremental import.
> [PATCH/RFC v3 13/16] Add a svnrdump-simulator replaying a dump file
> [PATCH/RFC v3 14/16] transport-helper: add import|export-marks to
> [PATCH/RFC v3 15/16] remote-svn: add marks-file regeneration.
> [PATCH/RFC v3 16/16] Add a test script for remote-svn.

Thank you Florian, this series was a great read. My apologies for the
limited interaction over the course of summer. You have done well and
engaged with the community to produce this result.

Thank you Jonathan for the persistent reviews. No doubt they have
contributed to the quality of the series.

Thank you Junio for your dedication to reviewing the traffic on this
mailing list.

I will no longer be reachable on this address after Friday.

I hope to make future contributions with the identity:
David Michael Barr <b@rr-dav.id.au>
This will be my persistent address.

--
David Barr

Re: [PATCH/RFC v3 02/16] Integrate remote-svn into svn-fe/Makefile.

[Florian Achleitner]

On Tuesday 14 August 2012 13:14:12 Junio C Hamano wrote:
> Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:
> > Requires some sha.h to be used and the libraries
> > to be linked, this is currently hardcoded.
> > 
> > Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> > ---
> > 
> >  contrib/svn-fe/Makefile |   16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/contrib/svn-fe/Makefile b/contrib/svn-fe/Makefile
> > index 360d8da..8f0eec2 100644
> > --- a/contrib/svn-fe/Makefile
> > +++ b/contrib/svn-fe/Makefile
> > @@ -1,14 +1,14 @@
> > -all:: svn-fe$X
> > +all:: svn-fe$X remote-svn$X
> > 
> >  CC = gcc
> >  RM = rm -f
> >  MV = mv
> > 
> > -CFLAGS = -g -O2 -Wall
> > +CFLAGS = -g -O2 -Wall -DSHA1_HEADER='<openssl/sha.h>'
> > -Wdeclaration-after-statement> 
> >  LDFLAGS =
> >  ALL_CFLAGS = $(CFLAGS)
> >  ALL_LDFLAGS = $(LDFLAGS)
> > 
> > -EXTLIBS =
> > +EXTLIBS = -lssl -lcrypto -lpthread ../../xdiff/lib.a
> 
> I haven't looked carefully, but didn't we have to do a bit more
> elaborate when linking with ssl/crypto in our main Makefile to be
> portable across various vintages of OpenSSL libraries?
> 
> Does contrib/svn-fe/ already depend on OpenSSL by the way?  It needs
> to be documented somewhere in the same directory.
> 
> If one builds the main Git binary with NO_OPENSSL, can this still be
> built and linked?
> 
> What does this use xdiff/lib.a for?
> 
> The above are just mental notes; I didn't read the later patches in
> the series that may already address these issues.

For the makefile, I've to say that this is just a hack to make it work. I'm not 
sure how it would be correctly integrated into git's makefile hierarchy.
The OPENSSL header and the xdiff/lib.a are here because it doesn't work 
otherwise. I need to dig into that to find out why. Any tips how to do it 
right?
 
> >  GIT_LIB = ../../libgit.a
> >  VCSSVN_LIB = ../../vcs-svn/lib.a
> > 
> > @@ -37,8 +37,12 @@ svn-fe$X: svn-fe.o $(VCSSVN_LIB) $(GIT_LIB)
> > 
> >  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ svn-fe.o \
> >  	
> >  		$(ALL_LDFLAGS) $(LIBS)
> > 
> > -svn-fe.o: svn-fe.c ../../vcs-svn/svndump.h
> > -	$(QUIET_CC)$(CC) -I../../vcs-svn -o $*.o -c $(ALL_CFLAGS) $<
> > +remote-svn$X: remote-svn.o $(VCSSVN_LIB) $(GIT_LIB)
> > +	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ remote-svn.o \
> > +		$(ALL_LDFLAGS) $(LIBS)
> > +
> > +%.o: %.c ../../vcs-svn/svndump.h
> > +	$(QUIET_CC)$(CC) -I../../vcs-svn -I../../ -o $*.o -c $(ALL_CFLAGS) $<
> > 
> >  svn-fe.html: svn-fe.txt
> >  
> >  	$(QUIET_SUBDIR0)../../Documentation $(QUIET_SUBDIR1) \
> > 
> > @@ -58,6 +62,6 @@ svn-fe.1: svn-fe.txt
> > 
> >  	$(QUIET_SUBDIR0)../.. $(QUIET_SUBDIR1) libgit.a
> >  
> >  clean:
> > -	$(RM) svn-fe$X svn-fe.o svn-fe.html svn-fe.xml svn-fe.1
> > +	$(RM) svn-fe$X svn-fe.o svn-fe.html svn-fe.xml svn-fe.1 remote-svn.o
> > 
> >  .PHONY: all clean FORCE

Re: [PATCH/RFC v3 07/16] Add a symlink 'git-remote-svn' in base dir.

[Florian Achleitner]

On Tuesday 14 August 2012 13:46:43 Junio C Hamano wrote:
> Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:
> > Allow execution of git-remote-svn even if the binary
> > currently is located in contrib/svn-fe/.
> > 
> > Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> > ---
> > 
> >  git-remote-svn |    1 +
> >  1 file changed, 1 insertion(+)
> >  create mode 120000 git-remote-svn
> > 
> > diff --git a/git-remote-svn b/git-remote-svn
> > new file mode 120000
> > index 0000000..d3b1c07
> > --- /dev/null
> > +++ b/git-remote-svn
> > @@ -0,0 +1 @@
> > +contrib/svn-fe/remote-svn
> > \ No newline at end of file
> 
> Please scratch my previous comment.  I thought you were adding an
> entry to .gitignore or something.
> 
> I'd rather not to see such a symbolic link that points at a build
> product in the source tree.  Making a symlink from the toplevel
> Makefile _after_ we built it in contrib/svn-fe/ (and removing it
> upon "make clean") is OK, though.

As with the makefile in contrib/svn-fe, this is just a hack. The toplevel 
Makefile doesn't seem to build contrib/* at all. I always need to call make 
explicitly in these subdirs.

Re: [PATCH/RFC v3 16/16] Add a test script for remote-svn.

[Florian Achleitner]

Forget this patch! It contains some unwanted content. Something with rebasing 
went wrong..

On Tuesday 14 August 2012 21:13:18 Florian Achleitner wrote:
> Use svnrdump_sim.py to emulate svnrdump without an svn server.
> Tests fetching, incremental fetching, fetching from file://,
> and the regeneration of fast-import's marks file.
> 
> Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> ---
>  t/t9020-remote-svn.sh |   69
> +++++++++++++++++++++++++++++++++++++++++++++++++ transport-helper.c    |  
> 15 ++++++-----
>  2 files changed, 77 insertions(+), 7 deletions(-)
>  create mode 100755 t/t9020-remote-svn.sh
> 
> diff --git a/t/t9020-remote-svn.sh b/t/t9020-remote-svn.sh
> new file mode 100755
> index 0000000..a0c6a21
> --- /dev/null
> +++ b/t/t9020-remote-svn.sh
> @@ -0,0 +1,69 @@
> +#!/bin/sh
> +
> +test_description='tests remote-svn'
> +
> +. ./test-lib.sh
> +
> +# We override svnrdump by placing a symlink to the svnrdump-emulator in .
> +export PATH="$HOME:$PATH"
> +ln -sf $GIT_BUILD_DIR/contrib/svn-fe/svnrdump_sim.py "$HOME/svnrdump"
> +
> +init_git () {
> +	rm -fr .git &&
> +	git init &&
> +	#git remote add svnsim svn::sim:///$TEST_DIRECTORY/t9020/example.svnrdump
> +	# let's reuse an exisiting dump file!?
> +	git remote add svnsim svn::sim:///$TEST_DIRECTORY/t9154/svn.dump
> +	git remote add svnfile svn::file:///$TEST_DIRECTORY/t9154/svn.dump
> +}
> +
> +test_debug '
> +	git --version
> +	which git
> +	which svnrdump
> +'
> +
> +test_expect_success 'simple fetch' '
> +	init_git &&
> +	git fetch svnsim &&
> +	test_cmp .git/refs/svn/svnsim/master .git/refs/remotes/svnsim/master  &&
> +	cp .git/refs/remotes/svnsim/master master.good
> +'
> +
> +test_debug '
> +	cat .git/refs/svn/svnsim/master
> +	cat .git/refs/remotes/svnsim/master
> +'
> +
> +test_expect_success 'repeated fetch, nothing shall change' '
> +	git fetch svnsim &&
> +	test_cmp master.good .git/refs/remotes/svnsim/master
> +'
> +
> +test_expect_success 'fetch from a file:// url gives the same result' '
> +	git fetch svnfile
> +'
> +
> +test_expect_failure 'the sha1 differ because the git-svn-id line in the
> commit msg contains the url' ' +	test_cmp .git/refs/remotes/svnfile/master
> .git/refs/remotes/svnsim/master +'
> +
> +test_expect_success 'mark-file regeneration' '
> +	mv .git/info/fast-import/marks/svnsim
> .git/info/fast-import/marks/svnsim.old && +	git fetch svnsim &&
> +	test_cmp .git/info/fast-import/marks/svnsim.old
> .git/info/fast-import/marks/svnsim +'
> +
> +test_expect_success 'incremental imports must lead to the same head' '
> +	export SVNRMAX=3 &&
> +	init_git &&
> +	git fetch svnsim &&
> +	test_cmp .git/refs/svn/svnsim/master .git/refs/remotes/svnsim/master  &&
> +	unset SVNRMAX &&
> +	git fetch svnsim &&
> +	test_cmp master.good .git/refs/remotes/svnsim/master
> +'
> +
> +test_debug 'git branch -a'
> +
> +test_done
> diff --git a/transport-helper.c b/transport-helper.c
> index 47db055..a363f2c 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -17,6 +17,7 @@ static int debug;
>  struct helper_data {
>  	const char *name;
>  	struct child_process *helper;
> +	struct argv_array argv;
>  	FILE *out;
>  	unsigned fetch : 1,
>  		import : 1,
> @@ -103,7 +104,6 @@ static void do_take_over(struct transport *transport)
>  static struct child_process *get_helper(struct transport *transport)
>  {
>  	struct helper_data *data = transport->data;
> -	struct argv_array argv = ARGV_ARRAY_INIT;
>  	struct strbuf buf = STRBUF_INIT;
>  	struct child_process *helper;
>  	const char **refspecs = NULL;
> @@ -125,10 +125,11 @@ static struct child_process *get_helper(struct
> transport *transport) helper->in = -1;
>  	helper->out = -1;
>  	helper->err = 0;
> -	argv_array_pushf(&argv, "git-remote-%s", data->name);
> -	argv_array_push(&argv, transport->remote->name);
> -	argv_array_push(&argv, remove_ext_force(transport->url));
> -	helper->argv = argv.argv;
> +	argv_array_init(&data->argv);
> +	argv_array_pushf(&data->argv, "git-remote-%s", data->name);
> +	argv_array_push(&data->argv, transport->remote->name);
> +	argv_array_push(&data->argv, remove_ext_force(transport->url));
> +	helper->argv = data->argv.argv;
>  	helper->git_cmd = 0;
>  	helper->silent_exec_failure = 1;
> 
> @@ -143,8 +144,6 @@ static struct child_process *get_helper(struct transport
> *transport)
> 
>  	data->helper = helper;
>  	data->no_disconnect_req = 0;
> -	free((void*) helper_env[1]);
> -	argv_array_clear(&argv);
> 
>  	/*
>  	 * Open the output as FILE* so strbuf_getline() can be used.
> @@ -247,6 +246,8 @@ static int disconnect_helper(struct transport
> *transport) close(data->helper->out);
>  		fclose(data->out);
>  		res = finish_command(data->helper);
> +		free((void*) data->helper->env[1]);
> +		argv_array_clear(&data->argv);
>  		free(data->helper);
>  		data->helper = NULL;
>  	}

Re: [PATCH/RFC v3 01/16] Implement a remote helper for svn in C.

[Florian Achleitner]

On Tuesday 14 August 2012 13:07:32 Junio C Hamano wrote:
> Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:
> > Enable basic fetching from subversion repositories. When processing remote
> > URLs starting with svn::, git invokes this remote-helper.
> > It starts svnrdump to extract revisions from the subversion repository in
> > the 'dump file format', and converts them to a git-fast-import stream
> > using the functions of vcs-svn/.
> 
> (nit) the above is a bit too wide, isn't it?
> 
> > Imported refs are created in a private namespace at
> > refs/svn/<remote-name/master.
> (nit) missing closing '>'?
> 
> > The revision history is imported linearly (no branch detection) and
> > completely, i.e. from revision 0 to HEAD.
> > 
> > The 'bidi-import' capability is used. The remote-helper expects data from
> > fast-import on its stdin. It buffers a batch of 'import' command lines
> > in a string_list before starting to process them.
> > 
> > Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> > ---
> > diff:
> > - incorporate review
> > - remove redundant strbuf_init
> > - add 'bidi-import' to capabilities
> > - buffer all lines of a command batch in string_list
> > 
> >  contrib/svn-fe/remote-svn.c |  183
> >  +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 183
> >  insertions(+)
> >  create mode 100644 contrib/svn-fe/remote-svn.c
> > 
> > diff --git a/contrib/svn-fe/remote-svn.c b/contrib/svn-fe/remote-svn.c
> > new file mode 100644
> > index 0000000..ce59344
> > --- /dev/null
> > +++ b/contrib/svn-fe/remote-svn.c
> > @@ -0,0 +1,183 @@
> > +
> 
> Remove.
> 
> > +#include "cache.h"
> > +#include "remote.h"
> > +#include "strbuf.h"
> > +#include "url.h"
> > +#include "exec_cmd.h"
> > +#include "run-command.h"
> > +#include "svndump.h"
> > +#include "notes.h"
> > +#include "argv-array.h"
> > +
> > +static const char *url;
> > +static const char *private_ref;
> > +static const char *remote_ref = "refs/heads/master";
> 
> Just wondering; is this name "master" (or "refs/heads/" for that
> matter) significant in any way when talking to a subversion remote?

No, it isn't. But it has to specify something in the list command.

> 
> > +static int cmd_capabilities(const char *line);
> > +static int cmd_import(const char *line);
> > +static int cmd_list(const char *line);
> > +
> > +typedef int (*input_command_handler)(const char *);
> > +struct input_command_entry {
> > +	const char *name;
> > +	input_command_handler fct;
> > +	unsigned char batchable;	/* whether the command starts or is part of a
> > batch */ +};
> > +
> > +static const struct input_command_entry input_command_list[] = {
> > +		{ "capabilities", cmd_capabilities, 0 },
> 
> One level too deeply indented?
> 
> > +		{ "import", cmd_import, 1 },
> > +		{ "list", cmd_list, 0 },
> > +		{ NULL, NULL }
> > +};
> > +
> > +static int cmd_capabilities(const char *line) {
> > +	printf("import\n");
> > +	printf("bidi-import\n");
> > +	printf("refspec %s:%s\n\n", remote_ref, private_ref);
> > +	fflush(stdout);
> > +	return 0;
> > +}
> > +
> > +static void terminate_batch(void)
> > +{
> > +	/* terminate a current batch's fast-import stream */
> > +		printf("done\n");
> 
> Likewise.
> 
> > +		fflush(stdout);
> > +}
> > +
> > +static int cmd_import(const char *line)
> > +{
> > +	int code;
> > +	int dumpin_fd;
> > +	unsigned int startrev = 0;
> > +	struct argv_array svndump_argv = ARGV_ARRAY_INIT;
> > +	struct child_process svndump_proc;
> > +
> > +	memset(&svndump_proc, 0, sizeof (struct child_process));
> 
> Please lose SP between sizeof and '('.
> 
> > +	svndump_proc.out = -1;
> > +	argv_array_push(&svndump_argv, "svnrdump");
> > +	argv_array_push(&svndump_argv, "dump");
> > +	argv_array_push(&svndump_argv, url);
> > +	argv_array_pushf(&svndump_argv, "-r%u:HEAD", startrev);
> > +	svndump_proc.argv = svndump_argv.argv;
> 
> (just me making a mental note) We read from "svnrdump", which would
> read (if it ever does) from the same stdin as ours and spits (if it
> ever does) its errors to the same stderr as ours.

Yes. svnrdump sometimes queries passwords, in this case it reads stdin, from 
the the terminal, and it writes errors and the password prompt to stderr, the 
terminal.

> 
> > +
> > +	code = start_command(&svndump_proc);
> > +	if (code)
> > +		die("Unable to start %s, code %d", svndump_proc.argv[0], code);
> > +	dumpin_fd = svndump_proc.out;
> > +
> > +	code = start_command(&svndump_proc);
> > +	if (code)
> > +		die("Unable to start %s, code %d", svndump_proc.argv[0], code);
> > +	dumpin_fd = svndump_proc.out;
> 
> You start it twice without finishing the first invocation, or just a
> double paste?

Sorry, thats garbage. It's meant to be only once. This was a mistake in a 
rebase merge.

> 
> > +	svndump_init_fd(dumpin_fd, STDIN_FILENO);
> > +	svndump_read(url, private_ref);
> > +	svndump_deinit();
> > +	svndump_reset();
> > +
> > +	close(dumpin_fd);
> 
> (my mental note) And at this point, we finished feeding whatever
> comes out of "svnrdump" to svndump_read().
> 
> > +	code = finish_command(&svndump_proc);
> > +	if (code)
> > +		warning("%s, returned %d", svndump_proc.argv[0], code);
> > +	argv_array_clear(&svndump_argv);
> > +
> > +	return 0;
> 
> Other than the "twice?" puzzle, this function looks straightforward.
> 
> > +}
> > +
> > +static int cmd_list(const char *line)
> > +{
> > +	printf("? %s\n\n", remote_ref);
> > +	fflush(stdout);
> > +	return 0;
> > +}
> > +
> > +static int do_command(struct strbuf *line)
> > +{
> > +	const struct input_command_entry *p = input_command_list;
> > +	static struct string_list batchlines = STRING_LIST_INIT_DUP;
> > +	static const struct input_command_entry *batch_cmd;
> > +	/*
> > +	 * commands can be grouped together in a batch.
> > +	 * Batches are ended by \n. If no batch is active the program ends.
> > +	 * During a batch all lines are buffered and passed to the handler
> > function +	 * when the batch is terminated.
> > +	 */
> > +	if (line->len == 0) {
> > +		if (batch_cmd) {
> > +			struct string_list_item *item;
> > +			for_each_string_list_item(item, &batchlines)
> > +				batch_cmd->fct(item->string);
> 
> (style) I think we tend to call these unnamed functions "fn" in our
> codebase.
> 
> > +			terminate_batch();
> > +			batch_cmd = NULL;
> > +			string_list_clear(&batchlines, 0);
> > +			return 0;	/* end of the batch, continue reading other commands. */
> > +		}
> > +		return 1;	/* end of command stream, quit */
> > +	}
> > +	if (batch_cmd) {
> > +		if (strcmp(batch_cmd->name, line->buf))
> > +			die("Active %s batch interrupted by %s", batch_cmd->name, line-
>buf);
> > +		/* buffer batch lines */
> > +		string_list_append(&batchlines, line->buf);
> > +		return 0;
> > +	}
> 
> A "batch-able" command, e.g. "import", will first cause the
> batch_cmd to point at the command structure in this function, and
> then the next and subsequent lines, as long as the input line is
> exactly the same as the current batch_cmd->name, e.g. "import", is
> appended into batchlines.
> 
> Would this mean that you can feed something like this:
> 
> 	import foobar
>         import
>         import
>         import
> 
>         another command
> 
> and buffer the four "import" lines in batchlines, and then on the
> empty line, have the for-each-string-list-item loop to call
> cmd_import() on "import foobar", "import", "import", then "import"
> (literally, without anything other than "import" on the line).
> 
> How is that useful?  With that "if (strcmp(batch_cmd->name, line->buf))",
> I cannot think of other valid input to make this "batch" mechanism
> to trigger and do something useful.  Am I missing something?
> 
That's a mistake I made when adding the line buffering. It should allow 
commands like:

import foo
import bar

some other command.

For this helper in it's current state it's anyways not useful to send more 
than one import command, because it can only import revisions it doesn't yet 
have. So a second import would always do nothing.
The import batches are specified in Documentation/git-remote-helpers.txt.
So this helper should support it, I thought.
The buffering of import lines is necessary with the bidi-import capability, 
suggested by Jonathan. It uses the helper's stdin for command input and for 
reading fast-imports output. So we must make sure these two datastreams don't 
mix.

> > +
> > +	for(p = input_command_list; p->name; p++) {
> 
> Have a SP between for and '('.
> 
> > +		if (!prefixcmp(line->buf, p->name) &&
> > +				(strlen(p->name) == line->len || line->buf[strlen(p->name)] == ' 
'))
> > {
> 
> A line way too wide.
> 
> > +			if (p->batchable) {
> > +				batch_cmd = p;
> > +				string_list_append(&batchlines, line->buf);
> > +				return 0;
> > +			}
> > +			return p->fct(line->buf);
> > +		}
> 
> OK, so a command word on a line by itself, or a command word
> followed by a SP (probably followed by its arguments) on a line
> triggers a command lookup, and individual command implementation
> parses the line.
> 
> > +	}
> > +	warning("Unknown command '%s'\n", line->buf);
> > +	return 0;
> 
> Why isn't this an error?

Yeah, it should.

> 
> > +}
> > +
> > +int main(int argc, const char **argv)
> > +{
> > +	struct strbuf buf = STRBUF_INIT;
> > +	int nongit;
> > +	static struct remote *remote;
> > +	const char *url_in;
> > +
> > +	git_extract_argv0_path(argv[0]);
> > +	setup_git_directory_gently(&nongit);
> > +	if (argc < 2 || argc > 3) {
> > +		usage("git-remote-svn <remote-name> [<url>]");
> > +		return 1;
> > +	}
> 
> If this is an importer, you would be importing _into_ a git
> repository, no?  How can you not error out when you are not in one?
> In other words, why &nongit with *_gently()?

Hm .. in fact it only feeds into fast-import. But you're right, it doesn't 
make much sense. I took remote-curl.c as a guideline for these first lines.
Will use the non-gentle function.

> 
> > +	remote = remote_get(argv[1]);
> > +	url_in = remote->url[0];
> > +	if (argc == 3)
> > +		url_in = argv[2];
> 
> Shouldn't it be more like this?
> 
> 	url_in = (argc == 3) ? argv[2] : remote->url[0];

Yes, much nicer.

> 
> > +	end_url_with_slash(&buf, url_in);
> > +	url = strbuf_detach(&buf, NULL);
> > +
> > +	strbuf_addf(&buf, "refs/svn/%s/master", remote->name);
> > +	private_ref = strbuf_detach(&buf, NULL);
> > +
> > +	while(1) {
> > +		if (strbuf_getline(&buf, stdin, '\n') == EOF) {
> > +			if (ferror(stdin))
> > +				die("Error reading command stream");
> > +			else
> > +				die("Unexpected end of command stream");
> > +		}
> > +		if (do_command(&buf))
> > +			break;
> > +		strbuf_reset(&buf);
> > +	}
> > +
> > +	strbuf_release(&buf);
> > +	free((void*)url);
> > +	free((void*)private_ref);
> > +	return 0;
> > +}

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

[Florian Achleitner]

On Tuesday 14 August 2012 13:40:20 Junio C Hamano wrote:
> Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:
> > The fast-import commands 'cat-blob' and 'ls' can be used by remote-helpers
> > to retrieve information about blobs and trees that already exist in
> > fast-import's memory. This requires a channel from fast-import to the
> > remote-helper.
> > remote-helpers that use this features shall advertise the new
> > 'bidi-import'
> 
> s/this fea/these fea/
> 
> > capability so signal that they require the communication channel.
> 
> s/so sig/to sig/, I think.
> 
> > When forking fast-import in transport-helper.c connect it to a dup of
> > the remote-helper's stdin-pipe. The additional file descriptor is passed
> > to fast-import via it's command line (--cat-blob-fd).
> 
> s/via it's/via its/;
> 
> > It follows that git and fast-import are connected to the remote-helpers's
> > stdin.
> > Because git can send multiple commands to the remote-helper on it's stdin,
> > it is required that helpers that advertise 'bidi-import' buffer all input
> > commands until the batch of 'import' commands is ended by a newline
> > before sending data to fast-import.
> > This is to prevent mixing commands and fast-import responses on the
> > helper's stdin.
> 
> Please have a blank line each between paragraphs; a solid block of
> text is very hard to follow.
> 
> > Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> > ---
> > 
> >  transport-helper.c |   45 ++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 32 insertions(+), 13 deletions(-)
> > 
> > diff --git a/transport-helper.c b/transport-helper.c
> > index cfe0988..257274b 100644
> > --- a/transport-helper.c
> > +++ b/transport-helper.c
> > @@ -10,6 +10,7 @@
> > 
> >  #include "string-list.h"
> >  #include "thread-utils.h"
> >  #include "sigchain.h"
> > 
> > +#include "argv-array.h"
> > 
> >  static int debug;
> > 
> > @@ -19,6 +20,7 @@ struct helper_data {
> > 
> >  	FILE *out;
> >  	unsigned fetch : 1,
> >  	
> >  		import : 1,
> > 
> > +		bidi_import : 1,
> > 
> >  		export : 1,
> >  		option : 1,
> >  		push : 1,
> > 
> > @@ -101,6 +103,7 @@ static void do_take_over(struct transport *transport)
> > 
> >  static struct child_process *get_helper(struct transport *transport)
> >  {
> >  
> >  	struct helper_data *data = transport->data;
> > 
> > +	struct argv_array argv = ARGV_ARRAY_INIT;
> > 
> >  	struct strbuf buf = STRBUF_INIT;
> >  	struct child_process *helper;
> >  	const char **refspecs = NULL;
> > 
> > @@ -122,11 +125,10 @@ static struct child_process *get_helper(struct
> > transport *transport)> 
> >  	helper->in = -1;
> >  	helper->out = -1;
> >  	helper->err = 0;
> > 
> > -	helper->argv = xcalloc(4, sizeof(*helper->argv));
> > -	strbuf_addf(&buf, "git-remote-%s", data->name);
> > -	helper->argv[0] = strbuf_detach(&buf, NULL);
> > -	helper->argv[1] = transport->remote->name;
> > -	helper->argv[2] = remove_ext_force(transport->url);
> > +	argv_array_pushf(&argv, "git-remote-%s", data->name);
> > +	argv_array_push(&argv, transport->remote->name);
> > +	argv_array_push(&argv, remove_ext_force(transport->url));
> > +	helper->argv = argv.argv;
> 
> Much nicer than before thanks to argv_array ;-)
> 
> >  	helper->git_cmd = 0;
> >  	helper->silent_exec_failure = 1;
> > 
> > @@ -141,6 +143,8 @@ static struct child_process *get_helper(struct
> > transport *transport)> 
> >  	data->helper = helper;
> >  	data->no_disconnect_req = 0;
> > 
> > +	free((void*) helper_env[1]);
> 
> What is this free() for???

Sorry, legacy from previous versions, will be deleted.
> 
> > +	argv_array_clear(&argv);
> 
> See below.
> 
> >  	/*
> >  	
> >  	 * Open the output as FILE* so strbuf_getline() can be used.
> > 
> > @@ -178,6 +182,8 @@ static struct child_process *get_helper(struct
> > transport *transport)> 
> >  			data->push = 1;
> >  		
> >  		else if (!strcmp(capname, "import"))
> >  		
> >  			data->import = 1;
> > 
> > +		else if (!strcmp(capname, "bidi-import"))
> > +			data->bidi_import = 1;
> > 
> >  		else if (!strcmp(capname, "export"))
> >  		
> >  			data->export = 1;
> >  		
> >  		else if (!data->refspecs && !prefixcmp(capname, "refspec ")) {
> > 
> > @@ -241,8 +247,6 @@ static int disconnect_helper(struct transport
> > *transport)> 
> >  		close(data->helper->out);
> >  		fclose(data->out);
> >  		res = finish_command(data->helper);
> > 
> > -		free((char *)data->helper->argv[0]);
> > -		free(data->helper->argv);
> > 
> >  		free(data->helper);
> >  		data->helper = NULL;
> >  	
> >  	}
> > 
> > @@ -376,14 +380,24 @@ static int fetch_with_fetch(struct transport
> > *transport,> 
> >  static int get_importer(struct transport *transport, struct child_process
> >  *fastimport) {
> >  
> >  	struct child_process *helper = get_helper(transport);
> > 
> > +	struct helper_data *data = transport->data;
> > +	struct argv_array argv = ARGV_ARRAY_INIT;
> > +	int cat_blob_fd, code;
> > 
> >  	memset(fastimport, 0, sizeof(*fastimport));
> >  	fastimport->in = helper->out;
> > 
> > -	fastimport->argv = xcalloc(5, sizeof(*fastimport->argv));
> > -	fastimport->argv[0] = "fast-import";
> > -	fastimport->argv[1] = "--quiet";
> > +	argv_array_push(&argv, "fast-import");
> > +	argv_array_push(&argv, "--quiet");
> > 
> > +	if (data->bidi_import) {
> > +		cat_blob_fd = xdup(helper->in);
> > +		argv_array_pushf(&argv, "--cat-blob-fd=%d", cat_blob_fd);
> > +	}
> > +	fastimport->argv = argv.argv;
> > 
> >  	fastimport->git_cmd = 1;
> > 
> > -	return start_command(fastimport);
> > +
> > +	code = start_command(fastimport);
> > +	argv_array_clear(&argv);
> > +	return code;
> > 
> >  }
> >  
> >  static int get_exporter(struct transport *transport,
> > 
> > @@ -438,11 +452,16 @@ static int fetch_with_import(struct transport
> > *transport,> 
> >  	}
> >  	
> >  	write_constant(data->helper->in, "\n");
> > 
> > +	/*
> > +	 * remote-helpers that advertise the bidi-import capability are required
> > to +	 * buffer the complete batch of import commands until this newline
> > before +	 * sending data to fast-import.
> > +	 * These helpers read back data from fast-import on their stdin, which
> > could +	 * be mixed with import commands, otherwise.
> > +	 */
> > 
> >  	if (finish_command(&fastimport))
> >  	
> >  		die("Error while running fast-import");
> > 
> > -	free(fastimport.argv);
> > -	fastimport.argv = NULL;
> 
> The updated code frees argv[] immediately after start_command()
> returns, and it may happen to be safe to do so with the current
> implementation of start_command() and friends, but I think it is a
> bad taste to free argv[] (or env[] for that matter) before calling
> finish_command().  These pieces of memory are still pointed by the
> child_process structure, and users of the structure may want to use
> contents of them (especially, argv[0]) for reporting errors and
> various other purposes, e.g.
> 
> 	child = get_helper();
> 
>         trace("started %s\n", child->argv[0]);
> 
> 	if (finish_command(child))
>         	return error("failed to cleanly finish %s", child->argv[0]);

Yes, sounds reasonable. The present of immedate clearing has the advantage 
that I don't have to store the struct argv_array, as struct child_process only 
has a member for const char **argv.
I'll improve postpone the free until the command finishes.

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

[Junio C Hamano]

Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:

>> The updated code frees argv[] immediately after start_command()
>> returns, and it may happen to be safe to do so with the current
>> implementation of start_command() and friends, but I think it is a
>> bad taste to free argv[] (or env[] for that matter) before calling
>> finish_command().  These pieces of memory are still pointed by the
>> child_process structure, and users of the structure may want to use
>> contents of them (especially, argv[0]) for reporting errors and
>> various other purposes, e.g.
>> 
>> 	child = get_helper();
>> 
>>         trace("started %s\n", child->argv[0]);
>> 
>> 	if (finish_command(child))
>>         	return error("failed to cleanly finish %s", child->argv[0]);
>
> Yes, sounds reasonable. The present of immedate clearing has the advantage 
> that I don't have to store the struct argv_array, as struct child_process only 
> has a member for const char **argv.

And updated code shouldn't have to store struct argv_array either.
If you just give the ownership of argv_array.argv to child_process
and clean it as part of destroying the child_process, you do not
have to worry about argv_array at all.

In order to cleanly support that use case at the API level, we may
want to introduce argv_array_detach() that is similar in spirit to
strbuf_detach(), which transfers ownership of the underlying memory
to the caller.

Re: [PATCH/RFC v3 10/16] Create a note for every imported commit containing svn metadata.

[Junio C Hamano]

Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:

> To provide metadata from svn dumps for further processing, e.g.
> branch detection, attach a note to each imported commit that
> stores additional information.
> The notes are currently hard-coded in refs/notes/svn/revs.
> Currently the following lines from the svn dump are directly
> accumulated in the note. This can be refined on purpose, of course.
> - "Revision-number"
> - "Node-path"
> - "Node-kind"
> - "Node-action"
> - "Node-copyfrom-path"
> - "Node-copyfrom-rev"
>
> Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> ---
>  vcs-svn/fast_export.c |   13 +++++++++++++
>  vcs-svn/fast_export.h |    2 ++
>  vcs-svn/svndump.c     |   21 +++++++++++++++++++--
>  3 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
> index 1ecae4b..796dd1a 100644
> --- a/vcs-svn/fast_export.c
> +++ b/vcs-svn/fast_export.c
> @@ -12,6 +12,7 @@
>  #include "svndiff.h"
>  #include "sliding_window.h"
>  #include "line_buffer.h"
> +#include "cache.h"

Shouldn't it be near the beginning?  Also if you include "cache.h",
it probably makes git-compat-util and strbuf redundant.

>  
>  #define MAX_GITSVN_LINE_LEN 4096
>  
> @@ -68,6 +69,18 @@ void fast_export_modify(const char *path, uint32_t mode, const char *dataref)
>  	putchar('\n');
>  }
>  
> +void fast_export_begin_note(uint32_t revision, const char *author,
> +		const char *log, unsigned long timestamp)
> +{
> +	timestamp = 1341914616;

The magic number needs some comment.

> +	size_t loglen = strlen(log);

decl-after-statement.  I am starting to suspect that the assignment
is a leftover from an earlier debugging effort, though.

Re: [PATCH/RFC v3 11/16] When debug==1, start fast-import with "--stats" instead of "--quiet".

[Junio C Hamano]

Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:

> fast-import prints statistics that could be interesting to the
> developer of remote helpers.
>
> Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> ---

Sounds sensible and could be useful outside the context of this
series.  Perhaps place it earlier in the series?

>  transport-helper.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 257274b..7fb52d4 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -386,7 +386,7 @@ static int get_importer(struct transport *transport, struct child_process *fasti
>  	memset(fastimport, 0, sizeof(*fastimport));
>  	fastimport->in = helper->out;
>  	argv_array_push(&argv, "fast-import");
> -	argv_array_push(&argv, "--quiet");
> +	argv_array_push(&argv, debug ? "--stats" : "--quiet");
>  
>  	if (data->bidi_import) {
>  		cat_blob_fd = xdup(helper->in);

Re: [PATCH/RFC v3 14/16] transport-helper: add import|export-marks to fast-import command line.

[Junio C Hamano]

Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:

> fast-import internally uses marks that refer to an object via its sha1.
> Those marks are created during import to find previously created objects.
> At exit the accumulated marks can be exported to a file and reloaded at
> startup, so that the previous marks are available.
> Add command line options to the fast-import command line to enable this.
> The mark files are stored in info/fast-import/marks/<remote-name>.
>
> Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> ---
>  transport-helper.c |    3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/transport-helper.c b/transport-helper.c
> index 7fb52d4..47db055 100644
> --- a/transport-helper.c
> +++ b/transport-helper.c
> @@ -387,6 +387,9 @@ static int get_importer(struct transport *transport, struct child_process *fasti
>  	fastimport->in = helper->out;
>  	argv_array_push(&argv, "fast-import");
>  	argv_array_push(&argv, debug ? "--stats" : "--quiet");
> +	argv_array_push(&argv, "--relative-marks");
> +	argv_array_pushf(&argv, "--import-marks-if-exists=marks/%s", transport->remote->name);
> +	argv_array_pushf(&argv, "--export-marks=marks/%s", transport->remote->name);

Is this something we want to do unconditionally?

Re: [PATCH/RFC v3 10/16] Create a note for every imported commit containing svn metadata.

[Florian Achleitner]

On Wednesday 15 August 2012 12:49:04 Junio C Hamano wrote:
> Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:
> > To provide metadata from svn dumps for further processing, e.g.
> > branch detection, attach a note to each imported commit that
> > stores additional information.
> > The notes are currently hard-coded in refs/notes/svn/revs.
> > Currently the following lines from the svn dump are directly
> > accumulated in the note. This can be refined on purpose, of course.
> > - "Revision-number"
> > - "Node-path"
> > - "Node-kind"
> > - "Node-action"
> > - "Node-copyfrom-path"
> > - "Node-copyfrom-rev"
> > 
> > Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> > ---
> > 
> >  vcs-svn/fast_export.c |   13 +++++++++++++
> >  vcs-svn/fast_export.h |    2 ++
> >  vcs-svn/svndump.c     |   21 +++++++++++++++++++--
> >  3 files changed, 34 insertions(+), 2 deletions(-)
> > 
> > diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
> > index 1ecae4b..796dd1a 100644
> > --- a/vcs-svn/fast_export.c
> > +++ b/vcs-svn/fast_export.c
> > @@ -12,6 +12,7 @@
> > 
> >  #include "svndiff.h"
> >  #include "sliding_window.h"
> >  #include "line_buffer.h"
> > 
> > +#include "cache.h"
> 
> Shouldn't it be near the beginning?  Also if you include "cache.h",
> it probably makes git-compat-util and strbuf redundant.

Ack.

> 
> >  #define MAX_GITSVN_LINE_LEN 4096
> > 
> > @@ -68,6 +69,18 @@ void fast_export_modify(const char *path, uint32_t
> > mode, const char *dataref)> 
> >  	putchar('\n');
> >  
> >  }
> > 
> > +void fast_export_begin_note(uint32_t revision, const char *author,
> > +		const char *log, unsigned long timestamp)
> > +{
> > +	timestamp = 1341914616;
> 
> The magic number needs some comment.
> 
> > +	size_t loglen = strlen(log);
> 
> decl-after-statement.  I am starting to suspect that the assignment
> is a leftover from an earlier debugging effort, though.

Oh yes sorry. Leftover from a previous experiment.
Thx for your reviews Junio, I got too blind to see this.

Re: [PATCH/RFC v3 14/16] transport-helper: add import|export-marks to fast-import command line.

[Florian Achleitner]

On Wednesday 15 August 2012 12:52:43 Junio C Hamano wrote:
> Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:
> > fast-import internally uses marks that refer to an object via its sha1.
> > Those marks are created during import to find previously created objects.
> > At exit the accumulated marks can be exported to a file and reloaded at
> > startup, so that the previous marks are available.
> > Add command line options to the fast-import command line to enable this.
> > The mark files are stored in info/fast-import/marks/<remote-name>.
> > 
> > Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> > ---
> > 
> >  transport-helper.c |    3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/transport-helper.c b/transport-helper.c
> > index 7fb52d4..47db055 100644
> > --- a/transport-helper.c
> > +++ b/transport-helper.c
> > @@ -387,6 +387,9 @@ static int get_importer(struct transport *transport,
> > struct child_process *fasti> 
> >  	fastimport->in = helper->out;
> >  	argv_array_push(&argv, "fast-import");
> >  	argv_array_push(&argv, debug ? "--stats" : "--quiet");
> > 
> > +	argv_array_push(&argv, "--relative-marks");
> > +	argv_array_pushf(&argv, "--import-marks-if-exists=marks/%s",
> > transport->remote->name); +	argv_array_pushf(&argv,
> > "--export-marks=marks/%s", transport->remote->name);
> Is this something we want to do unconditionally?

Good question. It doesn't hurt, but it maybe . We could add another capability 
for remote-helpers, that tells us if it needs masks. What do you think?

Re: [PATCH/RFC v3 14/16] transport-helper: add import|export-marks to fast-import command line.

[Florian Achleitner]

On Wednesday 15 August 2012 22:20:45 Florian Achleitner wrote:
> On Wednesday 15 August 2012 12:52:43 Junio C Hamano wrote:
> > Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:
> > > fast-import internally uses marks that refer to an object via its sha1.
> > > Those marks are created during import to find previously created
> > > objects.
> > > At exit the accumulated marks can be exported to a file and reloaded at
> > > startup, so that the previous marks are available.
> > > Add command line options to the fast-import command line to enable this.
> > > The mark files are stored in info/fast-import/marks/<remote-name>.
> > > 
> > > Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> > > ---
> > > 
> > >  transport-helper.c |    3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/transport-helper.c b/transport-helper.c
> > > index 7fb52d4..47db055 100644
> > > --- a/transport-helper.c
> > > +++ b/transport-helper.c
> > > @@ -387,6 +387,9 @@ static int get_importer(struct transport *transport,
> > > struct child_process *fasti>
> > > 
> > >  	fastimport->in = helper->out;
> > >  	argv_array_push(&argv, "fast-import");
> > >  	argv_array_push(&argv, debug ? "--stats" : "--quiet");
> > > 
> > > +	argv_array_push(&argv, "--relative-marks");
> > > +	argv_array_pushf(&argv, "--import-marks-if-exists=marks/%s",
> > > transport->remote->name); +	argv_array_pushf(&argv,
> > > "--export-marks=marks/%s", transport->remote->name);
> > 
> > Is this something we want to do unconditionally?
> 
> Good question. It doesn't hurt, but it maybe . We could add another
> capability for remote-helpers, that tells us if it needs masks. What do you
> think?

Btw, for fast-export, there is already such a capability. It specifies a 
filename, in addition.