patch, 3 partsgit-gui: Add ability to revert selected hunks and lines

> +	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} {
> 

> 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:
>> 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.

> 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
>
> --
> 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.

> 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?

> This series adds the ability to revert selected lines and hunks in
> git-gui. Partially based on the patch by Bert Wesarg [0].
>
> The commits can be found in the topic branch 'py/git-gui-revert-lines'
> at https://github.com/prati0100/git/tree/py/git-gui-revert-lines

> Pratyush Yadav <me@yadavpratyush.com> writes:
> 
> > This series adds the ability to revert selected lines and hunks in
> > git-gui. Partially based on the patch by Bert Wesarg [0].
> >
> > The commits can be found in the topic branch 'py/git-gui-revert-lines'
> > at https://github.com/prati0100/git/tree/py/git-gui-revert-lines
> 
> Please don't do this.  

> I would strongly prefer keeping the contination of history from the
> history in Pat's git-gui repository.  If you clone from
> 
>     git://repo.or.cz/git-gui.git/
> 
> you'll notice everything for git-gui is one level up, and nothing
> for git-core is duplicated in there.  You'll work on top of that, so
> the patches to the git-gui project should not say things like
> 
>     ---
>      git-gui/lib/index.tcl | 27 +++++++++++++++++----------
>      1 file changed, 17 insertions(+), 10 deletions(-)
> 
>     diff --git a/git-gui/lib/index.tcl b/git-gui/lib/index.tcl
>     index b588db11d9..cb7f74af45 100644
>     ...
> 
> The leading "git-gui/" should not appear.
> 
> I have a fork of Pat's history with a single topic on top at
> https://github.com/gitster/git-gui/ so building on top would
> maintain the continuity of the history as well.
> 
> Once you prepared your changes in such a clone of the git-gui
> project, in order to test them with the rest of Git, you'd make a
> trial merge with the -Xsubtree=git-gui option.  Perhaps you have
> git-gui's clone in $HOME/git-gui and git's clone in $HOME/git, like
> so
> 
> 	$ cd $HOME
> 	$ git clone https://github.com/gitster/git-gui git-gui
> 	$ cd git-gui
> 	... from now on, you'd work on git-gui in this directory  ...
> 	... do the work of this topic perhaps on 'revert-hunks' branch ...
> 
> 	$ git clone https://github.com/gitster/git git
> 	$ cd ../git
> 	... trial integration ...
> 	$ git pull -Xsubtree=git-gui ../git-gui/ revert-hunks
> 	... do whatever testing necessary ...
> 
> As an interim (and hopefully evantual) maintainer of the git-gui
> project, you'd publish from your local git-gui directory to a fork
> of git-gui project you host somewhere.  Your patches for review
> would also be taken from your local git-gui directory.
> 
> Thanks.

>
> Hi,
>
> This series adds the ability to revert selected lines and hunks in
> git-gui. Partially based on the patch by Bert Wesarg [0].
>
> The commits can be found in the topic branch 'py/git-gui-revert-lines'
> at https://github.com/prati0100/git/tree/py/git-gui-revert-lines
>
> Once reviewed, pull the commits from
> 832064f93d3ad432616e96ca70f682a7cfdcc3e0 till
> 72eed27a29f1e71cbefefa6b19f96c89793ac74d.
>
> Let me know if there is some other way I'm supposed to ask for a pull.
>
> [0]
> https://public-inbox.org/git/a9ba4550a29d7f3c653561e7029f0920bf8eb008.1326116492.git.bert.wesarg@googlemail.com/
>
> Pratyush Yadav (3):
>   git-gui: Move revert confirmation dialog creation to separate function
>   git-gui: Add the ability to revert selected lines
>   git-gui: Add the ability to revert selected hunk
>
>  git-gui/git-gui.sh    | 39 ++++++++++++++++++++++++--
>  git-gui/lib/diff.tcl  | 65 ++++++++++++++++++++++++++++++++++++-------
>  git-gui/lib/index.tcl | 27 +++++++++++-------
>  3 files changed, 109 insertions(+), 22 deletions(-)
>
> --
> 2.21.0
>

>
> 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.sh   | 25 ++++++++++++++++++++++++-
>  lib/diff.tcl | 31 ++++++++++++++++++++++++++-----
>  2 files changed, 50 insertions(+), 6 deletions(-)
>
> diff --git a/git-gui.sh b/git-gui.sh
> index b7811d8..9d84ba9 100755
> --- a/git-gui.sh
> +++ b/git-gui.sh
> @@ -3588,9 +3588,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"] \
> @@ -3688,15 +3694,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
> @@ -3707,11 +3717,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

> On Fri, Aug 23, 2019 at 12:01 AM Pratyush Yadav <me@yadavpratyush.com> wrote:
> >
> > 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.sh   | 25 ++++++++++++++++++++++++-
> >  lib/diff.tcl | 31 ++++++++++++++++++++++++++-----
> >  2 files changed, 50 insertions(+), 6 deletions(-)
> >
> > diff --git a/git-gui.sh b/git-gui.sh
> > index b7811d8..9d84ba9 100755
> > --- a/git-gui.sh
> > +++ b/git-gui.sh
> > @@ -3588,9 +3588,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"] \
> > @@ -3688,15 +3694,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
> > @@ -3707,11 +3717,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
> 
> so you have the 'Revert Line(s)'  menu entry always in the context
> menu, also when the context menu was opened for an staged file (than
> the menu entry is only disabled)? I think this is a step backwards
> from my original patch (which isn't mentioned/referenced at all in
> this patch anyway).

> My orignal patch also had this hunk in lib/diff.tcl:
> 
> @@ -614,6 +619,8 @@ proc apply_hunk {x y} {
> 
>      if {$current_diff_side eq $ui_index} {
>          set mi ${o}M
> +    } elseif {$revert} {
> +        set mi "[string index $mi 0]$o"
>      } elseif {[string index $mi 0] eq {_}} {
>          set mi M$o
>      } else {
> 
> which is missing here. I cannot remember why I added this here. But
> maybe you can?

> Best,
> Bert
> 
> >                 tk_popup $ctxm $X $Y
> >         }
> >  }
> > diff --git a/lib/diff.tcl b/lib/diff.tcl
> > index 4cae10a..00b15f5 100644
> > --- a/lib/diff.tcl
> > +++ b/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} {
> > --
> > 2.21.0
> >

> This series adds the ability to revert selected lines and hunks in
> git-gui. Partially based on the patch by Bert Wesarg [0].
>
> The commits can be found in the topic branch 'py/revert-hunks-lines'
> at https://github.com/prati0100/git-gui/tree/py/revert-hunks-lines
>
> Once reviewed, pull the commits from
> 415ce3f8582769d1d454b3796dc6c9c847cefa87 till
> 0a1f4ea92b97e673fda40918dae68deead43bb27, or just munge the patches and
> apply them locally, whichever you prefer.

> Pratyush Yadav (4):
>   git-gui: Move revert confirmation dialog creation to separate function
>   git-gui: Add option to disable the revert confirmation prompt
>   git-gui: Add the ability to revert selected lines
>   git-gui: Add the ability to revert selected hunk

>
>  git-gui.sh     | 40 +++++++++++++++++++++++++++++--
>  lib/diff.tcl   | 65 ++++++++++++++++++++++++++++++++++++++++++--------
>  lib/index.tcl  | 31 ++++++++++++++++--------
>  lib/option.tcl |  1 +
>  4 files changed, 115 insertions(+), 22 deletions(-)
>
> --
> 2.21.0

> Pratyush Yadav <me@yadavpratyush.com> writes:
> 
> > This series adds the ability to revert selected lines and hunks in
> > git-gui. Partially based on the patch by Bert Wesarg [0].
> >
> > The commits can be found in the topic branch 'py/revert-hunks-lines'
> > at https://github.com/prati0100/git-gui/tree/py/revert-hunks-lines
> >
> > Once reviewed, pull the commits from
> > 415ce3f8582769d1d454b3796dc6c9c847cefa87 till
> > 0a1f4ea92b97e673fda40918dae68deead43bb27, or just munge the patches and
> > apply them locally, whichever you prefer.
> 
> Let's see how we can work together by you playing the role of
> git-gui maintainer and the others on the list (including me) playing
> the role of reviewer and contributor.  So I may keep an eye on the
> discussion on this thread, I may even comment on them myself, but
> you'll be the one waiting for the discussion to settle, adjusting
> the patches in response to reviews, etc. and making the final
> decision when/if the topic is done, at which time you'd be telling
> me to pull from you.
> 
> > Pratyush Yadav (4):
> >   git-gui: Move revert confirmation dialog creation to separate function
> >   git-gui: Add option to disable the revert confirmation prompt
> >   git-gui: Add the ability to revert selected lines
> >   git-gui: Add the ability to revert selected hunk
> 
> "Move" and "Add" after "git-gui:" would better be downcased to be
> more in line with the others in "git shortlog --no-merges"; I also
> think "allow doing X" is shorter and better way to say "add the
> ability to do X".

> An earlier objection from j6t sounded like we
> require users to respond to an extra dialog after this series, but
> that does not look like the case.  Instead, step 2 adds a new
> feature to allow those to opt-out of the existing dialog (which may
> be reused to squelch the dialog to protect features added in steps 3
> and 4).  Am I reading the series correctly?

> 
> Thanks.
> 
> >
> >  git-gui.sh     | 40 +++++++++++++++++++++++++++++--
> >  lib/diff.tcl   | 65 ++++++++++++++++++++++++++++++++++++++++++--------
> >  lib/index.tcl  | 31 ++++++++++++++++--------
> >  lib/option.tcl |  1 +
> >  4 files changed, 115 insertions(+), 22 deletions(-)
> >
> > --
> > 2.21.0

>
> On 22/08/19 03:34PM, Junio C Hamano wrote:
> > Pratyush Yadav <me@yadavpratyush.com> writes:
> >
> > > This series adds the ability to revert selected lines and hunks in
> > > git-gui. Partially based on the patch by Bert Wesarg [0].
> > >
> > > The commits can be found in the topic branch 'py/revert-hunks-lines'
> > > at https://github.com/prati0100/git-gui/tree/py/revert-hunks-lines
> > >
> > > Once reviewed, pull the commits from
> > > 415ce3f8582769d1d454b3796dc6c9c847cefa87 till
> > > 0a1f4ea92b97e673fda40918dae68deead43bb27, or just munge the patches and
> > > apply them locally, whichever you prefer.
> >
> > Let's see how we can work together by you playing the role of
> > git-gui maintainer and the others on the list (including me) playing
> > the role of reviewer and contributor.  So I may keep an eye on the
> > discussion on this thread, I may even comment on them myself, but
> > you'll be the one waiting for the discussion to settle, adjusting
> > the patches in response to reviews, etc. and making the final
> > decision when/if the topic is done, at which time you'd be telling
> > me to pull from you.
> >
> > > Pratyush Yadav (4):
> > >   git-gui: Move revert confirmation dialog creation to separate function
> > >   git-gui: Add option to disable the revert confirmation prompt
> > >   git-gui: Add the ability to revert selected lines
> > >   git-gui: Add the ability to revert selected hunk
> >
> > "Move" and "Add" after "git-gui:" would better be downcased to be
> > more in line with the others in "git shortlog --no-merges"; I also
> > think "allow doing X" is shorter and better way to say "add the
> > ability to do X".
>
> Will fix. Thanks.
>
> > If I am reading the first patch correctly, we already ask for
> > confirmation before reverting local changes, and the steps 3 and 4
> > are about allowing partial reversion in addition to the wholesale
> > reversion, right?
>
> Yes. The ability to revert whole files already exists. This revert
> always asks for confirmation. Steps 3 and 4 allow for partial reverts.
> Step 2 allows the user to disable the confirmation dialog for both
> whole-file reverts and for partial reverts.
>
> > An earlier objection from j6t sounded like we
> > require users to respond to an extra dialog after this series, but
> > that does not look like the case.  Instead, step 2 adds a new
> > feature to allow those to opt-out of the existing dialog (which may
> > be reused to squelch the dialog to protect features added in steps 3
> > and 4).  Am I reading the series correctly?
>
> Yes. The users always responded to an extra dialog for whole file
> reverts even before these changes. j6t was running a fork of git-gui
> which had the ability for partial reverts, and his fork did not ask for
> confirmation for partial reverts. Always asking for confirmation
> disrupts his workflow, so I added an option to disable it. It disables
> the dialog for partial reverts (which j6t cares about), and also for
> whole file reverts, which I added to maintain consistency.

> The thing is, that the partial revert "just don't happen by accident".
> Here are the minimum user actions needed to get to this dialog:
>
> 1. whole-file revert
>
> - do a Ctrl+J, more or less anywhere in the GUI
>
> 2. hunk revert/revert one unselected line
>
> - right click anywhere in the diff pane (thats around 60% of the window area)
> - move the mouse pointer down 3/4 menu items
> - click this menu item
>
> 3. partially revert selected lines
>
> - select some content in the diff pane by starting by pressing and
> holding a left click
> - end the selection by releasing the left click
> - move the mouse pointer down 3/4 menu items
> - click this menu item
>
> Thats always at least 2 user actions more than the whole-file revert.
> Thus this cannot happen by accident quite easily in comparison to the
> whole-file revert. And thats the reason why this dialog exists, from
> my point of view.
>
> I can see the need to disable the dialog for the whole-file revert,
> and IIRC that was also requested a long time ago on this list. But I
> don't see a reason to have this dialog also for the partial reverts as
> a safety measure.

> as I'm the one who use this feature for more than 7 years, I can only
> object to this. I'm happy to have the confirmation dialog for the
> whole-file revert (the current state) but having the dialog also for
> partial revert would be too disruptive. And disabling both at the same
> time would not be an option for me.
> 
> The thing is, that the partial revert "just don't happen by accident".
> Here are the minimum user actions needed to get to this dialog:
> 
> 1. whole-file revert
> 
> - do a Ctrl+J, more or less anywhere in the GUI

> 2. hunk revert/revert one unselected line
> 
> - right click anywhere in the diff pane (thats around 60% of the window area)
> - move the mouse pointer down 3/4 menu items
> - click this menu item

> 3. partially revert selected lines
> 
> - select some content in the diff pane by starting by pressing and
> holding a left click
> - end the selection by releasing the left click
> - move the mouse pointer down 3/4 menu items
> - click this menu item
> 
> Thats always at least 2 user actions more than the whole-file revert.
> Thus this cannot happen by accident quite easily in comparison to the
> whole-file revert. And thats the reason why this dialog exists, from
> my point of view.
> 
> I can see the need to disable the dialog for the whole-file revert,
> and IIRC that was also requested a long time ago on this list. But I
> don't see a reason to have this dialog also for the partial reverts as
> a safety measure.

> Hi,
> 
> This series adds the ability to revert selected lines and hunks in
> git-gui. Partially based on the patch by Bert Wesarg [0].
> 
> The commits can be found in the topic branch 'py/revert-hunks-lines'
> at https://github.com/prati0100/git-gui/tree/py/revert-hunks-lines
> 
> Once reviewed, pull the commits from
> 415ce3f8582769d1d454b3796dc6c9c847cefa87 till
> 0a1f4ea92b97e673fda40918dae68deead43bb27, or just munge the patches and
> apply them locally, whichever you prefer.
> 
> Changes in v2:
> - Add an option to disable the revert confirmation prompt as suggested
>   by Johannes Sixt.
> - Base the patches on Pat's git-gui tree instead of git.git.

>
> On Fri, Aug 23, 2019 at 03:31:03AM +0530, Pratyush Yadav wrote:
> > Hi,
> >
> > This series adds the ability to revert selected lines and hunks in
> > git-gui. Partially based on the patch by Bert Wesarg [0].
> >
> > The commits can be found in the topic branch 'py/revert-hunks-lines'
> > at https://github.com/prati0100/git-gui/tree/py/revert-hunks-lines
> >
> > Once reviewed, pull the commits from
> > 415ce3f8582769d1d454b3796dc6c9c847cefa87 till
> > 0a1f4ea92b97e673fda40918dae68deead43bb27, or just munge the patches and
> > apply them locally, whichever you prefer.
> >
> > Changes in v2:
> > - Add an option to disable the revert confirmation prompt as suggested
> >   by Johannes Sixt.
> > - Base the patches on Pat's git-gui tree instead of git.git.
>
>
> We've had these features for years in git-cola.

>
> Please copy our keyboard shortcuts.
> IMO we should not re-invent the user interactions.
>
> http://git-cola.github.io/share/doc/git-cola/hotkeys.html

>
> The same hotkey is used for operating at the line level.
> If no lines are selected, the hunk surrounding the current cursor
> position is used.
>
> Please make keyboard interaction a first-class design consideration.
>

> I have a very strong opinion about the confirmation dialog, so I'll just
> mention that here since Hannes is on this thread.
>
> In cola we do have a confirmation dialog, and I strongly believe this is
> the correct behavior because it's an operation that drops data that
> cannot be recovered.
>
> In the other thread, it was mentioned that this dialog would be a
> nuisance.  Perhaps that is true -- for the dialog that may have been
> implemented in this series (I haven't run it to verify).
>
> Let's dive into that concern.
>
> In git-cola we have a confirmation dialog and it is by no way a
> detriment to the workflow, and I use that feature all the time.
> Why?  The reason is that we focused on the keyboard interaction.
>
> The workflow is as follows:
>
>         Ctrl-u to initiate the revert action
>         The prompt appears immediately.
>                 - Hitting any of "enter", "y", or "spacebar" will
>                   confirm the confirmation, and proceed.
>                 - Hitting any of "escape" or "n" will cancel the action.
>
> So essentially the workflow for the power user becomes "ctrl-u, enter"
> and that is such a tiny overhead that it really is not a bother at all.
>
> On the other hand, if I had to actually move my hand over to a mouse or
> trackpad and actually "click" on something then I would be super
> annoyed.  That would be simply horrible with RSI in mind.
>

> OTOH having to hit "enter" or "spacebar" (which is the largest key on
> your keyboard, and your thumbs have good hefty muscles) is totally
> acceptable in my book because it strikes the right balance between
> safety for a destructive operation and convenience.
>
> Now, let's consider the alternative -- adding an option to disable the
> prompt.  I don't like that.
>
> Why?  It's yet another option.  It's yet another thing to document, yet
> another code path, and yet another pitfall for a user who might run
> git-gui in a different configuration (and becomes surprised when revert
> doesn't prompt and suddenly loses their work).
>
> Do we really need an option, or do we need better usability instead?
> My opinion is that the latter is the real need.
>
>
> That's my $.02 from having used this feature in practice since 2013.

> On Sat, Aug 24, 2019 at 1:43 AM David Aguilar <davvid@gmail.com> wrote:
> > On the other hand, if I had to actually move my hand over to a mouse or
> > trackpad and actually "click" on something then I would be super
> > annoyed.  That would be simply horrible with RSI in mind.
> >
> 
> I take this as a point for *not* having a confirmation dialog when
> doing the action per mouse. Which matches exactly my original
> implementation.

> On Sat, Aug 24, 2019 at 1:43 AM David Aguilar <davvid@gmail.com> wrote:
>> On the other hand, if I had to actually move my hand over to a mouse or
>> trackpad and actually "click" on something then I would be super
>> annoyed.  That would be simply horrible with RSI in mind.
>>
> 
> I take this as a point for*not* having a confirmation dialog when
> doing the action per mouse. Which matches exactly my original
> implementation.

>
> 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
>

> 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?
> 
> 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>

>
> 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".
>

>
> > 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 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.

> 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?

>
> 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.

> 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.

> Hi,
> 
> This series adds the ability to revert selected lines and hunks in
> git-gui. Partially based on the patch by Bert Wesarg [0].
> 
> The commits can be found in the topic branch 'py/revert-hunks-lines'
> at https://github.com/prati0100/git-gui/tree/py/revert-hunks-lines
> 
> Changes in v3:
> - Drop the confirmation dialog on partial reverts. It is still there for
>   full file reverts (which was the original behaviour).
> - Allow undoing the last revert.
> - Update the context menu button layout. In v2, the layout was:
>    Stage Hunk
>    Revert Hunk
>    Stage Lines
>    Revert Lines
> 
>   Now it is:
>    Stage Hunk
>    Stage Lines
>    -----------
>    Revert Hunk
>    Revert Lines
>    Undo Last Revert
> - Return early when applying a patch fails. This is useful for this
>   series because in that case we don't save a faulty patch in
>   last_revert, causing the same error to pop up when reverting the patch
>   that failed to apply in the first place.
> 
> Changes in v2:
> - Add an option to disable the revert confirmation prompt as suggested
>   by Johannes Sixt.
> - Base the patches on Pat's git-gui tree instead of git.git.
> 
> [0]
> https://public-inbox.org/git/a9ba4550a29d7f3c653561e7029f0920bf8eb008.1326116492.git.bert.wesarg@googlemail.com/
> 
> Pratyush Yadav (4):
>   git-gui: allow reverting selected lines
>   git-gui: allow reverting selected hunk
>   git-gui: return early when patch fails to apply
>   git-gui: allow undoing last revert
> 
>  git-gui.sh   | 57 +++++++++++++++++++++++++++++--
>  lib/diff.tcl | 96 ++++++++++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 135 insertions(+), 18 deletions(-)
> 
> --
> 2.21.0
> 

>
> Am 10.09.19 um 21:21 schrieb Pratyush Yadav:
> > If there are no further objections with the series, I will merge it in.
>
> No objections. I use it in production.

> On Fri, Aug 23, 2019, 18:44 Pratyush Yadav <me@yadavpratyush.com> wrote:
> 
> > On 23/08/19 08:04AM, Bert Wesarg wrote:
> > > On Fri, Aug 23, 2019 at 12:51 AM Pratyush Yadav <me@yadavpratyush.com>
> > wrote:
> > > >
> > > > On 22/08/19 03:34PM, Junio C Hamano wrote:
> > [...]
> > > as I'm the one who use this feature for more than 7 years, I can only
> > > object to this. I'm happy to have the confirmation dialog for the
> > > whole-file revert (the current state) but having the dialog also for
> > > partial revert would be too disruptive. And disabling both at the same
> > > time would not be an option for me.
> > >
> > > The thing is, that the partial revert "just don't happen by accident".
> > > Here are the minimum user actions needed to get to this dialog:
> > >
> > > 1. whole-file revert
> > >
> > > - do a Ctrl+J, more or less anywhere in the GUI
> >
> > Hmm, have to agree with you on this. It is kind of easy to trigger. But
> > read below why I think partial reverts are just as easy to trigger.
> >
> > > 2. hunk revert/revert one unselected line
> > >
> > > - right click anywhere in the diff pane (thats around 60% of the window
> > area)
> > > - move the mouse pointer down 3/4 menu items
> > > - click this menu item
> >
> > But what if you wanted to click "Stage hunk", and instead click "Revert
> > hunk" instead. This is what I mean by "accidentally".
> >
> > This is even more a risk in the current layout of the buttons, which are
> > in the order:
> >
> > Stage Hunk
> > Revert Hunk
> > Stage Lines
> > Revert Lines
> >
> 
> but this is your current layout! my layout in the patch from 2012 was (and
> still is in my git-gui):
> 
> Stage Hunk
> Stage Line
> ----------
> Revert Hunk
> Revert Line
> 
> 
> thats a separator inbetween!

> bert
> 
> 
> 
> > In this layout, if you wanted to click Stage, you might end up clicking
> > Revert instead, losing part of your work. Even if we switch up the
> > layout a bit, I feel like the potential of "mis-aiming" your mouse is
> > there, but it can certainly be improved.
> >
> > > 3. partially revert selected lines
> > >
> > > - select some content in the diff pane by starting by pressing and
> > > holding a left click
> > > - end the selection by releasing the left click
> > > - move the mouse pointer down 3/4 menu items
> > > - click this menu item
> > >
> > > Thats always at least 2 user actions more than the whole-file revert.
> > > Thus this cannot happen by accident quite easily in comparison to the
> > > whole-file revert. And thats the reason why this dialog exists, from
> > > my point of view.
> > >
> > > I can see the need to disable the dialog for the whole-file revert,
> > > and IIRC that was also requested a long time ago on this list. But I
> > > don't see a reason to have this dialog also for the partial reverts as
> > > a safety measure.
> >
> > I do (not too strongly, but I do), as I explained why above.
> >
> > It shouldn't be too difficult to have separate knobs for whole-file and
> > partial reverts, but they will add two config options. Not necessarily a
> > bad thing, I just thought the people who wanted to disable partial
> > reverts would also want to disable whole-file reverts.
> >
> > But I have another suggestion in mind. I'll reply to one of the other
> > emails in this thread about it. Please read it there, I'd rather not
> > type it twice.
> >
> > --
> > Regards,
> > Pratyush Yadav
> >