[RFD] Strange patch formats (aka tricks with unified diffs)..

[RFD] Strange patch formats (aka tricks with unified diffs)..

[Linus Torvalds]

I'm not sure how many people on this list have worked with unified diffs 
for as long as I have, and are so used to them that they can edit them up 
(and fix up line numbers etc) in their sleep.

So I don't know if people are used to some of the really nice tricks you 
can do with them.

For example, a lot of traditional context diff patch proponents (you're 
wrong, btw) think that unified diffs are hard to read, because you don't 
see the "end result". There's a really trivial trick for this, which is to 
just do

	grep -v '^-'

on the patch, and suddenly you see the patch in a totally different light: 
you see what the end result is (in a much more readable format than a 
traditional context diff).

The reverse operation ("grep -v '^+'") also obviously works, but since it 
just shows the code the way it already is, the only reason to use it tends 
to be an incorrectly reversed patch, or when you just want to see the old 
code side-by-side with the new code (which is why some people prefer 
traditional context diffs, but the reason I hate them is that they break 
up the old/new vertically in random places, so you can't just have two 
windows open and compare them side by side).

I'm normally actually very good at doing that "grep" mentally, and don't 
actually often need to do it explicitly at all, unless the patch is just 
pretty messy. But sometimes the patch is just confusing enough that it 
helps to explicitly filter it.

Now, the reason I mention this is that I was just in "gitk", and while I 
love the gitk graphical revision view, I just noticed (once again) why I 
tend to hate most GUI programs. I can't do the clever tricks! The gitk 
diffs are nice and colorized, but the "show just end result" trick just 
doesn't work.

So I can see two ways to fix this:
 - bug Paul about alternate diff viewing capabilities in gitk. He's cc'd 
   here. A way to make the "diff" pane show just the new one, the old one, 
   or even both side-by-side with some mousy interface?
 - add some actual switch to git diff generation to hide the negative side 
   of a unified diff, and add some way to just make gitk pass that switch 
   in.

I dunno. I realize that it's not just gitk - gitweb, qgit etc don't allow 
the tricks *either*, but gitk was the one I just hit this with. I ended up 
just cutting-and-pasting the SHA1 and doing the thing in a terminal with 
the shell pipeline instead. Am wondering if maybe other people have ideas 
on this.

Maybe I don't need this often enough for it to matter (as mentioned, I'm 
so good at doing this in squishy-ware that I read patches _without_ the 
explicit help most of the time) but I realized that if I do it 
occasionally, perhaps people who haven't worked with unified diffs quite 
as much as I have never even realized..

		Linus

Re: [RFD] Strange patch formats (aka tricks with unified diffs)..

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> I'm normally actually very good at doing that "grep" mentally, and don't 
> actually often need to do it explicitly at all, unless the patch is just 
> pretty messy. But sometimes the patch is just confusing enough that it 
> helps to explicitly filter it.
>
> Now, the reason I mention this is that I was just in "gitk", and while I 
> love the gitk graphical revision view, I just noticed (once again) why I 
> tend to hate most GUI programs. I can't do the clever tricks! The gitk 
> diffs are nice and colorized, but the "show just end result" trick just 
> doesn't work.

> So I can see two ways to fix this:
>  - bug Paul about alternate diff viewing capabilities in gitk. He's cc'd 
>    here. A way to make the "diff" pane show just the new one, the old one, 
>    or even both side-by-side with some mousy interface?
>  - add some actual switch to git diff generation to hide the negative side 
>    of a unified diff, and add some way to just make gitk pass that switch 
>    in.

The switch itself would literally be a one-liner (see below).

However, I suspect what you want is _not_ "turn the negative
side completely off", but "make the negative side visually more
easily ignorable".

How about displaying the deleted lines with light gray on white
background, or something like that?

