[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: #774: [PATCH] avoid early exit when acting on some non-versioned files

From: Eric Gillespie <epg_at_pretzelnet.org>
Date: 2003-04-28 21:32:02 CEST

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

This is an archived mail posted to the Subversion Dev mailing list.