[patch]bug 32054 if commit fails commit message draft is lost

Wed Oct 18 08:40:33 BST 2006

Did you follow the mailing list thread? You seem to have addressed some
of my earlier comments, but not all of them.

...

> === modified file bzrlib/builtins.py
> --- bzrlib/builtins.py
> +++ bzrlib/builtins.py
> @@ -1797,27 +1797,49 @@
>              reporter = ReportCommitToLog()
>          else:
>              reporter = NullCommitReporter()
> -        
> +
> +        # save the commit message and only unlink it if the commit was
> +        # successful

v- We shouldn't import tempfile here. If it is getting imported
anywhere, it should be at the beginning of the file.

Further, we probably should try multiple directories. There are people
who use bzr to version /etc as a normal user. So they don't actually
have write access to ., only .bzr/.

So I'd like to see something more like (psuedocode):

try:
  message_in_user_encoding = message.encode(bzrlib.user_encoding)
except UnicodeError:
  mutter('cannot represent %r in user encoding %r, not saving commit
message', message, bzrlib.user_encoding)
  message_in_user_encoding = None

if message_in_user_encoding is not None:
  try:
    tempfile.mkstemp(prefix='bzr-commit-', dir=tree.basedir)
  except ??? (Probably IOError or OSError):
    try:
      # No access to working dir, try $TMP
      tempfile.mkstemp(prefix='bzr-commit-')
    except:
      # We can't create a temp file, try to work without it
      msgfilename = None

  if msgfilename is not None:
    msgfile = open(msgfilename, 'wt')
    try:
      msgfile.write(message_in_user_encoding)
    finally:
      msgfile.close()

These are the things that I'm handling with this:

1) Local dir is readonly, and $TMP doesn't work. You can still commit,
but if it fails, we didn't create a backup copy.

2) If the Unicode commit message isn't valid in
'bzrlib.user_encoding()', that is okay, we don't try to save it if we
can't. I don't really want to see us use "replace" because then we have
generated a corrupted message. Though it is a judgment call whether it
is better to save most of the message even if some of the characters are
incorrect.

3) The messages to the user need to be updated, in case msgfilename
wasn't actually created (it is None).

> +        import tempfile
> +        tmp_fileno, msgfilename = tempfile.mkstemp(prefix='bzr-commit-',
> +                                                   dir=tree.basedir)
> +        os.close(tmp_fileno)
> +        msgfile = open(msgfilename, "wt")
> +        msgfile.write(message.encode(bzrlib.user_encoding, 'replace'))
> +        msgfile.close()
> +
>          try:
>              tree.commit(message, specific_files=selected_list,
>                          allow_pointless=unchanged, strict=strict, local=local,
>                          reporter=reporter)
> +            try:
> +                os.unlink(msgfilename)
> +            except IOError, e:
> +                warning("failed to unlink %s: %s; ignored", msgfilename, e)
>          except PointlessCommit:
>              # FIXME: This should really happen before the file is read in;
>              # perhaps prepare the commit; get the message; then actually commit
>              raise errors.BzrCommandError("no changes to commit."
> -                                         " use --unchanged to commit anyhow")
> +                                  " use --unchanged to commit anyhow\n"
> +                                  "Commit message saved. To reuse the message,"
> +                                  " do\nbzr commit --file " + msgfilename)
>          except ConflictsInTree:
>              raise errors.BzrCommandError("Conflicts detected in working tree.  "
> -                'Use "bzr conflicts" to list, "bzr resolve FILE" to resolve.')
> +                'Use "bzr conflicts" to list, "bzr resolve FILE" to resolve.\n'
> +                'Commit message saved. To reuse the message,'
> +                ' do\nbzr commit --file ' + msgfilename)
>          except StrictCommitFailed:
>              raise errors.BzrCommandError("Commit refused because there are unknown "
> -                                         "files in the working tree.")
> +                                  "files in the working tree.\n"
> +                                  "Commit message saved. To reuse the message,"
> +                                  " do\nbzr commit --file " + msgfilename)
>          except errors.BoundBranchOutOfDate, e:
>              raise errors.BzrCommandError(str(e) + "\n"
>                  'To commit to master branch, run update and then commit.\n'
>                  'You can also pass --local to commit to continue working '
> -                'disconnected.')
> +                'disconnected.\n'
> +                'Commit message saved. To reuse the message,'
> +                ' do\nbzr commit --file ' + msgfilename)
>  
>  class cmd_check(Command):
>      """Validate consistency of branch history.
> 
> === modified file bzrlib/tests/blackbox/test_commit.py
> --- bzrlib/tests/blackbox/test_commit.py
> +++ bzrlib/tests/blackbox/test_commit.py
> @@ -35,10 +35,12 @@
>          # If forced, it should succeed, but this is not tested here.
>          self.run_bzr("init")
>          self.build_tree(['hello.txt'])
> -        result = self.run_bzr("commit", "-m", "empty", retcode=3)
> -        self.assertEqual(('', 'bzr: ERROR: no changes to commit.'
> -                              ' use --unchanged to commit anyhow\n'),
> -                         result)
> +        out,err = self.run_bzr("commit", "-m", "empty", retcode=3)
> +        self.assertEqual('', out)
> +        self.assertStartsWith(err, 'bzr: ERROR: no changes to commit.'
> +                                  ' use --unchanged to commit anyhow\n'
> +                                  'Commit message saved. To reuse the message,'
> +                                  ' do\nbzr commit --file ')
>  
>      def test_commit_with_path(self):
>          """Commit tree with path of root specified"""

^- Really this should be factored out into 2 new tests.

1) A successful commit should not leave behind a 'bzr-commit-*' file.
2) A failed commit should report that 'bzr-commit-*' exists, and that
file should be accessible and contain the log message.
3) A commit with a unicode message should still succeed. The pqm runs
'make check' which runs LANG=C bzr selftest, which means that any
unicode commit message should not be encodable in ASCII. But the commit
can still succeed because once you get to the internals we don't care as
much what LANG is.

There are other changes that we actually want to make to commit
messages. Specifically, we want to delay prompting the user until after
we have looked for conflicts, etc in the tree. Which means this sort of
change would need to be refactored as part of that. But that doesn't
mean we can't fix this bug now, and update it when we fix that bug.

If you are interested, it is:
https://launchpad.net/products/bzr/+bug/5798

I'd like to hear from others what they think about whether 'replace' is
better than 'strict'. But at the very least, we need better tests for
the commit message being available.

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

iD8DBQFFNdpxJdeBCYSNAAMRAiTsAKCun4zx2zguxNULm9DfNEi9S+HvbACeKF65
m7H3rKvCuIXjHVMG8HIxNOQ=
=qxvk
-----END PGP SIGNATURE-----