On Sat, Nov 16, 2019 at 9:11 AM Pratyush Yadav me-at-yadavpratyush.com
|GitHub Public/Example Allow| [off-list ref] wrote:

quoted

-             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

I didn't spot this earlier. Will this call to 'stop' interfere with the
'start' in 'delete_files'?

Hmm, I think this actually highlights a larger issue. Both
`write_checkout_index` and `delete_helper` display their progress in
the status bar, so if the user elects to do a check-out, and then
while it is still in progress asynchronously, elects to delete files,
they'll fight over who gets to set the status. If I'm understanding
correctly, this won't actually interfere with correct operation, but
of course it won't look very nice.

If they overlap in this manner, _then_ multiple calls to `stop` could
be made, though it does appear that `stop` is idempotent. The Tk
documentation states that `destroy` doesn't return any error if you
point it at a window that doesn't exist.

`start` is explicitly idempotent, only creating a new canvas if it
doesn't already have one.

I'll see what I can come up with for letting operations more cleanly
share the status bar.

quoted

      if {$update_index_cp >= $total_cnt} {
-             _close_updateindex $fd $after
+             if {[_close_updateindex_rescan_on_error $fd]} {
+                     unlock_index
+             }
+
+             uplevel #0 $after

This changes when $after is called. If you pass it to 'rescan', it runs
_after_ the rescan is finished. Now it runs "in parallel" with it. Are
you sure that is the intended behaviour? Should we just stick to passing
$after to rescan on failure?

[..]

While we're here, how about just moving this entire thing to
'_close_updateindex_rescan_on_error', since the only two consumers of
the function do the _exact_ same thing?

This would also allow us to pass $after to 'rescan'. It would also
hopefully make the code a bit easier to follow because you can clearly
see that we only unlock the index when there is no error.

Even better, unlock the index unconditionally in
'_close_updateindex_rescan_on_error', and remove the 'unlock_index' call
from 'rescan_on_error'. I generally prefer to keep locking/unlocking
paths as simple as possible.

Hmm, yeah, this makes sense. Pass it `$after`, and then if it calls
`rescan`, it can hand it off, and `rescan` also (I'm assuming?)
implicitly unlocks the index. If it doesn't need to call `rescan`,
then `_close_updateindex_rescan_on_error` itself unlocks the index
_and_ invokes `$after`.

quoted

      if {$update_index_cp >= $total_cnt} {
-             _close_updateindex $fd $after
+             if {[catch {_close_updateindex $fd} err]} {
+                     uplevel #0 $capture_error [list $err]
+             }
+
+             uplevel #0 $after
+

Nitpick: Please explicitly mention why we _don't_ want to unlock the
index here.

There are two function very similar to this one: 'write_update_index'
and 'write_update_indexinfo'. This subtle but important difference is
very easy to gloss over.

Hmm, so, this suggests a rename of
`_close_updateindex_rescan_on_error`, because (with the previous
proposal) it implicitly includes unlocking the index, whereas
`_close_updateindex` does not.

Thanks,

Jonathan Gilbert