You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by David Glasser <gl...@davidglasser.net> on 2008/04/09 02:11:31 UTC
Re: svn commit: r30438 - in branches/double-delete/subversion: libsvn_ra_neon libsvn_ra_serf libsvn_repos tests/cmdline
Bikeshedding, but I kind of think a more explicit error message would
be good here: "File or directory 'foo' already deleted"? Except
that's also a weird message to get. Honestly, AFAICT, the only real
issue we're trying to deal with here is double-renames, not a pure
double delete... perhaps the error should allude to that? Otherwise I
don't see how a user is going to figure out that what they're supposed
to be doing is making sure that this isn't a double-rename.
--dave
On Tue, Apr 8, 2008 at 2:13 PM, <sb...@tigris.org> wrote:
> Author: sbutler
> Date: Tue Apr 8 14:13:24 2008
> New Revision: 30438
>
> Log:
> On the double-delete feature branch, block any commit that includes
> the deletion of a file or directory that has already been deleted in
> the repository.
>
> * subversion/libsvn_ra_serf/commit.c
> (delete_entry): Treat the response '404 Not Found' as an error.
>
> * subversion/libsvn_ra_neon/commit.c
> (commit_delete_entry): Treat the response '404 Not Found' as an error.
>
> * subversion/libsvn_repos/commit.c
> (out_of_date): Extend for case where we don't know the node type
> because the node does not exist.
> (delete_entry): If a path is not found, return an error.
>
> * subversion/tests/cmdline/commit_tests.py
> (commit_out_of_date_deletions): Extend test cases to include all
> relevant combinations of deletion vs (property-change | text-change |
> deletion). Use svntest.actions.run_and_verify_commit consistently.
>
> Modified:
> branches/double-delete/subversion/libsvn_ra_neon/commit.c
> branches/double-delete/subversion/libsvn_ra_serf/commit.c
> branches/double-delete/subversion/libsvn_repos/commit.c
> branches/double-delete/subversion/tests/cmdline/commit_tests.py
>
> Modified: branches/double-delete/subversion/libsvn_ra_neon/commit.c
> URL: http://svn.collab.net/viewvc/svn/branches/double-delete/subversion/libsvn_ra_neon/commit.c?pathrev=30438&r1=30437&r2=30438
> ==============================================================================
> --- branches/double-delete/subversion/libsvn_ra_neon/commit.c Tue Apr 8 13:30:49 2008 (r30437)
> +++ branches/double-delete/subversion/libsvn_ra_neon/commit.c Tue Apr 8 14:13:24 2008 (r30438)
> @@ -751,15 +751,11 @@ static svn_error_t * commit_delete_entry
> APR_HASH_KEY_STRING, SVN_DAV_OPTION_KEEP_LOCKS);
> }
>
> - /* 404 is ignored, because mod_dav_svn is effectively merging
> - against the HEAD revision on-the-fly. In such a universe, a
> - failed deletion (because it's already missing) is OK; deletion
> - is an idempotent merge operation. */
> serr = svn_ra_neon__simple_request(&code, parent->cc->ras,
> "DELETE", child,
> extra_headers, NULL,
> - 204 /* Created */,
> - 404 /* Not Found */, pool);
> + 204 /* No Content */,
> + NULL, pool);
>
> /* A locking-related error most likely means we were deleting a
> directory rather than a file, and didn't send all of the
>
> Modified: branches/double-delete/subversion/libsvn_ra_serf/commit.c
> URL: http://svn.collab.net/viewvc/svn/branches/double-delete/subversion/libsvn_ra_serf/commit.c?pathrev=30438&r1=30437&r2=30438
> ==============================================================================
> --- branches/double-delete/subversion/libsvn_ra_serf/commit.c Tue Apr 8 13:30:49 2008 (r30437)
> +++ branches/double-delete/subversion/libsvn_ra_serf/commit.c Tue Apr 8 14:13:24 2008 (r30438)
> @@ -1287,11 +1287,8 @@ delete_entry(const char *path,
> return err;
> }
>
> - /* 204 No Content: item successfully deleted
> - 404 Not found: ignored, the item might have been deleted in this
> - transaction. */
> - if (delete_ctx->progress.status != 204 &&
> - delete_ctx->progress.status != 404)
> + /* 204 No Content: item successfully deleted */
> + if (delete_ctx->progress.status != 204)
> {
> return return_response_err(handler, &delete_ctx->progress);
> }
>
> Modified: branches/double-delete/subversion/libsvn_repos/commit.c
> URL: http://svn.collab.net/viewvc/svn/branches/double-delete/subversion/libsvn_repos/commit.c?pathrev=30438&r1=30437&r2=30438
> ==============================================================================
> --- branches/double-delete/subversion/libsvn_repos/commit.c Tue Apr 8 13:30:49 2008 (r30437)
> +++ branches/double-delete/subversion/libsvn_repos/commit.c Tue Apr 8 14:13:24 2008 (r30438)
> @@ -125,8 +125,10 @@ out_of_date(const char *path, svn_node_k
> return svn_error_createf(SVN_ERR_FS_TXN_OUT_OF_DATE, NULL,
> (kind == svn_node_dir
> ? _("Directory '%s' is out of date")
> - : _("File '%s' is out of date")),
> - path);
> + : kind == svn_node_file
> + ? _("File '%s' is out of date")
> + : _("File or directory '%s' is out of date")),
> + path);
> }
>
>
> @@ -237,10 +239,9 @@ delete_entry(const char *path,
> SVN_ERR(check_authz(eb, parent->path, eb->txn_root,
> svn_authz_write, pool));
>
> - /* If PATH doesn't exist in the txn, that's fine (merge
> - allows this). */
> + /* If PATH doesn't exist in the txn, the working copy is out of date. */
> if (kind == svn_node_none)
> - return SVN_NO_ERROR;
> + return out_of_date(full_path, kind);
>
> /* Now, make sure we're deleting the node we *think* we're
> deleting, else return an out-of-dateness error. */
>
> Modified: branches/double-delete/subversion/tests/cmdline/commit_tests.py
> URL: http://svn.collab.net/viewvc/svn/branches/double-delete/subversion/tests/cmdline/commit_tests.py?pathrev=30438&r1=30437&r2=30438
> ==============================================================================
> --- branches/double-delete/subversion/tests/cmdline/commit_tests.py Tue Apr 8 13:30:49 2008 (r30437)
> +++ branches/double-delete/subversion/tests/cmdline/commit_tests.py Tue Apr 8 14:13:24 2008 (r30438)
> @@ -1609,63 +1609,100 @@ def commit_nonrecursive(sbox):
> #----------------------------------------------------------------------
> # Regression for #1017: ra_neon was allowing the deletion of out-of-date
> # files or dirs, which majorly violates Subversion's semantics.
> -
> +# An out-of-date error should be raised if the object to be committed has
> +# already been deleted or modified in the repo.
>
> def commit_out_of_date_deletions(sbox):
> "commit deletion of out-of-date file or dir"
>
> + # Path WC 1 WC backup
> + # =========== ==== =========
> + # A/C pset del
> + # A/I del pset
> + # A/B/F del del
> + # A/D/H/omega text del
> + # A/B/E/alpha pset del
> + # A/D/H/chi del text
> + # A/B/E/beta del pset
> + # A/D/H/psi del del
> +
> sbox.build()
> wc_dir = sbox.wc_dir
> +
> + # Need another empty dir
> + I_path = os.path.join(wc_dir, 'A', 'I')
> + os.mkdir(I_path)
> + svntest.main.run_svn(None, 'add', I_path)
> + svntest.main.run_svn(None, 'ci', '-m', 'prep', wc_dir)
> + svntest.main.run_svn(None, 'up', wc_dir)
>
> # Make a backup copy of the working copy
> wc_backup = sbox.add_wc_path('backup')
> svntest.actions.duplicate_dir(wc_dir, wc_backup)
>
> - # Change omega's text, and make a propchange to A/C directory
> - omega_path = os.path.join(wc_dir, 'A', 'D', 'H', 'omega')
> + # Edits in wc 1
> C_path = os.path.join(wc_dir, 'A', 'C')
> - svntest.main.file_append(omega_path, 'appended omega text')
> + omega_path = os.path.join(wc_dir, 'A', 'D', 'H', 'omega')
> + alpha_path = os.path.join(wc_dir, 'A', 'B', 'E', 'alpha')
> svntest.main.run_svn(None, 'propset', 'fooprop', 'foopropval', C_path)
> + svntest.main.file_append(omega_path, 'appended omega text')
> + svntest.main.run_svn(None, 'propset', 'fooprop', 'foopropval', alpha_path)
>
> - # Commit revision 2.
> - expected_output = svntest.wc.State(wc_dir, {
> - 'A/D/H/omega' : Item(verb='Sending'),
> - 'A/C' : Item(verb='Sending'),
> - })
> - expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
> - expected_status.tweak('A/D/H/omega', 'A/C', wc_rev=2, status=' ')
> -
> - svntest.actions.run_and_verify_commit(wc_dir,
> - expected_output,
> - expected_status,
> - None,
> - wc_dir)
> + # Deletions in wc 1
> + I_path = os.path.join(wc_dir, 'A', 'I')
> + F_path = os.path.join(wc_dir, 'A', 'B', 'F')
> + chi_path = os.path.join(wc_dir, 'A', 'D', 'H', 'chi')
> + beta_path = os.path.join(wc_dir, 'A', 'B', 'E', 'beta')
> + psi_path = os.path.join(wc_dir, 'A', 'D', 'H', 'psi')
> + svntest.main.run_svn(None, 'rm', I_path, F_path, chi_path, beta_path,
> + psi_path)
>
> - # Now, in the second working copy, schedule both omega and C for deletion.
> - omega_path = os.path.join(wc_backup, 'A', 'D', 'H', 'omega')
> + # Commit in wc 1
> + expected_output = svntest.wc.State(wc_dir, {
> + 'A/C' : Item(verb='Sending'),
> + 'A/I' : Item(verb='Deleting'),
> + 'A/B/F' : Item(verb='Deleting'),
> + 'A/D/H/omega' : Item(verb='Sending'),
> + 'A/B/E/alpha' : Item(verb='Sending'),
> + 'A/D/H/chi' : Item(verb='Deleting'),
> + 'A/B/E/beta' : Item(verb='Deleting'),
> + 'A/D/H/psi' : Item(verb='Deleting'),
> + })
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 2)
> + expected_status.tweak('A/C', 'A/D/H/omega', 'A/B/E/alpha', wc_rev=3,
> + status=' ')
> + expected_status.remove('A/B/F', 'A/D/H/chi', 'A/B/E/beta', 'A/D/H/psi')
> + commit = svntest.actions.run_and_verify_commit
> + commit(wc_dir, expected_output, expected_status, None, wc_dir)
> +
> + # Edits in wc backup
> + I_path = os.path.join(wc_backup, 'A', 'I')
> + chi_path = os.path.join(wc_backup, 'A', 'D', 'H', 'chi')
> + beta_path = os.path.join(wc_backup, 'A', 'B', 'E','beta')
> + svntest.main.run_svn(None, 'propset', 'fooprop', 'foopropval', I_path)
> + svntest.main.file_append(chi_path, 'appended chi text')
> + svntest.main.run_svn(None, 'propset', 'fooprop', 'foopropval', beta_path)
> +
> + # Deletions in wc backup
> C_path = os.path.join(wc_backup, 'A', 'C')
> - svntest.main.run_svn(None, 'rm', omega_path, C_path)
> -
> - # Attempt to delete omega. This should return an (expected)
> - # out-of-dateness error.
> - exit_code, outlines, errlines = svntest.main.run_svn(1, 'commit', '-m',
> - 'blah', omega_path)
> - for line in errlines:
> - if re.match(".*[Oo]ut.of.date.*", line):
> - break
> - else:
> - raise svntest.Failure
> -
> - # Attempt to delete directory C. This should return an (expected)
> - # out-of-dateness error.
> - exit_code, outlines, errlines = svntest.main.run_svn(1, 'commit', '-m',
> - 'blah', C_path)
> - for line in errlines:
> - if re.match(".*[Oo]ut.of.date.*", line):
> - break
> - else:
> - raise svntest.Failure
> -
> + F_path = os.path.join(wc_backup, 'A', 'B', 'F')
> + omega_path = os.path.join(wc_backup, 'A', 'D', 'H', 'omega')
> + alpha_path = os.path.join(wc_backup, 'A', 'B', 'E', 'alpha')
> + psi_path = os.path.join(wc_backup, 'A', 'D', 'H', 'psi')
> + svntest.main.run_svn(None, 'rm', C_path, F_path, omega_path, alpha_path,
> + psi_path)
> +
> + # A commit of any one of these files or dirs should fail
> + error_re = "out of date"
> + commit(wc_backup, None, None, error_re, C_path)
> + commit(wc_backup, None, None, "File not found: transaction", I_path)
> + commit(wc_backup, None, None, error_re, F_path)
> + commit(wc_backup, None, None, error_re, omega_path)
> + commit(wc_backup, None, None, error_re, alpha_path)
> + commit(wc_backup, None, None, "File not found: transaction", chi_path)
> + commit(wc_backup, None, None, "File not found: transaction", beta_path)
> + commit(wc_backup, None, None, error_re, psi_path)
> +
> def commit_with_bad_log_message(sbox):
> "commit with a log message containing bad data"
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: svn-help@subversion.tigris.org
>
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org