Karl Fogel <kfogel@newton.ch.collab.net> writes:
> philip@tigris.org writes:
> > Log:
> > Prevent checkout into an existing working-copy.
> >
> > * subversion/libsvn_wc/update_editor.c (svn_wc_get_checkout_editor): Fail
> > if destination is already a working copy.
> >
> > * subversion/tests/clients/cmdline/basic_tests.py (basic_checkout): Test
> > that checkout into a working copy fails.
>
> Hmmm. Interesting fix. Shouldn't it be farther down in the code,
> though -- in the editor itself, effectively?
Yes. I realised last night there are lots of ways the check I put in
will fail.
>
> svn_wc_make_checkout_editor() just returns an editor. No checkout
> takes place until someone starts driving that editor. It seems
> improper to error because the target is a wc directory *at the time
> the editor is created*. It should only be an error if the target is a
> wc at the time the checkout is actually attempted (and, ideally, only
> if it's a wc from a different URL than is being checked out, but
> that's an optimization).
>
> Let's see... open_root() calls prep_directory(), which looks like
> this:
>
> static svn_error_t *
> prep_directory (svn_stringbuf_t *path,
> svn_stringbuf_t *ancestor_url,
> svn_revnum_t ancestor_revision,
> svn_boolean_t force,
> apr_pool_t *pool)
> {
> /* kff todo: how about a sanity check that it's not a dir of the
> same name from a different repository or something?
> Well, that will be later on down the line... */
>
> if (force) /* Make sure the directory exists. */
> SVN_ERR (svn_wc__ensure_directory (path, pool));
>
> /* Make sure it's the right working copy, either by creating it so,
> or by checking that it is so already. */
> return svn_wc__ensure_adm (path, ancestor_url, ancestor_revision,
> pool);
> }
>
> I think that `todo' comment is a red herring. We could implement the
> check directly here, but I think doing it in svn_wc__ensure_adm()
> makes more sense. In fact, svn_wc__ensure_adm() is supposed to be
> doing it, but (as its doc string says) it's currently not. It would
> really depend on check_adm_exists(), doing it correctly, anyway, and
> there's the root of the problem:
>
> /* Set *EXISTS to non-zero iff there's an adm area for PATH, and it
> * matches URL and REVISION.
> * ### todo: The url/rev match is not currently implemented.
> *
> * If an error occurs, just return the error and don't touch *EXISTS.
> */
> static svn_error_t *
> check_adm_exists (int *exists,
> svn_stringbuf_t *path,
> svn_stringbuf_t *url,
> svn_revnum_t revision,
> apr_pool_t *pool)
> {
> [...]
> }
>
> If that url match were working right, this problem and maybe some
> others would be solved.
>
> So, I think the "right" way to fix this is to make check_adm_exists()
> finally implement that check, and the behavior will propagate up.
>
> Do you agree?
Looks promising, I'll investigate.
--
Philip
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri May 3 13:55:13 2002