Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

git-gui: revert untracked files by deleting them #436

Closed
wants to merge 3 commits into from
Closed

git-gui: revert untracked files by deleting them #436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
da1704c
git-gui: consolidate naming conventions
logiclrd Oct 30, 2019
8fe9dfc
git-gui: update status bar to track operations
logiclrd Oct 30, 2019
5f8120f
git-gui: revert untracked files by deleting them
logiclrd Nov 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 17 additions & 14 deletions git-gui.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ along with this program; if not, see <http://www.gnu.org/licenses/>.}]
##
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

## 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 \
Expand Down Expand Up @@ -1797,10 +1797,10 @@ proc ui_status {msg} {
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

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."]
}
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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]
}
}

Expand Down Expand Up @@ -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]
}
}

Expand Down Expand Up @@ -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
Expand Down
24 changes: 17 additions & 7 deletions lib/blame.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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 \
Expand Down Expand Up @@ -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 restart $cur_s
}

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} {
Expand Down Expand Up @@ -806,10 +815,11 @@ method _read_blame {fd cur_w cur_d} {
[mc "Loading original location annotations..."]
} else {
set current_fd {}
$status stop [mc "Annotation complete."]
$status_operation stop [mc "Annotation complete."]
set status_operation {}
}
} else {
$status update $blame_lines $total_lines
$status_operation update $blame_lines $total_lines
}
} ifdeleted { catch {close $fd} }

Expand Down Expand Up @@ -1124,7 +1134,7 @@ method _blameparent {} {
set diffcmd [list diff-tree --unified=0 $cparent $cmit -- $new_path]
}
if {[catch {set fd [eval git_read $diffcmd]} err]} {
$status stop [mc "Unable to display parent"]
$status_operation stop [mc "Unable to display parent"]
error_popup [strcat [mc "Error loading diff:"] "\n\n$err"]
return
}
Expand Down
15 changes: 8 additions & 7 deletions lib/checkout_op.tcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand All @@ -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
Expand All @@ -384,7 +385,7 @@ $err
return
}

$::main_status stop
$status_bar_operation stop
_after_readtree $this
}

Expand Down