On Mon, Feb 24, 2014 at 2:45 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> On 23 September 2013 02:10, <stefan2_at_apache.org> wrote:
> > Author: stefan2
> > Date: Sun Sep 22 22:10:53 2013
> > New Revision: 1525460
> >
> > URL: http://svn.apache.org/r1525460
> > Log:
> > Support the MOVes in the RA log API:
> > Bump svn_ra_get_log to support the move_behavior option.
> >
> Hi Stefan, see my comments inline.
>
> > For ra_serf, we add an optional move-behavior element to the LOG report.
> > If not given, it defaults to 1.8 behavior reporting all moves as adds.
> >
> > For ra_svn, we append an optional integer parameter determining the
> > move-behavior option. Same default behavior as above.
> >
> mod_dav_svn change is not mention in log message btw.
>
Hm. I see:
* subversion/mod_dav_svn/merge.c
> (do_resources): add move_behavior in case we want to call this function
> from places other than dav_svn__merge_response;
> handle the new change types
> (dav_svn__merge_response): report commit result as recorded
>
What part is missing?
>
> >
> > Modified: subversion/trunk/subversion/libsvn_ra_svn/protocol
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_svn/protocol?rev=1525460&r1=1525459&r2=1525460&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_ra_svn/protocol (original)
> > +++ subversion/trunk/subversion/libsvn_ra_svn/protocol Sun Sep 22
> 22:10:53 2013
> > @@ -382,11 +382,13 @@ second place for auth-request point as n
> > [ end-rev:number ] changed-paths:bool strict-node:bool
> > ? limit:number
> > ? include-merged-revisions:bool
> > - all-revprops | revprops ( revprop:string ... ) )
> > + all-revprops | revprops ( revprop:string ... )
> > + ? move-behavior:number )
> > Before sending response, server sends log entries, ending with
> "done".
> > If a client does not want to specify a limit, it should send 0 as
> the
> > limit parameter. rev-props excludes author, date, and log; they are
> > sent separately for backwards-compatibility.
> > + Move-behavior is encoded like enum svn_move_behavior_t.
> > log-entry: ( ( change:changed-path-entry ... ) rev:number
> > [ author:string ] [ date:string ] [ message:string ]
> > ? has-children:bool invalid-revnum:bool
> >
> Currently Subversion uses symbolic names to marshal enums over the
> wire. See svn_depth_t for example. I don't see reason why
> svn_move_behavior_t should be different.
>
Fair point. That should be modeled similarly to e.g. svn_depth_t.
The code actually gets simpler using the string encoding.
> > Modified: subversion/trunk/subversion/mod_dav_svn/reports/log.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/reports/log.c?rev=1525460&r1=1525459&r2=1525460&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/mod_dav_svn/reports/log.c (original)
> > +++ subversion/trunk/subversion/mod_dav_svn/reports/log.c Sun Sep 22
> 22:10:53 2013
> > @@ -395,6 +398,24 @@ dav_svn__log_report(const dav_resource *
> > resource->pool);
> > APR_ARRAY_PUSH(paths, const char *) = target;
> > }
> > + else if (strcmp(child->name, "move-behavior") == 0)
> > + {
> > + int move_behavior_param;
> > + serr = svn_cstring_atoi(&move_behavior_param,
> > + dav_xml_get_cdata(child,
> resource->pool, 1));
> > + if (serr)
> > + return dav_svn__convert_err(serr, HTTP_BAD_REQUEST,
> > + "Malformed CDATA in element "
> > + "\"move-behavior\"",
> resource->pool);
> > +
> > + if ( move_behavior_param < 0
> > + || move_behavior_param > svn_move_behavior_auto_moves)
> > + return dav_svn__convert_err(serr, HTTP_BAD_REQUEST,
> > + "Invalid CDATA in element "
> > + "\"move-behavior\"",
> resource->pool);
> > +
> > + move_behavior = (svn_move_behavior_t) move_behavior_param;
> > + }
> Same is here: why use numbers to encode svn_move_behavior_t instead of
> symbolic name?
>
Mainly quick coding followed by a mere oversight.
Both protocols have been updated in r1572044.
Thanks for the review!
-- Stefan^2.
Received on 2014-02-27 12:35:38 CET