Re: [PATCH] gitk: Fix how remote branch names with / are drawn
- From
Mike Rappazzo <rappazzo@gmail.com>
- Date
- Apr 13, 2016, 18:28 UTC
- Message-ID
- <CANoM8SXSW6QrhBhq1GCOv6h4cWs8+KrnPF+GAkMPmW1_+nTuRg@mail.gmail.com>
- In-Reply-To
- <CAE8SKAMgZzyzoiy4JsqONN4wVWgVq-YmMn1+j2ZtELx+wJ1xEQ@mail.gmail.com>
On Wed, Apr 13, 2016 at 2:19 PM, David Holmer <odinguru@gmail.com> wrote:
Show 33 quoted lines
> 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.
Show 46 quoted lines
> 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?