You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@apache.org> on 2021/04/02 19:02:52 UTC
SVN-4874: Avoid foreign repository merge if repository UUIDs match
(Bringing to dev@.)
Daniel Shahaf wrote:
> Are you looking for feedback on the write-up and/or patch? I assume
> not since you haven't mentioned any of this on dev@.
I would be glad of any feedback. I was going to wait until I had a patch ready for review. Now there is a patch attached to the issue, and, though it really wants more tests, now could be a good time.
So: could anyone cast an eye over this for a sanity check, and speak up if there are any questions or concerns?
- Julian
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Daniel Sahlberg <da...@gmail.com>.
Den ons 7 apr. 2021 kl 15:34 skrev Julian Foad <ju...@apache.org>:
> > Ack, and thanks for picking up on this omission. Does that all stack up
> now?
>
> I have committed it now, https://svn.apache.org/r1888474 , as I have just
> got to the point of a complete implementation and tests. I am going to
> nominate it for backport.
>
> That doesn't imply that I assume all your concerns are addressed. It can
> still be questioned and changed.
>
A bit late to the party and I must confess I don't really understand all
the implications but I've tried to follow the discussions best I can.
I'm not too fond of the "relocate workaround" described in JIRA. I would
have prefered to restore the behaviour of 1.6/1.7 which would solve case 1
and 2 but also change case 3 from inconvenience to "let's do a a
same-repository-merge between repositories that are not equivalent". I
can't judge the frequency of case 1/2 versus the frequency of case 3 and I
can't judge impact of an incorrect merge in case 3. I suppose you have done
the math and found that incorrect merges pose an unacceptable risk?
Would it make sense to have a command line option to force a behaviour of
1.6/1.7? Then the error message could point at this option and users who
have case 1/2 can take that option to perform the merge without the
relocate workaround?
Kind regards
Daniel Sahlberg
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Julian Foad <ju...@apache.org>.
Nominated for backport to 1.14.x. Would anyone be kind enough to review and/or test it?
--
- Julian
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Julian Foad <ju...@apache.org>.
> Ack, and thanks for picking up on this omission. Does that all stack up now?
I have committed it now, https://svn.apache.org/r1888474 , as I have just got to the point of a complete implementation and tests. I am going to nominate it for backport.
That doesn't imply that I assume all your concerns are addressed. It can still be questioned and changed.
--
- Julian
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Julian Foad wrote on Fri, 09 Apr 2021 20:20 +00:00:
> 1. I agree with softening this to be a warning rather than a hard error
> in a patch release, and then a hard error in 1.15. I hadn't thought of
> doing a preprocessor-based time-bomb; good idea. I will retract my
> backport until I have done that.
Sounds good to me.
> (If you would like a specific answer to any of your specific points,
> would you kindly ask it/them again. Just picking up on one specific
> question, "how would a warning be more of a hindrance than a hard
> error?": I was thinking of two things. One: without seeing a warning
> at the top the first thing the user might see could be the scroll-by
> stopping at conflicts, and the user might then waste a lot of time
> resolving them. Two: having to revert a merge is in some cases
> non-trivial, if we not making assumptions about best practices and
> clean simple scenarios.)
*nod*
Cheers,
Daniel
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Julian Foad <ju...@apache.org>.
Daniel Sahlberg wrote:
> Would this be something that should go in the "Most wanted" list in https://subversion.apache.org/roadmap.html?
Not in my opinion.
- Julian
Re: Missing pilcrows (was: Re: SVN-4874: Avoid foreign repository
merge if repository UUIDs) match
Posted by Nathan Hartman <ha...@gmail.com>.
On Wed, Aug 4, 2021 at 6:58 AM Bert Huijben <be...@qqmail.nl> wrote:
> Historic note:
> A long time ago we had to be very strict not to do this, as some major
> Subversion hosting provider (of which I don't know the name) just copied a
> blank repository to create new repositories. So all repositories hosted
> there had the same UUID.
> (And they couldn't just 'fix' this, as that would immediately break other
> working copies)
>
>
> I really hope this comment is now long outdated and we can just enable
> this....
>
Which comment are you referring to?
Nathan
Re: Missing pilcrows (was: Re: SVN-4874: Avoid foreign repository
merge if repository UUIDs) match
Posted by Bert Huijben <be...@qqmail.nl>.
Historic note:
A long time ago we had to be very strict not to do this, as some major
Subversion hosting provider (of which I don't know the name) just copied a
blank repository to create new repositories. So all repositories hosted
there had the same UUID.
(And they couldn't just 'fix' this, as that would immediately break other
working copies)
I really hope this comment is now long outdated and we can just enable
this....
Bert
On Mon, Apr 26, 2021 at 8:39 PM Daniel Shahaf <da...@apache.org> wrote:
> Thanks a lot for fixing these! And kudos for giving the case-by-case
> details ☺
>
Re: Missing pilcrows (was: Re: SVN-4874: Avoid foreign repository merge if repository UUIDs) match
Posted by Daniel Shahaf <da...@apache.org>.
Thanks a lot for fixing these! And kudos for giving the case-by-case details ☺
Re: Missing pilcrows (was: Re: SVN-4874: Avoid foreign repository
merge if repository UUIDs) match
Posted by Nathan Hartman <ha...@gmail.com>.
On Tue, Apr 20, 2021 at 5:02 PM Daniel Shahaf <da...@apache.org> wrote:
>
> Nathan Hartman wrote on Tue, Apr 20, 2021 at 16:07:58 -0400:
> > On Tue, Apr 20, 2021 at 3:29 PM Daniel Shahaf <da...@apache.org>
wrote:
> > > (Aside: those <h4/>'s don't have the self-link pilcrows that all
other <h{1..4}/>'s do.)
> >
> > Good catch! That's fixed in r1889033 now :-)
>
> Thank you!
>
> I have now looked for other instances of this omission:
Thanks!
> % ag -B1 '<h\d' | grep -xv -- '--' | vim - # and run
'g/class="h\d/.,+d' interactively
As you suspected, this does produce some false positives by triggering
on the blank line preceding a heading (as well as a few that aren't
heading tags). However the results are still useful as it did correctly
locate numerous actual missing pilcrows.
I left the h1's alone as they are page titles and therefore (I think)
they don't need section links.
Detail of changes:
> 8 roadmap.html:137:<h3>Transition to LTS and Regular Releases</h3>
> 176 docs/community-guide/issues.part.html:413:<h3>Security approach
in Subversion's Design and Implementation</h3>
> 178 docs/community-guide/issues.part.html:427:<h3>Handling reported
security vulnerabilities</h3>
> 180 docs/community-guide/issues.part.html:443:<h4>Mailing Lists</h4>
> 182 docs/community-guide/issues.part.html:451:<h4>Tracking Issues</h4>
> 184 docs/community-guide/issues.part.html:459:<h4>Procedures We
Follow</h4>
> 128 docs/release-notes/1.1.html:20:<h2 style="text-align:
center">(and State of the Project)</h2>
> 130 docs/release-notes/1.1.html:29:<h3>Overview</h3>
> 132 docs/release-notes/1.1.html:40:<h3>Compatibility Concerns</h3>
> 134 docs/release-notes/1.1.html:66:<h3>New Major Features</h3>
> 136 docs/release-notes/1.1.html:68:<h4>Non-database repositories
(<em>new server feature</em>)</h4>
> 138 docs/release-notes/1.1.html:95:<h4>Symlink versioning (<em>new
client feature</em>)</h4>
> 140 docs/release-notes/1.1.html:112:<h4>Client follows renames
(<em>new client feature</em>)</h4>
> 142 docs/release-notes/1.1.html:126:<h4>Command line auto-escaping of
URI and IRIs (<em>new client
> 144 docs/release-notes/1.1.html:146:<h4>Localized messages (<em>new
client feature</em>)</h4>
> 146 docs/release-notes/1.1.html:165:<h3>Other Improvements</h3>
> 148 docs/release-notes/1.1.html:167:<h4>Speed optimizations:
(<em>requires both new client and server</em>)</h4>
> 150 docs/release-notes/1.1.html:171:<h4>Shareable working copies:
(<em>client fix</em>)</h4>
> 152 docs/release-notes/1.1.html:179:<h4>New 'store-passwords' runtime
variable: (<em>new client feature</em>)</h4>
> 154 docs/release-notes/1.1.html:187:<h4>Bugfixes:</h4>
> 156 docs/release-notes/1.1.html:192:<h4>New subcommand switches:</h4>
> 158 docs/release-notes/1.1.html:239:<h3>Developer Changes</h3>
> 160 docs/release-notes/1.1.html:254:<h3>Testimonials and Roadmap</h3>
Fixed in r1889186 and r1889188.
> 93 security/index.html:36:<h2 id="advisories">Previous Security
Advisories
Fixed missing div around the section. This caused the heading, rather
than the section, to be highlighted when the section link was visited.
Detail of non-changes:
> 38 faq.html:29:<h4>General questions:</h4>
> 40 faq.html:68:<h4>How-to:</h4>
> 42 faq.html:152:<h4>Troubleshooting:</h4>
> 44 faq.html:263:<h4>Developer questions:</h4>
> 46 faq.html:275:<h4>References:</h4>
> 61 faq.html:4483:<h4>Troubleshooting:</h4>
> 63 faq.html:4491:<h4>BDB questions:</h4>
> 26 faq.zh.html:30:<h4>常见问题：</h4>
> 28 faq.zh.html:54:<h4>如何使用：</h4>
> 30 faq.zh.html:99:<h4>疑难解答：</h4>
> 32 faq.zh.html:158:<h4>开发者问题</h4>
> 34 faq.zh.html:165:<h4>说明:</h4>
> 67 faq.ja.html:30:<h4>General questions:</h4>
> 69 faq.ja.html:53:<h4>How-to:</h4>
> 71 faq.ja.html:95:<h4>Troubleshooting:</h4>
> 73 faq.ja.html:143:<h4>Developer questions:</h4>
> 75 faq.ja.html:150:<h4>References:</h4>
The above are part of the FAQ TOC, which has an explicit comment: "no
'id' or 'title' attribute for TOC" so I'm leaving them as-is.
> 48 faq.html:765:<h3>How is Subversion affected by SHA-1 hash
collisions?
> 121 docs/release-notes/1.10.html:1070:<h4>RA-serf now compresses
deltas when possible
False positive due to extra anchors for compatibility with older IDs.
> 50 faq.html:3958:<h3>When performing Subversion operations
> 52 faq.html:4004:<h3>After importing files to my repository,
> 54 faq.html:4042:<h3>When does <tt>svn copy</tt> create
<tt>svn:mergeinfo</tt> properties?
> 56 faq.html:4089:<h3> Passwords which contain some special
characters do not seem to be working?
> 58 faq.html:4115:<h3>Why does an HTTP(S) URL-to-URL copy or
branch/tag operation
> 60 faq.html:4154:<h3>When performing Subversion operations
False positives due to long title text or extra newline between div
and heading tags.
> 79 security/CVE-2015-3184-advisory.txt:843:+ <h2>repos - Revision 1:
/A/D</h2>
> 81 security/CVE-2015-3184-advisory.txt:853:+ <h2>repos - Revision 1:
/A/D</h2>
> 83 security/CVE-2015-3184-advisory.txt:864:+ <h2>repos - Revision 1:
/A/D/H</h2>
> 85 security/CVE-2015-3184-advisory.txt:2924:+ <h2>repos - Revision
1: /A/D</h2>
> 87 security/CVE-2015-3184-advisory.txt:2934:+ <h2>repos - Revision
1: /A/D</h2>
> 89 security/CVE-2015-3184-advisory.txt:2945:+ <h2>repos - Revision
1: /A/D/H</h2>
Text files (patches, actually).
Cheers,
Nathan
Missing pilcrows (was: Re: SVN-4874: Avoid foreign repository merge
if repository UUIDs) match
Posted by Daniel Shahaf <da...@apache.org>.
Nathan Hartman wrote on Tue, Apr 20, 2021 at 16:07:58 -0400:
> On Tue, Apr 20, 2021 at 3:29 PM Daniel Shahaf <da...@apache.org> wrote:
> > (Aside: those <h4/>'s don't have the self-link pilcrows that all other <h{1..4}/>'s do.)
>
> Good catch! That's fixed in r1889033 now :-)
Thank you!
I have now looked for other instances of this omission:
% ag -B1 '<h\d' | grep -xv -- '--' | vim - # and run 'g/class="h\d/.,+d' interactively
1 pronunciation/index.html:19-
2 pronunciation/index.html:20:<h1>How To Pronounce "Subversion"</h1>
3 contributing.html:17-
4 contributing.html:18:<h1>Getting Involved with Apache Subversion and the Community</h1>
5 roadmap.html:18-
6 roadmap.html:19:<h1>Apache Subversion Roadmap</h1>
7 roadmap.html:136-
8 roadmap.html:137:<h3>Transition to LTS and Regular Releases</h3>
9 news.html:17-
10 news.html:18:<h1>Apache Subversion News Archives</h1>
11 features.html:18-
12 features.html:19:<h1>Apache Subversion Features</h1>
13 download.html:17-
14 download.html:18:<h1>Download Source Code</h1>
15 opw.html:17-
16 opw.html:18:<h1>Apache Subversion and the Outreach Program for Women</h1>
17 index.html:17-
18 index.html:18:<h1>Apache<span class="ss">®</span> Subversion<span class="ss">®</span></h1>
19 dev/index.html:17-
20 dev/index.html:18:<h1>Resources for Developers</h1>
21 packages.html:23-
22 packages.html:24:<h1>Apache Subversion Binary Packages</h1>
23 faq.zh.html:17-
24 faq.zh.html:18:<h1>Subversion FAQ(常见问题解答)</h1>
25 faq.zh.html:29-</p>
26 faq.zh.html:30:<h4>常见问题：</h4>
27 faq.zh.html:53-
28 faq.zh.html:54:<h4>如何使用：</h4>
29 faq.zh.html:98-
30 faq.zh.html:99:<h4>疑难解答：</h4>
31 faq.zh.html:157-
32 faq.zh.html:158:<h4>开发者问题</h4>
33 faq.zh.html:164-
34 faq.zh.html:165:<h4>说明:</h4>
35 faq.html:17-
36 faq.html:18:<h1>Apache Subversion FAQ</h1>
37 faq.html:28-
38 faq.html:29:<h4>General questions:</h4>
39 faq.html:67-
40 faq.html:68:<h4>How-to:</h4>
41 faq.html:151-
42 faq.html:152:<h4>Troubleshooting:</h4>
43 faq.html:262-
44 faq.html:263:<h4>Developer questions:</h4>
45 faq.html:274-
46 faq.html:275:<h4>References:</h4>
47 faq.html:764- of the anchor -->
48 faq.html:765:<h3>How is Subversion affected by SHA-1 hash collisions?
49 faq.html:3957-
50 faq.html:3958:<h3>When performing Subversion operations
51 faq.html:4003-
52 faq.html:4004:<h3>After importing files to my repository,
53 faq.html:4041-
54 faq.html:4042:<h3>When does <tt>svn copy</tt> create <tt>svn:mergeinfo</tt> properties?
55 faq.html:4088-
56 faq.html:4089:<h3> Passwords which contain some special characters do not seem to be working?
57 faq.html:4114-
58 faq.html:4115:<h3>Why does an HTTP(S) URL-to-URL copy or branch/tag operation
59 faq.html:4153-
60 faq.html:4154:<h3>When performing Subversion operations
61 faq.html:4483:<h4>Troubleshooting:</h4>
62 faq.html:4490-
63 faq.html:4491:<h4>BDB questions:</h4>
64 faq.ja.html:17-
65 faq.ja.html:18:<h1>Subversion FAQ</h1>
66 faq.ja.html:29-
67 faq.ja.html:30:<h4>General questions:</h4>
68 faq.ja.html:52-
69 faq.ja.html:53:<h4>How-to:</h4>
70 faq.ja.html:94-
71 faq.ja.html:95:<h4>Troubleshooting:</h4>
72 faq.ja.html:142-
73 faq.ja.html:143:<h4>Developer questions:</h4>
74 faq.ja.html:149-
75 faq.ja.html:150:<h4>References:</h4>
76 mailing-lists.html:17-
77 mailing-lists.html:18:<h1>Apache Subversion Mailing Lists</h1>
78 security/CVE-2015-3184-advisory.txt:842-+<body>
79 security/CVE-2015-3184-advisory.txt:843:+ <h2>repos - Revision 1: /A/D</h2>
80 security/CVE-2015-3184-advisory.txt:852-+<body>
81 security/CVE-2015-3184-advisory.txt:853:+ <h2>repos - Revision 1: /A/D</h2>
82 security/CVE-2015-3184-advisory.txt:863-+<body>
83 security/CVE-2015-3184-advisory.txt:864:+ <h2>repos - Revision 1: /A/D/H</h2>
84 security/CVE-2015-3184-advisory.txt:2923-+<body>
85 security/CVE-2015-3184-advisory.txt:2924:+ <h2>repos - Revision 1: /A/D</h2>
86 security/CVE-2015-3184-advisory.txt:2933-+<body>
87 security/CVE-2015-3184-advisory.txt:2934:+ <h2>repos - Revision 1: /A/D</h2>
88 security/CVE-2015-3184-advisory.txt:2944-+<body>
89 security/CVE-2015-3184-advisory.txt:2945:+ <h2>repos - Revision 1: /A/D/H</h2>
90 security/index.html:17-
91 security/index.html:18:<h1>Apache Subversion Security</h1>
92 security/index.html:35-
93 security/index.html:36:<h2 id="advisories">Previous Security Advisories
94 source-code.html:17-
95 source-code.html:18:<h1>Source Code</h1>
96 quick-start.html:18-
97 quick-start.html:19:<h1>Apache Subversion: Quick Start</h1>
98 reporting-issues.html:17-
99 reporting-issues.html:18:<h1>Apache Subversion Issues</h1>
100 docs/release-notes/1.12.html:23-
101 docs/release-notes/1.12.html:24:<h1 style="text-align: center">Apache Subversion 1.12 Release Notes</h1>
102 docs/release-notes/1.2.html:18-
103 docs/release-notes/1.2.html:19:<h1 style="text-align: center">Subversion 1.2 Release Notes</h1>
104 docs/release-notes/1.14.html:23-
105 docs/release-notes/1.14.html:24:<h1 style="text-align: center">Apache Subversion 1.14 LTS Release Notes</h1>
106 docs/release-notes/1.6.html:18-
107 docs/release-notes/1.6.html:19:<h1 style="text-align: center">Apache Subversion 1.6 Release Notes</h1>
108 docs/release-notes/1.3.html:18-
109 docs/release-notes/1.3.html:19:<h1 style="text-align: center">Subversion 1.3 Release Notes</h1>
110 docs/release-notes/index.html:18-
111 docs/release-notes/index.html:19:<h1>Apache Subversion Releases</h1>
112 docs/release-notes/1.13.html:23-
113 docs/release-notes/1.13.html:24:<h1 style="text-align: center">Apache Subversion 1.13 Release Notes</h1>
114 docs/release-notes/1.6.zh.html:18-
115 docs/release-notes/1.6.zh.html:19:<h1 style="text-align: center">Subversion 1.6发布说明</h1>
116 docs/release-notes/release-history.html:17-
117 docs/release-notes/release-history.html:18:<h2>Subversion Release History</h2>
118 docs/release-notes/1.10.html:23-
119 docs/release-notes/1.10.html:24:<h1 style="text-align: center">Apache Subversion 1.10 Release Notes</h1>
120 docs/release-notes/1.10.html:1069-<a name="serf-svndiff1"></a>
121 docs/release-notes/1.10.html:1070:<h4>RA-serf now compresses deltas when possible
122 docs/release-notes/1.4.html:18-
123 docs/release-notes/1.4.html:19:<h1 style="text-align: center">Subversion 1.4 Release Notes</h1>
124 docs/release-notes/1.5.html:18-
125 docs/release-notes/1.5.html:19:<h1 style="text-align: center">Subversion 1.5 Release Notes</h1>
126 docs/release-notes/1.1.html:18-
127 docs/release-notes/1.1.html:19:<h1 style="text-align: center">Subversion 1.1 Release Notes</h1>
128 docs/release-notes/1.1.html:20:<h2 style="text-align: center">(and State of the Project)</h2>
129 docs/release-notes/1.1.html:28-
130 docs/release-notes/1.1.html:29:<h3>Overview</h3>
131 docs/release-notes/1.1.html:39-
132 docs/release-notes/1.1.html:40:<h3>Compatibility Concerns</h3>
133 docs/release-notes/1.1.html:65-
134 docs/release-notes/1.1.html:66:<h3>New Major Features</h3>
135 docs/release-notes/1.1.html:67-
136 docs/release-notes/1.1.html:68:<h4>Non-database repositories (<em>new server feature</em>)</h4>
137 docs/release-notes/1.1.html:94-
138 docs/release-notes/1.1.html:95:<h4>Symlink versioning (<em>new client feature</em>)</h4>
139 docs/release-notes/1.1.html:111-
140 docs/release-notes/1.1.html:112:<h4>Client follows renames (<em>new client feature</em>)</h4>
141 docs/release-notes/1.1.html:125-
142 docs/release-notes/1.1.html:126:<h4>Command line auto-escaping of URI and IRIs (<em>new client
143 docs/release-notes/1.1.html:145-
144 docs/release-notes/1.1.html:146:<h4>Localized messages (<em>new client feature</em>)</h4>
145 docs/release-notes/1.1.html:164-
146 docs/release-notes/1.1.html:165:<h3>Other Improvements</h3>
147 docs/release-notes/1.1.html:166-
148 docs/release-notes/1.1.html:167:<h4>Speed optimizations: (<em>requires both new client and server</em>)</h4>
149 docs/release-notes/1.1.html:170-
150 docs/release-notes/1.1.html:171:<h4>Shareable working copies: (<em>client fix</em>)</h4>
151 docs/release-notes/1.1.html:178-
152 docs/release-notes/1.1.html:179:<h4>New 'store-passwords' runtime variable: (<em>new client feature</em>)</h4>
153 docs/release-notes/1.1.html:186-
154 docs/release-notes/1.1.html:187:<h4>Bugfixes:</h4>
155 docs/release-notes/1.1.html:191-
156 docs/release-notes/1.1.html:192:<h4>New subcommand switches:</h4>
157 docs/release-notes/1.1.html:238-
158 docs/release-notes/1.1.html:239:<h3>Developer Changes</h3>
159 docs/release-notes/1.1.html:253-
160 docs/release-notes/1.1.html:254:<h3>Testimonials and Roadmap</h3>
161 docs/release-notes/1.11.html:23-
162 docs/release-notes/1.11.html:24:<h1 style="text-align: center">Apache Subversion 1.11 Release Notes</h1>
163 docs/release-notes/1.9.html:23-
164 docs/release-notes/1.9.html:24:<h1 style="text-align: center">Apache Subversion 1.9 Release Notes</h1>
165 docs/release-notes/1.8.html:18-
166 docs/release-notes/1.8.html:19:<h1 style="text-align: center">Apache Subversion 1.8 Release Notes</h1>
167 docs/release-notes/1.7.html:18-
168 docs/release-notes/1.7.html:19:<h1 style="text-align: center">Apache Subversion 1.7 Release Notes</h1>
169 docs/index.html:17-
170 docs/index.html:18:<h1>Apache Subversion Documentation</h1>
171 docs/community-guide/index.html:18-<!--#include virtual="guide-nav.html" -->
172 docs/community-guide/index.html:19:<h1>Apache Subversion Community Guide (aka "HACKING")</h1>
173 docs/community-guide/community-guide.html:28-
174 docs/community-guide/community-guide.html:29:<h1>Subversion Community Guide (aka "HACKING")</h1>
175 docs/community-guide/issues.part.html:412-
176 docs/community-guide/issues.part.html:413:<h3>Security approach in Subversion's Design and Implementation</h3>
177 docs/community-guide/issues.part.html:426-
178 docs/community-guide/issues.part.html:427:<h3>Handling reported security vulnerabilities</h3>
179 docs/community-guide/issues.part.html:442-
180 docs/community-guide/issues.part.html:443:<h4>Mailing Lists</h4>
181 docs/community-guide/issues.part.html:450-
182 docs/community-guide/issues.part.html:451:<h4>Tracking Issues</h4>
183 docs/community-guide/issues.part.html:458-
184 docs/community-guide/issues.part.html:459:<h4>Procedures We Follow</h4>
185 ideas.html:17-
186 ideas.html:18:<h1>Apache Subversion Project Ideas</h1>
%
Most of these seem like false positives, but if anyone wants to go
through the results (doesn't have to be Nathan, of course), feel free.
Cheers,
Daniel
[«.,+d» means "delete from the current line to the following one,
inclusive". Perl isn't the only language that sometimes resembles line
noise ;-)]
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Julian Foad <ju...@apache.org>.
Daniel Shahaf wrote:
> Release notes text for 1.15.html (or possibly 1.16.html)? An API
> erratum in notes/? Is either of these needed?
I don't think this needs an API erratum. We should pick up the change in the change log and perhaps release notes as part of the usual process. I don't think it needs a special call-out or extra description. Let me know if you think it does.
--
- Julian
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Daniel Shahaf <da...@apache.org>.
Julian Foad wrote on Tue, 20 Apr 2021 13:58 +00:00:
> This appears to complete the issue. Do say if I have missed something.
Release notes text for 1.15.html (or possibly 1.16.html)? An API
erratum in notes/? Is either of these needed?
Cheers,
Daniel
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Julian Foad <ju...@apache.org>.
Daniel Shahaf wrote:
> There's actually an svntest.main.SVN_VER_MINOR constant to test. [...]
Huh. So there is. All by itself with no _MAJOR sibling, and used only in relation to the server-minor-version test option.
Thank you. Done and tested (with 1.15-dev, and by changing the constant and the code to pretend it's 1.16). That's better.
--
- Julian
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Nathan Hartman <ha...@gmail.com>.
On Tue, Apr 20, 2021 at 3:29 PM Daniel Shahaf <da...@apache.org> wrote:
> (Aside: those <h4/>'s don't have the self-link pilcrows that all other <h{1..4}/>'s do.)
Good catch! That's fixed in r1889033 now :-)
I don't have specific input (yet) about the SVN-4874 changes but since
I'm running a trunk build as my "daily driver" client (to exercise
recent optimizations to 'checkout', 'update', and 'status' under real
usage) I'll rebuild with the latest and try the scenarios this intends
to correct...
Cheers,
Nathan
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Daniel Shahaf <da...@apache.org>.
Julian Foad wrote on Tue, 20 Apr 2021 13:58 +00:00:
> Julian Foad wrote on 2021-04-14:
> > If we are not looking at back-porting it to 1.14.x, then we could
> > [...]
> > - use the checks in libsvn_client; add a new 'notification' type for
> > the warnings
>
> That way was easiest and didn't require much boiler-plate code at all.
> Done in http://svn.apache.org/r1889012 .
>
> As explained in the comment in https://subversion.apache.org/issue/4874
> this changes the 1.14 behaviour to just add a warning in 1.15 and then
> become an error in 1.16.
>
> For 1.16 a small modification to the test expectations will need to be
> made, as I didn't find a simple way to make the expectation depend on
> the client version being tested. This will come up as a failure with an
> obvious fix, when we bump the trunk version to 1.16 after branching
> 1.15. If someone spots a way to automate this part too, do say.
There's actually an svntest.main.SVN_VER_MINOR constant to test. It's not parsed
from svn_version.h, but gets updated manually (in
https://subversion.apache.org/docs/community-guide/releasing.html#creating-branch;
see under "Manual Procedure", fourth bullet), but we can use it all the
same.
I'll leave it to you to decide whether to test the constant directly
or to encapsulate the check in a named has_foo() helper function, as
done for other similar checks.
(Aside: those <h4/>'s don't have the self-link pilcrows that all other <h{1..4}/>'s do.)
> This appears to complete the issue. Do say if I have missed something.
Haven't looked at r1889012 yet, but all the same, thanks for your usual
thoroughness on this.
Cheers,
Daniel
> --
> - Julian
>
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Julian Foad <ju...@apache.org>.
Julian Foad wrote on 2021-04-14:
> If we are not looking at back-porting it to 1.14.x, then we could
> [...]
> - use the checks in libsvn_client; add a new 'notification' type for
> the warnings
That way was easiest and didn't require much boiler-plate code at all. Done in http://svn.apache.org/r1889012 .
As explained in the comment in https://subversion.apache.org/issue/4874 this changes the 1.14 behaviour to just add a warning in 1.15 and then become an error in 1.16.
For 1.16 a small modification to the test expectations will need to be made, as I didn't find a simple way to make the expectation depend on the client version being tested. This will come up as a failure with an obvious fix, when we bump the trunk version to 1.16 after branching 1.15. If someone spots a way to automate this part too, do say.
This appears to complete the issue. Do say if I have missed something.
--
- Julian
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Julian Foad <ju...@apache.org>.
Daniel Shahaf wrote:
> > > #if (SVN_VER_MAJOR > 1 || SVN_VER_MINOR >= 16)
> > (in which I assume you meant MINOR >= 15)...
>
> I assumed it would be a warning in 1.15 and an error in 1.16.
Oh, OK. I was thinking nearer term (back-porting to 1.14.x as a warning, then error in 1.15), but I haven't any particular reason except to get on with it.
I have just confirmed that the back-portable way of adding a warning in the client layer does indeed need to open additional RA-sessions at the wire protocol level, at least for RA-svn. The patch I have prepared opens 4 extra RA-sessions, which I could reduce to 3 or 2 or maybe 1 extra for typical cases with a couple hours more work; and each additional RA-session takes maybe 2 network round-trips at a guess (I could find out of course). That's per "svn merge" invocation, varying a bit depending on what form of merge.
I know folks would be very reluctant to accept any added overhead.
If we are not looking at back-porting it to 1.14.x, then we could instead add the warnings in a way that doesn't add any additional RA-session opens, but requires bumping APIs, so a bunch of boiler-plate work. There are several potential approaches to choose from, such as (roughly ordered from easiest to best):
- use the checks in libsvn_client; add a new warnings callback
- use the checks in libsvn_client; add a new 'notification' type for the warnings
- add checks in the app layer; pass the open sessions through to new (bumped) merge APIs
- add checks in the app layer; add general RA session caching
(I wish we had addressed the technical debt that I perceive in the lack of RA session caching. I know some of us have looked into it before -- e.g. the 'reuse-ra-session' branch -- but it seems we didn't ever complete a solution.)
--
- Julian
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Daniel Shahaf <da...@apache.org>.
Julian Foad wrote on Wed, 14 Apr 2021 07:34 +00:00:
> Re. this suggestion to issue a warning:
> > #if (SVN_VER_MAJOR > 1 || SVN_VER_MINOR >= 16)
> > hard_error();
> > #else
> > warning();
> > #endif
> (in which I assume you meant MINOR >= 15)...
>
I assumed it would be a warning in 1.15 and an error in 1.16.
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Julian Foad <ju...@apache.org>.
Re. this suggestion to issue a warning:
> #if (SVN_VER_MAJOR > 1 || SVN_VER_MINOR >= 16)
> hard_error();
> #else
> warning();
> #endif
(in which I assume you meant MINOR >= 15)...
Meh. This is library code. We can't just issue a warning in-line. We don't have a generic warning callback. We can't add new notifications to the notifications system in a patch release.
To add warnings in a patch release we can do so at the application layer. That involves some code duplication but not much. It does require opening RA sessions to the two or three repository URLs -- or, with sufficient logic, at least one of them plus any others that are not trivially pointing to the same repository. These opened RA sessions cannot be passed through the public API into the merge APIs; they will open new sessions. I cannot remember how much caching of sessions our session-opening layer might do, and haven't yet measured it, so I am not yet sure of the impact.
I will continue looking into the impact of adding warnings this way. If the run-time cost is effectively zero due to session caching that's fine. If not, I'm wondering if it's worth it. An alternative is to leave 1.14.x unpatched -- no warning, no error. We could still make those errors in 1.15.0, with no pre-warning.
If anyone wants to comment or suggest alternatives while I look into it, please do.
--
- Julian
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Julian Foad <ju...@foad.me.uk>.
Daniel Shahaf wrote:
> Julian Foad wrote:
>> [...]. I wouldn't want to see
>> us introduce any half-measure, but only a full policy of "we
>> distinguish repositories by their UUID rather than any given URL, in
>> all cases" which should include non-merge cases such as diff as well,
>> and I think that is out of scope at the present time and investment
>> level in Subversion; it would be a "Subversion 2" wish list item if we
>> ever go there.
>
> FWIW, I don't immediately see why this is a milestone=2.0 matter. It
> may be possible to implement this backwards-compatibly.
True in principle. I meant in practice I don't expect to see us doing it (because of low cost:benefit ratio) except as part of some bigger rewrite project.
- Julian
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Daniel Shahaf <da...@apache.org>.
Daniel Sahlberg wrote on Sun, 11 Apr 2021 10:59 +00:00:
> Den fre 9 apr. 2021 kl 22:20 skrev Julian Foad <ju...@apache.org>:
> > Daniel and Daniel, thanks for your replies.
> >
> > Responding in summary, after taking all your points and questions into consideration.
> >
> > 1. I agree with softening this to be a warning rather than a hard error in a patch release, and then a hard error in 1.15. I hadn't thought of doing a preprocessor-based time-bomb; good idea. I will retract my backport until I have done that.
> >
> > 2. Returning to 1.6/1.7 behaviour is not something to be aspired to. Never mind the reversion of the source:target comparison in 1.8 being accidental; at least in part, the omission of sufficient URL comparisons in the first place was accidental (the source1:source2 part), and even if some of the possible combinations of inputs "worked" in 1.6/1.7 we can agree for sure that the set of possible combinations is not well tested. 1.6/1.7 did not cleanly implement a policy of "URLs can differ as long as they point to the same or equivalent repositories". Rather it was, URLs can differ in the source:target inputs (but not the source1:source2 inputs) to certain merge entry points, in the sense that we don't reject this, and it may even partly or fully work with merge tracking merges, but we are not sure and nothing is tested except "foreign merge" cases. I wouldn't want to see us introduce any half-measure, but only a full policy of "we distinguish repositories by their UUID rather than any given URL, in all cases" which should include non-merge cases such as diff as well, and I think that is out of scope at the present time and investment level in Subversion; it would be a "Subversion 2" wish list item if we ever go there.
FWIW, I don't immediately see why this is a milestone=2.0 matter. It
may be possible to implement this backwards-compatibly.
> It seems to me that you have thought this through and I'm not the one
> to question your judgement.
>
> Would this be something that should go in the "Most wanted" list in
> https://subversion.apache.org/roadmap.html?
+1 to Julian's answer.
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Daniel Sahlberg <da...@gmail.com>.
Den fre 9 apr. 2021 kl 22:20 skrev Julian Foad <ju...@apache.org>:
> Daniel and Daniel, thanks for your replies.
>
> Responding in summary, after taking all your points and questions into
> consideration.
>
> 1. I agree with softening this to be a warning rather than a hard error in
> a patch release, and then a hard error in 1.15. I hadn't thought of doing a
> preprocessor-based time-bomb; good idea. I will retract my backport until I
> have done that.
>
> 2. Returning to 1.6/1.7 behaviour is not something to be aspired to. Never
> mind the reversion of the source:target comparison in 1.8 being accidental;
> at least in part, the omission of sufficient URL comparisons in the first
> place was accidental (the source1:source2 part), and even if some of the
> possible combinations of inputs "worked" in 1.6/1.7 we can agree for sure
> that the set of possible combinations is not well tested. 1.6/1.7 did not
> cleanly implement a policy of "URLs can differ as long as they point to the
> same or equivalent repositories". Rather it was, URLs can differ in the
> source:target inputs (but not the source1:source2 inputs) to certain merge
> entry points, in the sense that we don't reject this, and it may even
> partly or fully work with merge tracking merges, but we are not sure and
> nothing is tested except "foreign merge" cases. I wouldn't want to see us
> introduce any half-measure, but only a full policy of "we distinguish
> repositories by their UUID rather than any given URL, in all cases" which
> should include non-merge cases such as diff as well, and I think that is
> out of scope at the present time and investment level in Subversion; it
> would be a "Subversion 2" wish list item if we ever go there.
>
It seems to me that you have thought this through and I'm not the one to
question your judgement.
Would this be something that should go in the "Most wanted" list in
https://subversion.apache.org/roadmap.html?
Kind regards,
Daniel
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Julian Foad <ju...@apache.org>.
Daniel and Daniel, thanks for your replies.
Responding in summary, after taking all your points and questions into consideration.
1. I agree with softening this to be a warning rather than a hard error in a patch release, and then a hard error in 1.15. I hadn't thought of doing a preprocessor-based time-bomb; good idea. I will retract my backport until I have done that.
2. Returning to 1.6/1.7 behaviour is not something to be aspired to. Never mind the reversion of the source:target comparison in 1.8 being accidental; at least in part, the omission of sufficient URL comparisons in the first place was accidental (the source1:source2 part), and even if some of the possible combinations of inputs "worked" in 1.6/1.7 we can agree for sure that the set of possible combinations is not well tested. 1.6/1.7 did not cleanly implement a policy of "URLs can differ as long as they point to the same or equivalent repositories". Rather it was, URLs can differ in the source:target inputs (but not the source1:source2 inputs) to certain merge entry points, in the sense that we don't reject this, and it may even partly or fully work with merge tracking merges, but we are not sure and nothing is tested except "foreign merge" cases. I wouldn't want to see us introduce any half-measure, but only a full policy of "we distinguish repositories by their UUID rather than any given URL, in all cases" which should include non-merge cases such as diff as well, and I think that is out of scope at the present time and investment level in Subversion; it would be a "Subversion 2" wish list item if we ever go there.
(If you would like a specific answer to any of your specific points, would you kindly ask it/them again. Just picking up on one specific question, "how would a warning be more of a hindrance than a hard error?": I was thinking of two things. One: without seeing a warning at the top the first thing the user might see could be the scroll-by stopping at conflicts, and the user might then waste a lot of time resolving them. Two: having to revert a merge is in some cases non-trivial, if we not making assumptions about best practices and clean simple scenarios.)
- Julian
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs
match
Posted by Daniel Shahaf <da...@apache.org>.
Julian Foad wrote on Wed, Apr 07, 2021 at 11:33:35 +0100:
> Daniel Shahaf wrote:
> > Julian Foad wrote:
> > > If we warn [...] it also has Bad properties:
> > > supporting misleading assumptions about how other variations of the
> > > scenario might behave.
> >
> > Could you be more concrete about those assumptions?
>
> It's not the warning itself but that continuing to behave illogically would support bad assumptions. One example: if the user is accustomed to the foreign-repo merge behaviour (despite same UUIDs) using a script that says (x = repository root URL path)
> source1=http://x source2=https://x target=https://x
> then they might assume that "correcting" source1 in their script to say
> source1=https://x source2=https://x target=https://x
> would keep the same behaviour; but it doesn't.
>
> Of course a warning could try to explain sufficiently but the current behaviour is not self-consistent enough to explain in one short sentence.
Not in one sentence, but we aren't limited to one sentence. I did
propose example text in my previous reply.
> > [...] we can phrase the
> > warning clearly enough that anyone who runs into it unintentionally will
> > abort the merge and re-do it "correctly", without us needing to enforce
> > it by issueing a fatal error.
> [...]
> > I don't disagree with classifying this as a bug, but I still don't
> > understand why issueing a warning wouldn't be satisfactory — or, at
> > least, have 1.15 issue a warning and 1.16 upgrade that to a fatal error,
> > just to be on the safe side. See the example output above, line 4.
>
> That's a good strategy for dealing with cases in general where a
> behaviour one "supported" in practice needs to be withdrawn. If this
> were an issue in a more main-stream merge case, I would agree.
> However, the proposed benefit that we offer by continuing the merge is
> targetting the wrong group: it is a supposed convenience to users
> whose UUIDs are misconfigured, while it would be a hindrance to users
> who run into it by accident while having their UUIDs correctly
> configured.
I don't see how getting a warning would be more of a hindrance than
getting a hard error. Run some command, get a warning, read it, thank
the tool for catching your error, correct your error. Happens every day
with other tools, and even with svn(1)'s use of svn_cl__similarity_check().
How would getting a warning be more of a hindrance than getting a hard
error? Is it because the user might do the merge before reading the
error message (using up bandwidth and CPU), or because the user would be
presented with a decision to make, or what?
> Also, this is so much an edge case that it isn't worth
> the extra complexity on our side; we don't have a great track record
> of getting around to doing things that we scheduled for a later
> release, for instance.
Actually, I thought we could implement this by committing the code up
front with a preprocessor-based time bomb:
#if (SVN_VER_MAJOR > 1 || SVN_VER_MINOR >= 16)
hard_error();
#else
warning();
#endif
> Handling it so gently for the affected users is a nicety that doesn't
> feel worth it.
Just erring on the side of caution. For affected users, the impact of
releasing a hard error would be more significant than the impact of
releasing a warning (with or without upgrading it later to a hard
error); and we don't know how many users would be affected.
The impact would be doubly notable if the change is made in a patch
release, and triply if the patch release will also happen to feature
a fix to a vulnerability (nowadays that we don't do separate releases
for such).
> I think the reasonable use cases for the case in question and the
> likely number of occurrences in practice are both near zero. I don't
> have numbers to back up this feeling, but as far as I am aware nobody
> has reported this issue and it has existed for many years.
I assume that by "occurrences" you refer to people who invoke this
syntax unintentionally.
> > By the way, I don't feel strongly about this matter; it simply happens
> > to be the one place in the write-up of the issue where I couldn't see
> > why an alternative design was ruled out.
>
> Ack, and thanks for picking up on this omission. Does that all stack up now?
Not entirely, I'm afraid. Anyway, I don't feel strongly about 1.15.0,
but I'm not too comfortable with converting the foreign merge to a hard
error in a patch release. Even if doing a foreign merge on equal UUIDs
is wrong, I don't think it's so wrong that we can tell people they
should have realized on their own that they shouldn't be relying on this
behaviour.
Cheers,
Daniel
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Julian Foad <ju...@apache.org>.
Daniel Shahaf wrote:
> Julian Foad wrote:
> > If we warn [...] it also has Bad properties:
> > supporting misleading assumptions about how other variations of the
> > scenario might behave.
>
> Could you be more concrete about those assumptions?
It's not the warning itself but that continuing to behave illogically would support bad assumptions. One example: if the user is accustomed to the foreign-repo merge behaviour (despite same UUIDs) using a script that says (x = repository root URL path)
source1=http://x source2=https://x target=https://x
then they might assume that "correcting" source1 in their script to say
source1=https://x source2=https://x target=https://x
would keep the same behaviour; but it doesn't.
Of course a warning could try to explain sufficiently but the current behaviour is not self-consistent enough to explain in one short sentence.
> The negatives that would be waiting in a hypothetical release that
> issues warnings as proposed aren't that bad, and certainly not as bad
> as those in the current release, are they?
Not as bad as the current release, of course.
> [...] we can phrase the
> warning clearly enough that anyone who runs into it unintentionally will
> abort the merge and re-do it "correctly", without us needing to enforce
> it by issueing a fatal error.
[...]
> I don't disagree with classifying this as a bug, but I still don't
> understand why issueing a warning wouldn't be satisfactory — or, at
> least, have 1.15 issue a warning and 1.16 upgrade that to a fatal error,
> just to be on the safe side. See the example output above, line 4.
That's a good strategy for dealing with cases in general where a behaviour one "supported" in practice needs to be withdrawn. If this were an issue in a more main-stream merge case, I would agree. However, the proposed benefit that we offer by continuing the merge is targetting the wrong group: it is a supposed convenience to users whose UUIDs are misconfigured, while it would be a hindrance to users who run into it by accident while having their UUIDs correctly configured. Also, this is so much an edge case that it isn't worth the extra complexity on our side; we don't have a great track record of getting around to doing things that we scheduled for a later release, for instance. Handling it so gently for the affected users is a nicety that doesn't feel worth it. I think the reasonable use cases for the case in question and the likely number of occurrences in practice are both near zero. I don't have numbers to back up this feeling, but as far as I am aware nobody has reported this issue and it has existed for many years.
> By the way, I don't feel strongly about this matter; it simply happens
> to be the one place in the write-up of the issue where I couldn't see
> why an alternative design was ruled out.
Ack, and thanks for picking up on this omission. Does that all stack up now?
--
- Julian
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Daniel Shahaf <da...@apache.org>.
Julian Foad wrote on Tue, 06 Apr 2021 09:12 +00:00:
> Daniel Shahaf wrote:
> > The write-up doesn't seem to consider the possibility of issuing
> > a warning rather than a fatal error. This would negate the "does not
> > introduce a silent behavioural change" argument and not constitute
> > a flag day to anyone who may be "intentionally or unintentionally relying
> > on the current behaviour". Thoughts?
>
> Good question. TL;DR: The behaviour is a bug not a feature.
>
> The case in question is where someone is running a merge with differing
> source and target (repository-root) URLs, and getting the "foreign
> repository" kind of merge result, but their "foreign repository" has
> the same UUID. The UUIDs indicate these URLs point to logically the
> same repository and so a "foreign repository" merge is the wrong
> behaviour. If a user in this situation is getting results that "work"
> for them it is only by accident. This is an exceptional case that we
> reasonably assume to be rare.
>
> If we warn instead of error in this exceptional case, then in this case
> it would continue to perform the "foreign repository" merge. While
> that may be convenient for anyone currently using this scenario (they
> would not have to change anything), it also has Bad properties:
> supporting misleading assumptions about how other variations of the
> scenario might behave.
Could you be more concrete about those assumptions?
> But the convenience is only useful for users in this exceptional case,
> whereas the negatives are there waiting for everybody who may some day
> be caught out when they use a different variant of their URL.
>
The negatives that would be waiting in a hypothetical release that
issues warnings as proposed aren't that bad, and certainly not as bad
as those in the current release, are they? Imagine a user running into
the warning:
% svn merge https://svn-eu.apache.org/repos/asf/subversion/branches/foo
svn: warning: W000042: Performing a foreign-repository merge, despite the merge source looking like a non-foreign repository
svn: warning: W000042: To perform a non-foreign merge, revert the results and re-run the merge against the URL 'https://svn.apache.org/repos/asf/subversion/branches/foo'; see https://subversion.apache.org/faq#foo for details
svn: warning: W000042: This warning will become a fatal error in Subversion 1.16.0
M CHANGES
⋮
%
Never mind the specific wording; the point is that we can phrase the
warning clearly enough that anyone who runs into it unintentionally will
abort the merge and re-do it "correctly", without us needing to enforce
it by issueing a fatal error.
> In other words, continuing to support the current behaviour is a bug.
>
> When filing the issue I couldn't quite decide whether it was a bug or
> an enhancement (and started filing it as a bug at first, then finally
> filed it as an enhancement), but after thinking through all this I
> conclude it's a bug (and I've just changed the issue type to reflect
> that).
I don't disagree with classifying this as a bug, but I still don't
understand why issueing a warning wouldn't be satisfactory — or, at
least, have 1.15 issue a warning and 1.16 upgrade that to a fatal error,
just to be on the safe side. See the example output above, line 4.
By the way, I don't feel strongly about this matter; it simply happens
to be the one place in the write-up of the issue where I couldn't see
why an alternative design was ruled out.
Cheers,
Daniel
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Julian Foad <ju...@apache.org>.
Daniel Shahaf wrote:
> The write-up doesn't seem to consider the possibility of issuing
> a warning rather than a fatal error. This would negate the "does not
> introduce a silent behavioural change" argument and not constitute
> a flag day to anyone who may be "intentionally or unintentionally relying
> on the current behaviour". Thoughts?
Good question. TL;DR: The behaviour is a bug not a feature.
The case in question is where someone is running a merge with differing source and target (repository-root) URLs, and getting the "foreign repository" kind of merge result, but their "foreign repository" has the same UUID. The UUIDs indicate these URLs point to logically the same repository and so a "foreign repository" merge is the wrong behaviour. If a user in this situation is getting results that "work" for them it is only by accident. This is an exceptional case that we reasonably assume to be rare.
If we warn instead of error in this exceptional case, then in this case it would continue to perform the "foreign repository" merge. While that may be convenient for anyone currently using this scenario (they would not have to change anything), it also has Bad properties: supporting misleading assumptions about how other variations of the scenario might behave. But the convenience is only useful for users in this exceptional case, whereas the negatives are there waiting for everybody who may some day be caught out when they use a different variant of their URL.
In other words, continuing to support the current behaviour is a bug.
When filing the issue I couldn't quite decide whether it was a bug or an enhancement (and started filing it as a bug at first, then finally filed it as an enhancement), but after thinking through all this I conclude it's a bug (and I've just changed the issue type to reflect that).
--
- Julian
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Posted by Daniel Shahaf <da...@apache.org>.
Julian Foad wrote on Fri, 02 Apr 2021 19:02 +00:00:
> Daniel Shahaf wrote:
> > Are you looking for feedback on the write-up and/or patch? I assume
> > not since you haven't mentioned any of this on dev@.
>
> I would be glad of any feedback. I was going to wait until I had a
> patch ready for review. Now there is a patch attached to the issue,
> and, though it really wants more tests, now could be a good time.
>
> So: could anyone cast an eye over this for a sanity check, and speak up
> if there are any questions or concerns?
The write-up doesn't seem to consider the possibility of issuing
a warning rather than a fatal error. This would negate the "does not
introduce a silent behavioural change" argument and not constitute
a flag day to anyone who may be "intentionally or unintentionally relying
on the current behaviour". Thoughts?
Cheers,
Daniel