[PATCH 1/2] git-gui: Fix "Stage/Unstage Line" with one line of context.

[PATCH 1/2] git-gui: Fix "Stage/Unstage Line" with one line of context.

[Johannes Sixt]

To "Stage/Unstage Line" we construct a patch that contains exactly one
change (either addition or removal); the hunk header was forged by counting
the old side and adjusting the count by +/-1 for the new side. But when we
counted the context we never counted the changed line itself. If the hunk
had only one removal line and one line of context, like this:

    @@ -1,3 +1,2 @@
     context 1
    -removal
     context 2

We had constructed this patch:

    @@ -1,2 +1,1 @@
     context 1
    -removal
     context 2

which does not apply because git apply deduces that it must apply at the
end of the file. ("context 2" is considered garbage and ignored.) The fix
is that removal lines must be counted towards the context of the old side.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 lib/diff.tcl |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/lib/diff.tcl b/lib/diff.tcl
index 96ba949..ee7f391 100644
--- a/lib/diff.tcl
+++ b/lib/diff.tcl
@@ -423,6 +423,9 @@ proc apply_line {x y} {
 			# the line to stage/unstage
 			set ln [$ui_diff get $i_l $next_l]
 			set patch "$patch$ln"
+			if {$c1 eq {-}} {
+				set n [expr $n+1]
+			}
 		} elseif {$c1 ne {-} && $c1 ne {+}} {
 			# context line
 			set ln [$ui_diff get $i_l $next_l]
-- 
1.5.6.3.323.g1e58

[PATCH 2/2] git-gui: Allow "Stage Line" to stage adjacent changes independently

[Johannes Sixt]

Consider this hunk:

  @@ -10,4 +10,4 @@
   context before
  -old 1
  -old 2
  +new 1
  +new 2
   context after

[Nomenclature: to "stage change 2" means to stage lines "-old 1" and
"+new 1", in any order; likewise for "unstage" and "change 2".]

Previously, we had this situation:

  stage change 1 was not possible
  stage change 2 was possible
  unstage change 1 was possible
  unstage change 2 was not possible

With this change we have this situation:

  stage change 1 is possible
  stage change 2 is not possible
  unstage change 1 is possible
  unstage change 2 is not possible

What's the deal?

Previously, it was impossible to stage change 1 without also staging
change 2; not even by staging the complete hunk and unstaging the unwanted
part.

With this change we can achieve all goals:

  hunk is     want staged   want unstaged   do this
  ----------------------------------------------------------------------
  unstaged    change 1      change 2        stage change 1
  unstaged    change 2      change 1        stage hunk, unstage change 1
  staged      change 1      change 2        unstage hunk, stage change 1
  staged      change 2      change 1        unstage change 1

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 lib/diff.tcl |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/lib/diff.tcl b/lib/diff.tcl
index ee7f391..77990c5 100644
--- a/lib/diff.tcl
+++ b/lib/diff.tcl
@@ -411,6 +411,53 @@ proc apply_line {x y} {
 	set hh [lindex [split $hh ,] 0]
 	set hln [lindex [split $hh -] 1]
 
+	# There is a special situation to take care of. Consider this hunk:
+	#
+	#    @@ -10,4 +10,4 @@
+	#     context before
+	#    -old 1
+	#    -old 2
+	#    +new 1
+	#    +new 2
+	#     context after
+	#
+	# We used to keep the context lines in the order they appear in the
+	# hunk. But then it is not possible to correctly stage only
+	# "-old 1" and "+new 1" - it would result in this staged text:
+	#
+	#    context before
+	#    old 2
+	#    new 1
+	#    context after
+	#
+	# (By symmetry it is not possible to *un*stage "old 2" and "new 2".)
+	#
+	# We resolve the problem by introducing an asymmetry, namely, when
+	# a "+" line is *staged*, it is moved in front of the context lines
+	# that are generated from the "-" lines that are immediately before
+	# the "+" block. That is, we construct this patch:
+	#
+	#    @@ -10,4 +10,5 @@
+	#     context before
+	#    +new 1
+	#     old 1
+	#     old 2
+	#     context after
+	#
+	# But we do *not* treat "-" lines that are *un*staged in a special
+	# way.
+	#
+	# With this asymmetry it is possible to stage the change
+	# "old 1" -> "new 1" directly, and to stage the change
+	# "old 2" -> "new 2" by first staging the entire hunk and
+	# then unstaging the change "old 1" -> "new 1".
+
+	# This is non-empty if and only if we are _staging_ changes;
+	# then it accumulates the consecutive "-" lines (after converting
+	# them to context lines) in order to be moved after the "+" change
+	# line.
+	set pre_context {}
+
 	set n 0
 	set i_l [$ui_diff index "$i_l + 1 lines"]
 	set patch {}
@@ -422,19 +469,27 @@ proc apply_line {x y} {
 		    [$ui_diff compare $the_l < $next_l]} {
 			# the line to stage/unstage
 			set ln [$ui_diff get $i_l $next_l]
-			set patch "$patch$ln"
 			if {$c1 eq {-}} {
 				set n [expr $n+1]
+				set patch "$patch$pre_context$ln"
+			} else {
+				set patch "$patch$ln$pre_context"
 			}
+			set pre_context {}
 		} elseif {$c1 ne {-} && $c1 ne {+}} {
 			# context line
 			set ln [$ui_diff get $i_l $next_l]
-			set patch "$patch$ln"
+			set patch "$patch$pre_context$ln"
 			set n [expr $n+1]
+			set pre_context {}
 		} elseif {$c1 eq $to_context} {
 			# turn change line into context line
 			set ln [$ui_diff get "$i_l + 1 chars" $next_l]
-			set patch "$patch $ln"
+			if {$c1 eq {-}} {
+				set pre_context "$pre_context $ln"
+			} else {
+				set patch "$patch $ln"
+			}
 			set n [expr $n+1]
 		}
 		set i_l $next_l
-- 
1.5.6.3.323.g1e58

Re: [PATCH 2/2] git-gui: Allow "Stage Line" to stage adjacent changes independently

[Junio C Hamano]

Johannes Sixt <johannes.sixt@telecom.at> writes:

> Consider this hunk:
>
>   @@ -10,4 +10,4 @@
>    context before
>   -old 1
>   -old 2
>   +new 1
>   +new 2
>    context after
>
> [Nomenclature: to "stage change 2" means to stage lines "-old 1" and
> "+new 1", in any order; likewise for "unstage" and "change 2".]

You lost me.

Do you mean to say that you always interpret the above hunk as:

   @@ -10,4 +10,4 @@
    context before
   -old 1
   +new 1
   -old 2
   +new 2
    context after

and call "replace 'old 1' with 'new 1'" as "change 1", "replace 'old
2' with 'new 2'" as "change 2"?

If it is what you are doing, it does not make much sense to me.  "new 1"
may correspond to "old 1" and "old 2" while "new 2" may be an independent
addition.  E.g.

   @@ -10,4 +10,4 @@
    context before
   -#define add(x,y) \
   - (x) + (y)
   +#define add(x,y) ((x)+(y))
   +#define sub(x,y) ((x)-(y))
    context after

I might want to pick bugfix of add() definition without using the new
definition of sub().

Please call

	"-old 1" - change #1
        "-old 2" - change #2
        "+new 1" - change #3
        "+new 2" - change #4

and try explaining what you are doing again, pretty please?

Re: [PATCH 2/2] git-gui: Allow "Stage Line" to stage adjacent changes independently

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> Johannes Sixt <johannes.sixt@telecom.at> writes:
> 
> > Consider this hunk:
> >
> >   @@ -10,4 +10,4 @@
> >    context before
> >   -old 1
> >   -old 2
> >   +new 1
> >   +new 2
> >    context after
> >
> > [Nomenclature: to "stage change 2" means to stage lines "-old 1" and
> > "+new 1", in any order; likewise for "unstage" and "change 2".]
> 
> You lost me.
...
> and try explaining what you are doing again, pretty please?

Me too.  I really appreciate the effort spent to improve this
feature, but I really didn't follow the commit message at all.  :-|

-- 
Shawn.

[PATCH 2/2 v2] git-gui: "Stage Line": Treat independent changes in adjacent lines better

[Johannes Sixt]

Assume that we want to commit these states:

  Old state == HEAD    Intermediate state   New state
  --------------------------------------------------------
  context before       context before       context before
  old 1                new 1                new 1
  old 2                old 2                new 2
  context after        context after        context after

that is, want to commit two changes in this order:

  1. transform "old 1" into "new 1"
  2. transform "old 2" into "new 2"

[This discussion and this patch is about this very case and one other case
as outlined below; any other intermediate states that one could imagine are
not affected by this patch.]

Now assume further, that we have not staged and commited anything, but we
have already changed the working file to the new state. Then we will see
this hunk in the "Unstaged Changes":

  @@ -1,4 +1,4 @@
   context before
  -old 1
  -old 2
  +new 1
  +new 2
   context after

The obvious way to stage the intermediate state is to apply "Stage This
Line" to "-old 1" and "+new 1". Unfortunately, this resulted in this
intermediate state:

  context before
  old 2
  new 1
  context after

which is not what we wanted. In fact, it was impossible to stage the
intermediate state using "Stage Line". The crux was that if a "+" line was
staged, then the "-" lines were converted to context lines and arranged
*before* the "+" line in the forged hunk that we fed to 'git apply'.

With this patch we now treat "+" lines that are staged differently. In
particular, the "-" lines before the "+" block are moved *after* the
staged "+" line. Now it is possible to get the correct intermediate state
by staging "-old 1" and "+new 1". Problem solved.

But there is a catch.

Noticing that we didn't get the right intermediate state by staging
"-old 1" and "+new 1", we could have had the idea to stage the complete
hunk and to *unstage* "-old 2" and "+new 2". But... the result is the same.
The reason is that there is the exact symmetric problem with unstaging the
last "-" and "+" line that are in adjacent blocks of "-" and "+" lines.

This patch does *not* change the way in which "-" lines are *unstaged*.

Why? Because if we did (i.e. move "+" lines before the "-" line after
converting them to context lines), then it would be impossible to stage
this intermediate state:

  context before
  old 1
  new 2
  context after

that is, it would be impossible to stage the two independet changes in the
opposite order.

Let's look at this case a bit further: The obvious way to get this
intermediate state would be to apply "Stage This Line" to "-old 2" and
"+new 2". Before this patch, this worked as expected. With this patch, it
does not work as expected, but it can still be achieved by first staging
the entire hunk, then *unstaging* "-old 1" and "+new 1".

In summary, this patch makes a common case possible, at the expense that
a less common case is made more complicated for the user.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---

  The patch is unmodified, but I hope the commit message is clearer.

  -- Hannes

 lib/diff.tcl |   61 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/lib/diff.tcl b/lib/diff.tcl
index ee7f391..77990c5 100644
--- a/lib/diff.tcl
+++ b/lib/diff.tcl
@@ -411,6 +411,53 @@ proc apply_line {x y} {
 	set hh [lindex [split $hh ,] 0]
 	set hln [lindex [split $hh -] 1]
 
+	# There is a special situation to take care of. Consider this hunk:
+	#
+	#    @@ -10,4 +10,4 @@
+	#     context before
+	#    -old 1
+	#    -old 2
+	#    +new 1
+	#    +new 2
+	#     context after
+	#
+	# We used to keep the context lines in the order they appear in the
+	# hunk. But then it is not possible to correctly stage only
+	# "-old 1" and "+new 1" - it would result in this staged text:
+	#
+	#    context before
+	#    old 2
+	#    new 1
+	#    context after
+	#
+	# (By symmetry it is not possible to *un*stage "old 2" and "new 2".)
+	#
+	# We resolve the problem by introducing an asymmetry, namely, when
+	# a "+" line is *staged*, it is moved in front of the context lines
+	# that are generated from the "-" lines that are immediately before
+	# the "+" block. That is, we construct this patch:
+	#
+	#    @@ -10,4 +10,5 @@
+	#     context before
+	#    +new 1
+	#     old 1
+	#     old 2
+	#     context after
+	#
+	# But we do *not* treat "-" lines that are *un*staged in a special
+	# way.
+	#
+	# With this asymmetry it is possible to stage the change
+	# "old 1" -> "new 1" directly, and to stage the change
+	# "old 2" -> "new 2" by first staging the entire hunk and
+	# then unstaging the change "old 1" -> "new 1".
+
+	# This is non-empty if and only if we are _staging_ changes;
+	# then it accumulates the consecutive "-" lines (after converting
+	# them to context lines) in order to be moved after the "+" change
+	# line.
+	set pre_context {}
+
 	set n 0
 	set i_l [$ui_diff index "$i_l + 1 lines"]
 	set patch {}
@@ -422,19 +469,27 @@ proc apply_line {x y} {
 		    [$ui_diff compare $the_l < $next_l]} {
 			# the line to stage/unstage
 			set ln [$ui_diff get $i_l $next_l]
-			set patch "$patch$ln"
 			if {$c1 eq {-}} {
 				set n [expr $n+1]
+				set patch "$patch$pre_context$ln"
+			} else {
+				set patch "$patch$ln$pre_context"
 			}
+			set pre_context {}
 		} elseif {$c1 ne {-} && $c1 ne {+}} {
 			# context line
 			set ln [$ui_diff get $i_l $next_l]
-			set patch "$patch$ln"
+			set patch "$patch$pre_context$ln"
 			set n [expr $n+1]
+			set pre_context {}
 		} elseif {$c1 eq $to_context} {
 			# turn change line into context line
 			set ln [$ui_diff get "$i_l + 1 chars" $next_l]
-			set patch "$patch $ln"
+			if {$c1 eq {-}} {
+				set pre_context "$pre_context $ln"
+			} else {
+				set patch "$patch $ln"
+			}
 			set n [expr $n+1]
 		}
 		set i_l $next_l
-- 
1.5.6.1.275.g0a3e0f

Re: [PATCH 2/2 v2] git-gui: "Stage Line": Treat independent changes in adjacent lines better

[Junio C Hamano]

Johannes Sixt <johannes.sixt@telecom.at> writes:

> Assume that we want to commit these states:
>
>   Old state == HEAD    Intermediate state   New state
>   --------------------------------------------------------
>   context before       context before       context before
>   old 1                new 1                new 1
>   old 2                old 2                new 2
>   context after        context after        context after

Much easier to understand.  Thanks.

Re: [PATCH 2/2] git-gui: Allow "Stage Line" to stage adjacent changes independently

[Johannes Sixt]

Junio C Hamano schrieb:
> Johannes Sixt <johannes.sixt@telecom.at> writes:
> 
>> Consider this hunk:
>>
>>   @@ -10,4 +10,4 @@
>>    context before
>>   -old 1
>>   -old 2
>>   +new 1
>>   +new 2
>>    context after
>>
>> [Nomenclature: to "stage change 2" means to stage lines "-old 1" and
>> "+new 1", in any order; likewise for "unstage" and "change 2".]
> 
> You lost me.
> 
> Do you mean to say that you always interpret the above hunk as:
> 
>    @@ -10,4 +10,4 @@
>     context before
>    -old 1
>    +new 1
>    -old 2
>    +new 2
>     context after
> 
> and call "replace 'old 1' with 'new 1'" as "change 1", "replace 'old
> 2' with 'new 2'" as "change 2"?

No, it is not that I *always* interpret it this way. There is a problem to
fix only if I *want* to interpret it this way. Probably that's what I have
to make clear?

> If it is what you are doing, it does not make much sense to me.  "new 1"
> may correspond to "old 1" and "old 2" while "new 2" may be an independent
> addition.  E.g.
> 
>    @@ -10,4 +10,4 @@
>     context before
>    -#define add(x,y) \
>    - (x) + (y)
>    +#define add(x,y) ((x)+(y))
>    +#define sub(x,y) ((x)-(y))
>     context after
> 
> I might want to pick bugfix of add() definition without using the new
> definition of sub().

In order to that, there is nothing to fix; you can do that today without
this patch.

> Please call
> 
> 	"-old 1" - change #1
>         "-old 2" - change #2
>         "+new 1" - change #3
>         "+new 2" - change #4
> 
> and try explaining what you are doing again, pretty please?

No, this sounds like 4 independent changes, and that is not what this fix
is about.

I'll try to come up with a better wording.

-- Hannes