uncommit needs an option for saving commit messages

Added a bugtask for bzr-gtk, as there are some suggestions/requests here about having gcommit save messages when cancelling a commit, or picking up the messages saved away by uncommit.

Thank you for taking time report this bug.
I confirm it since I think this would be an useful feature.

I agree it will be useful feature but I think it could be implemented with -o/--output command-line option.

Hi Alexander. What do you mean, with -o option? Is it a suggestion for the name of a new option? Or is it an existing option (of what command)? Thanks!

GuilhemBichot пишет:
> Hi Alexander. What do you mean, with -o option? Is it a suggestion for
> the name of a new option? Or is it an existing option (of what command)?
> Thanks!
>
see help for bzr send as example of this option.

I think Alexander is suggesting that it be called "uncommit -o FILE" -- just a change in the option name.

I think the pending commit message draft should be kept in the WorkingTree object, and maybe actually written to disk into .bzr/checkout/draft. You can have wt methods to get and set the draft. I think any cancelled commit should update it, as should gcommit if you quit before committing. And anytime you start an interactive commit it should be loaded into the editor.

Martin Pool пишет:
> I think Alexander is suggesting that it be called "uncommit -o FILE" --
> just a change in the option name.
>
> I think the pending commit message draft should be kept in the
> WorkingTree object, and maybe actually written to disk into
> .bzr/checkout/draft. You can have wt methods to get and set the draft.
> I think any cancelled commit should update it, as should gcommit if you
> quit before committing. And anytime you start an interactive commit it
> should be loaded into the editor.

Actually I think it should be much complex, because you can uncommit more than
one revision.

It would be nice if we could turn this into more of a blueprint, and get a bit of UI design around it.

We certainly can just dump the raw log texts of "bzr uncommit -r XXX" to a file, but how should it work when there are more than 1 revision being uncommitted. And do you include the log messages for any merges? (Note that uncommit leaves pending merges, so you probably don't want to add them a second time.)

When one cancels a commit in "bzr qcommit" (QBzr, file commit.py), the global commit message gets saved:
    def save_message(self):
        message = unicode(self.message.toPlainText())
        if message:
            config = self.tree.branch.get_config()
            config.set_user_option('qbzr_commit_message', message)
and when "bzr qcommit" starts, the saved message is restored: def restore_message(self):
        config = self.tree.branch.get_config()._get_branch_data_config()
        message = config.get_user_option('qbzr_commit_message')
        if message:
            self.message.setText(message)
This is only for the global commit message, and is not about multiple revisions like "bzr uncommit -r" can do.

Suggestion for when one uncommits several revisions: concatenate comments; for example if we uncommit two revisions, r1 the oldest and r2 the newest,
the comments could be put in chronological order: the saved comment for a file would be:
r1's comment for this file\n
r2's comment for this file.

Did I guess right: writing a post_uncommit hook to save comments removed by "bzr uncommit" would not work, because this hook is called at the end of the uncommit process (so the hook cannot access the uncommitted revisions' comments as those revisions don't exist anymore) ?

No, the post_uncommit hook has the signature:
(local, master, old_revno, old_revid, new_revno, new_revid)

'old_revid' and 'new_revid' give you the indication of what the branch tip used to be at, and where it is now. Those revisions will not have been removed by the repository, so doing:

local.repository.get_revision(old_revid) should work fine.

You'll want to decide how you want to iterate the revisions that were uncommitted. (Do you just want the mainline, or do you want all of the merged revisions as well?) Uncommit (ATM) only follows along the mainline, so iterating the removed revisions is pretty trivial.

I propose the attached patch which does the following:
- when the gcommit window is terminated with the "Cancel" button or the "close" icon, if some global or per-file commit messages have been entered, it asks the user whether to save messages or not; if the user clicks "yes" it saves them; if the user clicks "no" it empties any saved messages. If the "Commit" button is pressed, it empties any saved messages.
- when "bzr uncommit" runs, it saves commit messages of the uncommitted revision. If multiple revisions are uncommitted (with the -r option), the messages are concatenated with a "\n******\n" separator line in between. So if in revision 2, file 'a' had commit message "a2" and file 'b' had "b2" and if in revision 3, 'a' had "a3" and 'b' had "b3", then "bzr uncommit -r1" saves messages "a2\n******\na3" for a and "b2\n******\nb3" for 'b'. If when "bzr uncommit" starts, it finds existing saved messages (from either a previous uncommit or cancelled gcommit), it concatenates with them (so, in the example above, running "bzr uncommit" twice produces the same saved messages as "bzr uncommit -r1").
I modified gtk/gcommit.py and bzrlib/uncommit.py. The messages are stored like qbzr does, in the branch's configuration, which is not perfect (I'm ready to move that to the working tree instead, but then I'll have questions).
The patch is against bzr revno 3405 and bzr-gtk revno 481.

Am Montag, den 12.05.2008, 12:03 +0000 schrieb Anne Mohsen:
> I propose the attached patch which does the following:
> - when the gcommit window is terminated with the "Cancel" button or the "close" icon, if some global or per-file commit messages have been entered, it asks the user whether to save messages or not; if the user clicks "yes" it saves them; if the user clicks "no" it empties any saved messages. If the "Commit" button is pressed, it empties any saved messages.
It really should do the saving without asking. There's no harm in always
saving (similarly to tortoisesvn) and asking the user a question is
going to be quite annoying.

gcommit could then show previously used commit messages in a drop-down
list.

Cheers,

Jelmer

--
Jelmer Vernooij <email address hidden> - http://samba.org/~jelmer/
Jabber: <email address hidden>

I took a quick look at the bzrlib part of this patch, and it looks very reasonable to me. Nice work Anne! The patch is missing tests, I think the reasonable place to put those tests would be in bzrlib/tests/test_uncommit.py.

You can run the uncommit tests by going into the root of the bzr source tree and running this command:
./bzr --no-plugins selftest -v uncommit

When I try running these tests in bzr.dev with no patch, I see all tests passing (3 skipped). When I try this in a tree with the bzrlib patch applied, I get a failure:
ERROR: blackbox.test_uncommit.TestUncommit.test_uncommit_nonascii
    'ascii' codec can't encode character u'\u1234' in position 0: ordinal not in range(128)

I have pushed a branch with the bzrlib portion of the patch applied to here: https://code.launchpad.net/~statik/bzr/bzr.ann_uncommit

Elliot and Jelmer, thank you for being so prompt to look at my work.

Jelmer :
The way I did (proposing the user to save when exiting) is quite common in applications like editors. It has the advantage of using disk space only if user chooses to save. And asking a yes-no question at exit time is not more annoying than letting the user select its comments in a drop-down list at start.
In your solution, would the drop-down list contain comments saved by all previous gcommit cancels? How many at most? In drop-down lists I have seen, there is only one line per item. What about when two saved messages differ only at their tenth line, for example? How does the user distinguish between them in the drop-down list?

Elliot: thank you for making me notice that there are tests :)
After this little change in the patch:
--- bzrlib/uncommit.py.2 2008-05-15 00:06:43.000000000 +0200
+++ bzrlib/uncommit.py 2008-05-15 00:06:06.000000000 +0200
@@ -79,6 +79,9 @@
             else:
                 file_info = bencode.bdecode(file_info.encode('UTF-8'))
             global_message = rev.message
