kompare won't parse git diffs

kompare won't parse git diffs

[Andy Parkins]

[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

Hello,

Kompare just shows blank for diffs redirected from git.  This makes it 

I've tracked down the cause and it seems to be on the file declaration lines.  
Here is a sample diff that kompare will not show.

diff --git a/file b/file
index a8b2ec6..a1e65fd 100644
--- a/file
+++ b/file
@@ -1,2 +1,3 @@
 initial contents goes in master branch
 additional content added in repo1#master
+blah

As you can see, it's a simple addition of the line "blah".  Kompare shows this 
diff as blank.  Now if I modify the file so that the "---" and "+++" lines 
both have "<tab>(something)" added:

diff --git a/file b/file
index a8b2ec6..a1e65fd 100644
--- a/file      (anything can go here)
+++ b/file      (anything can go here)
@@ -1,2 +1,3 @@
 initial contents goes in master branch
 additional content added in repo1#master
+blah

http://bugs.kde.org/show_bug.cgi?id=131717

I've posted a bug for kompare, but thought that maybe it would be handy if git 
outputted those bracketed comments anyway - I notice that "diff -u" from the 
command line does (although it uses the timestamp of the file).  Maybe the 
object hash of the files being compared could go here?


Andy

-- 
Dr Andy Parkins, M Eng (hons), MIEE
andyparkins@gmail.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: kompare won't parse git diffs

[Linus Torvalds]

On Wed, 2 Aug 2006, Andy Parkins wrote:
>
> Kompare just shows blank for diffs redirected from git. [ snip ]
> 
> As you can see, it's a simple addition of the line "blah".  Kompare shows this 
> diff as blank.  Now if I modify the file so that the "---" and "+++" lines 
> both have "<tab>(something)" added:

I'd definitely call this a pure kompare bug.

Not only is the git patch format perfectly standard and accepted by other 
tools, it's much better designed than the brain-damaged syntax that GNU 
patch uses (which adds a tab and a timestamp after the filenames). In 
particular, with git patches it is easy to get filenames that have spaces 
and tabs in them right. 

Now, if the kompare people can show that every single other patch 
generator adds the stupid tab + date format, I guess we could do it too, 
but
 (a) there is no valid date in general to use, so it's a fundamentally 
     broken notion and
 (b) I'm pretty sure that the kompare people only ever actually tested 
     with GNU patch or other very modern patches, because when I did the 
     patch apply logic (and designed the extended git format), I looked 
     around at things like "diffstat" that have been around a long time, 
     to see what they accept, and the whole thing is an unholy mess wrt 
     filenames.

The git format really is the best patch format I've seen by _far_, partly 
because it's designed to be totally unambiguous even in the presense of 
file renames (and new/deleted files, and file modes), but partly because 
it's also designed to be both extensible and detectable (ie the marker 
"diff --git " is there, so that you can _know_ and _depend_ on the git 
format, unlike, for example, the GNU patches that don't have a good fixed 
format).

I'm hoping that people will some day just wake up and notice that the git 
extended patches are really worth doing even for other projects. I was 
going to send in patches to GNU patch to try to make it at least 
understand them, but I got lazy.

		Linus

Re: kompare won't parse git diffs

[Jakub Narebski]

Linus Torvalds wrote:

> On Wed, 2 Aug 2006, Andy Parkins wrote:
>>
>> Kompare just shows blank for diffs redirected from git. [ snip ]
>> 
>> As you can see, it's a simple addition of the line "blah".  Kompare shows this 
>> diff as blank.  Now if I modify the file so that the "---" and "+++" lines 
>> both have "<tab>(something)" added:
> 
> I'd definitely call this a pure kompare bug.
> 
> Not only is the git patch format perfectly standard and accepted by other 
> tools, it's much better designed than the brain-damaged syntax that GNU 
> patch uses (which adds a tab and a timestamp after the filenames). In 
> particular, with git patches it is easy to get filenames that have spaces 
> and tabs in them right. 

What about filenames with end-of-line character in them? Is it quoted?

