threads / patch / 26379

v3, 3 partsTowards a Git-to-SVN bridge

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

## tl;dr

13 messages between Feb 1, 2011 and Feb 2, 2011. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

Ramkumar Ramachandra· Feb 1, 2011, 14:26 UTC · lore
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
Ramkumar Ramachandra· Feb 1, 2011, 14:26 UTC · re: Ramkumar Ramachandra · lore

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

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
Show changes to 7 files +568 −2

Makefile, vcs-svn/dir_cache.c, vcs-svn/dir_cache.h, vcs-svn/dump_export.c, vcs-svn/dump_export.h, vcs-svn/svnload.c, 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
Erik Faye-Lund· Feb 1, 2011, 14:46 UTC · re: Ramkumar Ramachandra · lore

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

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:
Show 20 quoted lines
> 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.
Show 20 quoted lines
> +       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.
Show 10 quoted lines
> +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
Show 5 quoted lines
> +int parse_filemodify_mode(char *val)
> +{
> +       char *t;
> +
> +       if (!(t = strchr(val, ' '))) goto error;
same
Show 23 quoted lines
> +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
Ramkumar Ramachandra· Feb 2, 2011, 02:53 UTC · re: Erik Faye-Lund · lore

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

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.

Show 11 quoted lines
> 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
Erik Faye-Lund· Feb 2, 2011, 12:43 UTC · re: Ramkumar Ramachandra · lore

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

On Wed, Feb 2, 2011 at 3:53 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
Show 10 quoted lines
> 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 :)
Show 15 quoted lines
>> 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
Ramkumar Ramachandra· Feb 1, 2011, 14:26 UTC · re: Ramkumar Ramachandra · lore

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

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
Show changes to 4 files +328 −1

.gitignore, Makefile, t/t9010-svn-fi.sh, 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
Jonathan Nieder· Feb 1, 2011, 18:58 UTC · re: Ramkumar Ramachandra · lore

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

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.
Show 12 quoted lines
> @@ -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.

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

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

Show 13 quoted lines
> +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).

Show 14 quoted lines
> +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?
Show 22 quoted lines
> +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

Ramkumar Ramachandra· Feb 2, 2011, 02:49 UTC · re: Jonathan Nieder · lore

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

Hi Jonathan,
Jonathan Nieder writes:
Show 6 quoted lines
> 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.
Show 27 quoted lines
> > @@ -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).

Show 8 quoted lines
> > +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.

Show 16 quoted lines
> > +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.
Show 8 quoted lines
> > +# 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?

Show 16 quoted lines
> > +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.
Show 25 quoted lines
> > +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
Jonathan Nieder· Feb 2, 2011, 03:18 UTC · re: Ramkumar Ramachandra · lore

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

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:
[...]
Show 8 quoted lines
> 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?
Show 5 quoted lines
>>> +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).

Show 18 quoted lines
>>> +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' '
[...]
Show 9 quoted lines
>>> +	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

Ramkumar Ramachandra· Feb 1, 2011, 14:26 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 2 files +49 −46

vcs-svn/dump_export.c, vcs-svn/dump_export.h

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
Jonathan Nieder· Feb 1, 2011, 17:42 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra wrote:
> +++ b/vcs-svn/dump_export.c
> @@ -11,6 +11,48 @@
[...]
Show 16 quoted lines
> +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

Junio C Hamano· Feb 1, 2011, 21:29 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 7 quoted lines
> Heh.  I think that Junio was suggesting making the _parser_
> table-driven, meaning something like
>
> 	... node_kinds[] = {
> 		{ "100644", sizeof("100644"), "file" },
> ...
> 	};
Yes, and thanks.
Ramkumar Ramachandra· Feb 2, 2011, 02:56 UTC · re: Jonathan Nieder · lore

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

Hi Jonathan,
Jonathan Nieder writes:
Show 24 quoted lines
> 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.
Show 21 quoted lines
> 	... 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

← back to recent threads