314097 - Support general autolinkification for integration with SCM systems

Status: RESOLVED WORKSFORME

(Bugzilla :: User Interface, enhancement)

Description

[Kristis Makris]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050718 Galeon/1.3.20 (Debian package 1.3.20-1)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.8) Gecko/20050718 Galeon/1.3.20 (Debian package 1.3.20-1)

Support general autolinkification for integration with SCM systems.

It'd be swell to see the problem of autolinking from Bugzilla or Mantis
to an SCM system addressed. There have been a few attempts in the past,
from various people (Aleksey Pershinhttp://bugzilla.mkgnu.net/show_bug.cgi?id=266#c4, Dirk Datzert
http://bugzilla.mkgnu.net/show_bug.cgi?id=266#c3, and Dave Swegen in bug #199116). It would be nice if there was a convention 3rd party integration of SCM systems could follow when it comes to inserting comments in a bug-tracker. Then bug-trackers could search and parse for that syntax and autolinkify
accordingly to their favorite linkification system (CVSView, WebSVN,
etc).

Developers of bug-tracking systems should come to an agreement on how this will be implemented. Aleksey Pershin's design is described in incredible detail and I'd recommend it is reviewed first.

The MantisBT developers will also be notified about this. Let's try to come to an agreement of what the format will be.

Until bug-trackers modify their database schemas to support a list of affected files, and possibly other fields (e.g. branch names), the _only_ thing an integration system can do is insert the list of changes in the comment directly. And the bug-tracker needs to interpret that list directly.


Reproducible: Always

Steps to Reproduce:
1. Integrate an SCM system with Bugzilla (or even Mantis)
2. Commit something in the SCM system, anything.
3. View in the bugtracker the comments:
4. **Wish that the list of affected files was autolinkified so you could view from the bug the source code diff, the older version of a modified file, or the new version**


Actual Results:  
The list of affected files is not autolinkified. e.g.:

Affected files:
---------------
388 --> 723 PDX:trunk/Common/Inc/HashCell.h
391 --> 723 PDX:trunk/Common/Src/HashCell.cpp
708 --> 723 PDX:trunk/Testing/Common/CommTest.cpp
690 --> 723 PDX:trunk/Testing/Common/Inc/SpecTabl.h
704 --> 723 PDX:trunk/Testing/Common/Src/SpecTabl.cpp

Expected Results:  
The list of affected files IS autolinkified. E.g:
- clicking on "388" brings up the source code for revision 388 of the file trunk/Common/Inc/HashCell.h in Project PDX in WebSVN.
- clicking on "723" brings up the source code for revision 723 of the file trunk/Common/Inc/HashCell.h in Project PDX in WebSVN.
- clicking on "-->" brings up the diff between revision 388 and revision 723 on file trunk/Common/Inc/HashCell.h in Project PDX in WebSVN.


The Source code viewing tool (WebSVN, ViewCVS) URL should be configurable in the bug-tracker.

Comment 1

[Kristis Makris]

MantisBT has an issue open for addressing this at
http://bugs.mantisbt.org/view.php?id=6368

Comment 2

[Kristis Makris]

Attachment: Autolinkification patch

This is an autolinkification patch written by Aleksey 'Commander' Pershin <[e-mail]>. It is based on Bugzilla 2.22, but has been used without any changes since Bugzilla 2.18, so it should apply to most Bugzilla versions.

The patch offers WebSVN autolinkification. It assumes that comments listing the changed files are formatted according to the SCM/Bugtracking Integration format defined in http://bugzilla.mkgnu.net/show_bug.cgi?id=266#c5 . This format was defined by Scmbug, but is open for discussion.

Please consider this patch as a basis for supporting generic autolinkification with other source viewers, like CVSView.

Comment 3

[victory <[e-mail]>]

you'll have to make a patch against cvs HEAD as this won't be taken into branches

Comment 4

[Kristis Makris]

Attachment: Autolinkification patch against HEAD as of 2006-08-18

Comment 5

[Kristis Makris]

Attachment: Autolinkification patch against HEAD as of 2006-08-18

Comment 6

[Kristis Makris]

Comment on attachment 239096 [details] [diff] [review]
Autolinkification patch against HEAD as of 2006-08-18

Sorry. I added it twice.

Comment 7

[Kristis Makris]

Comment on attachment 239097 [details] [diff] [review]
Autolinkification patch against HEAD as of 2006-08-18

Using "PDX" as a hack will not be needed. However, it is valuable to assume that the project prefix $6 is optional, since this will make the patch backwards compatible with existing Scmbug integration comments in a Bugzilla instance when the project  prefix was not inserted in comments.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 239097 [details] [diff] [review]
Autolinkification patch against HEAD as of 2006-08-18

Hmm, this looks like it would be quite useful for sites using Scmbug.

Thanks for including all the comments!  They are very helpful.


>+               (\s*-->\s*)  (?# 2  The arrow and spaces before/after      )

Does Scmbug really insert a literal >?  That seems suboptimal, since we do character escaping on comments ourselves and will probably escape that ampersand.  Or have we already escaped that character by the time we get to this subroutine?


>+              ~ "$1$2$3$4<a href=\"/wsvn/"

This URL should be a parameter with embedded directives for the various pieces of information (filename, versions, etc.), so admins can configure it to work with their own particular source code and revision viewers like LXR/Bonsai (or even with WebSVN when running on a different server).

Also, what are the performance implications of this patch?  In the past we've seen significant slowdown when modifying the regexps in this subroutine, and we want to make sure that doesn't happen when we add new regexps (i.e. we want to make sure all new regexps get optimized for speed).

Otherwise, this looks good.  But I'll want a second reviewer's opinion given the significant consequences of a performance regression in this part of the code.

Comment 9

[Commander]

(In reply to comment #7)
> Using "PDX" as a hack will not be needed. However, it is valuable to assume
> that the project prefix $6 is optional, since this will make the patch
> backwards compatible with existing Scmbug integration comments in a Bugzilla
> instance when the project  prefix was not inserted in comments.

Definitely, I hardcoded PDX to get around comments that did not
have the Subversion project in them. To make this a general solution,
I would recommend using the project from the bug itself. Obviously,
for this to work, the project name in Bugzilla must match the project
name in Subversion.

Comment 10

[Commander]

(In reply to comment #8)
> >+ (\s*-->\s*)  (?# 2  The arrow and spaces before/after      )
> 
> Does Scmbug really insert a literal >?  That seems suboptimal, since we do
> character escaping on comments ourselves and will probably escape that
> ampersand.  Or have we already escaped that character by the time we get to
> this subroutine?

In my original code I used > instead of >, but it did not work.
I assume that by the time this function is called, everything in the comment
has already been escaped properly.

> >+              ~ "$1$2$3$4<a href=\"/wsvn/"
> 
> This URL should be a parameter with embedded directives for the various
> pieces of information (filename, versions, etc.), so admins can configure
> it to work with their own particular source code and revision viewers like
> LXR/Bonsai (or even with WebSVN when running on a different server).

Definitely. IMHO, it would be better to have a URL parameter
in Bugzilla's configuration with several replacable variables
similarly to the old whining email. It would make it possible to
shuffle the file path, its version and so on in the URL to make
it work with other "webonizers" and not only with WebSVN.

> Also, what are the performance implications of this patch?  In the past we've
> seen significant slowdown when modifying the regexps in this subroutine, and
> we want to make sure that doesn't happen when we add new regexps (i.e. we
> want to make sure all new regexps get optimized for speed).

I have not noticed any significant performance degradation
on my server even on bugs with a large number of comments produced
by SCMBUG. It is a relatively weak system (AMD Semphron 2500+) running
on Windows 2000. However, the server is not pushed to its limits
that much: there are not so many users and it only runs Bugzilla
and Subversion. It would be hard to extrapolate my experience
to a large server like Bugzilla's.

There is another way to look at this issue. In the original patch
I tried to make the regex as restricting as possible, especially
in the beginning. Since there should not be that many lines that
would match the non-path part of the regex, the regex should run
relatively fast through non-matching lines.

Moreover, it does not look like the regex can be sped up that much.
There are certain things that it has to match. Even if it means
a 10% increase in page loading times, I can live with that. The
amount of time all those autolinkified comments save for the code
reviewer is simply unbelievable.

Comment 11

[Max Kanat-Alexander]

Comment on attachment 239097 [details] [diff] [review]
Autolinkification patch against HEAD as of 2006-08-18

I don't think this is the direction that we want to go, upstream. It's fine to include as a patch in scmbug, but if we add additional linkification, it should be a generic system where people can specify any particular thing that they want linked, with a particular regex format.

That is, I don't want to put a patch into Bugzilla *just* for ScmBug.

Comment 12

[Kristis Makris]

(In reply to comment #11)
> I don't think this is the direction that we want to go, upstream. It's fine to
> include as a patch in scmbug, but if we add additional linkification, it should
> be a generic system where people can specify any particular thing that they
> want linked, with a particular regex format.

Sadly, I agree.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

> I don't think this is the direction that we want to go, upstream. It's fine to
> include as a patch in scmbug, but if we add additional linkification, it should
> be a generic system where people can specify any particular thing that they
> want linked, with a particular regex format.

We've said such things a lot, and generally they mean not having functionality for years while we wait for the generic version to arrive.  In this case (and probably in some of those past ones), I think we're better off taking the specific functionality now and converting it to use the generic functionality if/when that becomes available.

Comment 14

[Dmitri Colebatch]

As a user, I'd be interested in this functionality.  Whilst I realise that you (rightly) don't want to implement something specifically for scmbug, it would be nice to somehow make some movements in this area.

Comment 15

[Frédéric Buclin]

This is now possible thanks to the bug_format_comment hook, see http://www.bugzilla.org/docs/tip/en/html/api/Bugzilla/Hook.html#bug_format_comment.