Configurable hyperlinking in gitk

Many projects use project-specific notations in comments to refer to bug trackers and the like. One example is the "Closes: #nnnnn" notation used in Debian.

Make gitk configurable so that arbitrary strings can be turned into clickable links that are opened in a web browser.
---
Some time ago I hardcoded this into gitk for $DAY_JOB and find it very useful. I made it configurable in the hopes that it might be adopted upstream. (unfortunately, the configurable version is radically different from the original hard-coded version, so I can't say this has had much testing yet)

The definition of the allowed regular expression in the docs probably needs some refinement. Basically, they have to also be REs that can be concatenated with the "|" character, which is not true of REs that begin with the *** flavor selector (which I had not heard of before rereading man re_syntax just now) or (?xyz) embedded options. Or maybe there's an efficient alternate approach to scanning for the next non-overlapping match among several patterns that doesn't involve concatenating the patterns.

I'm not sure about the "one line" restriction; at first I thought that everything was fed to 'appendwithlinks' in arbitrary chunks, but not I see that they are mostly logical chunks (and probably only the comment, not the headers or commit descriptors, will have anything to linkify). The problem again seems to be how to succinctly describe what is permitted.

There are probably better names for the configuration options, too.

Suggestions? Problems? Successes?

Jeff

 Documentation/config.txt |   31 ++++++++++++++++++-
 gitk-git/gitk            |   74 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 102 insertions(+), 3 deletions(-)

index 750c86d..67ed436 100644
@@ -1102,6 +1102,33 @@ All gitcvs variables except for 'gitcvs.usecrlfattr' and
 is one of "ext" and "pserver") to make them apply only for the given
 access method.
 
+gitk.linkify.<name>.re::
+	Specify a Tcl regular expression (which may not span lines)
+	defining a class of strings to automatically convert to hyperlinks.
+	You must also specify 'gitk.linkify.<name>.sub'.
+
+gitk.linkify.<name>.sub::
+	Specify a substitution that results in the target URL for the
+	related regular expression.  Back-references like '\1' refer
+	to capturing groups in the associated regular expression.
+	You must also specify 'gitk.linkify.<name>.re'.
+
+gitk.browser::
+	Specify the browser that will be used to display the linked
+	web page.
+
+For example, to automatically link from Debian-style "Closes: #nnnn"
+message to the Debian BTS,
+
+--------
+    git config gitk.linkify.debian-bts.re 'Closes: #(\d+)'
+    git config gitk.linkify.debian-bts.sub 'http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=\1'
+--------
+
+Regular expressions are as described in re_syntax(n).  Replacements
+are as described in regsub(n).  If multiple regular expressions match at
+the same location, it is undefined which match is used.
+
 grep.lineNumber::
 	If set to true, enable '-n' option by default.
 

@@ -1901,5 +1928,5 @@ user.signingkey::
 
 web.browser::
 	Specify a web browser that may be used by some commands.
-	Currently only linkgit:git-instaweb[1] and linkgit:git-help[1]
-	may use it.
+	Currently only linkgit:git-instaweb[1], linkgit:gitk[1],
+	and linkgit:git-help[1] may use it.

1.7.0.4

Hi,

This is definitely something folks at my $dayjob would be interested in. We've already done some customisation of gitweb to do something similar. I'm not actually sure what the changes where or how configurable they are. I'll see if I can dig them out on Monday someone else might want to polish them into something suitable (I might do it myself if I get some tuits).

For my use case the one line restriction is fine. We tend to put the bug number in the headline anyway.

Sometimes when a commit fixes multiple bugs we put all the bug numbers in separated by commas. I don't know Tcl well enough to tell if your code supports that or not.

It'd be nice if the config variables weren't gitk specific. .re and .sub could be applied to gitweb and maybe other git viewers outside of gig.git might decide to use them. My bikeshedding suggestion would be to just drop the gitk prefix and have linkify.re and linkify.sub.

Re-compiling now. I won't be able to actually test it properly until I'm back in the office but I can at least check that the links are generated.

This seems like a reasonable idea, though since the implementation languages of gitk and gitweb are different it means some REs might get different interpretations in the different programs.

Multiple matches per line are OK, but they must be non-overlapping.

