[PATCH] git gui: show diffs with a minimum of 1 context line

[PATCH] git gui: show diffs with a minimum of 1 context line

[Clemens Buchacher]

git apply does not handle diffs without context correctly. Configuring git
gui to show zero context lines therefore breaks staging.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

In reply to this patch I will send a first attempt at fixing this problem
instead of avoiding it. There does not seem to be a straightforward
solution, however, so this should hide the bug for now.

 git-gui/git-gui.sh     |    2 +-
 git-gui/lib/diff.tcl   |    2 +-
 git-gui/lib/option.tcl |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index ad65aaa..86402d4 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -1932,7 +1932,7 @@ proc show_more_context {} {
 
 proc show_less_context {} {
 	global repo_config
-	if {$repo_config(gui.diffcontext) >= 1} {
+	if {$repo_config(gui.diffcontext) > 1} {
 		incr repo_config(gui.diffcontext) -1
 		reshow_diff
 	}
diff --git a/git-gui/lib/diff.tcl b/git-gui/lib/diff.tcl
index 52b79e4..4a7138b 100644
--- a/git-gui/lib/diff.tcl
+++ b/git-gui/lib/diff.tcl
@@ -175,7 +175,7 @@ proc show_diff {path w {lno {}} {scroll_pos {}}} {
 
 	lappend cmd -p
 	lappend cmd --no-color
-	if {$repo_config(gui.diffcontext) >= 0} {
+	if {$repo_config(gui.diffcontext) >= 1} {
 		lappend cmd "-U$repo_config(gui.diffcontext)"
 	}
 	if {$w eq $ui_index} {
diff --git a/git-gui/lib/option.tcl b/git-gui/lib/option.tcl
index ffb3f00..5e1346e 100644
--- a/git-gui/lib/option.tcl
+++ b/git-gui/lib/option.tcl
@@ -125,7 +125,7 @@ proc do_options {} {
 		{b gui.matchtrackingbranch {mc "Match Tracking Branches"}}
 		{b gui.fastcopyblame {mc "Blame Copy Only On Changed Files"}}
 		{i-20..200 gui.copyblamethreshold {mc "Minimum Letters To Blame Copy On"}}
-		{i-0..99 gui.diffcontext {mc "Number of Diff Context Lines"}}
+		{i-1..99 gui.diffcontext {mc "Number of Diff Context Lines"}}
 		{i-0..99 gui.commitmsgwidth {mc "Commit Message Text Width"}}
 		{t gui.newbranchtemplate {mc "New Branch Name Template"}}
 		} {
-- 
1.6.0

[PATCH] git gui: use apply --unidiff-zero when staging hunks without context

[Clemens Buchacher]

git apply does not work correctly with zero-context patches. It does a
little better with --unidiff-zero.
---

This appears to fix staging hunks with zero context lines in the majority of
cases. Staging individual lines still is a problem frequently.

In any case, it's easy enough to break zero-context diff & patch like this:

echo a > victim
git add victim
echo b >> victim
git diff -U0 | git apply --cached --unidiff-zero
git diff

So before delving into this problem to deeply, I'd like to find out who
needs fixing exactly. Is there documentation defining how zero-context git
diff output should look like? Or is git apply the culprit in the bug above?

Or do we even want to support applying zero-context patches? If not, we
should detect and fail such attempts.

Clemens

 git-gui/lib/diff.tcl |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/git-gui/lib/diff.tcl b/git-gui/lib/diff.tcl
index 52b79e4..78c1b56 100644
--- a/git-gui/lib/diff.tcl
+++ b/git-gui/lib/diff.tcl
@@ -302,12 +302,15 @@ proc read_diff {fd scroll_pos} {
 
 proc apply_hunk {x y} {
 	global current_diff_path current_diff_header current_diff_side
-	global ui_diff ui_index file_states
+	global ui_diff ui_index file_states repo_config
 
 	if {$current_diff_path eq {} || $current_diff_header eq {}} return
 	if {![lock_index apply_hunk]} return
 
 	set apply_cmd {apply --cached --whitespace=nowarn}
+	if {$repo_config(gui.diffcontext) eq 0} {
+		lappend apply_cmd --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."]
@@ -375,12 +378,15 @@ proc apply_hunk {x y} {
 
 proc apply_line {x y} {
 	global current_diff_path current_diff_header current_diff_side
-	global ui_diff ui_index file_states
+	global ui_diff ui_index file_states repo_config
 
 	if {$current_diff_path eq {} || $current_diff_header eq {}} return
 	if {![lock_index apply_hunk]} return
 
 	set apply_cmd {apply --cached --whitespace=nowarn}
+	if {$repo_config(gui.diffcontext) eq 0} {
+		lappend apply_cmd --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 line."]
-- 
1.6.0

Re: [PATCH] git gui: use apply --unidiff-zero when staging hunks without context

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> echo a > victim
> git add victim
> echo b >> victim
> git diff -U0 | git apply --cached --unidiff-zero
> git diff

I think "diff -U0" there would say "@@ -1,0 +2 @@", iow "add this one line
after the first line", and "apply" has an off-by-one in this case.

[PATCH] git apply: do not match beginning in special unidiff-zero case

[Clemens Buchacher]

If a unidiff patch without any context modifies the second line, it does
not match the beginning, even though oldpos == 1.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Sat, Aug 30, 2008 at 10:53:15AM -0700, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> 
> > echo a > victim
> > git add victim
> > echo b >> victim
> > git diff -U0 | git apply --cached --unidiff-zero
> > git diff
> 
> I think "diff -U0" there would say "@@ -1,0 +2 @@", iow "add this one line
> after the first line", and "apply" has an off-by-one in this case.

Indeed. This appears to fix problems with staging hunks in git gui, even
with zero context lines. Staging individual lines still doesn't work,
though.

Clemens

 builtin-apply.c           |    9 ++++-----
 t/t4104-apply-boundary.sh |   15 ++++++++++++---
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 2216a0b..8402f9d 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1994,16 +1994,15 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,
 	trailing = frag->trailing;
 
 	/*
-	 * A hunk to change lines at the beginning would begin with
+	 * Unless the patch was generated with unidiff without any context, a
+	 * hunk to change lines at the beginning would begin with
 	 * @@ -1,L +N,M @@
 	 *
 	 * And a hunk to add to an empty file would begin with
 	 * @@ -0,0 +N,M @@
-	 *
-	 * In other words, a hunk that is (frag->oldpos <= 1) with or
-	 * without leading context must match at the beginning.
 	 */
-	match_beginning = frag->oldpos <= 1;
+	match_beginning = frag->oldpos == 0 ||
+		(!unidiff_zero && frag->oldpos == 1);
 
 	/*
 	 * A hunk without trailing lines must match at the end.
diff --git a/t/t4104-apply-boundary.sh b/t/t4104-apply-boundary.sh
index e7e2913..0e3ce36 100755
--- a/t/t4104-apply-boundary.sh
+++ b/t/t4104-apply-boundary.sh
@@ -27,6 +27,15 @@ test_expect_success setup '
 	git diff victim >add-a-patch.with &&
 	git diff --unified=0 >add-a-patch.without &&
 
+	: insert at line two
+	for i in b a '"$L"' y
+	do
+		echo $i
+	done >victim &&
+	cat victim >insert-a-expect &&
+	git diff victim >insert-a-patch.with &&
+	git diff --unified=0 >insert-a-patch.without &&
+
 	: modify at the head
 	for i in a '"$L"' y
 	do
@@ -55,7 +64,7 @@ test_expect_success setup '
 	git diff --unified=0 >add-z-patch.without &&
 
 	: modify at the tail
-	for i in a '"$L"' y
+	for i in b '"$L"' z
 	do
 		echo $i
 	done >victim &&
@@ -81,7 +90,7 @@ do
 	with) u= ;;
 	without) u='--unidiff-zero ' ;;
 	esac
-	for kind in add-a add-z mod-a mod-z del-a del-z
+	for kind in add-a add-z insert-a mod-a mod-z del-a del-z
 	do
 		test_expect_success "apply $kind-patch $with context" '
 			cat original >victim &&
@@ -95,7 +104,7 @@ do
 	done
 done
 
-for kind in add-a add-z mod-a mod-z del-a del-z
+for kind in add-a add-z insert-a mod-a mod-z del-a del-z
 do
 	rm -f $kind-ng.without
 	sed	-e "s/^diff --git /diff /" \
-- 
1.6.0

Re: [PATCH] git apply: do not match beginning in special unidiff-zero case

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> If a unidiff patch without any context modifies the second line, it does
> not match the beginning, even though oldpos == 1.

Heh, it seems we independently reached the same patch, so I'll squash in
your new test.

Having did all this, I have to warn you, git-gui, and anybody who is
tempted to use --unidiff-zero in "add -i [e]dit" that the -U0 format is
inherently unreliable _unless the patch you are feeding is perfect_.

By reducing the context, especially when you have only addition without
deletion next to it (iow, "modification to an existing line"), the only
information as to where that line needs to be inserted that you are
telling "apply" is the postimage offset, so you have to be absolutely sure
that the hunk header "@@ -K,L +M,N @@" you are feeding has the correct
value for "M".

For git-gui's "pick and apply", you should be able to, but once you start
allowing random manual editing of hunks, by definition the patch is _not_
perfect ("add -i [e]dit" even admits that it does not know what it is
doing by passing --recount to "apply").

So try to avoid --unidiff-zero.  git-gui should have _enough_ information
to add surrounding context back to the lines the user picked from the UI,
and I do not see a reason other than sheer lazyness for it to be using the
option.

[PATCH] git-apply: Loosen "match_beginning" logic

[Junio C Hamano]

Even after a handfle attempts, match_beginning logic still has corner
cases:

    1bf1a85 (apply: treat EOF as proper context., 2006-05-23)
    65aadb9 (apply: force matching at the beginning., 2006-05-24)
    4be6096 (apply --unidiff-zero: loosen sanity checks ..., 2006-09-17)
    ee5a317 (Fix "git apply" to correctly enforce "match ..., 2008-04-06)

This is a tricky piece of code.

We still incorrectly enforce "match_beginning" for -U0 matches.
I noticed this while trying out an example sequence from Clemens Buchacher:

    $ echo a >victim
    $ git add victim
    $ echo b >>victim
    $ git diff -U0 >patch
    $ cat patch
    diff --git i/victim w/victim
    index 7898192..422c2b7 100644
    --- i/victim
    +++ w/victim
    @@ -1,0 +2 @@ a
    +b
    $ git apply --cached --unidiff-zero <patch
    $ git show :victim
    b
    a

The change inserts a new line before the second line, but we insist it to
be applied at the beginning.  As the result, the code refuses to apply it
at the original offset, and we end up adding the line at the beginning.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-apply.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git c/builtin-apply.c w/builtin-apply.c
index 2216a0b..47261e1 100644
--- c/builtin-apply.c
+++ w/builtin-apply.c
@@ -1996,6 +1996,8 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,
 	/*
 	 * A hunk to change lines at the beginning would begin with
 	 * @@ -1,L +N,M @@
+	 * but we need to be careful.  -U0 that inserts before the second
+	 * line also has this pattern.
 	 *
 	 * And a hunk to add to an empty file would begin with
 	 * @@ -0,0 +N,M @@
@@ -2003,7 +2005,8 @@ static int apply_one_fragment(struct image *img, struct fragment *frag,
 	 * In other words, a hunk that is (frag->oldpos <= 1) with or
 	 * without leading context must match at the beginning.
 	 */
-	match_beginning = frag->oldpos <= 1;
+	match_beginning = (!frag->oldpos ||
+			   (frag->oldpos == 1 && !unidiff_zero));
 
 	/*
 	 * A hunk without trailing lines must match at the end.

Re: [PATCH] git gui: use apply --unidiff-zero when staging hunks without context

[Johannes Sixt]

Clemens Buchacher schrieb:
> git apply does not work correctly with zero-context patches. It does a
> little better with --unidiff-zero.

No, NO, NOOOOO! This kills your data!

http://thread.gmane.org/gmane.comp.version-control.git/67854/focus=68127

-- Hannes

Re: [PATCH] git gui: use apply --unidiff-zero when staging hunks without context

[Clemens Buchacher]

On Sat, Aug 30, 2008 at 09:43:19PM +0200, Johannes Sixt wrote:
> Clemens Buchacher schrieb:
>> git apply does not work correctly with zero-context patches. It does a
>> little better with --unidiff-zero.
>
> No, NO, NOOOOO! This kills your data!

Okay. Since we have 'Stage Line for Commit', supporting this would be almost
pointless anyways. So let's forget about trying to fix this and simply
disable zero-context diff in git-gui, as per my original patch

[PATCH] git gui: show diffs with a minimum of 1 context line

Clemens

Re: [PATCH] git gui: use apply --unidiff-zero when staging hunks without context

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> On Sat, Aug 30, 2008 at 09:43:19PM +0200, Johannes Sixt wrote:
>> Clemens Buchacher schrieb:
>>> git apply does not work correctly with zero-context patches. It does a
>>> little better with --unidiff-zero.
>>
>> No, NO, NOOOOO! This kills your data!
>
> Okay. Since we have 'Stage Line for Commit', supporting this would be almost
> pointless anyways. So let's forget about trying to fix this and simply
> disable zero-context diff in git-gui, as per my original patch
>
> [PATCH] git gui: show diffs with a minimum of 1 context line

Well, showing is Ok as long as you do not try pick and apply.  Or did I
miss something?

Re: [PATCH] git gui: use apply --unidiff-zero when staging hunks without context

[Clemens Buchacher]

On Sat, Aug 30, 2008 at 01:52:23PM -0700, Junio C Hamano wrote:
> Well, showing is Ok as long as you do not try pick and apply.  Or did I
> miss something?

If I understand correctly git gui bases its patches on what it shows in the
diff window - which makes sense, because otherwise it would be a PITA to
find out which hunk the user actually wanted.

We could allow diffs without context and disable staging in that case, but I
suspect this would only confuse the user.

Re: [PATCH] git gui: use apply --unidiff-zero when staging hunks without context

[Shawn O. Pearce]

Clemens Buchacher <drizzd@aon.at> wrote:
> On Sat, Aug 30, 2008 at 01:52:23PM -0700, Junio C Hamano wrote:
> > Well, showing is Ok as long as you do not try pick and apply.  Or did I
> > miss something?
> 
> If I understand correctly git gui bases its patches on what it shows in the
> diff window - which makes sense, because otherwise it would be a PITA to
> find out which hunk the user actually wanted.
> 
> We could allow diffs without context and disable staging in that case, but I
> suspect this would only confuse the user.

I agree.  Lets just disallow zero-context diffs.  Its far easier
for the user to understand the diff limit is 1.

-- 
Shawn.

Re: [PATCH] git gui: show diffs with a minimum of 1 context line

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> git apply does not handle diffs without context correctly.

NAK on this part of the proposed commit log message.

"git-apply" is more anal than other "patch" implementations in that it
tries to make sure that a hunk that touches the trailing end actually
applies to the trailing end of the file.  If a patch is generated with
non-zero context, we can detect by presense of trailing context lines that
a patch is _not_ about modifying the trailing end, but with a -U0 patch,
every hunk will come without trailing context, so you need to disable that
safety by asking for --unidiff-zero option.

> ... Configuring git
> gui to show zero context lines therefore breaks staging.

So another option might be to pass --unidiff-zero iff/when it is feeding
such a patch to fix this particular "user error" of "git-apply" program.

Having said that,

> In reply to this patch I will send a first attempt at fixing this problem
> instead of avoiding it.

I suspect there are some things "git-apply" should be able to _figure out_
that the user is giving it a -U0 patch and automatically flip unidiff_zero
option on.  For example, if the _first_ hunk of a patch does not begin
with "@@ -0,0 +N,M @@" nor with "@@ -1,L +N,M @@" (i.e. the hunk claims to
apply not at the beginning) and the hunk does not have leading context
lines, _and_ if that first hunk does not have trailing context lines, then
it is clearly a -U0 patch (or it could be a corrupt patch, but let's
discount that possibility for now).

Even if the hunk does claim to apply at the beginning, in which case we
cannot determine if it is a -U0 patch by looking at the lack of leading
context, if it has any context lines, we can tell it is _not_ a -U0 patch.
When the first hunk that applies to the beginning lacks any context, we
cannot really tell if it is -U0 or not (the other possibility is a total
rewrite of the file from the beginning to the end).  Even in that case,
you could look at the next hunk --- if you have a hunk that applies to
the same path after looking at such a "first" hunk without context, then
it clearly is a -U0 patch.

Re: [PATCH] git gui: show diffs with a minimum of 1 context line

[Junio C Hamano]

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

> I suspect there are some things "git-apply" should be able to _figure out_

s/able to/& do to/;

> that the user is giving it a -U0 patch and automatically flip unidiff_zero
> option on.  For example, if the _first_ hunk of a patch does not begin
> with "@@ -0,0 +N,M @@" nor with "@@ -1,L +N,M @@" (i.e. the hunk claims to
> apply not at the beginning) and the hunk does not have leading context
> lines, _and_ if that first hunk does not have trailing context lines, then

Eh, what was I smoking.  In short, if the first hunk does not have _any_
context lines and still says it is intended to apply to somewhere not at
the beginning, then that is a -U0 patch.

[PATCH] git gui: show diffs with a minimum of 1 context line

[Clemens Buchacher]

Staging hunks without context does not work, because line number
information would have to be recomputed for individual hunks.  Since it is
already possible to stage individual lines using 'Stage Line for Commit',
zero context diffs are not really necessary for git gui, however.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

Same patch, different commit message.

 git-gui/git-gui.sh     |    2 +-
 git-gui/lib/diff.tcl   |    2 +-
 git-gui/lib/option.tcl |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index ad65aaa..86402d4 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -1932,7 +1932,7 @@ proc show_more_context {} {
 
 proc show_less_context {} {
 	global repo_config
-	if {$repo_config(gui.diffcontext) >= 1} {
+	if {$repo_config(gui.diffcontext) > 1} {
 		incr repo_config(gui.diffcontext) -1
 		reshow_diff
 	}
diff --git a/git-gui/lib/diff.tcl b/git-gui/lib/diff.tcl
index 52b79e4..4a7138b 100644
--- a/git-gui/lib/diff.tcl
+++ b/git-gui/lib/diff.tcl
@@ -175,7 +175,7 @@ proc show_diff {path w {lno {}} {scroll_pos {}}} {
 
 	lappend cmd -p
 	lappend cmd --no-color
-	if {$repo_config(gui.diffcontext) >= 0} {
+	if {$repo_config(gui.diffcontext) >= 1} {
 		lappend cmd "-U$repo_config(gui.diffcontext)"
 	}
 	if {$w eq $ui_index} {
diff --git a/git-gui/lib/option.tcl b/git-gui/lib/option.tcl
index ffb3f00..5e1346e 100644
--- a/git-gui/lib/option.tcl
+++ b/git-gui/lib/option.tcl
@@ -125,7 +125,7 @@ proc do_options {} {
 		{b gui.matchtrackingbranch {mc "Match Tracking Branches"}}
 		{b gui.fastcopyblame {mc "Blame Copy Only On Changed Files"}}
 		{i-20..200 gui.copyblamethreshold {mc "Minimum Letters To Blame Copy On"}}
-		{i-0..99 gui.diffcontext {mc "Number of Diff Context Lines"}}
+		{i-1..99 gui.diffcontext {mc "Number of Diff Context Lines"}}
 		{i-0..99 gui.commitmsgwidth {mc "Commit Message Text Width"}}
 		{t gui.newbranchtemplate {mc "New Branch Name Template"}}
 		} {
-- 
1.6.0

Re: [PATCH] git gui: show diffs with a minimum of 1 context line

[Shawn O. Pearce]

Clemens Buchacher <drizzd@aon.at> wrote:
> git apply does not handle diffs without context correctly. Configuring git
> gui to show zero context lines therefore breaks staging.
> 
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>

Thanks, this is queued for 'maint'.
 
-- 
Shawn.

Re: [PATCH] git gui: show diffs with a minimum of 1 context line

[Clemens Buchacher]

On Mon, Sep 01, 2008 at 12:33:59PM -0700, Shawn O. Pearce wrote:
> Clemens Buchacher <drizzd@aon.at> wrote:
> > git apply does not handle diffs without context correctly. Configuring git
> > gui to show zero context lines therefore breaks staging.
> > 
> > Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> 
> Thanks, this is queued for 'maint'.

Actually, if you don't mind, I changed the commit message because 'git
apply' is not really to blame here:

Staging hunks without context does not work, because line number
information would have to be recomputed for individual hunks.  Since it is
already possible to stage individual lines using 'Stage Line for Commit',
zero context diffs are not really necessary for git gui, however.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>

Re: [PATCH] git gui: show diffs with a minimum of 1 context line

[Shawn O. Pearce]

Clemens Buchacher <drizzd@aon.at> wrote:
> Actually, if you don't mind, I changed the commit message because 'git
> apply' is not really to blame here:
> 
> Staging hunks without context does not work, because line number
> information would have to be recomputed for individual hunks.  Since it is
> already possible to stage individual lines using 'Stage Line for Commit',
> zero context diffs are not really necessary for git gui, however.

Thanks, message updated.

-- 
Shawn.