On Sun, Aug 4, 2013 at 9:08 AM, Daniel Shahaf <danielsh_at_elego.de> wrote:
> stefan2_at_apache.org 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.
>
Done in r1510134.
> 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 :-(
>
See log message of r1506545:
> This replaces the previous string-based API with one that represents
> IDs as quadruples of <revision,sub-id> pairs of integers. Thus, it
> now matches the implicit usage of the node_id, branch_id, txn_id and
> rev_offset parts of those IDs.
The semantics of the previous ID API is being preserved.
Apart from bits improving symmetry (e.g. rev_offset is now
accessible as a whole just like the other parts), this is a
primarily syntactic change from string to struct. It's the
sheer amount of code churn makes it non-trivial.
> 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?
>
How do you delete an element from a struct? I could have
made svn_fs_fs__id_txn_id() return a NULL if the txn_id part
is not used but that would make the ID API less explicit:
if (svn_fs_fs__id_is_txn(id))
do_stuff();
requires less knowledge about state machines etc. than
if (svn_fs_fs__id_txn_id(id) != NULL)
do_stuff();
> 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?
That would indeed be a coding *within* id.c. There is no API to
create an ID with both elements set nor to modify an existing ID.
> 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...)
>
Hence the svn_fs_fs__id_is_txn() API. It returns a binary value.
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?
>
Faster allocation, access and cache serialization.
Struct extensions ("inheritance") are one of the features of C++
I had loved to use here.
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?
>
I don't see the added benefit of having a new macro calling a function
than having a new trivial function.
Having both accessors made it easier to migrate existing code.
We could now decide to get rid of the *_id_offset and *_id_rev API.
They are being called only in 2 and 4 places, respectively.
> 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'.
>
'structure' is very explicit about the internal structure of
node_id, copy_id and txn_id. It also describes the internal
structure of the rev_offset element. ID API and implementation
now explicitly match that description instead of fiddling with
BDB-like IDs.
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"?
>
(Hopefully) clarified in r1510135.
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.)
>
I am aware that the binary structure of FSX is not defined, yet.
I updated the file in r1510158.
-- Stefan^2.
Received on 2013-08-04 14:30:31 CEST