Comment 1 by Richard Hopkins, Jan 25, 2011

Comment 2 by Richard Hopkins, Jan 25, 2011

This has passed preliminary testing, and is now ready for extensive 
testing.

See fc13d22f21a87c3a0f37d87a7fb8bbf667b65639 for more details.

Comment 3 by Thomas Keller, Feb 5, 2011

I had a look at the changes in this branch and you're heading the 
right direction. Please go the extra mile and externalize the code 
that iterates the edge_map, but instead of using booleans, use some 
local, namespaced enum.

Secondly, your branch is missing a short note in NEWS that this bug 
is fixed by the change. Its a good idea to do that in the branch 
already, so its not forgotten later on and can be reviewed as well. 

Finally, I'm not sure I haven't asked that before, but could you 
find out the reason (or reproduce at least) why the old 
implementation was so slow?

Comment 4 by Richard Hopkins, Feb 6, 2011

Thanks for reviewing - I'm currently on Windows and cannot build it 
at the moment but I'll look at it again soon.

"externalize the code that iterates the edge_map" - do you 
mean to put the for loop in a new function so it can be used by 
"mtn status" and "mtn ls changed" - because they 
are now practically identical ?

I'm also not sure what you mean by namespaced enum ? I guess you 
mean 

enum output_type { added, attr, dropped, patched, renamed };

Does that mean the method would then need a set<output_type> 
and only if the set contains "added" should the added 
files be output ?

Regarding the reason for the slowness, I couldn't actually reproduce 
it myself - but the original issue mentioned a 750MB and I don't 
have one that big.

However, I did compare both versions and the only difference was 
inside the edge_map for loop. The start of both functions performed 
the same things but just in a different order.

The inside of the edge_map seemed to be doing basically the same as 
well - the original called select_nodes_modified_by_cset - which 
loops around each type just like the new implementation does.

The only difference I can see is that select_nodes_modified_by_cset 
does extra loops at the end to copy what it has found into the 
output. select_nodes_modified_by_cset also has more memory overhead 
as it's keeping more track of more things in memory.

Comment 5 by Thomas Keller, Feb 18, 2011

@Richard H.: Any news on this one?

Comment 6 by Richard Hopkins, Feb 24, 2011

1ce52abe1d12b891c5c5783921287f41b5915b30 passes all the tests and 
now needs reviewing.

Comment 7 by joe 23, Mar 17, 2011

I haven't tested the fix, but I found proof that in 0.99.1 it was 
scanning the whole tree. This may also be helpful for a test case.

Assume two directories x/ and y/ in a repo, both with checked-in 
files.

1. rm x/foo (remove - NOT drop - some file in x)
2. mtn list changed y (FAILS on missing file in x/)
3. mtn status y (Works)

If you have a fix, step 2 should work.

Comment 8 by Richard Hopkins, Mar 21, 2011

I have now added a new test to the func test suite which mimics Joes 
new scenario (thanks).

There is also 2 heads now in the nvm.issue-120 branch. The first is 
the original refactoring of "mtn status", the second is a 
1 line change to apply the PATH mask.

Both heads contain the test, and both pass.

d0883e2ac70cb5ca8a3cf845fbb5fee2f59e390e is the 1 line change and 
will be pushed later.

Comment 9 by Richard Hopkins, Mar 25, 2011

This is now merged into net.venge.monotone and has revision id 
b2729ea31820bf3bca56abb985e92f7009e4d4d2.