+ if not isinstance(global_message, unicode):
+ # rev.message is sometimes unicode, sometimes str
+ global_message = global_message.decode('UTF-8')
             # Concatenate comments of all undone revisions:
             saved_commit_messages_manager.insert(global_message, file_info)
             # When we finish popping off the pending merges, we want

all tests are passing now : 'bzr self-test -v' shows the same failing tests before and after applying the patch. I'll soon attach a cumulative patch.
I haven't yet created new specific tests for this patch. I propose to add tests after there is a consensus between you, Jelmer and me on the above design points regarding gcommit.

Does bzr have equivalents to git-commit --amend and git-rebase -i? For me, they obviate the need for this feature in a simple way when using git, and perhaps that would be of interest to the subscribers of this bug.

Could someone take a look at these patches please? It would be
good to have this feature.

Thanks for your work on it Anne. Our usual workflow is to send patches
to the list for review, would you be willing to check the patches still
apply on the current version of bzr.dev and submit them for review?

Thanks,

James

James: here is a patch against the latest bzr.dev and bzr-gtk dev branches. Feel free to forward this to any list you wish.

When is this going to be resolved?
Please, either reject the patch and implement the feature anew, or review and accept the patch.
We've been forced to merge every pull of bzr-gtk manually in the past 4 months.

Thank you.

This patch, proposed by Anne Mohsen, allows saving the commit messages
from uncommit so that they can be reused later. It's currently
lacking a test, but I believe not everything in bzr-gtk is well tested
automatically, so perhaps that is not making things worse. Apparently
it is highly desired so I'm going to both post it and start reviewing
it myself.

I'm not sure if putting it into the branch config is the ideal
location but it's probably reasonable.

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

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

