You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2010/08/16 21:59:03 UTC
Re: [PATCH] was: [WIP] Enable passing copyfrom information for the
diff code when dealing with repository diffs.
Daniel Näslund wrote on Mon, Aug 16, 2010 at 15:16:33 +0200:
> And this time with a patch attached.
>
> On Mon, Aug 16, 2010 at 03:09:49PM +0200, Daniel Näslund wrote:
> > I'm touching a lot of code here. Reviews would be much apprecieated.
>
> >
> > [[[
> > Make the diff editor able to receive copyfrom information. Involves
> > passing down a 'send_copyfrom_args' to all RA implemtations.
> >
> > Note that this commit merely allows the copyfrom args to be passed to
> > the client. They copyfrom information is not yet stored and used.
This commit changes the ra_svn protocols, but not the other protocols.
Okay.
> > ]]]
> >
> > Thanks,
> > Daniel
> Index: subversion/libsvn_ra/deprecated.c
> ===================================================================
> --- subversion/libsvn_ra/deprecated.c (revision 985813)
> +++ subversion/libsvn_ra/deprecated.c (arbetskopia)
> @@ -239,6 +239,30 @@ svn_error_t *svn_ra_get_commit_editor(svn_ra_sessi
> keep_locks, pool);
> }
>
> +svn_error_t * svn_ra_do_diff3(svn_ra_session_t *session,
Style nit: no space after '*'.
> + apr_pool_t *pool)
> Index: subversion/libsvn_ra_svn/protocol
> ===================================================================
> --- subversion/libsvn_ra_svn/protocol (revision 985813)
> +++ subversion/libsvn_ra_svn/protocol (arbetskopia)
> @@ -345,7 +345,8 @@ second place for auth-request point as noted below
>
> diff
> params: ( [ rev:number ] target:string recurse:bool ignore-ancestry:bool
> - url:string ? text-deltas:bool ? depth:word )
> + url:string ? text-deltas:bool ? depth:word
> + send_copyfrom_param:bool )
Err, shouldn't this be
+ url:string ? text-deltas:bool ? depth:word
+ ? send_copyfrom_param:bool )
since clients before-your-change don't send the send_copyfrom_args param?
> Client switches to report command set.
> Upon finish-report, server sends auth-request.
> After auth exchange completes, server switches to editor command set.
> Index: subversion/include/svn_ra.h
> ===================================================================
> --- subversion/include/svn_ra.h (revision 985813)
> +++ subversion/include/svn_ra.h (arbetskopia)
> @@ -1291,9 +1291,30 @@ svn_ra_do_status(svn_ra_session_t *session,
> * needed, and sending too much data back, a pre-1.5 'recurse'
> * directive may be sent to the server, based on @a depth.
> *
> - * @since New in 1.5.
> + * @since New in 1.7.
> */
> svn_error_t *
> +svn_ra_do_diff4(svn_ra_session_t *session,
> + const svn_ra_reporter3_t **reporter,
> + void **report_baton,
> + svn_revnum_t revision,
> + const char *diff_target,
> + svn_depth_t depth,
> + svn_boolean_t send_copyfrom_args,
> + svn_boolean_t ignore_ancestry,
> + svn_boolean_t text_deltas,
> + const char *versus_url,
> + const svn_delta_editor_t *diff_editor,
> + void *diff_baton,
> + apr_pool_t *pool);
Need an empty line right here (as I "said" in a previous mail).
> +/**
> + * Similar to svn_ra_do_diff4(), but with @c send_copyfrom_args set to
> + * FALSE.
> + *
> + * @deprecated Provided for compatibility with the 1.5 API.
> + */
> +SVN_DEPRECATED
> +svn_error_t *
> svn_ra_do_diff3(svn_ra_session_t *session,
> const svn_ra_reporter3_t **reporter,
> void **report_baton,
> @@ -1306,7 +1327,6 @@ svn_ra_do_diff3(svn_ra_session_t *session,
> const svn_delta_editor_t *diff_editor,
> void *diff_baton,
> apr_pool_t *pool);
> -
And here you removed an empty line? Can we have it back please?
(that way one can use paragraph-motion commands (e.g., '}') when reading
the source.)
> /**
> * Similar to svn_ra_do_diff3(), but taking @c svn_ra_reporter2_t
> * instead of @c svn_ra_reporter3_t, and therefore only able to report
> Index: subversion/libsvn_client/repos_diff.c
> ===================================================================
> --- subversion/libsvn_client/repos_diff.c (revision 985813)
> +++ subversion/libsvn_client/repos_diff.c (arbetskopia)
> @@ -552,6 +552,8 @@ delete_entry(const char *path,
> svn_wc_notify_action_t action = svn_wc_notify_skip;
> svn_boolean_t tree_conflicted = FALSE;
>
> + SVN_DBG(("delete_entry: %s\n", path));
> +
Shouldn't be committed. (There are a few others in the diff.)
> Index: subversion/svnserve/serve.c
> ===================================================================
> --- subversion/svnserve/serve.c (revision 985813)
> +++ subversion/svnserve/serve.c (arbetskopia)
> @@ -1670,6 +1670,8 @@ static svn_error_t *diff(svn_ra_svn_conn_t *conn,
> /* Default to unknown. Old clients won't send depth, but we'll
> handle that by converting recurse if necessary. */
> svn_depth_t depth = svn_depth_unknown;
> + apr_uint64_t send_copyfrom_param;
> + svn_boolean_t send_copyfrom_args;
>
> /* Parse the arguments. */
> if (params->nelts == 5)
> @@ -1682,10 +1684,14 @@ static svn_error_t *diff(svn_ra_svn_conn_t *conn,
> }
> else
> {
> - SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbbcb?w",
> + /* ### I'm only duplicating the logic in update() for parsing
> + * ### the send_copyfrom_param. Find out how the svn protocol works.
> + */
Is this ### still outstanding?
> + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbbcb?wB",
Should this be
+ SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbbcb?w?B",
?
Have you tested a server with this patch against a client before the patch?
(and vice-versa)
> &rev, &target, &recurse,
> &ignore_ancestry, &versus_url,
> - &text_deltas, &depth_word));
> + &text_deltas, &depth_word,
> + &send_copyfrom_param));
> }
> target = svn_uri_canonicalize(target, pool);
> versus_url = svn_uri_canonicalize(versus_url, pool);
Re: [PATCH] was: [WIP] Enable passing copyfrom information for the
diff code when dealing with repository diffs.
Posted by Daniel Näslund <da...@longitudo.com>.
Daniel,
Sorry for the delayed response. I've committed the patch with your
suggestions in r990790. Below is just ACK's for those changes.
On Mon, Aug 16, 2010 at 10:59:03PM +0300, Daniel Shahaf wrote:
> Daniel Näslund wrote on Mon, Aug 16, 2010 at 15:16:33 +0200:
> > And this time with a patch attached.
> >
> > On Mon, Aug 16, 2010 at 03:09:49PM +0200, Daniel Näslund wrote:
> > > I'm touching a lot of code here. Reviews would be much apprecieated.
> >
> > >
> > > [[[
> > > Make the diff editor able to receive copyfrom information. Involves
> > > passing down a 'send_copyfrom_args' to all RA implemtations.
> > >
> > > Note that this commit merely allows the copyfrom args to be passed to
> > > the client. They copyfrom information is not yet stored and used.
>
> This commit changes the ra_svn protocols, but not the other protocols.
> Okay.
>
> > > ]]]
> > >
> > > Thanks,
> > > Daniel
>
> > Index: subversion/libsvn_ra/deprecated.c
> > ===================================================================
> > --- subversion/libsvn_ra/deprecated.c (revision 985813)
> > +++ subversion/libsvn_ra/deprecated.c (arbetskopia)
> > @@ -239,6 +239,30 @@ svn_error_t *svn_ra_get_commit_editor(svn_ra_sessi
> > keep_locks, pool);
> > }
> >
> > +svn_error_t * svn_ra_do_diff3(svn_ra_session_t *session,
>
> Style nit: no space after '*'.
Fixed.
> > + apr_pool_t *pool)
> > Index: subversion/libsvn_ra_svn/protocol
> > ===================================================================
> > --- subversion/libsvn_ra_svn/protocol (revision 985813)
> > +++ subversion/libsvn_ra_svn/protocol (arbetskopia)
> > @@ -345,7 +345,8 @@ second place for auth-request point as noted below
> >
> > diff
> > params: ( [ rev:number ] target:string recurse:bool ignore-ancestry:bool
> > - url:string ? text-deltas:bool ? depth:word )
> > + url:string ? text-deltas:bool ? depth:word
> > + send_copyfrom_param:bool )
>
> Err, shouldn't this be
>
> + url:string ? text-deltas:bool ? depth:word
> + ? send_copyfrom_param:bool )
>
> since clients before-your-change don't send the send_copyfrom_args param?
Yes it should. Fixed.
> > Client switches to report command set.
> > Upon finish-report, server sends auth-request.
> > After auth exchange completes, server switches to editor command set.
> > Index: subversion/include/svn_ra.h
> > ===================================================================
> > --- subversion/include/svn_ra.h (revision 985813)
> > +++ subversion/include/svn_ra.h (arbetskopia)
> > @@ -1291,9 +1291,30 @@ svn_ra_do_status(svn_ra_session_t *session,
> > * needed, and sending too much data back, a pre-1.5 'recurse'
> > * directive may be sent to the server, based on @a depth.
> > *
> > - * @since New in 1.5.
> > + * @since New in 1.7.
> > */
> > svn_error_t *
> > +svn_ra_do_diff4(svn_ra_session_t *session,
> > + const svn_ra_reporter3_t **reporter,
> > + void **report_baton,
> > + svn_revnum_t revision,
> > + const char *diff_target,
> > + svn_depth_t depth,
> > + svn_boolean_t send_copyfrom_args,
> > + svn_boolean_t ignore_ancestry,
> > + svn_boolean_t text_deltas,
> > + const char *versus_url,
> > + const svn_delta_editor_t *diff_editor,
> > + void *diff_baton,
> > + apr_pool_t *pool);
>
> Need an empty line right here (as I "said" in a previous mail).
Fixed.
> > +/**
> > + * Similar to svn_ra_do_diff4(), but with @c send_copyfrom_args set to
> > + * FALSE.
> > + *
> > + * @deprecated Provided for compatibility with the 1.5 API.
> > + */
> > +SVN_DEPRECATED
> > +svn_error_t *
> > svn_ra_do_diff3(svn_ra_session_t *session,
> > const svn_ra_reporter3_t **reporter,
> > void **report_baton,
> > @@ -1306,7 +1327,6 @@ svn_ra_do_diff3(svn_ra_session_t *session,
> > const svn_delta_editor_t *diff_editor,
> > void *diff_baton,
> > apr_pool_t *pool);
> > -
>
> And here you removed an empty line? Can we have it back please?
>
> (that way one can use paragraph-motion commands (e.g., '}') when reading
> the source.)
Fixed.
> > /**
> > * Similar to svn_ra_do_diff3(), but taking @c svn_ra_reporter2_t
> > * instead of @c svn_ra_reporter3_t, and therefore only able to report
> > Index: subversion/libsvn_client/repos_diff.c
> > ===================================================================
> > --- subversion/libsvn_client/repos_diff.c (revision 985813)
> > +++ subversion/libsvn_client/repos_diff.c (arbetskopia)
> > @@ -552,6 +552,8 @@ delete_entry(const char *path,
> > svn_wc_notify_action_t action = svn_wc_notify_skip;
> > svn_boolean_t tree_conflicted = FALSE;
> >
> > + SVN_DBG(("delete_entry: %s\n", path));
> > +
>
> Shouldn't be committed. (There are a few others in the diff.)
The debug statements are now removed.
> > Index: subversion/svnserve/serve.c
> > ===================================================================
> > --- subversion/svnserve/serve.c (revision 985813)
> > +++ subversion/svnserve/serve.c (arbetskopia)
> > @@ -1670,6 +1670,8 @@ static svn_error_t *diff(svn_ra_svn_conn_t *conn,
> > /* Default to unknown. Old clients won't send depth, but we'll
> > handle that by converting recurse if necessary. */
> > svn_depth_t depth = svn_depth_unknown;
> > + apr_uint64_t send_copyfrom_param;
> > + svn_boolean_t send_copyfrom_args;
> >
> > /* Parse the arguments. */
> > if (params->nelts == 5)
> > @@ -1682,10 +1684,14 @@ static svn_error_t *diff(svn_ra_svn_conn_t *conn,
> > }
> > else
> > {
> > - SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbbcb?w",
> > + /* ### I'm only duplicating the logic in update() for parsing
> > + * ### the send_copyfrom_param. Find out how the svn protocol works.
> > + */
>
> Is this ### still outstanding?
Nope, removed.
> > + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbbcb?wB",
>
> Should this be
> + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbbcb?w?B",
> ?
Yes, it should. Fixed.
>
> Have you tested a server with this patch against a client before the patch?
> (and vice-versa)
I've tested the patch with ra_neon, ra_local and ra_svn. Though, I've
only ran the testsuite over ra_local.
Thanks for the review,
Daniel
Re: [PATCH] was: [WIP] Enable passing copyfrom information for the
diff code when dealing with repository diffs.
Posted by Daniel Näslund <da...@longitudo.com>.
Daniel,
Sorry for the delayed response. I've committed the patch with your
suggestions in r990790. Below is just ACK's for those changes.
On Mon, Aug 16, 2010 at 10:59:03PM +0300, Daniel Shahaf wrote:
> Daniel Näslund wrote on Mon, Aug 16, 2010 at 15:16:33 +0200:
> > And this time with a patch attached.
> >
> > On Mon, Aug 16, 2010 at 03:09:49PM +0200, Daniel Näslund wrote:
> > > I'm touching a lot of code here. Reviews would be much apprecieated.
> >
> > >
> > > [[[
> > > Make the diff editor able to receive copyfrom information. Involves
> > > passing down a 'send_copyfrom_args' to all RA implemtations.
> > >
> > > Note that this commit merely allows the copyfrom args to be passed to
> > > the client. They copyfrom information is not yet stored and used.
>
> This commit changes the ra_svn protocols, but not the other protocols.
> Okay.
>
> > > ]]]
> > >
> > > Thanks,
> > > Daniel
>
> > Index: subversion/libsvn_ra/deprecated.c
> > ===================================================================
> > --- subversion/libsvn_ra/deprecated.c (revision 985813)
> > +++ subversion/libsvn_ra/deprecated.c (arbetskopia)
> > @@ -239,6 +239,30 @@ svn_error_t *svn_ra_get_commit_editor(svn_ra_sessi
> > keep_locks, pool);
> > }
> >
> > +svn_error_t * svn_ra_do_diff3(svn_ra_session_t *session,
>
> Style nit: no space after '*'.
Fixed.
> > + apr_pool_t *pool)
> > Index: subversion/libsvn_ra_svn/protocol
> > ===================================================================
> > --- subversion/libsvn_ra_svn/protocol (revision 985813)
> > +++ subversion/libsvn_ra_svn/protocol (arbetskopia)
> > @@ -345,7 +345,8 @@ second place for auth-request point as noted below
> >
> > diff
> > params: ( [ rev:number ] target:string recurse:bool ignore-ancestry:bool
> > - url:string ? text-deltas:bool ? depth:word )
> > + url:string ? text-deltas:bool ? depth:word
> > + send_copyfrom_param:bool )
>
> Err, shouldn't this be
>
> + url:string ? text-deltas:bool ? depth:word
> + ? send_copyfrom_param:bool )
>
> since clients before-your-change don't send the send_copyfrom_args param?
Yes it should. Fixed.
> > Client switches to report command set.
> > Upon finish-report, server sends auth-request.
> > After auth exchange completes, server switches to editor command set.
> > Index: subversion/include/svn_ra.h
> > ===================================================================
> > --- subversion/include/svn_ra.h (revision 985813)
> > +++ subversion/include/svn_ra.h (arbetskopia)
> > @@ -1291,9 +1291,30 @@ svn_ra_do_status(svn_ra_session_t *session,
> > * needed, and sending too much data back, a pre-1.5 'recurse'
> > * directive may be sent to the server, based on @a depth.
> > *
> > - * @since New in 1.5.
> > + * @since New in 1.7.
> > */
> > svn_error_t *
> > +svn_ra_do_diff4(svn_ra_session_t *session,
> > + const svn_ra_reporter3_t **reporter,
> > + void **report_baton,
> > + svn_revnum_t revision,
> > + const char *diff_target,
> > + svn_depth_t depth,
> > + svn_boolean_t send_copyfrom_args,
> > + svn_boolean_t ignore_ancestry,
> > + svn_boolean_t text_deltas,
> > + const char *versus_url,
> > + const svn_delta_editor_t *diff_editor,
> > + void *diff_baton,
> > + apr_pool_t *pool);
>
> Need an empty line right here (as I "said" in a previous mail).
Fixed.
> > +/**
> > + * Similar to svn_ra_do_diff4(), but with @c send_copyfrom_args set to
> > + * FALSE.
> > + *
> > + * @deprecated Provided for compatibility with the 1.5 API.
> > + */
> > +SVN_DEPRECATED
> > +svn_error_t *
> > svn_ra_do_diff3(svn_ra_session_t *session,
> > const svn_ra_reporter3_t **reporter,
> > void **report_baton,
> > @@ -1306,7 +1327,6 @@ svn_ra_do_diff3(svn_ra_session_t *session,
> > const svn_delta_editor_t *diff_editor,
> > void *diff_baton,
> > apr_pool_t *pool);
> > -
>
> And here you removed an empty line? Can we have it back please?
>
> (that way one can use paragraph-motion commands (e.g., '}') when reading
> the source.)
Fixed.
> > /**
> > * Similar to svn_ra_do_diff3(), but taking @c svn_ra_reporter2_t
> > * instead of @c svn_ra_reporter3_t, and therefore only able to report
> > Index: subversion/libsvn_client/repos_diff.c
> > ===================================================================
> > --- subversion/libsvn_client/repos_diff.c (revision 985813)
> > +++ subversion/libsvn_client/repos_diff.c (arbetskopia)
> > @@ -552,6 +552,8 @@ delete_entry(const char *path,
> > svn_wc_notify_action_t action = svn_wc_notify_skip;
> > svn_boolean_t tree_conflicted = FALSE;
> >
> > + SVN_DBG(("delete_entry: %s\n", path));
> > +
>
> Shouldn't be committed. (There are a few others in the diff.)
The debug statements are now removed.
> > Index: subversion/svnserve/serve.c
> > ===================================================================
> > --- subversion/svnserve/serve.c (revision 985813)
> > +++ subversion/svnserve/serve.c (arbetskopia)
> > @@ -1670,6 +1670,8 @@ static svn_error_t *diff(svn_ra_svn_conn_t *conn,
> > /* Default to unknown. Old clients won't send depth, but we'll
> > handle that by converting recurse if necessary. */
> > svn_depth_t depth = svn_depth_unknown;
> > + apr_uint64_t send_copyfrom_param;
> > + svn_boolean_t send_copyfrom_args;
> >
> > /* Parse the arguments. */
> > if (params->nelts == 5)
> > @@ -1682,10 +1684,14 @@ static svn_error_t *diff(svn_ra_svn_conn_t *conn,
> > }
> > else
> > {
> > - SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbbcb?w",
> > + /* ### I'm only duplicating the logic in update() for parsing
> > + * ### the send_copyfrom_param. Find out how the svn protocol works.
> > + */
>
> Is this ### still outstanding?
Nope, removed.
> > + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbbcb?wB",
>
> Should this be
> + SVN_ERR(svn_ra_svn_parse_tuple(params, pool, "(?r)cbbcb?w?B",
> ?
Yes, it should. Fixed.
>
> Have you tested a server with this patch against a client before the patch?
> (and vice-versa)
I've tested the patch with ra_neon, ra_local and ra_svn. Though, I've
only ran the testsuite over ra_local.
Thanks for the review,
Daniel