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

[Junio C Hamano <gitster@pobox.com>]

Mark Levedahl <mlevedahl@gmail.com> writes:

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

I missed this hunk in your updated get_explorer in [2/4]

 proc get_explorer {} {
-	if {[is_Cygwin] || [is_Windows]} {
+	if {[is_Windows]} {
 		set explorer "explorer.exe"
 	} elseif {[is_MacOSX]} {
 		set explorer "open"

and saw only this in [3/4]

 proc get_explorer {} {
 	if {[is_Windows]} {
 		set explorer "explorer.exe"
+	} elseif {[is_Cygwin]} {
+		set explorer "/bin/cygstart.exe --explore"
 	} elseif {[is_MacOSX]} {
 		set explorer "open"
 	} else {

As I missed the earlier change, the latter one alone looked to me
that for get_explorer to be share with GfW, the only explanation was
that is_Windows yields true on Cygwin, in which case the new elseif
did not make sense.

I think the hunk in [2/4] should be removed; it is confusing, it
does not have anything to do with the theme of [2/4], which is to
"remove obsolete Cygwin specific code".  And instead [3/4] should
be updated to do

+	if {[is_Cygwin] || [is_Windows]} {
-	if {[is_Windows]} {
		... do windows thing ...
+	} elseif {[is_Cygwin]} {
+		... do Cygwin thing ...
	} elseif {[is_MacOSX]} {
		... do macOS thing ...

The earlier explanation in the cover letter says this:

    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.

If the change for get_explorer is updated to read like so, then "was
shared with GfW, now we have one that is for Cygwin" starts making
sense for the file browsing.

But I still do not understand the issue with desktop icon, though.
do_windows_shortcut and do_cygwin_shortcut were separate proc before
this series---while I fully believe that do_cygwin_shortcut did not
work on modern Cygwin if you say so, and "uses Windows pathnames"
may be what makes the original implementation not work on modern
Cygwin, I do not see how the existing code for the desktop icon "is
shared with GfW".

Ah, this is again due to the suboptimal splitting of the patches.

The original does have do_cygwin_shortcut, but you remove it in step
[2/4], together with its caller.  The code before your series did
have its own do_cygwin_shortcut, but after [2/4] it and its caller
are removed. The code may not have worked before step [2/4], so it
is probably alright in the end, but it does make step [4/4] very
confusing.  Since [4/4] does need to add Cygwin specific code,
perhaps you should exclude the shortcut related change from [2/4]
and keep it focused on removing Cygwin specific code that will not
be used in the end (instead of getting fixed to keep it alive).

So, earlier I said [2/4] made sense and obviously good.  But not
anymore.  It does a bit too many things and then have later steps
compensate for it, which made reviewing the series harder than
necessary.  It needs to be cleaned up a bit, I think.

Thanks.