Julian Foad wrote on Thu, Dec 10, 2015 at 14:27:44 +0000: > Philip Martin wrote: > > This discussion seems to have died out. Are we going to leave the BDB > > code as is, in which case we should mark the failing test XFAIL, or make > > a change? I still prefer the option that makes the root path mutable on > > commit, primarily because for the vast majority of commits there is no > > change at all. > > Stepping back a little, I want to pose the rhetorical question: Who is > to say which FS layer implementation is the wrong one? BDB is the > earlier implementation. If we define correctness by precedent, then > it's the FSFS behaviour that we should consider to be wrong. On the > other hand, if we define correctness by specification, then we need to > specify this behaviour somewhere, not just "randomly" change it. > > So let's try to enumerate the issues. > > (1) In FS-BDB, a no-op commit may or may not produce a new root > node-rev (depending on the specific form of the commit), whereas in > FSFS, every commit produces a new root node-rev. > > (2) FS-BDB reports a different result from svn_fs_txn_base_revision() > before and after reloading by name, when the no-new-node-rev situation > in (1) occurs. > > (3) A recently added check can reject valid delete operations when (1) > and (2) occur. > > Which of those are bugs, which are acceptable, which need to stay as > they for backward compatibility even if they are bugs, and so on? > > It seems to me that (2) is definitely a bug, but I'm not sure about > (1) and therefore not sure about (3). >
About (2): I think svn_fs_txn_base_revision() should return the value of the REV argument of the svn_fs_begin_txn() call that created the txn. Therefore, (2) is a bug. About (1): I think that, unless we have made specific API promises about when noderevs are or aren't shared, the decision of whether to share noderevs is an implementation detail of the backend: the backend may choose to share or not to share, but neither choice is more "right" than the other. Therefore: if making BDB never share the root path's noderevs fixes the bug, then I think it's a fine way to proceed. I just don't think it's the *only* correct way to proceed. For example, I think the FSFS f7 logical addressing file format supports noderev sharing of the root path of revisions within a single pack file, so FSFS 1.10 could conceivably start sharing the root path's noderevs in some circumstances. So... yes, we can go ahead and make libsvn_fs_base never share the root path's noderevs. No problem at all with that. But higher-level code shouldn't rely on that: if it hasn't been documented as an API promise, it's an implementation detail and subject to change. I hope this clarifies my viewpoint... Cheers, Daniel
