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

[Zack Weinberg]

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

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

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

>   Compare tests/schema_migration, where 1) it's stupid-simple to drop
>   in a new test after you change the format, 2) you are supposed to do
>   this for your new format when you change the format, so that the
>   person adding the test is the same person who already has swapped in
>   the details of the format that they're adding a test for.

So I _intended_ this to have both the properties that you cite for
schema_migration.  I even based the design on schema_migration.  The
idea is that every time we add a new format, we record some _MTN
directories in the format-N-* files, and add a new stanza to
__driver__.lua.

I'm not sure what would help with clarity here.  Is it merely a matter
of commentary, or should I restructure __driver__.lua somehow?  If so,
what would be good?  Looking back at the file some weeks after having
written it, it does seem rather long and convoluted, so I'm open to
suggestions.

> +// 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?

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

> +-- Delete a directory tree constructed as above.
> +function deltree(tree)
>
> Does this have any purpose in particular?  (It seems to be unused, and
> rm -rf would do just as well, and we'd generally like to leave files
> around anyway for forensics when a test fails.)

I intended to use it in the migration test, but wound up not needing
it (I didn't realize that remove() in the test suite implements rm
-rf) and shall take it back out again.

> comment at the top of work.hh would be good to update (this is the one
> that says, for instance:
> // _MTN/revision     -- contains the id of the checked out revision
> // _MTN/work         -- (optional) a set of added, deleted or moved pathnames
> //                      this file is, syntactically, a cset
> )

Oh, ugh, and there's probably all the manual changes to do over again
to, that I forgot to bring over from .api.

zw