Re: bug in repos_replay

On 1/24/07, Vlad Georgescu <vgeorgescu@gmail.com> wrote:
> Hey, I actually spent some time today trying to get to the bottom of this.

Hooray, that was my secret wish :-)

> This
> is what I did:
> - when listing props or calculating deltas, add_subdir uses the *target* of the
> copy, not the source (this way we don't lose the modifications that were done
> after the copy)

Why not also when iterating over dirents?

> - in add_subdir, I eliminate directory entries from the changed_paths hash;
> these are then handled normally by add_subdir
> - if path_driver_cb_func is handed a path that was deleted from changed_paths,
> it ignores it (i.e. returns immediately)

Yeah, this is all reasonable. The patch seems to work, in that it at
least won't break like the old version, and it fixes the problem in
the test.

There's still one edge case that it doesn't handle correctly: Let's say we have

/private/foo
/public/trunk
/public/bar

(where private is unreadable by the syncer). Let's say we have a
single revision that does

svn cp /private/foo /public/trunk/
svn cp /public/bar /public/trunk/foo/

Before your patch, this would just fail (I believe). With your patch,
I'm pretty sure (haven't tested it) that it will give you the correct
files at the end, but /public/trunk/foo/bar and everything under it
will be added one by one instead of copied from /public/bar. I think
that can be fixed, though (I'll see if I get around to it).

But hey, missing some copies is way better than not working at all; thanks!

> Here's what I've got so far (no log message yet, sorry, and the test still
> doesn't pass because of a minor inconsistency in the expected output):

Yeah, as I mentioned in a comment, everything after "run_sync" in the
test was untested; your patch was fine.

> > This feels very dirty, though. Does anyone have an opinion? (It also
> > probably wouldn't work for newly added files inside the copied
> > directory, I guess, unless you actually throw in a loop over
> > changed_paths. Ick.)
>
> In my testing it also works for newly added files. We should of course extend
> the test case to cover this scenario.

Hmm. I'm a little confused about why this works (need to test it
myself): you're still iterating over dirents in the source, not the
target, so it seems like you wouldn't pick up adds and deletes. (This
is independent of the copy situation mentioned above.)

--dave

-- 
David Glasser | glasser_at_mit.edu | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org