On Sat, 2005-02-12 at 00:22 +0000, Philip Martin wrote:
> Daniel Berlin <dberlin@dberlin.org> writes:
>
> Just a few quick comments on the implementation:
>
> > +/* This is pseudo-adler32, and xdelta, as monotone does it. The
> > + adler32 has no prime modulus. This is a direct translation of the
> > + C++ code, hence the reason it looks a bit weird. */
>
> monotone is GPL isn't it, can a "direct translation" be released under
> the Subversion licence? I think Adler32 is widely used under a
> variety of licences, not so sure about xdelta.
Yes it is gpl, and xdelta is used under a wide variety of licenses.
Also, yes, you can release a direct translation under the subversion
license.
It's also not copyrightable in it's current state anyway, for various
reasons (literally just computes a mathemtical formula in the most
direct way possible).
His xdelta is just a translation of the algorithm in the xdelta paper
anyway.
It was translated from monotone, but that's because i didn't have
ghostview handy when i wrote it.
The paper describes it in basically the exact same terms (in terms of an
init_matches function, a compute_delta function, and a find_match
function that do exactly what we do).
In other words, this is a complete non-issue.
Regardless, for the purposes of removing all doubt, I have acquired
Graydon Hoare's (the original author of the untranslated C++ code in
question) specific permission to use and distribute the translated code
under the subversion license terms, and have noted the date and time he
gave it (this is sufficient for legal purposes, he'd be estopped from
claiming otherwise unless he can prove it didn't happen). I'm sure
graydon would be happy to confirm this or whatever.
> We sort of target C89 and don't use inline, how about using APR_INLINE?
Already fixed in the attached new version.
I noticed that.
> > + /* Extend the match forward as far as possible */
> > + while ((apos + alen >= 0)
> > + && (bpos + badvance >= 0)
>
> ../svn/subversion/libsvn_delta/vdelta.c: In function 'find_match':
> ../svn/subversion/libsvn_delta/vdelta.c:452: warning: comparison of unsigned expression >= 0 is always true
> ../svn/subversion/libsvn_delta/vdelta.c:453: warning: comparison of unsigned expression >= 0 is always true
Yes, fixed by removing the useless tests.
> > +
> > +/* Size of the blocks we compute checksums for. */
> > +#define MATCH_BLOCKSIZE 64
>
> Add some rationale for 64 please, even "64 is a guess" or an explicit
> "64 is what monotone uses" would do.
Will do.
I've attached a new patch
I've also moved the routine into it's own file, xdelta.c, and made the
approriate changes to call the right routine from compute_window.
New log:
Add xdelta algorithm, and use it by default
* subversion/libsvn_delta/xdelta.c: New file, xdelta algorithm.
* subversion/libsvn_delta/delta.h: Add svn_txdelta__xdelta prototype.
* subversion/libsvn_delta/text_delta.c (compute_window): Default to
xdelta.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Feb 12 02:32:42 2005