[patch]bug 32054 if commit fails commit message draft is lost
Cheuksan Edward Wang
wang02139 at gmail.com
Thu Oct 19 04:48:06 BST 2006
I have modified the code and tests according to your comments except the
'replace' part. The changeset is attached. I'll see what I can do with 5798.
Thanks
On 10/18/06, John Arbash Meinel <john at arbash-meinel.com> wrote:
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Cheuksan Edward Wang wrote:
> > I have cleaned up Stefan Metzmacher's fix to bug 32054. Please review
> the
> > changeset.
> >
> > Thanks
> >
> > Cheuksan Edward Wang
> >
>
> 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-----
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.ubuntu.com/archives/bazaar/attachments/20061019/e2ae5ba1/attachment.htm
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug_32054_2.patch
Type: text/x-patch
Size: 7803 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20061019/e2ae5ba1/attachment.bin
More information about the bazaar
mailing list