Skip to content

git-gui: GIT_ASK_YESNO/GIT_ASKPASS patches from Git for Windows #358

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: git-gui/master
Choose a base branch
from
Open

git-gui: GIT_ASK_YESNO/GIT_ASKPASS patches from Git for Windows #358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5897206
git-gui: provide question helper for retry fallback on Windows
hvoigt Feb 21, 2010
cb9f8c8
git gui: set GIT_ASKPASS=git-gui--askpass if not set yet
dscho Jul 23, 2010
8cfc935
git-gui--askyesno: allow overriding the window title
dscho Sep 20, 2017
6025b38
git-gui--askyesno (mingw): use Git for Windows' icon, if available
dscho Sep 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ install: all
$(QUIET)$(INSTALL_D0)'$(DESTDIR_SQ)$(gitexecdir_SQ)' $(INSTALL_D1)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Pratyush Yadav wrote (reply to this):

On 26/09/19 08:29AM, Heiko Voigt via GitGitGadget wrote:
> From: Heiko Voigt <[email protected]>
> 
> Make use of the new environment variable GIT_ASK_YESNO to support the
> recently implemented fallback in case unlink, rename or rmdir fail for
> files in use on Windows. The added dialog will present a yes/no question
> to the the user which will currently be used by the windows compat layer
> to let the user retry a failed file operation.

I can't find any documentation related to this environment variable. A 
Google search does not yield any promising results. I don't see this 
mentioned in the git man page either, though that is to be expected 
because it seems to be a Windows-only variable.

My point is, it would be nice if the commit message pointed to some sort 
of documentation for the environment variable. It would also help me in 
reviewing the patch if I know more about the variable.

Either way, some minor comments down below.

> 
> Signed-off-by: Heiko Voigt <[email protected]>
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
>  Makefile          |  2 ++
>  git-gui--askyesno | 51 +++++++++++++++++++++++++++++++++++++++++++++++
>  git-gui.sh        |  3 +++
>  3 files changed, 56 insertions(+)
>  create mode 100755 git-gui--askyesno
> 
> diff --git a/Makefile b/Makefile
> index fe30be38dc..85633b73df 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -291,6 +291,7 @@ install: all
>  	$(QUIET)$(INSTALL_D0)'$(DESTDIR_SQ)$(gitexecdir_SQ)' $(INSTALL_D1)
>  	$(QUIET)$(INSTALL_X0)git-gui $(INSTALL_X1) '$(DESTDIR_SQ)$(gitexecdir_SQ)'
>  	$(QUIET)$(INSTALL_X0)git-gui--askpass $(INSTALL_X1) '$(DESTDIR_SQ)$(gitexecdir_SQ)'
> +	$(QUIET)$(INSTALL_X0)git-gui--askyesno $(INSTALL_X1) '$(DESTDIR_SQ)$(gitexecdir_SQ)'
>  	$(QUIET)$(foreach p,$(GITGUI_BUILT_INS), $(INSTALL_L0)'$(DESTDIR_SQ)$(gitexecdir_SQ)/$p' $(INSTALL_L1)'$(DESTDIR_SQ)$(gitexecdir_SQ)/git-gui' $(INSTALL_L2)'$(DESTDIR_SQ)$(gitexecdir_SQ)/$p' $(INSTALL_L3) &&) true
>  ifdef GITGUI_WINDOWS_WRAPPER
>  	$(QUIET)$(INSTALL_R0)git-gui.tcl $(INSTALL_R1) '$(DESTDIR_SQ)$(gitexecdir_SQ)'
> @@ -309,6 +310,7 @@ uninstall:
>  	$(QUIET)$(CLEAN_DST) '$(DESTDIR_SQ)$(gitexecdir_SQ)'
>  	$(QUIET)$(REMOVE_F0)'$(DESTDIR_SQ)$(gitexecdir_SQ)'/git-gui $(REMOVE_F1)
>  	$(QUIET)$(REMOVE_F0)'$(DESTDIR_SQ)$(gitexecdir_SQ)'/git-gui--askpass $(REMOVE_F1)
> +	$(QUIET)$(REMOVE_F0)'$(DESTDIR_SQ)$(gitexecdir_SQ)'/git-gui--askyesno $(REMOVE_F1)
>  	$(QUIET)$(foreach p,$(GITGUI_BUILT_INS), $(REMOVE_F0)'$(DESTDIR_SQ)$(gitexecdir_SQ)'/$p $(REMOVE_F1) &&) true
>  ifdef GITGUI_WINDOWS_WRAPPER
>  	$(QUIET)$(REMOVE_F0)'$(DESTDIR_SQ)$(gitexecdir_SQ)'/git-gui.tcl $(REMOVE_F1)

