[PATCH v1 0/2] git-gui: reduce Tcl version requirement from 8.6 to 8.5

[PATCH v1 0/2] git-gui: reduce Tcl version requirement from 8.6 to 8.5

[Pratyush Yadav]

Hi,

Some MacOS distributions ship with Tcl 8.5. This means we can't use
TclOO. So, use our homegrown class.tcl instead.

While here, fix a potential variable name collision by creating a
separate namespace for a chord's script evaluation.

Jonathan,

Can you please test the patches the same way you tested your original
series just to be sure we don't break anything? A review would also be
nice.

Pratyush Yadav (2):
  git-gui: reduce Tcl version requirement from 8.6 to 8.5
  git-gui: create a new namespace for chord script evaluation

 git-gui.sh    |  4 ++--
 lib/chord.tcl | 56 +++++++++++++++++++++++++--------------------------
 lib/index.tcl | 10 +++++----
 3 files changed, 35 insertions(+), 35 deletions(-)

--
2.26.0.rc1.11.g30e9940356

[PATCH v1 1/2] git-gui: reduce Tcl version requirement from 8.6 to 8.5

[Pratyush Yadav]

On some MacOS distributions like High Sierra, Tcl 8.5 is shipped by
default. This makes git-gui error out at startup because of the version
mismatch.

The only part that requires Tcl 8.6 is SimpleChord, which depends on
TclOO. So, don't use it and use our homegrown class.tcl instead.

This means some slight syntax changes. Since class.tcl doesn't have an
"unknown" method like TclOO does, we can't just call '$note', but have
to use '$note activate' instead. The constructor now needs a proper
namespace qualifier. Update the documentation to reflect the new syntax.

As of now, the only part of git-gui that needs Tcl 8.5 is a call to
'apply' in lib/index.tcl::lambda. Keep using it until someone shows up
shouting that their OS ships with 8.4 only. Then we would have to look
into implementing it in pure Tcl.

Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
---
 git-gui.sh    |  4 ++--
 lib/chord.tcl | 54 ++++++++++++++++++++++++---------------------------
 lib/index.tcl | 10 ++++++----
 3 files changed, 33 insertions(+), 35 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index f41ed2e..027e093 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -30,8 +30,8 @@ along with this program; if not, see <http://www.gnu.org/licenses/>.}]
 ##
 ## Tcl/Tk sanity check

