Merge lp:~wence/bzr/check-clean into lp:bzr

[Lawrence Mitchell]

Only imply --check-clean for version-info if {clean} is in the template

[Martin Packman (gz)]

Thanks Lawrence! This change looks like the right approach.

+ if template:
+ include_history = True
             include_file_revisions = True
+ if '{clean}' in template:
+ check_clean = True

Looking at bzrlib.version_info_formats.format_custom the other two options are ignored in this case anyway, so you could collapse the check to:

    elif template and "{clean}" in template:
        check_clean = True

The version info command seems generally a little confused about which options are usefully combined, arguably `--template "{clean}"` shouldn't work at all unless `--check-clean` is also passed, but as your branch shows it's easy enough to do the right thing. The templating format is pretty simplistic so a basic contains substring check is correct.

There are three things needed before this can land.

1) Describe the fix and credit yourself in doc/en/release-notes/bzr-2.5.txt - see doc/developers/HACKING.txt for details but basically add an entry under the 2.5b3 Bug Fixes section, sorted alphabetically.

2) The hard bit, this really needs a test so we don't regress this with future changes to version-info. I'm failing to come up with an elegant way to do this, but a test method in bzrlib.blackbox.test_version_info which does run_bzr with a template not containing "{clean}" is the basic setup. The best idea I've had from there is doing before run_bzr something like:

    def should_not_be_called(self):
        raise AssertionError("Method on %r should not have been used" % (self,))
    self.overrideAttr(version_info_formats.VersionInfoBuilder,
        "_extract_file_revisions", should_not_be_called)
    out = self.run_bzr(["version-info", "--custom", "--template r{revno}\\n"])
    self.assertEqual("r0\n", out)

This is ugly monkey patching depending on a private method and is vulnerable to refactoring, but there don't seem to be any better placed hooks.

You'll want to check that the test fails without your fix, and passes with it, using `./bzr selftest -s bb.test_version_info` or similar.

3) Execute <http://www.canonical.com/contributors> copied to Martin Pool.

review: Needs Fixing

[Lawrence Mitchell (wence)]

> Thanks Lawrence! This change looks like the right approach.
>
> Looking at bzrlib.version_info_formats.format_custom the other two options are
> ignored in this case anyway, so you could collapse the check to:
>
> elif template and "{clean}" in template:
> check_clean = True

I don't understand how this would work. If template is not None this else leg would never be taken.

Do you mean?

if template and "{clean}" in template:
    check_clean = True
elif template:
    ...

> The version info command seems generally a little confused about which options
> are usefully combined, arguably `--template "{clean}"` shouldn't work at all
> unless `--check-clean` is also passed, but as your branch shows it's easy
> enough to do the right thing. The templating format is pretty simplistic so a
> basic contains substring check is correct.

The test suite disagrees, it has a test "custom_implies_all"

>
> There are three things needed before this can land.
>
> 1) Describe the fix and credit yourself in doc/en/release-notes/bzr-2.5.txt -
> see doc/developers/HACKING.txt for details but basically add an entry under
> the 2.5b3 Bug Fixes section, sorted alphabetically.
>
> 2) The hard bit, this really needs a test so we don't regress this with future
> changes to version-info. I'm failing to come up with an elegant way to do
> this, but a test method in bzrlib.blackbox.test_version_info which does
> run_bzr with a template not containing "{clean}" is the basic setup. The best
> idea I've had from there is doing before run_bzr something like:
>
> def should_not_be_called(self):
> raise AssertionError("Method on %r should not have been used" %
> (self,))
> self.overrideAttr(version_info_formats.VersionInfoBuilder,
> "_extract_file_revisions", should_not_be_called)
> out = self.run_bzr(["version-info", "--custom", "--template r{revno}\\n"])
> self.assertEqual("r0\n", out)
>
> This is ugly monkey patching depending on a private method and is vulnerable
> to refactoring, but there don't seem to be any better placed hooks.

I can't see anything obvious, but I don't know the code base that well, so I've gone with this.

> You'll want to check that the test fails without your fix, and passes with it,
> using `./bzr selftest -s bb.test_version_info` or similar.

Seems to work fine.

> 3) Execute <http://www.canonical.com/contributors> copied to Martin Pool.

