[RFC/PATCH] Configurable hyperlinking in gitk

[RFC/PATCH] Configurable hyperlinking in gitk

[Jeff Epler]

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(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 750c86d..67ed436 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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.
diff --git a/gitk-git/gitk b/gitk-git/gitk
index 4cde0c4..a21eea1 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -6684,7 +6684,7 @@ proc commit_descriptor {p} {
 # append some text to the ctext widget, and make any SHA1 ID
 # that we know about be a clickable link.
 proc appendwithlinks {text tags} {
-    global ctext linknum curview
+    global ctext linknum curview linkmakers
 
     set start [$ctext index "end - 1c"]
     $ctext insert end $text $tags
@@ -6699,6 +6699,30 @@ proc appendwithlinks {text tags} {
 	setlink $linkid link$linknum
 	incr linknum
     }
+
+    if {$linkmakers == {}} return
+
+    set link_re {}
+    foreach {re rep} $linkmakers { lappend link_re $re }
+    set link_re "([join $link_re {)|(}])"
+
+    set ee 0
+    while {[regexp -indices -start $ee -- $link_re $text l]} {
+	set s [lindex $l 0]
+	set e [lindex $l 1]
+	set linktext [string range $text $s $e]
+	incr e
+	set ee $e
+
+	foreach {re rep} $linkmakers {
+	    if {![regsub $re $linktext $rep linkurl]} continue
+	    $ctext tag delete link$linknum
+	    $ctext tag add link$linknum "$start + $s c" "$start + $e c"
+	    seturllink $linkurl link$linknum
+	    incr linknum
+	    break
+	}
+    }
 }
 
 proc setlink {id lk} {
@@ -6726,6 +6750,52 @@ proc setlink {id lk} {
     }
 }
 
+proc get_link_config {} {
+    if {[catch {exec git config -z --get-regexp {^gitk\.linkify\.}} linkers]} {
+	return {}
+    }
+
+    set linktypes [list]
+    foreach item [split $linkers "\0"] {
+	if {$item == ""} continue
+	if {![regexp {gitk\.linkify\.(\S+)\.(re|sub)\s(.*)} $item _ k t v]} {
+	    continue
+	}
+	set linkconfig($t,$k) $v
+	if {$t == "re"} { lappend linktypes $k }
+    }
+
+    set linkmakers [list]
+    foreach k $linktypes {
+	if {![info exists linkconfig(sub,$k)]} {
+	    puts stderr "Warning: link `$k' is missing a substitution string"
+	} elseif {[catch {regexp -inline -- $linkconfig(re,$k) ""} err]} {
+	    puts stderr "Warning: link `$k': $err"
+	} else {
+	    lappend linkmakers $linkconfig(re,$k) $linkconfig(sub,$k)
+	}
+	unset linkconfig(re,$k)
+	unset -nocomplain linkconfig(sub,$k)
+    }
+    foreach k [array names linkconfig] {
+	regexp "sub,(.*)" $k _ k
+	puts stderr "Warning: link `$k' is missing a regular expression"
+    }
+    set linkmakers
+}
+
+proc openlink {url} {
+    exec git web--browse --config=gitk.browser $url &
+}
+
+proc seturllink {url lk} {
+    global ctext
+    $ctext tag conf $lk -foreground blue -underline 1
+    $ctext tag bind $lk <1> [list openlink $url]
+    $ctext tag bind $lk <Enter> {linkcursor %W 1}
+    $ctext tag bind $lk <Leave> {linkcursor %W -1}
+}
+
 proc appendshortlink {id {pre {}} {post {}}} {
     global ctext linknum
 
@@ -11693,6 +11763,8 @@ if {[tk windowingsystem] eq "win32"} {
     focus -force .
 }
 
+set linkmakers [get_link_config]
+
 getcommits {}
 
 # Local variables:
-- 
1.7.0.4

Re: [RFC/PATCH] Configurable hyperlinking in gitk

[Chris Packham]

Hi,

On 17/09/11 14:29, Jeff Epler wrote:
> 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)

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

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

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.

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

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.

> Suggestions?  Problems?  Successes?

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.

Re: [RFC/PATCH] Configurable hyperlinking in gitk

[Chris Packham]

On 17/09/11 21:26, Chris Packham wrote:
> Hi,
> 
> On 17/09/11 14:29, Jeff Epler wrote:
>> 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)
> 
> 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).
> 
>> 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.
> 
> 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.
> 
>> There are probably better names for the configuration options, too.
> 
> 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.

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

