Allow the 'revert' option in Git Gui to operate on untracked files, deleting them

From: Jonathan Gilbert <[email protected]>

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. I have typically had to explicitly delete those files externally to Git Gui, and I want to be able to just select those newly- created untracked files and "revert" them into oblivion.

This change updates the revert_helper function to check for untracked files as well as changes, and then any 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.

This introduces new strings in index.tcl. I have been told that there is a separate process whereby the translations get updated.

Signed-off-by Jonathan Gilbert <[email protected]>

---
 git-gui/lib/index.tcl | 139 +++++++++++++++++++++++++++++++-----------
 1 file changed, 104 insertions(+), 35 deletions(-)

git-gui/lib/index.tcl +0 -0

index e07b7a3762..cc1b651d56 100644

git-gui/lib/index.tcl +77 -35

 	set after {}
 	foreach path $paths {
 		switch -glob -- [lindex $file_states($path) 0] {
 		U? {continue}
+		?O {
+			lappend untrackedList $path
+			if {$path eq $current_diff_path} {
+				set after {reshow_diff;}
+			}
+		}
 		?M -
 		?T -
 		?D {

@@ -410,45 +423,101 @@ proc revert_helper {txt paths} {
 	}
 
 
-	# 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 $pathList]
-	if {$n == 0} {
-		unlock_index
-		return
-	} elseif {$n == 1} {
-		set query [mc "Revert changes in file %s?" [short_path [lindex $pathList]]]
-	} else {
-		set query [mc "Revert changes in these %i files?" $n]
-	}
+	set numPaths [llength $pathList]
+	set numUntracked [llength $untrackedList]
 
-	set reply [tk_dialog \
-		.confirm_revert \
-		"[appname] ([reponame])" \
-		"$query
+	if {$numPaths > 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 {$numPaths == 1} {
+			set query [mc "Revert changes in file %s?" [short_path [lindex $pathList]]]
+		} else {
+			set query [mc "Revert changes in these %i files?" $numPaths]
+		}
+
+		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 \
-			$pathList \
-			[concat $after [list ui_ready]]
-	} else {
-		unlock_index
+			question \
+			1 \
+			[mc "Do Nothing"] \
+			[mc "Revert Changes"] \
+			]
+
+		if {$reply == 1} {
+			checkout_index \
+				$txt \
+				$pathList \
+				[concat $after [list ui_ready]]
+
+			already_unlocked
+		}
+	}
+
+	if {$numUntracked > 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 {$numUntracked == 1} {
+			set query [mc "Delete untracked file %s?" [short_path [lindex $untrackedList]]]
+		} else {
+			set query [mc "Delete these %i untracked files?" $numUntracked]
+		}
+
+		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} {
+			file delete -- {*}$untrackedList
+
+			foreach path $untrackedList {
+				set directoryPath [file dirname $path]
+
+				while {$directoryPath != $path} {
+					set contents [glob -nocomplain -dir $path *]

While git-gui is distributed in the main Git tree, the development happens on a separate repo, and the Git maintainer periodically pulls in changes from that repo. It can be found at [0]. For now, I munged your patch to apply on my tree, but please base it on the git-gui repo for your re-rolls or future patches. You can use GitGitGadget to do that [1].

Now, on to the patch.

Quoted 8 lines · Jonathan Gilbert via GitGitGadget

From: Jonathan Gilbert <[email protected]>

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. I have typically had to explicitly delete those files externally to Git Gui, and I want to be able to just select those newly- created untracked files and "revert" them into oblivion.

I think the description of your workflow belongs in the cover letter more than here. The commit message should take a more neutral tone. So, describe the problem in an objective way that not only you, but other git-gui users might face.

Quoted 1 line

+ # If an action is taken that implicitly unlocks the index, this gets cleared. Either way, it is executed at the end of the procedure.

The convention is to wrap lines at 80 columns wherever possible. Please follow that. You can look at the rest of the code for examples.

You have other lines too that are too long. The same comment applies to all those.

git-gui/lib/index.tcl proc revert_helper {txt paths} { · line 400

Quoted 4 lines

git-gui/lib/index.tcl +4 -0

+     set epilogue [list]
+     lappend epilogue {unlock_index}
+
+     proc already_unlocked {} { upvar epilogue epilogue; set epilogue [lsearch -inline -all -not -exact $epilogue {unlock_index}] }

A procedure defined inside a procedure? Please don't do that. Define it outside.

Also, what is this procedure supposed to do? It is not very clear at first read.

git-gui/lib/index.tcl proc revert_helper {txt paths} { · line 403

Quoted 3 lines

git-gui/lib/index.tcl +2 -0

+
      set pathList [list]
+     set untrackedList [list]

Nitpick: Ugh! camelCase in a sea of snake_cases. What's even more unfortunate is that pathList itself is in camelCase, so that's probably the reason you went with camelCase in the first place. Maybe re-name pathList to path_list while we're at it, and then use snake_case everywhere?

git-gui/lib/index.tcl proc revert_helper {txt paths} { · line 427

Quoted 2 lines

git-gui/lib/index.tcl +2 -0

+     set numPaths [llength $pathList]
+     set numUntracked [llength $untrackedList]

git-gui/lib/index.tcl proc revert_helper {txt paths} { · line 509

Quoted 7 lines

git-gui/lib/index.tcl +7 -0

+                                     try {
+                                             file delete -- $path
+                                     }
+                                     catch {
+                                             # This is just a best effort, don't annoy the user with failure to remove empty directories.
+                                             break
+                                     }

The convention in this project is to just use catch, and not try. So something like:

catch {file delete -- $path}

git-gui/lib/index.tcl proc revert_helper {txt paths} { · line 512

Quoted 2 lines

git-gui/lib/index.tcl +2 -0

+                                     set path $directoryPath
+                                     set directoryPath [file dirname $path]

I read this loop as "if all the paths in a directory are removed, remove the empty directory as well". Do I read correctly?

Will there be problems in deleting the directory? What if the user wants to keep the directory, and just delete the files? Is that even a valid use-case?

git-gui/lib/index.tcl proc revert_helper {txt paths} { · line 516

Quoted 4 lines

git-gui/lib/index.tcl +4 -0

+                             }
+                     }
+
+                     lappend epilogue {ui_do_rescan}

A rescan is an expensive operation, so we should use it judiciously. Are you sure it is really needed? The "Revert" code does not do a rescan but still manages to update the list of "unstaged files". How does it manage that? Can the new code do something similar?

git-gui/lib/index.tcl proc revert_helper {txt paths} { · line 520

Quoted 4 lines

git-gui/lib/index.tcl +3 -0

+             }
      }
+
+     foreach epilogueCommand $epilogue { {*}$epilogueCommand }

Why not use eval [2]? Are there any downsides to that compared to your way? If not, use eval. At least it means better readability if nothing else.

As far as I see, you use $epilogue for two things: unlocking the index and rescanning. Can you move the control flow around that both can be done in the "normal" way. That is, they are not a part of a list of things to do at the end, but instead are done when needed. For example, just move the call to unlock_index at the end instead of putting it in epilogue. Can the same be done for ui_do_rescan (if you do go with a rescan instead of doing it like the existing revert does)?

Quoted 3 lines

}

proc do_revert_selection {} {

While I appreciate the idea of such a feature, I'm surprised by how complex the implementation is. I expected something much simpler. The complexity can probably be managed a bit better by moving the control flow around.

I couldn't dive in the code as deep as I wanted to because I don't have too much time on my hands. But maybe I'll look further by the time your re-roll arrives. Thanks.

[0] https://github.com/prati0100/git-gui
[1] https://github.com/prati0100/git-gui#using-gitgitgadget
[2] https://www.tcl.tk/man/tcl8.6/TclCmd/eval.htm

--
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. I have typically had to explicitly delete those files externally to Git Gui, and I want to be able to just select those newly-created untracked files and "revert" them into oblivion.

Is there a reason these new files can't be ignored, with one of the.gitignore file, .git/info/exclude, or core.excludesFile?

Quoted 5 lines · Jonathan Gilbert via GitGitGadget

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. I have typically had to explicitly delete those files externally to Git Gui, and I want to be able to just select those newly-created untracked files and "revert" them into oblivion.

Is there a reason these new files can't be ignored, with one of the .gitignore file, .git/info/exclude, or core.excludesFile?

If so, it would be helpful to explain that in the commit message so we can more fully understand the rationale here.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

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. I have typically had to explicitly delete those files externally to Git Gui, and I want to be able to just select those newly-created untracked files and "revert" them into oblivion.

This change updates the revert_helper function to check for untracked files as well as changes, and then any 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.

This introduces new strings in index.tcl. I have been told that there is a separate process whereby the translations get updated.

Jonathan Gilbert (1):
git-gui: Revert untracked files by deleting them

 git-gui/lib/index.tcl | 139 +++++++++++++++++++++++++++++++-----------
 1 file changed, 104 insertions(+), 35 deletions(-)

Quoted 7 lines · Jonathan Gilbert via GitGitGadget

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. I have typically had to explicitly delete those files externally to Git Gui, and I want to be able to just select those newly-created untracked files and "revert" them into oblivion.

I have an entry in the 'Tools" menu for this called 'Delete':

[guitool "Delete"]
cmd = rm -f \"$FILENAME\"
noconsole = yes
needsfile = yes
confirm = yes

Best,
Bert

Quoted 18 lines

This change updates the revert_helper function to check for untracked files as well as changes, and then any 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.

This introduces new strings in index.tcl. I have been told that there is a separate process whereby the translations get updated.

Jonathan Gilbert (1):
git-gui: Revert untracked files by deleting them

 git-gui/lib/index.tcl | 139 +++++++++++++++++++++++++++++++-----------
 1 file changed, 104 insertions(+), 35 deletions(-)

Quoted 1 line

I have an entry in the "Tools" menu for this called 'Delete':

That's kind of neat, I wasn't aware of that facet of Git Gui :-) But, it isn't quite the same feature:

That's kind of neat, I wasn't aware of that facet of Git Gui :-) But, it isn't quite the same feature:

* It has to be manually set up on each installation.
* It invokes an external process, I don't know if it's safe to assume that "rm" will work on all platforms (though I just tested it on my Windows installation and it worked).
* It doesn't remove directories that it makes empty.
* I don't see a way to bind it to a keyboard shortcut. That could just be me not knowing enough about custom tools, though. :-)
* It only processes the first file selected.
* If I select a tracked file, it will still delete it, and the feature I'm looking for is more of a "return repository to clean state" type function, like "revert" already is but extended to handle files that you can't actually "git revert".

Thanks,

Jonathan Gilbert

Quoted 40 lines · On Tue, Oct 29, 2019 at 9:32 AM Bert Wesarg bert.wesarg-at-googlemail.com…

Quoted 7 lines · Jonathan Gilbert via GitGitGadget

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. I have typically had to explicitly delete those files externally to Git Gui, and I want to be able to just select those newly-created untracked files and "revert" them into oblivion.

I have an entry in the 'Tools" menu for this called 'Delete':

[guitool "Delete"]
cmd = rm -f \"$FILENAME\"
noconsole = yes
needsfile = yes
confirm = yes

Best,
Bert

Quoted 18 lines

This change updates the revert_helper function to check for untracked files as well as changes, and then any 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.

This introduces new strings in index.tcl. I have been told that there is a separate process whereby the translations get updated.

Jonathan Gilbert (1):
git-gui: Revert untracked files by deleting them

 git-gui/lib/index.tcl | 139 +++++++++++++++++++++++++++++++-----------
 1 file changed, 104 insertions(+), 35 deletions(-)

That's kind of neat, I wasn't aware of that facet of Git Gui :-) But, it isn't quite the same feature:

* It has to be manually set up on each installation.
* It invokes an external process, I don't know if it's safe to assume that "rm" will work on all platforms (though I just tested it on my Windows installation and it worked).
* It doesn't remove directories that it makes empty.
* I don't see a way to bind it to a keyboard shortcut. That could just be me not knowing enough about custom tools, though. :-)

* It only processes the first file selected.
* If I select a tracked file, it will still delete it, and the feature I'm looking for is more of a "return repository to clean state" type function, like "revert" already is but extended to handle files that you can't actually "git revert".