Re: [Monotone-devel] the changelog editor branch is ready for review

[Thomas Keller]

Am 11.04.10 05:32, schrieb Derek Scherger:
> On Sat, Apr 10, 2010 at 1:28 PM, Thomas Keller <address@hidden> 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:    address@hidden
> 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: address@hidden
>       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
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.

> In keeping with the Foo: header lines I was thinking of replacing "Changes
> against parent xxxxx" to something like "Changes: xxxxx" and omitting the
> "xxxxx" 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.

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.

>> 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.

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.)

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):

#########
mtn: warning: This revision will create a new branch
Old Branch: biz.thomaskeller.test
New Branch: biz.thomaskeller.test3

----------------------------------------------------------------------
Revision: c656088d9d89cf47a9351af9575709ec8ef8f220
Parent: 9efdf632e0fb8e149b47d3c23fe03b74f54ab861
Author: address@hidden
Date: 11.04.2010 20:34:57
Branch: biz.thomaskeller.test3
ChangeLog:



Changes against parent 9efdf632e0fb8e149b47d3c23fe03b74f54ab861

  patched  foo
#########

but mtn commit did not tell me that:


#########
Ensure the values for Author, Date and Branch are correct, then enter
a description of this change following the ChangeLog line. Any other
modifications to the lines below or to the summary of changes will
cause the commit to fail.
----------------------------------------------------------------------
Revision: c656088d9d89cf47a9351af9575709ec8ef8f220
Parent: 9efdf632e0fb8e149b47d3c23fe03b74f54ab861
Author: address@hidden
Date: 11.04.2010 20:36:53
Branch: biz.thomaskeller.test3
ChangeLog:



Changes against parent 9efdf632e0fb8e149b47d3c23fe03b74f54ab861

  patched  foo
##########

Since we have "pseudo" headers like Revision: and Parent: already, maybe
it might be a good idea to leave the warning on top for both, status and
commit, but move the "old branch" / "new branch" info into the header
section...?

Thomas.

-- 
GPG-Key 0x160D1092 | address@hidden | 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