-if {[catch {package require Tcl 8.6} err]
- || [catch {package require Tk  8.6} err]
+if {[catch {package require Tcl 8.5} err]
+ || [catch {package require Tk  8.5} err]
 } {
 	catch {wm withdraw .}
 	tk_messageBox \
diff --git a/lib/chord.tcl b/lib/chord.tcl
index 275a6cd..7de7cba 100644
--- a/lib/chord.tcl
+++ b/lib/chord.tcl
@@ -27,7 +27,7 @@
 #   # Turn off the UI while running a couple of async operations.
 #   lock_ui
 #
-#   set chord [SimpleChord new {
+#   set chord [SimpleChord::new {
 #     unlock_ui
 #     # Note: $notice here is not referenced in the calling scope
 #     if {$notice} { info_popup $notice }
@@ -37,9 +37,9 @@
 #   # all operations have been initiated.
 #   set common_note [$chord add_note]
 #
-#   # Pass notes as 'after' callbacks to other operations
-#   async_operation $args [$chord add_note]
-#   other_async_operation $args [$chord add_note]
+#   # Activate notes in 'after' callbacks to other operations
+#   set newnote [$chord add_note]
+#   async_operation $args [list $newnote activate]
 #
 #   # Communicate with the chord body
 #   if {$condition} {
@@ -48,7 +48,7 @@
 #   }
 #
 #   # Activate the common note, making the chord eligible to complete
-#   $common_note
+#   $common_note activate
 #
 # At this point, the chord will complete at some unknown point in the future.
 # The common note might have been the first note activated, or the async
@@ -60,18 +60,21 @@
 #   Represents a procedure that conceptually has multiple entrypoints that must
 #   all be called before the procedure executes. Each entrypoint is called a
 #   "note". The chord is only "completed" when all the notes are "activated".
-oo::class create SimpleChord {
-	variable notes body is_completed
+class SimpleChord {
+	field notes
+	field body
+	field is_completed

 	# Constructor:
-	#   set chord [SimpleChord new {body}]
+	#   set chord [SimpleChord::new {body}]
 	#     Creates a new chord object with the specified body script. The
 	#     body script is evaluated at most once, when a note is activated
 	#     and the chord has no other non-activated notes.
-	constructor {body} {
+	constructor new {i_body} {
 		set notes [list]
-		my eval [list set body $body]
+		set body $i_body
 		set is_completed 0
+		return $this
 	}

 	# Method:
@@ -80,7 +83,7 @@ oo::class create SimpleChord {
 	#     the chord body will be evaluated. This can be used to set variable
 	#     values for the chord body to use.
 	method eval {script} {
-		namespace eval [self] $script
+		namespace eval [namespace qualifiers $this] $script
 	}

 	# Method:
@@ -92,7 +95,7 @@ oo::class create SimpleChord {
 	method add_note {} {
 		if {$is_completed} { error "Cannot add a note to a completed chord" }

-		set note [ChordNote new [self]]
+		set note [ChordNote::new $this]

 		lappend notes $note

@@ -108,8 +111,8 @@ oo::class create SimpleChord {

 			set is_completed 1

-			namespace eval [self] $body
-			namespace delete [self]
+			namespace eval [namespace qualifiers $this] $body
+			delete_this
 		}
 	}
 }
@@ -119,15 +122,17 @@ oo::class create SimpleChord {
 #   final note of the chord is activated (this can be any note in the chord,
 #   with all other notes already previously activated in any order), the chord's
 #   body is evaluated.
-oo::class create ChordNote {
-	variable chord is_activated
+class ChordNote {
+	field chord
+	field is_activated

 	# Constructor:
 	#   Instances of ChordNote are created internally by calling add_note on
 	#   SimpleChord objects.
-	constructor {chord} {
-		my eval set chord $chord
+	constructor new {c} {
+		set chord $c
 		set is_activated 0
+		return $this
 	}

 	# Method:
@@ -138,20 +143,11 @@ oo::class create ChordNote {
 	}

 	# Method:
-	#   $note
+	#   $note activate
 	#     Activates the note, if it has not already been activated, and
 	#     completes the chord if there are no other notes awaiting
 	#     activation. Subsequent calls will have no further effect.
-	#
-	# NB: In TclOO, if an object is invoked like a method without supplying
-	#     any method name, then this internal method `unknown` is what
-	#     actually runs (with no parameters). It is used in the ChordNote
-	#     class for the purpose of allowing the note object to be called as
-	#     a function (see example above). (The `unknown` method can also be
-	#     used to support dynamic dispatch, but must take parameters to
-	#     identify the "unknown" method to be invoked. In this form, this
-	#     proc serves only to make instances behave directly like methods.)
-	method unknown {} {
+	method activate {} {
 		if {!$is_activated} {
 			set is_activated 1
 			$chord notify_note_activation
diff --git a/lib/index.tcl b/lib/index.tcl
index 1254145..ebcd6e4 100644
--- a/lib/index.tcl
+++ b/lib/index.tcl
@@ -436,7 +436,7 @@ proc revert_helper {txt paths} {
 	#
 	# The asynchronous operations are each indicated below by a comment
 	# before the code block that starts the async operation.
-	set after_chord [SimpleChord new {
+	set after_chord [SimpleChord::new {
 		if {[string trim $err] != ""} {
 			rescan_on_error $err
 		} else {
@@ -521,11 +521,12 @@ proc revert_helper {txt paths} {
 			[mc "Revert Changes"] \
 			]

+		set note [$after_chord add_note]
 		if {$reply == 1} {
 			checkout_index \
 				$txt \
 				$path_list \
-				[$after_chord add_note] \
+				[list $note activate] \
 				$capture_error
 		}
 	}
@@ -564,17 +565,18 @@ proc revert_helper {txt paths} {
 			[mc "Delete Files"] \
 			]

+		set note [$after_chord add_note]
 		if {$reply == 1} {
 			$after_chord eval { set should_reshow_diff 1 }

-			delete_files $untracked_list [$after_chord add_note]
+			delete_files $untracked_list [list $note activate]
 		}
 	}

 	# Activate the common note. If no other notes were created, this
 	# completes the chord. If other notes were created, then this common
 	# note prevents a race condition where the chord might complete early.
-	$after_common_note
+	$after_common_note activate
 }

 # Delete all of the specified files, performing deletion in batches to allow the
--
2.26.0.rc1.11.g30e9940356

[PATCH v1 2/2] git-gui: create a new namespace for chord script evaluation

[Pratyush Yadav]

Evaluating the script in the same namespace as the chord itself creates
potential for variable name collision. And in that case the script would
unknowingly use the chord's variables.

For example, say the script has a variable called 'is_completed', which
also exists in the chord's namespace. The script then calls 'eval' and
sets 'is_completed' to 1 thinking it is setting its own variable,
completely unaware of how the chord works behind the scenes. This leads
to the chord never actually executing because it sees 'is_completed' as
true and thinks it has already completed.

Avoid the potential collision by creating a separate namespace for the
script that is a child of the chord's namespace.

Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
---
 lib/chord.tcl | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/chord.tcl b/lib/chord.tcl
index 7de7cba..e21e7d3 100644
--- a/lib/chord.tcl
+++ b/lib/chord.tcl
@@ -64,6 +64,7 @@ class SimpleChord {
 	field notes
 	field body
 	field is_completed
+	field eval_ns
 
 	# Constructor:
 	#   set chord [SimpleChord::new {body}]
@@ -74,6 +75,7 @@ class SimpleChord {
 		set notes [list]
 		set body $i_body
 		set is_completed 0
+		set eval_ns "[namespace qualifiers $this]::eval"
 		return $this
 	}
 
@@ -83,7 +85,7 @@ class SimpleChord {
 	#     the chord body will be evaluated. This can be used to set variable
 	#     values for the chord body to use.
 	method eval {script} {
-		namespace eval [namespace qualifiers $this] $script
+		namespace eval $eval_ns $script
 	}
 
 	# Method:
@@ -111,7 +113,7 @@ class SimpleChord {
 
 			set is_completed 1
 
-			namespace eval [namespace qualifiers $this] $body
+			namespace eval $eval_ns $body
 			delete_this
 		}
 	}
-- 
2.26.0.rc1.11.g30e9940356

Re: [PATCH v1 0/2] git-gui: reduce Tcl version requirement from 8.6 to 8.5

[Eric Sunshine]

On Sat, Mar 14, 2020 at 9:47 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> Some MacOS distributions ship with Tcl 8.5. This means we can't use
> TclOO. So, use our homegrown class.tcl instead.

It should be mentioned that this patch series fixes a regression in
Git v2.25 in which git-gui could not even be launched on Mac OS. The
problem was reported here[1] a couple months ago.

I performed some rudimentary testing of this patch series on Mac OS,
and it appears to be working as expected; it certainly fixes the
problem of git-gui not launching on Mac OS. (I did notice a
misbehavior related to the original patch which caused git-gui to be
unusable on Mac OS in v2.25, but I suspect that misbehavior is not
related to or caused by this patch series, thus shouldn't prevent its
acceptance.)

[1]: https://github.com/prati0100/git-gui/issues/26

Re: [PATCH v1 0/2] git-gui: reduce Tcl version requirement from 8.6 to 8.5

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sat, Mar 14, 2020 at 9:47 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
>> Some MacOS distributions ship with Tcl 8.5. This means we can't use
>> TclOO. So, use our homegrown class.tcl instead.
>
> It should be mentioned that this patch series fixes a regression in
> Git v2.25 in which git-gui could not even be launched on Mac OS. The
> problem was reported here[1] a couple months ago.
>
> I performed some rudimentary testing of this patch series on Mac OS,
> and it appears to be working as expected; it certainly fixes the
> problem of git-gui not launching on Mac OS. (I did notice a
> misbehavior related to the original patch which caused git-gui to be
> unusable on Mac OS in v2.25, but I suspect that misbehavior is not
> related to or caused by this patch series, thus shouldn't prevent its
> acceptance.)

I was actually hesitant to see this kind of change for the first
time this late in the cycle (the code may work with old Tcl/Tk but
do we know it does with newer ones?)  

I'll pull git-gui updates when Pratyush tells me to, which would
happen before the final (scheduled on 22nd).  I'll trust git-gui
maintainer's decision to include these changes in it, or to cook
longer to wait for the 2.27 cycle.  Comments like yours that help
Pratyush make the right judgment is greatly appreciated.

Thanks.  

Re: [PATCH v1 0/2] git-gui: reduce Tcl version requirement from 8.6 to 8.5

[Pratyush Yadav]

On 16/03/20 08:48AM, Junio C Hamano wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> > On Sat, Mar 14, 2020 at 9:47 PM Pratyush Yadav <me@yadavpratyush.com> wrote:
> >> Some MacOS distributions ship with Tcl 8.5. This means we can't use
> >> TclOO. So, use our homegrown class.tcl instead.
> >
> > It should be mentioned that this patch series fixes a regression in
> > Git v2.25 in which git-gui could not even be launched on Mac OS. The
> > problem was reported here[1] a couple months ago.
> >
> > I performed some rudimentary testing of this patch series on Mac OS,
> > and it appears to be working as expected; it certainly fixes the
> > problem of git-gui not launching on Mac OS. (I did notice a
> > misbehavior related to the original patch which caused git-gui to be
> > unusable on Mac OS in v2.25, but I suspect that misbehavior is not
> > related to or caused by this patch series, thus shouldn't prevent its
> > acceptance.)
> 
> I was actually hesitant to see this kind of change for the first
> time this late in the cycle

*sigh* I intended to finish the change much sooner, but one thing after 
another, and I kept putting it on the backburner :-(

> (the code may work with old Tcl/Tk but do we know it does with newer 
> ones?)  

It is actually the other way around. My system has Tcl 8.6 installed, 
and it works well with it. The problem is checking if it works with 8.5. 
My distro's package manager doesn't show Tcl 8.5 as an option (only 
8.6), so I have to try and build 8.5 from source to test it. It is more 
work than I can afford right now. So I am relying on Eric and other 
MacOS users' reports.

But IIUC, unless there are any hidden gotchas in Tcl 8.5, there is a 
fairly little chance that this patch breaks anything significant that 
wasn't already broken before. The patch reverts from using a new 
8.6-specific feature to pure, backward-compatible Tcl that should work 
with older versions too. No, a larger concern for me is whether I missed 
something somewhere that breaks the feature for _all_ versions of 
git-gui.
 
> I'll pull git-gui updates when Pratyush tells me to, which would
> happen before the final (scheduled on 22nd).  I'll trust git-gui
> maintainer's decision to include these changes in it, or to cook
> longer to wait for the 2.27 cycle.  Comments like yours that help
> Pratyush make the right judgment is greatly appreciated.

Honestly, I'd like to cook it a bit longer but the reality is that very 
few people, if any, actually track and test my tree. Most people 
actually discover bugs when the changes hit a new Git release.

I cooked the original series that bumped the version requirement for 
quite some time because I suspected some platforms might still be on 
older versions. But I got absolutely no feedback/complaints, so I went 
ahead and sent you a pull request.

The change hit my 'master' on December 6. Git v2.25.0 was tagged on 
January 13th. MacOS breakage was reported on January 15th.

So, I have a choice between waiting for the 2.27 cycle keeping git-gui 
broken on MacOS for another few months in hopes that someone comes in 
and discovers a bug, or have it merged in v2.26 fixing git-gui for MacOS 
but risk (though it shouldn't be too high) introducing a bug in _all_ 
platforms. Neither choice is ideal, but I'm leaning towards the latter. 
Having _most_ things working is better than having nothing working at 
all.

-- 
Regards,
Pratyush Yadav

[PATCH v2 0/2] git-gui: reduce Tcl version requirement from 8.6 to 8.5

[Pratyush Yadav]

Hi,

Some MacOS distributions ship with Tcl 8.5. This means we can't use
TclOO. So, use our homegrown class.tcl instead.

While here, fix a potential variable name collision by creating a
separate namespace for a chord's script evaluation.

Jonathan,

Can you please test the patches the same way you tested your original
series just to be sure we don't break anything? A review would also be
nice.

Changes in v2:
- Add a note _after_ checking if the user agreed to the deletion.
  Otherwise, if the user denies, two "zombie" notes are left lying
  around which will never be activated. This means that the chord won't
  complete and the index won't be unlocked, leading to git-gui becoming
  frozen.

Pratyush Yadav (2):
  git-gui: reduce Tcl version requirement from 8.6 to 8.5
  git-gui: create a new namespace for chord script evaluation

 git-gui.sh    |  4 ++--
 lib/chord.tcl | 56 +++++++++++++++++++++++++--------------------------
 lib/index.tcl | 10 +++++----
 3 files changed, 35 insertions(+), 35 deletions(-)

--
2.26.0.rc1.11.g30e9940356

[PATCH v2 1/2] git-gui: reduce Tcl version requirement from 8.6 to 8.5

[Pratyush Yadav]

On some MacOS distributions like High Sierra, Tcl 8.5 is shipped by
default. This makes git-gui error out at startup because of the version
mismatch.

The only part that requires Tcl 8.6 is SimpleChord, which depends on
TclOO. So, don't use it and use our homegrown class.tcl instead.

This means some slight syntax changes. Since class.tcl doesn't have an
"unknown" method like TclOO does, we can't just call '$note', but have
to use '$note activate' instead. The constructor now needs a proper
namespace qualifier. Update the documentation to reflect the new syntax.

As of now, the only part of git-gui that needs Tcl 8.5 is a call to
'apply' in lib/index.tcl::lambda. Keep using it until someone shows up
shouting that their OS ships with 8.4 only. Then we would have to look
into implementing it in pure Tcl.

Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
---
 git-gui.sh    |  4 ++--
 lib/chord.tcl | 54 ++++++++++++++++++++++++---------------------------
 lib/index.tcl | 10 ++++++----
 3 files changed, 33 insertions(+), 35 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index f41ed2e..027e093 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -30,8 +30,8 @@ along with this program; if not, see <http://www.gnu.org/licenses/>.}]
 ##
 ## Tcl/Tk sanity check
 
-if {[catch {package require Tcl 8.6} err]
- || [catch {package require Tk  8.6} err]
+if {[catch {package require Tcl 8.5} err]
+ || [catch {package require Tk  8.5} err]
 } {
 	catch {wm withdraw .}
 	tk_messageBox \
diff --git a/lib/chord.tcl b/lib/chord.tcl
index 275a6cd..7de7cba 100644
--- a/lib/chord.tcl
+++ b/lib/chord.tcl
@@ -27,7 +27,7 @@
 #   # Turn off the UI while running a couple of async operations.
 #   lock_ui
 #
-#   set chord [SimpleChord new {
+#   set chord [SimpleChord::new {
 #     unlock_ui
 #     # Note: $notice here is not referenced in the calling scope
 #     if {$notice} { info_popup $notice }
@@ -37,9 +37,9 @@
 #   # all operations have been initiated.
 #   set common_note [$chord add_note]
 #
-#   # Pass notes as 'after' callbacks to other operations
-#   async_operation $args [$chord add_note]
-#   other_async_operation $args [$chord add_note]
+#   # Activate notes in 'after' callbacks to other operations
+#   set newnote [$chord add_note]
+#   async_operation $args [list $newnote activate]
 #
 #   # Communicate with the chord body
 #   if {$condition} {
@@ -48,7 +48,7 @@
 #   }
 #
 #   # Activate the common note, making the chord eligible to complete
-#   $common_note
+#   $common_note activate
 #
 # At this point, the chord will complete at some unknown point in the future.
 # The common note might have been the first note activated, or the async
@@ -60,18 +60,21 @@
 #   Represents a procedure that conceptually has multiple entrypoints that must
 #   all be called before the procedure executes. Each entrypoint is called a
 #   "note". The chord is only "completed" when all the notes are "activated".
-oo::class create SimpleChord {
-	variable notes body is_completed
+class SimpleChord {
+	field notes
+	field body
+	field is_completed
 
 	# Constructor:
-	#   set chord [SimpleChord new {body}]
+	#   set chord [SimpleChord::new {body}]
 	#     Creates a new chord object with the specified body script. The
 	#     body script is evaluated at most once, when a note is activated
 	#     and the chord has no other non-activated notes.
-	constructor {body} {
+	constructor new {i_body} {
 		set notes [list]
-		my eval [list set body $body]
+		set body $i_body
 		set is_completed 0
+		return $this
 	}
 
 	# Method:
@@ -80,7 +83,7 @@ oo::class create SimpleChord {
 	#     the chord body will be evaluated. This can be used to set variable
 	#     values for the chord body to use.
 	method eval {script} {
-		namespace eval [self] $script
+		namespace eval [namespace qualifiers $this] $script
 	}
 
 	# Method:
@@ -92,7 +95,7 @@ oo::class create SimpleChord {
 	method add_note {} {
 		if {$is_completed} { error "Cannot add a note to a completed chord" }
 
-		set note [ChordNote new [self]]
+		set note [ChordNote::new $this]
 
 		lappend notes $note
 
@@ -108,8 +111,8 @@ oo::class create SimpleChord {
 
 			set is_completed 1
 
-			namespace eval [self] $body
-			namespace delete [self]
+			namespace eval [namespace qualifiers $this] $body
+			delete_this
 		}
 	}
 }
@@ -119,15 +122,17 @@ oo::class create SimpleChord {
 #   final note of the chord is activated (this can be any note in the chord,
 #   with all other notes already previously activated in any order), the chord's
 #   body is evaluated.
-oo::class create ChordNote {
-	variable chord is_activated
+class ChordNote {
+	field chord
+	field is_activated
 
 	# Constructor:
 	#   Instances of ChordNote are created internally by calling add_note on
 	#   SimpleChord objects.
-	constructor {chord} {
-		my eval set chord $chord
+	constructor new {c} {
+		set chord $c
 		set is_activated 0
+		return $this
 	}
 
 	# Method:
@@ -138,20 +143,11 @@ oo::class create ChordNote {
 	}
 
 	# Method:
-	#   $note
+	#   $note activate
 	#     Activates the note, if it has not already been activated, and
 	#     completes the chord if there are no other notes awaiting
 	#     activation. Subsequent calls will have no further effect.
-	#
-	# NB: In TclOO, if an object is invoked like a method without supplying
-	#     any method name, then this internal method `unknown` is what
-	#     actually runs (with no parameters). It is used in the ChordNote
-	#     class for the purpose of allowing the note object to be called as
-	#     a function (see example above). (The `unknown` method can also be
-	#     used to support dynamic dispatch, but must take parameters to
-	#     identify the "unknown" method to be invoked. In this form, this
-	#     proc serves only to make instances behave directly like methods.)
-	method unknown {} {
+	method activate {} {
 		if {!$is_activated} {
 			set is_activated 1
 			$chord notify_note_activation
diff --git a/lib/index.tcl b/lib/index.tcl
index 1254145..1fc5b42 100644
--- a/lib/index.tcl
+++ b/lib/index.tcl
@@ -436,7 +436,7 @@ proc revert_helper {txt paths} {
 	#
 	# The asynchronous operations are each indicated below by a comment
 	# before the code block that starts the async operation.
-	set after_chord [SimpleChord new {
+	set after_chord [SimpleChord::new {
 		if {[string trim $err] != ""} {
 			rescan_on_error $err
 		} else {
@@ -522,10 +522,11 @@ proc revert_helper {txt paths} {
 			]
 
 		if {$reply == 1} {
+			set note [$after_chord add_note]
 			checkout_index \
 				$txt \
 				$path_list \
-				[$after_chord add_note] \
+				[list $note activate] \
 				$capture_error
 		}
 	}
@@ -567,14 +568,15 @@ proc revert_helper {txt paths} {
 		if {$reply == 1} {
 			$after_chord eval { set should_reshow_diff 1 }
 
-			delete_files $untracked_list [$after_chord add_note]
+			set note [$after_chord add_note]
+			delete_files $untracked_list [list $note activate]
 		}
 	}
 
 	# Activate the common note. If no other notes were created, this
 	# completes the chord. If other notes were created, then this common
 	# note prevents a race condition where the chord might complete early.
-	$after_common_note
+	$after_common_note activate
 }
 
 # Delete all of the specified files, performing deletion in batches to allow the
-- 
2.26.0.rc1.11.g30e9940356

[PATCH v2 2/2] git-gui: create a new namespace for chord script evaluation

[Pratyush Yadav]

Evaluating the script in the same namespace as the chord itself creates
potential for variable name collision. And in that case the script would
unknowingly use the chord's variables.

For example, say the script has a variable called 'is_completed', which
also exists in the chord's namespace. The script then calls 'eval' and
sets 'is_completed' to 1 thinking it is setting its own variable,
completely unaware of how the chord works behind the scenes. This leads
to the chord never actually executing because it sees 'is_completed' as
true and thinks it has already completed.

Avoid the potential collision by creating a separate namespace for the
script that is a child of the chord's namespace.

Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
---
 lib/chord.tcl | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/chord.tcl b/lib/chord.tcl
index 7de7cba..e21e7d3 100644
--- a/lib/chord.tcl
+++ b/lib/chord.tcl
@@ -64,6 +64,7 @@ class SimpleChord {
 	field notes
 	field body
 	field is_completed
+	field eval_ns
 
 	# Constructor:
 	#   set chord [SimpleChord::new {body}]
@@ -74,6 +75,7 @@ class SimpleChord {
 		set notes [list]
 		set body $i_body
 		set is_completed 0
+		set eval_ns "[namespace qualifiers $this]::eval"
 		return $this
 	}
 
@@ -83,7 +85,7 @@ class SimpleChord {
 	#     the chord body will be evaluated. This can be used to set variable
 	#     values for the chord body to use.
 	method eval {script} {
-		namespace eval [namespace qualifiers $this] $script
+		namespace eval $eval_ns $script
 	}
 
 	# Method:
@@ -111,7 +113,7 @@ class SimpleChord {
 
 			set is_completed 1
 
-			namespace eval [namespace qualifiers $this] $body
+			namespace eval $eval_ns $body
 			delete_this
 		}
 	}
-- 
2.26.0.rc1.11.g30e9940356

Re: [PATCH v2 0/2] git-gui: reduce Tcl version requirement from 8.6 to 8.5

[Eric Sunshine]

On Tue, Mar 17, 2020 at 9:29 AM Pratyush Yadav <me@yadavpratyush.com> wrote:
> Some MacOS distributions ship with Tcl 8.5. This means we can't use
> TclOO. So, use our homegrown class.tcl instead.
>
> Changes in v2:
> - Add a note _after_ checking if the user agreed to the deletion.
>   Otherwise, if the user denies, two "zombie" notes are left lying
>   around which will never be activated. This means that the chord won't
>   complete and the index won't be unlocked, leading to git-gui becoming
>   frozen.

Thanks. I did some light testing on Mac OS. This re-roll seems to
address the reported problems[1] and allows the new "delete unstaged
file" feature to work on older Tcl. As a fix for the Git 2.25
regression which resulted in git-gui being unable to launch on Mac OS,
this path series seems "good to go".

[1]: https://github.com/prati0100/git-gui/issues/26

Re: [PATCH v1 0/2] git-gui: reduce Tcl version requirement from 8.6 to 8.5

[Eric Sunshine]

On Tue, Mar 17, 2020 at 8:49 AM Pratyush Yadav <me@yadavpratyush.com> wrote:
> On 16/03/20 08:48AM, Junio C Hamano wrote:
> > I'll pull git-gui updates when Pratyush tells me to, which would
> > happen before the final (scheduled on 22nd).  I'll trust git-gui
> > maintainer's decision to include these changes in it, or to cook
> > longer to wait for the 2.27 cycle. [...]
>
> Honestly, I'd like to cook it a bit longer but the reality is that very
> few people, if any, actually track and test my tree. Most people
> actually discover bugs when the changes hit a new Git release.

Yep, that's the big issue. I track Junio's "next" branch pretty
closely but don't track the git-gui repository at all, so it wasn't
until Junio pulled from you, and after I pulled from Junio, that I
noticed the problem. So, in the longer run, asking Junio to pull more
often -- and earlier -- may be a good way forward.

Re: [PATCH v2 0/2] git-gui: reduce Tcl version requirement from 8.6 to 8.5

[Pratyush Yadav]

On 19/03/20 11:22AM, Eric Sunshine wrote:
> On Tue, Mar 17, 2020 at 9:29 AM Pratyush Yadav <me@yadavpratyush.com> wrote:
> > Some MacOS distributions ship with Tcl 8.5. This means we can't use
> > TclOO. So, use our homegrown class.tcl instead.
> >
> > Changes in v2:
> > - Add a note _after_ checking if the user agreed to the deletion.
> >   Otherwise, if the user denies, two "zombie" notes are left lying
> >   around which will never be activated. This means that the chord won't
> >   complete and the index won't be unlocked, leading to git-gui becoming
> >   frozen.
> 
> Thanks. I did some light testing on Mac OS. This re-roll seems to
> address the reported problems[1] and allows the new "delete unstaged
> file" feature to work on older Tcl. As a fix for the Git 2.25
> regression which resulted in git-gui being unable to launch on Mac OS,
> this path series seems "good to go".

Thanks for testing. Merged. Thanks all.
 
> [1]: https://github.com/prati0100/git-gui/issues/26

-- 
Regards,
Pratyush Yadav