From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Mark Levedahl <mlevedahl@gmail.com>
Cc: mdl123@verizon.net, git@vger.kernel.org, adam@dinwoodie.org,
me@yadavpratyush.com, gitster@pobox.com, sunshine@sunshineco.com
Subject: Re: [PATCH v1 0/4] Remove obsolete Cygwin support from git-gui
Date: Tue, 27 Jun 2023 13:51:15 +0200 (CEST) [thread overview]
Message-ID: <5c1dc85f-f3a3-2e60-be85-08eefb633e5c@gmx.de> (raw)
In-Reply-To: <20230626165305.37488-1-mlevedahl@gmail.com>
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
next prev parent reply other threads:[~2023-06-27 11:51 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
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 ` Johannes Schindelin [this message]
2023-06-27 17:52 ` [PATCH v1 0/4] Remove obsolete Cygwin support from git-gui 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5c1dc85f-f3a3-2e60-be85-08eefb633e5c@gmx.de \
--to=johannes.schindelin@gmx.de \
--cc=adam@dinwoodie.org \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=mdl123@verizon.net \
--cc=me@yadavpratyush.com \
--cc=mlevedahl@gmail.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).