Stefan Fuhrmann wrote:
> Thanks for the review, Julian! Uncovered a bug and
> fixed all the things you found:
Thanks.
I've proposed r1665894 and r1667101 for backport to 1.9.x.
I'm not quite sure of the severity of the bug, or what real-world
problem it may have caused, if any, so I haven't written a
'justification' in the proposal.
> Julian Foad wrote:
>> That says: the root node-rev of rX is never the same as that of rY when X
>> != Y.
>>
>> Can I just double-check: is that guaranteed to be true for every FSFS
>> repository? It's possible to commit an empty revision. The
>> "svn_fs_fs__create_txn" function ensures the new txn always has a new
>> root node-rev, but is it in any way possible that a repository could
>> exist where an (empty) commit has bypassed that code path and ended up
>> with the root noderev of r11 the same as of r10?
>
> That would not be a valid FSFS repository. Format 6 revisions and
> earlier end with a revision trailer that points to the root noderev and
> change list in that revision. Format 7 implies them as well albeit
> less explicitly. We check them explicitly in validate_root_noderev()
> and svn_fs_fs__verify_root().
Thanks for confirming that.
>> It's probably fine, but it was a bit surprising to me to find this
>> requirement made explicit. Is that consistent with the rest of FSFS?
>
> There are certainly places that assume that the root node is always
> available but I don't remember anything specific outside the consistency
> checks.
>
> If we were to allow for truly empty revisions in the future, then that
> would probably be a storage-only feature and the missing noderev
> structs would be created on-the-fly in the reader / parser function.
[...]
>> Would it be clearer to write:
>>
>> * Special case: Different txns may create the same (txn-local) node ID.
>> * These are not related to each other, nor to any other node ID so far.
>
> Committed as r1667090.
Thanks.
> r1667101 fixes another problem with that code.
Looks good.
>> Can you add test cases for the root node-rev, because the code for
>> that is special-cased?
>
> Done in r1667102.
Thanks.
- Julian
Received on 2015-03-17 10:45:03 CET