You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ivan Zhakov <iv...@visualsvn.com> on 2014/06/25 16:57:23 UTC
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
On 21 June 2014 17:15, <st...@apache.org> wrote:
> Author: stefan2
> Date: Sat Jun 21 15:15:30 2014
> New Revision: 1604421
>
> URL: http://svn.apache.org/r1604421
> Log:
> Append index data to the rev data file instead of using separate files.
>
> This gives unpacked FSFS format 7 similar I/O characteristics and disk
> space usage as earlier formats. It also eliminates the need for retries
> after a potential background pack run because each file is now always
> consistent with itself (earlier, data or index files might already have
> been deleted while the respective other was still valid). Overall,
> most of this patch is removing code necessary to handle separate files.
>
> The new file format is simple:
>
> <rep data><l2p index><p2l index><footer><footer length>
>
> with the first three being identical what we had before. <footer length>
> is a single byte giving the length of the preceding footer, so it's
> easier to extract than the pre-f7 rev trailer and there is only one
> per pack / rev file.
>
[..]
>Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>==============================================================================
>--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21 15:15:30 2014
>@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>+ "PLAIN\nEND\nENDREP\n"
>+ "id: 0.0.r0/2\n"
>+ "type: dir\n"
>+ "count: 0\n"
>+ "text: 0 3 4 4 "
>+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>+ "cpath: /\n"
>+ "\n\n"
>+
>+ /* L2P index */
>+ "\0\x80\x40" /* rev 0, 8k entries per page */
>+ "\1\1\1" /* 1 rev, 1 page, 1 page in 1st rev */
>+ "\6\4" /* page size: bytes, count */
>+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>+
>+ /* P2L index */
>+ "\0\x6b" /* start rev, rev file size */
>+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29 bytes */
>+ "\0" /* offset entry 0 page 1 */
>+ /* len, item & type, rev, checksum */
>+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k page */
>+
>+ /* Footer */
>+ "107 121\7",
>+ 107 + 14 + 38 + 7 + 1, fs->pool));
This constant makes code unreadable, unmaintainable and very error prone.
> Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_fs_fs/rev_file.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.c Sat Jun 21 15:15:30 2014
> @@ -23,6 +23,7 @@
> #include "rev_file.h"
> #include "fs_fs.h"
> #include "index.h"
> +#include "low_level.h"
> #include "util.h"
>
> #include "../libsvn_fs/fs-loader.h"
> @@ -46,6 +47,10 @@ init_revision_file(svn_fs_fs__revision_f
> file->stream = NULL;
> file->p2l_stream = NULL;
> file->l2p_stream = NULL;
> + file->block_size = ffd->block_size;
> + file->l2p_offset = -1;
> + file->p2l_offset = -1;
> + file->footer_offset = -1;
> file->pool = pool;
> }
>
> @@ -127,14 +132,40 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
> }
>
> svn_error_t *
> -svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
> - svn_fs_t *fs,
> - svn_revnum_t rev)
> +svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file)
> {
> - if (file->file)
> - svn_fs_fs__close_revision_file(file);
> + if (file->l2p_offset == -1)
> + {
> + apr_off_t filesize = 0;
> + unsigned char footer_length;
> + svn_stringbuf_t *footer;
> +
> + /* Determine file size. */
> + SVN_ERR(svn_io_file_seek(file->file, APR_END, &filesize, file->pool));
> +
> + /* Read last byte (containing the length of the footer). */
> + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size, NULL,
> + filesize - 1, file->pool));
> + SVN_ERR(svn_io_file_read_full2(file->file, &footer_length,
> + sizeof(footer_length), NULL, NULL,
> + file->pool));
> +
> + /* Read footer. */
> + footer = svn_stringbuf_create_ensure(footer_length, file->pool);
> + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size, NULL,
> + filesize - 1 - footer_length,
> + file->pool));
> + SVN_ERR(svn_io_file_read_full2(file->file, footer->data, footer_length,
> + &footer->len, NULL, file->pool));
You're passing pointer to possible 32-bit value to function accepting
pointer to 64-bit value. This will lead unpredictable results.
> + footer->data[footer->len] = '\0';
> +
> + /* Extract index locations. */
> + SVN_ERR(svn_fs_fs__parse_footer(&file->l2p_offset, &file->p2l_offset,
> + footer, file->start_revision));
> + file->footer_offset = filesize - footer_length - 1;
> + }
>
> - return svn_error_trace(open_pack_or_rev_file(file, fs, rev, file->pool));
> + return SVN_NO_ERROR;
> }
>
> svn_error_t *
> @@ -165,9 +196,6 @@ svn_fs_fs__close_revision_file(svn_fs_fs
> if (file->file)
> SVN_ERR(svn_io_file_close(file->file, file->pool));
>
> - SVN_ERR(svn_fs_fs__packed_stream_close(file->l2p_stream));
> - SVN_ERR(svn_fs_fs__packed_stream_close(file->p2l_stream));
> -
> file->file = NULL;
> file->stream = NULL;
> file->l2p_stream = NULL;
>
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/ subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Julian Foad <ju...@btopenworld.com>.
Hi Stefan.
Compared with the extent of what you have achieved, this is a minor thing, so I don't want to go on about it much.
However the point about readable code is not just whether it will need to be changed within the lifetime of "format 7" deployments, but about whether other software engineers have the reasonable ability to read the code, take it apart and put it together "in new and interesting ways". That's the part that's difficult here: if I want to experiment with "format 8" changes here, I would have a hard time putting this section of code back together.
Instructions how to run some external tool(s) to generate this constant would be sufficient to address my concern.
FWIW I think your logical addressing work is excellent and I am really looking forward to having it available.
- Julian
Stefan Fuhrmann wrote:
> What Ivan is certainly aware of as he claims to know FSFS
> very well but you may not is, that this particular constant
> describes a template with API-like guarantees. I.e. once
> released, it must never, ever change. If it had to change
> for some future extension, an independent variant must
> be created and both currently available once be retained.
[...]
>So, Ivan made two claims:
>
>
>1. Unreadable.
> I tried to give a rough "translation" or "orientation"
> in the comments. The details can be found in the
> FSFS spec. If you want to see "plain" numbers, run
> 'svnfsfs dump-index'.
>
>
>2. Unmaintainable & error prone.
>
> That makes only sense if we are talking change
> scenarios (maybe as part of a bug fix). The only
>
> way there could be a change to that template is
>
> if it so broken that we cannot use it under any
>
> circumstance. Otherwise, once released, it is set
>
> in stone.
>
>
> I wrote the template by hand and changed it by hand,
> not fun but doable. What it learned is that even the
> slightest mistake will cause the test suite to fail and
> in many case just terminate the server tests.
>
>
>As this is the "empty revision 0", there is little gray
>area between "all contents (the root node) can be
>read plus verified" and "virtually nothing works". So,
>the only change scenario that I can see here is a
>change in the index structure before release.
>
>
>Ivan seems to have other scenarios in mind to (potentially)
>change that code that I must obviously miss. Therefore:
>"How?"
>
>
>Among everything you've written do you really have no code that could help to generate and write the indexes without having to spell it all out bit-for-bit by hand? I won't believe it if you say "no".
>>
>
>
>There is svnfsfs now.
>
>
>
>>[...]
>>
>>
>>> Changing the rev 0 template has never been a fun
>>> activity and wont become one in the foreseeable
>>> future.
>>
>> In 1.8.x it looked like this:
>>
>> SVN_ERR(svn_io_file_create(path_revision_zero,
>> "PLAIN\nEND\nENDREP\n"
>> "id: 0.0.r0/17\n"
>> "type: dir\n"
>> "count: 0\n"
>> "text: 0 0 4 4 "
>> "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> "cpath: /\n"
>> "\n\n17 107\n", fs->pool));
>>
>>
>> Yes it contained a few magic constants, some of which probably should have been
>> generated, but still it was *much* simpler.
>
> It is still there and has been unchanged since its
> original release in 1.1. And that is my point.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Jun 25, 2014 at 6:19 PM, Julian Foad <ju...@btopenworld.com>
wrote:
> Stefan Fuhrmann wrote:
>
> > Ivan Zhakov wrote:
> >> @@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >>>+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >>>+ "PLAIN\nEND\nENDREP\n"
> >>>+ "id: 0.0.r0/2\n"
> >>>+ "type: dir\n"
> >>>+ "count: 0\n"
> >>>+ "text: 0 3 4 4 "
> >>>+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >>>+ "cpath: /\n"
> >>>+ "\n\n"
> >>>+
> >>>+ /* L2P index */
> >>>+ "\0\x80\x40" /* rev 0, 8k entries per page
> */
> >>>+ "\1\1\1" /* 1 rev, 1 page, 1 page in
> 1st rev */
> >>>+ "\6\4" /* page size: bytes, count */
> >>>+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >>>+
> >>>+ /* P2L index */
> >>>+ "\0\x6b" /* start rev, rev file size */
> >>>+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
> bytes */
> >>>+ "\0" /* offset entry 0 page 1 */
> >>>+ /* len, item & type, rev,
> checksum */
> >>>+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >>>+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >>>+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >>>+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
> page */
> >>>+
> >>>+ /* Footer */
> >>>+ "107 121\7",
> >>>+ 107 + 14 + 38 + 7 + 1, fs->pool));
> >>
> >> This constant makes code unreadable, unmaintainable and very error
> prone.
> >
> > How?
>
> "How?" Really? REALLY? I don't know if Ivan was referring to the numeric
> constant or the string constant or both, but I take issue with the string
> constant.
In fact, "really" ;).
What Ivan is certainly aware of as he claims to know FSFS
very well but you may not is, that this particular constant
describes a template with API-like guarantees. I.e. once
released, it must never, ever change. If it had to change
for some future extension, an independent variant must
be created and both currently available once be retained.
So, Ivan made two claims:
1. Unreadable.
I tried to give a rough "translation" or "orientation"
in the comments. The details can be found in the
FSFS spec. If you want to see "plain" numbers, run
'svnfsfs dump-index'.
2. Unmaintainable & error prone.
That makes only sense if we are talking change
scenarios (maybe as part of a bug fix). The only
way there could be a change to that template is
if it so broken that we cannot use it under any
circumstance. Otherwise, once released, it is set
in stone.
I wrote the template by hand and changed it by hand,
not fun but doable. What it learned is that even the
slightest mistake will cause the test suite to fail and
in many case just terminate the server tests.
As this is the "empty revision 0", there is little gray
area between "all contents (the root node) can be
read plus verified" and "virtually nothing works". So,
the only change scenario that I can see here is a
change in the index structure before release.
Ivan seems to have other scenarios in mind to (potentially)
change that code that I must obviously miss. Therefore:
"How?"
> Among everything you've written do you really have no code that could help
> to generate and write the indexes without having to spell it all out
> bit-for-bit by hand? I won't believe it if you say "no".
>
There is svnfsfs now.
>
> [...]
>
> > Changing the rev 0 template has never been a fun
> > activity and wont become one in the foreseeable
> > future.
>
> In 1.8.x it looked like this:
>
> SVN_ERR(svn_io_file_create(path_revision_zero,
> "PLAIN\nEND\nENDREP\n"
> "id: 0.0.r0/17\n"
> "type: dir\n"
> "count: 0\n"
> "text: 0 0 4 4 "
> "2d2977d1c96f487abe4a1e202dd03b4e\n"
> "cpath: /\n"
> "\n\n17 107\n", fs->pool));
>
>
> Yes it contained a few magic constants, some of which probably should have
> been generated, but still it was *much* simpler.
>
It is still there and has been unchanged since its
original release in 1.1. And that is my point.
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/ subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Fuhrmann wrote:
> Ivan Zhakov wrote:
>> @@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>>>+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>>>+ "PLAIN\nEND\nENDREP\n"
>>>+ "id: 0.0.r0/2\n"
>>>+ "type: dir\n"
>>>+ "count: 0\n"
>>>+ "text: 0 3 4 4 "
>>>+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>>>+ "cpath: /\n"
>>>+ "\n\n"
>>>+
>>>+ /* L2P index */
>>>+ "\0\x80\x40" /* rev 0, 8k entries per page */
>>>+ "\1\1\1" /* 1 rev, 1 page, 1 page in 1st rev */
>>>+ "\6\4" /* page size: bytes, count */
>>>+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>>>+
>>>+ /* P2L index */
>>>+ "\0\x6b" /* start rev, rev file size */
>>>+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29 bytes */
>>>+ "\0" /* offset entry 0 page 1 */
>>>+ /* len, item & type, rev, checksum */
>>>+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>>>+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>>>+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>>>+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k page */
>>>+
>>>+ /* Footer */
>>>+ "107 121\7",
>>>+ 107 + 14 + 38 + 7 + 1, fs->pool));
>>
>> This constant makes code unreadable, unmaintainable and very error prone.
>
> How?
"How?" Really? REALLY? I don't know if Ivan was referring to the numeric constant or the string constant or both, but I take issue with the string constant. Among everything you've written do you really have no code that could help to generate and write the indexes without having to spell it all out bit-for-bit by hand? I won't believe it if you say "no".
[...]
> Changing the rev 0 template has never been a fun
> activity and wont become one in the foreseeable
> future.
In 1.8.x it looked like this:
SVN_ERR(svn_io_file_create(path_revision_zero,
"PLAIN\nEND\nENDREP\n"
"id: 0.0.r0/17\n"
"type: dir\n"
"count: 0\n"
"text: 0 0 4 4 "
"2d2977d1c96f487abe4a1e202dd03b4e\n"
"cpath: /\n"
"\n\n17 107\n", fs->pool));
Yes it contained a few magic constants, some of which probably should have been generated, but still it was *much* simpler.
- Julian
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Jul 4, 2014 at 6:29 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 30 June 2014 14:19, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> >
> > On Fri, Jun 27, 2014 at 12:55 PM, Ivan Zhakov <iv...@visualsvn.com>
> wrote:
> >>
> >> On 26 June 2014 19:08, Stefan Fuhrmann <st...@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.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Jul 4, 2014 at 6:29 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 30 June 2014 14:19, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> >
> > On Fri, Jun 27, 2014 at 12:55 PM, Ivan Zhakov <iv...@visualsvn.com>
> wrote:
> >>
> >> On 26 June 2014 19:08, Stefan Fuhrmann <st...@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.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 30 June 2014 14:19, Stefan Fuhrmann <st...@wandisco.com> wrote:
>
> On Fri, Jun 27, 2014 at 12:55 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 26 June 2014 19:08, Stefan Fuhrmann <st...@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
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Mon, Jun 30, 2014 at 1:38 PM, Julian Foad <ju...@btopenworld.com>
wrote:
> Stefan Fuhrmann wrote:
> > r1606554 generates the index data dynamically now.
>
> That looks *much* better to my eyes. Now it only has a very few magic
> "offset" and "length" constants which are on a par with those we already
> had in the r0 header in the previous format. I don't much mind those,
> although I'd be even happier if they weren't there either.
>
> > + /* If the config file has not been initialized, yet, set some
> defaults
> > + here for r0. r0 is so small we could do with any non-zero
> values. */
> > + if (ffd->l2p_page_size == 0)
> > + ffd->l2p_page_size = 0x2000;
> > + if (ffd->p2l_page_size == 0)
> > + ffd->p2l_page_size = 0x10000;
>
> It's not clear why it's acceptable to initialize just those two parameters
> -- it looks like an implementation detail of the particular calls you make.
> It would be better if the caller set up the config before calling this.
> There is only one caller and it sets up the default config immediately
> after calling this. Why not swap the order of calls so that it sets the
> config first and then calls this write_revision_zero()? It would make sense
> from a logical point of view that the configuration is needed to tell the
> system how to write a revision, whereas r0 doesn't need to exist in order
> to create a default config.
>
r1606744 now delays the creation of r0 until after the config
structures have been fully initialized. The only downside is
that we now use slightly different values than in the original
template. But apart from r0 getting 1 or 2 bytes larger, that
has zero effect further down the path.
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/ subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Fuhrmann wrote:
> r1606554 generates the index data dynamically now.
That looks *much* better to my eyes. Now it only has a very few magic "offset" and "length" constants which are on a par with those we already had in the r0 header in the previous format. I don't much mind those, although I'd be even happier if they weren't there either.
> + /* If the config file has not been initialized, yet, set some defaults
> + here for r0. r0 is so small we could do with any non-zero values. */
> + if (ffd->l2p_page_size == 0)
> + ffd->l2p_page_size = 0x2000;
> + if (ffd->p2l_page_size == 0)
> + ffd->p2l_page_size = 0x10000;
It's not clear why it's acceptable to initialize just those two parameters -- it looks like an implementation detail of the particular calls you make. It would be better if the caller set up the config before calling this. There is only one caller and it sets up the default config immediately after calling this. Why not swap the order of calls so that it sets the config first and then calls this write_revision_zero()? It would make sense from a logical point of view that the configuration is needed to tell the system how to write a revision, whereas r0 doesn't need to exist in order to create a default config.
- Julian
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 30 June 2014 14:19, Stefan Fuhrmann <st...@wandisco.com> wrote:
>
> On Fri, Jun 27, 2014 at 12:55 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 26 June 2014 19:08, Stefan Fuhrmann <st...@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
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Jun 27, 2014 at 12:55 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 26 June 2014 19:08, Stefan Fuhrmann <st...@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?
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Fri, Jun 27, 2014 at 12:55 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 26 June 2014 19:08, Stefan Fuhrmann <st...@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?
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 26 June 2014 19:08, Stefan Fuhrmann <st...@wandisco.com> wrote:
>
>
> On Thu, Jun 26, 2014 at 11:52 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 25 June 2014 22:11, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
>> > Ivan Zhakov wrote on Wed, Jun 25, 2014 at 20:03:11 +0400:
>> >> On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com>
>> >> wrote:
>> >> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com>
>> >> > wrote:
>> >> >>
>> >> >> On 25 June 2014 19:24, Stefan Fuhrmann
>> >> >> <st...@wandisco.com>
>> >> >> wrote:
>> >> >> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
>> >> >> >> > Author: stefan2
>> >> >> >> > Date: Sat Jun 21 15:15:30 2014
>> >> >> >> > New Revision: 1604421
>> >> >> >> >
>> >> >> >> > URL: http://svn.apache.org/r1604421
>> >> >> >> > Log:
>> >> >> >> > Append index data to the rev data file instead of using
>> >> >> >> > separate
>> >> >> >> > files.
>
>
>>
>> >> >> >> > >
>> >> >> >> > > > >==============================================================================
>>
>> >> >> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> >> >> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
>> >> >> >> > 15:15:30
>> >> >> >> > 2014
>> >> >> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>> >> >> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>> >> >> >> >+ "PLAIN\nEND\nENDREP\n"
>> >> >> >> >+ "id: 0.0.r0/2\n"
>> >> >> >> >+ "type: dir\n"
>> >> >> >> >+ "count: 0\n"
>> >> >> >> >+ "text: 0 3 4 4 "
>> >> >> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> >> >> >> >+ "cpath: /\n"
>> >> >> >> >+ "\n\n"
>> >> >> >> >+
>> >> >> >> >+ /* L2P index */
>> >> >> >> >+ "\0\x80\x40" /* rev 0, 8k entries
>> >> >> >> > per page
>> >> >> >> > */
>> >> >> >> >+ "\1\1\1" /* 1 rev, 1 page, 1
>> >> >> >> > page in
>> >> >> >> > 1st
>> >> >> >> > rev */
>> >> >> >> >+ "\6\4" /* page size: bytes,
>> >> >> >> > count */
>> >> >> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>> >> >> >> >+
>> >> >> >> >+ /* P2L index */
>> >> >> >> >+ "\0\x6b" /* start rev, rev file
>> >> >> >> > size
>> >> >> >> > */
>> >> >> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page
>> >> >> >> > using 29
>> >> >> >> > bytes */
>> >> >> >> >+ "\0" /* offset entry 0 page
>> >> >> >> > 1 */
>> >> >> >> >+ /* len, item & type,
>> >> >> >> > rev,
>> >> >> >> > checksum */
>> >> >> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> >> >> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>> >> >> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>> >> >> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up
>> >> >> >> > 64k
>> >> >> >> > page
>> >> >> >> > */
>> >> >> >> >+
>> >> >> >> >+ /* Footer */
>> >> >> >> >+ "107 121\7",
>> >> >> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
>> >> >> >> This constant makes code unreadable, unmaintainable and very
>> >> >> >> error
>> >> >> >> prone.
>> > ...
>> >> I saw it, but it doesn't make code easier to maintain.
>> >
>> > Ivan, that's the third time in as many emails that you say "the new code
>> > is hard to maintain". Could you please explain *how* you find it hard
>> > to maintain?
>> >
>> > Anyway, that's just me guessing. Could you please clarify what exactly
>> > makes the code unmaintainable in your opinion?
>> >
>> In my opinion 'code maintainability' is how easy or hard to make
>> change and review the code. This magic constant with a lot 7b encoded
>> numbers are very hard to review and modify if needed in future. Even
>> Stefan2 mentioned that he made slight mistakes when changing that
>> constant that was caught by test suite. But I don't assume that code
>> that passes all test suite doesn't have bugs, while Stefan2 seems to
>> assume. I bet that nobody reviewed "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> constant in the code above. Even I didn't, because I think it's a
>> waste of time and proper code should be committed or change reverted.
>
>
> 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).
--
Ivan Zhakov
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 26 June 2014 19:08, Stefan Fuhrmann <st...@wandisco.com> wrote:
>
>
> On Thu, Jun 26, 2014 at 11:52 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 25 June 2014 22:11, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
>> > Ivan Zhakov wrote on Wed, Jun 25, 2014 at 20:03:11 +0400:
>> >> On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com>
>> >> wrote:
>> >> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com>
>> >> > wrote:
>> >> >>
>> >> >> On 25 June 2014 19:24, Stefan Fuhrmann
>> >> >> <st...@wandisco.com>
>> >> >> wrote:
>> >> >> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
>> >> >> >> > Author: stefan2
>> >> >> >> > Date: Sat Jun 21 15:15:30 2014
>> >> >> >> > New Revision: 1604421
>> >> >> >> >
>> >> >> >> > URL: http://svn.apache.org/r1604421
>> >> >> >> > Log:
>> >> >> >> > Append index data to the rev data file instead of using
>> >> >> >> > separate
>> >> >> >> > files.
>
>
>>
>> >> >> >> > >
>> >> >> >> > > > >==============================================================================
>>
>> >> >> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> >> >> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
>> >> >> >> > 15:15:30
>> >> >> >> > 2014
>> >> >> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>> >> >> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>> >> >> >> >+ "PLAIN\nEND\nENDREP\n"
>> >> >> >> >+ "id: 0.0.r0/2\n"
>> >> >> >> >+ "type: dir\n"
>> >> >> >> >+ "count: 0\n"
>> >> >> >> >+ "text: 0 3 4 4 "
>> >> >> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> >> >> >> >+ "cpath: /\n"
>> >> >> >> >+ "\n\n"
>> >> >> >> >+
>> >> >> >> >+ /* L2P index */
>> >> >> >> >+ "\0\x80\x40" /* rev 0, 8k entries
>> >> >> >> > per page
>> >> >> >> > */
>> >> >> >> >+ "\1\1\1" /* 1 rev, 1 page, 1
>> >> >> >> > page in
>> >> >> >> > 1st
>> >> >> >> > rev */
>> >> >> >> >+ "\6\4" /* page size: bytes,
>> >> >> >> > count */
>> >> >> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>> >> >> >> >+
>> >> >> >> >+ /* P2L index */
>> >> >> >> >+ "\0\x6b" /* start rev, rev file
>> >> >> >> > size
>> >> >> >> > */
>> >> >> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page
>> >> >> >> > using 29
>> >> >> >> > bytes */
>> >> >> >> >+ "\0" /* offset entry 0 page
>> >> >> >> > 1 */
>> >> >> >> >+ /* len, item & type,
>> >> >> >> > rev,
>> >> >> >> > checksum */
>> >> >> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> >> >> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>> >> >> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>> >> >> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up
>> >> >> >> > 64k
>> >> >> >> > page
>> >> >> >> > */
>> >> >> >> >+
>> >> >> >> >+ /* Footer */
>> >> >> >> >+ "107 121\7",
>> >> >> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
>> >> >> >> This constant makes code unreadable, unmaintainable and very
>> >> >> >> error
>> >> >> >> prone.
>> > ...
>> >> I saw it, but it doesn't make code easier to maintain.
>> >
>> > Ivan, that's the third time in as many emails that you say "the new code
>> > is hard to maintain". Could you please explain *how* you find it hard
>> > to maintain?
>> >
>> > Anyway, that's just me guessing. Could you please clarify what exactly
>> > makes the code unmaintainable in your opinion?
>> >
>> In my opinion 'code maintainability' is how easy or hard to make
>> change and review the code. This magic constant with a lot 7b encoded
>> numbers are very hard to review and modify if needed in future. Even
>> Stefan2 mentioned that he made slight mistakes when changing that
>> constant that was caught by test suite. But I don't assume that code
>> that passes all test suite doesn't have bugs, while Stefan2 seems to
>> assume. I bet that nobody reviewed "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> constant in the code above. Even I didn't, because I think it's a
>> waste of time and proper code should be committed or change reverted.
>
>
> 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).
--
Ivan Zhakov
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/ subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Julian Foad <ju...@btopenworld.com>.
Stefan Fuhrmann wrote:
> 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.
There's no need to obfuscate code in order to deter people from changing it. We have review, regression testing, social responsibility, and such like.
> * 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.
Ack.
> 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)
So it sounds like you're comfortable that explicit test cases would easily cover this.
I simply don't understand why you think writing r0 is special, compared with writing everything else in a repository. Everything that's written has to be backwards compatible with its claimed format, not just the r0 record.
Nor do I understand why you think it is especially "constant" -- why are we not allowed to change the order of a couple of records in it or introduce a leading zero or a space or some such change that would otherwise be backward-compatible but would break the idea that this file is strictly "constant"?
> Pros:
> * No or very little special code for r0 (not sure, yet).
>
> * Format / encoding changes don't require new r0 templates.
And:
* Software engineers can take the code apart and put it back together in new and interesting ways, such as to develop format 8, without unreasonable extra effort.
> 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?
>
> Which one would you pick and why?
>
> I'd be fine with switching to option 2 as long as everyone understands
> the implications.
Sounds good to me.
- Julian
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Branko Čibej <br...@wandisco.com>.
On 26.06.2014 17:45, Stefan Sperling wrote:
> On Thu, Jun 26, 2014 at 05:08:38PM +0200, Stefan Fuhrmann wrote:
>> I'd be fine with switching to option 2 as long as everyone understands
>> the implications.
> How about we write option 3 code to generate option 1 code, then hardcode
> the generated option 1 code but put the option 3 code in a comment near
> the option 1 code as a reference?
I don't see how requiring people to review the generator /and/ the
generated constant, independently of each other, and prove that the
former produces the latter, will make the code more maintainable and
understandable.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. brane@wandisco.com
Re: svn commit: r1604421 - in /subversion/trunk:
subversion/libsvn_fs_fs/ subversion/tests/cmdline/
subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jun 26, 2014 at 05:08:38PM +0200, Stefan Fuhrmann wrote:
> 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?
> Which one would you pick and why?
>
> I'd be fine with switching to option 2 as long as everyone understands
> the implications.
How about we write option 3 code to generate option 1 code, then hardcode
the generated option 1 code but put the option 3 code in a comment near
the option 1 code as a reference?
Re: svn commit: r1604421 - in /subversion/trunk:
subversion/libsvn_fs_fs/ subversion/tests/cmdline/
subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Sperling <st...@elego.de>.
On Thu, Jun 26, 2014 at 05:08:38PM +0200, Stefan Fuhrmann wrote:
> 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?
> Which one would you pick and why?
>
> I'd be fine with switching to option 2 as long as everyone understands
> the implications.
How about we write option 3 code to generate option 1 code, then hardcode
the generated option 1 code but put the option 3 code in a comment near
the option 1 code as a reference?
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Thu, Jun 26, 2014 at 11:52 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 25 June 2014 22:11, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > Ivan Zhakov wrote on Wed, Jun 25, 2014 at 20:03:11 +0400:
> >> On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> >> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com>
> wrote:
> >> >>
> >> >> On 25 June 2014 19:24, Stefan Fuhrmann <stefan.fuhrmann@wandisco.com
> >
> >> >> wrote:
> >> >> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com>
> wrote:
> >> >> >>
> >> >> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
> >> >> >> > Author: stefan2
> >> >> >> > Date: Sat Jun 21 15:15:30 2014
> >> >> >> > New Revision: 1604421
> >> >> >> >
> >> >> >> > URL: http://svn.apache.org/r1604421
> >> >> >> > Log:
> >> >> >> > Append index data to the rev data file instead of using separate
> >> >> >> > files.
>
> >> >> >> > >
> >==============================================================================
> >> >> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> >> >> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
> >> >> >> > 15:15:30
> >> >> >> > 2014
> >> >> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >> >> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >> >> >> >+ "PLAIN\nEND\nENDREP\n"
> >> >> >> >+ "id: 0.0.r0/2\n"
> >> >> >> >+ "type: dir\n"
> >> >> >> >+ "count: 0\n"
> >> >> >> >+ "text: 0 3 4 4 "
> >> >> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >> >> >> >+ "cpath: /\n"
> >> >> >> >+ "\n\n"
> >> >> >> >+
> >> >> >> >+ /* L2P index */
> >> >> >> >+ "\0\x80\x40" /* rev 0, 8k entries
> per page
> >> >> >> > */
> >> >> >> >+ "\1\1\1" /* 1 rev, 1 page, 1
> page in
> >> >> >> > 1st
> >> >> >> > rev */
> >> >> >> >+ "\6\4" /* page size: bytes,
> count */
> >> >> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >> >> >> >+
> >> >> >> >+ /* P2L index */
> >> >> >> >+ "\0\x6b" /* start rev, rev file
> size
> >> >> >> > */
> >> >> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page
> using 29
> >> >> >> > bytes */
> >> >> >> >+ "\0" /* offset entry 0 page
> 1 */
> >> >> >> >+ /* len, item & type,
> rev,
> >> >> >> > checksum */
> >> >> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >> >> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >> >> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >> >> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up
> 64k
> >> >> >> > page
> >> >> >> > */
> >> >> >> >+
> >> >> >> >+ /* Footer */
> >> >> >> >+ "107 121\7",
> >> >> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
> >> >> >> This constant makes code unreadable, unmaintainable and very error
> >> >> >> prone.
> > ...
> >> I saw it, but it doesn't make code easier to maintain.
> >
> > Ivan, that's the third time in as many emails that you say "the new code
> > is hard to maintain". Could you please explain *how* you find it hard
> > to maintain?
> >
> > Anyway, that's just me guessing. Could you please clarify what exactly
> > makes the code unmaintainable in your opinion?
> >
> In my opinion 'code maintainability' is how easy or hard to make
> change and review the code. This magic constant with a lot 7b encoded
> numbers are very hard to review and modify if needed in future. Even
> Stefan2 mentioned that he made slight mistakes when changing that
> constant that was caught by test suite. But I don't assume that code
> that passes all test suite doesn't have bugs, while Stefan2 seems to
> assume. I bet that nobody reviewed "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> constant in the code above. Even I didn't, because I think it's a
> waste of time and proper code should be committed or change reverted.
>
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?
Which one would you pick and why?
I'd be fine with switching to option 2 as long as everyone understands
the implications.
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Thu, Jun 26, 2014 at 11:52 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 25 June 2014 22:11, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> > Ivan Zhakov wrote on Wed, Jun 25, 2014 at 20:03:11 +0400:
> >> On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> >> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com>
> wrote:
> >> >>
> >> >> On 25 June 2014 19:24, Stefan Fuhrmann <stefan.fuhrmann@wandisco.com
> >
> >> >> wrote:
> >> >> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com>
> wrote:
> >> >> >>
> >> >> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
> >> >> >> > Author: stefan2
> >> >> >> > Date: Sat Jun 21 15:15:30 2014
> >> >> >> > New Revision: 1604421
> >> >> >> >
> >> >> >> > URL: http://svn.apache.org/r1604421
> >> >> >> > Log:
> >> >> >> > Append index data to the rev data file instead of using separate
> >> >> >> > files.
>
> >> >> >> > >
> >==============================================================================
> >> >> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> >> >> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
> >> >> >> > 15:15:30
> >> >> >> > 2014
> >> >> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >> >> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >> >> >> >+ "PLAIN\nEND\nENDREP\n"
> >> >> >> >+ "id: 0.0.r0/2\n"
> >> >> >> >+ "type: dir\n"
> >> >> >> >+ "count: 0\n"
> >> >> >> >+ "text: 0 3 4 4 "
> >> >> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >> >> >> >+ "cpath: /\n"
> >> >> >> >+ "\n\n"
> >> >> >> >+
> >> >> >> >+ /* L2P index */
> >> >> >> >+ "\0\x80\x40" /* rev 0, 8k entries
> per page
> >> >> >> > */
> >> >> >> >+ "\1\1\1" /* 1 rev, 1 page, 1
> page in
> >> >> >> > 1st
> >> >> >> > rev */
> >> >> >> >+ "\6\4" /* page size: bytes,
> count */
> >> >> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >> >> >> >+
> >> >> >> >+ /* P2L index */
> >> >> >> >+ "\0\x6b" /* start rev, rev file
> size
> >> >> >> > */
> >> >> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page
> using 29
> >> >> >> > bytes */
> >> >> >> >+ "\0" /* offset entry 0 page
> 1 */
> >> >> >> >+ /* len, item & type,
> rev,
> >> >> >> > checksum */
> >> >> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >> >> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >> >> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >> >> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up
> 64k
> >> >> >> > page
> >> >> >> > */
> >> >> >> >+
> >> >> >> >+ /* Footer */
> >> >> >> >+ "107 121\7",
> >> >> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
> >> >> >> This constant makes code unreadable, unmaintainable and very error
> >> >> >> prone.
> > ...
> >> I saw it, but it doesn't make code easier to maintain.
> >
> > Ivan, that's the third time in as many emails that you say "the new code
> > is hard to maintain". Could you please explain *how* you find it hard
> > to maintain?
> >
> > Anyway, that's just me guessing. Could you please clarify what exactly
> > makes the code unmaintainable in your opinion?
> >
> In my opinion 'code maintainability' is how easy or hard to make
> change and review the code. This magic constant with a lot 7b encoded
> numbers are very hard to review and modify if needed in future. Even
> Stefan2 mentioned that he made slight mistakes when changing that
> constant that was caught by test suite. But I don't assume that code
> that passes all test suite doesn't have bugs, while Stefan2 seems to
> assume. I bet that nobody reviewed "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> constant in the code above. Even I didn't, because I think it's a
> waste of time and proper code should be committed or change reverted.
>
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?
Which one would you pick and why?
I'd be fine with switching to option 2 as long as everyone understands
the implications.
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 25 June 2014 22:11, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Ivan Zhakov wrote on Wed, Jun 25, 2014 at 20:03:11 +0400:
>> On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com> wrote:
>> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> >>
>> >> On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com>
>> >> wrote:
>> >> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> >> >>
>> >> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
>> >> >> > Author: stefan2
>> >> >> > Date: Sat Jun 21 15:15:30 2014
>> >> >> > New Revision: 1604421
>> >> >> >
>> >> >> > URL: http://svn.apache.org/r1604421
>> >> >> > Log:
>> >> >> > Append index data to the rev data file instead of using separate
>> >> >> > files.
>> >> >> >
>> >> >> > This gives unpacked FSFS format 7 similar I/O characteristics and
>> >> >> > disk
>> >> >> > space usage as earlier formats. It also eliminates the need for
>> >> >> > retries
>> >> >> > after a potential background pack run because each file is now always
>> >> >> > consistent with itself (earlier, data or index files might already
>> >> >> > have
>> >> >> > been deleted while the respective other was still valid). Overall,
>> >> >> > most of this patch is removing code necessary to handle separate
>> >> >> > files.
>> >> >> >
>> >> >> > The new file format is simple:
>> >> >> >
>> >> >> > <rep data><l2p index><p2l index><footer><footer length>
>> >> >> >
>> >> >> > with the first three being identical what we had before. <footer
>> >> >> > length>
>> >> >> > is a single byte giving the length of the preceding footer, so it's
>> >> >> > easier to extract than the pre-f7 rev trailer and there is only one
>> >> >> > per pack / rev file.
>> >> >> >
>> >> >> [..]
>> >> >>
>> >> >>
>> >> >> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>> >> >> >URL:
>> >> >> >
>> >> >> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>> >> >>
>> >> >> >
>> >> >> > > >==============================================================================
>> >> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> >> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
>> >> >> > 15:15:30
>> >> >> > 2014
>> >> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>> >> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>> >> >> >+ "PLAIN\nEND\nENDREP\n"
>> >> >> >+ "id: 0.0.r0/2\n"
>> >> >> >+ "type: dir\n"
>> >> >> >+ "count: 0\n"
>> >> >> >+ "text: 0 3 4 4 "
>> >> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> >> >> >+ "cpath: /\n"
>> >> >> >+ "\n\n"
>> >> >> >+
>> >> >> >+ /* L2P index */
>> >> >> >+ "\0\x80\x40" /* rev 0, 8k entries per page
>> >> >> > */
>> >> >> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in
>> >> >> > 1st
>> >> >> > rev */
>> >> >> >+ "\6\4" /* page size: bytes, count */
>> >> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>> >> >> >+
>> >> >> >+ /* P2L index */
>> >> >> >+ "\0\x6b" /* start rev, rev file size
>> >> >> > */
>> >> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
>> >> >> > bytes */
>> >> >> >+ "\0" /* offset entry 0 page 1 */
>> >> >> >+ /* len, item & type, rev,
>> >> >> > checksum */
>> >> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> >> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>> >> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>> >> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
>> >> >> > page
>> >> >> > */
>> >> >> >+
>> >> >> >+ /* Footer */
>> >> >> >+ "107 121\7",
>> >> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
>> >> >> This constant makes code unreadable, unmaintainable and very error
>> >> >> prone.
> ...
>> I saw it, but it doesn't make code easier to maintain.
>
> Ivan, that's the third time in as many emails that you say "the new code
> is hard to maintain". Could you please explain *how* you find it hard
> to maintain?
>
> Anyway, that's just me guessing. Could you please clarify what exactly
> makes the code unmaintainable in your opinion?
>
In my opinion 'code maintainability' is how easy or hard to make
change and review the code. This magic constant with a lot 7b encoded
numbers are very hard to review and modify if needed in future. Even
Stefan2 mentioned that he made slight mistakes when changing that
constant that was caught by test suite. But I don't assume that code
that passes all test suite doesn't have bugs, while Stefan2 seems to
assume. I bet that nobody reviewed "\x11\x34\0\xf5\xd6\x8c\x81\x06"
constant in the code above. Even I didn't, because I think it's a
waste of time and proper code should be committed or change reverted.
>
> I assume you dislike the magic numbers and would prefer to see sizeof()
> uesd instead, e.g.,
>
>
> #define REV0_FILE_PART_1 "foo"
> #define REV0_FILE_PART_2 "bar"
>
> svn_file_create_binary(REV0_FILE_PART_1 REV0_FILE_PART_2,
> sizeof(REV0_FILE_PART_1)-1
> + sizeof(REV0_FILE_PART_2)-1);
>
This will make code a little bit better, but not so much. Because the
problem in magic hex numbers above.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 25 June 2014 22:11, Daniel Shahaf <d....@daniel.shahaf.name> wrote:
> Ivan Zhakov wrote on Wed, Jun 25, 2014 at 20:03:11 +0400:
>> On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com> wrote:
>> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> >>
>> >> On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com>
>> >> wrote:
>> >> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> >> >>
>> >> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
>> >> >> > Author: stefan2
>> >> >> > Date: Sat Jun 21 15:15:30 2014
>> >> >> > New Revision: 1604421
>> >> >> >
>> >> >> > URL: http://svn.apache.org/r1604421
>> >> >> > Log:
>> >> >> > Append index data to the rev data file instead of using separate
>> >> >> > files.
>> >> >> >
>> >> >> > This gives unpacked FSFS format 7 similar I/O characteristics and
>> >> >> > disk
>> >> >> > space usage as earlier formats. It also eliminates the need for
>> >> >> > retries
>> >> >> > after a potential background pack run because each file is now always
>> >> >> > consistent with itself (earlier, data or index files might already
>> >> >> > have
>> >> >> > been deleted while the respective other was still valid). Overall,
>> >> >> > most of this patch is removing code necessary to handle separate
>> >> >> > files.
>> >> >> >
>> >> >> > The new file format is simple:
>> >> >> >
>> >> >> > <rep data><l2p index><p2l index><footer><footer length>
>> >> >> >
>> >> >> > with the first three being identical what we had before. <footer
>> >> >> > length>
>> >> >> > is a single byte giving the length of the preceding footer, so it's
>> >> >> > easier to extract than the pre-f7 rev trailer and there is only one
>> >> >> > per pack / rev file.
>> >> >> >
>> >> >> [..]
>> >> >>
>> >> >>
>> >> >> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>> >> >> >URL:
>> >> >> >
>> >> >> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>> >> >>
>> >> >> >
>> >> >> > > >==============================================================================
>> >> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> >> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
>> >> >> > 15:15:30
>> >> >> > 2014
>> >> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>> >> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>> >> >> >+ "PLAIN\nEND\nENDREP\n"
>> >> >> >+ "id: 0.0.r0/2\n"
>> >> >> >+ "type: dir\n"
>> >> >> >+ "count: 0\n"
>> >> >> >+ "text: 0 3 4 4 "
>> >> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> >> >> >+ "cpath: /\n"
>> >> >> >+ "\n\n"
>> >> >> >+
>> >> >> >+ /* L2P index */
>> >> >> >+ "\0\x80\x40" /* rev 0, 8k entries per page
>> >> >> > */
>> >> >> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in
>> >> >> > 1st
>> >> >> > rev */
>> >> >> >+ "\6\4" /* page size: bytes, count */
>> >> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>> >> >> >+
>> >> >> >+ /* P2L index */
>> >> >> >+ "\0\x6b" /* start rev, rev file size
>> >> >> > */
>> >> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
>> >> >> > bytes */
>> >> >> >+ "\0" /* offset entry 0 page 1 */
>> >> >> >+ /* len, item & type, rev,
>> >> >> > checksum */
>> >> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> >> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>> >> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>> >> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
>> >> >> > page
>> >> >> > */
>> >> >> >+
>> >> >> >+ /* Footer */
>> >> >> >+ "107 121\7",
>> >> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
>> >> >> This constant makes code unreadable, unmaintainable and very error
>> >> >> prone.
> ...
>> I saw it, but it doesn't make code easier to maintain.
>
> Ivan, that's the third time in as many emails that you say "the new code
> is hard to maintain". Could you please explain *how* you find it hard
> to maintain?
>
> Anyway, that's just me guessing. Could you please clarify what exactly
> makes the code unmaintainable in your opinion?
>
In my opinion 'code maintainability' is how easy or hard to make
change and review the code. This magic constant with a lot 7b encoded
numbers are very hard to review and modify if needed in future. Even
Stefan2 mentioned that he made slight mistakes when changing that
constant that was caught by test suite. But I don't assume that code
that passes all test suite doesn't have bugs, while Stefan2 seems to
assume. I bet that nobody reviewed "\x11\x34\0\xf5\xd6\x8c\x81\x06"
constant in the code above. Even I didn't, because I think it's a
waste of time and proper code should be committed or change reverted.
>
> I assume you dislike the magic numbers and would prefer to see sizeof()
> uesd instead, e.g.,
>
>
> #define REV0_FILE_PART_1 "foo"
> #define REV0_FILE_PART_2 "bar"
>
> svn_file_create_binary(REV0_FILE_PART_1 REV0_FILE_PART_2,
> sizeof(REV0_FILE_PART_1)-1
> + sizeof(REV0_FILE_PART_2)-1);
>
This will make code a little bit better, but not so much. Because the
problem in magic hex numbers above.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1604421 - in /subversion/trunk:
subversion/libsvn_fs_fs/ subversion/tests/cmdline/
subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Ivan Zhakov wrote on Wed, Jun 25, 2014 at 20:03:11 +0400:
> On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com> wrote:
> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>
> >> On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com>
> >> wrote:
> >> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >> >>
> >> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
> >> >> > Author: stefan2
> >> >> > Date: Sat Jun 21 15:15:30 2014
> >> >> > New Revision: 1604421
> >> >> >
> >> >> > URL: http://svn.apache.org/r1604421
> >> >> > Log:
> >> >> > Append index data to the rev data file instead of using separate
> >> >> > files.
> >> >> >
> >> >> > This gives unpacked FSFS format 7 similar I/O characteristics and
> >> >> > disk
> >> >> > space usage as earlier formats. It also eliminates the need for
> >> >> > retries
> >> >> > after a potential background pack run because each file is now always
> >> >> > consistent with itself (earlier, data or index files might already
> >> >> > have
> >> >> > been deleted while the respective other was still valid). Overall,
> >> >> > most of this patch is removing code necessary to handle separate
> >> >> > files.
> >> >> >
> >> >> > The new file format is simple:
> >> >> >
> >> >> > <rep data><l2p index><p2l index><footer><footer length>
> >> >> >
> >> >> > with the first three being identical what we had before. <footer
> >> >> > length>
> >> >> > is a single byte giving the length of the preceding footer, so it's
> >> >> > easier to extract than the pre-f7 rev trailer and there is only one
> >> >> > per pack / rev file.
> >> >> >
> >> >> [..]
> >> >>
> >> >>
> >> >> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >> >> >URL:
> >> >> >
> >> >> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> >> >>
> >> >> >
> >> >> > > >==============================================================================
> >> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> >> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
> >> >> > 15:15:30
> >> >> > 2014
> >> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >> >> >+ "PLAIN\nEND\nENDREP\n"
> >> >> >+ "id: 0.0.r0/2\n"
> >> >> >+ "type: dir\n"
> >> >> >+ "count: 0\n"
> >> >> >+ "text: 0 3 4 4 "
> >> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >> >> >+ "cpath: /\n"
> >> >> >+ "\n\n"
> >> >> >+
> >> >> >+ /* L2P index */
> >> >> >+ "\0\x80\x40" /* rev 0, 8k entries per page
> >> >> > */
> >> >> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in
> >> >> > 1st
> >> >> > rev */
> >> >> >+ "\6\4" /* page size: bytes, count */
> >> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >> >> >+
> >> >> >+ /* P2L index */
> >> >> >+ "\0\x6b" /* start rev, rev file size
> >> >> > */
> >> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
> >> >> > bytes */
> >> >> >+ "\0" /* offset entry 0 page 1 */
> >> >> >+ /* len, item & type, rev,
> >> >> > checksum */
> >> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
> >> >> > page
> >> >> > */
> >> >> >+
> >> >> >+ /* Footer */
> >> >> >+ "107 121\7",
> >> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
> >> >> This constant makes code unreadable, unmaintainable and very error
> >> >> prone.
...
> I saw it, but it doesn't make code easier to maintain.
Ivan, that's the third time in as many emails that you say "the new code
is hard to maintain". Could you please explain *how* you find it hard
to maintain?
I assume you dislike the magic numbers and would prefer to see sizeof()
uesd instead, e.g.,
#define REV0_FILE_PART_1 "foo"
#define REV0_FILE_PART_2 "bar"
svn_file_create_binary(REV0_FILE_PART_1 REV0_FILE_PART_2,
sizeof(REV0_FILE_PART_1)-1
+ sizeof(REV0_FILE_PART_2)-1);
Anyway, that's just me guessing. Could you please clarify what exactly
makes the code unmaintainable in your opinion?
Thanks.
Re: svn commit: r1604421 - in /subversion/trunk:
subversion/libsvn_fs_fs/ subversion/tests/cmdline/
subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Ivan Zhakov wrote on Wed, Jun 25, 2014 at 20:03:11 +0400:
> On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com> wrote:
> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>
> >> On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com>
> >> wrote:
> >> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >> >>
> >> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
> >> >> > Author: stefan2
> >> >> > Date: Sat Jun 21 15:15:30 2014
> >> >> > New Revision: 1604421
> >> >> >
> >> >> > URL: http://svn.apache.org/r1604421
> >> >> > Log:
> >> >> > Append index data to the rev data file instead of using separate
> >> >> > files.
> >> >> >
> >> >> > This gives unpacked FSFS format 7 similar I/O characteristics and
> >> >> > disk
> >> >> > space usage as earlier formats. It also eliminates the need for
> >> >> > retries
> >> >> > after a potential background pack run because each file is now always
> >> >> > consistent with itself (earlier, data or index files might already
> >> >> > have
> >> >> > been deleted while the respective other was still valid). Overall,
> >> >> > most of this patch is removing code necessary to handle separate
> >> >> > files.
> >> >> >
> >> >> > The new file format is simple:
> >> >> >
> >> >> > <rep data><l2p index><p2l index><footer><footer length>
> >> >> >
> >> >> > with the first three being identical what we had before. <footer
> >> >> > length>
> >> >> > is a single byte giving the length of the preceding footer, so it's
> >> >> > easier to extract than the pre-f7 rev trailer and there is only one
> >> >> > per pack / rev file.
> >> >> >
> >> >> [..]
> >> >>
> >> >>
> >> >> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >> >> >URL:
> >> >> >
> >> >> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> >> >>
> >> >> >
> >> >> > > >==============================================================================
> >> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> >> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
> >> >> > 15:15:30
> >> >> > 2014
> >> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >> >> >+ "PLAIN\nEND\nENDREP\n"
> >> >> >+ "id: 0.0.r0/2\n"
> >> >> >+ "type: dir\n"
> >> >> >+ "count: 0\n"
> >> >> >+ "text: 0 3 4 4 "
> >> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >> >> >+ "cpath: /\n"
> >> >> >+ "\n\n"
> >> >> >+
> >> >> >+ /* L2P index */
> >> >> >+ "\0\x80\x40" /* rev 0, 8k entries per page
> >> >> > */
> >> >> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in
> >> >> > 1st
> >> >> > rev */
> >> >> >+ "\6\4" /* page size: bytes, count */
> >> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >> >> >+
> >> >> >+ /* P2L index */
> >> >> >+ "\0\x6b" /* start rev, rev file size
> >> >> > */
> >> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
> >> >> > bytes */
> >> >> >+ "\0" /* offset entry 0 page 1 */
> >> >> >+ /* len, item & type, rev,
> >> >> > checksum */
> >> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
> >> >> > page
> >> >> > */
> >> >> >+
> >> >> >+ /* Footer */
> >> >> >+ "107 121\7",
> >> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
> >> >> This constant makes code unreadable, unmaintainable and very error
> >> >> prone.
...
> I saw it, but it doesn't make code easier to maintain.
Ivan, that's the third time in as many emails that you say "the new code
is hard to maintain". Could you please explain *how* you find it hard
to maintain?
I assume you dislike the magic numbers and would prefer to see sizeof()
uesd instead, e.g.,
#define REV0_FILE_PART_1 "foo"
#define REV0_FILE_PART_2 "bar"
svn_file_create_binary(REV0_FILE_PART_1 REV0_FILE_PART_2,
sizeof(REV0_FILE_PART_1)-1
+ sizeof(REV0_FILE_PART_2)-1);
Anyway, that's just me guessing. Could you please clarify what exactly
makes the code unmaintainable in your opinion?
Thanks.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com> wrote:
> On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com>
>> wrote:
>> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> >>
>> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
>> >> > Author: stefan2
>> >> > Date: Sat Jun 21 15:15:30 2014
>> >> > New Revision: 1604421
>> >> >
>> >> > URL: http://svn.apache.org/r1604421
>> >> > Log:
>> >> > Append index data to the rev data file instead of using separate
>> >> > files.
>> >> >
>> >> > This gives unpacked FSFS format 7 similar I/O characteristics and
>> >> > disk
>> >> > space usage as earlier formats. It also eliminates the need for
>> >> > retries
>> >> > after a potential background pack run because each file is now always
>> >> > consistent with itself (earlier, data or index files might already
>> >> > have
>> >> > been deleted while the respective other was still valid). Overall,
>> >> > most of this patch is removing code necessary to handle separate
>> >> > files.
>> >> >
>> >> > The new file format is simple:
>> >> >
>> >> > <rep data><l2p index><p2l index><footer><footer length>
>> >> >
>> >> > with the first three being identical what we had before. <footer
>> >> > length>
>> >> > is a single byte giving the length of the preceding footer, so it's
>> >> > easier to extract than the pre-f7 rev trailer and there is only one
>> >> > per pack / rev file.
>> >> >
>> >> [..]
>> >>
>> >>
>> >> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>> >> >URL:
>> >> >
>> >> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>> >>
>> >> >
>> >> > > >==============================================================================
>> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
>> >> > 15:15:30
>> >> > 2014
>> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>> >> >+ "PLAIN\nEND\nENDREP\n"
>> >> >+ "id: 0.0.r0/2\n"
>> >> >+ "type: dir\n"
>> >> >+ "count: 0\n"
>> >> >+ "text: 0 3 4 4 "
>> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> >> >+ "cpath: /\n"
>> >> >+ "\n\n"
>> >> >+
>> >> >+ /* L2P index */
>> >> >+ "\0\x80\x40" /* rev 0, 8k entries per page
>> >> > */
>> >> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in
>> >> > 1st
>> >> > rev */
>> >> >+ "\6\4" /* page size: bytes, count */
>> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>> >> >+
>> >> >+ /* P2L index */
>> >> >+ "\0\x6b" /* start rev, rev file size
>> >> > */
>> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
>> >> > bytes */
>> >> >+ "\0" /* offset entry 0 page 1 */
>> >> >+ /* len, item & type, rev,
>> >> > checksum */
>> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
>> >> > page
>> >> > */
>> >> >+
>> >> >+ /* Footer */
>> >> >+ "107 121\7",
>> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
>> >> This constant makes code unreadable, unmaintainable and very error
>> >> prone.
>> >
>> >
>> > How?
>> >
>> > To make it more obvious that I intend to follow
>> > the svn_io_file_create_binary interface, I added
>> > some commentary explaining where the numbers
>> > come from. But even just placing the sum (without
>> > its elements) in there would be fine already.
>> >
>> > Changing the rev 0 template has never been a fun
>> > activity and wont become one in the foreseeable
>> > future.
>> >
>> I believe that the committer should be responsible forcommitting
>> readable and easy to maintain code, not the reviewer. So please fix or
>> revert.
>
>
> Sorry, I should have referenced r1605444 in my post.
>
I saw it, but it doesn't make code easier to maintain.
--
Ivan Zhakov
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 25 June 2014 19:54, Stefan Fuhrmann <st...@wandisco.com> wrote:
> On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com>
>> wrote:
>> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> >>
>> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
>> >> > Author: stefan2
>> >> > Date: Sat Jun 21 15:15:30 2014
>> >> > New Revision: 1604421
>> >> >
>> >> > URL: http://svn.apache.org/r1604421
>> >> > Log:
>> >> > Append index data to the rev data file instead of using separate
>> >> > files.
>> >> >
>> >> > This gives unpacked FSFS format 7 similar I/O characteristics and
>> >> > disk
>> >> > space usage as earlier formats. It also eliminates the need for
>> >> > retries
>> >> > after a potential background pack run because each file is now always
>> >> > consistent with itself (earlier, data or index files might already
>> >> > have
>> >> > been deleted while the respective other was still valid). Overall,
>> >> > most of this patch is removing code necessary to handle separate
>> >> > files.
>> >> >
>> >> > The new file format is simple:
>> >> >
>> >> > <rep data><l2p index><p2l index><footer><footer length>
>> >> >
>> >> > with the first three being identical what we had before. <footer
>> >> > length>
>> >> > is a single byte giving the length of the preceding footer, so it's
>> >> > easier to extract than the pre-f7 rev trailer and there is only one
>> >> > per pack / rev file.
>> >> >
>> >> [..]
>> >>
>> >>
>> >> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>> >> >URL:
>> >> >
>> >> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>> >>
>> >> >
>> >> > > >==============================================================================
>> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
>> >> > 15:15:30
>> >> > 2014
>> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>> >> >+ "PLAIN\nEND\nENDREP\n"
>> >> >+ "id: 0.0.r0/2\n"
>> >> >+ "type: dir\n"
>> >> >+ "count: 0\n"
>> >> >+ "text: 0 3 4 4 "
>> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> >> >+ "cpath: /\n"
>> >> >+ "\n\n"
>> >> >+
>> >> >+ /* L2P index */
>> >> >+ "\0\x80\x40" /* rev 0, 8k entries per page
>> >> > */
>> >> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in
>> >> > 1st
>> >> > rev */
>> >> >+ "\6\4" /* page size: bytes, count */
>> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>> >> >+
>> >> >+ /* P2L index */
>> >> >+ "\0\x6b" /* start rev, rev file size
>> >> > */
>> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
>> >> > bytes */
>> >> >+ "\0" /* offset entry 0 page 1 */
>> >> >+ /* len, item & type, rev,
>> >> > checksum */
>> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
>> >> > page
>> >> > */
>> >> >+
>> >> >+ /* Footer */
>> >> >+ "107 121\7",
>> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
>> >> This constant makes code unreadable, unmaintainable and very error
>> >> prone.
>> >
>> >
>> > How?
>> >
>> > To make it more obvious that I intend to follow
>> > the svn_io_file_create_binary interface, I added
>> > some commentary explaining where the numbers
>> > come from. But even just placing the sum (without
>> > its elements) in there would be fine already.
>> >
>> > Changing the rev 0 template has never been a fun
>> > activity and wont become one in the foreseeable
>> > future.
>> >
>> I believe that the committer should be responsible forcommitting
>> readable and easy to maintain code, not the reviewer. So please fix or
>> revert.
>
>
> Sorry, I should have referenced r1605444 in my post.
>
I saw it, but it doesn't make code easier to maintain.
--
Ivan Zhakov
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>
> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
> >> > Author: stefan2
> >> > Date: Sat Jun 21 15:15:30 2014
> >> > New Revision: 1604421
> >> >
> >> > URL: http://svn.apache.org/r1604421
> >> > Log:
> >> > Append index data to the rev data file instead of using separate
> files.
> >> >
> >> > This gives unpacked FSFS format 7 similar I/O characteristics and disk
> >> > space usage as earlier formats. It also eliminates the need for
> retries
> >> > after a potential background pack run because each file is now always
> >> > consistent with itself (earlier, data or index files might already
> have
> >> > been deleted while the respective other was still valid). Overall,
> >> > most of this patch is removing code necessary to handle separate
> files.
> >> >
> >> > The new file format is simple:
> >> >
> >> > <rep data><l2p index><p2l index><footer><footer length>
> >> >
> >> > with the first three being identical what we had before. <footer
> length>
> >> > is a single byte giving the length of the preceding footer, so it's
> >> > easier to extract than the pre-f7 rev trailer and there is only one
> >> > per pack / rev file.
> >> >
> >> [..]
> >>
> >>
> >> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >> >URL:
> >> >
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> >>
> >> >
> >==============================================================================
> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
> 15:15:30
> >> > 2014
> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >> >+ "PLAIN\nEND\nENDREP\n"
> >> >+ "id: 0.0.r0/2\n"
> >> >+ "type: dir\n"
> >> >+ "count: 0\n"
> >> >+ "text: 0 3 4 4 "
> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >> >+ "cpath: /\n"
> >> >+ "\n\n"
> >> >+
> >> >+ /* L2P index */
> >> >+ "\0\x80\x40" /* rev 0, 8k entries per page
> */
> >> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in
> 1st
> >> > rev */
> >> >+ "\6\4" /* page size: bytes, count */
> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >> >+
> >> >+ /* P2L index */
> >> >+ "\0\x6b" /* start rev, rev file size */
> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
> >> > bytes */
> >> >+ "\0" /* offset entry 0 page 1 */
> >> >+ /* len, item & type, rev,
> >> > checksum */
> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
> page
> >> > */
> >> >+
> >> >+ /* Footer */
> >> >+ "107 121\7",
> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
> >> This constant makes code unreadable, unmaintainable and very error
> prone.
> >
> >
> > How?
> >
> > To make it more obvious that I intend to follow
> > the svn_io_file_create_binary interface, I added
> > some commentary explaining where the numbers
> > come from. But even just placing the sum (without
> > its elements) in there would be fine already.
> >
> > Changing the rev 0 template has never been a fun
> > activity and wont become one in the foreseeable
> > future.
> >
> I believe that the committer should be responsible forcommitting
> readable and easy to maintain code, not the reviewer. So please fix or
> revert.
>
Sorry, I should have referenced r1605444 in my post.
> >> > Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
> >> > URL:
> >> >
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> >> >
> >> >
> ==============================================================================
> >> > --- subversion/trunk/subversion/libsvn_fs_fs/rev_file.c (original)
> >> > +++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.c Sat Jun 21
> >> > 15:15:30 2014
> >> > @@ -23,6 +23,7 @@
> >> > #include "rev_file.h"
> >> > #include "fs_fs.h"
> >> > #include "index.h"
> >> > +#include "low_level.h"
> >> > #include "util.h"
> >> >
> >> > #include "../libsvn_fs/fs-loader.h"
> >> > @@ -46,6 +47,10 @@ init_revision_file(svn_fs_fs__revision_f
> >> > file->stream = NULL;
> >> > file->p2l_stream = NULL;
> >> > file->l2p_stream = NULL;
> >> > + file->block_size = ffd->block_size;
> >> > + file->l2p_offset = -1;
> >> > + file->p2l_offset = -1;
> >> > + file->footer_offset = -1;
> >> > file->pool = pool;
> >> > }
> >> >
> >> > @@ -127,14 +132,40 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
> >> > }
> >> >
> >> > svn_error_t *
> >> > -svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
> >> > - svn_fs_t *fs,
> >> > - svn_revnum_t rev)
> >> > +svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file)
> >> > {
> >> > - if (file->file)
> >> > - svn_fs_fs__close_revision_file(file);
> >> > + if (file->l2p_offset == -1)
> >> > + {
> >> > + apr_off_t filesize = 0;
> >> > + unsigned char footer_length;
> >> > + svn_stringbuf_t *footer;
> >> > +
> >> > + /* Determine file size. */
> >> > + SVN_ERR(svn_io_file_seek(file->file, APR_END, &filesize,
> >> > file->pool));
> >> > +
> >> > + /* Read last byte (containing the length of the footer). */
> >> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
> >> > NULL,
> >> > + filesize - 1, file->pool));
> >> > + SVN_ERR(svn_io_file_read_full2(file->file, &footer_length,
> >> > + sizeof(footer_length), NULL,
> NULL,
> >> > + file->pool));
> >> > +
> >> > + /* Read footer. */
> >> > + footer = svn_stringbuf_create_ensure(footer_length,
> file->pool);
> >> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
> >> > NULL,
> >> > + filesize - 1 - footer_length,
> >> > + file->pool));
> >> > + SVN_ERR(svn_io_file_read_full2(file->file, footer->data,
> >> > footer_length,
> >> > + &footer->len, NULL,
> file->pool));
> >> You're passing pointer to possible 32-bit value to function accepting
> >> pointer to 64-bit value. This will lead unpredictable results.
> >
> >
> > I assume you are referring to the &footer->len?
> > In that case, I think I'm passing an apr_size_t*
> > to an interface expecting an apr_size_t*.
> > What am I missing?
> >
> You're right. I confused svn_io_file_read_full2() with
> svn_io_file_seek(). The code above doesn't have problem I mentioned.
>
ack.
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com>
> wrote:
> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> >>
> >> On 21 June 2014 17:15, <st...@apache.org> wrote:
> >> > Author: stefan2
> >> > Date: Sat Jun 21 15:15:30 2014
> >> > New Revision: 1604421
> >> >
> >> > URL: http://svn.apache.org/r1604421
> >> > Log:
> >> > Append index data to the rev data file instead of using separate
> files.
> >> >
> >> > This gives unpacked FSFS format 7 similar I/O characteristics and disk
> >> > space usage as earlier formats. It also eliminates the need for
> retries
> >> > after a potential background pack run because each file is now always
> >> > consistent with itself (earlier, data or index files might already
> have
> >> > been deleted while the respective other was still valid). Overall,
> >> > most of this patch is removing code necessary to handle separate
> files.
> >> >
> >> > The new file format is simple:
> >> >
> >> > <rep data><l2p index><p2l index><footer><footer length>
> >> >
> >> > with the first three being identical what we had before. <footer
> length>
> >> > is a single byte giving the length of the preceding footer, so it's
> >> > easier to extract than the pre-f7 rev trailer and there is only one
> >> > per pack / rev file.
> >> >
> >> [..]
> >>
> >>
> >> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >> >URL:
> >> >
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> >>
> >> >
> >==============================================================================
> >> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> >> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21
> 15:15:30
> >> > 2014
> >> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >> >+ "PLAIN\nEND\nENDREP\n"
> >> >+ "id: 0.0.r0/2\n"
> >> >+ "type: dir\n"
> >> >+ "count: 0\n"
> >> >+ "text: 0 3 4 4 "
> >> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >> >+ "cpath: /\n"
> >> >+ "\n\n"
> >> >+
> >> >+ /* L2P index */
> >> >+ "\0\x80\x40" /* rev 0, 8k entries per page
> */
> >> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in
> 1st
> >> > rev */
> >> >+ "\6\4" /* page size: bytes, count */
> >> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >> >+
> >> >+ /* P2L index */
> >> >+ "\0\x6b" /* start rev, rev file size */
> >> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
> >> > bytes */
> >> >+ "\0" /* offset entry 0 page 1 */
> >> >+ /* len, item & type, rev,
> >> > checksum */
> >> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k
> page
> >> > */
> >> >+
> >> >+ /* Footer */
> >> >+ "107 121\7",
> >> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
> >> This constant makes code unreadable, unmaintainable and very error
> prone.
> >
> >
> > How?
> >
> > To make it more obvious that I intend to follow
> > the svn_io_file_create_binary interface, I added
> > some commentary explaining where the numbers
> > come from. But even just placing the sum (without
> > its elements) in there would be fine already.
> >
> > Changing the rev 0 template has never been a fun
> > activity and wont become one in the foreseeable
> > future.
> >
> I believe that the committer should be responsible forcommitting
> readable and easy to maintain code, not the reviewer. So please fix or
> revert.
>
Sorry, I should have referenced r1605444 in my post.
> >> > Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
> >> > URL:
> >> >
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> >> >
> >> >
> ==============================================================================
> >> > --- subversion/trunk/subversion/libsvn_fs_fs/rev_file.c (original)
> >> > +++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.c Sat Jun 21
> >> > 15:15:30 2014
> >> > @@ -23,6 +23,7 @@
> >> > #include "rev_file.h"
> >> > #include "fs_fs.h"
> >> > #include "index.h"
> >> > +#include "low_level.h"
> >> > #include "util.h"
> >> >
> >> > #include "../libsvn_fs/fs-loader.h"
> >> > @@ -46,6 +47,10 @@ init_revision_file(svn_fs_fs__revision_f
> >> > file->stream = NULL;
> >> > file->p2l_stream = NULL;
> >> > file->l2p_stream = NULL;
> >> > + file->block_size = ffd->block_size;
> >> > + file->l2p_offset = -1;
> >> > + file->p2l_offset = -1;
> >> > + file->footer_offset = -1;
> >> > file->pool = pool;
> >> > }
> >> >
> >> > @@ -127,14 +132,40 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
> >> > }
> >> >
> >> > svn_error_t *
> >> > -svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
> >> > - svn_fs_t *fs,
> >> > - svn_revnum_t rev)
> >> > +svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file)
> >> > {
> >> > - if (file->file)
> >> > - svn_fs_fs__close_revision_file(file);
> >> > + if (file->l2p_offset == -1)
> >> > + {
> >> > + apr_off_t filesize = 0;
> >> > + unsigned char footer_length;
> >> > + svn_stringbuf_t *footer;
> >> > +
> >> > + /* Determine file size. */
> >> > + SVN_ERR(svn_io_file_seek(file->file, APR_END, &filesize,
> >> > file->pool));
> >> > +
> >> > + /* Read last byte (containing the length of the footer). */
> >> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
> >> > NULL,
> >> > + filesize - 1, file->pool));
> >> > + SVN_ERR(svn_io_file_read_full2(file->file, &footer_length,
> >> > + sizeof(footer_length), NULL,
> NULL,
> >> > + file->pool));
> >> > +
> >> > + /* Read footer. */
> >> > + footer = svn_stringbuf_create_ensure(footer_length,
> file->pool);
> >> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
> >> > NULL,
> >> > + filesize - 1 - footer_length,
> >> > + file->pool));
> >> > + SVN_ERR(svn_io_file_read_full2(file->file, footer->data,
> >> > footer_length,
> >> > + &footer->len, NULL,
> file->pool));
> >> You're passing pointer to possible 32-bit value to function accepting
> >> pointer to 64-bit value. This will lead unpredictable results.
> >
> >
> > I assume you are referring to the &footer->len?
> > In that case, I think I'm passing an apr_size_t*
> > to an interface expecting an apr_size_t*.
> > What am I missing?
> >
> You're right. I confused svn_io_file_read_full2() with
> svn_io_file_seek(). The code above doesn't have problem I mentioned.
>
ack.
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com> wrote:
> On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 21 June 2014 17:15, <st...@apache.org> wrote:
>> > Author: stefan2
>> > Date: Sat Jun 21 15:15:30 2014
>> > New Revision: 1604421
>> >
>> > URL: http://svn.apache.org/r1604421
>> > Log:
>> > Append index data to the rev data file instead of using separate files.
>> >
>> > This gives unpacked FSFS format 7 similar I/O characteristics and disk
>> > space usage as earlier formats. It also eliminates the need for retries
>> > after a potential background pack run because each file is now always
>> > consistent with itself (earlier, data or index files might already have
>> > been deleted while the respective other was still valid). Overall,
>> > most of this patch is removing code necessary to handle separate files.
>> >
>> > The new file format is simple:
>> >
>> > <rep data><l2p index><p2l index><footer><footer length>
>> >
>> > with the first three being identical what we had before. <footer length>
>> > is a single byte giving the length of the preceding footer, so it's
>> > easier to extract than the pre-f7 rev trailer and there is only one
>> > per pack / rev file.
>> >
>> [..]
>>
>>
>> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>> >URL:
>> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>>
>> > >==============================================================================
>> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21 15:15:30
>> > 2014
>> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>> >+ "PLAIN\nEND\nENDREP\n"
>> >+ "id: 0.0.r0/2\n"
>> >+ "type: dir\n"
>> >+ "count: 0\n"
>> >+ "text: 0 3 4 4 "
>> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> >+ "cpath: /\n"
>> >+ "\n\n"
>> >+
>> >+ /* L2P index */
>> >+ "\0\x80\x40" /* rev 0, 8k entries per page */
>> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in 1st
>> > rev */
>> >+ "\6\4" /* page size: bytes, count */
>> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>> >+
>> >+ /* P2L index */
>> >+ "\0\x6b" /* start rev, rev file size */
>> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
>> > bytes */
>> >+ "\0" /* offset entry 0 page 1 */
>> >+ /* len, item & type, rev,
>> > checksum */
>> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k page
>> > */
>> >+
>> >+ /* Footer */
>> >+ "107 121\7",
>> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
>> This constant makes code unreadable, unmaintainable and very error prone.
>
>
> How?
>
> To make it more obvious that I intend to follow
> the svn_io_file_create_binary interface, I added
> some commentary explaining where the numbers
> come from. But even just placing the sum (without
> its elements) in there would be fine already.
>
> Changing the rev 0 template has never been a fun
> activity and wont become one in the foreseeable
> future.
>
I believe that the committer should be responsible forcommitting
readable and easy to maintain code, not the reviewer. So please fix or
revert.
>>
>>
>> > Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
>> > URL:
>> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>> >
>> > ==============================================================================
>> > --- subversion/trunk/subversion/libsvn_fs_fs/rev_file.c (original)
>> > +++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.c Sat Jun 21
>> > 15:15:30 2014
>> > @@ -23,6 +23,7 @@
>> > #include "rev_file.h"
>> > #include "fs_fs.h"
>> > #include "index.h"
>> > +#include "low_level.h"
>> > #include "util.h"
>> >
>> > #include "../libsvn_fs/fs-loader.h"
>> > @@ -46,6 +47,10 @@ init_revision_file(svn_fs_fs__revision_f
>> > file->stream = NULL;
>> > file->p2l_stream = NULL;
>> > file->l2p_stream = NULL;
>> > + file->block_size = ffd->block_size;
>> > + file->l2p_offset = -1;
>> > + file->p2l_offset = -1;
>> > + file->footer_offset = -1;
>> > file->pool = pool;
>> > }
>> >
>> > @@ -127,14 +132,40 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
>> > }
>> >
>> > svn_error_t *
>> > -svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
>> > - svn_fs_t *fs,
>> > - svn_revnum_t rev)
>> > +svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file)
>> > {
>> > - if (file->file)
>> > - svn_fs_fs__close_revision_file(file);
>> > + if (file->l2p_offset == -1)
>> > + {
>> > + apr_off_t filesize = 0;
>> > + unsigned char footer_length;
>> > + svn_stringbuf_t *footer;
>> > +
>> > + /* Determine file size. */
>> > + SVN_ERR(svn_io_file_seek(file->file, APR_END, &filesize,
>> > file->pool));
>> > +
>> > + /* Read last byte (containing the length of the footer). */
>> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
>> > NULL,
>> > + filesize - 1, file->pool));
>> > + SVN_ERR(svn_io_file_read_full2(file->file, &footer_length,
>> > + sizeof(footer_length), NULL, NULL,
>> > + file->pool));
>> > +
>> > + /* Read footer. */
>> > + footer = svn_stringbuf_create_ensure(footer_length, file->pool);
>> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
>> > NULL,
>> > + filesize - 1 - footer_length,
>> > + file->pool));
>> > + SVN_ERR(svn_io_file_read_full2(file->file, footer->data,
>> > footer_length,
>> > + &footer->len, NULL, file->pool));
>> You're passing pointer to possible 32-bit value to function accepting
>> pointer to 64-bit value. This will lead unpredictable results.
>
>
> I assume you are referring to the &footer->len?
> In that case, I think I'm passing an apr_size_t*
> to an interface expecting an apr_size_t*.
> What am I missing?
>
You're right. I confused svn_io_file_read_full2() with
svn_io_file_seek(). The code above doesn't have problem I mentioned.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Ivan Zhakov <iv...@visualsvn.com>.
On 25 June 2014 19:24, Stefan Fuhrmann <st...@wandisco.com> wrote:
> On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>>
>> On 21 June 2014 17:15, <st...@apache.org> wrote:
>> > Author: stefan2
>> > Date: Sat Jun 21 15:15:30 2014
>> > New Revision: 1604421
>> >
>> > URL: http://svn.apache.org/r1604421
>> > Log:
>> > Append index data to the rev data file instead of using separate files.
>> >
>> > This gives unpacked FSFS format 7 similar I/O characteristics and disk
>> > space usage as earlier formats. It also eliminates the need for retries
>> > after a potential background pack run because each file is now always
>> > consistent with itself (earlier, data or index files might already have
>> > been deleted while the respective other was still valid). Overall,
>> > most of this patch is removing code necessary to handle separate files.
>> >
>> > The new file format is simple:
>> >
>> > <rep data><l2p index><p2l index><footer><footer length>
>> >
>> > with the first three being identical what we had before. <footer length>
>> > is a single byte giving the length of the preceding footer, so it's
>> > easier to extract than the pre-f7 rev trailer and there is only one
>> > per pack / rev file.
>> >
>> [..]
>>
>>
>> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
>> >URL:
>> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>>
>> > >==============================================================================
>> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
>> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21 15:15:30
>> > 2014
>> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
>> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
>> >+ "PLAIN\nEND\nENDREP\n"
>> >+ "id: 0.0.r0/2\n"
>> >+ "type: dir\n"
>> >+ "count: 0\n"
>> >+ "text: 0 3 4 4 "
>> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
>> >+ "cpath: /\n"
>> >+ "\n\n"
>> >+
>> >+ /* L2P index */
>> >+ "\0\x80\x40" /* rev 0, 8k entries per page */
>> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in 1st
>> > rev */
>> >+ "\6\4" /* page size: bytes, count */
>> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
>> >+
>> >+ /* P2L index */
>> >+ "\0\x6b" /* start rev, rev file size */
>> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
>> > bytes */
>> >+ "\0" /* offset entry 0 page 1 */
>> >+ /* len, item & type, rev,
>> > checksum */
>> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
>> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
>> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
>> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k page
>> > */
>> >+
>> >+ /* Footer */
>> >+ "107 121\7",
>> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
>> This constant makes code unreadable, unmaintainable and very error prone.
>
>
> How?
>
> To make it more obvious that I intend to follow
> the svn_io_file_create_binary interface, I added
> some commentary explaining where the numbers
> come from. But even just placing the sum (without
> its elements) in there would be fine already.
>
> Changing the rev 0 template has never been a fun
> activity and wont become one in the foreseeable
> future.
>
I believe that the committer should be responsible forcommitting
readable and easy to maintain code, not the reviewer. So please fix or
revert.
>>
>>
>> > Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
>> > URL:
>> > http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>> >
>> > ==============================================================================
>> > --- subversion/trunk/subversion/libsvn_fs_fs/rev_file.c (original)
>> > +++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.c Sat Jun 21
>> > 15:15:30 2014
>> > @@ -23,6 +23,7 @@
>> > #include "rev_file.h"
>> > #include "fs_fs.h"
>> > #include "index.h"
>> > +#include "low_level.h"
>> > #include "util.h"
>> >
>> > #include "../libsvn_fs/fs-loader.h"
>> > @@ -46,6 +47,10 @@ init_revision_file(svn_fs_fs__revision_f
>> > file->stream = NULL;
>> > file->p2l_stream = NULL;
>> > file->l2p_stream = NULL;
>> > + file->block_size = ffd->block_size;
>> > + file->l2p_offset = -1;
>> > + file->p2l_offset = -1;
>> > + file->footer_offset = -1;
>> > file->pool = pool;
>> > }
>> >
>> > @@ -127,14 +132,40 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
>> > }
>> >
>> > svn_error_t *
>> > -svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
>> > - svn_fs_t *fs,
>> > - svn_revnum_t rev)
>> > +svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file)
>> > {
>> > - if (file->file)
>> > - svn_fs_fs__close_revision_file(file);
>> > + if (file->l2p_offset == -1)
>> > + {
>> > + apr_off_t filesize = 0;
>> > + unsigned char footer_length;
>> > + svn_stringbuf_t *footer;
>> > +
>> > + /* Determine file size. */
>> > + SVN_ERR(svn_io_file_seek(file->file, APR_END, &filesize,
>> > file->pool));
>> > +
>> > + /* Read last byte (containing the length of the footer). */
>> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
>> > NULL,
>> > + filesize - 1, file->pool));
>> > + SVN_ERR(svn_io_file_read_full2(file->file, &footer_length,
>> > + sizeof(footer_length), NULL, NULL,
>> > + file->pool));
>> > +
>> > + /* Read footer. */
>> > + footer = svn_stringbuf_create_ensure(footer_length, file->pool);
>> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
>> > NULL,
>> > + filesize - 1 - footer_length,
>> > + file->pool));
>> > + SVN_ERR(svn_io_file_read_full2(file->file, footer->data,
>> > footer_length,
>> > + &footer->len, NULL, file->pool));
>> You're passing pointer to possible 32-bit value to function accepting
>> pointer to 64-bit value. This will lead unpredictable results.
>
>
> I assume you are referring to the &footer->len?
> In that case, I think I'm passing an apr_size_t*
> to an interface expecting an apr_size_t*.
> What am I missing?
>
You're right. I confused svn_io_file_read_full2() with
svn_io_file_seek(). The code above doesn't have problem I mentioned.
--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 21 June 2014 17:15, <st...@apache.org> wrote:
> > Author: stefan2
> > Date: Sat Jun 21 15:15:30 2014
> > New Revision: 1604421
> >
> > URL: http://svn.apache.org/r1604421
> > Log:
> > Append index data to the rev data file instead of using separate files.
> >
> > This gives unpacked FSFS format 7 similar I/O characteristics and disk
> > space usage as earlier formats. It also eliminates the need for retries
> > after a potential background pack run because each file is now always
> > consistent with itself (earlier, data or index files might already have
> > been deleted while the respective other was still valid). Overall,
> > most of this patch is removing code necessary to handle separate files.
> >
> > The new file format is simple:
> >
> > <rep data><l2p index><p2l index><footer><footer length>
> >
> > with the first three being identical what we had before. <footer length>
> > is a single byte giving the length of the preceding footer, so it's
> > easier to extract than the pre-f7 rev trailer and there is only one
> > per pack / rev file.
> >
> [..]
>
>
> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>
> >==============================================================================
> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21 15:15:30
> 2014
> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >+ "PLAIN\nEND\nENDREP\n"
> >+ "id: 0.0.r0/2\n"
> >+ "type: dir\n"
> >+ "count: 0\n"
> >+ "text: 0 3 4 4 "
> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >+ "cpath: /\n"
> >+ "\n\n"
> >+
> >+ /* L2P index */
> >+ "\0\x80\x40" /* rev 0, 8k entries per page */
> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in 1st
> rev */
> >+ "\6\4" /* page size: bytes, count */
> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >+
> >+ /* P2L index */
> >+ "\0\x6b" /* start rev, rev file size */
> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
> bytes */
> >+ "\0" /* offset entry 0 page 1 */
> >+ /* len, item & type, rev,
> checksum */
> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k page
> */
> >+
> >+ /* Footer */
> >+ "107 121\7",
> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
> This constant makes code unreadable, unmaintainable and very error prone.
>
How?
To make it more obvious that I intend to follow
the svn_io_file_create_binary interface, I added
some commentary explaining where the numbers
come from. But even just placing the sum (without
its elements) in there would be fine already.
Changing the rev 0 template has never been a fun
activity and wont become one in the foreseeable
future.
>
> > Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_fs_fs/rev_file.c (original)
> > +++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.c Sat Jun 21
> 15:15:30 2014
> > @@ -23,6 +23,7 @@
> > #include "rev_file.h"
> > #include "fs_fs.h"
> > #include "index.h"
> > +#include "low_level.h"
> > #include "util.h"
> >
> > #include "../libsvn_fs/fs-loader.h"
> > @@ -46,6 +47,10 @@ init_revision_file(svn_fs_fs__revision_f
> > file->stream = NULL;
> > file->p2l_stream = NULL;
> > file->l2p_stream = NULL;
> > + file->block_size = ffd->block_size;
> > + file->l2p_offset = -1;
> > + file->p2l_offset = -1;
> > + file->footer_offset = -1;
> > file->pool = pool;
> > }
> >
> > @@ -127,14 +132,40 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
> > }
> >
> > svn_error_t *
> > -svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
> > - svn_fs_t *fs,
> > - svn_revnum_t rev)
> > +svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file)
> > {
> > - if (file->file)
> > - svn_fs_fs__close_revision_file(file);
> > + if (file->l2p_offset == -1)
> > + {
> > + apr_off_t filesize = 0;
> > + unsigned char footer_length;
> > + svn_stringbuf_t *footer;
> > +
> > + /* Determine file size. */
> > + SVN_ERR(svn_io_file_seek(file->file, APR_END, &filesize,
> file->pool));
> > +
> > + /* Read last byte (containing the length of the footer). */
> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
> NULL,
> > + filesize - 1, file->pool));
> > + SVN_ERR(svn_io_file_read_full2(file->file, &footer_length,
> > + sizeof(footer_length), NULL, NULL,
> > + file->pool));
> > +
> > + /* Read footer. */
> > + footer = svn_stringbuf_create_ensure(footer_length, file->pool);
> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
> NULL,
> > + filesize - 1 - footer_length,
> > + file->pool));
> > + SVN_ERR(svn_io_file_read_full2(file->file, footer->data,
> footer_length,
> > + &footer->len, NULL, file->pool));
> You're passing pointer to possible 32-bit value to function accepting
> pointer to 64-bit value. This will lead unpredictable results.
>
I assume you are referring to the &footer->len?
In that case, I think I'm passing an apr_size_t*
to an interface expecting an apr_size_t*.
What am I missing?
-- Stefan^2.
Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/
subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On 21 June 2014 17:15, <st...@apache.org> wrote:
> > Author: stefan2
> > Date: Sat Jun 21 15:15:30 2014
> > New Revision: 1604421
> >
> > URL: http://svn.apache.org/r1604421
> > Log:
> > Append index data to the rev data file instead of using separate files.
> >
> > This gives unpacked FSFS format 7 similar I/O characteristics and disk
> > space usage as earlier formats. It also eliminates the need for retries
> > after a potential background pack run because each file is now always
> > consistent with itself (earlier, data or index files might already have
> > been deleted while the respective other was still valid). Overall,
> > most of this patch is removing code necessary to handle separate files.
> >
> > The new file format is simple:
> >
> > <rep data><l2p index><p2l index><footer><footer length>
> >
> > with the first three being identical what we had before. <footer length>
> > is a single byte giving the length of the preceding footer, so it's
> > easier to extract than the pre-f7 rev trailer and there is only one
> > per pack / rev file.
> >
> [..]
>
>
> >Modified: subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c
> >URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c?rev=1604421&r1=1604420&r2=1604421&view=diff
>
> >==============================================================================
> >--- subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c (original)
> >+++ subversion/trunk/subversion/libsvn_fs_fs/fs_fs.c Sat Jun 21 15:15:30
> 2014
> >@@ -1449,15 +1449,35 @@ write_revision_zero(svn_fs_t *fs)
> >+ SVN_ERR(svn_io_file_create_binary(path_revision_zero,
> >+ "PLAIN\nEND\nENDREP\n"
> >+ "id: 0.0.r0/2\n"
> >+ "type: dir\n"
> >+ "count: 0\n"
> >+ "text: 0 3 4 4 "
> >+ "2d2977d1c96f487abe4a1e202dd03b4e\n"
> >+ "cpath: /\n"
> >+ "\n\n"
> >+
> >+ /* L2P index */
> >+ "\0\x80\x40" /* rev 0, 8k entries per page */
> >+ "\1\1\1" /* 1 rev, 1 page, 1 page in 1st
> rev */
> >+ "\6\4" /* page size: bytes, count */
> >+ "\0\xd6\1\xb1\1\x21" /* phys offsets + 1 */
> >+
> >+ /* P2L index */
> >+ "\0\x6b" /* start rev, rev file size */
> >+ "\x80\x80\4\1\x1D" /* 64k pages, 1 page using 29
> bytes */
> >+ "\0" /* offset entry 0 page 1 */
> >+ /* len, item & type, rev,
> checksum */
> >+ "\x11\x34\0\xf5\xd6\x8c\x81\x06"
> >+ "\x59\x09\0\xc8\xfc\xf6\x81\x04"
> >+ "\1\x0d\0\x9d\x9e\xa9\x94\x0f"
> >+ "\x95\xff\3\x1b\0\0" /* last entry fills up 64k page
> */
> >+
> >+ /* Footer */
> >+ "107 121\7",
> >+ 107 + 14 + 38 + 7 + 1, fs->pool));
> This constant makes code unreadable, unmaintainable and very error prone.
>
How?
To make it more obvious that I intend to follow
the svn_io_file_create_binary interface, I added
some commentary explaining where the numbers
come from. But even just placing the sum (without
its elements) in there would be fine already.
Changing the rev 0 template has never been a fun
activity and wont become one in the foreseeable
future.
>
> > Modified: subversion/trunk/subversion/libsvn_fs_fs/rev_file.c
> > URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/rev_file.c?rev=1604421&r1=1604420&r2=1604421&view=diff
> >
> ==============================================================================
> > --- subversion/trunk/subversion/libsvn_fs_fs/rev_file.c (original)
> > +++ subversion/trunk/subversion/libsvn_fs_fs/rev_file.c Sat Jun 21
> 15:15:30 2014
> > @@ -23,6 +23,7 @@
> > #include "rev_file.h"
> > #include "fs_fs.h"
> > #include "index.h"
> > +#include "low_level.h"
> > #include "util.h"
> >
> > #include "../libsvn_fs/fs-loader.h"
> > @@ -46,6 +47,10 @@ init_revision_file(svn_fs_fs__revision_f
> > file->stream = NULL;
> > file->p2l_stream = NULL;
> > file->l2p_stream = NULL;
> > + file->block_size = ffd->block_size;
> > + file->l2p_offset = -1;
> > + file->p2l_offset = -1;
> > + file->footer_offset = -1;
> > file->pool = pool;
> > }
> >
> > @@ -127,14 +132,40 @@ svn_fs_fs__open_pack_or_rev_file(svn_fs_
> > }
> >
> > svn_error_t *
> > -svn_fs_fs__reopen_revision_file(svn_fs_fs__revision_file_t *file,
> > - svn_fs_t *fs,
> > - svn_revnum_t rev)
> > +svn_fs_fs__auto_read_footer(svn_fs_fs__revision_file_t *file)
> > {
> > - if (file->file)
> > - svn_fs_fs__close_revision_file(file);
> > + if (file->l2p_offset == -1)
> > + {
> > + apr_off_t filesize = 0;
> > + unsigned char footer_length;
> > + svn_stringbuf_t *footer;
> > +
> > + /* Determine file size. */
> > + SVN_ERR(svn_io_file_seek(file->file, APR_END, &filesize,
> file->pool));
> > +
> > + /* Read last byte (containing the length of the footer). */
> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
> NULL,
> > + filesize - 1, file->pool));
> > + SVN_ERR(svn_io_file_read_full2(file->file, &footer_length,
> > + sizeof(footer_length), NULL, NULL,
> > + file->pool));
> > +
> > + /* Read footer. */
> > + footer = svn_stringbuf_create_ensure(footer_length, file->pool);
> > + SVN_ERR(svn_io_file_aligned_seek(file->file, file->block_size,
> NULL,
> > + filesize - 1 - footer_length,
> > + file->pool));
> > + SVN_ERR(svn_io_file_read_full2(file->file, footer->data,
> footer_length,
> > + &footer->len, NULL, file->pool));
> You're passing pointer to possible 32-bit value to function accepting
> pointer to 64-bit value. This will lead unpredictable results.
>
I assume you are referring to the &footer->len?
In that case, I think I'm passing an apr_size_t*
to an interface expecting an apr_size_t*.
What am I missing?
-- Stefan^2.