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

From: Paul Mackerras <paulus@samba.org>
Date: Oct 23, 2008, 11:58 UTC
Message-ID: <18688.26370.467724.404768@cargo.ozlabs.ibm.com>
In-Reply-To: <1223449540-20457-4-git-send-email-angavrilov@gmail.com>

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... :)

Show 9 quoted lines

> +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 ^@@@?.

Show 17 quoted lines

> +    # 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.