threads / rfc / 11305

[RFC 2/4] gitk: Add diff-diff support

Subject: [RFC 2/4] gitk: Add diff-diff support

## tl;dr

8 messages between Dec 15, 2007 and Dec 15, 2007.

replies: 7people: 4as markdown or json

Steffen Prohaska· Dec 15, 2007, 16:51 UTC · lore

[RFC 1/4] Add diff-diff, which compares the diffs of two commits

The following patch series adds experimental diff-diff support. It adds a very basic command line version and experimental gitk support for displaying the differences between the patches associated with two commits.

"git diff-diff commit1 commit2" displays the differences between the changes introduced by commit1 and commit2. This is displayed as a unified diff between the two patches.

In gitk you can select one commit, right click on a second commit, and select "Diff Diff selected -> this" to display the changes between the patches for the two commits. Highlighting of the result could certainly be improved. But it is already kind of helpful.

This can, for example, be used to compare a first version of a patch with an improved version of the same patch. In this case it is helpful to display the difference between the patches (not the files changed). If you commented on the first version, you may just want to check if the original author improved the patch according to your comments.

At this point, I'm only seeking comments about the general direction. The patches should not be applied to git.git.

 - Do you think something like this would be helpful?
 - Are similar approaches already available?
 - How do you use git to support code review; besides discussing
   patches on mailing lists?
    Steffen
---
 Makefile         |    3 ++-
 git-diff-diff.sh |   22 ++++++++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletions(-)
 create mode 100755 git-diff-diff.sh
diff --git a/Makefile b/Makefile
index b9fe40b..2e015ad 100644
--- a/Makefile
+++ b/Makefile
@@ -227,7 +227,8 @@ SCRIPT_SH = \
 	git-lost-found.sh git-quiltimport.sh git-submodule.sh \
 	git-filter-branch.sh \
 	git-stash.sh \
-	git-browse--help.sh
+	git-browse--help.sh \
+	git-diff-diff.sh
 
 SCRIPT_PERL = \
 	git-add--interactive.perl \
diff --git a/git-diff-diff.sh b/git-diff-diff.sh
new file mode 100755
index 0000000..aa402b2
--- /dev/null
+++ b/git-diff-diff.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+
+OPTIONS_KEEPDASHDASH=
+OPTIONS_SPEC="\
+git-diff-diff <commit> <commit>
+--
+"
+
+. git-sh-setup
+require_work_tree
+
+[ $# = 3 ] || usage
+
+from=$(git rev-parse --verify $2) || exit
+to=$(git rev-parse --verify $3) || exit
+
+git show $from >.git-commit-$from
+git show $to >.git-commit-$to
+
+diff -u .git-commit-$from .git-commit-$to
+
+rm .git-commit-$from .git-commit-$to

-- 
1.5.4.rc0.37.geff3a-dirty
Steffen Prohaska· Dec 15, 2007, 16:51 UTC · re: Steffen Prohaska · lore
---
 gitk-git/gitk |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)
