On 25 June 2014 22:11, Daniel Shahaf <d.s_at_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 <stefan.fuhrmann_at_wandisco.com> wrote:
>> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
>> >>
>> >> On 25 June 2014 19:24, Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com>
>> >> wrote:
>> >> > On Wed, Jun 25, 2014 at 4:57 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
>> >> >>
>> >> >> On 21 June 2014 17:15, <stefan2_at_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
Received on 2014-06-26 11:53:46 CEST