Re: [PATCH 5/6] Add infrastructure to write revisions in fast-export format
From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:48:54
Hi Ram, Thanks for the style cleanup. Ramkumar Ramachandra wrote:
quoted hunk ↗ jump to hunk
--- /dev/null +++ b/vcs-svn/repo_tree.c
[...]
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ +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?
quoted hunk ↗ jump to hunk
+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)
[...]
quoted hunk ↗ jump to hunk
+ 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