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