>> Suggestions?  Problems?  Successes?
> 
> 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.

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

Re: [RFC/PATCH] Configurable hyperlinking in gitk

[Jeff Epler]

> > There are probably better names for the configuration options, too.
> 
> 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.

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.

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

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(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ae9913b..13e8aa6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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.
diff --git a/gitk-git/gitk b/gitk-git/gitk
index 4cde0c4..5532869 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -6684,7 +6684,7 @@ proc commit_descriptor {p} {
 # append some text to the ctext widget, and make any SHA1 ID
 # that we know about be a clickable link.
 proc appendwithlinks {text tags} {
-    global ctext linknum curview
+    global ctext linknum curview linkmakers
 
     set start [$ctext index "end - 1c"]
     $ctext insert end $text $tags
@@ -6699,6 +6699,30 @@ proc appendwithlinks {text tags} {
 	setlink $linkid link$linknum
 	incr linknum
     }
+
+    if {$linkmakers == {}} return
+
+    set link_re {}
+    foreach {re rep} $linkmakers { lappend link_re $re }
+    set link_re "([join $link_re {)|(}])"
+
+    set ee 0
+    while {[regexp -indices -start $ee -- $link_re $text l]} {
+	set s [lindex $l 0]
+	set e [lindex $l 1]
+	set linktext [string range $text $s $e]
+	incr e
+	set ee $e
+
+	foreach {re rep} $linkmakers {
+	    if {![regsub $re $linktext $rep linkurl]} continue
+	    $ctext tag delete link$linknum
+	    $ctext tag add link$linknum "$start + $s c" "$start + $e c"
+	    seturllink $linkurl link$linknum
+	    incr linknum
+	    break
+	}
+    }
 }
 
 proc setlink {id lk} {
@@ -6726,6 +6750,53 @@ proc setlink {id lk} {
     }
 }
 
+proc get_link_config {} {
+    if {[catch {exec git config -z --get-regexp {^gitk\.linkify\.}} linkers]} {
+	return {}
+    }
+
+    set linktypes [list]
+    foreach item [split $linkers "\0"] {
+	if {$item == ""} continue
+	if {![regexp {gitk\.linkify\.(\S+)\.(re|sub)\s(.*)} $item _ k t v]} {
+	    continue
+	}
+	set linkconfig($t,$k) $v
+	if {$t == "re"} { lappend linktypes $k }
+    }
+
+    set linkmakers [list]
+    foreach k $linktypes {
+	if {![info exists linkconfig(sub,$k)]} {
+	    puts stderr "Warning: link `$k' is missing a substitution string"
+	} elseif {[catch {regexp -inline -- $linkconfig(re,$k) ""} err]} {
+	    puts stderr "Warning: link `$k': $err"
+	} else {
+	    lappend linkmakers $linkconfig(re,$k) $linkconfig(sub,$k)
+	}
+	unset linkconfig(re,$k)
+	unset -nocomplain linkconfig(sub,$k)
+    }
+    foreach k [array names linkconfig] {
+	regexp "sub,(.*)" $k _ k
+	puts stderr "Warning: link `$k' is missing a regular expression"
+    }
+    set linkmakers
+}
+
+proc openlink {url} {
+    exec git web--browse --config=gitk.browser $url &
+}
+
+proc seturllink {url lk} {
+    set qurl [string map {% %%} $url]
+    global ctext
+    $ctext tag conf $lk -foreground blue -underline 1
+    $ctext tag bind $lk <1> [list openlink $qurl]
+    $ctext tag bind $lk <Enter> {linkcursor %W 1}
+    $ctext tag bind $lk <Leave> {linkcursor %W -1}
+}
+
 proc appendshortlink {id {pre {}} {post {}}} {
     global ctext linknum
 
@@ -11693,6 +11764,8 @@ if {[tk windowingsystem] eq "win32"} {
     focus -force .
 }
 
+set linkmakers [get_link_config]
+
 getcommits {}
 
 # Local variables:
-- 
1.7.2.5

Re: [RFC/PATCH] Configurable hyperlinking in gitk

[Chris Packham]

On 18/09/11 01:45, Jeff Epler wrote:
>>> There are probably better names for the configuration options, too.
>>
>> 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.
> 
> 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.
> 
>> 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.
> 
> 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'

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'

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

git web--browse error handling URL with & in it (Was Re: [RFC/PATCH] Configurable hyperlinking in gitk)

[Chris Packham]

On 18/09/11 11:33, Chris Packham wrote:
> On 18/09/11 01:45, Jeff Epler wrote:
>>>> There are probably better names for the configuration options, too.
>>>
>>> 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.
>>
>> 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.
>>
>>> 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.
>>
>> 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'
> 
> 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?

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.

Re: git web--browse error handling URL with & in it (Was Re: [RFC/PATCH] Configurable hyperlinking in gitk)

[Chris Packham]

On 18/09/11 12:30, Chris Packham wrote:
> On 18/09/11 11:33, Chris Packham wrote:
>> On 18/09/11 01:45, Jeff Epler wrote:
>>>>> There are probably better names for the configuration options, too.
>>>>
>>>> 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.
>>>
>>> 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.
>>>
>>>> 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.
>>>
>>> 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'
>>
>> 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?
> 
> 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

Re: git web--browse error handling URL with & in it (Was Re: [RFC/PATCH] Configurable hyperlinking in gitk)

[Jeff King]

On Sun, Sep 18, 2011 at 12:32:04PM +1200, Chris Packham wrote:

> 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

[PATCH] git-web--browse: invoke kfmclient directly

[Chris Packham]

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

Cc: peff@peff.net
Cc: chriscool@tuxfamily.org
Cc: jepler@unpythonic.net
Signed-off-by: Chris Packham <judge.packham@gmail.com>
---
> 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.

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

diff --git a/git-web--browse.sh b/git-web--browse.sh
index e9de241..1164a22 100755
--- a/git-web--browse.sh
+++ b/git-web--browse.sh
@@ -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" "$@" &
diff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh
new file mode 100755
index 0000000..7ed38a0
--- /dev/null
+++ b/t/t9901-git-web--browse.sh
@@ -0,0 +1,43 @@
+#!/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 with
+the supported browsers that are installed on the host system.'
+
+. ./test-lib.sh
+
+test -x /usr/bin/firefox && test_set_prereq FIREFOX
+test -x /usr/bin/konqueror && test_set_prereq KONQUEROR
+test -x /usr/bin/google-chrome && test_set_prereq CHROME
+test -x /usr/bin/opera && test_set_prereq OPERA
+
+test_expect_success \
+	'accepts a URL with an ampersand in it (default)' '
+    git web--browse http://example.com/foo\&bar/
+'
+
+test_expect_success FIREFOX \
+	'accepts a URL with an ampersand in it (firefox)' '
+    git web--browse --browser=firefox http://example.com/foo\&bar/
+'
+
+test_expect_success KONQUEROR \
+	'accepts a URL with an ampersand in it (konqueror)' '
+    git web--browse --browser=konqueror http://example.com/foo\&bar/
+'
+
+test_expect_success OPERA \
+	'accepts a URL with an ampersand in it (opera)' '
+    git web--browse --browser=opera http://example.com/foo\&bar/
+'
+
+test_expect_success CHROME \
+	'accepts a URL with an ampersand in it (chrome)' '
+    git web--browse --browser=google-chrome http://example.com/foo\&bar/
+'
+
+test_done
-- 
1.7.7.rc1.3.g5593.dirty

Re: git web--browse error handling URL with & in it (Was Re: [RFC/PATCH] Configurable hyperlinking in gitk)

[Christian Couder]

Hi,

On Sunday 18 September 2011 05:29:34 Jeff King wrote:
> On Sun, Sep 18, 2011 at 12:32:04PM +1200, Chris Packham wrote:
> > 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.

Yeah, I don't remember why I sometimes used 'eval "$browser_path" "$@"' when I 
wrote this code. Sorry!
 
> Unrelated, but it also looks like $browser_path is used unquoted in the
> firefox case (see inside the vers=$(...)).

Thanks,
Christian.

Re: [PATCH] git-web--browse: invoke kfmclient directly

[Jeff King]

On Sun, Sep 18, 2011 at 10:20:24PM +1200, Chris Packham wrote:

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

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

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

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

Re: [RFC/PATCH] Configurable hyperlinking in gitk

[Jakub Narebski]

Chris Packham <judge.packham@gmail.com> writes:
> On 17/09/11 14:29, Jeff Epler wrote:

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

That would be nice.  So called "committags" support was long planned
for gitweb, and even some preliminary work exists...
 
> > There are probably better names for the configuration options, too.
> 
> 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.

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

[RFC/PATCHv2] git-web--browse: avoid the use of eval

[Chris Packham]

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

Cc: peff@peff.net
Cc: chriscool@tuxfamily.org
Cc: jepler@unpythonic.net

Signed-off-by: Chris Packham <judge.packham@gmail.com>

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

diff --git a/git-web--browse.sh b/git-web--browse.sh
index e9de241..ee05f10 100755
--- a/git-web--browse.sh
+++ b/git-web--browse.sh
@@ -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
diff --git a/t/t9901-git-web--browse.sh b/t/t9901-git-web--browse.sh
new file mode 100755
index 0000000..141ed17
--- /dev/null
+++ b/t/t9901-git-web--browse.sh
@@ -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

Re: [RFC/PATCH] Configurable hyperlinking in gitk

[Marc Branchaud]

On 11-09-17 07:33 PM, Chris Packham wrote:
> 
> 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'

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.

Re: [PATCH] git-web--browse: invoke kfmclient directly

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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 \"\$@\""

Yeah, I agree, and the dq around $browser_cmd is kind of important, too,
for that to work and be readable.
> 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.

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

Thanks.

Re: [PATCH] git-web--browse: invoke kfmclient directly

[Jeff King]

On Mon, Sep 19, 2011 at 10:57:37AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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 \"\$@\""
> 
> Yeah, I agree, and the dq around $browser_cmd is kind of important, too,
> for that to work and be readable.

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

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

-Peff

Re: [RFC/PATCHv2] git-web--browse: avoid the use of eval

[Jeff King]

On Mon, Sep 19, 2011 at 09:26:55PM +1200, Chris Packham wrote:

> Using eval causes problems when the URL contains an appropriately
> escaped ampersand (\&). Dropping eval from the built-in browser
> invocation avoids the problem.
> 
> Cc: peff@peff.net
> Cc: chriscool@tuxfamily.org
> Cc: jepler@unpythonic.net

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.

> I've replaced my tests With the test suggested by Peff (should I be
> giving him credit in the copyright line or something?).

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.

Re: [PATCH] git-web--browse: invoke kfmclient directly

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Sep 19, 2011 at 10:57:37AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > 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 \"\$@\""
>> 
>> Yeah, I agree, and the dq around $browser_cmd is kind of important, too,
>> for that to work and be readable.
>
> Oops, good catch. Probably the most readable version would be:
>
>   eval "\"$browser_cmd\"" '"$@"'

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

Re: [PATCH] git-web--browse: invoke kfmclient directly

[Andreas Schwab]

Jeff King <peff@peff.net> writes:

> On Mon, Sep 19, 2011 at 10:57:37AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > 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 \"\$@\""
>> 
>> Yeah, I agree, and the dq around $browser_cmd is kind of important, too,
>> for that to work and be readable.
>
> Oops, good catch. Probably the most readable version would be:
>
>   eval "\"$browser_cmd\"" '"$@"'

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, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] git-web--browse: invoke kfmclient directly

[Jeff King]

On Mon, Sep 19, 2011 at 01:42:23PM -0700, Junio C Hamano wrote:

> >> Yeah, I agree, and the dq around $browser_cmd is kind of important, too,
> >> for that to work and be readable.
> >
> > Oops, good catch. Probably the most readable version would be:
> >
> >   eval "\"$browser_cmd\"" '"$@"'
> 
> 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

Re: [PATCH] git-web--browse: invoke kfmclient directly

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Sep 19, 2011 at 01:42:23PM -0700, Junio C Hamano wrote:
>
>> >> Yeah, I agree, and the dq around $browser_cmd is kind of important, too,
>> >> for that to work and be readable.
>> >
>> > Oops, good catch. Probably the most readable version would be:
>> >
>> >   eval "\"$browser_cmd\"" '"$@"'
>> 
>> 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.

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?

Re: [PATCH] git-web--browse: invoke kfmclient directly

[Jakub Narebski]

Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> Hmm, isn't '&' somewhat an unusual in URL? ...ah, not really, if it is in
> the query parameter part it is quite common.

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

Re: [PATCH] git-web--browse: invoke kfmclient directly

[Andreas Schwab]

Junio C Hamano <gitster@pobox.com> writes:

> 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, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] git-web--browse: invoke kfmclient directly

