Re: [Monotone-devel] the changelog editor branch is ready for review
On Tue, Apr 20, 2010 at 3:57 PM, Thomas Keller m...@thomaskeller.biz wrote: In the meantime what I would really really want to get rid of are boolean function arguments - so maybe you could start and make the code a little easier to read by passing a more describable enum value...? Agreed. I've replaced the bool with an enum. I tend to agree here and I've reverted it to use the Changes against parent... however I'm now wondering whether displaying the branch names associated with each parent in this section might be useful which makes me wonder again about using Parent: ... and Branch: headers here. This might surely make sense, but where do we stop? If we start listing cert values of *parents* we might start listing the parents of these parents and their certs and... endless loop :) No, I think the parent revision id is enough, really. Everything else should be part of that revision's log output. Heh, ok. Nothing else to do here then. Now this is something which I'd slightly change - I'd love to see this information in the editable area and the hint that the branch changes above the editable area - because its important and might otherwise get easily forgotten, so something like: I'm not particularly fond of the old branch value in the editable area only because it clutters up the otherwise nicely aligned headers. I'm also not sure including the notice that the branch is changing before or after the editable area makes much difference. It may scroll off the top of a terminal if it's too early so it may be better after the editable values. I've left these where they were in the last iteration but I've emphasized the branch change message so it has a better chance of being noticed. You can set EDITOR='emacsclient +15' to get what you want but this is probably not what you want EDITOR set to in general. We'd need to do some more work in the edit_comment hook to get this right. This would be nice to do but I don't think it's critical for getting this branch finished. Instead of giving the editor a jump point (which could be error-prone f.e. if you think about i18n) I'd try to decrease the verbosity of the entry line mainly by removing the warning in line three: Done. I'd like to preserve the option of telling the editor to jump though, and since that's currently done in EDITOR we can all set it to the appropriate value for our translations so that shouldn't be a real problem. And of course I'd remove Revision: and Parent: from the editable area to make it even less verbose, but we've had this discussion before and as I said I'm happy already that my separators made it in ;) Yes, you're arguing for removing these non-editable things and including the non-editable old branch value. I'm arguing for keeping these and not including the old branch value. It's pretty arbitrary either way there's no doubt. My rationale for leaving the Revision and Parent headers in is that without them the nicely aligned headers look a bit odd because they have extra alignment whitespace that is only relevant when those headers do appear in the output from log and status and I'd like to keep the display from these three commands as similar as possible. I think we definitely want to have it for 0.48. And I'm also voting for not discussing this useful feature to death (because then it won't get included at all) - so I'm shutting up now :) I appreciate the feedback all the same. All that remains is to update the manual to include some examples of the new editor display. I should have this done some time this week. Sorry it took me so long to get to this. Cheers, Derek ___ Monotone-devel mailing list Monotone-devel@nongnu.org http://lists.nongnu.org/mailman/listinfo/monotone-devel
Re: [Monotone-devel] the changelog editor branch is ready for review
Am 05.05.2010 07:11, schrieb Derek Scherger: On Tue, Apr 20, 2010 at 3:57 PM, Thomas Keller m...@thomaskeller.biz wrote: And of course I'd remove Revision: and Parent: from the editable area to make it even less verbose, but we've had this discussion before and as I said I'm happy already that my separators made it in ;) Yes, you're arguing for removing these non-editable things and including the non-editable old branch value. I'm arguing for keeping these and not including the old branch value. It's pretty arbitrary either way there's no doubt. Ok, you're right, I was not consistent here. Maybe I thought more of only show actual (editable) cert values in the editable area - both Parent: and Revision: are clearly not certs at all. My rationale for leaving the Revision and Parent headers in is that without them the nicely aligned headers look a bit odd because they have extra alignment whitespace that is only relevant when those headers do appear in the output from log and status and I'd like to keep the display from these three commands as similar as possible. Ok, as I said earlier, I don't want to argue on these two extra lines too much and I see this as trade-off between consistency with other command's output and cleanness of the editor's contents now. I think we definitely want to have it for 0.48. And I'm also voting for not discussing this useful feature to death (because then it won't get included at all) - so I'm shutting up now :) I appreciate the feedback all the same. All that remains is to update the manual to include some examples of the new editor display. I should have this done some time this week. Ok, cool! Thomas. -- GPG-Key 0x160D1092 | tommyd3...@jabber.ccc.de | http://thomaskeller.biz Please note that according to the EU law on data retention, information on every electronic information exchange might be retained for a period of six months or longer: http://www.vorratsdatenspeicherung.de/?lang=en signature.asc Description: OpenPGP digital signature ___ Monotone-devel mailing list Monotone-devel@nongnu.org http://lists.nongnu.org/mailman/listinfo/monotone-devel
Re: [Monotone-devel] the changelog editor branch is ready for review
Am 19.04.10 07:21, schrieb Derek Scherger:
2010/4/15 Derek Scherger de...@echologic.com
2010/4/14 Stéphane Gimenez d...@gim.name
First remark: mtn status ask for my password and it's rather
inconvenient (I'm not using ssh agent). Same for mtn commit just after
it's invocation and prior to the real commit.
Hrm.. I hadn't noticed that but I'll take a look.
This should be fixed in c66c53da6285693772f76e3f7cfa3b99a34f8b04 but I'm not
sure if the change I've made there is a good one or not.
Thomas and Tim, can you please have a look at this revision and see if you
have a better approach? I don't see any obvious way of getting the signing
key without attempting to decrypt it.
I guess we can't do much better right now. We have lots of places where
static functions fiddle / evaluate global state and change other global
state / calculate some value and I'd consider this bad practice, but
hey, there is no way to fix that without rewriting lots of code, right?
In the meantime what I would really really want to get rid of are
boolean function arguments - so maybe you could start and make the code
a little easier to read by passing a more describable enum value...?
About this, I find 'ChangeSet: ' really confusing, one could
think of '' as an id for the changeset. The good old 'Changes against
parent ' looks better to me.
I tend to agree here and I've reverted it to use the Changes against
parent... however I'm now wondering whether displaying the branch names
associated with each parent in this section might be useful which makes me
wonder again about using Parent: ... and Branch: headers here.
This might surely make sense, but where do we stop? If we start listing
cert values of *parents* we might start listing the parents of these
parents and their certs and... endless loop :)
No, I think the parent revision id is enough, really. Everything else
should be part of that revision's log output.
Since you want to know about what users think... See below for what
I'd like to see in my editor. I think it's pretty self explanatory
and compatible with most of the ideas you came up with.
Here's what I have now:
$ mtn commit
##
Enter a description of this change following the Changelog line.
The values of Author, Date and Branch may be modified as required.
Any other modifications will cause the commit to fail.
*** REMOVE THIS LINE TO CANCEL THE COMMIT ***
--
Revision: c66c53da6285693772f76e3f7cfa3b99a34f8b04
Parent: f016f1e3e91e181e1fee2320ad537d99ce236d7d
Author: de...@echologic.com
Date: Sun Apr 18 10:59:06 PM 2010
Branch: net.venge.monotone.experiment.changelog-editor
Changelog:
avoid requiring key passphrase for status and before editing commit message
* cmd_ws_commit.cc (status,commit): call get_user_key with new
don't-cache-key
flag; pass key_store object into complete_key_identity so that keys that
don't exist in the database are still available
* keys.{hh,cc} (get_user_key): add new cache boolean controlling whether
check_and_save_chosen_key is called to decrypt the private key
* tests/wrong_options_override_workspace_options/__driver__.lua: revert
changes
made on this branch that are no longer necessary; test now matches that
on
net.venge.monotone
--
Changes against parent f016f1e3e91e181e1fee2320ad537d99ce236d7d
patched cmd_ws_commit.cc
patched keys.cc
patched keys.hh
patched tests/wrong_options_override_workspace_options/__driver__.lua
##
Which includes a --- separator line after the Changelog section in the
commit editor. I've left the Revision: and Parent: lines between the ---
separator lines alone. I'm assuming that people won't have any reason to
edit these as they won't have any sensible values to put in them so they
aren't likely to touch them.
Ok, I got my separator lines - fair 'nuff...
$ mtn status
--
Revision: aa124b3ff5c488a0aeba8754821d00a374c61440
Parent: c66c53da6285693772f76e3f7cfa3b99a34f8b04
Author: de...@echologic.com
Date: Sun Apr 18 11:12:45 PM 2010
Branch: net.venge.monotone.experiment.changelog-editor.foo
--
This revision will create a new branch.
Old Branch: net.venge.monotone.experiment.changelog-editor
New Branch: net.venge.monotone.experiment.changelog-editor.foo
Changes against parent c66c53da6285693772f76e3f7cfa3b99a34f8b04
no changes
Now this is something which I'd slightly change - I'd love to see this
information in the editable area and the hint that the branch changes
above the editable area - because its important and might
Re: [Monotone-devel] the changelog editor branch is ready for review
2010/4/15 Derek Scherger de...@echologic.com
2010/4/14 Stéphane Gimenez d...@gim.name
First remark: mtn status ask for my password and it's rather
inconvenient (I'm not using ssh agent). Same for mtn commit just after
it's invocation and prior to the real commit.
Hrm.. I hadn't noticed that but I'll take a look.
This should be fixed in c66c53da6285693772f76e3f7cfa3b99a34f8b04 but I'm not
sure if the change I've made there is a good one or not.
Thomas and Tim, can you please have a look at this revision and see if you
have a better approach? I don't see any obvious way of getting the signing
key without attempting to decrypt it.
I wouldn't call it ChangeSet but Parent, simply because it is the parent
About this, I find 'ChangeSet: ' really confusing, one could
think of '' as an id for the changeset. The good old 'Changes against
parent ' looks better to me.
I tend to agree here and I've reverted it to use the Changes against
parent... however I'm now wondering whether displaying the branch names
associated with each parent in this section might be useful which makes me
wonder again about using Parent: ... and Branch: headers here.
Since you want to know about what users think... See below for what
I'd like to see in my editor. I think it's pretty self explanatory
and compatible with most of the ideas you came up with.
Here's what I have now:
$ mtn commit
##
Enter a description of this change following the Changelog line.
The values of Author, Date and Branch may be modified as required.
Any other modifications will cause the commit to fail.
*** REMOVE THIS LINE TO CANCEL THE COMMIT ***
--
Revision: c66c53da6285693772f76e3f7cfa3b99a34f8b04
Parent: f016f1e3e91e181e1fee2320ad537d99ce236d7d
Author: de...@echologic.com
Date: Sun Apr 18 10:59:06 PM 2010
Branch: net.venge.monotone.experiment.changelog-editor
Changelog:
avoid requiring key passphrase for status and before editing commit message
* cmd_ws_commit.cc (status,commit): call get_user_key with new
don't-cache-key
flag; pass key_store object into complete_key_identity so that keys that
don't exist in the database are still available
* keys.{hh,cc} (get_user_key): add new cache boolean controlling whether
check_and_save_chosen_key is called to decrypt the private key
* tests/wrong_options_override_workspace_options/__driver__.lua: revert
changes
made on this branch that are no longer necessary; test now matches that
on
net.venge.monotone
--
Changes against parent f016f1e3e91e181e1fee2320ad537d99ce236d7d
patched cmd_ws_commit.cc
patched keys.cc
patched keys.hh
patched tests/wrong_options_override_workspace_options/__driver__.lua
##
Which includes a --- separator line after the Changelog section in the
commit editor. I've left the Revision: and Parent: lines between the ---
separator lines alone. I'm assuming that people won't have any reason to
edit these as they won't have any sensible values to put in them so they
aren't likely to touch them.
$ mtn status
--
Revision: aa124b3ff5c488a0aeba8754821d00a374c61440
Parent: c66c53da6285693772f76e3f7cfa3b99a34f8b04
Author: de...@echologic.com
Date: Sun Apr 18 11:12:45 PM 2010
Branch: net.venge.monotone.experiment.changelog-editor.foo
--
This revision will create a new branch.
Old Branch: net.venge.monotone.experiment.changelog-editor
New Branch: net.venge.monotone.experiment.changelog-editor.foo
Changes against parent c66c53da6285693772f76e3f7cfa3b99a34f8b04
no changes
The second --- separator line is only included in status when the branch has
been changed. The note about the branch changing is also displayed by
commit.
Also, it would be nice if the editor was invoked with option '+13' to
position the cursor just after the ChangeLog line. It seems to work
with vim, emacs and nano.
You can set EDITOR='emacsclient +15' to get what you want but this is
probably not what you want EDITOR set to in general. We'd need to do some
more work in the edit_comment hook to get this right. This would be nice to
do but I don't think it's critical for getting this branch finished.
I'm not in any particular hurry to merge this but I think it's pretty much
ready. Are there any fundamental objections at this point? Do we want to
have it in for 0.48?
Cheers,
Derek
___
Monotone-devel mailing list
Monotone-devel@nongnu.org
http://lists.nongnu.org/mailman/listinfo/monotone-devel
Re: [Monotone-devel] the changelog editor branch is ready for review
2010/4/14 Stéphane Gimenez d...@gim.name Hi, I've been following your discussion, and I tried the nice features in this changlog editor branch. First remark: mtn status ask for my password and it's rather inconvenient (I'm not using ssh agent). Same for mtn commit just after it's invocation and prior to the real commit. Hrm.. I hadn't noticed that but I'll take a look. I wouldn't call it ChangeSet but Parent, simply because it is the parent About this, I find 'ChangeSet: ' really confusing, one could think of '' as an id for the changeset. The good old 'Changes against parent ' looks better to me. In my current workspace I've removed the Parent: lines from the top of the revision header and have used Parent: xxx for the changeset sections which avoids printing the parent lines twice. I'll post an example a bit later. Since you want to know about what users think... See below for what I'd like to see in my editor. I think it's pretty self explanatory and compatible with most of the ideas you came up with. Revision: 595feb9f09d68da0559e4f7ace01f5294090210e Parent: 803a401fd7a2703b5edc416155fbbdba0b287eb4 REMOVE THIS LINE TO CANCEL THE COMMIT * - or fill the following area with certificates - Author: m...@wherever Date: 2010-04-13 17:52:00 Branch: net.venge.monotone.experiment.changelog-editor ChangeLog: - end of editable area - Changes against parent 803a401fd7a2703b5edc416155fbbdba0b287eb4 patched README And, mtn status or mtn log would display the same, just remove '***' and '---' lines (plus empty lines around those). Also, it would be nice if the editor was invoked with option '+13' to position the cursor just after the ChangeLog line. It seems to work with vim, emacs and nano. Interesting. I hadn't noticed this option before but I agree. I have been wondering how annoying it would be to have to reposition the cursor before entering the ChangeLog message and being able to put the cursor right where it needs to go seems very good. This is a simple matter of a change to the lua hook to pass the appropriate arguments. Anyway, whatever you choose, thanks for all this! Thanks for the feedback. Cheers, Derek ___ Monotone-devel mailing list Monotone-devel@nongnu.org http://lists.nongnu.org/mailman/listinfo/monotone-devel
Re: [Monotone-devel] the changelog editor branch is ready for review
On Tue, Apr 13, 2010 at 1:29 AM, Stephen Leake stephen_le...@stephe-leake.org wrote: Derek Scherger de...@echologic.com writes: Presumably someone could write a monotone-commit-mode for emacs and something similar for vim that would allow the editor to prevent you from editing things you're not supposed to. I take the opposite approach in my Emacs front-end; I edit _MTN/log (almost directly), and tell commit to just use it. That means changing the branch, author etc require separate commands (which I have not implemented). Presumably you could pass --author, --date and --branch options? There are commands to enter comment templates from ediff, while reviewing changes. Good to know there are other options. I've been thinking about implementing 'automate log'; the current log implementation in the Emacs front end is horribly inefficient, and has at least one bug that I'm having a hard time fixing. It uses bunches of automate commands to reproduce approximately what 'mtn log' does. Yeah, I think I tried dvc at some point and found it to be quite slow iirc. I should probably try it again though. I still use monotone.el quite often but something better would be good. I've been using magit with git a bit and it seems quite nice as well. The ability to select particular diff hunks interactively can be quite useful, even though it can also be considered bad practice. Cheers, Derek ___ Monotone-devel mailing list Monotone-devel@nongnu.org http://lists.nongnu.org/mailman/listinfo/monotone-devel
Re: [Monotone-devel] the changelog editor branch is ready for review
Derek Scherger de...@echologic.com writes: Presumably someone could write a monotone-commit-mode for emacs and something similar for vim that would allow the editor to prevent you from editing things you're not supposed to. I take the opposite approach in my Emacs front-end; I edit _MTN/log (almost directly), and tell commit to just use it. That means changing the branch, author etc require separate commands (which I have not implemented). There are commands to enter comment templates from ediff, while reviewing changes. But your approach also makes sense. I was hoping/expecting to hear from anyone who might be parsing the log output if they were concerned with the changes being proposed. I've been thinking about implementing 'automate log'; the current log implementation in the Emacs front end is horribly inefficient, and has at least one bug that I'm having a hard time fixing. It uses bunches of automate commands to reproduce approximately what 'mtn log' does. -- -- Stephe ___ Monotone-devel mailing list Monotone-devel@nongnu.org http://lists.nongnu.org/mailman/listinfo/monotone-devel
Re: [Monotone-devel] the changelog editor branch is ready for review
On Mon, Apr 12, 2010 at 4:06 PM, Thomas Keller m...@thomaskeller.biz wrote: No, I just tried it out and it already looks much nicer, though I still miss the end of the editable changelog area somewhat - maybe this is just me who looks for some visual separator. One minor peeve I have with the current editor (on mainline) is that you don't really want to put a blank line after the message or it will be displayed as 2 blank lines after the revision is committed. If I reformat the paragraph I'm typing in emacs with fill-paragraph and there is no blank line following my message it pulls in all the MTN: prefixed lines and makes a bit of a mess. With the new format I was sort of relying on the fact that there would be a blank line following the message to prevent fill-paragraph from being overly greedy. Maybe this delimiter is too subtle though? Presumably someone could write a monotone-commit-mode for emacs and something similar for vim that would allow the editor to prevent you from editing things you're not supposed to. I have to admit I used the comment command the first time today. I now know what you mean, mtn comment REV fires up an editor with the possibility to add multi-line text. While you can add linebreaks in other certs as well, its usually a bit harder to do so and people don't trap into that. I initially thought that a comment cert's value could simply appended to the header section in mtn log or similar, but it makes more sense to deal with it just like with normal changelog certs. See 'mtn log -r 0f1782f7e2348f991a0b8eeac03c45a72c8633a2' for a revision with a few comment certs. I think the current format does a reasonable job there. ### Enter a description of this change following the ChangeLog line. The values of Author, Date and Branch may be modified as required. Any other modifications will will cause the commit to fail. *** REMOVE THIS LINE TO CANCEL THE COMMIT *** -- Author: m...@thomaskeller.biz Date: 12.04.2010 23:44:27 Branch: biz.thomaskeller.test3 ChangeLog: -- Parent: ace2791b0b3df530b449802ce82fd8d731a466f1 patched foo What has changed? Parent has been moved down into the ChangeSet section. I had considered this a while ago but then forgot about the idea so thanks for reminding me. We currently have the Parent listed twice, once in the header and a second time before the changeset. Maybe we should drop the one in the header and only include it with the changeset, DRY and all that. I wouldn't call it ChangeSet but Parent, simply because it is the parent Interestingly (maybe) in the code what is being displayed is exactly the changeset from the revision which is where the name came from. we denote with the revision. Revision is gone from the commit header as well - is there any use case displaying this for uncommitted revisions? None that I can think of... I was looking for consistency but I'm fine with dropping the Revision line from commit and status. Any objection to dropping (or moving) the Parent lines down with their changeset details? I would probably not change too much in mtn log, i.e. separators for marking an editable section are not needed here. I'd also keep most of the structure as it is now - also because I think we still have a couple of users which read in mtn log and parse it somehow directly because of the missing automate functionality for that. I know that this is a bit against your aim to provide a single way of displaying cert information, but maybe its worth to not do it exactly the same...? I was hoping/expecting to hear from anyone who might be parsing the log output if they were concerned with the changes being proposed. By unifying the formats I was hoping that people could see what their revision would look like before they've committed it so that there aren't any surprises introduced by the different formats. The mess of functions to display certs in log is mostly gone on this branch and that's one part of the change that I'd like to keep. ;) Hrm, no, probably not that far. Ok, forget the RFC :) If you think its easy enough to add some basic custom cert adding, then do it, otherwise we wait for a feature request... I'm punting for now. We can add this later if it becomes important. I'll have a look at this and see where the extra newline is coming from. # mtn: warning: This revision will create a new branch Old Branch: biz.thomaskeller.test New Branch: biz.thomaskeller.test3 -- Status explicitly outputs a blank line after the old/new branch details which can be changed if we want. I like the idea of adding this information to the instructions for commit if/when the branch is changing. A few extra lines there would make an
Re: [Monotone-devel] the changelog editor branch is ready for review
Am 11.04.10 05:32, schrieb Derek Scherger: On Sat, Apr 10, 2010 at 1:28 PM, Thomas Keller m...@thomaskeller.biz wrote: While I appreciate this unification, I think the ChangeLog: display in mtn status is bogus and should be removed, no? If there is text in _MTN/log then it is displayed in the changelog section. Ah, didn't thought of this - nice! We could choose to omit this section unless there is something in _MTN/log. Yes, I'd say so. I agree that as long as the changed data (well, at least the changelog entry itself) is preserved somehow, we don't really need a magic line. I've been wondering about a line following the instructions like: *** REMOVE THIS LINE TO CANCEL THE COMMIT *** just to make it completely obvious as to how you cancel a commit although that is a bit obnoxious. Usually you can cancel the commit simply by not entering a commit message. I'm not sure we need to have another way of cancelling a commit - after all its fairly easy to undo a commit even after it happened by triggering kill_rev_locally (an über feature would be to have kill_rev_locally store the log message in _MTN/log, but I think this is too much :)) Well, I think this is actually a locale bug and we should not work around that with custom code. Instead it should be noted in the documentation for the changelog editing feature that in some locales issues like this exist and that the user can change the default format %x %X via the get_date_format_spec hook. There's no custom code to deal with this. I was just wondering if %F would be a better default than %x which can have issues in some locales. I'm fine with leaving things as they are and mentioning it in the docs too. The problem I have with %F is that it simply expands to the ISO Y-m-d format which is not at all localized. Maybe we misunderstood each other here, I thought ahead already and headed towards condition-like code which sets %F in some locales and %x in others... this was the thing I wanted to prevent. While playing around I've noticed (positively) that whitespace around the ChangeLog entry is stripped off, but I also noticed that the space after the colon of an entry needed to be preserved in order to let it not bail out. It's fairly pedantic at the moment and I've been wondering whether trimming whitespace off of the localized cert headers would be a good idea. Currently it looks for exact matches of the localized headers, spaces and all. Well, if translators add additional whitespaces at the end of a cert key this should be considered a bug. Trimming is probably required if we choose to left align the headers anyway so I should probably just do that. Right. This is a little criticism I have - right now the overall text layout could be improved because it looks a bit clumsy and is hard to grasp - properly indenting the cert keys could already help. Agreed. So do we want the headers left aligned like this: Revision: acadeb019c234418924525f9c4387b03e2ce35bc Parent:89e8ee147a8555cd26ff2a39023d488ad0fe4b72 Author:de...@echologic.com Date: Sat Apr 10 12:10:52 AM 2010 -0600 Branch: net.venge.monotone.experiment.changelog-editor ChangeLog: or right aligned like this: Revision: acadeb019c234418924525f9c4387b03e2ce35bc Parent: 89e8ee147a8555cd26ff2a39023d488ad0fe4b72 Author: de...@echologic.com Date: Sat Apr 10 12:10:52 AM 2010 -0600 Branch: net.venge.monotone.experiment.changelog-editor The right-aligned version seems to be the better choice. ChangeLog: Note that I've added a line before the ChangeLog: header because it's longer than the others and looks odd otherwise. Multiple ChangeLog: and Comment: headers would presumably each have blank lines around them. Hrm, I haven't thought of the comment cert at all, is this a much used feature after all? If not I'd probably don't care about it and would display it like any other cert value we have. What about separating the end of the changelog section from the changes section with another line? As I already mentioned the changelog is cleaned off of leading and trailing whitespaces and the layout is fixed for the other certs anyways, what about removing the ChangeLog: Would another line of --- be ok in the log output as well? Keeping the output from status, commit and log consistent if possible seems like a nice quality to me. Omitting the ChangeLog: header completely is ok, except that there may be multiple changelog certs and clearly indicating this is probably good. Ditto for Comment certs. The log output already uses --- as revision separators, so it might be better to find a different separator for this then. Wrt the double comment / changelog values - I solved this in recent guitone versions twofold: At first I group a list of cert pairs by signer, so its clearer which set of certs have been added by which signer (while this of course gives the user no temporal or any other
Re: [Monotone-devel] the changelog editor branch is ready for review
On Sun, Apr 11, 2010 at 12:38 PM, Thomas Keller m...@thomaskeller.biz wrote: If there is text in _MTN/log then it is displayed in the changelog section. Ah, didn't thought of this - nice! We could choose to omit this section unless there is something in _MTN/log. Yes, I'd say so. Done. I've been wondering about a line following the instructions like: *** REMOVE THIS LINE TO CANCEL THE COMMIT *** I've also cleaned up the instructions a bit and added this explicit cancel line. Usually you can cancel the commit simply by not entering a commit message. I'm not sure we need to have another way of cancelling a commit If there is text in _MTN/log then there will be a commit message that you need to remove. The original text will be preserved in _MTN/log but I only know that from looking at the code. I would expect people to be reluctant to remove their entire message to cancel the commit and hope that their message was preserved somewhere. The problem I have with %F is that it simply expands to the ISO Y-m-d format which is not at all localized. Maybe we misunderstood each other here, I thought ahead already and headed towards condition-like code which sets %F in some locales and %x in others... this was the thing I wanted to prevent. No worries. I've left the default formats as they are on mainline and they work fine here, as long as the dates are between 1969 and 2068. Trimming is probably required if we choose to left align the headers anyway so I should probably just do that. Right. Done. The right-aligned version seems to be the better choice. Oops. I had the left aligned version done before your message. Let me know if you think we should change it. I have a very slight preference for the left aligned version now. In emacs shell-mode there is some colorization done by default and the left aligned headers are colored differently that other output lines. This doesn't seem to work with the right aligned versions. I'm not stuck on this and will change if you have a strong preference the other way. Hrm, I haven't thought of the comment cert at all, is this a much used feature after all? If not I'd probably don't care about it and would display it like any other cert value we have. I'm not quite sure what you mean by this. Comment certs are much like changelog certs, for the few that appear in the monotone database anyway and are quite likely to be multi-line comments so displaying them like the Date/Author/Branch certs doesn't make a lot of sense. The log output already uses --- as revision separators, so it might be better to find a different separator for this then. Wrt the double comment / changelog values - I solved this in recent guitone versions twofold: At first I group a list of cert pairs by signer, so its clearer which set of certs have been added by which signer (while this of course gives the user no temporal or any other information, we'd need the long discussed cert flag day for that purpose). Secondly if there is more than one changelog cert (and I only handle changelog certs special here, completly forgot about comment certs) from one signer, I group both under one Changelog entry and separate both by a horizontal line - pretty much what you propose here. What log will do at the moment is produce multiple ChangeLog: entries with the header preceeding each one. Right, there is only one small nitpick I have with a syntax like Changes: - this pretty much then looks like a cert and in total like the continuation of the header section. To make it clearer that this is not the case I thought adding another separator line might be a good idea, but I'm open for other ideas here. I'm not sure that representing the changes similar to the certs is good or bad. The latest version is using ChangeSet: as the header for these summaries but I'm still open to suggestions. I think we shouldn't make it too smart - it should be enough if it looks for the four most used certs - Date, Author, Branch and Changelog - and checks their syntax and single existance (to prevent the buffer duplication issue). Then, if it finds other Key: Value pairs in the header area, it should probably just read and add them as additional certs. Maybe its even a good idea to follow some of the basic rules of RFC2822 here, esp. when it comes to newline handling? (Of course we do not expect full CRLF's and 7bit ascii here... but basically giving the user something which looks and reacts familiar.) At the moment I'd rather not get too far into this. Would we read a Foobar: header and generate a foobar cert after converting the header to lowercase? On a completly unrelated note: one thing I haven't had in mind yet is how the new functionality reacts on branch changing - I've edited _MTN/options and set a new branch. mtn status told me that nicely (though I wonder where the additional newline comes from): I'll have a look at this and see where
Re: [Monotone-devel] the changelog editor branch is ready for review
Hi Derek! Hi folks, I think the experimental changelog editor branch that I've been working on recently is to the point where we can consider merging it to mainline. (see the net.venge.monotone.experiment.changelog-editor branch) Very cool! I haven't yet looked at the code, I just played around with it a bit. This branch changes a few things: [...] 2. Unifies the format of revisions displayed by status, commit and log so that a revision looks the same before it's committed, while it's being committed and after it has been committed. While I appreciate this unification, I think the ChangeLog: display in mtn status is bogus and should be removed, no? There are some relatively minor things that are open for discussion: 1. There used to be a magic line that the old commit editor would include if the text of the commit message came from the cached _MTN/log file. This line existed so that a commit could be aborted. If there was data in the _MTN/log file the magic line had to be removed before the commit would proceed. With the new editor changes outside of the legal zones described below will abort the commit so there is no real need for a magic line anymore. I agree that as long as the changed data (well, at least the changelog entry itself) is preserved somehow, we don't really need a magic line. 2. The default date formats on mainline use %x which (using en_CA.UTF-8) produce 2 digit years. This does not meet the requirement in point 3 above for years before 1969 or after 2068 (with glibc 2.1 or later). While this isn't a huge problem it might be a good idea to consider using the %F format which produces 4 digit years. Well, I think this is actually a locale bug and we should not work around that with custom code. Instead it should be noted in the documentation for the changelog editing feature that in some locales issues like this exist and that the user can change the default format %x %X via the get_date_format_spec hook. 3. Changing text in the commit editor outside of the Author, Date, Branch and ChangeLog values is prohibited and will abort a commit because it may prevent these values from being found and extracted. Perhaps the entire contents of the editor should be saved to something like _MTN/changelog to prevent losing a long, carefully worded commit message entirely if you accidentally touch something outside of these legal zones. While playing around I've noticed (positively) that whitespace around the ChangeLog entry is stripped off, but I also noticed that the space after the colon of an entry needed to be preserved in order to let it not bail out. There are also some rather bikeshed-esque things that can be considered: 1. Currently the various cert headers all have a single space following the ':' character which doesn't line up their values very nicely. I think I would prefer these to be aligned left or right but I'm curious to see what others think. This is a little criticism I have - right now the overall text layout could be improved because it looks a bit clumsy and is hard to grasp - properly indenting the cert keys could already help. 2. The wording of the preliminary text describing what to do in the commit editor can probably be improved. This is a minor detail that can easily be changed later. It would be cool if you could shorten this to at most two lines, to avoid further distraction from the main contents. 3. The Changes against parent line preceding the list of changed files and directories might be better as a Changes: line with an optional parent revision id which isn't included for root commits. What about separating the end of the changelog section from the changes section with another line? As I already mentioned the changelog is cleaned off of leading and trailing whitespaces and the layout is fixed for the other certs anyways, what about removing the ChangeLog: key completly? It would pretty much look like composing an email, i.e. you don't see To: foo Subject: bar Body: This is the body there, but To: foo Subject: bar This is the body what do you think? I still need to write up a NEWS entry, update monotone.texi and maybe fiddle with the output formatting a bit but otherwise I think this is done. Could we add support for custom certs here somehow? I thought of maybe adding a new hook get_commit_certs(branch), which would return a table of cert keys which are additionally added to the changelog editor (and which would be treated as optional, in the way that the user could remove them without letting the commit fail). Of course we could also enable the addition of any kind of custom cert just at commit time, but this might interfere with your current layout check code... Thomas. -- GPG-Key 0x160D1092 | tommyd3...@jabber.ccc.de |
Re: [Monotone-devel] the changelog editor branch is ready for review
On Sat, Apr 10, 2010 at 1:28 PM, Thomas Keller m...@thomaskeller.biz wrote: While I appreciate this unification, I think the ChangeLog: display in mtn status is bogus and should be removed, no? If there is text in _MTN/log then it is displayed in the changelog section. We could choose to omit this section unless there is something in _MTN/log. I agree that as long as the changed data (well, at least the changelog entry itself) is preserved somehow, we don't really need a magic line. I've been wondering about a line following the instructions like: *** REMOVE THIS LINE TO CANCEL THE COMMIT *** just to make it completely obvious as to how you cancel a commit although that is a bit obnoxious. Well, I think this is actually a locale bug and we should not work around that with custom code. Instead it should be noted in the documentation for the changelog editing feature that in some locales issues like this exist and that the user can change the default format %x %X via the get_date_format_spec hook. There's no custom code to deal with this. I was just wondering if %F would be a better default than %x which can have issues in some locales. I'm fine with leaving things as they are and mentioning it in the docs too. While playing around I've noticed (positively) that whitespace around the ChangeLog entry is stripped off, but I also noticed that the space after the colon of an entry needed to be preserved in order to let it not bail out. It's fairly pedantic at the moment and I've been wondering whether trimming whitespace off of the localized cert headers would be a good idea. Currently it looks for exact matches of the localized headers, spaces and all. Trimming is probably required if we choose to left align the headers anyway so I should probably just do that. This is a little criticism I have - right now the overall text layout could be improved because it looks a bit clumsy and is hard to grasp - properly indenting the cert keys could already help. Agreed. So do we want the headers left aligned like this: Revision: acadeb019c234418924525f9c4387b03e2ce35bc Parent:89e8ee147a8555cd26ff2a39023d488ad0fe4b72 Author:de...@echologic.com Date: Sat Apr 10 12:10:52 AM 2010 -0600 Branch: net.venge.monotone.experiment.changelog-editor ChangeLog: or right aligned like this: Revision: acadeb019c234418924525f9c4387b03e2ce35bc Parent: 89e8ee147a8555cd26ff2a39023d488ad0fe4b72 Author: de...@echologic.com Date: Sat Apr 10 12:10:52 AM 2010 -0600 Branch: net.venge.monotone.experiment.changelog-editor ChangeLog: Note that I've added a line before the ChangeLog: header because it's longer than the others and looks odd otherwise. Multiple ChangeLog: and Comment: headers would presumably each have blank lines around them. What about separating the end of the changelog section from the changes section with another line? As I already mentioned the changelog is cleaned off of leading and trailing whitespaces and the layout is fixed for the other certs anyways, what about removing the ChangeLog: Would another line of --- be ok in the log output as well? Keeping the output from status, commit and log consistent if possible seems like a nice quality to me. Omitting the ChangeLog: header completely is ok, except that there may be multiple changelog certs and clearly indicating this is probably good. Ditto for Comment certs. In keeping with the Foo: header lines I was thinking of replacing Changes against parent x to something like Changes: x and omitting the x for root commits where there is no parent. Note that there may be 2 changes sections describing the changes from each parent to the new revision. Could we add support for custom certs here somehow? I thought of maybe adding a new hook get_commit_certs(branch), which would return a table of cert keys which are additionally added to the changelog editor (and which would be treated as optional, in the way that the user could remove them without letting the commit fail). Of course we could also enable the addition of any kind of custom cert just at commit time, but this might interfere with your current layout check code... The current code is quite simple and avoids trying to guess where things are. If it can't find something it aborts without looking any further. Presumably the current checking code could be smarter about how it looks for things but smarter code might get confused if you do something like duplicating the entire buffer in your editor by accident. Thanks for the feedback. Cheers, Derek ___ Monotone-devel mailing list Monotone-devel@nongnu.org http://lists.nongnu.org/mailman/listinfo/monotone-devel
