this is not a complete review, just clafirying some of philip's points.
On Tue, 10 May 2005, Philip Martin wrote:
> "Madan US" <madan@collab.net> writes:
>
> > Index: subversion/include/svn_client.h
> > ===================================================================
> > --- subversion/include/svn_client.h (revision 14664)
> > +++ subversion/include/svn_client.h (working copy)
> > @@ -275,6 +275,9 @@
> > /** author of the commit. */
> > const char *author;
> >
> > + /** post-commit hook's stderr. */
> > + const char *post_commit_err;
> > +
> > } svn_client_commit_info_t;
>
> That's another ABI change. Adding elements to the end of a structure
> has been accepted in some cases in the past, but I don't know whether
> this one is OK. The new member needs to be documented "@since New ..."
>
Yeah, here comes the interesting question again. On one hand, our code
will not take these structures allocated by the users; it will always
return structures it allocated. OTOH, you can always write code that
breaks by this change if you really want to. I think it is OK to extend
this sturcutre, but we should add a note that this might happen in the
future.
> > Index:
subversion/mod_dav_svn/merge.c
> > ===================================================================
> > --- subversion/mod_dav_svn/merge.c (revision 14664)
> > +++ subversion/mod_dav_svn/merge.c (working copy)
> > @@ -29,6 +29,7 @@
> > #include "svn_props.h"
> >
> > #include "dav_svn.h"
> > +#include "svn_xml.h"
> >
> >
> > /* #################################################################
> > @@ -200,6 +201,7 @@
> > dav_error * dav_svn__merge_response(ap_filter_t *output,
> > const dav_svn_repos *repos,
> > svn_revnum_t new_rev,
> > + char *post_commit_err,
> > apr_xml_elem *prop_elem,
> > svn_boolean_t disable_merge_response,
> > apr_pool_t *pool)
> > @@ -209,6 +211,7 @@
> > svn_error_t *serr;
> > const char *vcc;
> > const char *rev;
> > + const char *post_commit_err_elem;
> > svn_string_t *creationdate, *creator_displayname;
> >
> > serr = svn_fs_revision_root(&root, repos->fs, new_rev, pool);
> > @@ -231,6 +234,15 @@
> > /* the version-name of the baseline is the revision number */
> > rev = apr_psprintf(pool, "%ld", new_rev);
> >
> > + /* get the post-commit hook stderr, if any */
> > + if (post_commit_err)
> > + post_commit_err_elem = apr_psprintf(pool,
> > + "<S:post-commit-err>%s"
> > + "</S:post-commit-err>",
> > + post_commit_err);
>
> I'm not a DAV/XML expert, can you put arbitrary data there? Do you
> have to encode it, or escape it?
>
> Hmm, a quick experiment shows that the existing hook output appears to
> get escaped over ra_dav, but not encoded. The hook needs to output
> UTF-8 if it is to be displayed by the client, otherwise I just see
> "svn: General svn error from server".
>
It definitely needs to be XML-escaped. Whether it should be recoded from
the locale encoding is another topic - and should be a separate change
IMO. Oh, and it needs base4 encoding as well, or filtering so that invalid
XML chars don't come through.
> > + post_commit_hook = svntest.main.get_post_commit_hook_path (repo_dir)
> > + svntest.main.file_append (post_commit_hook,
> > + """#!/bin/sh
> > + echo "Post-commit Hook says nothing doing on stderr" > /dev/stderr
> > + exit -1
>
> What about Windows? At present you will have to Skip() the test on
> some platforms.
>
And is 2> more portable than /dev/stderr?
> > Index: subversion/libsvn_ra_svn/client.c
> > ===================================================================
> > --- subversion/libsvn_ra_svn/client.c (revision 14664)
> > +++ subversion/libsvn_ra_svn/client.c (working copy)
> > @@ -785,15 +785,18 @@
> > svn_revnum_t new_rev;
> > const char *committed_date;
> > const char *committed_author;
> > + const char *post_commit_err;
> >
> > SVN_ERR(handle_auth_request(ccb->sess_baton, ccb->pool));
> > - SVN_ERR(svn_ra_svn_read_tuple(ccb->sess_baton->conn, ccb->pool, "r(?c)(?c)",
> > + SVN_ERR(svn_ra_svn_read_tuple(ccb->sess_baton->conn, ccb->pool,
> > + "r(?c)(?c)(?c)",
>
> Will the new server send the post-commit error to old clients? Will
> the old clients know how to handle it? The old clients must continue
> to work when used with the new server.
>
Exta elements in the tuples are ignored by both server and client. But it
should be tested to make sure, of course.
BUT, will the new client handle an old server? I'd say no, since the
client will expect an extra element (?c). So you need to have an question
mark before the left paren as well: "r(?c)(?c)?(?c)". Noe that you ned the
parens to support further extensibility.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue May 10 10:54:55 2005