* [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui @ 2023-06-24 21:23 Mark Levedahl 2023-06-24 21:23 ` [PATCH v0 1/4] git gui Makefile - remove Cygwin modiifications Mark Levedahl ` (5 more replies) 0 siblings, 6 replies; 27+ messages in thread From: Mark Levedahl @ 2023-06-24 21:23 UTC (permalink / raw) To: git; +Cc: adam, me, johannes.schindelin, Mark Levedahl 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. 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. 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 1 removes the obsolete Makefile code patch 2 removes all obsolete git-gui.sh code, wrapped in is_Cygwin... patch 3 implements Cygwin specific file browsing support patch 4 implemetns Cygwin specific desktop icon support Patches 1/2 cause git-gui to function as it has for the last decade on Cygwin, but without bugs masking bugs. Patches 3/4 restore functionality lost with the Tcl/Tk switch in 2012. Any argument for keeping the old Cygwin code must address who is going to test and maintain that code, on what platform, and who the target audience is. The old Tcl/Tk was only on 32-bit Cygwin and only supported for the Insight debugger, 32-bit Cygwin is no longer supported, git-gui is not supported on 8.4.1 Tcl/Tk, and the Windows versions targeted by 2012'ish 32-bit Cygwin are no longer supported. Mark Levedahl (4): git gui Makefile - remove Cygwin modiifications git-gui - remove obsolete Cygwin specific code git-gui - use cygstart to browse on Cygwin git-gui - use mkshortcut on Cygwin Makefile | 21 +------ git-gui.sh | 126 ++++---------------------------------- lib/choose_repository.tcl | 27 +------- lib/shortcut.tcl | 31 +++++----- 4 files changed, 31 insertions(+), 174 deletions(-) -- 2.41.0.99.19 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v0 1/4] git gui Makefile - remove Cygwin modiifications 2023-06-24 21:23 [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui Mark Levedahl @ 2023-06-24 21:23 ` Mark Levedahl 2023-06-24 21:23 ` [PATCH v0 2/4] git-gui - remove obsolete Cygwin specific code Mark Levedahl ` (4 subsequent siblings) 5 siblings, 0 replies; 27+ messages in thread From: Mark Levedahl @ 2023-06-24 21:23 UTC (permalink / raw) To: git; +Cc: adam, me, johannes.schindelin, Mark Levedahl git-gui's Makefile hardcodes the absolute Windows path of git-gui's libraries into git-gui, destroying the ability to package git-gui on one machine and distribute to others. The intent is to do this only if a non-Cygwin Tcl/Tk is installed, but the test for this is wrong with the unix/X11 Tcl/Tk shipped since 2012. Also, Cygwin does not support a non-Cygwin Tcl/Tk. The Cygwin git maintainer disables this code, so this code is definitely not in use in the Cygwin distribution, and targets an untested / unsupportable configuration. The simplest approach is to just delete the Cygwin specific code as stock Cygwin needs no special handling. Do so. Signed-off-by: Mark Levedahl <mlevedahl@gmail.com> --- Makefile | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/Makefile b/Makefile index a0d5a4b..3f80435 100644 --- a/Makefile +++ b/Makefile @@ -138,25 +138,10 @@ GITGUI_SCRIPT := $$0 GITGUI_RELATIVE := GITGUI_MACOSXAPP := -ifeq ($(uname_O),Cygwin) - GITGUI_SCRIPT := `cygpath --windows --absolute "$(GITGUI_SCRIPT)"` - - # Is this a Cygwin Tcl/Tk binary? If so it knows how to do - # POSIX path translation just like cygpath does and we must - # keep libdir in POSIX format so Cygwin packages of git-gui - # work no matter where the user installs them. - # - ifeq ($(shell echo 'puts [file normalize /]' | '$(TCL_PATH_SQ)'),$(shell cygpath --mixed --absolute /)) - gg_libdir_sed_in := $(gg_libdir) - else - gg_libdir_sed_in := $(shell cygpath --windows --absolute "$(gg_libdir)") - endif -else - ifeq ($(exedir),$(gg_libdir)) - GITGUI_RELATIVE := 1 - endif - gg_libdir_sed_in := $(gg_libdir) +ifeq ($(exedir),$(gg_libdir)) + GITGUI_RELATIVE := 1 endif +gg_libdir_sed_in := $(gg_libdir) ifeq ($(uname_S),Darwin) ifeq ($(shell test -d $(TKFRAMEWORK) && echo y),y) GITGUI_MACOSXAPP := YesPlease -- 2.41.0.99.19 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v0 2/4] git-gui - remove obsolete Cygwin specific code 2023-06-24 21:23 [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui Mark Levedahl 2023-06-24 21:23 ` [PATCH v0 1/4] git gui Makefile - remove Cygwin modiifications Mark Levedahl @ 2023-06-24 21:23 ` Mark Levedahl 2023-06-25 2:56 ` Eric Sunshine 2023-06-24 21:23 ` [PATCH v0 3/4] git-gui - use cygstart to browse on Cygwin Mark Levedahl ` (3 subsequent siblings) 5 siblings, 1 reply; 27+ messages in thread From: Mark Levedahl @ 2023-06-24 21:23 UTC (permalink / raw) To: git; +Cc: adam, me, johannes.schindelin, Mark Levedahl In the current git release, git-gui runs on Cygwin without enabling any of git-gui's Cygwin specific code. This happens as the Cygwin specific code in git-gui was (mostly) written in 2007-2008 to work with Cygwin's then supplied Tcl/Tk which was an incompletely ported variant of the 8.4.1 Windows Tcl/Tk code. In March, 2012, that 8.4.1 package was replaced with a full port based upon the upstream unix/X11 code, since maintained up to date. The two Tcl/Tk packages are completely incompatible, and have different sygnatures. When Cygwin's Tcl/Tk signature changed in 2012, git-gui no longer detected Cygwin, so did not enable Cygwin specific code, and the POSIX environment provided by Cygwin since 2012 supported git-gui as a generic unix. Thus, no-one apparently noticed the existence of incompatible Cygwin specific code. However, since commit c5766eae6f2b002396b6cd4f85b62317b707174e in upstream git-gui, the is_Cygwin funcion does detect current Cygwin. The Cygwin specific code is enabled, causing use of Windows rather than unix pathnames, and enabling incorrect warnings about environment variables that are not relevant for the fully functional unix/X11 Tcl/Tk. The end result is that git-gui is now incommpatible with Cygwin. So, delete all Cygwin specific code (code protected by "if is_Cygwin"), thus restoring the post-2012 behaviour. Note that Cygwin specific code is required to enable file browsing and shortcut creation (supported before 2012), but is not addressed in this patch. Signed-off-by: Mark Levedahl <mlevedahl@gmail.com> --- git-gui.sh | 122 +++----------------------------------- lib/choose_repository.tcl | 27 +-------- lib/shortcut.tcl | 41 ------------- 3 files changed, 9 insertions(+), 181 deletions(-) diff --git a/git-gui.sh b/git-gui.sh index cb92bba..b5dba80 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -84,14 +84,7 @@ proc _which {what args} { global env _search_exe _search_path if {$_search_path eq {}} { - if {[is_Cygwin] && [regexp {^(/|\.:)} $env(PATH)]} { - set _search_path [split [exec cygpath \ - --windows \ - --path \ - --absolute \ - $env(PATH)] {;}] - set _search_exe .exe - } elseif {[is_Windows]} { + if {[is_Windows]} { set gitguidir [file dirname [info script]] regsub -all ";" $gitguidir "\\;" gitguidir set env(PATH) "$gitguidir;$env(PATH)" @@ -342,14 +335,7 @@ proc gitexec {args} { if {[catch {set _gitexec [git --exec-path]} err]} { error "Git not installed?\n\n$err" } - if {[is_Cygwin]} { - set _gitexec [exec cygpath \ - --windows \ - --absolute \ - $_gitexec] - } else { - set _gitexec [file normalize $_gitexec] - } + set _gitexec [file normalize $_gitexec] } if {$args eq {}} { return $_gitexec @@ -364,14 +350,7 @@ proc githtmldir {args} { # Git not installed or option not yet supported return {} } - if {[is_Cygwin]} { - set _githtmldir [exec cygpath \ - --windows \ - --absolute \ - $_githtmldir] - } else { - set _githtmldir [file normalize $_githtmldir] - } + set _githtmldir [file normalize $_githtmldir] } if {$args eq {}} { return $_githtmldir @@ -1318,9 +1297,6 @@ if {$_gitdir eq "."} { set _gitdir [pwd] } -if {![file isdirectory $_gitdir] && [is_Cygwin]} { - catch {set _gitdir [exec cygpath --windows $_gitdir]} -} if {![file isdirectory $_gitdir]} { catch {wm withdraw .} error_popup [strcat [mc "Git directory not found:"] "\n\n$_gitdir"] @@ -1332,11 +1308,7 @@ apply_config # v1.7.0 introduced --show-toplevel to return the canonical work-tree if {[package vcompare $_git_version 1.7.0] >= 0} { - if { [is_Cygwin] } { - catch {set _gitworktree [exec cygpath --windows [git rev-parse --show-toplevel]]} - } else { - set _gitworktree [git rev-parse --show-toplevel] - } + set _gitworktree [git rev-parse --show-toplevel] } else { # try to set work tree from environment, core.worktree or use # cdup to obtain a relative path to the top of the worktree. If @@ -1561,24 +1533,8 @@ proc rescan {after {honor_trustmtime 1}} { } } -if {[is_Cygwin]} { - set is_git_info_exclude {} - proc have_info_exclude {} { - global is_git_info_exclude - - if {$is_git_info_exclude eq {}} { - if {[catch {exec test -f [gitdir info exclude]}]} { - set is_git_info_exclude 0 - } else { - set is_git_info_exclude 1 - } - } - return $is_git_info_exclude - } -} else { - proc have_info_exclude {} { - return [file readable [gitdir info exclude]] - } +proc have_info_exclude {} { + return [file readable [gitdir info exclude]] } proc rescan_stage2 {fd after} { @@ -2318,7 +2274,7 @@ proc do_git_gui {} { # Get the system-specific explorer app/command. proc get_explorer {} { - if {[is_Cygwin] || [is_Windows]} { + if {[is_Windows]} { set explorer "explorer.exe" } elseif {[is_MacOSX]} { set explorer "open" @@ -2874,11 +2830,7 @@ if {[is_enabled multicommit]} { .mbar.repository add separator - if {[is_Cygwin]} { - .mbar.repository add command \ - -label [mc "Create Desktop Icon"] \ - -command do_cygwin_shortcut - } elseif {[is_Windows]} { + if {[is_Windows]} { .mbar.repository add command \ -label [mc "Create Desktop Icon"] \ -command do_windows_shortcut @@ -3112,10 +3064,6 @@ if {[is_MacOSX]} { set doc_path [githtmldir] if {$doc_path ne {}} { set doc_path [file join $doc_path index.html] - - if {[is_Cygwin]} { - set doc_path [exec cygpath --mixed $doc_path] - } } if {[file isfile $doc_path]} { @@ -4087,60 +4035,6 @@ set file_lists($ui_workdir) [list] wm title . "[appname] ([reponame]) [file normalize $_gitworktree]" focus -force $ui_comm -# -- Warn the user about environmental problems. Cygwin's Tcl -# does *not* pass its env array onto any processes it spawns. -# This means that git processes get none of our environment. -# -if {[is_Cygwin]} { - set ignored_env 0 - set suggest_user {} - set msg [mc "Possible environment issues exist. - -The following environment variables are probably -going to be ignored by any Git subprocess run -by %s: - -" [appname]] - foreach name [array names env] { - switch -regexp -- $name { - {^GIT_INDEX_FILE$} - - {^GIT_OBJECT_DIRECTORY$} - - {^GIT_ALTERNATE_OBJECT_DIRECTORIES$} - - {^GIT_DIFF_OPTS$} - - {^GIT_EXTERNAL_DIFF$} - - {^GIT_PAGER$} - - {^GIT_TRACE$} - - {^GIT_CONFIG$} - - {^GIT_(AUTHOR|COMMITTER)_DATE$} { - append msg " - $name\n" - incr ignored_env - } - {^GIT_(AUTHOR|COMMITTER)_(NAME|EMAIL)$} { - append msg " - $name\n" - incr ignored_env - set suggest_user $name - } - } - } - if {$ignored_env > 0} { - append msg [mc " -This is due to a known issue with the -Tcl binary distributed by Cygwin."] - - if {$suggest_user ne {}} { - append msg [mc " - -A good replacement for %s -is placing values for the user.name and -user.email settings into your personal -~/.gitconfig file. -" $suggest_user] - } - warn_popup $msg - } - unset ignored_env msg suggest_user name -} - # -- Only initialize complex UI if we are going to stay running. # if {[is_enabled transport]} { diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl index af1fee7..d23abed 100644 --- a/lib/choose_repository.tcl +++ b/lib/choose_repository.tcl @@ -174,9 +174,6 @@ constructor pick {} { -foreground blue \ -underline 1 set home $::env(HOME) - if {[is_Cygwin]} { - set home [exec cygpath --windows --absolute $home] - } set home "[file normalize $home]/" set hlen [string length $home] foreach p $sorted_recent { @@ -374,18 +371,6 @@ proc _objdir {path} { return $objdir } - if {[is_Cygwin]} { - set objdir [file join $path .git objects.lnk] - if {[file isfile $objdir]} { - return [win32_read_lnk $objdir] - } - - set objdir [file join $path objects.lnk] - if {[file isfile $objdir]} { - return [win32_read_lnk $objdir] - } - } - return {} } @@ -623,12 +608,6 @@ method _do_clone2 {} { } set giturl $origin_url - if {[is_Cygwin] && [file isdirectory $giturl]} { - set giturl [exec cygpath --unix --absolute $giturl] - if {$clone_type eq {shared}} { - set objdir [exec cygpath --unix --absolute $objdir] - } - } if {[file exists $local_path]} { error_popup [mc "Location %s already exists." $local_path] @@ -668,11 +647,7 @@ method _do_clone2 {} { fconfigure $f_cp -translation binary -encoding binary cd $objdir while {[gets $f_in line] >= 0} { - if {[is_Cygwin]} { - puts $f_cp [exec cygpath --unix --absolute $line] - } else { - puts $f_cp [file normalize $line] - } + puts $f_cp [file normalize $line] } close $f_in close $f_cp diff --git a/lib/shortcut.tcl b/lib/shortcut.tcl index 97d1d7a..1d8374b 100644 --- a/lib/shortcut.tcl +++ b/lib/shortcut.tcl @@ -26,47 +26,6 @@ proc do_windows_shortcut {} { } } -proc do_cygwin_shortcut {} { - global argv0 _gitworktree - - if {[catch { - set desktop [exec cygpath \ - --windows \ - --absolute \ - --long-name \ - --desktop] - }]} { - set desktop . - } - set fn [tk_getSaveFile \ - -parent . \ - -title [mc "%s (%s): Create Desktop Icon" [appname] [reponame]] \ - -initialdir $desktop \ - -initialfile "Git [reponame].lnk"] - if {$fn != {}} { - if {[file extension $fn] ne {.lnk}} { - set fn ${fn}.lnk - } - if {[catch { - set sh [exec cygpath \ - --windows \ - --absolute \ - /bin/sh.exe] - set me [exec cygpath \ - --unix \ - --absolute \ - $argv0] - win32_create_lnk $fn [list \ - $sh -c \ - "CHERE_INVOKING=1 source /etc/profile;[sq $me] &" \ - ] \ - [file normalize $_gitworktree] - } err]} { - error_popup [strcat [mc "Cannot write shortcut:"] "\n\n$err"] - } - } -} - proc do_macosx_app {} { global argv0 env -- 2.41.0.99.19 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v0 2/4] git-gui - remove obsolete Cygwin specific code 2023-06-24 21:23 ` [PATCH v0 2/4] git-gui - remove obsolete Cygwin specific code Mark Levedahl @ 2023-06-25 2:56 ` Eric Sunshine 2023-06-25 11:29 ` Mark Levedahl 0 siblings, 1 reply; 27+ messages in thread From: Eric Sunshine @ 2023-06-25 2:56 UTC (permalink / raw) To: Mark Levedahl; +Cc: git, adam, me, johannes.schindelin On Sat, Jun 24, 2023 at 5:35 PM Mark Levedahl <mlevedahl@gmail.com> wrote: > In the current git release, git-gui runs on Cygwin without enabling any > of git-gui's Cygwin specific code. This happens as the Cygwin specific > code in git-gui was (mostly) written in 2007-2008 to work with Cygwin's > then supplied Tcl/Tk which was an incompletely ported variant of the > 8.4.1 Windows Tcl/Tk code. In March, 2012, that 8.4.1 package was > replaced with a full port based upon the upstream unix/X11 code, > since maintained up to date. The two Tcl/Tk packages are completely > incompatible, and have different sygnatures. Given the context, an understandable typo perhaps: s/sygnatures/signatures/ > When Cygwin's Tcl/Tk signature changed in 2012, git-gui no longer > detected Cygwin, so did not enable Cygwin specific code, and the POSIX > environment provided by Cygwin since 2012 supported git-gui as a generic > unix. Thus, no-one apparently noticed the existence of incompatible > Cygwin specific code. > > However, since commit c5766eae6f2b002396b6cd4f85b62317b707174e in > upstream git-gui, the is_Cygwin funcion does detect current Cygwin. The > Cygwin specific code is enabled, causing use of Windows rather than unix > pathnames, and enabling incorrect warnings about environment variables > that are not relevant for the fully functional unix/X11 Tcl/Tk. The end > result is that git-gui is now incommpatible with Cygwin. s/incommpatible/incompatible/ > So, delete all Cygwin specific code (code protected by "if is_Cygwin"), > thus restoring the post-2012 behaviour. Note that Cygwin specific code > is required to enable file browsing and shortcut creation (supported > before 2012), but is not addressed in this patch. > > Signed-off-by: Mark Levedahl <mlevedahl@gmail.com> ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v0 2/4] git-gui - remove obsolete Cygwin specific code 2023-06-25 2:56 ` Eric Sunshine @ 2023-06-25 11:29 ` Mark Levedahl 0 siblings, 0 replies; 27+ messages in thread From: Mark Levedahl @ 2023-06-25 11:29 UTC (permalink / raw) To: Eric Sunshine; +Cc: git, adam, me, johannes.schindelin On 6/24/23 22:56, Eric Sunshine wrote: > On Sat, Jun 24, 2023 at 5:35 PM Mark Levedahl <mlevedahl@gmail.com> wrote: >> In the current git release, git-gui runs on Cygwin without enabling any >> of git-gui's Cygwin specific code. This happens as the Cygwin specific >> code in git-gui was (mostly) written in 2007-2008 to work with Cygwin's >> then supplied Tcl/Tk which was an incompletely ported variant of the >> 8.4.1 Windows Tcl/Tk code. In March, 2012, that 8.4.1 package was >> replaced with a full port based upon the upstream unix/X11 code, >> since maintained up to date. The two Tcl/Tk packages are completely >> incompatible, and have different sygnatures. > Given the context, an understandable typo perhaps: s/sygnatures/signatures/ > >> When Cygwin's Tcl/Tk signature changed in 2012, git-gui no longer >> detected Cygwin, so did not enable Cygwin specific code, and the POSIX >> environment provided by Cygwin since 2012 supported git-gui as a generic >> unix. Thus, no-one apparently noticed the existence of incompatible >> Cygwin specific code. >> >> However, since commit c5766eae6f2b002396b6cd4f85b62317b707174e in >> upstream git-gui, the is_Cygwin funcion does detect current Cygwin. The >> Cygwin specific code is enabled, causing use of Windows rather than unix >> pathnames, and enabling incorrect warnings about environment variables >> that are not relevant for the fully functional unix/X11 Tcl/Tk. The end >> result is that git-gui is now incommpatible with Cygwin. > s/incommpatible/incompatible/ > >> So, delete all Cygwin specific code (code protected by "if is_Cygwin"), >> thus restoring the post-2012 behaviour. Note that Cygwin specific code >> is required to enable file browsing and shortcut creation (supported >> before 2012), but is not addressed in this patch. >> >> Signed-off-by: Mark Levedahl <mlevedahl@gmail.com> Thanks, will fix both (and a few other typos ...) Mark ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v0 3/4] git-gui - use cygstart to browse on Cygwin 2023-06-24 21:23 [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui Mark Levedahl 2023-06-24 21:23 ` [PATCH v0 1/4] git gui Makefile - remove Cygwin modiifications Mark Levedahl 2023-06-24 21:23 ` [PATCH v0 2/4] git-gui - remove obsolete Cygwin specific code Mark Levedahl @ 2023-06-24 21:23 ` Mark Levedahl 2023-06-24 21:23 ` [PATCH v0 4/4] git-gui - use mkshortcut " Mark Levedahl ` (2 subsequent siblings) 5 siblings, 0 replies; 27+ messages in thread From: Mark Levedahl @ 2023-06-24 21:23 UTC (permalink / raw) To: git; +Cc: adam, me, johannes.schindelin, Mark Levedahl Pre-2012, git-gui enabled the "Repository->Explore Working Copy" menu on Cygwin, offering open a Windows graphical file browser at the root working directory. The old code relied upon internal use of Windows pathnames, while git-gui must use unix pathnames on Cygwin since 2012, so was removed in a previous patch. A base install of Cygwin provides the /bin/cygstart utility that runs arbtitrary Windows applications after translating unix pathnames to Windows. Adding the --explore option guarantees that the Windows file explorer is opened, regardless of the supplied pathname's file type and avoiding possibility of some other action being taken. So, teach git-gui to use cygstart --explore on Cygwin, restoring the pre-2012 behavior of opening a Windows file explorer for browsing. Signed-off-by: Mark Levedahl <mlevedahl@gmail.com> --- git-gui.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/git-gui.sh b/git-gui.sh index b5dba80..523770a 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -2276,6 +2276,8 @@ proc do_git_gui {} { 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 { -- 2.41.0.99.19 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v0 4/4] git-gui - use mkshortcut on Cygwin 2023-06-24 21:23 [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui Mark Levedahl ` (2 preceding siblings ...) 2023-06-24 21:23 ` [PATCH v0 3/4] git-gui - use cygstart to browse on Cygwin Mark Levedahl @ 2023-06-24 21:23 ` Mark Levedahl 2023-06-24 23:30 ` [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui Junio C Hamano 2023-06-26 16:53 ` [PATCH v1 " Mark Levedahl 5 siblings, 0 replies; 27+ messages in thread From: Mark Levedahl @ 2023-06-24 21:23 UTC (permalink / raw) To: git; +Cc: adam, me, johannes.schindelin, Mark Levedahl Prior to 2012, git-gui enabled the "Repository->Create Desktop Icon" item on Cygwin, offering to create a shortcut that starts git-gui on a particular repository. The original code for this in lib/win32.tcl, shared with Git for Windows support, requires Windows pathnames, while git-gui must use unix pathnames with the unix/X11 Tcl/Tk since 2012. The ability to use this from Cygwin was removed in a previous patch. Cygwin's default installation provides /bin/mkshortcut for creating desktop shortuts, this is compatible with exec under tcl, and understands Cygwin's unix pathnames. So, teach git-gui to use mkshortcut on Cygwin, leaving lib/win32.tcl as Git for Windows specific support. Notes: "CHERE_INVOKING=1" is recognized by Cygwin's /etc/profile and prevents a "chdir $HOME", leaving the shell in the working directory specified by the shortcut. That directory is written directly by mkshortcut eliminating any problems with shell escapes and quoting. The pre-2012 code includes the full pathname of the git-gui creating the shortcut (rather than using the system git-gui), but that git-gui might not be compatible with the git found after /etc/profile sets the path, and might have a pathname that defies encoding using shell escapes that can survive the multiple incompatible interpreters involved in this chain. Instead, use "git gui", thus defaulting to the system git and avoiding both issues. Signed-off-by: Mark Levedahl <mlevedahl@gmail.com> --- git-gui.sh | 4 ++++ lib/shortcut.tcl | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/git-gui.sh b/git-gui.sh index 523770a..5c13521 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -2836,6 +2836,10 @@ if {[is_enabled multicommit]} { .mbar.repository add command \ -label [mc "Create Desktop Icon"] \ -command do_windows_shortcut + } elseif {[is_Cygwin]} { + .mbar.repository add command \ + -label [mc "Create Desktop Icon"] \ + -command do_cygwin_shortcut } elseif {[is_MacOSX]} { .mbar.repository add command \ -label [mc "Create Desktop Icon"] \ diff --git a/lib/shortcut.tcl b/lib/shortcut.tcl index 1d8374b..6c2a99e 100644 --- a/lib/shortcut.tcl +++ b/lib/shortcut.tcl @@ -26,6 +26,44 @@ proc do_windows_shortcut {} { } } +proc do_cygwin_shortcut {} { + global argv0 _gitworktree oguilib + + if {[catch { + set desktop [exec cygpath \ + --desktop] + }]} { + set desktop . + } + set fn [tk_getSaveFile \ + -parent . \ + -title [mc "%s (%s): Create Desktop Icon" [appname] [reponame]] \ + -initialdir $desktop \ + -initialfile "Git [reponame].lnk"] + if {$fn != {}} { + if {[file extension $fn] ne {.lnk}} { + set fn ${fn}.lnk + } + if {[catch { + set repodir [file normalize $_gitworktree] + set shargs {-c \ + "CHERE_INVOKING=1 \ + source /etc/profile; \ + git gui"} + exec /bin/mkshortcut.exe \ + -a $shargs \ + -d "git-gui on $repodir" \ + -i $oguilib/git-gui.ico \ + -n $fn \ + -s min \ + -w $repodir \ + /bin/sh.exe + } err]} { + error_popup [strcat [mc "Cannot write shortcut:"] "\n\n$err"] + } + } +} + proc do_macosx_app {} { global argv0 env -- 2.41.0.99.19 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui 2023-06-24 21:23 [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui Mark Levedahl ` (3 preceding siblings ...) 2023-06-24 21:23 ` [PATCH v0 4/4] git-gui - use mkshortcut " Mark Levedahl @ 2023-06-24 23:30 ` Junio C Hamano 2023-06-24 23:35 ` Junio C Hamano 2023-06-25 11:26 ` Mark Levedahl 2023-06-26 16:53 ` [PATCH v1 " Mark Levedahl 5 siblings, 2 replies; 27+ messages in thread From: Junio C Hamano @ 2023-06-24 23:30 UTC (permalink / raw) To: Mark Levedahl; +Cc: git, adam, me, johannes.schindelin 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui 2023-06-24 23:30 ` [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui Junio C Hamano @ 2023-06-24 23:35 ` Junio C Hamano 2023-06-25 11:28 ` Mark Levedahl 2023-06-25 11:26 ` Mark Levedahl 1 sibling, 1 reply; 27+ messages in thread From: Junio C Hamano @ 2023-06-24 23:35 UTC (permalink / raw) To: Mark Levedahl; +Cc: git, adam, me, johannes.schindelin 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui 2023-06-24 23:35 ` Junio C Hamano @ 2023-06-25 11:28 ` Mark Levedahl 0 siblings, 0 replies; 27+ messages in thread From: Mark Levedahl @ 2023-06-25 11:28 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, adam, me, johannes.schindelin 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui 2023-06-24 23:30 ` [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui Junio C Hamano 2023-06-24 23:35 ` Junio C Hamano @ 2023-06-25 11:26 ` Mark Levedahl 2023-06-25 12:10 ` Mark Levedahl 2023-06-25 15:46 ` Junio C Hamano 1 sibling, 2 replies; 27+ messages in thread From: Mark Levedahl @ 2023-06-25 11:26 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, adam, me, johannes.schindelin 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui 2023-06-25 11:26 ` Mark Levedahl @ 2023-06-25 12:10 ` Mark Levedahl 2023-06-25 15:46 ` Junio C Hamano 1 sibling, 0 replies; 27+ messages in thread From: Mark Levedahl @ 2023-06-25 12:10 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, adam, me, johannes.schindelin 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui 2023-06-25 11:26 ` Mark Levedahl 2023-06-25 12:10 ` Mark Levedahl @ 2023-06-25 15:46 ` Junio C Hamano 2023-06-25 17:01 ` Mark Levedahl 1 sibling, 1 reply; 27+ messages in thread From: Junio C Hamano @ 2023-06-25 15:46 UTC (permalink / raw) To: Mark Levedahl; +Cc: git, adam, me, johannes.schindelin 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui 2023-06-25 15:46 ` Junio C Hamano @ 2023-06-25 17:01 ` Mark Levedahl 2023-06-26 15:52 ` Junio C Hamano 0 siblings, 1 reply; 27+ messages in thread From: Mark Levedahl @ 2023-06-25 17:01 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, adam, me, johannes.schindelin 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui 2023-06-25 17:01 ` Mark Levedahl @ 2023-06-26 15:52 ` Junio C Hamano 2023-06-26 16:55 ` Mark Levedahl 0 siblings, 1 reply; 27+ messages in thread From: Junio C Hamano @ 2023-06-26 15:52 UTC (permalink / raw) To: Mark Levedahl; +Cc: git, adam, me, johannes.schindelin 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. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui 2023-06-26 15:52 ` Junio C Hamano @ 2023-06-26 16:55 ` Mark Levedahl 0 siblings, 0 replies; 27+ messages in thread From: Mark Levedahl @ 2023-06-26 16:55 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, adam, me, johannes.schindelin 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 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v1 0/4] Remove obsolete Cygwin support from git-gui 2023-06-24 21:23 [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui Mark Levedahl ` (4 preceding siblings ...) 2023-06-24 23:30 ` [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui Junio C Hamano @ 2023-06-26 16:53 ` Mark Levedahl 2023-06-26 16:53 ` [PATCH v1 1/4] git gui Makefile - remove Cygwin modifications Mark Levedahl ` (5 more replies) 5 siblings, 6 replies; 27+ messages in thread From: Mark Levedahl @ 2023-06-26 16:53 UTC (permalink / raw) To: mdl123, git Cc: adam, me, johannes.schindelin, gitster, sunshine, Mark Levedahl === This is an update, incorporating responses to Junio's and Eric's comments: -- clarified what the "upstream" git-gui branch is -- Removed some changes from patch 2 as requested by Junio, reducing changes in patch 3 and patch 4 All code is fixed only after applying patch 4 Differences in patch 3 and 4 are minimimized -- updated comments to clarify G4w dedicated code. -- updated all comments to (hopefully) clarify points of confusion === 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 from the Cygwin project 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 understands Windows pathnames but has incomplete support for unix pathnames (for instance, all pathnames output by that Tcl are Windows, not unix). The Cygwin git executables all use unix pathnames (though like all Cygwin executables have some capability to accept Windows pathnames). git-gui's Cygwin specific code causes git-gui to use Windows pathnames everywhere. The unix/X11 Tcl/Tk requires use of unix pathname, so none of the Cygwin specific code is compatible. git-gui is maintained at https://github.com/prati0100/git-gui. The git-gui in Junio's tree corresponds to commit c0698df057, behind the current git-gui master which is a5005ded. Fortunately, the git-gui/is_Cygwin function in Junio's tree relies upon the old Tcl/Tk that outputs Windows pathnames. As this fails to detect Cygwin, git-gui treats Cygwin as a unix variant with no platform specific code enabled and git-gui currently runs on Cygwin. But, commit c5766eae6f on the git-gui master branch fixes is_Cygwin to work with the new Tcl/Tk's signature (which is not that of a Windows Tcl/Tk). Thus, Cygwin is detected, the incompatible Cygwin code is enabled, and git-gui no longer runs on Cygwin. Also, there is Cygwin specific code in the Makefile, intended to allow a completely unsupported configuration with a Windows Tcl/Tk. However, the Makefile code mis-identifies the unix/X11 Tcl/Tk as Windows, triggering insertion of a hardcoded Windows path to the library directory into git-gui making it non-portable. The Cygwin git maintainer comments this code out, but the code should be removed as it is demonstrated to be incompatible with Cygwin and targets a configuration Cygwin does not support. The existing code for file browsing and creating a desktop icon is shared with Git For Windows support, and supports only Windows pathnames. This code does not work on Cygwin and needs replacement or update. The menu items for these functions are enabled by is_Cygwin, so appear only after is_Cygwin is fixed as discussed above. patch 1 removes the obsolete Makefile code patch 2 removes obsolete git-gui.sh code, wrapped in is_Cygwin except for code fixed in patches 3 and 4 patch 3 implements Cygwin specific file browsing support patch 4 implements Cygwin specific desktop icon support The end result is that git-gui on Cygwin is restored to the full capabilities existing prior to the Tcl/Tk switch in 2012. Also, the remaining Cygwin specific code, updated in patches 3 and 4, no longer overlaps with Git For Windows support. Any argument for keeping the old Cygwin code must address who is going to test and maintain that code, on what platform, and who the target audience is. The old Tcl/Tk was only on 32-bit Cygwin and only supported for the Insight debugger, 32-bit Cygwin is no longer supported, git-gui is not supported on 8.4.1 Tcl/Tk, and the Windows versions targeted by 2012'ish 32-bit Cygwin are no longer supported. Mark Levedahl (4): git gui Makefile - remove Cygwin modifications git-gui - remove obsolete Cygwin specific code git-gui - use cygstart to browse on Cygwin git-gui - use mkshortcut on Cygwin Makefile | 21 +------ git-gui.sh | 118 +++----------------------------------- lib/choose_repository.tcl | 27 +-------- lib/shortcut.tcl | 31 +++++----- 4 files changed, 27 insertions(+), 170 deletions(-) -- 2.41.0.99.19 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v1 1/4] git gui Makefile - remove Cygwin modifications 2023-06-26 16:53 ` [PATCH v1 " Mark Levedahl @ 2023-06-26 16:53 ` Mark Levedahl 2023-06-26 16:53 ` [PATCH v1 2/4] git-gui - remove obsolete Cygwin specific code Mark Levedahl ` (4 subsequent siblings) 5 siblings, 0 replies; 27+ messages in thread From: Mark Levedahl @ 2023-06-26 16:53 UTC (permalink / raw) To: mdl123, git Cc: adam, me, johannes.schindelin, gitster, sunshine, Mark Levedahl git-gui's Makefile hardcodes the absolute Windows path of git-gui's libraries into git-gui, destroying the ability to package git-gui on one machine and distribute to others. The intent is to do this only if a non-Cygwin Tcl/Tk is installed, but the test for this is wrong with the unix/X11 Tcl/Tk shipped since 2012. Also, Cygwin does not support a non-Cygwin Tcl/Tk. The Cygwin git maintainer disables this code, so this code is definitely not in use in the Cygwin distribution. The simplest fix is to just delete the Cygwin specific code, allowing the Makefile to work out of the box on Cygwin. Do so. Signed-off-by: Mark Levedahl <mlevedahl@gmail.com> --- Makefile | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/Makefile b/Makefile index a0d5a4b..3f80435 100644 --- a/Makefile +++ b/Makefile @@ -138,25 +138,10 @@ GITGUI_SCRIPT := $$0 GITGUI_RELATIVE := GITGUI_MACOSXAPP := -ifeq ($(uname_O),Cygwin) - GITGUI_SCRIPT := `cygpath --windows --absolute "$(GITGUI_SCRIPT)"` - - # Is this a Cygwin Tcl/Tk binary? If so it knows how to do - # POSIX path translation just like cygpath does and we must - # keep libdir in POSIX format so Cygwin packages of git-gui - # work no matter where the user installs them. - # - ifeq ($(shell echo 'puts [file normalize /]' | '$(TCL_PATH_SQ)'),$(shell cygpath --mixed --absolute /)) - gg_libdir_sed_in := $(gg_libdir) - else - gg_libdir_sed_in := $(shell cygpath --windows --absolute "$(gg_libdir)") - endif -else - ifeq ($(exedir),$(gg_libdir)) - GITGUI_RELATIVE := 1 - endif - gg_libdir_sed_in := $(gg_libdir) +ifeq ($(exedir),$(gg_libdir)) + GITGUI_RELATIVE := 1 endif +gg_libdir_sed_in := $(gg_libdir) ifeq ($(uname_S),Darwin) ifeq ($(shell test -d $(TKFRAMEWORK) && echo y),y) GITGUI_MACOSXAPP := YesPlease -- 2.41.0.99.19 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 2/4] git-gui - remove obsolete Cygwin specific code 2023-06-26 16:53 ` [PATCH v1 " Mark Levedahl 2023-06-26 16:53 ` [PATCH v1 1/4] git gui Makefile - remove Cygwin modifications Mark Levedahl @ 2023-06-26 16:53 ` Mark Levedahl 2023-06-26 16:53 ` [PATCH v1 3/4] git-gui - use cygstart to browse on Cygwin Mark Levedahl ` (3 subsequent siblings) 5 siblings, 0 replies; 27+ messages in thread From: Mark Levedahl @ 2023-06-26 16:53 UTC (permalink / raw) To: mdl123, git Cc: adam, me, johannes.schindelin, gitster, sunshine, Mark Levedahl In the current git release, git-gui runs on Cygwin without enabling any of git-gui's Cygwin specific code. This happens as the Cygwin specific code in git-gui was (mostly) written in 2007-2008 to work with Cygwin's then supplied Tcl/Tk which was an incompletely ported variant of the 8.4.1 Windows Tcl/Tk code. In March, 2012, that 8.4.1 package was replaced with a full port based upon the upstream unix/X11 code, since maintained up to date. The two Tcl/Tk packages are completely incompatible, and have different signatures. When Cygwin's Tcl/Tk signature changed in 2012, git-gui no longer detected Cygwin, so did not enable Cygwin specific code, and the POSIX environment provided by Cygwin since 2012 supported git-gui as a generic unix. Thus, no-one apparently noticed the existence of incompatible Cygwin specific code. However, since commit c5766eae6f in the git-gui source tree (https://github.com/prati0100/git-gui, master at a5005ded), and not yet pulled into the git repository, the is_Cygwin function does detect Cygwin using the unix/X11 Tcl/Tk. The Cygwin specific code is enabled, causing use of Windows rather than unix pathnames, and enabling incorrect warnings about environment variables that were relevant only to the old Tcl/Tk. The end result is that (upstream) git-gui is now incompatible with Cygwin. So, delete Cygwin specific code (code protected by "if is_Cygwin") that is not needed in any form to work with the unix/X11 Tcl/Tk. Cygwin specific code required to enable file browsing and shortcut creation is not addressed in this patch, does not currently work, and invocation of those items may leave git-gui in a confused state. Signed-off-by: Mark Levedahl <mlevedahl@gmail.com> --- changes since v0 -- do not touch any code fixed in patches 3/4, meaning the browsing and shortcut creating menu items do not work. git-gui.sh | 114 ++------------------------------------ lib/choose_repository.tcl | 27 +-------- 2 files changed, 7 insertions(+), 134 deletions(-) diff --git a/git-gui.sh b/git-gui.sh index cb92bba..3f7c31e 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -84,14 +84,7 @@ proc _which {what args} { global env _search_exe _search_path if {$_search_path eq {}} { - if {[is_Cygwin] && [regexp {^(/|\.:)} $env(PATH)]} { - set _search_path [split [exec cygpath \ - --windows \ - --path \ - --absolute \ - $env(PATH)] {;}] - set _search_exe .exe - } elseif {[is_Windows]} { + if {[is_Windows]} { set gitguidir [file dirname [info script]] regsub -all ";" $gitguidir "\\;" gitguidir set env(PATH) "$gitguidir;$env(PATH)" @@ -342,14 +335,7 @@ proc gitexec {args} { if {[catch {set _gitexec [git --exec-path]} err]} { error "Git not installed?\n\n$err" } - if {[is_Cygwin]} { - set _gitexec [exec cygpath \ - --windows \ - --absolute \ - $_gitexec] - } else { - set _gitexec [file normalize $_gitexec] - } + set _gitexec [file normalize $_gitexec] } if {$args eq {}} { return $_gitexec @@ -364,14 +350,7 @@ proc githtmldir {args} { # Git not installed or option not yet supported return {} } - if {[is_Cygwin]} { - set _githtmldir [exec cygpath \ - --windows \ - --absolute \ - $_githtmldir] - } else { - set _githtmldir [file normalize $_githtmldir] - } + set _githtmldir [file normalize $_githtmldir] } if {$args eq {}} { return $_githtmldir @@ -1318,9 +1297,6 @@ if {$_gitdir eq "."} { set _gitdir [pwd] } -if {![file isdirectory $_gitdir] && [is_Cygwin]} { - catch {set _gitdir [exec cygpath --windows $_gitdir]} -} if {![file isdirectory $_gitdir]} { catch {wm withdraw .} error_popup [strcat [mc "Git directory not found:"] "\n\n$_gitdir"] @@ -1332,11 +1308,7 @@ apply_config # v1.7.0 introduced --show-toplevel to return the canonical work-tree if {[package vcompare $_git_version 1.7.0] >= 0} { - if { [is_Cygwin] } { - catch {set _gitworktree [exec cygpath --windows [git rev-parse --show-toplevel]]} - } else { - set _gitworktree [git rev-parse --show-toplevel] - } + set _gitworktree [git rev-parse --show-toplevel] } else { # try to set work tree from environment, core.worktree or use # cdup to obtain a relative path to the top of the worktree. If @@ -1561,24 +1533,8 @@ proc rescan {after {honor_trustmtime 1}} { } } -if {[is_Cygwin]} { - set is_git_info_exclude {} - proc have_info_exclude {} { - global is_git_info_exclude - - if {$is_git_info_exclude eq {}} { - if {[catch {exec test -f [gitdir info exclude]}]} { - set is_git_info_exclude 0 - } else { - set is_git_info_exclude 1 - } - } - return $is_git_info_exclude - } -} else { - proc have_info_exclude {} { - return [file readable [gitdir info exclude]] - } +proc have_info_exclude {} { + return [file readable [gitdir info exclude]] } proc rescan_stage2 {fd after} { @@ -3112,10 +3068,6 @@ if {[is_MacOSX]} { set doc_path [githtmldir] if {$doc_path ne {}} { set doc_path [file join $doc_path index.html] - - if {[is_Cygwin]} { - set doc_path [exec cygpath --mixed $doc_path] - } } if {[file isfile $doc_path]} { @@ -4087,60 +4039,6 @@ set file_lists($ui_workdir) [list] wm title . "[appname] ([reponame]) [file normalize $_gitworktree]" focus -force $ui_comm -# -- Warn the user about environmental problems. Cygwin's Tcl -# does *not* pass its env array onto any processes it spawns. -# This means that git processes get none of our environment. -# -if {[is_Cygwin]} { - set ignored_env 0 - set suggest_user {} - set msg [mc "Possible environment issues exist. - -The following environment variables are probably -going to be ignored by any Git subprocess run -by %s: - -" [appname]] - foreach name [array names env] { - switch -regexp -- $name { - {^GIT_INDEX_FILE$} - - {^GIT_OBJECT_DIRECTORY$} - - {^GIT_ALTERNATE_OBJECT_DIRECTORIES$} - - {^GIT_DIFF_OPTS$} - - {^GIT_EXTERNAL_DIFF$} - - {^GIT_PAGER$} - - {^GIT_TRACE$} - - {^GIT_CONFIG$} - - {^GIT_(AUTHOR|COMMITTER)_DATE$} { - append msg " - $name\n" - incr ignored_env - } - {^GIT_(AUTHOR|COMMITTER)_(NAME|EMAIL)$} { - append msg " - $name\n" - incr ignored_env - set suggest_user $name - } - } - } - if {$ignored_env > 0} { - append msg [mc " -This is due to a known issue with the -Tcl binary distributed by Cygwin."] - - if {$suggest_user ne {}} { - append msg [mc " - -A good replacement for %s -is placing values for the user.name and -user.email settings into your personal -~/.gitconfig file. -" $suggest_user] - } - warn_popup $msg - } - unset ignored_env msg suggest_user name -} - # -- Only initialize complex UI if we are going to stay running. # if {[is_enabled transport]} { diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl index af1fee7..d23abed 100644 --- a/lib/choose_repository.tcl +++ b/lib/choose_repository.tcl @@ -174,9 +174,6 @@ constructor pick {} { -foreground blue \ -underline 1 set home $::env(HOME) - if {[is_Cygwin]} { - set home [exec cygpath --windows --absolute $home] - } set home "[file normalize $home]/" set hlen [string length $home] foreach p $sorted_recent { @@ -374,18 +371,6 @@ proc _objdir {path} { return $objdir } - if {[is_Cygwin]} { - set objdir [file join $path .git objects.lnk] - if {[file isfile $objdir]} { - return [win32_read_lnk $objdir] - } - - set objdir [file join $path objects.lnk] - if {[file isfile $objdir]} { - return [win32_read_lnk $objdir] - } - } - return {} } @@ -623,12 +608,6 @@ method _do_clone2 {} { } set giturl $origin_url - if {[is_Cygwin] && [file isdirectory $giturl]} { - set giturl [exec cygpath --unix --absolute $giturl] - if {$clone_type eq {shared}} { - set objdir [exec cygpath --unix --absolute $objdir] - } - } if {[file exists $local_path]} { error_popup [mc "Location %s already exists." $local_path] @@ -668,11 +647,7 @@ method _do_clone2 {} { fconfigure $f_cp -translation binary -encoding binary cd $objdir while {[gets $f_in line] >= 0} { - if {[is_Cygwin]} { - puts $f_cp [exec cygpath --unix --absolute $line] - } else { - puts $f_cp [file normalize $line] - } + puts $f_cp [file normalize $line] } close $f_in close $f_cp -- 2.41.0.99.19 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 3/4] git-gui - use cygstart to browse on Cygwin 2023-06-26 16:53 ` [PATCH v1 " Mark Levedahl 2023-06-26 16:53 ` [PATCH v1 1/4] git gui Makefile - remove Cygwin modifications Mark Levedahl 2023-06-26 16:53 ` [PATCH v1 2/4] git-gui - remove obsolete Cygwin specific code Mark Levedahl @ 2023-06-26 16:53 ` Mark Levedahl 2023-06-26 16:53 ` [PATCH v1 4/4] git-gui - use mkshortcut " Mark Levedahl ` (2 subsequent siblings) 5 siblings, 0 replies; 27+ messages in thread From: Mark Levedahl @ 2023-06-26 16:53 UTC (permalink / raw) To: mdl123, git Cc: adam, me, johannes.schindelin, gitster, sunshine, Mark Levedahl git-gui enables the "Repository->Explore Working Copy" menu on Cygwin, offering to open a Windows graphical file browser at the root of the working directory. This code, shared with Git For Windows support, depends upon use of Windows pathnames. However, git gui on Cygwin uses unix pathnames, so this shared code will not work on Cygwin. A base install of Cygwin provides the /bin/cygstart utility that runs a registered Windows application based upon the file type, after translating unix pathnames to Windows. Adding the --explore option guarantees that the Windows file explorer is opened, regardless of the supplied pathname's file type and avoiding possibility of some other action being taken. So, teach git-gui to use cygstart --explore on Cygwin, restoring the pre-2012 behavior of opening a Windows file explorer for browsing. This separates the Git For Windows and Cygwin code paths. Note that is_Windows is never true on Cygwin, and is_Cygwin is never true on Git for Windows, though this is not obvious by examining the code for those independent functions. Signed-off-by: Mark Levedahl <mlevedahl@gmail.com> --- changes since v0 -- assumes the if/else tree being modified is untouched by prior patches making the changes minimal and easier to review. git-gui.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/git-gui.sh b/git-gui.sh index 3f7c31e..8bc8892 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -2274,7 +2274,9 @@ proc do_git_gui {} { # Get the system-specific explorer app/command. proc get_explorer {} { - if {[is_Cygwin] || [is_Windows]} { + if {[is_Cygwin]} { + set explorer "/bin/cygstart.exe --explore" + } elseif {[is_Windows]} { set explorer "explorer.exe" } elseif {[is_MacOSX]} { set explorer "open" -- 2.41.0.99.19 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v1 4/4] git-gui - use mkshortcut on Cygwin 2023-06-26 16:53 ` [PATCH v1 " Mark Levedahl ` (2 preceding siblings ...) 2023-06-26 16:53 ` [PATCH v1 3/4] git-gui - use cygstart to browse on Cygwin Mark Levedahl @ 2023-06-26 16:53 ` Mark Levedahl 2023-06-27 11:51 ` [PATCH v1 0/4] Remove obsolete Cygwin support from git-gui Johannes Schindelin 2023-06-27 17:52 ` Junio C Hamano 5 siblings, 0 replies; 27+ messages in thread From: Mark Levedahl @ 2023-06-26 16:53 UTC (permalink / raw) To: mdl123, git Cc: adam, me, johannes.schindelin, gitster, sunshine, Mark Levedahl git-gui enables the "Repository->Create Desktop Icon" item on Cygwin, offering to create a shortcut that starts git-gui on the current repository. The code in do_cygwin_shortcut invokes function win32_create_lnk to create the shortcut. This latter function is shared between Cygwin and Git For Windows and expects Windows rather than unix pathnames, though do_cygwin_shortcut provides unix pathnames. Also, this function tries to invoke the Windows Script Host to run a javascript snippet, but this fails under Cygwin's Tcl. So, win32_create_lnk just does not support Cygwin. However, Cygwin's default installation provides /bin/mkshortcut for creating desktop shortcuts. This is compatible with exec under Cygwin's Tcl, understands Cygwin's unix pathnames, and avoids the need for shell escapes to encode troublesome paths. So, teach git-gui to use mkshortcut on Cygwin, leaving win32_create_lnk unchanged and for exclusive use by Git For Windows. Notes: "CHERE_INVOKING=1" is recognized by Cygwin's /etc/profile and prevents a "chdir $HOME", leaving the shell in the working directory specified by the shortcut. That directory is written directly by mkshortcut eliminating any problems with shell escapes and quoting. The code being replaced includes the full pathname of the git-gui creating the shortcut, but that git-gui might not be compatible with the git found after /etc/profile sets the path, and might have a pathname that defies encoding using shell escapes that can survive the multiple incompatible interpreters involved in the chain of creating and using this shortcut. The new code uses bare "git gui" as the command to execute, thus using the system git to launch the system git-gui, and avoiding both issues. Signed-off-by: Mark Levedahl <mlevedahl@gmail.com> --- changes since v0 -- assumes no changes to shortcut creation code in prior patches, so changes in this patch are easier to review. -- changed to use long option names for mkshortcut, better conforming to practice in git-gui overall. lib/shortcut.tcl | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/lib/shortcut.tcl b/lib/shortcut.tcl index 97d1d7a..674a41f 100644 --- a/lib/shortcut.tcl +++ b/lib/shortcut.tcl @@ -27,13 +27,10 @@ proc do_windows_shortcut {} { } proc do_cygwin_shortcut {} { - global argv0 _gitworktree + global argv0 _gitworktree oguilib if {[catch { set desktop [exec cygpath \ - --windows \ - --absolute \ - --long-name \ --desktop] }]} { set desktop . @@ -48,19 +45,19 @@ proc do_cygwin_shortcut {} { set fn ${fn}.lnk } if {[catch { - set sh [exec cygpath \ - --windows \ - --absolute \ - /bin/sh.exe] - set me [exec cygpath \ - --unix \ - --absolute \ - $argv0] - win32_create_lnk $fn [list \ - $sh -c \ - "CHERE_INVOKING=1 source /etc/profile;[sq $me] &" \ - ] \ - [file normalize $_gitworktree] + set repodir [file normalize $_gitworktree] + set shargs {-c \ + "CHERE_INVOKING=1 \ + source /etc/profile; \ + git gui"} + exec /bin/mkshortcut.exe \ + --arguments $shargs \ + --desc "git-gui on $repodir" \ + --icon $oguilib/git-gui.ico \ + --name $fn \ + --show min \ + --workingdir $repodir \ + /bin/sh.exe } err]} { error_popup [strcat [mc "Cannot write shortcut:"] "\n\n$err"] } -- 2.41.0.99.19 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v1 0/4] Remove obsolete Cygwin support from git-gui 2023-06-26 16:53 ` [PATCH v1 " Mark Levedahl ` (3 preceding siblings ...) 2023-06-26 16:53 ` [PATCH v1 4/4] git-gui - use mkshortcut " Mark Levedahl @ 2023-06-27 11:51 ` Johannes Schindelin 2023-06-27 17:52 ` Junio C Hamano 5 siblings, 0 replies; 27+ messages in thread From: Johannes Schindelin @ 2023-06-27 11:51 UTC (permalink / raw) To: Mark Levedahl; +Cc: mdl123, git, adam, me, gitster, sunshine Hi Mark, On Mon, 26 Jun 2023, Mark Levedahl wrote: > === This is an update, incorporating responses to Junio's and Eric's > comments: > -- clarified what the "upstream" git-gui branch is > -- Removed some changes from patch 2 as requested by Junio, reducing > changes in patch 3 and patch 4 > All code is fixed only after applying patch 4 > Differences in patch 3 and 4 are minimimized > -- updated comments to clarify G4w dedicated code. > -- updated all comments to (hopefully) clarify points of confusion > === And here is the range-diff: 1: 00000000000 ! 1: 00000000000 git gui Makefile - remove Cygwin modifications @@ Metadata Author: Mark Levedahl <mlevedahl@gmail.com> ## Commit message ## - git gui Makefile - remove Cygwin modiifications + git gui Makefile - remove Cygwin modifications git-gui's Makefile hardcodes the absolute Windows path of git-gui's libraries into git-gui, destroying the ability to package git-gui on one machine and @@ Commit message since 2012. Also, Cygwin does not support a non-Cygwin Tcl/Tk. The Cygwin git maintainer disables this code, so this code is definitely - not in use in the Cygwin distribution, and targets an untested / - unsupportable configuration. + not in use in the Cygwin distribution. - The simplest approach is to just delete the Cygwin specific code as - stock Cygwin needs no special handling. Do so. + The simplest fix is to just delete the Cygwin specific code, + allowing the Makefile to work out of the box on Cygwin. Do so. Signed-off-by: Mark Levedahl <mlevedahl@gmail.com> 2: 00000000000 ! 2: 00000000000 git-gui - remove obsolete Cygwin specific code @@ Commit message 8.4.1 Windows Tcl/Tk code. In March, 2012, that 8.4.1 package was replaced with a full port based upon the upstream unix/X11 code, since maintained up to date. The two Tcl/Tk packages are completely - incompatible, and have different sygnatures. + incompatible, and have different signatures. When Cygwin's Tcl/Tk signature changed in 2012, git-gui no longer detected Cygwin, so did not enable Cygwin specific code, and the POSIX @@ Commit message unix. Thus, no-one apparently noticed the existence of incompatible Cygwin specific code. - However, since commit c5766eae6f2b002396b6cd4f85b62317b707174e in - upstream git-gui, the is_Cygwin funcion does detect current Cygwin. The - Cygwin specific code is enabled, causing use of Windows rather than unix - pathnames, and enabling incorrect warnings about environment variables - that are not relevant for the fully functional unix/X11 Tcl/Tk. The end - result is that git-gui is now incommpatible with Cygwin. + However, since commit c5766eae6f in the git-gui source tree + (https://github.com/prati0100/git-gui, master at a5005ded), and not yet + pulled into the git repository, the is_Cygwin function does detect + Cygwin using the unix/X11 Tcl/Tk. The Cygwin specific code is enabled, + causing use of Windows rather than unix pathnames, and enabling + incorrect warnings about environment variables that were relevant only + to the old Tcl/Tk. The end result is that (upstream) git-gui is now + incompatible with Cygwin. - So, delete all Cygwin specific code (code protected by "if is_Cygwin"), - thus restoring the post-2012 behaviour. Note that Cygwin specific code - is required to enable file browsing and shortcut creation (supported - before 2012), but is not addressed in this patch. + So, delete Cygwin specific code (code protected by "if is_Cygwin") that + is not needed in any form to work with the unix/X11 Tcl/Tk. + + Cygwin specific code required to enable file browsing and shortcut + creation is not addressed in this patch, does not currently work, and + invocation of those items may leave git-gui in a confused state. Signed-off-by: Mark Levedahl <mlevedahl@gmail.com> @@ git-gui.sh: proc rescan {after {honor_trustmtime 1}} { } proc rescan_stage2 {fd after} { -@@ git-gui.sh: proc do_git_gui {} { - - # Get the system-specific explorer app/command. - proc get_explorer {} { -- if {[is_Cygwin] || [is_Windows]} { -+ if {[is_Windows]} { - set explorer "explorer.exe" - } elseif {[is_MacOSX]} { - set explorer "open" -@@ git-gui.sh: if {[is_enabled multicommit]} { - - .mbar.repository add separator - -- if {[is_Cygwin]} { -- .mbar.repository add command \ -- -label [mc "Create Desktop Icon"] \ -- -command do_cygwin_shortcut -- } elseif {[is_Windows]} { -+ if {[is_Windows]} { - .mbar.repository add command \ - -label [mc "Create Desktop Icon"] \ - -command do_windows_shortcut @@ git-gui.sh: if {[is_MacOSX]} { set doc_path [githtmldir] if {$doc_path ne {}} { @@ lib/choose_repository.tcl: method _do_clone2 {} { } close $f_in close $f_cp - - ## lib/shortcut.tcl ## -@@ lib/shortcut.tcl: proc do_windows_shortcut {} { - } - } - --proc do_cygwin_shortcut {} { -- global argv0 _gitworktree -- -- if {[catch { -- set desktop [exec cygpath \ -- --windows \ -- --absolute \ -- --long-name \ -- --desktop] -- }]} { -- set desktop . -- } -- set fn [tk_getSaveFile \ -- -parent . \ -- -title [mc "%s (%s): Create Desktop Icon" [appname] [reponame]] \ -- -initialdir $desktop \ -- -initialfile "Git [reponame].lnk"] -- if {$fn != {}} { -- if {[file extension $fn] ne {.lnk}} { -- set fn ${fn}.lnk -- } -- if {[catch { -- set sh [exec cygpath \ -- --windows \ -- --absolute \ -- /bin/sh.exe] -- set me [exec cygpath \ -- --unix \ -- --absolute \ -- $argv0] -- win32_create_lnk $fn [list \ -- $sh -c \ -- "CHERE_INVOKING=1 source /etc/profile;[sq $me] &" \ -- ] \ -- [file normalize $_gitworktree] -- } err]} { -- error_popup [strcat [mc "Cannot write shortcut:"] "\n\n$err"] -- } -- } --} -- - proc do_macosx_app {} { - global argv0 env - 3: 00000000000 ! 3: 00000000000 git-gui - use cygstart to browse on Cygwin @@ Metadata ## Commit message ## git-gui - use cygstart to browse on Cygwin - Pre-2012, git-gui enabled the "Repository->Explore Working Copy" menu on - Cygwin, offering open a Windows graphical file browser at the root - working directory. The old code relied upon internal use of Windows - pathnames, while git-gui must use unix pathnames on Cygwin since 2012, - so was removed in a previous patch. + git-gui enables the "Repository->Explore Working Copy" menu on Cygwin, + offering to open a Windows graphical file browser at the root of the + working directory. This code, shared with Git For Windows support, + depends upon use of Windows pathnames. However, git gui on Cygwin uses + unix pathnames, so this shared code will not work on Cygwin. A base install of Cygwin provides the /bin/cygstart utility that runs - arbtitrary Windows applications after translating unix pathnames to - Windows. Adding the --explore option guarantees that the Windows file - explorer is opened, regardless of the supplied pathname's file type and - avoiding possibility of some other action being taken. + a registered Windows application based upon the file type, after + translating unix pathnames to Windows. Adding the --explore option + guarantees that the Windows file explorer is opened, regardless of the + supplied pathname's file type and avoiding possibility of some other + action being taken. So, teach git-gui to use cygstart --explore on Cygwin, restoring the - pre-2012 behavior of opening a Windows file explorer for browsing. + pre-2012 behavior of opening a Windows file explorer for browsing. This + separates the Git For Windows and Cygwin code paths. Note that + is_Windows is never true on Cygwin, and is_Cygwin is never true on Git + for Windows, though this is not obvious by examining the code for those + independent functions. Signed-off-by: Mark Levedahl <mlevedahl@gmail.com> ## git-gui.sh ## @@ git-gui.sh: proc do_git_gui {} { + + # Get the system-specific explorer app/command. proc get_explorer {} { - if {[is_Windows]} { - set explorer "explorer.exe" -+ } elseif {[is_Cygwin]} { +- if {[is_Cygwin] || [is_Windows]} { ++ if {[is_Cygwin]} { + set explorer "/bin/cygstart.exe --explore" ++ } elseif {[is_Windows]} { + set explorer "explorer.exe" } elseif {[is_MacOSX]} { set explorer "open" - } else { 4: 00000000000 ! 4: 00000000000 git-gui - use mkshortcut on Cygwin @@ Metadata ## Commit message ## git-gui - use mkshortcut on Cygwin - Prior to 2012, git-gui enabled the "Repository->Create Desktop Icon" - item on Cygwin, offering to create a shortcut that starts git-gui on a - particular repository. The original code for this in lib/win32.tcl, - shared with Git for Windows support, requires Windows pathnames, while - git-gui must use unix pathnames with the unix/X11 Tcl/Tk since 2012. The - ability to use this from Cygwin was removed in a previous patch. + git-gui enables the "Repository->Create Desktop Icon" item on Cygwin, + offering to create a shortcut that starts git-gui on the current + repository. The code in do_cygwin_shortcut invokes function + win32_create_lnk to create the shortcut. This latter function is shared + between Cygwin and Git For Windows and expects Windows rather than unix + pathnames, though do_cygwin_shortcut provides unix pathnames. Also, this + function tries to invoke the Windows Script Host to run a javascript + snippet, but this fails under Cygwin's Tcl. So, win32_create_lnk just + does not support Cygwin. - Cygwin's default installation provides /bin/mkshortcut for creating - desktop shortuts, this is compatible with exec under tcl, and understands - Cygwin's unix pathnames. So, teach git-gui to use mkshortcut on Cygwin, - leaving lib/win32.tcl as Git for Windows specific support. + However, Cygwin's default installation provides /bin/mkshortcut for + creating desktop shortcuts. This is compatible with exec under Cygwin's + Tcl, understands Cygwin's unix pathnames, and avoids the need for shell + escapes to encode troublesome paths. So, teach git-gui to use mkshortcut + on Cygwin, leaving win32_create_lnk unchanged and for exclusive use by + Git For Windows. Notes: "CHERE_INVOKING=1" is recognized by Cygwin's /etc/profile and prevents a "chdir $HOME", leaving the shell in the working directory specified by the shortcut. That directory is written directly by mkshortcut eliminating any problems with shell escapes and quoting. - The pre-2012 code includes the full pathname of the git-gui creating the - shortcut (rather than using the system git-gui), but that git-gui might - not be compatible with the git found after /etc/profile sets the path, - and might have a pathname that defies encoding using shell escapes that - can survive the multiple incompatible interpreters involved in this - chain. Instead, use "git gui", thus defaulting to the system git and + The code being replaced includes the full pathname of the git-gui + creating the shortcut, but that git-gui might not be compatible with the + git found after /etc/profile sets the path, and might have a pathname + that defies encoding using shell escapes that can survive the multiple + incompatible interpreters involved in the chain of creating and using + this shortcut. The new code uses bare "git gui" as the command to + execute, thus using the system git to launch the system git-gui, and avoiding both issues. Signed-off-by: Mark Levedahl <mlevedahl@gmail.com> - ## git-gui.sh ## -@@ git-gui.sh: if {[is_enabled multicommit]} { - .mbar.repository add command \ - -label [mc "Create Desktop Icon"] \ - -command do_windows_shortcut -+ } elseif {[is_Cygwin]} { -+ .mbar.repository add command \ -+ -label [mc "Create Desktop Icon"] \ -+ -command do_cygwin_shortcut - } elseif {[is_MacOSX]} { - .mbar.repository add command \ - -label [mc "Create Desktop Icon"] \ - ## lib/shortcut.tcl ## @@ lib/shortcut.tcl: proc do_windows_shortcut {} { - } } -+proc do_cygwin_shortcut {} { + proc do_cygwin_shortcut {} { +- global argv0 _gitworktree + global argv0 _gitworktree oguilib -+ -+ if {[catch { -+ set desktop [exec cygpath \ -+ --desktop] -+ }]} { -+ set desktop . -+ } -+ set fn [tk_getSaveFile \ -+ -parent . \ -+ -title [mc "%s (%s): Create Desktop Icon" [appname] [reponame]] \ -+ -initialdir $desktop \ -+ -initialfile "Git [reponame].lnk"] -+ if {$fn != {}} { -+ if {[file extension $fn] ne {.lnk}} { -+ set fn ${fn}.lnk -+ } -+ if {[catch { + + if {[catch { + set desktop [exec cygpath \ +- --windows \ +- --absolute \ +- --long-name \ + --desktop] + }]} { + set desktop . +@@ lib/shortcut.tcl: proc do_cygwin_shortcut {} { + set fn ${fn}.lnk + } + if {[catch { +- set sh [exec cygpath \ +- --windows \ +- --absolute \ +- /bin/sh.exe] +- set me [exec cygpath \ +- --unix \ +- --absolute \ +- $argv0] +- win32_create_lnk $fn [list \ +- $sh -c \ +- "CHERE_INVOKING=1 source /etc/profile;[sq $me] &" \ +- ] \ +- [file normalize $_gitworktree] + set repodir [file normalize $_gitworktree] + set shargs {-c \ + "CHERE_INVOKING=1 \ + source /etc/profile; \ + git gui"} + exec /bin/mkshortcut.exe \ -+ -a $shargs \ -+ -d "git-gui on $repodir" \ -+ -i $oguilib/git-gui.ico \ -+ -n $fn \ -+ -s min \ -+ -w $repodir \ ++ --arguments $shargs \ ++ --desc "git-gui on $repodir" \ ++ --icon $oguilib/git-gui.ico \ ++ --name $fn \ ++ --show min \ ++ --workingdir $repodir \ + /bin/sh.exe -+ } err]} { -+ error_popup [strcat [mc "Cannot write shortcut:"] "\n\n$err"] -+ } -+ } -+} -+ - proc do_macosx_app {} { - global argv0 env - + } err]} { + error_popup [strcat [mc "Cannot write shortcut:"] "\n\n$err"] + } FWIW even v1 looked good to me (I don't care about typos as long as they don't change meaning). I tested the changes on top of Git for Windows and everything works as expected (if you want to cross check my work, look no further than the `git-gui-cygwin` branch at https://github.com/dscho/git). If you want, please feel free to add Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de> Thank you! Johannes ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 0/4] Remove obsolete Cygwin support from git-gui 2023-06-26 16:53 ` [PATCH v1 " Mark Levedahl ` (4 preceding siblings ...) 2023-06-27 11:51 ` [PATCH v1 0/4] Remove obsolete Cygwin support from git-gui Johannes Schindelin @ 2023-06-27 17:52 ` Junio C Hamano 2023-08-05 14:47 ` Mark Levedahl 2023-08-29 16:03 ` Mark Levedahl 5 siblings, 2 replies; 27+ messages in thread From: Junio C Hamano @ 2023-06-27 17:52 UTC (permalink / raw) To: Mark Levedahl; +Cc: mdl123, git, adam, me, johannes.schindelin, sunshine Mark Levedahl <mlevedahl@gmail.com> writes: > === This is an update, incorporating responses to Junio's and Eric's > comments: > -- clarified what the "upstream" git-gui branch is > -- Removed some changes from patch 2 as requested by Junio, reducing > changes in patch 3 and patch 4 > All code is fixed only after applying patch 4 > Differences in patch 3 and 4 are minimimized > -- updated comments to clarify G4w dedicated code. > -- updated all comments to (hopefully) clarify points of confusion > === > ... > Mark Levedahl (4): > git gui Makefile - remove Cygwin modifications > git-gui - remove obsolete Cygwin specific code > git-gui - use cygstart to browse on Cygwin > git-gui - use mkshortcut on Cygwin > > Makefile | 21 +------ > git-gui.sh | 118 +++----------------------------------- > lib/choose_repository.tcl | 27 +-------- > lib/shortcut.tcl | 31 +++++----- > 4 files changed, 27 insertions(+), 170 deletions(-) OK, Dscho says v1 looks good, and I have no further comments. Pratyush, can I expect that you take further comments and usher these patches to your tree, and eventually tell me to pull from your repository? Thanks, all. ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 0/4] Remove obsolete Cygwin support from git-gui 2023-06-27 17:52 ` Junio C Hamano @ 2023-08-05 14:47 ` Mark Levedahl 2023-08-24 15:54 ` Pratyush Yadav 2023-08-29 16:03 ` Mark Levedahl 1 sibling, 1 reply; 27+ messages in thread From: Mark Levedahl @ 2023-08-05 14:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: mdl123, git, adam, me, johannes.schindelin, sunshine On 6/27/23 13:52, Junio C Hamano wrote: > Mark Levedahl <mlevedahl@gmail.com> writes: > >> === This is an update, incorporating responses to Junio's and Eric's >> comments: >> -- clarified what the "upstream" git-gui branch is >> -- Removed some changes from patch 2 as requested by Junio, reducing >> changes in patch 3 and patch 4 >> All code is fixed only after applying patch 4 >> Differences in patch 3 and 4 are minimimized >> -- updated comments to clarify G4w dedicated code. >> -- updated all comments to (hopefully) clarify points of confusion >> === >> ... >> Mark Levedahl (4): >> git gui Makefile - remove Cygwin modifications >> git-gui - remove obsolete Cygwin specific code >> git-gui - use cygstart to browse on Cygwin >> git-gui - use mkshortcut on Cygwin >> >> Makefile | 21 +------ >> git-gui.sh | 118 +++----------------------------------- >> lib/choose_repository.tcl | 27 +-------- >> lib/shortcut.tcl | 31 +++++----- >> 4 files changed, 27 insertions(+), 170 deletions(-) > OK, Dscho says v1 looks good, and I have no further comments. > > Pratyush, can I expect that you take further comments and usher > these patches to your tree, and eventually tell me to pull from your > repository? > > Thanks, all. Junio, Thank you and Dscho for the detailed reviews. But, there is no response from Pratyush in over a month, is there a different maintainer then who should take this? Mark ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 0/4] Remove obsolete Cygwin support from git-gui 2023-08-05 14:47 ` Mark Levedahl @ 2023-08-24 15:54 ` Pratyush Yadav 0 siblings, 0 replies; 27+ messages in thread From: Pratyush Yadav @ 2023-08-24 15:54 UTC (permalink / raw) To: Mark Levedahl Cc: Junio C Hamano, mdl123, git, adam, me, johannes.schindelin, sunshine On Sat, Aug 05 2023, Mark Levedahl wrote: > On 6/27/23 13:52, Junio C Hamano wrote: >> Mark Levedahl <mlevedahl@gmail.com> writes: >> >>> === This is an update, incorporating responses to Junio's and Eric's >>> comments: >>> -- clarified what the "upstream" git-gui branch is >>> -- Removed some changes from patch 2 as requested by Junio, reducing >>> changes in patch 3 and patch 4 >>> All code is fixed only after applying patch 4 >>> Differences in patch 3 and 4 are minimimized >>> -- updated comments to clarify G4w dedicated code. >>> -- updated all comments to (hopefully) clarify points of confusion >>> === >>> ... >>> Mark Levedahl (4): >>> git gui Makefile - remove Cygwin modifications >>> git-gui - remove obsolete Cygwin specific code >>> git-gui - use cygstart to browse on Cygwin >>> git-gui - use mkshortcut on Cygwin >>> >>> Makefile | 21 +------ >>> git-gui.sh | 118 +++----------------------------------- >>> lib/choose_repository.tcl | 27 +-------- >>> lib/shortcut.tcl | 31 +++++----- >>> 4 files changed, 27 insertions(+), 170 deletions(-) >> OK, Dscho says v1 looks good, and I have no further comments. >> >> Pratyush, can I expect that you take further comments and usher >> these patches to your tree, and eventually tell me to pull from your >> repository? >> >> Thanks, all. > > Junio, > > Thank you and Dscho for the detailed reviews. But, there is no response from > Pratyush in over a month, is there a different maintainer then who should take > this? Almost 2 months now... I'm sorry. I just do not find enough time or energy for git-gui these days. More on that later. For now, I took a brief look at the patches. They look good to me. I appreciate the detailed commit messages. I did not test them since I do not have a Windows setup currently, but I believe Johannes did so it's all good for me. Applied to git-gui/master. Will send a pull request soon. -- Regards, Pratyush Yadav ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 0/4] Remove obsolete Cygwin support from git-gui 2023-06-27 17:52 ` Junio C Hamano 2023-08-05 14:47 ` Mark Levedahl @ 2023-08-29 16:03 ` Mark Levedahl 2023-08-29 16:18 ` Junio C Hamano 1 sibling, 1 reply; 27+ messages in thread From: Mark Levedahl @ 2023-08-29 16:03 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, adam, me, johannes.schindelin, sunshine On 6/27/23 13:52, Junio C Hamano wrote: > OK, Dscho says v1 looks good, and I have no further comments. > > Pratyush, can I expect that you take further comments and usher > these patches to your tree, and eventually tell me to pull from your > repository? > > Thanks, all. Junio, I see you merged latest git-gui from Pratyush onto next. As git-gui has no test facility I merged your py/git-gui-updates (a7935203) into your master (5dc72c0f), built on both Linux and Cygwin. git-gui works as expected on both. Looks good to me. Mark ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1 0/4] Remove obsolete Cygwin support from git-gui 2023-08-29 16:03 ` Mark Levedahl @ 2023-08-29 16:18 ` Junio C Hamano 0 siblings, 0 replies; 27+ messages in thread From: Junio C Hamano @ 2023-08-29 16:18 UTC (permalink / raw) To: Mark Levedahl; +Cc: git, adam, me, johannes.schindelin, sunshine Mark Levedahl <mlevedahl@gmail.com> writes: > On 6/27/23 13:52, Junio C Hamano wrote: >> OK, Dscho says v1 looks good, and I have no further comments. >> >> Pratyush, can I expect that you take further comments and usher >> these patches to your tree, and eventually tell me to pull from your >> repository? >> >> Thanks, all. > > Junio, > > I see you merged latest git-gui from Pratyush onto next. As git-gui > has no test facility I merged your py/git-gui-updates (a7935203) into > your master (5dc72c0f), built on both Linux and Cygwin. git-gui works > as expected on both. Looks good to me. Thanks! ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2023-08-29 16:19 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-24 21:23 [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui Mark Levedahl 2023-06-24 21:23 ` [PATCH v0 1/4] git gui Makefile - remove Cygwin modiifications Mark Levedahl 2023-06-24 21:23 ` [PATCH v0 2/4] git-gui - remove obsolete Cygwin specific code Mark Levedahl 2023-06-25 2:56 ` Eric Sunshine 2023-06-25 11:29 ` Mark Levedahl 2023-06-24 21:23 ` [PATCH v0 3/4] git-gui - use cygstart to browse on Cygwin Mark Levedahl 2023-06-24 21:23 ` [PATCH v0 4/4] git-gui - use mkshortcut " Mark Levedahl 2023-06-24 23:30 ` [PATCH v0 0/4] Remove obsolete Cygwin support from git-gui Junio C Hamano 2023-06-24 23:35 ` Junio C Hamano 2023-06-25 11:28 ` Mark Levedahl 2023-06-25 11:26 ` Mark Levedahl 2023-06-25 12:10 ` Mark Levedahl 2023-06-25 15:46 ` Junio C Hamano 2023-06-25 17:01 ` Mark Levedahl 2023-06-26 15:52 ` Junio C Hamano 2023-06-26 16:55 ` Mark Levedahl 2023-06-26 16:53 ` [PATCH v1 " Mark Levedahl 2023-06-26 16:53 ` [PATCH v1 1/4] git gui Makefile - remove Cygwin modifications Mark Levedahl 2023-06-26 16:53 ` [PATCH v1 2/4] git-gui - remove obsolete Cygwin specific code Mark Levedahl 2023-06-26 16:53 ` [PATCH v1 3/4] git-gui - use cygstart to browse on Cygwin Mark Levedahl 2023-06-26 16:53 ` [PATCH v1 4/4] git-gui - use mkshortcut " Mark Levedahl 2023-06-27 11:51 ` [PATCH v1 0/4] Remove obsolete Cygwin support from git-gui Johannes Schindelin 2023-06-27 17:52 ` Junio C Hamano 2023-08-05 14:47 ` Mark Levedahl 2023-08-24 15:54 ` Pratyush Yadav 2023-08-29 16:03 ` Mark Levedahl 2023-08-29 16:18 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).