Merge lp:~vila/bzr/284038-push-strict into lp:~bzr/bzr/trunk-old

[Vincent Ladeuil]

[Vincent Ladeuil (vila)]

This complements my first attempt to fix the bug while also fixing bugs #322808 and #65286,
by making --strict defaults to true.

Changing the default value revealed a bug in the previous implementation as push can be used without
a working tree.

Also the --strict option is meaningless if the --revision option is used.

Previous reviews mentioned introducing:
- a way to query config files for a bool option,
- a new method on working tree for checking changes.

Since both are also needed for bug #206577, I'd like to postpone these modifications until both fixes
have landed.

[John A Meinel (jameinel)]

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

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/284038-push-strict into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> This complements my first attempt to fix the bug while also fixing bugs #322808 and #65286,
> by making --strict defaults to true.
>
> Changing the default value revealed a bug in the previous implementation as push can be used without
> a working tree.
>
> Also the --strict option is meaningless if the --revision option is used.
>
> Previous reviews mentioned introducing:
> - a way to query config files for a bool option,
> - a new method on working tree for checking changes.
>
> Since both are also needed for bug #206577, I'd like to postpone these modifications until both fixes
> have landed.
>

To start with, I don't remember an agreement that strict should be the
default.

+ The ``push_strict`` option can be declared in a configuration
+ file. option is used.

^- The news entry needs a bit of updating. Something like:

The configuration option ``push_strict`` can be used to set the default
for this behavior.

...

+ if (tree is not None and revision_id is None
+ and (strict is None or strict)): # Default to True:
+ changes = tree.changes_from(tree.basis_tree())
+ if changes.has_changed() or len(tree.get_parent_ids()) > 1:
+ raise errors.UncommittedChanges(tree)

Rather than doing 'strict is None', just set:

def run(..., strict=True):

if we want that to be the default.

So provided that there was actually agreement that strict should be the
default, the patch seems ok with some small tweaks.

I'll note that "commit" is not strict by default, and I've often used
push before commit, especially if I'm about to switch.

I could agree that strict=True is the 'safer' default for most people,
so having to edit a config to turn it off is the "power-user" approach.

I suppose we could land it and see what we think...

 vote needs_fixing

Mostly I want to make sure that we did actually agree that we should
default to strict.

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

iEYEARECAAYFAkpIwN8ACgkQJdeBCYSNAAOuRQCdEXhhM7Z9KVXOjQZYAMKJPpYV
S9EAoJ90HFDdgbzP7/JRf4JM4YxO4B0N
=uiFO
-----END PGP SIGNATURE-----

review: Needs Fixing

[Vincent Ladeuil (vila)]

>>>>> "jam" == John A Meinel <email address hidden> writes:

<snip/>

    jam> To start with, I don't remember an agreement that strict
    jam> should be the default.

https://bugs.edge.launchpad.net/bzr/+bug/322808/comments/2
https://bugs.edge.launchpad.net/bzr/+bug/65286/comments/1
https://bugs.edge.launchpad.net/bzr/+bug/65286/comments/2

I interpreted that as an agreement, I also agree with the idea
that it may be confusing for new users if left off by default.

    jam> + The ``push_strict`` option can be declared in a configuration
    jam> + file. option is used.

    jam> ^- The news entry needs a bit of updating. Something like:

    jam> The configuration option ``push_strict`` can be used to set the default
    jam> for this behavior.

Right.

    jam> ...

    jam> + if (tree is not None and revision_id is None
    jam> + and (strict is None or strict)): # Default to True:
    jam> + changes = tree.changes_from(tree.basis_tree())
    jam> + if changes.has_changed() or len(tree.get_parent_ids()) > 1:
    jam> + raise errors.UncommittedChanges(tree)

    jam> Rather than doing 'strict is None', just set:

    jam> def run(..., strict=True):

    jam> if we want that to be the default.

No, because if we do that, we can't distinguish anymore between
the config file variable and the command-line option (the later
should still override the former).

    jam> So provided that there was actually agreement that strict should be the
    jam> default, the patch seems ok with some small tweaks.

    jam> I'll note that "commit" is not strict by default, and
    jam> I've often used push before commit, especially if I'm
    jam> about to switch.

Feel free to add 'push_strict=False' in your bazaar.conf file,
you're not a *new* user :-)

    jam> I could agree that strict=True is the 'safer' default
    jam> for most people, so having to edit a config to turn it
    jam> off is the "power-user" approach.

