Merge lp:~vila/bzr/401599-strict-warnings into lp:bzr

[Vincent Ladeuil]

Thanks for the heads-up John.
This patch fixes the warning message displayed when we handle --strict/--no-strict
for dpush, push and send. The additional part mentioniing --no-strict should only be
mentioned if --strict is used (or the conf variable set to True), *not* when we just
display the warning.

Note that tests have not been updated since they were testing the correct behaviour.

[lp:~vila/bzr/401599-strict-warnings]

5173. By Vincent Ladeuil on 2010-04-22

Explain that the uncommitted changes are not processed when
issuing the warning.

* bzrlib/mutabletree.py:
(MutableTree.check_changed_or_out_of_date): Use diferent 'more'
arguments depending on whether we issue a warning or an error.

* bzrlib/send.py:
(send): Add the more_warnings argument when calling
check_changed_or_out_of_date.

* bzrlib/foreign.py:
(cmd_dpush.run): Add the more_warnings argument when calling
check_changed_or_out_of_date.

* bzrlib/builtins.py:
(cmd_push.run): Add the more_warnings argument when calling
check_changed_or_out_of_date.

[Andrew Bennetts (spiv)]

+* Wrong section, should be 2.2b3, should be fixed at merge.

Don't forget to fix this bit!

Other than that, the changes here look ok. However, your cover letter says:

> Note that tests have not been updated since they were testing the correct behaviour.

Presumably this also means there are tests missing? i.e. that the incorrect behaviour does not happen?

e.g. shouldn't assertPushSucceeds explicitly assert that no warnings are emitted when with_warnings=False, perhaps by assertEquals('', stderr)? Or maybe assertNotContainsRe(stderr, 'Warning').

I think it would be good for the blackbox tests to default to asserting commands don't emit warnings.

review: Needs Fixing

[lp:~vila/bzr/401599-strict-warnings]

5174. By Vincent Ladeuil on 2010-04-28

Fixed as per Andrew's review.

* bzrlib/tests/blackbox/test_push.py:
(TestPushStrictMixin.assertPushFails): Ensure that the error
message mentions --no-strict.
(TestPushStrictMixin.assertPushSucceeds): Rely on branches only to
satisfy dpush needs. Check the warning in the error message.

* bzrlib/tests/blackbox/test_dpush.py:
(TestDpushStrictMixin): Simplified now that the base class do less
assumptions.

[Vincent Ladeuil (vila)]

>>>>> Andrew Bennetts <email address hidden> writes:

    > Review: Needs Fixing
    > +* Wrong section, should be 2.2b3, should be fixed at merge.

    > Don't forget to fix this bit!

Sure. By the way, how could your news_merge plugin help here ? For
branches started before a release but that needs to land after a release
(or more generally for daggy-fix branches) it would be nice to have a
way defined workflow that can be merged automatically. One possible way
is to define a fictional "merge target" release and create an entry
there may be ?

    > Other than that, the changes here look ok. However, your cover letter says:

    >> Note that tests have not been updated since they were testing the correct behaviour.

    > Presumably this also means there are tests missing? i.e. that the
    > incorrect behaviour does not happen?

No, that part is covered. The missing bit is about the precise content
of the message.

    > e.g. shouldn't assertPushSucceeds explicitly assert that no
    > warnings are emitted when with_warnings=False, perhaps by
    > assertEquals('', stderr)? Or maybe assertNotContainsRe(stderr,
    > 'Warning').

Right, I'm not a big fan of assertContainsRe usually, but in this case,
trying to build the full error ouput is far too messy ('Created new
branch' for push, 'Pushed up to revision n' for dpush), so: Done.

I've simplified TestDpushStrictMixin as a drive-by.

    > I think it would be good for the blackbox tests to default to
    > asserting commands don't emit warnings.

[Andrew Bennetts (spiv)]

Vincent Ladeuil wrote:
> >>>>> Andrew Bennetts <email address hidden> writes:
>
> > Review: Needs Fixing
> > +* Wrong section, should be 2.2b3, should be fixed at merge.
>
> > Don't forget to fix this bit!
>
> Sure. By the way, how could your news_merge plugin help here ? For
> branches started before a release but that needs to land after a release
> (or more generally for daggy-fix branches) it would be nice to have a
> way defined workflow that can be merged automatically. One possible way
> is to define a fictional "merge target" release and create an entry
> there may be ?

news_merge can't help with that yet. news_merge would need some way to
know “this addition from OTHER should always be added to the latest
release”. I'm not sure if that should be done in the source branch (by
putting the new text in a placeholder release as you suggest), or by
configuration in PQM's bzr to say “news_merge should always add new
entries to top release of this branch's NEWS”, or something else.

> > Other than that, the changes here look ok. However, your cover letter says:
>
> >> Note that tests have not been updated since they were testing the correct behaviour.
>
> > Presumably this also means there are tests missing? i.e. that the
> > incorrect behaviour does not happen?
>
> No, that part is covered. The missing bit is about the precise content
> of the message.

I don't understand: if this was already covered, then the tests in trunk
should have been failing. They weren't failing, so therefore it's not
covered.

As a concrete example, test_push was *not* asserting that certain
invocations should be warning-free (only that certain other invocations
with warnings included a particular warning).

As I understand it, if no warnings or errors are expected from push,
then stderr should be empty, right? So assertPushSucceeds ought to just
assert that directly, rather than that hoping it knows about all the
likely warnings it should search for in stderr. And if it *does* expect
warnings, perhaps it can assert that every line of stderr matches an
expected warning, so that additional (and thus unexpected) warnings will
fail the test.

[Robert Collins (lifeless)]

+1

[Robert Collins (lifeless)]

Arrgh - +1 to Andrew's comment, I meant.

[Vincent Ladeuil (vila)]

>>>>> Andrew Bennetts <email address hidden> writes:

<snip/>

    >>
    >> No, that part is covered. The missing bit is about the precise
    >> content of the message.

    > I don't understand: if this was already covered, then the tests in
    > trunk should have been failing. They weren't failing, so
    > therefore it's not covered.

No, the behavior has always been correct, that's why the tests never
failed.

But as John mentioned in the bug, the message were misleading.

    > As a concrete example, test_push was *not* asserting that certain
    > invocations should be warning-free (only that certain other
    > invocations with warnings included a particular warning).

That part wasn't covered, the exact messages weren't checked, that's
what the bug was about.

    > As I understand it, if no warnings or errors are expected from push,
    > then stderr should be empty, right?

No, stderr still contains 'Created new branch' for push and ... I said
that already :-/

    > So assertPushSucceeds ought to just assert that directly, rather
    > than that hoping it knows about all the likely warnings it should
    > search for in stderr.

I think I don't understand you there, may be you should try it yourself ?

I tried to build the full expected output for the errors and ended up
with a far more complicated version that didn't bring any additional
benefit.

The version I've landed:
- ensure the behavior is correct (well, I didn't really change that
  except to reduce code duplication for dpush),
- ensure that the warning is absent/present when needed,
- ensure that the '--no-strict' part is mentioned when needed

    > And if it *does* expect warnings, perhaps it can assert that every
    > line of stderr matches an expected warning, so that additional
    > (and thus unexpected) warnings will fail the test.

The actual tests are focused, making them depend on 'Created new branch'
or 'Pushed up to revision n' will just make them more brittle against
unrelated changes. I.e. they will fail for the wrong reason.

[Andrew Bennetts (spiv)]

For the sake of curious onlookers:

We talked on IRC: there's some confusion in this discussion is that I considered what warnings are emitted to the user to be "behaviour", and Vincent didn't.

=== modified file 'NEWS'
--- NEWS	2010-04-28 09:40:23 +0000
+++ NEWS	2010-04-28 10:33:37 +0000
@@ -29,6 +29,7 @@
 Bug Fixes
 *********
 
+<<<<<<< TREE
 * ``.bazaar``, ``.bazaar/bazaar.conf`` and ``.bzr.log`` inherit user and
   group ownership from the containing directory. This allow bzr to work
   better with sudo.
@@ -115,6 +116,12 @@
 Bug Fixes
 *********
 
+=======
+* Wrong section, should be 2.2b3, should be fixed at merge.
+  Don't mention --no-strict when we just issue the warning about unclean trees.
+  (Vincent Ladeuil, #401599)
+
+>>>>>>> MERGE-SOURCE
 * ``bzr dpush``, ``bzr push`` and ``bzr send`` will now issue a warning
   instead of failing when dirty trees are involved. The corresponding
   ``dpush_strict``, ``push_strict`` and ``send_strict`` should be set to

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-04-28 09:40:23 +0000
+++ bzrlib/builtins.py	2010-04-28 10:33:37 +0000
@@ -1132,8 +1132,10 @@
         else:
             revision_id = None
         if tree is not None and revision_id is None:
-            tree.warn_if_changed_or_out_of_date(
-                strict, 'push_strict', 'Use --no-strict to force the push.')
+            tree.check_changed_or_out_of_date(
+                strict, 'push_strict',
+                more_error='Use --no-strict to force the push.',
+                more_warning='Uncommitted changes will not be pushed.')
         # Get the stacked_on branch, if any
         if stacked_on is not None:
             stacked_on = urlutils.normalize_url(stacked_on)

=== modified file 'bzrlib/foreign.py'
--- bzrlib/foreign.py	2010-04-20 22:38:03 +0000
+++ bzrlib/foreign.py	2010-04-28 10:33:37 +0000
@@ -297,8 +297,10 @@
             source_branch = Branch.open(directory)
             source_wt = None
         if source_wt is not None:
-            source_wt.warn_if_changed_or_out_of_date(
-                strict, 'dpush_strict', 'Use --no-strict to force the push.')
+            source_wt.check_changed_or_out_of_date(
+                strict, 'dpush_strict',
+                more_error='Use --no-strict to force the push.',
+                more_warning='Uncommitted changes will not be pushed.')
         stored_loc = source_branch.get_push_location()
         if location is None:
             if stored_loc is None:

=== modified file 'bzrlib/mutabletree.py'
--- bzrlib/mutabletree.py	2010-04-15 06:47:06 +0000
+++ bzrlib/mutabletree.py	2010-04-28 10:33:37 +0000
@@ -258,7 +258,8 @@
             return False
 
     @needs_read_lock
-    def warn_if_changed_or_out_of_date(self, strict, opt_name, more_msg):
+    def check_changed_or_out_of_date(self, strict, opt_name,
+                                     more_error, more_warning):
         """Check the tree for uncommitted changes and branch synchronization.
 
         If strict is None and not set in the config files, a warning is issued.
@@ -269,25 +270,29 @@
 
         :param opt_name: strict option name to search in config file.
 
-        :param more_msg: Details about how to avoid the warnings.
+        :param more_error: Details about how to avoid the check.
+
+        :param more_warning: Details about what is happening.
         """
         if strict is None:
             strict = self.branch.get_config().get_user_option_as_bool(opt_name)
         if strict is not False:
-            err = None
+            err_class = None
             if (self.has_changes()):
-                err = errors.UncommittedChanges(self, more=more_msg)
+                err_class = errors.UncommittedChanges
             elif self.last_revision() != self.branch.last_revision():
                 # The tree has lost sync with its branch, there is little
                 # chance that the user is aware of it but he can still force
                 # the action with --no-strict
-                err = errors.OutOfDateTree(self, more=more_msg)
-            if err is not None:
+                err_class = errors.OutOfDateTree
+            if err_class is not None:
                 if strict is None:
+                    err = err_class(self, more=more_warning)
                     # We don't want to interrupt the user if he expressed no
                     # preference about strict.
                     trace.warning('%s', err._format())
                 else:
+                    err = err_class(self, more=more_error)
                     raise err
 
     @needs_read_lock

=== modified file 'bzrlib/send.py'
--- bzrlib/send.py	2010-04-12 16:54:35 +0000
+++ bzrlib/send.py	2010-04-28 10:33:37 +0000
@@ -111,8 +111,10 @@
                 base_revision_id = revision[0].as_revision_id(branch)
         if revision_id is None:
             if tree is not None:
-                tree.warn_if_changed_or_out_of_date(
-                    strict, 'send_strict', 'Use --no-strict to force the send.')
+                tree.check_changed_or_out_of_date(
+                    strict, 'send_strict',
+                    more_error='Use --no-strict to force the send.',
+                    more_warning='Uncommitted changes will not be sent.')
             revision_id = branch.last_revision()
         if revision_id == NULL_REVISION:
             raise errors.BzrCommandError('No revisions to submit.')

=== modified file 'bzrlib/tests/blackbox/test_dpush.py'
--- bzrlib/tests/blackbox/test_dpush.py	2010-04-13 06:42:19 +0000
+++ bzrlib/tests/blackbox/test_dpush.py	2010-04-28 10:33:37 +0000
@@ -153,24 +153,6 @@
         conf.set_user_option('dpush_strict', value)
 
     _default_command = ['dpush', '../to']
-    _default_pushed_revid = False # Doesn't aplly for dpush
-
-    def assertPushSucceeds(self, args, pushed_revid=None, with_warning=False):
-        if with_warning:
-            error_regexes = self._default_errors
-        else:
-            error_regexes = []
-        self.run_bzr(self._default_command + args,
-                     working_dir=self._default_wd, error_regexes=error_regexes)
-        if pushed_revid is None:
-            # dpush change the revids, so we need to get back to it
-            branch_from = branch.Branch.open(self._default_wd)
-            pushed_revid = branch_from.last_revision()
-        branch_to = branch.Branch.open('to')
-        repo_to = branch_to.repository
-        self.assertTrue(repo_to.has_revision(pushed_revid))
-        self.assertEqual(branch_to.last_revision(), pushed_revid)
-
 
 
 class TestDpushStrictWithoutChanges(TestDpushStrictMixin,

=== modified file 'bzrlib/tests/blackbox/test_push.py'
--- bzrlib/tests/blackbox/test_push.py	2010-04-15 06:47:06 +0000
+++ bzrlib/tests/blackbox/test_push.py	2010-04-28 10:33:37 +0000
@@ -661,27 +661,35 @@
     _default_wd = 'local'
     _default_errors = ['Working tree ".*/local/" has uncommitted '
                        'changes \(See bzr status\)\.',]
-    _default_pushed_revid = 'modified'
+    _default_additional_error = 'Use --no-strict to force the push.\n'
+    _default_additional_warning = 'Uncommitted changes will not be pushed.'
+
 
     def assertPushFails(self, args):
-        ret = self.run_bzr_error(self._default_errors,
-                                 self._default_command + args,
-                                 working_dir=self._default_wd, retcode=3)
+        out, err = self.run_bzr_error(self._default_errors,
+                                      self._default_command + args,
+                                      working_dir=self._default_wd, retcode=3)
+        self.assertContainsRe(err, self._default_additional_error)
 
-    def assertPushSucceeds(self, args, pushed_revid=None, with_warning=False):
+    def assertPushSucceeds(self, args, with_warning=False, revid_to_push=None):
         if with_warning:
             error_regexes = self._default_errors
         else:
             error_regexes = []
-        ret = self.run_bzr(self._default_command + args,
-                           working_dir=self._default_wd,
-                           error_regexes=error_regexes)
-        if pushed_revid is None:
-            pushed_revid = self._default_pushed_revid
-        tree_to = workingtree.WorkingTree.open('to')
-        repo_to = tree_to.branch.repository
-        self.assertTrue(repo_to.has_revision(pushed_revid))
-        self.assertEqual(tree_to.branch.last_revision_info()[1], pushed_revid)
+        out, err = self.run_bzr(self._default_command + args,
+                                working_dir=self._default_wd,
+                                error_regexes=error_regexes)
+        if with_warning:
+            self.assertContainsRe(err, self._default_additional_warning)
+        else:
+            self.assertNotContainsRe(err, self._default_additional_warning)
+        branch_from = branch.Branch.open(self._default_wd)
+        if revid_to_push is None:
+            revid_to_push = branch_from.last_revision()
+        branch_to = branch.Branch.open('to')
+        repo_to = branch_to.repository
+        self.assertTrue(repo_to.has_revision(revid_to_push))
+        self.assertEqual(revid_to_push, branch_to.last_revision())
 
 
 
@@ -748,13 +756,12 @@
         self._default_wd = 'checkout'
         self._default_errors = ["Working tree is out of date, please run"
                                 " 'bzr update'\.",]
-        self._default_pushed_revid = 'modified-in-local'
 
     def test_push_default(self):
         self.assertPushSucceeds([], with_warning=True)
 
     def test_push_with_revision(self):
-        self.assertPushSucceeds(['-r', 'revid:added'], pushed_revid='added')
+        self.assertPushSucceeds(['-r', 'revid:added'], revid_to_push='added')
 
     def test_push_no_strict(self):
         self.assertPushSucceeds(['--no-strict'])

=== modified file 'bzrlib/tests/blackbox/test_send.py'
--- bzrlib/tests/blackbox/test_send.py	2010-04-12 16:41:03 +0000
+++ bzrlib/tests/blackbox/test_send.py	2010-04-28 10:33:37 +0000
@@ -307,6 +307,8 @@
     _default_sent_revs = ['local']
     _default_errors = ['Working tree ".*/local/" has uncommitted '
                        'changes \(See bzr status\)\.',]
+    _default_additional_error = 'Use --no-strict to force the send.\n'
+    _default_additional_warning = 'Uncommitted changes will not be sent.'
 
     def set_config_send_strict(self, value):
         # set config var (any of bazaar.conf, locations.conf, branch.conf
@@ -315,7 +317,8 @@
         conf.set_user_option('send_strict', value)
 
     def assertSendFails(self, args):
-        self.run_send(args, rc=3, err_re=self._default_errors)
+        out, err = self.run_send(args, rc=3, err_re=self._default_errors)
+        self.assertContainsRe(err, self._default_additional_error)
 
     def assertSendSucceeds(self, args, revs=None, with_warning=False):
         if with_warning:
@@ -327,6 +330,7 @@
         out, err = self.run_send(args, err_re=err_re)
         bundling_revs = 'Bundling %d revision(s).\n' % len(revs)
         if with_warning:
+            self.assertContainsRe(err, self._default_additional_warning)
             self.assertEndsWith(err, bundling_revs)
         else:
             self.assertEquals(bundling_revs, err)