[PATCH] Teach git-gui to split hunks

[PATCH] Teach git-gui to split hunks

[Johannes Schindelin]

When you select the context menu item "Split Hunk" in the diff area,
git-gui will now split the current hunk so that a new hunk starts at
the current position.

For this to work, apply has to be called with --unidiff-zero, since
the new hunks can start or stop with a "-" or "+" line.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	No more complaining from me about the lack of that feature.

	And I did not even need a C compiler to do that.

 git-gui.sh   |    4 +++
 lib/diff.tcl |   75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 78 insertions(+), 1 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index a38293a..a0f7617 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -2400,6 +2400,10 @@ $ctxm add command \
 	-command {apply_hunk $cursorX $cursorY}
 set ui_diff_applyhunk [$ctxm index last]
 lappend diff_actions [list $ctxm entryconf $ui_diff_applyhunk -state]
+$ctxm add command \
+	-label [mc "Split Hunk"] \
+	-command {split_hunk $cursorX $cursorY}
+lappend diff_actions [list $ctxm entryconf [$ctxm index last] -state]
 $ctxm add separator
 $ctxm add command \
 	-label {Decrease Font Size} \
diff --git a/lib/diff.tcl b/lib/diff.tcl
index e09e125..88ec8f3 100644
--- a/lib/diff.tcl
+++ b/lib/diff.tcl
@@ -265,6 +265,79 @@ proc read_diff {fd} {
 	}
 }
 
