gitk: Turn short SHA1 names into links too

gitk: Turn short SHA1 names into links too

[Linus Torvalds]

Ok, so I'm a newbie when it comes to tcl/tk, but I can do copy-paste and 
make things work. 

And the thing I wanted to work was to have the abbreviated SHA1's that 
have started to get more common in the kernel commit logs work as links in 
gitk too, just the way a full 40-character SHA1 link works.

This patch does seem to work, but it's also buggy in exactly the same ways 
the regular 40-character links are buggy, and while I find those bugs 
very irritating _too_, I can't cut-and-paste myself to a solution for 
that.

The pre-existing bugs that this shares with the long links are:

 - since gitk started doing incremental showing of the graph, the whole 
   "check if it exists" doesn't work right if the target hasn't been 
   loaded yet. And when it _does_ end up being loaded one second later, 
   nothing re-does the scanning.

   This is just a small irritation, but it's quite common when the first 
   commit that is displayed has a link. You can fix it by moving to the 
   next commit and moving right back (cursor-down + cursor-up), which will 
   re-display the commit and now find the link that wasn't available 
   initially, but it's still irritating.

   I think gitk could re-display the commit it is on when the whole list 
   of commits has been parsed, and at least then show the links it missed 
   initially after a few seconds.

 - slightly related to the above: when we _do_ find a link, we create it 
   to be a link to line so-and-so, but since we now don't just 
   incrementally parse the commits that come in, but gitk _also_ actually 
   reflows the commits to be in topological order, the link we just 
   created may actually no longer point to the right line by the time the 
   link is then clicked on, so clicking on a link can actually take you to 
   the wrong commit!

   Again, re-displaying the current commit after we have gotten and 
   parsed all commits will fix it, but this is a more fundamental problem: 
   if we redisplay at the end, there is still a window when the link may 
   simply be wrong, because we've redone the topo sort, but we haven't 
   seen _all_ commits yet.

   But again, you can work around it by going back and retrying, and at 
   some time it will stabilize. But this one is _really_ irritating when 
   it triggers, because it can make you look at the wrong commit without 
   necessarily realizing it was wrong!

I suspect that the correct fix is to always do the link, whether we 
actually see it or not, and not make it point to a line number, but simply 
keep it as a SHA1, and then do the equivalent of "gotocommit" when 
clicking it. But I don't know how links workin tcl/tk, so I'm not going to 
be able to do that.

In the meantime, this patch introduces no new bugs, and the workarounds 
are the same for abbreviated SHA1's as they are for the full ones.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

I'm sure it could have been done better. In particular, I think the 
"short->long" translation could/should probably be a function of its own. 
But I'm so uncomfortable with wish programming that I'm not starting to 
write any new functions..

