{"thread":{"id":"25668","subject":"[PATCH] gitk: Add \"First parent\" checkbox","startedAt":"2010-11-08T10:42:59Z","lastAt":"2010-12-12T20:08:05Z","messageCount":9,"participants":["Stefan Haller","Jonathan Nieder","Paul Mackerras","lists@haller-berlin.de"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"155364","messageId":"1289212979-64246-1-git-send-email-lists@haller-berlin.de","threadId":"25668","inReplyTo":null,"subject":"[PATCH] gitk: Add \"First parent\" checkbox","fromName":"Stefan Haller","fromEmail":"lists@haller-berlin.de","sentAt":"2010-11-08T10:42:59Z","receivedAt":"2010-11-08T10:42:59Z","isPatch":true,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"Sometimes it's desirable to see what changes were introduced by a\nmerge commit, rather than how conflicts were resolved. This adds\na checkbox which, when turned on, makes gitk show the equivalent\nof \"git show --first-parent <commit>\" for merge commits.\n\nSigned-off-by: Stefan Haller <stefan@haller-berlin.de>\n---\nI realize this conflicts with Thomas Rast's recent patch to\nadd a word-diff dropdown box; things are fighting for space\nin the diff pane header...\n\n gitk |   24 +++++++++++++++++++++---\n 1 files changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/gitk b/gitk\nindex 45e3380..db0f022 100755\n--- a/gitk\n+++ b/gitk\n@@ -2245,6 +2245,9 @@ proc makewindow {} {\n     ${NS}::checkbutton .bleft.mid.ignspace -text [mc \"Ignore space change\"] \\\n \t-command changeignorespace -variable ignorespace\n     pack .bleft.mid.ignspace -side left -padx 5\n+    ${NS}::checkbutton .bleft.mid.firstparent -text [mc \"First parent\"] \\\n+\t-command changefirstparent -variable firstparent\n+    pack .bleft.mid.firstparent -side left -padx 5\n     set ctext .bleft.bottom.ctext\n     text $ctext -background $bgcolor -foreground $fgcolor \\\n \t-state disabled -font textfont \\\n@@ -6872,6 +6875,7 @@ proc selectline {l isnew {desired_loc {}}} {\n     global cmitmode showneartags allcommits\n     global targetrow targetid lastscrollrows\n     global autoselect jump_to_here\n+    global firstparent\n \n     catch {unset pending_select}\n     $canv delete hover\n@@ -7013,7 +7017,7 @@ proc selectline {l isnew {desired_loc {}}} {\n     init_flist [mc \"Comments\"]\n     if {$cmitmode eq \"tree\"} {\n \tgettree $id\n-    } elseif {[llength $olds] <= 1} {\n+    } elseif {[llength $olds] <= 1 || $firstparent} {\n \tstartdiff $id\n     } else {\n \tmergediff $id\n@@ -7416,7 +7420,7 @@ proc diffcmd {ids flags} {\n proc gettreediffs {ids} {\n     global treediff treepending\n \n-    if {[catch {set gdtf [open [diffcmd $ids {--no-commit-id}] r]}]} return\n+    if {[catch {set gdtf [open [diffcmd $ids {--no-commit-id -m --first-parent}] r]}]} return\n \n     set treepending $ids\n     set treediff {}\n@@ -7504,11 +7508,19 @@ proc changeignorespace {} {\n     reselectline\n }\n \n+proc changefirstparent {} {\n+    global treediffs\n+    catch {unset treediffs}\n+\n+    reselectline\n+}\n+\n proc getblobdiffs {ids} {\n     global blobdifffd diffids env\n     global diffinhdr treediffs\n     global diffcontext\n     global ignorespace\n+    global firstparent\n     global limitdiffs vfilelimit curview\n     global diffencoding targetline diffnparents\n     global git_version currdiffsubmod\n@@ -7521,10 +7533,15 @@ proc getblobdiffs {ids} {\n     if {[package vcompare $git_version \"1.6.6\"] >= 0} {\n \tset submodule \"--submodule\"\n     }\n-    set cmd [diffcmd $ids \"-p $textconv $submodule  -C --cc --no-commit-id -U$diffcontext\"]\n+    set cmd [diffcmd $ids \"-p $textconv $submodule  -C --no-commit-id -U$diffcontext\"]\n     if {$ignorespace} {\n \tappend cmd \" -w\"\n     }\n+    if {$firstparent} {\n+\tappend cmd \" -m --first-parent\"\n+    } else {\n+\tappend cmd \" --cc\"\n+    }\n     if {$limitdiffs && $vfilelimit($curview) ne {}} {\n \tset cmd [concat $cmd -- $vfilelimit($curview)]\n     }\n@@ -11393,6 +11410,7 @@ if {[tk windowingsystem] eq \"win32\"} {\n set diffcolors {red \"#00a000\" blue}\n set diffcontext 3\n set ignorespace 0\n+set firstparent 0\n set markbgcolor \"#e0e0ff\"\n \n set circlecolors {white blue gray blue blue}\n-- \n1.7.3.2.153.g8250e\n"},{"id":"155392","messageId":"20101108172421.GB3619@burratino","threadId":"25668","inReplyTo":"1289212979-64246-1-git-send-email-lists@haller-berlin.de","subject":"Re: [PATCH] gitk: Add \"First parent\" checkbox","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-08T17:24:21Z","receivedAt":"2010-11-08T17:24:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Haller wrote:\n\n> Sometimes it's desirable to see what changes were introduced by a\n> merge commit, rather than how conflicts were resolved. This adds\n> a checkbox which, when turned on, makes gitk show the equivalent\n> of \"git show --first-parent <commit>\" for merge commits.\n\nTo be clear: this is a diff option (like -m but limited to one\nparent), not a history traversal option, right?\n\n> I realize this conflicts with Thomas Rast's recent patch to\n> add a word-diff dropdown box; things are fighting for space\n> in the diff pane header...\n\nAny ideas for addressing the space shortage?\n"},{"id":"155418","messageId":"1jrnlcv.1hnnh8x1sh3ydiM%lists@haller-berlin.de","threadId":"25668","inReplyTo":"20101108172421.GB3619@burratino","subject":"Re: [PATCH] gitk: Add \"First parent\" checkbox","fromName":"Stefan Haller","fromEmail":"lists@haller-berlin.de","sentAt":"2010-11-08T19:40:40Z","receivedAt":"2010-11-08T19:40:40Z","isPatch":true,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n> Stefan Haller wrote:\n> \n> > Sometimes it's desirable to see what changes were introduced by a\n> > merge commit, rather than how conflicts were resolved. This adds\n> > a checkbox which, when turned on, makes gitk show the equivalent\n> > of \"git show --first-parent <commit>\" for merge commits.\n> \n> To be clear: this is a diff option (like -m but limited to one\n> parent), not a history traversal option, right?\n\nYes, that's right.  There is an existing history traversal option called\n\"Limit to first parent\" in the \"Edit View\" dialog, which can be\ncontrolled with the --first-parent command line option.  Do you think\nthis will cause confusion?\n\n> > I realize this conflicts with Thomas Rast's recent patch to\n> > add a word-diff dropdown box; things are fighting for space\n> > in the diff pane header...\n> \n> Any ideas for addressing the space shortage?\n\nMatthieu made the suggestion of \"line-wrapping\" the widgets in the diff\npane header if it becomes too narrow.  I guess this could work; I don't\nknow enough Tcl/Tk to be able to try it out though.\n\n\n-- \nStefan Haller\nBerlin, Germany\nhttp://www.haller-berlin.de/\n"},{"id":"155419","messageId":"20101108194817.GC6348@burratino","threadId":"25668","inReplyTo":"1jrnlcv.1hnnh8x1sh3ydiM%lists@haller-berlin.de","subject":"Re: [PATCH] gitk: Add \"First parent\" checkbox","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-08T19:48:17Z","receivedAt":"2010-11-08T19:48:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Haller wrote:\n>> Stefan Haller wrote:\n\n>>> I realize this conflicts with Thomas Rast's recent patch to\n>>> add a word-diff dropdown box; things are fighting for space\n>>> in the diff pane header...\n[...]\n> Matthieu made the suggestion of \"line-wrapping\" the widgets in the diff\n> pane header if it becomes too narrow.  I guess this could work; I don't\n> know enough Tcl/Tk to be able to try it out though.\n\nHmm.  I don't like where this method tends in the limit.\n\nMaybe we need a notion of a \"diff view\", for setting up various\ndiff-tree options (word diff, whitespace options, context lines,\npatience, diffstat, first-parent)?  The diff pane could then provide a\ndrop-down box for diff views already set up.\n"},{"id":"155426","messageId":"1jrnp0m.116vp4ycag4blM%lists@haller-berlin.de","threadId":"25668","inReplyTo":"20101108194817.GC6348@burratino","subject":"Re: [PATCH] gitk: Add \"First parent\" checkbox","fromName":"Stefan Haller","fromEmail":"lists@haller-berlin.de","sentAt":"2010-11-08T20:23:44Z","receivedAt":"2010-11-08T20:23:44Z","isPatch":true,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n> Stefan Haller wrote:\n>\n> > Matthieu made the suggestion of \"line-wrapping\" the widgets in the diff\n> > pane header if it becomes too narrow.\n>\n> Hmm.  I don't like where this method tends in the limit.\n>\n> Maybe we need a notion of a \"diff view\", for setting up various\n> diff-tree options (word diff, whitespace options, context lines,\n> patience, diffstat, first-parent)?  The diff pane could then provide a\n> drop-down box for diff views already set up.\n\nI don't think I would like this.  Most of the diff related options are\nones that I want to toggle with one click.  That's certainly true for\n\"Ignore space change\", \"First parent\", and the \"Diff/Old Version/New\nVersion\" radio buttons.  I would hate to see any of them be removed from\nthe diff pane.\n\n\n-- \nStefan Haller\nBerlin, Germany\nhttp://www.haller-berlin.de/\n"},{"id":"155433","messageId":"20101108211101.GA13114@burratino","threadId":"25668","inReplyTo":"1jrnp0m.116vp4ycag4blM%lists@haller-berlin.de","subject":"Re: [PATCH] gitk: Add \"First parent\" checkbox","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-08T21:11:01Z","receivedAt":"2010-11-08T21:11:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Haller wrote:\n\n>                                   Most of the diff related options are\n> ones that I want to toggle with one click.  That's certainly true for\n> \"Ignore space change\", \"First parent\", and the \"Diff/Old Version/New\n> Version\" radio buttons.  I would hate to see any of them be removed from\n> the diff pane.\n\nThanks, that's useful information.  Could you give example workflows\nto illustrate this?  Here's an example:\n\n   Toggling between \"Diff/Old version/New version\" with a single click\n   allows one to compare the preimage and postimage of a patch visually.\n   It makes what changed much more obvious.\n\n   Historically these have been radio buttons on the top-left part of\n   the diff pane.  If they move, it might be hard for people already\n   familiar with gitk to find them.\n\n   The preimage/postimage toggle is a useful and novel feature, and it\n   is greedy with respect to screen real estate (a few extra diff\n   lines can go a long way), so it would not be good to hide it in a\n   separate \"more diff options\" toolbar.\n\nThe case for one-click \"diff against parent\" is less clear.  Is it\nthat it gets toggled in practice often (i.e., avoiding pointless\nrepetitive actions)?\n\nHope that helps.\n"},{"id":"155572","messageId":"1jrqe9r.1i2zca71746ggiM%lists@haller-berlin.de","threadId":"25668","inReplyTo":"20101108211101.GA13114@burratino","subject":"Re: [PATCH] gitk: Add \"First parent\" checkbox","fromName":"Stefan Haller","fromEmail":"lists@haller-berlin.de","sentAt":"2010-11-10T07:17:19Z","receivedAt":"2010-11-10T07:17:19Z","isPatch":true,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"Jonathan Nieder <jrnieder@gmail.com> wrote:\n\n> Stefan Haller wrote:\n> \n> >                                   Most of the diff related options are\n> > ones that I want to toggle with one click.  That's certainly true for\n> > \"Ignore space change\", \"First parent\", and the \"Diff/Old Version/New\n> > Version\" radio buttons.  I would hate to see any of them be removed from\n> > the diff pane.\n> \n> Thanks, that's useful information.  Could you give example workflows\n> to illustrate this?\n\nWhen browsing history, I usually have \"First parent\" off, so that I see\nempty diffs for most merges, except the ones that had conflicts.\nOccasionally though, when looking at a merge of a larger branch, I want\nto see what changes were introduced by the merge; turning on \"First\nparent\" allows me to quickly see that, or in general gives me an idea of\njust how \"big\" the merge was.  I usually turn it back off right\nafterwards.\n\nSimilar for \"Ignore space change\": I usually have it off most of the\ntime, so that I see faithful diffs.  Occasionally, when just the\nindentation of a block of C code has changed, I turn it on just for that\none diff to make it easier to read; then I turn it back off right\nafterwards.\n\nAlso similar for \"Lines of context\": this is usually set to a medium\nvalue, like 3 or 4; sometimes I'll increase it to 12 or more for a\nsingle diff to see more of the surrounding code, and sometimes I'll set\nit to 1 for a merge diff with hunks so close to each other that they\notherwise show as conflicts.\n\nYour proposal of having presets for diff options has the drawback that I\nwould need a preset for each combination of these options.\n\n-Stefan\n\n\n-- \nStefan Haller\nBerlin, Germany\nhttp://www.haller-berlin.de/\n"},{"id":"157876","messageId":"20101212042732.GA7296@brick.ozlabs.ibm.com","threadId":"25668","inReplyTo":"1289212979-64246-1-git-send-email-lists@haller-berlin.de","subject":"Re: [PATCH] gitk: Add \"First parent\" checkbox","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2010-12-12T04:27:32Z","receivedAt":"2010-12-12T04:27:32Z","isPatch":true,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"On Mon, Nov 08, 2010 at 11:42:59AM +0100, Stefan Haller wrote:\n> Sometimes it's desirable to see what changes were introduced by a\n> merge commit, rather than how conflicts were resolved. This adds\n> a checkbox which, when turned on, makes gitk show the equivalent\n> of \"git show --first-parent <commit>\" for merge commits.\n> \n> Signed-off-by: Stefan Haller <stefan@haller-berlin.de>\n> ---\n> I realize this conflicts with Thomas Rast's recent patch to\n> add a word-diff dropdown box; things are fighting for space\n> in the diff pane header...\n\nI just applied Thomas Rast's patch, so you'll need to rebase.  Also\nyou're right that we're running out of space; perhaps we need to make\nthe pane header two rows high.  Finally, \"First parent\" doesn't really\nconvey to me immediately what it does -- I have to think about it, so\nit will probably confuse new users.  I don't know what would be\nbetter, though.\n\nPaul.\n"},{"id":"157895","messageId":"1292184485-39351-1-git-send-email-lists@haller-berlin.de","threadId":"25668","inReplyTo":"20101212042732.GA7296@brick.ozlabs.ibm.com","subject":"[PATCH v2/RFC] gitk: Add \"First parent\" checkbox","fromName":"","fromEmail":"lists@haller-berlin.de","sentAt":"2010-12-12T20:08:05Z","receivedAt":"2010-12-12T20:08:05Z","isPatch":true,"sender":{"key":"lists@haller-berlin.de","avatar":null},"body":"From: Stefan Haller <lists@haller-berlin.de>\n\nSometimes it's desirable to see what changes were introduced by a\nmerge commit, rather than how conflicts were resolved. This adds\na checkbox which, when turned on, makes gitk show the equivalent\nof \"git show --first-parent <commit>\" for merge commits.\n\nSigned-off-by: Stefan Haller <stefan@haller-berlin.de>\n---\nPaul Mackerras <paulus@samba.org> wrote:\n\n> I just applied Thomas Rast's patch, so you'll need to rebase. \n\nOK, here's a new patch, rebased onto current master (but otherwise\nunchanged for now).\n\n> Also you're right that we're running out of space; perhaps we need to make\n> the pane header two rows high.\n\nThe suggestion was to make it two rows high only if it doesn't fit on\none row (i.e. dynamically \"line-wrap\"), and I like the idea.  Unfortunately\nthat's beyond my Tk skills; anybody willing to help?\n\n> Finally, \"First parent\" doesn't really convey to me immediately what it\n> does -- I have to think about it, so it will probably confuse new users.\n> I don't know what would be better, though.\n\nWhat I like about it is that it's consistent with the command-line\nclient, \"git show --first-parent\".  But I don't insist on it if anybody\nhas a better suggestion.\n\n gitk |   25 ++++++++++++++++++++++---\n 1 files changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/gitk b/gitk\nindex e82c6bf..7201ba0 100755\n--- a/gitk\n+++ b/gitk\n@@ -2269,6 +2269,10 @@ proc makewindow {} {\n \tpack .bleft.mid.worddiff -side left -padx 5\n     }\n \n+    ${NS}::checkbutton .bleft.mid.firstparent -text [mc \"First parent\"] \\\n+\t-command changefirstparent -variable firstparent\n+    pack .bleft.mid.firstparent -side left -padx 5\n+\n     set ctext .bleft.bottom.ctext\n     text $ctext -background $bgcolor -foreground $fgcolor \\\n \t-state disabled -font textfont \\\n@@ -6897,6 +6901,7 @@ proc selectline {l isnew {desired_loc {}}} {\n     global cmitmode showneartags allcommits\n     global targetrow targetid lastscrollrows\n     global autoselect jump_to_here\n+    global firstparent\n \n     catch {unset pending_select}\n     $canv delete hover\n@@ -7038,7 +7043,7 @@ proc selectline {l isnew {desired_loc {}}} {\n     init_flist [mc \"Comments\"]\n     if {$cmitmode eq \"tree\"} {\n \tgettree $id\n-    } elseif {[llength $olds] <= 1} {\n+    } elseif {[llength $olds] <= 1 || $firstparent} {\n \tstartdiff $id\n     } else {\n \tmergediff $id\n@@ -7442,7 +7447,7 @@ proc diffcmd {ids flags} {\n proc gettreediffs {ids} {\n     global treediff treepending\n \n-    if {[catch {set gdtf [open [diffcmd $ids {--no-commit-id}] r]}]} return\n+    if {[catch {set gdtf [open [diffcmd $ids {--no-commit-id -m --first-parent}] r]}]} return\n \n     set treepending $ids\n     set treediff {}\n@@ -7534,12 +7539,20 @@ proc changeworddiff {name ix op} {\n     reselectline\n }\n \n+proc changefirstparent {} {\n+    global treediffs\n+    catch {unset treediffs}\n+\n+    reselectline\n+}\n+\n proc getblobdiffs {ids} {\n     global blobdifffd diffids env\n     global diffinhdr treediffs\n     global diffcontext\n     global ignorespace\n     global worddiff\n+    global firstparent\n     global limitdiffs vfilelimit curview\n     global diffencoding targetline diffnparents\n     global git_version currdiffsubmod\n@@ -7552,13 +7565,18 @@ proc getblobdiffs {ids} {\n     if {[package vcompare $git_version \"1.6.6\"] >= 0} {\n \tset submodule \"--submodule\"\n     }\n-    set cmd [diffcmd $ids \"-p $textconv $submodule  -C --cc --no-commit-id -U$diffcontext\"]\n+    set cmd [diffcmd $ids \"-p $textconv $submodule  -C --no-commit-id -U$diffcontext\"]\n     if {$ignorespace} {\n \tappend cmd \" -w\"\n     }\n     if {$worddiff ne [mc \"Line diff\"]} {\n \tappend cmd \" --word-diff=porcelain\"\n     }\n+    if {$firstparent} {\n+\tappend cmd \" -m --first-parent\"\n+    } else {\n+\tappend cmd \" --cc\"\n+    }\n     if {$limitdiffs && $vfilelimit($curview) ne {}} {\n \tset cmd [concat $cmd -- $vfilelimit($curview)]\n     }\n@@ -11453,6 +11471,7 @@ set diffcolors {red \"#00a000\" blue}\n set diffcontext 3\n set ignorespace 0\n set worddiff \"\"\n+set firstparent 0\n set markbgcolor \"#e0e0ff\"\n \n set circlecolors {white blue gray blue blue}\n-- \n1.7.3.2.442.g97e50\n"}]}