> I dunno. I realize that it's not just gitk - gitweb, qgit etc don't allow 
> the tricks *either*, but gitk was the one I just hit this with. I ended up 
> just cutting-and-pasting the SHA1 and doing the thing in a terminal with 
> the shell pipeline instead. Am wondering if maybe other people have ideas 
> on this.

diff --git a/diff.c b/diff.c
index fbb79d7..e61abed 100644
--- a/diff.c
+++ b/diff.c
@@ -565,6 +565,8 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 	}
 
 	if (color != DIFF_FILE_NEW) {
+		if (diff_no_deleted_lines && color == DIFF_FILE_OLD)
+			return;
 		emit_line(diff_get_color(ecbdata->color_diff, color),
 			  reset, line, len);
 		return;

Re: [RFD] Strange patch formats (aka tricks with unified diffs)..

[Marco Costalba]

On 4/12/07, Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>
> So I can see two ways to fix this:
>  - bug Paul about alternate diff viewing capabilities in gitk. He's cc'd
>    here. A way to make the "diff" pane show just the new one, the old one,
>    or even both side-by-side with some mousy interface?
>  - add some actual switch to git diff generation to hide the negative side
>    of a unified diff, and add some way to just make gitk pass that switch
>    in.
>
> I dunno. I realize that it's not just gitk - gitweb, qgit etc don't allow
> the tricks *either*, but gitk was the one I just hit this with.

Currently in qgit you can select a revision + a file and call an
external diff viewer, as example Kompare (tweakable from
'Edir->settings->External diff viewer' menu) to view the diff
side-by-side, or in any other way the viewer allows, for the selected
file.

I like also the idea to hide the negative side but I would rather
prefer a togglable command/button instead of a command line argument
or a setting, so to allow easy and quick hide/unhide on the same
patch.

I will work on it..... ;-)

Thanks for idea
Marco

Re: [RFD] Strange patch formats (aka tricks with unified diffs)..

[Marco Costalba]

On 4/13/07, Marco Costalba <mcostalba@gmail.com> wrote:
> On 4/12/07, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >
> >
> > So I can see two ways to fix this:
> >  - bug Paul about alternate diff viewing capabilities in gitk. He's cc'd
> >    here. A way to make the "diff" pane show just the new one, the old one,
> >    or even both side-by-side with some mousy interface?
> >  - add some actual switch to git diff generation to hide the negative side
> >    of a unified diff, and add some way to just make gitk pass that switch
> >    in.
> >
> > I dunno. I realize that it's not just gitk - gitweb, qgit etc don't allow
> > the tricks *either*, but gitk was the one I just hit this with.
>
> Currently in qgit you can select a revision + a file and call an
> external diff viewer, as example Kompare (tweakable from
> 'Edir->settings->External diff viewer' menu) to view the diff
> side-by-side, or in any other way the viewer allows, for the selected
> file.
>
> I like also the idea to hide the negative side but I would rather
> prefer a togglable command/button instead of a command line argument
> or a setting, so to allow easy and quick hide/unhide on the same
> patch.
>
> I will work on it..... ;-)
>

I have pushed a patch with support for toggling view of the negative
side of an unified diff (right most 'filter' button in patch viewer
tab or CTRL+H shortcut).

In case someone is interested repo is, as usual,

                git://git.kernel.org/pub/scm/qgit/qgit.git


  Marco

P.S: Another two useful shortcuts are the hide/unhide of revision
description header ('h' key or from 'View' menu) and hide/unhide of
secondary panes ('s' key or from 'View' menu) they work in all the
different tabs.

Re: [RFD] Strange patch formats (aka tricks with unified diffs)..

[Paul Mackerras]

Linus Torvalds writes:

>  - bug Paul about alternate diff viewing capabilities in gitk. He's cc'd 
>    here. A way to make the "diff" pane show just the new one, the old one, 
>    or even both side-by-side with some mousy interface?

