From: Pat Thoyts <patthoyts@users.sourceforge.net>
Date: Fri, 30 Jul 2010 00:22:05 GMT
Subject: Re: [PATCH] git-gui: use textconv filter for diff and blame
Message-ID: <871valstsi.fsf@fox.patthoyts.tk>
In-Reply-To: <1280319830-20483-1-git-send-email-Matthieu.Moy@imag.fr>

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

>From: Clément Poulain <clement.poulain@ensimag.imag.fr>
>
>Create a checkbox "Use Textconv For Diffs and Blame" in git-gui options.
>If checked and if the driver for the concerned file exists, git-gui calls diff
>and blame with --textconv option
>
>Signed-off-by: Clément Poulain <clement.poulain@ensimag.imag.fr>
>Signed-off-by: Diane Gasselin <diane.gasselin@ensimag.imag.fr>
>Signed-off-by: Axel Bonnet <axel.bonnet@ensimag.imag.fr>
>Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>---
>
>This patch was originally written by Clément Poulain (a student of
>mine), it was not mergeable at the time it was sent, since it relied
>on a patch serie introducing "git cat-file --textconv". Now that this
>cat-file --textconv is in a git release (1.7.2), I guess it's time to
>get this merged into git-gui.
>
>I did review and test the patch, but I'm mostly useless in TCL, so I
>may have missed the obvious. That said, the patch is relatively simple
>and looks OK.

This looks generally fine but can you suggest some test that I can use
to ensure it is actually doing something. I tried committing some test
files containing cyrillic characters but I see no difference between
using an unpatched git-gui and your patched version with git 1.7.2

>diff --git a/git-gui/lib/blame.tcl b/git-gui/lib/blame.tcl
>index 786b50b..ead68fd 100644
>--- a/git-gui/lib/blame.tcl
>+++ b/git-gui/lib/blame.tcl
>@@ -449,11 +449,28 @@ method _load {jump} {
> 
> 	$status show [mc "Reading %s..." "$commit:[escape_path $path]"]
> 	$w_path conf -text [escape_path $path]
>+
>+	set do_textconv 0
>+	if {![is_config_false gui.textconv] && [git-version >= 1.7.2]} {
>+		set filter [gitattr $path diff set]
>+		set textconv [get_config [join [list diff $filter textconv] .]]
>+		if {$filter ne {set} && $textconv ne {}} {
>+			set do_textconv 1
>+		}
>+	}
> 	if {$commit eq {}} {
>-		set fd [open $path r]
>+		if {$do_textconv ne 0} {
>+			set fd [open "|$textconv $path" r]

This is better written as
  set fd [open |[list $textconv $path] r]
in case there are spaces in either of the two components.

>+		} else {
>+			set fd [open $path r]
>+		}
> 		fconfigure $fd -eofchar {}
> 	} else {
>-		set fd [git_read cat-file blob "$commit:$path"]
>+		if {$do_textconv ne 0} {
>+			set fd [git_read cat-file --textconv "$commit:$path"]
>+		} else {
>+			set fd [git_read cat-file blob "$commit:$path"]
>+		}
> 	}
> 	fconfigure $fd \
> 		-blocking 0 \