bdenny@tigris.org writes:
> *
> * Find the common prefix of the paths in @a targets, and remove redundancies.
> *
> - * The elements in @a targets must be existing files or directories (as
> - * const char *).
> + * Each of the elements in @a targets must be a URL, or an existing file or
> + * directory (as const char *).
> *
> * If there are multiple targets, or exactly one target and it's not a
> * directory, then
> *
> * - @a *pbasename is set to the absolute path of the common parent
> - * directory of all of those targets, and
> + * directory of those targets (if the targets are files/directories),
> + * or the common URL prefix of the targets (if they are URLs).
> *
> * - If @a pcondensed_targets is non-null, @a *pcondensed_targets is set
> * to an array of targets relative to @a *pbasename, with
Hmmm. I have an awkward question (awkward partly because this
directly affects my changes-in-progress for issue #1195 :-) ):
The new doc string isn't really clear on how condensation behaves when
there is a mixture of URL and local path targets. Does *pbasename get
set to the local path prefix, or to the URL prefix? Whichever one it
is, does condensation (i.e., removal of redundancies) only happen to
that kind of path, and the other targets are left unchanged?
I can maybe work around the problem, by simply having the caller,
svn_client_commit(), guarantee that no paths are URLs, but in general,
callers may still need to know these bevahioral details.
> Modified: trunk/subversion/libsvn_subr/path.c
> ==============================================================================
> --- trunk/subversion/libsvn_subr/path.c (original)
> +++ trunk/subversion/libsvn_subr/path.c Tue Apr 1 21:58:04 2003
> @@ -468,10 +468,20 @@
> }
>
>
> -char *
> -svn_path_get_longest_ancestor (const char *path1,
> - const char *path2,
> - apr_pool_t *pool)
> +/* Return the longest common ancestor of PATH1 and PATH2.
> + *
> + * This function handles everything except the URL-handling logic
> + * of svn_path_get_longest_ancestor, and assumes that PATH1 and
> + * PATH2 are *not* URLs.
> + *
> + * If the two paths do not share a common ancestor, NULL is returned.
> + *
> + * New strings are allocated in POOL.
> + */
> +static char *
> +get_longest_path_ancestor (const char *path1,
> + const char *path2,
> + apr_pool_t *pool)
Shouldn't this return 'const char *'?
> +char *
> +svn_path_get_longest_ancestor (const char *path1,
> + const char *path2,
> + apr_pool_t *pool)
> +{
> + if (svn_path_is_url (path1))
> + {
> + if (svn_path_is_url (path2))
> + {
> + char *ancestor, *path_ancestor, *prefix;
> + int i = 0;
> +
> + /* Find ':' */
> + for (i = 0; path1[i] != ':'; i++)
Why not "(path1[i] != ':' || path2[i] != ':')" for the condition?
That gives you an early out, thought that's obviously negligible in
terms of performance -- I just like it because it's clearer to treat
the two paths symmetrically, since we're in the case where both are
URLs.
> + {
> + /* They're both URLs, so EOS can't come before ':' */
> + assert (path1[i] != '\0' && path2[i] != '\0');
> +
> + /* No shared protocol => no common prefix */
> + if (path1[i] != path2[i])
> + return NULL;
> + }
Why does protocol difference imply "real" difference?
Is http://foo/bar/baz != svn://foo/bar/baz? Once you get inside the
repository, they are the same. The fact that they use different
access methods might be irrelevant.
(Or maybe it *is* relevant, but then an explanation would help, since
it's not intuitively obvious why the protocol makes a difference.)
Minor nits, of course. Thanks for resolving this issue! And the
regression tests were most welcome.
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Apr 2 07:39:29 2003