threads / patch / 14493

patch, 3 partsImprove gui blame usability for large repositories

Subject: [PATCH (GIT-GUI) 0/3] Improve gui blame usability for large repositories

## tl;dr

6 messages between Jul 16, 2008 and Jul 18, 2008. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Alexander Gavrilov· Jul 16, 2008, 20:42 UTC · lore

Full copy detection can take quite some time on large repositories, which substantially decreases perceived usability of the 'git gui blame' command. This set of patches tries to overcome it by:

1) Allowing the user to disable '-C -C' and/or set the detection threshold.
2) Explicitly killing back-end processes, which don't produce any output
  during copy detection, and thus normally won't receive SIGPIPE until
  it is finished. Runaway processes are annoying.
3) To compensate for (1), adding a context menu item to manually invoke
  blame -C -C -C on a group of lines.
Alexander Gavrilov (3):
      Add options to control the search for copies in blame.
      Kill the blame back-end on window close.
      Add a menu item to invoke full copy detection in blame.
 git-gui.sh     |   16 ++++++++
 lib/blame.tcl  |  105 +++++++++++++++++++++++++++++++++++++++++++++++++------
 lib/option.tcl |    2 +
 3 files changed, 111 insertions(+), 12 deletions(-)
Alexander Gavrilov· Jul 16, 2008, 20:43 UTC · re: Alexander Gavrilov · lore

[PATCH (GIT-GUI) 1/3] Add options to control the search for copies in blame.

On huge repositories, -C -C can be way too slow to be unconditionally enabled, and it can also be useful to control its precision.

Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---
 git-gui.sh     |    2 ++
 lib/blame.tcl  |   20 ++++++++++++--------
 lib/option.tcl |    2 ++
 3 files changed, 16 insertions(+), 8 deletions(-)
Show changes to 3 files +16 −8

git-gui.sh, lib/blame.tcl, lib/option.tcl

diff --git a/git-gui.sh b/git-gui.sh
index e3b6669..b1ed0ec 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -642,6 +642,8 @@ set default_config(user.email) {}
 set default_config(gui.matchtrackingbranch) false
 set default_config(gui.pruneduringfetch) false
 set default_config(gui.trustmtime) false
+set default_config(gui.fastcopyblame) false
+set default_config(gui.copyblamethreshold) 40
 set default_config(gui.diffcontext) 5
 set default_config(gui.commitmsgwidth) 75
 set default_config(gui.newbranchtemplate) {}
diff --git a/lib/blame.tcl b/lib/blame.tcl
index 92fac1b..192505d 100644
--- a/lib/blame.tcl
+++ b/lib/blame.tcl
@@ -33,13 +33,6 @@ variable group_colors {
 	#ececec
 }
 
-# Switches for original location detection
-#
-variable original_options [list -C -C]
-if {[git-version >= 1.5.3]} {
-	lappend original_options -w ; # ignore indentation changes
-}
-
 # Current blame data; cleared/reset on each load
 #
 field commit               ; # input commit to blame
