Bert Huijben wrote on Sat, Nov 28, 2015 at 21:58:00 +0100:
>
>
> > -----Original Message-----
> > From: Daniel Shahaf [mailto:danielsh_at_apache.org]
> > Sent: zaterdag 28 november 2015 18:32
> > To: Philip Martin <philip.martin_at_wandisco.com>
> > Cc: Bert Huijben <bert_at_qqmail.nl>; 'Branko Čibej' <brane_at_apache.org>;
> > dev_at_subversion.apache.org
> > Subject: Re: svn commit: r1716548 -
> > /subversion/trunk/subversion/tests/libsvn_fs/fs-test.c
> >
> > On Thu, Nov 26, 2015 at 06:17:07PM +0000, Philip Martin wrote:
> > > Philip Martin <philip.martin_at_wandisco.com> writes:
> > >
> > > > I suppose one way to fix this would be to ensure that every BDB
> revision
> > > > generates a new node-revision-id.
> > >
> > > To do this we make '/' mutable either when creating the txn, or when
> > > commiting the txn. Patches to do both below. Not sure which one is
> > > best.
> >
> > Making '/' mutable at transaction creation would violate an explicit API
> > promise:
>
> If you interpret it this way FSFS always broke the promise
> >
> > /** Set @a *revision to the revision in which @a path under @a root
> was
> > * created. Use @a pool for any temporary allocations. @a *revision
> will
> > * be set to #SVN_INVALID_REVNUM for uncommitted nodes (i.e.
> > modified nodes
> > * under a transaction root). Note that the root of an unmodified
> > transaction
> > * is not itself considered to be modified; in that case, return the
> revision
> > * upon which the transaction was based.
> > */
> > svn_error_t *
> > svn_fs_node_created_rev(svn_revnum_t *revision,
>
> And strictly the root directory is not *under* the root.
>
I interpret the docstring as saying that calling
svn_fs_node_created_rev(.., path="", ...) is well-defined behaviour, and
I see no issue with that part of the docstring.
> I'm not sure what the right way to fix this problem is... but calling fsfs
> completely broken is not the solution for fixing this inconsistency.
>
No one was calling anything "completely broken". If FSFS was always
violating that particular API promise, then we should probably withdraw
that promise (by issueing an erratum) — especially now that no
non-deprecated backend meets the promise.
>
> And if you open a transaction against r3... then what is the revision the
> transaction is based on?
>
r3.
Even if r3 is a noop commit, I think the answer should be "r3",
regardless of whether (in that case) r3 and r2 share a root noderev.
> (The commit can even be applied on top of r4, if this doesn't conflict)
So, suppose a concurrent thread commits an r4 which is a noop commit
either before the open_txn(base=r3) call, or concurrently to it; should
the base revision be r3 or r4?
I think I would lean on the side of considering the choice of a base
revision a form of user data and preserve it, i.e., consider the base
revision r3, notwithstanding that the changes would not have conflicted
if they had been based on some other revision (e.g., r4).
Cheers,
Daniel
Received on 2015-11-30 04:38:49 CET