Merge lp:~gioele/bzr/forgot-commit-message into lp:bzr

[Gioele Barabucci]

[Gioele Barabucci (gioele)]

This patch adds a check to the commit command to catch the common mistake of passing a file name instead of a commit message to the '-m' option.

[John A Meinel (jameinel)]

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

Gioele Barabucci wrote:
> Gioele Barabucci has proposed merging lp:~gioele/bzr/forgot-commit-message into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #73073 warn when commit -m argument is omitted
> https://bugs.launchpad.net/bugs/73073
>
>
> This patch adds a check to the commit command to catch the common mistake of passing a file name instead of a commit message to the '-m' option.
>

So *I* would prefer if this would only warn and not actually prompt. A
user can generally "bzr uncommit" if they have a problem.

The main issue that comes to mind is that something like 'qcommit' will
fail if you gave the commit the same message as a filename. As it will
spawn commit under-the-covers which may block waiting for user input. I
see you do handle if they have a ui_factory that doesn't allow a boolean
to be requested.

Given that the only interactive commands are:

1) shelve
2) uncommit, which has had pretty much unanimous support for removing
the 'y' prompt by default.

Now, we could always have 'commit --interactive' though I think people
would expect that to also prompt for hunk-by-hunk changes that you chose
whether or not you actually want to commit.

I'm not going to block this if someone else really wants this, but I
don't prefer it.

John
=:->

 review: abstain

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

iEYEARECAAYFAksEB34ACgkQJdeBCYSNAANykACgx8GTxpJ+U0pknx0DWB+NwRYk
vBoAoJkVSV5DO5CK1T+tZGk5CuPCHEtv
=b3iI
-----END PGP SIGNATURE-----

review: Abstain

[Martin Pool (mbp)]

2009/11/19 John A Meinel <email address hidden>:
> Review: Abstain
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Gioele Barabucci wrote:
>> Gioele Barabucci has proposed merging lp:~gioele/bzr/forgot-commit-message into lp:bzr.
>>
>>     Requested reviews:
>>     bzr-core (bzr-core)
>> Related bugs:
>>   #73073 warn when commit -m argument is omitted
>>   https://bugs.launchpad.net/bugs/73073
>>
>>
>> This patch adds a check to the commit command to catch the common mistake of passing a file name instead of a commit message to the '-m' option.
>>
>
> So *I* would prefer if this would only warn and not actually prompt. A
> user can generally "bzr uncommit" if they have a problem.

I agree.

Also, can I suggest you please put both lines in one call to
ui_factory.warning, with a \n? For the current textui it makes no
difference, but if it was displayed in eg a dialog box it is obviously
just one logical warning.

--
Martin <http://launchpad.net/~mbp/>

[Gioele Barabucci (gioele)]

> 2009/11/19 John A Meinel <email address hidden>:
> Also, can I suggest you please put both lines in one call to
> ui_factory.warning, with a \n? For the current textui it makes no
> difference, but if it was displayed in eg a dialog box it is obviously
> just one logical warning.
Done.

[Gioele Barabucci (gioele)]

> So *I* would prefer if this would only warn and not actually prompt. A
> user can generally "bzr uncommit" if they have a problem.

People on the mailing list have pointed out that there could be problems with that approach.

For example Jelmer Vernooij commented
    FWIW bzr-svn does support uncommit, the main issue would be
    that Subversion still shows the original commit with the typo
    when running "svn log" on the repository root.

> The main issue that comes to mind is that something like 'qcommit' will
> fail if you gave the commit the same message as a filename. As it will
> spawn commit under-the-covers which may block waiting for user input.

Doesn't qcommit provide a new ui_factory implementation that overrides the textual ui interface? A correctly implemented qbzr ui_factory should show you a graphical warning message with the same GUI used by qcommit. Or did I misunderstand the purpose of UIFactory?

[John A Meinel (jameinel)]

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

