> Based on the fact that the svn client is already interpreting things of
> the form 'foo://bar/baz' as URLs, I'm assuming it's okay for
> svn_path_condense_targets to do the same.
>
> In any case, I'm not feeling so confident as to just commit this
> outright, so if somebody could look over it I'd be much obliged.
>
> (I'm in the process of running the full test suite on it; I've already
> checked that it passes log_tests and status_tests).
Sure -- here's a quick review (and thanks for taking the issue!):
> Index: subversion/include/svn_path.h
> ===================================================================
> --- subversion/include/svn_path.h (revision 5485)
> +++ subversion/include/svn_path.h (working copy)
> @@ -223,14 +223,15 @@
> *
> * 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 *).
> + * The elements in @a targets either must all be URLs, or they must all be
> + * existing files or directories (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
The new doc string implies that it is an error to mix URLs and
non-URLs (as it probably should be). But I didn't see anything that
checks for this case in the new code?
I think it would even be okay to assert() that they are all one or all
the other, rather than return a specific error, since this is a matter
of the caller using the api correctly. However, whichever way you do
it, the code should back up the implicit promise of consistency
checking made by the documentation.
> Index: subversion/libsvn_subr/target.c
> ===================================================================
> --- subversion/libsvn_subr/target.c (revision 5485)
> +++ subversion/libsvn_subr/target.c (working copy)
> @@ -32,6 +32,28 @@
>
> /*** Code. ***/
>
> +/* Get the absolute path corresponding to RELATIVE, treating URLs
> + * as absolute paths.
> + *
> + * If RELATIVE is a URL, return the same URL in *PABSOLUTE.
> + * Otherwise return an absolute path for RELATIVE.
> + *
> + * ALLOCATE everything in POOL.
> + */
<giggle> I think you went a little board with the all-caps :-)
"ALLOCATE" ?
Also, the doc string should make clear that you're not returning "the
same" url in *PABSOLUTE, but rather a copy of the url, allocated in
POOL. In other words, there's some unintentional contradiction
between that part of the doc string and the last line of the doc
string.
> Index: subversion/libsvn_client/log.c
> ===================================================================
> --- subversion/libsvn_client/log.c (revision 5485)
> +++ subversion/libsvn_client/log.c (working copy)
>
> [...]
>
> - /* Open a repository session to the URL. If we got here from a full URL
> - passed to the command line, then if the current directory is a
> + /* Open a repository session to the BASE_URL If we got here from a full
> + URL passed to the command line, then if the current directory is a
You dropped a period, right after "BASE_URL"..
> Index: subversion/tests/clients/cmdline/switch_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/switch_tests.py (revision 5485)
> +++ subversion/tests/clients/cmdline/switch_tests.py (working copy)
>
> [...]
>
> + for line in output:
> + if re.match("set prop on switched iota", line):
> + status = 0
Greg Stein just pointed out the line.find("SUBSTRING") idiom, which I
didn't know about either, but would be appropriate here.
Otherwise, it looks good to me. No disagreement with the overall
direction of the change (and that's even though it conflicts with some
local changes of mine for issue #1195! :-) ).
Quick sanity check: you made sure the new regression test not only
succeeds with the patch, but fails without it?
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Mar 29 06:26:13 2003