Comments? Paul?

			Linus

 gitk-git/gitk |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 2eaa2ae..f79643a 100644
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -5759,7 +5759,7 @@ proc appendwithlinks {text tags} {
 
     set start [$ctext index "end - 1c"]
     $ctext insert end $text $tags
-    set links [regexp -indices -all -inline {[0-9a-f]{40}} $text]
+    set links [regexp -indices -all -inline {[0-9a-f]{6,40}} $text]
     foreach l $links {
 	set s [lindex $l 0]
 	set e [lindex $l 1]
@@ -5773,7 +5773,18 @@ proc appendwithlinks {text tags} {
 }
 
 proc setlink {id lk} {
-    global curview ctext pendinglinks commitinterest
+    global curview ctext pendinglinks commitinterest varcid
+
+    # Turn a short ID into a full one
+    if {[regexp {^[0-9a-f]{4,39}$} $id]} {
+        set matches [array names varcid "$curview,$id*"]
+        if {$matches ne {}} {
+            if {[llength $matches] > 1} {
+                return
+            }
+            set id [lindex [split [lindex $matches 0] ","] 1]
+        }
+    }
 
     if {[commitinview $id $curview]} {
 	$ctext tag conf $lk -foreground blue -underline 1

Re: gitk: Turn short SHA1 names into links too

[Linus Torvalds]

On Thu, 25 Sep 2008, Linus Torvalds wrote:
> 
> And the thing I wanted to work was to have the abbreviated SHA1's that 
> have started to get more common in the kernel commit logs work as links in 
> gitk too, just the way a full 40-character SHA1 link works.

For a test-case, I just pushed out my current top-of-tree that finally 
pushed me over the edge. I've seen this before, but I couldn't really 
force me to do anything about it until now.

So to see this in action, do

	gitk v2.6.26..6ef190c

on the current kernel repo, and notice that "Commit ee1e2c82 ("IPoIB: 
Refresh paths .." thing, where we want that 'ee1e2c82' to be a link even 
though it's not a full SHA1.

Of course, the matching could be better, it will now accept any random 6+ 
character sequence of hex characters, even if they are surrounded by 
characters that make it clear that it's not a SHA1 ("Haahahhaaaaaa!" 
would find the 'aaaaaa' and if you have a commit that starts with that, 
link to it ;)

And because it's the top commit, you can also easily see the behavior that 
it doesn't turn out to be a link immediately, and you have to do 
cursor-down + cursor-up to get the link. The "it points to the wrong line" 
bug is much harder to trigger, and possibly impossible with that 
particular case (to trigger it, you need a setup where the first layout of 
the target commit then gets changed by the topo-sort, I'm not sure that 
will happen at all, and if it does, I'm not sure you could time it just 
right either, but I _have_ seen it in real life).

			Linus

Re: gitk: Turn short SHA1 names into links too

[Wincent Colaiuta]

El 26/9/2008, a las 2:37, Linus Torvalds escribió:

> On Thu, 25 Sep 2008, Linus Torvalds wrote:
>>
>> And the thing I wanted to work was to have the abbreviated SHA1's  
>> that
>> have started to get more common in the kernel commit logs work as  
>> links in
>> gitk too, just the way a full 40-character SHA1 link works.
>
> For a test-case, I just pushed out my current top-of-tree that finally
> pushed me over the edge. I've seen this before, but I couldn't really
> force me to do anything about it until now.
>
> So to see this in action, do
>
> 	gitk v2.6.26..6ef190c
>
> on the current kernel repo, and notice that "Commit ee1e2c82 ("IPoIB:
> Refresh paths .." thing, where we want that 'ee1e2c82' to be a link  
> even
> though it's not a full SHA1.
>
> Of course, the matching could be better, it will now accept any  
> random 6+
> character sequence of hex characters, even if they are surrounded by
> characters that make it clear that it's not a SHA1 ("Haahahhaaaaaa!"
> would find the 'aaaaaa' and if you have a commit that starts with  
> that,
> link to it ;)

I know nothing about tcl/tk, but will comment anyway:

It's a shame that tcl/tk regular expressions don't appear to support  
anchoring matches against word boundaries (ie. "\b").

If so, a regexp like:

  [regexp {\b[0-9a-f]{4,39}\b} $id]

would mostly eliminate that kind of false positive. But from my  
reading of the wiki[1], looks like there's no "\b" escape sequence.  
Nor does it look like tcl/tk has support for lookahead/lookbehind  
assertions which could be used to the same effect.

>

Cheers,
Wincent

[1] http://wiki.tcl.tk/396

Re: gitk: Turn short SHA1 names into links too

[Andreas Ericsson]

Wincent Colaiuta wrote:
> El 26/9/2008, a las 2:37, Linus Torvalds escribió:
> 
>> On Thu, 25 Sep 2008, Linus Torvalds wrote:
>>>
>>> And the thing I wanted to work was to have the abbreviated SHA1's that
>>> have started to get more common in the kernel commit logs work as 
>>> links in
>>> gitk too, just the way a full 40-character SHA1 link works.
>>
>> For a test-case, I just pushed out my current top-of-tree that finally
>> pushed me over the edge. I've seen this before, but I couldn't really
>> force me to do anything about it until now.
>>
>> So to see this in action, do
>>
>>     gitk v2.6.26..6ef190c
>>
>> on the current kernel repo, and notice that "Commit ee1e2c82 ("IPoIB:
>> Refresh paths .." thing, where we want that 'ee1e2c82' to be a link even
>> though it's not a full SHA1.
>>
>> Of course, the matching could be better, it will now accept any random 6+
>> character sequence of hex characters, even if they are surrounded by
>> characters that make it clear that it's not a SHA1 ("Haahahhaaaaaa!"
>> would find the 'aaaaaa' and if you have a commit that starts with that,
>> link to it ;)
> 
> I know nothing about tcl/tk, but will comment anyway:
> 
> It's a shame that tcl/tk regular expressions don't appear to support 
> anchoring matches against word boundaries (ie. "\b").
> 
> If so, a regexp like:
> 
>  [regexp {\b[0-9a-f]{4,39}\b} $id]
> 
> would mostly eliminate that kind of false positive. But from my reading 
> of the wiki[1], looks like there's no "\b" escape sequence. Nor does it 
> look like tcl/tk has support for lookahead/lookbehind assertions which 
> could be used to the same effect.
> 

It's not as if this will be a real problem anyway. gitk is designed to
be used by humans, who can fortunately parse such things a trillion
times better than any regex a billion monkeys could come up with in a
billion years, even if one was to take evolution into account. ;-)

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: gitk: Turn short SHA1 names into links too