Looking at the actual practice in Debian changelogs, I see that they do this:

    evince/changelog.Debian.gz:        (Closes: #388368, #396467, #405130)

so my original example would only linkify "Closes: #388638". But a revised pattern of #(\d+) would linkify "#388368", "#396467" and "#405130". (but risk a few more "false positive" links). I should revise my example accordingly.

As for the problems with your substitutions, "&" is special in a tcl regsub (it stands for the whole matched string, like \0), so you'd want to use a substitution like

    git config gitk.linkify.debian-bts.sub \
        'http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=\1\&foo=bar'

The problem with "%" has to do with Tk's event substitution and it's a bug that this happens; I should manually double the % at the proper point.

This revised patch fixes the problem with % in substitutions and changes the suggested RE for matching debian bts items, but it does not rename the configuration options.

-- >8 --
Many projects use project-specific notations in changelogs to refer to bug trackers and the like. One example is the "Closes: #12345" notation used in Debian.

Make gitk configurable so that arbitrary strings can be turned into clickable links that are opened in a web browser.

---
 Documentation/config.txt |   31 ++++++++++++++++++-
 gitk-git/gitk            |   75 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 103 insertions(+), 3 deletions(-)

index ae9913b..13e8aa6 100644
@@ -1064,6 +1064,33 @@ All gitcvs variables except for 'gitcvs.usecrlfattr' and
 is one of "ext" and "pserver") to make them apply only for the given
 access method.
 
+gitk.linkify.<name>.re::
+	Specify a Tcl regular expression (which may not span lines)
+	defining a class of strings to automatically convert to hyperlinks.
+	You must also specify 'gitk.linkify.<name>.sub'.
+
+gitk.linkify.<name>.sub::
+	Specify a substitution that results in the target URL for the
+	related regular expression.  Back-references like '\1' refer
+	to capturing groups in the associated regular expression.
+	You must also specify 'gitk.linkify.<name>.re'.
+
+gitk.browser::
+	Specify the browser that will be used to display the linked
+	web page.
+
+For example, to automatically link from Debian-style "Closes: #nnnn"
+message to the Debian BTS,
+
+--------
+    git config gitk.linkify.debian-bts.re '#(\d+)\M'
+    git config gitk.linkify.debian-bts.sub 'http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=\1'
+--------
+
+Regular expressions are as described in re_syntax(n).  Replacements
+are as described in regsub(n).  If multiple regular expressions match at
+the same location, it is undefined which match is used.
+
 grep.lineNumber::
 	If set to true, enable '-n' option by default.
 

@@ -1870,5 +1897,5 @@ user.signingkey::
 
 web.browser::
 	Specify a web browser that may be used by some commands.
-	Currently only linkgit:git-instaweb[1] and linkgit:git-help[1]
-	may use it.
+	Currently only linkgit:git-instaweb[1], linkgit:gitk[1],
+	and linkgit:git-help[1] may use it.

1.7.2.5

Hmm no joy with \&. Seems to upset the invocation of git web-browse

  git config gitk.linkify.bugtracker.sub \
       'https://internalhost/code\&stuff/bugs.php?id=\1'

gitk
/home/chrisp/libexec/git-core/git-web--browse: line 167:
stuff/bugs.php?id=bug123: No such file or directory
fatal: 'web--browse' appears to be a git command, but we were not able to execute it. Maybe git-web--browse is broken?

Using the following works as expected with no error with your updated patch.

  git config gitk.linkify.bugtracker.sub \
       'https://internalhost/code%26stuff/bugs.php?id=\1'

This is probably a issue with git web--browse and nothing to do with your changes.

Sure enough this works fine

  git web--browse --browser=firefox \
      https://internalhost/code\&stuff/bugs.php?id=foo

While this doesn't

git web--browse https://internalhost/code\&stuff/bugs.php?id=foo

/home/chrisp/libexec/git-core/git-web--browse: line 167:
stuff/bugs.php?id=foo: No such file or directory
fatal: 'web--browse' appears to be a git command, but we were not able to execute it. Maybe git-web--browse is broken?

Neither does this

  git web--browse --browser=konqueror \
     https://internalhost/code\&stuff/bugs.php?id=foo

A little bit more info that might help diagnose the issue - I'm running openSUSE 11.4 (kde 4.6) which ships with firefox set as the default web browser so 'kfmclient newTab http://www.example.com' actually opens firefox.

However trying kfmclient with my funny URL still works

