[Monotone-devel] roster code review

[Nathaniel Smith]

(CC'ing the list, cause hey, maybe they're curious what we're up to.
This is all about the code in net.venge.monotone.experiment.rosters,
which is the in-progress rewrite of all monotone's internal versioning
machinery to do many things more nicely.)

Using a db var in next_node_id makes me vaguely queasy; having a
command line way to totally corrupt your db seems bad?  Maybe I'm
being a fussbudget.

+ // If you acquire a new ID, we require that you write the new roster
+ // that uses the ID atomically with the counter update. A
+ // half-measure to meet this requirement is to require that we're at
+ // least inside a transaction when called.
^^ Don't understand the point here.  Can't we just use a transaction
inside next_node_id to make sure that the select/update pair is
atomic, so each number is only handed out once, and if it doesn't get
used, oh well, so what.

Guess it's okay that there's no check for overflow here, because
true_node_id_source::next() in roster4.cc already checks that
next_node_id()'s return value doesn't overflow into the temporary node
space, and temporary nodes are ATM a private concept to roster4.cc.

Still in next_node_id:
+  node_id n = 0;
^^ n gets unconditionally set just below; this default value is
confusing (especially since I happen to know that it is actually an
invalid node id -- maybe we should initialize the first node to
'the_null_node + 1' instead of '1'?).

Why is get_uncommon_ancestors in database::, unlike all the other
ancestry graph stuff (in revision.cc)?

I don't quite know what the intended use for
delete_existing_revs_and_certs is supposed to be, but it seems a
little funky for it to delete rosters too -- if you do this, in the
brave new world it will totally wipe your db.  It used to be used as a
way to wipe half of the db so it could be recreated from the other
half (in 'db rebuild'), but you can't 'db rebuild' without
rosters/manifests...

Should we just drop the old_manifest fields from revisions?  I don't
see what purpose they serve anymore, it reduces a bit of redundant
checking, and if we do that then we can make it illegal for
manifest_id to be empty (just like the cset format tweaks lets us do
for file_id).

In calculate_ident(roster_t, manifest_id&):
+  marking_map mm;
+  write_roster_and_marking(ros, mm, tmp, false);
^^ This will crash.  Already went through this writing unit tests (see
the hacky make_fake_marking_for function in roster4.cc, that I have to
use to print out manifests).  The problem is that
write_roster_and_marking will call check_sane on the (roster, marking)
pair, so even though the actual printing code doesn't care if the
marking is invalid, check_sane will catch you.  We should come up with
a better way to do this, IMHO the boolean argument is ugly too...

It occurs to me that we should port 'db check' over to rosters soon
too, because it can test things that nothing else can.  E.g., the code
path for roster hash checking.

-- Nathaniel

-- 
In mathematics, it's not enough to read the words
you have to hear the music