Greg Hudson wrote:
> * I invented the term "view" to describe the source and target
> substrings accessible to a window. Calling them the "source
> window" and "target window" seemed confusing given how we
> use the term "window" already.
That's a good idea, I like it.
> * Branko is going to have to do a little bit of code reorg
> when it comes time to be able to marshal and unmarshal
> deltas to vcdiff format. The existing text delta interface
> and data structures (independent of this patch) is pretty
> fixed on the idea that deltas are only created from source
> and target files, not read in from an external source.
Did you look at the VCDIFF->text delta declarations in
include/svn_delta.h? Unless I misunderstood completely, they're meant to
read an external delta stream and produce text delta windows.
> * I decided to make the applicator function use a generic
> output function, so I had to add that concept to svn_io.h.
Make sense. I was thinking along similar lines.
> I also wrote some wrappers for full reads and full writes,
> since generic read and write functions appear to be allowed
> to return short lengths for arbitrary reasons.
IMO it would be better to impose that restriction on the generic
read/write functions. I know APR has both interfaces, but I'm not sure
Subversion needs partial read/write capability. The various reader and
writer implemntations (for files, sockets, etc.) are the best place to
implement any restart/recovery functionality, which will probably depend
quite a lot on the actual stream type.
> * The vdelta code was already generating instruction offsets
> relative to the source window offset.
How else. :-)
> * window->tview_len is kind of redundant; you could compute it
> easily by summing the lengths of all the instructions in the
> window. But I think it makes a good check.
Leave it in. An extra check is always a good thing, unless it's too
expensive. this one isn't. (O.K., VCDIFF window headers don't include
that information, but it can be calcuated on the fly during conversion.)
>+ The source view must always slide forward from one window to the
>+ next; that is, neither the beginning nor the end of the source view
>+ may move to the left as we read from a window stream. This
>+ property allows us to apply deltas to non-seekable source streams
>+ without making a full copy of the source stream. */
This was a very necessary bit of documentation. Thanks.
>+#ifndef MAX
>+#define MAX(x, y) ((x) > (y) ? (x) : (y))
>+#endif
I must say I absolutely hate seeing min/max macros, especially if
they're defined outside common headers ... in this case, you're using it
exactly twice in almost identical bits of code. Why don't you factor out
those three lines of code into a static inline function?
static APR_INLINE apr_size_t
alloc_buffer (apr_off_t view_len, apr_size_t buf_size,
char **buf, apr_pool_t *pool)
{
apr_size_t new_size = 2 * buf_size;
if (new_size < view_len)
new_size = view_len;
*buf = apr_palloc (pool, new_size);
return new_size;
}
(BTW, you put "int new_size" in the bit of code where you allocate the
target buffer; should be apr_size_t. Also, it looks as if tview_len and
sview_len in the window should be apr_size_t, too.)
Right, that' my EUR 0.02.
Brane
Received on Sat Oct 21 14:36:08 2006