Merge lp:~gagern/bzr/bug869915-mkdir-quiet into lp:bzr

Looks good. A couple of test nits:

+ self.run_bzr('init')

You're not testing init, so instead do:

    self.make_tree_and_branch(".")

Asserting that foo exists afterwards wouldn't hurt either (though the other tests cover that too.

    self.assertPathExists("foo")

On 07.10.2011 16:39, Martin Packman wrote:
> Looks good. A couple of test nits:
>
> + self.run_bzr('init')
>
> You're not testing init, so instead do:
>
> self.make_tree_and_branch(".")

Copied that one from the other test cases for mkdir. In fact it's not
that easy:

    self.make_tree_and_branch('.')
AttributeError: 'TestVersioning' object has no attribute
                'make_tree_and_branch'

The error message makes sense. For one, it's the other way round, but
even make_branch_and_tree requires a TestCaseWithTransport. So I
collected all those mkdir tests in a single class (should this be in a
file test_mkdir.py instead?) derived from TestCaseWithTransport, and
also changed all occurrences of that init invocation with calls to
make_branch_and_tree.

While I'm at it, I also dropped the check_branch method which serves no
purpose at all, as it apparently is never called. Correct me if I'm
wrong, if there is some bizarre unit testing thing going on here, but
even raining an exception in that method had no effect on the other tests.

> Asserting that foo exists afterwards wouldn't hurt either (though the other tests cover that too.
>
> self.assertPathExists("foo")

One test method per fact to be tested, I believe. So I prefer to leave
that one to test_mkdir.

Martin

Ah, good job cleaning that up, I hadn't realised how ancient that test class was.

> While I'm at it, I also dropped the check_branch method which serves no
> purpose at all, as it apparently is never called. Correct me if I'm
> wrong, if there is some bizarre unit testing thing going on here, but
> even raining an exception in that method had no effect on the other tests.

Looks like dead code of equally ancient nature, no need to keep it around.

> One test method per fact to be tested, I believe. So I prefer to leave
> that one to test_mkdir.

That's fine. There are conflicting principles I think, the other view is that you need assert the result of call you're testing - imagine if someone accidentally moved the `os.mkdir` and `wt.add` calls inside the `if not is_quiet()` block.

sent to pqm by email

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

On 10/7/2011 6:10 PM, Martin Packman wrote:
> The proposal to merge lp:~gagern/bzr/bug869915-mkdir-quiet into
> lp:bzr has been updated.
>
> Commit Message changed to:
>
> Make cmd_mkdir obey the quiet option
>
> For more details, see:
> https://code.launchpad.net/~gagern/bzr/bug869915-mkdir-quiet/+merge/78601

If
>
this is meant to be quiet, it should probably be using
'trace.note()' instead of self.outf.write().

Writing to stdout is meant to be important messages as part of the run
(like 'bzr status' reporting content status, or 'bzr log' reporting
the revision information). Status updates, progress updates, etc, go
to stderr.

I think this fits stderr. Which would mean you could get rid of the
'is_quiet()' call as 'trace.note()' silences itself automatically.

Thoughts?

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

iEYEARECAAYFAk6PJhYACgkQJdeBCYSNAAP5DgCgmFN+7YNT4pxumIzaDRCQaD+L
kL4An0Dof1Pl9jGSxK9ugMuI+1TBy81Z
=VhzL
-----END PGP SIGNATURE-----

I believe that "bzr mkdir" should behave just as "bzr add" does.
Currently they both use outf.write. If one were to change mkdir, then I
think that add should be modified as well, which looks like a somewhat
bigger API change as the code for that is in the bzrlib.add module.

I'm also not certain that stderr is better than stdout. While
categorizing things into "important" and "not so important", I believe
the better distinction is "can be used for subsequent pipe phases" and
"is intended for the user only". Error messages should definitely be
directed at the user, as thy are too diverse for scripts to use.
Progress is irrelevant for scripts, too. This here is different, though.
I could well imagine a scenario where a script updates the working tree
(e.g. using rsync or some similar automatic means), then runs "bzr add"
on it to handle all newly added files, but wants to capture that output,
e.g. to ensure that the copyright mentioned in them starts in the
current year, or whatever you can imagine in that direction. The point
is, the script might be using the stdout, and we should encourage that
kind of output post-processing.

So as both add and mkdir have nothing more important to say to the user,
let the things they do say during the normal course of their operation
be directed to stdout, to be used as their caller sees fit.

PQM didn't manage to land this. All the tests passed, then it failed in mysterious circumstances. Almost certainly nothing to do with the code in this branch.

Tail of stdout:

# Check that there were no errors reported.
subunit-stats < selftest.log
Total tests: 26943
Passed tests: 24397
Failed tests: 0
Skipped tests: 2546
Seen tags:
schroot -c bzr-pqm -d /home/pqm/pqm-workdir/srv/+trunk -- sh -c LANG=en_GB.utf8 make check

pre-commit hook succeeded at Fri Oct 7 16:26:04 2011 (0:23:55.043057)

star-merge succeeded at Fri Oct 7 16:26:04 2011 (0:24:02.142211)
'Exception processing merge: [Errno 2] No such file or directory'

And stderr:

bzr: warning: some compiled extensions could not be loaded; see <https://answers.launchpad.net/bzr/+faq/703>
/usr/lib/python2.6/dist-packages/Pyrex/Compiler/Scanning.py:39: DeprecationWarning: the md5 module is deprecated; use hashlib instead
  import md5
bzrlib/_readdir_pyx.c: In function ‘__pyx_f_6bzrlib_12_readdir_pyx__read_dir’:
bzrlib/_readdir_pyx.c:992: warning: ‘__pyx_exc_lineno’ may be used uninitialized in this function
bzrlib/_readdir_pyx.c:1035: warning: ‘__pyx_exc_lineno’ may be used uninitialized in this function
bzrlib/_chk_map_pyx.c: In function ‘__pyx_f_6bzrlib_12_chk_map_pyx_safe_interned_string_from_size’:
bzrlib/_chk_map_pyx.c:282: warning: cast from pointer to integer of different size
In file included from /usr/include/python2.6/Python.h:8,
                 from bzrlib/_patiencediff_c.c:28:
/usr/include/python2.6/pyconfig.h:1031:1: warning: "_POSIX_C_SOURCE" redefined
In file included from /usr/include/stdlib.h:25,
                 from bzrlib/_patiencediff_c.c:26:
/usr/include/features.h:210:1: warning: this is the location of the previous definition
bzrlib/_btree_serializer_pyx.c: In function ‘__pyx_f_6bzrlib_21_btree_serializer_pyx_safe_string_from_size’:
bzrlib/_btree_serializer_pyx.c:306: warning: cast from pointer to integer of different size
bzrlib/_btree_serializer_pyx.c: In function ‘__pyx_f_6bzrlib_21_btree_serializer_pyx_safe_interned_string_from_size’:
bzrlib/_btree_serializer_pyx.c:358: warning: cast from pointer to integer of different size
Fri Oct 7 16:02:43 UTC 2011 : selftest starts
failed to open trace file: [Errno 13] Permission denied: '/you-should-use-TestCaseInTempDir-if-you-need-a-log-file'
Fri Oct 7 16:26:01 UTC 2011 : selftest ends

Vincent has a likely sounding guess that this is fallout from the attempt in rt #48346 to fix email to the bazaar-commits list.

sent to pqm by email

sent to pqm by email

Sorry for using your mp as a guinea pig to get PQM working again Martin, but it's landed now. :)