subversion-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <...@daniel.shahaf.name>
Subject Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/ subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/
Date Wed, 25 Jun 2014 18:11:39 GMT
Ivan Zhakov wrote on Wed, Jun 25, 2014 at 20:03:11 +0400:
> On 25 June 2014 19:54, Stefan Fuhrmann <stefan.fuhrmann@wandisco.com> wrote:
> > On Wed, Jun 25, 2014 at 5:46 PM, Ivan Zhakov <ivan@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 <ivan@visualsvn.com>
wrote:
> >> >>
> >> >> On 21 June 2014 17:15,  <stefan2@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.

Mime
View raw message