You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Karl Fogel <kf...@newton.ch.collab.net> on 2002/05/03 00:53:10 UTC
Re: svn commit: rev 1865 - trunk/subversion/libsvn_wc trunk/subversion/tests/clients/cmdline
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?
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?
-Karl
> Modified: trunk/subversion/libsvn_wc/update_editor.c
> ==============================================================================
> --- trunk/subversion/libsvn_wc/update_editor.c (original)
> +++ trunk/subversion/libsvn_wc/update_editor.c Thu May 2 18:17:44 2002
> @@ -1780,6 +1780,21 @@
> void **edit_baton,
> apr_pool_t *pool)
> {
> + svn_node_kind_t kind;
> +
> + /* Checkout into an existing working-copy isn't supported. */
> + SVN_ERR (svn_io_check_path (dest->data, &kind, pool));
> + if (kind == svn_node_dir)
> + {
> + svn_boolean_t is_wc;
> + SVN_ERR (svn_wc_check_wc (dest, &is_wc, pool));
> + if (is_wc)
> + return svn_error_createf
> + (SVN_ERR_UNSUPPORTED_FEATURE, 0, NULL, pool,
> + "checkout into '%s' which is already a working copy",
> + dest->data);
> + }
> +
> return make_editor (dest, NULL, target_revision,
> TRUE, ancestor_url,
> FALSE, NULL,
>
> Modified: trunk/subversion/tests/clients/cmdline/basic_tests.py
> ==============================================================================
> --- trunk/subversion/tests/clients/cmdline/basic_tests.py (original)
> +++ trunk/subversion/tests/clients/cmdline/basic_tests.py Thu May 2 18:17:45 2002
> @@ -37,7 +37,19 @@
> def basic_checkout(sbox):
> "basic checkout of a wc"
>
> - return sbox.build()
> + if sbox.build():
> + return 1
> +
> + # second checkout is expected to fail
> + wc_dir = sbox.wc_dir
> + url = svntest.main.current_repo_url
> + stdout_lines, stderr_lines = svntest.main.run_svn (1, 'checkout', url,
> + '-d', wc_dir)
> + for stderr_line in stderr_lines:
> + if re.match('.*already a working copy', stderr_line):
> + return 0
> +
> + return 1
>
> #----------------------------------------------------------------------
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 1865 - trunk/subversion/libsvn_wc trunk/subversion/tests/clients/cmdline
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
Philip Martin <ph...@codematters.co.uk> writes:
> Looks promising, I'll investigate.
Beautiful -- thanks!
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 1865 - trunk/subversion/libsvn_wc trunk/subversion/tests/clients/cmdline
Posted by Philip Martin <ph...@codematters.co.uk>.
Karl Fogel <kf...@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