threads / patch / 25668

patchgitk: Add "First parent" checkbox

Subject: [PATCH] gitk: Add "First parent" checkbox

## tl;dr

9 messages between Nov 8, 2010 and Dec 12, 2010. Diffs are folded; open one to read it.

replies: 8people: 3as markdown or json

Stefan Haller· Nov 8, 2010, 10:42 UTC · lore

Sometimes it's desirable to see what changes were introduced by a merge commit, rather than how conflicts were resolved. This adds a checkbox which, when turned on, makes gitk show the equivalent of "git show --first-parent <commit>" for merge commits.

Signed-off-by: Stefan Haller <stefan@haller-berlin.de>
---
I realize this conflicts with Thomas Rast's recent patch to
add a word-diff dropdown box; things are fighting for space
in the diff pane header...
 gitk |   24 +++++++++++++++++++++---
 1 files changed, 21 insertions(+), 3 deletions(-)
Show changes to gitk +21 −3
diff --git a/gitk b/gitk
index 45e3380..db0f022 100755
--- a/gitk
+++ b/gitk
@@ -2245,6 +2245,9 @@ proc makewindow {} {
     ${NS}::checkbutton .bleft.mid.ignspace -text [mc "Ignore space change"] \
 	-command changeignorespace -variable ignorespace
     pack .bleft.mid.ignspace -side left -padx 5
+    ${NS}::checkbutton .bleft.mid.firstparent -text [mc "First parent"] \
+	-command changefirstparent -variable firstparent
+    pack .bleft.mid.firstparent -side left -padx 5
     set ctext .bleft.bottom.ctext
     text $ctext -background $bgcolor -foreground $fgcolor \
 	-state disabled -font textfont \
@@ -6872,6 +6875,7 @@ proc selectline {l isnew {desired_loc {}}} {
     global cmitmode showneartags allcommits
     global targetrow targetid lastscrollrows
     global autoselect jump_to_here
+    global firstparent
 
     catch {unset pending_select}
     $canv delete hover
@@ -7013,7 +7017,7 @@ proc selectline {l isnew {desired_loc {}}} {
     init_flist [mc "Comments"]
     if {$cmitmode eq "tree"} {
 	gettree $id
-    } elseif {[llength $olds] <= 1} {
+    } elseif {[llength $olds] <= 1 || $firstparent} {
 	startdiff $id
     } else {
 	mergediff $id
@@ -7416,7 +7420,7 @@ proc diffcmd {ids flags} {
 proc gettreediffs {ids} {
     global treediff treepending
 
-    if {[catch {set gdtf [open [diffcmd $ids {--no-commit-id}] r]}]} return
+    if {[catch {set gdtf [open [diffcmd $ids {--no-commit-id -m --first-parent}] r]}]} return
 
     set treepending $ids
     set treediff {}
@@ -7504,11 +7508,19 @@ proc changeignorespace {} {
     reselectline
 }
 
+proc changefirstparent {} {
+    global treediffs
+    catch {unset treediffs}
+
+    reselectline
+}
+
 proc getblobdiffs {ids} {
     global blobdifffd diffids env
     global diffinhdr treediffs
     global diffcontext
     global ignorespace
+    global firstparent
     global limitdiffs vfilelimit curview
     global diffencoding targetline diffnparents
     global git_version currdiffsubmod
@@ -7521,10 +7533,15 @@ proc getblobdiffs {ids} {
     if {[package vcompare $git_version "1.6.6"] >= 0} {
 	set submodule "--submodule"
     }
-    set cmd [diffcmd $ids "-p $textconv $submodule  -C --cc --no-commit-id -U$diffcontext"]
+    set cmd [diffcmd $ids "-p $textconv $submodule  -C --no-commit-id -U$diffcontext"]
     if {$ignorespace} {
 	append cmd " -w"
     }
+    if {$firstparent} {
+	append cmd " -m --first-parent"
+    } else {
+	append cmd " --cc"
+    }
     if {$limitdiffs && $vfilelimit($curview) ne {}} {
 	set cmd [concat $cmd -- $vfilelimit($curview)]
     }
@@ -11393,6 +11410,7 @@ if {[tk windowingsystem] eq "win32"} {
 set diffcolors {red "#00a000" blue}
 set diffcontext 3
 set ignorespace 0
+set firstparent 0
 set markbgcolor "#e0e0ff"
 
 set circlecolors {white blue gray blue blue}

-- 
1.7.3.2.153.g8250e
Jonathan Nieder· Nov 8, 2010, 17:24 UTC · re: Stefan Haller · lore

Re: [PATCH] gitk: Add "First parent" checkbox

Stefan Haller wrote:
> Sometimes it's desirable to see what changes were introduced by a
> merge commit, rather than how conflicts were resolved. This adds
> a checkbox which, when turned on, makes gitk show the equivalent
> of "git show --first-parent <commit>" for merge commits.

To be clear: this is a diff option (like -m but limited to one parent), not a history traversal option, right?

> I realize this conflicts with Thomas Rast's recent patch to
> add a word-diff dropdown box; things are fighting for space
> in the diff pane header...
Any ideas for addressing the space shortage?
Stefan Haller· Nov 8, 2010, 19:40 UTC · re: Jonathan Nieder · lore

Re: [PATCH] gitk: Add "First parent" checkbox

Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 9 quoted lines
> Stefan Haller wrote:
> 
> > Sometimes it's desirable to see what changes were introduced by a
> > merge commit, rather than how conflicts were resolved. This adds
> > a checkbox which, when turned on, makes gitk show the equivalent
> > of "git show --first-parent <commit>" for merge commits.
> 
> To be clear: this is a diff option (like -m but limited to one
> parent), not a history traversal option, right?

Yes, that's right. There is an existing history traversal option called "Limit to first parent" in the "Edit View" dialog, which can be controlled with the --first-parent command line option. Do you think this will cause confusion?

Show 5 quoted lines
> > I realize this conflicts with Thomas Rast's recent patch to
> > add a word-diff dropdown box; things are fighting for space
> > in the diff pane header...
> 
> Any ideas for addressing the space shortage?

Matthieu made the suggestion of "line-wrapping" the widgets in the diff pane header if it becomes too narrow. I guess this could work; I don't know enough Tcl/Tk to be able to try it out though.

-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/
Jonathan Nieder· Nov 8, 2010, 19:48 UTC · re: Stefan Haller · lore

Re: [PATCH] gitk: Add "First parent" checkbox

Stefan Haller wrote:
>> Stefan Haller wrote:
>>> I realize this conflicts with Thomas Rast's recent patch to
>>> add a word-diff dropdown box; things are fighting for space
>>> in the diff pane header...
[...]
> Matthieu made the suggestion of "line-wrapping" the widgets in the diff
> pane header if it becomes too narrow.  I guess this could work; I don't
> know enough Tcl/Tk to be able to try it out though.
Hmm.  I don't like where this method tends in the limit.

Maybe we need a notion of a "diff view", for setting up various diff-tree options (word diff, whitespace options, context lines, patience, diffstat, first-parent)? The diff pane could then provide a drop-down box for diff views already set up.

Stefan Haller· Nov 8, 2010, 20:23 UTC · re: Jonathan Nieder · lore

Re: [PATCH] gitk: Add "First parent" checkbox

Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 11 quoted lines
> Stefan Haller wrote:
>
> > Matthieu made the suggestion of "line-wrapping" the widgets in the diff
> > pane header if it becomes too narrow.
>
> Hmm.  I don't like where this method tends in the limit.
>
> Maybe we need a notion of a "diff view", for setting up various
> diff-tree options (word diff, whitespace options, context lines,
> patience, diffstat, first-parent)?  The diff pane could then provide a
> drop-down box for diff views already set up.

I don't think I would like this. Most of the diff related options are ones that I want to toggle with one click. That's certainly true for "Ignore space change", "First parent", and the "Diff/Old Version/New Version" radio buttons. I would hate to see any of them be removed from the diff pane.

-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/
Jonathan Nieder· Nov 8, 2010, 21:11 UTC · re: Stefan Haller · lore

Re: [PATCH] gitk: Add "First parent" checkbox

Stefan Haller wrote:
Show 5 quoted lines
>                                   Most of the diff related options are
> ones that I want to toggle with one click.  That's certainly true for
> "Ignore space change", "First parent", and the "Diff/Old Version/New
> Version" radio buttons.  I would hate to see any of them be removed from
> the diff pane.

Thanks, that's useful information. Could you give example workflows to illustrate this? Here's an example:

   Toggling between "Diff/Old version/New version" with a single click
   allows one to compare the preimage and postimage of a patch visually.
   It makes what changed much more obvious.
   Historically these have been radio buttons on the top-left part of
   the diff pane.  If they move, it might be hard for people already
   familiar with gitk to find them.
   The preimage/postimage toggle is a useful and novel feature, and it
   is greedy with respect to screen real estate (a few extra diff
   lines can go a long way), so it would not be good to hide it in a
   separate "more diff options" toolbar.

The case for one-click "diff against parent" is less clear. Is it that it gets toggled in practice often (i.e., avoiding pointless repetitive actions)?

Hope that helps.
Stefan Haller· Nov 10, 2010, 07:17 UTC · re: Jonathan Nieder · lore

Re: [PATCH] gitk: Add "First parent" checkbox

Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 10 quoted lines
> Stefan Haller wrote:
> 
> >                                   Most of the diff related options are
> > ones that I want to toggle with one click.  That's certainly true for
> > "Ignore space change", "First parent", and the "Diff/Old Version/New
> > Version" radio buttons.  I would hate to see any of them be removed from
> > the diff pane.
> 
> Thanks, that's useful information.  Could you give example workflows
> to illustrate this?

When browsing history, I usually have "First parent" off, so that I see empty diffs for most merges, except the ones that had conflicts. Occasionally though, when looking at a merge of a larger branch, I want to see what changes were introduced by the merge; turning on "First parent" allows me to quickly see that, or in general gives me an idea of just how "big" the merge was. I usually turn it back off right afterwards.

Similar for "Ignore space change": I usually have it off most of the time, so that I see faithful diffs. Occasionally, when just the indentation of a block of C code has changed, I turn it on just for that one diff to make it easier to read; then I turn it back off right afterwards.

Also similar for "Lines of context": this is usually set to a medium value, like 3 or 4; sometimes I'll increase it to 12 or more for a single diff to see more of the surrounding code, and sometimes I'll set it to 1 for a merge diff with hunks so close to each other that they otherwise show as conflicts.

Your proposal of having presets for diff options has the drawback that I would need a preset for each combination of these options.

-Stefan
-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/
Paul Mackerras· Dec 12, 2010, 04:27 UTC · re: Stefan Haller · lore

Re: [PATCH] gitk: Add "First parent" checkbox

On Mon, Nov 08, 2010 at 11:42:59AM +0100, Stefan Haller wrote:
Show 10 quoted lines
> Sometimes it's desirable to see what changes were introduced by a
> merge commit, rather than how conflicts were resolved. This adds
> a checkbox which, when turned on, makes gitk show the equivalent
> of "git show --first-parent <commit>" for merge commits.
> 
> Signed-off-by: Stefan Haller <stefan@haller-berlin.de>
> ---
> I realize this conflicts with Thomas Rast's recent patch to
> add a word-diff dropdown box; things are fighting for space
> in the diff pane header...

I just applied Thomas Rast's patch, so you'll need to rebase. Also you're right that we're running out of space; perhaps we need to make the pane header two rows high. Finally, "First parent" doesn't really convey to me immediately what it does -- I have to think about it, so it will probably confuse new users. I don't know what would be better, though.

Paul.
lists@haller-berlin.de· Dec 12, 2010, 20:08 UTC · re: Paul Mackerras · lore

[PATCH v2/RFC] gitk: Add "First parent" checkbox

From: Stefan Haller <lists@haller-berlin.de>

Sometimes it's desirable to see what changes were introduced by a merge commit, rather than how conflicts were resolved. This adds a checkbox which, when turned on, makes gitk show the equivalent of "git show --first-parent <commit>" for merge commits.

Signed-off-by: Stefan Haller <stefan@haller-berlin.de>
---
Paul Mackerras <paulus@samba.org> wrote:
> I just applied Thomas Rast's patch, so you'll need to rebase.

OK, here's a new patch, rebased onto current master (but otherwise unchanged for now).

> Also you're right that we're running out of space; perhaps we need to make
> the pane header two rows high.

The suggestion was to make it two rows high only if it doesn't fit on one row (i.e. dynamically "line-wrap"), and I like the idea. Unfortunately that's beyond my Tk skills; anybody willing to help?

> Finally, "First parent" doesn't really convey to me immediately what it
> does -- I have to think about it, so it will probably confuse new users.
> I don't know what would be better, though.

What I like about it is that it's consistent with the command-line client, "git show --first-parent". But I don't insist on it if anybody has a better suggestion.

 gitk |   25 ++++++++++++++++++++++---
 1 files changed, 22 insertions(+), 3 deletions(-)
Show changes to gitk +22 −3
diff --git a/gitk b/gitk
index e82c6bf..7201ba0 100755
--- a/gitk
+++ b/gitk
@@ -2269,6 +2269,10 @@ proc makewindow {} {
 	pack .bleft.mid.worddiff -side left -padx 5
     }
 
+    ${NS}::checkbutton .bleft.mid.firstparent -text [mc "First parent"] \
+	-command changefirstparent -variable firstparent
+    pack .bleft.mid.firstparent -side left -padx 5
+
     set ctext .bleft.bottom.ctext
     text $ctext -background $bgcolor -foreground $fgcolor \
 	-state disabled -font textfont \
@@ -6897,6 +6901,7 @@ proc selectline {l isnew {desired_loc {}}} {
     global cmitmode showneartags allcommits
     global targetrow targetid lastscrollrows
     global autoselect jump_to_here
+    global firstparent
 
     catch {unset pending_select}
     $canv delete hover
@@ -7038,7 +7043,7 @@ proc selectline {l isnew {desired_loc {}}} {
     init_flist [mc "Comments"]
     if {$cmitmode eq "tree"} {
 	gettree $id
-    } elseif {[llength $olds] <= 1} {
+    } elseif {[llength $olds] <= 1 || $firstparent} {
 	startdiff $id
     } else {
 	mergediff $id
@@ -7442,7 +7447,7 @@ proc diffcmd {ids flags} {
 proc gettreediffs {ids} {
     global treediff treepending
 
-    if {[catch {set gdtf [open [diffcmd $ids {--no-commit-id}] r]}]} return
+    if {[catch {set gdtf [open [diffcmd $ids {--no-commit-id -m --first-parent}] r]}]} return
 
     set treepending $ids
     set treediff {}
@@ -7534,12 +7539,20 @@ proc changeworddiff {name ix op} {
     reselectline
 }
 
+proc changefirstparent {} {
+    global treediffs
+    catch {unset treediffs}
+
+    reselectline
+}
+
 proc getblobdiffs {ids} {
     global blobdifffd diffids env
     global diffinhdr treediffs
     global diffcontext
     global ignorespace
     global worddiff
+    global firstparent
     global limitdiffs vfilelimit curview
     global diffencoding targetline diffnparents
     global git_version currdiffsubmod
@@ -7552,13 +7565,18 @@ proc getblobdiffs {ids} {
     if {[package vcompare $git_version "1.6.6"] >= 0} {
 	set submodule "--submodule"
     }
-    set cmd [diffcmd $ids "-p $textconv $submodule  -C --cc --no-commit-id -U$diffcontext"]
+    set cmd [diffcmd $ids "-p $textconv $submodule  -C --no-commit-id -U$diffcontext"]
     if {$ignorespace} {
 	append cmd " -w"
     }
     if {$worddiff ne [mc "Line diff"]} {
 	append cmd " --word-diff=porcelain"
     }
+    if {$firstparent} {
+	append cmd " -m --first-parent"
+    } else {
+	append cmd " --cc"
+    }
     if {$limitdiffs && $vfilelimit($curview) ne {}} {
 	set cmd [concat $cmd -- $vfilelimit($curview)]
     }
@@ -11453,6 +11471,7 @@ set diffcolors {red "#00a000" blue}
 set diffcontext 3
 set ignorespace 0
 set worddiff ""
+set firstparent 0
 set markbgcolor "#e0e0ff"
 
 set circlecolors {white blue gray blue blue}

-- 
1.7.3.2.442.g97e50

← back to recent threads