Showing just the old lines, or just the new lines, would be dead easy,
since the Tk text widget has an "elide" option on tagged ranges of
text that effectively removes them from the display.  So I don't even
have to change the contents of the text widget, merely the display
options associated with the tags that I already have attached to the
old and new lines (that's how they get their color).

Side-by-side would be more challenging.

Alternatively I could provide a field where you can put in a filter
command to be put between the diff generator and the display parser.

Have you seen what dirdiff can do with displaying diffs?  You can
expose more context, move old and new lines up and down (but not in a
manner which changes the meaning of the diff), and split context lines
into old/new pairs, all with the mouse.  This lets me rearrange a diff
to make it more visually meaningful and understandable.  I could
import that stuff into gitk, since dirdiff is also written in Tcl/Tk.

Paul.

Re: [RFD] Strange patch formats (aka tricks with unified diffs)..

[Martin Langhoff]

On 4/13/07, Paul Mackerras <paulus@samba.org> wrote:
> Side-by-side would be more challenging.

<wish>If someone gets started on side-by-side, I'd love to have
char-level diff colouring, like xxdiff does. Oh, and chocolate
icecream too.</wish>

If there's enough rope to get started, I'll be happy to hack it into place.

cheers,


m

Re: [RFD] Strange patch formats (aka tricks with unified diffs)..

[Junio C Hamano]

"Martin Langhoff" <martin.langhoff@gmail.com> writes:

> On 4/13/07, Paul Mackerras <paulus@samba.org> wrote:
>> Side-by-side would be more challenging.
>
> <wish>If someone gets started on side-by-side, I'd love to have
> char-level diff colouring,...
> icecream too.</wish>

Is it different from the --color-words (whose implementation I
happen to hate)?

Re: [RFD] Strange patch formats (aka tricks with unified diffs)..

[Martin Langhoff]

On 4/13/07, Junio C Hamano <junkio@cox.net> wrote:
> "Martin Langhoff" <martin.langhoff@gmail.com> writes:
>
> > On 4/13/07, Paul Mackerras <paulus@samba.org> wrote:
> >> Side-by-side would be more challenging.
> >
> > <wish>If someone gets started on side-by-side, I'd love to have
> > char-level diff colouring,...
> > icecream too.</wish>
>
> Is it different from the --color-words (whose implementation I
> happen to hate)?

Didn't know about --color-words (goes away, tests...) no, nothing like
that. I don't like --color-words.

To see what I mean, try

   git-show v1.4.4:git-cvsserver.perl  > file1
   git-show v1.5.1:git-cvsserver.perl  > file2
   xxdiff file1 file2

Just press 'n' twice to get to the 2nd "hunk" and you'll see what I
mean. Next is 'n', previous is 'p'. It's useful to visualise subtle
differences, and to spot commonalities in large changes.

Some aspects of xxdiff UI are horrid but this feature... is great.
Probably tries a scan for commonalities from the start and end of the
line. And then perhaps from the middle or from each beginning of alpha
char sequence.

cheers,


m

Re: [RFD] Strange patch formats (aka tricks with unified diffs)..

[Johannes Schindelin]

Hi,

On Thu, 12 Apr 2007, Junio C Hamano wrote:

> [...] the --color-words (whose implementation I happen to hate) [...]

I know that we talked about it previously. And strictly, I think it is not 
the implementation that you hate, but the rules that were implemented.

I'll have a look at what you wrote back then, and try to come up with some 
sensible alternatives, so you don't have to hate the code so much.

Ciao,
Dscho

Re: [RFD] Strange patch formats (aka tricks with unified diffs)..

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Thu, 12 Apr 2007, Junio C Hamano wrote:
>
>> [...] the --color-words (whose implementation I happen to hate) [...]
>
> I know that we talked about it previously. And strictly, I think it is not 
> the implementation that you hate, but the rules that were implemented.
>
> I'll have a look at what you wrote back then, and try to come up with some 
> sensible alternatives, so you don't have to hate the code so much.

One sensible behaviour would be to show exactly the same output
lines as the regular output, but color-code only the words that
changed.  E.g (the words are capitalized to illustrate instead
of colored here):

        diff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt
        index 6a5fcfd..535214c 100644
        --- a/Documentation/git-cvsserver.txt
        +++ b/Documentation/git-cvsserver.txt
        @@ -125,7 +131,7 @@ By default it uses SQLite databases in the git directory, named
         temporary files in the same directory as the database file on
         write so it might not be enough to grant the users using
         git-cvsserver write access to the database file without granting
        -them ALSO write access to the directory.
        +them write access to the directory, TOO.

         You can configure the database backend with the following
         configuration variables:

Re: [RFD] Strange patch formats (aka tricks with unified diffs)..

[Jeff King]

On Sat, Apr 14, 2007 at 02:55:13AM -0700, Junio C Hamano wrote:

> One sensible behaviour would be to show exactly the same output
> lines as the regular output, but color-code only the words that
> changed.  E.g (the words are capitalized to illustrate instead
> of colored here):

That works great for highlighting a small change between two very
similar lines (and I think that is one use of --color-words). However,
it's terrible for the other use of color words, which is showing changes
which are not well-adapted to line oriented diffs (such as flowed text).

Try this:

cat >orig <<EOF
this is a paragraph that has
some text in it and is wrapped
at 30 characters
EOF

cat >rewrapped <<EOF
this is yet another paragraph
that has some text in it and
is wrapped at 30 characters
EOF

git-diff --color orig rewrapped
git-diff --color-words orig rewrapped


I think one behavior is best for one situation, and the other behavior
for another situation. Perhaps there is room for two modes?

-Peff

Re: [RFD] Strange patch formats (aka tricks with unified diffs)..

[Linus Torvalds]

On Fri, 13 Apr 2007, Paul Mackerras wrote:
>
> Linus Torvalds writes:
> 
> >  - bug Paul about alternate diff viewing capabilities in gitk. He's cc'd 
> >    here. A way to make the "diff" pane show just the new one, the old one, 
> >    or even both side-by-side with some mousy interface?
> 
> Showing just the old lines, or just the new lines, would be dead easy,
> since the Tk text widget has an "elide" option on tagged ranges of
> text that effectively removes them from the display.  So I don't even
> have to change the contents of the text widget, merely the display
> options associated with the tags that I already have attached to the
> old and new lines (that's how they get their color).

Ok, just a radio button for the patch case for "show patch" vs "show 
result" would certainly be sufficient for what I normally do. Adding the 
case "show origin" for completeness (ie filter out the new lines, rather 
than filtering out the old lines) is probably a good idea, even if I've 
never used it.

> Side-by-side would be more challenging.

Not a biggie. I've used kdiff3 etc in the past, and they do a good job of 
it and it "looks cool", but I've never done it on a command line which 
means that in practice _I_ don't use it. Maybe other people do.

> Have you seen what dirdiff can do with displaying diffs?  You can
> expose more context, move old and new lines up and down (but not in a
> manner which changes the meaning of the diff), and split context lines
> into old/new pairs, all with the mouse.

I've never used dirdiff. Sounds a bit like what kdiff3 does.

			Linus

Re: [RFD] Strange patch formats (aka tricks with unified diffs)..

[Paul Mackerras]

Linus Torvalds writes:

> Ok, just a radio button for the patch case for "show patch" vs "show 
> result" would certainly be sufficient for what I normally do. Adding the 
> case "show origin" for completeness (ie filter out the new lines, rather 
> than filtering out the old lines) is probably a good idea, even if I've 
> never used it.

Try this patch, let me know what you think...

Paul.

diff --git a/gitk b/gitk
index b1c65d7..a57e84c 100755
--- a/gitk
+++ b/gitk
@@ -593,6 +593,7 @@ proc makewindow {} {
         frame .bleft -width $geometry(botwidth) -height $geometry(botheight)
     }
     frame .bleft.top
+    frame .bleft.mid
 
     button .bleft.top.search -text "Search" -command dosearch \
 	-font $uifont
@@ -602,12 +603,20 @@ proc makewindow {} {
     lappend entries $sstring
     trace add variable searchstring write incrsearch
     pack $sstring -side left -expand 1 -fill x
+    radiobutton .bleft.mid.diff -text "Diff" \
+	-command changediffdisp -variable diffelide -value {0 0}
+    radiobutton .bleft.mid.old -text "Old version" \
+	-command changediffdisp -variable diffelide -value {0 1}
+    radiobutton .bleft.mid.new -text "New version" \
+	-command changediffdisp -variable diffelide -value {1 0}
+    pack .bleft.mid.diff .bleft.mid.old .bleft.mid.new -side left
     set ctext .bleft.ctext
     text $ctext -background $bgcolor -foreground $fgcolor \
 	-state disabled -font $textfont \
 	-yscrollcommand scrolltext -wrap none
     scrollbar .bleft.sb -command "$ctext yview"
     pack .bleft.top -side top -fill x
+    pack .bleft.mid -side top -fill x
     pack .bleft.sb -side right -fill y
     pack $ctext -side left -fill both -expand 1
     lappend bglist $ctext
@@ -4486,6 +4495,13 @@ proc getblobdiffline {bdf ids} {
     }
 }
 
+proc changediffdisp {} {
+    global ctext diffelide
+
+    $ctext tag conf d0 -elide [lindex $diffelide 0]
+    $ctext tag conf d1 -elide [lindex $diffelide 1]
+}
+
 proc prevfile {} {
     global difffilestart ctext
     set prev [lindex $difffilestart 0]
@@ -6330,6 +6346,7 @@ set highlight_paths {}
 set searchdirn -forwards
 set boldrows {}
 set boldnamerows {}
+set diffelide {0 0}
 
 set optim_delay 16
 

Re: [RFD] Strange patch formats (aka tricks with unified diffs)..

[Linus Torvalds]

On Wed, 18 Apr 2007, Paul Mackerras wrote:
> 
> Try this patch, let me know what you think...

Very good, looks great. 

		Linus

Re: [RFD] Strange patch formats (aka tricks with unified diffs)..

[Matthias Lederhofer]

Linus Torvalds <torvalds@linux-foundation.org> wrote:
>  - add some actual switch to git diff generation to hide the negative side 
>    of a unified diff, and add some way to just make gitk pass that switch 
>    in.

This one would still allow to show the added lines still colored in
green.  Or do I miss a way to do this?  git show --color is not very
grep friendly because lines begin with an escape sequence.

Re: [RFD] Strange patch formats (aka tricks with unified diffs)..

[Linus Torvalds]

On Fri, 13 Apr 2007, Matthias Lederhofer wrote:

> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >  - add some actual switch to git diff generation to hide the negative side 
> >    of a unified diff, and add some way to just make gitk pass that switch 
> >    in.
> 
> This one would still allow to show the added lines still colored in
> green.  Or do I miss a way to do this?  git show --color is not very
> grep friendly because lines begin with an escape sequence.

Heh. I'm not much for color, so I've never done it. But if you want it, 
you should be able to just do

	git show --color <commit> |
		grep -v '^[^ ]*-' |
		less -RS

because every line of a diff (and even the explanations, thanks to the 
indent-by-four rule of git log printing) should either start with a "+/-" 
*or* they will have a space before they will show up, so using the "no 
spaces before the -" rule instead of "beginning of line" should work..

So the above will *literally* just remove the "---" line and the actual 
removal lines of the diff, even in the presense of color coding. 

The exception, of course, is:
 - maybe I didn't think it through and I missed some case, and it removes 
   lines it shouldn't.
 - if you make the color coding itself have spaces or something in it 
   you break my assumption (but the vt100 codes should be fine)

Hmm?

		Linus