git-gui: revert untracked files by deleting them

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 .gitignore forever). 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_helper proc 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 proc delete_files takes care of actually deleting the files, using the Tcler's Wiki recommended approach for keeping the UI responsive.

This is the second revision of this change, which differs from the first version in the following ways:

* The change is now based on git-gui/master.
* With one exception, all lines are at most 80 characters long. The exception has a string literal in it that pushes it to 82 characters. I think it would be messy to try to split it, and I got advice on #git-devel to just let it go to 82 characters.
* camelCase is eliminated. I eliminated it from existing code in a separate commit.
* try is no longer used anywhere. The code that cares about the result (had code in a catch after a try) uses [catch].
* Deletion of files and removal of empty directories is now handled by separate procs.
* The deletion of a large number of files does not block the UI during its execution any more.
* The revert_helper code no longer uses an epilogue of generic statements to be evaluated on exit.
* When deleting files, the UI is notified about the deletion directly instead of doing a full rescan.

Jonathan Gilbert (2):
git-gui: consolidate naming conventions
git-gui: revert untracked files by deleting them

 lib/index.tcl | 343 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 266 insertions(+), 77 deletions(-)

base-commit: b524f6b399c77b40c8bf2b6217585fde4731472a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-436%2Flogiclrd%2Fgit-gui-revert-untracked-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-436/logiclrd/git-gui-revert-untracked-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/436

gitgitgadget

From: Jonathan Gilbert <[email protected]>

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.

---
 lib/index.tcl | 92 +++++++++++++++++++++++++--------------------------
 1 file changed, 46 insertions(+), 46 deletions(-)

index e07b7a3762..28d4d2a54e 100644
@@ -56,15 +56,15 @@ proc _close_updateindex {fd after} {
 	uplevel #0 $after
 }
 
-proc update_indexinfo {msg pathList after} {
+proc update_indexinfo {msg path_list after} {
 	global update_index_cp
 
 	if {![lock_index update]} return
 
 	set update_index_cp 0
-	set pathList [lsort $pathList]
-	set totalCnt [llength $pathList]
-	set batch [expr {int($totalCnt * .01) + 1}]
+	set path_list [lsort $path_list]
+	set total_cnt [llength $path_list]
+	set batch [expr {int($total_cnt * .01) + 1}]
 	if {$batch > 25} {set batch 25}
 
 	$::main_status start $msg [mc "files"]

@@ -78,26 +78,26 @@ proc update_indexinfo {msg pathList after} {
 	fileevent $fd writable [list \
 		write_update_indexinfo \
 		$fd \
-		$pathList \
-		$totalCnt \
+		$path_list \
+		$total_cnt \
 		$batch \
 		$after \
 		]
 }
 
-proc write_update_indexinfo {fd pathList totalCnt batch after} {
+proc write_update_indexinfo {fd path_list total_cnt batch after} {
 	global update_index_cp
 	global file_states current_diff_path
 
-	if {$update_index_cp >= $totalCnt} {
+	if {$update_index_cp >= $total_cnt} {
 		_close_updateindex $fd $after
 		return
 	}
 
 	for {set i $batch} \
-		{$update_index_cp < $totalCnt && $i > 0} \
+		{$update_index_cp < $total_cnt && $i > 0} \
 		{incr i -1} {
-		set path [lindex $pathList $update_index_cp]
+		set path [lindex $path_list $update_index_cp]
 		incr update_index_cp
 
 		set s $file_states($path)

@@ -119,18 +119,18 @@ proc write_update_indexinfo {fd pathList totalCnt batch after} {
 		display_file $path $new
 	}
 
-	$::main_status update $update_index_cp $totalCnt
+	$::main_status update $update_index_cp $total_cnt
 }
 
-proc update_index {msg pathList after} {
+proc update_index {msg path_list after} {
 	global update_index_cp
 
 	if {![lock_index update]} return
 
 	set update_index_cp 0
-	set pathList [lsort $pathList]
-	set totalCnt [llength $pathList]
-	set batch [expr {int($totalCnt * .01) + 1}]
+	set path_list [lsort $path_list]
+	set total_cnt [llength $path_list]
+	set batch [expr {int($total_cnt * .01) + 1}]
 	if {$batch > 25} {set batch 25}
 
 	$::main_status start $msg [mc "files"]

@@ -144,26 +144,26 @@ proc update_index {msg pathList after} {
 	fileevent $fd writable [list \
 		write_update_index \
 		$fd \
-		$pathList \
-		$totalCnt \
+		$path_list \
+		$total_cnt \
 		$batch \
 		$after \
 		]
 }
 
-proc write_update_index {fd pathList totalCnt batch after} {
+proc write_update_index {fd path_list total_cnt batch after} {
 	global update_index_cp
 	global file_states current_diff_path
 
-	if {$update_index_cp >= $totalCnt} {
+	if {$update_index_cp >= $total_cnt} {
 		_close_updateindex $fd $after
 		return
 	}
 
 	for {set i $batch} \
-		{$update_index_cp < $totalCnt && $i > 0} \
+		{$update_index_cp < $total_cnt && $i > 0} \
 		{incr i -1} {
-		set path [lindex $pathList $update_index_cp]
+		set path [lindex $path_list $update_index_cp]
 		incr update_index_cp
 
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -190,18 +190,18 @@ proc write_update_index {fd pathList totalCnt batch after} {
 		display_file $path $new
 	}
 
-	$::main_status update $update_index_cp $totalCnt
+	$::main_status update $update_index_cp $total_cnt
 }
 
-proc checkout_index {msg pathList after} {
+proc checkout_index {msg path_list after} {
 	global update_index_cp
 
 	if {![lock_index update]} return
 
 	set update_index_cp 0
-	set pathList [lsort $pathList]
-	set totalCnt [llength $pathList]
-	set batch [expr {int($totalCnt * .01) + 1}]
+	set path_list [lsort $path_list]
+	set total_cnt [llength $path_list]
+	set batch [expr {int($total_cnt * .01) + 1}]
 	if {$batch > 25} {set batch 25}
 
 	$::main_status start $msg [mc "files"]

@@ -221,26 +221,26 @@ proc checkout_index {msg pathList after} {
 	fileevent $fd writable [list \
 		write_checkout_index \
 		$fd \
-		$pathList \
-		$totalCnt \
+		$path_list \
+		$total_cnt \
 		$batch \
 		$after \
 		]
 }
 
-proc write_checkout_index {fd pathList totalCnt batch after} {
+proc write_checkout_index {fd path_list total_cnt batch after} {
 	global update_index_cp
 	global file_states current_diff_path
 
-	if {$update_index_cp >= $totalCnt} {
+	if {$update_index_cp >= $total_cnt} {
 		_close_updateindex $fd $after
 		return
 	}
 
 	for {set i $batch} \
-		{$update_index_cp < $totalCnt && $i > 0} \
+		{$update_index_cp < $total_cnt && $i > 0} \
 		{incr i -1} {
-		set path [lindex $pathList $update_index_cp]
+		set path [lindex $path_list $update_index_cp]
 		incr update_index_cp
 		switch -glob -- [lindex $file_states($path) 0] {
 		U? {continue}

@@ -253,7 +253,7 @@ proc write_checkout_index {fd pathList totalCnt batch after} {
 		}
 	}
 
-	$::main_status update $update_index_cp $totalCnt
+	$::main_status update $update_index_cp $total_cnt
 }
 
 proc unstage_helper {txt paths} {

@@ -261,7 +261,7 @@ proc unstage_helper {txt paths} {
 
 	if {![lock_index begin-update]} return
 
-	set pathList [list]
+	set path_list [list]
 	set after {}
 	foreach path $paths {
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -269,19 +269,19 @@ proc unstage_helper {txt paths} {
 		M? -
 		T? -
 		D? {
-			lappend pathList $path
+			lappend path_list $path
 			if {$path eq $current_diff_path} {
 				set after {reshow_diff;}
 			}
 		}
 		}
 	}
-	if {$pathList eq {}} {
+	if {$path_list eq {}} {
 		unlock_index
 	} else {
 		update_indexinfo \
 			$txt \
-			$pathList \
+			$path_list \
 			[concat $after [list ui_ready]]
 	}
 }

@@ -305,7 +305,7 @@ proc add_helper {txt paths} {
 
 	if {![lock_index begin-update]} return
 
-	set pathList [list]
+	set path_list [list]
 	set after {}
 	foreach path $paths {
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -321,19 +321,19 @@ proc add_helper {txt paths} {
 		?M -
 		?D -
 		?T {
-			lappend pathList $path
+			lappend path_list $path
 			if {$path eq $current_diff_path} {
 				set after {reshow_diff;}
 			}
 		}
 		}
 	}
-	if {$pathList eq {}} {
+	if {$path_list eq {}} {
 		unlock_index
 	} else {
 		update_index \
 			$txt \
-			$pathList \
+			$path_list \
 			[concat $after {ui_status [mc "Ready to commit."]}]
 	}
 }

@@ -393,7 +393,7 @@ proc revert_helper {txt paths} {
 
 	if {![lock_index begin-update]} return
 
-	set pathList [list]
+	set path_list [list]
 	set after {}
 	foreach path $paths {
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -401,7 +401,7 @@ proc revert_helper {txt paths} {
 		?M -
 		?T -
 		?D {
-			lappend pathList $path
+			lappend path_list $path
 			if {$path eq $current_diff_path} {
 				set after {reshow_diff;}
 			}

@@ -420,12 +420,12 @@ proc revert_helper {txt paths} {
 	# as they have quite complex plural-form rules. Unfortunately,
 	# msgcat doesn't seem to support that kind of string translation.
 	#
-	set n [llength $pathList]
+	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 $pathList]]]
+		set query [mc "Revert changes in file %s?" [short_path [lindex $path_list]]]
 	} else {
 		set query [mc "Revert changes in these %i files?" $n]
 	}

@@ -444,7 +444,7 @@ proc revert_helper {txt paths} {
 	if {$reply == 1} {
 		checkout_index \
 			$txt \
-			$pathList \
+			$path_list \
 			[concat $after [list ui_ready]]
 	} else {
 		unlock_index

gitgitgadget

Hi Jonathan,

Thanks for the cleanup. Looks good.

Regards,
Pratyush Yadav

From: Jonathan Gilbert <[email protected]>

Updates the revert_helper procedure to also detect untracked files. If 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.

---
 lib/index.tcl | 255 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 222 insertions(+), 33 deletions(-)

index 28d4d2a54e..9661ddb556 100644
@@ -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
+	# 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 }
+
+	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
+		} 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 \
+			]]
 	} 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]
+		}
+
+		reshow_diff
+		ui_ready
+	}
+}
+
+# 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 lexists name {
+	expr {![catch {file lstat $name finfo}]}
+}
+
+# Remove as many empty directories as we can starting at the specified path.
+# 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]
 	}
 }
 

gitgitgadget

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?

Typo: s/Updates/Update/ ?

  proc lexists {name} {

Regards,
Pratyush Yadav

Possibly, but I think the implications of such a change in the core tool are far greater than UI features in Git Gui.

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. :-)

Fixed. :-)

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. :-)

[..]

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.

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

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." :-)

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." :-)

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

On Sun, Nov 3, 2019 at 1:48 AM Pratyush Yadav me-at-yadavpratyush.com

|GitHub Public/Example Allow| <[email protected]> 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 \
> > +                     ]]

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

Thanks,

Jonathan Gilbert

[0] https://wiki.tcl-lang.org/page/Keep+a+GUI+alive+during+a+long+calculation

in Git speak, that operation is called 'clean' (see 'git clean') why should we overload the 'revert' operation here?

Bert

It's less about overloading the 'revert' operation as overloading the UI action which is currently called "Revert". I think it would be a worse experience to have to activate a different option to remove unwanted files as to remove unwanted changes. Maybe the UI option could be renamed "Revert & Clean" or something?

As a side note, git clean untracked-file won't do anything with a default configuration, you have to explicitly -f it. Not sure if that's relevant, but it does feel like a higher barrier to entry than git revert.

Thanks,

Jonathan Gilbert

I disagree. There are valid workflows where you want to remove all changes to tracked files, but leave untracked ones alone. As an example, say you wrote a small script to fix some textual things, like your variable re-name patch. Now you run a diff before you commit those changes just to be sure, and notice that your script was overzealous and made some changes it shouldn't have. So, you clean up all tracked files, and give your script a fresh start. Here, you don't want to delete your script.

And in the other direction, say you want to delete all untracked files but have unstaged changes in your tracked files. Combining "Revert" and "Clean" does not give you an option to only delete untracked files. So you now either have to stash your changes, or run git clean from the command line.

git revert is different from our "Revert", though I admit the naming is quite confusing. git revert creates a new commit that "reverses" the changes made in an earlier commit(s). The important point to note here is that git revert is used when you publish some commits, and then realise later they had some bugs. Now you can't just drop those commits because that would re-write the history, and it would change all the commit hashes since that commit. So, you use git revert to create a new commit that _textually_ reverses those changes. The buggy commit still exists in the tree, but its changes don't.

In contrast, git-gui's "Revert" works on unstaged changes. It does not create a new commit. In fact, our revert does something similar to git checkout -- <file> (it uses git checkout-index to be precise).

So I don't think you should, or _can_, use git revert for what you want to do. And so, I don't see why it is being factored in with this discussion. Am I missing something?

Regards,
Pratyush Yadav

But, since this is in this GUI interface, you can clearly see which are which and select only the files you want to affect. If you have so many files that you have to select indiscriminately, then the command-line is probably a better choice anyway. In any case, my proposed change prompts for each part of the change, so you _can_ just select everything, press ^J, and then say "Yes" to only one of the prompts.

[..]

You are entirely correct, this was just a massive brain fart. Every time I wrote git revert in my head I was actually thinking of exactly what Git Gui does, reverting working copy changes by checking out the file. I should have written "reverting using git checkout". My apologies!

In my defence, I have over the past few days found myself digging into code hosted in SVN repositories, and svn revert does exactly what git checkout does to an unstaged modified file. :-)

Jonathan Gilbert

[Dropping "Jonathan Gilbert <[email protected]>" from the To: list because my mail server says "Domain not found". Putting the GGG address in To:
instead.]

Ah yes! Makes sense. I got too tunnel-visioned when thinking about this, and lost context. Sorry.

I have never used svn, but I can imagine now confusing that might be ;)

Regards,
Pratyush Yadav

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 .gitignore forever). 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_helper proc 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 proc delete_files takes care of actually deleting the files, 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.

This is the third revision of this change, which differs from the second version in the following ways:

* A new construct called a "chord" is used to coordinate the completion of multiple asynchronous operations that can be kicked off by revert_helper. A chord is, conceptually, a procedure with multiple entrypoints whose body only executes once all entrypoints have been activated. The chord.tcl file includes comprehensive documentation of how to use the chord classes.

* Since we might not yet be ready to unlock the index when checkout_index returns, the _close_updateindex proc where it was ultimately unlocking the index has been modified so that unlocking the index is the responsibility of the caller. Since the $after functionality ran after unlock_index, that is also hoisted out. Nothing in _close_updateindex appears to be asynchronous, so the caller can simply make the calls itself upon its return.

* lexists has been renamed to path_exists.

* Up to 10 deletion errors are now shown simultaneously. I also confirmed that Tcl's file delete code will always return a nicely-formatted error including the filename, and changed the message so that it isn't also injecting the filename.

Jonathan Gilbert (2):
git-gui: consolidate naming conventions
git-gui: revert untracked files by deleting them

 lib/chord.tcl | 137 ++++++++++++++++++
 lib/index.tcl | 376 +++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 429 insertions(+), 84 deletions(-)
 create mode 100644 lib/chord.tcl

base-commit: b524f6b399c77b40c8bf2b6217585fde4731472a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-436%2Flogiclrd%2Fgit-gui-revert-untracked-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-436/logiclrd/git-gui-revert-untracked-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/436

Range-diff vs v1:

 1:  da1704c56e = 1:  da1704c56e git-gui: consolidate naming conventions
 2:  0190f6f2f9 ! 2:  9469beb599 git-gui: revert untracked files by deleting them
     @@ -2,24 +2,233 @@
      
          git-gui: revert untracked files by deleting them
      
     -    Updates the revert_helper procedure to also detect untracked files. If
     -    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.
     +    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 <[email protected]>
      
     + diff --git a/lib/chord.tcl b/lib/chord.tcl
     + new file mode 100644
     + --- /dev/null
     + +++ b/lib/chord.tcl
     +@@
     ++# 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.
     ++#
     ++# 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 {
     ++	variable Notes
     ++	variable Body
     ++	variable IsCompleted
     ++
     ++	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 {} {
     ++		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 {} {
     ++		if {!$IsActivated} {
     ++			set IsActivated 1
     ++			$Chord notify_note_activation
     ++		}
     ++	}
     ++}
     +
       diff --git a/lib/index.tcl b/lib/index.tcl
       --- a/lib/index.tcl
       +++ b/lib/index.tcl
     +@@
     + 	}
     + }
     + 
     +-proc _close_updateindex {fd after} {
     ++proc _close_updateindex {fd} {
     + 	global use_ttk NS
     + 	fconfigure $fd -blocking 1
     + 	if {[catch {close $fd} err]} {
     +@@
     + 	}
     + 
     + 	$::main_status stop
     +-	unlock_index
     +-	uplevel #0 $after
     + }
     + 
     + proc update_indexinfo {msg path_list 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
     + 	}
     + 
     +@@
     + 	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
     + 	}
     + 
     +@@
     + 	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
     + 		return
     + 	}
     + 
      @@
       
       	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
     -+	# index is still locked, we need to unlock it before returning.
     -+	set need_unlock_index 1
     ++	# Common "after" functionality that waits until multiple asynchronous
     ++	# operations are complete (by waiting for them to activate their notes
     ++	# on the chord).
     ++	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]
     @@ -33,7 +242,12 @@
       		?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 }
     + 			}
     + 		}
       		}
       	}
       
     @@ -110,14 +324,10 @@
      +			checkout_index \
      +				$txt \
      +				$path_list \
     -+				[concat $after [list ui_ready]]
     -+
     -+			set need_unlock_index 0
     ++				[$after_chord add_note]
      +		}
      +	}
      +
     -+	if {$need_unlock_index} { unlock_index }
     -+
      +	if {$untracked_cnt > 0} {
      +		# Split question between singular and plural cases, because
      +		# such distinction is needed in some languages.
     @@ -152,35 +362,40 @@
      +			]
      +
      +		if {$reply == 1} {
     -+			delete_files $untracked_list
     ++			$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} {
     ++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 deletion_error_path "not yet captured"
      +	set batch_size 50
      +
      +	delete_helper \
      +		$path_list \
      +		$path_index \
      +		$deletion_errors \
     -+		$deletion_error_path \
     -+		$batch_size
     ++		$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 deletion_error_path batch_size} {
     ++proc delete_helper {path_list path_index deletion_errors batch_size after} {
      +	global file_states
      +
      +	set path_cnt [llength $path_list]
     @@ -195,17 +410,13 @@
      +		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
     ++			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 {![lexists $path]} {
     ++			if {![path_exists $path]} {
      +				unset file_states($path)
      +				display_file $path __
      +			}
     @@ -231,8 +442,8 @@
      -			[concat $after [list ui_ready]]
      +			$path_index \
      +			$deletion_errors \
     -+			$deletion_error_path \
      +			$batch_size \
     ++			$after
      +			]]
       	} else {
      -		unlock_index
     @@ -242,27 +453,33 @@
      +		# 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] \
     -+				]
     ++		if {($deletion_error_cnt > 0) && ($deletion_error_cnt <= [MAX_VERBOSE_FILES_IN_DELETION_ERROR])} {
     ++			set error_text "Encountered errors deleting files:\n"
     ++
     ++			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 selected files could be deleted." \
     ++				"None of the %d selected files could be deleted." \
     ++				$path_cnt \
      +				]
      +		} elseif {$deletion_error_cnt > 1} {
      +			error_popup [mc \
     -+				"%d of the selected files could not be deleted." \
     -+				$deletion_error_cnt]
     ++				"%d of the %d selected files could not be deleted." \
     ++				$deletion_error_cnt \
     ++				$path_cnt \
     ++				]
      +		}
      +
     -+		reshow_diff
     -+		ui_ready
     ++		uplevel #0 $after
      +	}
      +}
      +
     ++proc MAX_VERBOSE_FILES_IN_DELETION_ERROR {} { return 10; }
     ++
      +# This function is from the TCL documentation:
      +#
      +#   https://wiki.tcl-lang.org/page/file+exists
     @@ -270,14 +487,15 @@
      +# [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 {
     ++proc path_exists {name} {
      +	expr {![catch {file lstat $name finfo}]}
      +}
      +
     -+# Remove as many empty directories as we can starting at the specified path.
     -+# 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.
     ++# 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]
      +

