Re: [PATCH v3 2/2] git-gui: revert untracked files by deleting them
- From
Jonathan Gilbert <logic@deltaq.org>
- Date
- Nov 16, 2019, 21:42 UTC
- Message-ID
- <CAPSOpYuv1d_yAgBnVBDXMwaA3B3QpmbVKtiSpexpJDwC6Bz33w@mail.gmail.com>
- In-Reply-To
- <20191116151113.mwbaendh6lgykfw3@yadavpratyush.com>
On Sat, Nov 16, 2019 at 9:11 AM Pratyush Yadav me-at-yadavpratyush.com |GitHub Public/Example Allow| <172q77k4bxwj0zt@sneakemail.com> wrote:
Show 17 quoted lines
> > - 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.
Show 27 quoted lines
> > 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`.
Show 15 quoted lines
> > 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