Merge lp:~parthm/bzr/405192-get_nick-recursion-2.2 into lp:bzr/2.2

[Parth Malwankar]

Detect recursive binding for checkouts and give a clear error message to user. (#405192)

=== Fixes bug #405192 ===
This patch fixes branch._get_nick to detect recursive binding and give a clear error message to the user.

Sample output:
[bzr-grep-checkout]% ~/src/bzr.dev/405192-get_nick-recursion-2.2/bzr ci -m "added z"
bzr: ERROR: Recursive binding detected.
Use `bzr info` to verify and `bzr unbind|bind` to fix.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Parth Malwankar wrote:
> Parth Malwankar has proposed merging lp:~parthm/bzr/405192-get_nick-recursion-2.2 into lp:bzr/2.2.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #405192 Self-bound checkouts errors with infinite recursion in _get_nick
> https://bugs.launchpad.net/bugs/405192
>
>
> === Fixes bug #405192 ===
> This patch fixes branch._get_nick to detect recursive binding and give a clear error message to the user.
>
> Sample output:
> [bzr-grep-checkout]% ~/src/bzr.dev/405192-get_nick-recursion-2.2/bzr ci -m "added z"
> bzr: ERROR: Recursive binding detected.
> Use `bzr info` to verify and `bzr unbind|bind` to fix.

I would probably change the error message a little bit. Something like:

 "Branch %s appears to be bound to itself, please unbind"

Otherwise the user doesn't really know any more about what to do, or
what branch is actually involved. (lightweight checkouts of a bound
branch, etc.)

Otherwise, looks good to me.

 review: needsfixing

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkw9d5QACgkQJdeBCYSNAAP3fwCeNwz3WU3UUklUhpJgiFkIuCUg
kO4AoNezZxiyaXh2HSikVp7dUUy0seUR
=AUlG
-----END PGP SIGNATURE-----

review: Needs Fixing

[Parth Malwankar (parthm)]

> I would probably change the error message a little bit. Something like:
>
> "Branch %s appears to be bound to itself, please unbind"
>

I have fixed the error message. Thanks for the review.

Sample output:
[bzr-grep-checkout]% ~/src/bzr.dev/405192-get_nick-recursion-2.2/bzr ci -m "added z"
bzr: ERROR: Branch "file:///home/parthm/tmp/bzr-grep-checkout/" appears to be bound to itself. Please use `bzr unbind` to fix.
[bzr-grep-checkout]%

[John A Meinel (jameinel)]

review: Approve

[John A Meinel (jameinel)]

I can't mark this approved because of permission issues, but feel free to merge this.

[Parth Malwankar (parthm)]

OK. I will go ahead and merge this. Thanks.

[Parth Malwankar (parthm)]

Hmm. Looks like I can't mark the overall status as "approved", it doesn't show up in the list. Is this new in launchpad? I will try to merge it anyway if feed-pqm allows me.

review: Approve

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Parth Malwankar wrote:
> Review: Approve
> Hmm. Looks like I can't mark the overall status as "approved", it doesn't show up in the list. Is this new in launchpad? I will try to merge it anyway if feed-pqm allows me.
>

feed-pqm won't, because it isn't marked Approved. You can't mark the
status for the same reason I can't, bzr/2.2 doesn't have a Reviewer team
(such that neither of us are members).

You could probably just use pqm-submit directly, but if you are
unfamiliar with that, I can do it for now.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkw9i7sACgkQJdeBCYSNAANE8QCfe4PC6Uf+gVnabbjQjY0YAW/r
37wAn3RmVhysc4v0Al/YaAhNTj08XM29
=e+bf
-----END PGP SIGNATURE-----

[Parth Malwankar (parthm)]

> feed-pqm won't, because it isn't marked Approved. You can't mark the
> status for the same reason I can't, bzr/2.2 doesn't have a Reviewer team
> (such that neither of us are members).
>

You are right. It didn't get listed.

> You could probably just use pqm-submit directly, but if you are
> unfamiliar with that, I can do it for now.
>

I am not very familiar with pqm-submit. It would be great if you could submit this. Thanks.

=== modified file 'NEWS'
--- NEWS	2010-07-13 16:17:52 +0000
+++ NEWS	2010-07-14 09:01:44 +0000
@@ -27,6 +27,9 @@
 * Don't traceback trying to unversion children files of an already
   unversioned directory.  (Vincent Ladeuil, #494221)
 
+* Recursive binding for checkouts is now detected by bzr. A clear error
+  message is shown to the user. (Parth Malwankar, #405192)
+
 Improvements
 ************
 

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2010-06-30 08:34:11 +0000
+++ bzrlib/branch.py	2010-07-14 09:01:44 +0000
@@ -246,9 +246,13 @@
         if not local and not config.has_explicit_nickname():
             try:
                 master = self.get_master_branch(possible_transports)
+                if master and self.user_url == master.user_url:
+                    raise errors.RecursiveBind(self.user_url)
                 if master is not None:
                     # return the master branch value
                     return master.nick
+            except errors.RecursiveBind, e:
+                raise e
             except errors.BzrError, e:
                 # Silently fall back to local implicit nick if the master is
                 # unavailable

=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2010-07-09 16:16:11 +0000
+++ bzrlib/errors.py	2010-07-14 09:01:44 +0000
@@ -3149,12 +3149,14 @@
     def __init__(self, bzrdir):
         self.bzrdir = bzrdir
 
+
 class NoWhoami(BzrError):
 
     _fmt = ('Unable to determine your name.\n'
         "Please, set your name with the 'whoami' command.\n"
         'E.g. bzr whoami "Your Name <[e-mail]>"')
 
+
 class InvalidPattern(BzrError):
 
     _fmt = ('Invalid pattern(s) found. %(msg)s')
@@ -3162,3 +3164,12 @@
     def __init__(self, msg):
         self.msg = msg
 
+
+class RecursiveBind(BzrError):
+
+    _fmt = ('Branch "%(branch_url)s" appears to be bound to itself. '
+        'Please use `bzr unbind` to fix.')
+
+    def __init__(self, branch_url):
+        self.branch_url = branch_url
+

=== modified file 'bzrlib/tests/blackbox/test_commit.py'
--- bzrlib/tests/blackbox/test_commit.py	2010-06-28 02:41:22 +0000
+++ bzrlib/tests/blackbox/test_commit.py	2010-07-14 09:01:44 +0000
@@ -22,6 +22,7 @@
 import sys
 
 from bzrlib import (
+    bzrdir,
     osutils,
     ignores,
     msgeditor,
@@ -757,3 +758,18 @@
         osutils.set_or_unset_env('BZR_EMAIL', None)
         out, err = self.run_bzr(['commit', '-m', 'initial'], 3)
         self.assertContainsRe(err, 'Unable to determine your name')
+
+    def test_commit_recursive_checkout(self):
+        """Ensure that a commit to a recursive checkout fails cleanly.
+        """
+        self.run_bzr(['init', 'test_branch'])
+        self.run_bzr(['checkout', 'test_branch', 'test_checkout'])
+        os.chdir('test_checkout')
+        self.run_bzr(['bind', '.']) # bind to self
+        open('foo.txt', 'w').write('hello')
+        self.run_bzr(['add'])
+        out, err = self.run_bzr(['commit', '-m', 'addedfoo'], 3)
+        self.assertEqual(out, '')
+        self.assertContainsRe(err,
+            'Branch.*test_checkout.*appears to be bound to itself')
+

=== modified file 'bzrlib/tests/test_errors.py'
--- bzrlib/tests/test_errors.py	2010-07-09 16:16:11 +0000
+++ bzrlib/tests/test_errors.py	2010-07-14 09:01:44 +0000
@@ -660,6 +660,12 @@
         self.assertEqualDiff("Invalid pattern(s) found. Bad pattern msg.",
             str(error))
 
+    def test_recursive_bind(self):
+        error = errors.RecursiveBind('foo_bar_branch')
+        msg = ('Branch "foo_bar_branch" appears to be bound to itself. '
+            'Please use `bzr unbind` to fix.')
+        self.assertEqualDiff(msg, str(error))
+
 
 class PassThroughError(errors.BzrError):