Re: [PATCH] Unlocking a file and pre-unlock hook failure

vijay <vijay_at_collab.net> 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'?

> 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.

-- 
Philip