{"thread":{"id":"28145","subject":"[PATCH RFC] gitk: Allow commit editing","startedAt":"2011-08-17T19:56:11Z","lastAt":"2011-08-19T14:40:31Z","messageCount":5,"participants":["Michal Sojka","Jeff King","Chris Packham"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"173692","messageId":"1313610971-1741-1-git-send-email-sojka@os.inf.tu-dresden.de","threadId":"28145","inReplyTo":null,"subject":"[PATCH RFC] gitk: Allow commit editing","fromName":"Michal Sojka","fromEmail":"sojka@os.inf.tu-dresden.de","sentAt":"2011-08-17T19:56:11Z","receivedAt":"2011-08-17T19:56:11Z","isPatch":true,"sender":{"key":"sojka@os.inf.tu-dresden.de","avatar":"https://gravatar.com/avatar/7aba1f13809fab0717a8c4299caa3ea599413c2028de580d1b736fc82bf10bfa?d=mp&s=160"},"body":"Hi, this is a proof of concept patch that allows editing of commits\nfrom gitk. I often review patches before pushing in gitk and if I\nwould like to have an easy way of fixing typos in commit messages etc.\n\nSo the patch adds \"Edit this commit\" item to gitk's context menu and\nthe actual editing is done by non-interactively invoking interactive\nrebase :-) and git gui.\n\nThere is almost no error checking etc., but before learning TCL and\ncoding this properly, I'd like to get some feedback.\n\n-Michal\n\nSigned-off-by: Michal Sojka <sojka@os.inf.tu-dresden.de>\n---\n gitk-git/gitk |   16 ++++++++++++++++\n 1 files changed, 16 insertions(+), 0 deletions(-)\n\ndiff --git a/gitk-git/gitk b/gitk-git/gitk\nindex 4cde0c4..121b926 100755\n--- a/gitk-git/gitk\n+++ b/gitk-git/gitk\n@@ -2497,6 +2497,7 @@ proc makewindow {} {\n \t{mc \"Return to mark\" command gotomark}\n \t{mc \"Find descendant of this and mark\" command find_common_desc}\n \t{mc \"Compare with marked commit\" command compare_commits}\n+\t{mc \"Edit this commit\" command edit_commit}\n     }\n     $rowctxmenu configure -tearoff 0\n \n@@ -9102,6 +9103,21 @@ proc cherrypick {} {\n     notbusy cherrypick\n }\n \n+proc edit_commit {} {\n+    global rowmenuid\n+\n+    nowbusy edit [mc \"Editing commit\"]\n+    if {[catch {exec sh -c \"(GIT_EDITOR='sed -ie 1s/^pick/edit/' git rebase -i $rowmenuid^ && git gui citool --amend && git rebase --continue) 2>&1\"} err]} {\n+\tnotbusy edit\n+\terror_popup $err\n+\texec git rebase --abort\n+\treturn\n+    }\n+    notbusy edit\n+    run updatecommits\n+}\n+\n+\n proc resethead {} {\n     global mainhead rowmenuid confirm_ok resettype NS\n \n-- \n1.7.5.4\n"},{"id":"173804","messageId":"20110818223346.GA8481@sigill.intra.peff.net","threadId":"28145","inReplyTo":"1313610971-1741-1-git-send-email-sojka@os.inf.tu-dresden.de","subject":"Re: [PATCH RFC] gitk: Allow commit editing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-18T22:33:47Z","receivedAt":"2011-08-18T22:33:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 17, 2011 at 09:56:11PM +0200, Michal Sojka wrote:\n\n> Hi, this is a proof of concept patch that allows editing of commits\n> from gitk. I often review patches before pushing in gitk and if I\n> would like to have an easy way of fixing typos in commit messages etc.\n> \n> So the patch adds \"Edit this commit\" item to gitk's context menu and\n> the actual editing is done by non-interactively invoking interactive\n> rebase :-) and git gui.\n\nInvoking rebase behind the scenes makes me very nervous. In particular:\n\n  1. There is nothing to indicate to the user that they are rewriting a\n     string of commits, which is going to wreak havoc if any of the\n     commits have been published elsewhere (either pushed somewhere, or\n     even present in another local branch). I.e., rebasing generally\n     needs to be a conscious decision of the user.\n\n     Yes, a veteran user who thinks about it will realize there is no\n     way to edit an old commit without rebasing, but I suspect relying\n     on that is not enough. There should probably be a prominent\n     warning at least the first time somebody uses this feature.\n\n  2. Even if you accept the hazard of rewriting commits, you don't pass\n     \"-p\" to rebase. So it will linearize your history. I don't know how\n     robust \"-p\" is these days, and if it's up to the challenge of\n     people using it to rebase potentially large segments of complex\n     history.\n\nSo I think your idea is sane, and if you use it appropriately (by\nediting commits in recent-ish linear stretches of history) your patch\nworks fine. But I really worry that it is going to be a problem for less\nclueful users to stumble across in the menu.\n\n-Peff\n"},{"id":"173848","messageId":"4E4E4C84.4030804@gmail.com","threadId":"28145","inReplyTo":"20110818223346.GA8481@sigill.intra.peff.net","subject":"Re: [PATCH RFC] gitk: Allow commit editing","fromName":"Chris Packham","fromEmail":"judge.packham@gmail.com","sentAt":"2011-08-19T11:44:04Z","receivedAt":"2011-08-19T11:44:04Z","isPatch":true,"sender":{"key":"judge.packham@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155667?v=4"},"body":"Hi,\n\nOn 19/08/11 10:33, Jeff King wrote:\n> On Wed, Aug 17, 2011 at 09:56:11PM +0200, Michal Sojka wrote:\n> \n>> Hi, this is a proof of concept patch that allows editing of commits\n>> from gitk. I often review patches before pushing in gitk and if I\n>> would like to have an easy way of fixing typos in commit messages etc.\n\nI've often _thought_ that I wanted something like this. As you say\nfiring up gitk to review the patch you're about to send (or commit\nyou're about to push) is probably a fairly common workflow.\n\n>>\n>> So the patch adds \"Edit this commit\" item to gitk's context menu and\n>> the actual editing is done by non-interactively invoking interactive\n>> rebase :-) and git gui.\n> \n> Invoking rebase behind the scenes makes me very nervous. In particular:\n> \n>   1. There is nothing to indicate to the user that they are rewriting a\n>      string of commits, which is going to wreak havoc if any of the\n>      commits have been published elsewhere (either pushed somewhere, or\n>      even present in another local branch). I.e., rebasing generally\n>      needs to be a conscious decision of the user.\n> \n>      Yes, a veteran user who thinks about it will realize there is no\n>      way to edit an old commit without rebasing, but I suspect relying\n>      on that is not enough. There should probably be a prominent\n>      warning at least the first time somebody uses this feature.\n> \n>   2. Even if you accept the hazard of rewriting commits, you don't pass\n>      \"-p\" to rebase. So it will linearize your history. I don't know how\n>      robust \"-p\" is these days, and if it's up to the challenge of\n>      people using it to rebase potentially large segments of complex\n>      history.\n> \n> So I think your idea is sane, and if you use it appropriately (by\n> editing commits in recent-ish linear stretches of history) your patch\n> works fine. But I really worry that it is going to be a problem for less\n> clueful users to stumble across in the menu.\n> \n> -Peff\n\nAnd as Peff points out there are some major caveats about how far back\nyou would want to let people edit. I suspect you could come up with some\nsane rules (e.g. only allow editing the last commit, the last N commits,\nonly commits that are not present in remote X). Whatever rules you come\nup with to protect people I think you'll end up frustrating them as well\n(why can I edit this commit but not that one).\n\nOne thing I've thought about (but don't know enough TCL to begin to\nimplement) is a graphical rebase front end. I often use git gui to make\ntweaks to the last commit (message and content) so why not extend that\nto a rebase operation. I think that might address some of Peffs concerns\nbecause the user would be invoking something specifically intended for\nrebasing and accepts all the caveats that go along with that.\n\nAnyway that's my 2c\n--\nChris\n"},{"id":"173857","messageId":"87sjoxv835.fsf@steelpick.2x.cz","threadId":"28145","inReplyTo":"4E4E4C84.4030804@gmail.com","subject":"Re: [PATCH RFC] gitk: Allow commit editing","fromName":"Michal Sojka","fromEmail":"sojka@os.inf.tu-dresden.de","sentAt":"2011-08-19T13:34:06Z","receivedAt":"2011-08-19T13:34:06Z","isPatch":true,"sender":{"key":"sojka@os.inf.tu-dresden.de","avatar":"https://gravatar.com/avatar/7aba1f13809fab0717a8c4299caa3ea599413c2028de580d1b736fc82bf10bfa?d=mp&s=160"},"body":"On Fri, 19 Aug 2011, Chris Packham wrote:\n> One thing I've thought about (but don't know enough TCL to begin to\n> implement) is a graphical rebase front end. I often use git gui to make\n> tweaks to the last commit (message and content) so why not extend that\n> to a rebase operation. I think that might address some of Peffs concerns\n> because the user would be invoking something specifically intended for\n> rebasing and accepts all the caveats that go along with that.\n\nHi Chris,\n\nthe version of the patch below supports not only editing of commit\nmessage but also of the commit itself. There is an easy way to split\ncommits and to remove lines/hunks form the commits. Additionally, with\nthe help of editor and \"Rescan\" button in git gui, you can add things to\nthe commits.\n\nI think that in the similar way as in this patch, it would be easy to\nallow gitk doing fixup and squash operations offered by rebase.\n\n-Michal\n\n--8<---------------cut here---------------start------------->8---\nFrom 1d0f0a778afbaeb928cdecb3f18065757b3aa2fa Mon Sep 17 00:00:00 2001\nFrom: Michal Sojka <sojka@os.inf.tu-dresden.de>\nDate: Fri, 19 Aug 2011 15:21:47 +0200\nSubject: [PATCH] gitk: Allow commit editing\n\nI often use gitk to review patches before pushing them and would like\nto have an easy way of fixing typos in commit messages. The current\napproach with copying the commitid, switching to terminal, invoking\ngit rebase -i, editing the commit and switching back to gitk is a way\ntoo complicated just for changing a single letter in commit message or\nremove a debug printf().\n\nThis patch adds \"Edit this commit\" item to gitk's context menu which\ninvokes interactive rebase in a non-interactive way :-). git gui is\nused to actually edit the commit.\n\nSplitting of commits (as described in git-rebase(1)) is also supported.\n\nThe user is warned if the commit that is going to be edited is\ncontained in a remote branch.\n\nSigned-off-by: Michal Sojka <sojka@os.inf.tu-dresden.de>\n---\n gitk-git/gitk |   40 ++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 40 insertions(+), 0 deletions(-)\n\ndiff --git a/gitk-git/gitk b/gitk-git/gitk\nindex 4cde0c4..432ca9b 100755\n--- a/gitk-git/gitk\n+++ b/gitk-git/gitk\n@@ -2487,6 +2487,7 @@ proc makewindow {} {\n     makemenu $rowctxmenu {\n \t{mc \"Diff this -> selected\" command {diffvssel 0}}\n \t{mc \"Diff selected -> this\" command {diffvssel 1}}\n+\t{mc \"Edit this commit\" command edit_commit}\n \t{mc \"Make patch\" command mkpatch}\n \t{mc \"Create tag\" command mktag}\n \t{mc \"Write commit to file\" command writecommit}\n@@ -9102,6 +9103,45 @@ proc cherrypick {} {\n     notbusy cherrypick\n }\n \n+proc edit_commit {} {\n+    global rowmenuid selectedline\n+\n+    if {[exec git branch -r --contains=$rowmenuid] ne {}} {\n+\tif {![confirm_popup [mc \"The commit you are going to edit is contained in at least\\\n+\t\t\t\t one remote branch. It is a bad idea to change a branch that is\\\n+\t\t\t\t possibly used by other people. See git-rebase(1) for details.\\n\\n\\\n+\t\t\t\t Do you want to continue?\"]]} return }\n+\n+    nowbusy edit [mc \"Editing commit\"]\n+    if {[catch {exec sh -c \"(GIT_EDITOR='sed -ie 1s/^pick/edit/' git rebase -p -i $rowmenuid^ && git gui citool --amend) 2>&1\"} err]} {\n+\tnotbusy edit\n+\terror_popup $err\n+\texec git rebase --abort\n+\treturn\n+    }\n+    set newcommit [exec git rev-parse HEAD]\n+    while {[catch {exec sh -c \"git diff-index --quiet --cached HEAD && git diff-files --quiet\"} err]} {\n+\tif {[confirm_popup [mc \"There are uncommited changes in the working tree or in the index.\\\n+\t\t\t\tDo you want to create a new commit (OK) or throw them away (Cancel)?\"]]} {\n+\t    catch {exec git gui citool} err;\n+\t    # In case of error (i.e. the user did not commit anything), we just ask him again\n+\t} else {\n+\t    exec git reset --hard\n+\t}\n+    }\n+    if {[catch {exec sh -c \"git rebase --continue 2>&1\"} err]} {\n+\tnotbusy edit\n+\terror_popup $err\n+\texec git rebase --abort\n+\treturn\n+    }\n+    updatecommits\n+    # XXX How to select the edited commit? This doesn't work.\n+    selbyid $newcommit\n+    notbusy edit\n+}\n+\n+\n proc resethead {} {\n     global mainhead rowmenuid confirm_ok resettype NS\n \n-- \n1.7.5.4\n\n--8<---------------cut here---------------end--------------->8---\n"},{"id":"173860","messageId":"87pqk1v50g.fsf@steelpick.2x.cz","threadId":"28145","inReplyTo":"87sjoxv835.fsf@steelpick.2x.cz","subject":"Re: [PATCH RFC] gitk: Allow commit editing","fromName":"Michal Sojka","fromEmail":"sojka@os.inf.tu-dresden.de","sentAt":"2011-08-19T14:40:31Z","receivedAt":"2011-08-19T14:40:31Z","isPatch":true,"sender":{"key":"sojka@os.inf.tu-dresden.de","avatar":"https://gravatar.com/avatar/7aba1f13809fab0717a8c4299caa3ea599413c2028de580d1b736fc82bf10bfa?d=mp&s=160"},"body":"On Fri, 19 Aug 2011, Michal Sojka wrote:\n> On Fri, 19 Aug 2011, Chris Packham wrote:\n> > One thing I've thought about (but don't know enough TCL to begin to\n> > implement) is a graphical rebase front end. I often use git gui to make\n> > tweaks to the last commit (message and content) so why not extend that\n> > to a rebase operation. I think that might address some of Peffs concerns\n> > because the user would be invoking something specifically intended for\n> > rebasing and accepts all the caveats that go along with that.\n> \n> Hi Chris,\n> \n> the version of the patch below supports not only editing of commit\n> message but also of the commit itself. There is an easy way to split\n> commits and to remove lines/hunks form the commits. Additionally, with\n> the help of editor and \"Rescan\" button in git gui, you can add things to\n> the commits.\n> \n> I think that in the similar way as in this patch, it would be easy to\n> allow gitk doing fixup and squash operations offered by rebase.\n\nHere is another proof of concept, that gitk can also fold commits\ntogether. It applies on top of the previously sent patch.\n\nThis operation is probably even more dangerous then simple edit because\nit reorders commits. Does anybody have an idea which checks should be\nemployed in order to warn users about unexpected results?\n\n-Michal\n\n--8<---------------cut here---------------start------------->8---\nFrom 5943dfcd9738f8da9d159f912302cfbb2b4d35b5 Mon Sep 17 00:00:00 2001\nFrom: Michal Sojka <sojka@os.inf.tu-dresden.de>\nDate: Fri, 19 Aug 2011 16:33:55 +0200\nSubject: [PATCH 2/2] gitk: Learn how to fixup commits\n\n\nSigned-off-by: Michal Sojka <sojka@os.inf.tu-dresden.de>\n---\n gitk-git/gitk |   41 ++++++++++++++++++++++++++++++++++++-----\n 1 files changed, 36 insertions(+), 5 deletions(-)\n\ndiff --git a/gitk-git/gitk b/gitk-git/gitk\nindex 432ca9b..53fa51d 100755\n--- a/gitk-git/gitk\n+++ b/gitk-git/gitk\n@@ -2488,6 +2488,7 @@ proc makewindow {} {\n \t{mc \"Diff this -> selected\" command {diffvssel 0}}\n \t{mc \"Diff selected -> this\" command {diffvssel 1}}\n \t{mc \"Edit this commit\" command edit_commit}\n+\t{mc \"Fold this into selected (fixup)\" command fixup_commit}\n \t{mc \"Make patch\" command mkpatch}\n \t{mc \"Create tag\" command mktag}\n \t{mc \"Write commit to file\" command writecommit}\n@@ -8448,6 +8449,7 @@ proc rowmenu {x y id} {\n     $menu entryconfigure [mca \"Diff this -> selected\"] -state $state\n     $menu entryconfigure [mca \"Diff selected -> this\"] -state $state\n     $menu entryconfigure [mca \"Make patch\"] -state $state\n+    $menu entryconfigure [mca \"Fold this into selected (fixup)\"] -state $state\n     tk_popup $menu $x $y\n }\n \n@@ -9103,14 +9105,24 @@ proc cherrypick {} {\n     notbusy cherrypick\n }\n \n+proc rebase_ok {id} {\n+    if {[exec git branch -r --contains=$id] ne {}} {\n+\tif { [confirm_popup [mc \"The commit you are going to edit is contained in at least\\\n+\t\t\t\t one remote branch. It is a bad idea to change a branch that is\\\n+\t\t\t\t possibly used by other people. See git-rebase(1) for details.\\n\\n\\\n+\t\t\t\t Do you want to continue?\"]]} {\n+\t    return 1\n+\t} else {\n+\t    return 0\n+\t}\n+    }\n+    return 1\n+}\n+\n proc edit_commit {} {\n     global rowmenuid selectedline\n \n-    if {[exec git branch -r --contains=$rowmenuid] ne {}} {\n-\tif {![confirm_popup [mc \"The commit you are going to edit is contained in at least\\\n-\t\t\t\t one remote branch. It is a bad idea to change a branch that is\\\n-\t\t\t\t possibly used by other people. See git-rebase(1) for details.\\n\\n\\\n-\t\t\t\t Do you want to continue?\"]]} return }\n+    if {![rebase_ok $rowmenuid]} return\n \n     nowbusy edit [mc \"Editing commit\"]\n     if {[catch {exec sh -c \"(GIT_EDITOR='sed -ie 1s/^pick/edit/' git rebase -p -i $rowmenuid^ && git gui citool --amend) 2>&1\"} err]} {\n@@ -9141,6 +9153,25 @@ proc edit_commit {} {\n     notbusy edit\n }\n \n+proc fixup_commit {} {\n+    global rowmenuid selectedline\n+    set selectedid [commitonrow $selectedline]\n+    set baseid [exec git merge-base $selectedid $rowmenuid]\n+\n+    if {![rebase_ok $baseid]} return\n+\n+    set this [string range $rowmenuid 0 6]\n+    set selected [string range $selectedid  0 6]\n+    nowbusy fixup\n+    if {[catch {exec sh -c \"GIT_EDITOR=\\\"sed -i -e '/^pick $selected/a fixup $this' -e '/^pick $this/d'\\\" git rebase -p -i $baseid^ 2>&1\"} err]} {\n+\tnotbusy fixup\n+\terror_popup $err\n+\texec git rebase --abort\n+\treturn\n+    }\n+    updatecommits\n+    notbusy fixup\n+}\n \n proc resethead {} {\n     global mainhead rowmenuid confirm_ok resettype NS\n-- \n1.7.5.4\n\n--8<---------------cut here---------------end--------------->8---\n"}]}