git-gui: fix error handling of Revert Changes command

The command Revert Changes has two different erroneous behaviors
depending on the Tcl version used.

The command uses a "chord" facility where different "notes" are
evaluated asynchronously and any error is reported after all of them
have finished. The intent is that a private namespace is used where
the notes can store the error state. Tcl 9 changed namespace handling
in a subtle way, as https://www.tcl-lang.org/software/tcltk/9.0.html
summarizes under "Notable incompatibilities":

    Unqualified varnames resolved in current namespace, not global.
    Note that in almost all cases where this causes a change, the
    change is actually the removal of a latent bug.

And that's exactly what happens here.

- Under Tcl 9:

  - When the command operates without any errors, the variable `err`
    is never set. When the error handler wants to inspect `err` (in
    the correct private namespace), it does not find it and a Tcl
    error about an unset variable occurs. Incidentally, this is also
    the case when the user cancels the operation with the option
    "Do Nothing"!

    On the other hand, when an error occurs during the operation, `err`
    is set and found as intended.

  Check for the existence of the variable `err` before the attempt to
  read it.

- Under Tcl 8.6:

  The error handler looks up `err` in the global namespace, which is
  bogus and unintended. The variable is set due to the many
  `catch ... err` that occur during startup in the global namespace.

  - When the command operates without any errors, the error handler
    finds the global `err`, which happens to be the empty string at
    this point, and no error is reported.

    On the other hand, when an error occurs during the operation, the
    global `err` is set and found, so that an error is reported as
    desired.

    However, the value of `err` persists in the global namespace. When
    the command is repeated, an error is reported again, even if there
    was actually no error, and even "Do Nothing" was used to cancel
    the operation.

  Clear the global `err` before the operation begins.

The lingering error message is not a problem under Tcl 9, because a
prestine namespace is established every time the command is used.

This fixes https://github.com/j6t/git-gui/issues/21.

Helped-by: Igor Stepushchik
Signed-off-by: Johannes Sixt <j6t@kdbg.org>

diff --git a/lib/index.tcl b/lib/index.tcl
index 7aa09c7728267e7f004688796f11f5e66a15708c..e1d38e54bee36ac77e0ce77d28b987da7122b22d 100644 (file)
--- a/lib/index.tcl
+++ b/lib/index.tcl
@@ -425,6 +425,11 @@ proc revert_helper {txt paths} {
 
        if {![lock_index begin-update]} return
 
+       # Workaround for Tcl < 9.0: chord namespaces are not obeyed and
+       # operated in the global namespace. This clears an error that could
+       # have been left over from a previous operation.
+       set ::err {}
+
        # Common "after" functionality that waits until multiple asynchronous
        # operations are complete (by waiting for them to activate their notes
        # on the chord).
@@ -432,7 +437,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 {
-               if {[string trim $err] != ""} {
+               if {[info exists err] && [string trim $err] ne ""} {
                        rescan_on_error $err
                } else {
                        unlock_index