[2/3] git-gui: Add the ability to revert selected lines
diff mbox series

Message ID 20190819214110.26461-3-me@yadavpratyush.com
State New
Headers show
Series
  • git-gui: Add ability to revert selected hunks and lines
Related show

Commit Message

Pratyush Yadav Aug. 19, 2019, 9:41 p.m. UTC
Just like the user can select lines to stage or unstage, add the
ability to revert selected lines.

Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
---
 git-gui/git-gui.sh   | 25 ++++++++++++++++++++++++-
 git-gui/lib/diff.tcl | 31 ++++++++++++++++++++++++++-----
 2 files changed, 50 insertions(+), 6 deletions(-)

Comments

Johannes Sixt Aug. 20, 2019, 7:21 p.m. UTC | #1
Am 19.08.19 um 23:41 schrieb Pratyush Yadav:
> Just like the user can select lines to stage or unstage, add the
> ability to revert selected lines.
> 
> Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>

> +	if {$revert} {
> +		set query "[mc "Revert changes in file %s?" \
> +			[short_path $current_diff_path]]
> +
> +[mc "The selected lines will be permanently lost by the revert."]"
> +
> +		set reply [revert_dialog $query]

Please don't do this. This confirmation dialog is unacceptable in my
workflow. I use reversals of hunks and lines frequently, almost like a
secondary code editor. My safety net is the undo function of the IDE,
which works across reloads that are triggered by these external edits.
These confirmations get in the way.

> +		if {$reply ne 1} {
> +			unlock_index
> +			return
> +		}
> +	}
> +
>  	set wholepatch {}
>  
>  	while {$first_l < $last_l} {
> 

-- Hannes
Pratyush Yadav Aug. 20, 2019, 7:29 p.m. UTC | #2
On 20/08/19 09:21PM, Johannes Sixt wrote:
> Am 19.08.19 um 23:41 schrieb Pratyush Yadav:
> > Just like the user can select lines to stage or unstage, add the
> > ability to revert selected lines.
> > 
> > Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
> 
> > +	if {$revert} {
> > +		set query "[mc "Revert changes in file %s?" \
> > +			[short_path $current_diff_path]]
> > +
> > +[mc "The selected lines will be permanently lost by the revert."]"
> > +
> > +		set reply [revert_dialog $query]
> 
> Please don't do this. This confirmation dialog is unacceptable in my
> workflow. I use reversals of hunks and lines frequently, almost like a
> secondary code editor. My safety net is the undo function of the IDE,
> which works across reloads that are triggered by these external edits.
> These confirmations get in the way.
 
But not everyone uses an IDE. I use vim and it does not have any such 
undo feature that works across reloads. Not one I'm aware of anyway. It 
is absolutely necessary IMO to ask the user for confirmation before 
deleting their work, unless we have a built in safety net.

So how about adding a config option that allows you to disable the 
confirmation dialog? Sounds like a reasonable compromise to me.

> > +		if {$reply ne 1} {
> > +			unlock_index
> > +			return
> > +		}
> > +	}
> > +
> >  	set wholepatch {}
> >  
> >  	while {$first_l < $last_l} {
> > 
> 
> -- Hannes
Johannes Sixt Aug. 20, 2019, 9:19 p.m. UTC | #3
Am 20.08.19 um 21:29 schrieb Pratyush Yadav:
> On 20/08/19 09:21PM, Johannes Sixt wrote:
>> Please don't do this. This confirmation dialog is unacceptable in my
>> workflow. I use reversals of hunks and lines frequently, almost like a
>> secondary code editor. My safety net is the undo function of the IDE,
>> which works across reloads that are triggered by these external edits.
>> These confirmations get in the way.
>  
> But not everyone uses an IDE. I use vim and it does not have any such 
> undo feature that works across reloads. Not one I'm aware of anyway. It 
> is absolutely necessary IMO to ask the user for confirmation before 
> deleting their work, unless we have a built in safety net.

But you have a safety net built-in: Commit the work, then do the
reversals in amend-mode. Now you can recover old state to your heart's
content. That's recommended anyway if stuff is potentially precious.

> So how about adding a config option that allows you to disable the 
> confirmation dialog? Sounds like a reasonable compromise to me.

That's always an option. Needless to say that I'd prefer it off by
default; I don't need three safety nets.

-- Hannes
Pratyush Yadav Aug. 21, 2019, 9:48 p.m. UTC | #4
On 20/08/19 11:19PM, Johannes Sixt wrote:
> Am 20.08.19 um 21:29 schrieb Pratyush Yadav:
> > On 20/08/19 09:21PM, Johannes Sixt wrote:
> >> Please don't do this. This confirmation dialog is unacceptable in my
> >> workflow. I use reversals of hunks and lines frequently, almost like a
> >> secondary code editor. My safety net is the undo function of the IDE,
> >> which works across reloads that are triggered by these external edits.
> >> These confirmations get in the way.
> >  
> > But not everyone uses an IDE. I use vim and it does not have any such 
> > undo feature that works across reloads. Not one I'm aware of anyway. It 
> > is absolutely necessary IMO to ask the user for confirmation before 
> > deleting their work, unless we have a built in safety net.
> 
> But you have a safety net built-in: Commit the work, then do the
> reversals in amend-mode. Now you can recover old state to your heart's
> content. That's recommended anyway if stuff is potentially precious.

I suppose we disagree on this. I feel very uncomfortable removing the 
prompt by default, because it is pretty easy to mis-click revert instead 
of stage, and all of a sudden lots of your work is gone. It is a pretty 
common workflow to make some changes, stage some hunks in one commit and 
then some others in the next. Not everyone (including me) will first 
commit changes, then amend them, especially if they are not that big or 
complicated. Accidentally deleting your work, no matter how small, 
because of a misclick sucks.

So, I feel strongly in favor of keeping the prompt on by default. I will 
add a config option to disable it for people who are willing to accept 
misclicks. That keeps both sides of the argument happy. You just have to 
disable it once in your global config and you're good to go.

> > So how about adding a config option that allows you to disable the 
> > confirmation dialog? Sounds like a reasonable compromise to me.
> 
> That's always an option. Needless to say that I'd prefer it off by
> default; I don't need three safety nets.
> 
> -- Hannes
Johannes Schindelin Aug. 23, 2019, 1:01 p.m. UTC | #5
Hi,

On Thu, 22 Aug 2019, Pratyush Yadav wrote:

> On 20/08/19 11:19PM, Johannes Sixt wrote:
> > Am 20.08.19 um 21:29 schrieb Pratyush Yadav:
> > > On 20/08/19 09:21PM, Johannes Sixt wrote:
> > >> Please don't do this. This confirmation dialog is unacceptable in my
> > >> workflow. I use reversals of hunks and lines frequently, almost like a
> > >> secondary code editor. My safety net is the undo function of the IDE,
> > >> which works across reloads that are triggered by these external edits.
> > >> These confirmations get in the way.
> > >
> > > But not everyone uses an IDE. I use vim and it does not have any such
> > > undo feature that works across reloads. Not one I'm aware of anyway. It
> > > is absolutely necessary IMO to ask the user for confirmation before
> > > deleting their work, unless we have a built in safety net.
> >
> > But you have a safety net built-in: Commit the work, then do the
> > reversals in amend-mode. Now you can recover old state to your heart's
> > content. That's recommended anyway if stuff is potentially precious.
>
> I suppose we disagree on this. I feel very uncomfortable removing the
> prompt by default, because it is pretty easy to mis-click revert instead
> of stage, and all of a sudden lots of your work is gone. It is a pretty
> common workflow to make some changes, stage some hunks in one commit and
> then some others in the next. Not everyone (including me) will first
> commit changes, then amend them, especially if they are not that big or
> complicated. Accidentally deleting your work, no matter how small,
> because of a misclick sucks.
>
> So, I feel strongly in favor of keeping the prompt on by default. I will
> add a config option to disable it for people who are willing to accept
> misclicks. That keeps both sides of the argument happy. You just have to
> disable it once in your global config and you're good to go.

Maybe the direction taken by this discussion merely suggests that the
design is a bit unfortunate. Why "revert"? Why not "stash" instead? Then
you don't need to have that annoying confirmation dialog.

Ciao,
Johannes

>
> > > So how about adding a config option that allows you to disable the
> > > confirmation dialog? Sounds like a reasonable compromise to me.
> >
> > That's always an option. Needless to say that I'd prefer it off by
> > default; I don't need three safety nets.
> >
> > -- Hannes
>
> --
> Regards,
> Pratyush Yadav
>
Junio C Hamano Aug. 23, 2019, 4:28 p.m. UTC | #6
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Maybe the direction taken by this discussion merely suggests that the
> design is a bit unfortunate. Why "revert"? Why not "stash" instead? Then
> you don't need to have that annoying confirmation dialog.

Interesting, but it would need a bit more tweak than a simple
"stash" for it to be truly helpful, I would imagine.

The primary purpose of stashing from end user's point of view is to
save some changes, that are not immediately usable in the context of
the corrent work, away, so that they can be retrieved later, with a
side effect of wiping the stashed changes from the working tree. The
command could be (re|ab)used to wipe local changes you have in the
working tree, but that would accumulate cruft that you most of the
time (unless you make a mistake) wanted to discard and wanted to
never look at again in the stash. If done using the same 'stash' ref
that is used by the normal "git stash", the stash entries created by
"git stash" because the user really wanted to keep them for later
use would be drowned among these "kept just in case" stash entries
that were created as a side effect of (ab)using stash in place of
"restore".

But "git stash" can be told to use a different ref, so perhaps by
using a separate one for this "just in case" purpose, with the
expectation that the entries are almost always safe to discard
unless the user made a mistake and take it back immediately
(i.e. "undo"), it would not hurt the normal use of "git stash" *and*
give the "revert" necessary safety valve at the same time.

Thanks.
Pratyush Yadav Aug. 23, 2019, 5:03 p.m. UTC | #7
+Cc Bert. This has the suggestion I was talking about in one of my 
previous replies.

On 23/08/19 09:28AM, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Maybe the direction taken by this discussion merely suggests that the
> > design is a bit unfortunate. Why "revert"? Why not "stash" instead? Then
> > you don't need to have that annoying confirmation dialog.
> 
> Interesting, but it would need a bit more tweak than a simple
> "stash" for it to be truly helpful, I would imagine.
> 
> The primary purpose of stashing from end user's point of view is to
> save some changes, that are not immediately usable in the context of
> the corrent work, away, so that they can be retrieved later, with a
> side effect of wiping the stashed changes from the working tree. The
> command could be (re|ab)used to wipe local changes you have in the
> working tree, but that would accumulate cruft that you most of the
> time (unless you make a mistake) wanted to discard and wanted to
> never look at again in the stash. If done using the same 'stash' ref
> that is used by the normal "git stash", the stash entries created by
> "git stash" because the user really wanted to keep them for later
> use would be drowned among these "kept just in case" stash entries
> that were created as a side effect of (ab)using stash in place of
> "restore".

Thank you. I was just about to write exactly this.

> But "git stash" can be told to use a different ref, so perhaps by
> using a separate one for this "just in case" purpose, with the
> expectation that the entries are almost always safe to discard
> unless the user made a mistake and take it back immediately
> (i.e. "undo"), it would not hurt the normal use of "git stash" *and*
> give the "revert" necessary safety valve at the same time.

I propose a simpler solution. Essentially how the revert works is it 
generates a diff and stores that in a variable. It then calls git-apply 
with --reverse passed in case of reverts and unstages, and without the 
--reverse in case of staging, and then feeds git-apply the generated 
diff.

So how about we keep a copy of the diff in another variable. This allows 
us to enable undoing of reverts. The obvious limitations are that 
firstly, unless we use a stack/deque that means only one undo is 
allowed. I'm not sure if using an undo stack would be worth the added 
complexity. Secondly, if the working tree is changed between the revert 
and the undo, there are chances of a merge conflict.

If people are okay with these limitations, we can be rid of the 
confirmation dialog while providing a safety net. Sounds like a good 
idea?
Johannes Sixt Aug. 23, 2019, 7:17 p.m. UTC | #8
Am 23.08.19 um 19:03 schrieb Pratyush Yadav:
> So how about we keep a copy of the diff in another variable. This allows 
> us to enable undoing of reverts. The obvious limitations are that 
> firstly, unless we use a stack/deque that means only one undo is 
> allowed. I'm not sure if using an undo stack would be worth the added 
> complexity. Secondly, if the working tree is changed between the revert 
> and the undo, there are chances of a merge conflict.
> 
> If people are okay with these limitations, we can be rid of the 
> confirmation dialog while providing a safety net. Sounds like a good 
> idea?

Yes, sounds like an idea worth persuing.

-- Hannes

Patch
diff mbox series

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 6de74ce639..2011894bef 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -3611,9 +3611,15 @@  set ui_diff_applyhunk [$ctxm index last]
 lappend diff_actions [list $ctxm entryconf $ui_diff_applyhunk -state]
 $ctxm add command \
 	-label [mc "Apply/Reverse Line"] \
-	-command {apply_range_or_line $cursorX $cursorY; do_rescan}
+	-command {apply_or_revert_range_or_line $cursorX $cursorY 0; do_rescan}
 set ui_diff_applyline [$ctxm index last]
 lappend diff_actions [list $ctxm entryconf $ui_diff_applyline -state]
+$ctxm add command \
+	-label [mc "Revert Line"] \
+	-command {apply_or_revert_range_or_line $cursorX $cursorY 1; do_rescan}
+set ui_diff_revertline [$ctxm index last]
+lappend diff_actions [list $ctxm entryconf $ui_diff_revertline -state]
+set ui_diff_revertline [$ctxm index last]
 $ctxm add separator
 $ctxm add command \
 	-label [mc "Show Less Context"] \
@@ -3711,15 +3717,19 @@  proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} {
 			set l [mc "Unstage Hunk From Commit"]
 			if {$has_range} {
 				set t [mc "Unstage Lines From Commit"]
+				set r [mc "Revert Lines"]
 			} else {
 				set t [mc "Unstage Line From Commit"]
+				set r [mc "Revert Line"]
 			}
 		} else {
 			set l [mc "Stage Hunk For Commit"]
 			if {$has_range} {
 				set t [mc "Stage Lines For Commit"]
+				set r [mc "Revert Lines"]
 			} else {
 				set t [mc "Stage Line For Commit"]
+				set r [mc "Revert Line"]
 			}
 		}
 		if {$::is_3way_diff
@@ -3730,11 +3740,24 @@  proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} {
 			|| [string match {T?} $state]
 			|| [has_textconv $current_diff_path]} {
 			set s disabled
+			set revert_state disabled
 		} else {
 			set s normal
+
+			# Only allow reverting changes in the working tree. If
+			# the user wants to revert changes in the index, they
+			# need to unstage those first.
+			if {$::ui_workdir eq $::current_diff_side} {
+				set revert_state normal
+			} else {
+				set revert_state disabled
+			}
 		}
+
 		$ctxm entryconf $::ui_diff_applyhunk -state $s -label $l
 		$ctxm entryconf $::ui_diff_applyline -state $s -label $t
+		$ctxm entryconf $::ui_diff_revertline -state $revert_state \
+			-label $r
 		tk_popup $ctxm $X $Y
 	}
 }
