threads / patch / 24286

patchgit-gui--askpass: Do not hang on exit

Subject: [PATCH git-gui] git-gui--askpass: Do not hang on exit

## tl;dr

6 messages between Jul 4, 2010 and Aug 18, 2010. Diffs are folded; open one to read it.

replies: 5people: 2as markdown or json

Jonathan Nieder· Jul 4, 2010, 21:21 UTC · lore

Anders Kaseorg wrote:

> When git-gui--askpass is manually copied from the source into
> /usr/lib/git-core, though, it doesn’t particularly work; its window
> just freezes after the password is typed.

The problem seems to be the

  bind . <Destroy>    {exit $::rc}

line; apparently each exit causes the window to be destroyed again, resulting in git gui hanging.

Reported-by: Anders Kaseorg <andersk@mit.edu>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---

Show changes to git-gui--askpass +3 −1

diff --git a/git-gui--askpass b/git-gui--askpass
index 12e117e..1537f75 100755
--- a/git-gui--askpass
+++ b/git-gui--askpass
@@ -39,7 +39,7 @@ pack .b -side bottom -fill x -padx 10 -pady 10
 bind . <Visibility> {focus -force .e}
 bind . <Key-Return> finish
 bind . <Key-Escape> {destroy .}
-bind . <Destroy>    {exit $rc}
+bind . <Destroy>    {exit $::rc}
 
 proc finish {} {
 	if {$::yesno} {
@@ -52,7 +52,9 @@ proc finish {} {
 
 	set ::rc 0
 	puts $::answer
+	bind . <Destroy> {}
 	destroy .
+	exit 0
 }
 
 wm title . "OpenSSH"

Jonathan Nieder· Jul 4, 2010, 21:24 UTC · re: Jonathan Nieder · lore

[PATCH v2 git-gui] git-gui--askpass: Do not hang on exit

Anders Kaseorg wrote:

> When git-gui--askpass is manually copied from the source into
> /usr/lib/git-core, though, it doesn’t particularly work; its window
> just freezes after the password is typed.

The problem seems to be the

  bind . <Destroy>    {exit $::rc}

line; apparently each exit causes the window to be destroyed again, resulting in git gui hanging.

Reported-by: Anders Kaseorg <andersk@mit.edu>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 I wrote:

 > @@ -52,7 +52,9 @@ proc finish {} {
 >  
 >  	set ::rc 0
 >  	puts $::answer
 > +	bind . <Destroy> {}
 >  	destroy .

 but that does nothing to help the case when the destroy event
 comes from the window manager.  Here’s a saner patch (sorry for
 the noise).

Show changes to git-gui--askpass +4 −1

diff --git a/git-gui--askpass b/git-gui--askpass
index 12e117e..20b8799 100755
--- a/git-gui--askpass
+++ b/git-gui--askpass
@@ -39,7 +39,10 @@ pack .b -side bottom -fill x -padx 10 -pady 10
 bind . <Visibility> {focus -force .e}
 bind . <Key-Return> finish
 bind . <Key-Escape> {destroy .}
-bind . <Destroy>    {exit $rc}
+bind . <Destroy>    {
+	bind . <Destroy> {}
+	exit $::rc
+}
 
 proc finish {} {
 	if {$::yesno} {

Pat Thoyts· Aug 14, 2010, 23:21 UTC · re: Jonathan Nieder · lore

[PATCH] git-gui: change the termination checks to avoid potential hang.

Supposedly the askpass utility can hang under some circumstances. This patch adjusts the Tk application to terminate properly and should avoid this problem.

Reported-by: Anders Kaseorg <andersk@mit.edu>
Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>
---
 git-gui--askpass |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

Show changes to git-gui--askpass +11 −6

diff --git a/git-gui--askpass b/git-gui--askpass
index 12e117e..8ba4779 100755
--- a/git-gui--askpass
+++ b/git-gui--askpass
@@ -30,16 +30,20 @@ if {!$yesno} {
 
 frame .b
 button .b.ok     -text OK     -command finish
-button .b.cancel -text Cancel -command {destroy .}
+button .b.cancel -text Cancel -command cancel
 
 pack .b.ok -side left -expand 1
 pack .b.cancel -side right -expand 1
 pack .b -side bottom -fill x -padx 10 -pady 10
 
 bind . <Visibility> {focus -force .e}
-bind . <Key-Return> finish
-bind . <Key-Escape> {destroy .}
-bind . <Destroy>    {exit $rc}
+bind . <Key-Return> [list .b.ok invoke]
+bind . <Key-Escape> [list .b.cancel invoke]
+bind . <Destroy>    {set rc $rc}
+
+proc cancel {} {
+	set ::rc 255
+}
 
 proc finish {} {
 	if {$::yesno} {
@@ -50,10 +54,11 @@ proc finish {} {
 		}
 	}
 
-	set ::rc 0
 	puts $::answer
-	destroy .
+	set ::rc 0
 }
 
 wm title . "OpenSSH"
 tk::PlaceWindow .
+vwait rc
+exit $rc

-- 
1.7.2.1.44.g721e7

Jonathan Nieder· Aug 18, 2010, 04:21 UTC · re: Pat Thoyts · lore

Re: [PATCH] git-gui: change the termination checks to avoid potential hang.

Pat Thoyts wrote:

> Supposedly

And consistently.

 $ wish
 % puts $tcl_version
 8.5
 % puts $tk_version
 8.5
 $ dpkg -l tcl8.5 tk8.5 | tail -2
 ii  tcl8.5         8.5.8-2       Tcl (the Tool Command Language) v8.5 - ru
 ii  tk8.5          8.5.8-1       Tk toolkit for Tcl and X11, v8.5 - run-ti
 $ ./git-gui--askpass
 <types passphrase>

The window would stay open and hang.

> Signed-off-by: Pat Thoyts <patthoyts@users.sourceforge.net>
> ---
>  git-gui--askpass |   17 +++++++++++------
>  1 files changed, 11 insertions(+), 6 deletions(-)

Tested-by: Jonathan Nieder <jrnieder@gmail.com>

> +++ b/git-gui--askpass

[...]

> +vwait rc
> +exit $rc

I like it.

Thanks for keeping git gui in good shape.

Pat Thoyts· Aug 18, 2010, 22:48 UTC · re: Jonathan Nieder · lore

Re: [PATCH] git-gui: change the termination checks to avoid potential hang.

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 15 quoted lines

>
>And consistently.
>
> $ wish
> % puts $tcl_version
> 8.5
> % puts $tk_version
> 8.5
> $ dpkg -l tcl8.5 tk8.5 | tail -2
> ii  tcl8.5         8.5.8-2       Tcl (the Tool Command Language) v8.5 - ru
> ii  tk8.5          8.5.8-1       Tk toolkit for Tcl and X11, v8.5 - run-ti
> $ ./git-gui--askpass
> <types passphrase>
>
>The window would stay open and hang.

OK I managed to confirm that using Tk 8.5. It seems 8.4 and 8.6 get away with it. I've pushed the fix to git-gui master now. Thanks for testing.

-- 
Pat Thoyts                            http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97  10 CE 11 E6 04 E0 B9 DD

Pat Thoyts· Aug 15, 2010, 23:53 UTC · re: Jonathan Nieder · lore

Re: [PATCH v2 git-gui] git-gui--askpass: Do not hang on exit

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 45 quoted lines

>Anders Kaseorg wrote:
>
>> When git-gui--askpass is manually copied from the source into
>> /usr/lib/git-core, though, it doesn’t particularly work; its window
>> just freezes after the password is typed.
>
>The problem seems to be the
>
>  bind . <Destroy>    {exit $::rc}
>
>line; apparently each exit causes the window to be destroyed again,
>resulting in git gui hanging.
>
>Reported-by: Anders Kaseorg <andersk@mit.edu>
>Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>---
> I wrote:
>
> > @@ -52,7 +52,9 @@ proc finish {} {
> >  
> >  	set ::rc 0
> >  	puts $::answer
> > +	bind . <Destroy> {}
> >  	destroy .
>
> but that does nothing to help the case when the destroy event
> comes from the window manager.  Here’s a saner patch (sorry for
> the noise).
>
>diff --git a/git-gui--askpass b/git-gui--askpass
>index 12e117e..20b8799 100755
>--- a/git-gui--askpass
>+++ b/git-gui--askpass
>@@ -39,7 +39,10 @@ pack .b -side bottom -fill x -padx 10 -pady 10
> bind . <Visibility> {focus -force .e}
> bind . <Key-Return> finish
> bind . <Key-Escape> {destroy .}
>-bind . <Destroy>    {exit $rc}
>+bind . <Destroy>    {
>+	bind . <Destroy> {}
>+	exit $::rc
>+}
> 
> proc finish {} {
> 	if {$::yesno} {

I'm not so keen to take this patch for git-gui. Exiting from bindings isn't such great Tk style and I'd rather we used a vwait to explicitly run the event loop and exit after terminating that. The following patch does this. 'git-gui: change the termination checks to avoid potential hang'

I'm not certain of the conditions that produced the error this is supposed to fix. I have not reproduced the hang so far on a linux system using Tk 8.5. It would be good to get confirmation that my proposed patch actually solves the issue reported.

-- 
Pat Thoyts                            http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97  10 CE 11 E6 04 E0 B9 DD