kfmclient newTab https://internalhost/code\&stuff/bugs.php?id=foo

I'm a little stumped as to what is going wrong in git web--browse.

Update: it's the call to eval that causes the problem

eval kfmclient newTab https://internalhost/code\&stuff/bugs.php?id=foo
[1] 14728
bash: stuff/bugs.php?id=foo: No such file or directory

Hmm. The offending lines look like:

eval "$browser_path" "$@" &

Normally in git we treat user-configured commands as shell snippets, meaning the user is responsible for any quoting. But in this script, we seem to run:

type "$browser_path"

several times. Which implies that "$browser_path" must be the actual executable. In which case, I would think that:

"$browser_path" "$@" &

would be the right thing. And indeed, that is what the firefox arm of the case statement does. But chrome, konqueror, and others use eval.

Unrelated, but it also looks like $browser_path is used unquoted in the firefox case (see inside the vers=$(...)).

-Peff

Instead of using eval which causes problems when a URL contains an appropriately escaped ampersand (\&).

---

So here is my attempt at a fix for kfmclient.

For what it's worth I've included a testcase that detects my problem. I'm not sure if the testcase is really worth it because the test library suppresses X applications and even if it didn't the testcase is fairly trivial and might just annoy people by opening web-browsers (and it snaps up the last t99xx prefix).

 git-web--browse.sh         |    4 ++--
 t/t9901-git-web--browse.sh |   43 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 2 deletions(-)
 create mode 100755 t/t9901-git-web--browse.sh

index e9de241..1164a22 100755
@@ -164,10 +164,10 @@ konqueror)
 		# It's simpler to use kfmclient to open a new tab in konqueror.
 		browser_path="$(echo "$browser_path" | sed -e 's/konqueror$/kfmclient/')"
 		type "$browser_path" > /dev/null 2>&1 || die "No '$browser_path' found."
-		eval "$browser_path" newTab "$@"
+		"$browser_path" newTab "$@" &
 		;;
 	kfmclient)
-		eval "$browser_path" newTab "$@"
+		"$browser_path" newTab "$@" &
 		;;
 	*)
 		"$browser_path" "$@" &

1.7.7.rc1.3.g5593.dirty

I think this probably should just remove all of the evals. I don't see how any of them is doing any good, and they're actively breaking URLs that need quoting.

Hmm. Actually, the one for custom browser commands might need it, because that one is expected to be a shell snippet. I suspect the simplest thing is to do something like:

eval "$browser_cmd \"\$@\""

The other option would be to actually shell-quote each argument, which is a pain to do in the shell (but is what C git does).

Ick, yeah. Actually starting real browsers interacts too much with the world outside of the test scripts. The results will be annoying (new browser windows) and cause non-deterministic test results.

If you want to make a test, I think you would do better with something like:

  echo someurl_with_&_in_it >expect &&
  git config browser.custom.cmd echo &&
  git web--browse --browser=custom someurl_with_&_in_it >actual &&
  test_cmp expect actual

That won't test that we are invoking kfmclient correctly, obviously, but you can confirm at least that URLs are making it through to the browser script intact.

-Peff

Using eval causes problems when the URL contains an appropriately escaped ampersand (\&). Dropping eval from the built-in browser invocation avoids the problem.

---
Here's an updated patch which drops the uses of eval when invoking a supported browser. The default case still uses eval but adds some extra quoting which also fixes the problem. I've avoided touching the 'start' case because I don't have access to a windows system to test with.

I've replaced my tests With the test suggested by Peff (should I be giving him credit in the copyright line or something?). I've grabbed t9901 but if there is a better set of miscellaneous minor tests that I should be using let me know.

 git-web--browse.sh         |   10 +++++-----
 t/t9901-git-web--browse.sh |   21 +++++++++++++++++++++
 2 files changed, 26 insertions(+), 5 deletions(-)
 create mode 100755 t/t9901-git-web--browse.sh

index e9de241..ee05f10 100755
@@ -156,7 +156,7 @@ firefox|iceweasel|seamonkey|iceape)
 	;;
 google-chrome|chrome|chromium|chromium-browser)
 	# No need to specify newTab. It's default in chromium
-	eval "$browser_path" "$@" &
+	"$browser_path" "$@" &
 	;;
 konqueror)
 	case "$(basename "$browser_path")" in

