{"thread":{"id":"42012","subject":"[PATCH] gitk: Fix how remote branch names with / are drawn","startedAt":"2016-04-13T01:59:03Z","lastAt":"2016-04-13T18:28:39Z","messageCount":4,"participants":["David Holmer","Mike Rappazzo"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"283289","messageId":"1460512743-27100-1-git-send-email-odinguru@gmail.com","threadId":"42012","inReplyTo":null,"subject":"[PATCH] gitk: Fix how remote branch names with / are drawn","fromName":"David Holmer","fromEmail":"odinguru@gmail.com","sentAt":"2016-04-13T01:59:03Z","receivedAt":"2016-04-13T01:59:03Z","isPatch":true,"sender":{"key":"odinguru@gmail.com","avatar":null},"body":"Consider this example branch:\n\nremotes/origin/master\n\ngitk displays this branch with different background colors for each part:\n\"remotes/origin\" in orange and \"master\" in green. The idea is to make it\nvisually easy to read the branch name separately from the remote name.\n\nHowever this fails when given this example branch:\n\nremotes/origin/foo/bar\n\ngitk displays this branch with \"remotes/origin/foo\" in orange and \"bar\" in\ngreen. This makes it hard to read the branch name \"foo/bar\". This is due\nto an inappropriately greedy regexp. This patch provides a fix so the same\nbranch will now be displayed with \"remotes/origin\" in orange and \"foo/bar\"\nin green.\n\nSigned-off-by: David Holmer \n---\n gitk | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/gitk b/gitk\nindex 805a1c7..ca2392b 100755\n--- a/gitk\n+++ b/gitk\n@@ -6640,7 +6640,7 @@ proc drawtags {id x xt y1} {\n \t set xl [expr {$xl - $delta/2}]\n \t $canv create polygon $x $yt $xr $yt $xr $yb $x $yb \\\n \t\t-width 1 -outline black -fill $col -tags tag.$id\n-\t if {[regexp {^(remotes/.*/|remotes/)} $tag match remoteprefix]} {\n+\t if {[regexp {^(remotes/[^/]*/|remotes/)} $tag match remoteprefix]} {\n \t set rwid [font measure mainfont $remoteprefix]\n \t\tset xi [expr {$x + 1}]\n \t\tset yti [expr {$yt + 1}]\n-- \n1.9.1\n"},{"id":"283313","messageId":"CANoM8SXixymz3=NQWgG5vSo7XDNh18_OzrNXU4+Y8CQ0LkB6sw@mail.gmail.com","threadId":"42012","inReplyTo":"1460512743-27100-1-git-send-email-odinguru@gmail.com","subject":"Re: [PATCH] gitk: Fix how remote branch names with / are drawn","fromName":"Mike Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2016-04-13T11:35:58Z","receivedAt":"2016-04-13T11:35:58Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"On Tue, Apr 12, 2016 at 9:59 PM, David Holmer wrote:\n> Consider this example branch:\n>\n> remotes/origin/master\n>\n> gitk displays this branch with different background colors for each part:\n> \"remotes/origin\" in orange and \"master\" in green. The idea is to make it\n> visually easy to read the branch name separately from the remote name.\n>\n> However this fails when given this example branch:\n>\n> remotes/origin/foo/bar\n>\n> gitk displays this branch with \"remotes/origin/foo\" in orange and \"bar\" in\n> green. This makes it hard to read the branch name \"foo/bar\". This is due\n> to an inappropriately greedy regexp. This patch provides a fix so the same\n> branch will now be displayed with \"remotes/origin\" in orange and \"foo/bar\"\n> in green.\n>\n> Signed-off-by: David Holmer \n> ---\n> gitk | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/gitk b/gitk\n> index 805a1c7..ca2392b 100755\n> --- a/gitk\n> +++ b/gitk\n> @@ -6640,7 +6640,7 @@ proc drawtags {id x xt y1} {\n> set xl [expr {$xl - $delta/2}]\n> $canv create polygon $x $yt $xr $yt $xr $yb $x $yb \\\n> -width 1 -outline black -fill $col -tags tag.$id\n> - if {[regexp {^(remotes/.*/|remotes/)} $tag match remoteprefix]} {\n> + if {[regexp {^(remotes/[^/]*/|remotes/)} $tag match remoteprefix]} {\n> set rwid [font measure mainfont $remoteprefix]\n> set xi [expr {$x + 1}]\n> set yti [expr {$yt + 1}]\n> --\n\nThis likely fixes the problem for most situations, but doesn't for a\nremote with a '/' in the name. Yet, I think this is a better state\nthan the present.\n\nIs the regex `[^/]*/` more efficient than '.*?/`? Or do you find the\nformer more readable?\n"},{"id":"283396","messageId":"CAE8SKAMgZzyzoiy4JsqONN4wVWgVq-YmMn1+j2ZtELx+wJ1xEQ@mail.gmail.com","threadId":"42012","inReplyTo":"CANoM8SXixymz3=NQWgG5vSo7XDNh18_OzrNXU4+Y8CQ0LkB6sw@mail.gmail.com","subject":"Re: [PATCH] gitk: Fix how remote branch names with / are drawn","fromName":"David Holmer","fromEmail":"odinguru@gmail.com","sentAt":"2016-04-13T18:19:39Z","receivedAt":"2016-04-13T18:19:39Z","isPatch":true,"sender":{"key":"odinguru@gmail.com","avatar":null},"body":"I agree that this switches the issue around and that a remote with a\n'/' in the name would be miss colored in the same way a branch with a\n'/' in the name is miss colored now. However, I would guess that\nbranches with '/' are MUCH MUCH more common than remotes with '/', so\nlike you say \"this is a better state than the present\". A \"complete\"\nsolution would take iterating through the list of remotes and matching\nthe explicit whole pattern (e.g. match\n\"remotes/my/remote/with/slashes/\" for remote \"my/remote/with/slashes\")\nbut I doubt that is worth it for 99.9% of people.\n\nThe alternative regex that you are asking about is either using some\nsyntax I am not familiar with or isn't quite correct. I'm most\nfamiliar with grep command line format, so perhaps tcl regex is\ndifferent.\n\nThe original code does the equivalent of this:\n\n~$ echo \"remotes/origin/dev/test1\" | grep -o \"remotes/.*/\"\nremotes/origin/dev/\n\nThe issue is that the '.*/' part is greedy in that it will match all\nthe way up to and including the last /\n\nMy solution was to change the . to [^/] which means \"any character but\n/\". This stops the match at the first / after the remote name starts:\n\n~$ echo \"remotes/origin/dev/test1\" | grep -o \"remotes/[^/]*/\"\nremotes/origin/\n\nThe alternative you suggested with '.*?/' doesn't seem to work with grep:\n\n~$ echo \"remotes/origin/dev/test1\" | grep -o \"remotes/.*?/\"\n(no output, i.e. does not match)\n\n\nThank you.\n\nOn Wed, Apr 13, 2016 at 7:35 AM, Mike Rappazzo wrote:\n> On Tue, Apr 12, 2016 at 9:59 PM, David Holmer wrote:\n>> Consider this example branch:\n>>\n>> remotes/origin/master\n>>\n>> gitk displays this branch with different background colors for each part:\n>> \"remotes/origin\" in orange and \"master\" in green. The idea is to make it\n>> visually easy to read the branch name separately from the remote name.\n>>\n>> However this fails when given this example branch:\n>>\n>> remotes/origin/foo/bar\n>>\n>> gitk displays this branch with \"remotes/origin/foo\" in orange and \"bar\" in\n>> green. This makes it hard to read the branch name \"foo/bar\". This is due\n>> to an inappropriately greedy regexp. This patch provides a fix so the same\n>> branch will now be displayed with \"remotes/origin\" in orange and \"foo/bar\"\n>> in green.\n>>\n>> Signed-off-by: David Holmer \n>> ---\n>> gitk | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/gitk b/gitk\n>> index 805a1c7..ca2392b 100755\n>> --- a/gitk\n>> +++ b/gitk\n>> @@ -6640,7 +6640,7 @@ proc drawtags {id x xt y1} {\n>> set xl [expr {$xl - $delta/2}]\n>> $canv create polygon $x $yt $xr $yt $xr $yb $x $yb \\\n>> -width 1 -outline black -fill $col -tags tag.$id\n>> - if {[regexp {^(remotes/.*/|remotes/)} $tag match remoteprefix]} {\n>> + if {[regexp {^(remotes/[^/]*/|remotes/)} $tag match remoteprefix]} {\n>> set rwid [font measure mainfont $remoteprefix]\n>> set xi [expr {$x + 1}]\n>> set yti [expr {$yt + 1}]\n>> --\n>\n> This likely fixes the problem for most situations, but doesn't for a\n> remote with a '/' in the name. Yet, I think this is a better state\n> than the present.\n>\n> Is the regex `[^/]*/` more efficient than '.*?/`? Or do you find the\n> former more readable?\n"},{"id":"283397","messageId":"CANoM8SXSW6QrhBhq1GCOv6h4cWs8+KrnPF+GAkMPmW1_+nTuRg@mail.gmail.com","threadId":"42012","inReplyTo":"CAE8SKAMgZzyzoiy4JsqONN4wVWgVq-YmMn1+j2ZtELx+wJ1xEQ@mail.gmail.com","subject":"Re: [PATCH] gitk: Fix how remote branch names with / are drawn","fromName":"Mike Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2016-04-13T18:28:39Z","receivedAt":"2016-04-13T18:28:39Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"On Wed, Apr 13, 2016 at 2:19 PM, David Holmer wrote:\n> I agree that this switches the issue around and that a remote with a\n> '/' in the name would be miss colored in the same way a branch with a\n> '/' in the name is miss colored now. However, I would guess that\n> branches with '/' are MUCH MUCH more common than remotes with '/', so\n> like you say \"this is a better state than the present\". A \"complete\"\n> solution would take iterating through the list of remotes and matching\n> the explicit whole pattern (e.g. match\n> \"remotes/my/remote/with/slashes/\" for remote \"my/remote/with/slashes\")\n> but I doubt that is worth it for 99.9% of people.\n>\n> The alternative regex that you are asking about is either using some\n> syntax I am not familiar with or isn't quite correct. I'm most\n> familiar with grep command line format, so perhaps tcl regex is\n> different.\n>\n> The original code does the equivalent of this:\n>\n> ~$ echo \"remotes/origin/dev/test1\" | grep -o \"remotes/.*/\"\n> remotes/origin/dev/\n>\n> The issue is that the '.*/' part is greedy in that it will match all\n> the way up to and including the last /\n>\n> My solution was to change the . to [^/] which means \"any character but\n> /\". This stops the match at the first / after the remote name starts:\n>\n> ~$ echo \"remotes/origin/dev/test1\" | grep -o \"remotes/[^/]*/\"\n> remotes/origin/\n>\n> The alternative you suggested with '.*?/' doesn't seem to work with grep:\n>\n> ~$ echo \"remotes/origin/dev/test1\" | grep -o \"remotes/.*?/\"\n> (no output, i.e. does not match)\n\n`.*?` is a lazy match. I think it is an extended-regex, and your\nversion is probably more efficient anyway.\necho \"remotes/origin/dev/test1\" | grep -Eo \"remotes/.*?/\"\n\n>\n>\n> Thank you.\n>\n\n(Most people on this list don't like \"top posting\"), please try to\nreply inline instead.\n\n\n> On Wed, Apr 13, 2016 at 7:35 AM, Mike Rappazzo wrote:\n>> On Tue, Apr 12, 2016 at 9:59 PM, David Holmer wrote:\n>>> Consider this example branch:\n>>>\n>>> remotes/origin/master\n>>>\n>>> gitk displays this branch with different background colors for each part:\n>>> \"remotes/origin\" in orange and \"master\" in green. The idea is to make it\n>>> visually easy to read the branch name separately from the remote name.\n>>>\n>>> However this fails when given this example branch:\n>>>\n>>> remotes/origin/foo/bar\n>>>\n>>> gitk displays this branch with \"remotes/origin/foo\" in orange and \"bar\" in\n>>> green. This makes it hard to read the branch name \"foo/bar\". This is due\n>>> to an inappropriately greedy regexp. This patch provides a fix so the same\n>>> branch will now be displayed with \"remotes/origin\" in orange and \"foo/bar\"\n>>> in green.\n>>>\n>>> Signed-off-by: David Holmer \n>>> ---\n>>> gitk | 2 +-\n>>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>>\n>>> diff --git a/gitk b/gitk\n>>> index 805a1c7..ca2392b 100755\n>>> --- a/gitk\n>>> +++ b/gitk\n>>> @@ -6640,7 +6640,7 @@ proc drawtags {id x xt y1} {\n>>> set xl [expr {$xl - $delta/2}]\n>>> $canv create polygon $x $yt $xr $yt $xr $yb $x $yb \\\n>>> -width 1 -outline black -fill $col -tags tag.$id\n>>> - if {[regexp {^(remotes/.*/|remotes/)} $tag match remoteprefix]} {\n>>> + if {[regexp {^(remotes/[^/]*/|remotes/)} $tag match remoteprefix]} {\n>>> set rwid [font measure mainfont $remoteprefix]\n>>> set xi [expr {$x + 1}]\n>>> set yti [expr {$yt + 1}]\n>>> --\n>>\n>> This likely fixes the problem for most situations, but doesn't for a\n>> remote with a '/' in the name. Yet, I think this is a better state\n>> than the present.\n>>\n>> Is the regex `[^/]*/` more efficient than '.*?/`? Or do you find the\n>> former more readable?\n"}]}