On Fri, 12 Aug 2005, Martin Hauner wrote:
> attached is a second version of a patch for the diff preview api call
> (which the gui clients like to have) that doesn't produce text deltas.
>
> Since the first version is a couple of weeks old i repeat a few notes:
>
> * this is an api only patch. For easier review i have created a small
> patch to run the preview from the command line.
>
This is good for the moment, but later I guess we want to be able to test
this API in our test suite.
> * there is a lot of code duplication between the diff and the diff
> preview code that i want to address in another patch if the preview
> stuff makes it into the repository.
>
OK. Ass you will discover, I didn't touch that part at all. We could do
this in the order you propose, but I'm not very fond of it. but I also
understand that you don't want to waste your time on lots of refactoring
that might be useless.
OK, let's make a deal. I promise to do what I can together with you to
get this into the repository (if no other full committer objects, of
course). Would you then like to do the refactoring work first, which
would probably make this patch significantly smaller?
> * the preview handles only the repository <-> repoditory case for now.
> the other combinations are another thing i would like to address in
> different patches.
>
>
So, currently BASE and WORKING revisions are not supported. OK.
> Unsolved problem
> ----------------
>
Already discussed.
> minor version compatibility:
> ----------------------------
>
> The preview patch adds an optional boolean to do_diff.
>
> To handle minor version compatibility i modified vparse_tuple to
> handle optional bool values simply by not returning an error. This
> requires to initialize the optional bool before calling vparse_tuple.
>
For revving get_commit_editor in 1.2, I solved this problem by checking
the number of parameters. I don't know if that's cleaner than the
inconsistency that your approach introduces. >
>
> preview or summarize:
> ---------------------
>
> How should it be named? preview or summarize?
> I named it preview after the inital confusion with #issue 2015
> (summarize) because my patch doesn't really solve that issue.
> But i have to problem with changing it again to summarize.
>
>
>
Doesn't it solve #2015 without the -v option, or do I miss something? I
like summarize more. (I mean make it possible to solve, since this patch
doesn't touch the cmdline client, of course.)
In the review below, I'm not picking on style, but am instead
concentrating on substantial things. But don't worry, we'll come to style
later:-)
> Index: subversion/libsvn_ra/ra_loader.c
> ===================================================================
> svn_error_t *svn_ra_do_diff (svn_ra_session_t *session,
> const svn_ra_reporter2_t **reporter,
> void **report_baton,
> @@ -423,7 +442,8 @@
> {
> return session->vtable->do_diff (session, reporter, report_baton, revision,
> diff_target, recurse, ignore_ancestry,
> - versus_url, diff_editor, diff_baton, pool);
> + TRUE, versus_url, diff_editor, diff_baton,
> + pool);
> }
>
When revving a function, implement the old API in terms of the new
one, literally even in a trivial case like this. Then we don't
have to touch it in the next revving round.
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 15693)
> +++ subversion/include/svn_client.h (working copy)
> @@ -500,6 +500,64 @@
> svn_client_commit_info2_t *
> svn_client_create_commit_info (apr_pool_t *pool);
>
> +
> +/** the difference type in an svn_diff_preview_t structure.
> + *
> + * @since New in 1.3.
> + */
> +typedef enum svn_diff_preview_kind
We use the _t suffix even on strct tags.
> +{
> + /** an unchagned item */
> + svn_diff_preview_kind_unchanged,
> +
This means no content changes, right?
> + /** an added item */
> + svn_diff_preview_kind_added,
> +
> + /** a modified item */
> + svn_diff_preview_kind_modified,
> +
> + /** a deleted item */
> + svn_diff_preview_kind_deleted
> +
> +} svn_diff_preview_kind_t;
> +
> +
> +/** a struct that describes the diff of an item. Passed to
> + * svn_diff_preview_func_t.
> + *
Please add a @note that this struct might be extended in future
versions. (See svn_wc_status2_t for an example.) I'm saying this now,
since we tend to forget about that and have to rev the APIs later for
backwards compatibility.
> + * @since New in 1.3.
> + */
> +typedef struct svn_diff_preview_t
> +{
> + /** file or dir */
> + svn_node_kind_t node_kind;
> +
> + /** item paths, relative to the old diff target and new diff target */
> + const char* path_old;
> + const char* path_new;
> +
Uh, are you not talking about a single path (I mean the part reaative
to the two targets)?
> +typedef svn_error_t *
> +(*svn_diff_preview_func_t) (void *preview_baton,
> + svn_diff_preview_t *diff);
> +
Please add a pool argument to make issue #1881 happier...
> +/**
> + * Produce a diff preview which lists the changed items between
> + * @a path1/@a revision1 and @a path2/@a revision2 without creating the
> + * delta. @a path1 and @a path2 can be either working-copy paths or URLs.
> + *
Maybe mention the limitation that the revisions might not be WORKING.
> + * Calls @a preview_func with @a preview_baton for each difference with
> + * a @c svn_diff_preview_t * structure describing the difference.
> + *
> + * See svn_client_diff3 for a description of the other parameters.
> + *
> + * @since New in 1.3.
> + */
> +svn_error_t *
> +svn_client_diff_preview (const char *path1,
> + const svn_opt_revision_t *revision1,
> + const char *path2,
> + const svn_opt_revision_t *revision2,
> + svn_boolean_t recurse,
> + svn_boolean_t ignore_ancestry,
> + svn_boolean_t no_diff_deleted,
no_diff_deleted is used to not show the contents of deleted
files. Seems not applicable here. Same for the _peg version below.
> Index: subversion/include/svn_ra.h
> ===================================================================
> --- subversion/include/svn_ra.h (revision 15693)
> +++ subversion/include/svn_ra.h (working copy)
> @@ -761,10 +761,31 @@
> * finishing the report, and may not perform any RA operations using
> * @a session from within the editing operations of @a diff_editor.
> *
> + * @a text_deltas instructs the driver of the @a diff_editor to enable
> + * the generation of text deltas.
> + *
Might be good to mention that if text_delts is FALSE, then
apply_text_delta wil be called with an empty delta to the provided
window handler.
> +/* Create an editor for a repository diff preview, i.e. comparing one
> + * repository version against the other and only providing information
> + * about the changed items without the text delta.
> + *
> + * @preview_func is called with @a preview_baton as parameter by the
> + * created svn_delta_editor_t for each changed item.
> + *
> + * See svn_client__get_diff_editor for a description of the other
> + * parameters.
> + */
> +svn_error_t *
> +svn_client__get_diff_preview_editor (const char *target,
> + svn_wc_adm_access_t *adm_access,
Is this...
> + svn_diff_preview_func_t preview_func,
> + void *preview_baton,
> + svn_boolean_t recurse,
> + svn_boolean_t dry_run,
and this argument used? (Maybe I get an answer below, but at least
dry_run only applies to merge AFAIK.)
> Index: subversion/libsvn_client/diff.c
> ===================================================================
> --- subversion/libsvn_client/diff.c (revision 15693)
> +++ subversion/libsvn_client/diff.c (working copy)
As you say, here we have a lot of duplicate code. I'm not looking into
it in detail (yet).
> Index: subversion/libsvn_client/repos_diff_preview.c
> ===================================================================
> --- subversion/libsvn_client/repos_diff_preview.c (revision 0)
> +++ subversion/libsvn_client/repos_diff_preview.c (revision 0)
> +/* An editor function. The root of the comparison hierarchy */
> +static svn_error_t *
> +set_target_revision (void *edit_baton,
> + svn_revnum_t target_revision,
> + apr_pool_t *pool)
> +{
> + return SVN_NO_ERROR;
> +}
> +
You don't need this, since that's what thedefault implementation does.
> +/* An editor function. The root of the comparison hierarchy */
> +static svn_error_t *
> +open_root (void *edit_baton,
> + svn_revnum_t base_revision,
> + apr_pool_t *pool,
> + void **root_baton)
> +{
> + struct preview_baton *nb = apr_palloc (pool, sizeof (*nb));
> + svn_diff_preview_t *diff = apr_palloc (pool, sizeof (*diff));
> +
> + diff->node_kind = svn_node_dir;
> + diff->preview_kind = svn_diff_preview_kind_unchanged;
> + diff->path_old = NULL;
> + diff->path_new = NULL;
Shouldn't this be the empty string?
> + diff->props_changed = FALSE;
> +
> + nb->edit_baton = edit_baton;
> + nb->preview = diff;
> + nb->path = NULL;
And this?
> +/* An editor function. */
> +static svn_error_t *
> +delete_entry (const char *path,
> + svn_revnum_t base_revision,
> + void *parent_baton,
> + apr_pool_t *pool)
> +{
> + struct preview_baton *pb = parent_baton;
> + svn_diff_preview_t *diff = apr_palloc (pool, sizeof (*diff));
> + struct preview_edit_baton *eb = pb->edit_baton;
> +
> + diff->node_kind = svn_node_file;
It could also be a directory. I guess you need to use *another* RA
session to figure out the kind of this path. Good that it is only
delete that require this.
> + diff->preview_kind = svn_diff_preview_kind_deleted;
> + diff->path_old = apr_pstrdup (pool, path);
> + diff->path_new = NULL;
> + diff->props_changed = FALSE;
> +
> + SVN_ERR (eb->preview_func (eb->preview_func_baton,diff));
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* An editor function. */
> +static svn_error_t *
> +add_directory (const char *path,
> + void *parent_baton,
> + const char *copyfrom_path,
> + svn_revnum_t copyfrom_revision,
> + apr_pool_t *pool,
> + void **child_baton)
> +{
> + struct preview_baton *pb = parent_baton;
> + struct preview_baton *nb = apr_palloc (pool, sizeof (*nb));
> + svn_diff_preview_t *diff = apr_palloc (pool, sizeof (*diff));
> +
> + nb->edit_baton = pb->edit_baton;
> + nb->preview = diff;
> + nb->path = apr_pstrdup (pool, path);
> +
> + diff->node_kind = svn_node_dir;
> + diff->preview_kind = svn_diff_preview_kind_added;
> + diff->path_old = apr_pstrdup (pool, copyfrom_path);
> + diff->path_new = apr_pstrdup (pool, path);
I think the use of path_old and path_new is confusing. OTOH, adding
copyfrom_path and copyfrom_rev to the preview struct might be a good
idea.
> + diff->props_changed = FALSE;
> +
> + *child_baton = nb;
> + return SVN_NO_ERROR;
> +}
> +
> +/* An editor function. */
> +static svn_error_t *
> +open_directory (const char *path,
> + void *parent_baton,
> + svn_revnum_t base_revision,
> + apr_pool_t *pool,
> + void **child_baton)
> +{
> + struct preview_baton *pb = parent_baton;
> + struct preview_baton *nb = apr_palloc (pool, sizeof (*nb));
> + svn_diff_preview_t *diff = apr_palloc (pool, sizeof (*diff));
> +
> + nb->edit_baton = pb->edit_baton;
> + nb->preview = diff;
> + nb->path = apr_pstrdup (pool, path);
> +
> + diff->node_kind = svn_node_dir;
> + diff->preview_kind = svn_diff_preview_kind_modified;
> + diff->path_old = NULL;
> + diff->path_new = apr_pstrdup (pool, path);
> + diff->props_changed = FALSE;
> +
Could the baton creation and preview struct dito be factored out into
a helper function. It seems quite similar in all these editor
functions.
> + *child_baton = nb;
> + return SVN_NO_ERROR;
> +}
> +
> +/* An editor function. */
> +static svn_error_t *
> +change_dir_prop (void *dir_baton,
> + const char *name,
> + const svn_string_t *value,
> + apr_pool_t *pool)
> +{
> + struct preview_baton *pb = dir_baton;
> +
> + if ( svn_property_kind (NULL,name) == svn_prop_regular_kind )
> + {
> + pb->preview->props_changed = TRUE;
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* An editor function. */
> +static svn_error_t *
> +close_directory (void *dir_baton,
> + apr_pool_t *pool)
> +{
> + struct preview_baton *pb = dir_baton;
> + svn_diff_preview_t *diff = pb->preview;
> + struct preview_edit_baton *eb = pb->edit_baton;
> +
> + if (!diff->path_old) /* ignore empty root */
> + return SVN_NO_ERROR;
> +
Didn't open_directory set path_old to NULL? And why ignore the root?
What happens if the root of th edit has property mods? (I haven't
tested, so I may be missing something.)
> + if (diff->preview_kind != svn_diff_preview_kind_unchanged
> + || diff->props_changed)
> + {
> + SVN_ERR (eb->preview_func (eb->preview_func_baton,diff));
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* An editor function. */
> +static svn_error_t *
> +absent_directory (const char *path,
> + void *parent_baton,
> + apr_pool_t *pool)
> +{
> + return SVN_NO_ERROR;
> +}
> +
> +/* An editor function. */
> +static svn_error_t *
> +add_file (const char *path,
> + void *parent_baton,
> + const char *copyfrom_path,
> + svn_revnum_t copyfrom_revision,
> + apr_pool_t *pool,
> + void **file_baton)
> +{
> + struct preview_baton *pb = parent_baton;
> + struct preview_baton *nb = apr_pcalloc (pool, sizeof (*nb));
> + svn_diff_preview_t *diff = apr_pcalloc (pool, sizeof (*diff));
> +
> + nb->edit_baton = pb->edit_baton;
> + nb->preview = diff;
> + nb->path = apr_pstrdup (pool, path);
> +
> + diff->node_kind = svn_node_file;
> + diff->preview_kind = svn_diff_preview_kind_added;
> + diff->path_old = apr_pstrdup (pool, copyfrom_path);
Same about path_old/path_new and copyfrom information here.
> + diff->path_new = apr_pstrdup (pool, path);
> + diff->props_changed = FALSE;
> +
> + *file_baton = nb;
> + return SVN_NO_ERROR;
> +}
> +
> +/* An editor function. */
> +static svn_error_t *
> +open_file (const char *path,
> + void *parent_baton,
> + svn_revnum_t base_revision,
> + apr_pool_t *pool,
> + void **file_baton)
> +{
> + /*
> + This code is also called for files that are not changed.
> +
> + It looks like this has something todo with merging.
> +
> + After creating a branch from trunk and merging several files
> + from trunk to that branch (making the files on the branch and
> + trunk equal again) they appear in a diff preview between the
> + branch and trunk.
> +
> + Is this supposed to happen? And if so how do i recognize
> + this situation so i can filter those files?
> + */
Just a tip for the future: When we add a comment regarding some issue
or incompleteness with the code like the above, we use ### markers, so
that such comments are easier to find in the future.
> + struct preview_baton *pb = parent_baton;
> + struct preview_baton *nb = apr_palloc (pool, sizeof (*nb));
> + svn_diff_preview_t *diff = apr_palloc (pool, sizeof (*diff));
> +
> + nb->edit_baton = pb->edit_baton;
> + nb->preview = diff;
> + nb->path = apr_pstrdup (pool, path);
> +
> + diff->node_kind = svn_node_file;
> + diff->preview_kind = svn_diff_preview_kind_modified;
As we discussed earlier in this thread, you should assume that the
contents are not modified here.
> +/* An editor function. Do the work of applying the text delta. */
> +static svn_error_t *
> +window_handler (svn_txdelta_window_t *window,
> + void *window_baton)
This function is not used.
> +/* An editor function. */
> +static svn_error_t *
> +apply_textdelta (void *file_baton,
> + const char *base_checksum,
> + apr_pool_t *pool,
> + svn_txdelta_window_handler_t *handler,
> + void **handler_baton)
> +{
> + *handler = svn_delta_noop_window_handler;
> + *handler_baton = NULL;
> +
Here you note that the contents were modified.
> + return SVN_NO_ERROR;
> +}
> +
> +/* An editor function. */
> +static svn_error_t *
> +change_file_prop (void *file_baton,
> + const char *name,
> + const svn_string_t *value,
> + apr_pool_t *pool)
> +{
> + struct preview_baton *pb = file_baton;
> +
> + if ( svn_property_kind (NULL,name) == svn_prop_regular_kind )
> + {
> + pb->preview->props_changed = TRUE;
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
This function feels very similar to change_dir_prop. Since you use
the same kind of baton for files and dirs, you could have just one
function.
> +/* An editor function. */
> +static svn_error_t *
> +absent_file (const char *path,
> + void *parent_baton,
> + apr_pool_t *pool)
> +{
> + struct preview_dir_baton *pb = parent_baton;
> +
> + return SVN_NO_ERROR;
> +}
> +
This is similar to absent_directory and you shouldn't eed either of
them since the default editor already provide dummy implementations.
> +/* An editor function. */
> +static svn_error_t *
> +close_edit (void *edit_baton,
> + apr_pool_t *pool)
> +{
> + return SVN_NO_ERROR;
> +}
> +
You don't need to provide this.
> +
> +
> +/* Create a repository diff preview editor and baton. */
> +svn_error_t *
> +svn_client__get_diff_preview_editor (const char *target,
> + svn_wc_adm_access_t *adm_access,
> + svn_diff_preview_func_t preview_func,
> + void *preview_baton,
> + svn_boolean_t recurse,
> + svn_boolean_t dry_run,
> + svn_ra_session_t *ra_session,
> + svn_revnum_t revision,
> + svn_wc_notify_func2_t notify_func,
> + void *notify_baton,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + const svn_delta_editor_t **editor,
> + void **edit_baton,
> + apr_pool_t *pool)
> +{
> + apr_pool_t *subpool = svn_pool_create (pool);
> +
You don't need a subpool in this function. See about pool usage in
the hacking document.
> + svn_delta_editor_t *tree_editor = svn_delta_default_editor (subpool);
> + struct preview_edit_baton *eb = apr_palloc (subpool, sizeof (*eb));
> +
> + eb->preview_func = preview_func;
> + eb->preview_func_baton = preview_baton;
> +
> + tree_editor->set_target_revision = set_target_revision;
> + tree_editor->open_root = open_root;
> +
> + tree_editor->delete_entry = delete_entry;
> + tree_editor->add_directory = add_directory;
> + tree_editor->open_directory = open_directory;
> + tree_editor->change_dir_prop = change_dir_prop;
> + tree_editor->close_directory = close_directory;
> + tree_editor->absent_directory = absent_directory;
> +
> + tree_editor->add_file = add_file;
> + tree_editor->open_file = open_file;
> + tree_editor->apply_textdelta = apply_textdelta;
> + tree_editor->change_file_prop = change_file_prop;
> + tree_editor->close_file = close_file;
> + tree_editor->absent_file = absent_file;
> +
> + tree_editor->close_edit = close_edit;
> +
> + SVN_ERR (svn_delta_get_cancellation_editor
> + (cancel_func, cancel_baton, tree_editor, eb, editor, edit_baton,
> + pool));
> +
> + return SVN_NO_ERROR;
> +}
There are some unused arguments that you could get rid of here.
> Index: subversion/libsvn_ra_svn/client.c
> ===================================================================
> --- subversion/libsvn_ra_svn/client.c (revision 15693)
> +++ subversion/libsvn_ra_svn/client.c (working copy)
> @@ -1050,6 +1050,7 @@
> svn_revnum_t rev, const char *target,
> svn_boolean_t recurse,
> 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)
> @@ -1058,8 +1059,9 @@
> svn_ra_svn_conn_t *conn = sess_baton->conn;
>
> /* Tell the server we want to start a diff. */
> - SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "diff", "(?r)cbbc", rev, target,
> - recurse, ignore_ancestry, versus_url));
> + SVN_ERR(svn_ra_svn_write_cmd(conn, pool, "diff", "(?r)cbbcb", rev,
> + target, recurse, ignore_ancestry,
> + versus_url, text_deltas));
> SVN_ERR(handle_auth_request(sess_baton, pool));
>
This means that old servers will send us full text deltas anyway. I
think that's fine, but we should document in the svn_ra_diff2
docstring that this might happen.
> /* Fetch a reporter for the caller to drive. The reporter will drive
> Index: subversion/libsvn_ra_svn/protocol
> ===================================================================
> --- subversion/libsvn_ra_svn/protocol (revision 15693)
> +++ subversion/libsvn_ra_svn/protocol (working copy)
> @@ -292,7 +292,8 @@
> response: ( )
>
> diff
> - params: ( [ rev:number ] target:string recurse:bool url:string )
> + params: ( [ rev:number ] target:string recurse:bool ignore-ancestry:bool
> + url:string ? text-deltas:bool )
Thanks for caring about the protocol document. You did an unrelated
fix to the prototype, which I committed in r15729. Thanks for that.
> 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/libsvn_ra_svn/marshal.c
> ===================================================================
> --- subversion/libsvn_ra_svn/marshal.c (revision 15693)
> +++ subversion/libsvn_ra_svn/marshal.c (working copy)
> @@ -691,6 +691,10 @@
> case 'n':
> *va_arg(*ap, apr_uint64_t *) = SVN_RA_SVN_UNSPECIFIED_NUMBER;
> break;
> + case 'b':
> + /* an optional bool value is supposed to have a default value
> + already set, so simply skip over it. */
> + break;
> case '(':
> list_level++;
> break;
As I said before, I'm not sure about this. But if you do this (and we
accept it), you should update the docstring for svn_ra_svn_parse_tuple
in svn_ra_svn.h.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Aug 18 14:19:41 2005