root-ids changing for some merges

[vila]

>>>>> Aaron Bentley <aaron at aaronbentley.com> writes:

    > On 05/07/2011 2:48 PM, vila wrote:
    >>>>>>> Aaron Bentley<aaron at aaronbentley.com>  writes:
    >> 
    >> >  On 11-07-05 12:38 PM, vila wrote:
    >> >>  The drawback that I just realized is that it breaks 'merge -r0..nn
    >> >>  ../unrelated-branch' by forcing the new root which in turn trigger a
    >> >>  rename for all existing children of the old root
    >> 
    >> >  That is the expected behaviour.
    >> 
    >> It wasn't the behaviour *before* your patch, the root-id was preserved
    >> and the existing files in THIS weren't renamed.

    > This is true, but most branches do change behaviour.  I pointed
    > this change out when I proposed the merge, so by approving the
    > proposal, Jelmer approved the change in behaviour.

Neither the submitter nor the reviewer are required to be perfect,
errors happen. And Jelmer wasn't involved, that was jam and me and none
of us caught the issue. But I'd rather continue to accept submissions
and revert/fix them when needed than block them.

    > What's much more interesting is what behaviour do we want, and
    > why?

The one we had was fine (bar the empty branch/tree merge bugs).

    > I felt that since there's a tie to break (a tree can't have two
    > roots), it made a lot of sense to break the tie in favour of
    > OTHER, since that's how merge usually breaks ties.

    > You clearly prefer the old behaviour, so I'd like to know why.

I think both behaviors may be valid but we shouldn't switch if the new
one introduce unwanted changes. Why the test suite didn't expose this in a
more obvious way is another debate.

    >> >  Could you explain why that is a drawback?
    >> 
    >> Seeing all the files present in the branch before the merge being
    >> renamed to the same name is quite surprising.

    > Okay, so there's bad UI consequences.

Yes and I'm saying they are bad enough to be fixed before we release
2.4b5. If we can't, then I'd rather revert this change.

    > Anything else?

No idea yet.

    >> May be my test case is wrong and falsely alarming, yet, it passed on 2.2
    >> and 2.3 but fails on trunk. Did you look at it ?

    > No.  I couldn't parse what you were saying.

vila trying mister boss, vila trying:

class TestNoFinalPath(script.TestCaseWithTransportAndScript):

    def test_bug_805809(self):
        self.run_script("""
$ bzr init trunk
Created a standalone tree (format: 2a)
$ cd trunk
$ echo trunk >file
$ bzr add
adding file
$ bzr commit -m 'create file on trunk'
2>Committing to: .../trunk/
2>added file
2>Committed revision 1.
# Create a debian branch based on trunk
$ cd ..
$ bzr branch trunk -r 1 debian
2>Branched 1 revision(s).
$ cd debian
$ mkdir dir
$ bzr add
adding dir
$ bzr mv file dir
file => dir/file
$ bzr commit -m 'rename file to dir/file for debian'
2>Committing to: .../debian/
2>added dir
2>renamed file => dir/file
2>Committed revision 2.
# Create an experimental branch with a new root-id
$ cd ..
$ bzr init experimental
Created a standalone tree (format: 2a)
$ cd experimental
$ echo not-empty-branch >not-empty
$ bzr add
adding not-empty
$ bzr commit -m 'Avoid merge into empty branch safeguard'
2>Committing to: .../experimental/
2>added not-empty
2>Committed revision 1.
# merge debian even without a common ancestor
$ bzr merge ../debian -r0..2
2>+N  dir/
2>+N  dir/file
2>-   /
2>All changes applied successfully.
$ bzr st

Running this led to:

Text attachment: traceback
------------
Traceback (most recent call last):
  File "/home/vila/lib/python/testtools/runtest.py", line 169, in _run_user
    return fn(*args, **kwargs)
  File "/home/vila/lib/python/testtools/testcase.py", line 540, in _run_test_method
    return self._get_test_method()()
  File "/home/vila/src/bzr/integration/trunk/bzrlib/tests/test_conflicts.py", line 1141, in test_bug_805809
    """)
  File "/home/vila/src/bzr/integration/trunk/bzrlib/tests/script.py", line 517, in run_script
    null_output_matches_anything=null_output_matches_anything)
  File "/home/vila/src/bzr/integration/trunk/bzrlib/tests/script.py", line 212, in run_script
    self.run_command(test_case, cmd, input, output, error)
  File "/home/vila/src/bzr/integration/trunk/bzrlib/tests/script.py", line 231, in run_command
    raise AssertionError(str(e) + " in stdout of command %s" % cmd)
AssertionError: texts not equal:
- 
- 
+ added:
+   dir/
+   dir/file
+ renamed:
+   not-empty => not-empty
+ pending merge tips: (use -v to see all merge revisions)
+   jrandom at example.com 2011-07-06 rename file to dir/file for debian
 in stdout of command ['bzr', 'st']

The not-empty => not-empty is the surprising bit I was referring to.

    Vincent