You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2013/07/24 16:32:48 UTC
svn commit: r1506576 - in
/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs: fs.h
fs_fs.c low_level.c temp_serializer.c transaction.c
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
* subversion/libsvn_fs_fs/fs.h
(representation_t): replace the uniquifier string by a proper struct
* subversion/libsvn_fs_fs/fs_fs.c
(svn_fs_fs__noderev_same_rep_key,
svn_fs_fs__rep_copy): simplify as uniquifiers can now simply memory blocks
* subversion/libsvn_fs_fs/low_level.c
(svn_fs_fs__parse_representation,
svn_fs_fs__unparse_representation): update parser and writer
* subversion/libsvn_fs_fs/temp_serializer.c
(serialize_representation,
deserialize_representation): embedded structs don't need a special handling
* subversion/libsvn_fs_fs/transaction.c
(set_uniquifier): update / simplify
Modified:
subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/fs.h
subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/fs_fs.c
subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/low_level.c
subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/temp_serializer.c
subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/transaction.c
Modified: subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/fs.h
URL: http://svn.apache.org/viewvc/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/fs.h?rev=1506576&r1=1506575&r2=1506576&view=diff
==============================================================================
--- subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/fs.h (original)
+++ subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/fs.h Wed Jul 24 14:32:48 2013
@@ -431,11 +431,12 @@ typedef struct representation_t
/* For rep-sharing, we need a way of uniquifying node-revs which share the
same representation (see svn_fs_fs__noderev_same_rep_key() ). So, we
store the original txn of the node rev (not the rep!), along with some
- intra-node uniqification content.
-
- May be NULL, in which case, it is considered to match other NULL
- values.*/
- const char *uniquifier;
+ intra-node uniqification content. */
+ struct
+ {
+ svn_fs_fs__id_part_t txn_id;
+ apr_uint64_t number;
+ } uniquifier;
} representation_t;
Modified: subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/fs_fs.c
URL: http://svn.apache.org/viewvc/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/fs_fs.c?rev=1506576&r1=1506575&r2=1506576&view=diff
==============================================================================
--- subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/fs_fs.c (original)
+++ subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/fs_fs.c Wed Jul 24 14:32:48 2013
@@ -939,13 +939,7 @@ svn_fs_fs__noderev_same_rep_key(represen
if (a->revision != b->revision)
return FALSE;
- if (a->uniquifier == b->uniquifier)
- return TRUE;
-
- if (a->uniquifier == NULL || b->uniquifier == NULL)
- return FALSE;
-
- return strcmp(a->uniquifier, b->uniquifier) == 0;
+ return memcmp(&a->uniquifier, &b->uniquifier, sizeof(a->uniquifier)) == 0;
}
svn_error_t *
@@ -990,7 +984,6 @@ svn_fs_fs__rep_copy(representation_t *re
memcpy(rep_new, rep, sizeof(*rep_new));
rep_new->md5_checksum = svn_checksum_dup(rep->md5_checksum, pool);
rep_new->sha1_checksum = svn_checksum_dup(rep->sha1_checksum, pool);
- rep_new->uniquifier = apr_pstrdup(pool, rep->uniquifier);
return rep_new;
}
Modified: subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/low_level.c
URL: http://svn.apache.org/viewvc/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/low_level.c?rev=1506576&r1=1506575&r2=1506576&view=diff
==============================================================================
--- subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/low_level.c (original)
+++ subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/low_level.c Wed Jul 24 14:32:48 2013
@@ -593,12 +593,19 @@ svn_fs_fs__parse_representation(represen
pool));
/* Read the uniquifier. */
+ str = svn_cstring_tokenize("/", &string);
+ if (str == NULL)
+ return svn_error_create(SVN_ERR_FS_CORRUPT, NULL,
+ _("Malformed text representation offset line in node-rev"));
+
+ SVN_ERR(svn_fs_fs__id_txn_parse(&rep->uniquifier.txn_id, str));
+
str = svn_cstring_tokenize(" ", &string);
if (str == NULL)
return svn_error_create(SVN_ERR_FS_CORRUPT, NULL,
_("Malformed text representation offset line in node-rev"));
- rep->uniquifier = apr_pstrdup(pool, str);
+ rep->uniquifier.number = svn__base36toui64(NULL, str);
return SVN_NO_ERROR;
}
@@ -794,6 +801,7 @@ svn_fs_fs__unparse_representation(repres
svn_boolean_t may_be_corrupt,
apr_pool_t *pool)
{
+ char buffer[SVN_INT64_BUFFER_SIZE];
if (svn_fs_fs__id_txn_used(&rep->txn_id) && mutable_rep_truncated)
return svn_stringbuf_ncreate("-1", 2, pool);
@@ -810,14 +818,16 @@ svn_fs_fs__unparse_representation(repres
rep->expanded_size,
DISPLAY_MAYBE_NULL_CHECKSUM(rep->md5_checksum));
+ svn__ui64tobase36(buffer, rep->uniquifier.number);
return svn_stringbuf_createf
(pool, "%ld %" APR_OFF_T_FMT " %" SVN_FILESIZE_T_FMT
- " %" SVN_FILESIZE_T_FMT " %s %s %s",
+ " %" SVN_FILESIZE_T_FMT " %s %s %s/_%s",
rep->revision, rep->offset, rep->size,
rep->expanded_size,
DISPLAY_MAYBE_NULL_CHECKSUM(rep->md5_checksum),
DISPLAY_MAYBE_NULL_CHECKSUM(rep->sha1_checksum),
- rep->uniquifier);
+ svn_fs_fs__id_txn_unparse(&rep->uniquifier.txn_id, pool),
+ buffer);
#undef DISPLAY_MAYBE_NULL_CHECKSUM
Modified: subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/temp_serializer.c
URL: http://svn.apache.org/viewvc/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/temp_serializer.c?rev=1506576&r1=1506575&r2=1506576&view=diff
==============================================================================
--- subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/temp_serializer.c (original)
+++ subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/temp_serializer.c Wed Jul 24 14:32:48 2013
@@ -183,8 +183,6 @@ serialize_representation(svn_temp_serial
serialize_checksum(context, &rep->md5_checksum);
serialize_checksum(context, &rep->sha1_checksum);
- svn_temp_serializer__add_string(context, &rep->uniquifier);
-
/* return to the caller's nesting level */
svn_temp_serializer__pop(context);
}
@@ -206,8 +204,6 @@ deserialize_representation(void *buffer,
/* fixup of sub-structures */
deserialize_checksum(rep, &rep->md5_checksum);
deserialize_checksum(rep, &rep->sha1_checksum);
-
- svn_temp_deserializer__resolve(rep, (void **)&rep->uniquifier);
}
/* auxilliary structure representing the content of a directory hash */
Modified: subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/transaction.c
URL: http://svn.apache.org/viewvc/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/transaction.c?rev=1506576&r1=1506575&r2=1506576&view=diff
==============================================================================
--- subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/transaction.c (original)
+++ subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/transaction.c Wed Jul 24 14:32:48 2013
@@ -1370,15 +1370,10 @@ set_uniquifier(svn_fs_t *fs,
apr_pool_t *pool)
{
svn_fs_fs__id_part_t temp;
- char buffer[SVN_INT64_BUFFER_SIZE];
SVN_ERR(get_new_txn_node_id(&temp, fs, &rep->txn_id, pool));
- svn__ui64tobase36(buffer, temp.number);
-
- rep->uniquifier
- = apr_psprintf(pool, "%s/_%s",
- svn_fs_fs__id_txn_unparse(&rep->txn_id, pool),
- buffer);
+ rep->uniquifier.txn_id = rep->txn_id;
+ rep->uniquifier.number = temp.number;
return SVN_NO_ERROR;
}
Re: svn commit: r1506576 - in /subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs:
fs.h fs_fs.c low_level.c temp_serializer.c transaction.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, Aug 4, 2013 at 9:08 AM, Daniel Shahaf <da...@elego.de> wrote:
> stefan2@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.
Re: svn commit: r1506576 - in /subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs:
fs.h fs_fs.c low_level.c temp_serializer.c transaction.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, Aug 4, 2013 at 9:08 AM, Daniel Shahaf <da...@elego.de> wrote:
> stefan2@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.
Re: svn commit: r1506576 - in
/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs:
fs.h fs_fs.c low_level.c temp_serializer.c transaction.c
Posted by Daniel Shahaf <da...@elego.de>.
stefan2@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.
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;
Re: svn commit: r1506576 - in
/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs:
fs.h fs_fs.c low_level.c temp_serializer.c transaction.c
Posted by Daniel Shahaf <da...@elego.de>.
Stefan Fuhrmann wrote on Sun, Aug 04, 2013 at 14:32:14 +0200:
> On Sun, Aug 4, 2013 at 9:12 AM, Daniel Shahaf <da...@elego.de> wrote:
>
> > stefan2@apache.org wrote on Wed, Jul 24, 2013 at 14:32:48 -0000:
> > > - const char *uniquifier;
> > > + intra-node uniqification content. */
> > > + struct
> > > + {
> > > + svn_fs_fs__id_part_t txn_id;
> > > + apr_uint64_t number;
> > > + } uniquifier;
> > > } representation_t;
> >
> > A later commit moved this to node_revision_t I think;
>
>
> Which later commit? This struct has never been (intentionally) moved.
>
Pilot error on my end I think. representation_t contains a 'uniquifier'
member, the pointer in line 471 is correct, and while I'm not sure about
the pointer in line 555 I believe the branch generates the same unparsed
uniquifiers as trunk; so all in all, nothing to see in this thread.
> -- Stefan^2.
>
>
> > shouldn't the
> > pointer in structure:471 be updated? (and I'm not sure if the statement
> > in structure:555 is still accurate)
> >
> > [ all line numbers are
> > ^/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/structure@1510099]
> >
Re: svn commit: r1506576 - in
/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs:
fs.h fs_fs.c low_level.c temp_serializer.c transaction.c
Posted by Daniel Shahaf <da...@elego.de>.
Stefan Fuhrmann wrote on Sun, Aug 04, 2013 at 14:32:14 +0200:
> On Sun, Aug 4, 2013 at 9:12 AM, Daniel Shahaf <da...@elego.de> wrote:
>
> > stefan2@apache.org wrote on Wed, Jul 24, 2013 at 14:32:48 -0000:
> > > - const char *uniquifier;
> > > + intra-node uniqification content. */
> > > + struct
> > > + {
> > > + svn_fs_fs__id_part_t txn_id;
> > > + apr_uint64_t number;
> > > + } uniquifier;
> > > } representation_t;
> >
> > A later commit moved this to node_revision_t I think;
>
>
> Which later commit? This struct has never been (intentionally) moved.
>
Pilot error on my end I think. representation_t contains a 'uniquifier'
member, the pointer in line 471 is correct, and while I'm not sure about
the pointer in line 555 I believe the branch generates the same unparsed
uniquifiers as trunk; so all in all, nothing to see in this thread.
> -- Stefan^2.
>
>
> > shouldn't the
> > pointer in structure:471 be updated? (and I'm not sure if the statement
> > in structure:555 is still accurate)
> >
> > [ all line numbers are
> > ^/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/structure@1510099]
> >
Re: svn commit: r1506576 - in /subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs:
fs.h fs_fs.c low_level.c temp_serializer.c transaction.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, Aug 4, 2013 at 9:12 AM, Daniel Shahaf <da...@elego.de> wrote:
> stefan2@apache.org wrote on Wed, Jul 24, 2013 at 14:32:48 -0000:
> > - const char *uniquifier;
> > + intra-node uniqification content. */
> > + struct
> > + {
> > + svn_fs_fs__id_part_t txn_id;
> > + apr_uint64_t number;
> > + } uniquifier;
> > } representation_t;
>
> A later commit moved this to node_revision_t I think;
Which later commit? This struct has never been (intentionally) moved.
-- Stefan^2.
> shouldn't the
> pointer in structure:471 be updated? (and I'm not sure if the statement
> in structure:555 is still accurate)
>
> [ all line numbers are
> ^/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/structure@1510099]
>
Re: svn commit: r1506576 - in /subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs:
fs.h fs_fs.c low_level.c temp_serializer.c transaction.c
Posted by Stefan Fuhrmann <st...@wandisco.com>.
On Sun, Aug 4, 2013 at 9:12 AM, Daniel Shahaf <da...@elego.de> wrote:
> stefan2@apache.org wrote on Wed, Jul 24, 2013 at 14:32:48 -0000:
> > - const char *uniquifier;
> > + intra-node uniqification content. */
> > + struct
> > + {
> > + svn_fs_fs__id_part_t txn_id;
> > + apr_uint64_t number;
> > + } uniquifier;
> > } representation_t;
>
> A later commit moved this to node_revision_t I think;
Which later commit? This struct has never been (intentionally) moved.
-- Stefan^2.
> shouldn't the
> pointer in structure:471 be updated? (and I'm not sure if the statement
> in structure:555 is still accurate)
>
> [ all line numbers are
> ^/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/structure@1510099]
>
Re: svn commit: r1506576 - in
/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs:
fs.h fs_fs.c low_level.c temp_serializer.c transaction.c
Posted by Daniel Shahaf <da...@elego.de>.
stefan2@apache.org wrote on Wed, Jul 24, 2013 at 14:32:48 -0000:
> - const char *uniquifier;
> + intra-node uniqification content. */
> + struct
> + {
> + svn_fs_fs__id_part_t txn_id;
> + apr_uint64_t number;
> + } uniquifier;
> } representation_t;
A later commit moved this to node_revision_t I think; shouldn't the
pointer in structure:471 be updated? (and I'm not sure if the statement
in structure:555 is still accurate)
[ all line numbers are ^/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/structure@1510099 ]
Re: svn commit: r1506576 - in
/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs:
fs.h fs_fs.c low_level.c temp_serializer.c transaction.c
Posted by Daniel Shahaf <da...@elego.de>.
stefan2@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.
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;
Re: svn commit: r1506576 - in
/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs:
fs.h fs_fs.c low_level.c temp_serializer.c transaction.c
Posted by Daniel Shahaf <da...@elego.de>.
stefan2@apache.org wrote on Wed, Jul 24, 2013 at 14:32:48 -0000:
> - const char *uniquifier;
> + intra-node uniqification content. */
> + struct
> + {
> + svn_fs_fs__id_part_t txn_id;
> + apr_uint64_t number;
> + } uniquifier;
> } representation_t;
A later commit moved this to node_revision_t I think; shouldn't the
pointer in structure:471 be updated? (and I'm not sure if the statement
in structure:555 is still accurate)
[ all line numbers are ^/subversion/branches/fsfs-improvements/subversion/libsvn_fs_fs/structure@1510099 ]