Greg Hudson wrote:
> The biggest problem I see:
>
> - SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!)(?r)(?r)bb)", start, end,
> + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "!)(?r)(?r)bbr)", start, end,
>
> I know I left a little gap in the ra_svn infrastructure by not allowing
> 'n's in the optional part of a tuple, but you don't get to gloss over
> that by pretending that your limit argument is a revision. That's a big
> giant -1.
>
> I suggest introducing SVN_RA_SVN_UNSPECIFIED_NUMBER, #defined to
> ~((unsigned apr_uint64_t) 0), and using that as the distinguished value
> for an unspecified optional number.
That makes sense. I agree, the current way of doing it (passing a
revision) is a hack.
> Other issues:
>
> In libsvn_repos/log.c, I would add a counter to the for loop rather than
> adjusting start or end. It's more straightforward, and I'm a little
> concerned about the correctness of your code in light of the continue
> clause in the loop body. Since the loop conditions are already
> complicated, the counter check might be best written as an "if (++count
>>= limit) break;" (or whatever) in the loop body, after the continue
>
> clause has been given a chance to fire.
I'll take a look.
> I think the limit argument should just be an int, not a "long int". Our
> code base only very rarely uses the "long" type. The only real
> difference between "int" and "long" is that long is guaranteed to be 32
> bits, and if ints are 16 bits (you have to go back pretty far to find an
> environment like that), we're already very sunk.
It was a long int so I could use the code for passing around revisions
in ra_svn ;-) That can certainly go away once that hack is removed.
> In three places you wrote "compatability", which is a misspelling of
> "compatibility".
Oops. Good catch. I'm too used to working with a copy editor who
catches those sort of things for me ;-)
Thanks for the review!
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Sep 25 15:34:12 2004