Indeed.

Since my other merge proposal (for send/bundle --strict) will
raise, at least, the exact same questions, I'll wait for your
answer here before landing.

        Vincent

[Vincent Ladeuil (vila)]

>>>>> "jam" == John A Meinel <email address hidden> writes:

<snip/>

    jam> To start with, I don't remember an agreement that strict
    jam> should be the default.

https://bugs.edge.launchpad.net/bzr/+bug/322808/comments/2
https://bugs.edge.launchpad.net/bzr/+bug/65286/comments/1
https://bugs.edge.launchpad.net/bzr/+bug/65286/comments/2

I interpreted that as an agreement, I also agree with the idea
that it may be confusing for new users if left off by default.

    jam> + The ``push_strict`` option can be declared in a configuration
    jam> + file. option is used.

    jam> ^- The news entry needs a bit of updating. Something like:

    jam> The configuration option ``push_strict`` can be used to set the default
    jam> for this behavior.

Right.

    jam> ...

    jam> + if (tree is not None and revision_id is None
    jam> + and (strict is None or strict)): # Default to True:
    jam> + changes = tree.changes_from(tree.basis_tree())
    jam> + if changes.has_changed() or len(tree.get_parent_ids()) > 1:
    jam> + raise errors.UncommittedChanges(tree)

    jam> Rather than doing 'strict is None', just set:

    jam> def run(..., strict=True):

    jam> if we want that to be the default.

No, because if we do that, we can't distinguish anymore between
the config file variable and the command-line option (the later
should still override the former).

    jam> So provided that there was actually agreement that strict should be the
    jam> default, the patch seems ok with some small tweaks.

    jam> I'll note that "commit" is not strict by default, and
    jam> I've often used push before commit, especially if I'm
    jam> about to switch.

Feel free to add 'push_strict=False' in your bazaar.conf file,
you're not a *new* user :-)

    jam> I could agree that strict=True is the 'safer' default
    jam> for most people, so having to edit a config to turn it
    jam> off is the "power-user" approach.

Indeed.

Since my other merge proposal (for send/bundle --strict) will
raise, at least, the exact same questions, I'll wait for your
answer here before landing.

        Vincent

[John A Meinel (jameinel)]

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

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/284038-push-strict into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> This complements my first attempt to fix the bug while also fixing bugs #322808 and #65286,
> by making --strict defaults to true.
>
> Changing the default value revealed a bug in the previous implementation as push can be used without
> a working tree.
>
> Also the --strict option is meaningless if the --revision option is used.
>
> Previous reviews mentioned introducing:
> - a way to query config files for a bool option,
> - a new method on working tree for checking changes.
>
> Since both are also needed for bug #206577, I'd like to postpone these modifications until both fixes
> have landed.
>

Also...

We should *definitely* be checking:

 tree.last_revision() == tree.branch.last_revision()

And possibly consider what to do in the case of bound branches and
heavyweight checkouts.
I'm not *as* concerned about the latter, but because in the former case,
IIRC we will actually be pushing "tree.branch.last_revision()" *not*
tree.last_revision()...

So certainly '--strict' should fail if the tree and branch aren't at the
same rev.

John
=:->

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

iEYEARECAAYFAkpIxUIACgkQJdeBCYSNAAPc/wCfVTwT8aIl1QBDg4ho7rv/C1zI
3e4AniMO3HZD1SgHyMKOAG0uXZY0qs2g
=Qf45
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

>>>>> "jam" == John A Meinel <email address hidden> writes:

<snip/>

    jam> Also...

    jam> We should *definitely* be checking:

    jam> tree.last_revision() == tree.branch.last_revision()

    jam> And possibly consider what to do in the case of bound
    jam> branches and heavyweight checkouts. I'm not *as*
    jam> concerned about the latter, but because in the former
    jam> case, IIRC we will actually be pushing
    jam> "tree.branch.last_revision()" *not*
    jam> tree.last_revision()...

Hmm, so far --strict is defined as triggering if there are
*uncommitted* changes, you're stretching the definition here...

    jam> So certainly '--strict' should fail if the tree and
    jam> branch aren't at the same rev.

I'm not sure I agree with you (nor that I disagree), but I think
that's worth filing a different bug.

       Vincent

[John A Meinel (jameinel)]

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

