You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by ep...@google.com on 2008/04/18 19:34:39 UTC

Re: svn commit: r30685 - branches/svnserve-logging/subversion/svnserve

epg@tigris.org writes:

> Author: epg
> Date: Fri Apr 18 10:49:34 2008
> New Revision: 30685
> 
> Log:
> Fix diff logging.  All done now!  I can haz reviewz plz?

Easy review command:

  svn diff http://svn.collab.net/repos/svn/trunk@30530 http://svn.collab.net/repos/svn/branches/svnserve-logging

In looking over that myself, I see I still had two questions:

Index: subversion/libsvn_subr/log.c

+const char *
+svn_log__change_rev_prop(svn_revnum_t rev, const char *name, apr_pool_t *pool)
+{
+  return apr_psprintf(pool, "change-rev-prop r%ld %s", rev,
+                      /* XXX Why do this and rev_prop (below) encode the
+                         property name?  Is that really necessary?  When I
+                         added logging for the log-with-revprops stuff, I
+                         didn't encode the revprop names there.  Can I stop
+                         encoding here and in rev_prop or must I encode in
+                         log? */
+                      svn_path_uri_encode(name, pool));
+}

+const char *
+svn_log__rev_prop(svn_revnum_t rev, const char *name, apr_pool_t *pool)
+{
+  return apr_psprintf(pool, "rev-prop r%ld %s", rev,
+                      /* XXX See change_rev_prop above. */
+                      svn_path_uri_encode(name, pool));
+}

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r30685 - branches/svnserve-logging/subversion/svnserve

Posted by ep...@google.com.
epg@google.com writes:

> +const char *
> +svn_log__change_rev_prop(svn_revnum_t rev, const char *name, apr_pool_t *pool)
> +{
> +  return apr_psprintf(pool, "change-rev-prop r%ld %s", rev,
> +                      /* XXX Why do this and rev_prop (below) encode the
> +                         property name?  Is that really necessary?  When I
> +                         added logging for the log-with-revprops stuff, I
> +                         didn't encode the revprop names there.  Can I stop
> +                         encoding here and in rev_prop or must I encode in
> +                         log? */
> +                      svn_path_uri_encode(name, pool));

Yep, they need to be encoded; see r30689.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r30685 - branches/svnserve-logging/subversion/svnserve

Posted by Eric Gillespie <ep...@pretzelnet.org>.
Stefan Sperling <st...@elego.de> writes:

> + * serve.c :  Functions for serving the Subversion protocol
>     ^^^^^^^
> 
> Should be log.c, right?

Fixed in r30830, thanks.

--  
Eric Gillespie <*> epg@pretzelnet.org

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r30685 - branches/svnserve-logging/subversion/svnserve

Posted by Stefan Sperling <st...@elego.de>.
On Mon, Apr 28, 2008 at 01:37:35PM -0700, epg@google.com wrote:
> I suppose the lack of response means this has already been
> reviewed, isn't going to be reviewed, or may get more attention
> on trunk ;->.  I'll commit today or tomorrow, and then commit
> some error logging changes I have pending.

I looked over the diff very briefly, but for lack of time didn't
look through it as thoroughly as I was planning to.

I didn't want to send a mail pointing out the single thing I noticed
while skimming over it, but now that you're gonna merge anyway,
and before it gets lost:

Index: subversion/libsvn_subr/log.c
===================================================================
--- subversion/libsvn_subr/log.c	(.../trunk)	(revision 0)
+++ subversion/libsvn_subr/log.c	(.../branches/svnserve-logging)	(revision 30692)
@@ -0,0 +1,379 @@
+/*
+ * serve.c :  Functions for serving the Subversion protocol
    ^^^^^^^

Should be log.c, right?

-- 
Stefan Sperling <st...@elego.de>                    Software Monkey
 
German law requires the following banner :(
elego Software Solutions GmbH                            HRB 77719
Gustav-Meyer-Allee 25, Gebaeude 12        Tel:  +49 30 23 45 86 96 
13355 Berlin                              Fax:  +49 30 23 45 86 95
http://www.elego.de                               CEO: Olaf Wagner
 
Store password unencrypted (yes/no)? No

Re: svn commit: r30685 - branches/svnserve-logging/subversion/svnserve

Posted by ep...@google.com.
> > Author: epg
> > Date: Fri Apr 18 10:49:34 2008
> > New Revision: 30685
> > 
> > Log:
> > Fix diff logging.  All done now!  I can haz reviewz plz?
> 
> Easy review command:

  svn diff http://svn.collab.net/repos/svn/trunk@30700 http://svn.collab.net/repos/svn/branches/svnserve-logging

I suppose the lack of response means this has already been
reviewed, isn't going to be reviewed, or may get more attention
on trunk ;->.  I'll commit today or tomorrow, and then commit
some error logging changes I have pending.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: svn commit: r30685 - branches/svnserve-logging/subversion/svnserve

Posted by ep...@google.com.
epg@google.com writes:

> epg@tigris.org writes:
> 
> > Author: epg
> > Date: Fri Apr 18 10:49:34 2008
> > New Revision: 30685
> > 
> > Log:
> > Fix diff logging.  All done now!  I can haz reviewz plz?
> 
> Easy review command:
> 
>   svn diff http://svn.collab.net/repos/svn/trunk@30530 http://svn.collab.net/repos/svn/branches/svnserve-logging

Oops, one more thing: I have a patch in progress to add ERR
logging in a few other places in svnserve, but I'll do that on
trunk after this goes in.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org