[email protected] wrote on Wed, Jul 24, 2013 at 14:32:48 -0000:
> Author: stefan2
> Date: Wed Jul 24 14:32:48 2013
> New Revision: 1506576
>
> URL: http://svn.apache.org/r1506576
> Log:
> On the fsfs-improvements branch: With the new ID types in place,
> we can now replace other string with a struct
>
> + intra-node uniqification content. */
> + struct
> + {
> + svn_fs_fs__id_part_t txn_id;
I'd rename this inner-struct member to avoid confusion with the
eponymous member of the outer struct.
As to the rest... I don't grok yet the new stuff. Looks like there is
an entirely new ID API, which was just thrown in with little docs as to
the intended end-result or difference from the current one :-(
What is the difference between svn_fs_fs__id_txn_id()
and svn_fs_fs__id_is_txn()? Why can't one of them be deleted?
svn_fs_fs__id_is_txn() uses svn_fs_fs__id_txn_used(). I haven't looked
at callers, but wouldn't it be a coding error (i.e., an assert()-level
bug) to have an fs_fs__id_t struct which has not been initialized to be
either a revid or a txnid? Or do we have now a trimodal object
[uninit'd, txnid, revid]? (If the latter, I would wonder how many
places in the code we have that assume a bimodal [txnid, revid]
model...)
Why does svn_fs_fs__id_is_txn() use a struct-extension instead of
svn_fs_id_t.fsap_data? Is that just in order to have svn_fs_id_t
struct and the FSFS-private part of it be allocated contiguously?
Why is svn_fs_fs__id_offset() not reimplemented as
#define svn_fs_fs__id_offset(id) svn_fs_fs__id_rev_offset(id).number
? i.e., why are both accessors needed?
Does the transition from 'const char *' to svn_fs_fs__id_part_t require
changes to 'structure'? The signature of svn_fs_fs__id_txn_create()
implies that a "noderev id" is now a three-tuple of three (revnum,
offset) pairs. That doesn't match 'structure'.
What is "rev node ID" in the docstring of svn_fs_fs__id_part_t? Should
it read "the txn-id component of a noderev-id"?
Bottom line: I don't know that I understand this. The duplication of
svn_fs_fs__id_is_txn() svn_fs_fs__id_txn_used() makes me wary of NIH,
and the other changes makes me worry if the all-too-common tendency to
change the format without updating 'structure' has striken again.
(Incidentally, trunk/subversion/libsvn_fs_x/structure is wrong --- it
describes FSFS f6, not FSX.)
Cheers,
Daniel
> + apr_uint64_t number;
> + } uniquifier;
> } representation_t;
