kfogel@collab.net writes:
> The only comment I have (aside from "Yay, thanks for fixing this!") is
> that if code is going to depend on certain error codes being returned
> from svn_client_revert(), then svn_client_revert's doc string needs to
> explicitly say that that error will be returned in that circumstance.
Well, that error actually comes from svn_wc_entry (actually it
calls svn_wc_entries_read which calls read_entries which calls
resolve_to_defaults, and that is what returns the error). All
public functions should document which errors they return, but
none currently do. I'm not sure that anything is helped if i add
this one error message to this one function.
However, there is a larger problem. I have always been wary of
the error handling used in svn, but until now i haven't seen it
cause an actual problem so i have kept quiet. Any time an error
is encountered, the function simply aborts (via SVN_ERR) and
returns the svn_error_t without doing any cleanup. This implies
that the top caller must treat it as an error, rather than being
able to recover. But that is exactly what i want to do.
With my change to svn revert, here is what happens when you pass
it an unversioned file first and a versioned file second:
0 trunk% svn revert 1084.diff configure.in
svn: warning: svn_wc_is_wc_root: '1084.diff' is not a versioned resource
/u/epg/work/svn/trunk/subversion/libsvn_wc/lock.c:116:
(apr_err=155004)
svn: Attempted to lock an already-locked dir
svn: working copy locked: .
svn: run 'svn cleanup' to remove locks (type 'svn help cleanup' for details)
What happens is that svn_client_revert locks the wc (implicitly,
via svn_wc_adm_probe_open), then tries to get an entry with
svn_wc_entry and immediately bombs WITHOUT letting go its lock.
svn_cl__revert (with my patch) ignores the error and tries the
next file. But now the wc is locked.
I could change
SVN_ERR (svn_wc_entry (&entry, path, adm_access, FALSE, pool));
to
err = svn_wc_entry (&entry, path, adm_access, FALSE, pool);
if (err)
{
svn_wc_adm_close (adm_access);
return err;
}
but i don't like that. Thoughts?
I'll go ahead and add a thought of my own, though not necesarily
a suggestion. Where i used to work, part of our coding standards
document prescribed a cleanup section at the very end of the
function, just before the return. The single acceptable use of
goto is leaping to this section. Like this:
error_t *foo()
{
error_t *result;
if (some_action) {
result = error(foo);
goto out;
}
...
/* all operations succeeded */
result = NULL;
out:
shutdown(whatever);
return result;
}
--
Eric Gillespie <*> epg@pretzelnet.org
Build a fire for a man, and he'll be warm for a day. Set a man on
fire, and he'll be warm for the rest of his life. -Terry Pratchett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Apr 28 21:32:50 2003