Stephen Butler wrote:
> Hi folks,
>
> Here is a patch that resolves issue #2740 ("out-of-date" error text
> varies depending on access method) and plugs a loophole in the commit
> safety net, as far as tree conflict detection is concerned.
>
> I mentioned this patch in an earlier email about the behavior of merge
> (http://svn.haxx.se/dev/archive-2008-02/0872.shtml). Our current tree
> conflict detection scheme relies on this patch, but this patch is not
> specific to tree conflicts.
Stephen,
This sounds like you're doing the right thing. I have reviewed the patch today
and have only a few simple observations. I'd be happy to see you commit this
(or I'll commit it for you) if you can address them.
> [[[
>
> When a Subversion client tries to commit a deletion, a property
> modification, or a text modification, and the item no longer exists in
> the repository, the commit should fail with an "out of date" error.
>
> The error message should be consistent for all remote access methods
> (v. issue #2740).
>
> * subversion/libsvn_client/commit_util.c
> (out_of_date): New function.
> (do_item_commit): To give the user an "out of date" error instead of
> "not found", replace SVN_ERR() with manual error handling when
> asking the repo to delete dirs or files, to open files, or to
> receive property deltas during commit.
>
> * subversion/libsvn_client/commit.c
> (svn_client_commit4): Give the user an "out of date" error instead
> of "not found", if the RA editor can't be initialized.
>
> * 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.
>
> ]]]
>
> Index: subversion/libsvn_client/commit_util.c
> ===================================================================
> --- subversion/libsvn_client/commit_util.c (revision 29632)
> +++ subversion/libsvn_client/commit_util.c (working copy)
> @@ -44,6 +44,16 @@
> #define SVN_CLIENT_COMMIT_DEBUG
> */
>
> +/* Create and return a generic out-of-dateness error. */
> +static svn_error_t *
> +out_of_date(const char *path, svn_node_kind_t kind)
> +{
> + return svn_error_createf(SVN_ERR_WC_NOT_UP_TO_DATE, NULL,
> + (kind == svn_node_dir
> + ? _("Directory '%s' is out of date")
> + : _("File '%s' is out of date")),
> + path);
> +}
>
>
> /*** Harvesting Commit Candidates ***/
> @@ -1082,6 +1092,7 @@ do_item_commit(void **dir_baton,
> apr_hash_t *file_mods = cb_baton->file_mods;
> const char *notify_path_prefix = cb_baton->notify_path_prefix;
> svn_client_ctx_t *ctx = cb_baton->ctx;
> + svn_error_t *err = SVN_NO_ERROR;
>
> /* Do some initializations. */
> *dir_baton = NULL;
> @@ -1191,8 +1202,17 @@ do_item_commit(void **dir_baton,
> if (item->state_flags & SVN_CLIENT_COMMIT_ITEM_DELETE)
> {
> assert(parent_baton);
> - SVN_ERR(editor->delete_entry(path, item->revision,
> - parent_baton, pool));
> + err = editor->delete_entry(path, item->revision, parent_baton, pool);
> + if (err)
> + {
> + if (err->apr_err == SVN_ERR_FS_NOT_FOUND
> + || err->apr_err == SVN_ERR_RA_DAV_PATH_NOT_FOUND)
So there are two errors that definitely mean "out of date", and these are them?
I've no reason to doubt it, but nor do I know why. I'd be happy to hear you
confirm, "Yes, there are exactly two such errors and those are them, in all
four cases that we deal with in this function."
> + {
> + svn_error_clear(err);
> + return out_of_date(path, kind);
> + }
> + return err;
> + }
> }
>
> /* If this item is supposed to be added, do so. */
> @@ -1248,9 +1268,19 @@ do_item_commit(void **dir_baton,
> if (! file_baton)
> {
> assert(parent_baton);
> - SVN_ERR(editor->open_file(path, parent_baton,
> - item->revision,
> - file_pool, &file_baton));
> + err = editor->open_file(path, parent_baton,
> + item->revision,
> + file_pool, &file_baton);
> + if (err)
> + {
> + if (err->apr_err == SVN_ERR_FS_NOT_FOUND
> + || err->apr_err == SVN_ERR_RA_DAV_PATH_NOT_FOUND)+ {
(A glitch in the patch there, but I inserted the missing newline by hand.)
> + svn_error_clear(err);
> + return out_of_date(path, kind);
> + }
> + return err;
> + }
> +
> }
> }
> else
> @@ -1259,24 +1290,38 @@ do_item_commit(void **dir_baton,
[...]
>
> SVN_ERR(svn_wc_entry(&tmp_entry, item->path, adm_access, TRUE, pool));
> - SVN_ERR(svn_wc_transmit_prop_deltas
> - (item->path, adm_access, tmp_entry, editor,
> - (kind == svn_node_dir) ? *dir_baton : file_baton, NULL, pool));
>
> + /* When committing a dir that no longer exists in the repo, the
> + "not found" error appears during the delta transmisssion. */
> + err = svn_wc_transmit_prop_deltas
> + (item->path, adm_access, tmp_entry, editor,
> + (kind == svn_node_dir) ? *dir_baton : file_baton, NULL, pool);
> +
> + if (err)
> + {
> + if (err->apr_err == SVN_ERR_FS_NOT_FOUND
> + || err->apr_err == SVN_ERR_RA_DAV_PATH_NOT_FOUND)
> + {
> + svn_error_clear(err);
> + return out_of_date(path, kind);
> + }
> + return err;
> + }
> +
> /* Make any additional client -> repository prop changes. */
> if (item->outgoing_prop_changes)
> {
> @@ -1312,9 +1357,19 @@ do_item_commit(void **dir_baton,
> if (! file_baton)
> {
> assert(parent_baton);
> - SVN_ERR(editor->open_file(path, parent_baton,
> - item->revision,
> - file_pool, &file_baton));
> + err = editor->open_file(path, parent_baton,
> + item->revision,
> + file_pool, &file_baton);
> + if (err)
> + {
> + if (err->apr_err == SVN_ERR_FS_NOT_FOUND
> + || err->apr_err == SVN_ERR_RA_DAV_PATH_NOT_FOUND)
> + {
> + svn_error_clear(err);
> + return out_of_date(path, svn_node_file);
> + }
> + return err;
> + }
Rather than repeating this code four times, please could you put it in the
helper function "out_of_date()"? I think it would fit very neatly there, with
the helper's purpose described as something like:
* If the error ERR is one that indicates an out-of-date
* condition, replace it with [or, at your option, 'append to it']
* a specific and consistent out-of-date error.
> }
>
> /* Add this file mod to the FILE_MODS hash. */
> Index: subversion/libsvn_client/commit.c
> ===================================================================
> --- subversion/libsvn_client/commit.c (revision 29632)
> +++ subversion/libsvn_client/commit.c (working copy)
> @@ -1689,8 +1689,18 @@ svn_client_commit4(svn_commit_info_t **commit_info_p,
> base_url, base_dir, base_dir_access,
> log_msg, commit_items, commit_info_p,
> TRUE, lock_tokens, keep_locks, pool)))
> - goto cleanup;
> -
> + {
> + /* For consistency, re-raise a "not found" error as "out of date". */
> + if (cmt_err->apr_err == SVN_ERR_FS_NOT_FOUND
> + || cmt_err->apr_err == SVN_ERR_RA_DAV_PATH_NOT_FOUND)
> + {
> + svn_error_clear(cmt_err);
> + cmt_err = svn_error_createf(SVN_ERR_WC_NOT_UP_TO_DATE, NULL,
> + _("Directory '%s' is out of date"),
> + base_dir);
I can see the point of giving an "out of date" error for consistency. I can't
see any value in hiding the lower-level errors that lead to this diagnosis. I
would expect you to add the "out of date" error to the chain of errors. I tried
doing so and your tests still pass. Not a big deal, but do you have an opinion
one way or the other?
The same question applies to the four cases in commit_util.c above.
> + }
> + goto cleanup;
> + }
> Index: subversion/libsvn_ra_neon/commit.c
> ===================================================================
> --- subversion/libsvn_ra_neon/commit.c (revision 29632)
> +++ subversion/libsvn_ra_neon/commit.c (working copy)
> @@ -751,15 +751,11 @@ static svn_error_t * commit_delete_entry(const char *p
> 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);
That should be "0" rather than "NULL" as it's a number rather than a pointer.
Otherwise, this is fine.
The tests (in commit_tests.py 28):
# Path Repo 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
By modifying the test suite to print the output, I see that each of the 8 cases
tested results in the line
svn: Commit failed (details follow):
followed by one "out of date" message, respectively:
svn: Directory '/A/C' is out of date
svn: Directory '' is out of date
svn: File or directory '/A/B/F' is out of date
svn: File '/A/D/H/omega' is out of date
svn: File '/A/B/E/alpha' is out of date
svn: File 'chi' is out of date
svn: File 'beta' is out of date
svn: File or directory '/A/D/H/psi' is out of date
So we see (i) ambiguity in the object's kind in the delete-and-delete cases,
(ii) lack of full relative path in some cases, and (iii) a complete absence of
any path identifier in one case. I believe (i) and (ii) are known
inconsistencies due to lack of information where they're detected, and I think
they're OK, but is (iii) something we can and should correct now? It's probably
a manifestation of (ii) in the case where the relative path being changed is ""
(properties on "this dir").
If this isn't something that you've made worse and isn't something you can
easily fix, that's fine, we can leave it for another time. (I should re-run the
test with your changes reverted to see what it was like before, but I don't
have time right now.)
Finally, before you commit please run a tab-to-space conversion as you inserted
a few tabs.
Thanks,
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-03-05 21:51:04 CET