[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] was: [WIP] Enable passing copyfrom information for the diff code when dealing with repository diffs.

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Mon, 16 Aug 2010 22:59:03 +0300

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);
Received on 2010-08-16 22:01:50 CEST

This is an archived mail posted to the Subversion Dev mailing list.