[Monotone-devel] Re: roster code review

[Graydon Hoare]

On 10/4/05, Nathaniel Smith <address@hidden> wrote:

> 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.

Makes me queasy too. I did it that way because it was easy and didn't
involve making a table whose sole purpose was to store a single value.
But I don't care either way.

> ^^ 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.

Oh, yeah, good point. Extra incrementing is harmless.

> 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.

Perhaps. Wouldn't mind making an overflow check there as well. There
are lots of cases where we check something twice in a row, on each
side of an API.

> 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'?).

Sure, agreed.

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

Because you put it there! I'm just filling in calls you left undefined :)

> 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...

Good point. Agreed.

> 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).

Eh, it's possible, but I like the extra bit of checking. Unless it
really offends you.

> 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...

I'm open to "better ways", but I suspect there'll always be some
tension. There are a few different contexts for building rosters in,
and I'm not sure we can make it feel "nice" in all of them.

> 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.

Agreed. I'd like to make "migrate" and "rebuild" actually *work*
first, mind you, but after that "check" is a good next candidate.

-graydon