Re: [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui

[Mark Levedahl <mlevedahl@gmail.com>]

On 6/24/23 19:30, Junio C Hamano wrote:
> Mark Levedahl <mlevedahl@gmail.com> 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