+proc split_hunk {x y} {
+	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 c_lno [lindex [split [$ui_diff index @$x,$y] .] 0]
+	set s_idx [$ui_diff search -backwards -regexp ^@@ $c_lno.0 0.0]
+	if {$s_idx eq {} || $s_idx >= [expr $c_lno - 1]} {
+		unlock_index
+		return
+	}
+	set s_lno [lindex [split $s_idx .] 0]
+
+	# the first hunk will look like this: @@ -$m1,$m2 +$p1,$p2 @@
+	# the second hunk will look like this: @@ -$m3,$m4 +$p3,$p4 @@
+
+	# get original hunk numbers
+	set hunk_line [$ui_diff get $s_idx "$s_idx lineend"]
+	set re "@@ +-(\[0-9\]+)(,(\[0-9\]+))? +\\+(\[0-9\]+)(,(\[0-9\]+))? *@@"
+	if {![regexp $re $hunk_line dummy m1 dummy m4 p1 dummy p4] ||
+			$m1 == 0 || $p1 == 0} { # create/delete file
+		unlock_index
+		return
+	}
+	if {$m4 == ""} {
+		set m4 1
+	}
+	if {$p4 == ""} {
+		set p4 1
+	}
+
+	# count changes
+	set m2 0
+	set p2 0
+	for {set l [expr $s_lno + 1]} {$l < $c_lno} {incr l} {
+		switch -exact -- [$ui_diff get $l.0] {
+			" " {
+				incr m2
+				incr p2
+			}
+			"+" {
+				incr p2
+			}
+			"-" {
+				incr m2
+			}
+		}
+	}
+
+	# We could check if {$m2 == $p2 && $m2 == [expr $c_lno - $s_lno]}
+	# and just remove the hunk.  But let's not be too clever here.
+
+	set m3 [expr $m1 + $m2]
+	set m4 [expr $m4 - $m2]
+	set p3 [expr $p1 + $p2]
+	set p4 [expr $p4 - $p2]
+
+	if {$m4 == 0 && $p4 == 0} {
+		index_unlock
+		return
+	}
+
+	$ui_diff configure -state normal
+	$ui_diff delete $s_idx "$s_idx lineend"
+	$ui_diff insert $s_idx "@@ -$m1,$m2 +$p1,$p2 @@" d_@
+	$ui_diff insert $c_lno.0 "@@ -$m3,$m4 +$p3,$p4 @@\n" d_@
+	$ui_diff configure -state disabled
+
+	unlock_index
+}
+
 proc apply_hunk {x y} {
 	global current_diff_path current_diff_header current_diff_side
 	global ui_diff ui_index file_states
@@ -272,7 +345,7 @@ proc apply_hunk {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 --cached --whitespace=nowarn --unidiff-zero}
 	set mi [lindex $file_states($current_diff_path) 0]
 	if {$current_diff_side eq $ui_index} {
 		set mode unstage
-- 
1.5.3.rc2.42.gda8d-dirty

Re: [PATCH] Teach git-gui to split hunks

[David Kastrup]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> When you select the context menu item "Split Hunk" in the diff area,
> git-gui will now split the current hunk so that a new hunk starts at
> the current position.
>
> For this to work, apply has to be called with --unidiff-zero, since
> the new hunks can start or stop with a "-" or "+" line.

Unless one splits right at the beginning or end of an existing hunk,
wouldn't there be context which one could use?  Or does it confuse
patch when adjacent hunks have overlapping contexts?  At least if the
first hunk patches what is to be used as context in the second hunk, I
could imagine this.  And there is really no danger of losing synch in
this situation, anyhow.  So it would be more of a convenience thing
than anything else to be able to omit --unidiff-zero.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] Teach git-gui to split hunks

[Shawn O. Pearce]

David Kastrup <dak@gnu.org> wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > When you select the context menu item "Split Hunk" in the diff area,
> > git-gui will now split the current hunk so that a new hunk starts at
> > the current position.
> >
> > For this to work, apply has to be called with --unidiff-zero, since
> > the new hunks can start or stop with a "-" or "+" line.
> 
> Unless one splits right at the beginning or end of an existing hunk,
> wouldn't there be context which one could use?  Or does it confuse
> patch when adjacent hunks have overlapping contexts?  At least if the
> first hunk patches what is to be used as context in the second hunk, I
> could imagine this.  And there is really no danger of losing synch in
> this situation, anyhow.  So it would be more of a convenience thing
> than anything else to be able to omit --unidiff-zero.

Yea, there's context there.  Junio and I talked about this patch on
#git a few minutes ago.  I really appreciate that Dscho wrote it,
especially given that he hasn't really been into Tcl hacking for
Git much before.  But I'd really like to save/create context, like
`git add -i` does, so that we don't have to use --unidiff-zero here.

It won't matter if git-apply rejected overlapping context in this
case.  git-gui will only ever feed one hunk at a time to git-apply.
And if things get really f'd in the diff buffer the user can easily
regenerate it (right click, Refresh).

Right now git-gui's apply doesn't correctly update the other hunk
headers when you apply a hunk.  I've seen git-apply fail on some
hunks just for this reason.  Refreshing the diff (so git recomputes
the headers) works around the issue.  So I'm a little worried about
using --unidiff-zero here.

-- 
Shawn.

Re: [PATCH] Teach git-gui to split hunks

[Johannes Sixt]

Johannes Schindelin wrote:
> 
> When you select the context menu item "Split Hunk" in the diff area,
> git-gui will now split the current hunk so that a new hunk starts at
> the current position.
> 
> For this to work, apply has to be called with --unidiff-zero, since
> the new hunks can start or stop with a "-" or "+" line.

For chrissake, NO!

I tried this already, and it immediately corrupted my data.

The problem case is when the hunk you want to apply is not the first one
and the first one does not add and remove the same number of lines. In
this case, all that git-apply can do is to rely on line numbers. But
they are WRONG and apply the patch at the WRONG spot.

First, I didn't believe Linus when he preached that --unidiff-zero is
bad; it took only a day to become a follower. ;)

-- Hannes

Re: [PATCH] Teach git-gui to split hunks

[Johannes Schindelin]

Hi,

On Thu, 26 Jul 2007, Johannes Sixt wrote:

> Johannes Schindelin wrote:
> > 
> > When you select the context menu item "Split Hunk" in the diff area,
> > git-gui will now split the current hunk so that a new hunk starts at
> > the current position.
> > 
> > For this to work, apply has to be called with --unidiff-zero, since
> > the new hunks can start or stop with a "-" or "+" line.
> 
> For chrissake, NO!
> 
> I tried this already, and it immediately corrupted my data.
> 
> The problem case is when the hunk you want to apply is not the first one
> and the first one does not add and remove the same number of lines. In
> this case, all that git-apply can do is to rely on line numbers. But
> they are WRONG and apply the patch at the WRONG spot.
> 
> First, I didn't believe Linus when he preached that --unidiff-zero is
> bad; it took only a day to become a follower. ;)

