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 2006/04/21 14:26:00 UTC
Reporting subtree deletes through svn_delta_editor_t?
[Sorry, long message. Summary: I'd like to change the editor interface
to inline reports of deleted subtrees, if that makes sense.]
Hello all,
I notice that Garrett has changed the svn_delta_editor_t interface in
the atomic renames branch, which has reminded me about something I've
been toying with for a while.
Issue #2333 is about my favourite topic: svn diff. Specifically,
reporting diffs against a repository revision for deleted directories.
When we do a wc-repos diff, we effectively request that BASE be updated
to the target repos revision, and use the editor to drive our diff output.
Repos-repos diffs are similar.
The problem occurs when we see a deleted directory, because we don't
get any detail about what the contents of that directory were. So if
we have a file /A/B/x and we delete /A/B, we'll only see the following
calls to the editor:
open_directory A
delete_entry B
close_directory A
There are effectively two ways to handle this that I can see.
The first is to pause whenever we see a delete_entry and separately
traverse the deleted subtree. I've already added code for repos->WORKING
diffs to do this (which is easy because we need to traverse the local wc),
but it's fragile and doesn't work for repos-BASE or WORKING->repos diffs
(where we'd need to contact the repository).
Also note that if we did this for repos-repos diffs, we'd need to use at
least three connections: one to receive the report request, a second to
traverse the deleted subtrees, and a third to fetch the contents of files.
The other way is to inline the deletions into the editor. What I've
played around with is adjusting the interface to recurse into deleted
subtrees, before deleting them. The above example then looks like this:
open_directory A
open_directory B
delete_entry x
close_directory B
delete_entry B
close_directory A
This works really quite well, and I can imagine other consumers of the
editor interface being interested in the information, so I don't see it
as a hack just for diff.
Doing this fixes simple repos-repos diffs and wc-repos diffs immediately,
because the consumers now only need to deal with file deletes, and they
all already do. I _think_ it might even be allowed by the spirit of
the existing interface as well.
But there's a problem that I can't fix, apart from the hackiness:
Currently, in a repos-WORKING diff, we wait until we see a close_directory
and then run through the wc outputting local modifications.
The problem is that if we have schedule-add files in a to-be-deleted
directory, these should be reported as deleted, but we don't know that
the directory is deleted until _after_ we close it. In other words,
we can't tell the difference between the deletion of /A/B itself, and
the deletion of all its contents, until it's too late.
So what I'd like to do is change the editor interface so that the caller
can request something like the following:
open_directory A
open_deleted_directory B
delete_entry x
close_deleted_directory B
close_directory A
Or in other words, optionally inline the deleted subtree at the point
of deletion.
What do people think?
Regards,
Malcolm
PS I've attached my current patch and reproduction recipe, if anyone's
interested.
[[[
Hackily force 'deep deletes' (recursion into deleted directories).
* libsvn_repos/reporter.c
(update_entry): recurse into deleted directories if the deep_deletes
option is set.
(delta_dirs): support a NULL target as well as a NULL source.
Call update_entry() to delete deleted entries as well as replaced
entries, rather than calling the delete_entry callback directly.
* libsvn_wc/diff.c (delete_entry): disable the wc traversal for
repos->WORKING diffs if we already recursed into the deleted directory.
]]]
Re: Reporting subtree deletes through svn_delta_editor_t?
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
Malcolm Rowe writes:
> On Fri, Apr 21, 2006 at 09:07:15PM +0200, Peter N. Lundblad wrote:
> > Malcolm Rowe writes:
> > > The first is to pause whenever we see a delete_entry and separately
> > > traverse the deleted subtree. I've already added code for repos->WORKING
> > > diffs to do this (which is easy because we need to traverse the local wc),
> > > but it's fragile and doesn't work for repos-BASE or WORKING->repos diffs
> > > (where we'd need to contact the repository).
> > >
> > > Also note that if we did this for repos-repos diffs, we'd need to use at
> > > least three connections: one to receive the report request, a second to
> > > traverse the deleted subtrees, and a third to fetch the contents of files.
> > >
> >
> > Why couldn't you just "check out" the deleted directory in the source revision
> > interpreting adds as deletes. This wouldn't require a third session
> > and it would work with old servers.
>
> Yes, I see no reason why that wouldn't work either (good idea). However,
> the number of connections isn't the real problem (I probably shouldn't
> have mentioned it).
>
>
> The real question was whether getting the names of deleted entries
> within a deleted subtree (rather than just the subtree root itself)
> is uncommon enough that we should require clients to implement the
> recursion logic themselves.
Well, isn't "the recursion logic" just to issue a single RA request on
an auxliary session and be prepared for the response? I don't know
how common that need would be except for the diff case. A nice thing
about the "checkout" idea would be that it works transparently with
servers from 1.0 and forwards.
A related topic here is the roundtrip problem of diff and merge.
Currently, for diff/merge, for each reported change, we need to do a
request to get the source of the delta, wait for that, and then
compare the source and target. Since this can't be pipelined, it
results in lots of waiting on the network layer.
Maybe, but this is an idea that just popped up in my mind, we could
introduce another editor into the mix. That editor would get the
relevant parts of the delta source "in paralell" with the main
edit. So, when we got an open_file with a text delta in the main edit,
we got the same operation, but with a fulltext for the delta source in
this new editor. When we got a delete_entry in the main editor, we
would get an add for the same entry (and its children) in the new
editor to describe the deleted target. If this is feasable (and I
don't know yet), this could solve both problems without hacking things
into the editor interface. I guess this idea has to live in our minds
for a while, so don't consider this a proposal:-)
Thoughts?
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reporting subtree deletes through svn_delta_editor_t?
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Fri, Apr 21, 2006 at 09:07:15PM +0200, Peter N. Lundblad wrote:
> Malcolm Rowe writes:
> > The first is to pause whenever we see a delete_entry and separately
> > traverse the deleted subtree. I've already added code for repos->WORKING
> > diffs to do this (which is easy because we need to traverse the local wc),
> > but it's fragile and doesn't work for repos-BASE or WORKING->repos diffs
> > (where we'd need to contact the repository).
> >
> > Also note that if we did this for repos-repos diffs, we'd need to use at
> > least three connections: one to receive the report request, a second to
> > traverse the deleted subtrees, and a third to fetch the contents of files.
> >
>
> Why couldn't you just "check out" the deleted directory in the source revision
> interpreting adds as deletes. This wouldn't require a third session
> and it would work with old servers.
Yes, I see no reason why that wouldn't work either (good idea). However,
the number of connections isn't the real problem (I probably shouldn't
have mentioned it).
The real question was whether getting the names of deleted entries
within a deleted subtree (rather than just the subtree root itself)
is uncommon enough that we should require clients to implement the
recursion logic themselves.
> If we have to extend the editor interface, there's nothing we can do,
> but I would like to avoid clutttering it if possible.
>
We don't _have_ to extend it, of course, because each client operation
can re-implement the necessary logic. It's just more convenient for
those callers if we do.
I'm not wedded to the interface I proposed either - I'm more interested in
whether the concept makes sense. For example, we could just add a single
new delete_directory call that works like a parallel to add_directory,
except that it can only contain other delete_directory, close_directory,
and delete_entry calls (rather than add_directory/add_file):
open_directory A
delete_directory B
delete_entry x
close_directory B
close_directory A
Just to make one thing clear: I'm not proposing to change the editor
interface before the atomic-renames work does, which I guess makes this
something for 1.5.
> On a side note, you can't do delete_entry after having opened a
> directory
> according to restriction 1. of the editor interface documentation, so
> we need something new here.
>
Yeah, that's why I said 'the spirit' of the interface. But let's no go
into that, it's not something I'm seriously proposing.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reporting subtree deletes through svn_delta_editor_t?
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
Malcolm Rowe writes:
> The first is to pause whenever we see a delete_entry and separately
> traverse the deleted subtree. I've already added code for repos->WORKING
> diffs to do this (which is easy because we need to traverse the local wc),
> but it's fragile and doesn't work for repos-BASE or WORKING->repos diffs
> (where we'd need to contact the repository).
>
> Also note that if we did this for repos-repos diffs, we'd need to use at
> least three connections: one to receive the report request, a second to
> traverse the deleted subtrees, and a third to fetch the contents of files.
>
I've been in the diff code (but not in any way as much as you), so I know
that this is complicated stuff, so excuse me for a silly question...
Why couldn't you just "check out" the deleted directory in the source revision
interpreting adds as deletes. This wouldn't require a third session
and it would work with old servers. I can't see why this shouldn't
work for repos-repos diffs, but I'm not sure for repos-BASE. Is there
an issue with mixed revisions or can that be handled on the client
side?
If we have to extend the editor interface, there's nothing we can do,
but I would like to avoid clutttering it if possible.
On a side note, you can't do delete_entry after having opened a
directory
according to restriction 1. of the editor interface documentation, so
we need something new here.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reporting subtree deletes through svn_delta_editor_t?
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Apr 24, 2006 at 09:48:42AM -0500, Ben Collins-Sussman wrote:
> On 4/21/06, Malcolm Rowe <ma...@farside.org.uk> wrote:
> >The problem occurs when we see a deleted directory, because we don't
> >get any detail about what the contents of that directory were. So if
> >we have a file /A/B/x and we delete /A/B, we'll only see the following
> >calls to the editor:
> >
> >open_directory A
> > delete_entry B
> >close_directory A
>
> I apologize for the ignorant question: why is this a problem? I sort
> of like the terse output. I always get annoyed when I run 'svn rm
> somedir/', and then 'svn status' shows me every single child as
> deleted. Was it decided long ago that terseness is bad, and universal
> verbosity is the goal?
>
It's a problem only in the sense that diff (and diff-summarise) would
rather like to show you what got deleted (as they do already for
single-file deletes).
Don't worry - I'm not in the slightest proposing that we change the editor
calls for those consumers that aren't interested in this information,
sorry if that wasn't clear.
(This is issue #2333, by the way).
I'm surprised that 'svn status' behaves like that, but it's probably
way too late to change its default behaviour.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Reporting subtree deletes through svn_delta_editor_t?
Posted by Ben Collins-Sussman <su...@red-bean.com>.
On 4/21/06, Malcolm Rowe <ma...@farside.org.uk> wrote:
> The problem occurs when we see a deleted directory, because we don't
> get any detail about what the contents of that directory were. So if
> we have a file /A/B/x and we delete /A/B, we'll only see the following
> calls to the editor:
>
> open_directory A
> delete_entry B
> close_directory A
I apologize for the ignorant question: why is this a problem? I sort
of like the terse output. I always get annoyed when I run 'svn rm
somedir/', and then 'svn status' shows me every single child as
deleted. Was it decided long ago that terseness is bad, and universal
verbosity is the goal?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org