[Jeff King]

On Mon, Sep 19, 2011 at 11:46:37PM +0200, Andreas Schwab wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > 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 "$@"

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

Re: [PATCH] git-web--browse: invoke kfmclient directly

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

Re: [RFC/PATCHv2] git-web--browse: avoid the use of eval

[Chris Packham]

On 20/09/11 06:34, Jeff King wrote:
> On Mon, Sep 19, 2011 at 09:26:55PM +1200, Chris Packham wrote:
> 
>> Using eval causes problems when the URL contains an appropriately
>> escaped ampersand (\&). Dropping eval from the built-in browser
>> invocation avoids the problem.
>>
>> Cc: peff@peff.net
>> Cc: chriscool@tuxfamily.org
>> Cc: jepler@unpythonic.net
> 
> 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.

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?

>> I've replaced my tests With the test suggested by Peff (should I be
>> giving him credit in the copyright line or something?).
> 
> 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.

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

Re: [RFC/PATCHv2] git-web--browse: avoid the use of eval

[Jeff King]

On Tue, Sep 20, 2011 at 09:04:46PM +1200, Chris Packham wrote:

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

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

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

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

Re: [RFC/PATCHv2] git-web--browse: avoid the use of eval

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.

Re: [RFC/PATCH] Configurable hyperlinking in gitk