I'm not too slick with Makefiles, but these changes look good.

> diff --git a/git-gui--askyesno b/git-gui--askyesno
> new file mode 100755
> index 0000000000..cf9c990d09
> --- /dev/null
> +++ b/git-gui--askyesno
> @@ -0,0 +1,51 @@
> +#!/bin/sh
> +# Tcl ignores the next line -*- tcl -*- \
> +exec wish "$0" -- "$@"
> +
> +# This is an implementation of a simple yes no dialog
> +# which is injected into the git commandline by git gui
> +# in case a yesno question needs to be answered.
> +
> +set NS {}
> +set use_ttk [package vsatisfies [package provide Tk] 8.5]
> +if {$use_ttk} {
> +	set NS ttk
> +}
> +
> +if {$argc < 1} {
> +	puts stderr "Usage: $argv0 <question>"
> +	exit 1
> +} else {
> +	set prompt [join $argv " "]
> +}
> +
> +${NS}::frame .t
> +${NS}::label .t.m -text $prompt -justify center -width 400px
> +.t.m configure -wraplength 400px
> +pack .t.m -side top -fill x -padx 20 -pady 20 -expand 1
> +pack .t -side top -fill x -ipadx 20 -ipady 20 -expand 1
> +
> +${NS}::frame .b
> +${NS}::frame .b.left -width 200
> +${NS}::button .b.yes -text Yes -command yes
> +${NS}::button .b.no  -text No  -command no

Do you really need separate functions for yes/no? I think something like

  -command {exit 1}

and

  -command {exit 0}

would do just fine.

Either way...

> +
> +

Nitpick: Drop one of the two blank lines.

> +pack .b.left -side left -expand 1 -fill x
> +pack .b.yes -side left -expand 1
> +pack .b.no -side right -expand 1 -ipadx 5
> +pack .b -side bottom -fill x -ipadx 20 -ipady 15
> +
> +bind . <Key-Return> {exit 0}
> +bind . <Key-Escape> {exit 1}

... do the same thing here. Call yes and no here too if you are using 
them above.

I have no preference for either way, but I would like uniformity in 
these two spots.

> +
> +proc no {} {
> +	exit 1
> +}
> +
> +proc yes {} {
> +	exit 0
> +}
> +
> +wm title . "Question?"
> +tk::PlaceWindow .
> diff --git a/git-gui.sh b/git-gui.sh
> index f9b323abff..76d8139b8d 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -1248,6 +1248,9 @@ set have_tk85 [expr {[package vcompare $tk_version "8.5"] >= 0}]
>  if {![info exists env(SSH_ASKPASS)]} {
>  	set env(SSH_ASKPASS) [gitexec git-gui--askpass]
>  }
> +if {![info exists env(GIT_ASK_YESNO)]} {
> +	set env(GIT_ASK_YESNO) [gitexec git-gui--askyesno]
> +}

Since this seems to be a Windows-only variable, you might want to enable 
it only on Windows. Are there workflows on other platforms that would 
use this environment variable?

-- 
Regards,
Pratyush Yadav

