On 10/4/05, Nathaniel Smith <[EMAIL PROTECTED]> 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 _______________________________________________ Monotone-devel mailing list [email protected] http://lists.nongnu.org/mailman/listinfo/monotone-devel
