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

[Thomas Keller]

Am 12.04.10 04:09, schrieb Derek Scherger:
>>> 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.

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.

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

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.

>> 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 my underlying problem is just that I head for a representation
which makes it dead obvious which sections are editable and which not.
If its dead obvious, only minimal to no in-place documentation is
needed. Take the current state for example:

###########
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 ***
----------------------------------------------------------------------
Revision: c243dd99b9c92cf0f9b20f6b2f65861722d81635
Parent:   ace2791b0b3df530b449802ce82fd8d731a466f1
Author:   address@hidden
Date:     12.04.2010 23:44:27
Branch:   biz.thomaskeller.test3

ChangeLog:



ChangeSet: ace2791b0b3df530b449802ce82fd8d731a466f1

  patched  foo

############

I skim over this and see a lot of sections starting with a noun,
followed by a colon and some value. Only a few of these are actually
editable and I'd probably "group" these accordingly to make this obvious:


###########
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:   address@hidden
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 wouldn't call it ChangeSet but Parent, simply because it is the parent
we denote with the revision. Revision is gone from the commit header as
well - is there any use case displaying this for uncommitted revisions?
And finally, there are clean separators which mark the editable area.
One could argue if it could also look like this

############
----------------------------------------------------------------------
Author:    address@hidden
Date:      12.04.2010 23:44:27
Branch:    biz.thomaskeller.test3
ChangeLog:

----------------------------------------------------------------------
############

to save space and still make it clear that ChangeLog can contain
multiple lines of text and I would find that this is ok as well, however
it would probably collide with the output of log and status.

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

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

>> 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 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
>>
>> ----------------------------------------------------------------------
>> 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:
> 
> 
>> 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...?
>>
> 
> Off the top of my head I don't have any great ideas here either. I'll play
> around with it a bit and see what I can come up with.

Ok, thanks for your continued work and for standing all the discussion
with me ;)

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