[PATCH] gitk: Fix how remote branch names with / are drawn

[PATCH] gitk: Fix how remote branch names with / are drawn

[David Holmer]

Consider this example branch:

remotes/origin/master

gitk displays this branch with different background colors for each part:
"remotes/origin" in orange and "master" in green. The idea is to make it
visually easy to read the branch name separately from the remote name.

However this fails when given this example branch:

remotes/origin/foo/bar

gitk displays this branch with "remotes/origin/foo" in orange and "bar" in
green. This makes it hard to read the branch name "foo/bar". This is due
to an inappropriately greedy regexp. This patch provides a fix so the same
branch will now be displayed with "remotes/origin" in orange and "foo/bar"
in green.

Signed-off-by: David Holmer <odinguru@gmail.com>
---
 gitk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gitk b/gitk
index 805a1c7..ca2392b 100755
--- a/gitk
+++ b/gitk
@@ -6640,7 +6640,7 @@ proc drawtags {id x xt y1} {
 	    set xl [expr {$xl - $delta/2}]
 	    $canv create polygon $x $yt $xr $yt $xr $yb $x $yb \
 		-width 1 -outline black -fill $col -tags tag.$id
-	    if {[regexp {^(remotes/.*/|remotes/)} $tag match remoteprefix]} {
+	    if {[regexp {^(remotes/[^/]*/|remotes/)} $tag match remoteprefix]} {
 	        set rwid [font measure mainfont $remoteprefix]
 		set xi [expr {$x + 1}]
 		set yti [expr {$yt + 1}]
-- 
1.9.1

Re: [PATCH] gitk: Fix how remote branch names with / are drawn

[Mike Rappazzo]

On Tue, Apr 12, 2016 at 9:59 PM, David Holmer <odinguru@gmail.com> wrote:
> Consider this example branch:
>
> remotes/origin/master
>
> gitk displays this branch with different background colors for each part:
> "remotes/origin" in orange and "master" in green. The idea is to make it
> visually easy to read the branch name separately from the remote name.
>
> However this fails when given this example branch:
>
> remotes/origin/foo/bar
>
> gitk displays this branch with "remotes/origin/foo" in orange and "bar" in
> green. This makes it hard to read the branch name "foo/bar". This is due
> to an inappropriately greedy regexp. This patch provides a fix so the same
> branch will now be displayed with "remotes/origin" in orange and "foo/bar"
> in green.
>
> Signed-off-by: David Holmer <odinguru@gmail.com>
> ---
>  gitk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gitk b/gitk
> index 805a1c7..ca2392b 100755
> --- a/gitk
> +++ b/gitk
> @@ -6640,7 +6640,7 @@ proc drawtags {id x xt y1} {
>             set xl [expr {$xl - $delta/2}]
>             $canv create polygon $x $yt $xr $yt $xr $yb $x $yb \
>                 -width 1 -outline black -fill $col -tags tag.$id
> -           if {[regexp {^(remotes/.*/|remotes/)} $tag match remoteprefix]} {
> +           if {[regexp {^(remotes/[^/]*/|remotes/)} $tag match remoteprefix]} {
>                 set rwid [font measure mainfont $remoteprefix]
>                 set xi [expr {$x + 1}]
>                 set yti [expr {$yt + 1}]
> --

This likely fixes the problem for most situations, but doesn't for a
remote with a '/' in the name.  Yet, I think this is a better state
than the present.

Is the regex `[^/]*/` more efficient than '.*?/`?  Or do you find the
former more readable?

Re: [PATCH] gitk: Fix how remote branch names with / are drawn

[David Holmer]

I agree that this switches the issue around and that a remote with a
'/' in the name would be miss colored in the same way a branch with a
'/' in the name is miss colored now. However, I would guess that
branches with '/' are MUCH MUCH more common than remotes with '/', so
like you say "this is a better state than the present". A "complete"
solution would take iterating through the list of remotes and matching
the explicit whole pattern (e.g. match
"remotes/my/remote/with/slashes/" for remote "my/remote/with/slashes")
but I doubt that is worth it for 99.9% of people.

The alternative regex that you are asking about is either using some
syntax I am not familiar with or isn't quite correct. I'm most
familiar with grep command line format, so perhaps tcl regex is
different.

The original code does the equivalent of this:

~$ echo "remotes/origin/dev/test1" | grep -o "remotes/.*/"
remotes/origin/dev/

The issue is that the '.*/' part is greedy in that it will match all
the way up to and including the last /

My solution was to change the . to [^/] which means "any character but
/". This stops the match at the first / after the remote name starts:

~$ echo "remotes/origin/dev/test1" | grep -o "remotes/[^/]*/"
remotes/origin/

The alternative you suggested with '.*?/' doesn't seem to work with grep:

~$ echo "remotes/origin/dev/test1" | grep -o "remotes/.*?/"
(no output, i.e. does not match)


Thank you.

On Wed, Apr 13, 2016 at 7:35 AM, Mike Rappazzo <rappazzo@gmail.com> wrote:
> On Tue, Apr 12, 2016 at 9:59 PM, David Holmer <odinguru@gmail.com> wrote:
>> Consider this example branch:
>>
>> remotes/origin/master
>>
>> gitk displays this branch with different background colors for each part:
>> "remotes/origin" in orange and "master" in green. The idea is to make it
>> visually easy to read the branch name separately from the remote name.
>>
>> However this fails when given this example branch:
>>
>> remotes/origin/foo/bar
>>
>> gitk displays this branch with "remotes/origin/foo" in orange and "bar" in
>> green. This makes it hard to read the branch name "foo/bar". This is due
>> to an inappropriately greedy regexp. This patch provides a fix so the same
>> branch will now be displayed with "remotes/origin" in orange and "foo/bar"
>> in green.
>>
>> Signed-off-by: David Holmer <odinguru@gmail.com>
>> ---
>>  gitk | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gitk b/gitk
>> index 805a1c7..ca2392b 100755
>> --- a/gitk
>> +++ b/gitk
>> @@ -6640,7 +6640,7 @@ proc drawtags {id x xt y1} {
>>             set xl [expr {$xl - $delta/2}]
>>             $canv create polygon $x $yt $xr $yt $xr $yb $x $yb \
>>                 -width 1 -outline black -fill $col -tags tag.$id
>> -           if {[regexp {^(remotes/.*/|remotes/)} $tag match remoteprefix]} {
>> +           if {[regexp {^(remotes/[^/]*/|remotes/)} $tag match remoteprefix]} {
>>                 set rwid [font measure mainfont $remoteprefix]
>>                 set xi [expr {$x + 1}]
>>                 set yti [expr {$yt + 1}]
>> --
>
> This likely fixes the problem for most situations, but doesn't for a
> remote with a '/' in the name.  Yet, I think this is a better state
> than the present.
>
> Is the regex `[^/]*/` more efficient than '.*?/`?  Or do you find the
> former more readable?

Re: [PATCH] gitk: Fix how remote branch names with / are drawn

[Mike Rappazzo]

On Wed, Apr 13, 2016 at 2:19 PM, David Holmer <odinguru@gmail.com> wrote:
> I agree that this switches the issue around and that a remote with a
> '/' in the name would be miss colored in the same way a branch with a
> '/' in the name is miss colored now. However, I would guess that
> branches with '/' are MUCH MUCH more common than remotes with '/', so
> like you say "this is a better state than the present". A "complete"
> solution would take iterating through the list of remotes and matching
> the explicit whole pattern (e.g. match
> "remotes/my/remote/with/slashes/" for remote "my/remote/with/slashes")
> but I doubt that is worth it for 99.9% of people.
>
> The alternative regex that you are asking about is either using some
> syntax I am not familiar with or isn't quite correct. I'm most
> familiar with grep command line format, so perhaps tcl regex is
> different.
>
> The original code does the equivalent of this:
>
> ~$ echo "remotes/origin/dev/test1" | grep -o "remotes/.*/"
> remotes/origin/dev/
>
> The issue is that the '.*/' part is greedy in that it will match all
> the way up to and including the last /
>
> My solution was to change the . to [^/] which means "any character but
> /". This stops the match at the first / after the remote name starts:
>
> ~$ echo "remotes/origin/dev/test1" | grep -o "remotes/[^/]*/"
> remotes/origin/
>
> The alternative you suggested with '.*?/' doesn't seem to work with grep:
>
> ~$ echo "remotes/origin/dev/test1" | grep -o "remotes/.*?/"
> (no output, i.e. does not match)

`.*?` is a lazy match. I think it is an extended-regex, and your
version is probably more efficient anyway.
echo "remotes/origin/dev/test1" | grep -Eo "remotes/.*?/"

>
>
> Thank you.
>

(Most people on this list don't like "top posting"), please try to
reply inline instead.


> On Wed, Apr 13, 2016 at 7:35 AM, Mike Rappazzo <rappazzo@gmail.com> wrote:
>> On Tue, Apr 12, 2016 at 9:59 PM, David Holmer <odinguru@gmail.com> wrote:
>>> Consider this example branch:
>>>
>>> remotes/origin/master
>>>
>>> gitk displays this branch with different background colors for each part:
>>> "remotes/origin" in orange and "master" in green. The idea is to make it
>>> visually easy to read the branch name separately from the remote name.
>>>
>>> However this fails when given this example branch:
>>>
>>> remotes/origin/foo/bar
>>>
>>> gitk displays this branch with "remotes/origin/foo" in orange and "bar" in
>>> green. This makes it hard to read the branch name "foo/bar". This is due
>>> to an inappropriately greedy regexp. This patch provides a fix so the same
>>> branch will now be displayed with "remotes/origin" in orange and "foo/bar"
>>> in green.
>>>
>>> Signed-off-by: David Holmer <odinguru@gmail.com>
>>> ---
>>>  gitk | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/gitk b/gitk
>>> index 805a1c7..ca2392b 100755
>>> --- a/gitk
>>> +++ b/gitk
>>> @@ -6640,7 +6640,7 @@ proc drawtags {id x xt y1} {
>>>             set xl [expr {$xl - $delta/2}]
>>>             $canv create polygon $x $yt $xr $yt $xr $yb $x $yb \
>>>                 -width 1 -outline black -fill $col -tags tag.$id
>>> -           if {[regexp {^(remotes/.*/|remotes/)} $tag match remoteprefix]} {
>>> +           if {[regexp {^(remotes/[^/]*/|remotes/)} $tag match remoteprefix]} {
>>>                 set rwid [font measure mainfont $remoteprefix]
>>>                 set xi [expr {$x + 1}]
>>>                 set yti [expr {$yt + 1}]
>>> --
>>
>> This likely fixes the problem for most situations, but doesn't for a
>> remote with a '/' in the name.  Yet, I think this is a better state
>> than the present.
>>
>> Is the regex `[^/]*/` more efficient than '.*?/`?  Or do you find the
>> former more readable?