[PATCH v3 0/2] git-gui: revert untracked files by deleting them

["Jonathan Gilbert via GitGitGadget" <gitgitgadget@gmail.com>]

My development environment sometimes makes automatic changes that I don't
want to keep. In some cases, this involves new files being added that I
don't want to commit or keep (but I also don't want to outright .gitignore 
forever). I have typically had to explicitly delete those files externally
to Git Gui, which is a context switch to a manual operation, and I want to
be able to just select those newly-created untracked files in the UI and
"revert" them into oblivion.

This change updates the revert_helper proc to check for untracked files as
well as changes, and then changes to be reverted and untracked files are
handled by independent blocks of code. The user is prompted independently
for untracked files, since the underlying action is fundamentally different
(rm -f). If after deleting untracked files, the directory containing them
becomes empty, then the directory is removed as well. A new proc 
delete_files takes care of actually deleting the files, using the Tcler's
Wiki recommended approach for keeping the UI responsive.

Since the checkout_index and delete_files calls are both asynchronous and
could potentially complete in any order, a "chord" is used to coordinate
unlocking the index and returning the UI to a usable state only after both
operations are complete.

The _close_updateindex proc contains error handling (added in d4e890e5) that
has the potential to interact badly with unlock_index running at the
completion of an async operation. I have refactored the procedure into
separate procs _close_updateindex and rescan_on_error, and the original
functionality is captured with _close_updateindex_rescan_on_error. Call
sites have been updated appropriately, and checkout_index has been reworked
to take a functor that captures any errors that occur in a caller-defined
way. revert_helper uses this to supply a lambda function that stashes the
error within the chord's body namespace, so that it can call rescan_on_error 
when the chord is completed, which might be substantially after 
checkout_index encounters its error. If a rescan is called for, it is done
once the deletion is complete.

This is the fourth revision of this change, which differs from the third
version in the following ways:

 * Merged some of the wording from the cover letter into the commit message.
   
   
 * The Tcl/Tk dependency is updated to 8.6.
   
   
 * The chord.tcl documentation has been reworked, moving the mechanistic
   class/member documentation next to the declarations, and including a more
   "human-readable" holistic explanation at the top.
   
   
 * Class variables defined within chord.tcl have snake_case names. When I
   made this switch, I saw one possible reason why the convention from the
   Tcl documentation was different: it's quite common to have a parameter to
   the constructor with the same name as a class variable, and if the names
   are literally identical then they conflict in the namespace (e.g. set
   body $body).
   
   
 * Removed unnecessary variable from revert_helper in index.tcl, added
   comments and split long line per feedback.
   
   
 * _close_updateindex's error handling is split out to new proc 
   rescan_on_error, and _close_updateindex_rescan_on_error calls 
   rescan_on_error if it catches an error from _close_updateindex. Most call
   sites are updated to call the wrapper function, but the split is crucial
   for error handling within revert_helper.
   
   
 * checkout_index uses the _close_updateindex without the error handler, and
   instead takes a functor from the caller that allows the caller to do
   whatever it wants with any errors that occur (but without abnormally
   terminating the execution of checkout_index the way throwing an error
   would).
   
   
 * revert_helper is where the most meaningful change is. The $after_chord 
   body now checks for a stashed error, and if there is one, it calls 
   rescan_on_error. I create a lambda function $capture_error that takes
   whatever error message is passed to it and stashes it in $after_chord's
   namespace. Then, this is passed to checkout_index. The result is that if
   an error happens closing the index, the error message is passed back up,
   and then the checkout note is activated, but the $after_chord body
   doesn't execute until the deletion note is also activated, delaying the
   rescan until after the deletion is complete too.
   
   

On Mon, Nov 11, 2019 at 1:25 PM Pratyush Yadav me@yadavpratyush.com
[me@yadavpratyush.com] wrote:

> The 'class' documentation [0] suggests adding a "package require TclOO". But
TclOO ships by default with Tcl 8.6 and above. So, I'm not really sure if we
need this.


I couldn't find any evidence that it is required in my testing.

On Tue, Nov 12, 2019 at 1:35 PM Pratyush Yadav me@yadavpratyush.com
[me@yadavpratyush.com] wrote:

