Re: Git Gui does not want to work on chunk level

Re: Git Gui does not want to work on chunk level

[Alexander Gladysh]

Hi, list!

On Wed, Aug 27, 2008 at 1:16 PM, Alexander Gladysh <agladysh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> $ git --version
> git version 1.6.0.GIT
>
> $ git gui --version
> git-gui version 0.11.GITGUI
>
> I like 'stage hunk for commit' feature in Git Gui, but often it gives
> me an error message box:
>
> fatal: git-apply: bad git-diff - inconsistent old filename on line 4

Please advise how should I formulate this question to get an answer. I
understand that it is hard to say something on the subject with that
little info I provide, but anyway. I'm encountering this problem on
roughly every fifth commit, and it is quite annoying, since I've got
used to small feature-based per-chunk commits.

Does anyone else encounter this kind of error? Any debugging advice?
Perhaps I can instrument git gui with logging somewhere (dump diffs
for example). (I have no experience with TCL though.)

If error appears for single file diff, it would be reproduced each
time I try to stage hunk from that diff. I have one such uncommitted
diff now (can't send it, sorry), please advise what can I do to debug
it.

Alexander.

Re: Git Gui does not want to work on chunk level

[Shawn O. Pearce]

Alexander Gladysh <agladysh@gmail.com> wrote:
> On Wed, Aug 27, 2008 at 1:16 PM, Alexander Gladysh <agladysh@gmail.com> wrote:
> > $ git --version
> > git version 1.6.0.GIT
> >
> > $ git gui --version
> > git-gui version 0.11.GITGUI
> >
> > I like 'stage hunk for commit' feature in Git Gui, but often it gives
> > me an error message box:
> >
> > fatal: git-apply: bad git-diff - inconsistent old filename on line 4
...
> Does anyone else encounter this kind of error? Any debugging advice?
> Perhaps I can instrument git gui with logging somewhere (dump diffs
> for example). (I have no experience with TCL though.)

I've never seen that before, but its clearly dumping a bad diff
into git-apply.

Open up lib/diff.tcl and find "proc apply_hunk".  Inside of the
catch maybe insert a puts to see the diff, e.g.:

diff --git a/git-gui/lib/diff.tcl b/git-gui/lib/diff.tcl
index 52b79e4..4acaccf 100644
--- a/git-gui/lib/diff.tcl
+++ b/git-gui/lib/diff.tcl
@@ -341,6 +341,8 @@ proc apply_hunk {x y} {
 		fconfigure $p -translation binary -encoding binary
 		puts -nonewline $p $current_diff_header
 		puts -nonewline $p [$ui_diff get $s_lno $e_lno]
+puts -nonewline stdout $current_diff_header
+puts -nonewline stdout [$ui_diff get $s_lno $e_lno]
 		close $p} err]} {
 		error_popup [append $failed_msg "\n\n$err"]
 		unlock_index

Then if you can scrub the context and +/- lines for a bad diff and
send us at least the headers, or tell us how they are malformed,
it would help to debug it.
 
-- 
Shawn.

Re: Git Gui does not want to work on chunk level

[Alexander Gladysh]

On Thu, Sep 4, 2008 at 3:08 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> Alexander Gladysh <agladysh@gmail.com> wrote:
>> On Wed, Aug 27, 2008 at 1:16 PM, Alexander Gladysh <agladysh@gmail.com> wrote:
>> > I like 'stage hunk for commit' feature in Git Gui, but often it gives
>> > me an error message box:
>> >
>> > fatal: git-apply: bad git-diff - inconsistent old filename on line 4
> ...
> I've never seen that before, but its clearly dumping a bad diff
> into git-apply.
>
> Open up lib/diff.tcl and find "proc apply_hunk".  Inside of the
> catch maybe insert a puts to see the diff, e.g.:
> <...>
> Then if you can scrub the context and +/- lines for a bad diff and
> send us at least the headers, or tell us how they are malformed,
> it would help to debug it.

Thank you!

Header looks well-formed to me:

diff --git a/path/file.ext b/path/file.ext
--- a/path/file.ext
+++ b/path/file.ext

I have sent you full output privately. I do not see anything unusual
in it... Anyplace where I may put more logging?

Thanks again,
Alexander.

Re: Git Gui does not want to work on chunk level

[Shawn O. Pearce]

Alexander Gladysh <agladysh@gmail.com> wrote:
> On Thu, Sep 4, 2008 at 3:08 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> >
> > Open up lib/diff.tcl and find "proc apply_hunk".  Inside of the
> > catch maybe insert a puts to see the diff, e.g.:
> > <...>
> 
> Header looks well-formed to me:
> 
> diff --git a/path/file.ext b/path/file.ext
> --- a/path/file.ext
> +++ b/path/file.ext
> 
> I have sent you full output privately. I do not see anything unusual
> in it... Anyplace where I may put more logging?

Actually the header you sent me privately wasn't well formed.
It looked something like this:

  diff --git a/path/file.ext b/path/file.ext
  --- a/path/file.ext
  +++ b/path/file.ext
  --- assert(a != b)
  --- assert(a != c)
  @@ -10,1 +10,2 @@

It looks to me like some part of the context became part of the
diff header, with three "-" stuck in front of it.  Which git-apply
then thinks is the pre-image path name.

Can you put in some more puts in that same section of the code
so we can find out which of the puts has these extra asserts?
Is it the first one for $current_diff_header or is it the second
one that is supposed to be pulling in the patch hunk?  Do you see
these extra asserts in the diff viewer like this before you try to
apply the patch?

-- 
Shawn.

Re: Git Gui does not want to work on chunk level

[Alexander Gladysh]

(Shawn, apologies for double-posting.)

> Actually the header you sent me privately wasn't well formed.
> It looked something like this:
>
>  diff --git a/path/file.ext b/path/file.ext
>  --- a/path/file.ext
>  +++ b/path/file.ext
>  --- assert(a != b)
>  --- assert(a != c)
>  @@ -10,1 +10,2 @@
>
> It looks to me like some part of the context became part of the
> diff header, with three "-" stuck in front of it.  Which git-apply
> then thinks is the pre-image path name.

Aha! Thank you! That "--" is a comment in Lua (that is commented line
that was removed in diff). First dash is from diff, next two came from
file itself.

Steps to reproduce the bug:

$ mkdir test
$ cd test
$ git init
Initialized empty Git repository in path/test/.git/
$ cat >file.ext
-- 1
-- 2
-- 3
-- 4
-- 5
^D
$ git add file.ext
$ git commit -m "one"
Created initial commit 9226cdf: one
 1 files changed, 5 insertions(+), 0 deletions(-)
 create mode 100644 file.ext
$ cat >file.ext
-- 1
-- 2
-- 33
-- 4
-- 5
$ git gui

And stage (sole) hunk. You see the error.

> Can you put in some more puts in that same section of the code
> so we can find out which of the puts has these extra asserts?
> Is it the first one for $current_diff_header or is it the second
> one that is supposed to be pulling in the patch hunk?  Do you see
> these extra asserts in the diff viewer like this before you try to
> apply the patch?

Hopefully the above steps would help to debug this bug directly. If
you still need any more info from me, please say, I'll be glad to
provide it.

Thank you again!
Alexander.

Re: Git Gui does not want to work on chunk level

[Shawn O. Pearce]

Alexander Gladysh <agladysh@gmail.com> wrote:
> 
> Aha! Thank you! That "--" is a comment in Lua (that is commented line
> that was removed in diff). First dash is from diff, next two came from
> file itself.
> 
> Steps to reproduce the bug:
...

So that was an awesome reproduction case.  I have committed the
following fix and will push it out in a few minutes:

--8<--
git-gui: Fix diff parsing for lines starting with "--" or "++"

Languages like Lua and SQL use "--" to mark a line as commented out.
If this appears at column 0 and is part of the pre-image we may see
"--- foo" in the diff, indicating that the line whose content is
 "-- foo" has been removed from the new version.

git-gui was incorrectly parsing "--- foo" as the old file name
in the file header, causing it to generate a bad patch file when
the user tried to stage or unstage a hunk or the selected line.
We need to keep track of where we are in the parsing so that we do
not misread a deletion or addition record as part of the header.

Reported-by: Alexander Gladysh <agladysh@gmail.com>
Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 lib/diff.tcl |   18 +++++++++++-------
 1 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/lib/diff.tcl b/lib/diff.tcl
index 4a7138b..1970b60 100644
--- a/lib/diff.tcl
+++ b/lib/diff.tcl
@@ -192,6 +192,7 @@ proc show_diff {path w {lno {}} {scroll_pos {}}} {
 		return
 	}
 
+	set ::current_diff_inheader 1
 	fconfigure $fd \
 		-blocking 0 \
 		-encoding binary \
@@ -207,18 +208,21 @@ proc read_diff {fd scroll_pos} {
 	while {[gets $fd line] >= 0} {
 		# -- Cleanup uninteresting diff header lines.
 		#
-		if {   [string match {diff --git *}      $line]
-			|| [string match {diff --cc *}       $line]
-			|| [string match {diff --combined *} $line]
-			|| [string match {--- *}             $line]
-			|| [string match {+++ *}             $line]} {
-			append current_diff_header $line "\n"
-			continue
+		if {$::current_diff_inheader} {
+			if {   [string match {diff --git *}      $line]
+			    || [string match {diff --cc *}       $line]
+			    || [string match {diff --combined *} $line]
+			    || [string match {--- *}             $line]
+			    || [string match {+++ *}             $line]} {
+				append current_diff_header $line "\n"
+				continue
+			}
 		}
 		if {[string match {index *} $line]} continue
 		if {$line eq {deleted file mode 120000}} {
 			set line "deleted symlink"
 		}
+		set ::current_diff_inheader 0
 
 		# -- Automatically detect if this is a 3 way diff.
 		#
-- 
1.6.0.1.274.g007e4


-- 
Shawn.

Re: Git Gui does not want to work on chunk level

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> git-gui was incorrectly parsing "--- foo" as the old file name
> in the file header, causing it to generate a bad patch file when
> the user tried to stage or unstage a hunk or the selected line.
> We need to keep track of where we are in the parsing so that we do
> not misread a deletion or addition record as part of the header.

While keeping track of state is obviously a good change, it also might be
worth noticing "@@ -k,l +m,n @@" header and actively using the
information.

Re: Git Gui does not want to work on chunk level

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > git-gui was incorrectly parsing "--- foo" as the old file name
> > in the file header, causing it to generate a bad patch file when
> > the user tried to stage or unstage a hunk or the selected line.
> > We need to keep track of where we are in the parsing so that we do
> > not misread a deletion or addition record as part of the header.
> 
> While keeping track of state is obviously a good change, it also might be
> worth noticing "@@ -k,l +m,n @@" header and actively using the
> information.

Yea.  But that was a lot more work.

Noticing a hunk header, getting the line counts from it, then
keeping a running counter to know how many lines before either EOF
or the next header are all part of that hunk is just a lot of code
I didn't feel like writing this evening.  ;-)

-- 
Shawn.

Re: Git Gui does not want to work on chunk level

[Alexander Gladysh]

> So that was an awesome reproduction case.  I have committed the
> following fix and will push it out in a few minutes:

<...>

Thank you! The fix helped indeed. I can commit on hunk level again! :-)

Alexander.

Re: Git Gui does not want to work on chunk level

[Johannes Sixt]

Shawn O. Pearce schrieb:
> git-gui: Fix diff parsing for lines starting with "--" or "++"
> 
> Languages like Lua and SQL use "--" to mark a line as commented out.
> If this appears at column 0 and is part of the pre-image we may see
> "--- foo" in the diff, indicating that the line whose content is
>  "-- foo" has been removed from the new version.
> 
> git-gui was incorrectly parsing "--- foo" as the old file name
> in the file header, causing it to generate a bad patch file when
> the user tried to stage or unstage a hunk or the selected line.
> We need to keep track of where we are in the parsing so that we do
> not misread a deletion or addition record as part of the header.

This (slightly) breaks parsing of new files and removed files that are
staged by showing the file names in the diff header:

 new file mode 100644
 --- /dev/null
 +++ b/foo
 @@ -0,0 +1 @@
 +foo

-- Hannes

Re: Git Gui does not want to work on chunk level

[Alexander Gladysh]

Hi, list!

On Fri, Sep 5, 2008 at 8:53 AM, Shawn O. Pearce <spearce@spearce.org> wrote:

> git-gui: Fix diff parsing for lines starting with "--" or "++"

Somewhat related bug, but reproduceable event without "--". If I set
diff context to none in git gui (by repeatedly clicking "show less
context") I'm sometimes not able to stage hunk or line.

git version 1.6.0.GIT
git-gui version 0.11.GITGUI

Steps to reproduce:

agladysh@agladysh-desktop:~$ mkdir test
agladysh@agladysh-desktop:~$ cd test
agladysh@agladysh-desktop:~/test$ git init
Initialized empty Git repository in /home/agladysh/test/.git/
agladysh@agladysh-desktop:~/test$ cat >file.ext
-- 1
-- 2
-- 3
-- 4
-- 5
agladysh@agladysh-desktop:~/test$ git add file.ext
agladysh@agladysh-desktop:~/test$ git commit -m "one"
Created initial commit aa4da16: one
 1 files changed, 5 insertions(+), 0 deletions(-)
 create mode 100644 file.ext
agladysh@agladysh-desktop:~/test$ cat >file.ext
-- 1
-- 2
-- 33
-- 4
-- 5
agladysh@agladysh-desktop:~/test$ git gui

Reduce context to none, then do stage hunk. It fails with message:

error: patch failed: file.ext:3
error: file.ext: patch does not apply
OK

Dump (by your debuging dumper patch from the same tread):

diff --git a/file.ext b/file.ext
--- a/file.ext
+++ b/file.ext
@@ -1,5 +1,5 @@
 -- 1
 -- 2
--- 3
+-- 33
 -- 4
 -- 5

Do stage line. It fails with message:

fatal: corrupt patch at line 5

Dump:

diff --git a/file.ext b/file.ext
--- a/file.ext
+++ b/file.ext
@@ -3 +3 @@
--- 3
+-- 33

HTH,
Alexander.

Re: Git Gui does not want to work on chunk level

[Shawn O. Pearce]

Alexander Gladysh <agladysh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Sep 5, 2008 at 8:53 AM, Shawn O. Pearce <spearce-o8xcbExO1WpAfugRpC6u6w@public.gmane.org> wrote:
> 
> > git-gui: Fix diff parsing for lines starting with "--" or "++"
> 
> Somewhat related bug, but reproduceable event without "--". If I set
> diff context to none in git gui (by repeatedly clicking "show less
> context") I'm sometimes not able to stage hunk or line.

git-apply refuses to apply a patch with 0 context lines.  Latest
git-gui in maint (and in Git 1.6.0.2?) won't let you reduce the
number of context lines below 1, so you never can see this error.
 
> Reduce context to none, then do stage hunk. It fails with message:
> 
> error: patch failed: file.ext:3
> error: file.ext: patch does not apply
> OK

-- 
Shawn.

Re: Git Gui does not want to work on chunk level

[Bert Wesarg]

On Mon, Sep 15, 2008 at 09:08, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Shawn O. Pearce schrieb:
>> git-gui: Fix diff parsing for lines starting with "--" or "++"
>>
>> Languages like Lua and SQL use "--" to mark a line as commented out.
>> If this appears at column 0 and is part of the pre-image we may see
>> "--- foo" in the diff, indicating that the line whose content is
>>  "-- foo" has been removed from the new version.
>>
>> git-gui was incorrectly parsing "--- foo" as the old file name
>> in the file header, causing it to generate a bad patch file when
>> the user tried to stage or unstage a hunk or the selected line.
>> We need to keep track of where we are in the parsing so that we do
>> not misread a deletion or addition record as part of the header.
>
> This (slightly) breaks parsing of new files and removed files that are
> staged by showing the file names in the diff header:
>
>  new file mode 100644
>  --- /dev/null
>  +++ b/foo
>  @@ -0,0 +1 @@
>  +foo

I just noticed the same, and ask me why this report wasn't honored.

Also git may spill out 2 diffs for type changes (file to symlink, for
example). In this case the we should probably disable any hunk- or
line-level (un)staging.

Bert

>
> -- Hannes

Re: Git Gui does not want to work on chunk level

[Bert Wesarg]

On Tue, Nov 30, 2010 at 22:19, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> On Mon, Sep 15, 2008 at 09:08, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>  new file mode 100644
>>  --- /dev/null
>>  +++ b/foo
>>  @@ -0,0 +1 @@
>>  +foo
>
> I just noticed the same, and ask me why this report wasn't honored.
>
> Also git may spill out 2 diffs for type changes (file to symlink, for
> example). In this case the we should probably disable any hunk- or
> line-level (un)staging.

Here is a proposal which fixes this case. Not tested on the OPs report.

--8<--
diff --git a/lib/diff.tcl b/lib/diff.tcl
index 8fea947..b3901c3 100644
--- a/lib/diff.tcl
+++ b/lib/diff.tcl
@@ -398,7 +398,6 @@ proc read_diff {fd conflict_size cont_info} {
 		if {$line eq {deleted file mode 120000}} {
 			set line "deleted symlink"
 		}
-		set ::current_diff_inheader 0

 		# -- Automatically detect if this is a 3 way diff.
 		#
@@ -414,6 +413,7 @@ proc read_diff {fd conflict_size cont_info} {
 			|| [regexp {^\* Unmerged path } $line]} {
 			set tags {}
 		} elseif {$is_3way_diff} {
+			set ::current_diff_inheader 0
 			set op [string range $line 0 1]
 			switch -- $op {
 			{  } {set tags {}}
@@ -440,6 +440,7 @@ proc read_diff {fd conflict_size cont_info} {
 			}
 			}
 		} elseif {$is_submodule_diff} {
+			set ::current_diff_inheader 0
 			if {$line == ""} continue
 			if {[regexp {^Submodule } $line]} {
 				set tags d_@
@@ -459,6 +460,7 @@ proc read_diff {fd conflict_size cont_info} {
 				}
 			}
 		} else {
+			set ::current_diff_inheader 0
 			set op [string index $line 0]
 			switch -- $op {
 			{ } {set tags {}}

Re: Git Gui does not want to work on chunk level

[Bert Wesarg]

On Tue, Nov 30, 2010 at 23:20, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> On Tue, Nov 30, 2010 at 22:19, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>> On Mon, Sep 15, 2008 at 09:08, Johannes Sixt <j.sixt@viscovery.net> wrote:
>>>  new file mode 100644
>>>  --- /dev/null
>>>  +++ b/foo
>>>  @@ -0,0 +1 @@
>>>  +foo
>>
>> I just noticed the same, and ask me why this report wasn't honored.
>>
>> Also git may spill out 2 diffs for type changes (file to symlink, for
>> example). In this case the we should probably disable any hunk- or
>> line-level (un)staging.
>
> Here is a proposal which fixes this case. Not tested on the OPs report.
>
> --8<--
> diff --git a/lib/diff.tcl b/lib/diff.tcl
> index 8fea947..b3901c3 100644
> --- a/lib/diff.tcl
> +++ b/lib/diff.tcl
> @@ -398,7 +398,6 @@ proc read_diff {fd conflict_size cont_info} {
>                if {$line eq {deleted file mode 120000}} {
>                        set line "deleted symlink"
>                }
> -               set ::current_diff_inheader 0
>
>                # -- Automatically detect if this is a 3 way diff.
>                #
> @@ -414,6 +413,7 @@ proc read_diff {fd conflict_size cont_info} {
>                        || [regexp {^\* Unmerged path } $line]} {
>                        set tags {}
>                } elseif {$is_3way_diff} {
> +                       set ::current_diff_inheader 0
>                        set op [string range $line 0 1]
>                        switch -- $op {
>                        {  } {set tags {}}
> @@ -440,6 +440,7 @@ proc read_diff {fd conflict_size cont_info} {
>                        }
>                        }
>                } elseif {$is_submodule_diff} {
> +                       set ::current_diff_inheader 0
>                        if {$line == ""} continue
>                        if {[regexp {^Submodule } $line]} {
>                                set tags d_@
> @@ -459,6 +460,7 @@ proc read_diff {fd conflict_size cont_info} {
>                                }
>                        }
>                } else {
> +                       set ::current_diff_inheader 0
>                        set op [string index $line 0]
>                        switch -- $op {
>                        { } {set tags {}}
>

Please ignore this proposal. I have a better solution in the pipe
(which solves both problems reported here).

Bert