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

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

Style nitpick: something like this would probably be better:

  set oguilib {@@GITGUI_LIBDIR@@}
  if {[is_Cygwin]} {
	...
  }

This makes it clear that Cygwin is the exception. For all other cases, 
we want to use @@GITGUI_LIBDIR@@ directly.

>  }
>  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?

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

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

>  proc is_enabled {option} {
>  	global enabled_options
>  	if {[catch {set on $enabled_options($option)}]} {return 0}

-- 
Regards,
Pratyush Yadav