Done

[Martin Packman (gz)]

Great! Thanks for following up.

> > elif template and "{clean}" in template:
> > check_clean = True
>
> I don't understand how this would work. If template is not None this else leg
> would never be taken.

I meant instead of the current `if template` logic, rather than in addition to it.

> The test suite disagrees, it has a test "custom_implies_all"

That suggested change would upset this test though. I think the test is... probably asserting the wrong behaviour, but there's no need to change it for this bug fix, so let's not do that in this branch.

review: Approve

[Vincent Ladeuil (vila)]

Looks good to me, just one not that could be fixed when landing:

62 + templates unless {clean} is explicitly asked for. (Lawrence
63 + Mitchell, #882541)

Author and bug number should be on the same line or some of our release related scripts will fail.

review: Approve

[Lawrence Mitchell (wence)]

> Looks good to me, just one not that could be fixed when landing:
>
> 62 + templates unless {clean} is explicitly asked for. (Lawrence
> 63 + Mitchell, #882541)
>
> Author and bug number should be on the same line or some of our release
> related scripts will fail.

Fixed

[Martin Packman (gz)]

You'll just need to merge bzr.dev to resolve the new conflict in the release notes. PQM which we use for landing branches is unfortunately not configured smart enough to move your news above the one just added by Jelmer. After that I'll land this,

[Lawrence Mitchell (wence)]

> You'll just need to merge bzr.dev to resolve the new conflict in the release
> notes. PQM which we use for landing branches is unfortunately not configured
> smart enough to move your news above the one just added by Jelmer. After that
> I'll land this,

Done in r6237.

Thanks,
Lawrence

[Martin Packman (gz)]

sent to pqm by email

=== modified file 'bzrlib/cmd_version_info.py'
--- bzrlib/cmd_version_info.py	2011-10-06 10:25:48 +0000
+++ bzrlib/cmd_version_info.py	2011-11-08 11:53:38 +0000
@@ -113,10 +113,15 @@
         else:
             b = wt.branch
 
-        if all or template:
+        if all:
             include_history = True
             check_clean = True
             include_file_revisions = True
+        if template:
+            include_history = True
+            include_file_revisions = True
+            if '{clean}' in template:
+                check_clean = True
 
         if revision is not None:
             revision_id = revision[0].as_revision_id(b)
 
=== modified file 'bzrlib/tests/blackbox/test_version_info.py'
--- bzrlib/tests/blackbox/test_version_info.py	2011-10-14 13:56:45 +0000
+++ bzrlib/tests/blackbox/test_version_info.py	2011-11-08 11:53:38 +0000
@@ -20,7 +20,7 @@
 
 from bzrlib.revision import NULL_REVISION
 from bzrlib.tests import TestCaseWithTransport
-
+from bzrlib.version_info_formats import VersionInfoBuilder
 
 class TestVersionInfo(TestCaseWithTransport):
 
@@ -150,7 +150,17 @@
             '"{revno} {branch_nick} {clean}\n" branch')
         self.assertEqual("2 branch 0\n", out)
         self.assertEqual("", err)
-    
+
+    def test_custom_no_clean_in_template(self):
+        def should_not_be_called(self):
+            raise AssertionError("Method on %r should not have been used" % (self,))
+        self.overrideAttr(VersionInfoBuilder, "_extract_file_revisions",
+                          should_not_be_called)
+        self.create_tree()
+        out, err = self.run_bzr('version-info --custom --template=r{revno} branch')
+        self.assertEqual("r2", out)
+        self.assertEqual("", err)
+
     def test_non_ascii(self):
         """Test that we can output non-ascii data"""
         
 
=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-11-07 15:08:19 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-11-08 11:53:38 +0000
@@ -72,6 +72,10 @@
 * ``bzr upgrade`` no longer treats 'already up-to-date' exceptions as
   errors. (Benoît Pierre, #716560).
 
+* ``bzr version-info`` no longer populates the clean state for custom
+  templates unless {clean} is explicitly asked for.
+  (Lawrence Mitchell, #882541)
+
 * Fix finding the CPU count when using Python >= 2.6 on BSD-based systems.
   (Jelmer Vernooij, #887151)