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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html