$(QUIET)$(INSTALL_X0)git-gui $(INSTALL_X1) '$(DESTDIR_SQ)$(gitexecdir_SQ)'
$(QUIET)$(INSTALL_X0)git-gui--askpass $(INSTALL_X1) '$(DESTDIR_SQ)$(gitexecdir_SQ)'
$(QUIET)$(INSTALL_X0)git-gui--askyesno $(INSTALL_X1) '$(DESTDIR_SQ)$(gitexecdir_SQ)'
$(QUIET)$(foreach p,$(GITGUI_BUILT_INS), $(INSTALL_L0)'$(DESTDIR_SQ)$(gitexecdir_SQ)/$p' $(INSTALL_L1)'$(DESTDIR_SQ)$(gitexecdir_SQ)/git-gui' $(INSTALL_L2)'$(DESTDIR_SQ)$(gitexecdir_SQ)/$p' $(INSTALL_L3) &&) true
ifdef GITGUI_WINDOWS_WRAPPER
$(QUIET)$(INSTALL_R0)git-gui.tcl $(INSTALL_R1) '$(DESTDIR_SQ)$(gitexecdir_SQ)'
Expand All @@ -309,6 +310,7 @@ uninstall:
$(QUIET)$(CLEAN_DST) '$(DESTDIR_SQ)$(gitexecdir_SQ)'
$(QUIET)$(REMOVE_F0)'$(DESTDIR_SQ)$(gitexecdir_SQ)'/git-gui $(REMOVE_F1)
$(QUIET)$(REMOVE_F0)'$(DESTDIR_SQ)$(gitexecdir_SQ)'/git-gui--askpass $(REMOVE_F1)
$(QUIET)$(REMOVE_F0)'$(DESTDIR_SQ)$(gitexecdir_SQ)'/git-gui--askyesno $(REMOVE_F1)
$(QUIET)$(foreach p,$(GITGUI_BUILT_INS), $(REMOVE_F0)'$(DESTDIR_SQ)$(gitexecdir_SQ)'/$p $(REMOVE_F1) &&) true
ifdef GITGUI_WINDOWS_WRAPPER
$(QUIET)$(REMOVE_F0)'$(DESTDIR_SQ)$(gitexecdir_SQ)'/git-gui.tcl $(REMOVE_F1)
Expand Down
68 changes: 68 additions & 0 deletions git-gui--askyesno
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
#!/bin/sh
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Pratyush Yadav wrote (reply to this):

One minor nitpick: please add a comment at the top of the file 
documenting the `--title` option, and the usage of the program in 
general.

Other than that, looks good. Thanks.

On 26/09/19 08:29AM, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[email protected]>
> 
> "Question?" is maybe not the most informative thing to ask. In the
> absence of better information, it is the best we can do, of course.
> 
> However, Git for Windows' auto updater just learned the trick to use
> git-gui--askyesno to ask the user whether to update now or not. And in
> this scripted scenario, we can easily pass a command-line option to
> change the window title.
> 
> So let's support that with the new `--title <title>` option.
> 
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
>  git-gui--askyesno | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/git-gui--askyesno b/git-gui--askyesno
> index cf9c990d09..45b0260eff 100755
> --- a/git-gui--askyesno
> +++ b/git-gui--askyesno
> @@ -12,10 +12,15 @@ if {$use_ttk} {
>  	set NS ttk
>  }
>  
> +set title "Question?"
>  if {$argc < 1} {
>  	puts stderr "Usage: $argv0 <question>"
>  	exit 1
>  } else {
> +	if {$argc > 2 && [lindex $argv 0] == "--title"} {

While this is probably not the most robust way of handling command line 
arguments, I guess it doesn't really make too much of a difference for 
something this simple.

> +		set title [lindex $argv 1]
> +		set argv [lreplace $argv 0 1]
> +	}
>  	set prompt [join $argv " "]
>  }
>  
> @@ -47,5 +52,5 @@ proc yes {} {
>  	exit 0
>  }
>  
> -wm title . "Question?"
> +wm title . $title
>  tk::PlaceWindow .
> -- 
> gitgitgadget
> 

-- 
Regards,
Pratyush Yadav

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Pratyush Yadav wrote (reply to this):

Since this is a git-gui dialog/prompt, why not use the git-gui icon? 
This will mean some uniformity between all the platforms (though I'm not 
sure if other platforms even use GIT_ASK_YESNO). It would also probably 
save you the hacks needed to find out the git-for-windows icon.

