monotone-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Derek Scherger
Subject: Re: [Monotone-devel] the changelog editor branch is ready for review
Date: Sat, 10 Apr 2010 21:32:54 -0600

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

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

> 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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]