Re: [Monotone-devel] [PATCH] lca selector RFC

[Nathaniel Smith]

On Fri, Mar 02, 2007 at 10:53:11AM -0800, Emile Snyder wrote:
> I think I still have a valid commit key for venge.net, but it's been a
> really long time since I've been active with monotone and feels impolite
> to start committing again without checking in ;)

There are always branches :-).

> The attached patch implements an "LCA selector," ie. it lets you do
> 
> mtn diff -r p:net.venge.monotone
> 
> to see the changes you have in your working copy vs. the ancestor in
> net.venge.monotone which would be selected if you merged the branch
> you're on with n.v.m.

Woohoo!

> Issues remaining with the patch:
> 
> * The name.  'p' seems odd to say the least (ostensibly for branch
> divergence Point).  Would people prefer allowing multi letter selector
> naming, ie. just use lca:<branchname>?  Or just some other single
> character choice?

Multi-letter makes sense to me, the restriction to single letters is
sorta silly if you think about it.  (I mean, h: is great for quick
typing, but no reason it can't just be short for head:.)  Other
options, divergence:, div:?

> * Working copy reliance.  At the moment it find your workspace parent
> revision, the head of the specified branch, and then calls
> find_ancestor_for_merge(parent, branch_head).  Does this make sense?
> Should it try to allow for specifying both sides of the
> find_ancestor...() call like
> lca:h:net.venge.monotone.win32,h:net.venge.monotone (or something?)

I think the technically correct thing might be to have it give you
  erase_ancestor(ancestors(workspace)  # or current branch, whatever
                 intersect ancestors(given branch))
?

This also removes the requirement that the branch(es) have (a) unique
head(s).

> * Finds merge ancestor, not propagate ancestor.  It doesn't do the smart
> thing about figuring out if either the left or right revision is an
> ancestor of the other.  I just have to fix that.

Merge ancestor and propagate ancestor are the same?  Not sure what you
mean there.

I don't know what find_ancestor_for_merge does if one input is an
ancestor of the other; I assume it does the right thing automatically.
The thing I pasted above certainly does the right thing.  I don't
think there's actually anything to worry about there.

> * Needs some more cases in the test (doing it outside workspace, the
> left or right being ancestor of the other thing, etc..)

Sure, can never have too many tests :-).

I think really the bottom line is that sooner or later we're going to
need to overhaul selectors -- they could be even more useful in lots
of ways.  In the mean time, we _really really want this selector_.  So
don't worry too much about the exact details of what it looks like,
let's get it in so we can start using it :-).

-- Nathaniel

-- 
The best book on programming is still Strunk and White.