[MERGE] #25449

[Roberto Aguilar]

On Feb 26, 2009, at 6:55 AM, Aaron Bentley wrote:
> [...]
> I think it would be better to move more of cmd_merge.run into  
> Merger, so
> that compare_basis can be invoked before  
> bundle.read_mergeable_from_url.
> Even the revised patch is changing things because it's not doing
> Merger.revision_tree, which gives the Merger a chance to cache it.

Just getting the merger object is taking quite a while.  I added a  
bunch of timestamps around the code to see how long things were  
taking.  On my machine, getting the merger object when merging a  
remote branch via ssh took between 3.6 and 4.7 seconds across 5 runs.   
With the test where I placed it, although it's not caching the result,  
uncommitted changes errors out between 0.54 and 0.65 seconds.  Putting  
this test in the Merger object will delay the error by quite a bit.

> But that isn't why I'm voting resubmit.  I'm voting resubmit because
> this patch breaks the --force option, and because this change has no  
> tests.
>
> $ bzr merge --force ../bzr.dev
> bzr: ERROR: Working tree "/home/abentley/bzr/bzr.ab/" has uncommitted
> changes.
>
> It also breaks the blackbox test
> bzrlib.tests.blackbox.test_merge.TestMerge.test_merge_remember.

The test_merge_remember test checks to make sure the parent branch  
location and the branch being merged are the same, but since the  
uncommitted change error is being handled up front, the following  
function is returning None:

branch_b.get_submit_branch()

It doesn't seem like it would matter if the submit branch matches the  
parent in this case because the error has to do with a dirty local  
copy.  I have modified the test so that it passes and checks that  
uncommitted changes error out.  Please let me know if the tests look  
alright.

> Please run the tests when working on code. [...]

Right on, sorry about that.

I ran the selftest like you suggested:

./bzr selftest [Mm]erge

and got a failure:

bzrlib.plugins.rebase.test_blackbox.TestRebaseSimple.test_pending_merges

But my changes don't appear to be the cause.  I reverted all my  
changes out, ran selftest again and it failed with the same test.  I  
think I'm safe this time.  :)

> By inverting the boolean, you are forcing it to check only when -- 
> force
> is specified.  You should be specifying False.

Oop, ok, fixed.

Third time's a charm?

Thanks for the help, Aaron!
-Roberto.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: check-uncommitted-before-merge.patch
Type: application/octet-stream
Size: 7357 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090227/8d93c206/attachment.obj 
-------------- next part --------------