Merge lp:~paulbrianstewart/bzr/207507-bzr-commit-message into lp:bzr

[Paul Stewart]

Mention ways of specifying a commit message or forcing an empty commit message when the user specifies an empty message.

In file bzr.bzrlib.builtins.py-Line 3331
if my_message == "":
    raise errors.BzrCommandError("Empty commit message specified"
                        " Please specify a commit message with either"
                        " --message or --file or leave a blank message"
                        " with --message '' ")

In file bzr.bzrlib.tests.blackbock.test_commit.py
  out, err = self.run_bzr_error(["Empty commit message specified"
            " Please specify a commit message with either"
            " --message or --file or leave a blank message"
            " with --message '' "],
      "commit tree/hello.txt", stdin="n\n")

This is a possible fix for bug #207507

[Jelmer Vernooij (jelmer)]

Hi Paul,

Thanks for working on these papercut bugs.

We have a policy in Bazaar to not use more than 80 characters per line, can you split the message up over multiple lines? The exception that's raised a couple of lines above is a good example.

The quotes around the -m look a bit odd, and are inconsistent with the way we mention options in other places. Perhaps it would also be useful to spell out type long option name and mention --file as an alternative to --message.

Have you tried running the test suite with these changes (./bzr selftest --no-plugins)? There might be a few tests that rely on the previous message.

review: Needs Fixing (code)

[Paul Stewart (paulbrianstewart)]

Thanks Jelmer,

I'll look back over it and try to do the tests you suggest.

Thanks for your guidance and help.
Paul

> Hi Paul,
>
> Thanks for working on these papercut bugs.
>
> We have a policy in Bazaar to not use more than 80 characters per line, can
> you split the message up over multiple lines? The exception that's raised a
> couple of lines above is a good example.
>
> The quotes around the -m look a bit odd, and are inconsistent with the way we
> mention options in other places. Perhaps it would also be useful to spell out
> type long option name and mention --file as an alternative to --message.
>
> Have you tried running the test suite with these changes (./bzr selftest --no-
> plugins)? There might be a few tests that rely on the previous message.

[Paul Stewart (paulbrianstewart)]

Hi Jelmer

I have been working away on understanding how to both change the commit error text, and also run the ./bzr selftest --no-plugins command on the my updated code. I noticed at first that there were 110 failed tests, so I went back to the original code, and changed just one thing, that being I changed the word 'empty' to 'Empty'. When I reran the code I got one failed test, in the ...bzr/bzrlib/tests/blackbox/test_commit.py file. It was looking for the message with 'empty', not 'Empty'.
I have pasted part of the message below.
Does this mean that when I change the message in the builtins.py file I also need to change it in the tests file as well? I'm just trying to figure out what I need to do to move forward. I am assuming that the idea is to modify/fix code until you get 0 failed tests?

Thanks for your help.
Paul

43.787 Traceback (most recent call last):
  File "/home/paul/bzr/bzrlib/commands.py", line 1225, in run_bzr_catch_user_errors
    return run_bzr(argv)
  File "/home/paul/bzr/bzrlib/commands.py", line 1118, in run_bzr
    ret = run(*run_argv)
  File "/home/paul/bzr/bzrlib/commands.py", line 676, in run_argv_aliases
    return self.run(**all_cmd_args)
  File "/home/paul/bzr/bzrlib/commands.py", line 698, in run
    return self._operation.run_simple(*args, **kwargs)
  File "/home/paul/bzr/bzrlib/cleanup.py", line 135, in run_simple
    self.cleanups, self.func, *args, **kwargs)
  File "/home/paul/bzr/bzrlib/cleanup.py", line 165, in _do_with_cleanups
    result = func(*args, **kwargs)
  File "/home/paul/bzr/bzrlib/builtins.py", line 3346, in run
    lossy=lossy)
  File "/home/paul/bzr/bzrlib/decorators.py", line 217, in write_locked
    result = unbound(self, *args, **kwargs)
  File "/home/paul/bzr/bzrlib/workingtree_4.py", line 209, in commit
    result = WorkingTree.commit(self, message, revprops, *args, **kwargs)
  File "/home/paul/bzr/bzrlib/decorators.py", line 217, in write_locked
    result = unbound(self, *args, **kwargs)
  File "/home/paul/bzr/bzrlib/mutabletree.py", line 210, in commit
    *args, **kwargs)
  File "/home/paul/bzr/bzrlib/commit.py", line 289, in commit
    lossy=lossy)
  File "/home/paul/bzr/bzrlib/cleanup.py", line 131, in run
    self.cleanups, self.func, self, *args, **kwargs)
  File "/home/paul/bzr/bzrlib/cleanup.py", line 165, in _do_with_cleanups
    result = func(*args, **kwargs)
  File "/home/paul/bzr/bzrlib/commit.py", line 436, in _commit
    message = message_callback(self)
  File "/home/paul/bzr/bzrlib/builtins.py", line 3331, in get_message
    raise errors.BzrCommandError("Empty commit message specified")
