Re: [PATCH 3/7] gitk: Allow starting gui blame for a specific line.

[Paul Mackerras <paulus@samba.org>]

Alexander Gavrilov writes:

> Adds a context menu item to the diff viewer pane that
> calls blame, focusing it on the clicked line. In case
> of combined diffs, it also automatically deduces which
> parent is to be blamed.

I have some comments on this one, but I haven't finished reviewing it
completely, so I may have more... :)

> +proc find_hunk_blamespec {base line} {
> +    global ctext
> +
> +    # Find and parse the hunk header
> +    set s_lix [$ctext search -backwards -regexp ^@@ "$line.0 lineend" $base.0]
> +    if {$s_lix eq {}} return
> +
> +    set s_line [$ctext get $s_lix "$s_lix + 1 lines"]
> +    if {![regexp {^@@@?(( -\d+(,\d+)?)+) \+(\d+)(,\d+)? @@} $s_line \

In fact there can be many @ characters at the beginning of the line;
the number of parents plus one, in fact.  See commit
c465a76af658b443075d6efee1c3131257643020 in the kernel tree for an
example.  I suggest starting with ^@@@* rather than ^@@@?.

> +    # Now scan the lines to determine offset within the hunk
> +    set parent {}
> +    set dline 0
> +    set s_lno [lindex [split $s_lix "."] 0]
> +
> +    for {set i $line} {$i > $s_lno} {incr i -1} {
> +	set c_line [$ctext get $i.0 "$i.0 + 1 lines"]
> +	if {$parent eq {}} {
> +	    # find first preceeding line that belongs to some parent
> +	    for {set j 0} {$j < [llength $old_lines]} {incr j} {
> +		set code [string index $c_line $j]
> +		if {$code ne {-} && $code ne { }} continue
> +		if {$code eq { } && $parent ne {}} continue
> +		set parent $j
> +		if {$code eq {-}} break
> +	    }
> +	}

This part worries me a bit.  If the user clicks on a line where all
the $code values are "+" then I think we should blame the current
commit.  Either that, or we disable the context menu item before
posting it if the user clicks on a line that starts with all "+"
characters (as many "+" as there are parents).

BTW, writing "-" rather than {-} in expressions is more idiomatic.

Paul.