@@ -164,10 +164,10 @@ konqueror)
 		# It's simpler to use kfmclient to open a new tab in konqueror.
 		browser_path="$(echo "$browser_path" | sed -e 's/konqueror$/kfmclient/')"
 		type "$browser_path" > /dev/null 2>&1 || die "No '$browser_path' found."
-		eval "$browser_path" newTab "$@"
+		"$browser_path" newTab "$@" &
 		;;
 	kfmclient)
-		eval "$browser_path" newTab "$@"
+		"$browser_path" newTab "$@" &
 		;;
 	*)
 		"$browser_path" "$@" &

@@ -175,7 +175,7 @@ konqueror)
 	esac
 	;;
 w3m|elinks|links|lynx|open)
-	eval "$browser_path" "$@"
+	"$browser_path" "$@"
 	;;
 start)
 	exec "$browser_path" '"web-browse"' "$@"

@@ -185,7 +185,7 @@ opera|dillo)
 	;;
 *)
 	if test -n "$browser_cmd"; then
-		( eval $browser_cmd "$@" )
+		( eval $browser_cmd \""$@"\" )
 	fi
 	;;
 esac

new file mode 100755
index 0000000..141ed17
@@ -0,0 +1,21 @@
+#!/bin/sh
+#
+# Copyright (c) 2011 Chris Packham
+#
+
+test_description='git web--browse basic tests
+
+This test checks that git web--browse can handle various valid URLs.'
+
+. ./test-lib.sh
+
+test_expect_success \
+	'accepts a URL with an ampersand in it' '
+	echo http://example.com/foo\&bar/ >expect &&
+	git config browser.custom.cmd echo &&
+	git web--browse --browser=custom \
+		http://example.com/foo\&bar/ >actual &&
+	test_cmp expect actual
+'
+
+test_done

1.7.7.rc1.4.g47f23.dirty

Although other projects do use "cc" in the commit message, I think we don't usually bother adding this noise in the git project. The cc headers in your email are enough.

For a minor bit of help, usually mentioning the person in the commit message (with a "Helped-by", or indicating which parts they contributed to) is plenty. Personally, I don't even care much about that. My contributions to git are thoroughly documented in the commit history and the mailing list at this point. :)

I also find the "Copyright ..." lines in the files to be overkill, too. They end up becoming out-of-date as other people work on the file. The commit history is the best way to get the right answer, and a comment in the file is at best redundant with what's there. But that is just my opinion; I don't know that we have a particular policy for such things[1].

-Peff

[1] Once upon a time, I think I saw the advice that every file should have a copyright notice and mention the license at the top of the file, but I don't know that it has ever been tested in court. I suppose the distributed tarballs of a particular version would lack the copyright attribution, but in that case, my solution would be to generate it from the commit history at packaging time.

That's more for git send-email's benefit than anything else. I'm working on a laptop with a touchpad (and a cat) so the less switching between editor and MUA the better. Any better suggestions for tracking Cc's for git send-email?

The example in t/README has has a copyright notice which is why I put one in but I don't consider the test (or the fix itself) to actually be copyrightable. If I wasn't creating a new file I wouldn't have bothered putting anything in (other than the testcase).

It would depend on your workflow, I think. You can use --cc to add headers to format-patch. You could get very fancy and store them in git-notes or somewhere else, and then pull them in with send-email's cc-cmd option. But I suspect you just want to stick them in the commit message one time and then have it used each time.

If put them after the double-dash line in your commit message, like:

subject

body
---
cc: whoever

Then that will be included verbatim in the mail by format-patch, send-email will respect the cc line, and those lines will be dropped by "git am" when Junio applies the patch (they are still a slight noise to readers of the mail, but at least they don't make it into the commit history).

Yeah, that's why I said I don't know if we have a policy. We clearly have a lot of copyright statements, but they are all horribly out of date. I was hoping Junio might weigh in.

-Peff

To be honest, I do not care very much either way. From the licensing point of view we know everything is covered by the top-level COPYING unless otherwise noted explicitly in an individual file (which is not the case for this patch anyway), and even without the copyright notice we can trace where the files come from with "git log", so these three lines in a small test file are essentially noise, not very useful but are not irritating enough to warrant an effort from me to amend it out.

I do have a mild objection to a patch that adds a new copyright notice line to an existing file when it adds only a few new lines, though. When the code is refactored and these new lines are made unneeded, it is likely that nobody would bother removing that copyright notice line that names the author of the patch that added these lines.

Yeah, I agree, and the dq around $browser_cmd is kind of important, too, for that to work and be readable.

Hmm, isn't '&' somewhat an unusual in URL? ...ah, not really, if it is in the query parameter part it is quite common.

Thanks.

Oops, good catch. Probably the most readable version would be:

eval "\"$browser_cmd\"" '"$@"'

-Peff

Actually I didn't mean that double dq.

In fact, if browser_cmd is meant to be split as a shell snippet, I do not think you want the string seen by eval to have dq around the expanded version of $browser_cmd. And I tend to prefer feeding a single string to eval, so the version in your message I quoted originally looks good to me.

Unless I am missing something here...?

Oh right. Sorry, I read your comment, thought that's what you meant, and that I had overlooked something. Forgetting that it was intentional to leave off the quotes inside.

So yeah, my original is right. I just got turned around in all of the discussion.

Sorry for the noise.

-Peff

Thinking about it a bit more, I suspect that we should just let the 'eval' grab value out of the $browser_cmd variable, i.e.

	eval '$browser_cmd "$@"'

no?

That's a Useless Use of Eval and 100% equivalent to this:

$browser_cmd "$@"

Andreas.

Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Yeah. Doing:

eval '$browser_cmd'

will do the whitespace-breaking we want, but it won't interpret actual shell magic characters, which we need in order to be compatible with other parts of git (which typically use "sh -c ..."). E.g.:

foo=worked
browser_cmd='echo $foo'

  # fail
  $browser_cmd
  # fail
  eval '$browser_cmd'
  # works
  eval "$browser_cmd"

-Peff

Yikes, I forgot that we do that. You are right.

This make the use of eval even more questionable. If $browser_cmd is supposed to be whitespace splitted then this won't do it (unless it contains embedded double quotes).

Andreas.

Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

In newstyle URLs the name=value pairs in CGI parameter query string are separated with semicolons ';' rather than ampersands '&', because of problem with & <-> &amp;.

Just FYI.

Jakub Narębski

Hi,

Yeah, I don't remember why I sometimes used 'eval "$browser_path" "$@"' when I wrote this code. Sorry!

Thanks,
Christian.

Jeff: This is great -- thanks!

I still had problems with using an & in the URL, even with the updated patch.
I had to apply Chris's git-web--browse patch to get it to work.

		M.

That should be linkify.<name>.re and linkify.<name>.sub

Slight complication. The URL of our bug tracker has an ampersand '&' in it. Tcl's substitution does what one might expect and puts the matched text where the '&' is. I've tried using url friendly %26 but something eats the %. I've also tried backslashes to no avail.

To answer my own question since I started writing this email I've found that using %% works (only the first one gets eaten). Not sure if that's expected behaviour or not (printf escaping maybe?).

Also since I've been playing around I've tried a commit with multiple bug numbers on one line and that works as expected.

Thanks
Chris

That would be nice. So called "committags" support was long planned for gitweb, and even some preliminary work exists...

Perhaps more descriptive name, i.e.

linkify.<name>.regexp
linkify.<name>.subst

would be better?

I guess that regexp is an extended regular expression, isn't it?

Jakub Narębski

If "regexp" is clearer than "re" then I have no quarrel with changing it. The typical user won't be typing these over and over, so the value of brevity is limited.

As written, it's whatever is accepted by tcl's regular expression matcher, which is described in re_syntax(n), installed as re_syntax(3tcl) on debian-derived systems. A one-sentence summary of a TCL "ARE" is "basically EREs with some significant extensions".

It is probably possible to write expressions that are going to work the same in tcl, perl, and posix regular expressions, but to some extent the user who writes a complex expression and then tries to use it with both gitk and a future gitweb will simply be permitted to keep both pieces when it breaks.

Is it unnecessarily complicated to design
linkify.<name>.(regexp|subst)
*AND*
gitk.linkify.<name>.(regexp|subst)
in from the start? This way the hypothetical power user can write a different version of the expression for gitk and future gitweb if it is required by RE dialect differences.

Jeff

Many projects use project-specific notations in changelogs to refer to bug trackers and the like. One example is the "Closes: #12345" notation used in Debian.

Make gitk configurable so that arbitrary strings can be turned into clickable links that are opened in a web browser.

---
Since the previous patch, I
* Renamed configuration variables to get rid of the "gitk" prefix to encourage other git-related programs to adopt the same functionality.

* Renamed configuration variables from cryptic ".re", ".sub" to less cryptic ".regexp" and "subst"

* Changed the example RE to be an ERE (no \d or \M)

* Documented that these are POSIX EREs; hopefully that's OK. I see in CodingGuidelines that in git itself "a subset of BREs" are used, so maybe even this is too much power. And hopefully tcl's re_syntax really is close enough to an ERE superset that this isn't a terrible lie about the initial implementation either.

* Added a Signed-Off-By, since I've had a number of positive feedbacks and the only problems I've heard of (since patch v2) are the ones related to 'eval' in git-web--browse.

In v2 of the patch, I had fixed a problem with %-signs in URLs and changed the documentation example.

 Documentation/config.txt |   30 +++++++++++++++++-
 gitk-git/gitk            |   75 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 102 insertions(+), 3 deletions(-)

index ae9913b..ffc9ccf 100644
@@ -1064,6 +1064,10 @@ All gitcvs variables except for 'gitcvs.usecrlfattr' and
 is one of "ext" and "pserver") to make them apply only for the given
 access method.
 
+gitk.browser::
+	Specify the browser that will be used to open links generated by
+	'linkify' configuration options.
+
 grep.lineNumber::
 	If set to true, enable '-n' option by default.
 

@@ -1317,6 +1321,28 @@ interactive.singlekey::
 	setting is silently ignored if portable keystroke input
 	is not available.
 
+linkify.<name>.regexp::
+	Specify a regular expression in the POSIX Extended Regular Expression
+	syntax defining a class of strings to automatically convert to
+	hyperlinks.  This regular expression many not span multiple lines.
+	You must also specify 'linkify.<name>.subst'.
+
+linkify.<name>.subst::
+	Specify a substitution that results in the target URL for the
+	related regular expression.  Back-references like '\1' refer
+	to capturing groups in the associated regular expression.
+	You must also specify 'linkify.<name>.regexp'.
++
+For example, to automatically link from Debian-style "Closes: #nnnn"
+message to the Debian BTS,
++
+--------
+    git config linkify.debian-bts.regexp '#([1-9][0-9]*)'
+    git config linkify.debian-bts.subst 'http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=\1'
+--------
++
+Currently, only linkgit:gitk[1] converts strings to links in this fashion.
+
 log.abbrevCommit::
 	If true, makes linkgit:git-log[1], linkgit:git-show[1], and
 	linkgit:git-whatchanged[1] assume `\--abbrev-commit`. You may

@@ -1870,5 +1896,5 @@ user.signingkey::
 
 web.browser::
 	Specify a web browser that may be used by some commands.
-	Currently only linkgit:git-instaweb[1] and linkgit:git-help[1]
-	may use it.
+	Currently only linkgit:git-instaweb[1], linkgit:gitk[1],
+	and linkgit:git-help[1] may use it.

1.7.2.5

Many projects use project-specific notations in changelogs to refer to bug trackers and the like. One example is the "Closes: #12345" notation used in Debian.

Make gitk configurable so that arbitrary strings can be turned into clickable links that are opened in a web browser.

---
This v3 patch didn't generate any discussion last time around (~3 weeks ago), so I've taken the liberty of reposting it.

I'm aware of no problems with this patch, and a number of people have commented that it is useful to them. For URLs that contain "&" and other shell metacharacters, it *does* depend on r480f062c "git-web--browse: avoid the use of eval" which is in next but not maint.

Since the V2 patch, I
* Renamed configuration variables to get rid of the "gitk" prefix to encourage other git-related programs to adopt the same functionality.

* Renamed configuration variables from cryptic "re", "sub" to less cryptic "regexp" and "subst"

* Changed the example RE to be an ERE (no \d or \M)

* Documented that these are POSIX EREs; hopefully that's OK. I see in CodingGuidelines that in git itself "a subset of BREs" are used, so maybe even this is too much power. And hopefully tcl's re_syntax really is close enough to an ERE superset that this isn't a terrible lie about the initial implementation either.

* Added a Signed-Off-By, since I've had a number of positive feedbacks and the only problems I've heard of (since patch v2) are the ones related to 'eval' in git-web--browse.

In v2 of the patch, I had fixed a problem with %-signs in URLs and changed the documentation example.

 Documentation/config.txt |   30 +++++++++++++++++-
 gitk-git/gitk            |   75 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 102 insertions(+), 3 deletions(-)

index ae9913b..ffc9ccf 100644
@@ -1064,6 +1064,10 @@ All gitcvs variables except for 'gitcvs.usecrlfattr' and
 is one of "ext" and "pserver") to make them apply only for the given
 access method.
 
