Skip to content

Commit

Permalink
Reduce coupling between diff drivers and diff writers.
Browse files Browse the repository at this point in the history 
Let diff drivers anchor the diff-processor at the requested target paths,
rather than sometimes there and sometimes at the parent of one of them, in
summarize mode. The quirky anchoring still persists in non-summarize mode.

* subversion/libsvn_client/diff.c
  (diff_driver_info_t): Expand comments.
  (diff_wc_wc,
   diff_repos_repos,
   diff_repos_wc): Remove 'root_relpath' and 'root_is_dir' outputs; instead
    anchor the diff processor at the requested targets if no 'ddi' coupling
    parameter is given.
  (do_diff,
   svn_client_diff7,
   svn_client_diff_peg7,
   svn_client_diff_summarize2,
   svn_client_diff_summarize_peg2): Update the calls accordingly.

* subversion/libsvn_client/diff_local.c,
  subversion/libsvn_client/client.h
  (svn_client__arbitrary_nodes_diff): Remove 'root_relpath' and 'root_is_dir'
    outputs. Take an 'anchor_at_given_paths' mode flag and obey it.

* subversion/libsvn_client/diff_summarize.c,
  subversion/libsvn_client/client.h
  (summarize_baton_t,
   send_summary): Remove path adjustment.
  (svn_client__get_diff_summarize_callbacks): Remove path adjustment and
    an unused parameter.

* subversion/libsvn_wc/diff_local.c,
  subversion/include/private/svn_wc_private.h
  (svn_wc__diff7): Remove 'root_relpath' and 'root_is_dir' outputs. Take an
    'anchor_at_given_paths' mode flag and obey it.
  (svn_wc_diff6): Update the call to svn_wc__diff7() accordingly.


git-svn-id: https://svn.apache.org/repos/asf/subversion/trunk@1835234 13f79535-47bb-0310-9956-ffa450edef68
  • Loading branch information
