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

[Thomas Keller]

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