Hi Jonathan,

Thanks for the re-roll.

[I removed some parts of the diff to make the reply easier to read. I am 
implicitly OK with the removed parts.]

On 13/11/19 09:56AM, Jonathan Gilbert via GitGitGadget wrote:

From: Jonathan Gilbert <redacted>

Update the revert_helper proc to check for untracked files as well as
changes, and then handle changes to be reverted and untracked files with
independent blocks of code. Prompt the user independently for untracked
files, since the underlying action is fundamentally different (rm -f).
If after deleting untracked files, the directory containing them becomes
empty, then remove the directory as well. Migrate unlocking of the index
out of _close_updateindex to a responsibility of the caller, to permit
paths that don't directly unlock the index, and refactor the error
handling added in d4e890e5 so that callers can make flow control
decisions in the event of errors.

A new proc delete_files takes care of actually deleting the files in
batches, using the Tcler's Wiki recommended approach for keeping the UI
responsive.

Since the checkout_index and delete_files calls are both asynchronous
and could potentially complete in any order, a "chord" is used to
coordinate unlocking the index and returning the UI to a usable state
only after both operations are complete. The `SimpleChord` class,
based on TclOO (Tcl/Tk 8.6), is added in this commit.

Looks much better!
 

I didn't spot this earlier. Will this call to 'stop' interfere with the 
'start' in 'delete_files'?

+}
 
-		$::main_status stop
-		unlock_index
-		rescan $after 0
-		return
-	}
+proc rescan_on_error {err} {
+	global use_ttk NS
+
+	set w .indexfried
+	Dialog $w
+	wm withdraw $w
+	wm title $w [strcat "[appname] ([reponame]): " [mc "Index Error"]]
+	wm geometry $w "+[winfo rootx .]+[winfo rooty .]"
+	set s [mc "Updating the Git index failed.  A rescan will be automatically started to resynchronize git-gui."]
+	text $w.msg -yscrollcommand [list $w.vs set] \
+		-width [string length $s] -relief flat \
+		-borderwidth 0 -highlightthickness 0 \
+		-background [get_bg_color $w]
+	$w.msg tag configure bold -font font_uibold -justify center
+	${NS}::scrollbar $w.vs -command [list $w.msg yview]
+	$w.msg insert end $s bold \n\n$err {}
+	$w.msg configure -state disabled
+
+	${NS}::button $w.continue \
+		-text [mc "Continue"] \
+		-command [list destroy $w]
+	${NS}::button $w.unlock \
+		-text [mc "Unlock Index"] \
+		-command "destroy $w; _delete_indexlock"
+	grid $w.msg - $w.vs -sticky news
+	grid $w.unlock $w.continue - -sticky se -padx 2 -pady 2
+	grid columnconfigure $w 0 -weight 1
+	grid rowconfigure $w 0 -weight 1
+
+	wm protocol $w WM_DELETE_WINDOW update
+	bind $w.continue <Visibility> "
+		grab $w
+		focus %W
+	"
+	wm deiconify $w
+	tkwait window $w
 
 	$::main_status stop

Same question here.

This changes when $after is called. If you pass it to 'rescan', it runs 
_after_ the rescan is finished. Now it runs "in parallel" with it. Are 
you sure that is the intended behaviour? Should we just stick to passing 
$after to rescan on failure?

While we're here, how about just moving this entire thing to 
'_close_updateindex_rescan_on_error', since the only two consumers of 
the function do the _exact_ same thing?

This would also allow us to pass $after to 'rescan'. It would also 
hopefully make the code a bit easier to follow because you can clearly 
see that we only unlock the index when there is no error.

Even better, unlock the index unconditionally in 
'_close_updateindex_rescan_on_error', and remove the 'unlock_index' call 
from 'rescan_on_error'. I generally prefer to keep locking/unlocking 
paths as simple as possible.

Nitpick: Please explicitly mention why we _don't_ want to unlock the 
index here.

There are two function very similar to this one: 'write_update_index' 
and 'write_update_indexinfo'. This subtle but important difference is 
very easy to gloss over.

 		return
 	}
 

This patch is almost ready to be merged. Looking forward to the 
(hopefully) final iteration of this topic :)

-- 
Regards,
Pratyush Yadav