27.04.2020, 22:48, "Pratyush Yadav" <me@xxxxxxxxxxxxxxxxx>: > Hi Konstantin, > > Thanks for the patch, and sorry for the late reply. > > On 14/04/20 09:45PM, Konstantin Podsvirov via GitGitGadget wrote: >> From: Konstantin Podsvirov <konstantin@xxxxxxxxxxxxx> >> >> Allow using `git gui` command via MSYS2's MINGW32/64 subsystems (apropriate shells). > > I think this should be the commit subject, instead of "msys2 > compatibility patches". I do not mind. >> Just install apropriate `tk` package: >> >> ```bash >> user@host MINGW32 ~ >> pacman -S mingw-w64-i686-tk >> ``` >> >> or >> >> ```bash >> user@host MINGW64 ~ >> pacman -S mingw-w64-x86_64-tk >> ``` >> >> For more info see: https://github.com/msys2/MSYS2-packages/pull/1912 > > Please don't just link to an external website. Put the explanation there > in the commit message. Explain what the problem was, and how this patch > fixes it. The startup script everywhere operates on Unix paths, but on Windows they are incomplete, and the interpreter expects full native paths. >> Signed-off-by: Konstantin Podsvirov <konstantin@xxxxxxxxxxxxx> >> --- >> git-gui.sh | 52 ++++++++++++++++++++++++++++++---------------------- >> 1 file changed, 30 insertions(+), 22 deletions(-) >> >> diff --git a/git-gui.sh b/git-gui.sh >> index 4610e4ca72a..512f4f121aa 100755 >> --- a/git-gui.sh >> +++ b/git-gui.sh >> @@ -44,6 +44,28 @@ if {[catch {package require Tcl 8.5} err] >> >> catch {rename send {}} ; # What an evil concept... >> >> +###################################################################### >> +## >> +## platform detection >> + >> +set _iscygwin {} >> + >> +proc is_Cygwin {} { >> + global _iscygwin >> + if {$_iscygwin eq {}} { >> + if {$::tcl_platform(platform) eq {windows}} { >> + if {[catch {set p [exec cygpath --windir]} err]} { >> + set _iscygwin 0 >> + } else { >> + set _iscygwin 1 >> + } >> + } else { >> + set _iscygwin 0 >> + } >> + } >> + return $_iscygwin >> +} >> + >> ###################################################################### >> ## >> ## locate our library >> @@ -51,7 +73,14 @@ catch {rename send {}} ; # What an evil concept... >> if { [info exists ::env(GIT_GUI_LIB_DIR) ] } { >> set oguilib $::env(GIT_GUI_LIB_DIR) >> } else { >> - set oguilib {@@GITGUI_LIBDIR@@} >> + if {[is_Cygwin]} { >> + set oguilib [exec cygpath \ >> + --windows \ >> + --absolute \ >> + @@GITGUI_LIBDIR@@] >> + } else { >> + set oguilib {@@GITGUI_LIBDIR@@} >> + } > > This would convert the Windows style path to a Unix style path if we are > running in Cygwin, right? This is what I assume the heart of the problem > is. It is true exactly the opposite. > Style nitpick: something like this would probably be better: > > set oguilib {@@GITGUI_LIBDIR@@} > if {[is_Cygwin]} { > ... > } Looks good. > This makes it clear that Cygwin is the exception. For all other cases, > we want to use @@GITGUI_LIBDIR@@ directly. Yes. >> } >> set oguirel {@@GITGUI_RELATIVE@@} >> if {$oguirel eq {1}} { >> @@ -163,7 +192,6 @@ set _isbare {} >> set _gitexec {} >> set _githtmldir {} >> set _reponame {} >> -set _iscygwin {} > > Why move the initialization? To use this above when setting `oguilib` variable. >> set _search_path {} >> set _shellpath {@@SHELL_PATH@@} >> >> @@ -266,26 +294,6 @@ proc is_Windows {} { >> return 0 >> } >> >> -proc is_Cygwin {} { >> - global _iscygwin >> - if {$_iscygwin eq {}} { >> - if {$::tcl_platform(platform) eq {windows}} { >> - if {[catch {set p [exec cygpath --windir]} err]} { >> - set _iscygwin 0 >> - } else { >> - set _iscygwin 1 >> - # Handle MSys2 which is only cygwin when MSYSTEM is MSYS. >> - if {[info exists ::env(MSYSTEM)] && $::env(MSYSTEM) ne "MSYS"} { >> - set _iscygwin 0 >> - } > > I'm afraid I don't understand this hunk. I don't use Windows, and don't > completely understand the difference between cygwin, msys, etc. Could > you please explain further why this check is removed? Are there any > negative side-effects? To use `git gui` we need `tk` (wish), but `tk` (wish) can be available only when MSYSTEM is equal to MINGW32 or MINGW64. >> - } >> - } else { >> - set _iscygwin 0 >> - } >> - } >> - return $_iscygwin >> -} >> - > > Why move the function? Can't this and the _iscygwin initialization just > stay in their place? It will make the diff much easier to read. To use this above when setting `oguilib` variable. >> proc is_enabled {option} { >> global enabled_options >> if {[catch {set on $enabled_options($option)}]} {return 0} > > -- > Regards, > Pratyush Yadav What is the further course of action? -- Regards, Konstantin Podsvirov
