>
> 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)?
It doesn't matter. trying to match names up between headers and source
is a losing war :)
> ...
>> --- (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.
Fixed.
>
>> +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".
That's what it was originally, but it pushes all the calls using it over
the 80 char limit causing more line splits.
Again, i don't really care either way, i think both are perfectly fine
variable names.
>
> ...
>> +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...
Again, whatever makes you feel good. If you want to change it, change
it. It's a temporary variable, used to store the result of a parse.
>
>> + 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.
This was copied from log.c :)
>
>> + 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.
They are pasted from log.c, too :)
>
> ...
>> --- (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.
style copied from log.c again :)
svn_boolean_t discover_changed_paths = 0; /* off by default */
svn_boolean_t strict_node_history = 0; /* off by default */
>
> ...
>> + /* ### 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.
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?
It doesn't actually matter, but whatever makes you feel good :)
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...
Doesn't matter to me.
We log basically all other ra level operations, so ....
> There is an extraneous set of parens around the first condition in
> this nested conditional. Personally, I'd dump the ones around !derr,
> too.
As usual, the mod_dav_svn formatting problems are from copying from
other mod_dav_svn code, so i'm not sure whether we want to be consistent
or "right".
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jul 19 01:56:10 2006