Re: [PATCH v4, ping] gitk: let you easily specify lines of context in diff view

From: Paul Mackerras <paulus@samba.org>
Date: Aug 12, 2007, 03:17 UTC
Message-ID: <18110.31705.747583.463206@cargo.ozlabs.ibm.com>
In-Reply-To: <11868462503722-git-send-email-prohaska@zib.de>

Steffen Prohaska writes:

> Any chance to get this patch applied? It works for me.

Some comments:

Show 5 quoted lines

> @@ -731,7 +732,16 @@ proc makewindow {} {
>  	-command changediffdisp -variable diffelide -value {0 1}
>      radiobutton .bleft.mid.new -text "New version" \
>  	-command changediffdisp -variable diffelide -value {1 0}
> -    pack .bleft.mid.diff .bleft.mid.old .bleft.mid.new -side left

Just add another pack command rather than extending this one.

> +    label .bleft.mid.labeldiffcontext -text "      Lines of context: " \
> +    -font $uifont

This is hard to read because the continuation line isn't indented further that the first line. Please indent continuation lines by an extra 4 spaces.

Show 12 quoted lines

> +# empty strings or integers accepted
> +proc diffcontextvalidate {v} {
> +    if {[string length $v] == 0} {
> +	return 1
> +    }
> +    if {[string is integer $v]} {
> +	if {$v > 0} {
> +	    return 1
> +	}
> +    }
> +    return 0
> +}

"string is integer" will already accept the null string and return 1.

Show 5 quoted lines

> +proc diffcontextchange {n1 n2 op} {
> +    global diffcontextstring diffcontext
> +
> +    if {[string is integer $diffcontextstring]} {
> +        if {$diffcontextstring > 0} {

Once again, "string is integer" returning 1 doesn't guarantee the string is non-empty. Use "string is integer -strict" if you want that.

> +            set diffcontext $diffcontextstring
> +		    reselectline

Inconsistent indentation.

> +set diffcontext 3
> +

It would be nice to save diffcontext in the ~/.gitkrc.

Paul.