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

[Jonathan Gilbert <logic@deltaq.org>]

On Sat, Nov 16, 2019 at 9:11 AM Pratyush Yadav me-at-yadavpratyush.com
|GitHub Public/Example Allow| <172q77k4bxwj0zt@sneakemail.com> wrote:
> > -             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.

> >       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`.

> >       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