Re: [PATCH 8/8] Add SVN dump parser

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 15, 2010, 19:52 UTC
Message-ID: <20100715195206.GE2774@burratino>
In-Reply-To: <1279210984-31604-9-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra wrote:

> svndump parses data that is in SVN dumpfile format produced by
> `svnadmin dump` with the help of line_buffer and uses repo_tree and
> fast_export to emit a git fast-import stream.

Probably worth mentioning the this requires a dumpfile v2 (i.e., it does not understand the svndiff0 delta format yet).

Show 11 quoted lines

> +/* Create memory pool for log messages */
> +obj_pool_gen(log, char, 4096);
> +
> +static char* log_copy(uint32_t length, char *log)
> +{
> +	char *buffer;
> +	log_free(log_pool.size);
> +	buffer = log_pointer(log_alloc(length));
> +	strncpy(buffer, log, length);
> +	return buffer;
> +}

A strbuf would do just as well. But using obj_pool looks like a fine way to avoid depending on another piece of git code.

Show 6 quoted lines

> +static struct {
> +	uint32_t svn_log, svn_author, svn_date, svn_executable, svn_special, uuid,
> +		revision_number, node_path, node_kind, node_action,
> +		node_copyfrom_path, node_copyfrom_rev, text_content_length,
> +		prop_content_length, content_length;
> +} keys;

Neat. This is a textbook example of where to use a perfect hash, but comparing interned strings is simpler and fast enough (and the bottlenecks are elsewhere).

> +static void read_props(void)

[...]

Show 14 quoted lines

> +			if (key == keys.svn_log) {
> +				/* Value length excludes terminating nul. */
> +				rev_ctx.log = log_copy(len + 1, val);
> +			} else if (key == keys.svn_author) {
> +				rev_ctx.author = pool_intern(val);
> +			} else if (key == keys.svn_date) {
> +				if (parse_date_basic(val, &rev_ctx.timestamp, NULL))
> +					fprintf(stderr, "Invalid timestamp: %s\n", val);
> +			} else if (key == keys.svn_executable) {
> +				node_ctx.type = REPO_MODE_EXE;
> +			} else if (key == keys.svn_special) {
> +				node_ctx.type = REPO_MODE_LNK;
> +			}
> +			key = ~0;

Unknown properties are ignored. Adding stream comments to allow recovering them is left as an exercise for the interested reader.

> +static void handle_node(void)
> +{

A simple reader does not cope well with this kind of function. It is hard to know if it exhaustively deals with all cases.

But: with real-world repos (e.g. ASF) it works well enough.

> +void svndump_read(char *url)
> +{

Too long. I realize that writing a state machine can be hard in C; maybe it would be easiest to package up the state in a struct and have a separate function for the main loop body.

The patches I didn’t comment on all look good. I don’t think anything I did comment on should prevent this reaching a wider audience.

Thanks for the pleasant read, Jonathan