BzrCommandError: Empty commit message specified

43.787 errors:
'Committing to: /tmp/testbzr-oox7Z3.tmp/bzrlib.tests.blackbox.test_commit.TestCommit.test_commit_hook_template_rejected/work/tree/\nadded hello.txt\nCommit message was not edited, use anyway? [y/n]: bzr: ERROR: Empty commit message specified\n'
------------
Text attachment: traceback
------------
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/testtools/runtest.py", line 169, in _run_user
    return fn(*args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/testtools/testcase.py", line 499, ...

[Jelmer Vernooij (jelmer)]

Hi Paul,

On Mon, 2011-07-25 at 23:14 +0000, Paul Stewart wrote:
> I have been working away on understanding how to both change the commit error text, and also run the ./bzr selftest --no-plugins command on the my updated code. I noticed at first that there were 110 failed tests, so I went back to the original code, and changed just one thing, that being I changed the word 'empty' to 'Empty'. When I reran the code I got one failed test, in the ...bzr/bzrlib/tests/blackbox/test_commit.py file. It was looking for the message with 'empty', not 'Empty'.
> I have pasted part of the message below.
> Does this mean that when I change the message in the builtins.py file I also need to change it in the tests file as well? I'm just trying to figure out what I need to do to move forward. I am assuming that the idea is to modify/fix code until you get 0 failed tests?
Yes, if you change the code you will also need to modify the tests to
reflect that the expected behaviour has changed.

If you follow test driven development, you could also modify the test
first. The test is making sure that Bazaar is doing the right thing, so
if you update the test first, you can then keep running the tests and
updating the code until all the tests pass.

Does that help?

Cheers,

Jelmer

>
> Thanks for your help.
> Paul
>
> 43.787 Traceback (most recent call last):
> File "/home/paul/bzr/bzrlib/commands.py", line 1225, in run_bzr_catch_user_errors
> return run_bzr(argv)
> File "/home/paul/bzr/bzrlib/commands.py", line 1118, in run_bzr
> ret = run(*run_argv)
> File "/home/paul/bzr/bzrlib/commands.py", line 676, in run_argv_aliases
> return self.run(**all_cmd_args)
> File "/home/paul/bzr/bzrlib/commands.py", line 698, in run
> return self._operation.run_simple(*args, **kwargs)
> File "/home/paul/bzr/bzrlib/cleanup.py", line 135, in run_simple
> self.cleanups, self.func, *args, **kwargs)
> File "/home/paul/bzr/bzrlib/cleanup.py", line 165, in _do_with_cleanups
> result = func(*args, **kwargs)
> File "/home/paul/bzr/bzrlib/builtins.py", line 3346, in run
> lossy=lossy)
> File "/home/paul/bzr/bzrlib/decorators.py", line 217, in write_locked
> result = unbound(self, *args, **kwargs)
> File "/home/paul/bzr/bzrlib/workingtree_4.py", line 209, in commit
> result = WorkingTree.commit(self, message, revprops, *args, **kwargs)
> File "/home/paul/bzr/bzrlib/decorators.py", line 217, in write_locked
> result = unbound(self, *args, **kwargs)
> File "/home/paul/bzr/bzrlib/mutabletree.py", line 210, in commit
> *args, **kwargs)
> File "/home/paul/bzr/bzrlib/commit.py", line 289, in commit
> lossy=lossy)
> File "/home/paul/bzr/bzrlib/cleanup.py", line 131, in run
> self.cleanups, self.func, self, *args, **kwargs)
> File "/home/paul/bzr/bzrlib/cleanup.py", line 165, in _do_with_cleanups
> result = func(*args, **kwargs)
> File "/home/paul/bzr/bzrlib/commit.py", line 436, in _commit
> message = message_callback(self)
> File "/home/paul/bzr/bzrlib/builtins.py", line 3331, in get_message
> raise errors.BzrCommandError("Empty commit message specified")
> BzrCommandError: Empty commit message spec...

