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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi Ram,

Thanks for the style cleanup.

Ramkumar Ramachandra wrote:

> --- /dev/null
> +++ b/vcs-svn/repo_tree.c
[...]
> +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.

> +
> +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)
[...]
> +	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