| Message ID | 20190828215725.13376-5-me@yadavpratyush.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-gui: Add ability to revert selected hunks and lines | expand |
Dear Pratyush, I just noticed that the 'Revert Last Hunk' menu entry is enabled in the stage-list. But I think it should be disabled, like the 'Revert Hunk' and 'Revert Line' menu entry. Can you confirm this? Thanks. Bert On Wed, Aug 28, 2019 at 11:57 PM Pratyush Yadav <me@yadavpratyush.com> wrote: > > Accidental clicks on the revert hunk/lines buttons can cause loss of > work, and can be frustrating. So, allow undoing the last revert. > > Right now, a stack or deque are not being used for the sake of > simplicity, so only one undo is possible. Any reverts before the > previous one are lost. > > Signed-off-by: Pratyush Yadav <me@yadavpratyush.com> > --- > git-gui.sh | 18 +++++++++++++++++- > lib/diff.tcl | 53 ++++++++++++++++++++++++++++++++++++++++++++++++---- > 2 files changed, 66 insertions(+), 5 deletions(-) > > diff --git a/git-gui.sh b/git-gui.sh > index 1592544..e03a2d2 100755 > --- a/git-gui.sh > +++ b/git-gui.sh > @@ -1350,6 +1350,8 @@ set is_submodule_diff 0 > set is_conflict_diff 0 > set selected_commit_type new > set diff_empty_count 0 > +set last_revert {} > +set last_revert_enc {} > > set nullid "0000000000000000000000000000000000000000" > set nullid2 "0000000000000000000000000000000000000001" > @@ -3601,6 +3603,11 @@ $ctxm add command \ > -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] > +$ctxm add command \ > + -label [mc "Undo Last Revert"] \ > + -command {undo_last_revert; do_rescan} > +set ui_diff_undorevert [$ctxm index last] > +lappend diff_actions [list $ctxm entryconf $ui_diff_undorevert -state] > $ctxm add separator > $ctxm add command \ > -label [mc "Show Less Context"] \ > @@ -3680,7 +3687,7 @@ proc has_textconv {path} { > } > > proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} { > - global current_diff_path file_states > + global current_diff_path file_states last_revert > set ::cursorX $x > set ::cursorY $y > if {[info exists file_states($current_diff_path)]} { > @@ -3694,6 +3701,7 @@ proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} { > tk_popup $ctxmsm $X $Y > } else { > set has_range [expr {[$::ui_diff tag nextrange sel 0.0] != {}}] > + set u [mc "Undo Last Revert"] > if {$::ui_index eq $::current_diff_side} { > set l [mc "Unstage Hunk From Commit"] > set h [mc "Revert Hunk"] > @@ -3739,12 +3747,20 @@ proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} { > } > } > > + if {$last_revert eq {}} { > + set undo_state disabled > + } else { > + set undo_state normal > + } > + > $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 > $ctxm entryconf $::ui_diff_reverthunk -state $revert_state \ > -label $h > + $ctxm entryconf $::ui_diff_undorevert -state $undo_state \ > + -label $u > > tk_popup $ctxm $X $Y > } > diff --git a/lib/diff.tcl b/lib/diff.tcl > index 0659029..96288fc 100644 > --- a/lib/diff.tcl > +++ b/lib/diff.tcl > @@ -569,7 +569,7 @@ proc read_diff {fd conflict_size cont_info} { > > proc apply_or_revert_hunk {x y revert} { > global current_diff_path current_diff_header current_diff_side > - global ui_diff ui_index file_states > + global ui_diff ui_index file_states last_revert last_revert_enc > > if {$current_diff_path eq {} || $current_diff_header eq {}} return > if {![lock_index apply_hunk]} return > @@ -610,18 +610,25 @@ proc apply_or_revert_hunk {x y revert} { > set e_lno end > } > > + set wholepatch "$current_diff_header[$ui_diff get $s_lno $e_lno]" > + > if {[catch { > set enc [get_path_encoding $current_diff_path] > set p [eval git_write $apply_cmd] > fconfigure $p -translation binary -encoding $enc > - puts -nonewline $p $current_diff_header > - puts -nonewline $p [$ui_diff get $s_lno $e_lno] > + puts -nonewline $p $wholepatch > close $p} err]} { > error_popup "$failed_msg\n\n$err" > unlock_index > return > } > > + if {$revert} { > + # Save a copy of this patch for undoing reverts. > + set last_revert $wholepatch > + set last_revert_enc $enc > + } > + > $ui_diff conf -state normal > $ui_diff delete $s_lno $e_lno > $ui_diff conf -state disabled > @@ -653,7 +660,7 @@ proc apply_or_revert_hunk {x y revert} { > > 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 > + global ui_diff ui_index file_states last_revert > > set selected [$ui_diff tag nextrange sel 0.0] > > @@ -852,5 +859,43 @@ proc apply_or_revert_range_or_line {x y revert} { > return > } > > + if {$revert} { > + # Save a copy of this patch for undoing reverts. > + set last_revert $current_diff_header$wholepatch > + set last_revert_enc $enc > + } > + > + unlock_index > +} > + > +# Undo the last line/hunk reverted. When hunks and lines are reverted, a copy > +# of the diff applied is saved. Re-apply that diff to undo the revert. > +# > +# Right now, we only use a single variable to hold the copy, and not a > +# stack/deque for simplicity, so multiple undos are not possible. Maybe this > +# can be added if the need for something like this is felt in the future. > +proc undo_last_revert {} { > + global last_revert current_diff_path current_diff_header > + global last_revert_enc > + > + if {$last_revert eq {}} return > + if {![lock_index apply_hunk]} return > + > + set apply_cmd {apply --whitespace=nowarn} > + set failed_msg [mc "Failed to undo last revert."] > + > + if {[catch { > + set enc $last_revert_enc > + set p [eval git_write $apply_cmd] > + fconfigure $p -translation binary -encoding $enc > + puts -nonewline $p $last_revert > + close $p} err]} { > + error_popup "$failed_msg\n\n$err" > + unlock_index > + return > + } > + > + set last_revert {} > + > unlock_index > } > -- > 2.21.0 >
On 21/10/19 11:16AM, Bert Wesarg wrote: > Dear Pratyush, > > I just noticed that the 'Revert Last Hunk' menu entry is enabled in > the stage-list. But I think it should be disabled, like the 'Revert > Hunk' and 'Revert Line' menu entry. I'm not sure what you mean. There is no "Revert Last Hunk" menu entry (I assume you are talking about the context menu in the diff view that we open by right clicking). My guess is that you mean the "Undo Last Revert" option. And you want it disabled if the diff shown is of a staged file, correct? I'm not sure if disabling it would be a good idea. Say I revert a hunk or line while the file is not staged, and stage the rest of the file. This would mean that file is no longer in the "Unstaged Changes" widget. Now if I choose the file from the "Staged Changes" widget, I get the option to undo the last revert. If I hit that, it will put whatever I reverted in the "Unstaged Changes" widget. So, now part of the file that was reverted is in "Unstaged Changes", and the rest in "Unstaged Changes". IIUC, this is what you think should not happen, correct? What's wrong with allowing the user to undo reverts from anywhere? The way I see it, it doesn't really matter what file is selected or whether it is staged or not, the action of the undo remains the same, so it makes sense to me to allow it from anywhere. > Can you confirm this? > > On Wed, Aug 28, 2019 at 11:57 PM Pratyush Yadav <me@yadavpratyush.com> > wrote: > > > > Accidental clicks on the revert hunk/lines buttons can cause loss of > > work, and can be frustrating. So, allow undoing the last revert. > > > > Right now, a stack or deque are not being used for the sake of > > simplicity, so only one undo is possible. Any reverts before the > > previous one are lost. > > > > Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
Am 21.10.19 um 11:16 schrieb Bert Wesarg: > Dear Pratyush, > > I just noticed that the 'Revert Last Hunk' menu entry is enabled in > the stage-list. But I think it should be disabled, like the 'Revert > Hunk' and 'Revert Line' menu entry. > > Can you confirm this? Technically, it need not be disabled because the hunk being reverted does not depend on the contents of any of diffs that can be shown. The entry should be disabled if reverting the stored hunk fails. But to know that, it would have to be tried: the file could have been edited since the hunk was generated so that the reversal of the hunk fails. Not sure what to do, though. -- Hannes
On Mon, Oct 21, 2019 at 9:35 PM Johannes Sixt <j6t@kdbg.org> wrote: > > Am 21.10.19 um 11:16 schrieb Bert Wesarg: > > Dear Pratyush, > > > > I just noticed that the 'Revert Last Hunk' menu entry is enabled in > > the stage-list. But I think it should be disabled, like the 'Revert > > Hunk' and 'Revert Line' menu entry. > > > > Can you confirm this? > > Technically, it need not be disabled because the hunk being reverted > does not depend on the contents of any of diffs that can be shown. > > The entry should be disabled if reverting the stored hunk fails. But to > know that, it would have to be tried: the file could have been edited > since the hunk was generated so that the reversal of the hunk fails. But the "Undo" changes the worktree not the stage, sure it indirectly also changes the view of the staged content, but that is only a secondary effect. As I only can revert in the worktree list, I think we should be consistent and also only allow to undo the revert in the worktree list. And I think it is independent of 'does the undo apply at all' question. Bert > > Not sure what to do, though. > > -- Hannes
On Mon, Oct 21, 2019 at 9:04 PM Pratyush Yadav <me@yadavpratyush.com> wrote: > > On 21/10/19 11:16AM, Bert Wesarg wrote: > > Dear Pratyush, > > > > I just noticed that the 'Revert Last Hunk' menu entry is enabled in > > the stage-list. But I think it should be disabled, like the 'Revert > > Hunk' and 'Revert Line' menu entry. > > I'm not sure what you mean. There is no "Revert Last Hunk" menu entry (I > assume you are talking about the context menu in the diff view that we > open by right clicking). > > My guess is that you mean the "Undo Last Revert" option. And you want it > disabled if the diff shown is of a staged file, correct? > > I'm not sure if disabling it would be a good idea. > > Say I revert a hunk or line while the file is not staged, and stage the > rest of the file. This would mean that file is no longer in the > "Unstaged Changes" widget. Now if I choose the file from the "Staged > Changes" widget, I get the option to undo the last revert. If I hit > that, it will put whatever I reverted in the "Unstaged Changes" widget. > So, now part of the file that was reverted is in "Unstaged Changes", and > the rest in "Unstaged Changes". > Sorry for this confusion. > IIUC, this is what you think should not happen, correct? What's wrong > with allowing the user to undo reverts from anywhere? The 'Undo' changes the worktree not the staged content,. > > The way I see it, it doesn't really matter what file is selected or > whether it is staged or not, the action of the undo remains the same, so > it makes sense to me to allow it from anywhere. It would make sense to undo the revert on the staged content, if there are no more changes to this file in the worktree. I.e., you wont be able to apply the 'undo' anymore to the worktree file if it is not listed anymore. Though even that case should be able to implement. Though the undo is currently not bound to a specific path. This may be the cause of our different view. I though the undo is bound to the path it was recorded, thus also would only be available when showing a diff on this path again. Therefore I think, having the 'Undo Last Revert' in the context menu may not be the best place to begin with, or at least indicate that this 'undo' operation does not necceseritly operate on the file currently shown. Bertt > > > Can you confirm this? > > > > On Wed, Aug 28, 2019 at 11:57 PM Pratyush Yadav <me@yadavpratyush.com> > > wrote: > > > > > > Accidental clicks on the revert hunk/lines buttons can cause loss of > > > work, and can be frustrating. So, allow undoing the last revert. > > > > > > Right now, a stack or deque are not being used for the sake of > > > simplicity, so only one undo is possible. Any reverts before the > > > previous one are lost. > > > > > > Signed-off-by: Pratyush Yadav <me@yadavpratyush.com> > > -- > Regards, > Pratyush Yadav
On 22/10/19 10:17AM, Bert Wesarg wrote: > On Mon, Oct 21, 2019 at 9:04 PM Pratyush Yadav <me@yadavpratyush.com> wrote: > > > > On 21/10/19 11:16AM, Bert Wesarg wrote: > > > Dear Pratyush, > > > > > > I just noticed that the 'Revert Last Hunk' menu entry is enabled in > > > the stage-list. But I think it should be disabled, like the 'Revert > > > Hunk' and 'Revert Line' menu entry. > > > > I'm not sure what you mean. There is no "Revert Last Hunk" menu entry (I > > assume you are talking about the context menu in the diff view that we > > open by right clicking). > > > > My guess is that you mean the "Undo Last Revert" option. And you want it > > disabled if the diff shown is of a staged file, correct? > > > > I'm not sure if disabling it would be a good idea. > > > > Say I revert a hunk or line while the file is not staged, and stage the > > rest of the file. This would mean that file is no longer in the > > "Unstaged Changes" widget. Now if I choose the file from the "Staged > > Changes" widget, I get the option to undo the last revert. If I hit > > that, it will put whatever I reverted in the "Unstaged Changes" widget. > > So, now part of the file that was reverted is in "Unstaged Changes", and > > the rest in "Unstaged Changes". > > > > Sorry for this confusion. > > > IIUC, this is what you think should not happen, correct? What's wrong > > with allowing the user to undo reverts from anywhere? > > The 'Undo' changes the worktree not the staged content,. > > > > > The way I see it, it doesn't really matter what file is selected or > > whether it is staged or not, the action of the undo remains the same, so > > it makes sense to me to allow it from anywhere. > > It would make sense to undo the revert on the staged content, if there > are no more changes to this file in the worktree. I.e., you wont be > able to apply the 'undo' anymore to the worktree file if it is not > listed anymore. Though even that case should be able to implement. > Though the undo is currently not bound to a specific path. This may be I did it this way because I thought it would be best to go for the simplest implementation first, and then re-evaluate if needed. And nobody objected in the reviews. Maybe I didn't do a good job of clarifying the exact behaviour in the commit message. I'm not opposed to something like a per-path undo though. But I'm not sure if that will cause any confusion as to what is being undone. Or maybe the current version is more confusing. I can't really say since I am the one who implemented it, so I know exactly what it does. > the cause of our different view. I though the undo is bound to the > path it was recorded, thus also would only be available when showing a > diff on this path again. Therefore I think, having the 'Undo Last > Revert' in the context menu may not be the best place to begin with, > or at least indicate that this 'undo' operation does not necceseritly > operate on the file currently shown. Where else would you put it if not the context menu? There is the option of putting it in the menu bar at the top under the "Edit" menu entry, but I think that is too hidden. The original idea of the feature was that you could quickly undo an accidental revert. Hiding this in the menu bar would hurt discoverability, and some people might not realize they could have undone the revert. Is there a better place to put it that I'm missing?
On 22/10/19 09:46AM, Bert Wesarg wrote: > On Mon, Oct 21, 2019 at 9:35 PM Johannes Sixt <j6t@kdbg.org> wrote: > > > > Am 21.10.19 um 11:16 schrieb Bert Wesarg: > > > Dear Pratyush, > > > > > > I just noticed that the 'Revert Last Hunk' menu entry is enabled in > > > the stage-list. But I think it should be disabled, like the 'Revert > > > Hunk' and 'Revert Line' menu entry. > > > > > > Can you confirm this? > > > > Technically, it need not be disabled because the hunk being reverted > > does not depend on the contents of any of diffs that can be shown. > > > > The entry should be disabled if reverting the stored hunk fails. But to > > know that, it would have to be tried: the file could have been edited > > since the hunk was generated so that the reversal of the hunk fails. > > But the "Undo" changes the worktree not the stage, sure it indirectly > also changes the view of the staged content, but that is only a I don't think the "Undo Last Revert" should affect "staged content" in any way. In fact, if it does, it is probably a bug. A more detailed reply is to your other email. I just wanted to clarify that an undo _should not_ affect the staged content. > secondary effect. As I only can revert in the worktree list, I think > we should be consistent and also only allow to undo the revert in the > worktree list. > > And I think it is independent of 'does the undo apply at all' question.
diff --git a/git-gui.sh b/git-gui.sh index 1592544..e03a2d2 100755 --- a/git-gui.sh +++ b/git-gui.sh @@ -1350,6 +1350,8 @@ set is_submodule_diff 0 set is_conflict_diff 0 set selected_commit_type new set diff_empty_count 0 +set last_revert {} +set last_revert_enc {} set nullid "0000000000000000000000000000000000000000" set nullid2 "0000000000000000000000000000000000000001" @@ -3601,6 +3603,11 @@ $ctxm add command \ -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] +$ctxm add command \ + -label [mc "Undo Last Revert"] \ + -command {undo_last_revert; do_rescan} +set ui_diff_undorevert [$ctxm index last] +lappend diff_actions [list $ctxm entryconf $ui_diff_undorevert -state] $ctxm add separator $ctxm add command \ -label [mc "Show Less Context"] \ @@ -3680,7 +3687,7 @@ proc has_textconv {path} { } proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} { - global current_diff_path file_states + global current_diff_path file_states last_revert set ::cursorX $x set ::cursorY $y if {[info exists file_states($current_diff_path)]} { @@ -3694,6 +3701,7 @@ proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} { tk_popup $ctxmsm $X $Y } else { set has_range [expr {[$::ui_diff tag nextrange sel 0.0] != {}}] + set u [mc "Undo Last Revert"] if {$::ui_index eq $::current_diff_side} { set l [mc "Unstage Hunk From Commit"] set h [mc "Revert Hunk"] @@ -3739,12 +3747,20 @@ proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} { } } + if {$last_revert eq {}} { + set undo_state disabled + } else { + set undo_state normal + } + $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 $ctxm entryconf $::ui_diff_reverthunk -state $revert_state \ -label $h + $ctxm entryconf $::ui_diff_undorevert -state $undo_state \ + -label $u tk_popup $ctxm $X $Y } diff --git a/lib/diff.tcl b/lib/diff.tcl index 0659029..96288fc 100644 --- a/lib/diff.tcl +++ b/lib/diff.tcl @@ -569,7 +569,7 @@ proc read_diff {fd conflict_size cont_info} { proc apply_or_revert_hunk {x y revert} { global current_diff_path current_diff_header current_diff_side - global ui_diff ui_index file_states + global ui_diff ui_index file_states last_revert last_revert_enc if {$current_diff_path eq {} || $current_diff_header eq {}} return if {![lock_index apply_hunk]} return @@ -610,18 +610,25 @@ proc apply_or_revert_hunk {x y revert} { set e_lno end } + set wholepatch "$current_diff_header[$ui_diff get $s_lno $e_lno]" + if {[catch { set enc [get_path_encoding $current_diff_path] set p [eval git_write $apply_cmd] fconfigure $p -translation binary -encoding $enc - puts -nonewline $p $current_diff_header - puts -nonewline $p [$ui_diff get $s_lno $e_lno] + puts -nonewline $p $wholepatch close $p} err]} { error_popup "$failed_msg\n\n$err" unlock_index return } + if {$revert} { + # Save a copy of this patch for undoing reverts. + set last_revert $wholepatch + set last_revert_enc $enc + } + $ui_diff conf -state normal $ui_diff delete $s_lno $e_lno $ui_diff conf -state disabled @@ -653,7 +660,7 @@ proc apply_or_revert_hunk {x y revert} { 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 + global ui_diff ui_index file_states last_revert set selected [$ui_diff tag nextrange sel 0.0] @@ -852,5 +859,43 @@ proc apply_or_revert_range_or_line {x y revert} { return } + if {$revert} { + # Save a copy of this patch for undoing reverts. + set last_revert $current_diff_header$wholepatch + set last_revert_enc $enc + } + + unlock_index +} + +# Undo the last line/hunk reverted. When hunks and lines are reverted, a copy +# of the diff applied is saved. Re-apply that diff to undo the revert. +# +# Right now, we only use a single variable to hold the copy, and not a +# stack/deque for simplicity, so multiple undos are not possible. Maybe this +# can be added if the need for something like this is felt in the future. +proc undo_last_revert {} { + global last_revert current_diff_path current_diff_header + global last_revert_enc + + if {$last_revert eq {}} return + if {![lock_index apply_hunk]} return + + set apply_cmd {apply --whitespace=nowarn} + set failed_msg [mc "Failed to undo last revert."] + + if {[catch { + set enc $last_revert_enc + set p [eval git_write $apply_cmd] + fconfigure $p -translation binary -encoding $enc + puts -nonewline $p $last_revert + close $p} err]} { + error_popup "$failed_msg\n\n$err" + unlock_index + return + } + + set last_revert {} + unlock_index }
Accidental clicks on the revert hunk/lines buttons can cause loss of work, and can be frustrating. So, allow undoing the last revert. Right now, a stack or deque are not being used for the sake of simplicity, so only one undo is possible. Any reverts before the previous one are lost. Signed-off-by: Pratyush Yadav <me@yadavpratyush.com> --- git-gui.sh | 18 +++++++++++++++++- lib/diff.tcl | 53 ++++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 66 insertions(+), 5 deletions(-)