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

Submitted-As:  Message-ID: pull.436.v3.git.1573638988.gitgitgadget@gmail.com
In-Reply-To:  Message-ID: pull.436.git.1572418123.gitgitgadget@gmail.com
In-Reply-To:  Message-ID: pull.436.v2.git.1573110335.gitgitgadget@gmail.com