Updates for Tcl 8.6 and 9.0 support

[mark987]

I've been working on tcl 8.6 / 9.0 updates for a while, have read the proposed patches from @qykth-git and I believe these miss some important things, it is easier for me to just show my work including explanations in the commit messages. Key differences:

There are a number of updates to mouse scroll handling we should make for Tcl/Tk 8.6, these depend upon TIP 171, and this precedes anything for TIP 474 / Tcl 9. We should update gitk requirements to be 8.6 before doing this as 8.4/8.5 don't support TIP 171, 8.6 will likely continue in use for a decade as 9.0 just hit. My 9.0 mouse version uses the Option- modifier to select large motion (like on x11 previously) vs fine motion (line on aqua previously), similar to how Tk changes this by default on other widgets.

Tcl 9 includes TIP 699 because of much confusion about binary channels. Following that document and looking carefully at gikt, I reach a different result than 042ede8, reasoning is in the 0f4b70c commit message.

There are additional changes using -profile needed to operate on Tcl 9 to cover some encoding issues, 5d5d337 and 036fc90.

I'm successfully using gitk (and git-gui) on Tcl8.6 and on Tcl9.0.1 with this set (no longer have 8.7, but did some of the earlier work with that).

[j6t]

Thank you! The extensive explanations in the commit messages are very much appreciated. This looks already very good.

I am concerned that the horizontal scrolling of the commit graph is a regression on MacOS: before, it was active when the mouse was on any of the three panels, now the mouse must be over the commit graph panel.

I am not happy with the simple tilde substitution with $env(HOME). I do not have Git for Windows installed and have set HOME manually because I know that it is needed. How does file tildeexpand work on Windows when HOME is not set, as is common outside of Git for Windows? Does it use the HOMEDRIVE\HOMEDIR combo?

#11 changes fconfigure to chan configure. Is this not needed for Tcl 9?

