You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Patrick Steinhardt <pa...@elegosoft.com> on 2016/10/18 12:22:25 UTC
[PATCH v2] Reject checkouts to existing directory
Hi,
finally got around to update my patch regarding checkouts to
existing directories. The semantics have been changed to accept
checkouts iff
- the target directory does not exist
- the target directory is empty
- the repository to check out is empty
- the --force flag is given
This should treat all cases pointed out by Ivan. There were quite
a lot of test cases that needed adjustment following this change.
I've mostly added the '--force' flag, but also replaced a test
which didn't make sense anymore with a new one (that is 'co to a
dir without --force with obstructions'). The new test case simply
check is we reject checking out a non-empty repo to a non-empty
dir.
All but two unrelated tests pass with this.
[[[
Reject checkouts which would create tree conflicts.
When a new checkout is done where the target dirctory already
exists, subversion will usually create a lot of tree conflicts
which are intimidating, especially to new users. This behavior
stems from release 1.8, where we started accepting checkouts to
existing directories without any safety-checks.
This patch changes semantics in that it introduces new safety
checks so that the user does not accidentally shoots himself into
the foot. We now only allow checkouts if one of the following
conditions holds true:
- the target directory does not exist
- the target directory is empty
- the repository to check out is empty
- the --force flag is given
The main use case solved by the above conditions is for
converting existing directories into a repository when the
repository is newly created.
* subversion/svn/checkout-cmd.c:
(listing_cb): New callback to check whether the remote
repository is empty.
(verify_checkout_target): New function to check whether the
target checkout directory is a valid one.
(svn_cl__checkout): Now calls `verify_checkout_target` if no
--force is specified.
* subversion/tests/cmdline/autoprop_tests.py:
(autoprops_test,
inheritable_autoprops_test): use 'co --force'.
* subversion/tests/cmdline/basic_tests.py:
(basic_checkout,
basic_auth_test): use 'co --force'.
* subversion/tests/cmdline/checkout_tests.py:
(checkout_with_obstructions): Replace old test and now assert
that subversion refuses to checkout to non-empty dirs.
* subversion/tests/cmdline/svntest/actions.py:
(run_and_verify_checkout): use 'co --force'.
* subversion/tests/cmdline/tree_conflict_tests.py:
(ensure_tree_conflict): use 'co --force'.
]]]
Regards
Patrick
--
Patrick Steinhardt, Entwickler
elego Software Solutions GmbH, http://www.elego.de
Gebäude 12 (BIG), Gustav-Meyer-Allee 25, 13355 Berlin, Germany
Sitz der Gesellschaft: Berlin, USt-IdNr.: DE 163214194
Handelsregister: Amtsgericht Charlottenburg HRB 77719
Geschäftsführer: Olaf Wagner
Re: [PATCH v2] Reject checkouts to existing directory
Posted by Patrick Steinhardt <pa...@elegosoft.com>.
On Fri, Oct 28, 2016 at 02:49:04AM +0200, bert@qqmail.nl wrote:
> Just checking the log message: This changes 90% of our testsuite to use ‘svn co --force’, by changing the standard checkout function in actions.py that is used by virtual all tests.
>
> If that is really necessary I would call this a breaking change.
>
> It also changes a few of the most core basic tests… That tells me that some very common usecases will change.
>
>
> If we have to change the behavior of 90% of our tests for this, I don’t think we can just commit it as any basic minor test.
>
> In the summary I don’t see the support for restartability of a broken checkout. See the documentation of svn checkout in the svn-book. (http://svnbook.red-bean.com/nightly/en/svn.ref.svn.c.checkout.html)
> [[
> If you interrupt a checkout (or something else interrupts your checkout, such as loss of connectivity, etc.), you can restart it either by issuing the identical checkout command again or by updating the incomplete working copy:
> $ svn checkout file:///var/svn/repos/test mine
> A mine/a
> A mine/b
> ^C
> svn: E200015: Caught signal
> $ svn checkout file:///var/svn/repos/test mine
> A mine/c
> ^C
> svn: E200015: Caught signal
> $ svn update mine
> Updating 'mine':
> A mine/d
> Updated to revision 20.
> $
> ]]
> Resuming will now require a –force, which also triggers different behavior in other parts of checkout.
>
> Which would require me to update dozens of buildscripts that worked just fine since 1.0, if released.
I guess it makes sense to allow for resuming canceled checkouts.
After all it should not be that hard to implement. Will implement
in the next revision.
[snip]
Regards
Patrick
--
Patrick Steinhardt, Entwickler
elego Software Solutions GmbH, http://www.elego.de
Gebäude 12 (BIG), Gustav-Meyer-Allee 25, 13355 Berlin, Germany
Sitz der Gesellschaft: Berlin, USt-IdNr.: DE 163214194
Handelsregister: Amtsgericht Charlottenburg HRB 77719
Geschäftsführer: Olaf Wagner
Re: [PATCH v2] Reject checkouts to existing directory
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Branko \u010cibej wrote on Fri, Oct 28, 2016 at 14:22:31 +0200:
> On 28.10.2016 09:02, Stefan Sperling wrote:
> > On Fri, Oct 28, 2016 at 02:49:04AM +0200, bert@qqmail.nl wrote:
> >> In the summary I don\u2019t see the support for restartability of a broken checkout. See the documentation of svn checkout in the svn-book. (http://svnbook.red-bean.com/nightly/en/svn.ref.svn.c.checkout.html)
> > Yes, we'll need to make checkout work automatically in that case.
> >
> > For example, inside the client library(!) the checkout code path could ask
> > if the target dir is already a working copy root (svn_wc__is_wcroot())
> > and if so get its url (svn_wc__node_get_repos_info()) and then compare
> > that url to the (canonicalized version of) the URL which was passed on
> > the command line. If those match, proceed without require the force flag.
> >
> > That's the idea -- I don't know if this would be straightforward to
> > implement right now, or if something would get in the way of this.
>
>
> It's never that simple ... you'd have to compare repos UUID, not URL,
> and possibly do an automatic relocate if the UUIDs match but the URLs don't.
Or perhaps compare UUID and the repos-root-relative URL, so it'd be
possible to resume a checkout using a different URL to the same
directory, but not using a URL to another directory.
Re: [PATCH v2] Reject checkouts to existing directory
Posted by Branko Čibej <br...@apache.org>.
On 28.10.2016 09:02, Stefan Sperling wrote:
> On Fri, Oct 28, 2016 at 02:49:04AM +0200, bert@qqmail.nl wrote:
>> In the summary I don\u2019t see the support for restartability of a broken checkout. See the documentation of svn checkout in the svn-book. (http://svnbook.red-bean.com/nightly/en/svn.ref.svn.c.checkout.html)
> Yes, we'll need to make checkout work automatically in that case.
>
> For example, inside the client library(!) the checkout code path could ask
> if the target dir is already a working copy root (svn_wc__is_wcroot())
> and if so get its url (svn_wc__node_get_repos_info()) and then compare
> that url to the (canonicalized version of) the URL which was passed on
> the command line. If those match, proceed without require the force flag.
>
> That's the idea -- I don't know if this would be straightforward to
> implement right now, or if something would get in the way of this.
It's never that simple ... you'd have to compare repos UUID, not URL,
and possibly do an automatic relocate if the UUIDs match but the URLs don't.
-- Brane
Re: [PATCH v2] Reject checkouts to existing directory
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Oct 28, 2016 at 02:49:04AM +0200, bert@qqmail.nl wrote:
> In the summary I don\u2019t see the support for restartability of a broken checkout. See the documentation of svn checkout in the svn-book. (http://svnbook.red-bean.com/nightly/en/svn.ref.svn.c.checkout.html)
Yes, we'll need to make checkout work automatically in that case.
For example, inside the client library(!) the checkout code path could ask
if the target dir is already a working copy root (svn_wc__is_wcroot())
and if so get its url (svn_wc__node_get_repos_info()) and then compare
that url to the (canonicalized version of) the URL which was passed on
the command line. If those match, proceed without require the force flag.
That's the idea -- I don't know if this would be straightforward to
implement right now, or if something would get in the way of this.
RE: [PATCH v2] Reject checkouts to existing directory
Posted by be...@qqmail.nl.
Just checking the log message: This changes 90% of our testsuite to use ‘svn co --force’, by changing the standard checkout function in actions.py that is used by virtual all tests.
If that is really necessary I would call this a breaking change.
It also changes a few of the most core basic tests… That tells me that some very common usecases will change.
If we have to change the behavior of 90% of our tests for this, I don’t think we can just commit it as any basic minor test.
In the summary I don’t see the support for restartability of a broken checkout. See the documentation of svn checkout in the svn-book. (http://svnbook.red-bean.com/nightly/en/svn.ref.svn.c.checkout.html)
[[
If you interrupt a checkout (or something else interrupts your checkout, such as loss of connectivity, etc.), you can restart it either by issuing the identical checkout command again or by updating the incomplete working copy:
$ svn checkout file:///var/svn/repos/test mine
A mine/a
A mine/b
^C
svn: E200015: Caught signal
$ svn checkout file:///var/svn/repos/test mine
A mine/c
^C
svn: E200015: Caught signal
$ svn update mine
Updating 'mine':
A mine/d
Updated to revision 20.
$
]]
Resuming will now require a –force, which also triggers different behavior in other parts of checkout.
Which would require me to update dozens of buildscripts that worked just fine since 1.0, if released.
Bert
Sent from Mail for Windows 10
From: Stefan Sperling
Sent: donderdag 27 oktober 2016 21:45
To: Patrick Steinhardt
Cc: Subversion; Stefan Sperling; Ivan Zhakov; Stefan
Subject: Re: [PATCH v2] Reject checkouts to existing directory
On Tue, Oct 18, 2016 at 02:22:25PM +0200, Patrick Steinhardt wrote:
> Hi,
>
> finally got around to update my patch regarding checkouts to
> existing directories. The semantics have been changed to accept
> checkouts iff
>
> - the target directory does not exist
> - the target directory is empty
> - the repository to check out is empty
> - the --force flag is given
I think this makes a lot of sense. I just have not yet had time to
review and test your patch. I will try to do so soon.
Does anyone reading this list have any concerns about this change?
Re: [PATCH v2] Reject checkouts to existing directory
Posted by Johan Corveleyn <jc...@gmail.com>.
On Fri, Oct 28, 2016 at 10:34 AM, Patrick Steinhardt
<pa...@elegosoft.com> wrote:
> On Fri, Oct 28, 2016 at 12:34:44AM +0200, Stefan wrote:
>> On 10/27/2016 21:45, Stefan Sperling wrote:
>> > On Tue, Oct 18, 2016 at 02:22:25PM +0200, Patrick Steinhardt wrote:
>> >> Hi,
>> >>
>> >> finally got around to update my patch regarding checkouts to
>> >> existing directories. The semantics have been changed to accept
>> >> checkouts iff
>> >>
>> >> - the target directory does not exist
>> >> - the target directory is empty
>> >> - the repository to check out is empty
>> >> - the --force flag is given
>> > I think this makes a lot of sense. I just have not yet had time to
>> > review and test your patch. I will try to do so soon.
>> >
>> > Does anyone reading this list have any concerns about this change?
>>
>> I'm +1 on the general design/behavior change. Didn't do a code/patch
>> review, though.
>>
>> On a minor side note:
>> While talking last week on IRC to Daniel, he mentioned (on a different
>> topic) that in general it might be preferable to use a separate explicit
>> command line options to control the exact behavior over one which
>> impacts several behaviors at once. Reflecting that onto this case, it
>> crossed my mind that --allow-non-empty-directory (or --allow-non-empty)
>> might be preferable over adding that behavior to the --force parameter,
>> since the --force parameter has (or in the future might have) other
>> implications in addition to allowing a co into a non-empty directory.
>>
>> Though in this case, I don't have a strong opinion to go one way or the
>> other.
>>
>> Regards,
>> Stefan
>>
>>
>
> I think I agree with introducing a new flag here. Mixing and
> extending semantics of an existing flag surely is not a nice
> route to go. I'd vote for '--allow-non-empty-target'.
I'm wondering if you also need to look at the behaviour of 'svn
update' (which also has a --force flag).
I have a valid use case for updating over an existing non-wc
directory, using 'svn update --force': converting an embedded checkout
into part of a sparse working copy (the embedded checkout was a
mistake, it should have been pulled in by 'svn update' to become part
of a parent (sparse) working copy).
For example:
trunk in repository has three subdirs: a, b, c
user has a sparse wc rooted at trunk, with 'a' as only child
user performs a checkout of 'b' into $wc/b
user does some work in $wc/b, i.e. local mods
user notices $wc/b does not get updated when he runs 'svn update
$wc' ... oops, b should have been part of the sparse parent working
copy
What I do in this case (as the local svn support guy):
- delete or rename $wc/b/.svn
- go to $wc, and run 'svn update --force b'
Result: 'E' notifications (for "exists") for all local files. Local
modifications remain present. Only moves are lost (deletes are now
"missing", and adds are now "unversioned" -- it's easy to fix those).
(btw: usually I only do this if there are no local moves that need to
be preserved)
--
Johan
Re: [PATCH v2] Reject checkouts to existing directory
Posted by Patrick Steinhardt <pa...@elegosoft.com>.
On Fri, Oct 28, 2016 at 12:34:44AM +0200, Stefan wrote:
> On 10/27/2016 21:45, Stefan Sperling wrote:
> > On Tue, Oct 18, 2016 at 02:22:25PM +0200, Patrick Steinhardt wrote:
> >> Hi,
> >>
> >> finally got around to update my patch regarding checkouts to
> >> existing directories. The semantics have been changed to accept
> >> checkouts iff
> >>
> >> - the target directory does not exist
> >> - the target directory is empty
> >> - the repository to check out is empty
> >> - the --force flag is given
> > I think this makes a lot of sense. I just have not yet had time to
> > review and test your patch. I will try to do so soon.
> >
> > Does anyone reading this list have any concerns about this change?
>
> I'm +1 on the general design/behavior change. Didn't do a code/patch
> review, though.
>
> On a minor side note:
> While talking last week on IRC to Daniel, he mentioned (on a different
> topic) that in general it might be preferable to use a separate explicit
> command line options to control the exact behavior over one which
> impacts several behaviors at once. Reflecting that onto this case, it
> crossed my mind that --allow-non-empty-directory (or --allow-non-empty)
> might be preferable over adding that behavior to the --force parameter,
> since the --force parameter has (or in the future might have) other
> implications in addition to allowing a co into a non-empty directory.
>
> Though in this case, I don't have a strong opinion to go one way or the
> other.
>
> Regards,
> Stefan
>
>
I think I agree with introducing a new flag here. Mixing and
extending semantics of an existing flag surely is not a nice
route to go. I'd vote for '--allow-non-empty-target'.
Regards
Patrick
--
Patrick Steinhardt, Entwickler
elego Software Solutions GmbH, http://www.elego.de
Gebäude 12 (BIG), Gustav-Meyer-Allee 25, 13355 Berlin, Germany
Sitz der Gesellschaft: Berlin, USt-IdNr.: DE 163214194
Handelsregister: Amtsgericht Charlottenburg HRB 77719
Geschäftsführer: Olaf Wagner
Re: [PATCH v2] Reject checkouts to existing directory
Posted by Stefan <lu...@posteo.de>.
On 10/27/2016 21:45, Stefan Sperling wrote:
> On Tue, Oct 18, 2016 at 02:22:25PM +0200, Patrick Steinhardt wrote:
>> Hi,
>>
>> finally got around to update my patch regarding checkouts to
>> existing directories. The semantics have been changed to accept
>> checkouts iff
>>
>> - the target directory does not exist
>> - the target directory is empty
>> - the repository to check out is empty
>> - the --force flag is given
> I think this makes a lot of sense. I just have not yet had time to
> review and test your patch. I will try to do so soon.
>
> Does anyone reading this list have any concerns about this change?
I'm +1 on the general design/behavior change. Didn't do a code/patch
review, though.
On a minor side note:
While talking last week on IRC to Daniel, he mentioned (on a different
topic) that in general it might be preferable to use a separate explicit
command line options to control the exact behavior over one which
impacts several behaviors at once. Reflecting that onto this case, it
crossed my mind that --allow-non-empty-directory (or --allow-non-empty)
might be preferable over adding that behavior to the --force parameter,
since the --force parameter has (or in the future might have) other
implications in addition to allowing a co into a non-empty directory.
Though in this case, I don't have a strong opinion to go one way or the
other.
Regards,
Stefan
Re: [PATCH v2] Reject checkouts to existing directory
Posted by Stefan Sperling <st...@elego.de>.
On Tue, Oct 18, 2016 at 02:22:25PM +0200, Patrick Steinhardt wrote:
> Hi,
>
> finally got around to update my patch regarding checkouts to
> existing directories. The semantics have been changed to accept
> checkouts iff
>
> - the target directory does not exist
> - the target directory is empty
> - the repository to check out is empty
> - the --force flag is given
I think this makes a lot of sense. I just have not yet had time to
review and test your patch. I will try to do so soon.
Does anyone reading this list have any concerns about this change?