threads / discuss / 15363

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

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

## tl;dr

15 messages between Sep 3, 2008 and Dec 6, 2010.

replies: 14people: 7as markdown or json

Alexander Gladysh· Sep 3, 2008, 19:23 UTC · lore
Hi, list!
On Wed, Aug 27, 2008 at 1:16 PM, Alexander Gladysh <agladysh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
Show 10 quoted lines
> $ 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.
Shawn O. Pearce· Sep 3, 2008, 23:08 UTC · re: Alexander Gladysh · lore
Alexander Gladysh <agladysh@gmail.com> wrote:
Show 11 quoted lines
> 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.
Alexander Gladysh· Sep 4, 2008, 09:20 UTC · re: Shawn O. Pearce · lore
On Thu, Sep 4, 2008 at 3:08 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
Show 16 quoted lines
> 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.
Shawn O. Pearce· Sep 4, 2008, 14:37 UTC · re: Alexander Gladysh · lore
Alexander Gladysh <agladysh@gmail.com> wrote:
Show 14 quoted lines
> 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.
Alexander Gladysh· Sep 4, 2008, 14:54 UTC · lore
(Shawn, apologies for double-posting.)
Show 13 quoted lines
> 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.
Show 6 quoted lines
> 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.

Shawn O. Pearce· Sep 5, 2008, 04:53 UTC · re: Alexander Gladysh · lore
Alexander Gladysh <agladysh@gmail.com> wrote:
Show 6 quoted lines
> 
> 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.
Junio C Hamano· Sep 5, 2008, 05:18 UTC · re: Shawn O. Pearce · lore
"Shawn O. Pearce" <spearce@spearce.org> writes:
Show 5 quoted lines
> 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.

Shawn O. Pearce· Sep 5, 2008, 05:20 UTC · re: Junio C Hamano · lore
Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> "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.
Johannes Sixt· Sep 15, 2008, 07:08 UTC · re: Shawn O. Pearce · lore
Shawn O. Pearce schrieb:
Show 12 quoted lines
> 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
Bert Wesarg· Nov 30, 2010, 21:19 UTC · re: Johannes Sixt · lore
On Mon, Sep 15, 2008 at 09:08, Johannes Sixt <j.sixt@viscovery.net> wrote:
Show 22 quoted lines
> 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
Bert Wesarg· Nov 30, 2010, 22:20 UTC · re: Bert Wesarg · lore
On Tue, Nov 30, 2010 at 22:19, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
Show 12 quoted lines
> 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 {}}
Bert Wesarg· Dec 6, 2010, 21:30 UTC · re: Bert Wesarg · lore
On Tue, Nov 30, 2010 at 23:20, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
Show 54 quoted lines
> 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
Alexander Gladysh· Sep 15, 2008, 14:21 UTC · re: Shawn O. Pearce · lore
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.
Alexander Gladysh· Sep 5, 2008, 19:27 UTC · lore
> 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.
Shawn O. Pearce· Sep 15, 2008, 14:24 UTC · lore
Alexander Gladysh <agladysh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
Show 7 quoted lines
> 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.
 
Show 5 quoted lines
> 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.

← back to recent threads