gitgitgadget

From: Jonathan Gilbert <[email protected]>

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.

---
 lib/index.tcl | 92 +++++++++++++++++++++++++--------------------------
 1 file changed, 46 insertions(+), 46 deletions(-)

index e07b7a3762..28d4d2a54e 100644
@@ -56,15 +56,15 @@ proc _close_updateindex {fd after} {
 	uplevel #0 $after
 }
 
-proc update_indexinfo {msg pathList after} {
+proc update_indexinfo {msg path_list after} {
 	global update_index_cp
 
 	if {![lock_index update]} return
 
 	set update_index_cp 0
-	set pathList [lsort $pathList]
-	set totalCnt [llength $pathList]
-	set batch [expr {int($totalCnt * .01) + 1}]
+	set path_list [lsort $path_list]
+	set total_cnt [llength $path_list]
+	set batch [expr {int($total_cnt * .01) + 1}]
 	if {$batch > 25} {set batch 25}
 
 	$::main_status start $msg [mc "files"]

@@ -78,26 +78,26 @@ proc update_indexinfo {msg pathList after} {
 	fileevent $fd writable [list \
 		write_update_indexinfo \
 		$fd \
-		$pathList \
-		$totalCnt \
+		$path_list \
+		$total_cnt \
 		$batch \
 		$after \
 		]
 }
 
-proc write_update_indexinfo {fd pathList totalCnt batch after} {
+proc write_update_indexinfo {fd path_list total_cnt batch after} {
 	global update_index_cp
 	global file_states current_diff_path
 
-	if {$update_index_cp >= $totalCnt} {
+	if {$update_index_cp >= $total_cnt} {
 		_close_updateindex $fd $after
 		return
 	}
 
 	for {set i $batch} \
-		{$update_index_cp < $totalCnt && $i > 0} \
+		{$update_index_cp < $total_cnt && $i > 0} \
 		{incr i -1} {
-		set path [lindex $pathList $update_index_cp]
+		set path [lindex $path_list $update_index_cp]
 		incr update_index_cp
 
 		set s $file_states($path)

@@ -119,18 +119,18 @@ proc write_update_indexinfo {fd pathList totalCnt batch after} {
 		display_file $path $new
 	}
 
-	$::main_status update $update_index_cp $totalCnt
+	$::main_status update $update_index_cp $total_cnt
 }
 
-proc update_index {msg pathList after} {
+proc update_index {msg path_list after} {
 	global update_index_cp
 
 	if {![lock_index update]} return
 
 	set update_index_cp 0
-	set pathList [lsort $pathList]
-	set totalCnt [llength $pathList]
-	set batch [expr {int($totalCnt * .01) + 1}]
+	set path_list [lsort $path_list]
+	set total_cnt [llength $path_list]
+	set batch [expr {int($total_cnt * .01) + 1}]
 	if {$batch > 25} {set batch 25}
 
 	$::main_status start $msg [mc "files"]

@@ -144,26 +144,26 @@ proc update_index {msg pathList after} {
 	fileevent $fd writable [list \
 		write_update_index \
 		$fd \
-		$pathList \
-		$totalCnt \
+		$path_list \
+		$total_cnt \
 		$batch \
 		$after \
 		]
 }
 
-proc write_update_index {fd pathList totalCnt batch after} {
+proc write_update_index {fd path_list total_cnt batch after} {
 	global update_index_cp
 	global file_states current_diff_path
 
-	if {$update_index_cp >= $totalCnt} {
+	if {$update_index_cp >= $total_cnt} {
 		_close_updateindex $fd $after
 		return
 	}
 
 	for {set i $batch} \
-		{$update_index_cp < $totalCnt && $i > 0} \
+		{$update_index_cp < $total_cnt && $i > 0} \
 		{incr i -1} {
-		set path [lindex $pathList $update_index_cp]
+		set path [lindex $path_list $update_index_cp]
 		incr update_index_cp
 
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -190,18 +190,18 @@ proc write_update_index {fd pathList totalCnt batch after} {
 		display_file $path $new
 	}
 
-	$::main_status update $update_index_cp $totalCnt
+	$::main_status update $update_index_cp $total_cnt
 }
 
-proc checkout_index {msg pathList after} {
+proc checkout_index {msg path_list after} {
 	global update_index_cp
 
 	if {![lock_index update]} return
 
 	set update_index_cp 0
-	set pathList [lsort $pathList]
-	set totalCnt [llength $pathList]
-	set batch [expr {int($totalCnt * .01) + 1}]
+	set path_list [lsort $path_list]
+	set total_cnt [llength $path_list]
+	set batch [expr {int($total_cnt * .01) + 1}]
 	if {$batch > 25} {set batch 25}
 
 	$::main_status start $msg [mc "files"]

@@ -221,26 +221,26 @@ proc checkout_index {msg pathList after} {
 	fileevent $fd writable [list \
 		write_checkout_index \
 		$fd \
-		$pathList \
-		$totalCnt \
+		$path_list \
+		$total_cnt \
 		$batch \
 		$after \
 		]
 }
 
-proc write_checkout_index {fd pathList totalCnt batch after} {
+proc write_checkout_index {fd path_list total_cnt batch after} {
 	global update_index_cp
 	global file_states current_diff_path
 
-	if {$update_index_cp >= $totalCnt} {
+	if {$update_index_cp >= $total_cnt} {
 		_close_updateindex $fd $after
 		return
 	}
 
 	for {set i $batch} \
-		{$update_index_cp < $totalCnt && $i > 0} \
+		{$update_index_cp < $total_cnt && $i > 0} \
 		{incr i -1} {
-		set path [lindex $pathList $update_index_cp]
+		set path [lindex $path_list $update_index_cp]
 		incr update_index_cp
 		switch -glob -- [lindex $file_states($path) 0] {
 		U? {continue}

@@ -253,7 +253,7 @@ proc write_checkout_index {fd pathList totalCnt batch after} {
 		}
 	}
 
-	$::main_status update $update_index_cp $totalCnt
+	$::main_status update $update_index_cp $total_cnt
 }
 
 proc unstage_helper {txt paths} {

@@ -261,7 +261,7 @@ proc unstage_helper {txt paths} {
 
 	if {![lock_index begin-update]} return
 
-	set pathList [list]
+	set path_list [list]
 	set after {}
 	foreach path $paths {
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -269,19 +269,19 @@ proc unstage_helper {txt paths} {
 		M? -
 		T? -
 		D? {
-			lappend pathList $path
+			lappend path_list $path
 			if {$path eq $current_diff_path} {
 				set after {reshow_diff;}
 			}
 		}
 		}
 	}
-	if {$pathList eq {}} {
+	if {$path_list eq {}} {
 		unlock_index
 	} else {
 		update_indexinfo \
 			$txt \
-			$pathList \
+			$path_list \
 			[concat $after [list ui_ready]]
 	}
 }

@@ -305,7 +305,7 @@ proc add_helper {txt paths} {
 
 	if {![lock_index begin-update]} return
 
-	set pathList [list]
+	set path_list [list]
 	set after {}
 	foreach path $paths {
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -321,19 +321,19 @@ proc add_helper {txt paths} {
 		?M -
 		?D -
 		?T {
-			lappend pathList $path
+			lappend path_list $path
 			if {$path eq $current_diff_path} {
 				set after {reshow_diff;}
 			}
 		}
 		}
 	}
-	if {$pathList eq {}} {
+	if {$path_list eq {}} {
 		unlock_index
 	} else {
 		update_index \
 			$txt \
-			$pathList \
+			$path_list \
 			[concat $after {ui_status [mc "Ready to commit."]}]
 	}
 }

@@ -393,7 +393,7 @@ proc revert_helper {txt paths} {
 
 	if {![lock_index begin-update]} return
 
-	set pathList [list]
+	set path_list [list]
 	set after {}
 	foreach path $paths {
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -401,7 +401,7 @@ proc revert_helper {txt paths} {
 		?M -
 		?T -
 		?D {
-			lappend pathList $path
+			lappend path_list $path
 			if {$path eq $current_diff_path} {
 				set after {reshow_diff;}
 			}

@@ -420,12 +420,12 @@ proc revert_helper {txt paths} {
 	# as they have quite complex plural-form rules. Unfortunately,
 	# msgcat doesn't seem to support that kind of string translation.
 	#
-	set n [llength $pathList]
+	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 $pathList]]]
+		set query [mc "Revert changes in file %s?" [short_path [lindex $path_list]]]
 	} else {
 		set query [mc "Revert changes in these %i files?" $n]
 	}

@@ -444,7 +444,7 @@ proc revert_helper {txt paths} {
 	if {$reply == 1} {
 		checkout_index \
 			$txt \
-			$pathList \
+			$path_list \
 			[concat $after [list ui_ready]]
 	} else {
 		unlock_index

gitgitgadget

From: Jonathan Gilbert <[email protected]>

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.

---
 lib/chord.tcl | 137 ++++++++++++++++++++++++
 lib/index.tcl | 288 +++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 385 insertions(+), 40 deletions(-)
 create mode 100644 lib/chord.tcl

new file mode 100644
index 0000000000..2d13af14fc
@@ -0,0 +1,137 @@
+# 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.
+#
+# 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 {
+	variable Notes
+	variable Body
+	variable IsCompleted
+
+	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 {} {
+		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 {} {
+		if {!$IsActivated} {
+			set IsActivated 1
+			$Chord notify_note_activation
+		}
+	}
+}

gitgitgadget

Hi Jonathan,

Thanks for the re-roll. Some comments below. Apart from those comments, this looks close to good enough for merging :)

Regards,
Pratyush Yadav

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?

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?

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?

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. :-)

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.

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

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

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

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.

It is indeed, oops!

Intentional, yes. I'll see if there's a concise way to document this.

Okeydoke.

Good catch.

Will do.

Ah, yes, I did that with most messages, this was an oversight.

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.

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

sounds like "Currying" a function but with the parameters taken in any order, though, in a sense, perhaps not generating intermediate functions...

Philip

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.

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

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

Hi Jonathan,

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.

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.

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.

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.

Yes, a comment explaining it is the default would be nice.

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.

Makes sense.

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

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?

Makes sense.

[0] https://wiki.tcl-lang.org/page/MeTOO

Regards,
Pratyush Yadav

The cover letter is so much more descriptive than the commit message. It would be nice to have all this context and commentary in the commit message. Of course, you'd remove the "personal workflow" bit and some other stuff, but most of this can be copied verbatim.

Also, like I mentioned in the review of your second patch, _close_updateindex _does_ have an asynchronous component unfortunately.

Regards,
Pratyush Yadav

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 .gitignore forever). 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_helper proc 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 proc delete_files takes care of actually deleting the files, 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 _close_updateindex proc contains error handling (added in d4e890e5) that has the potential to interact badly with unlock_index running at the completion of an async operation. I have refactored the procedure into separate procs _close_updateindex and rescan_on_error, and the original functionality is captured with _close_updateindex_rescan_on_error. Call sites have been updated appropriately, and checkout_index has been reworked to take a functor that captures any errors that occur in a caller-defined way. revert_helper uses this to supply a lambda function that stashes the error within the chord's body namespace, so that it can call rescan_on_error when the chord is completed, which might be substantially after checkout_index encounters its error. If a rescan is called for, it is done once the deletion is complete.

This is the fourth revision of this change, which differs from the third version in the following ways:

* Merged some of the wording from the cover letter into the commit message.

* The Tcl/Tk dependency is updated to 8.6.

* The chord.tcl documentation has been reworked, moving the mechanistic class/member documentation next to the declarations, and including a more "human-readable" holistic explanation at the top.

* Class variables defined within chord.tcl have snake_case names. When I made this switch, I saw one possible reason why the convention from the Tcl documentation was different: it's quite common to have a parameter to the constructor with the same name as a class variable, and if the names are literally identical then they conflict in the namespace (e.g. set body $body).

* Removed unnecessary variable from revert_helper in index.tcl, added comments and split long line per feedback.

* _close_updateindex's error handling is split out to new proc rescan_on_error, and _close_updateindex_rescan_on_error calls rescan_on_error if it catches an error from _close_updateindex. Most call sites are updated to call the wrapper function, but the split is crucial for error handling within revert_helper.

* checkout_index uses the _close_updateindex without the error handler, and instead takes a functor from the caller that allows the caller to do whatever it wants with any errors that occur (but without abnormally terminating the execution of checkout_index the way throwing an error would).

* revert_helper is where the most meaningful change is. The $after_chord body now checks for a stashed error, and if there is one, it calls rescan_on_error. I create a lambda function $capture_error that takes whatever error message is passed to it and stashes it in $after_chord's namespace. Then, this is passed to checkout_index. The result is that if an error happens closing the index, the error message is passed back up, and then the checkout note is activated, but the $after_chord body doesn't execute until the deletion note is also activated, delaying the rescan until after the deletion is complete too.

TclOO ships by default with Tcl 8.6 and above. So, I'm not really sure if we need this.

I couldn't find any evidence that it is required in my testing.

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

It looks like the "treat an object as a method" functionality that the unknown method provides is not easily duplicated with class.tcl. It would be possible to just replace it with a named method, but the code using it wouldn't look as nice. Also, not that it matters in this instance, but purely as a matter of principle, from what I've read, it seems that TclOO is significantly supported by native code in the runtime and has much better performance and far less overhead than all pre-8.6 OO solutions. This suggests that a long-term goal of eliminating class.tcl might not be a bad idea. I haven't seen any way that having chord.tcl use TclOO could interfere with other, unrelated things using class.tcl.

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.

It seems that there isn't any way in TclOO to get into a class without it having a public "door", whether it's the method itself, or some "accessor" method that returns its [my] functor. You can choose to unexport a method, but once you do that, there doesn't appear to exist any way at all to override this for just one call site.

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.

I did some testing and discovered that I was mistaken, if unknown has no parameters then it cannot receive calls to unknown method names, these generate errors. As written, it is only capable of processing calls against the object itself.

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 somethingreally bad happening.

[..]

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.

I came up with a fairly concise way to defer the rescan until all async operations are completed, by splitting the error handling out into a separate method and then making this flow call that method from the $after_chord body.

Jonathan Gilbert (2):
git-gui: consolidate naming conventions
git-gui: revert untracked files by deleting them

 git-gui.sh    |   4 +-
 lib/chord.tcl | 160 ++++++++++++++++
 lib/index.tcl | 500 +++++++++++++++++++++++++++++++++++++-------------
 3 files changed, 538 insertions(+), 126 deletions(-)
 create mode 100644 lib/chord.tcl

base-commit: b524f6b399c77b40c8bf2b6217585fde4731472a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-436%2Flogiclrd%2Fgit-gui-revert-untracked-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-436/logiclrd/git-gui-revert-untracked-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/436

Range-diff vs v2:

 1:  da1704c56e = 1:  da1704c56e git-gui: consolidate naming conventions
 2:  9469beb599 ! 2:  dc12c1668d git-gui: revert untracked files by deleting them
     @@ -2,66 +2,72 @@
      
          git-gui: revert untracked files by deleting them
      
     -    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.
     +    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.
      
          Signed-off-by: Jonathan Gilbert <[email protected]>
      
     + diff --git a/git-gui.sh b/git-gui.sh
     + --- a/git-gui.sh
     + +++ b/git-gui.sh
     +@@
     + ##
     + ## 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 \
     +
       diff --git a/lib/chord.tcl b/lib/chord.tcl
       new file mode 100644
       --- /dev/null
       +++ b/lib/chord.tcl
      @@
     -+# 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.
     ++# Simple Chord for Tcl
      +#
     -+#   Constructor:
     -+#     Instances of ChordNote are created internally by calling add_note on
     -+#     SimpleChord objects.
     ++# A "chord" is a method with more than one entrypoint and only one body, such
     ++# that the body runs only once all the entrypoints have been called by
     ++# different asynchronous tasks. In this implementation, the chord is defined
     ++# dynamically for each invocation. A SimpleChord object is created, supplying
     ++# body script to be run when the chord is completed, and then one or more notes
     ++# are added to the chord. Each note can be called like a proc, and returns
     ++# immediately if the chord isn't yet complete. When the last remaining note is
     ++# called, the body runs before the note returns.
      +#
     -+#   Methods:
     -+#     [$note is_activated]
     -+#       Returns true if this note has already been activated.
     ++# The SimpleChord class has a constructor that takes the body script, and a
     ++# method add_note that returns a note object. Since the body script does not
     ++# run in the context of the procedure that defined it, a mechanism is provided
     ++# for injecting variables into the chord for use by the body script. The
     ++# activation of a note is idempotent; multiple calls have the same effect as
     ++# a simple call.
      +#
     -+#     $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.
     ++# If you are invoking asynchronous operations with chord notes as completion
     ++# callbacks, and there is a possibility that earlier operations could complete
     ++# before later ones are started, it is a good practice to create a "common"
     ++# note on the chord that prevents it from being complete until you're certain
     ++# you've added all the notes you need.
      +#
      +# Example:
      +#
     @@ -97,61 +103,105 @@
      +# 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.
      +
     ++# 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".
      +oo::class create SimpleChord {
     -+	variable Notes
     -+	variable Body
     -+	variable IsCompleted
     ++	variable notes body is_completed
      +
     ++	# 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.
      +	constructor {body} {
     -+		set Notes [list]
     -+		set Body $body
     -+		set IsCompleted 0
     ++		set notes [list]
     ++		my eval [list set body $body]
     ++		set is_completed 0
      +	}
      +
     ++	# Method:
     ++	#   $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.
      +	method eval {script} {
      +		namespace eval [self] $script
      +	}
      +
     ++	# Method:
     ++	#   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.
      +	method add_note {} {
     -+		if {$IsCompleted} { error "Cannot add a note to a completed chord" }
     ++		if {$is_completed} { error "Cannot add a note to a completed chord" }
      +
      +		set note [ChordNote new [self]]
      +
     -+		lappend Notes $note
     ++		lappend notes $note
      +
      +		return $note
      +	}
      +
     ++	# This method is for internal use only and is intentionally undocumented.
      +	method notify_note_activation {} {
     -+		if {!$IsCompleted} {
     -+			foreach note $Notes {
     ++		if {!$is_completed} {
     ++			foreach note $notes {
      +				if {![$note is_activated]} { return }
      +			}
      +
     -+			set IsCompleted 1
     ++			set is_completed 1
      +
     -+			namespace eval [self] $Body
     ++			namespace eval [self] $body
      +			namespace delete [self]
      +		}
      +	}
      +}
      +
     ++# 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.
      +oo::class create ChordNote {
     -+	variable Chord IsActivated
     ++	variable chord is_activated
      +
     ++	# Constructor:
     ++	#   Instances of ChordNote are created internally by calling add_note on
     ++	#   SimpleChord objects.
      +	constructor {chord} {
     -+		set Chord $chord
     -+		set IsActivated 0
     ++		my eval set chord $chord
     ++		set is_activated 0
      +	}
      +
     ++	# Method:
     ++	#   [$note is_activated]
     ++	#     Returns true if this note has already been activated.
      +	method is_activated {} {
     -+		return $IsActivated
     ++		return $is_activated
      +	}
      +
     ++	# Method:
     ++	#   $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.
     ++	#
     ++	# NB: In TclOO, if an object is invoked like a method without supplying
     ++	#     any method name, then this internal method `unknown` is what
     ++	#     actually runs (with no parameters). It is used in the ChordNote
     ++	#     class for the purpose of allowing the note object to be called as
     ++	#     a function (see example above). (The `unknown` method can also be
     ++	#     used to support dynamic dispatch, but must take parameters to
     ++	#     identify the "unknown" method to be invoked. In this form, this
     ++	#     proc serves only to make instances behave directly like methods.)
      +	method unknown {} {
     -+		if {!$IsActivated} {
     -+			set IsActivated 1
     -+			$Chord notify_note_activation
     ++		if {!$is_activated} {
     ++			set is_activated 1
     ++			$chord notify_note_activation
      +		}
      +	}
      +}
     @@ -164,16 +214,103 @@
       }
       
      -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} {
     - 	global use_ttk NS
       	fconfigure $fd -blocking 1
     - 	if {[catch {close $fd} err]} {
     -@@
     - 	}
     +-	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
     ++}
     + 
     +-		$::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
     --	unlock_index
     + 	unlock_index
      -	uplevel #0 $after
     ++	rescan ui_ready 0
       }
       
       proc update_indexinfo {msg path_list after} {
     @@ -182,8 +319,10 @@
       
       	if {$update_index_cp >= $total_cnt} {
      -		_close_updateindex $fd $after
     -+		_close_updateindex $fd
     -+		unlock_index
     ++		if {[_close_updateindex_rescan_on_error $fd]} {
     ++			unlock_index
     ++		}
     ++
      +		uplevel #0 $after
       		return
       	}
     @@ -193,37 +332,86 @@
       
       	if {$update_index_cp >= $total_cnt} {
      -		_close_updateindex $fd $after
     -+		_close_updateindex $fd
     -+		unlock_index
     ++		if {[_close_updateindex_rescan_on_error $fd]} {
     ++			unlock_index
     ++		}
     ++
      +		uplevel #0 $after
       		return
       	}
       
      @@
     + 	$::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
     +@@
     + 		$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
     -+		_close_updateindex $fd $do_unlock_index $after
     ++		if {[catch {_close_updateindex $fd} err]} {
     ++			uplevel #0 $capture_error [list $err]
     ++		}
     ++
      +		uplevel #0 $after
     ++
       		return
       	}
       
      @@
     + 	add_helper [mc "Adding all changed files"] $paths
     + }
     + 
     ++# Copied from TclLib package "lambda".
     ++proc lambda {arguments body args} {
     ++	return [list ::apply [list $arguments $body] {*}$args]
     ++}
     ++
     + proc revert_helper {txt paths} {
     + 	global file_states current_diff_path
       
       	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).
     ++	#
     ++	# The asynchronous operations are each indicated below by a comment
     ++	# before the code block that starts the async operation.
      +	set after_chord [SimpleChord new {
      +		unlock_index
     -+		if {$should_reshow_diff} { reshow_diff }
     -+		ui_ready
     ++		if {[string trim $err] != ""} {
     ++			rescan_on_error $err
     ++		} else {
     ++			if {$should_reshow_diff} { reshow_diff }
     ++			ui_ready
     ++		}
      +	}]
      +
      +	$after_chord eval { set should_reshow_diff 0 }
      +
     ++	# This function captures an error for processing when after_chord is
     ++	# completed. (The chord is curried into the lambda function.)
     ++	set capture_error [lambda \
     ++		{chord error} \
     ++		{ $chord eval [list set err $error] } \
     ++		$after_chord]
     ++
      +	# 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
     @@ -231,8 +419,9 @@
      +	set after_common_note [$after_chord add_note]
      +
       	set path_list [list]
     +-	set after {}
      +	set untracked_list [list]
     - 	set after {}
     ++
       	foreach path $paths {
       		switch -glob -- [lindex $file_states($path) 0] {
       		U? {continue}
     @@ -253,26 +442,9 @@
       
      +	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]
     --	}
     ++
     ++	# Asynchronous operation: revert changes by checking them out afresh
     ++	# from the index.
      +	if {$path_cnt > 0} {
      +		# Split question between singular and plural cases, because
      +		# such distinction is needed in some languages. Previously, the
     @@ -295,25 +467,31 @@
      +				"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 \
     +-	# 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]
     ++[mc "Any unstaged changes will be permanently lost by the revert."]" \
      +			question \
      +			1 \
      +			[mc "Do Nothing"] \
     @@ -324,10 +502,16 @@
      +			checkout_index \
      +				$txt \
      +				$path_list \
     -+				[$after_chord add_note]
     ++				[$after_chord add_note] \
     ++				$capture_error
      +		}
     -+	}
     -+
     + 	}
     + 
     +-	set reply [tk_dialog \
     +-		.confirm_revert \
     +-		"[appname] ([reponame])" \
     +-		"$query
     ++	# Asynchronous operation: Deletion of untracked files.
      +	if {$untracked_cnt > 0} {
      +		# Split question between singular and plural cases, because
      +		# such distinction is needed in some languages.
     @@ -348,7 +532,16 @@
      +				$untracked_cnt \
      +				]
      +		}
     -+
     + 
     +-[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 \
      +		set reply [tk_dialog \
      +			.confirm_revert \
      +			"[appname] ([reponame])" \
     @@ -453,8 +646,9 @@
      +		# 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])} {
     -+			set error_text "Encountered errors deleting files:\n"
     ++		if {($deletion_error_cnt > 0)
     ++		 && ($deletion_error_cnt <= [MAX_VERBOSE_FILES_IN_DELETION_ERROR])} {
     ++			set error_text [mc "Encountered errors deleting files:\n"]
      +
      +			foreach deletion_error $deletion_errors {
      +				append error_text "* [lindex $deletion_error 0]\n"

gitgitgadget

From: Jonathan Gilbert <[email protected]>

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.

---
 lib/index.tcl | 92 +++++++++++++++++++++++++--------------------------
 1 file changed, 46 insertions(+), 46 deletions(-)

index e07b7a3762..28d4d2a54e 100644
@@ -56,15 +56,15 @@ proc _close_updateindex {fd after} {
 	uplevel #0 $after
 }
 
-proc update_indexinfo {msg pathList after} {
+proc update_indexinfo {msg path_list after} {
 	global update_index_cp
 
 	if {![lock_index update]} return
 
 	set update_index_cp 0
-	set pathList [lsort $pathList]
-	set totalCnt [llength $pathList]
-	set batch [expr {int($totalCnt * .01) + 1}]
+	set path_list [lsort $path_list]
+	set total_cnt [llength $path_list]
+	set batch [expr {int($total_cnt * .01) + 1}]
 	if {$batch > 25} {set batch 25}
 
 	$::main_status start $msg [mc "files"]

@@ -78,26 +78,26 @@ proc update_indexinfo {msg pathList after} {
 	fileevent $fd writable [list \
 		write_update_indexinfo \
 		$fd \
-		$pathList \
-		$totalCnt \
+		$path_list \
+		$total_cnt \
 		$batch \
 		$after \
 		]
 }
 
-proc write_update_indexinfo {fd pathList totalCnt batch after} {
+proc write_update_indexinfo {fd path_list total_cnt batch after} {
 	global update_index_cp
 	global file_states current_diff_path
 
-	if {$update_index_cp >= $totalCnt} {
+	if {$update_index_cp >= $total_cnt} {
 		_close_updateindex $fd $after
 		return
 	}
 
 	for {set i $batch} \
-		{$update_index_cp < $totalCnt && $i > 0} \
+		{$update_index_cp < $total_cnt && $i > 0} \
 		{incr i -1} {
-		set path [lindex $pathList $update_index_cp]
+		set path [lindex $path_list $update_index_cp]
 		incr update_index_cp
 
 		set s $file_states($path)

@@ -119,18 +119,18 @@ proc write_update_indexinfo {fd pathList totalCnt batch after} {
 		display_file $path $new
 	}
 
-	$::main_status update $update_index_cp $totalCnt
+	$::main_status update $update_index_cp $total_cnt
 }
 
-proc update_index {msg pathList after} {
+proc update_index {msg path_list after} {
 	global update_index_cp
 
 	if {![lock_index update]} return
 
 	set update_index_cp 0
-	set pathList [lsort $pathList]
-	set totalCnt [llength $pathList]
-	set batch [expr {int($totalCnt * .01) + 1}]
+	set path_list [lsort $path_list]
+	set total_cnt [llength $path_list]
+	set batch [expr {int($total_cnt * .01) + 1}]
 	if {$batch > 25} {set batch 25}
 
 	$::main_status start $msg [mc "files"]

@@ -144,26 +144,26 @@ proc update_index {msg pathList after} {
 	fileevent $fd writable [list \
 		write_update_index \
 		$fd \
-		$pathList \
-		$totalCnt \
+		$path_list \
+		$total_cnt \
 		$batch \
 		$after \
 		]
 }
 
-proc write_update_index {fd pathList totalCnt batch after} {
+proc write_update_index {fd path_list total_cnt batch after} {
 	global update_index_cp
 	global file_states current_diff_path
 
-	if {$update_index_cp >= $totalCnt} {
+	if {$update_index_cp >= $total_cnt} {
 		_close_updateindex $fd $after
 		return
 	}
 
 	for {set i $batch} \
-		{$update_index_cp < $totalCnt && $i > 0} \
+		{$update_index_cp < $total_cnt && $i > 0} \
 		{incr i -1} {
-		set path [lindex $pathList $update_index_cp]
+		set path [lindex $path_list $update_index_cp]
 		incr update_index_cp
 
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -190,18 +190,18 @@ proc write_update_index {fd pathList totalCnt batch after} {
 		display_file $path $new
 	}
 
-	$::main_status update $update_index_cp $totalCnt
+	$::main_status update $update_index_cp $total_cnt
 }
 
-proc checkout_index {msg pathList after} {
+proc checkout_index {msg path_list after} {
 	global update_index_cp
 
 	if {![lock_index update]} return
 
 	set update_index_cp 0
-	set pathList [lsort $pathList]
-	set totalCnt [llength $pathList]
-	set batch [expr {int($totalCnt * .01) + 1}]
+	set path_list [lsort $path_list]
+	set total_cnt [llength $path_list]
+	set batch [expr {int($total_cnt * .01) + 1}]
 	if {$batch > 25} {set batch 25}
 
 	$::main_status start $msg [mc "files"]

@@ -221,26 +221,26 @@ proc checkout_index {msg pathList after} {
 	fileevent $fd writable [list \
 		write_checkout_index \
 		$fd \
-		$pathList \
-		$totalCnt \
+		$path_list \
+		$total_cnt \
 		$batch \
 		$after \
 		]
 }
 
-proc write_checkout_index {fd pathList totalCnt batch after} {
+proc write_checkout_index {fd path_list total_cnt batch after} {
 	global update_index_cp
 	global file_states current_diff_path
 
-	if {$update_index_cp >= $totalCnt} {
+	if {$update_index_cp >= $total_cnt} {
 		_close_updateindex $fd $after
 		return
 	}
 
 	for {set i $batch} \
-		{$update_index_cp < $totalCnt && $i > 0} \
+		{$update_index_cp < $total_cnt && $i > 0} \
 		{incr i -1} {
-		set path [lindex $pathList $update_index_cp]
+		set path [lindex $path_list $update_index_cp]
 		incr update_index_cp
 		switch -glob -- [lindex $file_states($path) 0] {
 		U? {continue}

@@ -253,7 +253,7 @@ proc write_checkout_index {fd pathList totalCnt batch after} {
 		}
 	}
 
-	$::main_status update $update_index_cp $totalCnt
+	$::main_status update $update_index_cp $total_cnt
 }
 
 proc unstage_helper {txt paths} {

@@ -261,7 +261,7 @@ proc unstage_helper {txt paths} {
 
 	if {![lock_index begin-update]} return
 
-	set pathList [list]
+	set path_list [list]
 	set after {}
 	foreach path $paths {
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -269,19 +269,19 @@ proc unstage_helper {txt paths} {
 		M? -
 		T? -
 		D? {
-			lappend pathList $path
+			lappend path_list $path
 			if {$path eq $current_diff_path} {
 				set after {reshow_diff;}
 			}
 		}
 		}
 	}
-	if {$pathList eq {}} {
+	if {$path_list eq {}} {
 		unlock_index
 	} else {
 		update_indexinfo \
 			$txt \
-			$pathList \
+			$path_list \
 			[concat $after [list ui_ready]]
 	}
 }

@@ -305,7 +305,7 @@ proc add_helper {txt paths} {
 
 	if {![lock_index begin-update]} return
 
-	set pathList [list]
+	set path_list [list]
 	set after {}
 	foreach path $paths {
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -321,19 +321,19 @@ proc add_helper {txt paths} {
 		?M -
 		?D -
 		?T {
-			lappend pathList $path
+			lappend path_list $path
 			if {$path eq $current_diff_path} {
 				set after {reshow_diff;}
 			}
 		}
 		}
 	}
-	if {$pathList eq {}} {
+	if {$path_list eq {}} {
 		unlock_index
 	} else {
 		update_index \
 			$txt \
-			$pathList \
+			$path_list \
 			[concat $after {ui_status [mc "Ready to commit."]}]
 	}
 }

@@ -393,7 +393,7 @@ proc revert_helper {txt paths} {
 
 	if {![lock_index begin-update]} return
 
-	set pathList [list]
+	set path_list [list]
 	set after {}
 	foreach path $paths {
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -401,7 +401,7 @@ proc revert_helper {txt paths} {
 		?M -
 		?T -
 		?D {
-			lappend pathList $path
+			lappend path_list $path
 			if {$path eq $current_diff_path} {
 				set after {reshow_diff;}
 			}

@@ -420,12 +420,12 @@ proc revert_helper {txt paths} {
 	# as they have quite complex plural-form rules. Unfortunately,
 	# msgcat doesn't seem to support that kind of string translation.
 	#
-	set n [llength $pathList]
+	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 $pathList]]]
+		set query [mc "Revert changes in file %s?" [short_path [lindex $path_list]]]
 	} else {
 		set query [mc "Revert changes in these %i files?" $n]
 	}

@@ -444,7 +444,7 @@ proc revert_helper {txt paths} {
 	if {$reply == 1} {
 		checkout_index \
 			$txt \
-			$pathList \
+			$path_list \
 			[concat $after [list ui_ready]]
 	} else {
 		unlock_index

gitgitgadget

From: Jonathan Gilbert <[email protected]>

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.

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

index 0d21f5688b..dd6f3a3592 100755
@@ -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 \

gitgitgadget

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

Looks much better!

Regards,
Pratyush Yadav

On Sat, Nov 16, 2019 at 9:11 AM Pratyush Yadav me-at-yadavpratyush.com

|GitHub Public/Example Allow| <[email protected]> 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

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.

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.

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

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 .gitignore forever). 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_helper proc 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 proc delete_files takes care of actually deleting the files, 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.

Since the checkout_index and delete_files calls 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.

The _close_updateindex proc contains error handling (added in d4e890e5) that has the potential to interact badly with unlock_index running at the completion of an async operation. I have refactored the procedure into separate procs _close_updateindex and rescan_on_error, and the original functionality is captured with _close_updateindex_rescan_on_error. Call sites have been updated appropriately, and checkout_index has been reworked to take a functor that captures any errors that occur in a caller-defined way. revert_helper uses this to supply a lambda function that stashes the error within the chord's body namespace, so that it can call rescan_on_error when the chord is completed, which might be substantially after checkout_index encounters its error. If a rescan is called for, it is done once the deletion is complete.

This is the fifth revision of this change, which differs from the fourth version in the following ways:

* The status_bar.tcl code has been reworked so that tracking an operation with updates and progress is now done in instances of a separate status_bar_operation class. main_status start returns an operation instead of directly setting the state on the main progress bar, and then you send all your updates to the operation. This model allows the main progress bar to support multiple ongoing operations, in which case their progress data is merged: The status text is concatenated (separated by '/'), and the progress is combined mathematically into a single progress bar.

* All call sites that used to directly send updates to main_status have been updated to use the new model.

* The initialization code in git-gui.sh (which I'm assuming translates somehow to git-gui.tcl in the installation?) now explicitly clears the "Initializing..." status bar text, since the new status bar model won't do this automatically when operations are performed.

* rescan_on_error makes a call to a special status bar function to stop and unhook all ongoing operations, on the assumption that the rescan is superseding any and all ongoing operations. Of course, if the operations are still actually running, errors will result, but this seemed the clearest mapping from the previous revision's $::main_status stop call inside the rescan code.

* _close_updateindex_rescan_on_error has been renamed to close_and_unlock_updateindex_rescan_on_error, and once again takes $after . As indicated by the name change, it also encapsulates the unlocking of the index, so that is removed from the call sites.

* rescan_on_error takes $after as well, in case close_and_unlock_updateindex_rescan_on_error needs to call it.

* The after_chord body in revert_helper only calls unlock_index if it doesn't call rescan_on_error.

git remote add logiclrd https://github.com/logiclrd/git.git
git fetch logiclrd git-gui-revert-untracked revision4 git diff dc12c16..23d4f5d

Jonathan Gilbert (2):
git-gui: consolidate naming conventions
git-gui: revert untracked files by deleting them

 git-gui.sh          |   7 +-
 lib/checkout_op.tcl |  15 +-
 lib/chord.tcl       | 160 ++++++++++++++
 lib/index.tcl       | 517 +++++++++++++++++++++++++++++++++-----------
 lib/merge.tcl       |  14 +-
 lib/status_bar.tcl  | 221 +++++++++++++++++--
 6 files changed, 771 insertions(+), 163 deletions(-)
 create mode 100644 lib/chord.tcl

base-commit: b524f6b399c77b40c8bf2b6217585fde4731472a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-436%2Flogiclrd%2Fgit-gui-revert-untracked-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-436/logiclrd/git-gui-revert-untracked-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/436

Range-diff vs v3:

 1:  da1704c56e = 1:  da1704c56e git-gui: consolidate naming conventions
 2:  dc12c1668d ! 2:  23d4f5d829 git-gui: revert untracked files by deleting them
     @@ -11,7 +11,8 @@
          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.
     +    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
     @@ -23,6 +24,14 @@
          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.
     +
          Signed-off-by: Jonathan Gilbert <[email protected]>
      
       diff --git a/git-gui.sh b/git-gui.sh
     @@ -39,6 +48,73 @@
       } {
       	catch {wm withdraw .}
       	tk_messageBox \
     +@@
     + 	do_explore
     + }
     + 
     ++# Clear "Initializing..." status
     ++after idle {after 500 {$main_status show ""}}
     ++
     + # Local variables:
     + # mode: tcl
     + # indent-tabs-mode: t
     +
     + diff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl
     + --- a/lib/checkout_op.tcl
     + +++ b/lib/checkout_op.tcl
     +@@
     + 	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 \
     +@@
     + 		$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
     +@@
     + 		return
     + 	}
     + 
     +-	$::main_status stop
     ++	$status_bar_operation stop
     + 	_after_readtree $this
     + }
     + 
      
       diff --git a/lib/chord.tcl b/lib/chord.tcl
       new file mode 100644
     @@ -215,18 +291,7 @@
       
      -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
     +-	fconfigure $fd -blocking 1
      -	if {[catch {close $fd} err]} {
      -		set w .indexfried
      -		Dialog $w
     @@ -261,16 +326,29 @@
      -		"
      -		wm deiconify $w
      -		tkwait window $w
     -+	close $fd
     -+	$::main_status stop
     -+}
     - 
     +-
      -		$::main_status stop
     --		unlock_index
     ++# Returns true if the operation succeeded, false if a rescan has been initiated.
     ++proc close_and_unlock_updateindex_rescan_on_error {fd after} {
     ++	if {![catch {_close_updateindex $fd} err]} {
     + 		unlock_index
      -		rescan $after 0
      -		return
     --	}
     -+proc rescan_on_error {err} {
     ++		uplevel #0 $after
     ++		return 1
     ++	} else {
     ++		rescan_on_error $err $after
     ++		return 0
     + 	}
     ++}
     + 
     +-	$::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
     @@ -306,42 +384,91 @@
      +	"
      +	wm deiconify $w
      +	tkwait window $w
     - 
     - 	$::main_status stop
     ++
     ++	$::main_status stop_all
       	unlock_index
      -	uplevel #0 $after
     -+	rescan ui_ready 0
     ++	rescan [concat $after [list ui_ready]] 0
       }
       
       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 \
     +@@
     + 		$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
     -+		if {[_close_updateindex_rescan_on_error $fd]} {
     -+			unlock_index
     -+		}
     -+
     -+		uplevel #0 $after
     ++		$status_bar_operation stop
     ++		close_and_unlock_updateindex_rescan_on_error $fd $after
       		return
       	}
       
      @@
     + 		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} {
     +@@
     + 	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 \
     +@@
     + 		$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
     -+		if {[_close_updateindex_rescan_on_error $fd]} {
     -+			unlock_index
     -+		}
     -+
     -+		uplevel #0 $after
     ++		$status_bar_operation stop
     ++		close_and_unlock_updateindex_rescan_on_error $fd $after
       		return
       	}
       
      @@
     - 	$::main_status update $update_index_cp $total_cnt
     + 		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} {
     @@ -350,20 +477,34 @@
       
       	if {![lock_index update]} return
      @@
     + 	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 \
     +@@
     + 		$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 after capture_error} {
     ++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]} {
      +			uplevel #0 $capture_error [list $err]
      +		}
     @@ -373,6 +514,15 @@
       		return
       	}
       
     +@@
     + 		}
     + 	}
     + 
     +-	$::main_status update $update_index_cp $total_cnt
     ++	$status_bar_operation update $update_index_cp $total_cnt
     + }
     + 
     + proc unstage_helper {txt paths} {
      @@
       	add_helper [mc "Adding all changed files"] $paths
       }
     @@ -394,10 +544,10 @@
      +	# The asynchronous operations are each indicated below by a comment
      +	# before the code block that starts the async operation.
      +	set after_chord [SimpleChord new {
     -+		unlock_index
      +		if {[string trim $err] != ""} {
      +			rescan_on_error $err
      +		} else {
     ++			unlock_index
      +			if {$should_reshow_diff} { reshow_diff }
      +			ui_ready
      +		}
     @@ -467,11 +617,6 @@
      +				"Revert changes in these %i files?" \
      +				$path_cnt]
      +		}
     -+
     -+		set reply [tk_dialog \
     -+			.confirm_revert \
     -+			"[appname] ([reponame])" \
     -+			"$query
       
      -	# Split question between singular and plural cases, because
      -	# such distinction is needed in some languages. Previously, the
     @@ -491,6 +636,11 @@
      -		set query [mc "Revert changes in file %s?" [short_path [lindex $path_list]]]
      -	} else {
      -		set query [mc "Revert changes in these %i files?" $n]
     ++		set reply [tk_dialog \
     ++			.confirm_revert \
     ++			"[appname] ([reponame])" \
     ++			"$query
     ++
      +[mc "Any unstaged changes will be permanently lost by the revert."]" \
      +			question \
      +			1 \
     @@ -571,7 +721,10 @@
      +# 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 status_bar_operation [$::main_status \
     ++		start \
     ++		[mc "Deleting"] \
     ++		[mc "files"]]
      +
      +	set path_index 0
      +	set deletion_errors [list]
     @@ -582,13 +735,15 @@
      +		$path_index \
      +		$deletion_errors \
      +		$batch_size \
     ++		$status_bar_operation \
      +		$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} {
     ++proc delete_helper {path_list path_index deletion_errors batch_size \
     ++	status_bar_operation after} {
      +	global file_states
      +
      +	set path_cnt [llength $path_list]
     @@ -622,7 +777,7 @@
      +	# 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
     ++	$status_bar_operation update $path_index $path_cnt
      +
      +	if {$path_index < $path_cnt} {
      +		# The Tcler's Wiki lists this as the best practice for keeping
     @@ -636,12 +791,13 @@
      +			$path_index \
      +			$deletion_errors \
      +			$batch_size \
     ++			$status_bar_operation \
      +			$after
      +			]]
       	} else {
      -		unlock_index
      +		# Finish the status bar operation.
     -+		$::main_status stop
     ++		$status_bar_operation stop
      +
      +		# Report error, if any, based on how many deletions failed.
      +		set deletion_error_cnt [llength $deletion_errors]
     @@ -704,3 +860,348 @@
       	}
       }
       
     +
     + diff --git a/lib/merge.tcl b/lib/merge.tcl
     + --- a/lib/merge.tcl
     + +++ b/lib/merge.tcl
     +@@
     + 	if {[ask_popup $op_question] eq {yes}} {
     + 		set fd [git_read --stderr read-tree --reset -u -v HEAD]
     + 		fconfigure $fd -blocking 0 -translation binary
     +-		fileevent $fd readable [namespace code [list _reset_wait $fd]]
     +-		$::main_status start [mc "Aborting"] [mc "files reset"]
     ++		set status_bar_operation [$::main_status \
     ++			start \
     ++			[mc "Aborting"] \
     ++			[mc "files reset"]
     ++		fileevent $fd readable [namespace code [list \
     ++			_reset_wait $fd $status_bar_operation]]
     + 	} else {
     + 		unlock_index
     + 	}
     + }
     + 
     +-proc _reset_wait {fd} {
     ++proc _reset_wait {fd status_bar_operation} {
     + 	global ui_comm
     + 
     +-	$::main_status update_meter [read $fd]
     ++	$status_bar_operation update_meter [read $fd]
     + 
     + 	fconfigure $fd -blocking 1
     + 	if {[eof $fd]} {
     + 		set fail [catch {close $fd} err]
     +-		$::main_status stop
     + 		unlock_index
     ++		$status_bar_operation stop
     + 
     + 		$ui_comm delete 0.0 end
     + 		$ui_comm edit modified false
     +
     + diff --git a/lib/status_bar.tcl b/lib/status_bar.tcl
     + --- a/lib/status_bar.tcl
     + +++ b/lib/status_bar.tcl
     +@@
     + # 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
     +-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
     +@@
     + 	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
     +@@
     + 	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
     +@@
     + 	return $this
     + }
     + 
     +-method start {msg uds} {
     ++method ensure_canvas {} {
     + 	if {[winfo exists $w_c]} {
     + 		$w_c coords bar 0 0 0 20
     + 	} else {
     +@@
     + 		$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]} {
     ++		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} {
     +@@
     + 	}
     + }
     + 
     +-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
     + }
     + 
     + }

gitgitgadget

From: Jonathan Gilbert <[email protected]>

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.

---
 lib/index.tcl | 92 +++++++++++++++++++++++++--------------------------
 1 file changed, 46 insertions(+), 46 deletions(-)

index e07b7a3762..28d4d2a54e 100644
@@ -56,15 +56,15 @@ proc _close_updateindex {fd after} {
 	uplevel #0 $after
 }
 
-proc update_indexinfo {msg pathList after} {
+proc update_indexinfo {msg path_list after} {
 	global update_index_cp
 
 	if {![lock_index update]} return
 
 	set update_index_cp 0
-	set pathList [lsort $pathList]
-	set totalCnt [llength $pathList]
-	set batch [expr {int($totalCnt * .01) + 1}]
+	set path_list [lsort $path_list]
+	set total_cnt [llength $path_list]
+	set batch [expr {int($total_cnt * .01) + 1}]
 	if {$batch > 25} {set batch 25}
 
 	$::main_status start $msg [mc "files"]

@@ -78,26 +78,26 @@ proc update_indexinfo {msg pathList after} {
 	fileevent $fd writable [list \
 		write_update_indexinfo \
 		$fd \
-		$pathList \
-		$totalCnt \
+		$path_list \
+		$total_cnt \
 		$batch \
 		$after \
 		]
 }
 
-proc write_update_indexinfo {fd pathList totalCnt batch after} {
+proc write_update_indexinfo {fd path_list total_cnt batch after} {
 	global update_index_cp
 	global file_states current_diff_path
 
-	if {$update_index_cp >= $totalCnt} {
+	if {$update_index_cp >= $total_cnt} {
 		_close_updateindex $fd $after
 		return
 	}
 
 	for {set i $batch} \
-		{$update_index_cp < $totalCnt && $i > 0} \
+		{$update_index_cp < $total_cnt && $i > 0} \
 		{incr i -1} {
-		set path [lindex $pathList $update_index_cp]
+		set path [lindex $path_list $update_index_cp]
 		incr update_index_cp
 
 		set s $file_states($path)

@@ -119,18 +119,18 @@ proc write_update_indexinfo {fd pathList totalCnt batch after} {
 		display_file $path $new
 	}
 
-	$::main_status update $update_index_cp $totalCnt
+	$::main_status update $update_index_cp $total_cnt
 }
 
-proc update_index {msg pathList after} {
+proc update_index {msg path_list after} {
 	global update_index_cp
 
 	if {![lock_index update]} return
 
 	set update_index_cp 0
-	set pathList [lsort $pathList]
-	set totalCnt [llength $pathList]
-	set batch [expr {int($totalCnt * .01) + 1}]
+	set path_list [lsort $path_list]
+	set total_cnt [llength $path_list]
+	set batch [expr {int($total_cnt * .01) + 1}]
 	if {$batch > 25} {set batch 25}
 
 	$::main_status start $msg [mc "files"]

@@ -144,26 +144,26 @@ proc update_index {msg pathList after} {
 	fileevent $fd writable [list \
 		write_update_index \
 		$fd \
-		$pathList \
-		$totalCnt \
+		$path_list \
+		$total_cnt \
 		$batch \
 		$after \
 		]
 }
 
-proc write_update_index {fd pathList totalCnt batch after} {
+proc write_update_index {fd path_list total_cnt batch after} {
 	global update_index_cp
 	global file_states current_diff_path
 
-	if {$update_index_cp >= $totalCnt} {
+	if {$update_index_cp >= $total_cnt} {
 		_close_updateindex $fd $after
 		return
 	}
 
 	for {set i $batch} \
-		{$update_index_cp < $totalCnt && $i > 0} \
+		{$update_index_cp < $total_cnt && $i > 0} \
 		{incr i -1} {
-		set path [lindex $pathList $update_index_cp]
+		set path [lindex $path_list $update_index_cp]
 		incr update_index_cp
 
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -190,18 +190,18 @@ proc write_update_index {fd pathList totalCnt batch after} {
 		display_file $path $new
 	}
 
-	$::main_status update $update_index_cp $totalCnt
+	$::main_status update $update_index_cp $total_cnt
 }
 
-proc checkout_index {msg pathList after} {
+proc checkout_index {msg path_list after} {
 	global update_index_cp
 
 	if {![lock_index update]} return
 
 	set update_index_cp 0
-	set pathList [lsort $pathList]
-	set totalCnt [llength $pathList]
-	set batch [expr {int($totalCnt * .01) + 1}]
+	set path_list [lsort $path_list]
+	set total_cnt [llength $path_list]
+	set batch [expr {int($total_cnt * .01) + 1}]
 	if {$batch > 25} {set batch 25}
 
 	$::main_status start $msg [mc "files"]

@@ -221,26 +221,26 @@ proc checkout_index {msg pathList after} {
 	fileevent $fd writable [list \
 		write_checkout_index \
 		$fd \
-		$pathList \
-		$totalCnt \
+		$path_list \
+		$total_cnt \
 		$batch \
 		$after \
 		]
 }
 
-proc write_checkout_index {fd pathList totalCnt batch after} {
+proc write_checkout_index {fd path_list total_cnt batch after} {
 	global update_index_cp
 	global file_states current_diff_path
 
-	if {$update_index_cp >= $totalCnt} {
+	if {$update_index_cp >= $total_cnt} {
 		_close_updateindex $fd $after
 		return
 	}
 
 	for {set i $batch} \
-		{$update_index_cp < $totalCnt && $i > 0} \
+		{$update_index_cp < $total_cnt && $i > 0} \
 		{incr i -1} {
-		set path [lindex $pathList $update_index_cp]
+		set path [lindex $path_list $update_index_cp]
 		incr update_index_cp
 		switch -glob -- [lindex $file_states($path) 0] {
 		U? {continue}

@@ -253,7 +253,7 @@ proc write_checkout_index {fd pathList totalCnt batch after} {
 		}
 	}
 
-	$::main_status update $update_index_cp $totalCnt
+	$::main_status update $update_index_cp $total_cnt
 }
 
 proc unstage_helper {txt paths} {

@@ -261,7 +261,7 @@ proc unstage_helper {txt paths} {
 
 	if {![lock_index begin-update]} return
 
-	set pathList [list]
+	set path_list [list]
 	set after {}
 	foreach path $paths {
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -269,19 +269,19 @@ proc unstage_helper {txt paths} {
 		M? -
 		T? -
 		D? {
-			lappend pathList $path
+			lappend path_list $path
 			if {$path eq $current_diff_path} {
 				set after {reshow_diff;}
 			}
 		}
 		}
 	}
-	if {$pathList eq {}} {
+	if {$path_list eq {}} {
 		unlock_index
 	} else {
 		update_indexinfo \
 			$txt \
-			$pathList \
+			$path_list \
 			[concat $after [list ui_ready]]
 	}
 }

@@ -305,7 +305,7 @@ proc add_helper {txt paths} {
 
 	if {![lock_index begin-update]} return
 
-	set pathList [list]
+	set path_list [list]
 	set after {}
 	foreach path $paths {
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -321,19 +321,19 @@ proc add_helper {txt paths} {
 		?M -
 		?D -
 		?T {
-			lappend pathList $path
+			lappend path_list $path
 			if {$path eq $current_diff_path} {
 				set after {reshow_diff;}
 			}
 		}
 		}
 	}
-	if {$pathList eq {}} {
+	if {$path_list eq {}} {
 		unlock_index
 	} else {
 		update_index \
 			$txt \
-			$pathList \
+			$path_list \
 			[concat $after {ui_status [mc "Ready to commit."]}]
 	}
 }

@@ -393,7 +393,7 @@ proc revert_helper {txt paths} {
 
 	if {![lock_index begin-update]} return
 
-	set pathList [list]
+	set path_list [list]
 	set after {}
 	foreach path $paths {
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -401,7 +401,7 @@ proc revert_helper {txt paths} {
 		?M -
 		?T -
 		?D {
-			lappend pathList $path
+			lappend path_list $path
 			if {$path eq $current_diff_path} {
 				set after {reshow_diff;}
 			}

@@ -420,12 +420,12 @@ proc revert_helper {txt paths} {
 	# as they have quite complex plural-form rules. Unfortunately,
 	# msgcat doesn't seem to support that kind of string translation.
 	#
-	set n [llength $pathList]
+	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 $pathList]]]
+		set query [mc "Revert changes in file %s?" [short_path [lindex $path_list]]]
 	} else {
 		set query [mc "Revert changes in these %i files?" $n]
 	}

@@ -444,7 +444,7 @@ proc revert_helper {txt paths} {
 	if {$reply == 1} {
 		checkout_index \
 			$txt \
-			$pathList \
+			$path_list \
 			[concat $after [list ui_ready]]
 	} else {
 		unlock_index

gitgitgadget

From: Jonathan Gilbert <[email protected]>

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.

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

index 0d21f5688b..dc4ac577ac 100755
@@ -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 ""}}
+
 # Local variables:
 # mode: tcl
 # indent-tabs-mode: t

index a5228297db..21ea768d80 100644
@@ -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
 }
 

index 9f253db5b3..8df8ffae55 100644
@@ -241,23 +241,27 @@ Continue with resetting the current changes?"]
 	if {[ask_popup $op_question] eq {yes}} {
 		set fd [git_read --stderr read-tree --reset -u -v HEAD]
 		fconfigure $fd -blocking 0 -translation binary
-		fileevent $fd readable [namespace code [list _reset_wait $fd]]
-		$::main_status start [mc "Aborting"] [mc "files reset"]
+		set status_bar_operation [$::main_status \
+			start \
+			[mc "Aborting"] \
+			[mc "files reset"]
+		fileevent $fd readable [namespace code [list \
+			_reset_wait $fd $status_bar_operation]]
 	} else {
 		unlock_index
 	}
 }
 
-proc _reset_wait {fd} {
+proc _reset_wait {fd status_bar_operation} {
 	global ui_comm
 
-	$::main_status update_meter [read $fd]
+	$status_bar_operation update_meter [read $fd]
 
 	fconfigure $fd -blocking 1
 	if {[eof $fd]} {
 		set fail [catch {close $fd} err]
-		$::main_status stop
 		unlock_index
+		$status_bar_operation stop
 
 		$ui_comm delete 0.0 end
 		$ui_comm edit modified false

gitgitgadget

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.

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

Regards,
Pratyush Yadav

Well, the design is a bit strange. git-gui.sh happens to be _both_ a shell script and a Tcl script. When you run './git-gui.sh', it is executed as a shell script. That shell script then executes itself via 'wish' (which is the Tcl/Tk windowing shell), and Tcl ignores the first "line" (it is actually the lines 3-10, but they all have an escaped newline so it is effectively a single line).

It has been like since the very first revision of git-gui. I wonder why the original author went with this instead of just doing something like:

#!/usr/bin/env wish

which seems to work just fine on my quick testing, but that is another topic entirely ;)

PS: I am in the process of reviewing the latest revision of the patch series. But I have been short on free time recently so it might take me a couple more days.

Regards,
Pratyush Yadav

Ah, I see -- I had managed to convince myself that they were different files, but never actually diffed them. In my installation, I have a file "C:\Program Files\Git\mingw64\libexec\git-core\git-gui.tcl" and that _is_ the same file as git-gui.sh in the repository. So maybe it's Windows-specific, maybe not, I'm not sure, but at some point it gets renamed from git-gui.sh to git-gui.tcl. I incorrectly assumed that the .tcl file didn't have all the facets of the .sh file. :-P

Thanks,

Jonathan Gilbert

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 .gitignore forever). 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_helper proc 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 proc delete_files takes care of actually deleting the files, 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.

Since the checkout_index and delete_files calls 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_updateindex proc contains error handling (added in d4e890e5) that has the potential to interact badly with unlock_index running at the completion of an async operation. I have refactored the procedure into separate procs _close_updateindex and rescan_on_error. Call sites that exercised the combined functionality also unlocked the index, so a combined proc close_and_unlock_index calls _close_updateindex and then either rescan_on_error or unlock_index as appropriate. Call sites have been updated appropriately.

The revert_helper proc, with its overlapping operations, is an example of a call site that does not combine the close and unlock/rescan operations. The checkout_index proc has been reworked to only call _close_updateindex, and to call a functor supplied by the caller to captures any errors that occur. revert_helper uses this to supply a lambda function that stashes the error within the chord's body namespace, so that it can then separately call rescan_on_error when the chord is completed (or unlock_index, if no error was captured), which might be substantially after checkout_index encounters its error. If it turns out that a rescan is called for, it is done once the deletion is complete.

This is the sixth revision of this change, which differs from the fifth version in the following ways:

* The status_bar.tcl changes have been isolated into a separate commit.

* A method in status_bar.tcl that apparently had never been hit had some simple bugs in it that have been corrected.

* The show methods on the classes in status_bar.tcl no longer have a test parameter, as nothing was using that feature that I could see.

* The refresh method in status_bar.tcl now only tries to update the progress bar widget if it actually exists.

* blame.tcl had been missed when searching for code using the status bar, this has been addressed.

* rescan_on_error takes $after as well, in case close_and_unlock_updateindex_rescan_on_error needs to call it.

* close_and_unlock_updateindex_rescan_on_error has been renamed to simply close_and_unlock_index.

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"

I thought I'd found all places that used the status bar, but apparently overlooked blame.tcl. This has been addressed. My apologies!

choose_repository.tcl was addressed already in the previous commit.

that the entire system still works properly (so this means the commit would include updating the existing callsites).

Done. In doing so and testing it independently, I actually found paths that weren't being hit with the full set of changes and fixed a few bugs. In particular, stop_all was completely broken :-P I thought I had exercised it before and I hadn't.

""}}

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.

My thinking was to make it clear the status bar 500 milliseconds after the queue empties. It's quite possible that my understanding of what it means for the queue to have emptied means that this isn't a terribly meaningful thing to do. I was wanting it to continue saying "Initializing..." until, heuristically, it is done initializing, including any queued up operations. I guess if anything waits on I/O, then the queue may to go idle even though it's still busy doing things. So, a straight-up after 500 { } would make more sense? I've changed it to this, can make further changes if they are called for.

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.

Good point. The thinking that had been going on in my head is that close_and_unlock_updateindex_rescan_on_error was combining two operations that most but not all paths would do together, and then the paths that needed them separated would call close_and_unlock in one place and rescan_on_error in another. The previous "cover letter" actually explicitly described this, referring to proc names that actually weren't current any more by the time the code was submitted. Upon closer review, at this point, the place that calls rescan_on_error independently is not making a separate call to something like close_and_unlock, it's just calling _close_updateindex directly, and the unlock is occurring in the chord body.

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

I'm pretty sure there was an iteration of the code where at least one path checked, but since nothing checks it now, I've removed the return value.

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.

Done.

The field 'status' is removed, but the procedure 'show' still uses it. The if condition needs to be refactored.

As far as I can tell, this feature of show to only change the status if the test matches isn't actually used by any call sites. I've removed it. If this was in error, then I'll reintroduce it correctly. I think, generally speaking, supporting overlapping operations resolves the problem that this would have originally resolved, which is that something else has updated the status since it was previously set and the caller doesn't want to clear somebody else's more up-to-date status text.

This silently ignores multiple 'start's on a status bar that doesn't allow it, correct?

It does, yes. The only status bar that doesn't allow multiple operations is the two-line one created by choose_repository.

A caller that did this erroneously would get the same operation reference both times, which would mean that (presumably) it got stopped multiple times --- but stop on a status bar operation is idempotent, so this shouldn't actually result in any errors.

operations in parallel would still continue working exactly the same (after refactoring them to use 'status_bar_operation'), right?

This is the expectation, yes, and is what I have observed in my testing. :-)

git remote add logiclrd https://github.com/logiclrd/git.git
git fetch logiclrd git-gui-revert-untracked revision5 git diff 23d4f5d..d0d6593b42

Jonathan Gilbert (3):
git-gui: consolidate naming conventions
git-gui: update status bar to track operations
git-gui: revert untracked files by deleting them

 git-gui.sh          |  11 +-
 lib/blame.tcl       |  22 +-
 lib/checkout_op.tcl |  15 +-
 lib/chord.tcl       | 160 ++++++++++++++
 lib/index.tcl       | 523 +++++++++++++++++++++++++++++++++-----------
 lib/merge.tcl       |  14 +-
 lib/status_bar.tcl  | 228 ++++++++++++++++---
 7 files changed, 797 insertions(+), 176 deletions(-)
 create mode 100644 lib/chord.tcl

base-commit: b524f6b399c77b40c8bf2b6217585fde4731472a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-436%2Flogiclrd%2Fgit-gui-revert-untracked-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-436/logiclrd/git-gui-revert-untracked-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/436

Range-diff vs v4:

 1:  da1704c56e = 1:  da1704c56e git-gui: consolidate naming conventions
 -:  ---------- > 2:  aa05a78d28 git-gui: update status bar to track operations
 2:  23d4f5d829 ! 3:  d0d6593b42 git-gui: revert untracked files by deleting them
     @@ -11,8 +11,8 @@
          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.
     +    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
     @@ -24,14 +24,6 @@
          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.
     -
          Signed-off-by: Jonathan Gilbert <[email protected]>
      
       diff --git a/git-gui.sh b/git-gui.sh
     @@ -48,73 +40,6 @@
       } {
       	catch {wm withdraw .}
       	tk_messageBox \
     -@@
     - 	do_explore
     - }
     - 
     -+# Clear "Initializing..." status
     -+after idle {after 500 {$main_status show ""}}
     -+
     - # Local variables:
     - # mode: tcl
     - # indent-tabs-mode: t
     -
     - diff --git a/lib/checkout_op.tcl b/lib/checkout_op.tcl
     - --- a/lib/checkout_op.tcl
     - +++ b/lib/checkout_op.tcl
     -@@
     - 	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 \
     -@@
     - 		$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
     -@@
     - 		return
     - 	}
     - 
     --	$::main_status stop
     -+	$status_bar_operation stop
     - 	_after_readtree $this
     - }
     - 
      
       diff --git a/lib/chord.tcl b/lib/chord.tcl
       new file mode 100644
     @@ -327,22 +252,18 @@
      -		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} {
     +-		$::main_status stop_all
     ++proc close_and_unlock_index {fd after} {
      +	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
       	}
      +}
     - 
     --	$::main_status stop
     ++
      +proc _close_updateindex {fd} {
      +	fconfigure $fd -blocking 1
      +	close $fd
     @@ -384,8 +305,8 @@
      +	"
      +	wm deiconify $w
      +	tkwait window $w
     -+
     -+	$::main_status stop_all
     + 
     + 	$::main_status stop_all
       	unlock_index
      -	uplevel #0 $after
      +	rescan [concat $after [list ui_ready]] 0
     @@ -393,82 +314,25 @@
       
       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 \
     -@@
     - 		$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} {
     + 		$status_bar_operation stop
      -		_close_updateindex $fd $after
     -+		$status_bar_operation stop
     -+		close_and_unlock_updateindex_rescan_on_error $fd $after
     ++		close_and_unlock_index $fd $after
       		return
       	}
       
      @@
     - 		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} {
     -@@
     - 	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 \
     -@@
     - 		$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} {
     + 		$status_bar_operation stop
      -		_close_updateindex $fd $after
     -+		$status_bar_operation stop
     -+		close_and_unlock_updateindex_rescan_on_error $fd $after
     ++		close_and_unlock_index $fd $after
       		return
       	}
       
      @@
     - 		display_file $path $new
     - 	}
     - 
     --	$::main_status update $update_index_cp $total_cnt
     -+	$status_bar_operation update $update_index_cp $total_cnt
     + 	$status_bar_operation update $update_index_cp $total_cnt
       }
       
      -proc checkout_index {msg path_list after} {
     @@ -477,33 +341,31 @@
       
       	if {![lock_index update]} return
      @@
     - 	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 \
     -@@
     - 		$path_list \
     - 		$total_cnt \
       		$batch \
     -+		$status_bar_operation \
     + 		$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 \
     + proc write_checkout_index {fd path_list total_cnt batch status_bar_operation \
     +-	after} {
      +	after capture_error} {
       	global update_index_cp
       	global file_states current_diff_path
       
       	if {$update_index_cp >= $total_cnt} {
     + 		$status_bar_operation stop
      -		_close_updateindex $fd $after
     -+		$status_bar_operation stop
     ++
     ++		# We do not unlock the index directly here because this
     ++		# operation expects to potentially run in parallel with file
     ++		# deletions scheduled by revert_helper. We're done with the
     ++		# update index, so we close it, but actually unlocking the index
     ++		# and dealing with potential errors is deferred to the chord
     ++		# body that runs when all async operations are completed.
     ++		#
     ++		# (See after_chord in revert_helper.)
      +
      +		if {[catch {_close_updateindex $fd} err]} {
      +			uplevel #0 $capture_error [list $err]
     @@ -514,15 +376,6 @@
       		return
       	}
       
     -@@
     - 		}
     - 	}
     - 
     --	$::main_status update $update_index_cp $total_cnt
     -+	$status_bar_operation update $update_index_cp $total_cnt
     - }
     - 
     - proc unstage_helper {txt paths} {
      @@
       	add_helper [mc "Adding all changed files"] $paths
       }
     @@ -860,348 +713,3 @@
       	}
       }
       
     -
     - diff --git a/lib/merge.tcl b/lib/merge.tcl
     - --- a/lib/merge.tcl
     - +++ b/lib/merge.tcl
     -@@
     - 	if {[ask_popup $op_question] eq {yes}} {
     - 		set fd [git_read --stderr read-tree --reset -u -v HEAD]
     - 		fconfigure $fd -blocking 0 -translation binary
     --		fileevent $fd readable [namespace code [list _reset_wait $fd]]
     --		$::main_status start [mc "Aborting"] [mc "files reset"]
     -+		set status_bar_operation [$::main_status \
     -+			start \
     -+			[mc "Aborting"] \
     -+			[mc "files reset"]
     -+		fileevent $fd readable [namespace code [list \
     -+			_reset_wait $fd $status_bar_operation]]
     - 	} else {
     - 		unlock_index
     - 	}
     - }
     - 
     --proc _reset_wait {fd} {
     -+proc _reset_wait {fd status_bar_operation} {
     - 	global ui_comm
     - 
     --	$::main_status update_meter [read $fd]
     -+	$status_bar_operation update_meter [read $fd]
     - 
     - 	fconfigure $fd -blocking 1
     - 	if {[eof $fd]} {
     - 		set fail [catch {close $fd} err]
     --		$::main_status stop
     - 		unlock_index
     -+		$status_bar_operation stop
     - 
     - 		$ui_comm delete 0.0 end
     - 		$ui_comm edit modified false
     -
     - diff --git a/lib/status_bar.tcl b/lib/status_bar.tcl
     - --- a/lib/status_bar.tcl
     - +++ b/lib/status_bar.tcl
     -@@
     - # 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
     --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
     -@@
     - 	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
     -@@
     - 	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
     -@@
     - 	return $this
     - }
     - 
     --method start {msg uds} {
     -+method ensure_canvas {} {
     - 	if {[winfo exists $w_c]} {
     - 		$w_c coords bar 0 0 0 20
     - 	} else {
     -@@
     - 		$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]} {
     -+		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} {
     -@@
     - 	}
     - }
     - 
     --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
     - }
     - 
     - }

gitgitgadget

From: Jonathan Gilbert <[email protected]>

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.

---
 lib/index.tcl | 92 +++++++++++++++++++++++++--------------------------
 1 file changed, 46 insertions(+), 46 deletions(-)

index e07b7a3762..28d4d2a54e 100644
@@ -56,15 +56,15 @@ proc _close_updateindex {fd after} {
 	uplevel #0 $after
 }
 
-proc update_indexinfo {msg pathList after} {
+proc update_indexinfo {msg path_list after} {
 	global update_index_cp
 
 	if {![lock_index update]} return
 
 	set update_index_cp 0
-	set pathList [lsort $pathList]
-	set totalCnt [llength $pathList]
-	set batch [expr {int($totalCnt * .01) + 1}]
+	set path_list [lsort $path_list]
+	set total_cnt [llength $path_list]
+	set batch [expr {int($total_cnt * .01) + 1}]
 	if {$batch > 25} {set batch 25}
 
 	$::main_status start $msg [mc "files"]

@@ -78,26 +78,26 @@ proc update_indexinfo {msg pathList after} {
 	fileevent $fd writable [list \
 		write_update_indexinfo \
 		$fd \
-		$pathList \
-		$totalCnt \
+		$path_list \
+		$total_cnt \
 		$batch \
 		$after \
 		]
 }
 
-proc write_update_indexinfo {fd pathList totalCnt batch after} {
+proc write_update_indexinfo {fd path_list total_cnt batch after} {
 	global update_index_cp
 	global file_states current_diff_path
 
-	if {$update_index_cp >= $totalCnt} {
+	if {$update_index_cp >= $total_cnt} {
 		_close_updateindex $fd $after
 		return
 	}
 
 	for {set i $batch} \
-		{$update_index_cp < $totalCnt && $i > 0} \
+		{$update_index_cp < $total_cnt && $i > 0} \
 		{incr i -1} {
-		set path [lindex $pathList $update_index_cp]
+		set path [lindex $path_list $update_index_cp]
 		incr update_index_cp
 
 		set s $file_states($path)

@@ -119,18 +119,18 @@ proc write_update_indexinfo {fd pathList totalCnt batch after} {
 		display_file $path $new
 	}
 
-	$::main_status update $update_index_cp $totalCnt
+	$::main_status update $update_index_cp $total_cnt
 }
 
-proc update_index {msg pathList after} {
+proc update_index {msg path_list after} {
 	global update_index_cp
 
 	if {![lock_index update]} return
 
 	set update_index_cp 0
-	set pathList [lsort $pathList]
-	set totalCnt [llength $pathList]
-	set batch [expr {int($totalCnt * .01) + 1}]
+	set path_list [lsort $path_list]
+	set total_cnt [llength $path_list]
+	set batch [expr {int($total_cnt * .01) + 1}]
 	if {$batch > 25} {set batch 25}
 
 	$::main_status start $msg [mc "files"]

@@ -144,26 +144,26 @@ proc update_index {msg pathList after} {
 	fileevent $fd writable [list \
 		write_update_index \
 		$fd \
-		$pathList \
-		$totalCnt \
+		$path_list \
+		$total_cnt \
 		$batch \
 		$after \
 		]
 }
 
-proc write_update_index {fd pathList totalCnt batch after} {
+proc write_update_index {fd path_list total_cnt batch after} {
 	global update_index_cp
 	global file_states current_diff_path
 
-	if {$update_index_cp >= $totalCnt} {
+	if {$update_index_cp >= $total_cnt} {
 		_close_updateindex $fd $after
 		return
 	}
 
 	for {set i $batch} \
-		{$update_index_cp < $totalCnt && $i > 0} \
+		{$update_index_cp < $total_cnt && $i > 0} \
 		{incr i -1} {
-		set path [lindex $pathList $update_index_cp]
+		set path [lindex $path_list $update_index_cp]
 		incr update_index_cp
 
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -190,18 +190,18 @@ proc write_update_index {fd pathList totalCnt batch after} {
 		display_file $path $new
 	}
 
-	$::main_status update $update_index_cp $totalCnt
+	$::main_status update $update_index_cp $total_cnt
 }
 
-proc checkout_index {msg pathList after} {
+proc checkout_index {msg path_list after} {
 	global update_index_cp
 
 	if {![lock_index update]} return
 
 	set update_index_cp 0
-	set pathList [lsort $pathList]
-	set totalCnt [llength $pathList]
-	set batch [expr {int($totalCnt * .01) + 1}]
+	set path_list [lsort $path_list]
+	set total_cnt [llength $path_list]
+	set batch [expr {int($total_cnt * .01) + 1}]
 	if {$batch > 25} {set batch 25}
 
 	$::main_status start $msg [mc "files"]

@@ -221,26 +221,26 @@ proc checkout_index {msg pathList after} {
 	fileevent $fd writable [list \
 		write_checkout_index \
 		$fd \
-		$pathList \
-		$totalCnt \
+		$path_list \
+		$total_cnt \
 		$batch \
 		$after \
 		]
 }
 
-proc write_checkout_index {fd pathList totalCnt batch after} {
+proc write_checkout_index {fd path_list total_cnt batch after} {
 	global update_index_cp
 	global file_states current_diff_path
 
-	if {$update_index_cp >= $totalCnt} {
+	if {$update_index_cp >= $total_cnt} {
 		_close_updateindex $fd $after
 		return
 	}
 
 	for {set i $batch} \
-		{$update_index_cp < $totalCnt && $i > 0} \
+		{$update_index_cp < $total_cnt && $i > 0} \
 		{incr i -1} {
-		set path [lindex $pathList $update_index_cp]
+		set path [lindex $path_list $update_index_cp]
 		incr update_index_cp
 		switch -glob -- [lindex $file_states($path) 0] {
 		U? {continue}

@@ -253,7 +253,7 @@ proc write_checkout_index {fd pathList totalCnt batch after} {
 		}
 	}
 
-	$::main_status update $update_index_cp $totalCnt
+	$::main_status update $update_index_cp $total_cnt
 }
 
 proc unstage_helper {txt paths} {

@@ -261,7 +261,7 @@ proc unstage_helper {txt paths} {
 
 	if {![lock_index begin-update]} return
 
-	set pathList [list]
+	set path_list [list]
 	set after {}
 	foreach path $paths {
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -269,19 +269,19 @@ proc unstage_helper {txt paths} {
 		M? -
 		T? -
 		D? {
-			lappend pathList $path
+			lappend path_list $path
 			if {$path eq $current_diff_path} {
 				set after {reshow_diff;}
 			}
 		}
 		}
 	}
-	if {$pathList eq {}} {
+	if {$path_list eq {}} {
 		unlock_index
 	} else {
 		update_indexinfo \
 			$txt \
-			$pathList \
+			$path_list \
 			[concat $after [list ui_ready]]
 	}
 }

@@ -305,7 +305,7 @@ proc add_helper {txt paths} {
 
 	if {![lock_index begin-update]} return
 
-	set pathList [list]
+	set path_list [list]
 	set after {}
 	foreach path $paths {
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -321,19 +321,19 @@ proc add_helper {txt paths} {
 		?M -
 		?D -
 		?T {
-			lappend pathList $path
+			lappend path_list $path
 			if {$path eq $current_diff_path} {
 				set after {reshow_diff;}
 			}
 		}
 		}
 	}
-	if {$pathList eq {}} {
+	if {$path_list eq {}} {
 		unlock_index
 	} else {
 		update_index \
 			$txt \
-			$pathList \
+			$path_list \
 			[concat $after {ui_status [mc "Ready to commit."]}]
 	}
 }

@@ -393,7 +393,7 @@ proc revert_helper {txt paths} {
 
 	if {![lock_index begin-update]} return
 
-	set pathList [list]
+	set path_list [list]
 	set after {}
 	foreach path $paths {
 		switch -glob -- [lindex $file_states($path) 0] {

@@ -401,7 +401,7 @@ proc revert_helper {txt paths} {
 		?M -
 		?T -
 		?D {
-			lappend pathList $path
+			lappend path_list $path
 			if {$path eq $current_diff_path} {
 				set after {reshow_diff;}
 			}

@@ -420,12 +420,12 @@ proc revert_helper {txt paths} {
 	# as they have quite complex plural-form rules. Unfortunately,
 	# msgcat doesn't seem to support that kind of string translation.
 	#
-	set n [llength $pathList]
+	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 $pathList]]]
+		set query [mc "Revert changes in file %s?" [short_path [lindex $path_list]]]
 	} else {
 		set query [mc "Revert changes in these %i files?" $n]
 	}

@@ -444,7 +444,7 @@ proc revert_helper {txt paths} {
 	if {$reply == 1} {
 		checkout_index \
 			$txt \
-			$pathList \
+			$path_list \
 			[concat $after [list ui_ready]]
 	} else {
 		unlock_index

gitgitgadget

From: Jonathan Gilbert <[email protected]>

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.

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

index 0d21f5688b..db02e399e7 100755
@@ -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."]
 	}
 }
 

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

index a1aeb8b96e..888f98bab2 100644
@@ -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
 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.
+	if {$status_operation == {}} {
+		set status_operation [$status start \
+			$cur_s \
+			[mc "lines annotated"]]
+	}
+
 	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} {

@@ -806,10 +813,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} }
 

@@ -1124,7 +1132,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
 		}

index a5228297db..21ea768d80 100644
@@ -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
 }
 

index 28d4d2a54e..62f4773ef4 100644
@@ -45,13 +45,13 @@ proc _close_updateindex {fd after} {
 		wm deiconify $w
 		tkwait window $w
 
-		$::main_status stop
+		$::main_status stop_all
 		unlock_index
 		rescan $after 0
 		return
 	}
 
-	$::main_status stop
+	$::main_status stop_all
 	unlock_index
 	uplevel #0 $after
 }

@@ -67,7 +67,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,15 +81,18 @@ 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} {
+		$status_bar_operation stop
 		_close_updateindex $fd $after
 		return
 	}

@@ -119,7 +122,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 +136,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,15 +150,18 @@ 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} {
+		$status_bar_operation stop
 		_close_updateindex $fd $after
 		return
 	}

@@ -190,7 +196,7 @@ 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} {

@@ -204,7 +210,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,15 +230,18 @@ proc checkout_index {msg path_list after} {
 		$path_list \
 		$total_cnt \
 		$batch \
+		$status_bar_operation \
 		$after \
 		]
 }
 
-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} {
 	global update_index_cp
 	global file_states current_diff_path
 
 	if {$update_index_cp >= $total_cnt} {
+		$status_bar_operation stop
 		_close_updateindex $fd $after
 		return
 	}

@@ -253,7 +262,7 @@ proc write_checkout_index {fd path_list total_cnt batch after} {
 		}
 	}
 
-	$::main_status update $update_index_cp $total_cnt
+	$status_bar_operation update $update_index_cp $total_cnt
 }
 
 proc unstage_helper {txt paths} {

index 9f253db5b3..8df8ffae55 100644
@@ -241,23 +241,27 @@ Continue with resetting the current changes?"]
 	if {[ask_popup $op_question] eq {yes}} {
 		set fd [git_read --stderr read-tree --reset -u -v HEAD]
 		fconfigure $fd -blocking 0 -translation binary
-		fileevent $fd readable [namespace code [list _reset_wait $fd]]
-		$::main_status start [mc "Aborting"] [mc "files reset"]
+		set status_bar_operation [$::main_status \
+			start \
+			[mc "Aborting"] \
+			[mc "files reset"]
+		fileevent $fd readable [namespace code [list \
+			_reset_wait $fd $status_bar_operation]]
 	} else {
 		unlock_index
 	}
 }
 
-proc _reset_wait {fd} {
+proc _reset_wait {fd status_bar_operation} {
 	global ui_comm
 
-	$::main_status update_meter [read $fd]
+	$status_bar_operation update_meter [read $fd]
 
 	fconfigure $fd -blocking 1
 	if {[eof $fd]} {
 		set fail [catch {close $fd} err]
-		$::main_status stop
 		unlock_index
+		$status_bar_operation stop
 
 		$ui_comm delete 0.0 end
 		$ui_comm edit modified false