Well, there is the problem that the git-gui logo is not in any external 
file, and is inside git-gui.sh (as a vector image, but I'm not sure). 
But I'd like to at least start some discussion in this direction.

On 26/09/19 08:30AM, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[email protected]>
> 
> For additional GUI goodness.
> 
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
>  git-gui--askyesno | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/git-gui--askyesno b/git-gui--askyesno
> index 45b0260eff..c0c82e7cbd 100755
> --- a/git-gui--askyesno
> +++ b/git-gui--askyesno
> @@ -52,5 +52,17 @@ proc yes {} {
>  	exit 0
>  }
>  
> +if {$::tcl_platform(platform) eq {windows}} {
> +	set icopath [file dirname [file normalize $argv0]]
> +	if {[file tail $icopath] eq {git-core}} {
> +		set icopath [file dirname $icopath]
> +	}
> +	set icopath [file dirname $icopath]
> +	set icopath [file join $icopath share git git-for-windows.ico]
> +	if {[file exists $icopath]} {
> +		wm iconbitmap . -default $icopath
> +	}
> +}
> +
>  wm title . $title
>  tk::PlaceWindow .
> -- 
> gitgitgadget

-- 
Regards,
Pratyush Yadav

# Tcl ignores the next line -*- tcl -*- \
exec wish "$0" -- "$@"

# This is an implementation of a simple yes no dialog
# which is injected into the git commandline by git gui
# in case a yesno question needs to be answered.

set NS {}
set use_ttk [package vsatisfies [package provide Tk] 8.5]
if {$use_ttk} {
set NS ttk
}

set title "Question?"
if {$argc < 1} {
puts stderr "Usage: $argv0 <question>"
exit 1
} else {
if {$argc > 2 && [lindex $argv 0] == "--title"} {
set title [lindex $argv 1]
set argv [lreplace $argv 0 1]
}
set prompt [join $argv " "]
}

${NS}::frame .t
${NS}::label .t.m -text $prompt -justify center -width 400px
.t.m configure -wraplength 400px
pack .t.m -side top -fill x -padx 20 -pady 20 -expand 1
pack .t -side top -fill x -ipadx 20 -ipady 20 -expand 1

${NS}::frame .b
${NS}::frame .b.left -width 200
${NS}::button .b.yes -text Yes -command yes
${NS}::button .b.no -text No -command no


pack .b.left -side left -expand 1 -fill x
pack .b.yes -side left -expand 1
pack .b.no -side right -expand 1 -ipadx 5
pack .b -side bottom -fill x -ipadx 20 -ipady 15

bind . <Key-Return> {exit 0}
bind . <Key-Escape> {exit 1}

proc no {} {
exit 1
}

proc yes {} {
exit 0
}

if {$::tcl_platform(platform) eq {windows}} {
set icopath [file dirname [file normalize $argv0]]
if {[file tail $icopath] eq {git-core}} {
set icopath [file dirname $icopath]
}
set icopath [file dirname $icopath]
set icopath [file join $icopath share git git-for-windows.ico]
if {[file exists $icopath]} {
wm iconbitmap . -default $icopath
}
}

wm title . $title
tk::PlaceWindow .
6 changes: 6 additions & 0 deletions git-gui.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1248,6 +1248,12 @@ set have_tk85 [expr {[package vcompare $tk_version "8.5"] >= 0}]
if {![info exists env(SSH_ASKPASS)]} {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Pratyush Yadav wrote (reply to this):

What is the difference between SSH_ASKPASS and GIT_ASKPASS? On my first 
read, I assumed SSH_ASKPASS is replaced by GIT_ASKPASS, but I might be 
wrong.

On 26/09/19 08:29AM, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[email protected]>
> 
> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
>  git-gui.sh | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/git-gui.sh b/git-gui.sh
> index 76d8139b8d..66f046a0c7 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -1248,6 +1248,9 @@ set have_tk85 [expr {[package vcompare $tk_version "8.5"] >= 0}]
>  if {![info exists env(SSH_ASKPASS)]} {
>  	set env(SSH_ASKPASS) [gitexec git-gui--askpass]
>  }
> +if {![info exists env(GIT_ASKPASS)]} {
> +	set env(GIT_ASKPASS) [gitexec git-gui--askpass]
> +}
>  if {![info exists env(GIT_ASK_YESNO)]} {
>  	set env(GIT_ASK_YESNO) [gitexec git-gui--askyesno]
>  }

-- 
Regards,
Pratyush Yadav

set env(SSH_ASKPASS) [gitexec git-gui--askpass]
}
if {![info exists env(GIT_ASKPASS)]} {
set env(GIT_ASKPASS) [gitexec git-gui--askpass]
}
if {![info exists env(GIT_ASK_YESNO)]} {
set env(GIT_ASK_YESNO) [gitexec git-gui--askyesno]
}

######################################################################
##
Expand Down