You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Hudson <gh...@MIT.EDU> on 2001/03/01 01:25:44 UTC
Re: CVS update: subversion/subversion/include svn_delta.h
Ben committed:
> + When we call `svn_txdelta_next_window' on *STREAM, the window
> + returned will contain pointers directly into STRING (rather than
> + duplicating STRING's data.) So callers beware: make sure STRING
> + is not freed before STREAM and its windows!
You do realize that svn_txdelta_stream_t isn't currently a generic
interface, right? svn_txdelta_next_window always does the same thing,
which involves reading from source and target byte streams.
Re: CVS update: subversion/subversion/include svn_delta.h
Posted by Jim Blandy <ji...@zwingli.cygnus.com>.
Greg Stein <gs...@lyra.org> writes:
> Sure... I'd imagine those input variants would be handled by common routines
> in libsvn_delta. Whatever is on the "bottom" side of those would be handled
> by the FS.
X-actly.
Re: CVS update: subversion/subversion/include svn_delta.h
Posted by Greg Stein <gs...@lyra.org>.
Sure... I'd imagine those input variants would be handled by common routines
in libsvn_delta. Whatever is on the "bottom" side of those would be handled
by the FS.
Cheers,
-g
On Thu, Mar 01, 2001 at 11:15:27AM -0500, Jim Blandy wrote:
>
> Greg Stein <gs...@lyra.org> writes:
> > To the original question, I'd prefer to not deal with txdeltas and windows
> > and stuff. I want to give the FS a stream and a MIME type for that stream.
> > Let the FS sort out (based on content-type) whether to use txdelta or just
> > to drop the plain text into the node.
>
> We've discussed a similar situation before. I don't think the FS
> should deal with MIME types. It all needs to be put in a canonical
> form at some point, and I'm quite sure that point should be *outside*
> the filesystem interface. I have no problem adding any number of more
> specific text editing functions to the FS interface, but dealing with
> MIME issues is *not* part of the FS's contract.
>
> > So... the ideal interface for me can handle three forms of changing file
> > contents:
> >
> > 1) I feed it an entire plain text file in chunks (basically, I get a
> > writable svn_stream_t and drop data in)
> >
> > 2) I feed it an entire SVNDIFF in chunks (I'd write to an svn_stream and
> > something between the stream and the FS would interpret the SVNDIFF)
> >
> > 3) I feed it arbitrary ranges of plain text, with each range provided in
> > chunks (sort of a seek-write-write, seek-write pattern).
>
> I'm happy to add whatever functions you'd find helpful, as long as
> they've got a specific, well-defined behavior, and there's some reason
> it's beneficial for them to be inside the FS, instead of outside the
> FS and using some more generic interface to do their work.
--
Greg Stein, http://www.lyra.org/
Re: CVS update: subversion/subversion/include svn_delta.h
Posted by Jim Blandy <ji...@zwingli.cygnus.com>.
Greg Stein <gs...@lyra.org> writes:
> To the original question, I'd prefer to not deal with txdeltas and windows
> and stuff. I want to give the FS a stream and a MIME type for that stream.
> Let the FS sort out (based on content-type) whether to use txdelta or just
> to drop the plain text into the node.
We've discussed a similar situation before. I don't think the FS
should deal with MIME types. It all needs to be put in a canonical
form at some point, and I'm quite sure that point should be *outside*
the filesystem interface. I have no problem adding any number of more
specific text editing functions to the FS interface, but dealing with
MIME issues is *not* part of the FS's contract.
> So... the ideal interface for me can handle three forms of changing file
> contents:
>
> 1) I feed it an entire plain text file in chunks (basically, I get a
> writable svn_stream_t and drop data in)
>
> 2) I feed it an entire SVNDIFF in chunks (I'd write to an svn_stream and
> something between the stream and the FS would interpret the SVNDIFF)
>
> 3) I feed it arbitrary ranges of plain text, with each range provided in
> chunks (sort of a seek-write-write, seek-write pattern).
I'm happy to add whatever functions you'd find helpful, as long as
they've got a specific, well-defined behavior, and there's some reason
it's beneficial for them to be inside the FS, instead of outside the
FS and using some more generic interface to do their work.
Re: CVS update: subversion/subversion/include svn_delta.h
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Mar 01, 2001 at 05:37:27PM -0600, Sam TH wrote:
> On Thu, Mar 01, 2001 at 02:53:13PM -0600, Karl Fogel wrote:
> > Ben Collins-Sussman <su...@newton.ch.collab.net> writes:
> > > Oh, I just meant that if we ever change the definition of a struct,
> > > your static initialization would more likely break, compared to
> > > palloc'ing it.
> >
> > Well, if the definition of the struct changes, anyone constructing one
> > by hand will often have to compensate. I don't think that's affected
> > by whether it's allocated on the stack or in the heap. (In fact,
> > maybe the static initialization will generate a compiler warning if
> > not all fields are present? That would be nice.)
Gotta hope for type conflicts, actually.
But I agree that people will simply need to compensate. No biggy in my book.
> gcc, at least, generates such warnings.
Euh... it shouldn't generate a warning. Omitting fields at the end of a
structure is defined by ANSI C to set those fields to 0.
Is gcc warning for auto struct initializers, but not for globals?
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: CVS update: subversion/subversion/include svn_delta.h
Posted by Sam TH <sa...@uchicago.edu>.
On Thu, Mar 01, 2001 at 02:53:13PM -0600, Karl Fogel wrote:
> Ben Collins-Sussman <su...@newton.ch.collab.net> writes:
> > Oh, I just meant that if we ever change the definition of a struct,
> > your static initialization would more likely break, compared to
> > palloc'ing it.
>
> Well, if the definition of the struct changes, anyone constructing one
> by hand will often have to compensate. I don't think that's affected
> by whether it's allocated on the stack or in the heap. (In fact,
> maybe the static initialization will generate a compiler warning if
> not all fields are present? That would be nice.)
gcc, at least, generates such warnings.
sam th
sam@uchicago.edu
http://www.abisource.com/~sam/
GnuPG Key:
http://www.abisource.com/~sam/key
Re: CVS update: subversion/subversion/include svn_delta.h
Posted by Karl Fogel <kf...@galois.collab.net>.
Ben Collins-Sussman <su...@newton.ch.collab.net> writes:
> Oh, I just meant that if we ever change the definition of a struct,
> your static initialization would more likely break, compared to
> palloc'ing it.
Well, if the definition of the struct changes, anyone constructing one
by hand will often have to compensate. I don't think that's affected
by whether it's allocated on the stack or in the heap. (In fact,
maybe the static initialization will generate a compiler warning if
not all fields are present? That would be nice.)
-K
Re: CVS update: subversion/subversion/include svn_delta.h
Posted by Ben Collins-Sussman <su...@newton.ch.collab.net>.
Greg Stein <gs...@lyra.org> writes:
> > * I notice that you *statically* declare and initialize a window
> > struct, and even an svn_string_t. Isn't this a dangerous,
> > non-robust practice? :)
>
> Why would it be dangerous? Nobody can write outside of those bounds, and
> nobody should be attempting to free() the pointer.
Oh, I just meant that if we ever change the definition of a struct,
your static initialization would more likely break, compared to
palloc'ing it.
But who cares. :)
Re: CVS update: subversion/subversion/include svn_delta.h
Posted by Greg Stein <gs...@lyra.org>.
On Thu, Mar 01, 2001 at 11:10:54AM -0600, Ben Collins-Sussman wrote:
> Greg Stein <gs...@lyra.org> writes:
>
> > > svn_error_t *apply_string_as_delta (svn_string_t *string
> > > svn_txdelta_window_handler_t *handler,
> > > void *baton);
> > >
> > I have a function almost exactly like the above in libsvn_ra_dav/fetch.c.
> > Take a look at fetch_file_reader(). It happens to have a baton that carries
> > the "handler" and "baton" that GregH quoted above, but passing those
> > explicitly is easy. Then, it takes a buffer/length pair and applies that.
>
> Interesting... I may just steal your routine, GregS.
>
> Two questions about it:
>
> * I notice that you *statically* declare and initialize a window
> struct, and even an svn_string_t. Isn't this a dangerous,
> non-robust practice? :)
Why would it be dangerous? Nobody can write outside of those bounds, and
nobody should be attempting to free() the pointer.
It does point out, though, that the svn_txdelta_window_handler_t should take
a "const svn_txdelta_window_t" and that the "ops" member should be "const
svn_txdelta_op_t *".
Personally, I would also change new_data to "const char *" and a length.
> * Your function isn't pushing a 0 after pushing its window. Is that
> being done by the caller? Shouldn't it be done by your func?
0 signifies the end of stream. It isn't done after each window. Take a look
at the function again, though: if len==0, it does push a NULL.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: CVS update: subversion/subversion/include svn_delta.h
Posted by Ben Collins-Sussman <su...@newton.ch.collab.net>.
Greg Stein <gs...@lyra.org> writes:
> > svn_error_t *apply_string_as_delta (svn_string_t *string
> > svn_txdelta_window_handler_t *handler,
> > void *baton);
> >
> I have a function almost exactly like the above in libsvn_ra_dav/fetch.c.
> Take a look at fetch_file_reader(). It happens to have a baton that carries
> the "handler" and "baton" that GregH quoted above, but passing those
> explicitly is easy. Then, it takes a buffer/length pair and applies that.
Interesting... I may just steal your routine, GregS.
Two questions about it:
* I notice that you *statically* declare and initialize a window
struct, and even an svn_string_t. Isn't this a dangerous,
non-robust practice? :)
* Your function isn't pushing a 0 after pushing its window. Is that
being done by the caller? Shouldn't it be done by your func?
Re: CVS update: subversion/subversion/include svn_delta.h
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Feb 28, 2001 at 10:18:00PM -0500, Greg Hudson wrote:
> > Jim pointed out that I won't be the last person who wants to do
> > this, so I thought the prototype above would be a good general
> > solution: convert an svn_string_t into a txdelta_stream.
>
> Given our current machinery, what you would prototype would look
> something like:
>
> svn_error_t *apply_string_as_delta (svn_string_t *string
> svn_txdelta_window_handler_t *handler,
> void *baton);
>
> which would do the pushing itself, and then you'd pass the handler and
> baton you got from apply_textdelta to that routine.
>
> (You'd have to pick a better function name, though. A pool argument
> might be necessary, but I don't think so, since you'd just be
> constructing a single window and you could use stack storage for it.)
It might be necessary to pass a pool.
I have a function almost exactly like the above in libsvn_ra_dav/fetch.c.
Take a look at fetch_file_reader(). It happens to have a baton that carries
the "handler" and "baton" that GregH quoted above, but passing those
explicitly is easy. Then, it takes a buffer/length pair and applies that.
Inside, it constructs all the right doo-dads and calls the txdelta handler.
Note that a pool is referenced in the svn_txdelta_window_t structure. I
might argue that it shouldn't be there (since the handler/baton can always
create/destroy a subpool for temp allocations). I also happen to place a
pool into an svn_string_t, but that shouldn't be needed, as the string is
(theoretically) constant.
> > Instead, what if we had a func that converted an svn_string_t into a
> > *single* window containing a solitary `new' op? Then I could push
> > the one window, then push a NULL, and be done.
>
> This would also be fine.
I'd prefer the construct-on-stack approach. There isn't a lot of reason to
create windows on the heap.
To the original question, I'd prefer to not deal with txdeltas and windows
and stuff. I want to give the FS a stream and a MIME type for that stream.
Let the FS sort out (based on content-type) whether to use txdelta or just
to drop the plain text into the node.
The one gotcha that I have is the possibility for replacing portions of a
file. e.g. "place these 10 bytes in positions 53..62." The overall size
won't grow (unless they write at the end of the file), this would just be
patching (multiple) byte ranges. While this doesn't seem too bad, it gets a
bit uglier when you consider the byte ranges may arrive in arbitrary order
(i.e. we cannot guarantee that range2 is positioned after range1).
So... the ideal interface for me can handle three forms of changing file
contents:
1) I feed it an entire plain text file in chunks (basically, I get a
writable svn_stream_t and drop data in)
2) I feed it an entire SVNDIFF in chunks (I'd write to an svn_stream and
something between the stream and the FS would interpret the SVNDIFF)
3) I feed it arbitrary ranges of plain text, with each range provided in
chunks (sort of a seek-write-write, seek-write pattern).
The current txdelta interface in the FS supports this, I think, but there
would be a good chunk of code on my side for handling (2). For option (3), I
could construct windows that patch ranges. I seem to recall that delta
windows can only occur sequentially, so handling (3) may involve multiple
applications of a delta ("open. patch range 1. close. open. patch range 2").
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: CVS update: subversion/subversion/include svn_delta.h
Posted by Ben Collins-Sussman <su...@newton.ch.collab.net>.
Greg Hudson <gh...@MIT.EDU> writes:
> Ben committed:
> > + When we call `svn_txdelta_next_window' on *STREAM, the window
> > + returned will contain pointers directly into STRING (rather than
> > + duplicating STRING's data.) So callers beware: make sure STRING
> > + is not freed before STREAM and its windows!
>
> You do realize that svn_txdelta_stream_t isn't currently a generic
> interface, right? svn_txdelta_next_window always does the same thing,
> which involves reading from source and target byte streams.
Oh crud.
The problem I have is simple: I have an svn_string_t. I want to
stash its contents in a repository file, and the *only* interface for
this is svn_fs_apply_textdelta. As usual, this interface expects me
to push txdelta windows at a consumer routine.
This is an awful amount of work for such a seemingly simple task.
Jim pointed out that I won't be the last person who wants to do this,
so I thought the prototype above would be a good general solution:
convert an svn_string_t into a txdelta_stream. But Greg points out
that our machinery isn't generalized enough for this.
Instead, what if we had a func that converted an svn_string_t into a
*single* window containing a solitary `new' op? Then I could push the
one window, then push a NULL, and be done.
Thoughts?