On Tue, 18 Jul 2006, dberlin@tigris.org wrote:
...
> Make svn_ra_get_mergeinfo work over DAV, and fix bindings
> so svn_ra_get_mergeinfo and svn_repos_fs_get_mergeinfo work.
Wooooohooo, awesome DannyB!
The merge-tracking branch now has code for ra_dav, ra_svn, ra_local,
mod_dav_svn, and svnserve in place for retrieval of merge history.
A few comments below (other than what Garrett mentioned):
...
> --- branches/merge-tracking/subversion/include/svn_ra.h (original)
> +++ branches/merge-tracking/subversion/include/svn_ra.h Tue Jul 18 12:42:41 2006
> @@ -719,7 +719,7 @@
> * @since New in 1.5.
> */
> svn_error_t *svn_ra_get_merge_info(svn_ra_session_t *session,
> - apr_hash_t **mergeinfo,
> + apr_hash_t **mergeoutput,
> const apr_array_header_t *paths,
> svn_revnum_t revision,
> svn_boolean_t include_parents,
While I don't think that it's necessary for SWIG's sake, should the
implementation which delegates to the vtable, and vtable API/functions
reflect this variable rename as well (for consistency)?
> --- branches/merge-tracking/subversion/include/svn_repos.h (original)
> +++ branches/merge-tracking/subversion/include/svn_repos.h Tue Jul 18 12:42:41 2006
> @@ -1056,7 +1056,7 @@
> * @since New in 1.5.
> */
> svn_error_t *
> -svn_repos_fs_get_merge_info(apr_hash_t **mergeinfo,
> +svn_repos_fs_get_merge_info(apr_hash_t **mergeoutput,
> svn_repos_t *repos,
> const apr_array_header_t *paths,
> svn_revnum_t revision,
Similar question here.
...
> --- (empty file)
> +++ branches/merge-tracking/subversion/libsvn_ra_dav/mergeinfo.c Tue Jul 18 12:42:41 2006
...
> +struct mergeinfo_baton
> +{
> + apr_pool_t *pool;
> + const char *curr_path;
> + const char *curr_info;
> + apr_hash_t *result;
> + svn_error_t *err;
> +};
A brief doc string for the above struct would be nice to have.
> +static const svn_ra_dav__xml_elm_t minfo_report_elements[] =
> + {
> + { SVN_XML_NAMESPACE, "merge-info-report", ELEM_merge_info_report, 0 },
> + { SVN_XML_NAMESPACE, "merge-info-item", ELEM_merge_info_item, 0 },
> + { SVN_XML_NAMESPACE, "merge-info-path", ELEM_merge_info_path,
> + SVN_RA_DAV__XML_CDATA },
> + { SVN_XML_NAMESPACE, "merge-info-info", ELEM_merge_info_info,
> + SVN_RA_DAV__XML_CDATA },
> + { NULL }
> + };
While more verbose, "mergeinfo" would be a more obvious name for this
array than "minfo".
...
> +static int
> +end_element(void *baton, int state, const char *nspace, const char *elt_name)
> +{
> + struct mergeinfo_baton *mb = baton;
> +
> + const svn_ra_dav__xml_elm_t *elm
> + = svn_ra_dav__lookup_xml_elem(minfo_report_elements, nspace, elt_name);
> +
> + if (! elm)
> + return NE_XML_DECLINE;
> +
> + if (elm->id == ELEM_merge_info_item)
> + {
> + if (mb->curr_info && mb->curr_path)
> + {
> + apr_hash_t *temp;
How 'bout calling this "mergeinfo" instead of "temp"? As it's added
to RESULT, it doesn't really seem temporary...
> + mb->err = svn_mergeinfo_parse(mb->curr_info, &temp, mb->pool);
> + if (mb->err != SVN_NO_ERROR)
> + goto fail;
> +
> + apr_hash_set(mb->result, mb->curr_path, APR_HASH_KEY_STRING,
> + temp);
> + }
> + }
> + fail:
> + if (mb->err)
> + return NE_XML_ABORT;
> +
> + return SVN_RA_DAV__XML_VALID;
> +}
...
> +/* Request a merge-info-report from the URL attached to SESSION,
> + and fill in the MERGEINFO hash with the results. */
> +svn_error_t * svn_ra_dav__get_merge_info(svn_ra_session_t *session,
> + apr_hash_t **mergeinfo,
> + const apr_array_header_t *paths,
> + svn_revnum_t revision,
> + svn_boolean_t include_parents,
> + apr_pool_t *pool)
> +{
> + /* The Plan: Send a request to the server for a merge-info report.
> + */
> +
> + int i;
> + svn_ra_dav__session_t *ras = session->priv;
> + svn_stringbuf_t *request_body = svn_stringbuf_create("", pool);
> + struct mergeinfo_baton mb;
> +
> + svn_error_t *err;
> +
> + /* ### todo: I don't understand why the static, file-global
> + variables shared by update and status are called `report_head'
> + and `report_tail', instead of `request_head' and `request_tail'.
> + Maybe Greg can explain? Meanwhile, I'm tentatively using
> + "request_*" for my local vars below. */
I think the "report_" prefix is used in ra_dav because it's referring
to the payload of a specific type of HTTP request (REPORT). In HTTP
client/server apps, "request" usually refers to the entirety of the
HTTP request (e.g. including the request method and headers), rather
than solely to the request body.
> + static const char minfo_request_head[]
> + = "<S:merge-info-report xmlns:S=\"" SVN_XML_NAMESPACE "\">" DEBUG_CR;
> +
> + static const char minfo_request_tail[] = "</S:merge-info-report>" DEBUG_CR;
> +
> +
> +
> +
There are a lot of extraneous newlines here -- I think we can ditch
some of'em.
...
> --- (empty file)
> +++ branches/merge-tracking/subversion/mod_dav_svn/mergeinfo.c Tue Jul 18 12:42:41 2006
...
> +dav_error * dav_svn__get_merge_info_report(const dav_resource *resource,
> + const apr_xml_doc *doc,
> + ap_filter_t *output)
> +{
...
> + /* These get determined from the request document. */
> + svn_revnum_t rev = SVN_INVALID_REVNUM;
> + svn_boolean_t include_parents = 0; /* off by default */
This should be svn_boolean_t's constant FALSE rather than 0.
...
> + /* ### todo: okay, now go fill in svn_ra_dav__get_log() based on the
> + syntax implied below... */
I don't understand the pertinence of this comment -- cut'n'paste-o?
Looks like it should be svn_ra_dav__get_merge_info(), or should be
removed entirely.
> + for (child = doc->root->first_child; child != NULL; child = child->next)
> + {
> + /* if this element isn't one of ours, then skip it */
> + if (child->ns != ns)
> + continue;
> +
> + if (strcmp(child->name, "revision") == 0)
> + rev = SVN_STR_TO_REV(dav_xml_get_cdata(child, resource->pool, 1));
> + else if (strcmp(child->name, "include-parents") == 0)
> + include_parents = 1;
> + else if (strcmp(child->name, "path") == 0)
> + {
> + const char *target;
> + const char *rel_path = dav_xml_get_cdata(child, resource->pool, 0);
> + if ((derr = dav_svn__test_canonical(rel_path, resource->pool)))
> + return derr;
> + target = svn_path_join(resource->info->repos_path, rel_path,
> + resource->pool);
> + (*((const char **)(apr_array_push(paths)))) = target;
> + }
> + /* else unknown element; skip it */
...
> + for (hi = apr_hash_first(resource->pool, mergeinfo); hi;
> + hi = apr_hash_next(hi))
> + {
> + const char *path, *info;
There's a bunch of trailing whitespace after these string decls.
> + const char itemformat[] = "<S:merge-info-item>" DEBUG_CR "<S:merge-info-path>%s</S:merge-info-path>" DEBUG_CR "<S:merge-info-info>%s</S:merge-info-info>" DEBUG_CR "</S:merge-info-item>";
Should this format string be either static or a constant? How about
wrapping this long line?
...
> + cleanup:
> +
> + /* We've detected a 'high level' svn action to log. */
Is retrieval of merge info really a high enough level operation to
log? This logging typically applies to operations like "update",
"checkout", "diff", "log", etc. Merge info retrieval seems like more
of an implementation detail...
> + if (paths->nelts == 0)
> + action = "get-merge-info";
> + else if (paths->nelts == 1)
> + action = apr_psprintf(resource->pool, "get-merge-info '%s'",
> + svn_path_uri_encode(APR_ARRAY_IDX
> + (paths, 0, const char *),
> + resource->pool));
> + else
> + action = apr_psprintf(resource->pool, "get-merge-info-partial '%s'",
> + svn_path_uri_encode(APR_ARRAY_IDX
> + (paths, 0, const char *),
> + resource->pool));
> +
> + apr_table_set(resource->info->r->subprocess_env, "SVN-ACTION", action);
> +
> +
> + /* Flush the contents of the brigade (returning an error only if we
> + don't already have one). */
> + if (((apr_err = ap_fflush(output, bb))) && (! derr))
There is an extraneous set of parens around the first condition in
this nested conditional. Personally, I'd dump the ones around !derr,
too.
- application/pgp-signature attachment: stored
Received on Tue Jul 18 23:07:06 2006