Re: [PATCH] git-gui: msys2 compatibility patches

[Konstantin Podsvirov <konstantin@podsvirov.pro>]

27.04.2020, 22:48, "Pratyush Yadav" <me@yadavpratyush.com>:
> 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@podsvirov.pro>
>>
>>  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@podsvirov.pro>
>>  ---
>>   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