On Mon, 2008-09-08 at 18:31 -0400, Paul Burba wrote:
> On Thu, Sep 4, 2008 at 12:53 PM, Paul Burba <ptburba_at_gmail.com> wrote:
> > On Mon, Sep 1, 2008 at 11:39 AM, Arfrever Frehtes Taifersar Arahesis
> > <arfrever.fta_at_gmail.com> wrote:
> >> I discovered 2 bugs in handling of svn:mergeinfo when I was performing some
> >> merges on the svnpatch-diff branch.
> >>
> >> 1. Addition of incorrect svn:mergeinfo on some files or directories:
> >>
> >> Steps to reproduce:
> >> svn co -r32624 https://svn.collab.net/repos/svn/branches/svnpatch-diff subversion-svnpatch-diff-r32624
> >> cd subversion-svnpatch-diff-r32624
> >> svn merge -r32500:32620 ^/trunk
> >>
> >> This merge adds incorrect svn:mergeinfo on:
> >> build.conf
> >> subversion/include
> >> subversion/libsvn_subr
> >> subversion/tests/libsvn_subr
> >>
> >> These files / directories don't have svn:mergeinfo before merging.
> >
> > Hi Arfrever,
> >
> > Not that you are necessarily saying this, but the fact that these
> > paths don't have pre-existing mergeinfo is not a problem per se.
> > Because explicit mergeinfo was added to these paths on trunk in r32524
> > when the merge is performed that mergeinfo gets added just like any
> > other property.
> >
> >> After merging, svn:mergeinfo on these files / directories doesn't contain:
> >> /trunk:1-31375,31378-32620
> >
> > Now that *is* a problem. The mergeinfo from r32524 gets added the the
> > four paths in question but that's all. There are two separate
> > problems here (using build.conf to illustrate the problem, but it's
> > the same problem across all four paths):
> >
> > 1) libsvn_client/merge.c:merge_props_changed() adds the mergeinfo from
> > r32524 to build.conf, but it but does not consider what mergeinfo
> > build.conf inherited.
> >
> > Prior to the merge build.conf inherits this mergeinfo from the root of
> > the svnpatch-diff working copy:
> >
> > /branches/1.5.x-r30215/build.conf:30238
> > /branches/bdb-reverse-deltas/build.conf:31976-32455
> > /branches/diff-callbacks3/build.conf:29985-30687
> > /branches/dont-save-plaintext-passwords-by-default/build.conf:30654-31044
> > /branches/gnome-keyring/build.conf:30484-31336
> > /branches/in-memory-cache/build.conf:29755-31378
> > /branches/issue-3000/build.conf:31639,31642-31645,31647-31652,31654,31660
> > /branches/issue-3220-dev/build.conf:32136-32152
> > /branches/kwallet/build.conf:30711-31240
> > /branches/log-g-performance/build.conf:30867-30958
> > /branches/svn-mergeinfo-enhancements/build.conf:30045-30214
> > /branches/svnserve-logging/build.conf:29754-30819
> > /trunk/build.conf:1-31375,31378-32500
> >
> > But merge_props_changed() just adds this mergeinfo from r32524:
> >
> > /branches/1.5.x-r30215/build.conf:30238
> > /branches/bdb-reverse-deltas/build.conf:31976-32455
> > /branches/diff-callbacks3/build.conf:29985-30687
> > /branches/dont-save-plaintext-passwords-by-default/build.conf:30654-31044
> > /branches/fs-rep-sharing/build.conf:28976,29040,30035,30651,30784,30788-30789,30813,30822,30840,30846,30866,31092,31106,31113
> > /branches/gnome-keyring/build.conf:30484-31336
> > /branches/in-memory-cache/build.conf:29755-31378
> > /branches/issue-3000/build.conf:31639,31642-31645,31647-31652,31654,31660
> > /branches/issue-3220-dev/build.conf:32136-32152
> > /branches/kwallet/build.conf:30711-31240
> > /branches/log-g-performance/build.conf:30867-30958
> > /branches/svn-mergeinfo-enhancements/build.conf:30045-30214
> > /branches/svnserve-logging/build.conf:29754-30819
> >
> > So here we "lose" the inherited mergeinfo
> > '/trunk/build.conf:1-31375,31378-32500' from making it look like that
> > hasn't been merged to build.conf from trunk yet.
> >
> > 2) At the start of the merge
> > libsvn_client/merge.c:do_directory_merge() gathers up all the subtrees
> > with mergeinfo and stores them in children_with_mergeinfo, obviously
> > build.conf is not part of this list as it has no pre-existing
> > mergeinfo.
> >
> > Next do_directory_merge() then drives the editor, which adds explicit
> > mergeinfo to the four new paths. Then it updates the mergeinfo for
> > the paths in children_with_mergeinfo to reflect the merge just
> > performed. But since build.conf isn't described in
> > children_with_mergeinfo it doesn't get updated -- so effectively and
> > build.conf loses the mergeinfo '/trunk/build.conf:32501-32620'.
> >
> > Working on a fix for both of these problems.
>
> Added a test to replicate these problems in r32968, fixed both
> problems in r32975.
>
> Also nominated the test/fix for backport to 1.5.x.
>
> Paul
Fantastic! Thanks, Paul. Reviewing now...
You added a new call to find_child_or_parent() via a new intermediate
function find_child_with_mergeinfo(). I looked at find_child_or_parent()
and tried to validate the requirements that doc string states. The new
call ignores the requirement that the element at position START_INDEX is
the one whose child/parent we are seeking, and I looked and found that
that is not a necessary requirement, so it still works.
An improvement suggestion: Replace these search functions
find_child_or_parent() and find_child_with_mergeinfo() with a call to
the standard bsearch() binary search function. Stop passing around the
index at which a new element should be inserted (bsearch() doesn't tell
us that anyway), and make the "insert" functions find the index each
time they need it. That sounds like we will be doing the same search
twice, and we will, but it's more robust against programmer error.
(Changing from linear to binary searches at the same time should
alleviate any concern about the speed.)
See the attached patch for an attempt at that. It doesn't work: I made
some silly mistake somewhere and I can't find it. The search function
ends up being asked to search in an array that has a null element in it,
and so it crashes. merge_tests.py 50 and 62 fail, for example. I'm aware
that there is a function that intentionally sets some elements to null,
and I'm not sure whether that is what's happened or whether my
replacement "insert" function sometimes sets the new element to null.
Hmm... that wasn't actually much related to your patch, but there you
go.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-09-09 20:16:19 CEST