Edgewall Software
Modify

Opened 4 years ago

Closed 4 years ago

Last modified 6 weeks ago

#9570 closed enhancement (fixed)

Merge RepoRevisionSyntaxPlugin into core?

Reported by: shesek Owned by: rblank
Priority: normal Milestone: 0.12.1
Component: general Version:
Severity: normal Keywords: WikiFormatting, traclinks
Cc: trbs@…
Release Notes:
API Changes:

Description

When using multiple repositories, all the WikiFormatting / TracLinks has a syntax for linking to a changeset/revision of a specific repository ([123/repo] and [changeset:123/repo]), other than the r123 syntax.

I find the r123 syntax the most readable one and think its weird that it can't be used when using multiple repositories. Therefore, I suggest merging the functionality of RepoRevisionSyntaxPlugin (disclosure: this is a plugin I wrote) to core, or using some alternative syntax (repo/r123 maybe?) that allows using this syntax with multiple repositories somehow.

Attachments (1)

9570-restrict-path-r10006.patch (3.7 KB) - added by rblank 4 years ago.
Allow r123/path and r123:456/path syntax.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 4 years ago by rblank

  • Milestone set to 0.13
  • Owner set to rblank

Why use the syntax r123repo, and not e.g. r123/repo, which would be very similar to the [123/repo] syntax? Also, AFAIK the current regexp for the r123 syntax allows hexadecimal revisions (hashes), so without a separator, this might conflict with the repository name (e.g. revision 12 in repository "best" would be written "r12best", which could also be interpreted as revision 12b in repository "est", or revision 12be in repository "st").

So, how about r123/repo (which can conveniently be read as "revision 123 in repository "repo")?

comment:2 Changed 4 years ago by shesek

Sure, as I said - any syntax will be fine. In the plugin itself I noted that the only reason I used the r123repo syntax is that Trac parsed the r123 part before my plugin did, so I couldn't implement any `repo/r123' or 'r123/repo' syntax.

I do think that r123/repo is a better choice.

BTW, when does Trac has non-numeric revisions? it seems like 'r1b79' isn't getting parsed to a TracLink

comment:3 Changed 4 years ago by rblank

Let's see: r12ab [12ab]

But: r12ab34cd [12ab34cd]

You are right, the hexadecimal revision numbers are only supported with the bracket syntax, and only for 8-digit revisions or longer. The r123 syntax only supports digits. So there should be no ambiguity, but I still prefer r123/repo :)

comment:4 follow-up: Changed 4 years ago by shesek

Yeah, I agree. Any chance this syntax will be used by CommitTicketUpdater too, or make the syntax used by it configurable in TracIni?

comment:5 in reply to: ↑ 4 Changed 4 years ago by rblank

Replying to shesek:

Any chance this syntax will be used by CommitTicketUpdater too, or make the syntax used by it configurable in TracIni?

Nah, it looks too ugly with hexadecimal changesets. But you could create a small plugin that subclasses CommitTicketUpdater and overrides make_ticket_comment() to format the ticket comment differently.

comment:6 Changed 4 years ago by trbs <trbs@…>

  • Cc trbs@… added

Changed 4 years ago by rblank

Allow r123/path and r123:456/path syntax.

comment:7 Changed 4 years ago by rblank

9570-restrict-path-r10006.patch adds the r123/path syntax for changesets and r123:456/path for the log.

However, contrary to the [123/path] syntax, the path is limited to characters in [a-zA-Z0-9_/.+-]. Indeed, using a word boundary or whitespace as the end of path would make the comma part of the path in "changed in r123/path, and …". The period could also be removed, for the typical phrase "fixed in r123/path.". If characters outside of that set are needed, the [123/path] syntax should be used instead.

Please test.

Last edited 4 years ago by rblank (previous) (diff)

comment:8 follow-up: Changed 4 years ago by shesek

Seems to work fine here. I do think that period should not be matched too, and the [123/path] syntax should be used instead on those cases to support "fixed in r123/path." cases. BTW, I never used (nor knew it was possible to use) an entire path and not just the repository name there. Good to know.

comment:9 in reply to: ↑ 8 Changed 4 years ago by rblank

Replying to shesek:

I do think that period should not be matched too, and the [123/path] syntax should be used instead on those cases to support "fixed in r123/path." cases.

Agreed. I'll fix that before applying. Thanks for testing!

comment:10 Changed 4 years ago by rblank

  • Milestone changed from 0.13 to 0.12.1
  • Resolution set to fixed
  • Status changed from new to closed

Patch applied in [10008] (to 0.12-stable by mistake, sorry).

comment:11 Changed 6 weeks ago by rjollos

  • Keywords traclinks added; TracLinks removed

comment:12 Changed 6 weeks ago by rjollos

  • Keywords traclink added; traclinks removed

comment:13 Changed 6 weeks ago by rjollos

  • Keywords traclinks added; traclink removed

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed The owner will remain rblank.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from rblank to the specified user.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.