Martin Hauner wrote:
>
> attached is a patch whichh adds new diff preview api calls (which the
> gui clients like to have) that doesn't produce text deltas.
Thanks. I don't have the energy to review this fully. I'm just scanning
through it and giving whatever comments I can, enough to get you going on the
next iteration.
[...]
> Would you like to see some code to run the preview from "svn" too or
> will you accept an api only patch?
Personally I don't think it's a firm requirement that the command-line client
should be able to invoke this, but there ought to be some way of invoking it
for testing purposes - maybe through a dedicated test program, or maybe adding
an interface to the command-line client is the easiest way to do this.
> May i change vparse_tuple to ignore an (unspecified) optional boolean?
I imagine so, though I'm not at all familiar with that area. Please provide a
separate patch that does so, that we could apply first.
> subversion/libsvn_ra_svn/protocol
> ---------------------------------
>
> should the protocol file be updated?
Certainly! Are you volunteering?
> at least for diff it looks incomplete, it doesn't list the
> ignore_ancestry bool parameter.
> code duplication
> ----------------
>
> I have added some helper stuff to avoid a lot of code duplication
> between diff_preview and diff. The helper stuff is currently only
> called by the new diff_preview methods. I haven't changed the diff
> methods so far.
>
> Would you like to see that in another patch or should I add it to
> this patch? I would prefer another patch to have the two issues
> (new feature, remove code duplication) seperated.
Either way is fine by me.
> handles only repository/repository case
> ---------------------------------------
>
> the diff preview does currently only handle diff previews between
> two repository urls. I would like to add the other cases in another
> patch. The current patch is already getting large.
>
> Is this ok?
Yes. However, I wouldn't like to commit a patch that has commented-out code or
"#ifdef 0" or much in the way of "TODO" comments in it; if it is to be a
partial implementation, make it a clean implementation of that part of
functionality.
> Bug?
> ----
>
> Strange is that the diff preview sometimes lists files as changed
> that have not changed, ie. the preview delta editors open_file
> method gets called. If i run a normal svn diff on the same urls
> the files are not part of the patch.
I haven't investigated.
> ------------------------------------------------------------------------
>
> Added new svn_client api calls svn_diff_preview & svn_diff_preview_peg.
> A diff preview lists the changed items without creating text deltas.
>
> To achieve this the svn_ra__vtable_t do_diff function got a new boolean
> parameter text_deltas that is finally passed to svn_repos_begin_report
> to run the diff without creating text deltas.
>
>
> * subversion/include/svn_ra.h
> (svn_ra_do_diff2): new method, similar to svn_ra_do_diff but with a
> new text_deltas boolean parameter.
> (svn_ra_do_diff): updated method description, added deprecated note.
For "added deprecated note" you can just say "Deprecated."
> (svn_ra_plugin_t): do_diff, added new text_deltas boolean parameter.
What's "do_diff" supposed to mean in this sentence? Maybe this sentence should
be the same as the next one below.
>
> * subversion/libsvn_ra/ra_loader.h
> (svn_ra__vtable_t): added text_deltas parameter to do_diff function.
>
> * subversion/libsvn_ra/ra_loader.c
> (svn_ra_do_diff2): new method.
> (svn_ra_do_diff): hardcoded TRUE as value for the new text_deltas
> parameter of do_diff.
>
> * subversion/libsvn_ra/wrapper_template.h
> (compat_do_diff): added text_deltas parameter, pass new parameter to
> do_diff.
>
> * subversion/include/svn_diff_preview.h
We don't appear to have created new header files in other similar cases. Why
is this different?
> new file, callback declaration for svn_client_diff_preview and
> for svn_client_diff_preview_peg.
>
> * subversion/include/svn_client.h
> (svn_client_diff_preview,svn_client_diff_preview_peg): new methods.
>
> * subversion/libsvn_client/client.h
> (svn_client__get_diff_preview_editor): new method, implemented in
> subversion/libsvn_client/repos_diff_preview.c.
>
> * subversion/libsvn_client/repos_diff_preview.c
> new file, the implementation of the diff preview svn_delta_editor_t.
>
> * subversion/libsvn_client/diff.c
> (diff_parameters,diff_paths,diff_repos_repos): new helper structures
> used by the new helper functions.
> (svn_client_check_paths,svn_client_check_paths_peg,
The "svn_LIBNAME_" prefix on function names is for public API symbols; your
static functions don't need it and shouldn't have it.
> diff_prepare_repos_repos): new helper methods used to remove most of
> the code duplication between diff and diff preview.
> (diff_preview_repos_repos): do diff preview between two repository
> paths.
> (do_diff_preview): new method, diff preview without peg revision.
> (do_diff_preview_peg): new method, diff preview with peg revision.
> (svn_client_diff_preview): new method, implemementation of public api
> diff preview function (without peg revision).
> (svn_client_diff_preview_peg): new method, implemementation of public
> api diff preview peg function.
>
> * subversion/libsvn_ra_dav/ra_dav.h
> (svn_ra_dav__do_diff): added text_deltas boolean parameter.
>
> * subversion/libsvn_ra_dav/fetch.c
> (svn_ra_dav__do_diff): added text_deltas boolean parameter. Passing
> new parameter to make_reporter.
>
> * subversion/libsvn_ra_local/ra_plugin.c
> (svn_ra_local__do_diff): added text_deltas boolean parameter. Passing
> new parameter to make_reporter.
>
> * subversion/libsvn_ra_svn/client.c
> (ra_svn_diff): added text_deltas boolean parameter. Added text_deltas
> to "diff" cmd.
>
> * subversion/svnserve/serve.c
> (diff): added parsing of the new text_deltas parameter. Passing new
> parameter to accept_report.
> Index: subversion/include/svn_diff_preview.h
> ===================================================================
[...]
> +typedef void (*svn_diff_preview_func_t) (void *preview_baton,
> + svn_diff_preview_t *diff);
Might this not need to return an error? Usually we find that a callback
function does need to be able to return an error, and sometimes we have found
that after first making it public without the ability to do so, so I think this
should return "svn_error_t *".
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 15310)
> +++ subversion/include/svn_client.h (working copy)
> @@ -40,8 +40,8 @@
> #include "svn_error.h"
> #include "svn_opt.h"
> #include "svn_version.h"
> +#include "svn_diff_preview.h"
>
> -
Please try to avoid spurious whitespace changes.
> #ifdef __cplusplus
> extern "C" {
> #endif /* __cplusplus */
> @@ -1203,6 +1203,57 @@
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
>
> +/**
> + * 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.
> + *
> + * 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,
> + svn_diff_preview_func_t preview_func,
> + void *preview_baton,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool);
> +
> +/**
> + * Produce diff preview which lists the changed items between the
"Produce a diff ..."
> + * filesystem object @a path in peg revision @a peg_revision, as it
> + * changed between @a start_revision and @a end_revision. @a path can
> + * be either a working-copy path or URL.
> + *
> + * 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_diff_peg3 for a description of the other parameters.
> + *
> + * @since New in 1.3.
> + */
> +svn_error_t *
> +svn_client_diff_preview_peg (const char *path,
> + const svn_opt_revision_t *peg_revision,
> + const svn_opt_revision_t *start_revision,
> + const svn_opt_revision_t *end_revision,
> + svn_boolean_t recurse,
> + svn_boolean_t ignore_ancestry,
> + svn_boolean_t no_diff_deleted,
> + svn_diff_preview_func_t preview_func,
> + void *preview_baton,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool);
> +
> /** Merge changes from @a source1/@a revision1 to @a source2/@a revision2 into
> * the working-copy path @a target_wcpath.
> *
> Index: subversion/libsvn_ra_local/ra_plugin.c
> ===================================================================
> --- subversion/libsvn_ra_local/ra_plugin.c (revision 15310)
> +++ subversion/libsvn_ra_local/ra_plugin.c (working copy)
> @@ -697,6 +697,7 @@
> const char *update_target,
> svn_boolean_t recurse,
> svn_boolean_t ignore_ancestry,
> + svn_boolean_t text_deltas,
> const char *switch_url,
> const svn_delta_editor_t *update_editor,
> void *update_baton,
> @@ -708,7 +709,7 @@
> update_revision,
> update_target,
> switch_url,
> - TRUE,
> + text_deltas,
> recurse,
> ignore_ancestry,
> update_editor,
> Index: subversion/libsvn_client/client.h
> ===================================================================
> --- subversion/libsvn_client/client.h (revision 15310)
> +++ subversion/libsvn_client/client.h (working copy)
> @@ -400,6 +400,37 @@
>
> /* ---------------------------------------------------------------- */
>
> +/*** Editor for diff preview ***/
> +
> +/* 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,
> + 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);
> +
> +/* ---------------------------------------------------------------- */
> +
> /*** Commit Stuff ***/
>
> /* WARNING: This is all new, untested, un-peer-reviewed conceptual
> Index: subversion/libsvn_client/diff.c
> ===================================================================
> --- subversion/libsvn_client/diff.c (revision 15310)
> +++ subversion/libsvn_client/diff.c (working copy)
> @@ -1397,8 +1397,257 @@
> return SVN_NO_ERROR;
> }
>
> +/** Helper structure: for passing around the diff parameters */
> +struct diff_parameters
> +{
> + /* first input path */
> + const char *path1;
>
> + /* revision of first input path */
> + const svn_opt_revision_t *revision1;
>
> + /* second input path */
> + const char *path2;
> +
> + /* revision of second input path */
> + const svn_opt_revision_t *revision2;
> +
> + /* peg revision */
> + const svn_opt_revision_t *peg_revision;
> +
> + /* recurse */
> + svn_boolean_t recurse;
> +
> + /* ignore acestry */
> + svn_boolean_t ignore_ancestry;
> +
> + /* ignore deleted */
> + svn_boolean_t no_diff_deleted;
> +};
> +
> +/** Helper structure: filled by svn_client_check_paths(_peg) */
> +struct diff_paths
> +{
> + /* revision1 is a local revision */
> + svn_boolean_t is_local_rev1;
> +
> + /* revision2 is a local revision */
> + svn_boolean_t is_local_rev2;
> +
> + /* path1 is a repos url */
> + svn_boolean_t is_repos_path1;
> +
> + /* path2 is a repos url, not used by svn_client_check_paths_peg */
> + svn_boolean_t is_repos_path2;
> +};
> +
> +
> +/** check if paths are urls and if the revisions are local for a diff
> + * with two source paths or urls. Fills the @a paths structure. */
> +static svn_error_t *
> +svn_client_check_paths (const struct diff_parameters* params,
> + struct diff_paths* paths)
> +{
> + /* Either path could be a URL or a working copy path. Let's figure
> + out what's what. */
> + paths->is_repos_path1 = svn_path_is_url (params->path1);
> + paths->is_repos_path2 = svn_path_is_url (params->path2);
> +
> + /* Verify our revision arguments in light of the paths. */
> + if ((params->revision1->kind == svn_opt_revision_unspecified)
> + || (params->revision2->kind == svn_opt_revision_unspecified))
> + return svn_error_create (SVN_ERR_CLIENT_BAD_REVISION, NULL,
> + _("Not all required revisions are specified"));
> +
> + /* Revisions can be said to be local or remote. BASE and WORKING,
> + for example, are local. */
> + paths->is_local_rev1 =
> + ((params->revision1->kind == svn_opt_revision_base)
> + || (params->revision1->kind == svn_opt_revision_working));
> + paths->is_local_rev2 =
> + ((params->revision2->kind == svn_opt_revision_base)
> + || (params->revision2->kind == svn_opt_revision_working));
> +
> + /* Working copy paths with non-local revisions get turned into
> + URLs. We don't do that here, though. We simply record that it
> + needs to be done, which is information that helps us choose our
> + diff helper function. */
> + if ((! paths->is_repos_path1) && (! paths->is_local_rev1))
> + paths->is_repos_path1 = TRUE;
> + if ((! paths->is_repos_path2) && (! paths->is_local_rev2))
> + paths->is_repos_path2 = TRUE;
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/** check if path is a url and if the revisions are local for a peg
> + * diff with a single source path or url. */
> +static svn_error_t *
> +svn_client_check_paths_peg (const struct diff_parameters* params,
> + struct diff_paths* paths)
> +{
> + /* Either path could be a URL or a working copy path. Let's figure
> + out what's what. */
Comment doesn't match code.
> + paths->is_repos_path1 = svn_path_is_url (params->path1);
> + paths->is_repos_path2 = FALSE;
> +
> + /* Verify our revision arguments in light of the paths. */
> + if ((params->revision1->kind == svn_opt_revision_unspecified)
> + || (params->revision2->kind == svn_opt_revision_unspecified))
> + return svn_error_create (SVN_ERR_CLIENT_BAD_REVISION, NULL,
> + _("Not all required revisions are specified"));
> +
> + /* Revisions can be said to be local or remote. BASE and WORKING,
> + for example, are local. */
> + paths->is_local_rev1 =
> + ((params->revision1->kind == svn_opt_revision_base)
> + || (params->revision1->kind == svn_opt_revision_working));
> + paths->is_local_rev2 =
> + ((params->revision2->kind == svn_opt_revision_base)
> + || (params->revision2->kind == svn_opt_revision_working));
> +
> + if (paths->is_local_rev1 && paths->is_local_rev2)
> + return svn_error_create (SVN_ERR_CLIENT_BAD_REVISION, NULL,
> + _("At least one revision must be non-local for "
> + "a pegged diff"));
> + return SVN_NO_ERROR;
> +}
This function has a lot in common with the previous one - try to factor it out.
> +
> +/** Helper structure filled by diff_prepare_repos_repos */
> +struct diff_repos_repos
> +{
> + /* url created from path1 */
> + const char *url1;
> +
> + /* url created from path2 */
> + const char *url2;
> +
> + /* the BASE_PATH for the diff */
> + const char *base_path;
> +
> + /* url1 and url2 are the same */
> + svn_boolean_t same_urls;
> +
> + /* revision of url1 */
> + svn_revnum_t rev1;
> +
> + /* revision of url2 */
> + svn_revnum_t rev2;
> +
> + /* anchor & target based on url1 */
> + const char *anchor1;
> + const char *target1;
Style: Either group all the "1" fields together and then the "2" fields like
you are starting to do here, or pair up the "1" and "2" of each field like you
were doing at the beginning of this structure. This mixture of layout styles
is a little confusing.
> +
> + /* anchor & target based on url2 */
> + const char *anchor2;
> + const char *target2;
> +};
[...]
> +/* Perform a diff preview between two repository paths. */
> +static svn_error_t *
> +diff_preview_repos_repos (const struct diff_parameters* diff_param,
> + svn_diff_preview_func_t preview_func,
> + void *preview_baton,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + svn_ra_session_t *ra_session1, *ra_session2;
>
> + const svn_ra_reporter2_t *reporter;
> + void *report_baton;
> +
> + const svn_delta_editor_t *diff_editor;
> + void *diff_edit_baton;
> +
> + struct diff_repos_repos drr;
> +
> + /* prepare info for the repos repos diff */
> + SVN_ERR (diff_prepare_repos_repos (diff_param, &drr, ctx, pool ));
> +
> + /* Now, we reopen two RA session to the correct anchor/target
> + locations for our URLs. */
> + SVN_ERR (svn_client__open_ra_session_internal (&ra_session1,
> + drr.anchor1,
> + NULL, NULL, NULL,
> + FALSE, TRUE,
> + ctx, pool));
> + SVN_ERR (svn_client__open_ra_session_internal (&ra_session2,
> + drr.anchor1,
> + NULL, NULL, NULL,
> + FALSE, TRUE,
> + ctx, pool));
> +
> + /* Set up the repos_diff editor on BASE_PATH, if available.
> + Otherwise, we just use "". */
> + SVN_ERR (svn_client__get_diff_preview_editor (drr.base_path ?
> + drr.base_path : "",
> + NULL,
> + preview_func,
> + preview_baton,
> + diff_param->recurse,
> + /* doesn't matter for diff */ FALSE,
> + ra_session2,
> + drr.rev1,
> + /* no notify_func */ NULL,
> + /* no notify_baton */ NULL,
> + ctx->cancel_func,
> + ctx->cancel_baton,
> + &diff_editor,
> + &diff_edit_baton,
> + pool));
Style: while one parameter per line is great for declarations, in a calling
situation like this move the second open-parenthesis onto the next line (in the
column after the first one) and close up the arguments, to avoid using such a
lot of vertical space.
[...]
> 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,
> + svn_diff_preview_func_t preview_func,
> + void *preview_baton,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + struct diff_parameters diff_params;
> +
> + /* We will never do a pegged diff from here. */
> + svn_opt_revision_t* peg_revision = apr_palloc (pool,
> + sizeof(*peg_revision));
It's only a little structure and doesn't need a long lifetime, so just use a
stack variable.
> + peg_revision->kind = svn_opt_revision_unspecified;
> +
> + /* fill diff_param */
> + diff_params.path1 = path1;
> + diff_params.revision1 = revision1;
> + diff_params.path2 = path2;
> + diff_params.revision2 = revision2;
> + diff_params.peg_revision = peg_revision;
> + diff_params.recurse = recurse;
> + diff_params.ignore_ancestry = ignore_ancestry;
> + diff_params.no_diff_deleted = no_diff_deleted;
> +
> + return do_diff_preview (&diff_params, preview_func, preview_baton,
> + ctx, pool);
Hmm. Does this marshalling of the arguments into a structure actually help?
It looks to me like it just makes the whole thing longer. It's not as if there
is anything happening here in this wrapper that justifies the implementation
being a separate function, is there?
> +}
[...]
> Index: subversion/libsvn_client/repos_diff_preview.c
> ===================================================================
[...]
> +/* Directory level baton.
> + */
> +struct preview_dir_baton {
> +
> + /* the overall crawler editor baton */
> + struct preview_edit_baton *edit_baton;
> +
> + /* the preview filled by the editor calls */
> + svn_diff_preview_t *preview;
> +
> + /* The path of the directory within the repository */
> + const char *path;
> +};
> +
> +
> +/* File level baton.
> + */
> +struct preview_file_baton {
> +
> + /* the overall crawler editor baton */
> + struct preview_edit_baton *edit_baton;
> +
> + /* the preview filled by the editor calls */
> + svn_diff_preview_t *preview;
> +
> + /* The path of the file within the repository */
> + const char *path;
> +};
If that struct is identical to the preview_dir_baton, as it appears to be, just
typedef it if you want to keep the names distinct, or use the same type for both.
> +
> +
> +/* 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)
> +{
> + struct preview_edit_baton *eb = edit_baton;
Redundant line. Similarly in some other functions below.
> +
> + return SVN_NO_ERROR;
> +}
> +
> +/* 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_edit_baton *eb = edit_baton;
> + struct preview_dir_baton *db = apr_pcalloc (pool, sizeof (*db));
> + svn_diff_preview_t *diff = apr_pcalloc (pool, sizeof (*diff));
It doesn't look like you need this memory to be cleared when it is allocated,
as you are filling in all the fields. Just use apr_palloc.
> +
> + diff->node_kind = svn_node_dir;
> + diff->preview_kind = svn_diff_preview_kind_unchanged;
> + diff->path_old = 0;
Style: please write "NULL" rather than "0" for the null pointer, throughout.
> + diff->path_new = 0;
> + diff->props_changed = FALSE;
> + diff->mimetype_new = 0;
> + diff->mimetype_old = 0;
[...]
> +/* 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_dir_baton *pb = parent_baton;
> + struct preview_dir_baton *db = apr_pcalloc (pool, sizeof (*db));
> + svn_diff_preview_t *diff = apr_pcalloc (pool, sizeof (*diff));
> +
> + db->edit_baton = pb->edit_baton;
> + db->preview = diff;
> + db->path = apr_pstrdup (pool, path);
> +
> + diff->node_kind = svn_node_dir;
> + diff->preview_kind = svn_diff_preview_kind_unchanged;
Hmm... do we really want a directory to be reported as "unchanged" when
something in it has changed? I would think that saying "changed" for every
directory opened would be more accurate, assuming that directories are only
opened if they contain a change.
> + diff->path_old = apr_pstrdup (pool, path);
> + diff->path_new = 0;
> + diff->props_changed = FALSE;
> + diff->mimetype_new = 0;
> + diff->mimetype_old = 0;
> +
> + *child_baton = db;
> + return SVN_NO_ERROR;
> +}
[...]
> +/* An editor function. */
> +static svn_error_t *
> +close_directory (void *dir_baton,
> + apr_pool_t *pool)
> +{
> + struct preview_dir_baton *db = dir_baton;
> +
> + if (db->preview->path_old // ignore empty root
> + && (db->preview->preview_kind != svn_diff_preview_kind_unchanged
> + || db->preview->props_changed == TRUE))
Style: please don't write "== TRUE".
> + {
> + db->edit_baton->preview_func (db->edit_baton->preview_baton,db->preview);
> + }
> +
> + 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_dir_baton *pb = parent_baton;
> + struct preview_file_baton *fb = apr_pcalloc (pool, sizeof (*fb));
> + svn_diff_preview_t *diff = apr_pcalloc (pool, sizeof (*diff));
> +
> + fb->edit_baton = pb->edit_baton;
> + fb->preview = diff;
> + fb->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, path);
> + diff->path_new = 0;
Huh? Shouldn't the added file be the new file, and the null file the old file?
Same for the add-directory case.
> + diff->props_changed = FALSE;
> + diff->mimetype_new = 0;
> + diff->mimetype_old = 0;
> +
> + *file_baton = fb;
> + 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)
> +{
> + struct preview_dir_baton *pb = parent_baton;
> + struct preview_file_baton *fb = apr_pcalloc (pool, sizeof (*fb));
> + svn_diff_preview_t *diff = apr_pcalloc (pool, sizeof (*diff));
> +
> + fb->edit_baton = pb->edit_baton;
> + fb->preview = diff;
> + fb->path = apr_pstrdup (pool, path);
> +
> + diff->node_kind = svn_node_file;
> + diff->preview_kind = svn_diff_preview_kind_modified;
> + diff->path_old = apr_pstrdup (pool, path);
> + diff->path_new = 0;
New path should be filled in? Same for the open-directory case.
> + diff->props_changed = FALSE;
> + diff->mimetype_new = 0;
> + diff->mimetype_old = 0;
> +
> + *file_baton = fb;
> + return SVN_NO_ERROR;
> +}
[...]
> +/* 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);
> +
> + 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_baton = preview_baton;
> + //eb->revision = revision;
> + //eb->target_revision = SVN_INVALID_REVNUM;
You can't use C++-style comments. More to the point, why are these lines
commented out?
> Index: subversion/libsvn_ra_svn/marshal.c
> ===================================================================
> --- subversion/libsvn_ra_svn/marshal.c (revision 15310)
> +++ subversion/libsvn_ra_svn/marshal.c (working copy)
> @@ -691,6 +691,11 @@
> case 'n':
> *va_arg(*ap, apr_uint64_t *) = SVN_RA_SVN_UNSPECIFIED_NUMBER;
> break;
> +#if 0
> + case 'b':
> + *va_arg(*ap, svn_boolean_t *) = TRUE;
> + break;
> +#endif
(I'm just noting this unfinished part so I'll look again in the next iteration.)
> case '(':
> list_level++;
> break;
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jul 23 16:11:31 2005