Okay, convinced.  But I have some issues there, which I will outline in 
the reply to Shawn.

Ciao,
Dscho

Re: [PATCH] Teach git-gui to split hunks

[Johannes Schindelin]

Hi,

On Thu, 26 Jul 2007, Shawn O. Pearce wrote:

> [talk about the --unidiff-zero stuff]
> 
> Yea, there's context there.  Junio and I talked about this patch on #git 
> a few minutes ago.  I really appreciate that Dscho wrote it, especially 
> given that he hasn't really been into Tcl hacking for Git much before.

Heh.  I did my share of Tcl hacking.  In one (closed source) project, I 
was in constant awe why the devs chose Tcl as a scripting language, but Qt 
as windowing library (back when it was pretty expensive for a startup to 
use Qt).

> But I'd really like to save/create context, like `git add -i` does, so 
> that we don't have to use --unidiff-zero here.

See below.

> It won't matter if git-apply rejected overlapping context in this case.  
> git-gui will only ever feed one hunk at a time to git-apply. And if 
> things get really f'd in the diff buffer the user can easily regenerate 
> it (right click, Refresh).

But you're right, the other hunk headers should be updated.  I'll have a 
look at it this weekend.

> Right now git-gui's apply doesn't correctly update the other hunk 
> headers when you apply a hunk.  I've seen git-apply fail on some hunks 
> just for this reason.  Refreshing the diff (so git recomputes the 
> headers) works around the issue.  So I'm a little worried about using 
> --unidiff-zero here.

Okay, but just using more context means that you also have to _update_ the 
context of another hunk.  Imagine this:

--- a/x
+++ a/x
 one line
-a removed one
+an added one
 another line

Now you split between the "-" and "+" line.  If you stage the first hunk, 
not only do you have to update the hunk header of the second hunk (now the 
only one shown), which you already hinted should be done; you _also_ have 
to update the context in the second hunk, since it changed.

This is just tricky enough that I am tempted to have a go at it.  Not 
today, though, since I have other work to do, and the issues are easier to 
work around for the moment than to work around the lack of "Split Hunk" 
for me.

BTW did you think about any kind of integrity checking, i.e. see if the 
files involved still have the same hashes as when the diff was generated?  
It _is_ possible to use git-gui _and_ the command line...

Ciao,
Dscho

[PATCH] Teach git-gui to split hunks

[Johannes Schindelin]

When you select the context menu item "Split Hunk" in the diff area,
git-gui will now split the current hunk so that a new hunk starts at
the current position.

For this to work, apply has to be called with --unidiff-zero, since
the new hunks can start or stop with a "-" or "+" line.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Wed, 12 Dec 2007, Jason Sewall wrote:

	> On Dec 12, 2007 1:15 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
	> >
	> > I had a patch for splitting hunks in git-gui in August, but 
	> > there were some issues that I did not yet resolve.  If you 
	> > want to work on it, I'll gladly share that patch with you.
	> 
	> Sure, send it along. I'm not going to make any promises, but I 
	> could probably find time poke around in there.

	This was the next step that I did not yet quite complete:

+proc get_context_lines {line_number context_count direction variable} {
+       global ui_diff
+       upvar result $variable
+
+       set count 0
+       set result ""
+       for {set i $line_number} {$count < $context_count} {incr i $direction} {
+               switch -exact -- [$ui_diff get $i.0] {
+                       " " {
+                               append result [$ui_diff get $i.0 "$i.lineend"]
+                               incr count
+                       }
+                       "-" {
+                               append result [$ui_diff get $i.0 "$i.lineend"]
+                               incr count
+                       }
+                       "@" {
+                               return $count
+                       }
+                       "" {
+                               return $count
+                       }
+               }
+       }
+}

	Of course, this function would be used to add some context 
	to the new hunk boundaries (if needed), and to adjust the contexts
	of the remaining hunks whenever some hunk was applied.

	... and then get rid of that ugly unidiff-zero option.

 git-gui.sh   |    4 +++
 lib/diff.tcl |   75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 78 insertions(+), 1 deletions(-)