@julianfoad
julianfoad committed Jul 6, 2018
1 parent b6dfa2b commit d29903f
Show file tree
Hide file tree
Showing 6 changed files with 109 additions and 124 deletions.
22 changes: 5 additions & 17 deletions subversion/include/private/svn_wc_private.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2044,31 +2044,19 @@ svn_wc__acquire_write_lock_for_resolve(const char **lock_root_abspath,

/* The implemementation of svn_wc_diff6(), but reporting to a diff processor
*
* New mode: when ROOT_RELPATH is not NULL:
* New mode, when ANCHOR_AT_GIVEN_PATHS is true:
*
* If LOCAL_ABSPATH is the WC root, set *ROOT_RELPATH to "" and send
* diff processor relpaths relative to LOCAL_ABSPATH; otherwise set
* *ROOT_RELPATH to the last component of LOCAL_ABSPATH and send diff
* processor relpaths relative to the parent of LOCAL_ABSPATH.
* (*ROOT_RELPATH will be either "" or a single path component.)
* Anchor the DIFF_PROCESSOR at LOCAL_ABSPATH.
*
* Backward compatibility mode for svn_wc_diff6(): when ROOT_RELPATH is NULL:
* Backward compatibility mode for svn_wc_diff6(),
* when ANCHOR_AT_GIVEN_PATHS is false:
*
* Send diff processor relpaths relative to LOCAL_ABSPATH if it is a
* directory; otherwise, relative to the parent of LOCAL_ABSPATH.
* This matches the "anchor and target" semantics of svn_wc_diff6().
*
* If ROOT_IS_DIR is not NULL:
*
* Set *ROOT_IS_DIR to TRUE if the working version of LOCAL_ABSPATH is
* a directory, else to FALSE.
*
* Assignments to *ROOT_RELPATH and *ROOT_IS_DIR are made before the first
* call to DIFF_PROCESSOR.
*/
svn_error_t *
svn_wc__diff7(const char **root_relpath,
svn_boolean_t *root_is_dir,
svn_wc__diff7(svn_boolean_t anchor_at_given_paths,
svn_wc_context_t *wc_ctx,
const char *local_abspath,
svn_depth_t depth,
Expand Down
45 changes: 22 additions & 23 deletions subversion/libsvn_client/client.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -687,23 +687,21 @@ svn_client__get_diff_editor2(const svn_delta_editor_t **editor,
/* Set *DIFF_PROCESSOR to a diff processor that will report a diff summary
to SUMMARIZE_FUNC.
P_ROOT_RELPATH will return a pointer to a string that must be set to
the root of the operation before the processor is called.
P_ROOT_RELPATH will return a pointer to a string that must be set,
before the processor is called, to a prefix that will be found on
every DIFF_PROCESSOR relpath, that will be removed before passing
the path to SUMMARIZE_FUNC.
ORIGINAL_PATH specifies the original path and will be used with
**ANCHOR_PATH to create paths as the user originally provided them
to the diff function.
ORIGINAL_TARGET is not used.
SUMMARIZE_FUNC is called with SUMMARIZE_BATON as parameter by the
created callbacks for each changed item.
*/
svn_error_t *
svn_client__get_diff_summarize_callbacks(
const svn_diff_tree_processor_t **diff_processor,
const char ***p_root_relpath,
svn_client_diff_summarize_func_t summarize_func,
void *summarize_baton,
const char *original_target,
apr_pool_t *result_pool,
apr_pool_t *scratch_pool);

Expand Down Expand Up @@ -1122,28 +1120,29 @@ svn_client__resolve_conflicts(svn_boolean_t *conflicts_remain,
*
* No copy or move information is reported to the diff processor.
*
* If both LEFT_ABSPATH and RIGHT_ABSPATH are directories on disk:
* set *ROOT_RELPATH to "" and
* set *ROOT_IS_DIR to TRUE and
* send diff processor relpaths relative to LEFT_ABSPATH
* (which is the same as relative to RIGHT_ABSPATH);
* else:
* set *ROOT_RELPATH to the basename of LEFT_ABSPATH and
* set *ROOT_IS_DIR to FALSE and
* send diff processor relpaths relative to the parent of LEFT_ABSPATH
* (so they all start with a basename(LEFT_ABSPATH) component).
* If ANCHOR_AT_GIVEN_PATHS is true:
*
* Anchor the DIFF_PROCESSOR at the requested diff targets
* (LEFT_ABSPATH, RIGHT_ABSPATH).
*
* If ANCHOR_AT_GIVEN_PATHS is false:
*
* If both LEFT_ABSPATH and RIGHT_ABSPATH are directories on disk:
*
* Anchor the DIFF_PROCESSOR at the requested diff targets
* (LEFT_ABSPATH, RIGHT_ABSPATH).
*
* else:
*
* Anchor the DIFF_PROCESSOR at the parent of LEFT_ABSPATH
* (so the paths all start with a basename(LEFT_ABSPATH) component).
*
* As any children reached by recursion are matched by name, a diff
* processor relpath applies equally to both sides of the diff, except
* for its first component in the latter case above.
*
* Assignments to *ROOT_RELPATH and *ROOT_IS_DIR are made before the first
* call to DIFF_PROCESSOR. Each of ROOT_RELPATH and ROOT_IS_DIR may be NULL
* if not wanted.
*/
svn_error_t *
svn_client__arbitrary_nodes_diff(const char **root_relpath,
svn_boolean_t *root_is_dir,
svn_client__arbitrary_nodes_diff(svn_boolean_t anchor_at_given_paths,
const char *left_abspath,
const char *right_abspath,
svn_depth_t depth,
Expand Down
96 changes: 51 additions & 45 deletions subversion/libsvn_client/diff.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,14 @@ typedef struct diff_driver_info_t
/* The anchor to prefix before wc paths */
const char *anchor;

/* Relative path of ra session from repos_root_url */
/* Relative path of ra session from repos_root_url.
Used only in printing git diff headers. The repository-root-relative
path of ... ### what user-visible property of the diff? */
const char *session_relpath;

/* Used only in printing git diff headers. Used to find the
repository-root-relative path of a WC path. */
svn_wc_context_t *wc_ctx;

/* The original targets passed to the diff command. We may need
Expand Down Expand Up @@ -1803,18 +1808,16 @@ unsupported_diff_error(svn_error_t *child_err)
For now, require PATH1=PATH2, REVISION1='base', REVISION2='working',
otherwise return an error.
Set *ROOT_RELPATH to "" if PATH1 is a WC root, else to basename(PATH1).
Set *ROOT_IS_DIR to TRUE if the working version of PATH1 is
a directory, else to FALSE.
If DDI is null: anchor DIFF_PROCESSOR at the requested diff targets.
If DDI is non-null: Set DDI->anchor to the parent of PATH1 if the working
version of PATH1 is a dir, else to PATH1; set DDI->orig_path* to PATH*.
If DDI is non-null: Anchor DIFF_PROCESSOR at PATH1 if the working
version of PATH1 is a dir, else to the parent of PATH1, to match the
anchor chosen by svn_wc__diff7(). Set DDI->anchor to this anchor,
and set DDI->orig_path* to PATH*.
All other options are the same as those passed to svn_client_diff7(). */
static svn_error_t *
diff_wc_wc(const char **root_relpath,
svn_boolean_t *root_is_dir,
struct diff_driver_info_t *ddi,
diff_wc_wc(struct diff_driver_info_t *ddi,
const char *path1,
const svn_opt_revision_t *revision1,
const char *path2,
Expand Down Expand Up @@ -1862,7 +1865,7 @@ diff_wc_wc(const char **root_relpath,
ddi->orig_path_2 = path2;
}

SVN_ERR(svn_wc__diff7(root_relpath, root_is_dir,
SVN_ERR(svn_wc__diff7(!ddi,
ctx->wc_ctx, abspath1, depth,
ignore_ancestry, changelists,
diff_processor,
Expand All @@ -1879,11 +1882,16 @@ diff_wc_wc(const char **root_relpath,
and the actual two paths compared are determined by following copy
history from PATH_OR_URL2.
If DDI is null, anchor the DIFF_PROCESSOR at the requested diff
targets. (This case is used by diff-summarize.)
If DDI is non-null: Set DDI->orig_path_* to the two diff target URLs as
resolved at the given revisions; set DDI->anchor to an anchor WC path
if either of PATH_OR_URL* is given as a WC path, else to null; set
DDI->session_relpath to the repository-relpath of the anchor URL for
DDI->orig_path_1.
DDI->orig_path_1. Anchor the DIFF_PROCESSOR at the anchor chosen
for the underlying diff implementation if the target on either side
is a file, else at the actual requested targets.
(The choice of WC anchor implementated here for DDI->anchor appears to
be: choose PATH_OR_URL2 (if it's a WC path) or else PATH_OR_URL1 (if
Expand All @@ -1893,11 +1901,12 @@ diff_wc_wc(const char **root_relpath,
(For the choice of URL anchor for DDI->session_relpath, see
diff_prepare_repos_repos().)
### Bizarre anchoring. TODO: always anchor DIFF_PROCESSOR at the
requested targets.
All other options are the same as those passed to svn_client_diff7(). */
static svn_error_t *
diff_repos_repos(const char **root_relpath,
svn_boolean_t *root_is_dir,
struct diff_driver_info_t *ddi,
diff_repos_repos(struct diff_driver_info_t *ddi,
const char *path_or_url1,
const char *path_or_url2,
const svn_opt_revision_t *revision1,
Expand Down Expand Up @@ -1995,8 +2004,11 @@ diff_repos_repos(const char **root_relpath,

/* Filter the first path component using a filter processor, until we fixed
the diff processing to handle this directly */
if (root_relpath)
*root_relpath = apr_pstrdup(result_pool, target1);
if (!ddi)
{
diff_processor = svn_diff__tree_processor_filter_create(
diff_processor, target1, scratch_pool);
}
else if ((kind1 != svn_node_file && kind2 != svn_node_file)
&& target1[0] != '\0')
{
Expand Down Expand Up @@ -2063,16 +2075,17 @@ diff_repos_repos(const char **root_relpath,
If REVERSE is TRUE, the diff will be reported in reverse.
If DDI is null, anchor the DIFF_PROCESSOR at the requested diff
targets. (This case is used by diff-summarize.)
If DDI is non-null: Set DDI->orig_path_* to the URLs of the two diff
targets as resolved at the given revisions; set DDI->anchor to a WC path
anchor for PATH2; set DDI->session_relpath to the repository-relpath of
the URL of that same anchor WC path.
All other options are the same as those passed to svn_client_diff7(). */
static svn_error_t *
diff_repos_wc(const char **root_relpath,
svn_boolean_t *root_is_dir,
struct diff_driver_info_t *ddi,
diff_repos_wc(struct diff_driver_info_t *ddi,
const char *path_or_url1,
const svn_opt_revision_t *revision1,
const svn_opt_revision_t *peg_revision1,
Expand Down Expand Up @@ -2219,11 +2232,6 @@ diff_repos_wc(const char **root_relpath,
#endif
}

if (root_relpath)
*root_relpath = apr_pstrdup(result_pool, target);
if (root_is_dir)
*root_is_dir = (*target == '\0');

SVN_ERR(svn_ra_reparent(ra_session, anchor_url, scratch_pool));

if (ddi)
Expand Down Expand Up @@ -2252,6 +2260,11 @@ diff_repos_wc(const char **root_relpath,
anchor_url,
result_pool);
}
else
{
diff_processor = svn_diff__tree_processor_filter_create(
diff_processor, target, scratch_pool);
}

if (reverse)
diff_processor = svn_diff__tree_processor_reverse_create(
Expand Down Expand Up @@ -2334,9 +2347,7 @@ diff_repos_wc(const char **root_relpath,

/* This is basically just the guts of svn_client_diff[_summarize][_peg]6(). */
static svn_error_t *
do_diff(const char **root_relpath,
svn_boolean_t *root_is_dir,
diff_driver_info_t *ddi,
do_diff(diff_driver_info_t *ddi,
const char *path_or_url1,
const char *path_or_url2,
const svn_opt_revision_t *revision1,
Expand Down Expand Up @@ -2364,8 +2375,7 @@ do_diff(const char **root_relpath,
if (is_repos2)
{
/* Ignores changelists. */
SVN_ERR(diff_repos_repos(root_relpath, root_is_dir,
ddi,
SVN_ERR(diff_repos_repos(ddi,
path_or_url1, path_or_url2,
revision1, revision2,
peg_revision, depth, ignore_ancestry,
Expand All @@ -2375,7 +2385,7 @@ do_diff(const char **root_relpath,
}
else /* path_or_url2 is a working copy path */
{
SVN_ERR(diff_repos_wc(root_relpath, root_is_dir, ddi,
SVN_ERR(diff_repos_wc(ddi,
path_or_url1, revision1,
no_peg_revision ? revision1
: peg_revision,
Expand All @@ -2390,7 +2400,7 @@ do_diff(const char **root_relpath,
{
if (is_repos2)
{
SVN_ERR(diff_repos_wc(root_relpath, root_is_dir, ddi,
SVN_ERR(diff_repos_wc(ddi,
path_or_url2, revision2,
no_peg_revision ? revision2
: peg_revision,
Expand Down Expand Up @@ -2421,7 +2431,7 @@ do_diff(const char **root_relpath,
}

/* Ignores changelists, ignore_ancestry */
SVN_ERR(svn_client__arbitrary_nodes_diff(root_relpath, root_is_dir,
SVN_ERR(svn_client__arbitrary_nodes_diff(!ddi,
abspath1, abspath2,
depth,
diff_processor,
Expand All @@ -2430,7 +2440,7 @@ do_diff(const char **root_relpath,
}
else
{
SVN_ERR(diff_wc_wc(root_relpath, root_is_dir, ddi,
SVN_ERR(diff_wc_wc(ddi,
path_or_url1, revision1,
path_or_url2, revision2,
depth, ignore_ancestry, changelists,
Expand Down Expand Up @@ -2664,7 +2674,7 @@ svn_client_diff7(const apr_array_header_t *options,
outstream, errstream,
ctx, pool));

return svn_error_trace(do_diff(NULL, NULL, ddi,
return svn_error_trace(do_diff(ddi,
path_or_url1, path_or_url2,
revision1, revision2,
&peg_revision, TRUE /* no_peg_revision */,
Expand Down Expand Up @@ -2724,7 +2734,7 @@ svn_client_diff_peg7(const apr_array_header_t *options,
outstream, errstream,
ctx, pool));

return svn_error_trace(do_diff(NULL, NULL, ddi,
return svn_error_trace(do_diff(ddi,
path_or_url, path_or_url,
start_revision, end_revision,
peg_revision, FALSE /* no_peg_revision */,
Expand All @@ -2748,17 +2758,15 @@ svn_client_diff_summarize2(const char *path_or_url1,
{
const svn_diff_tree_processor_t *diff_processor;
svn_opt_revision_t peg_revision;
const char **p_root_relpath;

/* We will never do a pegged diff from here. */
peg_revision.kind = svn_opt_revision_unspecified;

SVN_ERR(svn_client__get_diff_summarize_callbacks(
&diff_processor, &p_root_relpath,
SVN_ERR(svn_client__get_diff_summarize_callbacks(&diff_processor,
summarize_func, summarize_baton,
path_or_url1, pool, pool));
pool, pool));

return svn_error_trace(do_diff(p_root_relpath, NULL, NULL,
return svn_error_trace(do_diff(NULL,
path_or_url1, path_or_url2,
revision1, revision2,
&peg_revision, TRUE /* no_peg_revision */,
Expand All @@ -2781,14 +2789,12 @@ svn_client_diff_summarize_peg2(const char *path_or_url,
apr_pool_t *pool)
{
const svn_diff_tree_processor_t *diff_processor;
const char **p_root_relpath;

SVN_ERR(svn_client__get_diff_summarize_callbacks(
&diff_processor, &p_root_relpath,
SVN_ERR(svn_client__get_diff_summarize_callbacks(&diff_processor,
summarize_func, summarize_baton,
path_or_url, pool, pool));
pool, pool));

return svn_error_trace(do_diff(p_root_relpath, NULL, NULL,
return svn_error_trace(do_diff(NULL,
path_or_url, path_or_url,
start_revision, end_revision,
peg_revision, FALSE /* no_peg_revision */,
Expand Down

0 comments on commit d29903f

Please sign in to comment.