You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ben Reser <be...@reser.org> on 2004/12/01 01:27:35 UTC
Re: [PATCH] Merging replacments of directories and Issue #2144
On Tue, Nov 30, 2004 at 11:55:05PM +0000, Philip Martin wrote:
> I am more likely to review patches that are inline in email...
Well it's already on the issue so it seems silly to inline it in an
email... Especially considering there is considerable documentation on
the issue that you need to look at anyway.
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Merging replacments of directories and Issue #2144
Posted by Ben Reser <be...@reser.org>.
On Wed, Dec 01, 2004 at 02:26:42AM +0000, Philip Martin wrote:
> I was expecting your elegant patch and lucid log message to be
> sufficient.
Well we don't usually put reproduction recipes into the log message and
well the test suite isn't exactly an elegant way of representing a
reproduction recipe...
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Merging replacments of directories and Issue #2144
Posted by Ben Reser <be...@reser.org>.
On Wed, Dec 01, 2004 at 10:44:36PM +0000, Philip Martin wrote:
> Ben Reser <be...@reser.org> writes:
>
> > As far as I've seen this is a problem that's pretty unique to the
> > merge operation.
>
> At present 'svn add' can be used to replace a schedule delete item but
> trying to use 'svn cp' doesn't work (an issue exists). There are
> problems in libsvn_client that stop the operation getting as far as
> svn_wc_ensure_adm but I think your change is part of the solution.
Hmm, I hadn't run into the svn_wc_ensure_adm problem. But I think I
know why. cp by itself can't replace a directory. Because if the
destination is a directory we place the source into the directory... I
hadn't bothered to try to implement the following scenario:
svn rm dir
svn cp -r 5 dir
This does indeed fail but it fails further up before
svn_wc_ensure_adm().
Anyway after looking at the revert problem in Issue #2148. I think the
original tactic I took in fixing the cp problem isn't the way to go.
Because I was ignoring the directory problem because I was thinking it
didn't exist.
I'll send an email detailing further what I think needs to be done.
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Merging replacments of directories and Issue #2144
Posted by Philip Martin <ph...@codematters.co.uk>.
Ben Reser <be...@reser.org> writes:
> As far as I've seen this is a problem that's pretty unique to the
> merge operation.
At present 'svn add' can be used to replace a schedule delete item but
trying to use 'svn cp' doesn't work (an issue exists). There are
problems in libsvn_client that stop the operation getting as far as
svn_wc_ensure_adm but I think your change is part of the solution.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Merging replacments of directories and Issue #2144
Posted by Ben Reser <be...@reser.org>.
On Wed, Dec 01, 2004 at 04:13:24PM -0600, kfogel@collab.net wrote:
> I think Ben had exactly that in mind, if I'm remembering a
> conversation correctly...
Not exactly. This problem was found by someone doing actual merges.
The fact that the problem ended up overlapping my work on allowing
replacements in the working copies is just a coincidence. As far as
I've seen this is a problem that's pretty unique to the merge operation.
That said, fixing Issue 2144 exposed some problems with revert, which
I'm working on now. At this point I believe it can be fixed in 1.1.x.
But if it can't I withdraw this change from 1.1.x and we'll just have to
wait on truely fixing this until 1.2.0.
So basically several things I was working on independently ended up
overlapping. Which I guess worked out rather nicely.
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Merging replacments of directories and Issue #2144
Posted by kf...@collab.net.
Philip Martin <ph...@codematters.co.uk> writes:
> kfogel@collab.net writes:
I wrote? Are you sure that wasn't breser? :-)
> > - /* The revisions must match except when adding a directory with a
> > - name that matches a directory scheduled for deletion. That's
> > - because the deleted directory's administrative dir will still be
> > - in place but will have an arbitrary revision. */
> > - if (entry->revision != revision
> > - && !(entry->schedule == svn_wc_schedule_delete && revision == 0))
> > - return
> > - svn_error_createf
> > - (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> > - _("Revision %ld doesn't match existing revision %ld in '%s'"),
> > - revision, entry->revision,
> > - svn_path_local_style (path, pool));
> > + /* When the directory exists and is scheduled for deletion do not
> > + * check the revision or the URL. The revision can be any
> > + * arbitrary revision and the URL may differ if the add is
> > + * being driven from a merge which will have a different URL. */
> > + if (entry->schedule != svn_wc_schedule_delete)
> > + {
> > + if (entry->revision != revision)
> > + return
> > + svn_error_createf
> > + (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> > + _("Revision %ld doesn't match existing revision %ld in '%s'"),
> > + revision, entry->revision, path);
> >
> > - /** ### comparing URLs, should they be canonicalized first? */
> > - if (strcmp (entry->url, url) != 0)
> > - return
> > - svn_error_createf
> > - (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> > - _("URL '%s' doesn't match existing URL '%s' in '%s'"),
> > - url, entry->url,
> > - svn_path_local_style (path, pool));
> > + /** ### comparing URLs, should they be canonicalized first? */
> > + if (strcmp (entry->url, url) != 0)
> > + return
> > + svn_error_createf
> > + (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> > + _("URL '%s' doesn't match existing URL '%s' in '%s'"),
> > + url, entry->url, path);
> > + }
>
> That looks reasonable. I remember looking at that code in the past,
> when merge was using an extra copy to do add-with-history, and I
> wasn't confident enough to change it!
>
> I think there's a related issue: 'svn cp' cannot be used to schedule a
> replacement for a schedule delete item. It's possible your change
> will lay the ground for fixing that as well, it might be just a
> libsvn_client problem now.
I think Ben had exactly that in mind, if I'm remembering a
conversation correctly...
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Merging replacments of directories and Issue #2144
Posted by Ben Reser <be...@reser.org>.
On Wed, Dec 01, 2004 at 08:52:47PM +0000, Philip Martin wrote:
> I think there's a related issue: 'svn cp' cannot be used to schedule a
> replacement for a schedule delete item. It's possible your change
> will lay the ground for fixing that as well, it might be just a
> libsvn_client problem now.
I've already got that mostly fixed in my working copy. I just need to
get around to finishing up dealing with handing properties.
--
Ben Reser <be...@reser.org>
http://ben.reser.org
"Conscience is the inner voice which warns us somebody may be looking."
- H.L. Mencken
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Merging replacments of directories and Issue #2144
Posted by Philip Martin <ph...@codematters.co.uk>.
kfogel@collab.net writes:
> - /* The revisions must match except when adding a directory with a
> - name that matches a directory scheduled for deletion. That's
> - because the deleted directory's administrative dir will still be
> - in place but will have an arbitrary revision. */
> - if (entry->revision != revision
> - && !(entry->schedule == svn_wc_schedule_delete && revision == 0))
> - return
> - svn_error_createf
> - (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> - _("Revision %ld doesn't match existing revision %ld in '%s'"),
> - revision, entry->revision,
> - svn_path_local_style (path, pool));
> + /* When the directory exists and is scheduled for deletion do not
> + * check the revision or the URL. The revision can be any
> + * arbitrary revision and the URL may differ if the add is
> + * being driven from a merge which will have a different URL. */
> + if (entry->schedule != svn_wc_schedule_delete)
> + {
> + if (entry->revision != revision)
> + return
> + svn_error_createf
> + (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> + _("Revision %ld doesn't match existing revision %ld in '%s'"),
> + revision, entry->revision, path);
>
> - /** ### comparing URLs, should they be canonicalized first? */
> - if (strcmp (entry->url, url) != 0)
> - return
> - svn_error_createf
> - (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> - _("URL '%s' doesn't match existing URL '%s' in '%s'"),
> - url, entry->url,
> - svn_path_local_style (path, pool));
> + /** ### comparing URLs, should they be canonicalized first? */
> + if (strcmp (entry->url, url) != 0)
> + return
> + svn_error_createf
> + (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
> + _("URL '%s' doesn't match existing URL '%s' in '%s'"),
> + url, entry->url, path);
> + }
That looks reasonable. I remember looking at that code in the past,
when merge was using an extra copy to do add-with-history, and I
wasn't confident enough to change it!
I think there's a related issue: 'svn cp' cannot be used to schedule a
replacement for a schedule delete item. It's possible your change
will lay the ground for fixing that as well, it might be just a
libsvn_client problem now.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Merging replacments of directories and Issue #2144
Posted by kf...@collab.net.
Philip Martin <ph...@codematters.co.uk> writes:
> *shrug* it's what I prefer.
Consider it done, sir.
> > Especially considering there is considerable documentation on
> > the issue that you need to look at anyway.
>
> I was expecting your elegant patch and lucid log message to be
> sufficient.
Ben's elegant patch and lucid log message follow:
--------------------8-<-------cut-here---------8-<-----------------------
Stop checking the URL and the revision of the existing working copy dir when
attempting to replace the directory with a new directory that has history.
Fixes Issue #2144.
* subversion/includes/svn_wc.h
(svn_wc_ensure_adm): Adjust the documentation about how we handle
scheduled deleted directories.
* subversion/libsvn_wc/adm_files.c
(check_adm_exists): Do not check the URL or the revision of a directory
scheduled for deletion.
* subversion/tests/clients/cmdline/merge_tests.py
(merge_dir_replace): New test for testing merging a directory replacement.
(test_list): Add merge_dir_replace to the list of tests to run.
Index: subversion/include/svn_wc.h
===================================================================
--- subversion/include/svn_wc.h (revision 12095)
+++ subversion/include/svn_wc.h (working copy)
@@ -1145,10 +1145,10 @@
* initialized to an unlocked state.
*
* If the administrative area already exists then the given @a url
- * must match the URL in the administrative area or an error will be
- * returned. The given @a revision must also match except for the
- * special case of adding a directory that has a name matching one
- * scheduled for deletion, in which case @a revision must be zero.
+ * must match the URL in the administrative area and the given
+ * @a revision must match the BASE of the working copy dir unless
+ * the admin directory is scheduled for deletion or the
+ * SVN_ERR_WC_OBSTRUCTED_UPDATE error will be returned.
*
* @a uuid may be @c NULL.
Index: subversion/libsvn_wc/adm_files.c
===================================================================
--- subversion/libsvn_wc/adm_files.c (revision 12095)
+++ subversion/libsvn_wc/adm_files.c (working copy)
@@ -892,27 +892,27 @@
_("No entry for '%s'"),
svn_path_local_style (path, pool));
- /* The revisions must match except when adding a directory with a
- name that matches a directory scheduled for deletion. That's
- because the deleted directory's administrative dir will still be
- in place but will have an arbitrary revision. */
- if (entry->revision != revision
- && !(entry->schedule == svn_wc_schedule_delete && revision == 0))
- return
- svn_error_createf
- (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
- _("Revision %ld doesn't match existing revision %ld in '%s'"),
- revision, entry->revision,
- svn_path_local_style (path, pool));
+ /* When the directory exists and is scheduled for deletion do not
+ * check the revision or the URL. The revision can be any
+ * arbitrary revision and the URL may differ if the add is
+ * being driven from a merge which will have a different URL. */
+ if (entry->schedule != svn_wc_schedule_delete)
+ {
+ if (entry->revision != revision)
+ return
+ svn_error_createf
+ (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
+ _("Revision %ld doesn't match existing revision %ld in '%s'"),
+ revision, entry->revision, path);
- /** ### comparing URLs, should they be canonicalized first? */
- if (strcmp (entry->url, url) != 0)
- return
- svn_error_createf
- (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
- _("URL '%s' doesn't match existing URL '%s' in '%s'"),
- url, entry->url,
- svn_path_local_style (path, pool));
+ /** ### comparing URLs, should they be canonicalized first? */
+ if (strcmp (entry->url, url) != 0)
+ return
+ svn_error_createf
+ (SVN_ERR_WC_OBSTRUCTED_UPDATE, NULL,
+ _("URL '%s' doesn't match existing URL '%s' in '%s'"),
+ url, entry->url, path);
+ }
}
*exists = wc_exists;
Index: subversion/tests/clients/cmdline/merge_tests.py
===================================================================
--- subversion/tests/clients/cmdline/merge_tests.py (revision 12095)
+++ subversion/tests/clients/cmdline/merge_tests.py (working copy)
@@ -2195,6 +2195,129 @@
os.chdir(saved_cwd)
+#----------------------------------------------------------------------
+# A merge that replaces a directory
+# Tests for Issue #2144
+
+def merge_dir_replace(sbox):
+ "merge a replacement of a directory"
+
+ sbox.build()
+ wc_dir = sbox.wc_dir
+
+ C_path = os.path.join(wc_dir, 'A', 'C')
+ F_path = os.path.join(wc_dir, 'A', 'B', 'F')
+ F_url = svntest.main.current_repo_url + '/A/B/F'
+
+ foo_path = os.path.join(F_path, 'foo')
+
+ # Create foo in F
+ svntest.actions.run_and_verify_svn(None, None, [], 'mkdir', foo_path)
+
+ expected_output = wc.State(wc_dir, {
+ 'A/B/F/foo' : Item(verb='Adding'),
+ })
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 2)
+ expected_status.tweak(wc_rev=1)
+ expected_status.add({
+ 'A/B/F/foo' : Item(status=' ', wc_rev=2, repos_rev=2),
+ })
+ svntest.actions.run_and_verify_commit(wc_dir,
+ expected_output,
+ expected_status,
+ None, None, None, None, None,
+ wc_dir)
+
+ # Merge foo onto C
+ expected_output = wc.State(C_path, {
+ 'foo' : Item(status='A '),
+ })
+ expected_disk = wc.State('', {
+ 'foo' : Item(),
+ })
+ expected_status = wc.State(C_path, {
+ '' : Item(status=' ', wc_rev=1, repos_rev=2),
+ 'foo' : Item(status='A ', wc_rev='-', copied='+', repos_rev=2),
+ })
+ expected_skip = wc.State(C_path, { })
+ svntest.actions.run_and_verify_merge(C_path, '1', '2', F_url,
+ expected_output,
+ expected_disk,
+ expected_status,
+ expected_skip)
+
+ # Commit merge of foo onto C
+ expected_output = svntest.wc.State(wc_dir, {
+ 'A/C/foo' : Item(verb='Adding'),
+ })
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 3)
+ expected_status.tweak(wc_rev=1)
+ expected_status.add({
+ 'A/B/F/foo' : Item(status=' ', wc_rev=2, repos_rev=3),
+ 'A/C/foo' : Item(status=' ', wc_rev=3, repos_rev=3),
+ })
+ svntest.actions.run_and_verify_commit(wc_dir,
+ expected_output,
+ expected_status,
+ None, None, None, None, None,
+ wc_dir)
+
+ # Delete foo on F
+ svntest.actions.run_and_verify_svn(None, None, [], 'rm', foo_path)
+ expected_output = svntest.wc.State(wc_dir, {
+ 'A/B/F/foo' : Item(verb='Deleting'),
+ })
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 4)
+ expected_status.tweak(wc_rev=1)
+ expected_status.add({
+ 'A/C/foo' : Item(status=' ', wc_rev=3, repos_rev=4),
+ })
+ svntest.actions.run_and_verify_commit(wc_dir,
+ expected_output,
+ expected_status,
+ None, None, None, None, None,
+ wc_dir)
+
+ # Recreate foo in F
+ svntest.actions.run_and_verify_svn(None, None, [], 'mkdir', foo_path)
+
+ expected_output = wc.State(wc_dir, {
+ 'A/B/F/foo' : Item(verb='Adding'),
+ })
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 5)
+ expected_status.tweak(wc_rev=1)
+ expected_status.add({
+ 'A/B/F/foo' : Item(status=' ', wc_rev=5, repos_rev=5),
+ 'A/C/foo' : Item(status=' ', wc_rev=3, repos_rev=5),
+ })
+ svntest.actions.run_and_verify_commit(wc_dir,
+ expected_output,
+ expected_status,
+ None, None, None, None, None,
+ wc_dir)
+ # Merge replacement of foo onto C
+ expected_output = wc.State(C_path, {
+ 'foo' : Item(status='D '),
+ 'foo' : Item(status='A '),
+ })
+ expected_disk = wc.State('', {
+ 'foo' : Item(),
+ })
+ expected_status = wc.State(C_path, {
+ '' : Item(status=' ', wc_rev=1, repos_rev=5),
+ 'foo' : Item(status='R ', wc_rev='-', copied='+', repos_rev=5),
+ })
+ expected_skip = wc.State(C_path, { })
+ svntest.actions.run_and_verify_merge(C_path, '2', '5', F_url,
+ expected_output,
+ expected_disk,
+ expected_status,
+ expected_skip,
+ None, None, None, None, None,
+ 0, # skip props
+ 0) # don't do a dry-run the output differs
+
+
########################################################################
# Run the tests
@@ -2220,6 +2343,7 @@
merge_funny_chars_on_path,
merge_keyword_expansions,
merge_prop_change_to_deleted_target,
+ merge_dir_replace,
# property_merges_galore, # Would be nice to have this.
# tree_merges_galore, # Would be nice to have this.
# various_merges_galore, # Would be nice to have this.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Merging replacments of directories and Issue #2144
Posted by Philip Martin <ph...@codematters.co.uk>.
Ben Reser <be...@reser.org> writes:
> On Tue, Nov 30, 2004 at 11:55:05PM +0000, Philip Martin wrote:
>> I am more likely to review patches that are inline in email...
>
> Well it's already on the issue so it seems silly to inline it in an
> email...
*shrug* it's what I prefer.
> Especially considering there is considerable documentation on
> the issue that you need to look at anyway.
I was expecting your elegant patch and lucid log message to be
sufficient.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org