git-gui: revert untracked files by deleting them #436
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
logiclrd
force-pushed
the
git-gui-revert-untracked
branch
from
f157622
to
f5b84b4
Compare
A few variables in this file use camelCase, while the overall standard is snake_case. A consistent naming scheme will improve readability of future changes. To avoid mixing naming changes with semantic changes, this commit contains only naming changes. Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
logiclrd
force-pushed
the
git-gui-revert-untracked
branch
from
f5b84b4
to
0190f6f
Compare
|
/submit |
|
Submitted as pull.436.git.1572418123.gitgitgadget@gmail.com |
|
On the Git mailing list, Bert Wesarg wrote (reply to this): |
|
On the Git mailing list, Jonathan Gilbert wrote (reply to this): |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
lib/index.tcl
Outdated
| @@ -56,15 +56,15 @@ proc _close_updateindex {fd after} { | |||
| uplevel #0 $after | |||
There was a problem hiding this comment.
On the Git mailing list, Pratyush Yadav wrote (reply to this):
Hi Jonathan,
On 30/10/19 06:48AM, Jonathan Gilbert via GitGitGadget wrote:
> From: Jonathan Gilbert <JonathanG@iQmetrix.com>
>
> A few variables in this file use camelCase, while the overall standard
> is snake_case. A consistent naming scheme will improve readability of
> future changes. To avoid mixing naming changes with semantic changes,
> this commit contains only naming changes.
Thanks for the cleanup. Looks good.
> Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
--
Regards,
Pratyush Yadav
|
On the Git mailing list, Pratyush Yadav wrote (reply to this): |
|
On the Git mailing list, Jonathan Gilbert wrote (reply to this): |
lib/index.tcl
Outdated
| @@ -56,15 +56,15 @@ proc _close_updateindex {fd after} { | |||
| uplevel #0 $after | |||
There was a problem hiding this comment.
On the Git mailing list, Pratyush Yadav wrote (reply to this):
Hi Jonathan,
Thanks for the quality re-roll. It was a pleasant read :)
I would have suggested just handing off the paths to `git clean`, but it
unfortunately does not do what we want it to do.
Say we have a directory 'foo' which has one file called 'bar.txt'. That
file is untracked. Now, I expected `git clean -fd foo/bar.txt` to delete
'bar.txt' _and_ 'foo/', but it only deletes bar.txt, and leaves 'foo/'
intact. What's worse is that since 'foo' is an empty directory, it
doesn't appear in git-status anymore, and so there is no way the user
can tell the directory exists unless they go there and do a `ls`.
Maybe something to fix upstream?
On 30/10/19 06:48AM, Jonathan Gilbert via GitGitGadget wrote:
> From: Jonathan Gilbert <JonathanG@iQmetrix.com>
>
> Updates the revert_helper procedure to also detect untracked files. If
Typo: s/Updates/Update/ ?
> files are present, the user is asked if they want them deleted. A new
> proc delete_files with helper delete_helper performs the deletion in
> batches, to allow the UI to remain responsive.
>
> Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
> ---
> lib/index.tcl | 255 +++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 222 insertions(+), 33 deletions(-)
>
> diff --git a/lib/index.tcl b/lib/index.tcl
> index 28d4d2a54e..9661ddb556 100644
> --- a/lib/index.tcl
> +++ b/lib/index.tcl
> @@ -393,11 +393,20 @@ proc revert_helper {txt paths} {
>
> if {![lock_index begin-update]} return
>
> + # The index is now locked. Some of the paths below include calls that
> + # unlock the index (e.g. checked_index). If we reach the end and the
Typo: s/checked_index/checkout_index/
> + # index is still locked, we need to unlock it before returning.
> + set need_unlock_index 1
> +
> set path_list [list]
> + set untracked_list [list]
> set after {}
> foreach path $paths {
> switch -glob -- [lindex $file_states($path) 0] {
> U? {continue}
> + ?O {
> + lappend untracked_list $path
> + }
> ?M -
> ?T -
> ?D {
> @@ -409,45 +418,225 @@ proc revert_helper {txt paths} {
> }
> }
>
> + set path_cnt [llength $path_list]
> + set untracked_cnt [llength $untracked_list]
>
> - # Split question between singular and plural cases, because
> - # such distinction is needed in some languages. Previously, the
> - # code used "Revert changes in" for both, but that can't work
> - # in languages where 'in' must be combined with word from
> - # rest of string (in different way for both cases of course).
> - #
> - # FIXME: Unfortunately, even that isn't enough in some languages
> - # as they have quite complex plural-form rules. Unfortunately,
> - # msgcat doesn't seem to support that kind of string translation.
> - #
> - set n [llength $path_list]
> - if {$n == 0} {
> - unlock_index
> - return
> - } elseif {$n == 1} {
> - set query [mc "Revert changes in file %s?" [short_path [lindex $path_list]]]
> - } else {
> - set query [mc "Revert changes in these %i files?" $n]
> - }
> + if {$path_cnt > 0} {
> + # Split question between singular and plural cases, because
> + # such distinction is needed in some languages. Previously, the
> + # code used "Revert changes in" for both, but that can't work
> + # in languages where 'in' must be combined with word from
> + # rest of string (in different way for both cases of course).
> + #
> + # FIXME: Unfortunately, even that isn't enough in some languages
> + # as they have quite complex plural-form rules. Unfortunately,
> + # msgcat doesn't seem to support that kind of string
> + # translation.
> + #
> + if {$path_cnt == 1} {
> + set query [mc \
> + "Revert changes in file %s?" \
> + [short_path [lindex $path_list]] \
> + ]
> + } else {
> + set query [mc \
> + "Revert changes in these %i files?" \
> + $path_cnt]
> + }
>
> - set reply [tk_dialog \
> - .confirm_revert \
> - "[appname] ([reponame])" \
> - "$query
> + set reply [tk_dialog \
> + .confirm_revert \
> + "[appname] ([reponame])" \
> + "$query
>
> [mc "Any unstaged changes will be permanently lost by the revert."]" \
> - question \
> - 1 \
> - [mc "Do Nothing"] \
> - [mc "Revert Changes"] \
> - ]
> - if {$reply == 1} {
> - checkout_index \
> - $txt \
> + question \
> + 1 \
> + [mc "Do Nothing"] \
> + [mc "Revert Changes"] \
> + ]
> +
> + if {$reply == 1} {
> + checkout_index \
> + $txt \
> + $path_list \
> + [concat $after [list ui_ready]]
> +
> + set need_unlock_index 0
> + }
> + }
> +
> + if {$need_unlock_index} { unlock_index }
Are you sure you want to unlock the index _before_ the cleanup of
untracked files is done? While it makes sense to unlock the index since
our "clean" operation would only touch the working tree, and not the
index, it would also mean people can do things like "Revert hunk" (from
the context menu). Right now, this operation can not be done on
untracked files (so this won't be a problem for now), but I do plan on
adding this in the future, and it wouldn't be obvious from that patch's
POV that this could be an issue. If someone does a "Revert hunk" on a
while that is queued for deletion, there might be problems.
Also, would doing an `unlock_index` early allow people to run multiple
"clean" jobs at the same time? Will that create race conditions that we
aren't ready to handle?
It also makes sense to evaluate what the downsides of keeping the index
locked are. So, does keeping the index locked prevent meaningful usage
of git-gui, making your batched deletion pointless? Is there some reason
for unlocking it early that I'm missing?
If we do decide keeping the index locked is a good idea, it would be
troublesome to implement. `checkout_index` is asynchronous. So, when it
returns, the index won't necessarily be unlocked. It would get unlocked
some time _after_ the return. I'm not sure how to work around this.
> +
> + if {$untracked_cnt > 0} {
> + # Split question between singular and plural cases, because
> + # such distinction is needed in some languages.
> + #
> + # FIXME: Unfortunately, even that isn't enough in some languages
> + # as they have quite complex plural-form rules. Unfortunately,
> + # msgcat doesn't seem to support that kind of string
> + # translation.
> + #
> + if {$untracked_cnt == 1} {
> + set query [mc \
> + "Delete untracked file %s?" \
> + [short_path [lindex $untracked_list]] \
> + ]
> + } else {
> + set query [mc \
> + "Delete these %i untracked files?" \
> + $untracked_cnt \
> + ]
> + }
> +
> + set reply [tk_dialog \
> + .confirm_revert \
> + "[appname] ([reponame])" \
> + "$query
> +
> +[mc "Files will be permanently deleted."]" \
> + question \
> + 1 \
> + [mc "Do Nothing"] \
> + [mc "Delete Files"] \
> + ]
> +
> + if {$reply == 1} {
> + delete_files $untracked_list
> + }
> + }
> +}
> +
> +# Delete all of the specified files, performing deletion in batches to allow the
> +# UI to remain responsive and updated.
> +proc delete_files {path_list} {
> + # Enable progress bar status updates
> + $::main_status start [mc "Deleting"] [mc "files"]
> +
> + set path_index 0
> + set deletion_errors [list]
> + set deletion_error_path "not yet captured"
> + set batch_size 50
> +
> + delete_helper \
> + $path_list \
> + $path_index \
> + $deletion_errors \
> + $deletion_error_path \
> + $batch_size
> +}
> +
> +# Helper function to delete a list of files in batches. Each call deletes one
> +# batch of files, and then schedules a call for the next batch after any UI
> +# messages have been processed.
> +proc delete_helper \
> + {path_list path_index deletion_errors deletion_error_path batch_size} {
> + global file_states
> +
> + set path_cnt [llength $path_list]
> +
> + set batch_remaining $batch_size
> +
> + while {$batch_remaining > 0} {
> + if {$path_index >= $path_cnt} { break }
> +
> + set path [lindex $path_list $path_index]
> +
> + set deletion_failed [catch {file delete -- $path} deletion_error]
> +
> + if {$deletion_failed} {
> + lappend deletion_errors $deletion_error
> +
> + # Optimistically capture the path that failed, in case
> + # there's only one.
> + set deletion_error_path $path
I don't see why you would do this for _only_ one path. Either do it for
every path. And since you're recording errors for each path, it makes
sense to record the corresponding path too. Or, just count how many
paths failed, and report that. I don't see why we'd want to be between
those two.
> + } else {
> + remove_empty_directories [file dirname $path]
> +
> + # Don't assume the deletion worked. Remove the file from
> + # the UI, but only if it no longer exists.
> + if {![lexists $path]} {
> + unset file_states($path)
> + display_file $path __
> + }
> + }
> +
> + incr path_index 1
> + incr batch_remaining -1
> + }
> +
> + # Update the progress bar to indicate that this batch has been
> + # completed. The update will be visible when this procedure returns
> + # and allows the UI thread to process messages.
> + $::main_status update $path_index $path_cnt
> +
> + if {$path_index < $path_cnt} {
> + # The Tcler's Wiki lists this as the best practice for keeping
> + # a UI active and processing messages during a long-running
> + # operation.
> +
> + after idle [list after 0 [list \
> + delete_helper \
> $path_list \
> - [concat $after [list ui_ready]]
> + $path_index \
> + $deletion_errors \
> + $deletion_error_path \
> + $batch_size \
> + ]]
Using `after idle` means in theory we put an undefined maximum time
limit on the deletion process. Though I suspect in real life it would be
a pretty short time.
Nonetheless, should you instead do this asynchronously, instead of
waiting for the event loop to enter an idle state? This means using
`after 0` directly, instead of doing `after idle [list after 0...`. I
haven't tested it, but AFAIK this should also keep the UI active while
not depending on the state of the event loop.
What benefits does your way have over just passing the entire list
(without batching) to an async script to do processing in the
background?
> } else {
> - unlock_index
> + # Finish the status bar operation.
> + $::main_status stop
> +
> + # Report error, if any, based on how many deletions failed.
> + set deletion_error_cnt [llength $deletion_errors]
> +
> + if {$deletion_error_cnt == 1} {
> + error_popup [mc \
> + "File %s could not be deleted: %s" \
> + $deletion_error_path \
> + [lindex $deletion_errors 0] \
> + ]
> + } elseif {$deletion_error_cnt == $path_cnt} {
> + error_popup [mc \
> + "None of the selected files could be deleted." \
> + ]
> + } elseif {$deletion_error_cnt > 1} {
> + error_popup [mc \
> + "%d of the selected files could not be deleted." \
> + $deletion_error_cnt]
> + }
The same comment as above applies here: either show error messages for
all paths, or for none. I don't see why you want to make a single error
path a special case.
> +
> + reshow_diff
> + ui_ready
> + }
> +}
> +
> +# This function is from the TCL documentation:
> +#
> +# https://wiki.tcl-lang.org/page/file+exists
Why include the link? My guess is "to give proper credit". Do I guess
correctly?
> +#
> +# [file exists] returns false if the path does exist but is a symlink to a path
> +# that doesn't exist. This proc returns true if the path exists, regardless of
> +# whether it is a symlink and whether it is broken.
> +proc lexists name {
Nitpick: wrap the "name" in braces like:
proc lexists {name} {
Also, maybe re-name it to 'path_exists'? 'lexists' is not very intuitive
unless being used _specifically_ in the context of links. Its _use_ is
in context of paths, even though it is used to work around links.
> + expr {![catch {file lstat $name finfo}]}
> +}
> +
> +# Remove as many empty directories as we can starting at the specified path.
Nitpick: maybe change it to something like this?
Remove as many empty directories as we can starting at the specified
path, going up in the directory tree.
It was not obvious to me from reading the comment that you were going up
the directory tree. I thought you were going across the breadth of the
directory, and was puzzled why you'd do that.
But maybe that's just me. So, I don't mind if you keep it the way it is
either.
> +# If we encounter a directory that is not empty, or if a directory deletion
> +# fails, then we stop the operation and return to the caller. Even if this
> +# procedure fails to delete any directories at all, it does not report failure.
> +proc remove_empty_directories {directory_path} {
> + set parent_path [file dirname $directory_path]
> +
> + while {$parent_path != $directory_path} {
> + set contents [glob -nocomplain -dir $directory_path *]
> +
> + if {[llength $contents] > 0} { break }
> + if {[catch {file delete -- $directory_path}]} { break }
> +
> + set directory_path $parent_path
> + set parent_path [file dirname $directory_path]
> }
> }
I did some quick testing on my system, and it works fine. Thanks.
--
Regards,
Pratyush Yadav
There was a problem hiding this comment.
On the Git mailing list, Jonathan Gilbert wrote (reply to this):
On Sun, Nov 3, 2019 at 1:48 AM Pratyush Yadav me-at-yadavpratyush.com
|GitHub Public/Example Allow| <172q77k4bxwj0zt@sneakemail.com> wrote:
> Hi Jonathan,
>
> Thanks for the quality re-roll. It was a pleasant read :)
>
> I would have suggested just handing off the paths to `git clean`, but it
> unfortunately does not do what we want it to do.
>
> Say we have a directory 'foo' which has one file called 'bar.txt'. That
> file is untracked. Now, I expected `git clean -fd foo/bar.txt` to delete
> 'bar.txt' _and_ 'foo/', but it only deletes bar.txt, and leaves 'foo/'
> intact. What's worse is that since 'foo' is an empty directory, it
> doesn't appear in git-status anymore, and so there is no way the user
> can tell the directory exists unless they go there and do a `ls`.
>
> Maybe something to fix upstream?
Possibly, but I think the implications of such a change in the core
tool are far greater than UI features in Git Gui.
> On 30/10/19 06:48AM, Jonathan Gilbert via GitGitGadget wrote:
> > From: Jonathan Gilbert <JonathanG@iQmetrix.com>
> >
> > Updates the revert_helper procedure to also detect untracked files. If
>
> Typo: s/Updates/Update/ ?
It wasn't a typo, I wrote it as an abbreviated form of basically "This
change updates the revert_helper procedure to ...". But, if that
choice of linguistic construct goes against convention I can change
it. :-)
> > + # The index is now locked. Some of the paths below include calls that
> > + # unlock the index (e.g. checked_index). If we reach the end and the
>
> Typo: s/checked_index/checkout_index/
Fixed. :-)
> > + if {$need_unlock_index} { unlock_index }
>
> Are you sure you want to unlock the index _before_ the cleanup of
> untracked files is done? While it makes sense to unlock the index since
> our "clean" operation would only touch the working tree, and not the
> index, it would also mean people can do things like "Revert hunk" (from
> the context menu). Right now, this operation can not be done on
> untracked files (so this won't be a problem for now), but I do plan on
> adding this in the future, and it wouldn't be obvious from that patch's
> POV that this could be an issue. If someone does a "Revert hunk" on a
> while that is queued for deletion, there might be problems.
>
> Also, would doing an `unlock_index` early allow people to run multiple
> "clean" jobs at the same time? Will that create race conditions that we
> aren't ready to handle?
>
> It also makes sense to evaluate what the downsides of keeping the index
> locked are. So, does keeping the index locked prevent meaningful usage
> of git-gui, making your batched deletion pointless? Is there some reason
> for unlocking it early that I'm missing?
>
> If we do decide keeping the index locked is a good idea, it would be
> troublesome to implement. `checkout_index` is asynchronous. So, when it
> returns, the index won't necessarily be unlocked. It would get unlocked
> some time _after_ the return. I'm not sure how to work around this.
Yeah, when I wrote this I was looking at the fact that the locking of
the index, on the surface, only seems to interact with Git working
copy operations, and as you mention the fact that both tails of the
function (`checkout_index` and `delete_files`) operate asynchronously
means that figuring out _when_ to unlock the index is a bit tricky.
But, based on what you've written I understand that locking the index
also disables UI interaction while it's locked, and that may be
desirable, so we probably do want to keep it locked until both
operations are complete.
What we need here is something I have seen referred to as a "chord" --
conceptually, a function with multiple entrypoints that get called
from different threads, and then the body of the function runs only
when all "notes" on the "chord" have been activated. So in this case,
an object that has one entry-point for "the checkout is complete" and
one entry-point for "the deletion is complete". The body of the
function is `unlock_index`, and then the two asynchronous functions
both call into their "note" on the "chord" instead of directly calling
`unlock_index`. This would mean that the `_close_updateindex` call
that `checkout_index` ultimately drills down to would have to, in some
circumstances, _not_ unlock the index itself. I'll take a hack at this
and see what transpires. :-)
> > + if {$deletion_failed} {
> > + lappend deletion_errors $deletion_error
> > +
> > + # Optimistically capture the path that failed, in case
> > + # there's only one.
> > + set deletion_error_path $path
>
> I don't see why you would do this for _only_ one path. Either do it for
> every path. And since you're recording errors for each path, it makes
> sense to record the corresponding path too. Or, just count how many
> paths failed, and report that. I don't see why we'd want to be between
> those two.
[..]
> > + } elseif {$deletion_error_cnt == $path_cnt} {
> > + error_popup [mc \
> > + "None of the selected files could be deleted." \
> > + ]
> > + } elseif {$deletion_error_cnt > 1} {
> > + error_popup [mc \
> > + "%d of the selected files could not be deleted." \
> > + $deletion_error_cnt]
> > + }
>
> The same comment as above applies here: either show error messages for
> all paths, or for none. I don't see why you want to make a single error
> path a special case.
Consistency -- the prompt that asks whether you want to do a revert
(checkout) or deletion (clean) in the first place has the same split,
where if only one file matches, it identifies the file, but if
multiple files match, it shows the number. For consistency with that,
I used the same logic in the error handling path.
> > - [concat $after [list ui_ready]]
> > + $path_index \
> > + $deletion_errors \
> > + $deletion_error_path \
> > + $batch_size \
> > + ]]
>
> Using `after idle` means in theory we put an undefined maximum time
> limit on the deletion process. Though I suspect in real life it would be
> a pretty short time.
>
> Nonetheless, should you instead do this asynchronously, instead of
> waiting for the event loop to enter an idle state? This means using
> `after 0` directly, instead of doing `after idle [list after 0...`. I
> haven't tested it, but AFAIK this should also keep the UI active while
> not depending on the state of the event loop.
>
> What benefits does your way have over just passing the entire list
> (without batching) to an async script to do processing in the
> background?
I'm not familiar with async scripts, I'm pretty new to Tcl. Is that
basically a mechanism like threads? I wrote the batching simply
because doing the call synchronously meant that if thousands of files
were selected, the UI would freeze hard for several seconds and that
seemed like a bad experience. If there's a better way to delete
thousands of files while keeping the UI responsive and providing
feedback, that'd make more sense, but I don't know how to do it :-)
> > +# This function is from the TCL documentation:
> > +#
> > +# https://wiki.tcl-lang.org/page/file+exists
>
> Why include the link? My guess is "to give proper credit". Do I guess
> correctly?
Actually it's more to say, "If you're reading through this code and
the specific nuances of this procedure aren't obvious, here's the
procedure's origin. I believe it to be a reliable source, so if that's
good enough for you too, then you don't need to concern yourself with
the implementation details, you can just trust that somebody else put
time into the definition of this above and beyond the scope of the
change where I'm using it." :-)
> > +# [file exists] returns false if the path does exist but is a symlink to a path
> > +# that doesn't exist. This proc returns true if the path exists, regardless of
> > +# whether it is a symlink and whether it is broken.
> > +proc lexists name {
>
> Nitpick: wrap the "name" in braces like:
>
> proc lexists {name} {
>
> Also, maybe re-name it to 'path_exists'? 'lexists' is not very intuitive
> unless being used _specifically_ in the context of links. Its _use_ is
> in context of paths, even though it is used to work around links.
I can make those changes. I had initially copy/pasted it with no
changes at all, so that, in the context of the preceding explanation,
a future reader could easily verify that, "Yes, this really is exactly
the same procedure definition." :-)
> > +# Remove as many empty directories as we can starting at the specified path.
>
> Nitpick: maybe change it to something like this?
>
> Remove as many empty directories as we can starting at the specified
> path, going up in the directory tree.
>
> It was not obvious to me from reading the comment that you were going up
> the directory tree. I thought you were going across the breadth of the
> directory, and was puzzled why you'd do that.
>
> But maybe that's just me. So, I don't mind if you keep it the way it is
> either.
That's legitimate :-) I knew exactly what the function did _before_ I
wrote the comment, after all.
Let me know about those few things, and I'll send in another iteration:
* Is it preferable to use imperative rather than third person singular
in commit messages? ("[I] make the change" vs. "[It] makes the
change")
* Should I simplify the error messages, rather than having parity with
the prompts w.r.t. one vs. multiple items?
* Async scripts for longer background operations, rather than batching
on the UI thread? Can they post ongoing status updates too?
* Should I modify `lexists` to fit the file's conventions, or keep it
an exact copy/paste from the external source?
Thanks very much,
Jonathan Gilbert
There was a problem hiding this comment.
On the Git mailing list, Jonathan Gilbert wrote (reply to this):
On Sun, Nov 3, 2019 at 1:48 AM Pratyush Yadav me-at-yadavpratyush.com
|GitHub Public/Example Allow| <172q77k4bxwj0zt@sneakemail.com> wrote:
> > + after idle [list after 0 [list \
> > + delete_helper \
> > $path_list \
> > - [concat $after [list ui_ready]]
> > + $path_index \
> > + $deletion_errors \
> > + $deletion_error_path \
> > + $batch_size \
> > + ]]
>
> Using `after idle` means in theory we put an undefined maximum time
> limit on the deletion process. Though I suspect in real life it would be
> a pretty short time.
>
> Nonetheless, should you instead do this asynchronously, instead of
> waiting for the event loop to enter an idle state? This means using
> `after 0` directly, instead of doing `after idle [list after 0...`. I
> haven't tested it, but AFAIK this should also keep the UI active while
> not depending on the state of the event loop.
>
> What benefits does your way have over just passing the entire list
> (without batching) to an async script to do processing in the
> background?
I forgot to include this in my point-form list at the end of the
preceding e-mail. What should I be looking into to achieve the same
sort of behaviour, where the UI isn't frozen and the user is getting
period updates about the progress of a large deletion, without using
batches on the UI thread? Is that a thing, or am I misunderstanding
you w.r.t. to doing this asynchronously?
For what it's worth, I used `after idle {after 0 ..}` based on the
recommendation of the Tcler's Wiki [0]:
> An after idle that reschedules itself causes trouble, as the manual warns (PYK 2012-09: the docs no-longer contain this warning, but it still applies):
>
> At present it is not safe for an idle callback to reschedule itself
> continuously. This will interact badly with certain features of
> Tk that attempt to wait for all idle callbacks to complete.
> If you would like for an idle callback to reschedule itself
> continuously, it is better to use a timer handler with a zero
> timeout period.
>
> Even this warning is oversimplified. Simply scheduling a timer handler with a zero timeout period can mean that the event loop will never be idle, keeping other idle callbacks from firing. The truly safe approach combines both:
>
> proc doOneStep {} {
> if { [::sim::one_step] } {
> after idle [list after 0 doOneStep]
> #this would work just as well:
> #after 0 [list after idle doOneStep]
> }
> return
> }
> sim::init .c 640 480
> doOneStep
>
> This skeleton should be considered the basic framework for performing long running calculations within a single Tcl interpreter.
Thanks,
Jonathan Gilbert
[0] https://wiki.tcl-lang.org/page/Keep+a+GUI+alive+during+a+long+calculation
|
On the Git mailing list, Pratyush Yadav wrote (reply to this): |
logiclrd
force-pushed
the
git-gui-revert-untracked
branch
from
0190f6f
to
9469beb
Compare
|
/submit |
|
Submitted as pull.436.v2.git.1573110335.gitgitgadget@gmail.com |
lib/chord.tcl
Outdated
| @@ -0,0 +1,137 @@ | |||
| # SimpleChord class: | |||
There was a problem hiding this comment.
On the Git mailing list, Pratyush Yadav wrote (reply to this):
Hi Jonathan,
Thanks for the re-roll. Some comments below. Apart from those comments,
this looks close to good enough for merging :)
On 07/11/19 07:05AM, Jonathan Gilbert via GitGitGadget wrote:
> From: Jonathan Gilbert <JonathanG@iQmetrix.com>
>
> Update the revert_helper procedure to also detect untracked files. If
> files are present, the user is asked if they want them deleted. Perform
> the deletion in batches, using new proc delete_files with helper
> delete_helper, to allow the UI to remain responsive. Coordinate the
> completion of multiple overlapping asynchronous operations using a new
> construct called a "chord". Migrate unlocking of the index out of
> _close_updateindex to a responsibility of the caller, to permit paths
> that don't directly unlock the index.
>
> Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
> ---
> lib/chord.tcl | 137 ++++++++++++++++++++++++
> lib/index.tcl | 288 +++++++++++++++++++++++++++++++++++++++++++-------
> 2 files changed, 385 insertions(+), 40 deletions(-)
> create mode 100644 lib/chord.tcl
>
> diff --git a/lib/chord.tcl b/lib/chord.tcl
> new file mode 100644
> index 0000000000..2d13af14fc
> --- /dev/null
> +++ b/lib/chord.tcl
> @@ -0,0 +1,137 @@
The 'class' documentation [0] suggests adding a "package require TclOO".
But TclOO ships by default with Tcl 8.6 and above. So, I'm not really
sure if we need this.
> +# SimpleChord class:
> +# Represents a procedure that conceptually has multiple entrypoints that must
> +# all be called before the procedure executes. Each entrypoint is called a
> +# "note". The chord is only "completed" when all the notes are "activated".
> +#
> +# Constructor:
> +# set chord [SimpleChord new {body}]
> +# Creates a new chord object with the specified body script. The body
> +# script is evaluated at most once, when a note is activated and the
> +# chord has no other non-activated notes.
> +#
> +# Methods:
> +# $chord eval {script}
> +# Runs the specified script in the same context (namespace) in which the
> +# chord body will be evaluated. This can be used to set variable values
> +# for the chord body to use.
> +#
> +# set note [$chord add_note]
> +# Adds a new note to the chord, an instance of ChordNote. Raises an
> +# error if the chord is already completed, otherwise the chord is updated
> +# so that the new note must also be activated before the body is
> +# evaluated.
> +#
> +# $chord notify_note_activation
> +# For internal use only.
> +#
> +# ChordNote class:
> +# Represents a note within a chord, providing a way to activate it. When the
> +# final note of the chord is activated (this can be any note in the chord,
> +# with all other notes already previously activated in any order), the chord's
> +# body is evaluated.
> +#
> +# Constructor:
> +# Instances of ChordNote are created internally by calling add_note on
> +# SimpleChord objects.
> +#
> +# Methods:
> +# [$note is_activated]
> +# Returns true if this note has already been activated.
> +#
> +# $note
> +# Activates the note, if it has not already been activated, and completes
> +# the chord if there are no other notes awaiting activation. Subsequent
> +# calls will have no further effect.
Nice to see some good documentation!
One nitpick: would it make more sense to have the documentation for a
method/constructor just above that method/constructor? This way, when
someone updates the code some time later, they'll also hopefully
remember to update the documentation. It is much more likely to be stale
if all of it just stays on the top.
> +#
> +# Example:
> +#
> +# # Turn off the UI while running a couple of async operations.
> +# lock_ui
> +#
> +# set chord [SimpleChord new {
> +# unlock_ui
> +# # Note: $notice here is not referenced in the calling scope
> +# if {$notice} { info_popup $notice }
> +# }
> +#
> +# # Configure a note to keep the chord from completing until
> +# # all operations have been initiated.
> +# set common_note [$chord add_note]
> +#
> +# # Pass notes as 'after' callbacks to other operations
> +# async_operation $args [$chord add_note]
> +# other_async_operation $args [$chord add_note]
> +#
> +# # Communicate with the chord body
> +# if {$condition} {
> +# # This sets $notice in the same context that the chord body runs in.
> +# $chord eval { set notice "Something interesting" }
> +# }
> +#
> +# # Activate the common note, making the chord eligible to complete
> +# $common_note
> +#
> +# At this point, the chord will complete at some unknown point in the future.
> +# The common note might have been the first note activated, or the async
> +# operations might have completed synchronously and the common note is the
> +# last one, completing the chord before this code finishes, or anything in
> +# between. The purpose of the chord is to not have to worry about the order.
> +
> +oo::class create SimpleChord {
This comes from the TclOO package, right?
git-gui has its own object-oriented system (lib/class.tcl). It was
written circa 2007. I suspect something like TclOO did not exist back
then.
Why not use that? Does it have some limitations that TclOO does not
have? I do not mind using the "official" OO system. I just want to know
why exactly you made the choice.
We would end up mixing the two implementations/flavors in the same
codebase, but as long as they don't interfere with each other and are
cross compatible (which I think they are, but I haven't tested), I don't
mind some "modernization" of the codebase.
More importantly, TclOO ships as part of the core distribution with Tcl
8.6, but as of now the minimum version required for git-gui is 8.4. So,
I think we should bump the minimum version (8.6 released circa 2012, so
most people should have caught up by now I hope).
> + variable Notes
> + variable Body
> + variable IsCompleted
Nitpick: Please use snake_case, here and in other places.
> +
> + constructor {body} {
> + set Notes [list]
> + set Body $body
> + set IsCompleted 0
> + }
> +
> + method eval {script} {
> + namespace eval [self] $script
> + }
> +
> + method add_note {} {
> + if {$IsCompleted} { error "Cannot add a note to a completed chord" }
> +
> + set note [ChordNote new [self]]
> +
> + lappend Notes $note
> +
> + return $note
> + }
> +
> + method notify_note_activation {} {
Since this method is for internal use only, can it be made "private"?
Does the OO library support something like this?
> + if {!$IsCompleted} {
> + foreach note $Notes {
> + if {![$note is_activated]} { return }
> + }
> +
> + set IsCompleted 1
> +
> + namespace eval [self] $Body
> + namespace delete [self]
> + }
> + }
> +}
> +
> +oo::class create ChordNote {
> + variable Chord IsActivated
> +
> + constructor {chord} {
> + set Chord $chord
> + set IsActivated 0
> + }
> +
> + method is_activated {} {
> + return $IsActivated
> + }
> +
> + method unknown {} {
I'm a bit lost here. This method is named 'unknown', but searching for
'unknown' in this patch just gives me two results: this line here, and
then one in a comment at the start of the file.
From what I understand looking at the code, it some sort of a "default"
method, and is called when you run just `$chord_note`. How exactly is
this method designated to be the default?
Also, "unknown" makes little sense in this context. Can you rename it to
something more meaningful? Maybe something like "activate_note"?
> + if {!$IsActivated} {
> + set IsActivated 1
> + $Chord notify_note_activation
> + }
> + }
> +}
From what I understand, the "Note" object is effectively used as a
count. There is no other state associated with it. When I first heard of
your description of this abstraction, I assumed that a Note would also
store a script to execute with it. So, when you "activate" a note, it
would first execute the script, and then mark itself as "activated", and
notify the chord. Would that abstraction make more sense?
I don't really mind keeping it this way, but I wonder if that design
would make the abstraction easier to wrap your head around.
> diff --git a/lib/index.tcl b/lib/index.tcl
> index 28d4d2a54e..64046d6833 100644
> --- a/lib/index.tcl
> +++ b/lib/index.tcl
> @@ -7,7 +7,7 @@ proc _delete_indexlock {} {
> }
> }
>
> -proc _close_updateindex {fd after} {
> +proc _close_updateindex {fd} {
> global use_ttk NS
> fconfigure $fd -blocking 1
> if {[catch {close $fd} err]} {
> @@ -52,8 +52,6 @@ proc _close_updateindex {fd after} {
> }
>
> $::main_status stop
> - unlock_index
> - uplevel #0 $after
There is a call to unlock_index in the body of the if statement above
too. Do we want to remove that too, or should it be left alone?
But immediately after the unlocking of the index there, a call to
`rescan` is made. `rescan` acquired the lock, so it would fail if we do
not unlock the index there. Note that `rescan` itself is asynchronous.
Since every call to `_close_updateindex` is followed by an index unlock,
it would mean the index would be unlocked for the rescan while it is in
progress (for all calls other than the one from `write_checkout_index`).
What a mess!
That codepath seems to be taken when a major error happens, and we just
resign to our fate and get a fresh start by doing a rescan and syncing
the repo state. So it is quite likely whatever operation we were doing
failed spectacularly.
Maybe the answer is to swallow the bitter pill and introduce a
switch/boolean in `_close_updateindex` that controls whether the index
is unlocked or not. We unlock it when the if statement is not taken, and
keep the current codepath when it is. I call it a "bitter pill" because
I'm usually not a huge fan of adding knobs like that in functions. Makes
the function harder to reason about and makes it more bug prone.
If you can think of a better/cleaner way of working around this,
suggestions are welcome!
> }
>
> proc update_indexinfo {msg path_list after} {
> @@ -90,7 +88,9 @@ proc write_update_indexinfo {fd path_list total_cnt batch after} {
> global file_states current_diff_path
>
> if {$update_index_cp >= $total_cnt} {
> - _close_updateindex $fd $after
> + _close_updateindex $fd
> + unlock_index
> + uplevel #0 $after
> return
> }
>
> @@ -156,7 +156,9 @@ proc write_update_index {fd path_list total_cnt batch after} {
> global file_states current_diff_path
>
> if {$update_index_cp >= $total_cnt} {
> - _close_updateindex $fd $after
> + _close_updateindex $fd
> + unlock_index
> + uplevel #0 $after
> return
> }
>
> @@ -233,7 +235,8 @@ proc write_checkout_index {fd path_list total_cnt batch after} {
> global file_states current_diff_path
>
> if {$update_index_cp >= $total_cnt} {
> - _close_updateindex $fd $after
> + _close_updateindex $fd $do_unlock_index $after
> + uplevel #0 $after
_close_updateindex takes only one argument, and you pass it 3.
$do_unlock_index does not seem to be defined anywhere. $after is
evaluated just after this line, and _close_updateindex doesn't accept
the argument anyway. I suspect this is a leftover from a different
approach you tried before this one.
Also, unlike all the other places where _close_updateindex is used, this
one does not make a call to unlock_index. Is that intended? IIUC, it
should be intended, since this is the part which uses the "chord", but a
confirmation would be nice.
> return
> }
>
> @@ -393,61 +396,266 @@ proc revert_helper {txt paths} {
>
> if {![lock_index begin-update]} return
>
> + # Common "after" functionality that waits until multiple asynchronous
> + # operations are complete (by waiting for them to activate their notes
> + # on the chord).
Nitpick: mention what the "multiple asynchronous operations" are exactly
(i.e, they are the deletion and index checkout operations).
> + set after_chord [SimpleChord new {
> + unlock_index
> + if {$should_reshow_diff} { reshow_diff }
> + ui_ready
> + }]
> +
> + $after_chord eval { set should_reshow_diff 0 }
> +
> + # We don't know how many notes we're going to create (it's dynamic based
> + # on conditional paths below), so create a common note that will delay
> + # the chord's completion until we activate it, and then activate it
> + # after all the other notes have been created.
> + set after_common_note [$after_chord add_note]
> +
> set path_list [list]
> + set untracked_list [list]
> set after {}
'after' seems to be an unused variable. This line can be deleted.
> foreach path $paths {
> switch -glob -- [lindex $file_states($path) 0] {
> U? {continue}
> + ?O {
> + lappend untracked_list $path
> + }
> ?M -
> ?T -
> ?D {
> lappend path_list $path
> if {$path eq $current_diff_path} {
> - set after {reshow_diff;}
> + $after_chord eval { set should_reshow_diff 1 }
> }
> }
> }
> }
>
> + set path_cnt [llength $path_list]
> + set untracked_cnt [llength $untracked_list]
>
> - # Split question between singular and plural cases, because
> - # such distinction is needed in some languages. Previously, the
> - # code used "Revert changes in" for both, but that can't work
> - # in languages where 'in' must be combined with word from
> - # rest of string (in different way for both cases of course).
> - #
> - # FIXME: Unfortunately, even that isn't enough in some languages
> - # as they have quite complex plural-form rules. Unfortunately,
> - # msgcat doesn't seem to support that kind of string translation.
> - #
> - set n [llength $path_list]
> - if {$n == 0} {
> - unlock_index
> - return
> - } elseif {$n == 1} {
> - set query [mc "Revert changes in file %s?" [short_path [lindex $path_list]]]
> - } else {
> - set query [mc "Revert changes in these %i files?" $n]
> - }
> + if {$path_cnt > 0} {
> + # Split question between singular and plural cases, because
> + # such distinction is needed in some languages. Previously, the
> + # code used "Revert changes in" for both, but that can't work
> + # in languages where 'in' must be combined with word from
> + # rest of string (in different way for both cases of course).
> + #
> + # FIXME: Unfortunately, even that isn't enough in some languages
> + # as they have quite complex plural-form rules. Unfortunately,
> + # msgcat doesn't seem to support that kind of string
> + # translation.
> + #
> + if {$path_cnt == 1} {
> + set query [mc \
> + "Revert changes in file %s?" \
> + [short_path [lindex $path_list]] \
> + ]
> + } else {
> + set query [mc \
> + "Revert changes in these %i files?" \
> + $path_cnt]
> + }
>
> - set reply [tk_dialog \
> - .confirm_revert \
> - "[appname] ([reponame])" \
> - "$query
> + set reply [tk_dialog \
> + .confirm_revert \
> + "[appname] ([reponame])" \
> + "$query
>
> [mc "Any unstaged changes will be permanently lost by the revert."]" \
> - question \
> - 1 \
> - [mc "Do Nothing"] \
> - [mc "Revert Changes"] \
> - ]
> - if {$reply == 1} {
> - checkout_index \
> - $txt \
> + question \
> + 1 \
> + [mc "Do Nothing"] \
> + [mc "Revert Changes"] \
> + ]
> +
> + if {$reply == 1} {
> + checkout_index \
> + $txt \
> + $path_list \
> + [$after_chord add_note]
> + }
> + }
> +
> + if {$untracked_cnt > 0} {
> + # Split question between singular and plural cases, because
> + # such distinction is needed in some languages.
> + #
> + # FIXME: Unfortunately, even that isn't enough in some languages
> + # as they have quite complex plural-form rules. Unfortunately,
> + # msgcat doesn't seem to support that kind of string
> + # translation.
> + #
> + if {$untracked_cnt == 1} {
> + set query [mc \
> + "Delete untracked file %s?" \
> + [short_path [lindex $untracked_list]] \
> + ]
> + } else {
> + set query [mc \
> + "Delete these %i untracked files?" \
> + $untracked_cnt \
> + ]
> + }
> +
> + set reply [tk_dialog \
> + .confirm_revert \
> + "[appname] ([reponame])" \
> + "$query
> +
> +[mc "Files will be permanently deleted."]" \
> + question \
> + 1 \
> + [mc "Do Nothing"] \
> + [mc "Delete Files"] \
> + ]
> +
> + if {$reply == 1} {
> + $after_chord eval { set should_reshow_diff 1 }
> +
> + delete_files $untracked_list [$after_chord add_note]
> + }
> + }
> +
> + # Activate the common note. If no other notes were created, this
> + # completes the chord. If other notes were created, then this common
> + # note prevents a race condition where the chord might complete early.
> + $after_common_note
> +}
> +
> +# Delete all of the specified files, performing deletion in batches to allow the
> +# UI to remain responsive and updated.
> +proc delete_files {path_list after} {
> + # Enable progress bar status updates
> + $::main_status start [mc "Deleting"] [mc "files"]
> +
> + set path_index 0
> + set deletion_errors [list]
> + set batch_size 50
> +
> + delete_helper \
> + $path_list \
> + $path_index \
> + $deletion_errors \
> + $batch_size \
> + $after
> +}
> +
> +# Helper function to delete a list of files in batches. Each call deletes one
> +# batch of files, and then schedules a call for the next batch after any UI
> +# messages have been processed.
> +proc delete_helper {path_list path_index deletion_errors batch_size after} {
> + global file_states
> +
> + set path_cnt [llength $path_list]
> +
> + set batch_remaining $batch_size
> +
> + while {$batch_remaining > 0} {
> + if {$path_index >= $path_cnt} { break }
> +
> + set path [lindex $path_list $path_index]
> +
> + set deletion_failed [catch {file delete -- $path} deletion_error]
> +
> + if {$deletion_failed} {
> + lappend deletion_errors [list "$deletion_error"]
> + } else {
> + remove_empty_directories [file dirname $path]
> +
> + # Don't assume the deletion worked. Remove the file from
> + # the UI, but only if it no longer exists.
> + if {![path_exists $path]} {
> + unset file_states($path)
> + display_file $path __
> + }
> + }
> +
> + incr path_index 1
> + incr batch_remaining -1
> + }
> +
> + # Update the progress bar to indicate that this batch has been
> + # completed. The update will be visible when this procedure returns
> + # and allows the UI thread to process messages.
> + $::main_status update $path_index $path_cnt
> +
> + if {$path_index < $path_cnt} {
> + # The Tcler's Wiki lists this as the best practice for keeping
> + # a UI active and processing messages during a long-running
> + # operation.
> +
> + after idle [list after 0 [list \
> + delete_helper \
> $path_list \
> - [concat $after [list ui_ready]]
> + $path_index \
> + $deletion_errors \
> + $batch_size \
> + $after
> + ]]
> } else {
> - unlock_index
> + # Finish the status bar operation.
> + $::main_status stop
> +
> + # Report error, if any, based on how many deletions failed.
> + set deletion_error_cnt [llength $deletion_errors]
> +
> + if {($deletion_error_cnt > 0) && ($deletion_error_cnt <= [MAX_VERBOSE_FILES_IN_DELETION_ERROR])} {
Nitpick: please split the line into two.
> + set error_text "Encountered errors deleting files:\n"
Wrap the string in a `mc [...]` so it can be translated some time in the
future.
> +
> + foreach deletion_error $deletion_errors {
> + append error_text "* [lindex $deletion_error 0]\n"
> + }
> +
> + error_popup $error_text
> + } elseif {$deletion_error_cnt == $path_cnt} {
> + error_popup [mc \
> + "None of the %d selected files could be deleted." \
> + $path_cnt \
> + ]
> + } elseif {$deletion_error_cnt > 1} {
> + error_popup [mc \
> + "%d of the %d selected files could not be deleted." \
> + $deletion_error_cnt \
> + $path_cnt \
> + ]
Nice! In case someone in the future wants to have a config variable to
change this limit, this makes it pretty easy to do so.
> + }
> +
> + uplevel #0 $after
> + }
> +}
> +
> +proc MAX_VERBOSE_FILES_IN_DELETION_ERROR {} { return 10; }
Why use a procedure, and not a global variable? My guess is to make it
impossible for some code to change this value by mistake. Do I guess
correctly?
> +
> +# This function is from the TCL documentation:
> +#
> +# https://wiki.tcl-lang.org/page/file+exists
> +#
> +# [file exists] returns false if the path does exist but is a symlink to a path
> +# that doesn't exist. This proc returns true if the path exists, regardless of
> +# whether it is a symlink and whether it is broken.
> +proc path_exists {name} {
> + expr {![catch {file lstat $name finfo}]}
> +}
> +
> +# Remove as many empty directories as we can starting at the specified path,
> +# walking up the directory tree. If we encounter a directory that is not
> +# empty, or if a directory deletion fails, then we stop the operation and
> +# return to the caller. Even if this procedure fails to delete any
> +# directories at all, it does not report failure.
> +proc remove_empty_directories {directory_path} {
> + set parent_path [file dirname $directory_path]
> +
> + while {$parent_path != $directory_path} {
> + set contents [glob -nocomplain -dir $directory_path *]
> +
> + if {[llength $contents] > 0} { break }
> + if {[catch {file delete -- $directory_path}]} { break }
> +
> + set directory_path $parent_path
> + set parent_path [file dirname $directory_path]
> }
> }
Wew! This took longer than I expected ;)
Tested on Linux. Works fine after fixing the extra arguments passed to
`_close_updateindex`. Thanks.
[0] https://www.tcl.tk/man/tcl8.6/TclCmd/class.htm
--
Regards,
Pratyush Yadav
There was a problem hiding this comment.
On the Git mailing list, Jonathan Gilbert wrote (reply to this):
On Mon, Nov 11, 2019 at 1:25 PM Pratyush Yadav me-at-yadavpratyush.com
|GitHub Public/Example Allow| <172q77k4bxwj0zt@sneakemail.com> wrote:
> On 07/11/19 07:05AM, Jonathan Gilbert via GitGitGadget wrote:
> > --- /dev/null
> > +++ b/lib/chord.tcl
> > @@ -0,0 +1,137 @@
>
> The 'class' documentation [0] suggests adding a "package require TclOO".
> But TclOO ships by default with Tcl 8.6 and above. So, I'm not really
> sure if we need this.
I'm not super familiar with it. I just checked what Tcl version I was
myself running, since it's only there because of the Git Gui
installation bundled with Git for Windows, and it was 8.6, so I
assumed it was fair game to use. It didn't occur to me that you could
already have an older version of Tcl installed and have Git Gui use
it. :-) So, if I'm understanding correctly, `TclOO` as a package could
potentially be used to allow TclOO to be used with 8.4, the minimum
supported version you mention below, and it just happened to work for
me in my testing without that because I have 8.6 installed but that's
technically newer than the supported baseline?
> Nice to see some good documentation!
>
> One nitpick: would it make more sense to have the documentation for a
> method/constructor just above that method/constructor? This way, when
> someone updates the code some time later, they'll also hopefully
> remember to update the documentation. It is much more likely to be stale
> if all of it just stays on the top.
Hmm, what do you think of both? I was thinking of the documentation as
a single self-contained block that someone could read to put together
an understanding of how the chord system fits together, and split out,
it wouldn't have that readability. What about a more abstract
description in a block at the top, and then more technically-detailed
& specific descriptions attached to each method?
> > +oo::class create SimpleChord {
>
> This comes from the TclOO package, right?
>
> git-gui has its own object-oriented system (lib/class.tcl). It was
> written circa 2007. I suspect something like TclOO did not exist back
> then.
>
> Why not use that? Does it have some limitations that TclOO does not
> have? I do not mind using the "official" OO system. I just want to know
> why exactly you made the choice.
Having limited experience with Tcl, I did a Google search for "tcl
object oriented" and ended up writing code using TclOO because that's
what came up. Do you think it makes sense to rework this to use
`class.tcl`, or perhaps instead the opposite: have a policy of using
the standard TclOO going forward, and let the rest of Git Gui
organically upgrade itself to some hypothetical point in the future
where class.tcl is no longer used by anything?
> More importantly, TclOO ships as part of the core distribution with Tcl
> 8.6, but as of now the minimum version required for git-gui is 8.4. So,
> I think we should bump the minimum version (8.6 released circa 2012, so
> most people should have caught up by now I hope).
If I understand correctly, you mentioned that TclOO was intrinsically
available to me because I was using Tcl 8.6, and that the manual
recommends `package require TclOO` -- does that package dependency
permit the use of TclOO on 8.4? If so, could that be a way to avoid
bumping the minimum version required? Simply in the interest of
keeping the scope of the change limited. If not, then bumping the
minimum required version to 8.6 from 2012 doesn't seem entirely
unreasonable either. :-)
> > + variable Notes
> > + variable Body
> > + variable IsCompleted
>
> Nitpick: Please use snake_case, here and in other places.
Okay, yep -- I had copied the convention that I saw in TclOO examples,
conscious of the fact that there might be a standard specific to
object-oriented Tcl.
> > + method notify_note_activation {} {
>
> Since this method is for internal use only, can it be made "private"?
> Does the OO library support something like this?
I don't think so, because it's called from outside the class. What
we'd be looking for is something like C++'s "friend" syntax. Tcl
doesn't seem to have this. Though, I just did some further Googling,
and saw a hint that it might be possible to bypass member security on
a case-by-case basis, so that the method is private but `ChordNote` is
able to call it anyway. I'll see if I can't figure this out. :-)
> > + method unknown {} {
>
> I'm a bit lost here. This method is named 'unknown', but searching for
> 'unknown' in this patch just gives me two results: this line here, and
> then one in a comment at the start of the file.
>
> From what I understand looking at the code, it some sort of a "default"
> method, and is called when you run just `$chord_note`. How exactly is
> this method designated to be the default?
>
> Also, "unknown" makes little sense in this context. Can you rename it to
> something more meaningful? Maybe something like "activate_note"?
I think it's the fact that it is named `unknown` that makes it the
"default" method. I think this just needs documentary comments next to
it. The TclOO documentation says:
> obj unknown ?methodName? ?arg ...?
> This method is called when an attempt to invoke the method methodName on
> object obj fails. The arguments that the user supplied to the method are
> given as arg arguments. If methodName is absent, the object was invoked with
> no method name at all (or any other arguments).
It was based on that last sentence that I interpreted `unknown` as,
"This is a mechanism for making an object that can be called like a
method."
> > + if {!$IsActivated} {
> > + set IsActivated 1
> > + $Chord notify_note_activation
> > + }
> > + }
> > +}
>
> From what I understand, the "Note" object is effectively used as a
> count. There is no other state associated with it. When I first heard of
> your description of this abstraction, I assumed that a Note would also
> store a script to execute with it. So, when you "activate" a note, it
> would first execute the script, and then mark itself as "activated", and
> notify the chord. Would that abstraction make more sense?
>
> I don't really mind keeping it this way, but I wonder if that design
> would make the abstraction easier to wrap your head around.
I learned about the concept of chords and notes from an experimental
language that Microsoft created many years back called "Polyphonic C#"
(which in turn got rolled into "Cw" (C-omega)), and in that
abstraction, the idea was that, well, as a baseline, for starters, we
have methods and each one, conceptually, has an entrypoint with a
certain set of parameters, and when you call that entrypoint, the
parameters are all set and the body runs. With a "chord", you have
more than one entrypoint attached to the same body -- the entrypoints
themselves don't have any logic associated with them individually.
Each note has its own parameter list, and when all the notes have been
called, the body is run with _all_ of those parameters.
I drew some ASCII art, don't know if it'll translate in the message,
but here goes :-)
Basic method (or, if you will, a "chord" with only one "note"):
(caller)
|
void Add(int X, int Y)
|
{ output(X + Y) }
A "chord" with two "notes":
(caller) (caller)
| |
void AddX(int X) void AddY(int Y)
| |
`-----------.-----------'
|
{ output(X + Y) }
The specific details differ from what I've written here. In Polyphonic
C#, you don't have to instantiate a chord, you simply start calling
methods, and the runtime matches up complete sets dynamically. (Just
thinking through the implications of this, if the notes aren't all
called at exactly the same rate this obviously leads very easily to
bugs that chew up all memory on incomplete chords. :-P) Also,
Microsoft's language has parameters to each of the notes that are
_all_ passed to the body at once. My implementation here is a "simple"
chord, I didn't bother with arguments, as they aren't needed in this
usage :-) I also found it much simpler to think of implementing the
chord with the activations being explicit instead of implicit. So
instead of saying up front, "Here is my method body and here are its 3
entrypoints", with this implementation the chord is a dynamic object,
you say "Here is my method body" and get back a thing that you can
start tacking entrypoints onto.
But, a "note" in a SimpleChord isn't a counter, it's a latch. The
chord itself is acting sort of like a counter, in that all the notes
need to be activated, but because the notes are latches, activating a
note repeatedly has the same effect as activating it once. There's no
way for one note to interfere with other notes, which wouldn't be the
case if it literally were just a counter.
It seems to me that a chord where each note has a script of its own is
actually basically just a class with methods, I guess with a common
joined epilogue?:
(caller) (caller)
| |
void AddX(int X) void AddY(int Y)
| |
{ script for AddX } {script for AddY }
| |
`-----------.-----------'
|
{ common tail?? }
The whole point is that the notes are conceptually different "headers"
into _the same_ body. When you call a note of a chord, it is because
you want the _chord_'s script to run, and the chord is acting as a
construct that says "okay, yes, I'll satisfy your request that I
execute, but you'll have to wait, because I'm going to satisfy _all_
your requests in one go".
> > $::main_status stop
> > - unlock_index
> > - uplevel #0 $after
>
> There is a call to unlock_index in the body of the if statement above
> too. Do we want to remove that too, or should it be left alone?
>
> That codepath seems to be taken when a major error happens, and we just
> resign to our fate and get a fresh start by doing a rescan and syncing
> the repo state. So it is quite likely whatever operation we were doing
> failed spectacularly.
>
> Maybe the answer is to swallow the bitter pill and introduce a
> switch/boolean in `_close_updateindex` that controls whether the index
> is unlocked or not. We unlock it when the if statement is not taken, and
> keep the current codepath when it is. I call it a "bitter pill" because
> I'm usually not a huge fan of adding knobs like that in functions. Makes
> the function harder to reason about and makes it more bug prone.
>
> If you can think of a better/cleaner way of working around this,
> suggestions are welcome!
Hmm, so, yeah, the entire if statement only occurs if it can't close
the file descriptor. Is that something that actually happens? If so,
then it should perhaps be throwing an exception, because having
started a rescan is probably more than the caller bargained for. That
would prevent the callers from unlocking the index out from under the
rescan, and also cancel any other processing they might be doing that
is probably making bad assumptions with a rescan running.
> > if {$update_index_cp >= $total_cnt} {
> > - _close_updateindex $fd $after
> > + _close_updateindex $fd $do_unlock_index $after
>
> _close_updateindex takes only one argument, and you pass it 3.
> $do_unlock_index does not seem to be defined anywhere. $after is
> evaluated just after this line, and _close_updateindex doesn't accept
> the argument anyway. I suspect this is a leftover from a different
> approach you tried before this one.
It is indeed, oops!
> Also, unlike all the other places where _close_updateindex is used, this
> one does not make a call to unlock_index. Is that intended? IIUC, it
> should be intended, since this is the part which uses the "chord", but a
> confirmation would be nice.
Intentional, yes. I'll see if there's a concise way to document this.
> > + # Common "after" functionality that waits until multiple asynchronous
> > + # operations are complete (by waiting for them to activate their notes
> > + # on the chord).
>
> Nitpick: mention what the "multiple asynchronous operations" are exactly
> (i.e, they are the deletion and index checkout operations).
Okeydoke.
> > set after {}
>
> 'after' seems to be an unused variable. This line can be deleted.
Good catch.
> > + if {($deletion_error_cnt > 0) && ($deletion_error_cnt <= [MAX_VERBOSE_FILES_IN_DELETION_ERROR])} {
>
> Nitpick: please split the line into two.
Will do.
> > + set error_text "Encountered errors deleting files:\n"
>
> Wrap the string in a `mc [...]` so it can be translated some time in the
> future.
Ah, yes, I did that with most messages, this was an oversight.
> > +proc MAX_VERBOSE_FILES_IN_DELETION_ERROR {} { return 10; }
>
> Why use a procedure, and not a global variable? My guess is to make it
> impossible for some code to change this value by mistake. Do I guess
> correctly?
A variable is by definition not a constant. This is the pattern that
came up when I did a search for how one makes a constant in Tcl. ""\_(
``_/ )_/""
Making it a procedure means that if someone wants to put actual logic
behind it in the future, it's already being called as a proc.
> Wew! This took longer than I expected ;)
>
> Tested on Linux. Works fine after fixing the extra arguments passed to
> `_close_updateindex`. Thanks.
Yeah, I did run things as I was changing them to verify, and felt like
I covered everything, I'm surprised I didn't bump into that, obviously
I didn't cover everything after all. Perfect demonstration of why
developers should never be exclusively responsible for testing their
own code :-D
Let me know w.r.t. which OO framework to employ and what that means
for minimum required versions and/or package references.
Thanks very much,
Jonathan Gilbert
There was a problem hiding this comment.
On the Git mailing list, Philip Oakley wrote (reply to this):
On 11/11/2019 21:55, Jonathan Gilbert wrote:
> Basic method (or, if you will, a "chord" with only one "note"):
>
> (caller)
> |
> void Add(int X, int Y)
> |
> { output(X + Y) }
>
> A "chord" with two "notes":
>
> (caller) (caller)
> | |
> void AddX(int X) void AddY(int Y)
> | |
> `-----------.-----------'
> |
> { output(X + Y) }
>
> The specific details differ from what I've written here. In Polyphonic
> C#, you don't have to instantiate a chord, you simply start calling
> methods, and the runtime matches up complete sets dynamically.
sounds like "Currying" a function but with the parameters taken in any
order, though, in a sense, perhaps not generating intermediate functions...
Philip
There was a problem hiding this comment.
On the Git mailing list, Jonathan Gilbert wrote (reply to this):
On Mon, Nov 11, 2019 at 4:59 PM Philip Oakley <philipoakley@iee.email> wrote:
> sounds like "Currying" a function but with the parameters taken in any
> order, though, in a sense, perhaps not generating intermediate functions...
It's like currying if you could pass g(x) = f(x, y) to one block of
code and h(y) = f(x, y) to another block of code, so that each of g
and h are each like curried versions of f that "bake in" one of the
arguments, without having to know which one will get called first. :-)
Jonathan Gilbert
There was a problem hiding this comment.
On the Git mailing list, Philip Oakley wrote (reply to this):
On 12/11/2019 04:49, Jonathan Gilbert wrote:
> On Mon, Nov 11, 2019 at 4:59 PM Philip Oakley <philipoakley@iee.email> wrote:
>> sounds like "Currying" a function but with the parameters taken in any
>> order, though, in a sense, perhaps not generating intermediate functions...
> It's like currying if you could pass g(x) = f(x, y) to one block of
> code and h(y) = f(x, y) to another block of code, so that each of g
> and h are each like curried versions of f that "bake in" one of the
> arguments, without having to know which one will get called first. :-)
>
> Jonathan Gilbert
So that would be called "Chording"...
(Is there a 'proper' technical term for that approach?)
P.
There was a problem hiding this comment.
On the Git mailing list, Jonathan Gilbert wrote (reply to this):
On Tue, Nov 12, 2019 at 4:45 AM Philip Oakley <philipoakley@iee.email> wrote:
> On 12/11/2019 04:49, Jonathan Gilbert wrote:
> > On Mon, Nov 11, 2019 at 4:59 PM Philip Oakley <philipoakley@iee.email> wrote:
> >> sounds like "Currying" a function but with the parameters taken in any
> >> order, though, in a sense, perhaps not generating intermediate functions...
> > It's like currying if you could pass g(x) = f(x, y) to one block of
> > code and h(y) = f(x, y) to another block of code, so that each of g
> > and h are each like curried versions of f that "bake in" one of the
> > arguments, without having to know which one will get called first. :-)
> >
> > Jonathan Gilbert
> So that would be called "Chording"...
> (Is there a 'proper' technical term for that approach?)
Not an entirely implausible term :-) The only other implementation
I've ever seen was Microsoft's "Polyphonic C#", which got rolled into
C-omega. I'm pretty sure, though, that it was never referred to as
something you _do to_ a function, but rather as a _different type_ of
function -- as in, the function hasn't been "chorded", it "is a
chord". Very little literature one way or the other though, and this
is the first actual, live use case for the structure I've encountered
in my years of programming :-)
Jonathan Gilbert
There was a problem hiding this comment.
On the Git mailing list, Pratyush Yadav wrote (reply to this):
Hi Jonathan,
On 11/11/19 03:55PM, Jonathan Gilbert wrote:
> On Mon, Nov 11, 2019 at 1:25 PM Pratyush Yadav me-at-yadavpratyush.com
> |GitHub Public/Example Allow| <172q77k4bxwj0zt@sneakemail.com> wrote:
> > On 07/11/19 07:05AM, Jonathan Gilbert via GitGitGadget wrote:
> > > --- /dev/null
> > > +++ b/lib/chord.tcl
> > > @@ -0,0 +1,137 @@
> >
> > The 'class' documentation [0] suggests adding a "package require TclOO".
> > But TclOO ships by default with Tcl 8.6 and above. So, I'm not really
> > sure if we need this.
>
> I'm not super familiar with it. I just checked what Tcl version I was
> myself running, since it's only there because of the Git Gui
> installation bundled with Git for Windows, and it was 8.6, so I
> assumed it was fair game to use. It didn't occur to me that you could
> already have an older version of Tcl installed and have Git Gui use
> it. :-) So, if I'm understanding correctly, `TclOO` as a package could
> potentially be used to allow TclOO to be used with 8.4, the minimum
> supported version you mention below, and it just happened to work for
> me in my testing without that because I have 8.6 installed but that's
> technically newer than the supported baseline?
>
> > Nice to see some good documentation!
> >
> > One nitpick: would it make more sense to have the documentation for a
> > method/constructor just above that method/constructor? This way, when
> > someone updates the code some time later, they'll also hopefully
> > remember to update the documentation. It is much more likely to be stale
> > if all of it just stays on the top.
>
> Hmm, what do you think of both? I was thinking of the documentation as
> a single self-contained block that someone could read to put together
> an understanding of how the chord system fits together, and split out,
> it wouldn't have that readability. What about a more abstract
> description in a block at the top, and then more technically-detailed
> & specific descriptions attached to each method?
Since you put it this way, it does make sense to create some flow. I'm
not sure if these relatively simple methods warrant specific detailed
documentation.
So, if you can figure out a reasonable split, that'd be great.
Otherwise, I guess we can just stick with this.
> > > +oo::class create SimpleChord {
> >
> > This comes from the TclOO package, right?
> >
> > git-gui has its own object-oriented system (lib/class.tcl). It was
> > written circa 2007. I suspect something like TclOO did not exist back
> > then.
> >
> > Why not use that? Does it have some limitations that TclOO does not
> > have? I do not mind using the "official" OO system. I just want to know
> > why exactly you made the choice.
>
> Having limited experience with Tcl, I did a Google search for "tcl
> object oriented" and ended up writing code using TclOO because that's
> what came up. Do you think it makes sense to rework this to use
> `class.tcl`, or perhaps instead the opposite: have a policy of using
> the standard TclOO going forward, and let the rest of Git Gui
> organically upgrade itself to some hypothetical point in the future
> where class.tcl is no longer used by anything?
Replacing class.tcl would be a big effort, and seeing how things stand
as of now in terms of active contributors, I don't think it would happen
in the near future.
So the question really boils down to "do we want to mix these two
flavors of OO frameworks?".
If TclOO gives us some benefit over our homegrown framework, or if our
framework is in some way hard to use, then I would certainly side on
just sticking with TclOO.
If not, it becomes a question of taste more of less. Which
implementation do we like more, and which more people would be
comfortable working with. And whether mixing the two is a good idea or
not.
That being said, I am more inclined towards using our homegrown
framework just for the sake of uniformity if nothing else.
So in the end I guess the answer is I dunno.
> > More importantly, TclOO ships as part of the core distribution with Tcl
> > 8.6, but as of now the minimum version required for git-gui is 8.4. So,
> > I think we should bump the minimum version (8.6 released circa 2012, so
> > most people should have caught up by now I hope).
>
> If I understand correctly, you mentioned that TclOO was intrinsically
> available to me because I was using Tcl 8.6, and that the manual
> recommends `package require TclOO` -- does that package dependency
> permit the use of TclOO on 8.4? If so, could that be a way to avoid
> bumping the minimum version required? Simply in the interest of
> keeping the scope of the change limited. If not, then bumping the
> minimum required version to 8.6 from 2012 doesn't seem entirely
> unreasonable either. :-)
I looked around a bit, and it seems that TclOO would not work with 8.4
[0]. So, a version bump is needed. Unless, of course, you decide to use
the OO framework provided by class.tcl.
The version can be bumped by editing the line git-gui.sh:33.
> > > + variable Notes
> > > + variable Body
> > > + variable IsCompleted
> >
> > Nitpick: Please use snake_case, here and in other places.
>
> Okay, yep -- I had copied the convention that I saw in TclOO examples,
> conscious of the fact that there might be a standard specific to
> object-oriented Tcl.
>
> > > + method notify_note_activation {} {
> >
> > Since this method is for internal use only, can it be made "private"?
> > Does the OO library support something like this?
>
> I don't think so, because it's called from outside the class. What
> we'd be looking for is something like C++'s "friend" syntax. Tcl
> doesn't seem to have this. Though, I just did some further Googling,
> and saw a hint that it might be possible to bypass member security on
> a case-by-case basis, so that the method is private but `ChordNote` is
> able to call it anyway. I'll see if I can't figure this out. :-)
I don't think too much complexity/hacking is warranted for something
like this. If you can figure out a really simple way to do it, great!
Otherwise, just keep it like it is.
> > > + method unknown {} {
> >
> > I'm a bit lost here. This method is named 'unknown', but searching for
> > 'unknown' in this patch just gives me two results: this line here, and
> > then one in a comment at the start of the file.
> >
> > From what I understand looking at the code, it some sort of a "default"
> > method, and is called when you run just `$chord_note`. How exactly is
> > this method designated to be the default?
> >
> > Also, "unknown" makes little sense in this context. Can you rename it to
> > something more meaningful? Maybe something like "activate_note"?
>
> I think it's the fact that it is named `unknown` that makes it the
> "default" method. I think this just needs documentary comments next to
> it. The TclOO documentation says:
Yes, a comment explaining it is the default would be nice.
> > obj unknown ?methodName? ?arg ...?
> > This method is called when an attempt to invoke the method methodName on
> > object obj fails. The arguments that the user supplied to the method are
> > given as arg arguments. If methodName is absent, the object was invoked with
> > no method name at all (or any other arguments).
>
> It was based on that last sentence that I interpreted `unknown` as,
> "This is a mechanism for making an object that can be called like a
> method."
Looks like this method would also be called if someone misspelled a
method name for this object. So say if someone by mistake writes
$note is_activate
this method would be called. This is a clear bug. So, add a check here
to make sure 'methodName' is actually absent. And if it isn't, display
an error. Displaying an error to the user on a programmer error can get
annoying. But since we don't have something like assertions in git-gui
yet, maybe that's the best way to get bugs noticed.
> > > + if {!$IsActivated} {
> > > + set IsActivated 1
> > > + $Chord notify_note_activation
> > > + }
> > > + }
> > > +}
> >
> > From what I understand, the "Note" object is effectively used as a
> > count. There is no other state associated with it. When I first heard of
> > your description of this abstraction, I assumed that a Note would also
> > store a script to execute with it. So, when you "activate" a note, it
> > would first execute the script, and then mark itself as "activated", and
> > notify the chord. Would that abstraction make more sense?
> >
> > I don't really mind keeping it this way, but I wonder if that design
> > would make the abstraction easier to wrap your head around.
>
> I learned about the concept of chords and notes from an experimental
> language that Microsoft created many years back called "Polyphonic C#"
> (which in turn got rolled into "Cw" (C-omega)), and in that
> abstraction, the idea was that, well, as a baseline, for starters, we
> have methods and each one, conceptually, has an entrypoint with a
> certain set of parameters, and when you call that entrypoint, the
> parameters are all set and the body runs. With a "chord", you have
> more than one entrypoint attached to the same body -- the entrypoints
> themselves don't have any logic associated with them individually.
> Each note has its own parameter list, and when all the notes have been
> called, the body is run with _all_ of those parameters.
>
> I drew some ASCII art, don't know if it'll translate in the message,
> but here goes :-)
>
> Basic method (or, if you will, a "chord" with only one "note"):
>
> (caller)
> |
> void Add(int X, int Y)
> |
> { output(X + Y) }
>
> A "chord" with two "notes":
>
> (caller) (caller)
> | |
> void AddX(int X) void AddY(int Y)
> | |
> `-----------.-----------'
> |
> { output(X + Y) }
>
> The specific details differ from what I've written here. In Polyphonic
> C#, you don't have to instantiate a chord, you simply start calling
> methods, and the runtime matches up complete sets dynamically. (Just
> thinking through the implications of this, if the notes aren't all
> called at exactly the same rate this obviously leads very easily to
> bugs that chew up all memory on incomplete chords. :-P) Also,
> Microsoft's language has parameters to each of the notes that are
> _all_ passed to the body at once. My implementation here is a "simple"
> chord, I didn't bother with arguments, as they aren't needed in this
> usage :-) I also found it much simpler to think of implementing the
> chord with the activations being explicit instead of implicit. So
> instead of saying up front, "Here is my method body and here are its 3
> entrypoints", with this implementation the chord is a dynamic object,
> you say "Here is my method body" and get back a thing that you can
> start tacking entrypoints onto.
>
> But, a "note" in a SimpleChord isn't a counter, it's a latch. The
> chord itself is acting sort of like a counter, in that all the notes
> need to be activated, but because the notes are latches, activating a
> note repeatedly has the same effect as activating it once. There's no
> way for one note to interfere with other notes, which wouldn't be the
> case if it literally were just a counter.
Makes sense.
> It seems to me that a chord where each note has a script of its own is
> actually basically just a class with methods, I guess with a common
> joined epilogue?:
>
> (caller) (caller)
> | |
> void AddX(int X) void AddY(int Y)
> | |
> { script for AddX } {script for AddY }
> | |
> `-----------.-----------'
> |
> { common tail?? }
Thanks for explaining.
I had a slightly different mental model of the abstraction. The figure
here is what I had in mind, with the exception being that the two
functions that the two callers call are independent of each other.
To put it in more detail, what I was thinking of was that you'd create a
bunch of scripts that had to be evaluated separately, independent of
each other. Each script is associated with a note. Activating a note
runs that script. And when all the notes are activated, the common tail
is executed.
As far as I see, the use of the chord in the patch has just two
independent operations that need to run a common tail once both are
complete.
That's not to say it has to be done this way. Your way works just as
well, just in a slightly different way :)
> The whole point is that the notes are conceptually different "headers"
> into _the same_ body. When you call a note of a chord, it is because
> you want the _chord_'s script to run, and the chord is acting as a
> construct that says "okay, yes, I'll satisfy your request that I
> execute, but you'll have to wait, because I'm going to satisfy _all_
> your requests in one go".
>
> > > $::main_status stop
> > > - unlock_index
> > > - uplevel #0 $after
> >
> > There is a call to unlock_index in the body of the if statement above
> > too. Do we want to remove that too, or should it be left alone?
> >
> > That codepath seems to be taken when a major error happens, and we just
> > resign to our fate and get a fresh start by doing a rescan and syncing
> > the repo state. So it is quite likely whatever operation we were doing
> > failed spectacularly.
> >
> > Maybe the answer is to swallow the bitter pill and introduce a
> > switch/boolean in `_close_updateindex` that controls whether the index
> > is unlocked or not. We unlock it when the if statement is not taken, and
> > keep the current codepath when it is. I call it a "bitter pill" because
> > I'm usually not a huge fan of adding knobs like that in functions. Makes
> > the function harder to reason about and makes it more bug prone.
> >
> > If you can think of a better/cleaner way of working around this,
> > suggestions are welcome!
>
> Hmm, so, yeah, the entire if statement only occurs if it can't close
> the file descriptor. Is that something that actually happens? If so,
> then it should perhaps be throwing an exception, because having
> started a rescan is probably more than the caller bargained for. That
> would prevent the callers from unlocking the index out from under the
> rescan, and also cancel any other processing they might be doing that
> is probably making bad assumptions with a rescan running.
This seems like defensive programming. It is accounting for something
_really bad_ happening.
If closing the file descriptor fails, it means the buffer was not
flushed properly for some reason. Whatever operations we thought we did
were potentially not completed. So, we just discard all
assumptions/state, and get a fresh start by doing a rescan. This was
introduced in d4e890e5 ("git-gui: Make sure we get errors from
git-update-index", 23-10-2007). The commit message says:
I'm seeing a lot of silent failures from git-update-index on
Windows and this is leaving the index.lock file intact, which
means users are later unable to perform additional operations.
When the index is locked behind our back and we are unable to
use it we may need to allow the user to delete the index lock
and try again. However our UI state is probably not currect
as we have assumed that some changes were applied but none of
them actually did. A rescan is the easiest (in code anyway)
solution to correct our UI to show what the index really has
(or doesn't have).
Since this is a _really_ old commit, I'm not sure if the problem still
exists today though.
So, this recovery code has to go somewhere. Yes, a rescan is certainly
more than what the caller wanted, but it is better than working on an
inconsistent in-memory state of the repo.
The question then becomes where the best place to do so is. This seems
like a good one if we can get our locking requirements to work with it
properly.
The glaring problem is that we don't want the rescan to run while the
deletion task is still running because they will interfere with each
other. Also, deletion expects the index to be locked, so the rescan and
deletion should be mutually exclusive.
One quick hack I can think of is to throw an error from this function,
and let the caller handle it. Then, in the callers that don't have the
deletion task to worry about, they just call the rescan (to be more
specific, the body of the if statement - moved to its own function). The
callers that do have to worry about the deletion somehow schedule it
after the deletion process finished. Or, they somehow cancel the
deletion operation, and then run the rescan.
Waiting till the deletion is over can probably be done by polling the
lock in an `after idle...`.
This is what I can think of at first glance. Maybe I'm missing a better
and cleaner way?
> > > if {$update_index_cp >= $total_cnt} {
> > > - _close_updateindex $fd $after
> > > + _close_updateindex $fd $do_unlock_index $after
> >
> > _close_updateindex takes only one argument, and you pass it 3.
> > $do_unlock_index does not seem to be defined anywhere. $after is
> > evaluated just after this line, and _close_updateindex doesn't accept
> > the argument anyway. I suspect this is a leftover from a different
> > approach you tried before this one.
>
> It is indeed, oops!
>
> > Also, unlike all the other places where _close_updateindex is used, this
> > one does not make a call to unlock_index. Is that intended? IIUC, it
> > should be intended, since this is the part which uses the "chord", but a
> > confirmation would be nice.
>
> Intentional, yes. I'll see if there's a concise way to document this.
>
> > > + # Common "after" functionality that waits until multiple asynchronous
> > > + # operations are complete (by waiting for them to activate their notes
> > > + # on the chord).
> >
> > Nitpick: mention what the "multiple asynchronous operations" are exactly
> > (i.e, they are the deletion and index checkout operations).
>
> Okeydoke.
>
> > > set after {}
> >
> > 'after' seems to be an unused variable. This line can be deleted.
>
> Good catch.
>
> > > + if {($deletion_error_cnt > 0) && ($deletion_error_cnt <= [MAX_VERBOSE_FILES_IN_DELETION_ERROR])} {
> >
> > Nitpick: please split the line into two.
>
> Will do.
>
> > > + set error_text "Encountered errors deleting files:\n"
> >
> > Wrap the string in a `mc [...]` so it can be translated some time in the
> > future.
>
> Ah, yes, I did that with most messages, this was an oversight.
>
> > > +proc MAX_VERBOSE_FILES_IN_DELETION_ERROR {} { return 10; }
> >
> > Why use a procedure, and not a global variable? My guess is to make it
> > impossible for some code to change this value by mistake. Do I guess
> > correctly?
>
> A variable is by definition not a constant. This is the pattern that
> came up when I did a search for how one makes a constant in Tcl. ""\_(
> ``_/ )_/""
>
> Making it a procedure means that if someone wants to put actual logic
> behind it in the future, it's already being called as a proc.
Makes sense.
> > Wew! This took longer than I expected ;)
> >
> > Tested on Linux. Works fine after fixing the extra arguments passed to
> > `_close_updateindex`. Thanks.
>
> Yeah, I did run things as I was changing them to verify, and felt like
> I covered everything, I'm surprised I didn't bump into that, obviously
> I didn't cover everything after all. Perfect demonstration of why
> developers should never be exclusively responsible for testing their
> own code :-D
>
> Let me know w.r.t. which OO framework to employ and what that means
> for minimum required versions and/or package references.
>
> Thanks very much,
>
> Jonathan Gilbert
[0] https://wiki.tcl-lang.org/page/MeTOO
--
Regards,
Pratyush Yadav
There was a problem hiding this comment.
On the Git mailing list, Philip Oakley wrote (reply to this):
On 12/11/2019 16:29, Jonathan Gilbert wrote:
> On Tue, Nov 12, 2019 at 4:45 AM Philip Oakley <philipoakley@iee.email> wrote:
>> On 12/11/2019 04:49, Jonathan Gilbert wrote:
>>> On Mon, Nov 11, 2019 at 4:59 PM Philip Oakley <philipoakley@iee.email> wrote:
>>>> sounds like "Currying" a function but with the parameters taken in any
>>>> order, though, in a sense, perhaps not generating intermediate functions...
>>> It's like currying if you could pass g(x) = f(x, y) to one block of
>>> code and h(y) = f(x, y) to another block of code, so that each of g
>>> and h are each like curried versions of f that "bake in" one of the
>>> arguments, without having to know which one will get called first. :-)
>>>
>>> Jonathan Gilbert
>> So that would be called "Chording"...
>> (Is there a 'proper' technical term for that approach?)
> Not an entirely implausible term :-) The only other implementation
> I've ever seen was Microsoft's "Polyphonic C#", which got rolled into
> C-omega. I'm pretty sure, though, that it was never referred to as
> something you _do to_ a function, but rather as a _different type_ of
> function -- as in, the function hasn't been "chorded", it "is a
> chord". Very little literature one way or the other though, and this
> is the first actual, live use case for the structure I've encountered
> in my years of programming :-)
>
A little bit of late follow up ;-)
The basic ideas that are embedded in "chording" would appear to be the
same as those used in Data Flow Diagrams and the older attempts at data
flow based machines such as the Transputer and it's message passing, and
out of order execution machines. See
https://en.wikipedia.org/wiki/Dataflow_architecture etc.
It just looks like it's now moved to the compiler, or JIT (just-in-time)
compilation, which appears to be the same thing with different branding!
Philip
|
On the Git mailing list, Pratyush Yadav wrote (reply to this): |
logiclrd
force-pushed
the
git-gui-revert-untracked
branch
from
9469beb
to
dc12c16
Compare
|
/submit |
|
Submitted as pull.436.v3.git.1573638988.gitgitgadget@gmail.com |
| @@ -30,8 +30,8 @@ along with this program; if not, see <http://www.gnu.org/licenses/>.}] | |||
| ## | |||
There was a problem hiding this comment.
On the Git mailing list, Pratyush Yadav wrote (reply to this):
Hi Jonathan,
Thanks for the re-roll.
[I removed some parts of the diff to make the reply easier to read. I am
implicitly OK with the removed parts.]
On 13/11/19 09:56AM, Jonathan Gilbert via GitGitGadget wrote:
> From: Jonathan Gilbert <JonathanG@iQmetrix.com>
>
> Update the revert_helper proc to check for untracked files as well as
> changes, and then handle changes to be reverted and untracked files with
> independent blocks of code. Prompt the user 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 remove the directory as well. Migrate unlocking of the index
> out of _close_updateindex to a responsibility of the caller, to permit
> paths that don't directly unlock the index, and refactor the error
> handling added in d4e890e5 so that callers can make flow control
> decisions in the event of errors.
>
> A new proc delete_files takes care of actually deleting the files in
> batches, 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. The `SimpleChord` class,
> based on TclOO (Tcl/Tk 8.6), is added in this commit.
Looks much better!
> Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
> ---
> git-gui.sh | 4 +-
> lib/chord.tcl | 160 +++++++++++++++++++
> lib/index.tcl | 416 ++++++++++++++++++++++++++++++++++++++++----------
> 3 files changed, 496 insertions(+), 84 deletions(-)
> create mode 100644 lib/chord.tcl
>
> diff --git a/lib/index.tcl b/lib/index.tcl
> index 28d4d2a54e..3ac08281c2 100644
> --- a/lib/index.tcl
> +++ b/lib/index.tcl
> @@ -7,53 +7,62 @@ proc _delete_indexlock {} {
> }
> }
>
> -proc _close_updateindex {fd after} {
> - global use_ttk NS
> +# Returns true if the operation succeeded, false if a rescan has been initiated.
> +proc _close_updateindex_rescan_on_error {fd} {
> + if {![catch {_close_updateindex $fd} err]} {
> + return true
> + } else {
> + rescan_on_error $err
> + return false
> + }
> +}
> +
> +proc _close_updateindex {fd} {
> fconfigure $fd -blocking 1
> - if {[catch {close $fd} err]} {
> - set w .indexfried
> - Dialog $w
> - wm withdraw $w
> - wm title $w [strcat "[appname] ([reponame]): " [mc "Index Error"]]
> - wm geometry $w "+[winfo rootx .]+[winfo rooty .]"
> - set s [mc "Updating the Git index failed. A rescan will be automatically started to resynchronize git-gui."]
> - text $w.msg -yscrollcommand [list $w.vs set] \
> - -width [string length $s] -relief flat \
> - -borderwidth 0 -highlightthickness 0 \
> - -background [get_bg_color $w]
> - $w.msg tag configure bold -font font_uibold -justify center
> - ${NS}::scrollbar $w.vs -command [list $w.msg yview]
> - $w.msg insert end $s bold \n\n$err {}
> - $w.msg configure -state disabled
> -
> - ${NS}::button $w.continue \
> - -text [mc "Continue"] \
> - -command [list destroy $w]
> - ${NS}::button $w.unlock \
> - -text [mc "Unlock Index"] \
> - -command "destroy $w; _delete_indexlock"
> - 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'?
> +}
>
> - $::main_status stop
> - unlock_index
> - rescan $after 0
> - return
> - }
> +proc rescan_on_error {err} {
> + global use_ttk NS
> +
> + set w .indexfried
> + Dialog $w
> + wm withdraw $w
> + wm title $w [strcat "[appname] ([reponame]): " [mc "Index Error"]]
> + wm geometry $w "+[winfo rootx .]+[winfo rooty .]"
> + set s [mc "Updating the Git index failed. A rescan will be automatically started to resynchronize git-gui."]
> + text $w.msg -yscrollcommand [list $w.vs set] \
> + -width [string length $s] -relief flat \
> + -borderwidth 0 -highlightthickness 0 \
> + -background [get_bg_color $w]
> + $w.msg tag configure bold -font font_uibold -justify center
> + ${NS}::scrollbar $w.vs -command [list $w.msg yview]
> + $w.msg insert end $s bold \n\n$err {}
> + $w.msg configure -state disabled
> +
> + ${NS}::button $w.continue \
> + -text [mc "Continue"] \
> + -command [list destroy $w]
> + ${NS}::button $w.unlock \
> + -text [mc "Unlock Index"] \
> + -command "destroy $w; _delete_indexlock"
> + 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
>
> $::main_status stop
Same question here.
> unlock_index
> - uplevel #0 $after
> + rescan ui_ready 0
> }
>
> proc update_indexinfo {msg path_list after} {
> @@ -90,7 +99,11 @@ proc write_update_indexinfo {fd path_list total_cnt batch after} {
> global file_states current_diff_path
>
> 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?
> return
> }
>
> @@ -156,7 +169,11 @@ proc write_update_index {fd path_list total_cnt batch after} {
> global file_states current_diff_path
>
> if {$update_index_cp >= $total_cnt} {
> - _close_updateindex $fd $after
> + if {[_close_updateindex_rescan_on_error $fd]} {
> + unlock_index
> + }
> +
> + uplevel #0 $after
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.
> return
> }
>
> @@ -193,7 +210,7 @@ proc write_update_index {fd path_list total_cnt batch after} {
> $::main_status update $update_index_cp $total_cnt
> }
>
> -proc checkout_index {msg path_list after} {
> +proc checkout_index {msg path_list after capture_error} {
> global update_index_cp
>
> if {![lock_index update]} return
> @@ -225,15 +242,21 @@ proc checkout_index {msg path_list after} {
> $total_cnt \
> $batch \
> $after \
> + $capture_error \
> ]
> }
>
> -proc write_checkout_index {fd path_list total_cnt batch after} {
> +proc write_checkout_index {fd path_list total_cnt batch after capture_error} {
> global update_index_cp
> global file_states current_diff_path
>
> 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.
> return
> }
>
This patch is almost ready to be merged. Looking forward to the
(hopefully) final iteration of this topic :)
--
Regards,
Pratyush Yadav
There was a problem hiding this comment.
On the Git mailing list, Jonathan Gilbert wrote (reply to this):
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
ed9fc40
to
23d4f5d
Compare
|
/submit |
|
Submitted as pull.436.v4.git.1573973770.gitgitgadget@gmail.com |
|
On the Git mailing list, Pratyush Yadav wrote (reply to this): |
|
On the Git mailing list, Jonathan Gilbert wrote (reply to this): |
| @@ -30,8 +30,8 @@ along with this program; if not, see <http://www.gnu.org/licenses/>.}] | |||
| ## | |||
There was a problem hiding this comment.
On the Git mailing list, Pratyush Yadav wrote (reply to this):
Hi Jonathan,
Thanks for the re-roll. Sorry for taking so long to review. I couldn't
find too much free time past few days.
On 17/11/19 06:56AM, Jonathan Gilbert via GitGitGadget wrote:
> From: Jonathan Gilbert <JonathanG@iQmetrix.com>
>
> Update the revert_helper proc to check for untracked files as well as
> changes, and then handle changes to be reverted and untracked files with
> independent blocks of code. Prompt the user 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 remove the directory as well. Migrate unlocking of the index
> out of _close_updateindex to a responsibility of the caller, to permit
> paths that don't directly unlock the index, and refactor the error
> handling added in d4e890e5 so that callers can make flow control
> decisions in the event of errors. Rework status_bar to explicitly handle
> multiple overlapping operations, and update all call sites.
>
> A new proc delete_files takes care of actually deleting the files in
> batches, 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. The `SimpleChord` class,
> based on TclOO (Tcl/Tk 8.6), is added in this commit.
>
> Since the checkout_index and delete_files calls are both asynchronous
> and overlap, they clash in wanting to update the status bar. This commit
> reworks the status bar 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.
The status bar is a major change, so I think it should be in a separate
commit. That would make it easier to debug it when bisecting, and to
revert it in case we discover a bug later.
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"
invalid command name "::status_bar::update"
while executing
"::status_bar::update ::status_bar::__o1::__d 5 4169"
("eval" body line 1)
invoked from within
"eval [list ::status_bar::$name ::status_bar::__o1::__d] $args"
(procedure "::status_bar::__o1::__d" line 1)
invoked from within
"$status update $blame_lines [set ${__this}::total_lines]"
(procedure "blame::_read_blame" line 184)
invoked from within
"blame::_read_blame ::blame::__o1::__d file7 .file_pane.out.asimple_t ::blame::__o1::asim_data"
This error is raised because of lib/blame.tcl:812, and causes blame to
not annotate lines properly, which is the entire reason to use blame.
Another caller that would probably break is 'lib/choose_repository.tcl'.
These are the only two broken callsites I can spot after some quick
looking around.
Are there any other backward-compatibility breaking changes to
status_bar? I have a feeling that this changeset is already getting a
bit too large in scope. Maybe we should figure out a simpler compromise
instead of making a huge re-work like this.
If the existing callsites can be updated without much trouble, then its
fine. Otherwise, maybe reducing the scope of this change would be a
better idea.
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).
> Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
> ---
> git-gui.sh | 7 +-
> lib/checkout_op.tcl | 15 +-
> lib/chord.tcl | 160 ++++++++++++++++
> lib/index.tcl | 443 +++++++++++++++++++++++++++++++++++---------
> lib/merge.tcl | 14 +-
> lib/status_bar.tcl | 221 +++++++++++++++++++---
> 6 files changed, 734 insertions(+), 126 deletions(-)
> create mode 100644 lib/chord.tcl
>
> diff --git a/git-gui.sh b/git-gui.sh
> index 0d21f5688b..dc4ac577ac 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -30,8 +30,8 @@ along with this program; if not, see <http://www.gnu.org/licenses/>.}]
> ##
> ## Tcl/Tk sanity check
>
> -if {[catch {package require Tcl 8.4} err]
> - || [catch {package require Tk 8.4} err]
> +if {[catch {package require Tcl 8.6} err]
> + || [catch {package require Tk 8.6} err]
> } {
> catch {wm withdraw .}
> tk_messageBox \
> @@ -4159,6 +4159,9 @@ if {$picked && [is_config_true gui.autoexplore]} {
> do_explore
> }
>
> +# 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.
> +
> # Local variables:
> # mode: tcl
> # indent-tabs-mode: t
> diff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl
> index a5228297db..21ea768d80 100644
> --- a/lib/checkout_op.tcl
> +++ b/lib/checkout_op.tcl
> @@ -341,9 +341,9 @@ method _readtree {} {
> global HEAD
>
> set readtree_d {}
> - $::main_status start \
> + set status_bar_operation [$::main_status start \
> [mc "Updating working directory to '%s'..." [_name $this]] \
> - [mc "files checked out"]
> + [mc "files checked out"]]
>
> set fd [git_read --stderr read-tree \
> -m \
> @@ -354,26 +354,27 @@ method _readtree {} {
> $new_hash \
> ]
> fconfigure $fd -blocking 0 -translation binary
> - fileevent $fd readable [cb _readtree_wait $fd]
> + fileevent $fd readable [cb _readtree_wait $fd $status_bar_operation]
> }
>
> -method _readtree_wait {fd} {
> +method _readtree_wait {fd status_bar_operation} {
> global current_branch
>
> set buf [read $fd]
> - $::main_status update_meter $buf
> + $status_bar_operation update_meter $buf
> append readtree_d $buf
>
> fconfigure $fd -blocking 1
> if {![eof $fd]} {
> fconfigure $fd -blocking 0
> + $status_bar_operation stop
> return
> }
>
> if {[catch {close $fd}]} {
> set err $readtree_d
> regsub {^fatal: } $err {} err
> - $::main_status stop [mc "Aborted checkout of '%s' (file level merging is required)." [_name $this]]
> + $status_bar_operation stop [mc "Aborted checkout of '%s' (file level merging is required)." [_name $this]]
> warn_popup [strcat [mc "File level merge required."] "
>
> $err
> @@ -384,7 +385,7 @@ $err
> return
> }
>
> - $::main_status stop
> + $status_bar_operation stop
> _after_readtree $this
> }
>
> diff --git a/lib/index.tcl b/lib/index.tcl
> index 28d4d2a54e..8d7590241e 100644
> --- a/lib/index.tcl
> +++ b/lib/index.tcl
> @@ -7,53 +7,63 @@ proc _delete_indexlock {} {
> }
> }
>
> -proc _close_updateindex {fd after} {
> - global use_ttk NS
> - fconfigure $fd -blocking 1
> - if {[catch {close $fd} err]} {
> - set w .indexfried
> - Dialog $w
> - wm withdraw $w
> - wm title $w [strcat "[appname] ([reponame]): " [mc "Index Error"]]
> - wm geometry $w "+[winfo rootx .]+[winfo rooty .]"
> - set s [mc "Updating the Git index failed. A rescan will be automatically started to resynchronize git-gui."]
> - text $w.msg -yscrollcommand [list $w.vs set] \
> - -width [string length $s] -relief flat \
> - -borderwidth 0 -highlightthickness 0 \
> - -background [get_bg_color $w]
> - $w.msg tag configure bold -font font_uibold -justify center
> - ${NS}::scrollbar $w.vs -command [list $w.msg yview]
> - $w.msg insert end $s bold \n\n$err {}
> - $w.msg configure -state disabled
> -
> - ${NS}::button $w.continue \
> - -text [mc "Continue"] \
> - -command [list destroy $w]
> - ${NS}::button $w.unlock \
> - -text [mc "Unlock Index"] \
> - -command "destroy $w; _delete_indexlock"
> - 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
> -
> - $::main_status stop
> +# 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.
> + 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?
> }
> +}
>
> - $::main_status stop
> +proc _close_updateindex {fd} {
> + fconfigure $fd -blocking 1
> + close $fd
> +}
> +
> +proc rescan_on_error {err {after {}}} {
> + global use_ttk NS
> +
> + set w .indexfried
> + Dialog $w
> + wm withdraw $w
> + wm title $w [strcat "[appname] ([reponame]): " [mc "Index Error"]]
> + wm geometry $w "+[winfo rootx .]+[winfo rooty .]"
> + set s [mc "Updating the Git index failed. A rescan will be automatically started to resynchronize git-gui."]
> + text $w.msg -yscrollcommand [list $w.vs set] \
> + -width [string length $s] -relief flat \
> + -borderwidth 0 -highlightthickness 0 \
> + -background [get_bg_color $w]
> + $w.msg tag configure bold -font font_uibold -justify center
> + ${NS}::scrollbar $w.vs -command [list $w.msg yview]
> + $w.msg insert end $s bold \n\n$err {}
> + $w.msg configure -state disabled
> +
> + ${NS}::button $w.continue \
> + -text [mc "Continue"] \
> + -command [list destroy $w]
> + ${NS}::button $w.unlock \
> + -text [mc "Unlock Index"] \
> + -command "destroy $w; _delete_indexlock"
> + 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
> +
> + $::main_status stop_all
> unlock_index
> - uplevel #0 $after
> + rescan [concat $after [list ui_ready]] 0
> }
>
> proc update_indexinfo {msg path_list after} {
> @@ -67,7 +77,7 @@ proc update_indexinfo {msg path_list after} {
> set batch [expr {int($total_cnt * .01) + 1}]
> if {$batch > 25} {set batch 25}
>
> - $::main_status start $msg [mc "files"]
> + set status_bar_operation [$::main_status start $msg [mc "files"]]
> set fd [git_write update-index -z --index-info]
> fconfigure $fd \
> -blocking 0 \
> @@ -81,16 +91,19 @@ proc update_indexinfo {msg path_list after} {
> $path_list \
> $total_cnt \
> $batch \
> + $status_bar_operation \
> $after \
> ]
> }
>
> -proc write_update_indexinfo {fd path_list total_cnt batch after} {
> +proc write_update_indexinfo {fd path_list total_cnt batch status_bar_operation \
> + after} {
> global update_index_cp
> global file_states current_diff_path
>
> if {$update_index_cp >= $total_cnt} {
> - _close_updateindex $fd $after
> + $status_bar_operation stop
> + close_and_unlock_updateindex_rescan_on_error $fd $after
> return
> }
>
> @@ -119,7 +132,7 @@ proc write_update_indexinfo {fd path_list total_cnt batch after} {
> display_file $path $new
> }
>
> - $::main_status update $update_index_cp $total_cnt
> + $status_bar_operation update $update_index_cp $total_cnt
> }
>
> proc update_index {msg path_list after} {
> @@ -133,7 +146,7 @@ proc update_index {msg path_list after} {
> set batch [expr {int($total_cnt * .01) + 1}]
> if {$batch > 25} {set batch 25}
>
> - $::main_status start $msg [mc "files"]
> + set status_bar_operation [$::main_status start $msg [mc "files"]]
> set fd [git_write update-index --add --remove -z --stdin]
> fconfigure $fd \
> -blocking 0 \
> @@ -147,16 +160,19 @@ proc update_index {msg path_list after} {
> $path_list \
> $total_cnt \
> $batch \
> + $status_bar_operation \
> $after \
> ]
> }
>
> -proc write_update_index {fd path_list total_cnt batch after} {
> +proc write_update_index {fd path_list total_cnt batch status_bar_operation \
> + after} {
> global update_index_cp
> global file_states current_diff_path
>
> if {$update_index_cp >= $total_cnt} {
> - _close_updateindex $fd $after
> + $status_bar_operation stop
> + close_and_unlock_updateindex_rescan_on_error $fd $after
> return
> }
>
> @@ -190,10 +206,10 @@ proc write_update_index {fd path_list total_cnt batch after} {
> display_file $path $new
> }
>
> - $::main_status update $update_index_cp $total_cnt
> + $status_bar_operation update $update_index_cp $total_cnt
> }
>
> -proc checkout_index {msg path_list after} {
> +proc checkout_index {msg path_list after capture_error} {
> global update_index_cp
>
> if {![lock_index update]} return
> @@ -204,7 +220,7 @@ proc checkout_index {msg path_list after} {
> set batch [expr {int($total_cnt * .01) + 1}]
> if {$batch > 25} {set batch 25}
>
> - $::main_status start $msg [mc "files"]
> + set status_bar_operation [$::main_status start $msg [mc "files"]]
> set fd [git_write checkout-index \
> --index \
> --quiet \
> @@ -224,16 +240,26 @@ proc checkout_index {msg path_list after} {
> $path_list \
> $total_cnt \
> $batch \
> + $status_bar_operation \
> $after \
> + $capture_error \
> ]
> }
>
> -proc write_checkout_index {fd path_list total_cnt batch after} {
> +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.
> + uplevel #0 $capture_error [list $err]
> + }
> +
> + uplevel #0 $after
> +
> return
> }
>
> diff --git a/lib/status_bar.tcl b/lib/status_bar.tcl
> index 02111a1742..6a73988b23 100644
> --- a/lib/status_bar.tcl
> +++ b/lib/status_bar.tcl
> @@ -1,16 +1,42 @@
> # git-gui status bar mega-widget
> # Copyright (C) 2007 Shawn Pearce
>
> +# The status_bar class manages the entire status bar. It is possible for
> +# multiple overlapping asynchronous operations to want to display status
> +# simultaneously. Each one receives a status_bar_operation when it calls the
> +# start method, and the status bar combines all active operations into the
> +# line of text it displays. Most of the time, there will be at most one
> +# ongoing operation.
> +#
> +# Note that the entire status bar can be either in single-line or two-line
> +# mode, depending on the constructor. Multiple active operations are only
> +# supported for single-line status bars.
> +
> class status_bar {
>
> +field allow_multiple ; # configured at construction
> +
> field w ; # our own window path
> field w_l ; # text widget we draw messages into
> field w_c ; # canvas we draw a progress bar into
> field c_pack ; # script to pack the canvas with
> -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.
> -field prefix {}; # text we format into status
> -field units {}; # unit of progress
> -field meter {}; # current core git progress meter (if active)
> +
> +field baseline_text ; # text to show if there are no operations
> +field status_bar_text ; # combined text for all operations
> +
> +field operations ; # list of current ongoing operations
> +
> +# The status bar can display a progress bar, updated when consumers call the
> +# update method on their status_bar_operation. When there are multiple
> +# operations, the status bar shows the combined status of all operations.
> +#
> +# When an overlapping operation completes, the progress bar is going to
> +# abruptly have one fewer operation in the calculation, causing a discontinuity.
> +# Therefore, whenever an operation completes, if it is not the last operation,
> +# this counter is increased, and the progress bar is calculated as though there
> +# were still another operation at 100%. When the last operation completes, this
> +# is reset to 0.
> +field completed_operation_count
>
> constructor new {path} {
> global use_ttk NS
> @@ -18,12 +44,19 @@ constructor new {path} {
> set w_l $w.l
> set w_c $w.c
>
> + # Standard single-line status bar: Permit overlapping operations
> + set allow_multiple 1
> +
> + set baseline_text ""
> + set operations [list]
> + set completed_operation_count 0
> +
> ${NS}::frame $w
> if {!$use_ttk} {
> $w configure -borderwidth 1 -relief sunken
> }
> ${NS}::label $w_l \
> - -textvariable @status \
> + -textvariable @status_bar_text \
> -anchor w \
> -justify left
> pack $w_l -side left
> @@ -44,9 +77,15 @@ constructor two_line {path} {
> set w_l $w.l
> set w_c $w.c
>
> + # Two-line status bar: Only one ongoing operation permitted.
> + set allow_multiple 0
> +
> + set baseline_text ""
> + set operations [list]
> +
> ${NS}::frame $w
> ${NS}::label $w_l \
> - -textvariable @status \
> + -textvariable @status_bar_text \
> -anchor w \
> -justify left
> pack $w_l -anchor w -fill x
> @@ -56,7 +95,7 @@ constructor two_line {path} {
> return $this
> }
>
> -method start {msg uds} {
> +method ensure_canvas {} {
> if {[winfo exists $w_c]} {
> $w_c coords bar 0 0 0 20
> } else {
> @@ -68,31 +107,169 @@ method start {msg uds} {
> $w_c create rectangle 0 0 0 20 -tags bar -fill navy
> eval $c_pack
> }
> +}
> +
> +method show {msg {test {}}} {
> + if {$test eq {} || $status eq $test} {
> + $this ensure_canvas
> + set baseline_text $msg
> + $this refresh
> + }
> +}
> +
> +method start {msg uds} {
> + set baseline_text ""
> +
> + if {!$allow_multiple && [llength $operations]} {
This silently ignores multiple 'start's on a status bar that doesn't
allow it, correct?
> + return [lindex $operations 0]
> + }
> +
> + $this ensure_canvas
> +
> + set operation [status_bar_operation::new $this $msg $uds]
> +
> + lappend operations $operation
> +
> + $this refresh
> +
> + return $operation
> +}
> +
> +method refresh {} {
> + set new_text ""
> +
> + set total [expr $completed_operation_count * 100]
> + set have $total
> +
> + foreach operation $operations {
> + if {$new_text != ""} {
> + append new_text " / "
> + }
> +
> + append new_text [$operation get_status]
> +
> + set total [expr $total + 100]
> + set have [expr $have + [$operation get_progress]]
> + }
> +
> + if {$new_text == ""} {
> + set new_text $baseline_text
> + }
> +
> + set status_bar_text $new_text
> +
> + set pixel_width 0
> + if {$have > 0} {
> + set pixel_width [expr {[winfo width $w_c] * $have / $total}]
> + }
> +
> + $w_c coords bar 0 0 $pixel_width 20
> +}
> +
> +method stop {operation stop_msg} {
> + set idx [lsearch $operations $operation]
> +
> + if {$idx >= 0} {
> + set operations [lreplace $operations $idx $idx]
> + set completed_operation_count [expr \
> + $completed_operation_count + 1]
> +
> + if {[llength operations] == 0} {
> + set completed_operation_count 0
> +
> + destroy $w_c
> + if {$stop_msg ne {}} {
> + set baseline_text $stop_msg
> + }
> + }
> +
> + $this refresh
> + }
> +}
> +
> +method stop_all {{stop_msg {}}} {
> + set operations_copy $operations
> + set operations [list] # This makes the operation's call to stop a no-op.
> +
> + foreach $operation operations_copy {
> + $operation stop
> + }
> +
> + if {$stop_msg ne {}} {
> + set baseline_text $stop_msg
> + }
> +
> + $this refresh
> +}
> +
> +method _delete {current} {
> + if {$current eq $w} {
> + delete_this
> + }
> +}
> +
> +}
> +
> +# The status_bar_operation class tracks a single consumer's ongoing status bar
> +# activity, with the context that there are a few situations where multiple
> +# overlapping asynchronous operations might want to display status information
> +# simultaneously. Instances of status_bar_operation are created by calling
> +# start on the status_bar, and when the caller is done with its stauts bar
> +# operation, it calls stop on the operation.
> +
> +class status_bar_operation {
> +
> +field status_bar; # reference back to the status_bar that owns this object
> +
> +field is_active;
> +
> +field status {}; # single line of text we show
> +field progress {}; # current progress (0 to 100)
> +field prefix {}; # text we format into status
> +field units {}; # unit of progress
> +field meter {}; # current core git progress meter (if active)
> +
> +constructor new {owner msg uds} {
> + set status_bar $owner
>
> set status $msg
> + set progress 0
> set prefix $msg
> set units $uds
> set meter {}
> +
> + set is_active 1
> +
> + return $this
> }
>
> +method get_is_active {} { return $is_active }
> +method get_status {} { return $status }
> +method get_progress {} { return $progress }
> +
> method update {have total} {
> - set pdone 0
> - set cdone 0
> + if {!$is_active} { return }
> +
> + set progress 0
> +
> if {$total > 0} {
> - set pdone [expr {100 * $have / $total}]
> - set cdone [expr {[winfo width $w_c] * $have / $total}]
> + set progress [expr {100 * $have / $total}]
> }
>
> set prec [string length [format %i $total]]
> +
> set status [mc "%s ... %*i of %*i %s (%3i%%)" \
> $prefix \
> $prec $have \
> $prec $total \
> - $units $pdone]
> - $w_c coords bar 0 0 $cdone 20
> + $units $progress]
> +
> + $status_bar refresh
> }
>
> method update_meter {buf} {
> + if {!$is_active} { return }
> +
> append meter $buf
> set r [string last "\r" $meter]
> if {$r == -1} {
> @@ -109,23 +286,25 @@ method update_meter {buf} {
> }
> }
>
> -method stop {{msg {}}} {
> - destroy $w_c
> - if {$msg ne {}} {
> - set status $msg
> +method stop {{stop_msg {}}} {
> + if {$is_active} {
> + set is_active 0
> + $status_bar stop $this $stop_msg
> }
> }
>
> method show {msg {test {}}} {
> + if {!$is_active} { return }
> +
> if {$test eq {} || $status eq $test} {
> set status $msg
> + $status_bar refresh
> }
> }
>
> -method _delete {current} {
> - if {$current eq $w} {
> - delete_this
> - }
> +method _delete {} {
> + stop
> + delete_this
> }
>
> }
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?
Works fine on some quick testing, though I haven't done anything too
thorough. Thanks.
--
Regards,
Pratyush Yadav
ca71186
to
89f4929
Compare
logiclrd
force-pushed
the
git-gui-revert-untracked
branch
2 times, most recently
from
dfe2bab
to
d0d6593
Compare
|
/submit |
|
Submitted as pull.436.v5.git.1574627876.gitgitgadget@gmail.com |
| @@ -1797,10 +1797,10 @@ proc ui_status {msg} { | |||
| } | |||
There was a problem hiding this comment.
On the Git mailing list, Pratyush Yadav wrote (reply to this):
Hi Jonathan,
Thanks for the re-roll.
On 24/11/19 08:37PM, Jonathan Gilbert via GitGitGadget wrote:
> From: Jonathan Gilbert <JonathanG@iQmetrix.com>
>
> Update the status bar to track updates as individual "operations" that
> can overlap. Update all call sites to interact with the new status bar
> mechanism. Update initialization to explicitly clear status text,
> since otherwise it may persist across future operations.
>
> Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
> ---
> git-gui.sh | 7 +-
> lib/blame.tcl | 22 +++--
> lib/checkout_op.tcl | 15 +--
> lib/index.tcl | 31 +++---
> lib/merge.tcl | 14 ++-
> lib/status_bar.tcl | 228 +++++++++++++++++++++++++++++++++++++++-----
> 6 files changed, 260 insertions(+), 57 deletions(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index 0d21f5688b..db02e399e7 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -1797,10 +1797,10 @@ proc ui_status {msg} {
> }
> }
>
> -proc ui_ready {{test {}}} {
> +proc ui_ready {} {
This is not quite correct. There is one user of 'ui_ready' that uses
'test'. It is in git-gui.sh:2211. It is used when starting gitk. This
change breaks that call. 10 seconds after opening gitk via the
"Visualise master's history" option, I get the following error:
wrong # args: should be "ui_ready"
while executing
"ui_ready $starting_gitk_msg"
("after" script)
The code that calls it (git-gui.sh:2211) looks like:
ui_status $::starting_gitk_msg
after 10000 {
ui_ready $starting_gitk_msg
}
I am not quite sure why this is done though. It was introduced in
e210e67 (git-gui: Corrected keyboard bindings on Windows, improved state
management., 2006-11-06) [0], but the commit message doesn't really
explain why (probably because it is a small part of a larger change,
though it doesn't really fit in with the topic of the change). I can't
find a mailing list thread about the commit so I don't think we'll ever
know for sure.
From looking at it, my guess is that it was added because gitk took a
long time to start up (maybe it still does, but for me its almost
instant). And so, this message was shown for 10 seconds, and then
cleared because by then it probably would have started. But to avoid
over-writing some other message, 'test' was used to make sure only the
message intended to be cleared is cleared.
I'm not sure if this heuristic/hack is really needed, and that we need
to keep the "Starting gitk..." message around for 10 seconds. The way I
see it, it doesn't add too much value unless gitk takes a long time to
start up on other platforms or repos. In that case an indication of
"we're working on starting gitk" would be nice. Otherwise, I don't mind
seeing this go. And even then, I think it is gitk's responsibility to
give some sort of indication to the user that it is booting up, and not
ours.
So, I vote for just getting rid of this hack.
> global main_status
> if {[info exists main_status]} {
> - $main_status show [mc "Ready."] $test
> + $main_status show [mc "Ready."]
> }
> }
>
> @@ -4159,6 +4159,9 @@ if {$picked && [is_config_true gui.autoexplore]} {
> do_explore
> }
>
> +# Clear "Initializing..." status
> +after 500 {$main_status show ""}
> +
> # Local variables:
> # mode: tcl
> # indent-tabs-mode: t
> diff --git a/lib/blame.tcl b/lib/blame.tcl
> index a1aeb8b96e..888f98bab2 100644
> --- a/lib/blame.tcl
> +++ b/lib/blame.tcl
> @@ -24,6 +24,7 @@ field w_cviewer ; # pane showing commit message
> field finder ; # find mini-dialog frame
> field gotoline ; # line goto mini-dialog frame
> field status ; # status mega-widget instance
> +field status_operation ; # status operation
Nitpick: The comment doesn't give any information the field name doesn't
already give. Either remove it or replace it with something more
descriptive.
> field old_height ; # last known height of $w.file_pane
>
>
> @@ -274,6 +275,7 @@ constructor new {i_commit i_path i_jump} {
> pack $w_cviewer -expand 1 -fill both
>
> set status [::status_bar::new $w.status]
> + set status_operation {}
>
> menu $w.ctxm -tearoff 0
> $w.ctxm add command \
> @@ -602,16 +604,21 @@ method _exec_blame {cur_w cur_d options cur_s} {
> } else {
> lappend options $commit
> }
> +
> + # We may recurse in from another call to _exec_blame and already have
> + # a status operation.
Thanks for being thorough enough to spot this :)
> + if {$status_operation == {}} {
> + set status_operation [$status start \
> + $cur_s \
> + [mc "lines annotated"]]
The call to this method from '_read_blame' specifies a different $cur_s.
So shouldn't we be destroying $status_operation (after stopping it), and
creating a new one?
> + }
> +
> lappend options -- $path
> set fd [eval git_read --nice blame $options]
> fconfigure $fd -blocking 0 -translation lf -encoding utf-8
> fileevent $fd readable [cb _read_blame $fd $cur_w $cur_d]
> set current_fd $fd
> set blame_lines 0
> -
> - $status start \
> - $cur_s \
> - [mc "lines annotated"]
> }
>
> method _read_blame {fd cur_w cur_d} {
You did not update 'lib/choose_repository.tcl'. It still uses the old
version of the status bar. Other than that, the rest of the patch looks
good. Thanks.
[0]:
Curiously, if I do 'git log -L 2208,+5:git-gui.sh' to find the origins
of the line, it leads me to the commit 25476c6. And looking at the
commit, it does indeed appear to be the origin of the line since the
line is in the post-image, and not the pre-image. But I accidentally
noticed the line in a parent of that commit. Looking further, it turns
out the line originated in e210e67. Probably a bug in some really old
versions of git. Interesting nonetheless.
--
Regards,
Pratyush Yadav
There was a problem hiding this comment.
On the Git mailing list, Jonathan Gilbert wrote (reply to this):
On Wed, Nov 27, 2019 at 3:59 PM Pratyush Yadav me-at-yadavpratyush.com
|GitHub Public/Example Allow| <172q77k4bxwj0zt@sneakemail.com> wrote:
> On 24/11/19 08:37PM, Jonathan Gilbert via GitGitGadget wrote:
> > -proc ui_ready {{test {}}} {
> > +proc ui_ready {} {
>
> This is not quite correct. There is one user of 'ui_ready' that uses
> 'test'. It is in git-gui.sh:2211. It is used when starting gitk. This
> change breaks that call. 10 seconds after opening gitk via the
> "Visualise master's history" option, I get the following error:
>
> wrong # args: should be "ui_ready"
> while executing
> "ui_ready $starting_gitk_msg"
> ("after" script)
[..]
> I'm not sure if this heuristic/hack is really needed, and that we need
> to keep the "Starting gitk..." message around for 10 seconds.
[..]
> So, I vote for just getting rid of this hack.
Oh geeze, I can't believe I missed this. This looks like it ought to
be relatively straightforward to port to the new operations, though,
which is a more isolated approach (keeping this change's fingers where
they belong), and then the operation provides segregation that means
it can just be ended after X seconds without caring what anything else
might be doing with the status bar. We can independently figure out if
we want to restructure that part. Given that computers are faster now
and that the status bar could end up doing something else in the
meantime (well let's be realistic, probably not, but who knows :-) ),
I'd vote for reducing the time the message is shown from 10 seconds
to, I dunno, 3 or 4 seconds.
One other thing I note is that both `do_gitk` and `do_git_gui` use
`$starting_gitk_msg`, which means that when `do_git_gui` is invoked to
launch a Git Gui child process for a submodule, it will be setting the
status bar text to say that it is launching Gitk.
Speaking of things that are out of scope for this PR, I did notice
this in the code:
> # -- Always start git gui through whatever we were loaded with. This
> # lets us bypass using shell process on Windows systems.
> #
> set exe [list [_which git]]
As far as I can tell, there's virtually no connection between the
comment and what the code is actually doing. I haven't yet figured out
what exactly it is or where it comes from, but on my Windows systems,
`git-gui` is actually an EXE file `git-gui.exe`, and I _think_ what it
is doing is running `wish.exe`, which I'm guessing has something to do
with hosting a Tcl interpreter with Win32 support for Tk GUI.
I'm not sure whether the code is doing the right thing here or not,
but I'm pretty sure what it's _not_ doing is figuring out how the
current `git-gui` process was started/is being hosted. :-P
> > field finder ; # find mini-dialog frame
> > field gotoline ; # line goto mini-dialog frame
> > field status ; # status mega-widget instance
> > +field status_operation ; # status operation
>
> Nitpick: The comment doesn't give any information the field name doesn't
> already give. Either remove it or replace it with something more
> descriptive.
Hmm, okay. I didn't want something that felt wildly imbalanced with
respect to the other lines, but you're right that this particular line
is literally just repeating the variable name. :-P
> > + if {$status_operation == {}} {
> > + set status_operation [$status start \
> > + $cur_s \
> > + [mc "lines annotated"]]
>
> The call to this method from '_read_blame' specifies a different $cur_s.
> So shouldn't we be destroying $status_operation (after stopping it), and
> creating a new one?
We can change the text by calling `$status_operation show`.
> > method _read_blame {fd cur_w cur_d} {
>
> You did not update 'lib/choose_repository.tcl'. It still uses the old
> version of the status bar. Other than that, the rest of the patch looks
> good. Thanks.
Ugh, I can't believe I overlooked this. I was aware of the file using
the status bar, because it's the one place that uses the `two_line`
constructor, but then I forgot to actually make it create and use the
(single concurrent) operation that a `two_line`-er is allowed.
The code in there seems to overload the purpose of the `o_cons`
variable, so that sometimes it is pointing at a status bar and
sometimes it is pointing at whatever `console::embed` returns. I will
change this.
This code also depends heavily on `update` to keep the UI active,
which as I understand it is problematic because it could potentially
result in re-entrance since the user can interact with the UI in the
middle of the operation. I will not make any attempt to change this,
though. :-)
> [0]:
> Curiously, if I do 'git log -L 2208,+5:git-gui.sh' to find the origins
> of the line, it leads me to the commit 25476c6. And looking at the
> commit, it does indeed appear to be the origin of the line since the
> line is in the post-image, and not the pre-image. But I accidentally
> noticed the line in a parent of that commit. Looking further, it turns
> out the line originated in e210e67. Probably a bug in some really old
> versions of git. Interesting nonetheless.
In e210e67, I see this:
set starting_gitk_msg {Please wait... Starting gitk...}
proc do_gitk {} {
global tcl_platform ui_status_value starting_gitk_msg
set ui_status_value $starting_gitk_msg
after 5000 {
if {$ui_status_value == $starting_gitk_msg} {
set ui_status_value {Ready.}
}
}
...
In 043f7011, all string comparisons were changed from ==/!= to eq/ne.
The commit message explains that when you use == and !=, Tcl will
attempt to convert either side to numeric if one of the two sides
looks like a numeric. Guess I should review my commit for this error
:-P
- if {$ui_status_value == $starting_gitk_msg} {
+ if {$ui_status_value eq $starting_gitk_msg} {
In 699d5601 "Refactor our ui_status_value update technique", this became:
set starting_gitk_msg [mc "Starting gitk... please wait..."]
...
global ... starting_gitk_msg
...
ui_status $starting_gitk_msg
after 10000 {
ui_ready $starting_gitk_msg
}
Finally it became this in 02efd48f, apparently an unrelated
refactoring removed the global variable declaration:
set starting_gitk_msg [mc "Starting gitk... please wait..."]
...
ui_status $::starting_gitk_msg
after 10000 {
ui_ready $starting_gitk_msg
}
I gathered this information using Git Gui's blame function, which I
guess is a good demonstration that my latest blame.tcl revision
corrects the problems in the earlier submission :-D
Next revision coming soon.
Jonathan Gilbert
| @@ -30,8 +30,8 @@ along with this program; if not, see <http://www.gnu.org/licenses/>.}] | |||
| ## | |||
There was a problem hiding this comment.
On the Git mailing list, Pratyush Yadav wrote (reply to this):
Hi Jonathan,
On 24/11/19 08:37PM, Jonathan Gilbert via GitGitGadget wrote:
> From: Jonathan Gilbert <JonathanG@iQmetrix.com>
>
> Update the revert_helper proc to check for untracked files as well as
> changes, and then handle changes to be reverted and untracked files with
> independent blocks of code. Prompt the user 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 remove the directory as well. Migrate unlocking of the index
> out of _close_updateindex to a responsibility of the caller, to permit
> paths that don't directly unlock the index, and refactor the error
> handling added in d4e890e5 so that callers can make flow control
> decisions in the event of errors. Update Tcl/Tk dependency from 8.4 to
> 8.6 in git-gui.sh.
>
> A new proc delete_files takes care of actually deleting the files in
> batches, 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. The `SimpleChord` class,
> based on TclOO (Tcl/Tk 8.6), is added in this commit.
>
> Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
> ---
> git-gui.sh | 4 +-
> lib/chord.tcl | 160 +++++++++++++++++++
> lib/index.tcl | 422 ++++++++++++++++++++++++++++++++++++++++----------
> 3 files changed, 502 insertions(+), 84 deletions(-)
> create mode 100644 lib/chord.tcl
From what I can tell, this re-roll of the patch only has minor changes
all of which look good. Thanks.
--
Regards,
Pratyush Yadav
logiclrd
force-pushed
the
git-gui-revert-untracked
branch
from
d0d6593
to
3388407
Compare
|
/submit |
|
Submitted as pull.436.v6.git.1574929833.gitgitgadget@gmail.com |
| @@ -30,8 +30,8 @@ along with this program; if not, see <http://www.gnu.org/licenses/>.}] | |||
| ## | |||
There was a problem hiding this comment.
On the Git mailing list, Pratyush Yadav wrote (reply to this):
Hi Jonathan,
Thanks for the re-roll.
On 28/11/19 08:30AM, Jonathan Gilbert via GitGitGadget wrote:
> From: Jonathan Gilbert <JonathanG@iQmetrix.com>
>
> Update the status bar to track updates as individual "operations" that
> can overlap. Update all call sites to interact with the new status bar
> mechanism. Update initialization to explicitly clear status text,
> since otherwise it may persist across future operations.
>
> Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
> ---
> git-gui.sh | 31 +++---
> lib/blame.tcl | 24 ++--
> lib/checkout_op.tcl | 15 +--
> lib/choose_repository.tcl | 120 +++++++++++++-------
> lib/index.tcl | 31 ++++--
> lib/merge.tcl | 14 ++-
> lib/status_bar.tcl | 229 +++++++++++++++++++++++++++++++++-----
> 7 files changed, 354 insertions(+), 110 deletions(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index 0d21f5688b..6dcf6551b6 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -30,8 +30,8 @@ along with this program; if not, see <http://www.gnu.org/licenses/>.}]
> ##
> ## Tcl/Tk sanity check
>
> -if {[catch {package require Tcl 8.4} err]
> - || [catch {package require Tk 8.4} err]
> +if {[catch {package require Tcl 8.6} err]
> + || [catch {package require Tk 8.6} err]
Nitpick: Since TclOO is introduced in patch 3 (and the commit message of
patch 3 mentions it), this hunk should be in that patch instead.
> } {
> catch {wm withdraw .}
> tk_messageBox \
> @@ -1797,10 +1797,10 @@ proc ui_status {msg} {
> }
> }
>
> -proc ui_ready {{test {}}} {
> +proc ui_ready {} {
> global main_status
> if {[info exists main_status]} {
> - $main_status show [mc "Ready."] $test
> + $main_status show [mc "Ready."]
> }
> }
>
> @@ -2150,8 +2150,6 @@ proc incr_font_size {font {amt 1}} {
> ##
> ## ui commands
>
> -set starting_gitk_msg [mc "Starting gitk... please wait..."]
> -
> proc do_gitk {revs {is_submodule false}} {
> global current_diff_path file_states current_diff_side ui_index
> global _gitdir _gitworktree
> @@ -2206,10 +2204,11 @@ proc do_gitk {revs {is_submodule false}} {
> set env(GIT_WORK_TREE) $_gitworktree
> cd $pwd
>
> - ui_status $::starting_gitk_msg
> - after 10000 {
> - ui_ready $starting_gitk_msg
> - }
> + set status_operation [$::main_status \
> + start \
> + [mc "Starting %s... please wait..." "gitk"]]
> +
> + after 3500 [list $status_operation stop]
> }
> }
>
> @@ -2240,10 +2239,11 @@ proc do_git_gui {} {
> set env(GIT_WORK_TREE) $_gitworktree
> cd $pwd
>
> - ui_status $::starting_gitk_msg
> - after 10000 {
> - ui_ready $starting_gitk_msg
> - }
> + set status_operation [$::main_status \
> + start \
> + [mc "Starting %s... please wait..." "git-gui"]]
> +
> + after 3500 [list $status_operation stop]
> }
> }
Looks good. Thanks for the cleanup.
>
> @@ -4159,6 +4159,9 @@ if {$picked && [is_config_true gui.autoexplore]} {
> do_explore
> }
>
> +# Clear "Initializing..." status
> +after 500 {$main_status show ""}
> +
> # Local variables:
> # mode: tcl
> # indent-tabs-mode: t
> diff --git a/lib/blame.tcl b/lib/blame.tcl
> index a1aeb8b96e..bfcacd5584 100644
> --- a/lib/blame.tcl
> +++ b/lib/blame.tcl
> @@ -24,6 +24,7 @@ field w_cviewer ; # pane showing commit message
> field finder ; # find mini-dialog frame
> field gotoline ; # line goto mini-dialog frame
> field status ; # status mega-widget instance
> +field status_operation ; # operation displayed by status mega-widget
> field old_height ; # last known height of $w.file_pane
>
>
> @@ -274,6 +275,7 @@ constructor new {i_commit i_path i_jump} {
> pack $w_cviewer -expand 1 -fill both
>
> set status [::status_bar::new $w.status]
> + set status_operation {}
>
> menu $w.ctxm -tearoff 0
> $w.ctxm add command \
> @@ -602,16 +604,23 @@ method _exec_blame {cur_w cur_d options cur_s} {
> } else {
> lappend options $commit
> }
> +
> + # We may recurse in from another call to _exec_blame and already have
> + # a status operation.
> + if {$status_operation == {}} {
> + set status_operation [$status start \
> + $cur_s \
> + [mc "lines annotated"]]
> + } else {
> + $status_operation show $cur_s
> + }
IIUC, in the previous version, a 'start' would reset the
progress/"meter". But this change only resets the label, not the actual
progress, which I think is what the caller wanted. So I think this
should be a full re-start instead.
> +
> lappend options -- $path
> set fd [eval git_read --nice blame $options]
> fconfigure $fd -blocking 0 -translation lf -encoding utf-8
> fileevent $fd readable [cb _read_blame $fd $cur_w $cur_d]
> set current_fd $fd
> set blame_lines 0
> -
> - $status start \
> - $cur_s \
> - [mc "lines annotated"]
> }
>
> method _read_blame {fd cur_w cur_d} {
> diff --git a/lib/choose_repository.tcl b/lib/choose_repository.tcl
> index 80f5a59bbb..1ea0c9f7b8 100644
> --- a/lib/choose_repository.tcl
> +++ b/lib/choose_repository.tcl
> @@ -9,6 +9,18 @@ field w_body ; # Widget holding the center content
> field w_next ; # Next button
> field w_quit ; # Quit button
> field o_cons ; # Console object (if active)
> +
> +# Status mega-widget instance during _do_clone2 (used by _copy_files and
> +# _link_files). Widget is destroyed before _do_clone2 calls
> +# _do_clone_checkout
> +field o_status
> +
> +# Operation displayed by status mega-widget during _do_clone_checkout =>
> +# _readtree_wait => _postcheckout_wait => _do_clone_submodules =>
> +# _do_validate_submodule_cloning. The status mega-widget is a difference
> +# instance than that stored in $o_status in earlier operations.
The last sentence doesn't make a lot of sense to me. What is "earlier
operations"? If this refers to previous versions of this file, then I
don't think such a comment belongs here. It should be in the commit
message instead.
> +field o_status_op
> +
> field w_types ; # List of type buttons in clone
> field w_recentlist ; # Listbox containing recent repositories
> field w_localpath ; # Entry widget bound to local_path
> @@ -659,12 +671,12 @@ method _do_clone2 {} {
>
> switch -exact -- $clone_type {
> hardlink {
> - set o_cons [status_bar::two_line $w_body]
> + set o_status [status_bar::two_line $w_body]
> pack $w_body -fill x -padx 10 -pady 10
>
> - $o_cons start \
> + set status_op [$o_status start \
> [mc "Counting objects"] \
> - [mc "buckets"]
> + [mc "buckets"]]
> update
>
> if {[file exists [file join $objdir info alternates]]} {
> @@ -689,6 +701,7 @@ method _do_clone2 {} {
> } err]} {
> catch {cd $pwd}
> _clone_failed $this [mc "Unable to copy objects/info/alternates: %s" $err]
> + $status_op stop
> return
> }
> }
> @@ -700,7 +713,7 @@ method _do_clone2 {} {
> -directory [file join $objdir] ??]
> set bcnt [expr {[llength $buckets] + 2}]
> set bcur 1
> - $o_cons update $bcur $bcnt
> + $status_op update $bcur $bcnt
> update
>
> file mkdir [file join .git objects pack]
> @@ -708,7 +721,7 @@ method _do_clone2 {} {
> -directory [file join $objdir pack] *] {
> lappend tolink [file join pack $i]
> }
> - $o_cons update [incr bcur] $bcnt
> + $status_op update [incr bcur] $bcnt
> update
>
> foreach i $buckets {
> @@ -717,10 +730,10 @@ method _do_clone2 {} {
> -directory [file join $objdir $i] *] {
> lappend tolink [file join $i $j]
> }
> - $o_cons update [incr bcur] $bcnt
> + $status_op update [incr bcur] $bcnt
> update
> }
> - $o_cons stop
> + $status_op stop
>
> if {$tolink eq {}} {
> info_popup [strcat \
> @@ -747,6 +760,8 @@ method _do_clone2 {} {
> if {!$i} return
>
> destroy $w_body
> +
> + set o_status {}
Should we be calling a destructor for this here? There is the '_delete'
method in status_bar.tcl, but I don't see any usages of it so I'm not
sure what exactly it is supposed to do.
That said, the previous version of this file doesn't call any sort of
destructor either, so maybe we should just leave it like it is for now.
I dunno.
> }
> full {
> set o_cons [console::embed \
> @@ -976,33 +1010,9 @@ method _do_clone_checkout {HEAD} {
> fileevent $fd readable [cb _readtree_wait $fd]
> }
>
> -method _do_validate_submodule_cloning {ok} {
> - if {$ok} {
> - $o_cons done $ok
> - set done 1
> - } else {
> - _clone_failed $this [mc "Cannot clone submodules."]
> - }
> -}
> -
> -method _do_clone_submodules {} {
> - if {$recursive eq {true}} {
> - destroy $w_body
> - set o_cons [console::embed \
> - $w_body \
> - [mc "Cloning submodules"]]
> - pack $w_body -fill both -expand 1 -padx 10
> - $o_cons exec \
> - [list git submodule update --init --recursive] \
> - [cb _do_validate_submodule_cloning]
> - } else {
> - set done 1
> - }
> -}
> -
Is there a reason for moving these two methods around? Not that its a
bad thing, I'm just curious.
> method _readtree_wait {fd} {
> set buf [read $fd]
> - $o_cons update_meter $buf
> + $o_status_op update_meter $buf
> append readtree_err $buf
>
> fconfigure $fd -blocking 1
Everything other than a couple of minor comments above looks good.
Thanks for the quality contribution. Looking forward to finally merging
the next and final version of the series :)
--
Regards,
Pratyush Yadav
There was a problem hiding this comment.
On the Git mailing list, Jonathan Gilbert wrote (reply to this):
On Sat, Nov 30, 2019 at 5:05 PM Pratyush Yadav me-at-yadavpratyush.com
|GitHub Public/Example Allow| <172q77k4bxwj0zt@sneakemail.com> wrote:
> Hi Jonathan,
>
> Thanks for the re-roll.
You are most welcome :-)
> On 28/11/19 08:30AM, Jonathan Gilbert via GitGitGadget wrote:
> > +# Operation displayed by status mega-widget during _do_clone_checkout =>
> > +# _readtree_wait => _postcheckout_wait => _do_clone_submodules =>
> > +# _do_validate_submodule_cloning. The status mega-widget is a difference
> > +# instance than that stored in $o_status in earlier operations.
>
> The last sentence doesn't make a lot of sense to me. What is "earlier
> operations"? If this refers to previous versions of this file, then I
> don't think such a comment belongs here. It should be in the commit
> message instead.
A clone starts out by calling `_do_clone2`, which, for `$clone_type`
of `hardlink`, creates a status "mega-widget" and uses it to track
linking and/or copying the underlying files. Then, this part of the UI
is destroyed. Later, the code calls into _do_clone_checkout, which
sets up its own, different view. This view _also_ uses a status
"mega-widget", but it's not the same one as before. This wasn't
obvious to me in my first read-through, and I erroneously wrote code
that assumed the widget objects would carry forward. As such, I felt
it might be useful to other readers to have this detail called out
up-front. In the context of `_do_clone_checkout`, the "earlier
operations" is what happens in `_do_clone2`.
> > destroy $w_body
> > +
> > + set o_status {}
>
> Should we be calling a destructor for this here? There is the '_delete'
> method in status_bar.tcl, but I don't see any usages of it so I'm not
> sure what exactly it is supposed to do.
>
> That said, the previous version of this file doesn't call any sort of
> destructor either, so maybe we should just leave it like it is for now.
> I dunno.
As far as I can tell, `destroy $w_body` automatically deletes the
entire subtree of UI components. I mentioned that I had written broken
code at first because I didn't realize the status widget got replaced
between `_do_clone2` and `_do_clone_checkout` -- that code encountered
an error that indicated that the status widget object no longer
existed at all. Thus, I have proceeded on the assumption that `destroy
$w_body` handles that particular detail, and all that's left is to
clear `o_status` of its dangling reference to the object that no
longer exists.
> > -method _do_validate_submodule_cloning {ok} {
> > [..]
> > -method _do_clone_submodules {} {
>
> Is there a reason for moving these two methods around? Not that its a
> bad thing, I'm just curious.
I touched on this in the cover letter. I'll just copy/paste that text
since it says it just as well as I could re-synthesize here :-)
* In `choose_repository.tcl`, there is a sequence of functions
involved performing the checkout on the clone: `_do_clone_checkout` =>
`_readtree_wait` => `_postcheckout_wait` => `_do_clone_submodules` =>
`_do_validate_submodule_cloning`. The functions have been re-ordered
in the source code to match the sequence in which they execute to
improve clarity.
Re-roll (final?) incoming.
Thanks,
Jonathan Gilbert
There was a problem hiding this comment.
On the Git mailing list, Philip Oakley wrote (reply to this):
On 30/11/2019 23:05, Pratyush Yadav wrote:
> On 28/11/19 08:30AM, Jonathan Gilbert via GitGitGadget wrote:
>> +# Operation displayed by status mega-widget during _do_clone_checkout =>
>> +# _readtree_wait => _postcheckout_wait => _do_clone_submodules =>
>> +# _do_validate_submodule_cloning. The status mega-widget is a difference
should this be "different", rather than 'difference'?
>> +# instance than that stored in $o_status in earlier operations.
> The last sentence doesn't make a lot of sense to me. What is "earlier
> operations"? If this refers to previous versions of this file, then I
> don't think such a comment belongs here. It should be in the commit
> message instead.
>
Philip
There was a problem hiding this comment.
On the Git mailing list, Jonathan Gilbert wrote (reply to this):
On Sun, Dec 1, 2019 at 5:43 AM Philip Oakley philipoakley-at-iee.email
|GitHub Public/Example Allow| <ogvdf9gsg7oxult@sneakemail.com> wrote:
> On 30/11/2019 23:05, Pratyush Yadav wrote:
> > On 28/11/19 08:30AM, Jonathan Gilbert via GitGitGadget wrote:
> >> +# Operation displayed by status mega-widget during _do_clone_checkout =>
> >> +# _readtree_wait => _postcheckout_wait => _do_clone_submodules =>
> >> +# _do_validate_submodule_cloning. The status mega-widget is a difference
>
> should this be "different", rather than 'difference'?
It absolutely should and I have corrected that in the re-roll.
Thanks :-)
Jonathan
Update the status bar to track updates as individual "operations" that can overlap. Update all call sites to interact with the new status bar mechanism. Update initialization to explicitly clear status text, since otherwise it may persist across future operations. Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
Update the revert_helper proc to check for untracked files as well as changes, and then handle changes to be reverted and untracked files with independent blocks of code. Prompt the user 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 remove the directory as well. Migrate unlocking of the index out of _close_updateindex to a responsibility of the caller, to permit paths that don't directly unlock the index, and refactor the error handling added in d4e890e so that callers can make flow control decisions in the event of errors. Update Tcl/Tk dependency from 8.4 to 8.6 in git-gui.sh. A new proc delete_files takes care of actually deleting the files in batches, 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. The `SimpleChord` class, based on TclOO (Tcl/Tk 8.6), is added in this commit. Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
logiclrd
force-pushed
the
git-gui-revert-untracked
branch
from
3388407
to
5f8120f
Compare
|
/submit |
|
Submitted as pull.436.v7.git.1575167313.gitgitgadget@gmail.com |
|
On the Git mailing list, Pratyush Yadav wrote (reply to this): |
|
GitGitGadget doesn't seem to auto-close PRs for git-gui. This was merged upstream as prati0100/git-gui@2763530 so this PR can be closed. |
@prati0100 GitGitGadget auto-closes PRs only when they hit |
|
Okeydoke :-) |
To be clear: this would eventually have happened with this PR, too ;-) |
|
Oh, okay :-) Should I re-open it, then, so we can watch that happen? 😁 |
|
Nah... 😀 |
| @@ -1797,10 +1797,10 @@ proc ui_status {msg} { | |||
| } | |||
There was a problem hiding this comment.
On the Git mailing list, Benjamin Poirier wrote (reply to this):
On 2019/12/01 02:28 +0000, Jonathan Gilbert via GitGitGadget wrote:
> From: Jonathan Gilbert <JonathanG@iQmetrix.com>
>
> Update the status bar to track updates as individual "operations" that
> can overlap. Update all call sites to interact with the new status bar
> mechanism. Update initialization to explicitly clear status text,
> since otherwise it may persist across future operations.
>
> Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
> ---
Hi Jonathan,
It appears that this change has caused a regression when using git-gui
blame <file> -> right click on a source line -> "Show History Context"
There is an "Application Error" window that appears with the following
details:
can't read "::main_status": no such variable
can't read "::main_status": no such variable
while executing
"$::main_status start [mc "Starting %s... please wait..." "gitk"]"
(procedure "do_gitk" line 55)
invoked from within
"do_gitk $cmdline"
(procedure "blame::_gitkcommit" line 47)
invoked from within
"blame::_gitkcommit ::blame::__o1::__d"
invoked from within
".ctxm invoke active"
("uplevel" body line 1)
invoked from within
"uplevel #0 [list $w invoke active]"
(procedure "tk::MenuInvoke" line 50)
invoked from within
"tk::MenuInvoke .ctxm 1"
(command bound to event)
The rest of the functionality seems unaffected but it's pretty annoying
to have to dismiss this message each time.
Can you please look into it?
There was a problem hiding this comment.
On the Git mailing list, Pratyush Yadav wrote (reply to this):
Hi Benjamin,
On 26/02/20 05:24PM, Benjamin Poirier wrote:
> On 2019/12/01 02:28 +0000, Jonathan Gilbert via GitGitGadget wrote:
> > From: Jonathan Gilbert <JonathanG@iQmetrix.com>
> >
> > Update the status bar to track updates as individual "operations" that
> > can overlap. Update all call sites to interact with the new status bar
> > mechanism. Update initialization to explicitly clear status text,
> > since otherwise it may persist across future operations.
> >
> > Signed-off-by: Jonathan Gilbert <JonathanG@iQmetrix.com>
> > ---
>
> Hi Jonathan,
>
> It appears that this change has caused a regression when using git-gui
> blame <file> -> right click on a source line -> "Show History Context"
>
> There is an "Application Error" window that appears with the following
> details:
> can't read "::main_status": no such variable
> can't read "::main_status": no such variable
> while executing
> "$::main_status start [mc "Starting %s... please wait..." "gitk"]"
> (procedure "do_gitk" line 55)
> invoked from within
> "do_gitk $cmdline"
> (procedure "blame::_gitkcommit" line 47)
> invoked from within
> "blame::_gitkcommit ::blame::__o1::__d"
> invoked from within
> ".ctxm invoke active"
> ("uplevel" body line 1)
> invoked from within
> "uplevel #0 [list $w invoke active]"
> (procedure "tk::MenuInvoke" line 50)
> invoked from within
> "tk::MenuInvoke .ctxm 1"
> (command bound to event)
>
> The rest of the functionality seems unaffected but it's pretty annoying
> to have to dismiss this message each time.
>
> Can you please look into it?
Does the following patch fix the problem? I will submit a proper patch
soon.
-----8<-----
diff --git a/git-gui.sh b/git-gui.sh
index f41ed2e..d939844 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -2205,11 +2205,13 @@ proc do_gitk {revs {is_submodule false}} {
set env(GIT_WORK_TREE) $_gitworktree
cd $pwd
- set status_operation [$::main_status \
- start \
- [mc "Starting %s... please wait..." "gitk"]]
+ if {[info exists main_status]} {
+ set status_operation [$::main_status \
+ start \
+ [mc "Starting %s... please wait..." "gitk"]]
- after 3500 [list $status_operation stop]
+ after 3500 [list $status_operation stop]
+ }
}
}
-----8<-----
--
Regards,
Pratyush Yadav
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
.gitignoreforever). 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_helperproc 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 procdelete_filestakes care of actually deleting the files, using the Tcler's Wiki recommended approach for keeping the UI responsive.Since the
checkout_indexanddelete_filescalls 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_indexanddelete_filescalls 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_updateindexproc contains error handling (added in d4e890e) that has the potential to interact badly withunlock_indexrunning at the completion of an async operation. I have refactored the procedure into separate procs_close_updateindexandrescan_on_error. Call sites that exercised the combined functionality also unlocked the index, so a combined procclose_and_unlock_indexcalls_close_updateindexand then eitherrescan_on_errororunlock_indexas appropriate. Call sites have been updated appropriately.The
revert_helperproc, with its overlapping operations, is an example of a call site that does not combine the close and unlock/rescan operations. Thecheckout_indexproc has been reworked to only call _close_updateindex, and to call a functor supplied by the caller to captures any errors that occur.revert_helperuses this to supply a lambda function that stashes the error within the chord's body namespace, so that it can then separately callrescan_on_errorwhen the chord is completed (orunlock_index, if no error was captured), which might be substantially aftercheckout_indexencounters its error. If it turns out that a rescan is called for, it is done once the deletion is complete.This is the eighth revision of this change, which differs from the seventh version in the following ways (most of which are in the second of the three commits, to do with the status bar rework):
The bump of the Tcl/Tk dependency from 8.4 to 8.6 now takes place in the third commit, where it is needed and whose commit message actually calls it out.
The
showmethod instatus_bar_operationhas been renamed torestart, and the meter is cleared. Also, the supplied message is set as the prefix for futureupdatecalls.The call site for
$status_operation showin blame.tcl has been corresponding changed to$status_operation restart.A typo has been corrected in a comment. :-)