On 6/24/23 19:30, Junio C Hamano wrote:
Mark Levedahl <mlevedahl@xxxxxxxxx> writes:
git-gui has many snippets of code guarded by an is_Cygwin test, all of
which target a problematic hybrid Cygwin/Windows 8.4.1 Tcl/Tk removed in
March 2012. That is when Cygwin switched to a well-supported unix/X11
Tcl/Tk package. 64-bit Cygwin was released later so has always had the
unix/X11 package. git-gui runs as-is on more recent Cygwin, treating it
as a Linux variant, though two functions are disabled.
The old Tcl/Tk understood Windows pathnames, so git-gui's Cygwin
specific code uses Windows pathnames. The unix/X11 code requires use of
unix pathnames, so none of the Cygwin specific code is compatible, and
all should be removed.
Fortunately, the is_Cygwin funcion in git-gui (on the git master branch)
relies upon the old Tcl/Tk and doesn't detect Cygwin. But, commit
c5766eae6f2b002396b6cd4f85b62317b707174e on the git-gui master branch
"fixed" is_Cygwin, enabling the incompatible code, so upstream git-gui
is now broken on Cygwin.
Here I presume "upstream git-gui master" refers to a5005ded (Merge
branch 'ab/makeflags', 2023-01-25) sitting at 'master' in Pratyush's
https://github.com/prati0100/git-gui/ repository.
Yes.
There is Cygwin specific code in the Makefile, intended to allow a
completely unsupported configuration with a Windows TclTk. This code
misdetects the configuration, creating a non-portable installation. The
Cygwin git maintainer comments this code out. The code should be
removed.
...
patch 1 removes the obsolete Makefile code
patch 2 removes all obsolete git-gui.sh code, wrapped in is_Cygwin...
As it has been quite a while since I had access to any Windows box
or Cygwin, but the earlier two patches look obviously correct to me.
The existing code for file browsing and creating a desktop icon is
shared with Git For Windows support, and uses Windows pathnames. This
code does not work on Cygwin, and needs replacement. These functions
have not worked since 2012.
...
patch 3 implements Cygwin specific file browsing support
patch 4 implemetns Cygwin specific desktop icon support
Both of these two patches do
if {[is_Windows]} {
... do Windows thing ...
+ } elseif {[is_Cygwin]} {
+ ... do Cygwin thing ...
} elseif {[is_MacOSX]} {
... do macOS thing ...
} else {
... do it for others ...
}
which I do not quite understand how the existing code meshes with
your "is shared with Git For Windows support", though. If "is
shared with GfW" is to be trusted, on a modern Cygwin box,
"is_Windows" would be yielding true (that is the only way the "do
Windows thing" block will be entered on Cygwin box, sharing the
support with GfW. But then, adding elseif _after_ we check for
Windows would be pointless. Puzzled...
Thanks.
git-gui has three independent functions (is_Cygwin, is_Windows, and
is_MaxOSX), each determine if running on that platform, and "generic
Unix/Linux" can be considered the result if all three functions return
false. In Pratyush's tree, those three functions essentially are:
is_Cygwin: $::tcl_platform(os) startswith("CYGWIN")
is_MaxOSX: [tk windowingsystem] == "AQUA"
is_Windows: $::tcl_platform(platform) == "Windows"
It turns out, only one of the . is ever true, and none are true on
Linux. So, the if/else tree above is not confused by Windows / Cygwin.
But, different Tcl/Tk signatures as platforms evolve could cause
problems. A better design might be to just have a $HOSTTYPE variable set
once, perhaps in startup, perhaps even by the makefile, to assure
exactly one hosttype is ever active and make this clear to others.
Normal configuration checking in the makefile could have uncovered this
whole problem in 2012. But, this is a possible cleanup topic for another
day.
So, the code under the is_Windows and is_Cygwin branches of the if/else
trees are now completely independent, and the is_Windows branch is never
entered on Cygwin.
Thank you,
Mark