Martin Pool wrote:
> This patch, proposed by Anne Mohsen, allows saving the commit messages
> from uncommit so that they can be reused later. It's currently
> lacking a test, but I believe not everything in bzr-gtk is well tested
> automatically, so perhaps that is not making things worse. Apparently
> it is highly desired so I'm going to both post it and start reviewing
> it myself.
>
> I'm not sure if putting it into the branch config is the ideal
> location but it's probably reasonable.
>
>

I started looking at it a bit. One issue is that it only allows a single
commit to be saved. If you compare that to things like "TSVN" which has
a whole list of items. (Including the one you accidentally failed to
commit because you hit escape thinking you were in Vim :)

There is also some friction because you can only really commit from a
WorkingTree, but it is saving the value in a Branch location. Also, for
things like TBZR, it would be more similar if we had a global cache of
recent commit messages, rather than just the local one.

Then again, being able to do:

  bzr uncommit
  bzr gcommit

and have it default to the uncommitted message, would really indicate
that it is a local-to-the-tree cache.

There is also a lot of PEP8 issues in the bzr side of things (note that
the bzrlib.uncommit.py.diff is meant as a bzr patch, not as a patch to
bzr-gtk.)

Also, it certainly shouldn't be called "gtk_global_commit_message" if it
is going to be in bzrlib. I think it would be fair to bring something
in, which could then be shared by "bzr uncommit", "bzr-gtk gcommit",
"qbzr qcommit" etc.

John
=:->

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

iEYEARECAAYFAkjstPIACgkQJdeBCYSNAANd3gCgwx3PNImmyTFsS+j7ROTkU9yC
nWYAn0pnBcraJ45dfx5botBEBf+3idgD
=hsDR
-----END PGP SIGNATURE-----

On Wed, 2008-10-08 at 19:35 +1100, Martin Pool wrote:
> This patch, proposed by Anne Mohsen, allows saving the commit messages
> from uncommit so that they can be reused later. It's currently
> lacking a test, but I believe not everything in bzr-gtk is well tested
> automatically, so perhaps that is not making things worse. Apparently
> it is highly desired so I'm going to both post it and start reviewing
> it myself.
>
> I'm not sure if putting it into the branch config is the ideal
> location but it's probably reasonable.

The formatting needs some work, but otherwise it looks fine.

bb:tweak

Cheers,

Jelmer
--
Jelmer Vernooij <email address hidden> - http://samba.org/~jelmer/
Jabber: <email address hidden>

fwiw, the gtk bit of this looks ok to me (bb:tweak), apart from the reformatting.

The patch to bzrlib looks ok in general, but needs some more changes than the gtk one; the inline unpacking of the file-info revision property in uncommit.py seems wrong to me; it should use some other shared function to unpack that property. The bzrlib patch also needs tests, at least for storing and retrieving messages using SavedCommitMessageManager. It should also just store commit messages in a generic configuration variable rather than one starting with gtk_.

This is still alive at MySQL; from a colleague today: "I'm wondering however, is there a chance that this patch will be added to the main source base? It's quite annoying to re-apply the patch every time I upgrade/change bzr installation...".

If I look at the last review comments made previously, they seem to fall in two categories:
- code (about names of variables etc)
- UI of the feature (see John's post of 2008-10-08); those should be sorted out sooner.

What I wonder is, if this UI thing mentioned by John (multiple commits saved instead of one) a blocker for any inclusion of the patch?

I have adapted Anne's patch to the latest bzr and bzr-gtk. I'm attaching that now.

8 months ago I kindly requested a date when this annoying problem will be resolved.
I've just lost another hour of work..
There is a flurry of comments against the bug report and no commitment.
The defect has a patch; this patch has been tested at MySQL and we are
happy with the functionality it provides.
Doesn't support contract entitle us to a speedier consideration of our matters?
I hope I won't need to blog about this bug to get it sped up.
Thank you,
-kostja

There's a Bazaar Sprint next week where I'd like to go through all pending Bazaar-GTK bugs, and bring any pending patches into trunk, and have a release by the end of the week. So I can promise some progress on the bzr-gtk side.

Thanks for the heads-up,
Szilveszter

Thank you so much :)

--

The patch proposed in the lp:~vila/bzr-gtk/215674-uncommit-save-message doesn't require changing bzrlib but make use of the post_uncommit Branch hook instead.

Previous users of Anne's patches may want to be cautious during the deployment phase as some duplication may happen in their branch.conf files (i.e. the commit messages may contain duplicates).

The rule is: don't mix Anne's patches with the one proposed.