| Message ID | 20190819214110.26461-3-me@yadavpratyush.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-gui: Add ability to revert selected hunks and lines | expand |
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
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
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
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
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 >
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.
+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?
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
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} {
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(-)