@@ -511,7 +504,6 @@ method _exec_blame {cur_w cur_d options cur_s} {
 method _read_blame {fd cur_w cur_d} {
 	upvar #0 $cur_d line_data
 	variable group_colors
-	variable original_options
 
 	if {$fd ne $current_fd} {
 		catch {close $fd}
@@ -684,6 +676,18 @@ method _read_blame {fd cur_w cur_d} {
 	if {[eof $fd]} {
 		close $fd
 		if {$cur_w eq $w_asim} {
+			# Switches for original location detection
+			set threshold [get_config gui.copyblamethreshold]
+			set original_options [list "-C$threshold"]
+
+			if {![is_config_true gui.fastcopyblame]} {
+				# thorough copy search; insert before the threshold
+				set original_options [linsert $original_options 0 -C]
+			}
+			if {[git-version >= 1.5.3]} {
+				lappend original_options -w ; # ignore indentation changes
+			}
+
 			_exec_blame $this $w_amov @amov_data \
 				$original_options \
 				[mc "Loading original location annotations..."]
diff --git a/lib/option.tcl b/lib/option.tcl
index 9270512..ffb3f00 100644
--- a/lib/option.tcl
+++ b/lib/option.tcl
@@ -123,6 +123,8 @@ proc do_options {} {
 		{b gui.trustmtime  {mc "Trust File Modification Timestamps"}}
 		{b gui.pruneduringfetch {mc "Prune Tracking Branches During Fetch"}}
 		{b gui.matchtrackingbranch {mc "Match Tracking Branches"}}
+		{b gui.fastcopyblame {mc "Blame Copy Only On Changed Files"}}
+		{i-20..200 gui.copyblamethreshold {mc "Minimum Letters To Blame Copy On"}}
 		{i-0..99 gui.diffcontext {mc "Number of Diff Context Lines"}}
 		{i-0..99 gui.commitmsgwidth {mc "Commit Message Text Width"}}
 		{t gui.newbranchtemplate {mc "New Branch Name Template"}}

-- 
1.5.6.3.17.g3f148
Alexander Gavrilov· Jul 16, 2008, 20:48 UTC · re: Alexander Gavrilov · lore

[PATCH (GIT-GUI) 2/3] Kill the blame back-end on window close.

Currently 'git-gui blame' does not kill its back-end process, hoping that it will die anyway when the pipe is closed. However, in some cases the process works for a long time without producing any output. This behavior results in a runaway CPU hog.

Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---
	The -f flag is necessary for msysgit.
	For this fix I submitted to msysgit a patch that includes
	a Cygwin-compatible kill.exe in the installer.
	-- Alexander
 git-gui.sh    |   14 ++++++++++++++
 lib/blame.tcl |   16 ++++++++++++----
 2 files changed, 26 insertions(+), 4 deletions(-)
Show changes to 2 files +26 −4

git-gui.sh, lib/blame.tcl

diff --git a/git-gui.sh b/git-gui.sh
index b1ed0ec..83e2645 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -497,6 +497,20 @@ proc githook_read {hook_name args} {
 	return {}
 }
 
+proc kill_file_process {fd} {
+	set process [pid $fd]
+
+	catch {
+		if {[is_Windows]} {
+			# Use a Cygwin-specific flag to allow killing
+			# native Windows processes
+			exec kill -f $process
+		} else {
+			exec kill $process
+		}
+	}
+}
+
 proc sq {value} {
 	regsub -all ' $value "'\\''" value
 	return "'$value'"
diff --git a/lib/blame.tcl b/lib/blame.tcl
index 192505d..2c19048 100644
--- a/lib/blame.tcl
+++ b/lib/blame.tcl
@@ -326,19 +326,27 @@ constructor new {i_commit i_path} {
 	bind $w.file_pane <Configure> \
 	"if {{$w.file_pane} eq {%W}} {[cb _resize %h]}"
 
+	wm protocol $top WM_DELETE_WINDOW "destroy $top"
+	bind $top <Destroy> [cb _kill]
+
 	_load $this {}
 }
 
+method _kill {} {
+	if {$current_fd ne {}} {
+		kill_file_process $current_fd
+		catch {close $current_fd}
+		set current_fd {}
+	}
+}
+
 method _load {jump} {
 	variable group_colors
 
 	_hide_tooltip $this
 
 	if {$total_lines != 0 || $current_fd ne {}} {
-		if {$current_fd ne {}} {
-			catch {close $current_fd}
-			set current_fd {}
-		}
+		_kill $this
 
 		foreach i $w_columns {
 			$i conf -state normal

-- 
1.5.6.3.17.g3f148
Alexander Gavrilov· Jul 16, 2008, 20:51 UTC · re: Alexander Gavrilov · lore

[PATCH (GIT-GUI) 3/3] Add a menu item to invoke full copy detection in blame.

Add a context menu item to invoke blame -C -C -C on a chunk of the file. The results are used to update the 'original location' column of the blame display.

The chunk is computed as the smallest line range that covers both the 'last change' and 'original location' ranges of the line that was clicked to open the menu.

Signed-off-by: Alexander Gavrilov <angavrilov@gmail.com>
---
	This is my most complex Tcl/Tk code so far, so I might have
	done some stupid things.
	-- Alexander
 lib/blame.tcl |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 69 insertions(+), 0 deletions(-)
Show changes to lib/blame.tcl +69 −0
diff --git a/lib/blame.tcl b/lib/blame.tcl
index 2c19048..b6e42cb 100644
--- a/lib/blame.tcl
+++ b/lib/blame.tcl
@@ -256,6 +256,9 @@ constructor new {i_commit i_path} {
 	$w.ctxm add command \
 		-label [mc "Copy Commit"] \
 		-command [cb _copycommit]
+	$w.ctxm add command \
+		-label [mc "Do Full Copy Detection"] \
+		-command [cb _fullcopyblame]
 
 	foreach i $w_columns {
 		for {set g 0} {$g < [llength $group_colors]} {incr g} {
@@ -708,6 +711,72 @@ method _read_blame {fd cur_w cur_d} {
 	}
 } ifdeleted { catch {close $fd} }
 
+method _find_commit_bound {data_list start_idx delta} {
+	upvar #0 $data_list line_data
+	set pos $start_idx
+	set limit       [expr {[llength $line_data] - 1}]
+	set base_commit [lindex $line_data $pos 0]
+
+	while {$pos > 0 && $pos < $limit} {
+		set new_pos [expr {$pos + $delta}]
+		if {[lindex $line_data $new_pos 0] ne $base_commit} {
+			return $pos
+		}
+
+		set pos $new_pos
+	}
+
+	return $pos
+}
+
+method _fullcopyblame {} {
+	if {$current_fd ne {}} {
+		tk_messageBox \
+			-icon error \
+			-type ok \
+			-title [mc "Busy"] \
+			-message [mc "Annotation process is already running."]
+
+		return
+	}
+
+	# Switches for original location detection
+	set threshold [get_config gui.copyblamethreshold]
+	set original_options [list -C -C "-C$threshold"]
+
+	if {[git-version >= 1.5.3]} {
+		lappend original_options -w ; # ignore indentation changes
+	}
+
+	# Find the line range
+	set pos @$::cursorX,$::cursorY
+	set lno [lindex [split [$::cursorW index $pos] .] 0]
+	set min_amov_lno [_find_commit_bound $this @amov_data $lno -1]
+	set max_amov_lno [_find_commit_bound $this @amov_data $lno 1]
+	set min_asim_lno [_find_commit_bound $this @asim_data $lno -1]
+	set max_asim_lno [_find_commit_bound $this @asim_data $lno 1]
+
+	if {$min_asim_lno < $min_amov_lno} {
+		set min_amov_lno $min_asim_lno
+	}
+
+	if {$max_asim_lno > $max_amov_lno} {
+		set max_amov_lno $max_asim_lno
+	}
+
+	lappend original_options -L "$min_amov_lno,$max_amov_lno"
+
+	# Clear lines
+	for {set i $min_amov_lno} {$i <= $max_amov_lno} {incr i} {
+		lset amov_data $i [list ]
+	}
+
+	# Start the back-end process
+	_exec_blame $this $w_amov @amov_data \
+		$original_options \
+		[mc "Running thorough copy detection..."]
+}
+
 method _click {cur_w pos} {
 	set lno [lindex [split [$cur_w index $pos] .] 0]
 	_showcommit $this $cur_w $lno

-- 
1.5.6.3.17.g3f148
Shawn O. Pearce· Jul 17, 2008, 02:16 UTC · re: Alexander Gavrilov · lore

Re: [PATCH (GIT-GUI) 0/3] Improve gui blame usability for large repositories

Alexander Gavrilov <angavrilov@gmail.com> wrote:
Show 12 quoted lines
> Full copy detection can take quite some time on large repositories, which
> substantially decreases perceived usability of the 'git gui blame' command.
> This set of patches tries to overcome it by:
> 
> 1) Allowing the user to disable '-C -C' and/or set the detection threshold.
> 
> 2) Explicitly killing back-end processes, which don't produce any output
>   during copy detection, and thus normally won't receive SIGPIPE until
>   it is finished. Runaway processes are annoying.
> 
> 3) To compensate for (1), adding a context menu item to manually invoke
>   blame -C -C -C on a group of lines.

This series is nicely done. Works well on revision.c in git.git, where the blame can be costly to compute with full copy detection. And getting the incremental from the context menu also works well.

Thanks.
 
-- 
Shawn.
Alexander Gavrilov· Jul 18, 2008, 05:39 UTC · re: Shawn O. Pearce · lore

Re: [PATCH (GIT-GUI) 0/3] Improve gui blame usability for large repositories

Hello,
On Thursday 17 July 2008 06:16:23 Shawn O. Pearce wrote:
Show 17 quoted lines
> Alexander Gavrilov <angavrilov@gmail.com> wrote:
> > Full copy detection can take quite some time on large repositories, which
> > substantially decreases perceived usability of the 'git gui blame' command.
> > This set of patches tries to overcome it by:
> > 
> > 1) Allowing the user to disable '-C -C' and/or set the detection threshold.
> > 
> > 2) Explicitly killing back-end processes, which don't produce any output
> >   during copy detection, and thus normally won't receive SIGPIPE until
> >   it is finished. Runaway processes are annoying.
> > 
> > 3) To compensate for (1), adding a context menu item to manually invoke
> >   blame -C -C -C on a group of lines.
> 
> This series is nicely done.  Works well on revision.c in git.git,
> where the blame can be costly to compute with full copy detection.
> And getting the incremental from the context menu also works well.

I also thought that it would be useful to be able to specify the line range explicitly, but couldn't invent a good UI for it. Selecting lines with the mouse also causes a commit to be selected and recolored in green, and popping up a dialog to request line numbers seems too lame.

Alexander

← back to recent threads