diff --git a/gitk-git/gitk b/gitk-git/gitk
index 1da0b0a..ca121e4 100644
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -1019,6 +1019,8 @@ proc makewindow {} {
 	-command {diffvssel 0}
     $rowctxmenu add command -label "Diff selected -> this" \
 	-command {diffvssel 1}
+    $rowctxmenu add command -label "Diff Diff selected -> this" \
+	-command {diffdiffvssel 1}
     $rowctxmenu add command -label "Make patch" -command mkpatch
     $rowctxmenu add command -label "Create tag" -command mktag
     $rowctxmenu add command -label "Write commit to file" -command writecommit
@@ -6010,6 +6012,50 @@ proc doseldiff {oldid newid} {
     startdiff [list $oldid $newid]
 }
 
+proc diffdiffvssel {dirn} {
+    global rowmenuid selectedline displayorder
+
+    if {![info exists selectedline]} return
+    if {$dirn} {
+	set oldid [lindex $displayorder $selectedline]
+	set newid $rowmenuid
+    } else {
+	set oldid $rowmenuid
+	set newid [lindex $displayorder $selectedline]
+    }
+    addtohistory [list doseldiffdiff $oldid $newid]
+    doseldiffdiff $oldid $newid
+}
+
+proc doseldiffdiff {oldid newid} {
+    global ctext
+    global commitinfo
+
+    $ctext conf -state normal
+    clear_ctext
+    init_flist "Top"
+    $ctext insert end "From Commit "
+    $ctext insert end $oldid link0
+    setlink $oldid link0
+    $ctext insert end "\n     "
+    $ctext insert end [lindex $commitinfo($oldid) 0]
+    $ctext insert end "\n\nTo Commit   "
+    $ctext insert end $newid link1
+    setlink $newid link1
+    $ctext insert end "\n     "
+    $ctext insert end [lindex $commitinfo($newid) 0]
+    $ctext insert end "\n"
+    $ctext insert end "\n"
+    $ctext conf -state disabled
+    $ctext tag remove found 1.0 end
+
+    set diff [exec git diff-diff $oldid $newid]
+    $ctext conf -state normal
+    $ctext insert end "\n"
+    $ctext insert end $diff
+    $ctext conf -state disabled
+}
+
 proc mkpatch {} {
     global rowmenuid currentid commitinfo patchtop patchnum
 

-- 
1.5.4.rc0.37.geff3a-dirty
Steffen Prohaska· Dec 15, 2007, 16:51 UTC · re: Steffen Prohaska · lore

[RFC 3/4] gitk: Refactor getblobdiffline to split off adddiffline

getblobdiffline used to read from a file descriptor, parsed the line read, and displayed the line nicely formatted.

Parsing and displaying the line is split off to a separate function adddiffline.

---
 gitk-git/gitk |   30 +++++++++++++++++++-----------
 1 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/gitk-git/gitk b/gitk-git/gitk
index ca121e4..3c4ec03 100644
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -5324,18 +5324,11 @@ proc makediffhdr {fname ids} {
     $ctext insert $curdiffstart "$pad $fname $pad" filesep
 }
 
-proc getblobdiffline {bdf ids} {
+proc adddiffline {line ids} {
     global diffids blobdifffd ctext curdiffstart
     global diffnexthead diffnextnote difffilestart
     global diffinhdr treediffs
 
-    set nr 0
-    $ctext conf -state normal
-    while {[incr nr] <= 1000 && [gets $bdf line] >= 0} {
-	if {$ids != $diffids || $bdf != $blobdifffd($ids)} {
-	    close $bdf
-	    return 0
-	}
 	if {![string compare -length 11 "diff --git " $line]} {
 	    # trim off "diff --git "
 	    set line [string range $line 11 end]
@@ -5357,7 +5350,7 @@ proc getblobdiffline {bdf ids} {
 	    if {!(($l & 1) && [string index $line $i] eq " " &&
 		  [string range $line 2 [expr {$i - 1}]] eq \
 		      [string range $line [expr {$i + 3}] end])} {
-		continue
+		return
 	    }
 	    # unescape if quoted and chop off the a/ from the front
 	    if {[string index $line 0] eq "\""} {
@@ -5391,10 +5384,10 @@ proc getblobdiffline {bdf ids} {
 		makediffhdr $fname $ids
 	    } elseif {[string compare -length 3 $line "---"] == 0} {
 		# do nothing
-		continue
+		return
 	    } elseif {[string compare -length 3 $line "+++"] == 0} {
 		set diffinhdr 0
-		continue
+		return
 	    }
 	    $ctext insert end "$line\n" filesep
 
@@ -5411,6 +5404,21 @@ proc getblobdiffline {bdf ids} {
 		$ctext insert end "$line\n" hunksep
 	    }
 	}
+}
+
+proc getblobdiffline {bdf ids} {
+    global diffids blobdifffd ctext curdiffstart
+    global diffnexthead diffnextnote difffilestart
+    global diffinhdr treediffs
+
+    set nr 0
+    $ctext conf -state normal
+    while {[incr nr] <= 1000 && [gets $bdf line] >= 0} {
+	if {$ids != $diffids || $bdf != $blobdifffd($ids)} {
+	    close $bdf
+	    return 0
+	}
+	adddiffline $line $ids
     }
     $ctext conf -state disabled
     if {[eof $bdf]} {

-- 
1.5.4.rc0.37.geff3a-dirty
Steffen Prohaska· Dec 15, 2007, 16:51 UTC · re: Steffen Prohaska · lore

[RFC 4/4] gitk: Use adddiffline to nicely format diff-diff

---
 gitk-git/gitk |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/gitk-git/gitk b/gitk-git/gitk
index 3c4ec03..31b2b5f 100644
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -6059,8 +6059,9 @@ proc doseldiffdiff {oldid newid} {
 
     set diff [exec git diff-diff $oldid $newid]
     $ctext conf -state normal
-    $ctext insert end "\n"
-    $ctext insert end $diff
+    foreach line [split $diff "\n"] {
+	adddiffline $line [list $oldid $newid]
+    }
     $ctext conf -state disabled
 }
 

-- 
1.5.4.rc0.37.geff3a-dirty
Johannes Schindelin· Dec 15, 2007, 16:55 UTC · re: Steffen Prohaska · lore

Re: [RFC 1/4] Add diff-diff, which compares the diffs of two commits

Hi,
On Sat, 15 Dec 2007, Steffen Prohaska wrote:
> The following patch series adds experimental diff-diff support. It adds 
> a very basic command line version and experimental gitk support for 
> displaying the differences between the patches associated with two 
> commits.

I briefly considered adding something like interdiff a year ago, but decided that installing patchutils and having an alias was enough for my needs.

IOW I do not need this to be a git command.

Ciao, Dscho

Björn Steinbrink· Dec 15, 2007, 17:04 UTC · re: Steffen Prohaska · lore

Re: [RFC 1/4] Add diff-diff, which compares the diffs of two commits

On 2007.12.15 17:51:42 +0100, Steffen Prohaska wrote:
> The following patch series adds experimental diff-diff support.
[...]
> At this point, I'm only seeking comments about the general direction.
> The patches should not be applied to git.git.
> 
>  - Do you think something like this would be helpful?

I think I might use it to e.g. compare stuff when upstream modified the patch I submitted.

>  - Are similar approaches already available?

interdiff from patchutils. Probably interdiff would also be a better name than diff-diff, just to be consistent with what is already there. And using interdiff instead of plain diff in your script might also yield better results (I didn't use interdiff that often yet).

Björn
Steffen Prohaska· Dec 15, 2007, 17:33 UTC · re: Björn Steinbrink · lore

Re: [RFC 1/4] Add diff-diff, which compares the diffs of two commits

On Dec 15, 2007, at 6:04 PM, Björn Steinbrink wrote:
Show 12 quoted lines
> On 2007.12.15 17:51:42 +0100, Steffen Prohaska wrote:
>> The following patch series adds experimental diff-diff support.
> [...]
>> At this point, I'm only seeking comments about the general direction.
>> The patches should not be applied to git.git.
>>
>>  - Are similar approaches already available?
>
> interdiff from patchutils. Probably interdiff would also be a better
> name than diff-diff, just to be consistent with what is already there.
> And using interdiff instead of plain diff in your script might also
> yield better results (I didn't use interdiff that often yet).

At a first glance, interdiff yields more intuitive results if it works, but it may fail if the two patches differ to much. I need to play more to see how useful it is in real world examples. I did not know interdiff before.

At this point I'm not convinced that interdiff is exactly what I have in mind. For example it doesn't show differences between commit messages of two git patches.

	Steffen
Junio C Hamano· Dec 15, 2007, 19:34 UTC · re: Steffen Prohaska · lore

Re: [RFC 1/4] Add diff-diff, which compares the diffs of two commits

Steffen Prohaska <prohaska@zib.de> writes:
Show 7 quoted lines
> At this point, I'm only seeking comments about the general direction.
> The patches should not be applied to git.git.
>
>  - Do you think something like this would be helpful?
>  - Are similar approaches already available?
>  - How do you use git to support code review; besides discussing
>    patches on mailing lists?

I think there are other good building blocks you can use outside git to help reviewing evolution of patches (interdiff comes to mind).

When I receive a replacement series to a topic, I use RP script (available in my 'todo' branch and checked out in Meta/ directory in my work area, together with other scripts from 'todo'), which:

 * finds where the older series were applied;
 * detaches HEAD at that commit, and apply the new series;
 * runs diff between the old and new;
 * updates the tip of that topic with the new series.

After it finishes, "diff topic@{1} topic" becomes the incremental diff between the old and the new series, and if I do not like the end result, I can reset --hard back to topic@{1}.

← back to recent threads