BTW. It should be not that hard to get filename with spaces and tabs even
in GNU diff format: everything up to last <tab> is filename.

> Now, if the kompare people can show that every single other patch 
> generator adds the stupid tab + date format, I guess we could do it too, 
> but
>  (a) there is no valid date in general to use, so it's a fundamentally 
>      broken notion and

Meaning we don't save timestamp in git ;-) Well, we could use date of the 
commit which created given file contents (first commit from root, or last
from head which contains given version)... but the same contents might be
introduced independently in different commits. And different clones of the
same repository might have different commit dates...


-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: kompare won't parse git diffs

[Andy Parkins]

[-- Attachment #1: Type: text/plain, Size: 2333 bytes --]

On Wednesday 2006, August 02 18:19, Linus Torvalds wrote:

> I'd definitely call this a pure kompare bug.

Me too, but it seemed such a small thing change to git that I thought I'd 
mention it.

> Not only is the git patch format perfectly standard and accepted by other
> tools, it's much better designed than the brain-damaged syntax that GNU
> patch uses (which adds a tab and a timestamp after the filenames). In
> particular, with git patches it is easy to get filenames that have spaces
> and tabs in them right.

The only three things I checked were diff, git and subversion.  git is the 
only one of the three that didn't include anything after the filenames.  
Subversion uses the space to write (working copy) and (revision XXX), so it's 
clear that these fields are simply being treated as commentary.  That only 
adds weight to the argument that it is a kompare bug, as surely <nil> is a 
perfectly acceptable comment?

> Now, if the kompare people can show that every single other patch
> generator adds the stupid tab + date format, I guess we could do it too,
> but

As it seems to be acceptable to put something other than a date, perhaps the 
hash of the object being compared would be more useful than the unavailable 
date?

>  (b) I'm pretty sure that the kompare people only ever actually tested
>      with GNU patch or other very modern patches, because when I did the

I'm sure that that is true.  I certainly don't think that there is a fight 
here, I've not seen any feedback on the kompare bug yet, but I'd be very 
surprised if the response is "git is stupid - WONTFIX".

> I'm hoping that people will some day just wake up and notice that the git
> extended patches are really worth doing even for other projects. I was

I think in the cases I've been testing, the extended format hasn't shown its 
face.  I used git-svnimport to duplicate an svn repository then copied the 
same changes into both an svn working directory and a git working directory.  
I then ran "git diff" and "svn diff", and diffed the two outputs.  Apart from 
some differences in how many lines where added and deleted (and the 
difference that started this thread of course) the two diffs were equivalent.


Andy

-- 
Dr Andrew Parkins, M Eng (Hons), AMIEE
andyparkins@gmail.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: kompare won't parse git diffs

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

>> Not only is the git patch format perfectly standard and accepted by other 
>> tools, it's much better designed than the brain-damaged syntax that GNU 
>> patch uses (which adds a tab and a timestamp after the filenames). In 
>> particular, with git patches it is easy to get filenames that have spaces 
>> and tabs in them right. 
>
> What about filenames with end-of-line character in them? Is it quoted?

You could have done a bit of homework yourself to find that out
easily ;-).  Anyway, the answer is yes.

	http://marc.theaimsgroup.com/?l=git&m=112927316408690

I wonder what happened to the plan to update GNU diff/patch to
also emit/understand the c-style quoted paths.  Paul?

Re: kompare won't parse git diffs

[Linus Torvalds]

On Wed, 2 Aug 2006, Jakub Narebski wrote:
> 
> What about filenames with end-of-line character in them? Is it quoted?

I would _not_ guarantee that git handles filenames with newlines in them 
correctly in all cases (I won't guarantee tabs either, but the likelihood 
is much higher ;). But yes, "git diff" does handle it right.

That said, yes, it should be quoted. A quoted filename in "git diff" not 
only changes a newline into the normal quoted sequence (ie "\n"), but will 
also put the whole filename in quotes.

So if you have a file called "hello", the diff headers will say

	--- a/hello
	+++ b/hello

but if you have a file with an embedded newline, it would look something 
like

	--- "a/hello\nthere"
	+++ "b/hello\nthere"

