Re: svn commit: rev 1923 - trunk/subversion trunk/subversion/libsvn_ra_pipe

On Fri, May 10, 2002 at 01:58:31AM -0700, Greg Stein wrote:
> On Thu, May 09, 2002 at 10:03:58PM -0500, kevin@tigris.org wrote:
> >...
> > +++ trunk/subversion/libsvn_ra_pipe/ra_pipe.c Thu May 9 22:03:45 2002
> > @@ -0,0 +1,501 @@
> > +#include <svn_ra.h>
> > +#include <svn_xml.h>
> > +#include <svn_time.h>
> > +#include <svn_io.h>
>
> The file needs the standard summary, copyright, license blurb at the top.
> For SVN includes, we place them in double quotes rather than angle brackets.

Of course. I add all that stuff to the (empty) header I didn't check in, and
leave it out here.

Will fix the includes.
>
> > +/** XML Stuff for this file (needs to go somewhere public, so libsvn_server
> > + * can see it.) */
> > +#define SVN_RA_PIPE__REQUEST_TAG "svn:pipe:request"
>
> XML tags cannot contain colon characters. When using XML namespaces, you can
> include a "prefix", separated from the tag name by a colon, but you would
> also need to ensure the namespace/prefix is declared.

Okay, I wasn't sure about this. It might be good to declare a namespace like
"svn-pipe" then use it as s:request though.
>
> >...
> > +/** Structures **/
> > +typedef struct
> > +{
> > + apr_file_t *input;
> > + apr_file_t *output;
>
> ra_pipe is going to need more than this. You're going to need to make
> requests to the "server" (the other end of the pipe) and get a response
> back. But you can't just use stdin/stdout because stdout is still needed to
> tell the user about the operation (e.g. update items).

Of course it will. As per your other post, it will have to spawn some other
program, and connect input and output to it's stdin/stdout. However, I found
this easier to debug, and I prefer to add things incrementally as I need them,
rather than all up front.

I did warn you that this was pretty rough :)
>
> >...
> > + sess->pool = pool;
> > + sess->url = svn_stringbuf_dup (repos_URL, pool);
>
> You might want to consider just using 'const char *' rather than a full
> stringbuf. i.e. sess->url = apr_pstrdup(pool, repos_URL->data);

Yep, good idea.
>
> >...
> > + svn_xml_make_open_tag (&buf, sess->pool, svn_xml_normal,
> > + SVN_RA_PIPE__GET_LOG_TAG,
> > + SVN_RA_PIPE__ATT_STARTREV,
> > + svn_stringbuf_create (apr_psprintf (sess->pool,
> > + "%ld",
> > + start),
> > + sess->pool),
>
> It is more efficient to use svn_stringbuf_createf(), rather than the pair of
> create and apr_psprintf.
>
Now why didn't I think of that?

-- 
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson                    http://www.pilch-bisson.net
     "Historically speaking, the presences of wheels in Unix
     has never precluded their reinvention." - Larry Wall
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~