BTW, the horizontal scrolling on X11 now works secretly with the mouse wheel (Shift+Alt+wheel), but it uses a very small increment here. It takes a lot of wheel turns to scroll even half the window width. Is that intended? (I haven't tested elsewhere, yet.)

[mark987]

I've updated the branch to:

• correct spelling errors and generally improve wording in commit messages
• moved settting and use of colspc variable into 432717b from its successor (now 61da1f5)
• increased the value of colspc from 5 to 25, horizontal scrolling of the commit graph is now 5x larger than before.

To @j6t on your previous questions:
horizontal scrolling of the commit graph is a regression on MacOS

• I updated comments in 432717b about this: basically, normal GUI design has the mouse pointer indicate the window to be scrolled. While vertical scolling in any of the top 3 panes makes sense, as all scroll, horizontal scrolling of the commit graph from any pane does not as ONLY the commit graph will scroll.

I am not happy with the simple tilde substitution with $env(HOME)

• What I wrote works on all the platforms I know how to test and can explain. I know the configuration of git on Windows using either Cygwin or MSYS (the latter underlies g4w). MSYS defines HOME, g4w assures this is propagated, but not of other approaches.
• Tilde expansion on Tcl 9 uses $HOME if it is defined. Tcl sets up the environment when starting, and tries to assure $HOME is defined, using $HOMEDRIVE and $HOMEDIR if $HOME is not there. But, those variables are wrong on Cygwin, might not point to what g4w thinks is $HOME, so I'm not at all sure what they mean.
• So, as long as $HOME is honored and not touched, this doesn't matter to me. I just don't understand a supportable configuration where $HOME is not defined.

#11 changes fconfigure to chan configure. Is this not needed for Tcl 9?

• The "chan" command was introduced in Tcl 8.5 per TIP 208 to consolidate many (by no means all) channel related commands into one group. A few commands, including fconfigure, were recommended to be deprecated in TIP 208, but this did not happen in 8.5 or 8.6. In 9.0, fconfigure and chan configure are the same thing, though the Tcl 9 documentation for fconfigure shows this is now deprecated. Nothing in the documentation on migrating to Tcl 9 mentions this, and fconfigure shows up in the migration instructions.
• My take is this is optional, not required. We could do a global search/replace, it makes no difference. So, I didn't do it.

horizontal scrolling on X11 now works secretly with the mouse wheel (Shift+Alt+wheel), but it uses a very small increment here.

• The colspc variable mentioned above can be increased to make this scroll more quickly, I changed it from 5 to 25, giving a 5x increase.
• We might also use $linespc instead of $colspc to set horizontal scolling increments : linespc is computed as the vertical font size, so is roughly the size of a line, this could be a reasonable value to use for horizontal scrolling as well.
• You have to use Tcl9 to see any difference when the Alt key is pressed: Tcl8 does not generate the Option- modifier.

[qykth-git]

@j6t
$env(HOME) was defined like this on Windows Tcl.
https://github.com/tcltk/tcl/blob/9f8cc0d427b6fea52116a71060c9a37a757efea4/win/tclWinInit.c#L558

Windows Tcl build own $env(HOME) with these order.

1. Use supplied $HOME
2. Build $HOME from $HOMEDRIVE and $HOMEPATH
3. Build $HOME from $USERPROFILE
4. Just use C:\\ as $HOME

[j6t]

Windows Tcl build own $env(HOME) with these order.

1. Use supplied $HOME
2. Build $HOME from $HOMEDRIVE and $HOMEPATH
3. Build $HOME from $USERPROFILE
4. Just use C:\\ as $HOME

Thank you, this matches the results of my experiments today. Hence, using $env(HOME) instead of the tilde is fine.

[j6t]

@mark987 Thank you for the update and the explanations. Using $env(HOME) for the tilde is alright. The horizontal scrolling speed is now a lot better.

The need for -profile tcl8 is scary stuff. (Who the hell writes software that expects a perfect environment, and crashes at the first imperfection, I have to ask the Tcl people?) Do we need a plan to do something similar for the exec calls that capture output from the commands?

[mark987]

@j6t - I will amend 432717b to use $linespc for vertical and horizontal increments so that this is modified by font metrics.

gitk uses exec as a pipe_open and read stdout (maybe stderr?) function, and exec has no -profile setting. git-gui's git_read is a good substitute that calls open, and we can modify open to use -profile tcl8. gitk has at least one exec call that does not invoke git, this will require similar treatment but git_read itself won't do it. My git-gui tree already has modifications to handle Tcl 9 including -profile tcl8 on all opened channels, will push that out later.

[mark987]

The use of -profile strict in Tcl is really driven by the internationalization standards (not sure exactly what spec): the tcl8 behavior is apparently non-compliant, this raises security / vulnerability issues. I won't be surprised to find this is just the start of a new campaign that will force git's permissive approach to change, possibly forcing tools to rewrite history with full encoding headers validated to be correct.

[mark987]

By experiment, it seems that [exec ...] uses the "replace" strategy. Using a simple test prog:

#include <stdio.h>
int main (int argc, char *argv[])
{
        int c = 0x70;
        while (c < 0x90) {
                for (int col = 0 ; col < 10; col++) {
                        printf("%4x %c  ", c, c);
                        ++c;
                }
                printf("\n");
        }
        return(0);
}

Compiled, ./a.out gives

  70 p    71 q    72 r    73 s    74 t    75 u    76 v    77 w    78 x    79 y  
  7a z    7b {    7c |    7d }    7e ~    7f     80 �    81 �    82 �    83 �  
  84 �    85 �    86 �    87 �    88 �    89 �    8a �    8b �    8c �    8d �  
  8e �    8f �    90 �    91 �    92 �    93 �    94 �    95 �    96 �    97 �  

My system is configured for utf-8, does not recognize bytes >= 0x80 as valid, so displays those with the defined "replacement" character �.

Running this in tclsh 9 using a pipe, thus has -profile strict, gives the expected tantrum

tclsh [~/] set fd [open [list | ./a.out] r]
file5
tclsh [~/] gets $fd
  70 p    71 q    72 r    73 s    74 t    75 u    76 v    77 w    78 x    79 y  
tclsh [~/] gets $fd
error reading "file5": invalid or incomplete multibyte or wide character

But, using exec is ok:

tclsh [~/maint/src/git-gui] set s [exec ./a.out]
  70 p    71 q    72 r    73 s    74 t    75 u    76 v    77 w    78 x    79 y  
  7a z    7b {    7c |    7d }    7e ~    7f     80 �    81 �    82 �    83 �  
  84 �    85 �    86 �    87 �    88 �    89 �    8a �    8b �    8c �    8d �  
  8e �    8f �    90 �    91 �    92 �    93 �    94 �    95 �    96 �    97 �  

Under 8.6, I get somewhat different results - no errors, but no glyph for anything >= 0x7f:

set fd [open [list | ./a.out] r]
file5
> gets $fd
  70 p    71 q    72 r    73 s    74 t    75 u    76 v    77 w    78 x    79 y  
> gets $fd
  7a z    7b {    7c |    7d }    7e ~    7f     80     81     82     83   
> gets $fd
  84     85     86     87     88     89     8a     8b     8c     8d   
> gets $fd
  8e     8f     90     92     93     94     95     96     97   
> gets $fd

> 

set s [exec ./a.out]
  70 p    71 q    72 r    73 s    74 t    75 u    76 v    77 w    78 x    79 y  
  7a z    7b {    7c |    7d }    7e ~    7f     80     81     82     83   
  84     85     86     87     88     89     8a     8b     8c     8d   
  8e     8f     90     92     93     94     95     96     97

exec on Tclsh8.6 stops after reading character 0x97, but setting the range starting at 0xa0 gives:

set s [exec ./a.out]
  a8 ¨    a9 ©    aa ª    ab «    ac ¬    ad ­    ae ®    af ¯    b0 °    b1 ±  
  b2 ²    b3 ³    b4 ´    b5 µ    b6 ¶    b7 ·    b8 ¸    b9 ¹    ba º    bb »  
  bc ¼    bd ½    be ¾    bf ¿    c0 À    c1 Á    c2 Â    c3 Ã    c4 Ä    c5 Å  
  c6 Æ    c7 Ç    c8 È    c9 É    ca Ê    cb Ë    cc Ì    cd Í    ce Î    cf Ï  
  d0 Ð    d1 Ñ    d2 Ò    d3 Ó    d4 Ô    d5 Õ    d6 Ö    d7 ×    d8 Ø    d9 Ù  
  da Ú    db Û    dc Ü    dd Ý    de Þ    df ß    e0 à    e1 á    e2 â    e3 ã  
  e4 ä    e5 å    e6 æ    e7 ç    e8 è    e9 é    ea ê    eb ë    ec ì    ed í  
  ee î    ef ï    f0 ð    f1 ñ    f2 ò    f3 ó    f4 ô    f5 õ    f6 ö    f7 ÷  
  f8 ø    f9 ù    fa ú    fb û    fc ü    fd ý    fe þ    ff ÿ 

My inference:

tcl8.6 operates as expected, transcoding (some) valid extended ascii characters via the ISO-8899-1 code page. Tcl 9.0 uses the 'replace' profile, so does not throw an exception but also gives different results than 8.6.
So, we probably should use a variant of git_read to replace set x [exec ...]

BTW, git-filter-repo, written in python, throws errors on repos with extended ascii characters as well. This is not just a tcl thing.

[mark987]

The solution actually removes lines of code, as we no longer need the Windows exec override :^) Instead, just use a pipeline from [open | ..], and take advantage that we already overwrote open to add -profile tcl8.

