On Sun, Aug 4, 2013 at 9:08 AM, Daniel Shahaf <[email protected]> wrote:
> [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. > 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.
