> -----Original Message-----
> From: Daniel Shahaf [mailto:d.s_at_daniel.shahaf.name]
> Sent: zaterdag 12 december 2015 09:20
> To: Julian Foad <julianfoad_at_gmail.com>
> Cc: Philip Martin <philip.martin_at_wandisco.com>; Bert Huijben
> <bert_at_qqmail.nl>; dev_at_subversion.apache.org
> Subject: Re: svn commit: r1716548 -
> /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
>
> 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...
Thanks for this explanation...
With this explanation I think we can go ahead and commit the fix suggested
by philip to fix the actual problem we found... (2).
But we explicitly *don't* make (1) promised behavior...
Which results in fixing (3), and happy buildbots :-)
With my recent work on ra-git, I really don't want to attach anything to
noderev information... We can map our whole filesystem api implementation
over git repositories, except for the noderevision thing.
We moved far enough to node relations in our backend to avoid implementing
actual noderevisions in libsvn_fs_git now...
Bert
Received on 2015-12-12 13:14:47 CET