Vincent Ladeuil wrote:
>>>>>> "jam" == John A Meinel <email address hidden> writes:
>
> <snip/>
>
> jam> Also...
>
> jam> We should *definitely* be checking:
>
> jam> tree.last_revision() == tree.branch.last_revision()
>
> jam> And possibly consider what to do in the case of bound
> jam> branches and heavyweight checkouts. I'm not *as*
> jam> concerned about the latter, but because in the former
> jam> case, IIRC we will actually be pushing
> jam> "tree.branch.last_revision()" *not*
> jam> tree.last_revision()...
>
> Hmm, so far --strict is defined as triggering if there are
> *uncommitted* changes, you're stretching the definition here...

So I can agree that it is a different bug except:

1) I think 'bzr push --strict' is about 'push should push the state I
*think* I have'. And uncommitted changes, (uncommitted merges), and a
tree last revision being out of sync with the branch's revision, are all
cases where the push isn't pushing what the user expected.

2) I think the source of '--strict' is MySQL fallout from switching from
BK where a merge had autocommit so "bk merge; bk push" did something,
while with bzr you need "bzr merge; bzr commit; bzr push".

The fact that they haven't run into issues with a workingtree being out
of sync with the branch is more luck than noticing that we have a
serious flaw.

3) The fix is fairly trivial, the bug is somewhat serious, and you're
already working on the code. Yes it is feature creep, but it is a small
amount for something worthwhile (IMO).

>
> jam> So certainly '--strict' should fail if the tree and
> jam> branch aren't at the same rev.
>
> I'm not sure I agree with you (nor that I disagree), but I think
> that's worth filing a different bug.
>
> Vincent
>

I read your links, and while there wasn't a general discussion on it,
Martin at least seems to agree it should be the default. I'm just a bit
extra sensitive to commandline behavioral changes. Since suddenly
something that used to work won't anymore.

Also, note that "UncommittedChanges" has no help text to tell the user
that they can use "--no-strict" in order to push anyway. I don't know if
that matters or not. But since we are explictly changing a behavior, it
is good to help users find out how to get their old behavior back.

Anyway, you can probably merge this as is, though I'd like it if you
added a tree.last_revision() check.

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

iEYEARECAAYFAkpI7LsACgkQJdeBCYSNAANHawCgzbfvzWlHnaFF07twKpOsj9Dd
dioAoLoZtL3GnDb1V05wi0kQyjDaeE0y
=a+Cd
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

>>>>> "jam" == John A Meinel writes:

    jam> Vincent Ladeuil wrote:
    >>>>>>> "jam" == John A Meinel writes:
    >>
    >> <snip/>
    >>
    jam> Also...
    jam> We should *definitely* be checking:
    jam> tree.last_revision() == tree.branch.last_revision()

    jam> And possibly consider what to do in the case of bound
    jam> branches and heavyweight checkouts. I'm not *as*
    jam> concerned about the latter, but because in the former
    jam> case, IIRC we will actually be pushing
    jam> "tree.branch.last_revision()" *not*
    jam> tree.last_revision()...
    >>
    >> Hmm, so far --strict is defined as triggering if there are
    >> *uncommitted* changes, you're stretching the definition here...

    jam> So I can agree that it is a different bug except:

    jam> 1) I think 'bzr push --strict' is about 'push should
    jam> push the state I *think* I have'.

I understood that :)

    jam> And uncommitted changes, (uncommitted merges), and a
    jam> tree last revision being out of sync with the branch's
    jam> revision, are all cases where the push isn't pushing
    jam> what the user expected.

Right. You're right. I agree with the rationale, but... I don't
clearly understand when a user can find itself in this
situation... I can see one case for a bound branch, except that I
can't commit in that situation so I have nothing to push... Or
maybe you mean a commit --local ? But in that case push should
already fail with "diverged branches" (strict or no strict) no ?

    jam> 2) I think the source of '--strict' is MySQL fallout
    jam> from switching from BK where a merge had autocommit so
    jam> "bk merge; bk push" did something, while with bzr you
    jam> need "bzr merge; bzr commit; bzr push".

<snip/>

I agree here too, mysql workflow wasn't my main reason for
agreeing about making --strict the default. In fact my first fix
addressed mysql workflow *without* making it he default.

