Re: [PATCH] issue #3620: svn add ^/ triggers assertion failure

On 04/23/2010 11:24 PM, Stefan Sperling wrote:
> On Fri, Apr 23, 2010 at 12:46:44PM -0400, C. Michael Pilato wrote:
>
>> As I noted, the check needs to happen inside the client API. The question
>> becomes "Do we patch svn_client_addX() and svn_client_statusX() and
>> svn_client_updateX() and ..." or is there a more general fix to be made
>> (like perhaps a wrapper around svn_dirent_get_absolute() that gracefully
>> handles the my-caller-passed-a-url case)?
>>
> I think the problem is that the CLI client passes non-canonicalized
> paths to the client library.
>
> If we make the CLI client canonicalize its arguments using
> svn_dirent_canonicalize(), the assertion does not trigger.
> This makes sense since the 'svn add' command only accepts local working
> copy paths, so it should try to interpret its arguments as such.
>
> With the patch below I get an error such as:
>
> $ svn add ^/
> svn: warning: '/path/to/working/copy/file:/tmp/svn-sandbox/repos' not found
>
> (The repository lives in "/tmp/svn-sandbox/repos".)
>
> Do others agree? If so, I'd like to ask Uwe to send similar patches
> for other subcommands, making sure they canonicalize their arguments
> correctly.
>
> [...]

Meanwhile Michael Pilato and Stefan Sperling gave me even
more input (thank you) and I was able to prepare a suite of tests
modeled after others in subversion/tests in order to check most
of the other commands which, like 'add' and 'status', only accept
paths or URLs in certain argument positions.

Nearly all tests are now marked as XFail() [and the error message
which the tests expect is debatable], but I intend to follow up with
patches that will make them succeed.

The first patch, attached for review and criticism, changes only
the CLI commands. It is mostly boring, adding new checks except
in checkout-cmd.c. (Note that the checkout test fails before and
after the patch because of something happening inside
svn_cl__args_to_target_array_print_reserved() that I couldn't
explain yet.)

Now, since it has been a while I will try to sum up what has been
suggested - (feel free to skip to the end)...

a) Validating all input in the libsvn_client library and gracefully
    handling any errors instead of triggering an assertion allows the
    client to recover. I think everyone agreed that this is good.

> --- subversion/libsvn_client/add.c (revision 953325)
> +++ subversion/libsvn_client/add.c (working copy)
> @@ -593,6 +593,12 @@
> const char *local_abspath;
> struct add_with_write_lock_baton baton;
>
> + if (svn_path_is_url(path))
> + return svn_error_createf(SVN_ERR_WC_BAD_PATH,
> + NULL,
> + _("Path '%s' is not a working copy path"),
> + path);
> +
> SVN_ERR(svn_dirent_get_absolute(&local_abspath, path, pool));
>
> /* ### this is a hack.

b) Checking the arguments in the CLI commands before calling
    libsvn_client functions makes it possible to abort early and not
    in the middle of processing the command.

> --- subversion/svn/add-cmd.c (revision 953325)
> +++ subversion/svn/add-cmd.c (working copy)
> @@ -66,6 +68,17 @@
>
> SVN_ERR(svn_opt_eat_peg_revisions(&targets, targets, pool));
>
> + /* Check that no targets are URLs */
> + for (i = 0; i < targets->nelts; i++)
> + {
> + const char *target = APR_ARRAY_IDX(targets, i, const char *);
> + if (svn_path_is_url(target))
> + return svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR,
> + NULL,
> + _("Path '%s' is not a working copy
> path"),
> + target);
> + }
> +
> subpool = svn_pool_create(pool);
> for (i = 0; i < targets->nelts; i++)
> {

    But svn_path_is_url() only does a syntactic check, and ^/ (or the
    expanded URL) is in fact a syntactically correct POSIX pathname.
    That is why c) also appeals to me.

c) Stefan's suggested fix through canonicalization of path arguments
    produces an error message that seems more rational to me than, say,
    "Path '%s' is not a working copy path, but a URL" - since even a URL
    is a syntactically correct pathname (on POSIX systems). However,
    this would imply that target arguments aren't parsed and interpreted
    in a uniform way and could lead to confusion? That is why d) also
    seems plausible.

(citing Stefan)
> Index: subversion/svn/add-cmd.c
> ===================================================================
> --- subversion/svn/add-cmd.c (revision 937185)
> +++ subversion/svn/add-cmd.c (working copy)
> @@ -30,6 +30,7 @@
> #include<apr_want.h>
>
> #include "svn_client.h"
> +#include "svn_dirent_uri.h"
> #include "svn_error.h"
> #include "svn_pools.h"
> #include "cl.h"
> @@ -70,11 +71,15 @@ svn_cl__add(apr_getopt_t *os,
> for (i = 0; i< targets->nelts; i++)
> {
> const char *target = APR_ARRAY_IDX(targets, i, const char *);
> + const char *canon_target;
>
> svn_pool_clear(subpool);
> +
> SVN_ERR(svn_cl__check_cancel(ctx->cancel_baton));
> +
> + canon_target = svn_dirent_canonicalize(target, subpool);
> SVN_ERR(svn_cl__try
> - (svn_client_add4(target,
> + (svn_client_add4(canon_target,
> opt_state->depth,
> opt_state->force, opt_state->no_ignore,
> opt_state->parents, ctx, subpool),
>

d) With consistent expansion of ^/ to the repository URL and consistent
    parsing of @peg-revisions in all target arguments, and behavioral
    differences depending on the kind of target given, I could understand
    a policy that treats pathnames which look like URLs as syntactically
    incorrect and rejects them where only local pathnames are expected
    and vice versa.

I'm unsure, however, whether c) path canonicalization, can be entirely
dismissed. I don't know the library interface contracts well enough yet.

Cheers (sorry for the delay),
Uwe Stuehler