Re: [PATCH] Issue #443: post-commit hook script (error) output lost

"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.

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.

> 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 ..."

> 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".

> + 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?

> 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?

> 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.

> 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.

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 */
>
> /* 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.

> 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.

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