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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx> wrote:
> On Tue, Apr 12, 2016 at 9:59 PM, David Holmer <odinguru@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>>  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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]