Resending this to get it back on s.a.o. Bert's reply (which I originally replied to all to) jumped it to s.t.o. When are we shutting down the s.t.o mailing lists again?
On Thu, Dec 10, 2009 at 4:25 AM, Bert Huijben <rhuij...@sharpsvn.net> wrote: >> -----Original Message----- >> From: C. Michael Pilato [mailto:cmpil...@collab.net] >> Sent: woensdag 9 december 2009 22:08 >> To: Subversion Development >> Subject: HEADS UP: Damaged mergeinfo in our history; likely pain in our >> future. >> >> While trying to figure out why my attempt at performing a catch-up merge of >> the 'issue-3242-dev' branch with ^/subversion/trunk was conflicting like >> crazy for me today, I discovered something unhappy: >> >> $ svn pget svn:mergeinfo . >> subversion/branches/1.5.x-r30215:870312 >> subversion/branches/bdb-reverse-deltas:872050-872529 >> subversion/branches/diff-callbacks3:870059-870761 >> [...] >> >> See the leading slashes on those mergeinfo paths? Oh, that's right, you >> can't see them *because they aren't there*! >> >> A quick glance at the code behind 'svnadmin load --parent-dir' which is >> supposed to prepend the parent directory onto each of the mergeinfo paths >> confirms that the code naively does this prepending without any attention to >> the possibility that the parent directory lacks a leading slash. While >> that's not an errorful situation in general (parent_dir lacking the leading >> slash), the slash-less value certainly should have been detected and >> corrected before being used in a storage format that cares deeply about such >> things (like our svn:mergeinfo property format does). >> >> So. Bug in libsvn_repos loading code. Filed as issue #3547. I'm testing a >> fix for that now. >> >> But the bigger question is, "Where do we go from here?" I suspect that >> *all* of our repository-stored mergeinfo -- the entire history thereof -- is >> technically, syntactically invalid at this point. Do we try to repair it? >> Or do we take this opportunity to make our merge tracking code a little more >> ... flexible (always storing one format, but accepting either)? > > I think we can (and probably should) be a bit more flexible here. I don't see > major issues in accepting paths without a '/' here. > > I would actually prefer repository root relative paths without '/', but we > can't change the design on these values. (The new svn_relpath_ functions > should be used on them but can't because they have a leading '/') > > I hope we can fix this by just updating the code that parses the property > into our internal representation, but I think Paul can answer that easily. >From the "It's Never Quite So Simple" Files... It's easy enough to tweak svn_mergeinfo_parse() to create svn_mergeinfo_t with absolute path keys even if the input has relative paths -- see the attached path. This solves Mike's original problem with the sync merge to the 'issue-3242-dev' branch. It also passes our 1.6.x test suite (still running the trunk tests). This fix does however, produce some odd side effects based on the fact that mergeinfo that differs only by the leading slash is considered the same. So the svn_mergeinfo_(diff|merge|remove) APIs behave oddly. For example, if we checkout the issue 3242 branch @888941 we see the relative path mergeinfo that the "bad" load into the ASF repos produced: issue-3242-dev-wc>svn pg svn:mergeinfo -vR Properties on '.': svn:mergeinfo subversion/branches/1.5.x-r30215:870312 subversion/branches/bdb-reverse-deltas:872050-872529 subversion/branches/diff-callbacks3:870059-870761 subversion/branches/dont-save-plaintext-passwords-by-default:870728-871118 subversion/branches/double-delete:870511-872970 subversion/branches/explore-wc:875486,875493,875497,875507,875511,875514,875559,875580-875581,875584,875587,875611,875627,875647,875667-875668,875711-875712 ,875733-875734,875736,875744-875748,875751,875758,875782,875795-875796,875830,875836,875838,875842,875852,875855,875864,875870,875873,875880,875885-875888,87589 0,875897-875898,875905,875907-875909,875935,875943-875944,875946,875979,875982-875983,875985-875986,875990,875997 subversion/branches/file-externals:871779-873302 subversion/branches/fs-rep-sharing:869036-873803 subversion/branches/fsfs-pack:873717-874575 subversion/branches/gnome-keyring:870558-871410 subversion/branches/http-protocol-v2:874395-876041 subversion/branches/in-memory-cache:869829-871452 subversion/branches/issue-2843-dev:871432-874179 subversion/branches/issue-3000:871713,871716-871719,871721-871726,871728,871734 subversion/branches/issue-3067-deleted-subtrees:873375-874084 subversion/branches/issue-3148-dev:875193-875204 subversion/branches/issue-3220-dev:872210-872226 subversion/branches/issue-3334-dirs:875156-875867 subversion/branches/kwallet:870785-871314 subversion/branches/log-g-performance:870941-871032 subversion/branches/merge-skips-obstructions:874525-874615 subversion/branches/ra_serf-digest-authn:875693-876404 subversion/branches/reintegrate-improvements:873853-874164 subversion/branches/subtree-mergeinfo:876734-878766 subversion/branches/svn-mergeinfo-enhancements:870119-870195,870197-870288 subversion/branches/svnpatch-diff:865738-876477 subversion/branches/svnraisetc:874709-875149 subversion/branches/svnserve-logging:869828-870893 subversion/branches/tc-issue-3334:874697-874773 subversion/branches/tc-merge-notify:874017-874062 subversion/branches/tc-resolve:874191-874239 subversion/branches/tc_url_rev:874351-874483 subversion/branches/tree-conflicts:868291-873154 subversion/branches/tree-conflicts-notify:873926-874008 subversion/trunk:879653-880579 # With the patch in place (this is a 1.6.x client) a merge # covering already merged revisions now works... issue-3242-dev-wc>svn merge ^^/subversion/trunk . -r0:880583 --- Merging r880580 through r880583 into '.': U tools\buildbot\master\master.cfg issue-3242-dev-wc>svn st M . M tools\buildbot\master\master.cfg # ...and while diff appears to show that the mergeinfo # has changed only by the newly merged revisions... issue-3242-dev-wc>svn diff --depth empty Property changes on: . ___________________________________________________________________ Modified: svn:mergeinfo Merged /subversion/trunk:r880580-880583 # ...when we actually look at the mergeinfo we see that # *all* of it has been quietly "fixed" to absolute paths: issue-3242-dev-wc>svn pg svn:mergeinfo -vR Properties on '.': svn:mergeinfo /subversion/branches/1.5.x-r30215:870312 /subversion/branches/bdb-reverse-deltas:872050-872529 /subversion/branches/diff-callbacks3:870059-870761 /subversion/branches/dont-save-plaintext-passwords-by-default:870728-871118 /subversion/branches/double-delete:870511-872970 /subversion/branches/explore-wc:875486,875493,875497,875507,875511,875514,875559,875580-875581,875584,875587,875611,875627,875647,875667-875668,875711-87571 2,875733-875734,875736,875744-875748,875751,875758,875782,875795-875796,875830,875836,875838,875842,875852,875855,875864,875870,875873,875880,875885-875888,8758 90,875897-875898,875905,875907-875909,875935,875943-875944,875946,875979,875982-875983,875985-875986,875990,875997 /subversion/branches/file-externals:871779-873302 /subversion/branches/fs-rep-sharing:869036-873803 /subversion/branches/fsfs-pack:873717-874575 /subversion/branches/gnome-keyring:870558-871410 /subversion/branches/http-protocol-v2:874395-876041 /subversion/branches/in-memory-cache:869829-871452 /subversion/branches/issue-2843-dev:871432-874179 /subversion/branches/issue-3000:871713,871716-871719,871721-871726,871728,871734 /subversion/branches/issue-3067-deleted-subtrees:873375-874084 /subversion/branches/issue-3148-dev:875193-875204 /subversion/branches/issue-3220-dev:872210-872226 /subversion/branches/issue-3334-dirs:875156-875867 /subversion/branches/kwallet:870785-871314 /subversion/branches/log-g-performance:870941-871032 /subversion/branches/merge-skips-obstructions:874525-874615 /subversion/branches/ra_serf-digest-authn:875693-876404 /subversion/branches/reintegrate-improvements:873853-874164 /subversion/branches/subtree-mergeinfo:876734-878766 /subversion/branches/svn-mergeinfo-enhancements:870119-870195,870197-870288 /subversion/branches/svnpatch-diff:865738-876477 /subversion/branches/svnraisetc:874709-875149 /subversion/branches/svnserve-logging:869828-870893 /subversion/branches/tc-issue-3334:874697-874773 /subversion/branches/tc-merge-notify:874017-874062 /subversion/branches/tc-resolve:874191-874239 /subversion/branches/tc_url_rev:874351-874483 /subversion/branches/tree-conflicts:868291-873154 /subversion/branches/tree-conflicts-notify:873926-874008 /subversion/trunk:879653-880583 Revving svn_mergeinfo_diff() to be able to consider differences in abs/rel paths would solve this particular problem. Of course this assumes we *want* to correct rel path mergeinfo we come across and are changing anyway. I tend to think we do, otherwise we'll end up with mergeinfo with mixed rel/abs paths, quite likely with a rel and abs version of the *same* path, e.g.: Properties on '.': svn:mergeinfo /subversion/trunk:879653-880579 subversion/trunk:880580-880583 Which is horrible from a human-readable perspective alone, not to mention the fact that we effectively have two instances of the same key when creating svn_mergeinfo_t from this value (hilarity is sure to ensue if we allow this). Anyway, I'm working to be sure we understand all the implications...more to come, just brain dumping where I am at the moment. Paul
