Mailing-List: contact commits-help@subversion.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@subversion.apache.org
Received-SPF: pass (athena.apache.org: domain of ivan@visualsvn.com designates
 209.85.216.173 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <CA+t0gk3sBAZ2Gy7ad7HuVy070sEPQCs1dTFnF_u9b2Vnn=O2Ag@mail.gmail.com>
References: <20140621151532.86D5C2388980@eris.apache.org>
 <CABw-3YchZe-G4Y35eDm4F=M76Scbf-krpeTU_DCr-yUx3iV3ow@mail.gmail.com>
 <CA+t0gk2Jw4KswoiNpNhHHnxHHdvQ+6Qb-3=TWmC-FpSrjdmnGQ@mail.gmail.com>
 <CABw-3Yfd8cv5PuTrHz-95WuhbPcrVRhPeqy-aLx8yBH8kr6Z3w@mail.gmail.com>
 <CA+t0gk16XiJyqudN5huF0bMGLfMHavaX-ej9QuK3jZQBO=XYKg@mail.gmail.com>
 <CABw-3YfOoA4z_yUQC2=ChnGX3hV9fdLPNF3Jm1z3+U943jBNbw@mail.gmail.com>
 <20140625181139.GE1843@tarsus.local2>
 <CABw-3YeBUdsF7nsFDfumDruuPnFQCGXKEg9mmxEft8JCyCR=vg@mail.gmail.com>
 <CA+t0gk3n6zH-go7mvEbOXOrX6Li05LCz-vOc9zqTEnyt6O_7CA@mail.gmail.com>
 <CABw-3YfQZ2WOmbwXGLimEGxmaZ9-pjde=bTx0i4buOhL4iwFvQ@mail.gmail.com>
 <CA+t0gk3sBAZ2Gy7ad7HuVy070sEPQCs1dTFnF_u9b2Vnn=O2Ag@mail.gmail.com>
From: Ivan Zhakov <ivan@visualsvn.com>
Date: Fri, 4 Jul 2014 20:29:07 +0400
Message-ID: 
 <CABw-3Yep9p9crQpCAM5yObva-0DdzeifwdrjTNm8QuzaZb+65w@mail.gmail.com>
Subject: Re: svn commit: r1604421 - in /subversion/trunk:
 subversion/libsvn_fs_fs/
 subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/
 tools/server-side/svnfsfs/
To: Stefan Fuhrmann <stefan.fuhrmann@wandisco.com>
Cc: Daniel Shahaf <d.s@daniel.shahaf.name>,
	Subversion Development <dev@subversion.apache.org>,
	"commits@subversion.apache.org" <commits@subversion.apache.org>
Content-Type: multipart/mixed; boundary=001a11c2e8f026b85804fd60a1e0

--001a11c2e8f026b85804fd60a1e0
Content-Type: text/plain; charset=UTF-8

On 30 June 2014 14:19, Stefan Fuhrmann <stefan.fuhrmann@wandisco.com> wrote:
>
> On Fri, Jun 27, 2014 at 12:55 PM, Ivan Zhakov <ivan@visualsvn.com> wrote:
>>
>> On 26 June 2014 19:08, Stefan Fuhrmann <stefan.fuhrmann@wandisco.com>
>> wrote:
>> >
>> > Hi Ivan,
>> >
>> > I see three alternative ways to code that function
>> >
>> > 1. As hard coded string / byte sequence (current implementation).
>> >   Cons:
>> > * Hard to write, hard to review by just looking at it (applies to time
>> >   until initial release only).
>> >   Pros:
>> > * Explicitly coded as constant, deterring people from changing it.
>> > * Independent of other code, i.e. unintended changes to the format /
>> >   encoding generated by the normal code usually become apparent
>> >   when running the test suite.
>> >
>> > 2. Use our txn code to write r0. This should be simple and might
>> >   at most require some special ID handling.
>> >   Cons:
>> > * Generating incompatible r0s is likely not caught by our test suite
>> >   (assuming that reader and writer functions are in sync). Basically
>> >   all the risk that comes with dynamically generating a "constant".
>> > * Test cases must make sure our backward compat repo creation
>> >   options create repos that can actually be used by old releases.
>> >   (we might want explicit test for that anyway, though)
>> >   Pros:
>> > * No or very little special code for r0 (not sure, yet).
>> > * Format / encoding changes don't require new r0 templates.
>> >
>> > 3. Write code to "stitch" r0 together, e.g. string_add(md5("END\n")).
>> >   Cons:
>> > * No more robust than 1. but requires much more code.
>> > * May _look_ easy to understand but an actual offline review is still
>> > hard.
>> >   Pros:
>> > * Widely independent of other code, although not as much as 1.
>> >
>> > Do you generally agree with the range of options and their assessment?
>> Yes, I generally agree with range of options. The only other I have is
>> do not implement log addressing in first place.
>>
>> > Which one would you pick and why?
>> >
>> It's hard to pick option without looking to code, but I would start
>> with leaving string constant for revision body and then appending
>> indexes data using API. I.e. somewhat modified (2).
>
>
> r1606554 generates the index data dynamically now.
>
> It makes repo creation slightly more expensive but that
> does not seem to affect our test suite in any significant
> way. So, I think that is not an issue.
>
> Are you o.k. with the code as it stands now?
>
Now code is much better, but still not good as it could be. The
semantic and implementation of function
svn_fs_fs__open_pack_or_rev_file_writable() is really bad: it makes
file writable and then changes it back to read only on pool cleanup.

The code your committed works like this:
1. Open zero revision file for writing
2. Write revision content
3. Close revision file
4. Check that revision file doesn't have read only attribute
5. Make it writeable if needed and register pool cleanup handler
6. Open revision file for writing
7. Calculate required checksums
8. Append indexes to revision file
9. Close revision file.
10. Make revision file readonly

But this could be implemented much better:
1. Open zero revision file for writing
2. Write revision content
3. Calculate required data without closing revision file
4. Append indexes to revision file
5. Close revision file
6. Make revision file readonly

Proof of concept patch is attached.

---
Ivan Zhakov

--001a11c2e8f026b85804fd60a1e0
Content-Type: text/plain; charset=US-ASCII;
	name="svn-simplify-write_revision_zero.patch.txt"
Content-Disposition: attachment;
	filename="svn-simplify-write_revision_zero.patch.txt"
Content-Transfer-Encoding: base64
X-Attachment-Id: f_hx7pl14q0

SW5kZXg6IHN1YnZlcnNpb24vbGlic3ZuX2ZzX2ZzL2ZzX2ZzLmMNCj09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0NCi0tLSBz
dWJ2ZXJzaW9uL2xpYnN2bl9mc19mcy9mc19mcy5jCShyZXZpc2lvbiAxNjA3ODg0KQ0KKysrIHN1
YnZlcnNpb24vbGlic3ZuX2ZzX2ZzL2ZzX2ZzLmMJKHdvcmtpbmcgY29weSkNCkBAIC0xNDgxLDEy
ICsxNDgxLDEwIEBADQogICAgIHsKICAgICAgIGFwcl9hcnJheV9oZWFkZXJfdCAqaW5kZXhfZW50
cmllczsKICAgICAgIHN2bl9mc19mc19fcDJsX2VudHJ5X3QgKmVudHJ5OwotICAgICAgc3ZuX2Zz
X2ZzX19yZXZpc2lvbl9maWxlX3QgKnJldl9maWxlOwogICAgICAgY29uc3QgY2hhciAqbDJwX3By
b3RvX2luZGV4LCAqcDJsX3Byb3RvX2luZGV4OwotCi0gICAgICAvKiBXcml0ZSBhIHNrZWxldG9u
IHIwIHdpdGggbm8gaW5kZXhlcy4gKi8KLSAgICAgIFNWTl9FUlIoc3ZuX2lvX2ZpbGVfY3JlYXRl
KHBhdGhfcmV2aXNpb25femVybywKLSAgICAgICAgICAgICAgICAgICAgIlBMQUlOXG5FTkRcbkVO
RFJFUFxuIgorICAgICAgYXByX2ZpbGVfdCAqZmlsZTsKKyAgICAgIGNvbnN0IGNoYXIgKmNvbnRl
bnQgPSAKKyAgICAgICAgICAiUExBSU5cbkVORFxuRU5EUkVQXG4iCiAgICAgICAgICAgICAgICAg
ICAgICJpZDogMC4wLnIwLzJcbiIKICAgICAgICAgICAgICAgICAgICAgInR5cGU6IGRpclxuIgog
ICAgICAgICAgICAgICAgICAgICAiY291bnQ6IDBcbiIKQEAgLTE0OTMsOCArMTQ5MSwxNSBAQA0K
ICAgICAgICAgICAgICAgICAgICAgInRleHQ6IDAgMyA0IDQgIgogICAgICAgICAgICAgICAgICAg
ICAiMmQyOTc3ZDFjOTZmNDg3YWJlNGExZTIwMmRkMDNiNGVcbiIKICAgICAgICAgICAgICAgICAg
ICAgImNwYXRoOiAvXG4iCi0gICAgICAgICAgICAgICAgICAgICJcblxuIiwgc3VicG9vbCkpOwor
ICAgICAgICAgICAgICAgICAgICAiXG5cbiI7CiAKKyAgICAgIC8qIFdyaXRlIGEgc2tlbGV0b24g
cjAgd2l0aCBubyBpbmRleGVzLiAqLworICAgICAgU1ZOX0VSUihzdm5faW9fZmlsZV9vcGVuKCZm
aWxlLCBwYXRoX3JldmlzaW9uX3plcm8sCisgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
QVBSX1dSSVRFIHwgQVBSX1JFQUQgfCBBUFJfQ1JFQVRFIHwgQVBSX0VYQ0wsCisgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgQVBSX09TX0RFRkFVTFQsCisgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgc3VicG9vbCkpOworICAgICAgU1ZOX0VSUihzdm5faW9fZmlsZV93cml0ZV9m
dWxsKGZpbGUsIGNvbnRlbnQsIHN0cmxlbihjb250ZW50KSwgTlVMTCwgc3VicG9vbCkpOworCiAg
ICAgICAvKiBDb25zdHJ1Y3QgdGhlIGluZGV4IFAyTCBjb250ZW50czogZGVzY3JpYmUgdGhlIDMg
aXRlbXMgd2UgaGF2ZS4KICAgICAgICAgIEJlIHN1cmUgdG8gY3JlYXRlIHRoZW0gaW4gb24tZGlz
ayBvcmRlci4gKi8KICAgICAgIGluZGV4X2VudHJpZXMgPSBhcHJfYXJyYXlfbWFrZShzdWJwb29s
LCAzLCBzaXplb2YoZW50cnkpKTsKQEAgLTE1MjUsMTcgKzE1MzAsMTUgQEANCiAKICAgICAgIC8q
IE5vdyByZS1vcGVuIHIwLCBjcmVhdGUgcHJvdG8taW5kZXggZmlsZXMgZnJvbSBvdXIgZW50cmll
cyBhbmQKICAgICAgICAgIHJld3JpdGUgdGhlIGluZGV4IHNlY3Rpb24gb2YgcjAuICovCi0gICAg
ICBTVk5fRVJSKHN2bl9mc19mc19fb3Blbl9wYWNrX29yX3Jldl9maWxlX3dyaXRhYmxlKCZyZXZf
ZmlsZSwgZnMsIDAsCi0gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgIHN1YnBvb2wsIHN1YnBvb2wpKTsKICAgICAgIFNWTl9FUlIoc3ZuX2ZzX2Zz
X19wMmxfaW5kZXhfZnJvbV9wMmxfZW50cmllcygmcDJsX3Byb3RvX2luZGV4LCBmcywKLSAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICByZXZfZmlsZSwg
aW5kZXhfZW50cmllcywKKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICBmaWxlLCBpbmRleF9lbnRyaWVzLAogICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgIHN1YnBvb2wsIHN1YnBvb2wpKTsKICAgICAgIFNW
Tl9FUlIoc3ZuX2ZzX2ZzX19sMnBfaW5kZXhfZnJvbV9wMmxfZW50cmllcygmbDJwX3Byb3RvX2lu
ZGV4LCBmcywKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICBpbmRleF9lbnRyaWVzLAogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgIHN1YnBvb2wsIHN1YnBvb2wpKTsKLSAgICAgIFNWTl9FUlIoc3ZuX2Zz
X2ZzX19hZGRfaW5kZXhfZGF0YShmcywgcmV2X2ZpbGUtPmZpbGUsIGwycF9wcm90b19pbmRleCwK
KyAgICAgIFNWTl9FUlIoc3ZuX2ZzX2ZzX19hZGRfaW5kZXhfZGF0YShmcywgZmlsZSwgbDJwX3By
b3RvX2luZGV4LAogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIHAybF9w
cm90b19pbmRleCwgMCwgc3VicG9vbCkpOwotICAgICAgU1ZOX0VSUihzdm5fZnNfZnNfX2Nsb3Nl
X3JldmlzaW9uX2ZpbGUocmV2X2ZpbGUpKTsKKyAgICAgIFNWTl9FUlIoc3ZuX2lvX2ZpbGVfY2xv
c2UoZmlsZSwgc3VicG9vbCkpOwogICAgIH0KICAgZWxzZQogICAgIFNWTl9FUlIoc3ZuX2lvX2Zp
bGVfY3JlYXRlKHBhdGhfcmV2aXNpb25femVybywKSW5kZXg6IHN1YnZlcnNpb24vbGlic3ZuX2Zz
X2ZzL2luZGV4LmMNCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT0NCi0tLSBzdWJ2ZXJzaW9uL2xpYnN2bl9mc19mcy9pbmRl
eC5jCShyZXZpc2lvbiAxNjA3ODg0KQ0KKysrIHN1YnZlcnNpb24vbGlic3ZuX2ZzX2ZzL2luZGV4
LmMJKHdvcmtpbmcgY29weSkNCkBAIC0yNjI3LDcgKzI2MjcsNyBAQA0KICAqIGFsbG9jYXRpb25z
LiAqLwogc3RhdGljIHN2bl9lcnJvcl90ICoKIGNhbGNfZm52MShzdm5fZnNfZnNfX3AybF9lbnRy
eV90ICplbnRyeSwKLSAgICAgICAgICBzdm5fZnNfZnNfX3JldmlzaW9uX2ZpbGVfdCAqcmV2X2Zp
bGUsCisgICAgICAgICAgYXByX2ZpbGVfdCAqcmV2X2ZpbGUsCiAgICAgICAgICAgYXByX3Bvb2xf
dCAqcG9vbCkKIHsKICAgdW5zaWduZWQgY2hhciBidWZmZXJbNDA5Nl07CkBAIC0yNjQ2LDEzICsy
NjQ2LDEzIEBADQogICAgIH0KIAogICAvKiBSZWFkIHRoZSBibG9jayBhbmQgZmVlZCBpdCB0byB0
aGUgY2hlY2tzdW0gY2FsY3VsYXRvci4gKi8KLSAgU1ZOX0VSUihzdm5faW9fZmlsZV9zZWVrKHJl
dl9maWxlLT5maWxlLCBBUFJfU0VULCAmZW50cnktPm9mZnNldCwgcG9vbCkpOworICBTVk5fRVJS
KHN2bl9pb19maWxlX3NlZWsocmV2X2ZpbGUsIEFQUl9TRVQsICZlbnRyeS0+b2Zmc2V0LCBwb29s
KSk7CiAgIHdoaWxlIChzaXplID4gMCkKICAgICB7CiAgICAgICBhcHJfc2l6ZV90IHRvX3JlYWQg
PSBzaXplID4gc2l6ZW9mKGJ1ZmZlcikKICAgICAgICAgICAgICAgICAgICAgICAgICA/IHNpemVv
ZihidWZmZXIpCiAgICAgICAgICAgICAgICAgICAgICAgICAgOiAoYXByX3NpemVfdClzaXplOwot
ICAgICAgU1ZOX0VSUihzdm5faW9fZmlsZV9yZWFkX2Z1bGwyKHJldl9maWxlLT5maWxlLCBidWZm
ZXIsIHRvX3JlYWQsIE5VTEwsCisgICAgICBTVk5fRVJSKHN2bl9pb19maWxlX3JlYWRfZnVsbDIo
cmV2X2ZpbGUsIGJ1ZmZlciwgdG9fcmVhZCwgTlVMTCwKICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICBOVUxMLCBwb29sKSk7CiAgICAgICBTVk5fRVJSKHN2bl9jaGVja3N1bV91
cGRhdGUoY29udGV4dCwgYnVmZmVyLCB0b19yZWFkKSk7CiAgICAgICBzaXplIC09IHRvX3JlYWQ7
CkBAIC0yNjcyLDcgKzI2NzIsNyBAQA0KIHN2bl9lcnJvcl90ICoKIHN2bl9mc19mc19fcDJsX2lu
ZGV4X2Zyb21fcDJsX2VudHJpZXMoY29uc3QgY2hhciAqKnByb3RvbmFtZSwKICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgc3ZuX2ZzX3QgKmZzLAotICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICBzdm5fZnNfZnNfX3JldmlzaW9uX2ZpbGVfdCAqcmV2X2Zp
bGUsCisgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGFwcl9maWxlX3QgKnJl
dl9maWxlLAogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBhcHJfYXJyYXlf
aGVhZGVyX3QgKmVudHJpZXMsCiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
IGFwcl9wb29sX3QgKnJlc3VsdF9wb29sLAogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICBhcHJfcG9vbF90ICpzY3JhdGNoX3Bvb2wpCkluZGV4OiBzdWJ2ZXJzaW9uL2xpYnN2
bl9mc19mcy9pbmRleC5oDQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09
PT09PT09PT09PT09PT09PT09PT09PT09PT09DQotLS0gc3VidmVyc2lvbi9saWJzdm5fZnNfZnMv
aW5kZXguaAkocmV2aXNpb24gMTYwNzg4NCkNCisrKyBzdWJ2ZXJzaW9uL2xpYnN2bl9mc19mcy9p
bmRleC5oCSh3b3JraW5nIGNvcHkpDQpAQCAtMjU5LDcgKzI1OSw3IEBADQogc3ZuX2Vycm9yX3Qg
Kgogc3ZuX2ZzX2ZzX19wMmxfaW5kZXhfZnJvbV9wMmxfZW50cmllcyhjb25zdCBjaGFyICoqcHJv
dG9uYW1lLAogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBzdm5fZnNfdCAq
ZnMsCi0gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIHN2bl9mc19mc19fcmV2
aXNpb25fZmlsZV90ICpyZXZfZmlsZSwKKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgYXByX2ZpbGVfdCAqcmV2X2ZpbGUsCiAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgIGFwcl9hcnJheV9oZWFkZXJfdCAqZW50cmllcywKICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgYXByX3Bvb2xfdCAqcmVzdWx0X3Bvb2wsCiAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgIGFwcl9wb29sX3QgKnNjcmF0Y2hfcG9vbCk7Cg==
--001a11c2e8f026b85804fd60a1e0--