You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Henner Zeller <h....@acm.org> on 2007/02/06 10:55:08 UTC
[PATCH] cleanup fails on missing .svn/tmp
Hi,
I tested what happens, if a .svn/tmp directory is missing:
----
$ rm -rf .svn/tmp
$ svn update
svn: Your .svn/tmp directory may be missing or corrupt; run 'svn
cleanup' and try again
svn: Can't open file '.svn/tmp/entries': No such file or directory
$ svn cleanup
svn: Can't open directory '.svn/tmp': No such file or directory
---
cleanup fails, because it cannot remove the tmp-directory it is about
to rebuild
(adm_files.c [svn_wc__adm_cleanup_tmp_area()]). So it didn't work as
advertised ;-)
Just ignoring ENOENT-errors in svn_wc__adm_cleanup_tmp_area() wouldn't
be correct, because we couldn't distinguish a ENOENT failures in the
toplevel directory to remove and any recursively removed
sub-files/dirs.
Patch attached.
-- proposed log message --
'svn cleanup' should not fail on a missing .svn/tmp directory. It is
removed and rebuild anyway so failing if it does not exist is
counterproductive.
* subversion/include/svn_io.h
- added svn_io_remove_dir2(), deprecated svn_io_remove_dir()
* subversion/libsvn_subr/io.c
(svn_io_remove_dir2): modified old svn_io_remove_dir with
ignore_enoent flag parameter
(svn_io_remove_dir): implemented using svn_io_remove_dir2
* subversion/tests/cmdline/basic_tests.py
(basic_cleanup): add regression: missing tmp-dir in cleanup
* subversion/tests/cmdline/svntest/actions.py
(remove_admin_tmp_dir): added
* subversion/libsvn_fs_base/fs.c,
subversion/libsvn_wc/adm_ops.c,
subversion/libsvn_wc/adm_files.c,
subversion/libsvn_wc/lock.c,
subversion/libsvn_client/add.c,
subversion/tests/svn_test_main.c,
subversion/libsvn_repos/repos.c,
subversion/libsvn_fs_fs/fs_fs.c,
subversion/libsvn_fs_fs/fs.c:
use new svn_io_remove_dir2() instead of deprecated svn_io_remove_dir()
---
cheers,
-henner
Re: [PATCH] cleanup fails on missing .svn/tmp
Posted by Peter Lundblad <pl...@google.com>.
Malcolm Rowe writes:
> As I understood it, 'svn cleanup' is supposed to fix a particular
> problem (completing aborted in-progress working-copy transactions, and
> in doing so, unlocking the wc); it's not supposed to be a general-purpose
> recovery tool.
>
I agree that cleanup is not a general recovery tool, even though the error
message that Henner showed suggests otherwise. I think that it is good
to recreate the tmp area on cleanup, though, since failed operations
can always leave around temp files that just stay there forever.
And while doing this, it seems silly to not just ignore the case where
.svn/tmp doesn't exist.
If other feel that we shouldn't, then we should at least make the
suggestion in the error message less confusing.
> > If you look at the code, this error occurs while removing the tmp/
> > directory unconditionally to create a fresh version of it. So this
> > code will in any case remove tmp/ whether there are files or not.
>
> I'm not that familiar with the code myself, but I'm surprised that we
> unconditionally remove the contents without checking them first - Erik's
> reply [1] in particular seems to indicate that we'd not want to just
> recreate the directory unconditionally (plus there's the 'something bad
> happened' indicator that I think would be a good idea to retain).
>
> [1] http://svn.haxx.se/dev/archive-2006-07/0283.shtml
>
I don't understand what essential files Erik is referring to.
At cleanup time, the log is first rerun. If that does *not*
result in an error, then the tmp area is removed and rebuilt.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] cleanup fails on missing .svn/tmp
Posted by Erik Huelsmann <eh...@gmail.com>.
On 2/7/07, Henner Zeller <h....@acm.org> wrote:
> Hi,
> > Well, but the fact that the tmp area is missing doesn't tell us
> > whether it was between a failed operation and a rerun of logs and
> > whether it was empty or not, or am I misunderstanding what the patch
> > is supposed to fix?
>
> This patch changes a piece of code that would dump .svn/tmp
> completely, unconditionally and without checking if there is something
> in these directories. It will only be called after logs have been run
> (successfully).
>
> The function I am changing is svn_wc__adm_cleanup_tmp_area() and it is
> only called in svn_wc_cleanup2() after everything is done. The comment
> before calling the function sais "...The logs have been run, so
> anything left here has no hope of being useful".
Ah. ok. thanks for the clarification.
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] cleanup fails on missing .svn/tmp
Posted by Henner Zeller <h....@acm.org>.
Hi,
> Well, but the fact that the tmp area is missing doesn't tell us
> whether it was between a failed operation and a rerun of logs and
> whether it was empty or not, or am I misunderstanding what the patch
> is supposed to fix?
This patch changes a piece of code that would dump .svn/tmp
completely, unconditionally and without checking if there is something
in these directories. It will only be called after logs have been run
(successfully).
The function I am changing is svn_wc__adm_cleanup_tmp_area() and it is
only called in svn_wc_cleanup2() after everything is done. The comment
before calling the function sais "...The logs have been run, so
anything left here has no hope of being useful".
cheers,
-henner
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] cleanup fails on missing .svn/tmp
Posted by Erik Huelsmann <eh...@gmail.com>.
On 2/6/07, Peter Lundblad <pl...@google.com> wrote:
> Erik Huelsmann writes:
> > On 2/6/07, Daniel Rall <dl...@collab.net> wrote:
> > > On Tue, 06 Feb 2007, Malcolm Rowe wrote:
> > >
> > > > On Tue, Feb 06, 2007 at 03:50:17PM +0100, Henner Zeller wrote:
> > > > > >I'm not that familiar with the code myself, but I'm surprised that we
> > > > > >unconditionally remove the contents without checking them first
> > > > >
> > > > > Well its good to do so because the tmp/ area might have some old files
> > > > > being lying around that we don't want to keep (hence the name tmp/).
> > > > > Instead of looking for these files, the whole tmp/ is just removed
> > > > > completely and rebuild.
> > > > >
> > > >
> > > > Okay, that does make sense. I see that we only recreate the tmp/ area
> > > > after we run the log for the wc, which will presumably fail if there are
> > > > files referenced in the log that don't exist in tmp/ (because tmp/
> > > > itself doesn't exist).
> >
> > Exactly. This is the problem I was referring to. The problem is that
> > successful execution of the log file doesn't guarantee we did the
> > right thing if the tmp area doesn't exist: we ignore ENOENT, so, we
> > can't assume successful completion was really successful, can we?
> >
> Then, that is a problem that would need fixing in general.
> In general, we assume that nothing changes in the admin area between a failing
> operation and the rerun of the logs.
> If that's not the case (say a file inside .svn/tmp disapears), all sorts of
> things can go wrong, but this has nothing to do with this patch.
Well, but the fact that the tmp area is missing doesn't tell us
whether it was between a failed operation and a rerun of logs and
whether it was empty or not, or am I misunderstanding what the patch
is supposed to fix?
> > If those files referenced in the logs don't exist *and* we execute the
> > log, is the end result guaranteed to be a consistent (known) state? Or
> > can we end up with a modified working copy admin area which is missing
> > the files in the working copy?
> >
> > > I'm in favor of this patch. It passes the test suite; I'll be
> > > committing it later today unless any other issues with it are raised.
> >
> > Do we know for certain this is a change for the better? Our working
> > copy isn't known for its stability as it is, so, I'm all in favor of
> > changing it, but I'd like it to be a sure change on the side of
> > stability...
> >
> I'd say this is a *slight* usability improvement which doesn't
> make the situation with the current log-based system worse
> than it already is. Improving the robustness of rerunning logs
> (say by making sure that the destination of the move
> is the expected one or something) would be another change
> that might add more complexity than is worth.
Right.
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] cleanup fails on missing .svn/tmp
Posted by Peter Lundblad <pl...@google.com>.
Erik Huelsmann writes:
> On 2/6/07, Daniel Rall <dl...@collab.net> wrote:
> > On Tue, 06 Feb 2007, Malcolm Rowe wrote:
> >
> > > On Tue, Feb 06, 2007 at 03:50:17PM +0100, Henner Zeller wrote:
> > > > >I'm not that familiar with the code myself, but I'm surprised that we
> > > > >unconditionally remove the contents without checking them first
> > > >
> > > > Well its good to do so because the tmp/ area might have some old files
> > > > being lying around that we don't want to keep (hence the name tmp/).
> > > > Instead of looking for these files, the whole tmp/ is just removed
> > > > completely and rebuild.
> > > >
> > >
> > > Okay, that does make sense. I see that we only recreate the tmp/ area
> > > after we run the log for the wc, which will presumably fail if there are
> > > files referenced in the log that don't exist in tmp/ (because tmp/
> > > itself doesn't exist).
>
> Exactly. This is the problem I was referring to. The problem is that
> successful execution of the log file doesn't guarantee we did the
> right thing if the tmp area doesn't exist: we ignore ENOENT, so, we
> can't assume successful completion was really successful, can we?
>
Then, that is a problem that would need fixing in general.
In general, we assume that nothing changes in the admin area between a failing
operation and the rerun of the logs.
If that's not the case (say a file inside .svn/tmp disapears), all sorts of
things can go wrong, but this has nothing to do with this patch.
> If those files referenced in the logs don't exist *and* we execute the
> log, is the end result guaranteed to be a consistent (known) state? Or
> can we end up with a modified working copy admin area which is missing
> the files in the working copy?
>
> > I'm in favor of this patch. It passes the test suite; I'll be
> > committing it later today unless any other issues with it are raised.
>
> Do we know for certain this is a change for the better? Our working
> copy isn't known for its stability as it is, so, I'm all in favor of
> changing it, but I'd like it to be a sure change on the side of
> stability...
>
I'd say this is a *slight* usability improvement which doesn't
make the situation with the current log-based system worse
than it already is. Improving the robustness of rerunning logs
(say by making sure that the destination of the move
is the expected one or something) would be another change
that might add more complexity than is worth.
thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] cleanup fails on missing .svn/tmp
Posted by Erik Huelsmann <eh...@gmail.com>.
On 2/6/07, Daniel Rall <dl...@collab.net> wrote:
> On Tue, 06 Feb 2007, Malcolm Rowe wrote:
>
> > On Tue, Feb 06, 2007 at 03:50:17PM +0100, Henner Zeller wrote:
> > > >I'm not that familiar with the code myself, but I'm surprised that we
> > > >unconditionally remove the contents without checking them first
> > >
> > > Well its good to do so because the tmp/ area might have some old files
> > > being lying around that we don't want to keep (hence the name tmp/).
> > > Instead of looking for these files, the whole tmp/ is just removed
> > > completely and rebuild.
> > >
> >
> > Okay, that does make sense. I see that we only recreate the tmp/ area
> > after we run the log for the wc, which will presumably fail if there are
> > files referenced in the log that don't exist in tmp/ (because tmp/
> > itself doesn't exist).
Exactly. This is the problem I was referring to. The problem is that
successful execution of the log file doesn't guarantee we did the
right thing if the tmp area doesn't exist: we ignore ENOENT, so, we
can't assume successful completion was really successful, can we?
If those files referenced in the logs don't exist *and* we execute the
log, is the end result guaranteed to be a consistent (known) state? Or
can we end up with a modified working copy admin area which is missing
the files in the working copy?
> I'm in favor of this patch. It passes the test suite; I'll be
> committing it later today unless any other issues with it are raised.
Do we know for certain this is a change for the better? Our working
copy isn't known for its stability as it is, so, I'm all in favor of
changing it, but I'd like it to be a sure change on the side of
stability...
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] cleanup fails on missing .svn/tmp
Posted by Daniel Rall <dl...@collab.net>.
On Tue, 06 Feb 2007, Malcolm Rowe wrote:
> On Tue, Feb 06, 2007 at 03:50:17PM +0100, Henner Zeller wrote:
> > >I'm not that familiar with the code myself, but I'm surprised that we
> > >unconditionally remove the contents without checking them first
> >
> > Well its good to do so because the tmp/ area might have some old files
> > being lying around that we don't want to keep (hence the name tmp/).
> > Instead of looking for these files, the whole tmp/ is just removed
> > completely and rebuild.
> >
>
> Okay, that does make sense. I see that we only recreate the tmp/ area
> after we run the log for the wc, which will presumably fail if there are
> files referenced in the log that don't exist in tmp/ (because tmp/
> itself doesn't exist).
I'm in favor of this patch. It passes the test suite; I'll be
committing it later today unless any other issues with it are raised.
- dan
Re: [PATCH] cleanup fails on missing .svn/tmp
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Tue, Feb 06, 2007 at 03:50:17PM +0100, Henner Zeller wrote:
> >I'm not that familiar with the code myself, but I'm surprised that we
> >unconditionally remove the contents without checking them first
>
> Well its good to do so because the tmp/ area might have some old files
> being lying around that we don't want to keep (hence the name tmp/).
> Instead of looking for these files, the whole tmp/ is just removed
> completely and rebuild.
>
Okay, that does make sense. I see that we only recreate the tmp/ area
after we run the log for the wc, which will presumably fail if there are
files referenced in the log that don't exist in tmp/ (because tmp/
itself doesn't exist).
I'm now confused as to what Erik was talking about.
Regards,
Malcolm
Re: [PATCH] cleanup fails on missing .svn/tmp
Posted by Henner Zeller <h....@acm.org>.
Hi,
> I'm not that familiar with the code myself, but I'm surprised that we
> unconditionally remove the contents without checking them first
Well its good to do so because the tmp/ area might have some old files
being lying around that we don't want to keep (hence the name tmp/).
Instead of looking for these files, the whole tmp/ is just removed
completely and rebuild.
The two lines in the code doing this are:
----- subversion/libsvn_wc/adm_files.c:1225
/* ... create tmp_path here [...] */
SVN_ERR(svn_io_remove_dir(tmp_path, pool));
/* Now, rebuild the tmp area. */
SVN_ERR(init_adm_tmp_area(adm_access, pool));
------
Yes, remove unconditionally the whole tmp/ directory and rebuild it.
cheers,
-henner
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] cleanup fails on missing .svn/tmp
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Tue, Feb 06, 2007 at 01:44:44PM +0100, Henner Zeller wrote:
> Hi,
> >The last time this was brought up, the consensus seemed to be that this
> >wasn't something we wanted to attempt to clear up.
> >
> >(one reason was that you have no way to know whether the tmp/ directory
> >actually was empty before it was removed)
> >
> >See e.g. http://svn.haxx.se/dev/archive-2006-07/0217.shtml and
> >surrounding thread.
>
> Well, this thread ends in a half religous discussion about error
> checking and if tools should be user friendly, but doesn't really
> address the issue.
>
Well, some of the thread is useful. The basic concept, I thought, was
that if the tmp/ directory has been removed, the administrative area
is essentially corrupt, and so rather than try to deal with the edge
cases we might encounter during repair, we essentially punt and say
'Something broke here'.
As I understood it, 'svn cleanup' is supposed to fix a particular
problem (completing aborted in-progress working-copy transactions, and
in doing so, unlocking the wc); it's not supposed to be a general-purpose
recovery tool.
> If you look at the code, this error occurs while removing the tmp/
> directory unconditionally to create a fresh version of it. So this
> code will in any case remove tmp/ whether there are files or not.
I'm not that familiar with the code myself, but I'm surprised that we
unconditionally remove the contents without checking them first - Erik's
reply [1] in particular seems to indicate that we'd not want to just
recreate the directory unconditionally (plus there's the 'something bad
happened' indicator that I think would be a good idea to retain).
[1] http://svn.haxx.se/dev/archive-2006-07/0283.shtml
Regards,
Malcolm
Re: [PATCH] cleanup fails on missing .svn/tmp
Posted by Henner Zeller <h....@acm.org>.
Hi,
> The last time this was brought up, the consensus seemed to be that this
> wasn't something we wanted to attempt to clear up.
>
> (one reason was that you have no way to know whether the tmp/ directory
> actually was empty before it was removed)
>
> See e.g. http://svn.haxx.se/dev/archive-2006-07/0217.shtml and
> surrounding thread.
Well, this thread ends in a half religous discussion about error
checking and if tools should be user friendly, but doesn't really
address the issue.
If you look at the code, this error occurs while removing the tmp/
directory unconditionally to create a fresh version of it. So this
code will in any case remove tmp/ whether there are files or not.
However, this code fails in the first step (removing the tmp/
unconditionally) if the directory doesn't exist (which is exactly the
expected result of this removing). So this shouldn't fail here.
Ignoring the 'error' of this directory not beeing here is the Right
Thing to do.
cheers,
-henner
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] cleanup fails on missing .svn/tmp
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Tue, Feb 06, 2007 at 11:55:08AM +0100, Henner Zeller wrote:
> Hi,
>
> I tested what happens, if a .svn/tmp directory is missing:
> ----
> $ rm -rf .svn/tmp
> $ svn update
> svn: Your .svn/tmp directory may be missing or corrupt; run 'svn
> cleanup' and try again
> svn: Can't open file '.svn/tmp/entries': No such file or directory
> $ svn cleanup
> svn: Can't open directory '.svn/tmp': No such file or directory
> ---
> cleanup fails, because it cannot remove the tmp-directory it is about
> to rebuild
> (adm_files.c [svn_wc__adm_cleanup_tmp_area()]). So it didn't work as
> advertised ;-)
>
The last time this was brought up, the consensus seemed to be that this
wasn't something we wanted to attempt to clear up.
(one reason was that you have no way to know whether the tmp/ directory
actually was empty before it was removed)
See e.g. http://svn.haxx.se/dev/archive-2006-07/0217.shtml and
surrounding thread.
Regards,
Malcolm