Greg Stein <gstein@lyra.org> writes:
> On Mon, Sep 22, 2003 at 04:24:34PM -0500, philip@tigris.org wrote:
>>...
>> +++ trunk/subversion/libsvn_wc/lock.c Mon Sep 22 16:24:33 2003
>> @@ -126,9 +126,14 @@
>> static svn_error_t *
>> remove_lock (const char *path, apr_pool_t *pool)
>> {
>> - if (svn_wc__adm_path_exists (path, FALSE, pool, NULL))
>> - SVN_ERR (svn_wc__remove_adm_file (path, pool, SVN_WC__ADM_LOCK, NULL));
>> -
>> + svn_error_t *err = svn_wc__remove_adm_file (path, pool, SVN_WC__ADM_LOCK,
>> + NULL);
>> + if (err)
>> + {
>> + if (svn_wc__adm_path_exists (path, FALSE, pool, NULL))
>> + return err;
>> + svn_error_clear (err);
>> + }
>
> This certainly helps in removing a syscall or so, but I'll note that
> svn_wc__remove_adm_file() still does a chmod() before trying to remove the
> file. It might be nice to just rm the thing, and then chmod/rm if the
> original rm fails.
I think svn_wc__remove_adm_file is the right function to use here, the
lock is an adm file and it's being removed. A separate issue already
exists to deal with how svn_wc__remove_adm_file should handle the
chmod.
> Second, do we need to the "exists" test? Couldn't we just check the error
> return value? If it says "doesn't exist", then we clear the error and
> we're happy. Otherwise, we just propagate the error from remove_adm_file.
>
> IOW, the error from remove_adm_file has one of two cases:
>
> 1) the file doesn't exist. that's the error.
> 2) the file does exist, we've got some other error
>
> In case 1, we ignore the error (and clear it). In case 2, we propagate. I
> don't see a need to check whether the file exists, since we "know" it
> does. Even if there is something *really* funny going on (like a mount
> timeout on the rm), then I still think we propagate that error regardless
> of whether the file exists.
Your case 1) above is really two cases
1a) the adm directory exists but the lock file does not
1b) neither the adm directory nor the lock file exist
2) the lock file exists but some OS reason prevented removal
The path existance check (note: it checks the adm dir, not the lock
file) distinguishes cases 1a and 1b. I suppose it might be possible
to do that by examining the APR errors, but the APR documentation
didn't make clear what I could, or should, check.
An earlier version of the locking code recorded (or tried to record)
the deletion of versioned directories, so that the absence of the lock
file would be known. It didn't work as well as the current code.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Sep 23 17:13:30 2003