| Message ID | 20230624212347.179656-1-mlevedahl@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Remove obsolete Cygwin support from git-gui | expand |
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. > 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.
Junio C Hamano <gitster@pobox.com> writes: >> 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. Ehh, in an early draft, I had "I cannot comment on patches #3 and #4" before that "but", but I ended up commenting on them anyway, and ended up with such a garbled construction. I should have copyedited the above to "Even though it has been ... or Cygwin, the earlier...". Sorry for the noise.
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
On 6/24/23 19:35, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >>> 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. > Ehh, in an early draft, I had "I cannot comment on patches #3 and > #4" before that "but", but I ended up commenting on them anyway, and > ended up with such a garbled construction. I should have copyedited > the above to "Even though it has been ... or Cygwin, the earlier...". > > Sorry for the noise. No problem, I appreciate the review. The last two patches absolutely need the Cygwin / G4W folks to review. Mark
On 6/25/23 07:26, Mark Levedahl wrote: > > On 6/24/23 19:30, Junio C Hamano wrote: 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 > A follow up - I have Cygwin in a Windows VM on my laptop, no G4W, no Mac ... Cygwin gives: $::tcl_platform(os) = CYGWIN_NT-10.0-22621 $::tcl_platform(platform) = unix tk windowingsystem = x11 Linux gives $::tcl_platform(os) = Linux $::tcl_platform(platform) = unix tk windowingsystem = x11 So, neither Cygwin nor Linux trigger the checks for is_Windows or is_MacOSX
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.
On 6/25/23 11:46, Junio C Hamano wrote: > 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. > > 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. > I had originally organized as you suggest, no problem doing so again. What gave me pause was this paragraph I originally wrote for the cover letter: Patches 1/2 cause git-gui to function as it has for the last decade on Cygwin, but with Cygwin being detected. However, the browsing and shortcut creation menu items, removed in 2012 then re-added when is_Cygwin was fixed, do not work, and shortcut creation will crash git-gui if used. These are fixed in patches 3 / 4. So, I'm just checking that the above situation is ok. I agree, this makes the changes easier to follow. Mark
Mark Levedahl <mlevedahl@gmail.com> writes: > I had originally organized as you suggest, no problem doing so > again. What gave me pause was this paragraph I originally wrote for > the cover letter: > > Patches 1/2 cause git-gui to function as it has for the last decade on > Cygwin, but with Cygwin being detected. However, the browsing and > shortcut creation menu items, removed in 2012 then re-added when is_Cygwin > was fixed, do not work, and shortcut creation will crash git-gui if used. > These are fixed in patches 3 / 4. As you are removing (ancient) Cygwin specific code that did not work with modern Cygwin at all in step [2/4], it is not so unexpected that some stuff does still not work after that step. I am not sure what your reservation exactly is, but if you are wondering if code to disable browsing and shortcut creation on Cygwin temporarily needs to be there in the same step (instead of crashing or not working), it may make sense if and only if it is done with minimal changes. Thanks.
On 6/26/23 11:52, Junio C Hamano wrote: > Mark Levedahl <mlevedahl@gmail.com> writes: > >> I had originally organized as you suggest, no problem doing so >> again. What gave me pause was this paragraph I originally wrote for >> the cover letter: >> >> Patches 1/2 cause git-gui to function as it has for the last decade on >> Cygwin, but with Cygwin being detected. However, the browsing and >> shortcut creation menu items, removed in 2012 then re-added when is_Cygwin >> was fixed, do not work, and shortcut creation will crash git-gui if used. >> These are fixed in patches 3 / 4. > As you are removing (ancient) Cygwin specific code that did not work > with modern Cygwin at all in step [2/4], it is not so unexpected > that some stuff does still not work after that step. I am not sure > what your reservation exactly is, but if you are wondering if code > to disable browsing and shortcut creation on Cygwin temporarily > needs to be there in the same step (instead of crashing or not > working), it may make sense if and only if it is done with minimal > changes. > > Thanks. > Timely response ... yes, that was my concern. I resolved this by making the cover letter and patch 2 commit message explicit that broken code remains. Thank you, Mark