Re: [PATCH] gitk: synchronize highlighting in file view for 'f'&'b' commands

[Junio C Hamano <gitster@pobox.com>]

"Eric Raible" <raible@gmail.com> writes:

> From 03603a207b2caad8b52dd4a24860e387d418f800 Mon Sep 17 00:00:00 2001
> From: Eric Raible <raible+git@gmail.com>
> Date: Sun, 16 Mar 2008 19:33:52 -0700
> Subject: [PATCH] gitk: synchronize highlighting in file view for
> 'f'&'b' commands

Please do not do this.  The first line is not part of anything but is only
a mail message boundary in mbox format.  Reproducing From: is fine if the
patch author is different from the person who is sending the patch, but I
do not think it is necessary in this case.  Date and Subject should also
go, as taking them from the e-mail header is just as good.

And please do not send gitk patches to me.  It should come through Paul
Mackerras <paulus@samba.org>, so if your patch is obviously correct for
application, then To: him with Cc: list (and I do not mind being on the
Cc:), otherwise if the patch is for review and discussion, please send it
addressed To: the list (and again I do not mind being on the Cc:).

> Previously, 'b', backspace, and delete all did the same thing.
> This changes 'b' to perform the inverse of 'f'.  And both of
> them now highlight the filename of the currently diff.
>
> This makes it easier to review the diffs associated with
> a particular commit using only f,b, and space.

As to the change itself, I think it makes sense to have a binding
available for prevfile (previously there was none), so I'd be supportive
of that keybinding change.

> +proc highlightfile {loc} {
> +    global ctext
> +    $ctext yview $loc
> +
> +    global cmitmode ctext cflist cflist_top
> +    if {$cmitmode eq "tree"} return
> +
> +    $cflist tag remove highlight $cflist_top.0 "$cflist_top.0 lineend"
> +
> +    set file [regsub -- "-* (.*?) -*" [$ctext get $loc "$loc lineend"] "\\1"]
> +    set cline [$cflist search -regexp [subst {^$file$}] 0.0]
> +
> +    $cflist tag add highlight $cline "$cline lineend"
> +    $cflist see $cline
> +    set cflist_top [lindex [split $cline .] 0]
> +}

I however think you also should describe why you had to change "$ctext
yview" in both nextfile and prevfile to a more complex "highlightfile" in
your commit message.  It is not very obvious how it is an improvement, and
typing 'b' in tree view (as opposed to patch view) seems to trigger

	Error: bad text index""

with this patch.