On 30 June 2014 14:19, Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com> wrote:
>
> On Fri, Jun 27, 2014 at 12:55 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
>>
>> On 26 June 2014 19:08, Stefan Fuhrmann <stefan.fuhrmann_at_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
Received on 2014-07-04 18:29:58 CEST