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

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Jun 7, 2010, 12:35 UTC
Message-ID: <AANLkTilqFNqGj8rq3fHZld6ddkHLp_usQSQKgQqeAt0X@mail.gmail.com>
In-Reply-To: <20100604190222.GB21295@progeny.tock>

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.

Show 11 quoted lines

> 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