Re: Missing pilcrows (was: Re: SVN-4874: Avoid foreign repository merge if repository UUIDs) match
On Wed, Aug 4, 2021 at 6:58 AM Bert Huijben 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
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 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
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
On Tue, Apr 20, 2021 at 5:02 PM Daniel Shahaf 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 wrote: > > > (Aside: those 's don't have the self-link pilcrows that all other '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 '(and State of the Project) >130 docs/release-notes/1.1.html:29:Overview >132 docs/release-notes/1.1.html:40:Compatibility Concerns >134 docs/release-notes/1.1.html:66:New Major Features >136 docs/release-notes/1.1.html:68:Non-database repositories (new server feature) >138 docs/release-notes/1.1.html:95:Symlink versioning (new client feature) >140 docs/release-notes/1.1.html:112:Client follows renames (new client feature) >142 docs/release-notes/1.1.html:126:Command line auto-escaping of URI and IRIs (new client >144 docs/release-notes/1.1.html:146:Localized messages (new client feature) >146 docs/release-notes/1.1.html:165:Other Improvements >148 docs/release-notes/1.1.html:167:Speed optimizations: (requires both new client and server) >150 docs/release-notes/1.1.html:171:Shareable working copies: (client fix) >152 docs/release-notes/1.1.html:179:New 'store-passwords' runtime variable: (new client feature) >154 docs/release-notes/1.1.html:187:Bugfixes: >156 docs/release-notes/1.1.html:192:New subcommand switches: >158 docs/release-notes/1.1.html:239:Developer Changes >160 docs/release-notes/1.1.html:254:Testimonials and Roadmap Fixed in r1889186 and r1889188. > 93 security/index.html:36: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:General questions: > 40 faq.html:68:How-to: > 42 faq.html:152:Troubleshooting: > 44 faq.html:263:Developer questions: > 46 faq.html:275:References: > 61 faq.html:4483:Troubleshooting: > 63 faq.html:4491:BDB questions: > 26 faq.zh.html:30:常见问题： > 28 faq.zh.html:54:如何使用： > 30 faq.zh.html:99:疑难解答： > 32 faq.zh.html:158:开发者问题 > 34 faq.zh.html:165:说明: > 67 faq.ja.html:30:General questions: > 69 faq.ja.html:53:How-to: > 71 faq.ja.html:95:Troubleshooting: > 73 faq.ja.html:143:Developer questions: > 75 faq.ja.html:150:References: 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:How is Subversion affected by SHA-1 hash collisions? >121 docs/release-notes/1.10.html:1070:RA-serf now compresses deltas when possible False positive due to extra anchors for compatibility with older IDs. > 50 faq.html:3958:When performing Subversion operations > 52 faq.html:4004:After importing files to my repository, > 54 faq.html:4042:When does svn copy create svn:mergeinfo properties? > 56 faq.html:4089: Passwords which contain some special characters do not seem to be working? > 58 faq.html:4115:Why does an HTTP(S) URL-to-URL copy or branch/tag operation > 60 faq.html:4154: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:+ repos - Revision 1: /A/D > 81 security/CVE-2015-3184-advisory.txt:853:+ repos - Revision 1: /A/D > 83 security/CVE-2015-3184-advisory.txt:864:+ repos - Revision 1: /A/D/H > 85 security/CVE-2015-3184-advisory.txt:2924:+ repos - Revision 1: /A/D > 87 security/CVE-2015-3184-advisory.txt:2934:+ repos - Revision 1: /A/D > 89 security/CVE-2015-3184-advisory.txt:2945:+ repos - Revision 1: /A/D/H Text files (patches, actually). Cheers, Nathan
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
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
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
Missing pilcrows (was: Re: SVN-4874: Avoid foreign repository merge if repository UUIDs) match
Nathan Hartman wrote on Tue, Apr 20, 2021 at 16:07:58 -0400: > On Tue, Apr 20, 2021 at 3:29 PM Daniel Shahaf wrote: > > (Aside: those 's don't have the self-link pilcrows that all other > > 's do.) > > Good catch! That's fixed in r1889033 now :-) Thank you! I have now looked for other instances of this omission: % ag -B1 ' 3 contributing.html:17- 4 contributing.html:18:Getting Involved with Apache Subversion and the Community 5 roadmap.html:18- 6 roadmap.html:19:Apache Subversion Roadmap 7 roadmap.html:136- 8 roadmap.html:137:Transition to LTS and Regular Releases 9 news.html:17- 10 news.html:18:Apache Subversion News Archives 11 features.html:18- 12 features.html:19:Apache Subversion Features 13 download.html:17- 14 download.html:18:Download Source Code 15 opw.html:17- 16 opw.html:18:Apache Subversion and the Outreach Program for Women 17 index.html:17- 18 index.html:18:Apache® Subversion® 19 dev/index.html:17- 20 dev/index.html:18:Resources for Developers 21 packages.html:23- 22 packages.html:24:Apache Subversion Binary Packages 23 faq.zh.html:17- 24 faq.zh.html:18:Subversion FAQ(常见问题解答) 25 faq.zh.html:29- 26 faq.zh.html:30:常见问题： 27 faq.zh.html:53- 28 faq.zh.html:54:如何使用： 29 faq.zh.html:98- 30 faq.zh.html:99:疑难解答： 31 faq.zh.html:157- 32 faq.zh.html:158:开发者问题 33 faq.zh.html:164- 34 faq.zh.html:165:说明: 35 faq.html:17- 36 faq.html:18:Apache Subversion FAQ 37 faq.html:28- 38 faq.html:29:General questions: 39 faq.html:67- 40 faq.html:68:How-to: 41 faq.html:151- 42 faq.html:152:Troubleshooting: 43 faq.html:262- 44 faq.html:263:Developer questions: 45 faq.html:274- 46 faq.html:275:References: 47 faq.html:764- of the anchor --> 48 faq.html:765:How is Subversion affected by SHA-1 hash collisions? 49 faq.html:3957- 50 faq.html:3958:When performing Subversion operations 51 faq.html:4003- 52 faq.html:4004:After importing files to my repository, 53 faq.html:4041- 54 faq.html:4042:When does svn copy create svn:mergeinfo properties? 55 faq.html:4088- 56 faq.html:4089: Passwords which contain some special characters do not seem to be working? 57 faq.html:4114- 58 faq.html:4115:Why does an HTTP(S) URL-to-URL copy or branch/tag operation 59 faq.html:4153- 60 faq.html:4154:When performing Subversion operations 61 faq.html:4483:Troubleshooting: 62 faq.html:4490- 63 faq.html:4491:BDB questions: 64 faq.ja.html:17- 65 faq.ja.html:18:Subversion FAQ 66 faq.ja.html:29- 67 faq.ja.html:30:General questions: 68 faq.ja.html:52- 69 faq.ja.html:53:How-to: 70 faq.ja.html:94- 71 faq.ja.html:95:Troubleshooting: 72 faq.ja.html:142- 73 faq.ja.html:143:Developer questions: 74 faq.ja.html:149- 75 faq.ja.html:150:References: 76 mailing-lists.html:17- 77 mailing-lists.html:18:Apache Subversion Mailing Lists 78 security/CVE-2015-3184-advisory.txt:842-+ 79 security/CVE-2015-3184-advisory.txt:843:+ repos - Revision 1: /A/D 80 security/CVE-2015-3184-advisory.txt:852-+ 81 security/CVE-2015-3184-advisory.txt:853:+ repos - Revision 1: /A/D 82 security/CVE-2015-3184-advisory.txt:863-+ 83 security/CVE-2015-3184-advisory.txt:864:+ repos - Revision 1: /A/D/H 84 security/CVE-2015-3184-advisory.txt:2923-+ 85 security/CVE-2015-3184-advisory.txt:2924:+ repos - Revision 1: /A/D 86 security/CVE-2015-3184-advisory.txt:2933-+ 87 security/CVE-2015-3184-advisory.txt:2934:+ repos - Revision 1: /A/D 88 security/CVE-2015-3184-advisory.txt:2944-+ 89 security/CVE-2015-3184-advisory.txt:2945:+ repos - Revision 1: /A/D/H 90 security/index.html:17- 91 security/index.html:18:Apache Subversion Security 92 security/index.html:35- 93 security/index.html:36:Previous Security Advisories 94 source-code.html:17- 95 source-code.html:18:Source Code 96 quick-start.html:18- 97 quick-start.html:19:Apache Subversion: Quick Start 98 reporting-issues.html:17- 99 reporting-issues.html:18:Apache Subversion Issues 100 docs/release-notes/1.12.html:23- 101 docs/release-notes/1.12.html:24:Apache Subversion 1.12 Release Notes 102 docs/release-notes/1.2.html:18- 103 docs/release-notes/1.2.html:19:Subversion 1.2 Release Notes 104 docs/release-notes/1.14.html:23- 105 docs/release-notes/1.14.html:24:Apache Subversion 1.14 LTS Release Notes 106 docs/release-notes/1.6.html:18- 107 docs/release-notes/1.6.html:19:Apache Subversion 1.6 Release Notes 108 docs/release-notes/1.3.html:18- 109 docs/release-notes/1.3.html:19:Subversion 1.3 Release Notes 110 docs/release-notes/index.html:18- 111 docs/release-notes/index.html:19:A
Re: SVN-4874: Avoid foreign repository merge if repository UUIDs match
On Tue, Apr 20, 2021 at 3:29 PM Daniel Shahaf wrote: > (Aside: those 's don't have the self-link pilcrows that all other > '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
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
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 's don't have the self-link pilcrows that all other '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
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
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
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
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
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
Daniel Sahlberg wrote on Sun, 11 Apr 2021 10:59 +00:00: > Den fre 9 apr. 2021 kl 22:20 skrev Julian Foad : > > 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
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: SVN-4874: Avoid foreign repository merge if repository UUIDs match
Den fre 9 apr. 2021 kl 22:20 skrev Julian Foad : > 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
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
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
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
Den ons 7 apr. 2021 kl 15:34 skrev Julian Foad : > > 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
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
> 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
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
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: W42: Performing a foreign-repository merge, despite the merge source looking like a non-foreign repository svn: warning: W42: 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: W42: 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
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
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
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
