On Thursday 02 June 2011 05:05 PM, Philip Martin wrote:
vijay<[email protected]> writes:
At the end of the testcase, I thought of checking the status of the
working copy with expected status as "writelocked=K". But I couldn't
do it as the test raises exception in between, i.e., during unlock
operation itself.
I don't understand this. With your patch the test is a PASS, it's also
a PASS with the other RA layers. Why can't you test for 'K'?
Sorry for the confusion here. I have updated the test to check wc status
at the end.
ra_serf:
The test passes with ra_serf as all kind of errors has been handled here.
Please review the patch and respond.
Thanks& Regards,
Vijayaguru
[1] https://issues.apache.org/bugzilla/show_bug.cgi?id=51297
P.S: I will be very happy if someone from Apache's mod_dav development
team can take a look at the patch in [1]. Please let me know if we can
handle it in some other way. Eagerly awaiting for your response.
Index: subversion/libsvn_ra_neon/lock.c
===================================================================
--- subversion/libsvn_ra_neon/lock.c (revision 1130003)
+++ subversion/libsvn_ra_neon/lock.c (working copy)
@@ -460,7 +460,7 @@
_("No lock on path '%s'"
" (%d Bad Request)"), path, code);
default:
- break; /* Handle as error */
+ SVN_ERR(err);
}
}
else
That also fixes an error leak.
However, I think that bit of code has too many return paths which is why
the bug happened. I'd make the code simpler by moving the err
declaration, getting rid of the "else SVN_ERR" and changing the
SVN_NO_ERROR return to "return svn_error_return(err)". Then it's
obvious that the error is always returned.
Done.
Attached the updated patch and log message.
Thanks & Regards,
Vijayaguru
Index: subversion/libsvn_ra_neon/lock.c
===================================================================
--- subversion/libsvn_ra_neon/lock.c (revision 1130445)
+++ subversion/libsvn_ra_neon/lock.c (working copy)
@@ -399,6 +399,7 @@
const char *url;
const char *url_path;
ne_uri uri;
+ svn_error_t *err = SVN_NO_ERROR;
apr_hash_t *extra_headers = apr_hash_make(pool);
@@ -441,7 +442,6 @@
{
int code = 0;
- svn_error_t *err;
err = svn_ra_neon__simple_request(&code, ras, "UNLOCK", url_path,
extra_headers, NULL, 204, 0, pool);
@@ -460,13 +460,11 @@
_("No lock on path '%s'"
" (%d Bad Request)"), path, code);
default:
- break; /* Handle as error */
+ break;
}
}
- else
- SVN_ERR(err);
}
- return SVN_NO_ERROR;
+ return svn_error_return(err);
}
Index: subversion/tests/cmdline/lock_tests.py
===================================================================
--- subversion/tests/cmdline/lock_tests.py (revision 1130445)
+++ subversion/tests/cmdline/lock_tests.py (working copy)
@@ -1693,6 +1693,32 @@
None, expected_status)
+#----------------------------------------------------------------------
+def block_unlock_if_pre_unlock_hook_fails(sbox):
+ "block unlock operation if pre-unlock hook fails"
+
+ sbox.build()
+ wc_dir = sbox.wc_dir
+ repo_dir = sbox.repo_dir
+
+ svntest.actions.create_failing_hook(repo_dir, "pre-unlock", "")
+
+ # lock a file.
+ pi_path = os.path.join(wc_dir, 'A', 'D', 'G', 'pi')
+ expected_status = svntest.actions.get_virginal_state(wc_dir, 1)
+ expected_status.tweak('A/D/G/pi', writelocked='K')
+
+ svntest.actions.run_and_verify_svn(None, ".*locked by user", [], 'lock',
+ '-m', '', pi_path)
+
+ svntest.actions.run_and_verify_status(wc_dir, expected_status)
+
+ # Now, unlock the file via ra_dav. Make sure the unlock operation fails as
+ # pre-unlock hook blocks it.
+ svntest.actions.run_and_verify_svn2(None, None, "", 1, 'unlock', pi_path)
+ svntest.actions.run_and_verify_status(wc_dir, expected_status)
+
+
########################################################################
# Run the tests
@@ -1739,6 +1765,7 @@
replace_and_propset_locked_path,
cp_isnt_ro,
update_locked_deleted,
+ block_unlock_if_pre_unlock_hook_fails,
]
if __name__ == '__main__':
With ra_neon, unlocking a file succeeds in client side, though pre-unlock
hook exits with non-zero return code. Make sure the error is returned
to the user in case of any failures.
* subversion/libsvn_ra_neon/lock.c
(do_unlock): Change SVN_NO_ERROR return to "svn_error_return".
* subversion/tests/cmdline/lock_tests.py
(block_unlock_if_pre_unlock_hook_fails): New test.
(test_list): Add block_unlock_if_pre_unlock_hook_fails to the list.
Patch by: Vijayaguru G <vijay{_AT_}collab.net>
