threads / patch / 53234

patchgit-gui: msys2 compatibility patches

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

## tl;dr

4 messages between Apr 14, 2020 and May 5, 2020. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Konstantin Podsvirov via GitGitGadget· Apr 14, 2020, 21:45 UTC · lore
From: Konstantin Podsvirov <konstantin@podsvirov.pro>
Allow using `git gui` command via MSYS2's MINGW32/64 subsystems (apropriate shells).
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
Signed-off-by: Konstantin Podsvirov <konstantin@podsvirov.pro>
---
    git-gui: msys2 compatibility patches
    
    Allow using git gui command via MSYS2's MINGW32/64 subsystems
    (apropriate shells).
    
    Just install apropriate tk package:
    
    user@host MINGW32 ~
    pacman -S mingw-w64-i686-tk
    
    or
    
    user@host MINGW64 ~
    pacman -S mingw-w64-x86_64-tk
    
    For more info see: https://github.com/msys2/MSYS2-packages/pull/1912
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-612%2Fpodsvirov%2Fgit-gui%2Fmsys2-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-612/podsvirov/git-gui/msys2-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/612
 git-gui.sh | 52 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 30 insertions(+), 22 deletions(-)
Show changes to git-gui.sh +30 −22
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@@}
+	}
 }
 set oguirel {@@GITGUI_RELATIVE@@}
 if {$oguirel eq {1}} {
@@ -163,7 +192,6 @@ set _isbare {}
 set _gitexec {}
 set _githtmldir {}
 set _reponame {}
-set _iscygwin {}
 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
-				}
-			}
-		} else {
-			set _iscygwin 0
-		}
-	}
-	return $_iscygwin
-}
-
 proc is_enabled {option} {
 	global enabled_options
 	if {[catch {set on $enabled_options($option)}]} {return 0}

base-commit: a5728022e07c53e5ac91db0960870518e243b7c1

-- 
gitgitgadget
Pratyush Yadav· Apr 27, 2020, 19:45 UTC · re: Konstantin Podsvirov via GitGitGadget · lore

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

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".
Show 15 quoted lines
> 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.
Show 51 quoted lines
> 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.

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.

Show 8 quoted lines
>  }
>  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?
Show 19 quoted lines
>  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?

Show 8 quoted lines
> -			}
> -		} 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
Konstantin Podsvirov· Apr 27, 2020, 21:33 UTC · re: Pratyush Yadav · lore

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

27.04.2020, 22:48, "Pratyush Yadav" <me@yadavpratyush.com>:
Show 11 quoted lines
> 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.
Show 19 quoted lines
>>  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.
Show 55 quoted lines
>>  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.
Show 6 quoted lines
> 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.
Show 10 quoted lines
>>   }
>>   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.
Show 24 quoted lines
>>   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.
Show 11 quoted lines
>>  - }
>>  - } 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.
Show 7 quoted lines
>>   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

Pratyush Yadav· May 5, 2020, 12:28 UTC · re: Konstantin Podsvirov · lore

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

Hi Konstantin,
On 28/04/20 12:33AM, Konstantin Podsvirov wrote:
Show 38 quoted lines
> 
> 
> 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.
Ok. Thanks for the explanation.
Show 57 quoted lines
> >>  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.
Got it.
Show 26 quoted lines
> > 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.

Ok. Though I wonder if we need this variable at all. It is only used with the function is_Cygwin, and seems like a performance optimization so we don't need to perform the check for every call to the function. But since it only is a couple of if statements, I'm not sure if it will make a significant performance difference.

Show 26 quoted lines
> >>   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.
So if MYSYTEM is something other than these two, would it be possible to 
run git-gui at all?
Show 13 quoted lines
> >>  - }
> >>  - } 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.
Ok.
Show 9 quoted lines
> >>   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?

You should send another version of this series with the suggestions implemented. Since you are using GitGitGadget, you can amend the commit, force push it, and then issue another `/submit` command, and GGG will take care of the rest. The GGG welcome message would have all the details as well.

-- 
Regards,
Pratyush Yadav

← back to recent threads