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.

Since the checkout_index and delete_files calls are both asynchronous and
overlap, they clash in wanting to update the status bar. To address this,
the status bar is reworked so that when an operation wants to display
ongoing updates/progress, it explicitly starts an "operation", which is
tracked by its own object, and the status bar handles multiple concurrent
operations by merging their progress and concatenating their text. This is
captured in a separate commit, since it touches a variety of files.

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. Call sites that
exercised the combined functionality also unlocked the index, so a combined
proc close_and_unlock_index calls _close_updateindex and then either
rescan_on_error or unlock_index as appropriate. Call sites have been updated
appropriately.

The revert_helper proc, with its overlapping operations, is an example of a
call site that does not combine the close and unlock/rescan operations. The
checkout_index proc has been reworked to only call _close_updateindex, and
to call a functor supplied by the caller to captures any errors that occur.
revert_helper uses this to supply a lambda function that stashes the error
within the chord's body namespace, so that it can then separately call
rescan_on_error when the chord is completed (or unlock_index, if no error
was captured), which might be substantially after checkout_index encounters
its error. If it turns out that a rescan is called for, it is done once the
deletion is complete.

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

 * The status_bar.tcl changes have been isolated into a separate commit.

 * A method in status_bar.tcl that apparently had never been hit had some
   simple bugs in it that have been corrected.

 * The show methods on the classes in status_bar.tcl no longer have a test
   parameter, as nothing was using that feature that I could see.

 * The refresh method in status_bar.tcl now only tries to update the
   progress bar widget if it actually exists.

 * blame.tcl had been missed when searching for code using the status bar,
   this has been addressed.

 * rescan_on_error takes $after as well, in case
   close_and_unlock_updateindex_rescan_on_error needs to call it.

 * close_and_unlock_updateindex_rescan_on_error has been renamed to simply
   close_and_unlock_index.

On Sun, Nov 24, 2019 at 7:09 AM Pratyush Yadav wrote:

> Unfortunately, this change breaks things. The users of 'status_bar' that
aren't updated don't work. As an example, if I run 'git gui blame
git-gui.sh' with your patch applied, I get the following error:

 invalid command name "::status_bar::update"

I thought I'd found all places that used the status bar, but apparently
overlooked blame.tcl. This has been addressed. My apologies!

choose_repository.tcl was addressed already in the previous commit.

> Either way, please split the status bar refactor in a separate commit such
that the entire system still works properly (so this means the commit would
include updating the existing callsites).

Done. In doing so and testing it independently, I actually found paths that
weren't being hit with the full set of changes and fixed a few bugs. In
particular, stop_all was completely broken :-P I thought I had exercised it
before and I hadn't.

> > +# Clear "Initializing..." status +after idle {after 500 {$main_status show
""}}

Why put this in an 'after idle'? What's wrong with just 'after 500'? This is
not an expensive operation so we shouldn't really require the application to
be idle to run it.

My thinking was to make it clear the status bar 500 milliseconds after the
queue empties. It's quite possible that my understanding of what it means
for the queue to have emptied means that this isn't a terribly meaningful
thing to do. I was wanting it to continue saying "Initializing..." until,
heuristically, it is done initializing, including any queued up operations.
I guess if anything waits on I/O, then the queue may to go idle even though
it's still busy doing things. So, a straight-up after 500 { } would make
more sense? I've changed it to this, can make further changes if they are
called for.

> > +# Returns true if the operation succeeded, false if a rescan has been
initiated. +proc close_and_unlock_updateindex_rescan_on_error {fd after} {

Nitpick: That name is a bit too unwieldy. Maybe something a bit more concise
like 'close_and_unlock_index' (I'm not great at naming things. Maybe you can
figure out something better)? Let the readers figure out what happens on
error.

Good point. The thinking that had been going on in my head is that
close_and_unlock_updateindex_rescan_on_error was combining two operations
that most but not all paths would do together, and then the paths that
needed them separated would call close_and_unlock in one place and
rescan_on_error in another. The previous "cover letter" actually explicitly
described this, referring to proc names that actually weren't current any
more by the time the code was submitted. Upon closer review, at this point,
the place that calls rescan_on_error independently is not making a separate
call to something like close_and_unlock, it's just calling
_close_updateindex directly, and the unlock is occurring in the chord body.

> >  * if {![catch {_close_updateindex $fd} err]} {    unlock_index

 * rescan $after 0
 * return
 * uplevel #0 $after
 * return 1
 * } else {
 * rescan_on_error $err $after
 * return 0

Neither of the two callers use the return value. Are these really needed?

I'm pretty sure there was an iteration of the code where at least one path
checked, but since nothing checks it now, I've removed the return value.

> > +proc write_checkout_index {fd path_list total_cnt batch
status_bar_operation \

 * after capture_error} { global update_index_cp global file_states
   current_diff_path

   if {$update_index_cp >= $total_cnt} {

 * _close_updateindex $fd $after

 * $status_bar_operation stop

 *
 * if {[catch {_close_updateindex $fd} err]} {

Nitpick: Please mention exactly why we don't use
'close_and_unlock_updateindex_rescan_on_error' (or whatever its new name
would be) here. This function is very similar to 'write_update_index' and
'write_checkout_index', so this subtle difference is not very easily
apparent.

Done.

> > -field status {}; # single line of text we show

The field 'status' is removed, but the procedure 'show' still uses it. The
if condition needs to be refactored.

As far as I can tell, this feature of show to only change the status if the
test matches isn't actually used by any call sites. I've removed it. If this
was in error, then I'll reintroduce it correctly. I think, generally
speaking, supporting overlapping operations resolves the problem that this
would have originally resolved, which is that something else has updated the
status since it was previously set and the caller doesn't want to clear
somebody else's more up-to-date status text.

> >  * if {!$allow_multiple && [llength $operations]} {

This silently ignores multiple 'start's on a status bar that doesn't allow
it, correct?

It does, yes. The only status bar that doesn't allow multiple operations is
the two-line one created by choose_repository.

A caller that did this erroneously would get the same operation reference
both times, which would mean that (presumably) it got stopped multiple times
--- but stop on a status bar operation is idempotent, so this shouldn't
actually result in any errors.

> One quick question: the consumers of status_bar who don't run multiple
operations in parallel would still continue working exactly the same (after
refactoring them to use 'status_bar_operation'), right?

This is the expectation, yes, and is what I have observed in my testing. :-)

git remote add logiclrd https://github.com/logiclrd/git.git
git fetch logiclrd git-gui-revert-untracked revision5
git diff 23d4f5d..d0d6593b42

Jonathan Gilbert (3):
  git-gui: consolidate naming conventions
  git-gui: update status bar to track operations
  git-gui: revert untracked files by deleting them

 git-gui.sh          |  11 +-
 lib/blame.tcl       |  22 +-
 lib/checkout_op.tcl |  15 +-
 lib/chord.tcl       | 160 ++++++++++++++
 lib/index.tcl       | 523 +++++++++++++++++++++++++++++++++-----------
 lib/merge.tcl       |  14 +-
 lib/status_bar.tcl  | 228 ++++++++++++++++---
 7 files changed, 797 insertions(+), 176 deletions(-)
 create mode 100644 lib/chord.tcl

base-commit: b524f6b399c77b40c8bf2b6217585fde4731472a

Submitted-As:  Message-ID: pull.436.v5.git.1574627876.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
In-Reply-To:  Message-ID: pull.436.v3.git.1573638988.gitgitgadget@gmail.com
In-Reply-To:  Message-ID: pull.436.v4.git.1573973770.gitgitgadget@gmail.com