On Wed, 2005-11-16 at 19:36 +0000, Philip Martin wrote:
> Daniel Berlin <dberlin@dberlin.org> writes:
>
> > Index: subversion/libsvn_wc/lock.c
> > ===================================================================
> > --- subversion/libsvn_wc/lock.c (revision 17369)
> > +++ subversion/libsvn_wc/lock.c (working copy)
> > @@ -675,10 +675,89 @@ svn_wc_adm_retrieve (svn_wc_adm_access_t
> > generally makes the calling code simpler as it doesn't need to check
> > for NULL batons. */
> > if (! *adm_access)
> > - return svn_error_createf (SVN_ERR_WC_NOT_LOCKED, NULL,
> > - _("Working copy '%s' is missing or not locked"),
> > - svn_path_local_style (path, pool));
> > + {
> > + const char *wcpath;
> > + const svn_wc_entry_t *subdir_entry;
> > + svn_node_kind_t wckind;
> > + svn_node_kind_t kind;
> > + svn_error_t *err;
> > +
> > + err = svn_wc_entry (&subdir_entry, path, associated, TRUE, pool);
> > +
> > + /* If we can't get an entry here, we are in pretty bad shape,
> > + and will have to fall back to using just regular old paths to
> > + see what's going on. */
> > + if (err)
> > + {
> > + err = NULL;
>
> You need to svn_error_clear if you are going to ignore err.
Fixed, thanks.
>
> > + subdir_entry = NULL;
> > + }
> > +
> > + err = svn_io_check_path (path, &kind, pool);
> > +
> > + /* If we can't check the path, we can't make a good error
> > + message. */
> > + if (err)
> > + {
> > + svn_error_clear (err);
> > + return svn_error_createf (SVN_ERR_WC_NOT_LOCKED, NULL,
> > + _("Unable to check path existence for %s"),
> > + svn_path_local_style (path, pool));
> > + }
> > +
> > + if (subdir_entry)
> > + {
> > + if (subdir_entry->kind == svn_node_dir
> > + && kind == svn_node_file)
> > + {
> > + return svn_error_createf (SVN_ERR_WC_NOT_LOCKED, NULL,
> > + _("Expected %s to be a directory but found a file"),
> > + svn_path_local_style (path, pool));
> > + }
> > + else if (subdir_entry->kind == svn_node_file
> > + && kind == svn_node_dir)
> > + {
> > + return svn_error_createf (SVN_ERR_WC_NOT_LOCKED, NULL,
> > + _("Expected %s to be a file but found a directory"),
> > + svn_path_local_style (path, pool));
> > + }
> > + }
> > +
> > + wcpath = svn_wc__adm_path (path, FALSE, pool, NULL);
> > + err = svn_io_check_path (wcpath, &wckind, pool);
>
> That's the second new stat() call in a function that previously did no
> disk IO at all :-( You may have improved update but have other
> operations have been penalised?
This is a hard failure. If we hit this part of this function, we are
going to be issuing an error to the user and going away. :)
The only operation that survives this function returning an error is
status, and even then, only in certain cases (and only to print out a
weird character representing what is wrong :P)
>
> At the very least you should use the result of the first stat() to
> avoid doing the second stat() when it must fail. The access baton
> stuff already has support for "missing" directories, can you use that
> to avoid some of the stat() calls?
We effectively are.
The problem is
1. Those that don't use the information we are failing to retrieve above
don't give accurate information in the above case, precisely *because*
of the problem we are going to issue an error about.
2. The rest end up calling this function, directly or indirectly, and
we'd get into an infinite loop.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Nov 16 20:56:42 2005