[MERGE] Re: Implementation and tests for -r revno:N:path

[Matthieu Moy]

John Arbash Meinel <john at arbash-meinel.com> writes:

> What about:
>
> if (revision is not None and len(revision) == 2
>     and not revision[0].needs_tree()
>     and not revision[1].needs_tree()):
>     tree1, tree2 = None, None
> else:
>     raise

No problem. Indeed, I was probably tired the day I wrote this because
I didn't manage to get the lazy evaluation right ;-).

> I also think that 'needs_tree' is an inaccurate statement. It should be
> 'needs_branch'.

Hmm, right. Indeed, the UI needs a tree, but this is managed in
builtins.py, so the revision spec should only talk about branches.

> Also, the revision specs for 'branch:' and 'ancestor:' don't need a
> branch, since they define one internally.

'branch:' does a fetch to the local branch, and as it is, it does need
to be run in a local tree. Probably this could be changed, but just
setting needs_branch there will not work and would be a bug.

AFAIK, 'ancestor:X' says "the common ancestor between X and the branch
against which we match", so it also needs a branch. It uses the
"branch" argument at least.

> You need a blank line before 'def test_diff_outside_tree'.

done.

> You have a couple of lines that exceed 79 characters.

done

> In general, I want to merge something before we release 0.9, since it is
> a regression.

I also think so, and my patch is relatively trivial and well tested,
so there shouldn't be any problem.

BTW, my patch now also fixes a typo in diff test that I even had
cut-and-pasted to my own test ;-).

-- 
Matthieu