Jeremy Whitlock wrote:
> Hi All,
> Per Mark's request, I have added new logic to check for sameness
> of the diffed path and the relative output path. The direct link to
> the patch is here:
>
> http://subversion.tigris.org/nonav/issues/showattachment.cgi/828/Issue-2723-8.patch.txt
Hi Jeremy.
Thanks for this fix which I've displayed below for convenience of reference.
It looks right in itself and I see it's been committed, but I have some
comments about how that section of code works.
If you have the time and wouldn't mind, it would be nice to wrap the repeated
bunch of code in a helper function which could replace
MAKE_ERR_BAD_RELATIVE_PATH like this (untested):
[[[
/* Set *CHILD_PATH to the relative path of PATH within the directory
* REL_TO_DIR, or return an error if it is not within it.
* Paths are in canonical form except the output is "." to
* represent "the same directory".
* Allocate *CHILD_PATH in POOL or statically. */
static svn_error_t *
get_relative_path(const char **child_path,
const char *rel_to_dir,
const char *path,
apr_pool_t *pool)
{
*child_path = svn_path_is_child(rel_to_dir, path, pool);
if (child_path)
return SVN_NO_ERROR;
else if (!svn_path_compare_paths(rel_to_dir, path))
*child_path = ".";
else
return svn_error_createf(SVN_ERR_BAD_RELATIVE_PATH, NULL,
_("Path '%s' must be an immediate child of "
"the directory '%s'"), path, rel_to_dir);
}
]]]
Not specifically addressing this patch, but why do these relativisations of
{path,path1,path2} come AFTER the formatting of path1 = "path\t(.../path1)" and
path2 = "path\t(.../path2)"? In a simple and possibly unrealistic mental
exercise, with rel_to_dir="/dir":
# Starting point:
path="/dir/a" path1="/dir/b" path2="/dir/c"
# After formatting
path="/dir/a" path1="/dir/a (.../b)" path2="/dir/a (.../c)"
# After relativisation
path="a" path1="b)" path2="c)"
Even if it gives the right result in all cases that are actually possible in
the present system, it seems to improperly treat the labels path1 and path2
(after formatting) as if they were paths. Am I missing something or should this
step be done before the formatting?
While we're here, is it correct that the error message should say "must be an
immediate child"? I would have thought any level of child was OK.
- Julian
> [[[
> Patch that adds logic to tell whether the path being diffed is the same
> as the path the diff paths should be relative to.
>
> * subversion/libsvn_client/diff.c:
> (display_prop_diffs, diff_content_changed): Updated for new path check.
>
> * subversion/bindings/javahl/tests/org/tigris/subversion/javahl/BasicTests.java
> (testDiff): New test.
> ]]]
>
> Index: subversion/libsvn_client/diff.c
> ===================================================================
> --- subversion/libsvn_client/diff.c (revision 28370)
> +++ subversion/libsvn_client/diff.c (working copy)
> @@ -185,6 +185,8 @@
>
> if (child_path)
> path = child_path;
> + else if (!svn_path_compare_paths(relative_to_dir, path))
> + path = ".";
> else
> return MAKE_ERR_BAD_RELATIVE_PATH(path, relative_to_dir);
> }
> @@ -481,6 +483,8 @@
>
> if (child_path)
> path = child_path;
> + else if (!svn_path_compare_paths(rel_to_dir, path))
> + path = ".";
> else
> return MAKE_ERR_BAD_RELATIVE_PATH(path, rel_to_dir);
>
> @@ -488,6 +492,8 @@
>
> if (child_path)
> path1 = child_path;
> + else if (!svn_path_compare_paths(rel_to_dir, path1))
> + path1 = ".";
> else
> return MAKE_ERR_BAD_RELATIVE_PATH(path1, rel_to_dir);
>
> @@ -495,6 +501,8 @@
>
> if (child_path)
> path2 = child_path;
> + else if (!svn_path_compare_paths(rel_to_dir, path))
> + path2 = ".";
> else
> return MAKE_ERR_BAD_RELATIVE_PATH(path2, rel_to_dir);
> }
> Index: subversion/bindings/javahl/tests/org/tigris/subversion/javahl/BasicTests.java
> ===================================================================
> --- subversion/bindings/javahl/tests/org/tigris/subversion/javahl/BasicTests.java (revision 28370)
> +++ subversion/bindings/javahl/tests/org/tigris/subversion/javahl/BasicTests.java (working copy)
> @@ -2532,6 +2532,19 @@
> assertFileContentsEquals("Unexpected diff output in file '" +
> diffOutput.getPath() + '\'',
> expectedDiffOutput, diffOutput);
> +
> + // Test diff where relativeToDir and path are the same.
> + expectedDiffOutput = NL + "Property changes on: ." + NL +
> + underSepLine +
> + "Added: testprop" + NL +
> + " + Test property value." + NL + NL;
> +
> + client.propertySet(aPath, "testprop", "Test property value.", false);
> + client.diff(aPath, Revision.BASE, aPath, Revision.WORKING, aPath,
> + diffOutput.getPath(), Depth.infinity, true, true, false);
> + assertFileContentsEquals("Unexpected diff output in file '" +
> + diffOutput.getPath() + '\'',
> + expectedDiffOutput, diffOutput);
>
>
> /*
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Dec 10 23:22:42 2007