threads / discuss / 15679

gitk: Turn short SHA1 names into links too

Subject: gitk: Turn short SHA1 names into links too

## tl;dr

12 messages between Sep 26, 2008 and Oct 21, 2008.

replies: 11people: 8as markdown or json

Linus Torvalds· Sep 26, 2008, 00:11 UTC · lore

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
Linus Torvalds· Sep 26, 2008, 00:37 UTC · re: Linus Torvalds · lore

Re: gitk: Turn short SHA1 names into links too

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
Wincent Colaiuta· Sep 26, 2008, 06:32 UTC · re: Linus Torvalds · lore

Re: gitk: Turn short SHA1 names into links too

El 26/9/2008, a las 2:37, Linus Torvalds escribió:
Show 28 quoted lines
> 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
Andreas Ericsson· Sep 26, 2008, 07:26 UTC · re: Wincent Colaiuta · lore

Re: gitk: Turn short SHA1 names into links too

Wincent Colaiuta wrote:
Show 41 quoted lines
> 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
Wincent Colaiuta· Sep 26, 2008, 07:29 UTC · re: Andreas Ericsson · lore

Re: gitk: Turn short SHA1 names into links too

El 26/9/2008, a las 9:26, Andreas Ericsson escribió:
Show 47 quoted lines
> 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

Mikael Magnusson· Sep 26, 2008, 08:21 UTC · re: Wincent Colaiuta · lore

Re: gitk: Turn short SHA1 names into links too

2008/9/26 Wincent Colaiuta <win@wincent.com>:
Show 40 quoted lines
> 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
Brad King· Sep 26, 2008, 12:15 UTC · re: Mikael Magnusson · lore

Re: gitk: Turn short SHA1 names into links too

Mikael Magnusson wrote:
Show 18 quoted lines
> 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
Marco Costalba· Sep 26, 2008, 10:41 UTC · re: Wincent Colaiuta · lore

Re: gitk: Turn short SHA1 names into links too

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.

Paul Mackerras· Sep 27, 2008, 03:18 UTC · re: Wincent Colaiuta · lore

Re: gitk: Turn short SHA1 names into links too

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.
Paul Mackerras· Sep 27, 2008, 03:16 UTC · re: Linus Torvalds · lore

Re: gitk: Turn short SHA1 names into links too

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...
Show 11 quoted lines
> 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...

Show 7 quoted lines
>  - 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.

Show 5 quoted lines
> 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]

Show 12 quoted lines
> 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.
Paul Mackerras· Oct 20, 2008, 23:20 UTC · re: Linus Torvalds · lore

Re: gitk: Turn short SHA1 names into links too

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.
Junio C Hamano· Oct 21, 2008, 19:09 UTC · re: Paul Mackerras · lore

Re: gitk: Turn short SHA1 names into links too

Paul Mackerras <paulus@samba.org> writes:
Show 8 quoted lines
> 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.

← back to recent threads