Just for the record svk has this feature and it's called
svk diff --summarize
I'd suggest that if the svn client were to adopt this feature you
stick with the same name unless you have a good reason not to. Thus
calling it summarize instead of preview.
Michael
On Jul 23, 2005, at 7:10 AM, Julian Foad wrote:
> 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
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Jul 23 18:42:31 2005