> > > > +oo::class create SimpleChord {


This comes from the TclOO package, right?

git-gui has its own object-oriented system (lib/class.tcl). It was written
circa 2007. I suspect something like TclOO did not exist back then.

Why not use that? Does it have some limitations that TclOO does not have? I
do not mind using the "official" OO system. I just want to know why exactly
you made the choice.


Having limited experience with Tcl, I did a Google search for "tcl object
oriented" and ended up writing code using TclOO because that's what came up.
Do you think it makes sense to rework this to useclass.tcl, or perhaps
instead the opposite: have a policy of using the standard TclOO going
forward, and let the rest of Git Gui organically upgrade itself to some
hypothetical point in the future where class.tcl is no longer used by
anything?


Replacing class.tcl would be a big effort, and seeing how things stand as of
now in terms of active contributors, I don't think it would happen in the
near future.

So the question really boils down to "do we want to mix these two flavors of
OO frameworks?".

If TclOO gives us some benefit over our homegrown framework, or if our
framework is in some way hard to use, then I would certainly side on just
sticking with TclOO.


It looks like the "treat an object as a method" functionality that the 
unknown method provides is not easily duplicated with class.tcl. It would be
possible to just replace it with a named method, but the code using it
wouldn't look as nice. Also, not that it matters in this instance, but
purely as a matter of principle, from what I've read, it seems that TclOO is
significantly supported by native code in the runtime and has much better
performance and far less overhead than all pre-8.6 OO solutions. This
suggests that a long-term goal of eliminating class.tcl might not be a bad
idea. I haven't seen any way that having chord.tcl use TclOO could interfere
with other, unrelated things using class.tcl.

> > > >  * method notify_note_activation {} {


Since this method is for internal use only, can it be made "private"? Does
the OO library support something like this?


I don't think so, because it's called from outside the class. What we'd be
looking for is something like C++'s "friend" syntax. Tcl doesn't seem to
have this. Though, I just did some further Googling, and saw a hint that it
might be possible to bypass member security on a case-by-case basis, so that
the method is private but ChordNote is able to call it anyway. I'll see if I
can't figure this out. :-)


I don't think too much complexity/hacking is warranted for something like
this. If you can figure out a really simple way to do it, great! Otherwise,
just keep it like it is.


It seems that there isn't any way in TclOO to get into a class without it
having a public "door", whether it's the method itself, or some "accessor"
method that returns its [my] functor. You can choose to unexport a method,
but once you do that, there doesn't appear to exist any way at all to
override this for just one call site.

> > "This is a mechanism for making an object that can be called like a method."


Looks like this method would also be called if someone misspelled a method
name for this object. So say if someone by mistake writes

 $note is_activate

this method would be called. This is a clear bug. So, add a check here to
make sure 'methodName' is actually absent. And if it isn't, display an
error. Displaying an error to the user on a programmer error can get
annoying. But since we don't have something like assertions in git-gui yet,
maybe that's the best way to get bugs noticed.


I did some testing and discovered that I was mistaken, if unknown has no
parameters then it cannot receive calls to unknown method names, these
generate errors. As written, it is only capable of processing calls against
the object itself.

> > Hmm, so, yeah, the entire if statement only occurs if it can't close the
file descriptor. Is that something that actually happens? If so, then it
should perhaps be throwing an exception, because having started a rescan is
probably more than the caller bargained for. That would prevent the callers
from unlocking the index out from under the rescan, and also cancel any
other processing they might be doing that is probably making bad assumptions
with a rescan running.


This seems like defensive programming. It is accounting for somethingreally
bad happening.


[..]

> So, this recovery code has to go somewhere. Yes, a rescan is certainly more
than what the caller wanted, but it is better than working on an
inconsistent in-memory state of the repo.

The question then becomes where the best place to do so is. This seems like
a good one if we can get our locking requirements to work with it properly.>
The glaring problem is that we don't want the rescan to run while the
deletion task is still running because they will interfere with each other.
Also, deletion expects the index to be locked, so the rescan and deletion
should be mutually exclusive.


I came up with a fairly concise way to defer the rescan until all async
operations are completed, by splitting the error handling out into a
separate method and then making this flow call that method from the
$after_chord body.

Jonathan Gilbert (2):
  git-gui: consolidate naming conventions
  git-gui: revert untracked files by deleting them

 git-gui.sh    |   4 +-
 lib/chord.tcl | 160 ++++++++++++++++
 lib/index.tcl | 500 +++++++++++++++++++++++++++++++++++++-------------
 3 files changed, 538 insertions(+), 126 deletions(-)
 create mode 100644 lib/chord.tcl


base-commit: b524f6b399c77b40c8bf2b6217585fde4731472a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-436%2Flogiclrd%2Fgit-gui-revert-untracked-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-436/logiclrd/git-gui-revert-untracked-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/436

Range-diff vs v2:

 1:  da1704c56e = 1:  da1704c56e git-gui: consolidate naming conventions
 2:  9469beb599 ! 2:  dc12c1668d git-gui: revert untracked files by deleting them
     @@ -2,66 +2,72 @@
      
          git-gui: revert untracked files by deleting them
      
     -    Update the revert_helper procedure to also detect untracked files. If
     -    files are present, the user is asked if they want them deleted. Perform
     -    the deletion in batches, using new proc delete_files with helper
     -    delete_helper, to allow the UI to remain responsive. Coordinate the
     -    completion of multiple overlapping asynchronous operations using a new
     -    construct called a "chord". Migrate unlocking of the index out of
     -    _close_updateindex to a responsibility of the caller, to permit paths
     -    that don't directly unlock the index.
     +    Update the revert_helper proc to check for untracked files as well as
     +    changes, and then handle changes to be reverted and untracked files with
     +    independent blocks of code. Prompt the user independently for untracked
     +    files, since the underlying action is fundamentally different (rm -f).
     +    If after deleting untracked files, the directory containing them becomes
     +    empty, then remove the directory as well. Migrate unlocking of the index
     +    out of _close_updateindex to a responsibility of the caller, to permit
     +    paths that don't directly unlock the index, and refactor the error
     +    handling added in d4e890e5 so that callers can make flow control
     +    decisions in the event of errors.
     +
     +    A new proc delete_files takes care of actually deleting the files in
     +    batches, using the Tcler's Wiki recommended approach for keeping the UI
     +    responsive.
     +
     +    Since the checkout_index and delete_files calls are both asynchronous
     +    and could potentially complete in any order, a "chord" is used to
     +    coordinate unlocking the index and returning the UI to a usable state
     +    only after both operations are complete. The `SimpleChord` class,
     +    based on TclOO (Tcl/Tk 8.6), is added in this commit.
      
          Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
      
     + diff --git a/git-gui.sh b/git-gui.sh
     + --- a/git-gui.sh
     + +++ b/git-gui.sh
     +@@
     + ##
     + ## Tcl/Tk sanity check
     + 
     +-if {[catch {package require Tcl 8.4} err]
     +- || [catch {package require Tk  8.4} err]
     ++if {[catch {package require Tcl 8.6} err]
     ++ || [catch {package require Tk  8.6} err]
     + } {
     + 	catch {wm withdraw .}
     + 	tk_messageBox \
     +
       diff --git a/lib/chord.tcl b/lib/chord.tcl
       new file mode 100644
       --- /dev/null
       +++ b/lib/chord.tcl
      @@
     -+# SimpleChord class:
     -+#   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".
     -+#
     -+#   Constructor:
     -+#     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.
     -+#
     -+#   Methods:
     -+#     $chord eval {script}
     -+#       Runs the specified script in the same context (namespace) in which the
     -+#       chord body will be evaluated. This can be used to set variable values
     -+#       for the chord body to use.
     -+#
     -+#     set note [$chord add_note]
     -+#       Adds a new note to the chord, an instance of ChordNote. Raises an
     -+#       error if the chord is already completed, otherwise the chord is updated
     -+#       so that the new note must also be activated before the body is
     -+#       evaluated.
     -+#
     -+#     $chord notify_note_activation
     -+#       For internal use only.
     -+#
     -+# ChordNote class:
     -+#   Represents a note within a chord, providing a way to activate it. When the
     -+#   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.
     ++# Simple Chord for Tcl
      +#
     -+#   Constructor:
     -+#     Instances of ChordNote are created internally by calling add_note on
     -+#     SimpleChord objects.
     ++# A "chord" is a method with more than one entrypoint and only one body, such
     ++# that the body runs only once all the entrypoints have been called by
     ++# different asynchronous tasks. In this implementation, the chord is defined
     ++# dynamically for each invocation. A SimpleChord object is created, supplying
     ++# body script to be run when the chord is completed, and then one or more notes
     ++# are added to the chord. Each note can be called like a proc, and returns
     ++# immediately if the chord isn't yet complete. When the last remaining note is
     ++# called, the body runs before the note returns.
      +#
     -+#   Methods:
     -+#     [$note is_activated]
     -+#       Returns true if this note has already been activated.
     ++# The SimpleChord class has a constructor that takes the body script, and a
     ++# method add_note that returns a note object. Since the body script does not
     ++# run in the context of the procedure that defined it, a mechanism is provided
     ++# for injecting variables into the chord for use by the body script. The
     ++# activation of a note is idempotent; multiple calls have the same effect as
     ++# a simple call.
      +#
     -+#     $note
     -+#       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.
     ++# If you are invoking asynchronous operations with chord notes as completion
     ++# callbacks, and there is a possibility that earlier operations could complete
     ++# before later ones are started, it is a good practice to create a "common"
     ++# note on the chord that prevents it from being complete until you're certain
     ++# you've added all the notes you need.
      +#
      +# Example:
      +#
     @@ -97,61 +103,105 @@
      +# last one, completing the chord before this code finishes, or anything in
      +# between. The purpose of the chord is to not have to worry about the order.
      +
     ++# SimpleChord class:
     ++#   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
     -+	variable Body
     -+	variable IsCompleted
     ++	variable notes body is_completed
      +
     ++	# Constructor:
     ++	#   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} {
     -+		set Notes [list]
     -+		set Body $body
     -+		set IsCompleted 0
     ++		set notes [list]
     ++		my eval [list set body $body]
     ++		set is_completed 0
      +	}
      +
     ++	# Method:
     ++	#   $chord eval {script}
     ++	#     Runs the specified script in the same context (namespace) in which
     ++	#     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
      +	}
      +
     ++	# Method:
     ++	#   set note [$chord add_note]
     ++	#     Adds a new note to the chord, an instance of ChordNote. Raises an
     ++	#     error if the chord is already completed, otherwise the chord is
     ++	#     updated so that the new note must also be activated before the
     ++	#     body is evaluated.
      +	method add_note {} {
     -+		if {$IsCompleted} { error "Cannot add a note to a completed chord" }
     ++		if {$is_completed} { error "Cannot add a note to a completed chord" }
      +
      +		set note [ChordNote new [self]]
      +
     -+		lappend Notes $note
     ++		lappend notes $note
      +
      +		return $note
      +	}
      +
     ++	# This method is for internal use only and is intentionally undocumented.
      +	method notify_note_activation {} {
     -+		if {!$IsCompleted} {
     -+			foreach note $Notes {
     ++		if {!$is_completed} {
     ++			foreach note $notes {
      +				if {![$note is_activated]} { return }
      +			}
      +
     -+			set IsCompleted 1
     ++			set is_completed 1
      +
     -+			namespace eval [self] $Body
     ++			namespace eval [self] $body
      +			namespace delete [self]
      +		}
      +	}
      +}
      +
     ++# ChordNote class:
     ++#   Represents a note within a chord, providing a way to activate it. When the
     ++#   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 IsActivated
     ++	variable chord is_activated
      +
     ++	# Constructor:
     ++	#   Instances of ChordNote are created internally by calling add_note on
     ++	#   SimpleChord objects.
      +	constructor {chord} {
     -+		set Chord $chord
     -+		set IsActivated 0
     ++		my eval set chord $chord
     ++		set is_activated 0
      +	}
      +
     ++	# Method:
     ++	#   [$note is_activated]
     ++	#     Returns true if this note has already been activated.
      +	method is_activated {} {
     -+		return $IsActivated
     ++		return $is_activated
      +	}
      +
     ++	# Method:
     ++	#   $note
     ++	#     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 {} {
     -+		if {!$IsActivated} {
     -+			set IsActivated 1
     -+			$Chord notify_note_activation
     ++		if {!$is_activated} {
     ++			set is_activated 1
     ++			$chord notify_note_activation
      +		}
      +	}
      +}
     @@ -164,16 +214,103 @@
       }
       
      -proc _close_updateindex {fd after} {
     +-	global use_ttk NS
     ++# Returns true if the operation succeeded, false if a rescan has been initiated.
     ++proc _close_updateindex_rescan_on_error {fd} {
     ++	if {![catch {_close_updateindex $fd} err]} {
     ++		return true
     ++	} else {
     ++		rescan_on_error $err
     ++		return false
     ++	}
     ++}
     ++
      +proc _close_updateindex {fd} {
     - 	global use_ttk NS
       	fconfigure $fd -blocking 1
     - 	if {[catch {close $fd} err]} {
     -@@
     - 	}
     +-	if {[catch {close $fd} err]} {
     +-		set w .indexfried
     +-		Dialog $w
     +-		wm withdraw $w
     +-		wm title $w [strcat "[appname] ([reponame]): " [mc "Index Error"]]
     +-		wm geometry $w "+[winfo rootx .]+[winfo rooty .]"
     +-		set s [mc "Updating the Git index failed.  A rescan will be automatically started to resynchronize git-gui."]
     +-		text $w.msg -yscrollcommand [list $w.vs set] \
     +-			-width [string length $s] -relief flat \
     +-			-borderwidth 0 -highlightthickness 0 \
     +-			-background [get_bg_color $w]
     +-		$w.msg tag configure bold -font font_uibold -justify center
     +-		${NS}::scrollbar $w.vs -command [list $w.msg yview]
     +-		$w.msg insert end $s bold \n\n$err {}
     +-		$w.msg configure -state disabled
     +-
     +-		${NS}::button $w.continue \
     +-			-text [mc "Continue"] \
     +-			-command [list destroy $w]
     +-		${NS}::button $w.unlock \
     +-			-text [mc "Unlock Index"] \
     +-			-command "destroy $w; _delete_indexlock"
     +-		grid $w.msg - $w.vs -sticky news
     +-		grid $w.unlock $w.continue - -sticky se -padx 2 -pady 2
     +-		grid columnconfigure $w 0 -weight 1
     +-		grid rowconfigure $w 0 -weight 1
     +-
     +-		wm protocol $w WM_DELETE_WINDOW update
     +-		bind $w.continue <Visibility> "
     +-			grab $w
     +-			focus %W
     +-		"
     +-		wm deiconify $w
     +-		tkwait window $w
     ++	close $fd
     ++	$::main_status stop
     ++}
     + 
     +-		$::main_status stop
     +-		unlock_index
     +-		rescan $after 0
     +-		return
     +-	}
     ++proc rescan_on_error {err} {
     ++	global use_ttk NS
     ++
     ++	set w .indexfried
     ++	Dialog $w
     ++	wm withdraw $w
     ++	wm title $w [strcat "[appname] ([reponame]): " [mc "Index Error"]]
     ++	wm geometry $w "+[winfo rootx .]+[winfo rooty .]"
     ++	set s [mc "Updating the Git index failed.  A rescan will be automatically started to resynchronize git-gui."]
     ++	text $w.msg -yscrollcommand [list $w.vs set] \
     ++		-width [string length $s] -relief flat \
     ++		-borderwidth 0 -highlightthickness 0 \
     ++		-background [get_bg_color $w]
     ++	$w.msg tag configure bold -font font_uibold -justify center
     ++	${NS}::scrollbar $w.vs -command [list $w.msg yview]
     ++	$w.msg insert end $s bold \n\n$err {}
     ++	$w.msg configure -state disabled
     ++
     ++	${NS}::button $w.continue \
     ++		-text [mc "Continue"] \
     ++		-command [list destroy $w]
     ++	${NS}::button $w.unlock \
     ++		-text [mc "Unlock Index"] \
     ++		-command "destroy $w; _delete_indexlock"
     ++	grid $w.msg - $w.vs -sticky news
     ++	grid $w.unlock $w.continue - -sticky se -padx 2 -pady 2
     ++	grid columnconfigure $w 0 -weight 1
     ++	grid rowconfigure $w 0 -weight 1
     ++
     ++	wm protocol $w WM_DELETE_WINDOW update
     ++	bind $w.continue <Visibility> "
     ++		grab $w
     ++		focus %W
     ++	"
     ++	wm deiconify $w
     ++	tkwait window $w
       
       	$::main_status stop
     --	unlock_index
     + 	unlock_index
      -	uplevel #0 $after
     ++	rescan ui_ready 0
       }
       
       proc update_indexinfo {msg path_list after} {
     @@ -182,8 +319,10 @@
       
       	if {$update_index_cp >= $total_cnt} {
      -		_close_updateindex $fd $after
     -+		_close_updateindex $fd
     -+		unlock_index
     ++		if {[_close_updateindex_rescan_on_error $fd]} {
     ++			unlock_index
     ++		}
     ++
      +		uplevel #0 $after
       		return
       	}
     @@ -193,37 +332,86 @@
       
       	if {$update_index_cp >= $total_cnt} {
      -		_close_updateindex $fd $after
     -+		_close_updateindex $fd
     -+		unlock_index
     ++		if {[_close_updateindex_rescan_on_error $fd]} {
     ++			unlock_index
     ++		}
     ++
      +		uplevel #0 $after
       		return
       	}
       
      @@
     + 	$::main_status update $update_index_cp $total_cnt
     + }
     + 
     +-proc checkout_index {msg path_list after} {
     ++proc checkout_index {msg path_list after capture_error} {
     + 	global update_index_cp
     + 
     + 	if {![lock_index update]} return
     +@@
     + 		$total_cnt \
     + 		$batch \
     + 		$after \
     ++		$capture_error \
     + 		]
     + }
     + 
     +-proc write_checkout_index {fd path_list total_cnt batch after} {
     ++proc write_checkout_index {fd path_list total_cnt batch after capture_error} {
     + 	global update_index_cp
       	global file_states current_diff_path
       
       	if {$update_index_cp >= $total_cnt} {
      -		_close_updateindex $fd $after
     -+		_close_updateindex $fd $do_unlock_index $after
     ++		if {[catch {_close_updateindex $fd} err]} {
     ++			uplevel #0 $capture_error [list $err]
     ++		}
     ++
      +		uplevel #0 $after
     ++
       		return
       	}
       
      @@
     + 	add_helper [mc "Adding all changed files"] $paths
     + }
     + 
     ++# Copied from TclLib package "lambda".
     ++proc lambda {arguments body args} {
     ++	return [list ::apply [list $arguments $body] {*}$args]
     ++}
     ++
     + proc revert_helper {txt paths} {
     + 	global file_states current_diff_path
       
       	if {![lock_index begin-update]} return
       
      +	# Common "after" functionality that waits until multiple asynchronous
      +	# operations are complete (by waiting for them to activate their notes
      +	# on the chord).
     ++	#
     ++	# The asynchronous operations are each indicated below by a comment
     ++	# before the code block that starts the async operation.
      +	set after_chord [SimpleChord new {
      +		unlock_index
     -+		if {$should_reshow_diff} { reshow_diff }
     -+		ui_ready
     ++		if {[string trim $err] != ""} {
     ++			rescan_on_error $err
     ++		} else {
     ++			if {$should_reshow_diff} { reshow_diff }
     ++			ui_ready
     ++		}
      +	}]
      +
      +	$after_chord eval { set should_reshow_diff 0 }
      +
     ++	# This function captures an error for processing when after_chord is
     ++	# completed. (The chord is curried into the lambda function.)
     ++	set capture_error [lambda \
     ++		{chord error} \
     ++		{ $chord eval [list set err $error] } \
     ++		$after_chord]
     ++
      +	# We don't know how many notes we're going to create (it's dynamic based
      +	# on conditional paths below), so create a common note that will delay
      +	# the chord's completion until we activate it, and then activate it
     @@ -231,8 +419,9 @@
      +	set after_common_note [$after_chord add_note]
      +
       	set path_list [list]
     +-	set after {}
      +	set untracked_list [list]
     - 	set after {}
     ++
       	foreach path $paths {
       		switch -glob -- [lindex $file_states($path) 0] {
       		U? {continue}
     @@ -253,26 +442,9 @@
       
      +	set path_cnt [llength $path_list]
      +	set untracked_cnt [llength $untracked_list]
     - 
     --	# Split question between singular and plural cases, because
     --	# such distinction is needed in some languages. Previously, the
     --	# code used "Revert changes in" for both, but that can't work
     --	# in languages where 'in' must be combined with word from
     --	# rest of string (in different way for both cases of course).
     --	#
     --	# FIXME: Unfortunately, even that isn't enough in some languages
     --	# as they have quite complex plural-form rules. Unfortunately,
     --	# msgcat doesn't seem to support that kind of string translation.
     --	#
     --	set n [llength $path_list]
     --	if {$n == 0} {
     --		unlock_index
     --		return
     --	} elseif {$n == 1} {
     --		set query [mc "Revert changes in file %s?" [short_path [lindex $path_list]]]
     --	} else {
     --		set query [mc "Revert changes in these %i files?" $n]
     --	}
     ++
     ++	# Asynchronous operation: revert changes by checking them out afresh
     ++	# from the index.
      +	if {$path_cnt > 0} {
      +		# Split question between singular and plural cases, because
      +		# such distinction is needed in some languages. Previously, the
     @@ -295,25 +467,31 @@
      +				"Revert changes in these %i files?" \
      +				$path_cnt]
      +		}
     - 
     --	set reply [tk_dialog \
     --		.confirm_revert \
     --		"[appname] ([reponame])" \
     --		"$query
     ++
      +		set reply [tk_dialog \
      +			.confirm_revert \
      +			"[appname] ([reponame])" \
      +			"$query
       
     - [mc "Any unstaged changes will be permanently lost by the revert."]" \
     --		question \
     --		1 \
     --		[mc "Do Nothing"] \
     --		[mc "Revert Changes"] \
     --		]
     --	if {$reply == 1} {
     --		checkout_index \
     --			$txt \
     +-	# Split question between singular and plural cases, because
     +-	# such distinction is needed in some languages. Previously, the
     +-	# code used "Revert changes in" for both, but that can't work
     +-	# in languages where 'in' must be combined with word from
     +-	# rest of string (in different way for both cases of course).
     +-	#
     +-	# FIXME: Unfortunately, even that isn't enough in some languages
     +-	# as they have quite complex plural-form rules. Unfortunately,
     +-	# msgcat doesn't seem to support that kind of string translation.
     +-	#
     +-	set n [llength $path_list]
     +-	if {$n == 0} {
     +-		unlock_index
     +-		return
     +-	} elseif {$n == 1} {
     +-		set query [mc "Revert changes in file %s?" [short_path [lindex $path_list]]]
     +-	} else {
     +-		set query [mc "Revert changes in these %i files?" $n]
     ++[mc "Any unstaged changes will be permanently lost by the revert."]" \
      +			question \
      +			1 \
      +			[mc "Do Nothing"] \
     @@ -324,10 +502,16 @@
      +			checkout_index \
      +				$txt \
      +				$path_list \
     -+				[$after_chord add_note]
     ++				[$after_chord add_note] \
     ++				$capture_error
      +		}
     -+	}
     -+
     + 	}
     + 
     +-	set reply [tk_dialog \
     +-		.confirm_revert \
     +-		"[appname] ([reponame])" \
     +-		"$query
     ++	# Asynchronous operation: Deletion of untracked files.
      +	if {$untracked_cnt > 0} {
      +		# Split question between singular and plural cases, because
      +		# such distinction is needed in some languages.
     @@ -348,7 +532,16 @@
      +				$untracked_cnt \
      +				]
      +		}
     -+
     + 
     +-[mc "Any unstaged changes will be permanently lost by the revert."]" \
     +-		question \
     +-		1 \
     +-		[mc "Do Nothing"] \
     +-		[mc "Revert Changes"] \
     +-		]
     +-	if {$reply == 1} {
     +-		checkout_index \
     +-			$txt \
      +		set reply [tk_dialog \
      +			.confirm_revert \
      +			"[appname] ([reponame])" \
     @@ -453,8 +646,9 @@
      +		# Report error, if any, based on how many deletions failed.
      +		set deletion_error_cnt [llength $deletion_errors]
      +
     -+		if {($deletion_error_cnt > 0) && ($deletion_error_cnt <= [MAX_VERBOSE_FILES_IN_DELETION_ERROR])} {
     -+			set error_text "Encountered errors deleting files:\n"
     ++		if {($deletion_error_cnt > 0)
     ++		 && ($deletion_error_cnt <= [MAX_VERBOSE_FILES_IN_DELETION_ERROR])} {
     ++			set error_text [mc "Encountered errors deleting files:\n"]
      +
      +			foreach deletion_error $deletion_errors {
      +				append error_text "* [lindex $deletion_error 0]\n"

-- 
gitgitgadget