Gioele Barabucci wrote:
>> So *I* would prefer if this would only warn and not actually prompt. A
>> user can generally "bzr uncommit" if they have a problem.
>
> People on the mailing list have pointed out that there could be problems with that approach.
>
> For example Jelmer Vernooij commented
> FWIW bzr-svn does support uncommit, the main issue would be
> that Subversion still shows the original commit with the typo
> when running "svn log" on the repository root.
>
>
>> The main issue that comes to mind is that something like 'qcommit' will
>> fail if you gave the commit the same message as a filename. As it will
>> spawn commit under-the-covers which may block waiting for user input.
>
> Doesn't qcommit provide a new ui_factory implementation that overrides the textual ui interface? A correctly implemented qbzr ui_factory should show you a graphical warning message with the same GUI used by qcommit. Or did I misunderstand the purpose of UIFactory?

I still would prefer a warning to a prompt. I realize this may have some
implications for bzr-svn, but it doesn't for bzr-hg and bzr-git. I'd
rather not become interactive for the least-common-denominator here. We
can revisit it later, but I don't think bzr-svn is sufficient to cause
all 'bzr commit' actions to become potentially interactive. (Hang in
scripts, etc.)

John
=:->

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

iEYEARECAAYFAksFYGcACgkQJdeBCYSNAAM/fwCglK4KQ1Ib33QTkJmttIA++6Qh
isgAn0xJsusUbOKJbK09iKiB/kc1doTa
=Qc7/
-----END PGP SIGNATURE-----

[Gioele Barabucci (gioele)]

I implemented your suggestions: now "bzr commit -m a_file" only generates a warning.

[John A Meinel (jameinel)]

Do we still need the is_interactive check on this?

I think it is useful to add is_interactive as a general design thing, but I don't see it benefiting us here. (Since you aren't actually prompting anymore.)

review: Needs Information

[Gioele Barabucci (gioele)]

The default UI factor will not log this warning for non interactive tasks like cron jobs.

If it is OK to print the message without explicitly logging it with trace, then it can be removed.

Isn't is_interactive an useful method anyway? Subject for another branch?

[Martin Pool (mbp)]

2009/12/3 Gioele Barabucci <email address hidden>:
> The default UI factor will not log this warning for non interactive tasks like cron jobs.
>
> If it is OK to print the message without explicitly logging it with trace, then it can be removed.
>
> Isn't is_interactive an useful method anyway? Subject for another branch?

I think it would be good for another branch. Possibly we actually
want to split this into a separate one, say .wants_confirmations().

--
Martin <http://launchpad.net/~mbp/>

[Gioele Barabucci (gioele)]

Changed the patch yet again to emit the warning regardless of the interactivity status.

[Ian Clatworthy (ian-clatworthy)]

review: Approve

[Vincent Ladeuil (vila)]

I'll add a NEWS entry and merge.

review: Approve

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2009-12-03 20:56:18 +0000
+++ bzrlib/builtins.py	2009-12-04 10:29:12 +0000
@@ -3069,6 +3069,14 @@
         if local and not tree.branch.get_bound_location():
             raise errors.LocalRequiresBoundBranch()
 
+        if message is not None:
+            if osutils.lexists(message):
+                warning_msg = ("The commit message is a file"
+                    " name: \"%(filename)s\".\n"
+                    "(use --file \"%(filename)s\" to take commit message from"
+                    " that file)" % { 'filename': message })
+                ui.ui_factory.show_warning(warning_msg)
+
         def get_message(commit_obj):
             """Callback to get commit message"""
             my_message = message

=== modified file 'bzrlib/tests/blackbox/test_commit.py'
--- bzrlib/tests/blackbox/test_commit.py	2009-11-19 06:28:13 +0000
+++ bzrlib/tests/blackbox/test_commit.py	2009-12-04 10:29:12 +0000
@@ -107,6 +107,14 @@
                               'modified hello\.txt\n'
                               'Committed revision 2\.\n$')
 
+    def test_warn_about_forgotten_commit_message(self):
+        """Test that the lack of -m parameter is caught"""
+        wt = self.make_branch_and_tree('.')
+        self.build_tree(['one', 'two'])
+        wt.add(['two'])
+        out, err = self.run_bzr('commit -m one two')
+        self.assertContainsRe(err, "The commit message is a file name")
+
     def test_verbose_commit_renamed(self):
         # Verbose commit of renamed file should say so
         wt = self.prepare_simple_history()