{"thread":{"id":"15722","subject":"[PATCH (GITK) v2 0/4] Encoding support in GUI","startedAt":"2008-09-30T11:00:18Z","lastAt":"2008-10-11T12:03:34Z","messageCount":12,"participants":["Alexander Gavrilov","Paul Mackerras"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"91924","messageId":"1222772422-28020-1-git-send-email-angavrilov@gmail.com","threadId":"15722","inReplyTo":null,"subject":"[PATCH (GITK) v2 0/4] Encoding support in GUI","fromName":"Alexander Gavrilov","fromEmail":"angavrilov@gmail.com","sentAt":"2008-09-30T11:00:18Z","receivedAt":"2008-09-30T11:00:18Z","isPatch":true,"sender":{"key":"angavrilov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42666?v=4"},"body":"Currently GUI tools don't provide enough support for\nviewing files that contain non-ASCII characters. This set of\npatches addresses this issue. The git-gui part of the series\nis based on patches that were in the 'pu' branch of the\ngit-gui repository.\n\nUPDATE: the git-gui part of the series is in master.\n\nFile encoding can be specified in the following ways:\n\n1) It defaults to the current locale encoding.\n2) It can be overridden by setting the gui.encoding option.\n3) It can be further set on per-file basis by specifying\n   the 'encoding' attribute in gitattributes.\n4) Finally, git-gui allows directly selecting encodings\n   through a popup menu.\n\nSince git apparently cannot work with filenames in non-locale\nencodings anyway, I did not try to do anything about it apart\nfrom fixing some obvious bugs.\n\n\nCHANGES:\n\n- Wrote better comments for the first three patches.\n- Added a patch to speed up loading of very large diffs.\n\n\nGITK:\n\tgitk: Port new encoding logic from git-gui.\n\t---\n\tgitk |   50 +++++++++++++++++++++++++++++++++++++++++++++++---\n\t1 files changed, 47 insertions(+), 3 deletions(-)\n\n\tgitk: Implement file contents encoding support.\n\t---\n\tgitk |   19 ++++++++++++++++---\n\t1 files changed, 16 insertions(+), 3 deletions(-)\n\n\tgitk: Support filenames in the locale encoding.\n\t---\n\tgitk |   16 +++++++++++-----\n\t1 files changed, 11 insertions(+), 5 deletions(-)\n\n\tgitk: Implement batch lookup and caching of encoding attrs.\n\t---\n\tgitk |   35 ++++++++++++++++++++++++++++++++++-\n\t1 files changed, 34 insertions(+), 1 deletions(-)\n"},{"id":"91925","messageId":"1222772422-28020-2-git-send-email-angavrilov@gmail.com","threadId":"15722","inReplyTo":"1222772422-28020-1-git-send-email-angavrilov@gmail.com","subject":"[PATCH (GITK) v2 1/4] gitk: Port new encoding logic from git-gui.","fromName":"Alexander Gavrilov","fromEmail":"angavrilov@gmail.com","sentAt":"2008-09-30T11:00:19Z","receivedAt":"2008-09-30T11:00:19Z","isPatch":true,"sender":{"key":"angavrilov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42666?v=4"},"body":"Recently handling of file encodings in GIT-GUI has\nbeen greatly enhanced. It now follows these rules:\n\n- File encoding defaults to the system encoding.\n- It can be overridden by setting the gui.encoding option.\n- Finally, the 'encoding' attribute is checked on\n  per-file basis; it has the last word.\n\nThis patch inserts copies of git-gui functions necessary\nfor supporting this into gitk. The following patch will\nconnect them to the rest of the code.\n\nNote: Since git-check-attr does not provide support for\nreading attributes from trees, attribute lookup is done\nusing files from the working directory.\n\nIt also extends the range of supported encoding\nnames, to match changes in git-gui.\n\nSigned-off-by: Alexander Gavrilov <angavrilov@gmail.com>\nTested-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n gitk |   50 +++++++++++++++++++++++++++++++++++++++++++++++---\n 1 files changed, 47 insertions(+), 3 deletions(-)\n\ndiff --git a/gitk b/gitk\nindex 546627f..b210f79 100755\n--- a/gitk\n+++ b/gitk\n@@ -9726,7 +9726,7 @@ set encoding_aliases {\n     { ISO-8859-16 iso-ir-226 ISO_8859-16:2001 ISO_8859-16 latin10 l10 }\n     { GBK CP936 MS936 windows-936 }\n     { JIS_Encoding csJISEncoding }\n-    { Shift_JIS MS_Kanji csShiftJIS }\n+    { Shift_JIS MS_Kanji csShiftJIS ShiftJIS Shift-JIS }\n     { Extended_UNIX_Code_Packed_Format_for_Japanese csEUCPkdFmtJapanese\n       EUC-JP }\n     { Extended_UNIX_Code_Fixed_Width_for_Japanese csEUCFixWidJapanese }\n@@ -9768,7 +9768,7 @@ proc tcl_encoding {enc} {\n     set i [lsearch -exact $lcnames $enc]\n     if {$i < 0} {\n \t# look for \"isonnn\" instead of \"iso-nnn\" or \"iso_nnn\"\n-\tif {[regsub {^iso[-_]} $enc iso encx]} {\n+\tif {[regsub {^(iso|cp|ibm|jis)[-_]} $enc {\\1} encx]} {\n \t    set i [lsearch -exact $lcnames $encx]\n \t}\n     }\n@@ -9780,7 +9780,7 @@ proc tcl_encoding {enc} {\n \t    foreach e $ll {\n \t\tset i [lsearch -exact $lcnames $e]\n \t\tif {$i < 0} {\n-\t\t    if {[regsub {^iso[-_]} $e iso ex]} {\n+\t\t    if {[regsub {^(iso|cp|ibm|jis)[-_]} $e {\\1} ex]} {\n \t\t\tset i [lsearch -exact $lcnames $ex]\n \t\t    }\n \t\t}\n@@ -9795,6 +9795,45 @@ proc tcl_encoding {enc} {\n     return {}\n }\n \n+proc gitattr {path attr default} {\n+\tif {[catch {set r [exec git check-attr $attr -- $path]}]} {\n+\t\tset r unspecified\n+\t} else {\n+\t\tset r [join [lrange [split $r :] 2 end] :]\n+\t\tregsub {^ } $r {} r\n+\t}\n+\tif {$r eq {unspecified}} {\n+\t\treturn $default\n+\t}\n+\treturn $r\n+}\n+\n+proc get_path_encoding {path} {\n+\tglobal gui_encoding\n+\tset tcl_enc [tcl_encoding $gui_encoding]\n+\tif {$tcl_enc eq {}} {\n+\t\tset tcl_enc [encoding system]\n+\t}\n+\tif {$path ne {}} {\n+\t\tset enc2 [tcl_encoding [gitattr $path encoding $tcl_enc]]\n+\t\tif {$enc2 ne {}} {\n+\t\t\tset tcl_enc $enc2\n+\t\t}\n+\t}\n+\treturn $tcl_enc\n+}\n+\n+proc get_cached_encoding {path} {\n+\tglobal path_encoding_cache\n+\tif {[info exists path_encoding_cache($path)]} {\n+\t\treturn $path_encoding_cache($path)\n+\t} else {\n+\t\tset enc [get_path_encoding $path]\n+\t\tset path_encoding_cache($path) $enc\n+\t\treturn $enc\n+\t}\n+}\n+\n # First check that Tcl/Tk is recent enough\n if {[catch {package require Tk 8.4} err]} {\n     show_error {} . [mc \"Sorry, gitk cannot run with this version of Tcl/Tk.\\n\\\n@@ -9817,6 +9856,11 @@ if {$tclencoding == {}} {\n     puts stderr \"Warning: encoding $gitencoding is not supported by Tcl/Tk\"\n }\n \n+set gui_encoding [encoding system]\n+catch {\n+\tset gui_encoding [exec git config --get gui.encoding]\n+}\n+\n set mainfont {Helvetica 9}\n set textfont {Courier 9}\n set uifont {Helvetica 9 bold}\n-- \n1.6.0.20.g6148bc\n"},{"id":"91927","messageId":"1222772422-28020-3-git-send-email-angavrilov@gmail.com","threadId":"15722","inReplyTo":"1222772422-28020-2-git-send-email-angavrilov@gmail.com","subject":"[PATCH (GITK) v2 2/4] gitk: Implement file contents encoding support.","fromName":"Alexander Gavrilov","fromEmail":"angavrilov@gmail.com","sentAt":"2008-09-30T11:00:20Z","receivedAt":"2008-09-30T11:00:20Z","isPatch":true,"sender":{"key":"angavrilov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42666?v=4"},"body":"The previous patch has added functions that implement\nper-file encoding selection logic. This one modifies\ncore gitk code to use them.\n\nSince diffs usually contain multiple files, which may\nhave different values of the encoding attribute, they\nare initially read as binary, and encoding conversion\nis applied to individual lines, based on the active\nset of diff headers.\n\nSigned-off-by: Alexander Gavrilov <angavrilov@gmail.com>\nTested-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n gitk |   19 ++++++++++++++++---\n 1 files changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/gitk b/gitk\nindex b210f79..98923e7 100755\n--- a/gitk\n+++ b/gitk\n@@ -6295,7 +6295,7 @@ proc showfile {f} {\n \t    return\n \t}\n     }\n-    fconfigure $bf -blocking 0\n+    fconfigure $bf -blocking 0 -encoding [get_cached_encoding $f]\n     filerun $bf [list getblobline $bf $diffids]\n     $ctext config -state normal\n     clear_ctext $commentend\n@@ -6333,6 +6333,7 @@ proc mergediff {id} {\n     global diffids\n     global parents\n     global diffcontext\n+    global diffencoding\n     global limitdiffs vfilelimit curview\n \n     set diffmergeid $id\n@@ -6346,9 +6347,10 @@ proc mergediff {id} {\n \terror_popup \"[mc \"Error getting merge diffs:\"] $err\"\n \treturn\n     }\n-    fconfigure $mdf -blocking 0\n+    fconfigure $mdf -blocking 0 -encoding binary\n     set mdifffd($id) $mdf\n     set np [llength $parents($curview,$id)]\n+    set diffencoding [get_cached_encoding {}]\n     settabs $np\n     filerun $mdf [list getmergediffline $mdf $id $np]\n }\n@@ -6356,6 +6358,7 @@ proc mergediff {id} {\n proc getmergediffline {mdf id np} {\n     global diffmergeid ctext cflist mergemax\n     global difffilestart mdifffd\n+    global diffencoding\n \n     $ctext conf -state normal\n     set nr 0\n@@ -6371,14 +6374,17 @@ proc getmergediffline {mdf id np} {\n \t    set here [$ctext index \"end - 1c\"]\n \t    lappend difffilestart $here\n \t    add_flist [list $fname]\n+\t    set diffencoding [get_cached_encoding $fname]\n \t    set l [expr {(78 - [string length $fname]) / 2}]\n \t    set pad [string range \"----------------------------------------\" 1 $l]\n \t    $ctext insert end \"$pad $fname $pad\\n\" filesep\n \t} elseif {[regexp {^@@} $line]} {\n+\t    set line [encoding convertfrom $diffencoding $line]\n \t    $ctext insert end \"$line\\n\" hunksep\n \t} elseif {[regexp {^[0-9a-f]{40}$} $line] || [regexp {^index} $line]} {\n \t    # do nothing\n \t} else {\n+\t    set line [encoding convertfrom $diffencoding $line]\n \t    # parse the prefix - one ' ', '-' or '+' for each parent\n \t    set spaces {}\n \t    set minuses {}\n@@ -6586,6 +6592,7 @@ proc getblobdiffs {ids} {\n     global diffcontext\n     global ignorespace\n     global limitdiffs vfilelimit curview\n+    global diffencoding\n \n     set cmd [diffcmd $ids \"-p -C --no-commit-id -U$diffcontext\"]\n     if {$ignorespace} {\n@@ -6599,7 +6606,8 @@ proc getblobdiffs {ids} {\n \treturn\n     }\n     set diffinhdr 0\n-    fconfigure $bdf -blocking 0\n+    set diffencoding [get_cached_encoding {}]\n+    fconfigure $bdf -blocking 0 -encoding binary\n     set blobdifffd($ids) $bdf\n     filerun $bdf [list getblobdiffline $bdf $diffids]\n }\n@@ -6633,6 +6641,7 @@ proc getblobdiffline {bdf ids} {\n     global diffids blobdifffd ctext curdiffstart\n     global diffnexthead diffnextnote difffilestart\n     global diffinhdr treediffs\n+    global diffencoding\n \n     set nr 0\n     $ctext conf -state normal\n@@ -6670,10 +6679,12 @@ proc getblobdiffline {bdf ids} {\n \t    } else {\n \t\tset fname [string range $line 2 [expr {$i - 1}]]\n \t    }\n+\t    set diffencoding [get_cached_encoding $fname]\n \t    makediffhdr $fname $ids\n \n \t} elseif {[regexp {^@@ -([0-9]+)(,[0-9]+)? \\+([0-9]+)(,[0-9]+)? @@(.*)} \\\n \t\t       $line match f1l f1c f2l f2c rest]} {\n+\t    set line [encoding convertfrom $diffencoding $line]\n \t    $ctext insert end \"$line\\n\" hunksep\n \t    set diffinhdr 0\n \n@@ -6693,6 +6704,7 @@ proc getblobdiffline {bdf ids} {\n \t\tif {[string index $fname 0] eq \"\\\"\"} {\n \t\t    set fname [lindex $fname 0]\n \t\t}\n+\t\tset diffencoding [get_cached_encoding $fname]\n \t\tmakediffhdr $fname $ids\n \t    } elseif {[string compare -length 3 $line \"---\"] == 0} {\n \t\t# do nothing\n@@ -6704,6 +6716,7 @@ proc getblobdiffline {bdf ids} {\n \t    $ctext insert end \"$line\\n\" filesep\n \n \t} else {\n+\t    set line [encoding convertfrom $diffencoding $line]\n \t    set x [string range $line 0 0]\n \t    if {$x == \"-\" || $x == \"+\"} {\n \t\tset tag [expr {$x == \"+\"}]\n-- \n1.6.0.20.g6148bc\n"},{"id":"91926","messageId":"1222772422-28020-4-git-send-email-angavrilov@gmail.com","threadId":"15722","inReplyTo":"1222772422-28020-3-git-send-email-angavrilov@gmail.com","subject":"[PATCH (GITK) v2 3/4] gitk: Support filenames in the locale encoding.","fromName":"Alexander Gavrilov","fromEmail":"angavrilov@gmail.com","sentAt":"2008-09-30T11:00:21Z","receivedAt":"2008-09-30T11:00:21Z","isPatch":true,"sender":{"key":"angavrilov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42666?v=4"},"body":"The previous patch changed the encoding used for\nreading diffs to binary, which broke non-ASCII filename\nsupport. This one fixes the breakage, together with some\nexisting bugs related to filename encoding.\n\nSince core git apparently assumes that filenames are\nspecified in the locale encoding, this is what we should\nuse for them in gitk as well.\n\nSigned-off-by: Alexander Gavrilov <angavrilov@gmail.com>\nTested-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n gitk |   16 +++++++++++-----\n 1 files changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/gitk b/gitk\nindex 98923e7..254faa1 100755\n--- a/gitk\n+++ b/gitk\n@@ -6228,7 +6228,7 @@ proc gettree {id} {\n \t    set treepending $id\n \t    set treefilelist($id) {}\n \t    set treeidlist($id) {}\n-\t    fconfigure $gtf -blocking 0\n+\t    fconfigure $gtf -blocking 0 -encoding binary\n \t    filerun $gtf [list gettreeline $gtf $id]\n \t}\n     } else {\n@@ -6250,11 +6250,12 @@ proc gettreeline {gtf id} {\n \t    set line [string range $line 0 [expr {$i-1}]]\n \t    if {$diffids ne $nullid2 && [lindex $line 1] ne \"blob\"} continue\n \t    set sha1 [lindex $line 2]\n-\t    if {[string index $fname 0] eq \"\\\"\"} {\n-\t\tset fname [lindex $fname 0]\n-\t    }\n \t    lappend treeidlist($id) $sha1\n \t}\n+\tif {[string index $fname 0] eq \"\\\"\"} {\n+\t    set fname [lindex $fname 0]\n+\t}\n+\tset fname [encoding convertfrom $fname]\n \tlappend treefilelist($id) $fname\n     }\n     if {![eof $gtf]} {\n@@ -6370,6 +6371,7 @@ proc getmergediffline {mdf id np} {\n \t}\n \tif {[regexp {^diff --cc (.*)} $line match fname]} {\n \t    # start of a new file\n+\t    set fname [encoding convertfrom $fname]\n \t    $ctext insert end \"\\n\"\n \t    set here [$ctext index \"end - 1c\"]\n \t    lappend difffilestart $here\n@@ -6519,7 +6521,7 @@ proc gettreediffs {ids} {\n \n     set treepending $ids\n     set treediff {}\n-    fconfigure $gdtf -blocking 0\n+    fconfigure $gdtf -blocking 0 -encoding binary\n     filerun $gdtf [list gettreediffline $gdtf $ids]\n }\n \n@@ -6535,6 +6537,7 @@ proc gettreediffline {gdtf ids} {\n \t    if {[string index $file 0] eq \"\\\"\"} {\n \t\tset file [lindex $file 0]\n \t    }\n+\t    set file [encoding convertfrom $file]\n \t    lappend treediff $file\n \t}\n     }\n@@ -6679,6 +6682,7 @@ proc getblobdiffline {bdf ids} {\n \t    } else {\n \t\tset fname [string range $line 2 [expr {$i - 1}]]\n \t    }\n+\t    set fname [encoding convertfrom $fname]\n \t    set diffencoding [get_cached_encoding $fname]\n \t    makediffhdr $fname $ids\n \n@@ -6694,6 +6698,7 @@ proc getblobdiffline {bdf ids} {\n \t\tif {[string index $fname 0] eq \"\\\"\"} {\n \t\t    set fname [lindex $fname 0]\n \t\t}\n+\t\tset fname [encoding convertfrom $fname]\n \t\tset i [lsearch -exact $treediffs($ids) $fname]\n \t\tif {$i >= 0} {\n \t\t    setinlist difffilestart $i $curdiffstart\n@@ -6704,6 +6709,7 @@ proc getblobdiffline {bdf ids} {\n \t\tif {[string index $fname 0] eq \"\\\"\"} {\n \t\t    set fname [lindex $fname 0]\n \t\t}\n+\t\tset fname [encoding convertfrom $fname]\n \t\tset diffencoding [get_cached_encoding $fname]\n \t\tmakediffhdr $fname $ids\n \t    } elseif {[string compare -length 3 $line \"---\"] == 0} {\n-- \n1.6.0.20.g6148bc\n"},{"id":"91928","messageId":"1222772422-28020-5-git-send-email-angavrilov@gmail.com","threadId":"15722","inReplyTo":"1222772422-28020-4-git-send-email-angavrilov@gmail.com","subject":"[PATCH (GITK) v2 4/4] gitk: Implement batch lookup and caching of encoding attrs.","fromName":"Alexander Gavrilov","fromEmail":"angavrilov@gmail.com","sentAt":"2008-09-30T11:00:22Z","receivedAt":"2008-09-30T11:00:22Z","isPatch":true,"sender":{"key":"angavrilov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42666?v=4"},"body":"When the diff contains thousands of files, calling git-check-attr\nonce per file is very slow. With this patch gitk does attribute\nlookup in batches of 30 files while reading the diff file list,\nwhich leads to a very noticeable speedup.\n\nIt may be possible to reimplement this even more efficiently,\nif git-check-attr is modified to support a --stdin-paths option.\nAdditionally, it should quote the ':' character in file paths,\nor provide a more robust way of column separation.\n\nSigned-off-by: Alexander Gavrilov <angavrilov@gmail.com>\nTested-by: Johannes Sixt <johannes.sixt@telecom.at>\n---\n gitk |   35 ++++++++++++++++++++++++++++++++++-\n 1 files changed, 34 insertions(+), 1 deletions(-)\n\ndiff --git a/gitk b/gitk\nindex 1355aa2..cf557c3 100755\n--- a/gitk\n+++ b/gitk\n@@ -6530,6 +6530,7 @@ proc gettreediffline {gdtf ids} {\n     global cmitmode vfilelimit curview limitdiffs\n \n     set nr 0\n+    set sublist {}\n     while {[incr nr] <= 1000 && [gets $gdtf line] >= 0} {\n \tset i [string first \"\\t\" $line]\n \tif {$i >= 0} {\n@@ -6539,8 +6540,10 @@ proc gettreediffline {gdtf ids} {\n \t    }\n \t    set file [encoding convertfrom $file]\n \t    lappend treediff $file\n+\t    lappend sublist $file\n \t}\n     }\n+    cache_gitattr encoding $sublist\n     if {![eof $gdtf]} {\n \treturn [expr {$nr >= 1000? 2: 1}]\n     }\n@@ -9839,18 +9842,48 @@ proc tcl_encoding {enc} {\n }\n \n proc gitattr {path attr default} {\n-\tif {[catch {set r [exec git check-attr $attr -- $path]}]} {\n+\tglobal path_attr_cache\n+\tif {[info exists path_attr_cache($attr,$path)]} {\n+\t\tset r $path_attr_cache($attr,$path)\n+\t} elseif {[catch {set r [exec git check-attr $attr -- $path]}]} {\n \t\tset r unspecified\n \t} else {\n \t\tset r [join [lrange [split $r :] 2 end] :]\n \t\tregsub {^ } $r {} r\n \t}\n+\tset path_attr_cache($attr,$path) $r\n \tif {$r eq {unspecified}} {\n \t\treturn $default\n \t}\n \treturn $r\n }\n \n+proc cache_gitattr {attr pathlist} {\n+\tglobal path_attr_cache\n+\tset newlist {}\n+\tforeach path $pathlist {\n+\t\tif {[info exists path_attr_cache($attr,$path)]} continue\n+\t\tlappend newlist $path\n+\t}\n+\twhile {$newlist ne {}} {\n+\t\tset head [lrange $newlist 0 29]\n+\t\tset newlist [lrange $newlist 30 end]\n+\t\tif {![catch {set rlist [eval exec git check-attr $attr -- $head]}]} {\n+\t\t\tforeach row [split $rlist \"\\n\"] {\n+\t\t\t\tset cols [split $row :]\n+\t\t\t\tset path [lindex $cols 0]\n+\t\t\t\tset value [join [lrange $cols 2 end] :]\n+\t\t\t\tif {[string index $path 0] eq \"\\\"\"} {\n+\t\t\t\t\tset path [encoding convertfrom [lindex $path 0]]\n+\t\t\t\t}\n+\t\t\t\tregsub {^ } $value {} value\n+\t\t\t\tset path_attr_cache($attr,$path) $value\n+\t\t\t}\n+\t\t}\n+\t\tupdate\n+\t}\n+}\n+\n proc get_path_encoding {path} {\n \tglobal gui_encoding\n \tset tcl_enc [tcl_encoding $gui_encoding]\n-- \n1.6.0.20.g6148bc\n"},{"id":"92745","messageId":"18671.15020.13951.943822@cargo.ozlabs.ibm.com","threadId":"15722","inReplyTo":"1222772422-28020-4-git-send-email-angavrilov@gmail.com","subject":"Re: [PATCH (GITK) v2 3/4] gitk: Support filenames in the locale encoding.","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2008-10-10T11:21:16Z","receivedAt":"2008-10-10T11:21:16Z","isPatch":true,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"Alexander Gavrilov writes:\n\n> The previous patch changed the encoding used for\n> reading diffs to binary, which broke non-ASCII filename\n> support. This one fixes the breakage, together with some\n> existing bugs related to filename encoding.\n\nSounds like this patch should be combined with the previous one.\n\n> @@ -6250,11 +6250,12 @@ proc gettreeline {gtf id} {\n>  \t    set line [string range $line 0 [expr {$i-1}]]\n>  \t    if {$diffids ne $nullid2 && [lindex $line 1] ne \"blob\"} continue\n>  \t    set sha1 [lindex $line 2]\n> -\t    if {[string index $fname 0] eq \"\\\"\"} {\n> -\t\tset fname [lindex $fname 0]\n> -\t    }\n>  \t    lappend treeidlist($id) $sha1\n>  \t}\n> +\tif {[string index $fname 0] eq \"\\\"\"} {\n> +\t    set fname [lindex $fname 0]\n> +\t}\n\nConcerning this part of the change, do we know whether git ls-files\nwill quote filenames containing special characters or not?\n\nPaul.\n"},{"id":"92744","messageId":"18671.15132.660369.249649@cargo.ozlabs.ibm.com","threadId":"15722","inReplyTo":"18671.15020.13951.943822@cargo.ozlabs.ibm.com","subject":"Re: [PATCH (GITK) v2 3/4] gitk: Support filenames in the locale encoding.","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2008-10-10T11:23:08Z","receivedAt":"2008-10-10T11:23:08Z","isPatch":true,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"I wrote:\n\n> > @@ -6250,11 +6250,12 @@ proc gettreeline {gtf id} {\n> >  \t    set line [string range $line 0 [expr {$i-1}]]\n> >  \t    if {$diffids ne $nullid2 && [lindex $line 1] ne \"blob\"} continue\n> >  \t    set sha1 [lindex $line 2]\n> > -\t    if {[string index $fname 0] eq \"\\\"\"} {\n> > -\t\tset fname [lindex $fname 0]\n> > -\t    }\n> >  \t    lappend treeidlist($id) $sha1\n> >  \t}\n> > +\tif {[string index $fname 0] eq \"\\\"\"} {\n> > +\t    set fname [lindex $fname 0]\n> > +\t}\n> \n> Concerning this part of the change, do we know whether git ls-files\n> will quote filenames containing special characters or not?\n\nA little experiment has convinced me that it does.  So this part looks\nOK.\n\nPaul.\n"},{"id":"92748","messageId":"18671.16658.667581.499095@cargo.ozlabs.ibm.com","threadId":"15722","inReplyTo":"1222772422-28020-5-git-send-email-angavrilov@gmail.com","subject":"Re: [PATCH (GITK) v2 4/4] gitk: Implement batch lookup and caching of encoding attrs.","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2008-10-10T11:48:34Z","receivedAt":"2008-10-10T11:48:34Z","isPatch":true,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"Alexander Gavrilov writes:\n\n> When the diff contains thousands of files, calling git-check-attr\n> once per file is very slow. With this patch gitk does attribute\n> lookup in batches of 30 files while reading the diff file list,\n> which leads to a very noticeable speedup.\n\nWhy only 30 at a time?  The logic would be simpler if cache_gitattr\njust did all the paths in one call to git check-attr, and it should be\nable to cope with 1000 paths in one call, I would think, which is the\nmost that gettreediffline will give it.\n\nAlso, I wonder why we now have two levels of caching of the encoding\nattribute.  Your patch 1/4 introduced path_encoding_cache, which was\nfine, but now we have path_attr_cache as well, which seems to me to\nserve exactly the same function since the encoding is the only\nattribute we ever ask about.  Surely we don't need both caches?\n\nEven with this batching, I am a bit concerned that adding the encoding\nsupport might make things noticeably slower for people who don't need\nany encoding support (which would be the majority, I think).  We may\nend up needing an option to turn off the checking of the encoding\nattribute.\n\nPaul.\n"},{"id":"92749","messageId":"bb6f213e0810100522v653507d6r75cc4c64b57aa459@mail.gmail.com","threadId":"15722","inReplyTo":"18671.16658.667581.499095@cargo.ozlabs.ibm.com","subject":"Re: [PATCH (GITK) v2 4/4] gitk: Implement batch lookup and caching of encoding attrs.","fromName":"Alexander Gavrilov","fromEmail":"angavrilov@gmail.com","sentAt":"2008-10-10T12:22:50Z","receivedAt":"2008-10-10T12:22:50Z","isPatch":true,"sender":{"key":"angavrilov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42666?v=4"},"body":"On Fri, Oct 10, 2008 at 3:48 PM, Paul Mackerras <paulus@samba.org> wrote:\n> Alexander Gavrilov writes:\n>\n>> When the diff contains thousands of files, calling git-check-attr\n>> once per file is very slow. With this patch gitk does attribute\n>> lookup in batches of 30 files while reading the diff file list,\n>> which leads to a very noticeable speedup.\n>\n> Why only 30 at a time?  The logic would be simpler if cache_gitattr\n> just did all the paths in one call to git check-attr, and it should be\n> able to cope with 1000 paths in one call, I would think, which is the\n> most that gettreediffline will give it.\n\nOS-enforced command-line size limit on Windows is 32K. Cramming in\n1000 paths would leave only 32 characters for each path.\n\n> Also, I wonder why we now have two levels of caching of the encoding\n> attribute.  Your patch 1/4 introduced path_encoding_cache, which was\n> fine, but now we have path_attr_cache as well, which seems to me to\n> serve exactly the same function since the encoding is the only\n> attribute we ever ask about.  Surely we don't need both caches?\n\nIf the (git-gui) patch that reimplements the tcl_encoding procedure is\napplied, we may drop the path_encoding_cache. Current implementation\nis too slow for batch lookup, especially if the encoding is actually\nnot supported, and without the cache the lookup would be done on every\nloading of a diff.\n\n> Even with this batching, I am a bit concerned that adding the encoding\n> support might make things noticeably slower for people who don't need\n> any encoding support (which would be the majority, I think).  We may\n> end up needing an option to turn off the checking of the encoding\n> attribute.\n\nI hope that most diffs don't contain thousands of files at once. And\nactual huge diffs are likely to be relatively slow to load anyway.\n\nAlexander\n"},{"id":"92778","messageId":"18671.62417.328489.317909@cargo.ozlabs.ibm.com","threadId":"15722","inReplyTo":"bb6f213e0810100522v653507d6r75cc4c64b57aa459@mail.gmail.com","subject":"Re: [PATCH (GITK) v2 4/4] gitk: Implement batch lookup and caching of encoding attrs.","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2008-10-11T00:31:13Z","receivedAt":"2008-10-11T00:31:13Z","isPatch":true,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"Alexander Gavrilov writes:\n\n> OS-enforced command-line size limit on Windows is 32K. Cramming in\n> 1000 paths would leave only 32 characters for each path.\n\nEeek.  OK.\n\n> > Also, I wonder why we now have two levels of caching of the encoding\n> > attribute.  Your patch 1/4 introduced path_encoding_cache, which was\n> > fine, but now we have path_attr_cache as well, which seems to me to\n> > serve exactly the same function since the encoding is the only\n> > attribute we ever ask about.  Surely we don't need both caches?\n> \n> If the (git-gui) patch that reimplements the tcl_encoding procedure is\n> applied, we may drop the path_encoding_cache. Current implementation\n> is too slow for batch lookup, especially if the encoding is actually\n> not supported, and without the cache the lookup would be done on every\n> loading of a diff.\n\nI was thinking more in terms of dropping the path_attr_cache actually.\n\nActually, if [tcl_encoding] is slow, then why is $gui_encoding the\nuntranslated version, so that we do [tcl_encoding $gui_encoding] on\neach call to get_path_encoding?  Why don't we do the tcl_encoding call\nonce and have $gui_encoding be the result of that?  In fact\n$gui_encoding should be the result of this code (from\nget_path_encoding):\n\n\tset tcl_enc [tcl_encoding $gui_encoding]\n\tif {$tcl_enc eq {}} {\n\t\tset tcl_enc [encoding system]\n\t}\n\nAnd if [tcl_encoding] is slow, then it should have a cache.  There's\nonly likely to be at most 2 or 3 values it gets called for, and it's\na constant function.\n\nAt this point, what I think I might do is apply your set of patches\n(but with 2/4 and 3/4 folded into a single patch) and then go through\nand do another commit that addresses the concerns I've raised.  OK?\n\nPaul.\n"},{"id":"92786","messageId":"200810111328.50951.angavrilov@gmail.com","threadId":"15722","inReplyTo":"18671.62417.328489.317909@cargo.ozlabs.ibm.com","subject":"Re: [PATCH (GITK) v2 4/4] gitk: Implement batch lookup and caching of encoding attrs.","fromName":"Alexander Gavrilov","fromEmail":"angavrilov@gmail.com","sentAt":"2008-10-11T09:28:50Z","receivedAt":"2008-10-11T09:28:50Z","isPatch":true,"sender":{"key":"angavrilov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42666?v=4"},"body":"On Saturday 11 October 2008 04:31:13 Paul Mackerras wrote:\n> > > Also, I wonder why we now have two levels of caching of the encoding\n> > > attribute.  Your patch 1/4 introduced path_encoding_cache, which was\n> > > fine, but now we have path_attr_cache as well, which seems to me to\n> > > serve exactly the same function since the encoding is the only\n> > > attribute we ever ask about.  Surely we don't need both caches?\n> > \n> > If the (git-gui) patch that reimplements the tcl_encoding procedure is\n> > applied, we may drop the path_encoding_cache. Current implementation\n> > is too slow for batch lookup, especially if the encoding is actually\n> > not supported, and without the cache the lookup would be done on every\n> > loading of a diff.\n> \n> I was thinking more in terms of dropping the path_attr_cache actually.\n\nSince gitattr is a general-purpose function that can read any attribute,\nI decided that it should use its own cache.\n\nBasically, all this double-caching issue is fallout from my failure to\nanticipate the need of batch attribute lookup from the beginning...\n\n> Actually, if [tcl_encoding] is slow, then why is $gui_encoding the\n> untranslated version, so that we do [tcl_encoding $gui_encoding] on\n> each call to get_path_encoding?  Why don't we do the tcl_encoding call\n> once and have $gui_encoding be the result of that?  In fact\n> $gui_encoding should be the result of this code (from\n> get_path_encoding):\n> \n> \tset tcl_enc [tcl_encoding $gui_encoding]\n> \tif {$tcl_enc eq {}} {\n> \t\tset tcl_enc [encoding system]\n> \t}\n\nWell, that code was copied from git-gui, where it looks like this:\n\nset tcl_enc [tcl_encoding [get_config gui.encoding]]\n\nI.e. there is no \"$gui_encoding\" variable, although get_config does use a cache.\n\n> And if [tcl_encoding] is slow, then it should have a cache.  There's\n> only likely to be at most 2 or 3 values it gets called for, and it's\n> a constant function.\n\nIn git-gui the slowdown appeared during the construction of the menu\nlisting all available encodings, so a simple cache would not have helped. \nI  reimplemented it using a lookup table to resolve aliases (constructed\non the first run). But it can be thought of as a precalculated cache.\n\n> At this point, what I think I might do is apply your set of patches\n> (but with 2/4 and 3/4 folded into a single patch) and then go through\n> and do another commit that addresses the concerns I've raised.  OK?\n\nMaybe I should resend the patches, scrapping path_encoding_cache,\nand adding the optimized version of tcl_encoding?\n\nAlexander\n"},{"id":"92796","messageId":"18672.38422.200764.101389@cargo.ozlabs.ibm.com","threadId":"15722","inReplyTo":"200810111328.50951.angavrilov@gmail.com","subject":"Re: [PATCH (GITK) v2 4/4] gitk: Implement batch lookup and caching of encoding attrs.","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2008-10-11T12:03:34Z","receivedAt":"2008-10-11T12:03:34Z","isPatch":true,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"Alexander Gavrilov writes:\n\n> > And if [tcl_encoding] is slow, then it should have a cache.  There's\n> > only likely to be at most 2 or 3 values it gets called for, and it's\n> > a constant function.\n> \n> In git-gui the slowdown appeared during the construction of the menu\n> listing all available encodings, so a simple cache would not have helped. \n> I  reimplemented it using a lookup table to resolve aliases (constructed\n> on the first run). But it can be thought of as a precalculated cache.\n\nHmmm, one that uses more time and memory than it needs to for gitk's\nuse...  I guess it's not a lot, but it still seems unnecessary, unless\nyou can see a need for a menu of encodings in gitk.\n\n> > At this point, what I think I might do is apply your set of patches\n> > (but with 2/4 and 3/4 folded into a single patch) and then go through\n> > and do another commit that addresses the concerns I've raised.  OK?\n> \n> Maybe I should resend the patches, scrapping path_encoding_cache,\n> and adding the optimized version of tcl_encoding?\n\nOK.\n\nPaul.\n"}]}