I think the default should be strict because uneducated users
will easily forget to commit before pushing, and even experienced
ones may prefer, for some branches, to work in strict mode.

    jam> 3) The fix is fairly trivial, the bug is somewhat
    jam> serious, and you're already working on the code. Yes it
    jam> is feature creep, but it is a small amount for something
    jam> worthwhile (IMO).

I don't have a problem with working more on that, but I just
don't know how to write the tests (the what, not the how).

    jam> I read your links, and while there wasn't a general
    jam> discussion on it, Martin at least seems to agree it
    jam> should be the default.

Sorry to have jumped to that conclusion without discussing it
first then.

    jam> I'm just a bit extra sensitive to commandline behavioral
    jam> changes. Since suddenly something that used to work
    jam> won't anymore.

Well, I thought about that and I came to the conclusion that it's
pretty rare than one find itself in the situation to push while
having uncommitted changes. At least to me, it sounds like
advanced usage and I don't mind having to add an option in that
case, even if I didn't have to do it before, especially since I
can configure that behavior at ...

[John A Meinel (jameinel)]

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

Vincent Ladeuil wrote:
>>>>>> "jam" == John A Meinel writes:
>
> jam> Vincent Ladeuil wrote:
> >>>>>>> "jam" == John A Meinel writes:
> >>
> >> <snip/>
> >>
> jam> Also...
> jam> We should *definitely* be checking:
> jam> tree.last_revision() == tree.branch.last_revision()
>
> jam> And possibly consider what to do in the case of bound
> jam> branches and heavyweight checkouts. I'm not *as*
> jam> concerned about the latter, but because in the former
> jam> case, IIRC we will actually be pushing
> jam> "tree.branch.last_revision()" *not*
> jam> tree.last_revision()...
> >>
> >> Hmm, so far --strict is defined as triggering if there are
> >> *uncommitted* changes, you're stretching the definition here...
>
> jam> So I can agree that it is a different bug except:
>
> jam> 1) I think 'bzr push --strict' is about 'push should
> jam> push the state I *think* I have'.
>
> I understood that :)
>
> jam> And uncommitted changes, (uncommitted merges), and a
> jam> tree last revision being out of sync with the branch's
> jam> revision, are all cases where the push isn't pushing
> jam> what the user expected.
>
> Right. You're right. I agree with the rationale, but... I don't
> clearly understand when a user can find itself in this
> situation... I can see one case for a bound branch, except that I
> can't commit in that situation so I have nothing to push... Or
> maybe you mean a commit --local ? But in that case push should
> already fail with "diverged branches" (strict or no strict) no ?
>
> jam> 2) I think the source of '--strict' is MySQL fallout
> jam> from switching from BK where a merge had autocommit so
> jam> "bk merge; bk push" did something, while with bzr you
> jam> need "bzr merge; bzr commit; bzr push".
>
> <snip/>
>
> I agree here too, mysql workflow wasn't my main reason for
> agreeing about making --strict the default. In fact my first fix
> addressed mysql workflow *without* making it he default.
>
> I think the default should be strict because uneducated users
> will easily forget to commit before pushing, and even experienced
> ones may prefer, for some branches, to work in strict mode.
>
> jam> 3) The fix is fairly trivial, the bug is somewhat
> jam> serious, and you're already working on the code. Yes it
> jam> is feature creep, but it is a small amount for something
> jam> worthwhile (IMO).
>
> I don't have a problem with working more on that, but I just
> don't know how to write the tests (the what, not the how).
>
> jam> I read your links, and while there wasn't a general
> jam> discussion on it, Martin at least seems to agree it
> jam> should be the default.
>
> Sorry to have jumped to that conclusion without discussing it
> first then.
>
> jam> I'm just a bit extra sensitive to commandline behavioral
> jam> changes. Since suddenly something that used to work
> jam> won't anymore.
>
> Well, I thought about that and I came to the conclusion that it's
> pretty rare than one find itself in the situation to push whi...

=== modified file 'NEWS'
--- NEWS	2009-06-30 05:34:47 +0000
+++ NEWS	2009-06-30 08:35:22 +0000
@@ -21,9 +21,10 @@
 New Features
 ************
 