[Paul Stewart (paulbrianstewart)]

Thanks Jelmer,

That answers my question exactly. I have changed the code to the expected behavior in the test file and now don't get any errors, so I will keep working away at this and then push it back up and request another review.

Thanks for your help
Paul

[Paul Stewart (paulbrianstewart)]

Hi Jelmer,

I have updated and formatted the empty commit message text in the bzrlib.builtins.py and bzrlib.tests.blackbox.test_commit.py files to be split over multiple lines (so there are less than 80 characters per line) and have updated the options formats to be consistent with how they are listed in other parts of the file(s).

When you have time could you please review it and let me know if there is anything else I need to do.

Thanks for your time.
Paul Stewart

review: Needs Resubmitting

[Jelmer Vernooij (jelmer)]

Cool, this looks alright to me.

Can't the message be '' rather than ' ' ?

[Paul Stewart (paulbrianstewart)]

> Cool, this looks alright to me.
>
> Can't the message be '' rather than ' ' ?
My bad...let me fix that...

Paul

[Paul Stewart (paulbrianstewart)]

Hi Jelmer,

I have changed the --message ' ' to --message '' as you suggested.

Let me know if you need me to do anything else.

Thanks
Paul Stewart

review: Needs Resubmitting

[Paul Stewart (paulbrianstewart)]

Hi Jelmer,

Let me know if there is anything else I need to do for this.

Thanks for your time.
Paul Stewart

review: Needs Resubmitting

[Jelmer Vernooij (jelmer)]

Hi Paul,

The change seems reasonable, but the recommended usage ("Specify --message ''") does not seem to work. E.g.:

$ bzr ci --message ""
Committing to: /tmp/t/
bzr: ERROR: empty commit message specified

I think we need to fix "bzr commit" to allow empty commit messages (perhaps only when specified on the command-line, not from the file editor).

[Jelmer Vernooij (jelmer)]

It would be nice to land your changes though; if you don't have time to look at adding support for "bzr commit -m ''", I could have a look at it next week.

[Paul Stewart (paulbrianstewart)]

Hi Jelmer,

Thanks for working on this....I'll have a look, but am not sure if I have the know-how to fix the part that allows the bzr commit to let empty messages...either way I'll take a look and see what i can do. If you haven't heard from me by next week please feel free to have a look at it yourself. I mistakenly thought that if the self tests passed then it must have been working...still learning :)

THanks for your help.
Paul

[Jelmer Vernooij (jelmer)]

Hi Paul,

On Sun, 2011-07-31 at 00:02 +0000, Paul Stewart wrote:
> Thanks for working on this....I'll have a look, but am not sure if I
> have the know-how to fix the part that allows the bzr commit to let
> empty messages...either way I'll take a look and see what i can do. If
> you haven't heard from me by next week please feel free to have a look
> at it yourself. I mistakenly thought that if the self tests passed then
> it must have been working...still learning :)
Unfortunately our test suite is not yet capable of interpreting and
executing full English sentences. I wish it was. ;-)

Cheers,

Jelmer

[Paul Stewart (paulbrianstewart)]

> Hi Paul,
>
> On Sun, 2011-07-31 at 00:02 +0000, Paul Stewart wrote:
> > Thanks for working on this....I'll have a look, but am not sure if I
> > have the know-how to fix the part that allows the bzr commit to let
> > empty messages...either way I'll take a look and see what i can do. If
> > you haven't heard from me by next week please feel free to have a look
> > at it yourself. I mistakenly thought that if the self tests passed then
> > it must have been working...still learning :)
> Unfortunately our test suite is not yet capable of interpreting and
> executing full English sentences. I wish it was. ;-)
>
> Cheers,
>
> Jelmer

Cheers Jelmer....I guess that was a bit of ambitious to wish for aye :)

Paul

[Paul Stewart (paulbrianstewart)]

Hi Jelmer,