+gitk.browser::
+	Specify the browser that will be used to open links generated by
+	'linkify' configuration options.
+
 grep.lineNumber::
 	If set to true, enable '-n' option by default.
 

@@ -1317,6 +1321,28 @@ interactive.singlekey::
 	setting is silently ignored if portable keystroke input
 	is not available.
 
+linkify.<name>.regexp::
+	Specify a regular expression in the POSIX Extended Regular Expression
+	syntax defining a class of strings to automatically convert to
+	hyperlinks.  This regular expression many not span multiple lines.
+	You must also specify 'linkify.<name>.subst'.
+
+linkify.<name>.subst::
+	Specify a substitution that results in the target URL for the
+	related regular expression.  Back-references like '\1' refer
+	to capturing groups in the associated regular expression.
+	You must also specify 'linkify.<name>.regexp'.
++
+For example, to automatically link from Debian-style "Closes: #nnnn"
+message to the Debian BTS,
++
+--------
+    git config linkify.debian-bts.regexp '#([1-9][0-9]*)'
+    git config linkify.debian-bts.subst 'http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=\1'
+--------
++
+Currently, only linkgit:gitk[1] converts strings to links in this fashion.
+
 log.abbrevCommit::
 	If true, makes linkgit:git-log[1], linkgit:git-show[1], and
 	linkgit:git-whatchanged[1] assume `\--abbrev-commit`. You may

