would be nice to have smart commit-newline formatting for trac-post-commit-hook ticket messages

[xris(a)siliconmechanics.com]

Just started playing with trac-post-commit-hook, but noticed that the nice smart-newline code used for commit messages is not applied to ticket messages added by the script. Not sure if modifying the script is good idea (since merely replaceing newlines with [[BR]] might screw up other wiki formatting in the commit message), so I thought I'd post a bug to see if anyone else has a good idea.

[Christian Boos]

The special handling of newlines in commit messages is done at parse and rendering time, e.g. when a changeset is shown.

What happens here is that the commit log message is inserted in a ticket comment, and normally the Wiki content for ticket comments is parsed the normal way, i.e. with the escape_newlines argument set to False.

I could propose a solution, though: the trac-post-commit-hook script should enclose the comment log in a Wiki mark-up block:

{{{
#!text/x-trac-wiki; escape_newlines=True
...
}}}

The text/x-trac-wiki MIME type is already implemented, but not the parsing of arguments…

Alternatively, one could imagine passing directives to the parser, like MoinMoin's ​Processing Instructions.

There could be something like this:

#pragma escape-newlines true

[Christian Boos]

(changing the status, as I've not actually started to work on that yet)

+ feedback on the ideas would be needed before I actually start something :)

[wfragg@…]

If I understand correctly, the same feature could be used to implement "automatic newlines" when creating the tickets (some testers do not want to learn WikiFormatting and complain that single newline does not split the text into two lines).

[Christian Boos]

wfragg: if I understand you correctly, you're talking about respecting the newlines written in a ticket description. That's actually not what this ticket was about (i.e. trac-post-commit-hook).

For your needs, hacking the source along those lines would do the trick:

trac/ticket/web_ui.py

@@ -87 +87 @@
         ticket.values.setdefault('reporter', util.get_reporter_id(req))
 
         if ticket.values.has_key('description'):
-            description = wiki_to_html(ticket['description'], self.env, req, db)
+            description = wiki_to_html(ticket['description'], self.env,
+                                       req, db, escape_newlines=True)
             req.hdf['newticket.description_preview'] = description
 
         req.hdf['title'] = 'New Ticket'
@@ -412 +413 @@
 
         req.hdf['ticket.reporter_id'] = reporter_id
         req.hdf['title'] = '#%d (%s)' % (ticket.id, ticket['summary'])
-        req.hdf['ticket.description.formatted'] = wiki_to_html(ticket['description'],
-                                                               self.env, req, db)
+        req.hdf['ticket.description.formatted'] = wiki_to_html(
+            ticket['description'], self.env, req, db, escape_newlines=True)
 
         req.hdf['ticket.opened'] = util.format_datetime(ticket.time_created)
         req.hdf['ticket.opened_delta'] = util.pretty_timedelta(ticket.time_created)

[wfragg@…]

Yes, you are right, I created new ticket #2914

Actually I'm talking about the way to trigger (optional) automatic newlines during the ticket creation, not rendering. E.g, some kind of checkbox "Manual line breaks" that automatically. This could be implemented the same way as in one of the proposals above - by automatically adding processing instruction at the beginning of the description.

[Christian Boos]

See also #3307.

[Christian Boos]

There are several different topics here…

We should refocus this ticket on the trac-post-commit-hook, or on whatever replaces it in the future.

[Christian Boos]

Replying to cboos:

The special handling of newlines in commit messages is done at parse and rendering time, e.g. when a changeset is shown.

What happens here is that the commit log message is inserted in a ticket comment, and normally the Wiki content for ticket comments is parsed the normal way, i.e. with the escape_newlines argument set to False.

I could propose a solution, though: the trac-post-commit-hook script should enclose the comment log in a Wiki mark-up block:

{{{
#!text/x-trac-wiki; escape_newlines=True
...
}}}

This can now be achieved using a div block.

In the context of MultiRepos, one extra thing which would be interesting is to specify in this block which is the repository the changeset belongs to.

e.g.

