Helllo Stefan.
Thanks for trying to solve this problem.
Unfortunately you patch doesn't solve the problem because of 2 reasons.
1. It doesn't change "target" (in diff request), and it still may contain '/'.
2. If in my example (just replace http://localhost/svn == file:///tmp/test)
diff fails if I compare r2
and r4.
$ svn diff file:///tmp/test/directory/subdirectory@2
file:///tmp/test/directory/subdirectory@4
svn: E125007: Path 'subdirectory/file1' must be an immediate child of the
directory
'directory/subdirectory'
Though I can confirm that for r1-r4 comparison the output is ok with your patch.
Actually I didn't expect involving "relative_to_dir" feature because currently
svn client doesn't
set this parameter anywhere. And I don't think it's a good idea to to use it
now. As I wrote I think
a better approach is to eliminate that do{}while(); cycle and to do something
like (though it may
require some significant efforts)
if (any or urls to compare doesn't exist in peg revision) {
generate full addition/deletion diff using a special report
//or alternative throw an exception like svn 1.6 does
} else {
perform diff that is currently performed
}
> On Tue, Mar 20, 2012 at 07:16:44PM +0100, Dmitry Pavlenko wrote:
> > > I expected you'd get an "/directory/subdirectory@1 doesn't exist"
> > > error.
> >
> > I was the older (1.6) SVN behaviour
> >
> > Now "do {} while()" cycle in "diff_prepare_repos_repos" takes parent URL
> > until URL exists in both start and end revisions. So maybe the cycle
> > should be run only once not to let '/'. Or, that is maybe better, if URL
> > doesn't exist in one of revision, diff should show complete addition or
> > complete removal. So maybe this case should be handled separatedly,
> > using a special report.
> >
> > I a "doesn't exist" error is expected, the efforts in that "do {} while
> > ()" cycle seem to be useless.
> >
> > To my opinion It would be reasonable to guarantee that all directories
> > are displayed relative to the parent of url1 and url2 arguments that
> > exists in both revisions (that is found in that "do {} while()" cycle).
> > Or maybe it would be reasonable to display all paths relative to targets
> > (or parents --- for diff on files) as you proposed.
>
> Hi Dmitry,
>
> I'm looking into this.
>
> I think all paths should be relative to user-specified targets.
>
> In the case where we show newly added items we need to use parent paths
> internally. These ended up being used for diff output, too, which is
> wrong in my opinion.
>
> If you want to help you can test and review the patch below. Thanks!
>
> I am currently running tests over all ra layers with this patch and
> should have results in a while.
>
> (sorry, no log message yet -- not enough time right now)
>
> Index: subversion/libsvn_client/diff.c
> ===================================================================
> --- subversion/libsvn_client/diff.c (revision 1333046)
> +++ subversion/libsvn_client/diff.c (working copy)
> @@ -1540,6 +1540,14 @@ resolve_pegged_diff_target_url(const char **resolv
> * *TARGET1 and *TARGET2, based on *URL1 and *URL2.
> * Set *BASE_PATH corresponding to the URL opened in the new *RA_SESSION
> * which is pointing at *ANCHOR1.
> + *
> + * If one of the diff targets does not exist in the repository, diff
> targets + * specified by the user will be adjusted so that a diff can be
> generated. + * However, the resulting paths in diff output can differ from
> what users + * expect to see. If *RELATIVE_TO_DIR is not NULL and does not
> already + * contain a valid path, set it up such that generated diff
> output meets user + * expectations even if targets have to be adjusted.
> + *
> * Use client context CTX. Do all allocations in POOL. */
> static svn_error_t *
> diff_prepare_repos_repos(const char **url1,
> @@ -1551,6 +1559,7 @@ diff_prepare_repos_repos(const char **url1,
> const char **anchor2,
> const char **target1,
> const char **target2,
> + const char **relative_to_dir,
> svn_ra_session_t **ra_session,
> svn_client_ctx_t *ctx,
> const char *path_or_url1,
> @@ -1713,6 +1722,31 @@ diff_prepare_repos_repos(const char **url1,
> }
> while (kind != svn_node_dir);
> *anchor1 = *anchor2 = new_anchor;
> +
> + /* Diff labels must appear relative to previous anchor. */
> + if (relative_to_dir && *relative_to_dir == NULL)
> + {
> + const char *relative_to_url;
> +
> + if (kind1 == svn_node_none)
> + {
> + if (kind2 == svn_node_file)
> + svn_uri_split(&relative_to_url, NULL, *url2, pool);
> + else
> + relative_to_url = *url2;
> + }
> + else
> + {
> + if (kind1 == svn_node_file)
> + svn_uri_split(&relative_to_url, NULL, *url1, pool);
> + else
> + relative_to_url = *url1;
> + }
> +
> + *relative_to_dir = svn_uri_skip_ancestor(repos_root,
> + relative_to_url, pool);
> + }
> +
> /* Diff targets must be relative to the new anchor. */
> *target1 = svn_uri_skip_ancestor(new_anchor, *url1, pool);
> *target2 = svn_uri_skip_ancestor(new_anchor, *url2, pool);
> @@ -2372,6 +2406,7 @@ diff_repos_repos(const svn_wc_diff_callbacks4_t *c
> /* Prepare info for the repos repos diff. */
> SVN_ERR(diff_prepare_repos_repos(&url1, &url2, &base_path, &rev1, &rev2,
> &anchor1, &anchor2, &target1, &target2,
> + &callback_baton->relative_to_dir,
> &ra_session, ctx, path_or_url1,
> path_or_url2, revision1, revision2, peg_revision, pool));
> @@ -2745,7 +2780,7 @@ diff_summarize_repos_repos(svn_client_diff_summari
> /* Prepare info for the repos repos diff. */
> SVN_ERR(diff_prepare_repos_repos(&url1, &url2, &base_path, &rev1, &rev2,
> &anchor1, &anchor2, &target1, &target2,
> - &ra_session, ctx,
> + NULL, &ra_session, ctx,
> path_or_url1, path_or_url2, revision1,
> revision2, peg_revision, pool));
