[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1665894 - in /subversion/trunk/subversion: libsvn_fs_fs/id.c libsvn_fs_fs/tree.c libsvn_fs_x/fs_id.c libsvn_fs_x/tree.c tests/libsvn_fs/fs-test.c

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Tue, 17 Mar 2015 09:44:06 +0000

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

This is an archived mail posted to the Subversion Dev mailing list.