I have some month end reporting to do right now, so won't be able to look at it for another couple of days, so if someone else wants to fix it please go ahead...I'll look at it again as soon as I have cleared up my schedule.

Thanks
Paul

[Paul Stewart (paulbrianstewart)]

Hi Jelmer,

If I changed the code to what I originally had, which was --message ' ' (with the space), wouldn't that eliminate the problem/need to make bzr allow empty bzr commit messages? The space would technically make the message not empty? I don't know if that is good form, but just a thought?

What do you think?

Thanks
Paul Stewart

[John A Meinel (jameinel)]

On 8/4/2011 4:51 AM, Paul Stewart wrote:
> Hi Jelmer,
>
> If I changed the code to what I originally had, which was --message '
> ' (with the space), wouldn't that eliminate the problem/need to make
> bzr allow empty bzr commit messages? The space would technically make
> the message not empty? I don't know if that is good form, but just a
> thought?
>
> What do you think?
>
> Thanks Paul Stewart
>

Yes, 'bzr commit -m " "' should work today because we get a blank space
and not an empty commit message.

I'm pretty sure we had reason to not default to allowing empty messages,
(it *really* is hard to understand history with them), however, if
people feel strongly we can be more flexible.

Basically, I think our position should be that the user should
explicitly request a blank message, and 'bzr commit -m ""' seems to
cover that.

I would definitely make the warning message use "" and not '' because ''
doesn't work on Windows.

John
=:->

[Jelmer Vernooij (jelmer)]

On Fri, 2011-08-05 at 10:01 +0000, John A Meinel wrote:
> On 8/4/2011 4:51 AM, Paul Stewart wrote:
> > Hi Jelmer,
> >
> > If I changed the code to what I originally had, which was --message '
> > ' (with the space), wouldn't that eliminate the problem/need to make
> > bzr allow empty bzr commit messages? The space would technically make
> > the message not empty? I don't know if that is good form, but just a
> > thought?
> >
> > What do you think?

> Yes, 'bzr commit -m " "' should work today because we get a blank space
> and not an empty commit message.
>
> I'm pretty sure we had reason to not default to allowing empty messages,
> (it *really* is hard to understand history with them), however, if
> people feel strongly we can be more flexible.
I think this was my first bazaar code patch. :) It's especially useful
if you don't use -m but bzr puts you into an editor to type the commit
message. Quitting the editor without saving is a good way of aborting
the commit.

-m "" indeed seems like a good way of saying "no, I really want to
commit with a empty commit message".

Cheers,

Jelmer

[Paul Stewart (paulbrianstewart)]

Hi Jelmer,

I have updated the message to use the double quotes "" rather than the single quotes '' so it works in Windows. Please check it out as you have time.

Thanks
Paul Stewart

review: Needs Resubmitting

[Jelmer Vernooij (jelmer)]

Hi Paul,

On 06/08/11 00:32, Paul Stewart wrote:
> Review: Resubmit
> Hi Jelmer,
>
> I have updated the message to use the double quotes "" rather than the single quotes '' so it works in Windows. Please check it out as you have time.
>
Thanks for the update. This looks good, though we'll still need to fix
bzr to accept '-m ""' without complaining.

Cheers,

Jelmer

[Paul Stewart (paulbrianstewart)]

> Hi Paul,
>
> On 06/08/11 00:32, Paul Stewart wrote:
> > Review: Resubmit
> > Hi Jelmer,
> >
> > I have updated the message to use the double quotes "" rather than the
> single quotes '' so it works in Windows. Please check it out as you have time.
> >
> Thanks for the update. This looks good, though we'll still need to fix
> bzr to accept '-m ""' without complaining.
>
> Cheers,
>
> Jelmer

Yes, I forgot that point...apologies...
I'll see if I can find it and do it.
Thanks
Paul

[Paul Stewart (paulbrianstewart)]

> Hi Paul,
>
> On 06/08/11 00:32, Paul Stewart wrote:
> > Review: Resubmit
> > Hi Jelmer,
> >
> > I have updated the message to use the double quotes "" rather than the
> single quotes '' so it works in Windows. Please check it out as you have time.
> >
> Thanks for the update. This looks good, though we'll still need to fix
> bzr to accept '-m ""' without complaining.
>
> Cheers,
>
> Jelmer

