patch, 2 partsgit-gui: Fix "Stage/Unstage Line" with one line of context.

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

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

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

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?

Junio C Hamano <gitster@pobox.com> wrote:

...

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

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

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

Much easier to understand.  Thanks.

Junio C Hamano schrieb:

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?

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

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