Re: [Monotone-devel] Re: comments on net.venge.monotone.workspace-merge.migration

[Nathaniel Smith]

On Mon, Aug 21, 2006 at 04:59:38PM -0700, Zack Weinberg wrote:
> On 8/21/06, Nathaniel Smith <address@hidden> wrote:
> >Regarding
> >  
> > http://thread.gmane.org/gmane.comp.version-control.monotone.devel/7752/focus=7756
> >Do you have any opinion?
> 
> Not a direct opinion.  (The original version of the patch eliminated
> both _MTN/revision and _MTN/work; I did it this way for the rewrite
> only because it was easier, now that _MTN/format allows us to tell the
> difference without changing the pathnames.)  However, I am leery of
> doing things that encourage people to rely on what's inside _MTN.

I'm leery too, just trying to balance that against inertia and
practicality -- the problem is that people (myself included!) have
come to depend on _MTN/revision as the only convenient way to get at
that information...

> Note for instance that we have automate get_base_revision, so keeping
> around _MTN/revision in the old format so that people can cat it seems
> silly, and people who are _writing_ to _MTN/revision should know that
> they are voiding their lack of warranty (as you put it IIRC) and may
> have to change scripts.  It's not like there's no warning when things
> stop working.

Sure.  And, hey -- automate get_base_revision_id -- that's a cool
command, thanks for pointing it out :-).  That definitely removes much
of my objection (maybe Marcel has an opinion too).

> >It also occurs to me since I wrote that that
> >there actually might be utility for us in keeping _MTN/revision as it
> >is now -- because the UI for workspace merge (and friends) probably
> >_will_ want to break symmetry, e.g., to have THIS and OTHER selectors
> >(where which parent each refers to depends on what revision you were
> >on before you ran 'merge').
> 
> I understand what you are saying after the "because" but not why it
> argues for the part before the "because".  Could you try to explain
> again please?

Sure, sorry.  Major premise: we want to break symmetry between the
parents of a multi-parent workspace.  Minor premise: revisions, per
se, are symmetrical by design.  Conclusion: we need to store an extra
piece of information someplace to break the symmetry.  _MTN/revision
is a place.

There are, of course, also other places, so this isn't a particularly
strong argument.

> >Regarding tests/migrate_workspace:
> >  I can't really tell how I would add a new test to this, or if it's
> >  even testing migration from format 2.  (I see that it tests that
> >  running the migration command with a workspace generated by whatever
> >  monotone is being tested, but we should assume that eventually that
> >  will stop being the same as "format 2".)
> 
> I can't parse the sentence in parentheses, and I don't understand how
> we could possibly be testing migration _from_ format 2 when format 2
> is the new thing being introduced.  The tests are deliberately
> agnostic as to what format is being migrated _to_ (except for checking
> _MTN/format, and the value is abstracted in 'current_workspace_format'
> constant) because that way they don't have to be modified when we add
> a new format ...

Right, maybe I should write these emails a little earlier in the
day...

So, if you look at tests/schema_migration, its approach is:
  1) generate a database using the mtn-under-test
  2) for each canned database, each of which was generated by some
     historical run of step (1), run 'mtn-under-test db migrate'
  3) assert that the resulting databases are equal to the database we
     just generated from scratch

This has a few advantages:
  * because the test also generates its own input data as a
    side-effect, it's totally brainless to add a new test case.
    There's a big comment at the top of the file that lists exactly
    what to do.
  * It's testing things end-to-end -- not just that the migration
    did this thing to those bytes, but that particular user actions
    performed both with an old version and a new version lead to the
    same results.  (I.e., upgrading commutes with user actions.)  This
    also makes writing tests easier, because the actual bytes stored
    in the test case differ for every version, but the user actions
    are more stable.

Note also that for any given schema version, we add a migration test
case for it at the same time we enable the schema at all.  This means
that for a while that particular test case is actually exercising the
"do nothing" path in "db migrate", but that's okay, that path needs
exercise too :-), and it makes life simpler if the person who adds the
test case is the same person who just modified the schema.

Does that make sense?  I'm not sure if all this applies straight
through to workspace migration, because that's a different set of
data... much less data, with much less strict requirements on
compatibility.  But hopefully it gives the idea.

The single most important thing is that we never-ever-ever want to be
in the situation where, in order to properly test the upgrade from
version n -> version m, we have to go back and rearrange the code that
deals with version n.

Some things that are bad:
  gettree("format-1-unmodified")
  copy("saved-options", "_MTN/options")
^^ if _MTN/options ever changes (e.g., goes away), we'll have to make
some sort of changes to this part of this test case.

  check(mtn("status", "--brief"), 0, nil, nil)
^^ this in fact is broken with current mainline (--brief became the
default and went away).  Because it's directly in the test case (not
factored out into some generic code, like check_migrate_from and
check_same_db_contents in tests/schema_migration), we'll have to go
through and make this change to all the test cases...

> >+// This is the oldest released version of monotone that supports the 
> >current
> >+// format.
> >+static const char first_version_supporting_current_format[] = "0.29";
> >
> >^^ needs to be bumped, obviously :-).
> 
> Will fix.  0.30 will be next, yes?

Yes.

> >+bool
> >+roster_t::get_attr(split_path const & pth,
> >+                   attr_key const & name,
> >+                   attr_value & val) const
> >+{
> >+  if (has_node(pth))
> >+    {
> >
> >Not I(has_node(pth))?  Would someone ever use this with a non-existent
> >pth?
> 
> It wuz that way before I moved it.  I was wondering why it wasn't an
> invariant myself, but I decided not to mess.

Err, yeah, I see that this is a bit of a minor mess on mainline...
it _looks_ like an I() would not break anything, but I am not entirely
sure.  All the more reason to fix it, I guess :-).  (Probably this
should give an I(), and if any code wants to call it on something that
may or may not exist, it should explicitly check whether it exists
first -- intentions would be much clearer all around then.)

-- Nathaniel

-- 
"But in Middle-earth, the distinct accusative case disappeared from
the speech of the Noldor (such things happen when you are busy
fighting Orcs, Balrogs, and Dragons)."