Re: [PATCH 5/6] Add infrastructure to write revisions in fast-export format

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jun 4, 2010, 19:02 UTC
Message-ID: <20100604190222.GB21295@progeny.tock>
In-Reply-To: <1275658871-1473-6-git-send-email-artagnon@gmail.com>

Hi Ram,

Thanks for the style cleanup.

Ramkumar Ramachandra wrote:

> --- /dev/null
> +++ b/vcs-svn/repo_tree.c

[...]

Show 20 quoted lines

> +static struct repo_dir *repo_clone_dir(struct repo_dir *orig_dir, uint32_t padding)
> +{
> +	uint32_t orig_o, new_o, dirent_o;
> +	orig_o = dir_offset(orig_dir);
> +	if (orig_o < num_dirs_saved) {
> +		new_o = dir_with_dirents_alloc(orig_dir->size + padding);
> +		orig_dir = dir_pointer(orig_o);
> +		dirent_o = dir_pointer(new_o)->first_offset;
> +	} else {
> +		if (padding == 0)
> +			return orig_dir;
> +		new_o = orig_o;
> +		dirent_o = dirent_alloc(orig_dir->size + padding);
> +	}
> +	memcpy(dirent_pointer(dirent_o), repo_first_dirent(orig_dir),
> +		   orig_dir->size * sizeof(struct repo_dirent));
> +	dir_pointer(new_o)->size = orig_dir->size + padding;
> +	dir_pointer(new_o)->first_offset = dirent_o;
> +	return dir_pointer(new_o);
> +}

This function expands a list of directory entries, copying it if already committed as part of a previous revision.

The big downside is that as David mentioned it does not scale well with the size of the directory and number of expansions. But a patch is in the pipeline to fix that. I do not think it should hold the series up.

Show 16 quoted lines

> +
> +static struct repo_dirent *repo_read_dirent(uint32_t revision, uint32_t *path)
> +{
> +	uint32_t name = 0;
> +	struct repo_dir *dir = NULL;
> +	struct repo_dirent *dirent = NULL;
> +	dir = repo_commit_root_dir(commit_pointer(revision));
> +	while (~(name = *path++)) {
> +		dirent = repo_dirent_by_name(dir, name);
> +		if (dirent == NULL) {
> +			return NULL;
> +		} else if (repo_dirent_is_dir(dirent)) {
> +			dir = repo_dir_from_dirent(dirent);
> +		} else {
> +			break;
> +		}

Style: would probably be clearer to write:

	while (~(name = *path++)) {
		dirent = repo_dirent_by_name(dir, name);
		if (!dirent || !repo_dirent_is_dir(dirent))
			break;
		dir = repo_dir_from_dirent(dirent);
	}

i.e., fewer unnecessary braces, and dealing with the exceptional cases separately from the normal case.

As before, I wonder about the error cases. Might it make sense to report the error if someone tries to copy a nonexistent directory from a previous revision?

> +static void
> +repo_write_dirent(uint32_t *path, uint32_t mode, uint32_t content_offset,
> +                  uint32_t del)

Function is too long for my taste (I realize this is a matter of taste). The innermost blocks would make sense as functions in their own right.

[...]

> +static void repo_diff_r(uint32_t depth, uint32_t *path, struct repo_dir *dir1,
> +			struct repo_dir *dir2)

[...]

Show 12 quoted lines

> +	while (de1 < max_de1 && de2 < max_de2) {
> +		if (de1->name_offset < de2->name_offset) {
> +			path[depth] = (de1++)->name_offset;
> +			fast_export_delete(depth + 1, path);
> +		} else if (de1->name_offset > de2->name_offset) {
> +			path[depth] = de2->name_offset;
> +			repo_git_add(depth + 1, path, de2++);
> +		} else {
> +			path[depth] = de1->name_offset;
> +			if (de1->mode != de2->mode ||
> +				de1->content_offset != de2->content_offset) {
> +				if (repo_dirent_is_dir(de1) && repo_dirent_is_dir(de2)) {

My 80-column terminal is suffering.  Why not use the common

	while (...) {
		if (exceptional case) {
			....
			continue;
		} else if (exceptional case) {
			...
			continue;
		}
		path[depth] = de1->name_offset;
		if (easy case) {
			...
			continue;
		}
		usual case
	}

pattern?

> +#define REPO_MAX_PATH_LEN 4096
> +#define REPO_MAX_PATH_DEPTH 1000

These limits are not checked; is the caller supposed to check them itself? Does svn obey them?

Jonathan