[j6t]

Thank you for the extensive tests.

The exec replacement that you suggest isn't something I would take. It's too scary. There could be library calls outside of Gitk that expect a certain API, but the suggested replacement isn't fully compatible.

At any rate, the important finding is that exec of neither Tcl 8.6 nor Tcl 9 fails on invalid UTF-8 input. That the two versions don't give the same result (or an "unexpected" result), is of no concern for Gitk. (After all, its a form of garbage-in-garbage-out.) Therefore, I wouldn't do anything about it at this time.

[mark987]

I agree - the patch was really a throw-away test to see if it made any difference, I didn't find one, even running with profile -strict. But, if this is needed in future, a careful analysis of any call-site and how it parses data is needed to do this correctly - I punted on that.

A more interesting question is whether -profile replace should be used rather than -profile tcl8. This would change output, but would better reflect that we don't know the correct character to show anyway.

The big danger under all of this is that some repo has a tree with extended ascii on filenames. I don't know how git handles encoding trees now, whether that has changed, etc. Certainly utf-8 was less universal when git started. Regardless, if this problem exists, it is one for git to solve, not gitk/git-gui.

[j6t]

I have now used this series in production on Windows for a day. It works as expected.

But... I notice that the mouse wheel scrolls the patch text and the file list a lot slower than before, to the point that it becomes hard work (and dangerous for the finger that operates the wheel). The reason is that we no longer override wheel scrolling for those two windows and use Tk's default binding. Whatever they recommend to use as scrolling unit, it is far too less and our values were a lot better. In my case, the old version scrolled 5 lines at a minimum (don't know exactly), while the new version scrolls only 2 lines. Opinions? Should we increase -yscrollincr for these two windows?