Yes, I forgot that point...apologies...
I'll see if I can find it and do it.
Thanks
Paul

[Jelmer Vernooij (jelmer)]

Hi Paul,

On 04/08/11 04:51, Paul Stewart wrote:
> Hi Jelmer,
>
> If I changed the code to what I originally had, which was --message ' ' (with the space), wouldn't that eliminate the problem/need to make bzr allow empty bzr commit messages? The space would technically make the message not empty? I don't know if that is good form, but just a thought?
I've just submitted a branch that makes Bazaar accept empty commit
messages if explicitly specified. Once that lands I think we could land
your branch.

See https://code.launchpad.net/~jelmer/bzr/empty-commit-message/+merge/71315

Cheers,

Jelmer

[Paul Stewart (paulbrianstewart)]

> Hi Paul,
>
> On 04/08/11 04:51, Paul Stewart wrote:
> > Hi Jelmer,
> >
> > If I changed the code to what I originally had, which was --message ' '
> (with the space), wouldn't that eliminate the problem/need to make bzr allow
> empty bzr commit messages? The space would technically make the message not
> empty? I don't know if that is good form, but just a thought?
> I've just submitted a branch that makes Bazaar accept empty commit
> messages if explicitly specified. Once that lands I think we could land
> your branch.
>
> See https://code.launchpad.net/~jelmer/bzr/empty-commit-message/+merge/71315
>
> Cheers,
>
> Jelmer

Excellent!..thanks Jelmer.
So do you need me to do anything els at this stage?

Thanks
Paul Stewart

[Jelmer Vernooij (jelmer)]

Hi Paul,

No, let's have a look at landing it :)

review: Approve (code)

[Jelmer Vernooij (jelmer)]

Actually, one more question - can you add punctuation to the error message?

Thanks for your patience with this MP, and for working on it. :)

review: Needs Fixing

[Paul Stewart (paulbrianstewart)]

> Actually, one more question - can you add punctuation to the error message?
>
> Thanks for your patience with this MP, and for working on it. :)

Sure, I can do that....I'll work on that sometime today....

THanks for your patience too.
Paul

[Paul Stewart (paulbrianstewart)]

Hi Jelmer,

I have added punctuation to the message as requested.

Thanks
Paul Stewart

review: Needs Resubmitting

[Jelmer Vernooij (jelmer)]

review: Approve

[Jelmer Vernooij (jelmer)]

sent to pqm by email

[Jelmer Vernooij (jelmer)]

On 14/08/11 01:13, Paul Stewart wrote:
> Review: Resubmit
> Hi Jelmer,
>
> I have added punctuation to the message as requested.
>
Thanks, I've submitted your branch to PQM.

Cheers,

Jelmer

[Paul Stewart (paulbrianstewart)]

Jelmer,

Thanks. Let me know if you need me to do anything else.

Cheers
Paul Stewart

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2011-07-21 07:08:05 +0000
+++ bzrlib/builtins.py	2011-08-13 23:09:27 +0000
@@ -3328,7 +3328,10 @@
                     raise errors.BzrCommandError("please specify a commit"
                         " message with either --message or --file")
             if my_message == "":
-                raise errors.BzrCommandError("empty commit message specified")
+                raise errors.BzrCommandError("Empty commit message specified."
+                        " Please specify a commit message with either"
+                        " --message or --file or leave a blank message"
+                        " with --message \"\".")
             return my_message
 
         # The API permits a commit with a filter of [] to mean 'select nothing'

=== modified file 'bzrlib/tests/blackbox/test_commit.py'
--- bzrlib/tests/blackbox/test_commit.py	2011-06-14 01:26:41 +0000
+++ bzrlib/tests/blackbox/test_commit.py	2011-08-13 23:09:27 +0000
@@ -750,7 +750,10 @@
     def test_commit_hook_template_rejected(self):
         tree = self.setup_commit_with_template()
         expected = tree.last_revision()
-        out, err = self.run_bzr_error(["empty commit message"],
+        out, err = self.run_bzr_error(["Empty commit message specified."
+                  " Please specify a commit message with either"
+                  " --message or --file or leave a blank message"
+                  " with --message \"\"."],
             "commit tree/hello.txt", stdin="n\n")
         self.assertEqual(expected, tree.last_revision())