{"thread":{"id":"21380","subject":"[PATCH] gitk: Use the --submodule option for diffs","startedAt":"2009-10-27T14:59:21Z","lastAt":"2009-11-03T11:32:16Z","messageCount":6,"participants":["Jens Lehmann","Johannes Schindelin","Paul Mackerras","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"126024","messageId":"4AE70AC9.6040302@web.de","threadId":"21380","inReplyTo":null,"subject":"[PATCH] gitk: Use the --submodule option for diffs","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2009-10-27T14:59:21Z","receivedAt":"2009-10-27T14:59:21Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Instead of just showing not-quite-helpful SHA-1 pairs display the first\nlines of the corresponding commit messages in the submodule (similar to\nthe output of 'git submodule summary').\n\nSigned-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n---\n\nThis patch applies to 'next' and uses the new --submodule option of git\ndiff to achieve a more meaningful output of submodule differences in\ngitk.\n\nAny objections against making this the default?\n\n(for those interested: a version of git gui with similar functionality\nis available in 'master' of Shawn's repo but not yet merged).\n\n\n gitk-git/gitk |   23 +++++++++++++++++++----\n 1 files changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/gitk-git/gitk b/gitk-git/gitk\nindex a0214b7..5e2572d 100644\n--- a/gitk-git/gitk\n+++ b/gitk-git/gitk\n@@ -7207,7 +7207,7 @@ proc diffcmd {ids flags} {\n     if {$i >= 0} {\n \tif {[llength $ids] > 1 && $j < 0} {\n \t    # comparing working directory with some specific revision\n-\t    set cmd [concat | git diff-index $flags]\n+\t    set cmd [concat | git diff-index --submodule $flags]\n \t    if {$i == 0} {\n \t\tlappend cmd -R [lindex $ids 1]\n \t    } else {\n@@ -7215,13 +7215,13 @@ proc diffcmd {ids flags} {\n \t    }\n \t} else {\n \t    # comparing working directory with index\n-\t    set cmd [concat | git diff-files $flags]\n+\t    set cmd [concat | git diff-files --submodule $flags]\n \t    if {$j == 1} {\n \t\tlappend cmd -R\n \t    }\n \t}\n     } elseif {$j >= 0} {\n-\tset cmd [concat | git diff-index --cached $flags]\n+\tset cmd [concat | git diff-index --cached --submodule $flags]\n \tif {[llength $ids] > 1} {\n \t    # comparing index with specific revision\n \t    if {$i == 0} {\n@@ -7234,7 +7234,7 @@ proc diffcmd {ids flags} {\n \t    lappend cmd HEAD\n \t}\n     } else {\n-\tset cmd [concat | git diff-tree -r $flags $ids]\n+\tset cmd [concat | git diff-tree -r --submodule $flags $ids]\n     }\n     return $cmd\n }\n@@ -7481,6 +7481,21 @@ proc getblobdiffline {bdf ids} {\n \t    set diffnparents [expr {[string length $ats] - 1}]\n \t    set diffinhdr 0\n\n+\t} elseif {![string compare -length 10 \"Submodule \" $line]} {\n+\t    # start of a new submodule\n+\t    if {[string compare [$ctext get \"end - 4c\" end] \"\\n \\n\\n\"]} {\n+\t\t$ctext insert end \"\\n\";     # Add newline after commit message\n+\t    }\n+\t    set curdiffstart [$ctext index \"end - 1c\"]\n+\t    lappend ctext_file_names \"\"\n+\t    set fname [string range $line 10 [expr [string last \" \" $line] - 1]]\n+\t    lappend ctext_file_lines $fname\n+\t    makediffhdr $fname $ids\n+\t    $ctext insert end \"\\n$line\\n\" filesep\n+\t} elseif {![string compare -length 3 \"  >\" $line]} {\n+\t    $ctext insert end \"$line\\n\" dresult\n+\t} elseif {![string compare -length 3 \"  <\" $line]} {\n+\t    $ctext insert end \"$line\\n\" d0\n \t} elseif {$diffinhdr} {\n \t    if {![string compare -length 12 \"rename from \" $line]} {\n \t\tset fname [string range $line [expr 6 + [string first \" from \" $line] ] end]\n-- \n1.6.5.2.182.g338ab.dirty\n"},{"id":"126028","messageId":"alpine.DEB.1.00.0910271640540.11562@felix-maschine","threadId":"21380","inReplyTo":"4AE70AC9.6040302@web.de","subject":"Re: [PATCH] gitk: Use the --submodule option for diffs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-27T15:41:31Z","receivedAt":"2009-10-27T15:41:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 27 Oct 2009, Jens Lehmann wrote:\n\n> Instead of just showing not-quite-helpful SHA-1 pairs display the first\n> lines of the corresponding commit messages in the submodule (similar to\n> the output of 'git submodule summary').\n> \n> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n> ---\n> \n> This patch applies to 'next' and uses the new --submodule option of git\n> diff to achieve a more meaningful output of submodule differences in\n> gitk.\n> \n> Any objections against making this the default?\n\nCertainly not from me.  It would help people in my day-job project.\n\nCiao,\nDscho\n"},{"id":"126082","messageId":"19175.49800.697048.349753@drongo.ozlabs.ibm.com","threadId":"21380","inReplyTo":"4AE70AC9.6040302@web.de","subject":"Re: [PATCH] gitk: Use the --submodule option for diffs","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2009-10-28T04:03:20Z","receivedAt":"2009-10-28T04:03:20Z","isPatch":true,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"Jens Lehmann writes:\n\n> Instead of just showing not-quite-helpful SHA-1 pairs display the first\n> lines of the corresponding commit messages in the submodule (similar to\n> the output of 'git submodule summary').\n> \n> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n> ---\n> \n> This patch applies to 'next' and uses the new --submodule option of git\n> diff to achieve a more meaningful output of submodule differences in\n> gitk.\n\n(That sentence should have been in the commit message.)\n\n> Any objections against making this the default?\n\nWhat version of git is the first to have the --submodule option?\nSince it's a new option we should make gitk use it only if the\nunderlying git is new enough, like we do with the --textconv option.\n\nAlso, the commit message is a bit sparse; it doesn't mention that this\naffects the diff display, for instance.\n\nPaul.\n"},{"id":"126106","messageId":"7v7hugrnmy.fsf@alter.siamese.dyndns.org","threadId":"21380","inReplyTo":"4AE70AC9.6040302@web.de","subject":"Re: [PATCH] gitk: Use the --submodule option for diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-28T07:19:49Z","receivedAt":"2009-10-28T07:19:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> Instead of just showing not-quite-helpful SHA-1 pairs display the first\n> lines of the corresponding commit messages in the submodule (similar to\n> the output of 'git submodule summary').\n>\n> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n> ---\n>\n> This patch applies to 'next' and uses the new --submodule option of git\n> diff to achieve a more meaningful output of submodule differences in\n> gitk.\n>\n> Any objections against making this the default?\n\nIt looks like you are not making this the default, but are making it\nmandatory.  That's quite different.\n\nAs long as gitk ships with matching version of git, I do not think it is a\nhuge problem to force \"diff\" to always run with --submodule option, but if\nthat is what the patch does, I'd prefer to see the patch says so, instead\nof giving a false impression that there may be a way to disable it if one\nwants to.\n\nLooking at the patched text, I had to wonder where these $flags come from.\nThe callers of \"diffcmd\" give it to you, and I am not sure if all of them\nwant -p format diffs.  Specifically, what does gettreediffs do?  Does it\nmake sense to give --submodule to that invocation?\n\nYes, I know --submodule happens to be a no-op in non-patch format diffs,\nbut I do not think that is by design.  It is something somebody may notice\nand correct it later, at which time this patch will be broken.\n\nI also suspect that this might break getpatchid, but as long as all the\npatches consistently change/break ids it would be Ok.\n"},{"id":"126126","messageId":"4AE82DBD.5070202@web.de","threadId":"21380","inReplyTo":"19175.49800.697048.349753@drongo.ozlabs.ibm.com","subject":"[PATCH v2] gitk: Use the --submodule option for displaying diffs when available","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2009-10-28T11:40:45Z","receivedAt":"2009-10-28T11:40:45Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Instead of just showing not-quite-helpful SHA-1 pairs in the diff display\nof a submodule gitk will display the first lines of the corresponding\ncommit messages (similar to the output of 'git submodule summary') when\nthe underlying git installation supports this. That makes it much easier\nto evaluate the changes, as it eliminates the need to start a gitk inside\nthe submodule and use the superprojects hashes there to find out what the\ncommits are about.\n\nThis patch applies to 'next' - which will be 1.6.6 or 1.7.0 when merged -\nand uses the new --submodule option of git diff when a version of 1.6.6 or\nhigher is detected to achieve a more human readable output of submodule\ndifferences in gitk.\n\nSigned-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n---\n\n\nThanks for the review of the first version of this patch. I tried to\naddress all the issues raised and moved the additions of the --submodule\nflag to the getblobdiffs procedure, where the --textconv flag is handled\ntoo.\n\n\n gitk-git/gitk |   21 ++++++++++++++++++++-\n 1 files changed, 20 insertions(+), 1 deletions(-)\n\ndiff --git a/gitk-git/gitk b/gitk-git/gitk\nindex a0214b7..b06cac7 100644\n--- a/gitk-git/gitk\n+++ b/gitk-git/gitk\n@@ -7343,7 +7343,11 @@ proc getblobdiffs {ids} {\n     if {[package vcompare $git_version \"1.6.1\"] >= 0} {\n \tset textconv \"--textconv\"\n     }\n-    set cmd [diffcmd $ids \"-p $textconv -C --cc --no-commit-id -U$diffcontext\"]\n+    set submodule {}\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     if {$ignorespace} {\n \tappend cmd \" -w\"\n     }\n@@ -7481,6 +7485,21 @@ proc getblobdiffline {bdf ids} {\n \t    set diffnparents [expr {[string length $ats] - 1}]\n \t    set diffinhdr 0\n\n+\t} elseif {![string compare -length 10 \"Submodule \" $line]} {\n+\t    # start of a new submodule\n+\t    if {[string compare [$ctext get \"end - 4c\" end] \"\\n \\n\\n\"]} {\n+\t\t$ctext insert end \"\\n\";     # Add newline after commit message\n+\t    }\n+\t    set curdiffstart [$ctext index \"end - 1c\"]\n+\t    lappend ctext_file_names \"\"\n+\t    set fname [string range $line 10 [expr [string last \" \" $line] - 1]]\n+\t    lappend ctext_file_lines $fname\n+\t    makediffhdr $fname $ids\n+\t    $ctext insert end \"\\n$line\\n\" filesep\n+\t} elseif {![string compare -length 3 \"  >\" $line]} {\n+\t    $ctext insert end \"$line\\n\" dresult\n+\t} elseif {![string compare -length 3 \"  <\" $line]} {\n+\t    $ctext insert end \"$line\\n\" d0\n \t} elseif {$diffinhdr} {\n \t    if {![string compare -length 12 \"rename from \" $line]} {\n \t\tset fname [string range $line [expr 6 + [string first \" from \" $line] ] end]\n-- \n1.6.5.2.182.g3d976.dirty\n"},{"id":"126644","messageId":"19184.5312.712632.894685@cargo.ozlabs.ibm.com","threadId":"21380","inReplyTo":"4AE82DBD.5070202@web.de","subject":"Re: [PATCH v2] gitk: Use the --submodule option for displaying diffs when available","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2009-11-03T11:32:16Z","receivedAt":"2009-11-03T11:32:16Z","isPatch":true,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"Jens Lehmann writes:\n\n> Instead of just showing not-quite-helpful SHA-1 pairs in the diff display\n> of a submodule gitk will display the first lines of the corresponding\n> commit messages (similar to the output of 'git submodule summary') when\n> the underlying git installation supports this. That makes it much easier\n> to evaluate the changes, as it eliminates the need to start a gitk inside\n> the submodule and use the superprojects hashes there to find out what the\n> commits are about.\n> \n> This patch applies to 'next' - which will be 1.6.6 or 1.7.0 when merged -\n> and uses the new --submodule option of git diff when a version of 1.6.6 or\n> higher is detected to achieve a more human readable output of submodule\n> differences in gitk.\n> \n> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n\nThanks, applied.\n\nPaul.\n"}]}