[Wincent Colaiuta]

El 26/9/2008, a las 9:26, Andreas Ericsson escribió:

> Wincent Colaiuta wrote:
>> El 26/9/2008, a las 2:37, Linus Torvalds escribió:
>>> On Thu, 25 Sep 2008, Linus Torvalds wrote:
>>>>
>>>> And the thing I wanted to work was to have the abbreviated SHA1's  
>>>> that
>>>> have started to get more common in the kernel commit logs work as  
>>>> links in
>>>> gitk too, just the way a full 40-character SHA1 link works.
>>>
>>> For a test-case, I just pushed out my current top-of-tree that  
>>> finally
>>> pushed me over the edge. I've seen this before, but I couldn't  
>>> really
>>> force me to do anything about it until now.
>>>
>>> So to see this in action, do
>>>
>>>    gitk v2.6.26..6ef190c
>>>
>>> on the current kernel repo, and notice that "Commit ee1e2c82  
>>> ("IPoIB:
>>> Refresh paths .." thing, where we want that 'ee1e2c82' to be a  
>>> link even
>>> though it's not a full SHA1.
>>>
>>> Of course, the matching could be better, it will now accept any  
>>> random 6+
>>> character sequence of hex characters, even if they are surrounded by
>>> characters that make it clear that it's not a SHA1 ("Haahahhaaaaaa!"
>>> would find the 'aaaaaa' and if you have a commit that starts with  
>>> that,
>>> link to it ;)
>> I know nothing about tcl/tk, but will comment anyway:
>> It's a shame that tcl/tk regular expressions don't appear to  
>> support anchoring matches against word boundaries (ie. "\b").
>> If so, a regexp like:
>> [regexp {\b[0-9a-f]{4,39}\b} $id]
>> would mostly eliminate that kind of false positive. But from my  
>> reading of the wiki[1], looks like there's no "\b" escape sequence.  
>> Nor does it look like tcl/tk has support for lookahead/lookbehind  
>> assertions which could be used to the same effect.
>
> It's not as if this will be a real problem anyway. gitk is designed to
> be used by humans, who can fortunately parse such things a trillion
> times better than any regex a billion monkeys could come up with in a
> billion years, even if one was to take evolution into account. ;-)

Of course it's never going to be a _real_ problem (ie. a deal- 
breaker), but most technical users will look on it as sloppy  
programming when the 'aaaaaa' in 'Haahahhaaaaaa!' gets turned into a  
link.

Cheers,
Wincent

Re: gitk: Turn short SHA1 names into links too

[Mikael Magnusson]