@@ -1870,5 +1896,5 @@ user.signingkey::
 
 web.browser::
 	Specify a web browser that may be used by some commands.
-	Currently only linkgit:git-instaweb[1] and linkgit:git-help[1]
-	may use it.
+	Currently only linkgit:git-instaweb[1], linkgit:gitk[1],
+	and linkgit:git-help[1] may use it.

1.7.2.5

Hmmm, "didn't generate any discussion" does not mesh very well with "a number of people are happy". Which one should I trust?

I think it is better to be honest and say these are fed to the native regexp engine of Tcl somewhere in the documentation.

Declaring "these are POSIX EREs" invites a user to expect they truly are. When the pattern the user wrote triggers a strange Tcl extension to cause unexpected things to match, the documentation needs to help the user to understand why. I understand the longer-term wish to reuse these in gitweb and elsewhere, but it becomes even more important that it is clearly documented that these "regexp" are fed to native regexp engines of Tcl and Perl depending on the program that they are used in. Unless the documentation spells it out, the user will not be able to decide how to work the implementation around, avoiding constructs that would behave differently between Tcl and Perl.

Doesn't tcl have/use pcre these days, by the way? If we envision that this will be shared with gitweb, perhaps using that might be a better option to reduce potential confusion.

By the way, "This patch is good" does not have anything to do with signing off a patch.

Paul wanted to keep gitk sources separately available from the rest of the git. After all, that is how gitk project started. Even after 5569bf9 (Do a cross-project merge of Paul Mackerras' gitk visualizer, 2005-06-22), we kept it so that git://git.kernel.org/pub/scm/gitk/gitk was the primary project to make changes to gitk, and git.git pulled from it (it is an assymmetric pull, as gitk cannot pull from git without contaminating its history with the changes to the rest of git).

I do not know how motivated Paul is to keep gitk part separated in its own project these days. I do not think the /pub/scm/gitk/gitk repository has been re-populated yet. Assuming that it will eventually come back on-line, could you send the gitk part of this change to Paul (i.e. the diff header of your patch should read "diff --git a/gitk b/gitk") and another separate patch to Documentation/ part?

Paul, if you are orphaning gitk, I do _not_ mind start taking patches that touch gitk myself directly into git tree.

But I would still need reviewers who are motivated and interested in enhancing and maintaining gitk.

For what it's worth I've (just) tested v3 and it works well for me.