Re: svn commit: r1604421 - in /subversion/trunk: subversion/libsvn_fs_fs/ subversion/tests/cmdline/ subversion/tests/libsvn_fs_fs/ tools/server-side/svnfsfs/

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
Received on 2014-06-30 13:41:23 CEST