[mark987]

The existing mouse scrolling code is a mess: three different codes for three different platforms, each giving different scrolling behavior. To wit:

• aqua has the hidden horizontal commt graph scrolling, scrolls by one line for each event, and default Tk bindings do the same.
• win32 has mouse_wheel_redirector that gives default 5 lines vertical scrolling for each event, and does this for every widget in the main window. Horizontal scrolling is handled by any default bindings that might exist.
• x11 binds mouse buttons to get default 5 units vertical scrolling on the top 3 panes, 5 units horizontal scrolling on the text window (lower left), defaults on all else.

The "correct" scrolling value is obviously non-obvious: @j6t complains of < 5 lines per event, but aqua has 1. I suspect we need a config item scroll_lines_per_mouse_event or some such, expecting values in the range 1-10.

My patches for 8.6 do not achieve harmony as I hadn't noticed all I wrote above, and they cannot do what Tcl 9 does.

For Tcl 9, my patches try to unify everything around the new model given by Tk 9. Nominal scrolling is 1 line per event, with the Option- modifier (Alt key on PC, Option on Mac), we get about 5 lines per event, which is also the Tk default. All good.

I'm pushing a modified branch that gets win32 and x11 close at default 5 lines per event scrolling for the top 3 panes and bottom 2. I didn't touch aqua, but it should get modified to do the common thing. I'll take a stab once we agree on what we are trying to achieve for 8.6.

[mark987]

I've reworked the 8.6 scrolling, with the goal of making all platforms do the same thing (they did not come even close, it turns out, and for no good reason). I also wanted to make 8.6 and 9.0 do the same thing but failed. The problem is that 9.0 adds the Option- modifier using the Alt key on PC, and Option key on Mac, to get ~5x (or more) motion than the nominal. This key cannot be changed. But, in 8.6 and earlier, the Alt- key on PC/Windows activates the menu-bar, this cannot be changed, and makes the Alt- key as a modifier for mouse scrolling essentially unusable.

I added a mouse motion multiplier (kscroll) in the preferences dialog, default value = 1, allowed [1..20]. 1 gives essentially the old aqua motion, 5 the win32 motion, and the x11 had a mixture of those two. A value < 1 is not useful, though you can type in whatever fractional value you wish even though the spinbox only does integers.

So, in 8.6, on all platforms, the 5 main windows (3 top panes, two text widgets below) are scrolled vertically by kscroll lines (the text widgets are weird, move 2-3 lines when asked to move 1, can do nothing about that). Horizontal scrolling is available on the two left panes.

