Eric Gillespie <epg@pretzelnet.org> writes:
> 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.
Yes, the entire call stack down has to document the error being
dependend on...
Our practice has been, if some caller tests for a specific error, then
that particular error should be documented in the callee, and all the
way down as necessary.
(Most errors are never tested for explicitly, they're just handled at
the top level and the error string is thrown at the user.)
> 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?
Er, yup, we need a general non-fatal internal feedback system, and we
don't have one. :-(
> 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;
> }
Sure, we do that in a lot of functions already. I think it would be
fine to do that here.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Apr 28 22:41:12 2003