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 Stein <gs...@gmail.com> on 2010/05/17 18:56:05 UTC
Re: svn commit: r945280 - in /subversion/trunk: build.conf
subversion/libsvn_ra_serf/commit.c subversion/mod_dav_svn/dav_svn.h
subversion/mod_dav_svn/posts/ subversion/mod_dav_svn/posts/create-transaction.c
subversion/mod_dav_svn/repos.c
More XML crap? Yuck, ugh, and bleagh.
On Mon, May 17, 2010 at 14:09, <cm...@apache.org> wrote:
>...
> +++ subversion/trunk/subversion/mod_dav_svn/posts/create-transaction.c Mon May 17 18:09:28 2010
>...
> +/* Respond to a S:dated-rev-report request. */
> +int
> +dav_svn__create_transaction_post(const dav_resource *resource,
> + const apr_xml_doc *doc,
> + ap_filter_t *output)
> +{
> + request_rec *r = resource->info->r;
> + apr_bucket_brigade *bb;
> + apr_status_t apr_err;
> + dav_error *derr = NULL;
> + const char *txn_name;
> +
> + /* Create a Subversion repository transaction based on HEAD, and
> + return the new transaction's name in a custom "201 Created"
> + response header. */
> + derr = dav_svn__create_txn(resource->info->repos, &txn_name, resource->pool);
> + if (derr)
> + return dav_svn__error_response_tag(r, derr);
This doesn't match the "int" return type you've defined, and it
results in build errors (see the buildbots).
>...
> + {
> + if (strcmp(doc->root->name, "create-transaction") == 0)
> + {
> + return dav_svn__create_transaction_post(resource, doc,
> + r->output_filters);
And this seems to want an int.
>...
Can I say again that I hate XML, and am very much -0.5 on using it in
our protocol like this? These bodies have nothing to conform to, so
there is no reason to use XML. If you're doing it for *expedience*
rather than *propriety*, then I'd suggest taking short cuts now could
hurt us in the long term. XML means we need to deal with escaping as
we move forward. It is overly wordy. Hard to write parsing code. etc
etc.
-g
Re: svn commit: r945280 - in /subversion/trunk: build.conf subversion/libsvn_ra_serf/commit.c
subversion/mod_dav_svn/dav_svn.h subversion/mod_dav_svn/posts/ subversion/mod_dav_svn/posts/create-transaction.c
subversion/mod_dav_svn/repos.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
Greg Stein wrote:
>> I'll revert what's been committed thus far, and possibly return again when I
>> find a sufficiently sized block of time.
>
> Yah, I was kinda surprised to see it go in there so quickly. Wasn't
> expecting other needs for the body in 1.7. But thanks for backing out
> the XML stuffs!
Well, one of the enterprise complaints we've heard is that, due to our
failure to support directory locks, folks tend to just use 'svn lock
--targets' on a whole bunch of files. The RA API supports locking/unlocking
multiple paths in one shot, but for the DAV cases, that devolves into a loop
around countless LOCK requests. I was hoping that HTTPv2 would allow us to
do a batch lock/unlock operations (and return a multi-status-ish thing
reporting the success/fail+errmsg for each path in the request). That would
have been low-hanging fruit in XML.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r945280 - in /subversion/trunk: build.conf
subversion/libsvn_ra_serf/commit.c subversion/mod_dav_svn/dav_svn.h
subversion/mod_dav_svn/posts/ subversion/mod_dav_svn/posts/create-transaction.c
subversion/mod_dav_svn/repos.c
Posted by Greg Stein <gs...@gmail.com>.
On Tue, May 18, 2010 at 13:31, C. Michael Pilato <cm...@collab.net> wrote:
> Greg Stein wrote:
>> On Mon, May 17, 2010 at 15:35, C. Michael Pilato <cm...@collab.net> wrote:
>>> Greg Stein wrote:
>>>>> I have a local patch here that adds a mod_dav_svn utility function for
>>>>> stringbuf'ing a request into a giant block of memory and parsing into a
>>>>> skel. I can certainly switch to that approach if you feel that strongly
>>>>> about it. Would you be okay with a documented requirement that these new
>>>>> POSTs use "text/plain", skel input, where the first atom of the skel is the
>>>>> POST operation verb ("create-transaction", "lock-paths", etc.)?
>>>> Much more preferable!
>>>>
>>>> And note that skels are application/octet-stream since they can encode
>>>> binary data (one the ways it will be easier to support!).
>>> The primary bit of misfortune about our skel library is that it isn't
>>> conducive to piecemeal generation of skel-string output. You can't really
>>> say "print a string that contains a list opener" and then "print the
>>> following N list items" and then "print the list closure". Obviously, this
>>> speaks merely to the immaturity of the library (and a bit to its use thus
>>> far only for relatively small pieces of information), so "it's a simple
>>> matter of programming" to fix. :-)
>>
>> Happy to help out, if you can start sketching something (I'm hoping to
>> do in-db props this week).
>
> Unfortunately, after looking more at this, I think the effort required has
> crossed my threshold of pain tolerance. I haven't the cycles to completely
> create from scratch a skel-based HTTP protocol which can be incrementally
> generated, streamily parsed, deal with errors which might need to be
> embedded willy nilly into the stream, maintains some degree of human
> readability, etc, merely for the sake of not extending the use of our
> existing XML-based protocol that already does all those things (and which
> will have to be forever maintained for compatibility anyway).
I hear ya, and that's why I suggested "copy the body into a $MAX-sized
buffer, then parse it". Setting MAX to (say) 64k would provide lots of
room for whatever is being requested. And should we need a larger
size, code for fancy parsing could be done in the future.
> I totally understand where you're coming from about XML's limitations, and I
> respect your experience and opinions on the matter. But all I'm seeing at
> the moment is the perfect being the enemy of the good (enough).
Understood, and while I might tend to agree... it is hard to get away
from protocols. We can rewrite code all we want to move from "good
enough" up to "perfect". Protocols don't have that luxury,
unfortunately. We would always be stuck with "good enough" even in the
face of "I have time to make it 'perfect', but no longer can..."
Thus, my concern about taking a shortcut today.
> I'll revert what's been committed thus far, and possibly return again when I
> find a sufficiently sized block of time.
Yah, I was kinda surprised to see it go in there so quickly. Wasn't
expecting other needs for the body in 1.7. But thanks for backing out
the XML stuffs!
Cheers,
-g
Re: svn commit: r945280 - in /subversion/trunk: build.conf subversion/libsvn_ra_serf/commit.c
subversion/mod_dav_svn/dav_svn.h subversion/mod_dav_svn/posts/ subversion/mod_dav_svn/posts/create-transaction.c
subversion/mod_dav_svn/repos.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
Greg Stein wrote:
> On Mon, May 17, 2010 at 15:35, C. Michael Pilato <cm...@collab.net> wrote:
>> Greg Stein wrote:
>>>> I have a local patch here that adds a mod_dav_svn utility function for
>>>> stringbuf'ing a request into a giant block of memory and parsing into a
>>>> skel. I can certainly switch to that approach if you feel that strongly
>>>> about it. Would you be okay with a documented requirement that these new
>>>> POSTs use "text/plain", skel input, where the first atom of the skel is the
>>>> POST operation verb ("create-transaction", "lock-paths", etc.)?
>>> Much more preferable!
>>>
>>> And note that skels are application/octet-stream since they can encode
>>> binary data (one the ways it will be easier to support!).
>> The primary bit of misfortune about our skel library is that it isn't
>> conducive to piecemeal generation of skel-string output. You can't really
>> say "print a string that contains a list opener" and then "print the
>> following N list items" and then "print the list closure". Obviously, this
>> speaks merely to the immaturity of the library (and a bit to its use thus
>> far only for relatively small pieces of information), so "it's a simple
>> matter of programming" to fix. :-)
>
> Happy to help out, if you can start sketching something (I'm hoping to
> do in-db props this week).
Unfortunately, after looking more at this, I think the effort required has
crossed my threshold of pain tolerance. I haven't the cycles to completely
create from scratch a skel-based HTTP protocol which can be incrementally
generated, streamily parsed, deal with errors which might need to be
embedded willy nilly into the stream, maintains some degree of human
readability, etc, merely for the sake of not extending the use of our
existing XML-based protocol that already does all those things (and which
will have to be forever maintained for compatibility anyway).
I totally understand where you're coming from about XML's limitations, and I
respect your experience and opinions on the matter. But all I'm seeing at
the moment is the perfect being the enemy of the good (enough).
I'll revert what's been committed thus far, and possibly return again when I
find a sufficiently sized block of time.
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r945280 - in /subversion/trunk: build.conf
subversion/libsvn_ra_serf/commit.c subversion/mod_dav_svn/dav_svn.h
subversion/mod_dav_svn/posts/ subversion/mod_dav_svn/posts/create-transaction.c
subversion/mod_dav_svn/repos.c
Posted by Greg Stein <gs...@gmail.com>.
On Mon, May 17, 2010 at 15:35, C. Michael Pilato <cm...@collab.net> wrote:
> Greg Stein wrote:
>>> I have a local patch here that adds a mod_dav_svn utility function for
>>> stringbuf'ing a request into a giant block of memory and parsing into a
>>> skel. I can certainly switch to that approach if you feel that strongly
>>> about it. Would you be okay with a documented requirement that these new
>>> POSTs use "text/plain", skel input, where the first atom of the skel is the
>>> POST operation verb ("create-transaction", "lock-paths", etc.)?
>>
>> Much more preferable!
>>
>> And note that skels are application/octet-stream since they can encode
>> binary data (one the ways it will be easier to support!).
>
> The primary bit of misfortune about our skel library is that it isn't
> conducive to piecemeal generation of skel-string output. You can't really
> say "print a string that contains a list opener" and then "print the
> following N list items" and then "print the list closure". Obviously, this
> speaks merely to the immaturity of the library (and a bit to its use thus
> far only for relatively small pieces of information), so "it's a simple
> matter of programming" to fix. :-)
Happy to help out, if you can start sketching something (I'm hoping to
do in-db props this week).
Cheers,
-g
Re: svn commit: r945280 - in /subversion/trunk: build.conf subversion/libsvn_ra_serf/commit.c
subversion/mod_dav_svn/dav_svn.h subversion/mod_dav_svn/posts/ subversion/mod_dav_svn/posts/create-transaction.c
subversion/mod_dav_svn/repos.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
Greg Stein wrote:
>> I have a local patch here that adds a mod_dav_svn utility function for
>> stringbuf'ing a request into a giant block of memory and parsing into a
>> skel. I can certainly switch to that approach if you feel that strongly
>> about it. Would you be okay with a documented requirement that these new
>> POSTs use "text/plain", skel input, where the first atom of the skel is the
>> POST operation verb ("create-transaction", "lock-paths", etc.)?
>
> Much more preferable!
>
> And note that skels are application/octet-stream since they can encode
> binary data (one the ways it will be easier to support!).
The primary bit of misfortune about our skel library is that it isn't
conducive to piecemeal generation of skel-string output. You can't really
say "print a string that contains a list opener" and then "print the
following N list items" and then "print the list closure". Obviously, this
speaks merely to the immaturity of the library (and a bit to its use thus
far only for relatively small pieces of information), so "it's a simple
matter of programming" to fix. :-)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r945280 - in /subversion/trunk: build.conf
subversion/libsvn_ra_serf/commit.c subversion/mod_dav_svn/dav_svn.h
subversion/mod_dav_svn/posts/ subversion/mod_dav_svn/posts/create-transaction.c
subversion/mod_dav_svn/repos.c
Posted by Greg Stein <gs...@gmail.com>.
On Mon, May 17, 2010 at 15:05, C. Michael Pilato <cm...@collab.net> wrote:
>...
>> This doesn't match the "int" return type you've defined, and it
>> results in build errors (see the buildbots).
>
> Hrm. This from util.c:
>
> int
> dav_svn__error_response_tag(request_rec *r,
> dav_error *err)
> {
> ...
Oh. Hmm. Thought it was that, but I don't have the code checked out
yet, nor the line numbers to align.
> I'll look into this (and the other similarly unexplainable) return mismatch
> you mentioned.
Cool.
Seems that it is just the windows compiler being strict, but that is a
Good Thing!! :-)
On your dev checkout, gcc should have lots of flags/warnings, so it
may pop right out at you.
>> Can I say again that I hate XML, and am very much -0.5 on using it in
>> our protocol like this? These bodies have nothing to conform to, so
>> there is no reason to use XML. If you're doing it for *expedience*
>> rather than *propriety*, then I'd suggest taking short cuts now could
>> hurt us in the long term. XML means we need to deal with escaping as
>> we move forward. It is overly wordy. Hard to write parsing code. etc
>> etc.
>
> I have a local patch here that adds a mod_dav_svn utility function for
> stringbuf'ing a request into a giant block of memory and parsing into a
> skel. I can certainly switch to that approach if you feel that strongly
> about it. Would you be okay with a documented requirement that these new
> POSTs use "text/plain", skel input, where the first atom of the skel is the
> POST operation verb ("create-transaction", "lock-paths", etc.)?
Much more preferable!
And note that skels are application/octet-stream since they can encode
binary data (one the ways it will be easier to support!).
Cheers,
-g
Re: svn commit: r945280 - in /subversion/trunk: build.conf subversion/libsvn_ra_serf/commit.c
subversion/mod_dav_svn/dav_svn.h subversion/mod_dav_svn/posts/ subversion/mod_dav_svn/posts/create-transaction.c
subversion/mod_dav_svn/repos.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
C. Michael Pilato wrote:
> Greg Stein wrote:
>> More XML crap? Yuck, ugh, and bleagh.
>>
>> On Mon, May 17, 2010 at 14:09, <cm...@apache.org> wrote:
>>> ...
>>> +++ subversion/trunk/subversion/mod_dav_svn/posts/create-transaction.c Mon May 17 18:09:28 2010
>>> ...
>>> +/* Respond to a S:dated-rev-report request. */
>>> +int
>>> +dav_svn__create_transaction_post(const dav_resource *resource,
>>> + const apr_xml_doc *doc,
>>> + ap_filter_t *output)
>>> +{
>>> + request_rec *r = resource->info->r;
>>> + apr_bucket_brigade *bb;
>>> + apr_status_t apr_err;
>>> + dav_error *derr = NULL;
>>> + const char *txn_name;
>>> +
>>> + /* Create a Subversion repository transaction based on HEAD, and
>>> + return the new transaction's name in a custom "201 Created"
>>> + response header. */
>>> + derr = dav_svn__create_txn(resource->info->repos, &txn_name, resource->pool);
>>> + if (derr)
>>> + return dav_svn__error_response_tag(r, derr);
>> This doesn't match the "int" return type you've defined, and it
>> results in build errors (see the buildbots).
>
> Hrm. This from util.c:
>
> int
> dav_svn__error_response_tag(request_rec *r,
> dav_error *err)
> {
> ...
>
> I'll look into this (and the other similarly unexplainable) return mismatch
> you mentioned.
>
>> Can I say again that I hate XML, and am very much -0.5 on using it in
>> our protocol like this? These bodies have nothing to conform to, so
>> there is no reason to use XML. If you're doing it for *expedience*
>> rather than *propriety*, then I'd suggest taking short cuts now could
>> hurt us in the long term. XML means we need to deal with escaping as
>> we move forward. It is overly wordy. Hard to write parsing code. etc
>> etc.
>
> I have a local patch here that adds a mod_dav_svn utility function for
> stringbuf'ing a request into a giant block of memory and parsing into a
> skel. I can certainly switch to that approach if you feel that strongly
> about it. Would you be okay with a documented requirement that these new
> POSTs use "text/plain", skel input, where the first atom of the skel is the
> POST operation verb ("create-transaction", "lock-paths", etc.)?
Actually, scratch the "text/plain" bit. We send non-human-readable stuff
across the wire, too. (Perhaps use a custom MIME type to indicate skelful
content?)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r945280 - in /subversion/trunk: build.conf subversion/libsvn_ra_serf/commit.c
subversion/mod_dav_svn/dav_svn.h subversion/mod_dav_svn/posts/ subversion/mod_dav_svn/posts/create-transaction.c
subversion/mod_dav_svn/repos.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
Greg Stein wrote:
> More XML crap? Yuck, ugh, and bleagh.
>
> On Mon, May 17, 2010 at 14:09, <cm...@apache.org> wrote:
>> ...
>> +++ subversion/trunk/subversion/mod_dav_svn/posts/create-transaction.c Mon May 17 18:09:28 2010
>> ...
>> +/* Respond to a S:dated-rev-report request. */
>> +int
>> +dav_svn__create_transaction_post(const dav_resource *resource,
>> + const apr_xml_doc *doc,
>> + ap_filter_t *output)
>> +{
>> + request_rec *r = resource->info->r;
>> + apr_bucket_brigade *bb;
>> + apr_status_t apr_err;
>> + dav_error *derr = NULL;
>> + const char *txn_name;
>> +
>> + /* Create a Subversion repository transaction based on HEAD, and
>> + return the new transaction's name in a custom "201 Created"
>> + response header. */
>> + derr = dav_svn__create_txn(resource->info->repos, &txn_name, resource->pool);
>> + if (derr)
>> + return dav_svn__error_response_tag(r, derr);
>
> This doesn't match the "int" return type you've defined, and it
> results in build errors (see the buildbots).
Hrm. This from util.c:
int
dav_svn__error_response_tag(request_rec *r,
dav_error *err)
{
...
I'll look into this (and the other similarly unexplainable) return mismatch
you mentioned.
> Can I say again that I hate XML, and am very much -0.5 on using it in
> our protocol like this? These bodies have nothing to conform to, so
> there is no reason to use XML. If you're doing it for *expedience*
> rather than *propriety*, then I'd suggest taking short cuts now could
> hurt us in the long term. XML means we need to deal with escaping as
> we move forward. It is overly wordy. Hard to write parsing code. etc
> etc.
I have a local patch here that adds a mod_dav_svn utility function for
stringbuf'ing a request into a giant block of memory and parsing into a
skel. I can certainly switch to that approach if you feel that strongly
about it. Would you be okay with a documented requirement that these new
POSTs use "text/plain", skel input, where the first atom of the skel is the
POST operation verb ("create-transaction", "lock-paths", etc.)?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand