Bazaar

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

  1. 401599-strict-warnings
  2. Merge into bzr.dev
Proposed by Vincent Ladeuil
Status: Merged
Merged at revision: not available
Proposed branch: lp:~vila/bzr/401599-strict-warnings
Merge into: lp:bzr
Diff against target: 253 lines (+58/-47) (has conflicts)
8 files modified
NEWS (+7/-0)
bzrlib/builtins.py (+4/-2)
bzrlib/foreign.py (+4/-2)
bzrlib/mutabletree.py (+11/-6)
bzrlib/send.py (+4/-2)
bzrlib/tests/blackbox/test_dpush.py (+0/-18)
bzrlib/tests/blackbox/test_push.py (+23/-16)
bzrlib/tests/blackbox/test_send.py (+5/-1)
Text conflict in NEWS
To merge this branch: bzr merge lp:~vila/bzr/401599-strict-warnings
Reviewer Review Type Date Requested Status
Andrew Bennetts Needs Fixing
Review via email: mp+23932@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Andrew Bennetts (spiv) wrote :

+* 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
Revision history for this message
Vincent Ladeuil (vila) 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 ?

    > 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.

Revision history for this message
Andrew Bennetts (spiv) wrote :

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.

Revision history for this message
Robert Collins (lifeless) wrote :

+1

Revision history for this message
Robert Collins (lifeless) wrote :

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

Revision history for this message
Vincent Ladeuil (vila) wrote :

>>>>> 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.

Revision history for this message
Andrew Bennetts (spiv) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-04-28 09:40:23 +0000
3+++ NEWS 2010-04-28 10:33:37 +0000
4@@ -29,6 +29,7 @@
5 Bug Fixes
6 *********
7
8+<<<<<<< TREE
9 * ``.bazaar``, ``.bazaar/bazaar.conf`` and ``.bzr.log`` inherit user and
10 group ownership from the containing directory. This allow bzr to work
11 better with sudo.
12@@ -115,6 +116,12 @@
13 Bug Fixes
14 *********
15
16+=======
17+* Wrong section, should be 2.2b3, should be fixed at merge.
18+ Don't mention --no-strict when we just issue the warning about unclean trees.
19+ (Vincent Ladeuil, #401599)
20+
21+>>>>>>> MERGE-SOURCE
22 * ``bzr dpush``, ``bzr push`` and ``bzr send`` will now issue a warning
23 instead of failing when dirty trees are involved. The corresponding
24 ``dpush_strict``, ``push_strict`` and ``send_strict`` should be set to
25
26=== modified file 'bzrlib/builtins.py'
27--- bzrlib/builtins.py 2010-04-28 09:40:23 +0000
28+++ bzrlib/builtins.py 2010-04-28 10:33:37 +0000
29@@ -1132,8 +1132,10 @@
30 else:
31 revision_id = None
32 if tree is not None and revision_id is None:
33- tree.warn_if_changed_or_out_of_date(
34- strict, 'push_strict', 'Use --no-strict to force the push.')
35+ tree.check_changed_or_out_of_date(
36+ strict, 'push_strict',
37+ more_error='Use --no-strict to force the push.',
38+ more_warning='Uncommitted changes will not be pushed.')
39 # Get the stacked_on branch, if any
40 if stacked_on is not None:
41 stacked_on = urlutils.normalize_url(stacked_on)
42
43=== modified file 'bzrlib/foreign.py'
44--- bzrlib/foreign.py 2010-04-20 22:38:03 +0000
45+++ bzrlib/foreign.py 2010-04-28 10:33:37 +0000
46@@ -297,8 +297,10 @@
47 source_branch = Branch.open(directory)
48 source_wt = None
49 if source_wt is not None:
50- source_wt.warn_if_changed_or_out_of_date(
51- strict, 'dpush_strict', 'Use --no-strict to force the push.')
52+ source_wt.check_changed_or_out_of_date(
53+ strict, 'dpush_strict',
54+ more_error='Use --no-strict to force the push.',
55+ more_warning='Uncommitted changes will not be pushed.')
56 stored_loc = source_branch.get_push_location()
57 if location is None:
58 if stored_loc is None:
59
60=== modified file 'bzrlib/mutabletree.py'
61--- bzrlib/mutabletree.py 2010-04-15 06:47:06 +0000
62+++ bzrlib/mutabletree.py 2010-04-28 10:33:37 +0000
63@@ -258,7 +258,8 @@
64 return False
65
66 @needs_read_lock
67- def warn_if_changed_or_out_of_date(self, strict, opt_name, more_msg):
68+ def check_changed_or_out_of_date(self, strict, opt_name,
69+ more_error, more_warning):
70 """Check the tree for uncommitted changes and branch synchronization.
71
72 If strict is None and not set in the config files, a warning is issued.
73@@ -269,25 +270,29 @@
74
75 :param opt_name: strict option name to search in config file.
76
77- :param more_msg: Details about how to avoid the warnings.
78+ :param more_error: Details about how to avoid the check.
79+
80+ :param more_warning: Details about what is happening.
81 """
82 if strict is None:
83 strict = self.branch.get_config().get_user_option_as_bool(opt_name)
84 if strict is not False:
85- err = None
86+ err_class = None
87 if (self.has_changes()):
88- err = errors.UncommittedChanges(self, more=more_msg)
89+ err_class = errors.UncommittedChanges
90 elif self.last_revision() != self.branch.last_revision():
91 # The tree has lost sync with its branch, there is little
92 # chance that the user is aware of it but he can still force
93 # the action with --no-strict
94- err = errors.OutOfDateTree(self, more=more_msg)
95- if err is not None:
96+ err_class = errors.OutOfDateTree
97+ if err_class is not None:
98 if strict is None:
99+ err = err_class(self, more=more_warning)
100 # We don't want to interrupt the user if he expressed no
101 # preference about strict.
102 trace.warning('%s', err._format())
103 else:
104+ err = err_class(self, more=more_error)
105 raise err
106
107 @needs_read_lock
108
109=== modified file 'bzrlib/send.py'
110--- bzrlib/send.py 2010-04-12 16:54:35 +0000
111+++ bzrlib/send.py 2010-04-28 10:33:37 +0000
112@@ -111,8 +111,10 @@
113 base_revision_id = revision[0].as_revision_id(branch)
114 if revision_id is None:
115 if tree is not None:
116- tree.warn_if_changed_or_out_of_date(
117- strict, 'send_strict', 'Use --no-strict to force the send.')
118+ tree.check_changed_or_out_of_date(
119+ strict, 'send_strict',
120+ more_error='Use --no-strict to force the send.',
121+ more_warning='Uncommitted changes will not be sent.')
122 revision_id = branch.last_revision()
123 if revision_id == NULL_REVISION:
124 raise errors.BzrCommandError('No revisions to submit.')
125
126=== modified file 'bzrlib/tests/blackbox/test_dpush.py'
127--- bzrlib/tests/blackbox/test_dpush.py 2010-04-13 06:42:19 +0000
128+++ bzrlib/tests/blackbox/test_dpush.py 2010-04-28 10:33:37 +0000
129@@ -153,24 +153,6 @@
130 conf.set_user_option('dpush_strict', value)
131
132 _default_command = ['dpush', '../to']
133- _default_pushed_revid = False # Doesn't aplly for dpush
134-
135- def assertPushSucceeds(self, args, pushed_revid=None, with_warning=False):
136- if with_warning:
137- error_regexes = self._default_errors
138- else:
139- error_regexes = []
140- self.run_bzr(self._default_command + args,
141- working_dir=self._default_wd, error_regexes=error_regexes)
142- if pushed_revid is None:
143- # dpush change the revids, so we need to get back to it
144- branch_from = branch.Branch.open(self._default_wd)
145- pushed_revid = branch_from.last_revision()
146- branch_to = branch.Branch.open('to')
147- repo_to = branch_to.repository
148- self.assertTrue(repo_to.has_revision(pushed_revid))
149- self.assertEqual(branch_to.last_revision(), pushed_revid)
150-
151
152
153 class TestDpushStrictWithoutChanges(TestDpushStrictMixin,
154
155=== modified file 'bzrlib/tests/blackbox/test_push.py'
156--- bzrlib/tests/blackbox/test_push.py 2010-04-15 06:47:06 +0000
157+++ bzrlib/tests/blackbox/test_push.py 2010-04-28 10:33:37 +0000
158@@ -661,27 +661,35 @@
159 _default_wd = 'local'
160 _default_errors = ['Working tree ".*/local/" has uncommitted '
161 'changes \(See bzr status\)\.',]
162- _default_pushed_revid = 'modified'
163+ _default_additional_error = 'Use --no-strict to force the push.\n'
164+ _default_additional_warning = 'Uncommitted changes will not be pushed.'
165+
166
167 def assertPushFails(self, args):
168- ret = self.run_bzr_error(self._default_errors,
169- self._default_command + args,
170- working_dir=self._default_wd, retcode=3)
171+ out, err = self.run_bzr_error(self._default_errors,
172+ self._default_command + args,
173+ working_dir=self._default_wd, retcode=3)
174+ self.assertContainsRe(err, self._default_additional_error)
175
176- def assertPushSucceeds(self, args, pushed_revid=None, with_warning=False):
177+ def assertPushSucceeds(self, args, with_warning=False, revid_to_push=None):
178 if with_warning:
179 error_regexes = self._default_errors
180 else:
181 error_regexes = []
182- ret = self.run_bzr(self._default_command + args,
183- working_dir=self._default_wd,
184- error_regexes=error_regexes)
185- if pushed_revid is None:
186- pushed_revid = self._default_pushed_revid
187- tree_to = workingtree.WorkingTree.open('to')
188- repo_to = tree_to.branch.repository
189- self.assertTrue(repo_to.has_revision(pushed_revid))
190- self.assertEqual(tree_to.branch.last_revision_info()[1], pushed_revid)
191+ out, err = self.run_bzr(self._default_command + args,
192+ working_dir=self._default_wd,
193+ error_regexes=error_regexes)
194+ if with_warning:
195+ self.assertContainsRe(err, self._default_additional_warning)
196+ else:
197+ self.assertNotContainsRe(err, self._default_additional_warning)
198+ branch_from = branch.Branch.open(self._default_wd)
199+ if revid_to_push is None:
200+ revid_to_push = branch_from.last_revision()
201+ branch_to = branch.Branch.open('to')
202+ repo_to = branch_to.repository
203+ self.assertTrue(repo_to.has_revision(revid_to_push))
204+ self.assertEqual(revid_to_push, branch_to.last_revision())
205
206
207
208@@ -748,13 +756,12 @@
209 self._default_wd = 'checkout'
210 self._default_errors = ["Working tree is out of date, please run"
211 " 'bzr update'\.",]
212- self._default_pushed_revid = 'modified-in-local'
213
214 def test_push_default(self):
215 self.assertPushSucceeds([], with_warning=True)
216
217 def test_push_with_revision(self):
218- self.assertPushSucceeds(['-r', 'revid:added'], pushed_revid='added')
219+ self.assertPushSucceeds(['-r', 'revid:added'], revid_to_push='added')
220
221 def test_push_no_strict(self):
222 self.assertPushSucceeds(['--no-strict'])
223
224=== modified file 'bzrlib/tests/blackbox/test_send.py'
225--- bzrlib/tests/blackbox/test_send.py 2010-04-12 16:41:03 +0000
226+++ bzrlib/tests/blackbox/test_send.py 2010-04-28 10:33:37 +0000
227@@ -307,6 +307,8 @@
228 _default_sent_revs = ['local']
229 _default_errors = ['Working tree ".*/local/" has uncommitted '
230 'changes \(See bzr status\)\.',]
231+ _default_additional_error = 'Use --no-strict to force the send.\n'
232+ _default_additional_warning = 'Uncommitted changes will not be sent.'
233
234 def set_config_send_strict(self, value):
235 # set config var (any of bazaar.conf, locations.conf, branch.conf
236@@ -315,7 +317,8 @@
237 conf.set_user_option('send_strict', value)
238
239 def assertSendFails(self, args):
240- self.run_send(args, rc=3, err_re=self._default_errors)
241+ out, err = self.run_send(args, rc=3, err_re=self._default_errors)
242+ self.assertContainsRe(err, self._default_additional_error)
243
244 def assertSendSucceeds(self, args, revs=None, with_warning=False):
245 if with_warning:
246@@ -327,6 +330,7 @@
247 out, err = self.run_send(args, err_re=err_re)
248 bundling_revs = 'Bundling %d revision(s).\n' % len(revs)
249 if with_warning:
250+ self.assertContainsRe(err, self._default_additional_warning)
251 self.assertEndsWith(err, bundling_revs)
252 else:
253 self.assertEquals(bundling_revs, err)