On Fri, Jul 4, 2014 at 6:29 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> 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.
>
What part of that is bad? As a courtesy to the user,
we mark all repository files as r/o in a hope that it
may help prevent accidental modification. But nothing
requires us to do so.
If we want to modify the revision files, e.g. to rewrite
the index data by tools like svn-fsfs, we need to reset
the r/o flag before we can open the file for writing.
Restoring the r/o flag upon pool cleanup is probably
the safest way wrt making sure it eventually happens.
One might discuss whether svn_fs_fs__close_revision_file
should reset the flag explicitly as well.
IOW, this function is already required for features not
linked to repo creation and has well-defined semantics.
The repo creation simply uses existing functionality.
> 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
>
I agree that this does less work, i.e. faster but it comes at
a price: we need to switch *one* of the index interfaces
from revision_file_t to apr_file_t, creating an asymmetry
and we have to update all callers (which are not all
covered by your patch).
More importantly, the patch does not make the code any
easier to review - which was the point of the exercise.
So, we are left with some code churn and worse interfaces
for the sake of a micro-optimization of a rare operation.
Something to which you have expressed utmost
opposition in the past.
Therefore, thank you for the feedback but I will not apply
that patch as is.
-- Stefan^2.
Received on 2014-08-11 17:52:04 CEST