(where a '"' in a filename causes the same quoting to take effect, so if 
you haev embedded double-quotes in a filename, it would be shown as 
something like

	--- "a/embedded\"quote"
	+++ "b/embedded\"quote"

instead).

> BTW. It should be not that hard to get filename with spaces and tabs even
> in GNU diff format: everything up to last <tab> is filename.

That's true _if_ you know that the patch was generated by GNU diff, but 
since you can't know that (since GNU diff doesn't add any markers of its 
own), you cannot know if it's a patch generated by an old version of diff 
(for a filename with a tab inside of it) or if it's a filename with the 
GNU date extensions.

> > Now, if the kompare people can show that every single other patch 
> > generator adds the stupid tab + date format, I guess we could do it too, 
> > but
> >  (a) there is no valid date in general to use, so it's a fundamentally 
> >      broken notion and
> 
> Meaning we don't save timestamp in git ;-) Well, we could use date of the 
> commit which created given file contents (first commit from root, or last
> from head which contains given version)... but the same contents might be
> introduced independently in different commits. And different clones of the
> same repository might have different commit dates...

Exactly, a "date" in _any_ distributed SCM makes no sense what-so-ever. 
What happens across a merge? Which date do you take? Do you follow the 
thing down and basically do a full "annotate" on the file?

The fact is, dates in SCM diffs are insane and stupid. They do not make 
sense in the presense of an SCM, and they only make sense on individual 
_files_ (and quite limited there too, but at least it has _some_ meaning).

In the SCM, the _changes_ may have timestamps, but the files sure as hell 
should not. Of course, a file-based SCM easily gets confused about these 
things (eg in CVS, there is very much a 1:1 relationship between files and 
changes, so in the CVS mindset it can make sense - but that's because CVS 
is stupid to begin with, and the problem really goes much deeper than 
timestamps on the diffs).

			Linus

Re: kompare won't parse git diffs

[Jakub Narebski]

Linus Torvalds wrote:

> On Wed, 2 Aug 2006, Jakub Narebski wrote:
>
>> > Now, if the kompare people can show that every single other patch 
>> > generator adds the stupid tab + date format, I guess we could do it too, 
>> > but
>> >  (a) there is no valid date in general to use, so it's a fundamentally 
>> >      broken notion and
>> 
>> Meaning we don't save timestamp in git ;-) Well, we could use date of the 
>> commit which created given file contents (first commit from root, or last
>> from head which contains given version)... but the same contents might be
>> introduced independently in different commits. And different clones of the
>> same repository might have different commit dates...
> 
> Exactly, a "date" in _any_ distributed SCM makes no sense what-so-ever. 
> What happens across a merge? Which date do you take? Do you follow the 
> thing down and basically do a full "annotate" on the file?
> 
> The fact is, dates in SCM diffs are insane and stupid. They do not make 
> sense in the presense of an SCM, and they only make sense on individual 
> _files_ (and quite limited there too, but at least it has _some_ meaning).

What about putting e.g. sha1 (or abbreviated sha1) of blob if file exist 
in repository, "(in index)" or "(working area)" in two other cases, instead
of date or file version?

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: kompare won't parse git diffs

[Linus Torvalds]

On Wed, 2 Aug 2006, Jakub Narebski wrote:
> 
> What about putting e.g. sha1 (or abbreviated sha1) of blob if file exist 
> in repository, "(in index)" or "(working area)" in two other cases, instead
> of date or file version?

Quite frankly, it will just break the current git header standard, and 
make diffs look uglier.

Just get kompare fixed instead. If you want to use it with git, you would 
really want it to know about all the other git diff extensions too anyway, 
and right now it's _buggy_ for requiring a tab where none needs to be.

If "patch" doesn't want the tab, then dammit, neither should kompare.

			Linus

Re: kompare won't parse git diffs

[Paul Eggert]

Junio C Hamano <junkio@cox.net> writes:

> I wonder what happened to the plan to update GNU diff/patch to
> also emit/understand the c-style quoted paths.  Paul?

It's still intended, but not yet implemented unfortunately.