On Tue, 2005-05-10 at 06:53, Philip Martin wrote:
> "Madan US" <madan@collab.net> writes:
>
> > Index: subversion/include/svn_types.h
> > ===================================================================
> > --- subversion/include/svn_types.h (revision 14664)
> > +++ subversion/include/svn_types.h (working copy)
> > @@ -349,6 +349,7 @@
> > svn_revnum_t new_revision,
> > const char *date,
> > const char *author,
> > + const char *post_commit_err,
> > void *baton);
>
> That's an ABI change in a public interface so it's not acceptable.
> You need to retain svn_commit_callback_t and provide a new name,
> svn_commit_callback2_t say, with the new parameter.
Whats ABI? I think all functions, pointers that comply to
svn_commit_callback_t have been touched in this patch. However, I
understand the client-server version compatibility issue. Is this your
concern. Could you please explain your concern with an illustration ( a
scenario ) so that I can make changes with an inherent understanding...
this would also be a learning for me. Thanks.
>
> Unfortunately this looks like it could be quite a big change. You
> need to keep all the current interfaces that use svn_commit_callback_t
> and ensure that they continue to work. Then you can provide new
> versions of those interfaces that support the new callback.\
Yes, all functions adhering to and using this typedef will be changed,
but we would be keeping the old versions nevertheless.... that IMHO
would be inefficient. Pl. comment.
>
> > 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 ..."\
documented.... in the header file??? or somewhere else?
>
> > 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".
yes, I missed this point... the output of the post-commit hook should be
encoded to utf-8. Anyways, I do not know how to do this... will try to
find out... thanks for that.
>
> > + else
> > + post_commit_err_elem = "" ;
> > +
> > /* get the creationdate and creator-displayname of the new revision,
too. */
> > serr = svn_fs_revision_prop(&creationdate, repos->fs, new_rev,
> > SVN_PROP_REVISION_DATE, pool);
> > @@ -252,7 +264,8 @@
> >
> > (void) ap_fputstrs(output, bb,
> > DAV_XML_HEADER DEBUG_CR
> > - "<D:merge-response xmlns:D=\"DAV:\">" DEBUG_CR
> > + "<D:merge-response xmlns:D=\"DAV:\""
> > + " xmlns:S=\"", SVN_XML_NAMESPACE, "\">" DEBUG_CR
> > "<D:updated-set>" DEBUG_CR
>
> Are old clients going to handle that?
they should ideally ignore the stuff they dont understand.... but
sussman was raising some doubts about this.... lemme check back...
>
> > Index: subversion/mod_dav_svn/version.c
> > ===================================================================
> > --- subversion/mod_dav_svn/version.c (revision 14664)
> > +++ subversion/mod_dav_svn/version.c (working copy)
> > @@ -1619,6 +1619,7 @@
> > svn_fs_txn_t *txn;
> > const char *conflict;
> > svn_error_t *serr;
> > + char *post_commit_err = NULL ;
> > svn_revnum_t new_rev;
> > apr_hash_t *locks;
> > svn_boolean_t disable_merge_response = FALSE;
> > @@ -1699,7 +1700,8 @@
> > return dav_svn_convert_err(serr, HTTP_CONFLICT, msg, pool);
> > }
> > else if (serr)
> > - svn_error_clear(serr);
> > + if (serr->child->message)
> > + post_commit_err = apr_pstrdup (pool, serr->child->message);
>
> That leaks serr doesn't it?
yes.... my bad... would correct it...thanks.
>
> > Index: subversion/tests/clients/cmdline/commit_tests.py
> > ===================================================================
> > --- subversion/tests/clients/cmdline/commit_tests.py (revision
> > 14664)> > +++ subversion/tests/clients/cmdline/commit_tests.py (working
> > copy)> > @@ -898,6 +898,45 @@
> >
> > #----------------------------------------------------------------------
> >
> > +def post_commit_hook_test(sbox):
> > + "post commit hook failure case testing"
> > +
> > + sbox.build()
> > +
> > + # Get paths to the working copy and repository
> > + wc_dir = sbox.wc_dir
> > + repo_dir = sbox.repo_dir
> > +
> > + # Setup the hook configs to echo data back
> > + 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.
mmm.... or write an appropriate batch file for windows...
>
> > 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.
mmm.... I should be using r(?c)(?c)?(?c) as lundblad suggests in this
thread....I think that should solve the problem.... pl. let me know what
you think....
>
> You need to update the libsvn_ra_svn/protocol document as well.
>
> > &new_rev,
> > &committed_date,
> > - &committed_author));
> > + &committed_author,
> > + &post_commit_err));
> >
> > return ccb->callback(new_rev, committed_date, committed_author,
> > - ccb->callback_baton);
> > + post_commit_err, ccb->callback_baton);
> >
> > }
> >
> > Index: subversion/libsvn_ra_dav/merge.c
> > ===================================================================
> > --- subversion/libsvn_ra_dav/merge.c (revision 14664)
> > +++ subversion/libsvn_ra_dav/merge.c (working copy)
> > @@ -58,6 +58,8 @@
> > { "DAV:", "collection", ELEM_collection, 0 },
> > { "DAV:", "baseline", ELEM_baseline, 0 },
> > { "DAV:", "version-name", ELEM_version_name, SVN_RA_DAV__XML_CDATA },
> > + { SVN_XML_NAMESPACE, "post-commit-err",
> > + ELEM_post_commit_err, SVN_RA_DAV__XML_CDATA },
> > { "DAV:", "creationdate", ELEM_creationdate, SVN_RA_DAV__XML_CDATA },
> > { "DAV:", "creator-displayname", ELEM_creator_displayname,
> > SVN_RA_DAV__XML_CDATA },
> > @@ -104,6 +106,8 @@
> > svn_stringbuf_t *committed_date; /* DAV:creationdate for this
> > resource */> > svn_stringbuf_t *last_author; /* DAV:creator-displayname for this
> > resource */
> > + svn_stringbuf_t *post_commit_err;/* SVN_XML_NAMESPACE:post-commit
> > hook's> > + stderr */
> > (?c)
> > /* We only invoke set_prop() on targets listed in valid_targets.
> > Some entities (such as directories that have had changes
> > @@ -341,6 +345,7 @@
> > || child == ELEM_version_name
> > || child == ELEM_creationdate
> > || child == ELEM_creator_displayname
> > + || child == ELEM_post_commit_err
> > /* other props */)
> > return SVN_RA_DAV__XML_VALID;
> > else
> > @@ -524,6 +529,10 @@
> > svn_stringbuf_set(mc->vsn_name, cdata);
> > break;
> >
> > + case ELEM_post_commit_err:
> > + svn_stringbuf_set(mc->post_commit_err, cdata);
> > + break;
> > +
>
> I *think* that means that old clients will simply ignore the new
> data. Good.
yes... you banged the nail right on the head... anyways I will try to
get it tested with older clients also
>
> > Index: subversion/svnserve/serve.c
> > ===================================================================
> > --- subversion/svnserve/serve.c (revision 14664)
> > +++ subversion/svnserve/serve.c (working copy)
> > @@ -62,6 +62,7 @@
> > svn_revnum_t *new_rev;
> > const char **date;
> > const char **author;
> > + const char **post_commit_err;
> > } commit_callback_baton_t;
> >
> > typedef struct {
> > @@ -622,13 +623,15 @@
> > }
> >
> > static svn_error_t *commit_done(svn_revnum_t new_rev, const char *date,
> > - const char *author, void *baton)
> > + const char *author, const char
*post_commit_err,
> > + void *baton)
> > {
> > commit_callback_baton_t *ccb = baton;
> >
> > *ccb->new_rev = new_rev;
> > *ccb->date = date;
> > *ccb->author = author;
> > + *ccb->post_commit_err = post_commit_err;
> > return SVN_NO_ERROR;
> > }
> >
> > @@ -722,7 +725,7 @@
> > apr_array_header_t *params, void *baton)
> > {
> > server_baton_t *b = baton;
> > - const char *log_msg, *date, *author;
> > + const char *log_msg, *date, *author, *post_commit_err;
> > apr_array_header_t *lock_tokens;
> > svn_boolean_t keep_locks;
> > const svn_delta_editor_t *editor;
> > @@ -751,6 +754,7 @@
> > ccb.new_rev = &new_rev;
> > ccb.date = &date;
> > ccb.author = &author;
> > + ccb.post_commit_err = &post_commit_err;
> > /* ### Note that svn_repos_get_commit_editor actually wants a decoded
URL. */
> > SVN_CMD_ERR(svn_repos_get_commit_editor2
> > (&editor, &edit_baton, b->repos, NULL,
> > @@ -775,8 +779,8 @@
> > if (! keep_locks && lock_tokens)
> > SVN_ERR(unlock_paths(lock_tokens, b, pool));
> >
> > - SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "r(?c)(?c)",
> > - new_rev, date, author));
> > + SVN_ERR(svn_ra_svn_write_tuple(conn, pool, "r(?c)(?c)(?c)",
> > + new_rev, date, author,
post_commit_err));
>
> I don't know enough about ra_svn to know whether the old client will
> handle this.
r(?c)(?c)?(?c) ??? as suggested by lundblad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu May 12 07:33:17 2005