diff --git a/git-gui/lib/diff.tcl b/git-gui/lib/diff.tcl
index 68c4a6c736..4b2b00df4b 100644
--- a/git-gui/lib/diff.tcl
+++ b/git-gui/lib/diff.tcl
@@ -640,7 +640,7 @@  proc apply_hunk {x y} {
 	}
 }
 
-proc apply_range_or_line {x y} {
+proc apply_or_revert_range_or_line {x y revert} {
 	global current_diff_path current_diff_header current_diff_side
 	global ui_diff ui_index file_states
 
@@ -660,25 +660,46 @@  proc apply_range_or_line {x y} {
 	if {$current_diff_path eq {} || $current_diff_header eq {}} return
 	if {![lock_index apply_hunk]} return
 
-	set apply_cmd {apply --cached --whitespace=nowarn}
+	set apply_cmd {apply --whitespace=nowarn}
 	set mi [lindex $file_states($current_diff_path) 0]
 	if {$current_diff_side eq $ui_index} {
 		set failed_msg [mc "Failed to unstage selected line."]
 		set to_context {+}
-		lappend apply_cmd --reverse
+		lappend apply_cmd --reverse --cached
 		if {[string index $mi 0] ne {M}} {
 			unlock_index
 			return
 		}
 	} else {
-		set failed_msg [mc "Failed to stage selected line."]
-		set to_context {-}
+		if {$revert} {
+			set failed_msg [mc "Failed to revert selected line."]
+			set to_context {+}
+			lappend apply_cmd --reverse
+		} else {
+			set failed_msg [mc "Failed to stage selected line."]
+			set to_context {-}
+			lappend apply_cmd --cached
+		}
+
 		if {[string index $mi 1] ne {M}} {
 			unlock_index
 			return
 		}
 	}
 
+	if {$revert} {
+		set query "[mc "Revert changes in file %s?" \
+			[short_path $current_diff_path]]
+
+[mc "The selected lines will be permanently lost by the revert."]"
+
+		set reply [revert_dialog $query]
+		if {$reply ne 1} {
+			unlock_index
+			return
+		}
+	}
+
 	set wholepatch {}
 
 	while {$first_l < $last_l} {