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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 17, 2008, 07:42 UTC
Message-ID: <7v63vldcfm.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <279b37b20803161950q73853290w864eda69896efb9a@mail.gmail.com>

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

Show 5 quoted lines

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

Show 6 quoted lines

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

Show 16 quoted lines

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