2008/9/26 Wincent Colaiuta <win@wincent.com>:
> El 26/9/2008, a las 2:37, Linus Torvalds escribió:
>
>> On Thu, 25 Sep 2008, Linus Torvalds wrote:
>>>
>>> And the thing I wanted to work was to have the abbreviated SHA1's that
>>> have started to get more common in the kernel commit logs work as links
>>> in
>>> gitk too, just the way a full 40-character SHA1 link works.
>>
>> For a test-case, I just pushed out my current top-of-tree that finally
>> pushed me over the edge. I've seen this before, but I couldn't really
>> force me to do anything about it until now.
>>
>> So to see this in action, do
>>
>>        gitk v2.6.26..6ef190c
>>
>> on the current kernel repo, and notice that "Commit ee1e2c82 ("IPoIB:
>> Refresh paths .." thing, where we want that 'ee1e2c82' to be a link even
>> though it's not a full SHA1.
>>
>> Of course, the matching could be better, it will now accept any random 6+
>> character sequence of hex characters, even if they are surrounded by
>> characters that make it clear that it's not a SHA1 ("Haahahhaaaaaa!"
>> would find the 'aaaaaa' and if you have a commit that starts with that,
>> link to it ;)
>
> I know nothing about tcl/tk, but will comment anyway:
>
> It's a shame that tcl/tk regular expressions don't appear to support
> anchoring matches against word boundaries (ie. "\b").
>
> If so, a regexp like:
>
>  [regexp {\b[0-9a-f]{4,39}\b} $id]
>
> would mostly eliminate that kind of false positive. But from my reading of
> the wiki[1], looks like there's no "\b" escape sequence. Nor does it look
> like tcl/tk has support for lookahead/lookbehind assertions which could be
> used to the same effect.

\y appears to achieve this;

% regexp {abc\y} 'abc'
1
% regexp {abc\y} 'abcd'
0
% regexp {\yabc\y} 'uabc,d'
0
% regexp {\yabc\y} 'u+abc,d'
1

I have tcl/tk 8.5 so I cannot promise that isn't a new addition, I didn't
look it up anywhere, but it's \y in some other implementations too, so I
tried it.

-- 
Mikael Magnusson

Re: gitk: Turn short SHA1 names into links too

[Marco Costalba]

On Fri, Sep 26, 2008 at 8:32 AM, Wincent Colaiuta <win@wincent.com> wrote:
>
>  [regexp {\b[0-9a-f]{4,39}\b} $id]
>
> would mostly eliminate that kind of false positive.

This feature is already implemented in qgit from a long time. The most
difficult false positive I have found is the debug backtracing in the
log messages. Memory dumps are a big sources of false positives.

Re: gitk: Turn short SHA1 names into links too

[Brad King]

Mikael Magnusson wrote:
> 2008/9/26 Wincent Colaiuta <win@wincent.com>:
>> It's a shame that tcl/tk regular expressions don't appear to support
>> anchoring matches against word boundaries (ie. "\b").
> 
> \y appears to achieve this;
> 
> % regexp {abc\y} 'abc'
> 1
> % regexp {abc\y} 'abcd'
> 0
> % regexp {\yabc\y} 'uabc,d'
> 0
> % regexp {\yabc\y} 'u+abc,d'
> 1
> 
> I have tcl/tk 8.5 so I cannot promise that isn't a new addition, I didn't
> look it up anywhere, but it's \y in some other implementations too, so I
> tried it.

It goes back at least as far as tcl/tk 8.2:

http://www.tcl.tk/man/tcl8.2.3/TclCmd/re_syntax.htm#M54

-Brad

Re: gitk: Turn short SHA1 names into links too

[Paul Mackerras]

Linus Torvalds writes:

> And the thing I wanted to work was to have the abbreviated SHA1's that 
> have started to get more common in the kernel commit logs work as links in 
> gitk too, just the way a full 40-character SHA1 link works.

Fair enough...

