{"thread":{"id":"23986","subject":"[RFC BANDAID PATCH] gitk: Don't add missing parents to display list when pickaxing","startedAt":"2010-06-02T20:32:40Z","lastAt":"2010-06-02T20:32:40Z","messageCount":1,"participants":["Damien Diederen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"142838","messageId":"87ljaxcffb.fsf@mini.crosstwine.com","threadId":"23986","inReplyTo":null,"subject":"[RFC BANDAID PATCH] gitk: Don't add missing parents to display list when pickaxing","fromName":"Damien Diederen","fromEmail":"dd@crosstwine.com","sentAt":"2010-06-02T20:32:40Z","receivedAt":"2010-06-02T20:32:40Z","isPatch":true,"sender":{"key":"dd@crosstwine.com","avatar":null},"body":"\nGitk unconditionally prepends --parents --boundary to the\nuser-provided argument list, and relies on boundary commits to\n\"neatly\" close history lines.\n\nGit log's '-S' option, however, interacts badly with --boundary as it\nis implemented as a post-processing filter of the object graph.\nResults coming from a common sub-graph end up producing no boundary,\nyet may appear disjoint as no parent rewriting occurs.  In git.git:\n\n    $ git log -SGMail --oneline --parents --boundary\n    e498257 bd7440f Documentation/SubmittingPatches: clarify GMail section and SMTP\n    df5753c a751b5c SubmittingPatches: update GMail section\n    50dffd4 28001d0 Google has renamed the imap folder\n\nThis confuses gitk, which truncates its \"display order\"\nlist (containing the six commits he knows about--three plus three\nparents) to a three-row display:\n\n    @ Documentation/SubmittingPatches: clarify GMail section and SMTP\n    |\n    O show-branch: use DEFAULT_ABBREV instead of 7\n\n    @ SubmittingPatches: update GMail section\n\nNote how it picked up \"show-branch: ...\" (bd7440f) as a\npseudo-boundary of e498257, marking it with an \"empty\" bullet, but\nthen stopped prematurely--missing one of the matching commits!\n\nThis bandaid patch cause gitk not to try to \"close\" the arcs when\npickaxe is in effect.  \"Missing parents\" are not added to the display\norder list, whose length then matches the number of rows.  It produces\nthe following imperfect display, which has the advantage of not\nomitting any result:\n\n    @ Documentation/SubmittingPatches: clarify GMail section and SMTP\n    |\n    | @ SubmittingPatches: update GMail section\n    | |\n    v v @ Google has renamed the imap folder\n\nThe parallel history lines are incorrect, and the arrows don't point\nanywhere.  Fixing this would require more extensive changes to gitk's\ndisplay engine, and possibly invalidate some of the invariants on\nwhich it currently relies.\n\nPreliminary investigation suggests that adding \"proper\" --boundary\nsupport to pickaxe would require to:\n\n 1. disable rev_info->boundary when producing the revision list;\n\n 2. manually maintain and trim rev_info->boundary_commits in\n    cmd_log_walk according to the return value of log_tree_commit;\n\n 3. perform a second pass of 'log_tree_commit' on the \"fake\" boundary,\n    with pickaxe filtering disabled.\n\nThis seems to be a superior solution, but is not too pretty, and I may\nbe missing something (besides the fact that it slightly changes the\ndefinition of boundary).  It may also fix --graph's current behaviour,\nwhich is, err..., interesting.\n\nI have not looked into parent rewriting, which may be the correct\nthing to do.  Note, however, that it seems it would not match the way\ne.g. 'gitk --grep=GMail' works--at a first glance.\n\nComments?  Suggestions?\n\n---\n gitk |   12 ++++++++++--\n 1 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/gitk b/gitk\nindex 45e3380..5cd3787 100755\n--- a/gitk\n+++ b/gitk\n@@ -130,7 +130,7 @@ proc unmerged_files {files} {\n }\n \n proc parseviewargs {n arglist} {\n-    global vdatemode vmergeonly vflags vdflags vrevs vfiltered vorigargs env\n+    global vdatemode vmergeonly vflags vdflags vrevs vfiltered vnoboundary vorigargs env\n \n     set vdatemode($n) 0\n     set vmergeonly($n) 0\n@@ -141,6 +141,7 @@ proc parseviewargs {n arglist} {\n     set origargs $arglist\n     set allknown 1\n     set filtered 0\n+    set noboundary 0\n     set i -1\n     foreach arg $arglist {\n \tincr i\n@@ -194,6 +195,9 @@ proc parseviewargs {n arglist} {\n \t\t# These mean that we get a subset of the commits\n \t\tset filtered 1\n \t\tlappend glflags $arg\n+\t\tif {[string match \"-S*\" $arg]} {\n+\t\t    set noboundary 1\n+\t\t}\n \t    }\n \t    \"-n\" {\n \t\t# This appears to be the only one that has a value as a\n@@ -237,6 +241,7 @@ proc parseviewargs {n arglist} {\n     set vflags($n) $glflags\n     set vrevs($n) $revargs\n     set vfiltered($n) $filtered\n+    set vnoboundary($n) $noboundary\n     set vorigargs($n) $origargs\n     return $allknown\n }\n@@ -1360,6 +1365,7 @@ proc getcommitlines {fd inst view updating}  {\n     global parents children curview hlview\n     global idpending ordertok\n     global varccommits varcid varctok vtokmod vfilelimit\n+    global vnoboundary\n \n     set stuff [read $fd 500000]\n     # git log doesn't terminate the last commit with a null...\n@@ -1401,7 +1407,9 @@ proc getcommitlines {fd inst view updating}  {\n \t    set viewcomplete($view) 1\n \t    # Check if we have seen any ids listed as parents that haven't\n \t    # appeared in the list\n-\t    closevarcs $view\n+\t    if {!$vnoboundary($view)} {\n+\t\tclosevarcs $view\n+\t    }\n \t    notbusy $view\n \t}\n \tif {$view == $curview} {\n-- \n1.7.1\n"}]}