Re: svn commit: rev 7144 - trunk/subversion/libsvn_wc

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.

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.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org