You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ko...@apache.org on 2017/11/20 12:43:33 UTC
svn commit: r1815799 - in /subversion/trunk/subversion:
libsvn_ra_serf/commit.c tests/libsvn_ra/ra-test.c
Author: kotkov
Date: Mon Nov 20 12:43:33 2017
New Revision: 1815799
URL: http://svn.apache.org/viewvc?rev=1815799&view=rev
Log:
ra_serf: Properly process lock tokens for empty relative paths ("")
within the commit editor.
Previously, the condition in maybe_set_lock_token_header() would silently
skip adding the corresponding headers for the supplied lock tokens with
empty relpaths. That, in turn, would result in a failure when committing
changes for a locked file with an RA session opened for a file URL,
even with appropriate lock tokens passed to the commit editor.
(Apparently, "svn commit" is not affected by this issue, as it uses the
file's parent URL when opening the RA session; but nevertheless, the
issue prevents a legit usage of the public API.)
* subversion/libsvn_ra_serf/commit.c
(maybe_set_lock_token_header): Disallow NULL relpaths, but allow
the empty ones.
* subversion/tests/libsvn_ra/ra-test.c
(commit_locked_empty_relpath_test): New test.
(test_funcs): Add new test.
Modified:
subversion/trunk/subversion/libsvn_ra_serf/commit.c
subversion/trunk/subversion/tests/libsvn_ra/ra-test.c
Modified: subversion/trunk/subversion/libsvn_ra_serf/commit.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/commit.c?rev=1815799&r1=1815798&r2=1815799&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/commit.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/commit.c Mon Nov 20 12:43:33 2017
@@ -687,7 +687,7 @@ maybe_set_lock_token_header(serf_bucket_
{
const char *token;
- if (! (*relpath && commit_ctx->lock_tokens))
+ if (! (relpath && commit_ctx->lock_tokens))
return SVN_NO_ERROR;
if (! svn_hash_gets(commit_ctx->deleted_entries, relpath))
Modified: subversion/trunk/subversion/tests/libsvn_ra/ra-test.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_ra/ra-test.c?rev=1815799&r1=1815798&r2=1815799&view=diff
==============================================================================
--- subversion/trunk/subversion/tests/libsvn_ra/ra-test.c (original)
+++ subversion/trunk/subversion/tests/libsvn_ra/ra-test.c Mon Nov 20 12:43:33 2017
@@ -1682,6 +1682,119 @@ commit_empty_last_change(const svn_test_
return SVN_NO_ERROR;
}
+static svn_error_t *
+commit_locked_empty_relpath_test(const svn_test_opts_t *opts,
+ apr_pool_t *pool)
+{
+ const char *url;
+ svn_ra_callbacks2_t *cbtable;
+ svn_ra_session_t *session;
+ const svn_delta_editor_t *editor;
+ void *edit_baton;
+ void *root_baton;
+ void *file_baton;
+ const char *file_url;
+ struct lock_result_t *lock_result;
+ apr_hash_t *lock_tokens;
+ svn_txdelta_window_handler_t handler;
+ void *handler_baton;
+ svn_revnum_t fetched_rev;
+ apr_hash_t *fetched_props;
+ const svn_string_t *propval;
+
+ SVN_ERR(svn_test__create_repos2(NULL, &url, NULL,
+ "test-repo-commit-locked-empty-relpath-test",
+ opts, pool, pool));
+
+ SVN_ERR(svn_ra_initialize(pool));
+ SVN_ERR(svn_ra_create_callbacks(&cbtable, pool));
+ SVN_ERR(svn_test__init_auth_baton(&cbtable->auth_baton, pool));
+
+ SVN_ERR(svn_ra_open4(&session, NULL, url, NULL, cbtable,
+ NULL, NULL, pool));
+ SVN_ERR(svn_ra_get_commit_editor3(session, &editor, &edit_baton,
+ apr_hash_make(pool),
+ NULL, NULL, NULL, TRUE, pool));
+ /* Add a file. */
+ SVN_ERR(editor->open_root(edit_baton, SVN_INVALID_REVNUM,
+ pool, &root_baton));
+ SVN_ERR(editor->add_file("file", root_baton, NULL, SVN_INVALID_REVNUM,
+ pool, &file_baton));
+ SVN_ERR(editor->close_file(file_baton, NULL, pool));
+ SVN_ERR(editor->close_directory(root_baton, pool));
+ SVN_ERR(editor->close_edit(edit_baton, pool));
+
+ /* Acquire a lock on this file. */
+ {
+ struct lock_baton_t baton = {0};
+ svn_revnum_t rev = 1;
+ apr_hash_t *lock_targets;
+
+ baton.results = apr_hash_make(pool);
+ baton.pool = pool;
+
+ lock_targets = apr_hash_make(pool);
+ svn_hash_sets(lock_targets, "file", &rev);
+ SVN_ERR(svn_ra_lock(session, lock_targets, "comment", FALSE,
+ lock_cb, &baton, pool));
+
+ SVN_ERR(expect_lock("file", baton.results, session, pool));
+ lock_result = svn_hash_gets(baton.results, "file");
+ }
+
+ /* Open a new session using the file's URL (since now, we would be
+ * using "" as the file's relpath). */
+ file_url = svn_path_url_add_component2(url, "file", pool);
+ SVN_ERR(svn_ra_open4(&session, NULL, file_url, NULL, cbtable,
+ NULL, NULL, pool));
+
+ /* Create a new commit editor supplying our lock token. */
+ lock_tokens = apr_hash_make(pool);
+ svn_hash_sets(lock_tokens, "", lock_result->lock->token);
+ SVN_ERR(svn_ra_get_commit_editor3(session, &editor, &edit_baton,
+ apr_hash_make(pool), NULL, NULL,
+ lock_tokens, TRUE, pool));
+ /* Edit the locked file. */
+ SVN_ERR(editor->open_root(edit_baton, SVN_INVALID_REVNUM,
+ pool, &root_baton));
+ SVN_ERR(editor->open_file("", root_baton, SVN_INVALID_REVNUM, pool,
+ &file_baton));
+ SVN_ERR(editor->apply_textdelta(file_baton, NULL, pool, &handler,
+ &handler_baton));
+ SVN_ERR(svn_txdelta_send_string(svn_string_create("A", pool),
+ handler, handler_baton, pool));
+ SVN_ERR(editor->close_file(file_baton, NULL, pool));
+ SVN_ERR(editor->close_directory(root_baton, pool));
+ SVN_ERR(editor->close_edit(edit_baton, pool));
+
+ /* Check the result. */
+ SVN_ERR(svn_ra_get_file(session, "", SVN_INVALID_REVNUM, NULL,
+ &fetched_rev, NULL, pool));
+ SVN_TEST_INT_ASSERT((int) fetched_rev, 2);
+
+ /* Change property of the locked file. */
+ SVN_ERR(editor->open_root(edit_baton, SVN_INVALID_REVNUM,
+ pool, &root_baton));
+ SVN_ERR(editor->open_file("", root_baton, SVN_INVALID_REVNUM, pool,
+ &file_baton));
+ SVN_ERR(editor->change_file_prop(file_baton, "propname",
+ svn_string_create("propval", pool),
+ pool));
+ SVN_ERR(editor->close_file(file_baton, NULL, pool));
+ SVN_ERR(editor->close_directory(root_baton, pool));
+ SVN_ERR(editor->close_edit(edit_baton, pool));
+
+ /* Check the result. */
+ SVN_ERR(svn_ra_get_file(session, "", SVN_INVALID_REVNUM, NULL,
+ &fetched_rev, &fetched_props, pool));
+ SVN_TEST_INT_ASSERT((int) fetched_rev, 3);
+ propval = svn_hash_gets(fetched_props, "propname");
+ SVN_TEST_ASSERT(propval);
+ SVN_TEST_STRING_ASSERT(propval->data, "propval");
+
+ return SVN_NO_ERROR;
+}
+
/* The test table. */
@@ -1716,6 +1829,8 @@ static struct svn_test_descriptor_t test
"verify checkout over a tunnel"),
SVN_TEST_OPTS_PASS(commit_empty_last_change,
"check how last change applies to empty commit"),
+ SVN_TEST_OPTS_PASS(commit_locked_empty_relpath_test,
+ "check commit editor with relpath='' and lock"),
SVN_TEST_NULL
};
Re: svn commit: r1815799 - in /subversion/trunk/subversion:
libsvn_ra_serf/commit.c tests/libsvn_ra/ra-test.c
Posted by Julian Foad <ju...@apache.org>.
Evgeny Kotkov wrote:
> Julian Foad <ju...@apache.org> writes:
>> Don't replace it with 'relpath &&' instead, however. If relpath is null then
>> I think the next line (svn_hash_gets(..., relpath)) would immediately crash
>> anyway, so allowing it here is useless and therefore confusing. Remove that
>> condition entirely. That's my suggestion.
>
> The condition is inverted in the sense that it checks for a null relpath
> and returns if so — in other words, the svn_hash_gets() won't get called
> if the relpath is null.
Oh... oops! I was confused. Right you are.
> However, as it turns out, this condition can indeed be simplified by not
> checking for null, as the only calling site where the relpath might be
> null, setup_proppatch_headers(), already checks for it. (Among the other
> two calling sites, the relpath cannot be null as it would have segfaulted
> earlier).
>
> I committed this simplification in r1816061, thanks!
Thanks.
- Julian
Re: svn commit: r1815799 - in /subversion/trunk/subversion:
libsvn_ra_serf/commit.c tests/libsvn_ra/ra-test.c
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Evgeny Kotkov <ev...@visualsvn.com> writes:
> (2) I am going to tweak the new test so that it would properly open the
> parent directory and commit to a locked file, to have this case
> covered with a native test.
Committed in r1816060.
Julian Foad <ju...@apache.org> writes:
>> (1) Keep the new simpler check in maybe_set_lock_token_header() — as,
>> unless I missing something, there should be no reason to explicitly
>> filter empty relpaths for the lock tokens since they are invalid.
>
> I agree it is good to remove the '*relpath &&' condition if there is no
> reason why it needs to be there.
>
> Don't replace it with 'relpath &&' instead, however. If relpath is null then
> I think the next line (svn_hash_gets(..., relpath)) would immediately crash
> anyway, so allowing it here is useless and therefore confusing. Remove that
> condition entirely. That's my suggestion.
The condition is inverted in the sense that it checks for a null relpath
and returns if so — in other words, the svn_hash_gets() won't get called
if the relpath is null.
However, as it turns out, this condition can indeed be simplified by not
checking for null, as the only calling site where the relpath might be
null, setup_proppatch_headers(), already checks for it. (Among the other
two calling sites, the relpath cannot be null as it would have segfaulted
earlier).
I committed this simplification in r1816061, thanks!
Regards,
Evgeny Kotkov
Re: svn commit: r1815799 - in /subversion/trunk/subversion:
libsvn_ra_serf/commit.c tests/libsvn_ra/ra-test.c
Posted by Julian Foad <ju...@apache.org>.
Evgeny Kotkov wrote:
> I think that I have missed that open_root() must open a directory and so
> it cannot be used when working with a file URL. This, in turn, means
> that the test example is indeed an undocumented/invalid usage of the
> delta editor API and that it works by a coincidence.
>
> I can propose doing the following:
>
> (1) Keep the new simpler check in maybe_set_lock_token_header() — as,
> unless I missing something, there should be no reason to explicitly
> filter empty relpaths for the lock tokens since they are invalid.
I agree it is good to remove the '*relpath &&' condition if there is no
reason why it needs to be there.
Don't replace it with 'relpath &&' instead, however. If relpath is null
then I think the next line (svn_hash_gets(..., relpath)) would
immediately crash anyway, so allowing it here is useless and therefore
confusing. Remove that condition entirely. That's my suggestion.
- Julian
> (2) I am going to tweak the new test so that it would properly open the
> parent directory and commit to a locked file, to have this case
> covered with a native test.
Re: svn commit: r1815799 - in /subversion/trunk/subversion:
libsvn_ra_serf/commit.c tests/libsvn_ra/ra-test.c
Posted by Evgeny Kotkov <ev...@visualsvn.com>.
Bert Huijben <be...@qqmail.nl> writes:
> I'm not sure if we should really allow this.
>
> The delta editor explicitly describes that you are opening a directory and
> then edit the nodes inside. Only changing properties on the root is allowed
> and other operations are all on nodes within. Allowing to open the node
> itself again may cause all kinds of problems as there are now multiple
> handles pointing to the same thing. How will this be expressed in the
> filesystem/transaction?
>
> I'm surprised that all the other filesystems allow this, so perhaps this
> is a safe change... but the documentation in svn_delta.h doesn't describe
> this as a safe extension. (Which would theoretically allow this as a safe
> extension in later versions... but we must make sure that we are not
> opening new issues this way)
>
> Currently I would guess that making the ra layers provide a proper error
> for this case would not be a bad thing... All our drivers explicitly open
> an existing directory when they want to edit a file...
I think that I have missed that open_root() must open a directory and so
it cannot be used when working with a file URL. This, in turn, means
that the test example is indeed an undocumented/invalid usage of the
delta editor API and that it works by a coincidence.
I can propose doing the following:
(1) Keep the new simpler check in maybe_set_lock_token_header() — as,
unless I missing something, there should be no reason to explicitly
filter empty relpaths for the lock tokens since they are invalid.
(2) I am going to tweak the new test so that it would properly open the
parent directory and commit to a locked file, to have this case
covered with a native test.
Thanks,
Evgeny Kotkov
RE: svn commit: r1815799 - in /subversion/trunk/subversion: libsvn_ra_serf/commit.c tests/libsvn_ra/ra-test.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: kotkov@apache.org [mailto:kotkov@apache.org]
> Sent: maandag 20 november 2017 13:44
> To: commits@subversion.apache.org
> Subject: svn commit: r1815799 - in /subversion/trunk/subversion:
> libsvn_ra_serf/commit.c tests/libsvn_ra/ra-test.c
>
> Author: kotkov
> Date: Mon Nov 20 12:43:33 2017
> New Revision: 1815799
>
> URL: http://svn.apache.org/viewvc?rev=1815799&view=rev
> Log:
> ra_serf: Properly process lock tokens for empty relative paths ("")
> within the commit editor.
I'm not sure if we should really allow this.
The delta editor explicitly describes that you are opening a directory and then edit the nodes inside. Only changing properties on the root is allowed and other operations are all on nodes within. Allowing to open the node itself again may cause all kinds of problems as there are now multiple handles pointing to the same thing. How will this be expressed in the filesystem/transaction?
I'm surprised that all the other filesystems allow this, so perhaps this is a safe change... but the documentation in svn_delta.h doesn't describe this as a safe extension. (Which would theoretically allow this as a safe extension in later versions... but we must make sure that we are not opening new issues this way)
Currently I would guess that making the ra layers provide a proper error for this case would not be a bad thing... All our drivers explicitly open an existing directory when they want to edit a file...
Bert