| Message ID | 20201101170505.71246-2-stefan@haller-berlin.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | git-gui: Auto-rescan on activate | expand |
Hi Stefan, On 01/11/20 06:05PM, Stefan Haller wrote: > This is to ensure that a rescan is only performed once, even if it is > requested multiple times during one event. We don't need this yet, because > we only ever call do_rescan once per event so far; this is going to change > with the next commit, when we also call it from FocusIn. I don't understand what this is trying to achieve. The calls to do_rescan below only happen when the user explicitly does something, like stage/unstage selected lines. Why would that event coincide with the FocusIn event? If you mean to account for a situation where the rescan for "Apply/Reverse Line" is executed before the rescan from FocusIn finishes, then in that case the procedure rescan already accounts for it by checking $rescan_active and the index lock. Have you noticed multiple rescans in parallel? If yes then we might want to look at why the check is not working. > Signed-off-by: Stefan Haller <stefan@haller-berlin.de> > --- > git-gui.sh | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/git-gui.sh b/git-gui.sh > index 867b8ce..8864c14 100755 > --- a/git-gui.sh > +++ b/git-gui.sh > @@ -2376,8 +2376,21 @@ proc do_quit {{rc {1}}} { > destroy . > } > > +# Not to be called directly; use schedule_rescan instead > proc do_rescan {} { > + global rescan_id > + > rescan ui_ready > + unset rescan_id Not sure if you're aware of it already, but it is worth mentioning that rescan is asynchronous. The procedure call will return before the rescan in actually complete. See the `fileevent` calls in rescan and rescan_stage2. So in this case, rescan_id will be unset before the rescan is actually done. This can be the right or wrong thing depending on what you want to accomplish, which I'm not clear on. > +} > + > +proc schedule_rescan {} { > + global rescan_id > + > + if {[info exists rescan_id]} { > + after cancel $rescan_id > + } > + set rescan_id [after idle do_rescan] > } > > proc ui_do_rescan {} { > @@ -3683,7 +3696,7 @@ set ui_diff_applyhunk [$ctxm index last] > lappend diff_actions [list $ctxm entryconf $ui_diff_applyhunk -state] > $ctxm add command \ > -label [mc "Apply/Reverse Line"] \ > - -command {apply_or_revert_range_or_line $cursorX $cursorY 0; do_rescan} > + -command {apply_or_revert_range_or_line $cursorX $cursorY 0; schedule_rescan} > set ui_diff_applyline [$ctxm index last] > lappend diff_actions [list $ctxm entryconf $ui_diff_applyline -state] > $ctxm add separator > @@ -3694,12 +3707,12 @@ set ui_diff_reverthunk [$ctxm index last] > lappend diff_actions [list $ctxm entryconf $ui_diff_reverthunk -state] > $ctxm add command \ > -label [mc "Revert Line"] \ > - -command {apply_or_revert_range_or_line $cursorX $cursorY 1; do_rescan} > + -command {apply_or_revert_range_or_line $cursorX $cursorY 1; schedule_rescan} > set ui_diff_revertline [$ctxm index last] > lappend diff_actions [list $ctxm entryconf $ui_diff_revertline -state] > $ctxm add command \ > -label [mc "Undo Last Revert"] \ > - -command {undo_last_revert; do_rescan} > + -command {undo_last_revert; schedule_rescan} > set ui_diff_undorevert [$ctxm index last] > lappend diff_actions [list $ctxm entryconf $ui_diff_undorevert -state] > $ctxm add separator > @@ -4171,7 +4184,7 @@ after 1 { > if {[is_enabled initialamend]} { > force_amend > } else { > - do_rescan > + schedule_rescan > } > > if {[is_enabled nocommitmsg]} { > -- > 2.29.2 >
On 02.11.20 16:45, Pratyush Yadav wrote: > Hi Stefan, > > On 01/11/20 06:05PM, Stefan Haller wrote: >> This is to ensure that a rescan is only performed once, even if it is >> requested multiple times during one event. We don't need this yet, because >> we only ever call do_rescan once per event so far; this is going to change >> with the next commit, when we also call it from FocusIn. > > I don't understand what this is trying to achieve. The calls to > do_rescan below only happen when the user explicitly does something, > like stage/unstage selected lines. Why would that event coincide with > the FocusIn event? > > If you mean to account for a situation where the rescan for > "Apply/Reverse Line" is executed before the rescan from FocusIn > finishes, then in that case the procedure rescan already accounts for it > by checking $rescan_active and the index lock. I'm aware that the rescan runs asynchronously, but I wasn't worried about the case where it's triggered concurrently while another one is running already; I was worried about the case where a rescan (e.g. coming from "Apply/Reverse Line") was so fast that it was already finished by the time the FocusIn comes. But I guess I misunderstood Tk's threading model, and this can never happen. So it does indeed seem that the existing $rescan_active logic is enough to prevent unnecessary rescans; I'll drop this commit. > Have you noticed multiple rescans in parallel? If yes then we might want > to look at why the check is not working. > >> Signed-off-by: Stefan Haller <stefan@haller-berlin.de> >> --- >> git-gui.sh | 21 +++++++++++++++++---- >> 1 file changed, 17 insertions(+), 4 deletions(-) >> >> diff --git a/git-gui.sh b/git-gui.sh >> index 867b8ce..8864c14 100755 >> --- a/git-gui.sh >> +++ b/git-gui.sh >> @@ -2376,8 +2376,21 @@ proc do_quit {{rc {1}}} { >> destroy . >> } >> >> +# Not to be called directly; use schedule_rescan instead >> proc do_rescan {} { >> + global rescan_id >> + >> rescan ui_ready >> + unset rescan_id > > Not sure if you're aware of it already, but it is worth mentioning that > rescan is asynchronous. The procedure call will return before the rescan > in actually complete. See the `fileevent` calls in rescan and > rescan_stage2. > > So in this case, rescan_id will be unset before the rescan is actually > done. This can be the right or wrong thing depending on what you want to > accomplish, which I'm not clear on. > >> +} >> + >> +proc schedule_rescan {} { >> + global rescan_id >> + >> + if {[info exists rescan_id]} { >> + after cancel $rescan_id >> + } >> + set rescan_id [after idle do_rescan] >> } >> >> proc ui_do_rescan {} { >> @@ -3683,7 +3696,7 @@ set ui_diff_applyhunk [$ctxm index last] >> lappend diff_actions [list $ctxm entryconf $ui_diff_applyhunk -state] >> $ctxm add command \ >> -label [mc "Apply/Reverse Line"] \ >> - -command {apply_or_revert_range_or_line $cursorX $cursorY 0; do_rescan} >> + -command {apply_or_revert_range_or_line $cursorX $cursorY 0; schedule_rescan} >> set ui_diff_applyline [$ctxm index last] >> lappend diff_actions [list $ctxm entryconf $ui_diff_applyline -state] >> $ctxm add separator >> @@ -3694,12 +3707,12 @@ set ui_diff_reverthunk [$ctxm index last] >> lappend diff_actions [list $ctxm entryconf $ui_diff_reverthunk -state] >> $ctxm add command \ >> -label [mc "Revert Line"] \ >> - -command {apply_or_revert_range_or_line $cursorX $cursorY 1; do_rescan} >> + -command {apply_or_revert_range_or_line $cursorX $cursorY 1; schedule_rescan} >> set ui_diff_revertline [$ctxm index last] >> lappend diff_actions [list $ctxm entryconf $ui_diff_revertline -state] >> $ctxm add command \ >> -label [mc "Undo Last Revert"] \ >> - -command {undo_last_revert; do_rescan} >> + -command {undo_last_revert; schedule_rescan} >> set ui_diff_undorevert [$ctxm index last] >> lappend diff_actions [list $ctxm entryconf $ui_diff_undorevert -state] >> $ctxm add separator >> @@ -4171,7 +4184,7 @@ after 1 { >> if {[is_enabled initialamend]} { >> force_amend >> } else { >> - do_rescan >> + schedule_rescan >> } >> >> if {[is_enabled nocommitmsg]} { >> -- >> 2.29.2 >> >
diff --git a/git-gui.sh b/git-gui.sh index 867b8ce..8864c14 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -2376,8 +2376,21 @@ proc do_quit {{rc {1}}} { destroy . } +# Not to be called directly; use schedule_rescan instead proc do_rescan {} { + global rescan_id + rescan ui_ready + unset rescan_id +} + +proc schedule_rescan {} { + global rescan_id + + if {[info exists rescan_id]} { + after cancel $rescan_id + } + set rescan_id [after idle do_rescan] } proc ui_do_rescan {} { @@ -3683,7 +3696,7 @@ set ui_diff_applyhunk [$ctxm index last] lappend diff_actions [list $ctxm entryconf $ui_diff_applyhunk -state] $ctxm add command \ -label [mc "Apply/Reverse Line"] \ - -command {apply_or_revert_range_or_line $cursorX $cursorY 0; do_rescan} + -command {apply_or_revert_range_or_line $cursorX $cursorY 0; schedule_rescan} set ui_diff_applyline [$ctxm index last] lappend diff_actions [list $ctxm entryconf $ui_diff_applyline -state] $ctxm add separator @@ -3694,12 +3707,12 @@ set ui_diff_reverthunk [$ctxm index last] lappend diff_actions [list $ctxm entryconf $ui_diff_reverthunk -state] $ctxm add command \ -label [mc "Revert Line"] \ - -command {apply_or_revert_range_or_line $cursorX $cursorY 1; do_rescan} + -command {apply_or_revert_range_or_line $cursorX $cursorY 1; schedule_rescan} set ui_diff_revertline [$ctxm index last] lappend diff_actions [list $ctxm entryconf $ui_diff_revertline -state] $ctxm add command \ -label [mc "Undo Last Revert"] \ - -command {undo_last_revert; do_rescan} + -command {undo_last_revert; schedule_rescan} set ui_diff_undorevert [$ctxm index last] lappend diff_actions [list $ctxm entryconf $ui_diff_undorevert -state] $ctxm add separator @@ -4171,7 +4184,7 @@ after 1 { if {[is_enabled initialamend]} { force_amend } else { - do_rescan + schedule_rescan } if {[is_enabled nocommitmsg]} {
This is to ensure that a rescan is only performed once, even if it is requested multiple times during one event. We don't need this yet, because we only ever call do_rescan once per event so far; this is going to change with the next commit, when we also call it from FocusIn. Signed-off-by: Stefan Haller <stefan@haller-berlin.de> --- git-gui.sh | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)