vijay wrote on Thu, Jun 02, 2011 at 20:18:22 +0530:
> 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", "")
> +
Could you pass a non-"" here and then test that it appears in the error message?
> + # 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
The comment is wrong in mentioning ra_dav.
> + # 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__':
