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

From: David Michael Barr <david.barr@cordelta.com>
Date: Jun 7, 2010, 13:36 UTC
Message-ID: <B78F4B61-E006-47CD-9186-EF81BE5CDAC7@cordelta.com>
In-Reply-To: <AANLkTilqFNqGj8rq3fHZld6ddkHLp_usQSQKgQqeAt0X@mail.gmail.com>

Hi Ram,

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

Oh no, it was intentional.

>> My 80-column terminal is suffering.  Why not use the common
>> pattern?
> 
> Re-factored (again, probably unintentionally) by David during the merge.

As above, totally intentional.

@Jonathan: Thanks for the review, I took it into account when refactoring dirents.

Show 7 quoted lines

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

I am indeed in favour of less arbitrary limits. The current magic numbers were chosen as common practical bounds.