9.0 does the same, but adds the Option- modifier to get more motion. With this I set the kscroll preference to 1, use Alt- to move far. But, as I said, cannot get this for 8.6.

More lines of bindings than I wished, but this is the only way to get the uniform scrolling I had put into win32 15 years ago, and forgot all about if I even noticed it then :^).

[j6t]

Thank you for this update! The new setting makes it very easy to apply the personal preference.

BTW, should we call it "Wheel scrolling multiplier" to emphasize that it is specific to the mouse wheel?

The commit message of abb97b6 says "increase scrolling motion by a factor of 5". Is this "5" outdated and the new setting should be mentioned, or is it something else?

[mark987]

@j6t - I've incorporated your suggestion on the preference name, and clarified that the Option- modifier gets 5x the defined scrolling value having nothing to do with prior defaults.

But, i've also refactored the scrolling code. I was bothered about the number of mouse binding lines added, their replication, as well as missing details in the scrolling calculations. My new branch adds some features using a common [scroll_val ...] function

• the scroll val is integer, required on Tk 8.6, regardless of the user setting a floating point scrolling multiplier
• the scroll value adjusts to accommodate the nominal minimum 3 line scrolling of text widgets.
• the default preference is now 3, a compromise of the former behaviors on the different platforms, and also the minimum value at which the canvas and text widgets scroll by the same amount.
  Overall, fewer lines added, more understandable to me.

I don't have anymore nagging problems with this code, so won't change anything more except to address feedback. Tested on Linux/X11 with Tcl 8.6.15 and Tcl 9.0.1, Cygwin/X11 with Tcl 8.6.12, G4W/win32 with Tcl 8.6.13. I don't have Tcl 9 except on Linux, no access to aqua anywhere, so those are the big holes in testing.

[j6t]

Thank you so much, this looks very good!

[j6t]

Two small things I noticed while going over my patch list today:

• The text "Wheel scrolling multiplier" is not prepared for translation. It should be.
• We have two variables have_tk85 and have_tk86. They become obsolete with this series and all code using them can be simplified by assuming that the flags are true.

[mark987]

I've updated my branch with:

• scrubbed commit messages
• added [mc "Wheel scrolling multiplier"]
• changed mouse_wheel_scale to scroll_D0, as this is expected value of %D passed into the event
• moved the commit allowing Tcl9 until after all Tcl9 compatibility is added.
• added a number of additional commits removing compatibility with pre 8.6 Tcl/Tk and early git.

I was holding the latter patches back until the dust settles on the first, but, you asked :^) The first one, a3a7e5e, removes have_tk85 and have_tk86. I found much more to do, though. I've been running with all of these for a while.

[j6t]

Thank you for the update.

Since I have a few topics in flight that change the user-interface, I would prefer to postpone the ttk changes until those topics are merged.

Regarding the requirement of a modern git, I of course agree that we remove compatibility for ancient versions. I would nevertheless target Debian Buster as the minimum, which has Git 2.20. Looking at 8c7307e, I don't think we must require 2.36.

[mark987]

The issue on git version traces to git-gui, gitk launches git-gui, so gitk logically needs git supporting gitk AND git-gui.
git added core.hooksPath in v2.10, and this broke git-gui until I patched git-gui for Junio's git 2.43 release. This relies upon git-hook-run added in v2.36. So, git-gui post 2.43 is compatible with git v2.36 and later, earlier git-gui is compatible only up to git 2.09. git 2.10 --> 2.35 is not compatible as hooks do not run (or the wrong hooks run, this got reported to me by a former colleague causing me to pursue fixing the problem).

So, git 2.20 in Debian Buster is in "no-man's" land vis-a-vis git-gui. For reasons above, I have trouble saying gitk is ok.

[j6t]

But as long as Git GUI isn't used, Gitk can work just fine with a Git version before 2.36. On top of that, if the Git GUI is a version before 2.43, then it still works (it just doesn't obey core.hooksPath). So, 2.36 isn't a hard requirement for Gitk.