[PATCH 0/3] git-gui: Add ability to revert selected hunks and lines

[PATCH 0/3] git-gui: Add ability to revert selected hunks and lines

[Pratyush Yadav]

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

[PATCH 1/3] git-gui: Move revert confirmation dialog creation to separate function

[Pratyush Yadav]

Upcoming commits will introduce reverting lines and hunks. They also
need to prompt the user for confirmation. Put the dialog creation in its
separate function so the same code won't be repeated again and again.

Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
---
 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
--- a/git-gui/lib/index.tcl
+++ b/git-gui/lib/index.tcl
@@ -388,6 +388,18 @@ proc do_add_all {} {
 	add_helper [mc "Adding all changed files"] $paths
 }
 
+proc revert_dialog {query} {
+	return [tk_dialog \
+		.confirm_revert \
+		"[appname] ([reponame])" \
+		"$query" \
+		question \
+		1 \
+		[mc "Do Nothing"] \
+		[mc "Revert Changes"] \
+		]
+}
+
 proc revert_helper {txt paths} {
 	global file_states current_diff_path
 
@@ -430,17 +442,12 @@ proc revert_helper {txt paths} {
 		set query [mc "Revert changes in these %i files?" $n]
 	}
 
-	set reply [tk_dialog \
-		.confirm_revert \
-		"[appname] ([reponame])" \
-		"$query
+	set query "$query
+
+[mc "Any unstaged changes will be permanently lost by the revert."]"
+
+	set reply [revert_dialog $query]
 
-[mc "Any unstaged changes will be permanently lost by the revert."]" \
-		question \
-		1 \
-		[mc "Do Nothing"] \
-		[mc "Revert Changes"] \
-		]
 	if {$reply == 1} {
 		checkout_index \
 			$txt \
-- 
2.21.0

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

[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>
---
 git-gui/git-gui.sh   | 25 ++++++++++++++++++++++++-
 git-gui/lib/diff.tcl | 31 ++++++++++++++++++++++++++-----
 2 files changed, 50 insertions(+), 6 deletions(-)

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

[PATCH 3/3] git-gui: Add the ability to revert selected hunk

[Pratyush Yadav]

Just like the user can select a hunk to stage or unstage, add the
ability to revert hunks.

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

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 2011894bef..cfa682ff59 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -3606,9 +3606,14 @@ set ctxm .vpane.lower.diff.body.ctxm
 menu $ctxm -tearoff 0
 $ctxm add command \
 	-label [mc "Apply/Reverse Hunk"] \
-	-command {apply_hunk $cursorX $cursorY}
+	-command {apply_or_revert_hunk $cursorX $cursorY 0}
 set ui_diff_applyhunk [$ctxm index last]
 lappend diff_actions [list $ctxm entryconf $ui_diff_applyhunk -state]
+$ctxm add command \
+	-label [mc "Revert Hunk"] \
+	-command {apply_or_revert_hunk $cursorX $cursorY 1}
+set ui_diff_reverthunk [$ctxm index last]
+lappend diff_actions [list $ctxm entryconf $ui_diff_reverthunk -state]
 $ctxm add command \
 	-label [mc "Apply/Reverse Line"] \
 	-command {apply_or_revert_range_or_line $cursorX $cursorY 0; do_rescan}
@@ -3715,6 +3720,8 @@ proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} {
 		set has_range [expr {[$::ui_diff tag nextrange sel 0.0] != {}}]
 		if {$::ui_index eq $::current_diff_side} {
 			set l [mc "Unstage Hunk From Commit"]
+			set h [mc "Revert Hunk"]
+
 			if {$has_range} {
 				set t [mc "Unstage Lines From Commit"]
 				set r [mc "Revert Lines"]
@@ -3724,6 +3731,8 @@ proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} {
 			}
 		} else {
 			set l [mc "Stage Hunk For Commit"]
+			set h [mc "Revert Hunk"]
+
 			if {$has_range} {
 				set t [mc "Stage Lines For Commit"]
 				set r [mc "Revert Lines"]
@@ -3758,6 +3767,9 @@ proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} {
 		$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
+
 		tk_popup $ctxm $X $Y
 	}
 }
diff --git a/git-gui/lib/diff.tcl b/git-gui/lib/diff.tcl
index 4b2b00df4b..a818e68dad 100644
--- a/git-gui/lib/diff.tcl
+++ b/git-gui/lib/diff.tcl
@@ -567,30 +567,50 @@ proc read_diff {fd conflict_size cont_info} {
 	}
 }
 
-proc apply_hunk {x y} {
+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
 
 	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 hunk."]
-		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 hunk."]
+		if {$revert} {
+			set failed_msg [mc "Failed to revert selected hunk."]
+			lappend apply_cmd --reverse
+		} else {
+			set failed_msg [mc "Failed to stage selected hunk."]
+			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 hunk will be permanently lost by the revert."]"
+
+		set reply [revert_dialog $query]
+		if {$reply ne 1} {
+			unlock_index
+			return
+		}
+	}
+
 	set s_lno [lindex [split [$ui_diff index @$x,$y] .] 0]
 	set s_lno [$ui_diff search -backwards -regexp ^@@ $s_lno.0 0.0]
 	if {$s_lno eq {}} {
@@ -619,13 +639,17 @@ proc apply_hunk {x y} {
 	$ui_diff delete $s_lno $e_lno
 	$ui_diff conf -state disabled
 
+	# Check if the hunk was the last one in the file.
 	if {[$ui_diff get 1.0 end] eq "\n"} {
 		set o _
 	} else {
 		set o ?
 	}
 
-	if {$current_diff_side eq $ui_index} {
+	# Update the status flags.
+	if {$revert} {
+		set mi [string index $mi 0]$o
+	} elseif {$current_diff_side eq $ui_index} {
 		set mi ${o}M
 	} elseif {[string index $mi 0] eq {_}} {
 		set mi M$o
-- 
2.21.0

Re: [PATCH 0/3] git-gui: Add ability to revert selected hunks and lines

[Junio C Hamano]

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.

Re: [PATCH 2/3] git-gui: Add the ability to revert selected lines

[Johannes Sixt]

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

Re: [PATCH 2/3] git-gui: Add the ability to revert selected lines

[Pratyush Yadav]

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

-- 
Regards,
Pratyush Yadav

Re: [PATCH 0/3] git-gui: Add ability to revert selected hunks and lines

[Pratyush Yadav]

On 20/08/19 11:47AM, 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/git-gui-revert-lines'
> > at https://github.com/prati0100/git/tree/py/git-gui-revert-lines
> 
> Please don't do this.  
 
All right, I'll send a re-roll of the patches based on your fork of 
Pat's tree as soon as I get some time.

PS: Thanks for the detailed explanation of how I should structure my 
workflow :)

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

-- 
Regards,
Pratyush Yadav

Re: [PATCH 2/3] git-gui: Add the ability to revert selected lines

[Johannes Sixt]

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

Re: [PATCH 0/3] git-gui: Add ability to revert selected hunks and lines

[Bert Wesarg]

Dear Pratyush,

putting me in Cc would have been nice :(

I will look into your patches in the comming hours.

Bert

On Mon, Aug 19, 2019 at 11:41 PM Pratyush Yadav <me@yadavpratyush.com> 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/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
>

Re: [PATCH 0/3] git-gui: Add ability to revert selected hunks and lines

[Pratyush Yadav]

On 21/08/19 09:06AM, Bert Wesarg wrote:
> Dear Pratyush,
> 
> putting me in Cc would have been nice :(
 
I wasn't sure if you were interested in git-gui any more, so I decided 
to not bother you. I will Cc you when I send a re-roll.

> I will look into your patches in the comming hours.
 
Thanks.

-- 
Regards,
Pratyush Yadav

> Bert
> 
> On Mon, Aug 19, 2019 at 11:41 PM Pratyush Yadav <me@yadavpratyush.com> 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/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
> >

Re: [PATCH 2/3] git-gui: Add the ability to revert selected lines

[Pratyush Yadav]

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

[PATCH v2 0/4] git-gui: Add ability to revert selected hunks and lines

[Pratyush Yadav]

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.

[0]
https://public-inbox.org/git/a9ba4550a29d7f3c653561e7029f0920bf8eb008.1326116492.git.bert.wesarg@googlemail.com/

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

[PATCH v2 1/4] git-gui: Move revert confirmation dialog creation to separate function

[Pratyush Yadav]

Upcoming commits will introduce reverting lines and hunks. They also
need to prompt the user for confirmation. Put the dialog creation in its
separate function so the same code won't be repeated again and again.

Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
---
 lib/index.tcl | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/lib/index.tcl b/lib/index.tcl
index b588db1..cb7f74a 100644
--- a/lib/index.tcl
+++ b/lib/index.tcl
@@ -388,6 +388,18 @@ proc do_add_all {} {
 	add_helper [mc "Adding all changed files"] $paths
 }
 
+proc revert_dialog {query} {
+	return [tk_dialog \
+		.confirm_revert \
+		"[appname] ([reponame])" \
+		"$query" \
+		question \
+		1 \
+		[mc "Do Nothing"] \
+		[mc "Revert Changes"] \
+		]
+}
+
 proc revert_helper {txt paths} {
 	global file_states current_diff_path
 
@@ -430,17 +442,12 @@ proc revert_helper {txt paths} {
 		set query [mc "Revert changes in these %i files?" $n]
 	}
 
-	set reply [tk_dialog \
-		.confirm_revert \
-		"[appname] ([reponame])" \
-		"$query
+	set query "$query
+
+[mc "Any unstaged changes will be permanently lost by the revert."]"
+
+	set reply [revert_dialog $query]
 
-[mc "Any unstaged changes will be permanently lost by the revert."]" \
-		question \
-		1 \
-		[mc "Do Nothing"] \
-		[mc "Revert Changes"] \
-		]
 	if {$reply == 1} {
 		checkout_index \
 			$txt \
-- 
2.21.0

[PATCH v2 2/4] git-gui: Add option to disable the revert confirmation prompt

[Pratyush Yadav]

When reverting files (or hunks or lines that future commits will add), a
confirmation dialog is shown to make sure the user actually wanted to
revert, and did not accidentally click revert.

But for some people's workflow this is an hindrance. So add an option to
disable this behaviour for people who are comfortable risking accidental
reverts.

The default behaviour is to ask for confirmation.

Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
---
 git-gui.sh     |  1 +
 lib/index.tcl  | 22 +++++++++++++---------
 lib/option.tcl |  1 +
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index 5bc21b8..b7811d8 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -903,6 +903,7 @@ set font_descs {
 }
 set default_config(gui.stageuntracked) ask
 set default_config(gui.displayuntracked) true
+set default_config(gui.revertaskconfirmation) true
 
 ######################################################################
 ##
diff --git a/lib/index.tcl b/lib/index.tcl
index cb7f74a..dd694d1 100644
--- a/lib/index.tcl
+++ b/lib/index.tcl
@@ -389,15 +389,19 @@ proc do_add_all {} {
 }
 
 proc revert_dialog {query} {
-	return [tk_dialog \
-		.confirm_revert \
-		"[appname] ([reponame])" \
-		"$query" \
-		question \
-		1 \
-		[mc "Do Nothing"] \
-		[mc "Revert Changes"] \
-		]
+	if {[is_config_true gui.revertaskconfirmation]} {
+		return [tk_dialog \
+			.confirm_revert \
+			"[appname] ([reponame])" \
+			"$query" \
+			question \
+			1 \
+			[mc "Do Nothing"] \
+			[mc "Revert Changes"] \
+			]
+	} else {
+		return 1
+	}
 }
 
 proc revert_helper {txt paths} {
diff --git a/lib/option.tcl b/lib/option.tcl
index e43971b..cb62d5d 100644
--- a/lib/option.tcl
+++ b/lib/option.tcl
@@ -162,6 +162,7 @@ proc do_options {} {
 		{s gui.stageuntracked {mc "Staging of untracked files"} {list "yes" "no" "ask"}}
 		{b gui.displayuntracked {mc "Show untracked files"}}
 		{i-1..99 gui.tabsize {mc "Tab spacing"}}
+		{b gui.revertaskconfirmation {mc "Ask for confirmation when reverting changes"}}
 		} {
 		set type [lindex $option 0]
 		set name [lindex $option 1]
-- 
2.21.0

[PATCH v2 3/4] git-gui: Add the ability to revert selected lines

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

[PATCH v2 4/4] git-gui: Add the ability to revert selected hunk

[Pratyush Yadav]

Just like the user can select a hunk to stage or unstage, add the
ability to revert hunks.

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

diff --git a/git-gui.sh b/git-gui.sh
index 9d84ba9..13fef74 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -3583,9 +3583,14 @@ set ctxm .vpane.lower.diff.body.ctxm
 menu $ctxm -tearoff 0
 $ctxm add command \
 	-label [mc "Apply/Reverse Hunk"] \
-	-command {apply_hunk $cursorX $cursorY}
+	-command {apply_or_revert_hunk $cursorX $cursorY 0}
 set ui_diff_applyhunk [$ctxm index last]
 lappend diff_actions [list $ctxm entryconf $ui_diff_applyhunk -state]
+$ctxm add command \
+	-label [mc "Revert Hunk"] \
+	-command {apply_or_revert_hunk $cursorX $cursorY 1}
+set ui_diff_reverthunk [$ctxm index last]
+lappend diff_actions [list $ctxm entryconf $ui_diff_reverthunk -state]
 $ctxm add command \
 	-label [mc "Apply/Reverse Line"] \
 	-command {apply_or_revert_range_or_line $cursorX $cursorY 0; do_rescan}
@@ -3692,6 +3697,8 @@ proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} {
 		set has_range [expr {[$::ui_diff tag nextrange sel 0.0] != {}}]
 		if {$::ui_index eq $::current_diff_side} {
 			set l [mc "Unstage Hunk From Commit"]
+			set h [mc "Revert Hunk"]
+
 			if {$has_range} {
 				set t [mc "Unstage Lines From Commit"]
 				set r [mc "Revert Lines"]
@@ -3701,6 +3708,8 @@ proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} {
 			}
 		} else {
 			set l [mc "Stage Hunk For Commit"]
+			set h [mc "Revert Hunk"]
+
 			if {$has_range} {
 				set t [mc "Stage Lines For Commit"]
 				set r [mc "Revert Lines"]
@@ -3735,6 +3744,9 @@ proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} {
 		$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
+
 		tk_popup $ctxm $X $Y
 	}
 }
diff --git a/lib/diff.tcl b/lib/diff.tcl
index 00b15f5..6a9c760 100644
--- a/lib/diff.tcl
+++ b/lib/diff.tcl
@@ -567,30 +567,50 @@ proc read_diff {fd conflict_size cont_info} {
 	}
 }
 
-proc apply_hunk {x y} {
+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
 
 	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 hunk."]
-		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 hunk."]
+		if {$revert} {
+			set failed_msg [mc "Failed to revert selected hunk."]
+			lappend apply_cmd --reverse
+		} else {
+			set failed_msg [mc "Failed to stage selected hunk."]
+			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 hunk will be permanently lost by the revert."]"
+
+		set reply [revert_dialog $query]
+		if {$reply ne 1} {
+			unlock_index
+			return
+		}
+	}
+
 	set s_lno [lindex [split [$ui_diff index @$x,$y] .] 0]
 	set s_lno [$ui_diff search -backwards -regexp ^@@ $s_lno.0 0.0]
 	if {$s_lno eq {}} {
@@ -619,13 +639,17 @@ proc apply_hunk {x y} {
 	$ui_diff delete $s_lno $e_lno
 	$ui_diff conf -state disabled
 
+	# Check if the hunk was the last one in the file.
 	if {[$ui_diff get 1.0 end] eq "\n"} {
 		set o _
 	} else {
 		set o ?
 	}
 
-	if {$current_diff_side eq $ui_index} {
+	# Update the status flags.
+	if {$revert} {
+		set mi [string index $mi 0]$o
+	} elseif {$current_diff_side eq $ui_index} {
 		set mi ${o}M
 	} elseif {[string index $mi 0] eq {_}} {
 		set mi M$o
-- 
2.21.0

Re: [PATCH v2 0/4] git-gui: Add ability to revert selected hunks and lines

[Junio C Hamano]

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

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

Re: [PATCH v2 0/4] git-gui: Add ability to revert selected hunks and lines

[Pratyush Yadav]

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.

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

-- 
Regards,
Pratyush Yadav

Re: [PATCH v2 0/4] git-gui: Add ability to revert selected hunks and lines

[Bert Wesarg]

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

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.

Best,
Bert

>
> --
> Regards,
> Pratyush Yadav

Re: [PATCH v2 3/4] git-gui: Add the ability to revert selected lines

[Bert Wesarg]

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
>

Re: [PATCH 2/3] git-gui: Add the ability to revert selected lines

[Johannes Schindelin]

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
>

Re: [PATCH v2 0/4] git-gui: Add ability to revert selected hunks and lines

[Junio C Hamano]

Bert Wesarg <bert.wesarg@googlemail.com> writes:

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

Thanks for walking us readers through your thought process.

Re: [PATCH 2/3] git-gui: Add the ability to revert selected lines

[Junio C Hamano]

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.

Re: [PATCH v2 0/4] git-gui: Add ability to revert selected hunks and lines

[Pratyush Yadav]

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

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

Re: [PATCH v2 3/4] git-gui: Add the ability to revert selected lines

[Pratyush Yadav]

On 23/08/19 08:29AM, Bert Wesarg wrote:
> 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).
 
I intentionally did this because I thought keeping the menu layout the 
same at all times will help a bit with muscle memory. I don't mind your 
way either though. If you feel very strongly about this, I'll do it your 
way.

> 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?
 
You are looking at the wrong patch. This patch is for reverting lines, 
not hunks. Since the current "Stage selected line(s)" function relies on 
a rescan to update file states, I went with that convention. The 
-command parameter for the menu entry triggers a rescan after the revert 
is done.

The next patch deals with hunks, and it contains this if statement.

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

-- 
Regards,
Pratyush Yadav

Re: [PATCH 2/3] git-gui: Add the ability to revert selected lines

[Pratyush Yadav]

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

-- 
Regards,
Pratyush Yadav

Re: [PATCH 2/3] git-gui: Add the ability to revert selected lines

[Johannes Sixt]

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

Re: [PATCH v2 0/4] git-gui: Add ability to revert selected hunks and lines

[David Aguilar]

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

Ctrl-u is our revert-unstaged-edits hotkeys.  "s" is for
staging/unstaging (or Ctrl-s if the file list is focused).

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

Re: [PATCH v2 0/4] git-gui: Add ability to revert selected hunks and lines

[Bert Wesarg]

On Sat, Aug 24, 2019 at 1:43 AM David Aguilar <davvid@gmail.com> wrote:
>
> 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.

this series does not introduce new hotkeys.

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

my part for the homework:

>
> Ctrl-u is our revert-unstaged-edits hotkeys.

https://github.com/patthoyts/git-gui/commit/b677c66e299c8754a6093cbd19ce71b0ad2a8a17

> "s" is for
> staging/unstaging (or Ctrl-s if the file list is focused).

https://github.com/patthoyts/git-gui/commit/cd16a6c9298778265a044e5f9a39b006277b32f2

https://github.com/patthoyts/git-gui/commit/e210e67451f22f97c1476d6b78b6fa7fdd5817f9#diff-ceba4b88c7e634c5401a4487d45d3ab4R774

Bert

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

Re: [PATCH v2 0/4] git-gui: Add ability to revert selected hunks and lines

[Bert Wesarg]

On Sat, Aug 24, 2019 at 1:43 AM David Aguilar <davvid@gmail.com> wrote:
>

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

I take this as a point for*not* having a confirmation dialog when
doing the action per mouse. Which matches exactly my original
implementation.

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

2012

Best,
Bert


> --
> David

Re: [PATCH v2 0/4] git-gui: Add ability to revert selected hunks and lines

[David Aguilar]

On Sat, Aug 24, 2019 at 08:57:22AM +0200, Bert Wesarg wrote:
> 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.

+1

> > That's my $.02 from having used this feature in practice since 2013.
> 
> 2012

Nice! ;-)  I agree with everything you've written here.

cheers,
-- 
David

Re: [PATCH v2 0/4] git-gui: Add ability to revert selected hunks and lines

[Johannes Sixt]

Am 24.08.19 um 08:57 schrieb Bert Wesarg:
> 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.

I totally agree.

-- Hannes

Re: [PATCH v2 0/4] git-gui: Add ability to revert selected hunks and lines

[Pratyush Yadav]

On 23/08/19 11:12PM, Bert Wesarg wrote:
> 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!
 
Looking at how this discussion has been going, I see that people really 
don't like the dialog, even with a config option. So I will send a 
re-roll with this layout of buttons and ability to undo last revert. I 
hope that will satisfy most people.

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

-- 
Regards,
Pratyush Yadav

[PATCH v3 0/4] git-gui: Add ability to revert selected hunks and lines

[Pratyush Yadav]

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

[PATCH v3 1/4] git-gui: allow reverting selected lines

[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>
---
 git-gui.sh   | 25 ++++++++++++++++++++++++-
 lib/diff.tcl | 20 ++++++++++++++------
 2 files changed, 38 insertions(+), 7 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index 5bc21b8..6d4d002 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -3587,10 +3587,16 @@ 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 separator
+$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]
+$ctxm add separator
 $ctxm add command \
 	-label [mc "Show Less Context"] \
 	-command show_less_context
@@ -3687,15 +3693,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
@@ -3706,11 +3716,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/lib/diff.tcl b/lib/diff.tcl
index 4cae10a..d6bee29 100644
--- a/lib/diff.tcl
+++ b/lib/diff.tcl
@@ -55,7 +55,7 @@ proc reshow_diff {{after {}}} {
 
 proc force_diff_encoding {enc} {
 	global current_diff_path
-	
+
 	if {$current_diff_path ne {}} {
 		force_path_encoding $current_diff_path $enc
 		reshow_diff
@@ -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,19 +660,27 @@ 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
-- 
2.21.0

[PATCH v3 2/4] git-gui: allow reverting selected hunk

[Pratyush Yadav]

Just like the user can select a hunk to stage or unstage, add the
ability to revert hunks.

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

diff --git a/git-gui.sh b/git-gui.sh
index 6d4d002..1592544 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -3582,7 +3582,7 @@ set ctxm .vpane.lower.diff.body.ctxm
 menu $ctxm -tearoff 0
 $ctxm add command \
 	-label [mc "Apply/Reverse Hunk"] \
-	-command {apply_hunk $cursorX $cursorY}
+	-command {apply_or_revert_hunk $cursorX $cursorY 0}
 set ui_diff_applyhunk [$ctxm index last]
 lappend diff_actions [list $ctxm entryconf $ui_diff_applyhunk -state]
 $ctxm add command \
@@ -3591,6 +3591,11 @@ $ctxm add command \
 set ui_diff_applyline [$ctxm index last]
 lappend diff_actions [list $ctxm entryconf $ui_diff_applyline -state]
 $ctxm add separator
+$ctxm add command \
+	-label [mc "Revert Hunk"] \
+	-command {apply_or_revert_hunk $cursorX $cursorY 1}
+set ui_diff_reverthunk [$ctxm index last]
+lappend diff_actions [list $ctxm entryconf $ui_diff_reverthunk -state]
 $ctxm add command \
 	-label [mc "Revert Line"] \
 	-command {apply_or_revert_range_or_line $cursorX $cursorY 1; do_rescan}
@@ -3691,6 +3696,8 @@ proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} {
 		set has_range [expr {[$::ui_diff tag nextrange sel 0.0] != {}}]
 		if {$::ui_index eq $::current_diff_side} {
 			set l [mc "Unstage Hunk From Commit"]
+			set h [mc "Revert Hunk"]
+
 			if {$has_range} {
 				set t [mc "Unstage Lines From Commit"]
 				set r [mc "Revert Lines"]
@@ -3700,6 +3707,8 @@ proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} {
 			}
 		} else {
 			set l [mc "Stage Hunk For Commit"]
+			set h [mc "Revert Hunk"]
+
 			if {$has_range} {
 				set t [mc "Stage Lines For Commit"]
 				set r [mc "Revert Lines"]
@@ -3734,6 +3743,9 @@ proc popup_diff_menu {ctxm ctxmmg ctxmsm x y X Y} {
 		$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
+
 		tk_popup $ctxm $X $Y
 	}
 }
diff --git a/lib/diff.tcl b/lib/diff.tcl
index d6bee29..ffca788 100644
--- a/lib/diff.tcl
+++ b/lib/diff.tcl
@@ -567,24 +567,31 @@ proc read_diff {fd conflict_size cont_info} {
 	}
 }
 
-proc apply_hunk {x y} {
+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
 
 	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 hunk."]
-		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 hunk."]
+		if {$revert} {
+			set failed_msg [mc "Failed to revert selected hunk."]
+			lappend apply_cmd --reverse
+		} else {
+			set failed_msg [mc "Failed to stage selected hunk."]
+			lappend apply_cmd --cached
+		}
+
 		if {[string index $mi 1] ne {M}} {
 			unlock_index
 			return
@@ -619,13 +626,17 @@ proc apply_hunk {x y} {
 	$ui_diff delete $s_lno $e_lno
 	$ui_diff conf -state disabled
 
+	# Check if the hunk was the last one in the file.
 	if {[$ui_diff get 1.0 end] eq "\n"} {
 		set o _
 	} else {
 		set o ?
 	}
 
-	if {$current_diff_side eq $ui_index} {
+	# Update the status flags.
+	if {$revert} {
+		set mi [string index $mi 0]$o
+	} elseif {$current_diff_side eq $ui_index} {
 		set mi ${o}M
 	} elseif {[string index $mi 0] eq {_}} {
 		set mi M$o
-- 
2.21.0

[PATCH v3 3/4] git-gui: return early when patch fails to apply

[Pratyush Yadav]

In the procedure apply_or_revert_range_or_line, if the patch does not
apply successfully, a dialog is shown, but execution proceeds after
that. Instead, return early on error so the parts that come after this
don't work on top of an error state.

Signed-off-by: Pratyush Yadav <me@yadavpratyush.com>
---
 lib/diff.tcl | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/diff.tcl b/lib/diff.tcl
index ffca788..0659029 100644
--- a/lib/diff.tcl
+++ b/lib/diff.tcl
@@ -848,6 +848,8 @@ proc apply_or_revert_range_or_line {x y revert} {
 		puts -nonewline $p $wholepatch
 		close $p} err]} {
 		error_popup "$failed_msg\n\n$err"
+		unlock_index
+		return
 	}
 
 	unlock_index
-- 
2.21.0

[PATCH v3 4/4] git-gui: allow undoing last revert

[Pratyush Yadav]

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

Re: [PATCH v3 0/4] git-gui: Add ability to revert selected hunks and lines

[Pratyush Yadav]

Johannes, Bert, All,

If there are no further objections with the series, I will merge it in.

On 29/08/19 03:27AM, 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
> 
> 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
> 

-- 
Regards,
Pratyush Yadav

Re: [PATCH v3 0/4] git-gui: Add ability to revert selected hunks and lines

[Johannes Sixt]

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.

-- Hannes

Re: [PATCH v3 0/4] git-gui: Add ability to revert selected hunks and lines

[Bert Wesarg]

On Tue, Sep 10, 2019 at 10:26 PM Johannes Sixt <j6t@kdbg.org> wrote:
>
> 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.

yep, Since 2012 ;-)

>
> -- Hannes

Re: [PATCH v3 4/4] git-gui: allow undoing last revert

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

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
>

Re: [PATCH v3 4/4] git-gui: allow undoing last revert

[Pratyush Yadav]

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>

-- 
Regards,
Pratyush Yadav

Re: [PATCH v3 4/4] git-gui: allow undoing last revert

[Johannes Sixt]

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

Re: [PATCH v3 4/4] git-gui: allow undoing last revert

[Bert Wesarg]

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

Re: [PATCH v3 4/4] git-gui: allow undoing last revert

[Bert Wesarg]

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

Re: [PATCH v3 4/4] git-gui: allow undoing last revert

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

-- 
Regards,
Pratyush Yadav

Re: [PATCH v3 4/4] git-gui: allow undoing last revert

[Pratyush Yadav]

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.

-- 
Regards,
Pratyush Yadav