[Jeff Epler]

On Sun, Sep 18, 2011 at 11:50:30AM -0700, Jakub Narebski wrote:
> 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?

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

[PATCH v3] Configurable hyperlinking in gitk

[Jeff Epler]

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.

Signed-off-by: Jeff Epler <jepler@unpythonic.net>
---
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(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ae9913b..ffc9ccf 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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.
diff --git a/gitk-git/gitk b/gitk-git/gitk
index 4cde0c4..9db5525 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -6684,7 +6684,7 @@ proc commit_descriptor {p} {
 # append some text to the ctext widget, and make any SHA1 ID
 # that we know about be a clickable link.
 proc appendwithlinks {text tags} {
-    global ctext linknum curview
+    global ctext linknum curview linkmakers
 
     set start [$ctext index "end - 1c"]
     $ctext insert end $text $tags
@@ -6699,6 +6699,30 @@ proc appendwithlinks {text tags} {
 	setlink $linkid link$linknum
 	incr linknum
     }
+
+    if {$linkmakers == {}} return
+
+    set link_re {}
+    foreach {re rep} $linkmakers { lappend link_re $re }
+    set link_re "([join $link_re {)|(}])"
+
+    set ee 0
+    while {[regexp -indices -start $ee -- $link_re $text l]} {
+	set s [lindex $l 0]
+	set e [lindex $l 1]
+	set linktext [string range $text $s $e]
+	incr e
+	set ee $e
+
+	foreach {re rep} $linkmakers {
+	    if {![regsub $re $linktext $rep linkurl]} continue
+	    $ctext tag delete link$linknum
+	    $ctext tag add link$linknum "$start + $s c" "$start + $e c"
+	    seturllink $linkurl link$linknum
+	    incr linknum
+	    break
+	}
+    }
 }
 
 proc setlink {id lk} {
@@ -6726,6 +6750,53 @@ proc setlink {id lk} {
     }
 }
 
+proc get_link_config {} {
+    if {[catch {exec git config -z --get-regexp {^linkify\.}} linkers]} {
+	return {}
+    }
+
+    set linktypes [list]
+    foreach item [split $linkers "\0"] {
+	if {$item == ""} continue
+	if {![regexp {linkify\.(\S+)\.(regexp|subst)\s(.*)} $item _ k t v]} {
+	    continue
+	}
+	set linkconfig($t,$k) $v
+	if {$t == "regexp"} { lappend linktypes $k }
+    }
+
+    set linkmakers [list]
+    foreach k $linktypes {
+	if {![info exists linkconfig(subst,$k)]} {
+	    puts stderr "Warning: link `$k' is missing a substitution string"
+	} elseif {[catch {regexp -inline -- $linkconfig(regexp,$k) ""} err]} {
+	    puts stderr "Warning: link `$k': $err"
+	} else {
+	    lappend linkmakers $linkconfig(regexp,$k) $linkconfig(subst,$k)
+	}
+	unset linkconfig(regexp,$k)
+	unset -nocomplain linkconfig(subst,$k)
+    }
+    foreach k [array names linkconfig] {
+	regexp "subst,(.*)" $k _ k
+	puts stderr "Warning: link `$k' is missing a regular expression"
+    }
+    set linkmakers
+}
+
+proc openlink {url} {
+    exec git web--browse --config=gitk.browser $url &
+}
+
+proc seturllink {url lk} {
+    set qurl [string map {% %%} $url]
+    global ctext
+    $ctext tag conf $lk -foreground blue -underline 1
+    $ctext tag bind $lk <1> [list openlink $qurl]
+    $ctext tag bind $lk <Enter> {linkcursor %W 1}
+    $ctext tag bind $lk <Leave> {linkcursor %W -1}
+}
+
 proc appendshortlink {id {pre {}} {post {}}} {
     global ctext linknum
 
@@ -11693,6 +11764,8 @@ if {[tk windowingsystem] eq "win32"} {
     focus -force .
 }
 
+set linkmakers [get_link_config]
+
 getcommits {}
 
 # Local variables:
-- 
1.7.2.5

[RESEND PATCH v3] Configurable hyperlinking in gitk

[Jeff Epler]

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.

Signed-off-by: Jeff Epler <jepler@unpythonic.net>
---
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(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ae9913b..ffc9ccf 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -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.
diff --git a/gitk-git/gitk b/gitk-git/gitk
index 4cde0c4..9db5525 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -6684,7 +6684,7 @@ proc commit_descriptor {p} {
 # append some text to the ctext widget, and make any SHA1 ID
 # that we know about be a clickable link.
 proc appendwithlinks {text tags} {
-    global ctext linknum curview
+    global ctext linknum curview linkmakers
 
     set start [$ctext index "end - 1c"]
     $ctext insert end $text $tags
@@ -6699,6 +6699,30 @@ proc appendwithlinks {text tags} {
 	setlink $linkid link$linknum
 	incr linknum
     }
+
+    if {$linkmakers == {}} return
+
+    set link_re {}
+    foreach {re rep} $linkmakers { lappend link_re $re }
+    set link_re "([join $link_re {)|(}])"
+
+    set ee 0
+    while {[regexp -indices -start $ee -- $link_re $text l]} {
+	set s [lindex $l 0]
+	set e [lindex $l 1]
+	set linktext [string range $text $s $e]
+	incr e
+	set ee $e
+
+	foreach {re rep} $linkmakers {
+	    if {![regsub $re $linktext $rep linkurl]} continue
+	    $ctext tag delete link$linknum
+	    $ctext tag add link$linknum "$start + $s c" "$start + $e c"
+	    seturllink $linkurl link$linknum
+	    incr linknum
+	    break
+	}
+    }
 }
 
 proc setlink {id lk} {
@@ -6726,6 +6750,53 @@ proc setlink {id lk} {
     }
 }
 
+proc get_link_config {} {
+    if {[catch {exec git config -z --get-regexp {^linkify\.}} linkers]} {
+	return {}
+    }
+
+    set linktypes [list]
+    foreach item [split $linkers "\0"] {
+	if {$item == ""} continue
+	if {![regexp {linkify\.(\S+)\.(regexp|subst)\s(.*)} $item _ k t v]} {
+	    continue
+	}
+	set linkconfig($t,$k) $v
+	if {$t == "regexp"} { lappend linktypes $k }
+    }
+
+    set linkmakers [list]
+    foreach k $linktypes {
+	if {![info exists linkconfig(subst,$k)]} {
+	    puts stderr "Warning: link `$k' is missing a substitution string"
+	} elseif {[catch {regexp -inline -- $linkconfig(regexp,$k) ""} err]} {
+	    puts stderr "Warning: link `$k': $err"
+	} else {
+	    lappend linkmakers $linkconfig(regexp,$k) $linkconfig(subst,$k)
+	}
+	unset linkconfig(regexp,$k)
+	unset -nocomplain linkconfig(subst,$k)
+    }
+    foreach k [array names linkconfig] {
+	regexp "subst,(.*)" $k _ k
+	puts stderr "Warning: link `$k' is missing a regular expression"
+    }
+    set linkmakers
+}
+
+proc openlink {url} {
+    exec git web--browse --config=gitk.browser $url &
+}
+
+proc seturllink {url lk} {
+    set qurl [string map {% %%} $url]
+    global ctext
+    $ctext tag conf $lk -foreground blue -underline 1
+    $ctext tag bind $lk <1> [list openlink $qurl]
+    $ctext tag bind $lk <Enter> {linkcursor %W 1}
+    $ctext tag bind $lk <Leave> {linkcursor %W -1}
+}
+
 proc appendshortlink {id {pre {}} {post {}}} {
     global ctext linknum
 
@@ -11693,6 +11764,8 @@ if {[tk windowingsystem] eq "win32"} {
     focus -force .
 }
 
+set linkmakers [get_link_config]
+
 getcommits {}
 
 # Local variables:
-- 
1.7.2.5

Re: [RESEND PATCH v3] Configurable hyperlinking in gitk

[Junio C Hamano]

Jeff Epler <jepler@unpythonic.net> writes:

> I'm aware of no problems with this patch, and a number of people have
> commented that it is useful to them.

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

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

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.

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

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.

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

Saying "You must ..." without explicitly saying "why" is a bad style. If
the reader already _knows_ the .regexp is used to supply captured
substring to the corresponding .subst, then it is obvious that whenever
you have .regexp you need a matching .subst, but that is not even
explained here.

How about this?

	A string that matches this regexp is converted to a hyperlink
	using the value of corresponding `linkify.<name>.subst` variable.
	The regular expression is passed to the regexp engine of Tcl (in
	gitk) or Perl (in gitweb).

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

Likewise.

	A string matched the value of the corresponding
	`linkify.<name>.regexp` variable is rewritten to this URL. The
	value of this variable can contain back-references like `\1` to
	refer to capturing groups in the associated regular expression.

Re: [RESEND PATCH v3] Configurable hyperlinking in gitk

[Chris Packham]

On 12/10/11 11:13, Junio C Hamano wrote:
> Jeff Epler <jepler@unpythonic.net> writes:
> 
>> I'm aware of no problems with this patch, and a number of people have
>> commented that it is useful to them.
> 
> Hmmm, "didn't generate any discussion" does not mesh very well with "a
> number of people are happy". Which one should I trust?
> 

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