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

[Ramkumar Ramachandra <artagnon@gmail.com>]

Hi Jonathan,

Jonathan Nieder wrote:
> 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.

Right. The `dirents` branch is now merged.

> 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.

This was (probably unintentionally) re-factored by David when merging
in his `dirents` branch.

> 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?


> 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.

This came up in your previous review- I tried to split it up into more
functions, but due to the number of local variables, it turned out to
be a mess and I had to revert. Would you like to try re-factoring it?

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

Re-factored (again, probably unintentionally) by David during the merge.

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

I asked David too, and as far as we know, these limits are pretty
arbitrary. They're no enforced by SVN or any specific filesystem. We
can discuss about having less arbitrary limits and checking it in
svndump.c (while parsing the dump).

-- Ram