diff --git a/git-gui.sh b/git-gui.sh
index 1fca11f..0798d41 100755
--- a/git-gui.sh
+++ b/git-gui.sh
@@ -2567,6 +2567,10 @@ $ctxm add command \
 	-command {apply_hunk $cursorX $cursorY}
 set ui_diff_applyhunk [$ctxm index last]
 lappend diff_actions [list $ctxm entryconf $ui_diff_applyhunk -state]
+$ctxm add command \
+	-label [mc "Split Hunk"] \
+	-command {split_hunk $cursorX $cursorY}
+lappend diff_actions [list $ctxm entryconf [$ctxm index last] -state]
 $ctxm add separator
 $ctxm add command \
 	-label [mc "Decrease Font Size"] \
diff --git a/lib/diff.tcl b/lib/diff.tcl
index 43565e4..0c3f790 100644
--- a/lib/diff.tcl
+++ b/lib/diff.tcl
@@ -289,6 +289,79 @@ proc read_diff {fd} {
 	}
 }
 
+proc split_hunk {x y} {
+	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 c_lno [lindex [split [$ui_diff index @$x,$y] .] 0]
+	set s_idx [$ui_diff search -backwards -regexp ^@@ $c_lno.0 0.0]
+	if {$s_idx eq {} || $s_idx >= [expr $c_lno - 1]} {
+		unlock_index
+		return
+	}
+	set s_lno [lindex [split $s_idx .] 0]
+
+	# the first hunk will look like this: @@ -$m1,$m2 +$p1,$p2 @@
+	# the second hunk will look like this: @@ -$m3,$m4 +$p3,$p4 @@
+
+	# get original hunk numbers
+	set hunk_line [$ui_diff get $s_idx "$s_idx lineend"]
+	set re "@@ +-(\[0-9\]+)(,(\[0-9\]+))? +\\+(\[0-9\]+)(,(\[0-9\]+))? *@@"
+	if {![regexp $re $hunk_line dummy m1 dummy m4 p1 dummy p4] ||
+			$m1 == 0 || $p1 == 0} { # create/delete file
+		unlock_index
+		return
+	}
+	if {$m4 == ""} {
+		set m4 1
+	}
+	if {$p4 == ""} {
+		set p4 1
+	}
+
+	# count changes
+	set m2 0
+	set p2 0
+	for {set l [expr $s_lno + 1]} {$l < $c_lno} {incr l} {
+		switch -exact -- [$ui_diff get $l.0] {
+			" " {
+				incr m2
+				incr p2
+			}
+			"+" {
+				incr p2
+			}
+			"-" {
+				incr m2
+			}
+		}
+	}
+
+	# We could check if {$m2 == $p2 && $m2 == [expr $c_lno - $s_lno]}
+	# and just remove the hunk.  But let's not be too clever here.
+
+	set m3 [expr $m1 + $m2]
+	set m4 [expr $m4 - $m2]
+	set p3 [expr $p1 + $p2]
+	set p4 [expr $p4 - $p2]
+
+	if {$m4 == 0 && $p4 == 0} {
+		index_unlock
+		return
+	}
+
+	$ui_diff configure -state normal
+	$ui_diff delete $s_idx "$s_idx lineend"
+	$ui_diff insert $s_idx "@@ -$m1,$m2 +$p1,$p2 @@" d_@
+	$ui_diff insert $c_lno.0 "@@ -$m3,$m4 +$p3,$p4 @@\n" d_@
+	$ui_diff configure -state disabled
+
+	unlock_index
+}
+
 proc apply_hunk {x y} {
 	global current_diff_path current_diff_header current_diff_side
 	global ui_diff ui_index file_states
@@ -296,7 +369,7 @@ proc apply_hunk {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 --cached --whitespace=nowarn --unidiff-zero}
 	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."]
-- 
1.5.3.7.2250.g3893

Re: [PATCH] Teach git-gui to split hunks

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> When you select the context menu item "Split Hunk" in the diff area,
> git-gui will now split the current hunk so that a new hunk starts at
> the current position.
>
> For this to work, apply has to be called with --unidiff-zero, since
> the new hunks can start or stop with a "-" or "+" line.
> ...

I still have conceptual problem with this whole thing.  For example,
what does that MEAN to split this hunk from your patch...

> @@ -296,7 +369,7 @@ proc apply_hunk {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 --cached --whitespace=nowarn --unidiff-zero}
>  	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."]

... by clicking between the '-' and '+' lines, and apply only one half?

Well, the question was not very well stated.  I know what it means --
remove that old line, without replacing with the corrected/updated one.
The real question is how would that be useful?

Re: [PATCH] Teach git-gui to split hunks

[Johannes Schindelin]

Hi,

On Wed, 12 Dec 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > When you select the context menu item "Split Hunk" in the diff area, 
> > git-gui will now split the current hunk so that a new hunk starts at 
> > the current position.
> >
> > For this to work, apply has to be called with --unidiff-zero, since 
> > the new hunks can start or stop with a "-" or "+" line. ...
> 
> I still have conceptual problem with this whole thing.  For example, 
> what does that MEAN to split this hunk from your patch...
> 
> > @@ -296,7 +369,7 @@ proc apply_hunk {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 --cached --whitespace=nowarn --unidiff-zero}
> >  	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."]
> 
> ... by clicking between the '-' and '+' lines, and apply only one half?
> 
> Well, the question was not very well stated.  I know what it means -- 
> remove that old line, without replacing with the corrected/updated one. 
> The real question is how would that be useful?

The thing is: sometimes there is a patch which contains just one garbage 
line.  (I am talking about my current working tree, so you are not allowed 
to be offended by my language in this case.)

The thing I would like to do is right click on that line, start a new 
hunk, then right click on the next line to start yet another hunk, and 
apply this and the first hunk.

It is a lazy way to edit a patch.

Ciao,
Dscho

Re: [PATCH] Teach git-gui to split hunks

[Jean-François Veillette]

> Well, the question was not very well stated.  I know what it means --
> remove that old line, without replacing with the corrected/updated  
> one.
> The real question is how would that be useful?

I often get big hunk just because I modified whitespaces around  
relevent pieces of code, the ability to segment the changes and only  
pick isolated and specific lines for a commit (not commiting  
whitespaces surrounding real code changes) would be very welcome.
Maybe I should know better, but the actual hunk selection in git gui  
is quite good already, but the ability to be more precise on how a  
hunk is defined is a welcome change.

- jfv

Re: [PATCH] Teach git-gui to split hunks

[Junio C Hamano]

Jean-François Veillette <jean_francois_veillette@yahoo.ca> writes:

>> Well, the question was not very well stated.  I know what it means --
>> remove that old line, without replacing with the corrected/updated
>> one.
>> The real question is how would that be useful?
>
> I often get big hunk just because I modified whitespaces around
> relevent pieces of code, the ability to segment the changes and only
> pick isolated and specific lines for a commit (not commiting
> whitespaces surrounding real code changes) would be very welcome.
> Maybe I should know better, but the actual hunk selection in git gui
> is quite good already, but the ability to be more precise on how a
> hunk is defined is a welcome change.

Oh, I wasn't questioning the usefulness of hunk splitting in general.
It is sometimes useful and that is why we have "add -i".

If you have something like this:

        @@ -j,k +l,m @@
         common 1
         common 2
        -preimage
        +postimage
         common 3
        -deleted
         common 4
         common 5

I think it makes sense to split it into two logical (overlapping) hunks:

        @@ -j,(k-3) +l,(m-2) @@
         common 1
         common 2
        -preimage
        +postimage
         common 3

and

        @@ -j,(k-3) +l,(m-3) @@
         common 3
        -deleted
         common 4
         common 5

and being able to apply one of them independent from the other, or
re-combine them back into one hunk.

I was just questioning if it makes sense to split a hunk like this in
the middle of -/+ lines:

	@@ -j,k +l,m @@
	 common
	 common
	-pre 1
	-pre 2
        -pre 3
        +post 1
	+post 2
	 common

You could split between "-pre 2" and "-pre 3", but I do not think that
would be so useful.  It is a different story if you allowed the above to
first be transformed into this way (assuming that "pre 1" and "pre 2"
corresponds to "post 1"):

	@@ -j,k +l,m @@
	 common
	 common
	-pre 1
	-pre 2
        +post 1
        -pre 3
	+post 2
	 common

and then be split between "+post 1" and "-pre 3".  That may make sense
in some context.

Re: [PATCH] Teach git-gui to split hunks

[Wincent Colaiuta]

El 12/12/2007, a las 21:18, Junio C Hamano escribió:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> When you select the context menu item "Split Hunk" in the diff area,
>> git-gui will now split the current hunk so that a new hunk starts at
>> the current position.
>>
>> For this to work, apply has to be called with --unidiff-zero, since
>> the new hunks can start or stop with a "-" or "+" line.
>> ...
>
> I still have conceptual problem with this whole thing.  For example,
> what does that MEAN to split this hunk from your patch...
>
>> @@ -296,7 +369,7 @@ proc apply_hunk {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 --cached --whitespace=nowarn --unidiff-zero}
>> 	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."]
>
> ... by clicking between the '-' and '+' lines, and apply only one  
> half?
>
> Well, the question was not very well stated.  I know what it means --
> remove that old line, without replacing with the corrected/updated  
> one.
> The real question is how would that be useful?

I don't know if it would be useful, but I think the more important  
concern here is consistency. ie. it should split hunks the same way  
"git add -i" does. Both "git gui" and "git add -i" are official parts  
of Git, so in the interests of coherency they should share the same  
concept of "what it means to split a hunk".

"git add -i" considers any hunk where a there are multiple groups of  
deletions and/or insertions separated by context lines to be  
"splittable" (on the boundaries defined by those intervening context  
line), and all others to be unsplittable. I think this is a fairly  
intuitive way to conceptualize splitting, so if it comes down to  
making "git gui" split like "git add -i", or making "git add -i" split  
like this patch proposes that "git gui" should do it, then I'd vote  
for the former.

Cheers,
Wincent

Re: [PATCH] Teach git-gui to split hunks

[Johannes Sixt]

Johannes Schindelin schrieb:
> When you select the context menu item "Split Hunk" in the diff area,
> git-gui will now split the current hunk so that a new hunk starts at
> the current position.
> 
> For this to work, apply has to be called with --unidiff-zero, since
> the new hunks can start or stop with a "-" or "+" line.

NACK! --unidiff-zero eats your data.

1. Prepare a modification that adds 2 lines that are *not* adjacent, like this:

	@@ -6,6 +6,8 @@ git-checkout [options] [<branch>] [<paths>...]
	 --
	 b=          create a new branch started at <branch>
	+first
	 l           create the new branch's reflog
	 track       arrange that the new branch tracks the remote branch
	+after track
	 f           proceed even if the index or working tree is not HEAD
	 m           merge local modifications into the new branch

2. Reduce context to zero.
3. Stage *second* hunk.

Result: It is staged at the wrong place:

	@@ -9,4 +9,5 @@ l           create the new branch's reflog
	 track       arrange that the new branch tracks the remote branch
	 f           proceed even if the index or working tree is not HEAD
	+after track
	 m           merge local modifications into the new branch
	 q,quiet     be quiet


Reason: --unidiff-zero can only look at the line numbers. And those are
wrong because it doesn't account for the shift in line numbers caused by the
first hunk.

-- Hannes

Re: [PATCH] Teach git-gui to split hunks

[Shawn O. Pearce]

Johannes Sixt <j.sixt@viscovery.net> wrote:
> Johannes Schindelin schrieb:
> > When you select the context menu item "Split Hunk" in the diff area,
> > git-gui will now split the current hunk so that a new hunk starts at
> > the current position.
> > 
> > For this to work, apply has to be called with --unidiff-zero, since
> > the new hunks can start or stop with a "-" or "+" line.
> 
> NACK! --unidiff-zero eats your data.

Yea, don't worry about that, I won't be applying any patch to git-gui
that feeds data to git-apply with --undiff-zero.  Not unless its
completely bullet-proof that the hunk headers will *never* be wrong.

I'd rather always apply with context and let git-apply do its thing
to validate the hunks.  If you can get the hunk headers computed
right you can also get the context computed right, which means
git-apply can actually verify the patch can be applied, thus double
checking the splitter.
 
> Reason: --unidiff-zero can only look at the line numbers. And those are
> wrong because it doesn't account for the shift in line numbers caused by the
> first hunk.

-- 
Shawn.

Re: [PATCH] Teach git-gui to split hunks

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> For this to work, apply has to be called with --unidiff-zero, since
> the new hunks can start or stop with a "-" or "+" line.

You do not have to do "unidiff zero".  Suppose you have this hunk you
need to split.

diff --git a/read-cache.c b/read-cache.c
index 7db5588..4d12073 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -12,8 +12,8 @@
 /* Index extensions.
  *
  * The first letter should be 'A'..'Z' for extensions that are not
- * necessary for a correct operation (i.e. optimization data).
- * When new extensions are added that _needs_ to be understood in
+ * necessary for a correct operation (that is, optimization data).
+ * When new extensions are added that needs to be understood in
  * order to correctly interpret the index file, pick character that
  * is outside the range, to cause the reader to abort.
  */

Think about taking the s/i.e./that is,/ substitution without taking the
other s/_needs_/needs/ substitution.  You do not split the hunk between
two '-' lines, but effectively make it into this hunk instead:

diff --git a/read-cache.c b/read-cache.c
index 7db5588..4d12073 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -12,8 +12,8 @@
 /* Index extensions.
  *
  * The first letter should be 'A'..'Z' for extensions that are not
- * necessary for a correct operation (i.e. optimization data).
+ * necessary for a correct operation (that is, optimization data).
  * When new extensions are added that _needs_ to be understood in
  * order to correctly interpret the index file, pick character that
  * is outside the range, to cause the reader to abort.
  */

That is, , if you want to do finer grained hunk splitting than what "git
add -p" lets you do, you do _not_ let user specify "I want to split the
hunk into two, before this point and after this point".  Instead, let
the user pick zero or more '-' line and zero or more '+' line, and
adjust the context around it.  An unpicked '-' line becomes the common
context, and an unpicked '+' line disappears.  After that, you recount
the diff.  That way, you do not have to do any "unidiff zero" cop-out.

At the same time, you can stash away what was _not_ picked, creating two
variants to be applied on top of the result of applying (or not
applying) the picked patch, if you want to allow "undo".

(variant one: applies after the above is applied)
@@ -12,8 +12,8 @@
 /* Index extensions.
  *
  * The first letter should be 'A'..'Z' for extensions that are not
  * necessary for a correct operation (that is, optimization data).
- * When new extensions are added that _needs_ to be understood in
+ * When new extensions are added that needs to be understood in
  * order to correctly interpret the index file, pick character that
  * is outside the range, to cause the reader to abort.
  */

(variant two: applies if the above is not applied)
@@ -12,8 +12,8 @@
 /* Index extensions.
  *
  * The first letter should be 'A'..'Z' for extensions that are not
  * necessary for a correct operation (i.e. optimization data).
- * When new extensions are added that _needs_ to be understood in
+ * When new extensions are added that needs to be understood in
  * order to correctly interpret the index file, pick character that
  * is outside the range, to cause the reader to abort.
  */

Re: [PATCH] Teach git-gui to split hunks

[Johannes Sixt]

Junio C Hamano schrieb:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>> For this to work, apply has to be called with --unidiff-zero, since
>> the new hunks can start or stop with a "-" or "+" line.
> 
> You do not have to do "unidiff zero".  Suppose you have this hunk you
> need to split.
> 
> diff --git a/read-cache.c b/read-cache.c
> index 7db5588..4d12073 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -12,8 +12,8 @@
>  /* Index extensions.
>   *
>   * The first letter should be 'A'..'Z' for extensions that are not
> - * necessary for a correct operation (i.e. optimization data).
> - * When new extensions are added that _needs_ to be understood in
> + * necessary for a correct operation (that is, optimization data).
> + * When new extensions are added that needs to be understood in
>   * order to correctly interpret the index file, pick character that
>   * is outside the range, to cause the reader to abort.
>   */
...
> That is, , if you want to do finer grained hunk splitting than what "git
> add -p" lets you do, you do _not_ let user specify "I want to split the
> hunk into two, before this point and after this point".  Instead, let
> the user pick zero or more '-' line and zero or more '+' line, and
> adjust the context around it.  An unpicked '-' line becomes the common
> context, and an unpicked '+' line disappears.  After that, you recount
> the diff.  That way, you do not have to do any "unidiff zero" cop-out.

In this case I would expect two adjacent hunks: one that covers the selected
changes, another one with the remaining changes, but each against the original:

@@ -12,7 +12,7 @@
 /* Index extensions.
  *
  * The first letter should be 'A'..'Z' for extensions that are not
- * necessary for a correct operation (i.e. optimization data).
+ * necessary for a correct operation (that is, optimization data).
  * When new extensions are added that _needs_ to be understood in
  * order to correctly interpret the index file, pick character that
  * is outside the range, to cause the reader to abort.
@@ -13,7 +13,7 @@
  *
  * The first letter should be 'A'..'Z' for extensions that are not
  * necessary for a correct operation (i.e. optimization data).
- * When new extensions are added that _needs_ to be understood in
+ * When new extensions are added that needs to be understood in
  * order to correctly interpret the index file, pick character that
  * is outside the range, to cause the reader to abort.
  */

Then I can stage either one. After that operation, git-gui refreshes the
patch display. This is now the time where the hunk that was not staged
should be updated to reflect the correct diff against the staged hunk.

-- Hannes

Re: [PATCH] Teach git-gui to split hunks

[Johannes Schindelin]

Hi,

On Thu, 13 Dec 2007, Johannes Sixt wrote:

> Johannes Schindelin schrieb:
> > When you select the context menu item "Split Hunk" in the diff area,
> > git-gui will now split the current hunk so that a new hunk starts at
> > the current position.
> > 
> > For this to work, apply has to be called with --unidiff-zero, since
> > the new hunks can start or stop with a "-" or "+" line.
> 
> NACK! --unidiff-zero eats your data.

Did I not make crystal clear that I intended this patch to be cleaned up 
first, to not need unidiff-zero?

If so, my sincerest apologies.

THIS PATCH IS NOT MEANT FOR APPLICATION, BUT FOR JASON TO PLAY WITH.

Ciao,
Dscho

Re: [PATCH] Teach git-gui to split hunks

[Johannes Schindelin]

Hi,

On Thu, 13 Dec 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > For this to work, apply has to be called with --unidiff-zero, since
> > the new hunks can start or stop with a "-" or "+" line.
> 
> You do not have to do "unidiff zero".  Suppose you have this hunk you
> need to split.
>
> [describes to pick zero or more '-' lines and zero or more '+' lines]

I thought about that, but the UI is not trivial.  The UI for my solution 
is.

Ciao,
Dscho

Re: [PATCH] Teach git-gui to split hunks

[Johannes Sixt]

Johannes Schindelin schrieb:
> On Thu, 13 Dec 2007, Junio C Hamano wrote:
> 
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>>> For this to work, apply has to be called with --unidiff-zero, since
>>> the new hunks can start or stop with a "-" or "+" line.
>> You do not have to do "unidiff zero".  Suppose you have this hunk you
>> need to split.
>>
>> [describes to pick zero or more '-' lines and zero or more '+' lines]
> 
> I thought about that, but the UI is not trivial.  The UI for my solution 
> is.

It's probably sufficient to have an option "Stage this Line": Once you have
staged enough lines, the hunk will be split automatically by the current
number-of-context-lines setting.

-- Hannes

Re: [PATCH] Teach git-gui to split hunks

[Johannes Schindelin]

Hi,

On Thu, 13 Dec 2007, Johannes Sixt wrote:

> Johannes Schindelin schrieb:
> > On Thu, 13 Dec 2007, Junio C Hamano wrote:
> > 
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >>
> >>> For this to work, apply has to be called with --unidiff-zero, since 
> >>> the new hunks can start or stop with a "-" or "+" line.
> >> You do not have to do "unidiff zero".  Suppose you have this hunk you 
> >> need to split.
> >>
> >> [describes to pick zero or more '-' lines and zero or more '+' lines]
> > 
> > I thought about that, but the UI is not trivial.  The UI for my 
> > solution is.
> 
> It's probably sufficient to have an option "Stage this Line": Once you 
> have staged enough lines, the hunk will be split automatically by the 
> current number-of-context-lines setting.

And your hand falls off... ;-)

Seriously, I often have a big chunk of changes, for example with 
an indentation change, where I want to stage everything _but_ one line in 
the middle.  Just staging that, "git checkout <file>" and fixing the 
indentation of that single line speeds up my procedure vastly.

Ciao,
Dscho