-* ``bzr push`` now checks if uncommitted changes are present in the working
-  tree if the ``--strict`` option is used.
-  (Vincent Ladeuil, #284038)
+* ``bzr push`` now aborts if uncommitted changes (including pending merges)
+  are present in the working tree (if one is present) and no revision is
+  speficied. The ``push_strict`` option can be declared in a configuration
+  file.  option is used.  (Vincent Ladeuil, #284038, #322808, #65286)
 
 
 Bug Fixes

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2009-06-26 03:44:30 +0000
+++ bzrlib/builtins.py	2009-06-30 08:35:22 +0000
@@ -1057,16 +1057,17 @@
                     strict = bools[strict.lower()]
                 except KeyError:
                     strict = None
-        if strict:
-            changes = tree.changes_from(tree.basis_tree())
-            if changes.has_changed():
-                raise errors.UncommittedChanges(tree)
         # Get the tip's revision_id
         revision = _get_one_revision('push', revision)
         if revision is not None:
             revision_id = revision.in_history(br_from).rev_id
         else:
             revision_id = None
+        if (tree is not None and revision_id is None
+            and (strict is None or strict)): # Default to True:
+            changes = tree.changes_from(tree.basis_tree())
+            if changes.has_changed() or len(tree.get_parent_ids()) > 1:
+                raise errors.UncommittedChanges(tree)
 
         # Get the stacked_on branch, if any
         if stacked_on is not None:

=== modified file 'bzrlib/tests/blackbox/test_push.py'
--- bzrlib/tests/blackbox/test_push.py	2009-06-22 08:14:24 +0000
+++ bzrlib/tests/blackbox/test_push.py	2009-06-30 08:35:22 +0000
@@ -35,6 +35,28 @@
 from bzrlib.transport import memory
 
 
+def load_tests(standard_tests, module, loader):
+    """Multiply tests for the push command."""
+    result = loader.suiteClass()
+
+    # one for each king of change
+    changes_tests, remaining_tests = tests.split_suite_by_condition(
+        standard_tests, tests.condition_isinstance((
+                TestPushStrictWithChanges,
+                )))
+    changes_scenarios = [
+        ('uncommitted',
+         dict(_changes_type= '_uncommitted_changes')),
+        ('pending_merges',
+         dict(_changes_type= '_pending_merges')),
+        ]
+    tests.multiply_tests(changes_tests, changes_scenarios, result)
+    # No parametrization for the remaining tests
+    result.addTests(remaining_tests)
+
+    return result
+
+
 class TestPush(tests.TestCaseWithTransport):
 
     def test_push_error_on_vfs_http(self):
@@ -569,72 +591,112 @@
 class TestPushStrict(tests.TestCaseWithTransport):
 
     def make_local_branch_and_tree(self):
-        tree = self.make_branch_and_tree('local')
+        self.tree = self.make_branch_and_tree('local')
         self.build_tree_contents([('local/file', 'initial')])
-        tree.add('file')
-        tree.commit('adding file', rev_id='from-1')
-        return tree
-
-    def make_local_branch_and_tree_with_changes(self):
-        tree = self.make_local_branch_and_tree()
-        # Make some changes
+        self.tree.add('file')
+        self.tree.commit('adding file', rev_id='added')
         self.build_tree_contents([('local/file', 'modified')])
-        return tree
+        self.tree.commit('modify file', rev_id='modified')
 
-    def set_config_push_strict(self, tree, value):
+    def set_config_push_strict(self, value):
         # set config var (any of bazaar.conf, locations.conf, branch.conf
         # should do)
-        conf = tree.branch.get_config()
+        conf = self.tree.branch.get_config()
         conf.set_user_option('push_strict', value)
 
-    def assertPushFails(self, location, *args):
+    def assertPushFails(self, args):
         self.run_bzr_error(['Working tree ".*/local/"'
                             ' has uncommitted changes.$',],
-                           ['push', '../' + location] + list(args),
+                           ['push', '../to'] + args,
                            working_dir='local', retcode=3)
 
-    def assertPushSucceeds(self, location, *args):
-        self.run_bzr(['push', '../' + location] + list(args),
+    def assertPushSucceeds(self, args, pushed_revid=None):
+        self.run_bzr(['push', '../to'] + args,
                      working_dir='local')
-        tree_to = workingtree.WorkingTree.open(location)
+        if pushed_revid is None:
+            pushed_revid = 'modified'
+        tree_to = workingtree.WorkingTree.open('to')
         repo_to = tree_to.branch.repository
-        self.assertTrue(repo_to.has_revision('from-1'))
-        self.assertEqual(tree_to.branch.last_revision_info()[1], 'from-1')
-
-    def test_push_default(self):
-        tree = self.make_local_branch_and_tree_with_changes()
-        self.assertPushSucceeds('to')
-
-    def test_push_no_strict_with_changes(self):
-        tree = self.make_local_branch_and_tree_with_changes()
-        self.assertPushSucceeds('to', '--no-strict')
+        self.assertTrue(repo_to.has_revision(pushed_revid))
+        self.assertEqual(tree_to.branch.last_revision_info()[1], pushed_revid)
+
+
+
+class TestPushStrictWithoutChanges(TestPushStrict):
+
+    def setUp(self):
+        super(TestPushStrictWithoutChanges, self).setUp()
+        self.make_local_branch_and_tree()
+
+    def test_push_default(self):
+        self.assertPushSucceeds([])
+
+    def test_push_strict(self):
+        self.assertPushSucceeds(['--strict'])
+
+    def test_push_no_strict(self):
+        self.assertPushSucceeds(['--no-strict'])
+
+    def test_push_config_var_strict(self):
+        self.set_config_push_strict('true')
+        self.assertPushSucceeds([])
+
+    def test_push_config_var_no_strict(self):
+        self.set_config_push_strict('false')
+        self.assertPushSucceeds([])
+
+
+class TestPushStrictWithChanges(TestPushStrict):
+
+    _changes_type = None # Set by load_tests
+
+    def setUp(self):
+        super(TestPushStrictWithChanges, self).setUp()
+        getattr(self, self._changes_type)()
+
+    def _uncommitted_changes(self):
+        self.make_local_branch_and_tree()
+        # Make a change without committing it
+        self.build_tree_contents([('local/file', 'in progress')])
+
+    def _pending_merges(self):
+        self.make_local_branch_and_tree()
+        # Create 'other' branch containing a new file
+        other_bzrdir = self.tree.bzrdir.sprout('other')
+        other_tree = other_bzrdir.open_workingtree()
+        self.build_tree_contents([('other/other-file', 'other')])
+        other_tree.add('other-file')
+        other_tree.commit('other commit', rev_id='other')
+        # Merge and revert, leaving a pending merge
+        self.tree.merge_from_branch(other_tree.branch)
+        self.tree.revert(filenames=['other-file'], backups=False)
+
+    def test_push_default(self):
+        self.assertPushFails([])
+
+    def test_push_with_revision(self):
+        self.assertPushSucceeds(['-r', 'revid:added'], pushed_revid='added')
+
+    def test_push_no_strict(self):
+        self.assertPushSucceeds(['--no-strict'])
 
     def test_push_strict_with_changes(self):
-        tree = self.make_local_branch_and_tree_with_changes()
-        self.assertPushFails('to', '--strict')
-
-    def test_push_strict_without_changes(self):
-        tree = self.make_local_branch_and_tree()
-        self.assertPushSucceeds('to', '--strict')
+        self.assertPushFails(['--strict'])
 
     def test_push_respect_config_var_strict(self):
-        tree = self.make_local_branch_and_tree_with_changes()
-        self.set_config_push_strict(tree, 'true')
-        self.assertPushFails('to')
+        self.set_config_push_strict('true')
+        self.assertPushFails([])
 
     def test_push_bogus_config_var_ignored(self):
-        tree = self.make_local_branch_and_tree_with_changes()
-        self.set_config_push_strict(tree, "I don't want you to be strict")
-        self.assertPushSucceeds('to')
+        self.set_config_push_strict("I don't want you to be strict")
+        self.assertPushFails([])
 
     def test_push_no_strict_command_line_override_config(self):
-        tree = self.make_local_branch_and_tree_with_changes()
-        self.set_config_push_strict(tree, 'yES')
-        self.assertPushFails('to')
-        self.assertPushSucceeds('to', '--no-strict')
+        self.set_config_push_strict('yES')
+        self.assertPushFails([])
+        self.assertPushSucceeds(['--no-strict'])
 
     def test_push_strict_command_line_override_config(self):
-        tree = self.make_local_branch_and_tree_with_changes()
-        self.set_config_push_strict(tree, 'oFF')
-        self.assertPushFails('to', '--strict')
-        self.assertPushSucceeds('to')
+        self.set_config_push_strict('oFF')
+        self.assertPushFails(['--strict'])
+        self.assertPushSucceeds([])