You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Malcolm Rowe <ma...@farside.org.uk> on 2007/02/08 10:17:23 UTC
Re: svn commit: r23370 - in trunk/subversion: include libsvn_client libsvn_fs_base libsvn_fs_fs libsvn_repos libsvn_subr libsvn_wc tests tests/cmdline tests/cmdline/svntest
On Thu, Feb 08, 2007 at 01:13:40AM -0800, lundblad@tigris.org wrote:
> Make 'svn cleanup' not fail on a missing .svn/tmp directory. It is
> removed and rebuilt anyway so failing if it does not exist is
> counterproductive.
>
> Patch by: Henner Zeller <h....@acm.org>
>
> * subversion/include/svn_io.h
> (svn_io_remove_dir2): New function. Add ignore_enoent flag.
> (svn_io_remove_dir): Deprecate. All callers updated to use
> svn_io_remove_dir2.
>
> * subversion/libsvn_subr/io.c
> (svn_io_remove_dir2): Copy from svn_io_remove_dir, adding ignore_enoent
> parameter.
> (svn_io_remove_dir): Wrap svn_io_remove_dir2.
>
> [...]
>
> Modified:
> trunk/subversion/include/svn_io.h
> trunk/subversion/libsvn_client/add.c
> trunk/subversion/libsvn_fs_base/fs.c
> trunk/subversion/libsvn_fs_fs/fs.c
> trunk/subversion/libsvn_fs_fs/fs_fs.c
> trunk/subversion/libsvn_repos/repos.c
> trunk/subversion/libsvn_subr/io.c
> trunk/subversion/libsvn_wc/adm_files.c
> trunk/subversion/libsvn_wc/adm_ops.c
> trunk/subversion/libsvn_wc/lock.c
> trunk/subversion/tests/cmdline/basic_tests.py
> trunk/subversion/tests/cmdline/svntest/actions.py
> trunk/subversion/tests/svn_test_main.c
The log message seems to be missing most of the changes to libsvn_*/.
While most of them are just 'update callers', I would have thought you'd
mention the change in libsvn_wc/adm_files.c where the key part of this
change was made.
More importantly, though, I'm not sure that the API change to
svn_io_remove_dir() is appropriate. Is there any reason that the single
caller who wishes to ignore ENOENT can't simply ignore that error?
In other words, can we avoid the addition of what (to me) appears to be
a very specific feature in the API and just do something like this
(untested, and we should probably also document svn_io_remove_dir()'s
return in its doc-comments).
[[[
* subversion/libsvn_wc/adm_files.c
(svn_wc__adm_cleanup_tmp_area): Don't raise an error if the directory
is already missing.
]]]
Index: subversion/libsvn_wc/adm_files.c
===================================================================
--- subversion/libsvn_wc/adm_files.c (revision 23341)
+++ subversion/libsvn_wc/adm_files.c (working copy)
@@ -1216,13 +1216,18 @@ svn_wc__adm_cleanup_tmp_area(svn_wc_adm_
apr_pool_t *pool)
{
const char *tmp_path;
+ svn_error_t *err;
SVN_ERR(svn_wc__adm_write_check(adm_access));
/* Get the path to the tmp area, and blow it away. */
tmp_path = extend_with_adm_name(svn_wc_adm_access_path(adm_access),
NULL, 0, pool, SVN_WC__ADM_TMP, NULL);
- SVN_ERR(svn_io_remove_dir(tmp_path, pool));
+ err = svn_io_remove_dir(tmp_path, pool);
+ if (err && APR_STATUS_IS_ENOENT(err->apr_err))
+ svn_error_clear(err);
+ else
+ SVN_ERR(err);
/* Now, rebuild the tmp area. */
SVN_ERR(init_adm_tmp_area(adm_access, pool));
Regards,
Malcolm
Re: svn commit: r23370 - in trunk/subversion: include libsvn_client libsvn_fs_base libsvn_fs_fs libsvn_repos libsvn_subr libsvn_wc tests tests/cmdline tests/cmdline/svntest
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Wed, Feb 14, 2007 at 02:15:12PM -0800, Daniel Rall wrote:
> > On Wed, Feb 14, 2007 at 09:55:22AM -0800, Daniel Rall wrote:
> > > > But if we think that the API is generally useful, let's keep it (I'm
> > > > not convinced myself, though :-)).
> > >
> > > I do find the new API useful along the lines of 'rm' vs. 'rm -f'.
> > > It's a rather common situation in programming that the target of a
> > > deletion may not exist; having a shorthand to handle this situation is
> > > certainly not required, but is a nice convenience. *shrug*
> > >
> >
> > I did notice that we decided not to do that for svn_io_remove_file()
> > though. But like you said, *shrug*.
>
> I think they should be consistent. I can either:
>
> a) Introduce a svn_io_remove_file2() along the lines of svn_io_remove_dir2().
>
> b) Revert the "remove_dir" version, and use your suggested approach
> for both.
>
> Thoughts?
>
I think we should definitely do one of the above, and I think you know
that my preference was for the latter :-) But I'd be interested to hear
what other people think.
> p.s. Feel free to add dev@ back to the CC list.
Done.
Regards,
Malcolm
Re: svn commit: r23370 - in trunk/subversion: include libsvn_client libsvn_fs_base libsvn_fs_fs libsvn_repos libsvn_subr libsvn_wc tests tests/cmdline tests/cmdline/svntest
Posted by Daniel Rall <dl...@collab.net>.
On Wed, 14 Feb 2007, Malcolm Rowe wrote:
> On Thu, Feb 08, 2007 at 09:12:38AM -0800, Daniel Rall wrote:
> > > > More importantly, though, I'm not sure that the API change to
> > > > svn_io_remove_dir() is appropriate. Is there any reason that the single
> > > > caller who wishes to ignore ENOENT can't simply ignore that error?
> > >
> > > The difference is that the new function let's make sure that the directory
> > > actually doesn't exist afterwards. In general, we could have an ENOENT
> > > further down in the tree and in that case, ignoring the error would leave
> > > a half-removed directory. I think this behaviour can be more generally useful
> > > in cleanup actions, hence the new API.
> >
> > I noted the same thing while reviewing this patch; the new API is more generally
> > useful, so worth the change.
>
> Unless I misunderstand what you're suggesting, we could have created
> a specific error to distinguish an ENOENT at the root of the deletion
> (which indicates a 'normal' condition, merely that the directory doesn't
> exist) from an ENOENT midway through the deletion (which indicates
> an unexpected problem), which is something we could have done without
> revving the interface.
You don't misunderstand me, and are spot on.
> But if we think that the API is generally useful, let's keep it (I'm
> not convinced myself, though :-)).
I do find the new API useful along the lines of 'rm' vs. 'rm -f'.
It's a rather common situation in programming that the target of a
deletion may not exist; having a shorthand to handle this situation is
certainly not required, but is a nice convenience. *shrug*
- Dan
Re: svn commit: r23370 - in trunk/subversion: include libsvn_client libsvn_fs_base libsvn_fs_fs libsvn_repos libsvn_subr libsvn_wc tests tests/cmdline tests/cmdline/svntest
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Thu, Feb 08, 2007 at 09:12:38AM -0800, Daniel Rall wrote:
> > > More importantly, though, I'm not sure that the API change to
> > > svn_io_remove_dir() is appropriate. Is there any reason that the single
> > > caller who wishes to ignore ENOENT can't simply ignore that error?
> >
> > The difference is that the new function let's make sure that the directory
> > actually doesn't exist afterwards. In general, we could have an ENOENT
> > further down in the tree and in that case, ignoring the error would leave
> > a half-removed directory. I think this behaviour can be more generally useful
> > in cleanup actions, hence the new API.
>
> I noted the same thing while reviewing this patch; the new API is more generally
> useful, so worth the change.
>
Unless I misunderstand what you're suggesting, we could have created
a specific error to distinguish an ENOENT at the root of the deletion
(which indicates a 'normal' condition, merely that the directory doesn't
exist) from an ENOENT midway through the deletion (which indicates
an unexpected problem), which is something we could have done without
revving the interface.
But if we think that the API is generally useful, let's keep it (I'm not
convinced myself, though :-)).
Regards,
Malcolm
Re: svn commit: r23370 - in trunk/subversion: include libsvn_client libsvn_fs_base libsvn_fs_fs libsvn_repos libsvn_subr libsvn_wc tests tests/cmdline tests/cmdline/svntest
Posted by Daniel Rall <dl...@collab.net>.
On Thu, 08 Feb 2007, Peter Lundblad wrote:
...
> > The log message seems to be missing most of the changes to libsvn_*/.
> > While most of them are just 'update callers', I would have thought you'd
> > mention the change in libsvn_wc/adm_files.c where the key part of this
> > change was made.
> >
> Of course. I accidentally removed two lines too much from the original
> log message;) Fixed.
I still have this change (and complete change log message) sitting in my WC
at the office, but I fell sick, and haven't been in to commit it since
discussion with eh completed.
Thanks for committing it, Peter.
> > More importantly, though, I'm not sure that the API change to
> > svn_io_remove_dir() is appropriate. Is there any reason that the single
> > caller who wishes to ignore ENOENT can't simply ignore that error?
>
> The difference is that the new function let's make sure that the directory
> actually doesn't exist afterwards. In general, we could have an ENOENT
> further down in the tree and in that case, ignoring the error would leave
> a half-removed directory. I think this behaviour can be more generally useful
> in cleanup actions, hence the new API.
I noted the same thing while reviewing this patch; the new API is more generally
useful, so worth the change.
- Dan
Re: svn commit: r23370 - in trunk/subversion: include libsvn_client libsvn_fs_base libsvn_fs_fs libsvn_repos libsvn_subr libsvn_wc tests tests/cmdline tests/cmdline/svntest
Posted by Peter Lundblad <pl...@google.com>.
Hi, Malcolm,
Malcolm Rowe writes:
> On Thu, Feb 08, 2007 at 01:13:40AM -0800, lundblad@tigris.org wrote:
> > Make 'svn cleanup' not fail on a missing .svn/tmp directory. It is
> > removed and rebuilt anyway so failing if it does not exist is
> > counterproductive.
> >
> > Patch by: Henner Zeller <h....@acm.org>
> >
> > * subversion/include/svn_io.h
> > (svn_io_remove_dir2): New function. Add ignore_enoent flag.
> > (svn_io_remove_dir): Deprecate. All callers updated to use
> > svn_io_remove_dir2.
> >
> > * subversion/libsvn_subr/io.c
> > (svn_io_remove_dir2): Copy from svn_io_remove_dir, adding ignore_enoent
> > parameter.
> > (svn_io_remove_dir): Wrap svn_io_remove_dir2.
> >
> > [...]
> >
> > Modified:
> > trunk/subversion/include/svn_io.h
> > trunk/subversion/libsvn_client/add.c
> > trunk/subversion/libsvn_fs_base/fs.c
> > trunk/subversion/libsvn_fs_fs/fs.c
> > trunk/subversion/libsvn_fs_fs/fs_fs.c
> > trunk/subversion/libsvn_repos/repos.c
> > trunk/subversion/libsvn_subr/io.c
> > trunk/subversion/libsvn_wc/adm_files.c
> > trunk/subversion/libsvn_wc/adm_ops.c
> > trunk/subversion/libsvn_wc/lock.c
> > trunk/subversion/tests/cmdline/basic_tests.py
> > trunk/subversion/tests/cmdline/svntest/actions.py
> > trunk/subversion/tests/svn_test_main.c
>
> The log message seems to be missing most of the changes to libsvn_*/.
> While most of them are just 'update callers', I would have thought you'd
> mention the change in libsvn_wc/adm_files.c where the key part of this
> change was made.
>
Of course. I accidentally removed two lines too much from the original
log message;) Fixed.
> More importantly, though, I'm not sure that the API change to
> svn_io_remove_dir() is appropriate. Is there any reason that the single
> caller who wishes to ignore ENOENT can't simply ignore that error?
>
The difference is that the new function let's make sure that the directory
actually doesn't exist afterwards. In general, we could have an ENOENT
further down in the tree and in that case, ignoring the error would leave
a half-removed directory. I think this behaviour can be more generally useful
in cleanup actions, hence the new API.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org