Greg Hudson wrote:
>
> When you get back, I have a number of comments on vdelta.c.
>
> The algorithm itself looks correct to me, although it could generate
> short copies (less than four bytes) in the edge cases. The code might
> come out a little simpler and not generate short copies if, instead of
> having a single processing run with a source/target barrier, you have
> one processing run for the source data and another one for the target
> data (both runs using the same hash table, of course). That was my
> experience with my mockup, although I wasn't handling the detail of
> combining insert instructions.
>
> Your instruction generation interface isn't necessarily a great fit
> for vcdiff. vcdiff doesn't distinguish between "copy from source" and
> "copy from target"; it just has "copy from the abstract string which
> is the concatenation of the source and the target."
I want to avoid specifics of vcdiff in the internal format. The way I
see it, the internal representation should be tailored to suit subversion's
needs. Vcdiff-related optimizations should be done in the conversion
functions, not in the delta generator.
> I feel like your hash table could be simplified and improved.
[snip]
> I realize your hash table and management routines are a bit more
> generic whereas the ones I visualize only supports the particular
> operations needed by vdelta. I think in this case we want a tailored
> data structure with only the stuff we need.
Indeed. I just used a generic hash table I had lying around. I plan to
optimize that part, but wanted to get to grips with the actual vdelta
generator first.
Anyway, judging by the vdelta info that came in last week, I'll have
to rewrite all of this, anyway ...
> [The useful content of this mail pretty much ends here. The rest
> devolves into a fine point of coding style.]
>
> I ran into a style difference with the way you declare pointer
> variables and the way the rest of the subversion code appears to. You
> write:
>
> char const* key;
Yup, that's my C++ background rearing its head. Will fix that.
> which I've seen a lot from programmers who wish C had a sane type
> declaration syntax; "*" is, after all, part of the type, not the
> variable. However, C doesn't work that way, and if you or any naive
> future programmer changes that line to:
>
> char const* key, value;
That naive future programmer forgot the const, too, not just the *.
(Both of which will be caught by the compiler.)
> Anyway, regardless of the merits, the subversion code should be
> stylistically consistent.
I agree. I'll try to keep my habits in check.
Brane
--
Branko Čibej <branko.cibej@hermes.si>
HERMES SoftLab, Litijska 51, 1000 Ljubljana, Slovenia
voice: (+386 1) 586 53 49 fax: (+386 1) 586 52 70
Received on Sat Oct 21 14:36:07 2006