{{{
#!div style="white-space: pre" repository="trac"

Fix typo introduced in r4567. 
Closes !#234
}}}

That way, the "r4567" referenced could be associated to the correct repository (through the Context properties).

[Remy Blank]

I'll try to integrate this into the commit ticket updater plugin in multirepos.

I'm not sure I understand yet how the repository="trac" in the div will allow changeset references to reference the correct repository, I'll have to dig into the wiki engine some more.

[Christian Boos]

Replying to rblank:

I'm not sure I understand yet how the repository="trac" in the div will allow changeset references to reference the correct repository, I'll have to dig into the wiki engine some more.

He, yes, there are still some missing pieces at this point. Look at the mimeview Context render "hints". The idea would be to set those hints in a "local" Context from the parameters given to the div (and ideally from any wiki processor block for that matter, for other use cases, like discussed yesterday in #5654). Then the changeset wiki formatter could retrieve that information from its formatter.context.

[Remy Blank]

As a first test, I tried wrapping the commit message with the following block:

{{{
#!div style="white-space: pre"

...
}}}

Strangely, the resulting div gets the wikipage class, which makes the block display indented. Adding class="" to the div fixes the issue, but in the resulting HTML, the div gets a class="""" attribute.

• Is it intentional that divs in ticket comments get the wikipage class?

• I'll see if I can fix the issue with specifying an empty class.

[Christian Boos]

Replying to rblank:

As a first test, I tried wrapping the commit message with the following block: #!div style="white-space: pre"…

• Is it intentional that divs in ticket comments get the wikipage class?

Yes, that was my fix for #7970.

• I'll see if I can fix the issue with specifying an empty class.

I don't understand how this got quoted.

[Christian Boos]

Replying to cboos:

Replying to rblank:

• I'll see if I can fix the issue with specifying an empty class.

I don't understand how this got quoted.

Now I do, r8096 ;-)

But regardless of the fix, what about specifying a "commitlog" class? That way the whitespace: pre could be done in the CSS and people could customize it more easily.

sample-plugins/commit_ticket_update.py

@@ -173 +173 @@
         revstring = str(changeset.rev)
         if repos.reponame:
             revstring += '/' + repos.reponame
-        return 'In [%s]:\n\n%s' % (revstring, changeset.message.strip())
+        return """
+In [%s]:
+{{{
+#!div class="commitlog"
+%s
+}}}
+""" % (revstring, changeset.message.strip())
 
     def _update_tickets(self, tickets, author, comment, date):
         """Update the tickets with the given comment."""

trac/htdocs/css/trac.css

@@ -302 +302 @@
 .wikipage { padding-left: 18px }
 .wikipage h1, .wikipage h2, .wikipage h3 { margin-left: -18px }
 
+.commitlog { white-space: pre }
+
 a.missing:link, a.missing:visited, a.missing, span.missing,
 a.forbidden, span.forbidden { color: #998 }
 a.missing:hover { color: #000 }

[Remy Blank]

Replying to cboos:

Yes, that was my fix for #7970.

I find it a bit surprising that a block of text wrapped in a div gets indented by default. So the correct way to have a non-indented div is to specify class=""?

[Remy Blank]

Replying to cboos:

But regardless of the fix, what about specifying a "commitlog" class? That way the whitespace: pre could be done in the CSS and people could customize it more easily.

Theoretically, we should take into account the [changeset] wiki_format_messages setting, and either interpret as wiki formatting, or wrap the plain text in a <pre>.

How about a commitlog wiki processor? We could then wrap the message as follows:

{{{
#!commitlog
This is the commit message
}}}

and display according to the wiki_format_messages setting. The processor could be implemented in the plugin as well.

One issue I see with wrapping the commit message (with both div or a specific processor) is how to handle a commit message that contains the sequence }}}. Is it possible to escape that sequence? Does !}}} work? Doesn't seem to.

[Remy Blank]

…or even better: a [[CommitMessage(repos,rev)]] macro that fetches the message directly from the repository? The drawback is that the ticket notification will not be very useful.

[Christian Boos]

Replying to rblank:

Replying to cboos:

But regardless of the fix, what about specifying a "commitlog" class? That way the whitespace: pre could be done in the CSS and people could customize it more easily.

Theoretically, we should take into account the [changeset] wiki_format_messages setting, and either interpret as wiki formatting, or wrap the plain text in a <pre>.

That's true.

How about a commitlog wiki processor? We could then wrap the message as follows:

{{{
#!commitlog
This is the commit message
}}}

and display according to the wiki_format_messages setting. The processor could be implemented in the plugin as well.

Rather in the ChangesetModule directly: one day a project can decide to stop using the automatic ticket comments from the commit message… that shouldn't imply that all their pre-existing comments will be suddenly unavailable. This remark applies to the CommitMessage macro as well.

One issue I see with wrapping the commit message (with both div or a specific processor) is how to handle a commit message that contains the sequence }}}. Is it possible to escape that sequence? Does !}}} work? Doesn't seem to.

That shouldn't be a problem, as the {{{...}}} blocks can be nested. If you meant a stray }}} then I think this should be rare enough for not bothering.

Replying to rblank:

…or even better: a [[CommitMessage(repos,rev)]] macro that fetches the message directly from the repository? The drawback is that the ticket notification will not be very useful.

… unless you implement a quick wiki_to_text ;-)

Yeah, I think that's a good solution as well.

So to sum up the possible solutions:

1. #!div class="commitlog" repository="<reponame>"
2. #!commitlog repository="<reponame>"
3. [[CommitMessage(reponame, rev)]]

1. is out if we want to support the [changeset] wiki_format_messages setting dynamically (i.e. so that it reflects the current value of the setting when viewing the ticket as opposed to the current value when updating the ticket).

2. is an interesting middle ground, it will embed the commit message in plain text as well, which is useful for notifications, but it will also adapt the rendering to the current settings.

3. is out until we have wiki_to_text … but will also always present the latest "version" of the changeset message, in case it was edited in the meantime. That can be an advantage or a disadvantage, depending on how we implement the changeset_modified. With one occurrence only (as in the initial impl.), that's fine. With multiple occurrences, all will show the same text.

To explain more precisely, I see that in the current version of changeset_modified, you won't update tickets already updated. But imagine the following scenario, someone writing: "Fix for xyz, see #123. Actually clses #123". Noticing the typo, s/he updates the log with "… closes #123", but nothing happens! Ok, this example is a bit contrived, see+closes for the same ticket, but I think that a natural extension to the script would be not require a command for the refs action - i.e. a simple reference to the ticket would do (I wonder if I didn't see a ticket about that, but I saw quite a lot of tickets recently ;-) ). In this case, the example doesn't require "see" and becomes more realistic.

Besides, I was also thinking of the situation where you would write "Closes #123" when you actually meant "Closes #1234". This would ideally require re-opening the wrongly closed #123.

(now I reckon all this goes a bit beyond the original topic of this ticket ;-) )

[Remy Blank]

Replying to cboos:

Rather in the ChangesetModule directly: one day a project can decide to stop using the automatic ticket comments from the commit message… that shouldn't imply that all their pre-existing comments will be suddenly unavailable. This remark applies to the CommitMessage macro as well.

My intention was to have the processor or macro in the plugin, but as a separate component, so that it could remain enabled even if the ticket updater is disabled. But the ChangesetModule is ok as well.

That shouldn't be a problem, as the {{{...}}} blocks can be nested. If you meant a stray }}} then I think this should be rare enough for not bothering.

Ooh, I hadn't thought of that. Cool!

… unless you implement a quick wiki_to_text ;-)

You mean that we render the ticket comment as text before sending the e-mail? That would be useful anyway.

2. is an interesting middle ground, it will embed the commit message in plain text as well, which is useful for notifications, but it will also adapt the rendering to the current settings.

Even better. If we also pass the revision:

#!commitlog repository="<repos>" revision="<rev>"

we can have the current plain text and adapt the message. Indeed, the processor could get the text to be rendered from the repository instead of using the text contained in the block. It could even make the text disappear if the ticket number doesn't appear in the commit message anymore (it would leave an empty comment, though, or a note "commit message changed").

Noticing the typo, s/he updates the log with "… closes #123", but nothing happens!

For such rare cases, the user can still close the ticket by hand, so I wouldn't bother about it.

but I think that a natural extension to the script would be not require a command for the refs action

I could add a special case when the config setting for the refs commands is empty, that any ticket reference is considered a ref.

Besides, I was also thinking of the situation where you would write "Closes #123" when you actually meant "Closes #1234". This would ideally require re-opening the wrongly closed #123.

Again, for such rare cases, I assume the user can undo the operations herself. I don't want to make the plugin much more complicated to catch all corner cases, as we won't catch all anyway due to the old changeset metadata possibly missing. As long as the normal case works well enough, and that it is possible to fix the edge cases by hand, I'm happy.

So at the moment, I think the best idea is the #!commitlog processor with the complete text, but with rendering of the real commit message. The notification e-mail will be readable, and the ticket will display the correct message. I'll implement it in the plugin, and we can move it elsewhere if it turns out well enough.

Maybe even a [[ChangesetMeta(<repos>, <rev>, <key>)]] (where <key> is one of "author", "date", "message") that can also be used as #!ChangesetMeta repos=<repos> rev=<rev> key=message.

(Completely OT: we must really do something about these small comment text boxes. I like the idea used in Drupal where you can drag a bar that is below the box to increase its height.)

[Christian Boos]

Replying to rblank:

2. is an interesting middle ground, it will embed the commit message in plain text as well, which is useful for notifications, but it will also adapt the rendering to the current settings.

Even better. If we also pass the revision:

#!commitlog repository="<repos>" revision="<rev>"

we can have the current plain text and adapt the message. (…)

Fine.

but I think that a natural extension to the script would be not require a command for the refs action

I could add a special case when the config setting for the refs commands is empty, that any ticket reference is considered a ref.

Also fine.

(snip hairy cases)

Again, for such rare cases, I assume the user can undo the operations herself.

Well, OK.

So at the moment, I think the best idea is the #!commitlog processor with the complete text, but with rendering of the real commit message. (…) I'll implement it in the plugin, and we can move it elsewhere if it turns out well enough.

Good! I think I prefer that to the idea below.

Maybe even a [[ChangesetMeta(<repos>, <rev>, <key>)]] (where <key> is one of "author", "date", "message") that can also be used as #!ChangesetMeta repos=<repos> rev=<rev> key=message.

(Completely OT: we must really do something about these small comment text boxes. I like the idea used in Drupal where you can drag a bar that is below the box to increase its height.)

Ever tried a WebKit based browser? ;-) You have a size grip in the lower right part of textareas in Chrome or Safari…

But yes, that feature would be neat when working with the other browsers.

[Remy Blank]

Mmh, shouldn't the concepts of "macro" and "processor" be unified? Currently:

• When macros are called through the #! syntax, they lose the arguments.
• There is no extension point for defining processors.

Both concepts are so close they are almost identical. The only difference is that a processor gets additional arguments compared to macros.

Passing the processor arguments as "context hints" smells wrong. They're arguments, not hints, and they should probably not be inherited from a parent context.

How about adding an additional argument to expand_macro() to contain the processor arguments (None if rendering as a macro)? This would make macros full-fledged processors.

[Christian Boos]

Replying to rblank:

Mmh, shouldn't the concepts of "macro" and "processor" be unified? … How about adding an additional argument to expand_macro() to contain the processor arguments (None if rendering as a macro)? This would make macros full-fledged processors.

Sounds good, yes.

Passing the processor arguments as "context hints" smells wrong. They're arguments, not hints, and they should probably not be inherited from a parent context.

Well, it's not about passing systematically all processors arguments as hints. I think in general a processor may decide to set some hints as a "scope" that can be made available to nested processors.

But I think here it's even simpler as in this #!commitlog we should simply create a subcontext having the changeset as its associated resource, and format the message with a formatter using that subcontext. No further change will be required.

[Remy Blank]

Replying to cboos:

Replying to rblank:

Mmh, shouldn't the concepts of "macro" and "processor" be unified? …

Sounds good, yes.

Ok, I have separated this into #8204. I'll leave the plugin as it is for now, and come back to implement #!commitlog once #8204 is closed.

[Remy Blank]

Committed the two remaining improvements in [8378]:

• Wrapped the message in a {{{#!ChangesetMessage}}} wiki processor, so that:
  • Formatting is done according to [changeset] wiki_format_messages.
  • The message is always up-to-date, even if it is changed after the commit.
  • The comment can be hidden if the current message doesn't reference the ticket.
  • The message is readable in the ticket notification e-mail.

• Added a special value <ALL> for [ticket] commit_ticket_update_commands.refs so that all ticket references generate a ticket comment, regardless of the presence of a known command.

This concludes the work on this ticket. I'll leave it open until MultiRepos is merged to trunk (should we have a "done" or "tobemerged" ticket state for that?).

On a related note: should we start a new tracext package in parallel with the current trac package, where we could stuff such plugins? Similar to what is done in Mercurial with the hgext package, those plugins would have to be enabled explicitly. I see two major advantages over the sample-plugins directory:

• The plugins are updated together with Trac, instead of having to be copied regularly.
• Enabling the plugin is as easy as adding a single line to trac.ini.

Of course, for the previous post-commit hook, it made sense to have it separate, as it had to be customized for every installation. But now that it is a full-fledged plugin, this doesn't apply anymore.

Thoughts?

[Christian Boos]

Replying to rblank:

Committed the two remaining improvements in [8378]: <snip>

Good! I see now that it's better to have that ChangesetMessage macro in the plugin, since it contains code to check for the referenced tickets.

Maybe the macro name could be made a bit more specific though, like CommitTicketReference, as one could easily imagine a true ChangesetMessage macro showing only the a given changeset message, nothing more.

This concludes the work on this ticket. I'll leave it open until MultiRepos is merged to trunk (should we have a "done" or "tobemerged" ticket state for that?).

What about using a sub-milestone, milestone:0.12-multirepos, so that we can see the progress, and at integration time, delete that milestone and retarget the tickets to 0.12?

On a related note: should we start a new tracext package in parallel with the current trac package, where we could stuff such plugins? Similar to what is done in Mercurial with the hgext package, those plugins would have to be enabled explicitly. I see two major advantages over the sample-plugins directory:

• The plugins are updated together with Trac, instead of having to be copied regularly.
• Enabling the plugin is as easy as adding a single line to trac.ini.

That's quite interesting. We would have to be careful with the tracext namespace though, as it would get populated by Trac's own default optional plugins and 3^rd party ones. A wiki page for coordination could be used.

[Remy Blank]

Replying to cboos:

Maybe the macro name could be made a bit more specific though, like CommitTicketReference, as one could easily imagine a true ChangesetMessage macro showing only the a given changeset message, nothing more.

Heh, funnily enough, I selected ChangesetMessage on purpose to allow moving the macro to the changeset module at some point :-)

But yes, now that there's specific code to check for references, it makes sense to have a more specific name. Coming shortly.

What about using a sub-milestone, milestone:0.12-multirepos, so that we can see the progress, and at integration time, delete that milestone and retarget the tickets to 0.12?

Hey, good idea!

That's quite interesting. We would have to be careful with the tracext namespace though, as it would get populated by Trac's own default optional plugins and 3^rd party ones. A wiki page for coordination could be used.

I thought the only thing that should be coordinated is to make tracext a ​namespace? Anyway, I would keep the tracext namespace for "official" use only, that is, for plugins developed on t.e.o and not for third-party plugins.

[Remy Blank]

Renamed the ChangesetMessage macro to CommitTicketReference in [8380].

[Christian Boos]

Closing. The ticket still needs to be moved to milestone:0.12-multirepos once it gets created. In the meantime, one can use a custom query selecting the multirepos keyword in order to see the status (like the one in the MultipleRepositorySupport page).