threads / rfc / 28411

RFC patchConfigurable hyperlinking in gitk

Subject: [RFC/PATCH] Configurable hyperlinking in gitk

## tl;dr

33 messages between Sep 17, 2011 and Oct 12, 2011. Diffs are folded; open one to read it.

replies: 32people: 8as markdown or json

Jeff Epler· Sep 17, 2011, 02:29 UTC · lore

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(-)
Show changes to 2 files +102 −3

Documentation/config.txt, gitk-git/gitk

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
Chris Packham· Sep 17, 2011, 09:26 UTC · re: Jeff Epler · lore

Re: [RFC/PATCH] Configurable hyperlinking in gitk

Hi,
On 17/09/11 14:29, Jeff Epler wrote:
Show 5 quoted lines
> 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).

Show 15 quoted lines
> 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.

Chris Packham· Sep 17, 2011, 10:01 UTC · re: Chris Packham · lore

Re: [RFC/PATCH] Configurable hyperlinking in gitk

On 17/09/11 21:26, Chris Packham wrote:
Show 45 quoted lines
> 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

Jeff Epler· Sep 17, 2011, 13:45 UTC · re: Chris Packham · lore

Re: [RFC/PATCH] Configurable hyperlinking in gitk

Show 6 quoted lines
> > 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(-)
Show changes to 2 files +103 −3

Documentation/config.txt, gitk-git/gitk

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
Chris Packham· Sep 17, 2011, 23:33 UTC · re: Jeff Epler · lore

Re: [RFC/PATCH] Configurable hyperlinking in gitk

On 18/09/11 01:45, Jeff Epler wrote:
Show 30 quoted lines
>>> 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.
> 
Chris Packham· Sep 18, 2011, 00:30 UTC · re: Chris Packham · lore

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

On 18/09/11 11:33, Chris Packham wrote:
Show 42 quoted lines
> 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.
Chris Packham· Sep 18, 2011, 00:32 UTC · re: Chris Packham · lore

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

On 18/09/11 12:30, Chris Packham wrote:
Show 76 quoted lines
> 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
Jeff King· Sep 18, 2011, 03:29 UTC · re: Chris Packham · lore

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

On Sun, Sep 18, 2011 at 12:32:04PM +1200, Chris Packham wrote:
Show 5 quoted lines
> 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
Chris Packham· Sep 18, 2011, 10:20 UTC · re: Jeff King · lore

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

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>
---
Show 7 quoted lines
> 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
Show changes to 2 files +45 −2

git-web--browse.sh, 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
Jeff King· Sep 18, 2011, 18:38 UTC · re: Chris Packham · lore

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

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

Show 5 quoted lines
> 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
Chris Packham· Sep 19, 2011, 09:26 UTC · re: Jeff King · lore

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

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
Show changes to 2 files +26 −5

git-web--browse.sh, 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
Jeff King· Sep 19, 2011, 18:34 UTC · re: Chris Packham · lore

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

On Mon, Sep 19, 2011 at 09:26:55PM +1200, Chris Packham wrote:
Show 7 quoted lines
> 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.

Chris Packham· Sep 20, 2011, 09:04 UTC · re: Jeff King · lore

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

On 20/09/11 06:34, Jeff King wrote:
Show 13 quoted lines
> 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?

Show 24 quoted lines
>> 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).

Jeff King· Sep 20, 2011, 18:49 UTC · re: Chris Packham · lore

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

On Tue, Sep 20, 2011 at 09:04:46PM +1200, Chris Packham wrote:
Show 8 quoted lines
> > 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
Junio C Hamano· Sep 20, 2011, 19:35 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 8 quoted lines
>> 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.

Junio C Hamano· Sep 19, 2011, 17:57 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> 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.

Show 11 quoted lines
> 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.
Jeff King· Sep 19, 2011, 18:20 UTC · re: Junio C Hamano · lore

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

On Mon, Sep 19, 2011 at 10:57:37AM -0700, Junio C Hamano wrote:
Show 10 quoted lines
> 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
Junio C Hamano· Sep 19, 2011, 20:42 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 16 quoted lines
> 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...?
Jeff King· Sep 19, 2011, 20:44 UTC · re: Junio C Hamano · lore

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

On Mon, Sep 19, 2011 at 01:42:23PM -0700, Junio C Hamano wrote:
Show 15 quoted lines
> >> 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
Junio C Hamano· Sep 19, 2011, 21:22 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 24 quoted lines
> 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?
Andreas Schwab· Sep 19, 2011, 21:46 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
> 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."
Jeff King· Sep 19, 2011, 22:23 UTC · re: Andreas Schwab · lore

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

On Mon, Sep 19, 2011 at 11:46:37PM +0200, Andreas Schwab wrote:
Show 12 quoted lines
> 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
Junio C Hamano· Sep 19, 2011, 22:28 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 10 quoted lines
> 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.
Andreas Schwab· Sep 19, 2011, 20:44 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 16 quoted lines
> 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."
Jakub Narebski· Sep 19, 2011, 21:32 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 16 quoted lines
> 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
Christian Couder· Sep 18, 2011, 14:46 UTC · re: Jeff King · lore

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

Hi,
On Sunday 18 September 2011 05:29:34 Jeff King wrote:
Show 24 quoted lines
> 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.

Marc Branchaud· Sep 19, 2011, 15:05 UTC · re: Chris Packham · lore

Re: [RFC/PATCH] Configurable hyperlinking in gitk

On 11-09-17 07:33 PM, Chris Packham wrote:
Show 16 quoted lines
> 
> 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.
Jakub Narebski· Sep 18, 2011, 18:50 UTC · re: Chris Packham · lore

Re: [RFC/PATCH] Configurable hyperlinking in gitk

Chris Packham <judge.packham@gmail.com> writes:
> On 17/09/11 14:29, Jeff Epler wrote:
Show 12 quoted lines
> > 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...
 
Show 6 quoted lines
> > 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
Jeff Epler· Sep 22, 2011, 01:31 UTC · re: Jakub Narebski · lore

Re: [RFC/PATCH] Configurable hyperlinking in gitk

On Sun, Sep 18, 2011 at 11:50:30AM -0700, Jakub Narebski wrote:
Show 8 quoted lines
> 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
Jeff Epler· Sep 22, 2011, 02:15 UTC · re: Jeff Epler · lore

[PATCH v3] Configurable hyperlinking in gitk

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(-)
Show changes to 2 files +102 −3

Documentation/config.txt, gitk-git/gitk

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
Jeff Epler· Oct 11, 2011, 18:37 UTC · re: Jeff Epler · lore

[RESEND PATCH v3] Configurable hyperlinking in gitk

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(-)
Show changes to 2 files +102 −3

Documentation/config.txt, gitk-git/gitk

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
Junio C Hamano· Oct 11, 2011, 22:13 UTC · re: Jeff Epler · lore

Re: [RESEND PATCH v3] Configurable hyperlinking in gitk

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?

Show 5 quoted lines
>  * 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.

Show 5 quoted lines
> +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).
Show 5 quoted lines
> +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.
Chris Packham· Oct 12, 2011, 09:07 UTC · re: Junio C Hamano · lore

Re: [RESEND PATCH v3] Configurable hyperlinking in gitk

On 12/10/11 11:13, Junio C Hamano wrote:
Show 8 quoted lines
> 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.

← back to recent threads