You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2003/05/05 23:05:03 UTC
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
bdenny@tigris.org writes:
> Author: bdenny
> Date: Mon May 5 16:40:25 2003
> New Revision: 5814
>
> Modified:
> trunk/subversion/libsvn_wc/adm_ops.c
> trunk/subversion/libsvn_wc/adm_ops.h
> trunk/subversion/libsvn_wc/log.c
> trunk/subversion/libsvn_wc/update_editor.c
> trunk/subversion/tests/clients/cmdline/update_tests.py
> Log:
> Fix issue #1075 - 'update receiving delete for missing directory'.
> Thanks to Mike Pilato and Ben Collins-Sussman for their suggestions.
>
> * libsvn_wc/log.c
> (log_do_delete_entry): Don't try to recurse on a missing directory;
> just delete the entry in the parent directory.
>
> * libsvn_wc/adm_ops.c
> (recursively_tweak_entries): Accept a notification function and baton.
> If remove_missing_dirs is set, remove the entry in the parent directory
> for any missing directory, and notify the client of the deletion.
> (svn_wc__do_update_cleanup): Accept a notification function and baton,
> and a flag 'remove_missing_dirs'. Pass these to
> recursively_tweak_entries.
> (svn_wc_add): Pass NULL to svn_wc__do_update_cleanup for the
> notification function and baton. Pass FALSE for remove_missing_dirs.
> (svn_wc_remove_from_revision_control): Don't try to recurse on a
> missing directory; just delete the entry in the parent directory.
>
> * libsvn_wc/adm_ops.h
> (svn_wc__do_update_cleanup): Accept a notification function and baton,
> and a flag 'remove_missing_dirs'. Comment accordingly.
>
> * libsvn_wc/update_editor.c
> (close_edit): Pass our notification function and baton to
> svn_wc__do_update_cleanup. Pass TRUE for remove_missing_dirs.
There is a problem here. There are two changes to be made to the
parent directory of the missing directory: the revision number is
changed and the missing directory's entry is removed. Making the
changes separately and holding state in memory means that the update
process is vulnerable, if it gets interrupted before
svn_wc__do_update_cleanup runs then the revision number will be
changed but the entry for the missing directory will still be present.
Further attempts to update are likely to fail as the working copy now
has an entry for an item that does not exist.
I still think the correct approach is to make the repository send the
delete for the missing subdirectory. Update reports the missing
directory to the repository, see the reporter->delete_path calls in
libsvn_wc/adm_crawler.c:report_revisions. The repository delta
generator should make better use of this information.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by cm...@collab.net.
Brian Denny <br...@briandenny.net> writes:
> On Mon, May 05, 2003 at 09:11:25PM -0500, cmpilato@collab.net wrote:
> >
> > And I still disagree with you. The only problem with this patch is
> > the timing of this cleanup. The removal of missing items needs to
> > happen at the same time as the bumping of the directory version
> > number. When I originally came up with this solution, my idea was
> > that this cleanup code would occur in two places:
> >
> > (1) the update_editor's close_dir() (which might require a new loggy
> > operation)
> > (2) the global post-update revision bump (do_update_cleanup).
>
>
> in recursively_tweak_entries, we call svn_wc__tweak_entry with the new
> revnum. is that what you're referring to in (2) above?
>
> the update_editor's close_directory function calls maybe_bump_dir_info,
> which looks like it also (potentially) changes the revnum? is that why
> the removal of missing items needs to happen in the case of (1)?
>
> why do we need to have these two separate places in the code that bump
> the revnum?
>
> (for that matter, i don't really understand the difference between
> close_directory and close_edit -- i mean, i understand that close_edit
> is only called once at the end, where close_directory might be called on
> any directory being visited along the way... but why do some operations
> have to happen at the one time, and others at the other?)
The answer to several of these questions is the same -- state
validity. We do our darnedest in Subversion to make it as resilient to
nasty situations as possible, where "nasty situations" includes such
things as network and power outages that might happen in the middle of
an operation. Obviously, it's not likely that we'll do a perfect job
with this, but it doesn't mean we won't try.
To be specific, there are several places where we bump the revision of
a directory because those are places where it is safe to do so, and
because *not* doing so would result in a working copy whose
administrative data is bogus. For example, after closing an updated
directory, we want to bump its revision because if the update were to
fail immediately thereafter, we'd like to be able to just run 'svn up'
again and continue the process of getting our working copy updated to
HEAD. If we hadn't bumped the revision number, we would have a
directory that claims to be at some revision when in fact all of its
contents are proof that, no, we most certainly don't have the revision
of the directory we claim to have.
This is also the reason we have the svn_wc__log_* functions. It's
essentially our implementation of a journaling filesystem, where one
or more actions are scheduled to be -- well, acted out -- and then
later we actually act those actions. :-) The idea here is that if
something goes wrong in the middle of playing out the tasks in the log
file, when you run 'svn cleanup' Subversion will try those loggy
actions again in an attempt to get your working copy into a sane
state.
--
Wow. I feel like I just wrote a section that should be included in
Chapter 7 of the book (the Development chapter).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Brian Denny <br...@briandenny.net>.
On Mon, May 05, 2003 at 09:11:25PM -0500, cmpilato@collab.net wrote:
>
> And I still disagree with you. The only problem with this patch is
> the timing of this cleanup. The removal of missing items needs to
> happen at the same time as the bumping of the directory version
> number. When I originally came up with this solution, my idea was
> that this cleanup code would occur in two places:
>
> (1) the update_editor's close_dir() (which might require a new loggy
> operation)
> (2) the global post-update revision bump (do_update_cleanup).
in recursively_tweak_entries, we call svn_wc__tweak_entry with the new
revnum. is that what you're referring to in (2) above?
the update_editor's close_directory function calls maybe_bump_dir_info,
which looks like it also (potentially) changes the revnum? is that why
the removal of missing items needs to happen in the case of (1)?
why do we need to have these two separate places in the code that bump
the revnum?
(for that matter, i don't really understand the difference between
close_directory and close_edit -- i mean, i understand that close_edit
is only called once at the end, where close_directory might be called on
any directory being visited along the way... but why do some operations
have to happen at the one time, and others at the other?)
-brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by cm...@collab.net.
Brian Denny <br...@briandenny.net> writes:
> On Wed, May 07, 2003 at 04:46:19PM -0500, cmpilato@collab.net wrote:
> > > I probably won't have a chance to work on svn seriously until Saturday.
> > > In the meantime, should I reopen #1075 and/or file a new issue for this?
> >
> > Re-open, please. And if you determine that we've regressed on that
> > other bug that you suspected your change caused, please make a value
> > judgment about whether or not we should recommend to our release
> > manager (Michael Price) that your change be reverted from the release
> > branch. I see no reason in any case to revert from the trunk at this
> > point.
> >
>
> AFAICT rev. 5814 didn't make it into the release branch in the first
> place. The branch was created in r5724 and only selected revisons have
> been merged into it since.
Oh. Well there you have it then. Cool. :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Brian Denny <br...@briandenny.net>.
On Wed, May 07, 2003 at 04:46:19PM -0500, cmpilato@collab.net wrote:
> > I probably won't have a chance to work on svn seriously until Saturday.
> > In the meantime, should I reopen #1075 and/or file a new issue for this?
>
> Re-open, please. And if you determine that we've regressed on that
> other bug that you suspected your change caused, please make a value
> judgment about whether or not we should recommend to our release
> manager (Michael Price) that your change be reverted from the release
> branch. I see no reason in any case to revert from the trunk at this
> point.
>
AFAICT rev. 5814 didn't make it into the release branch in the first
place. The branch was created in r5724 and only selected revisons have
been merged into it since.
-brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by cm...@collab.net.
Brian Denny <br...@briandenny.net> writes:
> On Tue, May 06, 2003 at 02:23:08PM -0500, Ben Collins-Sussman wrote:
> > >
> > > so, does that mean that our treatment of 'deleted' items is 'vulnerable'
> > > in the same way that Philip describes? their entries, too, get removed
> > > in svn_wc__do_update_cleanup and its callees. an interruption before
> > > do_update_cleanup could leave the parent dir with a 'deleted' entry
> > > which isn't really deleted with respect to the (bumped) revnum.
> >
> > Ooh, yeah, you're right. Our current treatment of 'deleted' items is
> > just as vulnerable. Not a good thing.
> >
>
> I probably won't have a chance to work on svn seriously until Saturday.
> In the meantime, should I reopen #1075 and/or file a new issue for this?
Re-open, please. And if you determine that we've regressed on that
other bug that you suspected your change caused, please make a value
judgment about whether or not we should recommend to our release
manager (Michael Price) that your change be reverted from the release
branch. I see no reason in any case to revert from the trunk at this
point.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Brian Denny <br...@briandenny.net>.
On Tue, May 06, 2003 at 02:23:08PM -0500, Ben Collins-Sussman wrote:
> >
> > so, does that mean that our treatment of 'deleted' items is 'vulnerable'
> > in the same way that Philip describes? their entries, too, get removed
> > in svn_wc__do_update_cleanup and its callees. an interruption before
> > do_update_cleanup could leave the parent dir with a 'deleted' entry
> > which isn't really deleted with respect to the (bumped) revnum.
>
> Ooh, yeah, you're right. Our current treatment of 'deleted' items is
> just as vulnerable. Not a good thing.
>
I probably won't have a chance to work on svn seriously until Saturday.
In the meantime, should I reopen #1075 and/or file a new issue for this?
-brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Ben Collins-Sussman <su...@collab.net>.
Brian Denny <br...@briandenny.net> writes:
> On Tue, May 06, 2003 at 09:54:44AM -0500, Ben Collins-Sussman wrote:
> >
> > Remember that we're already using this post-update technique for
> > 'deleted' items. After the client finishes applying the tree delta to
> > the working copy, it looks for 'deleted' items -- if any exist, the
> > logic is "huh... well the server didn't re-add them, I guess they're
> > really supposed to be gone for good, then."
> >
> > All we're doing now is extending that same idea to 'missing items':
> > "if the server didn't re-add them, then they're really supposed to be
> > gone for good." It seems like a perfectly consistent strategy to me.
>
> so, does that mean that our treatment of 'deleted' items is 'vulnerable'
> in the same way that Philip describes? their entries, too, get removed
> in svn_wc__do_update_cleanup and its callees. an interruption before
> do_update_cleanup could leave the parent dir with a 'deleted' entry
> which isn't really deleted with respect to the (bumped) revnum.
Ooh, yeah, you're right. Our current treatment of 'deleted' items is
just as vulnerable. Not a good thing.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Brian Denny <br...@briandenny.net>.
On Tue, May 06, 2003 at 09:54:44AM -0500, Ben Collins-Sussman wrote:
>
> Remember that we're already using this post-update technique for
> 'deleted' items. After the client finishes applying the tree delta to
> the working copy, it looks for 'deleted' items -- if any exist, the
> logic is "huh... well the server didn't re-add them, I guess they're
> really supposed to be gone for good, then."
>
> All we're doing now is extending that same idea to 'missing items':
> "if the server didn't re-add them, then they're really supposed to be
> gone for good." It seems like a perfectly consistent strategy to me.
so, does that mean that our treatment of 'deleted' items is 'vulnerable'
in the same way that Philip describes? their entries, too, get removed
in svn_wc__do_update_cleanup and its callees. an interruption before
do_update_cleanup could leave the parent dir with a 'deleted' entry
which isn't really deleted with respect to the (bumped) revnum.
or is there some other place where 'deleted' entries get removed, too?
-brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: .svn Directory Question
Posted by cm...@collab.net.
Edmund Horner <ed...@chrysophylax.cjb.net> writes:
> Hopefully, by "hidden" you mean has the hidden attribute set. AFAIK,
> in Windows this is only used for displaying the item in directory
> lists. There should be few applications (and Subversion should not be
> one of them) that use it for anything else.
>
> On Unix .svn is "hidden" because it begins with a dot. This is
> standard practice for things that users normally don't want cluttering
> up their listings. It doesn't affect the file in any other way.
>
> Perhaps for consistency (in appearance, if not in practice) Subversion
> should set the hidden attribute in Windows.
Yeah, I've since been convinced that this would be a good thing. I
originally opposed it (long ago) because the use of hidden file and
directories is so much less common on Windows than on Unix. Whereas
every Unix user learns on Day One that adding the '-a' to 'ls' will
show them everything, there aren't generally that many Windows users
who know that you can use 'DIR /A:H', or that you can set the Explorer
options to show hidden files.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: .svn Directory Question
Posted by Mark Watts <mw...@stny.rr.com>.
> -----Original Message-----
> From: Edmund Horner [mailto:edmund@chrysophylax.cjb.net]
> Sent: Tuesday, May 06, 2003 11:37
> Cc: dev@subversion.tigris.org
>
> > I have some questions about the .svn subdirectory on windows. On
> > Linux it is a hidden directory while on windows platforms it is not.
> >
> > 1. Was this done intentionally because of some requirement of
> > subversion on windows 2. Will it do any harm if I flag those
> > subdirectories as hidden. (NOTE: I have tried it already on a test
> > repository and working directory and it does not seem to
> harm anything
> > but I thought I should ask.)
> >
> > The reason I want to do this is because these directories
> are picked
> > up in all sub-directory searches, touches, etc and they
> shouldn't be.
> > This is Subversion private data that I should not be
> messing with yet
> > a simple directory for a particular file mask that includes sub
> > directories will find data in .SVN directory
>
> Hopefully, by "hidden" you mean has the hidden attribute set.
> AFAIK, in Windows this is only used for displaying the item
> in directory lists.
> There should be few applications (and Subversion should not be one of
> them) that use it for anything else.
Yes, that is what I mean. Setting hidden attribute flag for the directory.
> On Unix .svn is "hidden" because it begins with a dot. This
> is standard practice for things that users normally don't
> want cluttering up their listings. It doesn't affect the
> file in any other way.
Likewise in windows, it does not affect the directory or file in any other
way. The file or directory with the hidden attribute set will not appear in
Explorer or in DOS/CMD directory listings unless options are set to display
hidden files/directories. The file or directory is still accessable.
> Perhaps for consistency (in appearance, if not in practice)
> Subversion should set the hidden attribute in Windows.
I tend to agree.
> I would be very surprised if doing this manually would cause
> any problems. For your search problems, it sounds like you
> just need tools which can understand an option to ignore
> hidden items. Windows XP search has such an option, for starters.
All standars search tools in windows already ignore files and directories
with the hidden attribute set. What I am unsure of is if I set this
attribute will it negativly affect subversion in some way.
Thanks
-Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: .svn Directory Question
Posted by Edmund Horner <ed...@chrysophylax.cjb.net>.
> I have some questions about the .svn subdirectory on windows. On Linux it
> is a hidden directory while on windows platforms it is not.
>
> 1. Was this done intentionally because of some requirement of subversion on
> windows
> 2. Will it do any harm if I flag those subdirectories as hidden. (NOTE: I
> have tried it already on a test repository and working directory and it does
> not seem to harm anything but I thought I should ask.)
>
> The reason I want to do this is because these directories are picked up in
> all sub-directory searches, touches, etc and they shouldn't be. This is
> Subversion private data that I should not be messing with yet a simple
> directory for a particular file mask that includes sub directories will find
> data in .SVN directory
Hopefully, by "hidden" you mean has the hidden attribute set. AFAIK, in
Windows this is only used for displaying the item in directory lists.
There should be few applications (and Subversion should not be one of
them) that use it for anything else.
On Unix .svn is "hidden" because it begins with a dot. This is standard
practice for things that users normally don't want cluttering up their
listings. It doesn't affect the file in any other way.
Perhaps for consistency (in appearance, if not in practice) Subversion
should set the hidden attribute in Windows.
I would be very surprised if doing this manually would cause any
problems. For your search problems, it sounds like you just need tools
which can understand an option to ignore hidden items. Windows XP
search has such an option, for starters.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: .svn Directory Question
Posted by Jack Repenning <jr...@collab.net>.
> In the later one kfogel says they "concluded that 'hidden' was not
> appropriate for .svn". I haven't read the earlier thread so I don't
> know why.
Well, Karl's summary may not have been entirely thorough. As I read the
original discussion, the idea trickled away because Windows hidden
directories don't hide things quite as well as some people wanted them
to (didn't accomplish the requirements of the original suggester).
But it's all very odd, since I don't think Unix dot files accomplish
those goals, either. Windows hidden-file semantics are definitely
different from Unix dot-file semantics. It does no good to say simply
"it's 'hidden' on Unix, it should be 'hidden' on Windows." But heck,
man: Windows anything-you-care-to-name semantics are "definitely
different" from Unix closest-analogue semantics, that's no argument,
either.
What we should do is decide if there's some value to Windows hiding, and
if so prioritize the change based on that value. Windows hiding does
two things:
* Naïve users casually browsing around using Windows Explorer or "dir"
(those who don't set the Folder Option) don't see them.
* Naïve users searching with Windows Explorer (those who don't check the
search option) don't see them.
Those seem like good things to me, but they don't seem particularly
urgent or crucial things.
-===-
Jack Repenning
CollabNet, Inc.
o: 650.228.2562
c: 408.835.8090
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: .svn Directory Question
Posted by Edmund Horner <ed...@chrysophylax.cjb.net>.
> I have some questions about the .svn subdirectory on windows. On Linux it
> is a hidden directory while on windows platforms it is not.
>
> 1. Was this done intentionally because of some requirement of subversion on
> windows
> 2. Will it do any harm if I flag those subdirectories as hidden. (NOTE: I
> have tried it already on a test repository and working directory and it does
> not seem to harm anything but I thought I should ask.)
>
> The reason I want to do this is because these directories are picked up in
> all sub-directory searches, touches, etc and they shouldn't be. This is
> Subversion private data that I should not be messing with yet a simple
> directory for a particular file mask that includes sub directories will find
> data in .SVN directory
As for the idea of getting Subversion to set the hidden attribute, there
are some mailing list discussions on this:
2003-01-25 "hide .svn on win"
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=30009
2002-09-06 "Why not hide .SVN dir on windows?"
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=21394&list=dev
In the later one kfogel says they "concluded that 'hidden' was not
appropriate for .svn". I haven't read the earlier thread so I don't
know why.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
.svn Directory Question
Posted by Mark Watts <mw...@stny.rr.com>.
I have some questions about the .svn subdirectory on windows. On Linux it
is a hidden directory while on windows platforms it is not.
1. Was this done intentionally because of some requirement of subversion on
windows
2. Will it do any harm if I flag those subdirectories as hidden. (NOTE: I
have tried it already on a test repository and working directory and it does
not seem to harm anything but I thought I should ask.)
The reason I want to do this is because these directories are picked up in
all sub-directory searches, touches, etc and they shouldn't be. This is
Subversion private data that I should not be messing with yet a simple
directory for a particular file mask that includes sub directories will find
data in .SVN directory
Thanks
-Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Ben Collins-Sussman <su...@collab.net>.
cmpilato@collab.net writes:
> But it's also (obviously) not enough to solve the problem. In the
> recipe, the working copy is missing something that has also been
> deleted in the repository since the last time the client updated. Of
> course, the client doesn't know this. It just knows that it's missing
> something. So, when updating, the client builds this transaction that
> looks like the working copy, calling reporter->delete_path() on the
> missing item. Now there is a transaction with absolutely no reference
> to this missing thing. That is diffed against a pristine revision
> which, again, has absolutely no reference to that missing path,
> because in that revision, that path no longer exists. Obviously, if
> you diff two trees that are both missing the same thing, there is no
> diff output regarding that thing.
Remember that we're already using this post-update technique for
'deleted' items. After the client finishes applying the tree delta to
the working copy, it looks for 'deleted' items -- if any exist, the
logic is "huh... well the server didn't re-add them, I guess they're
really supposed to be gone for good, then."
All we're doing now is extending that same idea to 'missing items':
"if the server didn't re-add them, then they're really supposed to be
gone for good." It seems like a perfectly consistent strategy to me.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Philip Martin <ph...@codematters.co.uk>.
cmpilato@collab.net writes:
> Yes, we might have various places in our client where we need to be
> aware of this stuff, but so what?
It seems a little odd that we may have to put code in several
places--update, diff, and status--to handle this rather than modify
the repository code.
I note that 'svn diff' doesn't work at all with missing directories,
and I don't think fixing it is that important. As long as 'svn st wc'
works so that the user can see the problem, and as long as it can be
fixed using 'svn up [-rN] wc', then the system can be used.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by cm...@collab.net.
Philip Martin <ph...@codematters.co.uk> writes:
> cmpilato@collab.net writes:
>
> > Philip Martin <ph...@codematters.co.uk> writes:
> >
> > > I agree it could be solved there, but solving it on the client side
> > > means that every client has to be aware of the problem and has to find
> > > a solution.
> >
> > Well, yes, but that's why we have a librarized implementation. These
> > changes all live in libsvn_wc -- I dunno anybody crazy enough to write
> > their own libsvn_wc library.
>
> We have more than one implementation. I think switch shares the
> update implementation, but consider
>
> 'svn diff -rX'
>
> where there is a missing item in the working copy that has been
> deleted in revision X. Do we need to modify the diff client to handle
> this case? (Do we even know what it should do? I think the diff
> command should indicate the deletion, but it would not surprise me if
> other people thought differently.)
>
> Then there is 'svn status -u', do we need to modify that client as
> well?
Ok ok ok. Let's normalize our terminology. You're using "client" to
mean "any old chunk of code that deals with the update response" where
I mean "a program that communicates interestingly with a Subversion
repository". I like my terminology better, so pbbbbbt. :-P
Yes, we might have various places in our client where we need to be
aware of this stuff, but so what?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by mark benedetto king <mb...@boredom.org>.
On Tue, May 06, 2003 at 04:43:38PM +0100, Philip Martin wrote:
> We have more than one implementation. I think switch shares the
> update implementation, but consider
>
> 'svn diff -rX'
>
> where there is a missing item in the working copy that has been
> deleted in revision X. Do we need to modify the diff client to handle
> this case? (Do we even know what it should do? I think the diff
> command should indicate the deletion, but it would not surprise me if
> other people thought differently.)
>
I frequently use "svn diff" as an extra-verbose version of "svn status".
It would be strange for "svn diff -r BASE" to behave differently
from "svn diff", and strange for "svn diff -r X" to behave differently
from "svn diff -r BASE".
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Philip Martin <ph...@codematters.co.uk>.
cmpilato@collab.net writes:
> Philip Martin <ph...@codematters.co.uk> writes:
>
> > I agree it could be solved there, but solving it on the client side
> > means that every client has to be aware of the problem and has to find
> > a solution.
>
> Well, yes, but that's why we have a librarized implementation. These
> changes all live in libsvn_wc -- I dunno anybody crazy enough to write
> their own libsvn_wc library.
We have more than one implementation. I think switch shares the
update implementation, but consider
'svn diff -rX'
where there is a missing item in the working copy that has been
deleted in revision X. Do we need to modify the diff client to handle
this case? (Do we even know what it should do? I think the diff
command should indicate the deletion, but it would not surprise me if
other people thought differently.)
Then there is 'svn status -u', do we need to modify that client as
well?
Eeek! I've just come across a bug while trying that status command
svnadmin create repo
svn mkdir file://`pwd`/repo/foo
svn rm file://`pwd`/repo/foo
svn co -r1 file://`pwd`/repo wc
svn st -u wc
../svn/subversion/libsvn_wc/lock.c:488: (apr_err=155005)
svn: Working copy not locked
svn: directory not locked ()
Oddly enough 'svn st -uv wc' works.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by cm...@collab.net.
Philip Martin <ph...@codematters.co.uk> writes:
> I didn't look at Brian's repos code in detail, so I can't comment on
> that, my question is about svn_ra_reporter_t.delete_path. This is the
> function that the client calls to describe a working copy path as
> missing. Obviously the client must know about the path to be able to
> call delete_path, it strikes me as odd that in this case the client
> doesn't get any response from the repository. In the normal case (no
> repository delete) the repository responds to the delete_path call,
> why should the delete case be different? It's possible I don't
> understand the intention behind delete_path.
Keep in mind that the reporter interface exists for no other reason
than to build a filesystem transaction that mimics the state of the
working copy. We build this exact duplicate of the working copy base
as a filesystem transaction so that, when diffed against a pristine
revision tree (the revision to which we are updating), that diff
represents how we need to change our working to make it look like that
revision.
Reporter->delete_path() is meant only to only to inform that
repository that the client is missing something that appears to exist
as a child of its parent in the revision of the parent that the client
currently has. Entries marked 'deleted' fit this criteria (because
this state represents the very definition of what it means to be
'deleted'), and missing directories fit the criteria. We've always
been calling delete_path for missing directories -- that won't change.
But it's also (obviously) not enough to solve the problem. In the
recipe, the working copy is missing something that has also been
deleted in the repository since the last time the client updated. Of
course, the client doesn't know this. It just knows that it's missing
something. So, when updating, the client builds this transaction that
looks like the working copy, calling reporter->delete_path() on the
missing item. Now there is a transaction with absolutely no reference
to this missing thing. That is diffed against a pristine revision
which, again, has absolutely no reference to that missing path,
because in that revision, that path no longer exists. Obviously, if
you diff two trees that are both missing the same thing, there is no
diff output regarding that thing.
In the normal case you cite, the repository responds to the
reporter->delete_path() call in the same way as usual, by removing the
path from the temporary transaction before doing the tree delta. The
difference is that typically the path still exists in the pristine
update revision, so there actually *is* delta output, specifically
output that says "the revision has this path, you don't -- so you need
to add this path".
> > I dunno how that it would all turn out. I still think the fix should
> > be client side.
>
> I agree it could be solved there, but solving it on the client side
> means that every client has to be aware of the problem and has to find
> a solution.
Well, yes, but that's why we have a librarized implementation. These
changes all live in libsvn_wc -- I dunno anybody crazy enough to write
their own libsvn_wc library.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Philip Martin <ph...@codematters.co.uk>.
cmpilato@collab.net writes:
> Philip Martin <ph...@codematters.co.uk> writes:
>
> > I still think the correct approach is to make the repository send the
> > delete for the missing subdirectory.
[...]
> And I still disagree with you.
[...]
> Now, part of my disgust with your insistance on making the repository
> do this work is that I assumed (as did Brian Denny) that this change
> would require modifications to svn_repos_dir_delta(). Those
> modifications (at least the theory behind what Brian started doing
> there) were completely I'm-gonna-minus-one-all-over-your-head wrong
> in nature.
I didn't look at Brian's repos code in detail, so I can't comment on
that, my question is about svn_ra_reporter_t.delete_path. This is the
function that the client calls to describe a working copy path as
missing. Obviously the client must know about the path to be able to
call delete_path, it strikes me as odd that in this case the client
doesn't get any response from the repository. In the normal case (no
repository delete) the repository responds to the delete_path call,
why should the delete case be different? It's possible I don't
understand the intention behind delete_path.
> However, it occurs to me that perhaps the repos layer could provider a
> second editor which is composed with the dir-delta editor, and whose
> close_dir() function would perform the necessary checks and transmit
> the extra editor->delete_entry() calls.
Interesting idea.
> I dunno how that it would all turn out. I still think the fix should
> be client side.
I agree it could be solved there, but solving it on the client side
means that every client has to be aware of the problem and has to find
a solution.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Brian Denny <br...@briandenny.net>.
Okay, I hope to get some time to work on this again this week -- in case
anyone wants to pick up the ol' server-side/client-side argument. ;)
I'll probably continue in the client-side fashion unless some other
consensus is achieved on-list.
-brian
On Thu, May 15, 2003 at 10:23:21AM -0700, Brian Denny wrote:
>
> Hi all,
>
> As much as I want to solve this problem and solve it right, I am going
> to be incredibly swamped with The Rest Of My Life for the next... at
> least a week, maybe two. So as far as 0.23 goes, if somebody else wants
> to take action on this issue, go ahead.
>
> I'm also, as I said, fine with leaving things just as they are for now.
> IMO the current state of Issue #1075 is at least no worse than it was
> before. If others disagree, r5814 can be reverted.
>
> If no one else does it sooner, I *will* get this cleaned up just as soon
> as I can find the time.
>
> -brian
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Brian Denny <br...@briandenny.net>.
Hi all,
As much as I want to solve this problem and solve it right, I am going
to be incredibly swamped with The Rest Of My Life for the next... at
least a week, maybe two. So as far as 0.23 goes, if somebody else wants
to take action on this issue, go ahead.
I'm also, as I said, fine with leaving things just as they are for now.
IMO the current state of Issue #1075 is at least no worse than it was
before. If others disagree, r5814 can be reverted.
If no one else does it sooner, I *will* get this cleaned up just as soon
as I can find the time.
-brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Brian Denny <br...@briandenny.net>.
On Tue, May 13, 2003 at 10:21:54PM +0100, Philip Martin wrote:
>
> It's a problem already.
>
> svnadmin create repo
> svn co file://`pwd`/repo wc
> svn mkdir wc/bar
> rm -rf wc/bar
> svn st wc
> ! wc/bar
> svn mkdir file://`pwd`/repo/foo
> svn up wc
> A wc/foo
> D wc/bar
> Updated to revision 1.
>
> I don't think the wc/bar entry should be removed.
I agree. Pre-r5814 behavior is a "working copy not locked" error, which
is presumably not ideal either. I presume that the best thing would be
to leave the entry untouched?
...
As far as the fragility of this solution, basically we have to think
of all the possible cases under which it might be wrong to delete a
missing directory after a successful update. If we forget one, bad
news. But I don't understand how that's different from, e.g., having
to determine the set of conditions under which the crawler needs to
call reporter->delete_path...
-brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Philip Martin <ph...@codematters.co.uk>.
Brian Denny <br...@briandenny.net> writes:
> On Tue, May 13, 2003 at 07:40:40PM +0100, Philip Martin wrote:
> >
> > I am wary of this removal of missing directories, does it mean that
> > the sequence
> >
> > svn mkdir foo
> > mv foo bar
> > svn up
> >
> > will remove the foo entry? If so, it would fall into the "I don't
> > expect that to happen" category.
>
> oh, crap. you are right, of course.
It's a problem already.
svnadmin create repo
svn co file://`pwd`/repo wc
svn mkdir wc/bar
rm -rf wc/bar
svn st wc
! wc/bar
svn mkdir file://`pwd`/repo/foo
svn up wc
A wc/foo
D wc/bar
Updated to revision 1.
I don't think the wc/bar entry should be removed.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Philip Martin <ph...@codematters.co.uk>.
Brian Denny <br...@briandenny.net> writes:
> can you think of any other problematic cases?
Off the top of my head, no I can't. That you ask the question
illustrates why I think the concept of "cleaning-up" entries for
missing items is dubious. I'd be much happier if it was done in
response to a positive delete signal, rather than in response to
absence of any signal.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Brian Denny <br...@briandenny.net>.
On Tue, May 13, 2003 at 07:40:40PM +0100, Philip Martin wrote:
>
> I am wary of this removal of missing directories, does it mean that
> the sequence
>
> svn mkdir foo
> mv foo bar
> svn up
>
> will remove the foo entry? If so, it would fall into the "I don't
> expect that to happen" category.
oh, crap. you are right, of course.
if this problem is limited to schedule-add items, then it's easy to
fix. can you think of any other problematic cases?
-brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Philip Martin <ph...@codematters.co.uk>.
Branko Äibej <br...@xbc.nu> writes:
> >>>I am wary of this removal of missing directories, does it mean that
> >>>the sequence
> >>>
> >>> svn mkdir foo
> >>> mv foo bar
> >>> svn up
> >>>
> >>>will remove the foo entry? If so, it would fall into the "I don't
> >>>expect that to happen" category.
> >>>
> >>I think there should be an "except..." clause there. :-)
> >>"Except if the entry is marked as moved". Of course, we don't do that
> >>yet -- but we should, to support atomic renames on the client side.
> >
> >That's not an 'svn mv', just a plain mv, so that foo is missing.
>
> Oh duh, right. Silly me. Sorry.
>
> Your example is still good; that entry shouldn't be removed just because
> the directory is missing. Instead, the "svn up" should pull a new copy
> of foo.
:) It can't do that either... it's not been committed.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Branko Čibej <br...@xbc.nu>.
Philip Martin wrote:
>Branko ÄŒibej <br...@xbc.nu> writes:
>
>
>
>>Philip Martin wrote:
>>
>>
>>
>>>I am wary of this removal of missing directories, does it mean that
>>>the sequence
>>>
>>> svn mkdir foo
>>> mv foo bar
>>> svn up
>>>
>>>will remove the foo entry? If so, it would fall into the "I don't
>>>expect that to happen" category.
>>>
>>>
>>>
>>>
>>I think there should be an "except..." clause there. :-)
>>"Except if the entry is marked as moved". Of course, we don't do that
>>yet -- but we should, to support atomic renames on the client side.
>>
>>
>
>That's not an 'svn mv', just a plain mv, so that foo is missing.
>
>
Oh duh, right. Silly me. Sorry.
Your example is still good; that entry shouldn't be removed just because
the directory is missing. Instead, the "svn up" should pull a new copy
of foo.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Philip Martin <ph...@codematters.co.uk>.
Branko Äibej <br...@xbc.nu> writes:
> Philip Martin wrote:
>
> >I am wary of this removal of missing directories, does it mean that
> >the sequence
> >
> > svn mkdir foo
> > mv foo bar
> > svn up
> >
> >will remove the foo entry? If so, it would fall into the "I don't
> >expect that to happen" category.
> >
> >
> I think there should be an "except..." clause there. :-)
> "Except if the entry is marked as moved". Of course, we don't do that
> yet -- but we should, to support atomic renames on the client side.
That's not an 'svn mv', just a plain mv, so that foo is missing.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Branko Čibej <br...@xbc.nu>.
Philip Martin wrote:
>I am wary of this removal of missing directories, does it mean that
>the sequence
>
> svn mkdir foo
> mv foo bar
> svn up
>
>will remove the foo entry? If so, it would fall into the "I don't
>expect that to happen" category.
>
>
I think there should be an "except..." clause there. :-)
"Except if the entry is marked as moved". Of course, we don't do that
yet -- but we should, to support atomic renames on the client side.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Philip Martin <ph...@codematters.co.uk>.
Brian Denny <br...@briandenny.net> writes:
> Followup to r5814 (Issue #1075 - update receiving delete for missing
> directory). Make sure to clean up missing directories any time we bump
> a revision number.
>
> * update_editor.c
> (maybe_bump_dir_info): For each directory that we actually bump,
> remove any entries for missing or 'deleted' children.
>
>
> Index: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c (revision 5877)
> +++ subversion/libsvn_wc/update_editor.c (working copy)
[...]
> +
> + /* Clean up deleted/missing directories. */
> + SVN_ERR (svn_wc_adm_retrieve (&adm_access, eb->adm_access, bdi->path,
> + pool));
> + SVN_ERR (svn_wc_entries_read (&entries, adm_access, TRUE, pool));
> + SVN_ERR (svn_io_get_dirents (&dirents, bdi->path, pool));
> + for (hi = apr_hash_first (pool, entries); hi; hi = apr_hash_next (hi))
> + {
> + const void *key;
> + apr_ssize_t klen;
> + void *val;
> + const svn_wc_entry_t *current_entry;
> +
> + /* Get the next entry */
> + apr_hash_this (hi, &key, &klen, &val);
> + current_entry = val;
> +
> + /* Skip THIS_DIR. */
> + if (! strcmp (key, SVN_WC_ENTRY_THIS_DIR))
> + continue;
> +
> + /* If the item is a 'deleted' or missing dir, remove it. */
> + if (current_entry->kind == svn_node_dir
> + && (current_entry->deleted
> + || (! apr_hash_get (dirents, key, klen))))
> + {
> + svn_wc__entry_remove (entries, current_entry->name);
> + }
I am wary of this removal of missing directories, does it mean that
the sequence
svn mkdir foo
mv foo bar
svn up
will remove the foo entry? If so, it would fall into the "I don't
expect that to happen" category.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Ben Collins-Sussman <su...@collab.net>.
Brian Denny <br...@briandenny.net> writes:
> On Tue, May 13, 2003 at 01:31:41PM -0500, Ben Collins-Sussman wrote:
> >
> > I'm about to make major changes to update_editor.c.
> >
> > * add_dir() will immediately add an entry to the parent's entries
> > file, and create a new directory containing an entries file with
> > only a THIS_DIR entry at update's target-revnum, with an
> > 'incomplete=true' attribute.
> >
> > * open_dir() will immediately mark the directory as incomplete, and as
> > being at the update's target-revnum
> >
> > * close_dir() will still call maybe_bump_dir_info()
> >
> > * maybe_bump_dir_info() will remove the 'incomplete' flag if the dir's
> > refcount hits zero (rather than changing the revnum)
> >
> > Dunno if this has weird affects regarding your patch....?
>
> if the process is interrupted inbetween open_dir and close_dir,
> leaving the directory marked as incomplete (and at the target revnum),
> what happens when you run 'update' again?
>
> (i assume that dealing with this case is one reason for the 'incomplete'
> flag, but how does it work?)
Yes, that's exactly the case we're trying to handle.
When 'svn up' sees an incomplete directory, it tells the server that
it "has revision N of the directory, but assume it's empty". Then it
forcibly reports exactly the entries it has. And thus the server
knows exactly what needs to be sent to 'complete' the directory.
Voila, restartable checkouts.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Brian Denny <br...@briandenny.net>.
On Tue, May 13, 2003 at 01:31:41PM -0500, Ben Collins-Sussman wrote:
>
> I'm about to make major changes to update_editor.c.
>
> * add_dir() will immediately add an entry to the parent's entries
> file, and create a new directory containing an entries file with
> only a THIS_DIR entry at update's target-revnum, with an
> 'incomplete=true' attribute.
>
> * open_dir() will immediately mark the directory as incomplete, and as
> being at the update's target-revnum
>
> * close_dir() will still call maybe_bump_dir_info()
>
> * maybe_bump_dir_info() will remove the 'incomplete' flag if the dir's
> refcount hits zero (rather than changing the revnum)
>
> Dunno if this has weird affects regarding your patch....?
if the process is interrupted inbetween open_dir and close_dir,
leaving the directory marked as incomplete (and at the target revnum),
what happens when you run 'update' again?
(i assume that dealing with this case is one reason for the 'incomplete'
flag, but how does it work?)
-brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Ben Collins-Sussman <su...@collab.net>.
Brian Denny <br...@briandenny.net> writes:
> [Some comments on this patch which should make it easier to review:
> "maybe_bump_dir_info" is the place under close_directory where revision
> numbers in the entries file may get updated. My thinking is, if we are
> ready to bump the revnum, then we should also be ready to prune
> missing/deleted directories. So iterate through the entries, removing
> any entry for a missing or deleted directory.]
I'm about to make major changes to update_editor.c.
* add_dir() will immediately add an entry to the parent's entries
file, and create a new directory containing an entries file with
only a THIS_DIR entry at update's target-revnum, with an
'incomplete=true' attribute.
* open_dir() will immediately mark the directory as incomplete, and as
being at the update's target-revnum
* close_dir() will still call maybe_bump_dir_info()
* maybe_bump_dir_info() will remove the 'incomplete' flag if the dir's
refcount hits zero (rather than changing the revnum)
Dunno if this has weird affects regarding your patch....?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Brian Denny <br...@briandenny.net>.
On Fri, May 09, 2003 at 09:53:36PM -0700, Brian Denny wrote:
> Alright, here is a stab at cleaning up this loose end. I don't know
> how to create a test for this case since it requires that the process
> be interrupted at a particular point, but I checked by hand that a
> scenario which, without this patch, would leave behind an entry for
> the missing item, now does the right thing.
>
> Comments welcome (as always).
With the 0.23 release approaching, we need to decide whether that
release should include r5814 with or without this (or a similar) patch.
In my view, r5814 is an improvement and *could* go in even without any
followup. It basically solves Issue #1075; the flaw that Philip pointed
out (a) isn't much of a regression since it already existed for the
'deleted' case, (b) only applies if the update is interrupted at a
certain point, and (c) although it can result in bogus entries data
in case of such an interruption, "updating again" actually worked in the
simple use case i tried (and fixed the entries data).
I'm not saying that a followup to r5814 isn't needed -- just that I
think it would be okay to release 0.23 with r5814 sans followup.
If others disagree, speak up.
All that said, this patch works for me as the needed followup.
But I don't want to commit it without a review. If no one has time,
that's fine, we still have the option of rolling 0.23 with (or without)
r5814.
[Some comments on this patch which should make it easier to review:
"maybe_bump_dir_info" is the place under close_directory where revision
numbers in the entries file may get updated. My thinking is, if we are
ready to bump the revnum, then we should also be ready to prune
missing/deleted directories. So iterate through the entries, removing
any entry for a missing or deleted directory.]
-brian
> Log:
> Followup to r5814 (Issue #1075 - update receiving delete for missing
> directory). Make sure to clean up missing directories any time we bump
> a revision number.
>
> * update_editor.c
> (maybe_bump_dir_info): For each directory that we actually bump,
> remove any entries for missing or 'deleted' children.
>
>
> Index: subversion/libsvn_wc/update_editor.c
> ===================================================================
> --- subversion/libsvn_wc/update_editor.c (revision 5877)
> +++ subversion/libsvn_wc/update_editor.c (working copy)
> @@ -295,6 +295,8 @@
> {
> svn_wc_entry_t tmp_entry;
> svn_wc_adm_access_t *adm_access;
> + apr_hash_t *entries, *dirents;
> + apr_hash_index_t *hi;
>
> if (--bdi->ref_count > 0)
> return SVN_NO_ERROR; /* directory isn't done yet */
> @@ -338,6 +340,38 @@
> | SVN_WC__ENTRY_MODIFY_DELETED),
> TRUE, pool));
> }
> +
> + /* Clean up deleted/missing directories. */
> + SVN_ERR (svn_wc_adm_retrieve (&adm_access, eb->adm_access, bdi->path,
> + pool));
> + SVN_ERR (svn_wc_entries_read (&entries, adm_access, TRUE, pool));
> + SVN_ERR (svn_io_get_dirents (&dirents, bdi->path, pool));
> + for (hi = apr_hash_first (pool, entries); hi; hi = apr_hash_next (hi))
> + {
> + const void *key;
> + apr_ssize_t klen;
> + void *val;
> + const svn_wc_entry_t *current_entry;
> +
> + /* Get the next entry */
> + apr_hash_this (hi, &key, &klen, &val);
> + current_entry = val;
> +
> + /* Skip THIS_DIR. */
> + if (! strcmp (key, SVN_WC_ENTRY_THIS_DIR))
> + continue;
> +
> + /* If the item is a 'deleted' or missing dir, remove it. */
> + if (current_entry->kind == svn_node_dir
> + && (current_entry->deleted
> + || (! apr_hash_get (dirents, key, klen))))
> + {
> + svn_wc__entry_remove (entries, current_entry->name);
> + }
> +
> + } /* end entries loop */
> +
> + SVN_ERR (svn_wc__entries_write (entries, adm_access, pool));
> }
> /* we exited the for loop because there are no more parents */
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
> For additional commands, e-mail: dev-help@subversion.tigris.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Brian Denny <br...@briandenny.net>.
On Wed, May 07, 2003 at 04:44:19PM -0500, cmpilato@collab.net wrote:
> Brian Denny <br...@briandenny.net> writes:
>
> > I'm thinking that cleaning up missing/deleted dirs right *before* the
> > call to maybe_bump_dir_info might be the right thing?
> >
> > No, wait, maybe_bump_dir_info can recurse up the directory tree... so
> > the cleanup needs to happen *inside* that loop?
>
> Hm... I'm not able to say without re-learning what that code does.
> I'll try to take a look at this later and help you make a decision.
Alright, here is a stab at cleaning up this loose end. I don't know
how to create a test for this case since it requires that the process
be interrupted at a particular point, but I checked by hand that a
scenario which, without this patch, would leave behind an entry for
the missing item, now does the right thing.
Comments welcome (as always).
-brian
Log:
Followup to r5814 (Issue #1075 - update receiving delete for missing
directory). Make sure to clean up missing directories any time we bump
a revision number.
* update_editor.c
(maybe_bump_dir_info): For each directory that we actually bump,
remove any entries for missing or 'deleted' children.
Index: subversion/libsvn_wc/update_editor.c
===================================================================
--- subversion/libsvn_wc/update_editor.c (revision 5877)
+++ subversion/libsvn_wc/update_editor.c (working copy)
@@ -295,6 +295,8 @@
{
svn_wc_entry_t tmp_entry;
svn_wc_adm_access_t *adm_access;
+ apr_hash_t *entries, *dirents;
+ apr_hash_index_t *hi;
if (--bdi->ref_count > 0)
return SVN_NO_ERROR; /* directory isn't done yet */
@@ -338,6 +340,38 @@
| SVN_WC__ENTRY_MODIFY_DELETED),
TRUE, pool));
}
+
+ /* Clean up deleted/missing directories. */
+ SVN_ERR (svn_wc_adm_retrieve (&adm_access, eb->adm_access, bdi->path,
+ pool));
+ SVN_ERR (svn_wc_entries_read (&entries, adm_access, TRUE, pool));
+ SVN_ERR (svn_io_get_dirents (&dirents, bdi->path, pool));
+ for (hi = apr_hash_first (pool, entries); hi; hi = apr_hash_next (hi))
+ {
+ const void *key;
+ apr_ssize_t klen;
+ void *val;
+ const svn_wc_entry_t *current_entry;
+
+ /* Get the next entry */
+ apr_hash_this (hi, &key, &klen, &val);
+ current_entry = val;
+
+ /* Skip THIS_DIR. */
+ if (! strcmp (key, SVN_WC_ENTRY_THIS_DIR))
+ continue;
+
+ /* If the item is a 'deleted' or missing dir, remove it. */
+ if (current_entry->kind == svn_node_dir
+ && (current_entry->deleted
+ || (! apr_hash_get (dirents, key, klen))))
+ {
+ svn_wc__entry_remove (entries, current_entry->name);
+ }
+
+ } /* end entries loop */
+
+ SVN_ERR (svn_wc__entries_write (entries, adm_access, pool));
}
/* we exited the for loop because there are no more parents */
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by cm...@collab.net.
Brian Denny <br...@briandenny.net> writes:
> I'm thinking that cleaning up missing/deleted dirs right *before* the
> call to maybe_bump_dir_info might be the right thing?
>
> No, wait, maybe_bump_dir_info can recurse up the directory tree... so
> the cleanup needs to happen *inside* that loop?
Hm... I'm not able to say without re-learning what that code does.
I'll try to take a look at this later and help you make a decision.
> [I am trying to find the right balance here between asking questions
> and figuring stuff out for myself. If I make y'all do too much of my
> research for me, I'm not saving you any time by taking this issue.
> On the other hand, there are many cases where a core subversion
> developer can tell me in 5 minutes what it would take me hours to
> figure out for myself. I figure you probably know where the right
> balance is better than I do, so I'm going to err on the side of asking
> lots of questions for a while and you can tell me to go study the code
> when you think that's appropriate.]
I am impressed by your honesty and consideration. Wow. +1 on marking
the "Brian Denny is clueless" issue as CLOSED/INVALID. :-) Seriously,
I think you're putting just the right amount of effort into this.
> > (2) the global post-update revision bump (do_update_cleanup).
>
> I think that r5814 covers place (2). Do you disagree?
Nope, from what I can tell, r5814 does indeed cover place (2).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by Brian Denny <br...@briandenny.net>.
On Mon, May 05, 2003 at 09:11:25PM -0500, cmpilato@collab.net wrote:
> the timing of this cleanup. The removal of missing items needs to
> happen at the same time as the bumping of the directory version
> number. When I originally came up with this solution, my idea was
> that this cleanup code would occur in two places:
>
> (1) the update_editor's close_dir() (which might require a new loggy
> operation)
I'm thinking that cleaning up missing/deleted dirs right *before* the
call to maybe_bump_dir_info might be the right thing?
No, wait, maybe_bump_dir_info can recurse up the directory tree... so
the cleanup needs to happen *inside* that loop?
[I am trying to find the right balance here between asking questions
and figuring stuff out for myself. If I make y'all do too much of my
research for me, I'm not saving you any time by taking this issue.
On the other hand, there are many cases where a core subversion
developer can tell me in 5 minutes what it would take me hours to
figure out for myself. I figure you probably know where the right
balance is better than I do, so I'm going to err on the side of asking
lots of questions for a while and you can tell me to go study the code
when you think that's appropriate.]
> (2) the global post-update revision bump (do_update_cleanup).
I think that r5814 covers place (2). Do you disagree?
-brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 5814 - in trunk/subversion: libsvn_wc tests/clients/cmdline
Posted by cm...@collab.net.
Philip Martin <ph...@codematters.co.uk> writes:
> bdenny@tigris.org writes:
>
> > Author: bdenny
> > Date: Mon May 5 16:40:25 2003
> > New Revision: 5814
[...]
> I still think the correct approach is to make the repository send the
> delete for the missing subdirectory. Update reports the missing
> directory to the repository, see the reporter->delete_path calls in
> libsvn_wc/adm_crawler.c:report_revisions. The repository delta
> generator should make better use of this information.
And I still disagree with you. The only problem with this patch is
the timing of this cleanup. The removal of missing items needs to
happen at the same time as the bumping of the directory version
number. When I originally came up with this solution, my idea was
that this cleanup code would occur in two places:
(1) the update_editor's close_dir() (which might require a new loggy
operation)
(2) the global post-update revision bump (do_update_cleanup).
Though, I betcha my brain was moving faster than my fingers and I
failed to articulate that.
--
Now, part of my disgust with your insistance on making the repository
do this work is that I assumed (as did Brian Denny) that this change
would require modifications to svn_repos_dir_delta(). Those
modifications (at least the theory behind what Brian started doing
there) were completely I'm-gonna-minus-one-all-over-your-head wrong
in nature.
However, it occurs to me that perhaps the repos layer could provider a
second editor which is composed with the dir-delta editor, and whose
close_dir() function would perform the necessary checks and transmit
the extra editor->delete_entry() calls.
I dunno how that it would all turn out. I still think the fix should
be client side.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org