> 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);
> }
> ]]]
That's fine with me. It's only used in two places right now but I can
see it being more consumable as a standalone function.
> 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?
The idea of doing the formatting after the creation of the label is to
ensure that this code does not alter the code for the diff. I figured
if I were to modify the path after Subversion had used it, it would be
safer. As it stands now, the code only affects the output of the diff
and has no chance of altering or causing anomalies with diff itself.
> 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.
I can see your point. Any child is fine. I can submit a minor change
to this after all eyes have had time to digest. Please feel free to
email me back with further questions.
Take care,
Jeremy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Dec 11 07:03:03 2007