> This patch does seem to work, but it's also buggy in exactly the same ways 
> the regular 40-character links are buggy, and while I find those bugs 
> very irritating _too_, I can't cut-and-paste myself to a solution for 
> that.
> 
> The pre-existing bugs that this shares with the long links are:
> 
>  - since gitk started doing incremental showing of the graph, the whole 
>    "check if it exists" doesn't work right if the target hasn't been 
>    loaded yet. And when it _does_ end up being loaded one second later, 
>    nothing re-does the scanning.

Actually there *is* logic in gitk to remember that certain SHA1 ids
are "interesting" and do something when we come across them later.  So
when we see a string of 40 hex digits in a commit message and we don't
recognize that as an ID that we know about, we remember it as an
interesting ID, with the action being to turn the string into a link.
So when the ID turns up later, the string in the commit message
becomes a link.

However, that currently only works for the full 40-character IDs, not
for abbreviations, because the way we remember that the ID is
interesting is with an associative array (i.e. a hash).  What we need
to do is use only the first 6 characters and then have each array
element be a list of (ID, action) pairs, where the ID can now be a
partial ID.

I'd hack it up now except that I just got home from the US and I need
to sleep...

>  - slightly related to the above: when we _do_ find a link, we create it 
>    to be a link to line so-and-so, but since we now don't just 
>    incrementally parse the commits that come in, but gitk _also_ actually 
>    reflows the commits to be in topological order, the link we just 
>    created may actually no longer point to the right line by the time the 
>    link is then clicked on, so clicking on a link can actually take you to 
>    the wrong commit!

Yep, guilty as charged.  We need to defer the ID -> line number
translation until the user actually clicks on the link.

> I suspect that the correct fix is to always do the link, whether we 
> actually see it or not, and not make it point to a line number, but simply 
> keep it as a SHA1, and then do the equivalent of "gotocommit" when 
> clicking it. But I don't know how links workin tcl/tk, so I'm not going to 
> be able to do that.

We need this change in setlink (caution, completely untested):

-	$ctext tag bind $lk <1> [list selectline [rowofcommit $id] 1]
+	$ctext tag bind $lk <1> [list selbyid $id]

> In the meantime, this patch introduces no new bugs, and the workarounds 
> are the same for abbreviated SHA1's as they are for the full ones.
> 
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
> 
> I'm sure it could have been done better. In particular, I think the 
> "short->long" translation could/should probably be a function of its own. 
> But I'm so uncomfortable with wish programming that I'm not starting to 
> write any new functions..
> 
> Comments? Paul?

The main thing is to change how we handle the commitinterest array so
that we can use it to match on short IDs as well as full 40-char ones.
And yes, we should pull out the short->long translation into its own
function.

Paul.

Re: gitk: Turn short SHA1 names into links too

[Paul Mackerras]

Wincent Colaiuta writes:

> It's a shame that tcl/tk regular expressions don't appear to support  
> anchoring matches against word boundaries (ie. "\b").

They do.  From the re_syntax (3tcl) man page:

         \m    matches only at the beginning of a word

         \M    matches only at the end of a word

         \y    matches only at the beginning or end of a word

         \Y    matches only at a point that is not the beginning or end  of  a
               word

Paul.

Re: gitk: Turn short SHA1 names into links too

[Paul Mackerras]

Linus Torvalds writes:

> And the thing I wanted to work was to have the abbreviated SHA1's that 
> have started to get more common in the kernel commit logs work as links in 
> gitk too, just the way a full 40-character SHA1 link works.

I just pushed out a commit to gitk that makes this work, and fixes the
other bugs you mentioned.

Paul.

Re: gitk: Turn short SHA1 names into links too

[Junio C Hamano]

Paul Mackerras <paulus@samba.org> writes:

> Linus Torvalds writes:
>
>> And the thing I wanted to work was to have the abbreviated SHA1's that 
>> have started to get more common in the kernel commit logs work as links in 
>> gitk too, just the way a full 40-character SHA1 link works.
>
> I just pushed out a commit to gitk that makes this work, and fixes the
> other bugs you mentioned.

FWIW, pulled and pushed out.  Thanks.