[PATCH v3 0/3] Towards a Git-to-SVN bridge

[PATCH v3 0/3] Towards a Git-to-SVN bridge

[Ramkumar Ramachandra]

Hi,

Here's another attempt. I've omitted 3 patches in this round:
1. The --inline-blobs patch
2. The time_to_tm patch
3. The contrib/svn-fe and test-svn-fi patch
(since those haven't changed from last time)

Changes since last time:
1. Bugfixes and testsuite. All tests pass.
2. Style fixes after Junio's review:
<7v39olok4l.fsf@alter.siamese.dyndns.org>
3. Implemented an experimetal dispatch table suggested by Jonathan:
<20110122191801.GB13023@burratino>

Thanks for reading.

Ramkumar Ramachandra (3):
  vcs-svn: Introduce svnload, a dumpfile producer
  t9010-svn-fi: Add tests for svn-fi
  vcs-svn: Refactor dump_export code into dispatch table

 .gitignore            |    1 +
 Makefile              |    9 +-
 t/t9010-svn-fi.sh     |  303 ++++++++++++++++++++++++++++++++++++++++++++++
 test-svn-fi.c         |   20 +++
 vcs-svn/dir_cache.c   |   40 ++++++
 vcs-svn/dir_cache.h   |   12 ++
 vcs-svn/dump_export.c |  150 +++++++++++++++++++++++
 vcs-svn/dump_export.h |   35 ++++++
 vcs-svn/svnload.c     |  322 +++++++++++++++++++++++++++++++++++++++++++++++++
 vcs-svn/svnload.h     |   10 ++
 10 files changed, 899 insertions(+), 3 deletions(-)
 create mode 100644 t/t9010-svn-fi.sh
 create mode 100644 test-svn-fi.c
 create mode 100644 vcs-svn/dir_cache.c
 create mode 100644 vcs-svn/dir_cache.h
 create mode 100644 vcs-svn/dump_export.c
 create mode 100644 vcs-svn/dump_export.h
 create mode 100644 vcs-svn/svnload.c
 create mode 100644 vcs-svn/svnload.h

-- 
1.7.4.rc1.7.g2cf08.dirty

[PATCH 1/3] vcs-svn: Introduce svnload, a dumpfile producer

[Ramkumar Ramachandra]

Design-wise, svnload resembles svndump. Include a Makefile rule to
build it into vcs-svn/lib.a.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Makefile              |    4 +-
 vcs-svn/dir_cache.c   |   40 ++++++
 vcs-svn/dir_cache.h   |   12 ++
 vcs-svn/dump_export.c |  149 +++++++++++++++++++++++
 vcs-svn/dump_export.h |   33 +++++
 vcs-svn/svnload.c     |  322 +++++++++++++++++++++++++++++++++++++++++++++++++
 vcs-svn/svnload.h     |   10 ++
 7 files changed, 568 insertions(+), 2 deletions(-)
 create mode 100644 vcs-svn/dir_cache.c
 create mode 100644 vcs-svn/dir_cache.h
 create mode 100644 vcs-svn/dump_export.c
 create mode 100644 vcs-svn/dump_export.h
 create mode 100644 vcs-svn/svnload.c
 create mode 100644 vcs-svn/svnload.h

diff --git a/Makefile b/Makefile
index 1345c38..d9c2442 100644
--- a/Makefile
+++ b/Makefile
@@ -1834,9 +1834,9 @@ ifndef NO_CURL
 endif
 XDIFF_OBJS = xdiff/xdiffi.o xdiff/xprepare.o xdiff/xutils.o xdiff/xemit.o \
 	xdiff/xmerge.o xdiff/xpatience.o
-VCSSVN_OBJS = vcs-svn/line_buffer.o \
+VCSSVN_OBJS = vcs-svn/line_buffer.o vcs-svn/svnload.o vcs-svn/dump_export.o \
 	vcs-svn/repo_tree.o vcs-svn/fast_export.o vcs-svn/sliding_window.o \
-	vcs-svn/svndiff.o vcs-svn/svndump.o
+	vcs-svn/svndiff.o vcs-svn/svndump.o vcs-svn/dir_cache.o
 VCSSVN_TEST_OBJS = test-obj-pool.o \
 	test-line-buffer.o test-treap.o test-svn-fe.o
 OBJECTS := $(GIT_OBJS) $(XDIFF_OBJS) $(VCSSVN_OBJS)
diff --git a/vcs-svn/dir_cache.c b/vcs-svn/dir_cache.c
new file mode 100644
index 0000000..9a608ce
--- /dev/null
+++ b/vcs-svn/dir_cache.c
@@ -0,0 +1,40 @@
+/*
+ * Licensed under a two-clause BSD-style license.
+ * See LICENSE for details.
+ */
+
+#include "git-compat-util.h"
+#include "string-list.h"
+#include "line_buffer.h"
+#include "dump_export.h"
+
+static struct string_list dirents = STRING_LIST_INIT_DUP;
+static struct string_list_item *dir = NULL;
+
+void dir_cache_add(const char *path, enum node_kind kind) {
+	dir = string_list_insert(&dirents, path);
+	dir->util = malloc(sizeof(enum node_kind));
+	*((enum node_kind *)(dir->util)) = kind;
+}
+
+void dir_cache_remove(const char *path) {
+	dir = string_list_lookup(&dirents, path);
+	if (dir)
+		*((enum node_kind *)(dir->util)) = NODE_KIND_UNKNOWN;
+}
+
+enum node_kind dir_cache_lookup(const char *path) {
+	dir = string_list_lookup(&dirents, path);
+	if (dir)
+		return *((enum node_kind *)(dir->util));
+	else
+		return NODE_KIND_UNKNOWN;
+}
+
+void dir_cache_init() {
+	return;
+}
+
+void dir_cache_deinit() {
+	string_list_clear(&dirents, 1);
+}
diff --git a/vcs-svn/dir_cache.h b/vcs-svn/dir_cache.h
new file mode 100644
index 0000000..43c3797
--- /dev/null
+++ b/vcs-svn/dir_cache.h
@@ -0,0 +1,12 @@
+#ifndef DIR_CACHE_H_
+#define DIR_CACHE_H_
+
+#include "dump_export.h"
+
+void dir_cache_add(const char *path, enum node_kind kind);
+void dir_cache_remove(const char *path);
+enum node_kind dir_cache_lookup(const char *path);
+void dir_cache_init();
+void dir_cache_deinit();
+
+#endif
diff --git a/vcs-svn/dump_export.c b/vcs-svn/dump_export.c
new file mode 100644
index 0000000..2b23f77
--- /dev/null
+++ b/vcs-svn/dump_export.c
@@ -0,0 +1,149 @@
+/*
+ * Licensed under a two-clause BSD-style license.
+ * See LICENSE for details.
+ */
+
+#include "git-compat-util.h"
+#include "strbuf.h"
+#include "line_buffer.h"
+#include "dump_export.h"
+#include "dir_cache.h"
+
+static struct strbuf props;
+
+void dump_export_begin_rev(int revision, const char *revprops,
+			int prop_len)
+{
+	printf("Revision-number: %d\n", revision);
+	printf("Prop-content-length: %d\n", prop_len);
+	printf("Content-length: %d\n\n", prop_len);
+	printf("%s\n", revprops);
+}
+
+void dump_export_node(const char *path, enum node_kind kind,
+		enum node_action action, unsigned long text_len,
+		unsigned long copyfrom_rev, const char *copyfrom_path)
+{
+	int dump_props = 1; /* Boolean */
+	strbuf_reset(&props);
+	printf("Node-path: %s\n", path);
+	switch (kind) {
+	case NODE_KIND_NORMAL:
+		printf("Node-kind: file\n");
+		break;
+	case NODE_KIND_EXECUTABLE:
+		printf("Node-kind: file\n");
+		strbuf_addf(&props, "K 14\nsvn:executable\nV 1\n*\n");
+		break;
+	case NODE_KIND_SYMLINK:
+		printf("Node-kind: file\n");
+		strbuf_addf(&props, "K 11\nsvn:special\nV 1\n*\n");
+		break;
+	case NODE_KIND_GITLINK:
+		printf("Node-kind: file\n");
+		break;
+	case NODE_KIND_DIR:
+		printf("Node-kind: dir\n");
+		break;
+	case NODE_KIND_SUBDIR:
+		die("Unsupported: subdirectory");
+	default:
+		break;
+	}
+	strbuf_add(&props, "PROPS-END\n", 10);
+
+	switch (action) {
+	case NODE_ACTION_CHANGE:
+		printf("Node-action: change\n");
+		break;
+	case NODE_ACTION_ADD:
+		printf("Node-action: add\n");
+		break;
+	case NODE_ACTION_REPLACE:
+		printf("Node-action: replace\n");
+		break;
+	case NODE_ACTION_DELETE:
+		printf("Node-action: delete\n");
+		dump_props = 0;
+		break;
+	default:
+		break;
+	}
+	if (copyfrom_rev != SVN_INVALID_REV) {
+		printf("Node-copyfrom-rev: %lu\n", copyfrom_rev);
+		printf("Node-copyfrom-path: %s\n", copyfrom_path);
+	}
+	if (dump_props) {
+		printf("Prop-delta: false\n");
+		printf("Prop-content-length: %lu\n", props.len);
+	}
+	if (text_len) {
+		printf("Text-delta: false\n");		
+		printf("Text-content-length: %lu\n", text_len);
+	}
+	if (text_len || dump_props) {
+		printf("Content-length: %lu\n\n", text_len + props.len);
+		printf("%s", props.buf);
+	}
+	if (!text_len)
+		printf("\n");
+}
+
+void dump_export_node_r(const char *path, enum node_kind kind,
+			enum node_action action, unsigned long text_len,
+			unsigned long copyfrom_rev, const char *copyfrom_path)
+{
+	char *start, *t;
+	start = (char *) path;
+
+	while ((t = strchr(start, '/'))) {
+			*t = '\0';
+			if (dir_cache_lookup(path) == NODE_KIND_UNKNOWN) {
+				dir_cache_add(path, NODE_KIND_NORMAL);
+				dump_export_node(path, NODE_KIND_DIR,
+						NODE_ACTION_ADD, 0,
+						SVN_INVALID_REV, NULL);
+			}
+			*t = '/';   /* Change it back */
+			start = t + 1;
+	}
+	switch (dir_cache_lookup(path)) {
+	case NODE_KIND_UNKNOWN:
+		action = NODE_ACTION_ADD;
+		break;
+	case NODE_KIND_SYMLINK:
+		dir_cache_remove(path);
+		dump_export_node(path, NODE_KIND_UNKNOWN,
+				NODE_ACTION_DELETE,
+				0, SVN_INVALID_REV, NULL);
+		action = NODE_ACTION_ADD;
+		break;
+	case NODE_KIND_DIR:
+		die("File was previously a directory?");
+		break;
+	case NODE_KIND_SUBDIR:
+		die("Subdirectories unsupported");
+		break;
+	default:
+		action = NODE_ACTION_CHANGE;
+		break;
+	}
+	dir_cache_add(path, kind);
+	dump_export_node(path, kind, action, text_len, copyfrom_rev, copyfrom_path);
+}
+
+void dump_export_text(struct line_buffer *data, off_t len)
+{
+	buffer_copy_bytes(data, len);
+}
+
+void dump_export_init()
+{
+	strbuf_init(&props, MAX_GITSVN_LINE_LEN);
+	printf("SVN-fs-dump-format-version: 3\n\n");
+}
+
+void dump_export_deinit()
+{
+	strbuf_release(&props);
+}
diff --git a/vcs-svn/dump_export.h b/vcs-svn/dump_export.h
new file mode 100644
index 0000000..e9f51a3
--- /dev/null
+++ b/vcs-svn/dump_export.h
@@ -0,0 +1,33 @@
+#ifndef DUMP_EXPORT_H_
+#define DUMP_EXPORT_H_
+
+#define MAX_GITSVN_LINE_LEN 4096
+#define SVN_INVALID_REV 0
+
+enum node_action {
+	NODE_ACTION_UNKNOWN,
+	NODE_ACTION_CHANGE,
+	NODE_ACTION_ADD,
+	NODE_ACTION_DELETE,
+	NODE_ACTION_REPLACE
+};
+
+enum node_kind {
+	NODE_KIND_UNKNOWN,       /* Missing node */
+	NODE_KIND_NORMAL,
+	NODE_KIND_EXECUTABLE,
+	NODE_KIND_SYMLINK,
+	NODE_KIND_GITLINK,
+	NODE_KIND_DIR,           /* SVN-specific */
+	NODE_KIND_SUBDIR
+};
+
+void dump_export_begin_rev(int revision, const char *revprops, int prop_len);
+void dump_export_text(struct line_buffer *data, off_t len);
+void dump_export_node_r(const char *path, enum node_kind kind,
+			enum node_action action, unsigned long text_len,
+			unsigned long copyfrom_rev, const char *copyfrom_path);
+void dump_export_init();
+void dump_export_deinit();
+
+#endif
diff --git a/vcs-svn/svnload.c b/vcs-svn/svnload.c
new file mode 100644
index 0000000..40fc1db
--- /dev/null
+++ b/vcs-svn/svnload.c
@@ -0,0 +1,322 @@
+/*
+ * Produce a dumpfile v3 from a fast-import stream.
+ * Load the dump into the SVN repository with:
+ * svnrdump load <URL> <dumpfile
+ *
+ * Licensed under a two-clause BSD-style license.
+ * See LICENSE for details.
+ */
+
+#include "cache.h"
+#include "git-compat-util.h"
+#include "line_buffer.h"
+#include "dump_export.h"
+#include "dir_cache.h"
+
+#define SVN_DATE_FORMAT "%Y-%m-%dT%H:%M:%S.000000Z"
+#define SVN_DATE_LEN 27
+#define LENGTH_UNKNOWN (~0)
+
+static struct line_buffer input = LINE_BUFFER_INIT;
+
+static struct {
+	unsigned long prop_len, text_len, copyfrom_rev;
+	int text_delta, prop_delta; /* false=0, true=1, unknown=-1 */
+	enum node_action action;
+	enum node_kind kind;
+	struct strbuf copyfrom_path, path;
+} node_ctx;
+
+static struct {
+	int rev, text_len;
+	struct strbuf props, log;
+	struct strbuf svn_author, author, committer;
+	struct strbuf author_date, committer_date;
+	struct strbuf author_email, committer_email;
+} rev_ctx;
+
+static enum {
+	UNKNOWN_CTX,
+	COMMIT_CTX,
+	BLOB_CTX
+} active_ctx;
+
+static void populate_revprops(struct strbuf *props, size_t author_len,
+			const char *author, size_t log_len, const char *log,
+			size_t date_len, const char *date)
+{
+	strbuf_reset(props);
+	strbuf_addf(props, "K 10\nsvn:author\nV %lu\n%s\n", author_len, author);
+	strbuf_addf(props, "K 7\nsvn:log\nV %lu\n%s\n", log_len, log);
+	if (date_len)
+		/* SVN doesn't like an empty svn:date value */
+		strbuf_addf(props, "K 8\nsvn:date\nV %lu\n%s\n", date_len, date);
+	strbuf_add(props, "PROPS-END\n", 10);
+}
+
+static void parse_author_line(char *val, struct strbuf *name,
+			struct strbuf *email, struct strbuf *date)
+{
+	char *t, *tz_off;
+	char time_buf[SVN_DATE_LEN + 1];
+	int tz_off_buf;
+	const struct tm *tm_time;
+
+	/* Author Name <author@email.com> 1170199019 +0530 */
+	strbuf_reset(name);
+	strbuf_reset(email);
+	strbuf_reset(date);
+	if (!val) die("Malformed author line");
+	if (!(tz_off = strrchr(val, ' '))) goto error;
+	*tz_off++ = '\0';
+	if (!(t = strrchr(val, ' '))) goto error;
+	*(t - 1) = '\0'; /* Ignore '>' from email */
+	t++;
+	tz_off_buf = atoi(tz_off);
+	if (tz_off_buf > 1200 || tz_off_buf  < -1200) goto error;
+	tm_time = time_to_tm(strtoul(t, NULL, 10), tz_off_buf);
+	strftime(time_buf, SVN_DATE_LEN + 1, SVN_DATE_FORMAT, tm_time);
+	strbuf_add(date, time_buf, SVN_DATE_LEN);
+	if (!(t = strchr(val, '<'))) goto error;
+	*(t - 1) = '\0'; /* Ignore ' <' from email */
+	t++;
+	strbuf_add(email, t, strlen(t));
+	strbuf_add(name, val, strlen(val));
+	return;
+error:
+	die("Malformed author line: %s", val);
+}
+
+void build_svn_author(struct strbuf *svn_author)
+{
+	char *t, *email;
+
+	strbuf_reset(svn_author);
+	email = rev_ctx.author_email.buf;
+	if (!(t = strchr(email, '@')))
+		goto error;
+	strbuf_add(svn_author, email, t - email);
+	return;
+error:
+	die("Malformed email: %s", email);
+}
+
+int parse_filemodify_mode(char *val)
+{
+	char *t;
+
+	if (!(t = strchr(val, ' '))) goto error;
+	switch (t - val) {
+	case 6:
+		if (!memcmp(val, "100644", 6))
+			node_ctx.kind = NODE_KIND_NORMAL;
+		else if (!memcmp(val, "100755", 6))
+			node_ctx.kind = NODE_KIND_EXECUTABLE;
+		else if (!memcmp(val, "120000", 6))
+			node_ctx.kind = NODE_KIND_SYMLINK;
+		else if (!memcmp(val, "160000", 6))
+			node_ctx.kind = NODE_KIND_GITLINK;
+		else if (!memcmp(val, "040000", 6))
+			node_ctx.kind = NODE_KIND_SUBDIR;
+		else
+			goto error;
+		break;
+	case 3:
+		if (!memcmp(val, "755", 3))
+			node_ctx.kind = NODE_KIND_EXECUTABLE;
+		else if (!memcmp(val, "644", 3))
+			node_ctx.kind = NODE_KIND_NORMAL;
+		else
+			goto error;
+		break;
+	default:
+		goto error;
+	}
+	return t - val + 1;
+error:
+	die("Unrecognized mode: %s", val);
+}
+
+void svnload_read(void)
+{
+	char *t, *val;
+	int len;
+
+	while ((t = buffer_read_line(&input))) {
+		if ((val = strchr(t, ' ')))
+			*val++ = '\0';
+		len = (val ? val - t - 1 : strlen(t));
+
+		switch (len) {
+		case 1:
+			if (!memcmp(t, "D", 1)) {
+				node_ctx.action = NODE_ACTION_DELETE;
+			} else if (!memcmp(t, "C", 1)) {
+				node_ctx.action = NODE_ACTION_ADD;
+			} else if (!memcmp(t, "R", 1)) {
+				node_ctx.action = NODE_ACTION_REPLACE;
+			} else if (!memcmp(t, "M", 1)) {
+				if (!val) goto error;
+				node_ctx.action = NODE_ACTION_CHANGE;
+				val += parse_filemodify_mode(val);
+				if (!(t = strchr(val, ' '))) goto error;
+				*t++ = '\0';
+				strbuf_reset(&node_ctx.path);
+				strbuf_add(&node_ctx.path, t, strlen(t));
+				if (!strncmp(val, "inline", 6))
+					active_ctx = BLOB_CTX;
+				else if (*val == ':')
+					die("Unsupported dataref: marks");
+				else {
+				error:
+					die("Malformed filemodify line: %s", t);
+				}
+			}
+			break;
+		case 2:
+			if (!memcmp(t, "ls", 2))
+				die("ls not supported");
+		case 3:
+			if (!memcmp(t, "tag", 3))
+				continue;
+			break;
+		case 4:
+			if (!memcmp(t, "blob", 4))
+				continue;
+			else if (!memcmp(t, "mark", 4))
+				continue;
+			else if (!memcmp(t, "from", 4))
+				continue;
+			else if (!memcmp(t, "data", 4)) {
+				switch (active_ctx) {
+				case COMMIT_CTX:
+					strbuf_reset(&rev_ctx.log);
+					buffer_read_binary(&input,
+							&rev_ctx.log,
+							strtoul(val, NULL, 10));
+					populate_revprops(&rev_ctx.props,
+							rev_ctx.svn_author.len,
+							rev_ctx.svn_author.buf,
+							rev_ctx.log.len,
+							rev_ctx.log.buf,
+							rev_ctx.author_date.len,
+							rev_ctx.author_date.buf);
+					dump_export_begin_rev(rev_ctx.rev,
+							rev_ctx.props.buf,
+							rev_ctx.props.len);
+					break;
+				case BLOB_CTX:
+					node_ctx.text_len = strtoul(val, NULL, 10);
+					dump_export_node_r(node_ctx.path.buf, node_ctx.kind,
+							node_ctx.action, node_ctx.text_len,
+							SVN_INVALID_REV, NULL);
+					buffer_copy_bytes(&input, node_ctx.text_len);
+					break;
+				default:
+					break;
+				}
+			}
+			break;
+		case 5:
+			if (!memcmp(t, "reset", 5))
+				continue;
+			if (!memcmp(t, "merge", 5))
+				continue;
+			break;
+		case 6:
+			if (!memcmp(t, "author", 6)) {
+				parse_author_line(val, &rev_ctx.author,
+						&rev_ctx.author_email,
+						&rev_ctx.author_date);
+				build_svn_author(&rev_ctx.svn_author);
+			} else if (!memcmp(t, "commit", 6)) {
+				rev_ctx.rev++;
+				active_ctx = COMMIT_CTX;
+			}
+			break;
+		case 8:
+			if (!memcmp(t, "cat-blob", 8))
+				die("cat-blob unsupported");
+			break;
+		case 9:
+			if (!memcmp(t, "deleteall", 9))
+				continue;
+			else if (!memcmp(t, "committer", 9))
+				parse_author_line(val, &rev_ctx.committer,
+						&rev_ctx.committer_email,
+						&rev_ctx.committer_date);
+			break;
+		default:
+			break;
+		}
+	}
+}
+
+static void reset_rev_ctx(int revision)
+{
+	rev_ctx.rev = revision;
+	strbuf_reset(&rev_ctx.props);
+	strbuf_reset(&rev_ctx.log);
+	strbuf_reset(&rev_ctx.svn_author);
+	strbuf_reset(&rev_ctx.author);
+	strbuf_reset(&rev_ctx.committer);
+	strbuf_reset(&rev_ctx.author_date);
+	strbuf_reset(&rev_ctx.committer_date);
+	strbuf_reset(&rev_ctx.author_email);
+	strbuf_reset(&rev_ctx.committer_email);
+}
+
+static void reset_node_ctx(void)
+{
+	node_ctx.prop_len = LENGTH_UNKNOWN;
+	node_ctx.text_len = LENGTH_UNKNOWN;
+	node_ctx.copyfrom_rev = SVN_INVALID_REV;
+	node_ctx.text_delta = -1;
+	node_ctx.prop_delta = -1;
+	strbuf_reset(&node_ctx.copyfrom_path);
+	strbuf_reset(&node_ctx.path);
+}
+
+int svnload_init(const char *filename)
+{
+	if (buffer_init(&input, filename))
+		return error("cannot open %s: %s", filename, strerror(errno));
+	active_ctx = UNKNOWN_CTX;
+	strbuf_init(&rev_ctx.props, MAX_GITSVN_LINE_LEN);
+	strbuf_init(&rev_ctx.log, MAX_GITSVN_LINE_LEN);
+	strbuf_init(&rev_ctx.author, MAX_GITSVN_LINE_LEN);
+	strbuf_init(&rev_ctx.committer, MAX_GITSVN_LINE_LEN);
+	strbuf_init(&rev_ctx.svn_author, MAX_GITSVN_LINE_LEN);
+	strbuf_init(&rev_ctx.author_date, MAX_GITSVN_LINE_LEN);
+	strbuf_init(&rev_ctx.committer_date, MAX_GITSVN_LINE_LEN);
+	strbuf_init(&rev_ctx.author_email, MAX_GITSVN_LINE_LEN);
+	strbuf_init(&rev_ctx.committer_email, MAX_GITSVN_LINE_LEN);
+	strbuf_init(&node_ctx.path, MAX_GITSVN_LINE_LEN);
+	strbuf_init(&node_ctx.copyfrom_path, MAX_GITSVN_LINE_LEN);
+	dump_export_init();
+	dir_cache_init();
+	return 0;
+}
+
+void svnload_deinit(void)
+{
+	reset_rev_ctx(0);
+	reset_node_ctx();
+	strbuf_release(&rev_ctx.props);
+	strbuf_release(&rev_ctx.log);
+	strbuf_release(&rev_ctx.author);
+	strbuf_release(&rev_ctx.committer);
+	strbuf_release(&rev_ctx.svn_author);
+	strbuf_release(&rev_ctx.author_date);
+	strbuf_release(&rev_ctx.committer_date);
+	strbuf_release(&rev_ctx.author_email);
+	strbuf_release(&rev_ctx.committer_email);
+	strbuf_release(&node_ctx.path);
+	strbuf_release(&node_ctx.copyfrom_path);
+	dump_export_deinit();
+	dir_cache_deinit();
+	if (buffer_deinit(&input))
+		fprintf(stderr, "Input error\n");
+	if (ferror(stdout))
+		fprintf(stderr, "Output error\n");
+}
diff --git a/vcs-svn/svnload.h b/vcs-svn/svnload.h
new file mode 100644
index 0000000..0c8fe8b
--- /dev/null
+++ b/vcs-svn/svnload.h
@@ -0,0 +1,10 @@
+#ifndef SVNLOAD_H_
+#define SVNLOAD_H_
+
+#define SVN_INVALID_REV -1
+
+int svnload_init(const char *filename);
+void svnload_deinit(void);
+void svnload_read(void);
+
+#endif
-- 
1.7.4.rc1.7.g2cf08.dirty

Re: [PATCH 1/3] vcs-svn: Introduce svnload, a dumpfile producer

[Erik Faye-Lund]

A very superficial review, because I don't have much time, and don't
know the surrounding code well. Sorry about that.

On Tue, Feb 1, 2011 at 3:26 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> diff --git a/vcs-svn/dir_cache.c b/vcs-svn/dir_cache.c
> new file mode 100644
> index 0000000..9a608ce
> --- /dev/null
> +++ b/vcs-svn/dir_cache.c
> @@ -0,0 +1,40 @@
> +/*
> + * Licensed under a two-clause BSD-style license.
> + * See LICENSE for details.
> + */
> +
> +#include "git-compat-util.h"
> +#include "string-list.h"
> +#include "line_buffer.h"
> +#include "dump_export.h"
> +
> +static struct string_list dirents = STRING_LIST_INIT_DUP;
> +static struct string_list_item *dir = NULL;
> +
> +void dir_cache_add(const char *path, enum node_kind kind) {

Style: we put the opening bracket of functions on the next line.

> +       dir = string_list_insert(&dirents, path);
> +       dir->util = malloc(sizeof(enum node_kind));
> +       *((enum node_kind *)(dir->util)) = kind;

Unchecked malloc; perhaps you should use xmalloc instead?

> +}
> +
> +void dir_cache_remove(const char *path) {

Same style-violation as above.

> +       dir = string_list_lookup(&dirents, path);
> +       if (dir)
> +               *((enum node_kind *)(dir->util)) = NODE_KIND_UNKNOWN;
> +}
> +
> +enum node_kind dir_cache_lookup(const char *path) {
> +       dir = string_list_lookup(&dirents, path);
> +       if (dir)
> +               return *((enum node_kind *)(dir->util));
> +       else
> +               return NODE_KIND_UNKNOWN;
> +}
> +
> +void dir_cache_init() {
> +       return;
> +}
> +
> +void dir_cache_deinit() {
> +       string_list_clear(&dirents, 1);
> +}

Three more.

> +static void populate_revprops(struct strbuf *props, size_t author_len,
> +                       const char *author, size_t log_len, const char *log,
> +                       size_t date_len, const char *date)
> +{
> +       strbuf_reset(props);
> +       strbuf_addf(props, "K 10\nsvn:author\nV %lu\n%s\n", author_len, author);
> +       strbuf_addf(props, "K 7\nsvn:log\nV %lu\n%s\n", log_len, log);
> +       if (date_len)
> +               /* SVN doesn't like an empty svn:date value */
> +               strbuf_addf(props, "K 8\nsvn:date\nV %lu\n%s\n", date_len, date);

Perhaps a scope around here will make this a bit easier to read. At
first glance it looked like a missing scope to me, due to the indented
comment...

> +       if (!val) die("Malformed author line");
> +       if (!(tz_off = strrchr(val, ' '))) goto error;
> +       *tz_off++ = '\0';
> +       if (!(t = strrchr(val, ' '))) goto error;

style: use
"if (x)
	do_stuff();"
instead of
"if (x) do_stuff();"


> +       *(t - 1) = '\0'; /* Ignore '>' from email */
> +       t++;
> +       tz_off_buf = atoi(tz_off);
> +       if (tz_off_buf > 1200 || tz_off_buf  < -1200) goto error;

same

> +       tm_time = time_to_tm(strtoul(t, NULL, 10), tz_off_buf);
> +       strftime(time_buf, SVN_DATE_LEN + 1, SVN_DATE_FORMAT, tm_time);
> +       strbuf_add(date, time_buf, SVN_DATE_LEN);
> +       if (!(t = strchr(val, '<'))) goto error;

same

> +int parse_filemodify_mode(char *val)
> +{
> +       char *t;
> +
> +       if (!(t = strchr(val, ' '))) goto error;

same

> +void svnload_read(void)
> +{
> +       char *t, *val;
> +       int len;
> +
> +       while ((t = buffer_read_line(&input))) {
> +               if ((val = strchr(t, ' ')))
> +                       *val++ = '\0';
> +               len = (val ? val - t - 1 : strlen(t));
> +
> +               switch (len) {
> +               case 1:
> +                       if (!memcmp(t, "D", 1)) {
> +                               node_ctx.action = NODE_ACTION_DELETE;
> +                       } else if (!memcmp(t, "C", 1)) {
> +                               node_ctx.action = NODE_ACTION_ADD;
> +                       } else if (!memcmp(t, "R", 1)) {
> +                               node_ctx.action = NODE_ACTION_REPLACE;
> +                       } else if (!memcmp(t, "M", 1)) {
> +                               if (!val) goto error;
> +                               node_ctx.action = NODE_ACTION_CHANGE;
> +                               val += parse_filemodify_mode(val);
> +                               if (!(t = strchr(val, ' '))) goto error;

same

Re: [PATCH 1/3] vcs-svn: Introduce svnload, a dumpfile producer

[Ramkumar Ramachandra]

Hi Erik,

Erik Faye-Lund writes:
> A very superficial review, because I don't have much time, and don't
> know the surrounding code well. Sorry about that.

Thanks. I have to often switch back and fourth between GNU-style (for
Subversion) and Linux-style (for Git), so please bear with my silly
style errors.

> On Tue, Feb 1, 2011 at 3:26 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> > +       if (!val) die("Malformed author line");
> > +       if (!(tz_off = strrchr(val, ' '))) goto error;
> > +       *tz_off++ = '\0';
> > +       if (!(t = strrchr(val, ' '))) goto error;
> 
> style: use
> "if (x)
> 	do_stuff();"
> instead of
> "if (x) do_stuff();"

This was intentional -- I'm only using it when do_stuff() is 'goto
error'. I thought it looked nicer that way.

-- Ram

Re: [PATCH 1/3] vcs-svn: Introduce svnload, a dumpfile producer

[Erik Faye-Lund]

On Wed, Feb 2, 2011 at 3:53 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Hi Erik,
>
> Erik Faye-Lund writes:
>> A very superficial review, because I don't have much time, and don't
>> know the surrounding code well. Sorry about that.
>
> Thanks. I have to often switch back and fourth between GNU-style (for
> Subversion) and Linux-style (for Git), so please bear with my silly
> style errors.
>

That's completely fair :)

>> On Tue, Feb 1, 2011 at 3:26 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
>> > +       if (!val) die("Malformed author line");
>> > +       if (!(tz_off = strrchr(val, ' '))) goto error;
>> > +       *tz_off++ = '\0';
>> > +       if (!(t = strrchr(val, ' '))) goto error;
>>
>> style: use
>> "if (x)
>>       do_stuff();"
>> instead of
>> "if (x) do_stuff();"
>
> This was intentional -- I'm only using it when do_stuff() is 'goto
> error'. I thought it looked nicer that way.
>

This is still not the style we use in git:

$ git grep "if (" | grep goto | wc -l
      0
$ git grep -B1 "if (" | grep goto | wc -l
     33

[PATCH 2/3] t9010-svn-fi: Add tests for svn-fi

[Ramkumar Ramachandra]

Create a test-svn-fi in toplevel directory, add rules to build it, and
add some basic tests.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 .gitignore        |    1 +
 Makefile          |    5 +-
 t/t9010-svn-fi.sh |  303 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 test-svn-fi.c     |   20 ++++
 4 files changed, 328 insertions(+), 1 deletions(-)
 create mode 100644 t/t9010-svn-fi.sh
 create mode 100644 test-svn-fi.c

diff --git a/.gitignore b/.gitignore
index b48d1ee..c8c8a17 100644
--- a/.gitignore
+++ b/.gitignore
@@ -177,6 +177,7 @@
 /test-sigchain
 /test-subprocess
 /test-svn-fe
+/test-svn-fi
 /common-cmds.h
 *.tar.gz
 *.dsc
diff --git a/Makefile b/Makefile
index d9c2442..cb21b78 100644
--- a/Makefile
+++ b/Makefile
@@ -431,6 +431,7 @@ TEST_PROGRAMS_NEED_X += test-sha1
 TEST_PROGRAMS_NEED_X += test-sigchain
 TEST_PROGRAMS_NEED_X += test-subprocess
 TEST_PROGRAMS_NEED_X += test-svn-fe
+TEST_PROGRAMS_NEED_X += test-svn-fi
 TEST_PROGRAMS_NEED_X += test-index-version
 
 TEST_PROGRAMS = $(patsubst %,%$X,$(TEST_PROGRAMS_NEED_X))
@@ -1838,7 +1839,7 @@ VCSSVN_OBJS = vcs-svn/line_buffer.o vcs-svn/svnload.o vcs-svn/dump_export.o \
 	vcs-svn/repo_tree.o vcs-svn/fast_export.o vcs-svn/sliding_window.o \
 	vcs-svn/svndiff.o vcs-svn/svndump.o vcs-svn/dir_cache.o
 VCSSVN_TEST_OBJS = test-obj-pool.o \
-	test-line-buffer.o test-treap.o test-svn-fe.o
+	test-line-buffer.o test-treap.o test-svn-fe.o test-svn-fi.o
 OBJECTS := $(GIT_OBJS) $(XDIFF_OBJS) $(VCSSVN_OBJS)
 
 dep_files := $(foreach f,$(OBJECTS),$(dir $f).depend/$(notdir $f).d)
@@ -2129,6 +2130,8 @@ test-parse-options$X: parse-options.o
 
 test-svn-fe$X: vcs-svn/lib.a
 
+test-svn-fi$X: vcs-svn/lib.a
+
 .PRECIOUS: $(TEST_OBJS)
 
 test-%$X: test-%.o $(GITLIBS)
diff --git a/t/t9010-svn-fi.sh b/t/t9010-svn-fi.sh
new file mode 100644
index 0000000..676c7fc
--- /dev/null
+++ b/t/t9010-svn-fi.sh
@@ -0,0 +1,303 @@
+#!/bin/sh
+
+test_description='check svn dumpfile exporter'
+
+. ./test-lib.sh
+
+if ! svnadmin -h >/dev/null 2>&1
+then
+	skip_all='skipping svn-fi tests, svn not available'
+	test_done
+fi
+
+svnrepo="testsvn"
+
+reinit_svn () {
+	rm -rf "$svnrepo" &&
+	rm -f stream &&
+	svnadmin create "$svnrepo" &&
+	printf "#!/bin/sh" > "$svnrepo"/hooks/pre-revprop-change &&
+	chmod +x "$svnrepo"/hooks/pre-revprop-change &&
+	mkfifo stream
+}
+
+svn_look () {
+	subcommand=$1 &&
+	shift &&
+	svnlook "$subcommand" "$svnrepo" "$@"
+}
+
+try_load () {
+	input=$1 &&
+	maybe_fail=${2:+test_$2} &&
+
+	{
+		$maybe_fail test-svn-fi "$input" >stream &
+	} &&
+	svnadmin load "$svnrepo" <stream &&
+	wait $!
+}
+
+test_expect_success 'normal empty files' '
+	reinit_svn &&
+	cat >expect.tree <<-\EOF &&
+	/
+	 foo
+	 bar
+	EOF
+	cat >input <<-\EOF &&
+	reset refs/heads/master
+	commit refs/heads/master
+	mark :1
+	author nobody <nobody@localhost> 1170199019 +0100
+	committer nobody <nobody@localhost> 1170199019 +0100
+	data 0
+	M 100644 inline foo
+	data 0
+	M 644 inline bar
+	data 0
+
+	EOF
+	try_load input &&
+	svn_look tree >actual.tree &&
+	test_cmp expect.tree actual.tree
+'
+
+# TODO: How to test date? Need to convert from local timestamp
+test_expect_success 'svn:author and svn:log' '
+	reinit_svn &&
+	echo "nothing" >expect.log &&
+	echo "nobody" >expect.author &&
+	cat >input <<-\EOF &&
+	reset refs/heads/master
+	commit refs/heads/master
+	mark :1
+	author nobody <nobody@localhost> 1170199019 +0100
+	committer nobody <nobody@localhost> 1170199019 +0100
+	data 7
+	nothing
+	M 100644 inline foo
+	data 0
+
+	EOF
+	try_load input &&
+	svn_look log >actual.log &&
+	svn_look author >actual.author &&
+	test_cmp expect.log actual.log &&
+	test_cmp expect.author actual.author
+'
+
+test_expect_success 'missing author line' '
+	reinit_svn &&
+	cat >expect.tree <<-\EOF &&
+	/
+	 foo
+	EOF
+	cat >input <<-\EOF &&
+	reset refs/heads/master
+	commit refs/heads/master
+	mark :1
+	committer nobody <nobody@localhost> 1170199019 +0100
+	data 0
+	M 100644 inline foo
+	data 0
+
+	EOF
+	try_load input &&
+	svn_look tree >actual.tree &&
+	test_cmp expect.tree actual.tree
+'
+
+test_expect_success 'blob marks unsupported' '
+	reinit_svn &&
+	cat >input <<-\EOF &&
+	blob
+	mark :1
+	data 0
+
+	reset refs/heads/master
+	commit refs/heads/master
+	mark :2
+	author nobody <nobody@localhost> 1170199019 +0100
+	committer nobody <nobody@localhost> 1170199019 +0100
+	data 0
+	M 100644 :1 foo
+
+	EOF
+	try_load input must_fail
+'
+
+test_expect_success 'malformed fast-import stream: filemodify' '
+	reinit_svn &&
+	cat >input <<-\EOF &&
+	reset refs/heads/master
+	commit refs/heads/master
+	mark :1
+	author nobody <nobody@localhost> 1170199019 +0100
+	committer nobody <nobody@localhost> 1170199019 +0100
+	data 0
+	M 100644 inline
+
+	EOF
+	try_load input must_fail
+'
+
+test_expect_success 'malformed fast-import stream: author' '
+	reinit_svn &&
+	cat >input <<-\EOF &&
+	reset refs/heads/master
+	commit refs/heads/master
+	mark :1
+	author 2d3%*s&f#k|
+	committer nobody <nobody@localhost> 1170199019 +0100
+	data 0
+	M 100644 inline foo
+	data 0
+
+	EOF
+	try_load input must_fail
+'
+
+test_expect_success 'malformed fast-import stream: author 2' '
+	reinit_svn &&
+	cat >input <<-\EOF &&
+	reset refs/heads/master
+	commit refs/heads/master
+	mark :1
+	author nobody <localhost> 1170199019 +0100
+	committer nobody <nobody@localhost> 1170199019 +0100
+	data 0
+	M 100644 inline foo
+	data 0
+
+	EOF
+	try_load input must_fail
+'
+
+test_expect_success 'malformed fast-import stream: data length' '
+	reinit_svn &&
+	cat >input <<-\EOF &&
+	reset refs/heads/master
+	commit refs/heads/master
+	mark :1
+	author nobody <nobody@localhost> 1170199019 +0100
+	committer nobody <nobody@localhost> 1170199019 +0100
+	data 0
+	M 100644 inline foo
+	data 14238
+
+	EOF
+	test_must_fail try_load input
+'
+
+test_expect_success 'recursive directory creation' '
+	reinit_svn &&
+	cat >expect.tree <<-\EOF &&
+	/
+	 alpha/
+	  beta/
+	   gamma
+	EOF
+	cat >input <<-\EOF &&
+	reset refs/heads/master
+	commit refs/heads/master
+	mark :1
+	author nobody <nobody@localhost> 1170199019 +0100
+	committer nobody <nobody@localhost> 1170199019 +0100
+	data 7
+	nothing
+	M 100644 inline alpha/beta/gamma
+	data 12
+	some content
+
+	EOF
+	try_load input &&
+	svn_look tree >actual.tree &&
+	test_cmp expect.tree actual.tree
+'
+
+test_expect_success 'svn:special and svn:executable' '
+	reinit_svn &&
+	cat >input <<-\EOF &&
+	reset refs/heads/master
+	commit refs/heads/master
+	mark :1
+	author nobody <nobody@localhost> 1170199019 +0100
+	committer nobody <nobody@localhost> 1170199019 +0100
+	data 7
+	nothing
+	M 100755 inline foo
+	data 0
+	M 755 inline moo
+	data 0
+	M 120000 inline bar
+	data 0
+
+	EOF
+	try_load input &&
+	svn_look propget svn:executable foo &&
+	svn_look propget svn:executable moo &&
+	svn_look propget svn:special bar
+'
+
+test_expect_success 'replace symlink with normal file' '
+	reinit_svn &&
+	cat >expect.tree <<-\EOF &&
+	/
+	 alpha/
+	  beta/
+	   gamma
+	EOF
+	cat >input <<-\EOF &&
+	reset refs/heads/master
+	commit refs/heads/master
+	mark :1
+	author nobody <nobody@localhost> 1170199019 +0100
+	committer nobody <nobody@localhost> 1170199019 +0100
+	data 7
+	nothing
+	M 120000 inline alpha/beta/gamma
+	data 0
+	commit refs/heads/master
+	mark :1
+	author nobody <nobody@localhost> 1170199019 +0100
+	committer nobody <nobody@localhost> 1170199019 +0100
+	data 7
+	nothing
+	M 100644 inline alpha/beta/gamma
+	data 0
+
+	EOF
+	try_load input &&
+	svn_look tree -r1 >actual.tree1 &&
+	svn_look tree -r2 >actual.tree2 &&
+	test_cmp expect.tree actual.tree1 &&
+	test_cmp expect.tree actual.tree2
+'
+
+test_expect_success 'path includes symlink' '
+	reinit_svn &&
+	cat >input <<-\EOF &&
+	reset refs/heads/master
+	commit refs/heads/master
+	mark :1
+	author nobody <nobody@localhost> 1170199019 +0100
+	committer nobody <nobody@localhost> 1170199019 +0100
+	data 7
+	nothing
+	M 120000 inline alpha/beta/gamma
+	data 0
+	commit refs/heads/master
+	mark :1
+	author nobody <nobody@localhost> 1170199019 +0100
+	committer nobody <nobody@localhost> 1170199019 +0100
+	data 7
+	nothing
+	M 100644 inline alpha/beta/gamma/bar
+	data 0
+
+	EOF
+	test_must_fail try_load input
+'
+
+test_done
diff --git a/test-svn-fi.c b/test-svn-fi.c
new file mode 100644
index 0000000..b0605fe
--- /dev/null
+++ b/test-svn-fi.c
@@ -0,0 +1,20 @@
+/*
+ * test-svn-fe: Code to exercise the svn import lib
+ */
+
+#include "git-compat-util.h"
+#include "vcs-svn/svnload.h"
+
+int main(int argc, char *argv[])
+{
+	static const char test_svnfe_usage[] =
+		"test-svn-fe (<dumpfile>";
+	if (argc == 2) {
+		if (svnload_init(argv[1]))
+			return 1;
+		svnload_read();
+		svnload_deinit();
+		return 0;
+	}
+	usage(test_svnfe_usage);
+}
-- 
1.7.4.rc1.7.g2cf08.dirty

Re: [PATCH 2/3] t9010-svn-fi: Add tests for svn-fi

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Create a test-svn-fi in toplevel directory, add rules to build it, and
> add some basic tests.

Thanks.  Probably this should be squashed with patch #1?

> --- /dev/null
> +++ b/t/t9010-svn-fi.sh

Test number's already taken.  9012 is free, though.

> @@ -0,0 +1,303 @@
> +#!/bin/sh
> +
> +test_description='check svn dumpfile exporter'
> +
> +. ./test-lib.sh
> +
> +if ! svnadmin -h >/dev/null 2>&1
> +then
> +	skip_all='skipping svn-fi tests, svn not available'
> +	test_done
> +fi

Is it impossible to test without svn present or otherwise speed
these up?  Some random ideas:

 - feed svn-fi output into svn-fe and compare git repos
 - check if "svn-fi after svn-fe" is idempotent (should it be?)
 - check for individual salient features from svn-fi output, using
   "grep", "sed", or "awk" to ignore the rest
 - use svnrdump in place of "svnadmin load" if available (is it
   faster?)
 - find the bottleneck in svnadmin and fix it

Of course at least one test of the "svn-fe | svnadmin load"
pipeline seems worthwhile; I'm just thinking we should avoid
habits that slow down the test suite too much.

> +svn_look () {
> +	subcommand=$1 &&
> +	shift &&
> +	svnlook "$subcommand" "$svnrepo" "$@"
> +}

Needed?  The svn_cmd function exists to point to a configuration
directory.

> +test_expect_success 'normal empty files' '
> +	reinit_svn &&
> +	cat >expect.tree <<-\EOF &&
> +	/
> +	 foo
> +	 bar
> +	EOF
> +	cat >input <<-\EOF &&
> +	reset refs/heads/master
> +	commit refs/heads/master
> +	mark :1
> +	author nobody <nobody@localhost> 1170199019 +0100
> +	committer nobody <nobody@localhost> 1170199019 +0100

Where do the dates come from?  Why should I (the reader) expect
them?

> +# TODO: How to test date? Need to convert from local timestamp

Ah, you saw. :)

One possibility is to fuzz away whatever is unimportant when
comparing results (and using tools like "awk" to extract the
date to perform whatever tests on it are appropriate where it
matters).

> +test_expect_success 'malformed fast-import stream: author 2' '
> +	reinit_svn &&
> +	cat >input <<-\EOF &&
> +	reset refs/heads/master
> +	commit refs/heads/master
> +	mark :1
> +	author nobody <localhost> 1170199019 +0100
> +	committer nobody <nobody@localhost> 1170199019 +0100
> +	data 0
> +	M 100644 inline foo
> +	data 0
> +
> +	EOF
> +	try_load input must_fail

What's wrong with this stream?

> +test_expect_success 'svn:special and svn:executable' '
> +	reinit_svn &&
> +	cat >input <<-\EOF &&
> +	reset refs/heads/master
> +	commit refs/heads/master
> +	mark :1
> +	author nobody <nobody@localhost> 1170199019 +0100
> +	committer nobody <nobody@localhost> 1170199019 +0100
> +	data 7
> +	nothing
> +	M 100755 inline foo
> +	data 0
> +	M 755 inline moo
> +	data 0
> +	M 120000 inline bar
> +	data 0
> +
> +	EOF
> +	try_load input &&
> +	svn_look propget svn:executable foo &&
> +	svn_look propget svn:executable moo &&
> +	svn_look propget svn:special bar

Maybe worth checking for the absence of svn:executable +
svn:special from an ordinary file, too?

> +test_expect_success 'replace symlink with normal file' '

Nice.  "replace symlink with executable" and "replace symlink
with directory" might also be interesting.

Regards,
Jonathan

Re: [PATCH 2/3] t9010-svn-fi: Add tests for svn-fi

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> 
> > Create a test-svn-fi in toplevel directory, add rules to build it, and
> > add some basic tests.
> 
> Thanks.  Probably this should be squashed with patch #1?

Right. Will do.

> > --- /dev/null
> > +++ b/t/t9010-svn-fi.sh
> 
> Test number's already taken.  9012 is free, though.

Ok. Will rename in the next iteration.

> > @@ -0,0 +1,303 @@
> > +#!/bin/sh
> > +
> > +test_description='check svn dumpfile exporter'
> > +
> > +. ./test-lib.sh
> > +
> > +if ! svnadmin -h >/dev/null 2>&1
> > +then
> > +	skip_all='skipping svn-fi tests, svn not available'
> > +	test_done
> > +fi
> 
> Is it impossible to test without svn present or otherwise speed
> these up?  Some random ideas:
> 
>  - feed svn-fi output into svn-fe and compare git repos
>  - check if "svn-fi after svn-fe" is idempotent (should it be?)
>  - check for individual salient features from svn-fi output, using
>    "grep", "sed", or "awk" to ignore the rest
>  - use svnrdump in place of "svnadmin load" if available (is it
>    faster?)
>  - find the bottleneck in svnadmin and fix it
> 
> Of course at least one test of the "svn-fe | svnadmin load"
> pipeline seems worthwhile; I'm just thinking we should avoid
> habits that slow down the test suite too much.

Hm, I'm not entirely convinced -- the final verdict is always dictated
by whether or not the emitted dumpstream loads. Many streams that look
alright to the eye don't actually load because of small intricacies
like missing newlines. It'll be awfully complicated reverse-engineer
all these intricate rules and write them in terms of sed/ awk
commands. Yes, svnrdump will be faster, but it'll only be available
with the later versions of Subversion (same problem with fixing
svnadmin bottlenecks).

> > +svn_look () {
> > +	subcommand=$1 &&
> > +	shift &&
> > +	svnlook "$subcommand" "$svnrepo" "$@"
> > +}
> 
> Needed?  The svn_cmd function exists to point to a configuration
> directory.

Just a convinience: I am using svn_look to look at the properties of
nodes in several tests. If I don't use this in enough tests, I'll
factor it out.

> > +test_expect_success 'normal empty files' '
> > +	reinit_svn &&
> > +	cat >expect.tree <<-\EOF &&
> > +	/
> > +	 foo
> > +	 bar
> > +	EOF
> > +	cat >input <<-\EOF &&
> > +	reset refs/heads/master
> > +	commit refs/heads/master
> > +	mark :1
> > +	author nobody <nobody@localhost> 1170199019 +0100
> > +	committer nobody <nobody@localhost> 1170199019 +0100
> 
> Where do the dates come from?  Why should I (the reader) expect
> them?

What do you suggest? I need some valid "generic" timestamp + offset.

> > +# TODO: How to test date? Need to convert from local timestamp
> 
> Ah, you saw. :)
> 
> One possibility is to fuzz away whatever is unimportant when
> comparing results (and using tools like "awk" to extract the
> date to perform whatever tests on it are appropriate where it
> matters).

Okay, I'll try this. Is there no other way? Can we whip up a shell
script to convert the timestamp?

> > +test_expect_success 'malformed fast-import stream: author 2' '
> > +	reinit_svn &&
> > +	cat >input <<-\EOF &&
> > +	reset refs/heads/master
> > +	commit refs/heads/master
> > +	mark :1
> > +	author nobody <localhost> 1170199019 +0100
> > +	committer nobody <nobody@localhost> 1170199019 +0100
> > +	data 0
> > +	M 100644 inline foo
> > +	data 0
> > +
> > +	EOF
> > +	try_load input must_fail
> 
> What's wrong with this stream?

author nobody <localhost>
                        ^
Parse error there, since build_svn_author tries to use the part of the
email address appearing before the '@'. This is just a temporary test
-- we should make the svn_author generation logic configurable.

> > +test_expect_success 'svn:special and svn:executable' '
> > +	reinit_svn &&
> > +	cat >input <<-\EOF &&
> > +	reset refs/heads/master
> > +	commit refs/heads/master
> > +	mark :1
> > +	author nobody <nobody@localhost> 1170199019 +0100
> > +	committer nobody <nobody@localhost> 1170199019 +0100
> > +	data 7
> > +	nothing
> > +	M 100755 inline foo
> > +	data 0
> > +	M 755 inline moo
> > +	data 0
> > +	M 120000 inline bar
> > +	data 0
> > +
> > +	EOF
> > +	try_load input &&
> > +	svn_look propget svn:executable foo &&
> > +	svn_look propget svn:executable moo &&
> > +	svn_look propget svn:special bar
> 
> Maybe worth checking for the absence of svn:executable +
> svn:special from an ordinary file, too?

Good idea.

> > +test_expect_success 'replace symlink with normal file' '
> 
> Nice.  "replace symlink with executable" and "replace symlink
> with directory" might also be interesting.

Good idea. Will do.

-- Ram

Re: [PATCH 2/3] t9010-svn-fi: Add tests for svn-fi

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Jonathan Nieder writes:
>> Ramkumar Ramachandra wrote:

>> Is it impossible to test without svn present or otherwise speed
>> these up?  Some random ideas:
[...]
> Hm, I'm not entirely convinced -- the final verdict is always dictated
> by whether or not the emitted dumpstream loads. Many streams that look
> alright to the eye don't actually load because of small intricacies
> like missing newlines. It'll be awfully complicated reverse-engineer
> all these intricate rules and write them in terms of sed/ awk
> commands. Yes, svnrdump will be faster, but it'll only be available
> with the later versions of Subversion (same problem with fixing
> svnadmin bottlenecks).

If svnrdump is fast enough, wouldn't something like

 if svnrdump --help >/dev/null 2>&1
 then
	load_dump () {
		svnrdump load "file://$1"
	}
	test_set_prereq SVN
 elif svnadmin --help >/dev/null 2>&1
 then
	load_dump () {
		svnadmin load "$1"
	}
	test_set_prereq SVN
 else
	: no usable svn installation
 fi

do it?

>>> +svn_look () {
>>> +	subcommand=$1 &&
>>> +	shift &&
>>> +	svnlook "$subcommand" "$svnrepo" "$@"
>>> +}
[...]
> Just a convinience: I am using svn_look to look at the properties of
> nodes in several tests.

Ah, I missed the implicit $svnrepo argument.  Makes sense then (might
be good to have a comment to explain the purpose, though).

>>> +test_expect_success 'normal empty files' '
>>> +	reinit_svn &&
>>> +	cat >expect.tree <<-\EOF &&
>>> +	/
>>> +	 foo
>>> +	 bar
>>> +	EOF
>>> +	cat >input <<-\EOF &&
>>> +	reset refs/heads/master
>>> +	commit refs/heads/master
>>> +	mark :1
>>> +	author nobody <nobody@localhost> 1170199019 +0100
>>> +	committer nobody <nobody@localhost> 1170199019 +0100
>>
>> Where do the dates come from?  Why should I (the reader) expect
>> them?
>
> What do you suggest? I need some valid "generic" timestamp + offset.

	test_tick
	...
	author nobody <nobody@localhost> $GIT_AUTHOR_DATE

[...]
> Okay, I'll try this. Is there no other way? Can we whip up a shell
> script to convert the timestamp?

Maybe the standard "date" utility can help with conversions?

I'm not sure what kinds of validation you're doing on the dates, hence
the vague answers.  For examples of fuzzing out the unimportant bits,

 git grep fuzz t/*.sh

can help.

>>> +test_expect_success 'malformed fast-import stream: author 2' '
[...]
>>> +	try_load input must_fail
>> 
>> What's wrong with this stream?
>
> author nobody <localhost>
>                         ^
> Parse error there, since build_svn_author tries to use the part of the
> email address appearing before the '@'. This is just a temporary test
> -- we should make the svn_author generation logic configurable.

So the stream is not malformed?  In that case, I guess the
intent is

 test_expect_failure 'does something reasonable with email address without @' '
	...
	try_load input
 '

Thanks again.
Jonathan

[PATCH 3/3] vcs-svn: Refactor dump_export code into dispatch table

[Ramkumar Ramachandra]

Suggested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 vcs-svn/dump_export.c |   89 +++++++++++++++++++++++++------------------------
 vcs-svn/dump_export.h |    6 ++-
 2 files changed, 49 insertions(+), 46 deletions(-)

diff --git a/vcs-svn/dump_export.c b/vcs-svn/dump_export.c
index 2b23f77..a8331fd 100644
--- a/vcs-svn/dump_export.c
+++ b/vcs-svn/dump_export.c
@@ -11,6 +11,48 @@
 
 static struct strbuf props;
 
+typedef void state_fn(void);
+
+static void Kfile   (void) { printf("Node-kind: file\n"); }
+static void Kdir    (void) { printf("Node-kind: dir\n"); }
+static void Achange (void) { printf("Node-action: change\n"); }
+static void Aadd    (void) { printf("Node-action: add\n"); }
+static void Adelete (void) { printf("Node-action: delete\n"); }
+static void Areplace(void) { printf("Node-action: replace\n"); }
+static void Pexec   (void) { strbuf_addf(&props, "K 14\nsvn:executable\nV 1\n*\n"); }
+static void Psym    (void) { strbuf_addf(&props, "K 11\nsvn:special\nV 1\n*\n"); }
+static void Pend    (void) { strbuf_add(&props, "PROPS-END\n", 10); }
+
+static void Nchange (void) { Kfile(); Achange(); }
+static void Nadd    (void) { Kfile(); Aadd(); }
+static void Nreplace(void) { Kfile(); Areplace(); }
+static void Echange (void) { Pexec(); Pend(); Kfile(); Achange(); }
+static void Eadd    (void) { Pexec(); Pend(); Kfile(); Aadd(); }
+static void Ereplace(void) { Pexec(); Pend(); Kfile(); Areplace(); }
+static void Schange (void) { Psym(); Pend(); Kfile(); Achange(); }
+static void Sadd    (void) { Psym(); Pend(); Kfile(); Aadd(); }
+static void Sreplace(void) { Psym(); Pend(); Kfile(); Areplace(); }
+static void Dchange (void) { Kdir(); Achange(); }
+static void Dadd    (void) { Kdir(); Aadd(); }
+static void Dreplace(void) { Kdir(); Areplace(); }
+
+static state_fn *const dispatch_table[NODE_KIND_COUNT][NODE_ACTION_COUNT] = {
+	/* NODE_KIND_UNKNOWN */
+	{abort, abort, abort, Adelete, abort},
+	/* NODE_KIND_NORMAL */
+	{abort, Nchange, Nadd, Adelete, Nreplace},
+	/* NODE_KIND_EXECUTABLE */
+	{abort, Echange, Eadd, Adelete, Ereplace},
+	/* NODE_KIND_SYMLINK */
+	{abort, Schange, Sadd, Adelete, Sreplace},
+	/* NODE_KIND_GITLINK */
+	{abort, abort, abort, abort, abort},
+	/* NODE_KIND_DIR */
+	{abort, Dchange, Dadd, Adelete, Dreplace},
+	/* NODE_KIND_SUBDIR */
+	{abort, abort, abort, abort, abort}
+};
+
 void dump_export_begin_rev(int revision, const char *revprops,
 			int prop_len)
 {
@@ -24,56 +66,15 @@ void dump_export_node(const char *path, enum node_kind kind,
 		enum node_action action, unsigned long text_len,
 		unsigned long copyfrom_rev, const char *copyfrom_path)
 {
-	int dump_props = 1; /* Boolean */
 	strbuf_reset(&props);
 	printf("Node-path: %s\n", path);
-	switch (kind) {
-	case NODE_KIND_NORMAL:
-		printf("Node-kind: file\n");
-		break;
-	case NODE_KIND_EXECUTABLE:
-		printf("Node-kind: file\n");
-		strbuf_addf(&props, "K 14\nsvn:executable\nV 1\n*\n");
-		break;
-	case NODE_KIND_SYMLINK:
-		printf("Node-kind: file\n");
-		strbuf_addf(&props, "K 11\nsvn:special\nV 1\n*\n");
-		break;
-	case NODE_KIND_GITLINK:
-		printf("Node-kind: file\n");
-		break;
-	case NODE_KIND_DIR:
-		printf("Node-kind: dir\n");
-		break;
-	case NODE_KIND_SUBDIR:
-		die("Unsupported: subdirectory");
-	default:
-		break;
-	}
-	strbuf_add(&props, "PROPS-END\n", 10);
+	dispatch_table[kind][action]();
 
-	switch (action) {
-	case NODE_ACTION_CHANGE:
-		printf("Node-action: change\n");
-		break;
-	case NODE_ACTION_ADD:
-		printf("Node-action: add\n");
-		break;
-	case NODE_ACTION_REPLACE:
-		printf("Node-action: replace\n");
-		break;
-	case NODE_ACTION_DELETE:
-		printf("Node-action: delete\n");
-		dump_props = 0;
-		break;
-	default:
-		break;
-	}
 	if (copyfrom_rev != SVN_INVALID_REV) {
 		printf("Node-copyfrom-rev: %lu\n", copyfrom_rev);
 		printf("Node-copyfrom-path: %s\n", copyfrom_path);
 	}
-	if (dump_props) {
+	if (props.len) {
 		printf("Prop-delta: false\n");
 		printf("Prop-content-length: %lu\n", props.len);
 	}
@@ -81,7 +82,7 @@ void dump_export_node(const char *path, enum node_kind kind,
 		printf("Text-delta: false\n");		
 		printf("Text-content-length: %lu\n", text_len);
 	}
-	if (text_len || dump_props) {
+	if (text_len || props.len) {
 		printf("Content-length: %lu\n\n", text_len + props.len);
 		printf("%s", props.buf);
 	}
diff --git a/vcs-svn/dump_export.h b/vcs-svn/dump_export.h
index e9f51a3..8265170 100644
--- a/vcs-svn/dump_export.h
+++ b/vcs-svn/dump_export.h
@@ -9,7 +9,8 @@ enum node_action {
 	NODE_ACTION_CHANGE,
 	NODE_ACTION_ADD,
 	NODE_ACTION_DELETE,
-	NODE_ACTION_REPLACE
+	NODE_ACTION_REPLACE,
+	NODE_ACTION_COUNT
 };
 
 enum node_kind {
@@ -19,7 +20,8 @@ enum node_kind {
 	NODE_KIND_SYMLINK,
 	NODE_KIND_GITLINK,
 	NODE_KIND_DIR,           /* SVN-specific */
-	NODE_KIND_SUBDIR
+	NODE_KIND_SUBDIR,
+	NODE_KIND_COUNT
 };
 
 void dump_export_begin_rev(int revision, const char *revprops, int prop_len);
-- 
1.7.4.rc1.7.g2cf08.dirty

Re: [PATCH 3/3] vcs-svn: Refactor dump_export code into dispatch table

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> +++ b/vcs-svn/dump_export.c
> @@ -11,6 +11,48 @@
[...]
> +static state_fn *const dispatch_table[NODE_KIND_COUNT][NODE_ACTION_COUNT] = {
> +	/* NODE_KIND_UNKNOWN */
> +	{abort, abort, abort, Adelete, abort},
> +	/* NODE_KIND_NORMAL */
> +	{abort, Nchange, Nadd, Adelete, Nreplace},
> +	/* NODE_KIND_EXECUTABLE */
> +	{abort, Echange, Eadd, Adelete, Ereplace},
> +	/* NODE_KIND_SYMLINK */
> +	{abort, Schange, Sadd, Adelete, Sreplace},
> +	/* NODE_KIND_GITLINK */
> +	{abort, abort, abort, abort, abort},
> +	/* NODE_KIND_DIR */
> +	{abort, Dchange, Dadd, Adelete, Dreplace},
> +	/* NODE_KIND_SUBDIR */
> +	{abort, abort, abort, abort, abort}
> +};

Heh.  I think that Junio was suggesting making the _parser_
table-driven, meaning something like

	... node_kinds[] = {
		{ "100644", sizeof("100644"), "file" },
		{ "100755", sizeof("100755"), "file", "svn:executable" },
		{ "120000", sizeof("120000"), "file", "svn:special" },
		{ "160000", sizeof("160000"), "file" },	/* NEEDSWORK: seems wrong" */
		{ "040000", sizeof("040000"), "dir" }
	};

(Side note: remember that 644 and 755 are permitted synonyms for
100644 and 100755!)

I personally think that simple state machines tend to be easier to
follow if the current state is represented by the instruction pointer
rather than a variable, as in the current fast-import.c.  But maybe
that's a matter of taste?

Anyway, my other complaints about this dispatch_table are that the
function names leave me rubbing my head and I can't keep the list
of states you're transitioning between straight in my head.  I guess
"Adelete" is an abbreviation for print_delete_node_action?  Is a
callback needed at all (rather than just a string) for such actions?

Hope that helps,
Jonathan

Re: [PATCH 3/3] vcs-svn: Refactor dump_export code into dispatch table

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Heh.  I think that Junio was suggesting making the _parser_
> table-driven, meaning something like
>
> 	... node_kinds[] = {
> 		{ "100644", sizeof("100644"), "file" },
> ...
> 	};

Yes, and thanks.

Re: [PATCH 3/3] vcs-svn: Refactor dump_export code into dispatch table

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> 
> > +++ b/vcs-svn/dump_export.c
> > @@ -11,6 +11,48 @@
> [...]
> > +static state_fn *const dispatch_table[NODE_KIND_COUNT][NODE_ACTION_COUNT] = {
> > +	/* NODE_KIND_UNKNOWN */
> > +	{abort, abort, abort, Adelete, abort},
> > +	/* NODE_KIND_NORMAL */
> > +	{abort, Nchange, Nadd, Adelete, Nreplace},
> > +	/* NODE_KIND_EXECUTABLE */
> > +	{abort, Echange, Eadd, Adelete, Ereplace},
> > +	/* NODE_KIND_SYMLINK */
> > +	{abort, Schange, Sadd, Adelete, Sreplace},
> > +	/* NODE_KIND_GITLINK */
> > +	{abort, abort, abort, abort, abort},
> > +	/* NODE_KIND_DIR */
> > +	{abort, Dchange, Dadd, Adelete, Dreplace},
> > +	/* NODE_KIND_SUBDIR */
> > +	{abort, abort, abort, abort, abort}
> > +};
> 
> Heh.  I think that Junio was suggesting making the _parser_
> table-driven, meaning something like

Oops :p I'll fix this in the next round.

> 	... node_kinds[] = {
> 		{ "100644", sizeof("100644"), "file" },
> 		{ "100755", sizeof("100755"), "file", "svn:executable" },
> 		{ "120000", sizeof("120000"), "file", "svn:special" },
> 		{ "160000", sizeof("160000"), "file" },	/* NEEDSWORK: seems wrong" */
> 		{ "040000", sizeof("040000"), "dir" }
> 	};
> 
> (Side note: remember that 644 and 755 are permitted synonyms for
> 100644 and 100755!)
> 
> I personally think that simple state machines tend to be easier to
> follow if the current state is represented by the instruction pointer
> rather than a variable, as in the current fast-import.c.  But maybe
> that's a matter of taste?
> 
> Anyway, my other complaints about this dispatch_table are that the
> function names leave me rubbing my head and I can't keep the list
> of states you're transitioning between straight in my head.  I guess
> "Adelete" is an abbreviation for print_delete_node_action?  Is a
> callback needed at all (rather than just a string) for such actions?

True -- there are many intermediate layers where all kinds of
variables are getting set. It'll probably be a good idea to remove all
these abstraction layers and map fast-import commands directly to
dumpstream strings. I'll do this in the next iteration.

-- Ram