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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 24, 2023, 23:30 UTC
Message-ID: <xmqq8rc8781p.fsf@gitster.g>
In-Reply-To: <20230624212347.179656-1-mlevedahl@gmail.com>

Mark Levedahl <mlevedahl@gmail.com> writes:

Show 17 quoted lines

> 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.

Show 10 quoted lines

> 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.

Show 7 quoted lines

> 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.