{"thread":{"id":"28163","subject":"[PATCH] gitk: Allow commit editing","startedAt":"2011-08-19T12:23:39Z","lastAt":"2011-09-13T23:11:24Z","messageCount":10,"participants":["Michal Sojka","Jeff King","Paul Mackerras"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"173854","messageId":"87obzlwpx0.fsf@steelpick.2x.cz","threadId":"28163","inReplyTo":"20110818223346.GA8481@sigill.intra.peff.net","subject":"Re: [PATCH RFC] gitk: Allow commit editing","fromName":"Michal Sojka","fromEmail":"sojka@os.inf.tu-dresden.de","sentAt":"2011-08-19T12:23:39Z","receivedAt":"2011-08-19T12:23:39Z","isPatch":true,"sender":{"key":"sojka@os.inf.tu-dresden.de","avatar":"https://gravatar.com/avatar/7aba1f13809fab0717a8c4299caa3ea599413c2028de580d1b736fc82bf10bfa?d=mp&s=160"},"body":"On Thu, 18 Aug 2011, 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> > \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\nI added a warning if the edited commit is contained in a remote branch.\nWould you consider this sufficient?\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\nI added \"-p\" to rebase. I do not know about the robustness of \"-p\" as\nwell, but is anything goes wrong during the rebase, git rebase --abort\nhopefully reverts everything.\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\nSee the next version of the patch in a follow-up mail.\n\n-Michal\n"},{"id":"173853","messageId":"1313756753-26498-1-git-send-email-sojka@os.inf.tu-dresden.de","threadId":"28163","inReplyTo":"87obzlwpx0.fsf@steelpick.2x.cz","subject":"[PATCH] gitk: Allow commit editing","fromName":"Michal Sojka","fromEmail":"sojka@os.inf.tu-dresden.de","sentAt":"2011-08-19T12:25:53Z","receivedAt":"2011-08-19T12:25:53Z","isPatch":true,"sender":{"key":"sojka@os.inf.tu-dresden.de","avatar":"https://gravatar.com/avatar/7aba1f13809fab0717a8c4299caa3ea599413c2028de580d1b736fc82bf10bfa?d=mp&s=160"},"body":"I 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 if copying the commitid, switching to terminal, invoking git\nrebase -i, editing the commit and switching back to gitk is a way too\ncomplicated just for chaning a single letter.\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\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 |   31 +++++++++++++++++++++++++++++++\n 1 files changed, 31 insertions(+), 0 deletions(-)\n\ndiff --git a/gitk-git/gitk b/gitk-git/gitk\nindex 4cde0c4..83b3307 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,36 @@ 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 appears in at least one\\\n+\t\t\t\t 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+    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"},{"id":"174207","messageId":"20110825030755.GA3465@sigill.intra.peff.net","threadId":"28163","inReplyTo":"87obzlwpx0.fsf@steelpick.2x.cz","subject":"Re: [PATCH RFC] gitk: Allow commit editing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-25T03:07:55Z","receivedAt":"2011-08-25T03:07:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 19, 2011 at 02:23:39PM +0200, Michal Sojka wrote:\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> I added a warning if the edited commit is contained in a remote branch.\n> Would you consider this sufficient?\n\nIt's likely problematic if the commit appears in the history of any\nother ref, unless they are also planning on rebasing that ref, too\n(which you can't really know, but it is probably better to warn).\n\n-Peff\n"},{"id":"174208","messageId":"20110825031420.GB3465@sigill.intra.peff.net","threadId":"28163","inReplyTo":"1313756753-26498-1-git-send-email-sojka@os.inf.tu-dresden.de","subject":"Re: [PATCH] gitk: Allow commit editing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-25T03:14:20Z","receivedAt":"2011-08-25T03:14:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 19, 2011 at 02:25:53PM +0200, Michal Sojka wrote:\n\n> +    if {[exec git branch -r --contains=$rowmenuid] ne {}} {\n> +\tif {![confirm_popup [mc \"The commit you are going to edit appears in at least one\\\n> +\t\t\t\t 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\nWe frown on using porcelain like \"git branch\" here, because it was not\nmeant to be scriptable (i.e., its behavior may change in future\nreleases).\n\nAs I mentioned elsewhere, I think you really want to see whether the\ncommit is referenced by any other ref, not just branches; if it is, then\nthe rebase is potentially problematic. You can do that with something\nlike:\n\n  us=`git symbolic-ref HEAD`\n  git for-each-ref --format='%(refname)' |\n  while read ref; do\n    test \"$ref\" = $us\" && continue\n    echo \"^$ref\"\n  done |\n  git rev-list HEAD --stdin\n\nThat will give a list of commits found only on HEAD and nowhere else\n(i.e., those which are safe to rebase). If your commit is among them,\nthen it's safe.\n\nSpeaking of which, notice that I used HEAD here. What happens with your\npatch if I do:\n\n  $ git checkout foo\n  $ gitk bar\n\nand select a commit to edit that is not in \"foo\"?\n\n-Peff\n"},{"id":"174226","messageId":"87bovdvdhd.fsf@steelpick.2x.cz","threadId":"28163","inReplyTo":"20110825031420.GB3465@sigill.intra.peff.net","subject":"Re: [PATCH v3] gitk: Allow commit editing","fromName":"Michal Sojka","fromEmail":"sojka@os.inf.tu-dresden.de","sentAt":"2011-08-25T13:15:42Z","receivedAt":"2011-08-25T13:15:42Z","isPatch":true,"sender":{"key":"sojka@os.inf.tu-dresden.de","avatar":"https://gravatar.com/avatar/7aba1f13809fab0717a8c4299caa3ea599413c2028de580d1b736fc82bf10bfa?d=mp&s=160"},"body":"On Thu, 25 Aug 2011, Jeff King wrote:\n> On Fri, Aug 19, 2011 at 02:25:53PM +0200, Michal Sojka wrote:\n> \n> > +    if {[exec git branch -r --contains=$rowmenuid] ne {}} {\n> > +\tif {![confirm_popup [mc \"The commit you are going to edit appears in at least one\\\n> > +\t\t\t\t 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> We frown on using porcelain like \"git branch\" here, because it was not\n> meant to be scriptable (i.e., its behavior may change in future\n> releases).\n> \n> As I mentioned elsewhere, I think you really want to see whether the\n> commit is referenced by any other ref, not just branches; if it is, then\n> the rebase is potentially problematic. You can do that with something\n> like:\n> \n>   us=`git symbolic-ref HEAD`\n>   git for-each-ref --format='%(refname)' |\n>   while read ref; do\n>     test \"$ref\" = $us\" && continue\n>     echo \"^$ref\"\n>   done |\n>   git rev-list HEAD --stdin\n> \n> That will give a list of commits found only on HEAD and nowhere else\n> (i.e., those which are safe to rebase). If your commit is among them,\n> then it's safe.\n\nI implemented something like you ontlined above (see below). Instead of\nrev-listing HEAD, I use the commit id to be edited. This way I don't\nhave to find the commit in the returned list, but I only check whether\nthe list is empty or not.\n\nAdditionally, besides skiping HEAD ref, I also skip refs/stash. Rebasing\nbefore stash should not (I hope) cause any problems and therefore it is\nnot necessary to warn the user.\n\n> Speaking of which, notice that I used HEAD here. What happens with your\n> patch if I do:\n> \n>   $ git checkout foo\n>   $ gitk bar\n> \n> and select a commit to edit that is not in \"foo\"?\n\nI added a check for this. If this is detected, error message is\ndisplayed and no edit is possible.\n\nThe other changes in this version are:\n- added --no-autosquash to rebase invocation\n- fixed indexes of menu entries following the new edit entry.\n\nRegards,\n-Michal\n\n--8<---------------cut here---------------start------------->8---\nI often use gitk to review patches before pushing them out and I would\nlike to have an easy way to edit commits. The current approach with\ncopying the commitid, switching to terminal, invoking git rebase -i,\nediting the commit and switching back to gitk is a way too complicated\njust for changing a single letter in the commit message or removing a\ndebug 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\nBesides editing the commit message, splitting of commits, as described\nin git-rebase(1), is also supported.\n\nThe user is warned if the commit to be edited is contained in another\nref besides the current branch and the stash (e.g. in a remote\nbranch). Additionally, error box is displayed if the user attempts to\nedit a commit not contained in the current branch.\n\nSigned-off-by: Michal Sojka <sojka@os.inf.tu-dresden.de>\n---\n gitk-git/gitk |   71 +++++++++++++++++++++++++++++++++++++++++++++++++++++---\n 1 files changed, 67 insertions(+), 4 deletions(-)\n\ndiff --git a/gitk-git/gitk b/gitk-git/gitk\nindex 4cde0c4..0968efa 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@@ -8428,18 +8429,18 @@ proc rowmenu {x y id} {\n     if {$id ne $nullid && $id ne $nullid2} {\n \tset menu $rowctxmenu\n \tif {$mainhead ne {}} {\n-\t    $menu entryconfigure 7 -label [mc \"Reset %s branch to here\" $mainhead] -state normal\n+\t    $menu entryconfigure 8 -label [mc \"Reset %s branch to here\" $mainhead] -state normal\n \t} else {\n-\t    $menu entryconfigure 7 -label [mc \"Detached head: can't reset\" $mainhead] -state disabled\n+\t    $menu entryconfigure 8 -label [mc \"Detached head: can't reset\" $mainhead] -state disabled\n \t}\n \tif {[info exists markedid] && $markedid ne $id} {\n-\t    $menu entryconfigure 9 -state normal\n \t    $menu entryconfigure 10 -state normal\n \t    $menu entryconfigure 11 -state normal\n+\t    $menu entryconfigure 12 -state normal\n \t} else {\n-\t    $menu entryconfigure 9 -state disabled\n \t    $menu entryconfigure 10 -state disabled\n \t    $menu entryconfigure 11 -state disabled\n+\t    $menu entryconfigure 12 -state disabled\n \t}\n     } else {\n \tset menu $fakerowmenu\n@@ -9102,6 +9103,68 @@ proc cherrypick {} {\n     notbusy cherrypick\n }\n \n+proc rebase_ok {id} {\n+    if {[exec git merge-base $id HEAD] ne $id} {\n+\terror_popup [mc \"You cannot edit commits outside of the current branch.\"]\n+\treturn 0\n+    }\n+    set headref [exec git symbolic-ref HEAD]\n+    set allrefs [split [exec git for-each-ref --format=%(refname)] \"\\n\"]\n+    set otherrefs {}\n+    set otherrefsneg \"\"\n+    foreach ref $allrefs {\n+\tif {$ref eq \"refs/stash\" || $ref eq $headref} continue\n+\tset otherrefsneg \"$otherrefsneg^$ref\\n\"\n+    }\n+\n+    # Check whether some other refs contain the commit to be edited\n+    if {[exec git rev-list --stdin $id << $otherrefsneg] eq {}} {\n+\tif { [confirm_popup [mc \"The commit you are going to edit is contained in another, possibly non-local, ref (e.g. branch or tag).\\\n+\t\t\t\tIt is a bad idea to change a ref that is possibly used by other people. See git-rebase(1) for details.\\n\\n\\\n+\t\t\t\tDo 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 {![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 --no-autosquash $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 discard them (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":"174240","messageId":"20110825173018.GA519@sigill.intra.peff.net","threadId":"28163","inReplyTo":"87bovdvdhd.fsf@steelpick.2x.cz","subject":"Re: [PATCH v3] gitk: Allow commit editing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-08-25T17:30:19Z","receivedAt":"2011-08-25T17:30:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 25, 2011 at 03:15:42PM +0200, Michal Sojka wrote:\n\n> I implemented something like you ontlined above (see below). Instead of\n> rev-listing HEAD, I use the commit id to be edited. This way I don't\n> have to find the commit in the returned list, but I only check whether\n> the list is empty or not.\n\nYeah, that makes sense.  Technically you are also rewriting every commit\n_after_ the commit in question, so you want to be sure that those aren't\nincluded elsewhere, too. But by definition, any ref which includes them\nmust also include the commit in question, so I think your test is\nsufficient.\n\n> Additionally, besides skiping HEAD ref, I also skip refs/stash. Rebasing\n> before stash should not (I hope) cause any problems and therefore it is\n> not necessary to warn the user.\n\nYes, that makes sense to me, too.\n\n> > Speaking of which, notice that I used HEAD here. What happens with your\n> > patch if I do:\n> > \n> >   $ git checkout foo\n> >   $ gitk bar\n> > \n> > and select a commit to edit that is not in \"foo\"?\n> \n> I added a check for this. If this is detected, error message is\n> displayed and no edit is possible.\n\nI was going to suggest that we could actually do the rebase on \"bar\",\nbut that is probably too much complexity hiding behind the user's back\n(not to mention that there are lots of corner cases where figuring out\nthe right branch is tough, like \"gitk <sha1>\").\n\n> The other changes in this version are:\n> - added --no-autosquash to rebase invocation\n\nYeah, that is probably a good idea.\n\n> +    # Check whether some other refs contain the commit to be edited\n> +    if {[exec git rev-list --stdin $id << $otherrefsneg] eq {}} {\n> +\tif { [confirm_popup [mc \"The commit you are going to edit is contained in another, possibly non-local, ref (e.g. branch or tag).\\\n> +\t\t\t\tIt is a bad idea to change a ref that is possibly used by other people. See git-rebase(1) for details.\\n\\n\\\n> +\t\t\t\tDo you want to continue?\"]]} {\n> +\t    return 1\n\nMinor micro-optimization: this can be \"git rev-list -1\". You only care\nif it produces the one commit, so that's sufficient. Without \"-1\", git\nwill keep reporting commits all the way down to the merge base with some\nother ref.\n\n\nAnother question I had while thinking about whether or not this whole\nidea is sane: what happens with conflicts in later commits that are\ncaused by your amended changes? Rebase will complain to the terminal,\nno? Which the user may or may not even see, depending on how they\nstarted gitk.\n\n-Peff\n"},{"id":"174376","messageId":"87fwknaveh.fsf@steelpick.2x.cz","threadId":"28163","inReplyTo":"20110825173018.GA519@sigill.intra.peff.net","subject":"[PATCH v4] gitk: Allow commit editing","fromName":"Michal Sojka","fromEmail":"sojka@os.inf.tu-dresden.de","sentAt":"2011-08-27T12:31:02Z","receivedAt":"2011-08-27T12:31:02Z","isPatch":true,"sender":{"key":"sojka@os.inf.tu-dresden.de","avatar":"https://gravatar.com/avatar/7aba1f13809fab0717a8c4299caa3ea599413c2028de580d1b736fc82bf10bfa?d=mp&s=160"},"body":"On Thu, 25 Aug 2011, Jeff King wrote:\n> On Thu, Aug 25, 2011 at 03:15:42PM +0200, Michal Sojka wrote:\n> > +    # Check whether some other refs contain the commit to be edited\n> > +    if {[exec git rev-list --stdin $id << $otherrefsneg] eq {}} {\n> > +\tif { [confirm_popup [mc \"The commit you are going to edit is contained in another, possibly non-local, ref (e.g. branch or tag).\\\n> > +\t\t\t\tIt is a bad idea to change a ref that is possibly used by other people. See git-rebase(1) for details.\\n\\n\\\n> > +\t\t\t\tDo you want to continue?\"]]} {\n> > +\t    return 1\n> \n> Minor micro-optimization: this can be \"git rev-list -1\". You only care\n> if it produces the one commit, so that's sufficient. Without \"-1\", git\n> will keep reporting commits all the way down to the merge base with some\n> other ref.\n\nHere is a new version with the micro-optimization.\n\nAnother minor change is that this patch now applies to gitk repo\n(http://git.kernel.org/pub/scm/gitk/gitk.git) instead of the main git\nrepo.\n\n-Michal\n\n--8<---------------cut here---------------start------------->8---\nI often use gitk to review patches before pushing them out and I would\nlike to have an easy way to edit commits. The current approach with\ncopying the commitid, switching to terminal, invoking git rebase -i,\nediting the commit and switching back to gitk is a way too complicated\njust for changing a single letter in the commit message or removing a\ndebug 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\nBesides editing the commit message, splitting of commits, as described\nin git-rebase(1), is also supported.\n\nThe user is warned if the commit to be edited is contained in another\nref besides the current branch and the stash (e.g. in a remote\nbranch). Additionally, error box is displayed if the user attempts to\nedit a commit not contained in the current branch.\n\nSigned-off-by: Michal Sojka <sojka@os.inf.tu-dresden.de>\n---\n gitk |   71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---\n 1 files changed, 67 insertions(+), 4 deletions(-)\n\ndiff --git a/gitk b/gitk\nindex a701e0d..1ca2d00 100755\n--- a/gitk\n+++ b/gitk\n@@ -2481,6 +2481,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@@ -8445,18 +8446,18 @@ proc rowmenu {x y id} {\n     if {$id ne $nullid && $id ne $nullid2} {\n \tset menu $rowctxmenu\n \tif {$mainhead ne {}} {\n-\t    $menu entryconfigure 7 -label [mc \"Reset %s branch to here\" $mainhead] -state normal\n+\t    $menu entryconfigure 8 -label [mc \"Reset %s branch to here\" $mainhead] -state normal\n \t} else {\n-\t    $menu entryconfigure 7 -label [mc \"Detached head: can't reset\" $mainhead] -state disabled\n+\t    $menu entryconfigure 8 -label [mc \"Detached head: can't reset\" $mainhead] -state disabled\n \t}\n \tif {[info exists markedid] && $markedid ne $id} {\n-\t    $menu entryconfigure 9 -state normal\n \t    $menu entryconfigure 10 -state normal\n \t    $menu entryconfigure 11 -state normal\n+\t    $menu entryconfigure 12 -state normal\n \t} else {\n-\t    $menu entryconfigure 9 -state disabled\n \t    $menu entryconfigure 10 -state disabled\n \t    $menu entryconfigure 11 -state disabled\n+\t    $menu entryconfigure 12 -state disabled\n \t}\n     } else {\n \tset menu $fakerowmenu\n@@ -9120,6 +9121,68 @@ proc cherrypick {} {\n     notbusy cherrypick\n }\n \n+proc rebase_ok {id} {\n+    if {[exec git merge-base $id HEAD] ne $id} {\n+\terror_popup [mc \"You cannot edit commits outside of the current branch.\"]\n+\treturn 0\n+    }\n+    set headref [exec git symbolic-ref HEAD]\n+    set allrefs [split [exec git for-each-ref --format=%(refname)] \"\\n\"]\n+    set otherrefs {}\n+    set otherrefsneg \"\"\n+    foreach ref $allrefs {\n+\tif {$ref eq \"refs/stash\" || $ref eq $headref} continue\n+\tset otherrefsneg \"$otherrefsneg^$ref\\n\"\n+    }\n+\n+    # Check whether some other refs contain the commit to be edited\n+    if {[exec git rev-list --stdin $id --max-count=1 << $otherrefsneg] eq {}} {\n+\tif { [confirm_popup [mc \"The commit you are going to edit is contained in another, possibly non-local, ref (e.g. branch or tag).\\\n+\t\t\t\tIt is a bad idea to change a ref that is possibly used by other people. See git-rebase(1) for details.\\n\\n\\\n+\t\t\t\tDo 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 {![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 --no-autosquash $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 discard them (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":"175027","messageId":"20110907201011.GF13364@sigill.intra.peff.net","threadId":"28163","inReplyTo":"87fwknaveh.fsf@steelpick.2x.cz","subject":"Re: [PATCH v4] gitk: Allow commit editing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-09-07T20:10:11Z","receivedAt":"2011-09-07T20:10:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 27, 2011 at 02:31:02PM +0200, Michal Sojka wrote:\n\n> Here is a new version with the micro-optimization.\n> \n> Another minor change is that this patch now applies to gitk repo\n> (http://git.kernel.org/pub/scm/gitk/gitk.git) instead of the main git\n> repo.\n\nThanks. This version fixes most of my complaints and looks reasonable\nsane, as long as you accept the idea that starting an interactive rebase\nbehind the scenes is sane.  I'm still not 100% convinced. But then, I am\nnot a big gitk user, either (I like visualizing with it, but I rarely\nwant to do anything interactive). So I'll leave Paul and anyone else to\nargue or make a decision on that count.\n\n-Peff\n"},{"id":"175101","messageId":"20110908205945.GB8091@bloggs.ozlabs.ibm.com","threadId":"28163","inReplyTo":"87fwknaveh.fsf@steelpick.2x.cz","subject":"Re: [PATCH v4] gitk: Allow commit editing","fromName":"Paul Mackerras","fromEmail":"paulus@samba.org","sentAt":"2011-09-08T20:59:45Z","receivedAt":"2011-09-08T20:59:45Z","isPatch":true,"sender":{"key":"paulus@samba.org","avatar":"https://avatars.githubusercontent.com/u/1606439?v=4"},"body":"On Sat, Aug 27, 2011 at 02:31:02PM +0200, Michal Sojka wrote:\n> Here is a new version with the micro-optimization.\n> \n> Another minor change is that this patch now applies to gitk repo\n> (http://git.kernel.org/pub/scm/gitk/gitk.git) instead of the main git\n> repo.\n> \n> -Michal\n> \n> --8<---------------cut here---------------start------------->8---\n> I often use gitk to review patches before pushing them out and I would\n> like to have an easy way to edit commits. The current approach with\n> copying the commitid, switching to terminal, invoking git rebase -i,\n> editing the commit and switching back to gitk is a way too complicated\n> just for changing a single letter in the commit message or removing a\n> debug printf().\n> \n> This patch adds \"Edit this commit\" item to gitk's context menu which\n> invokes interactive rebase in a non-interactive way :-). git gui is\n> used to actually edit the commit.\n> \n> Besides editing the commit message, splitting of commits, as described\n> in git-rebase(1), is also supported.\n> \n> The user is warned if the commit to be edited is contained in another\n> ref besides the current branch and the stash (e.g. in a remote\n> branch). Additionally, error box is displayed if the user attempts to\n> edit a commit not contained in the current branch.\n\nI have to say that this patch makes me pretty nervous.  I can see the\nattractiveness of the feature, but I don't like making gitk\nunresponsive for a potentially long time, i.e. until git gui exits.\nIt may not be clear to users that the reason gitk isn't responding is\nbecause some other git gui window is still running.\n\nAlso, if some subsequent commit no longer applies because of the\nchanges you make to a commit, it's going to abort the rebase\ncompletely and thus lose the changes you made.  That could be\nannoying.\n\nI usually do this by starting a new branch just before the commit I\nwant to change and then use a combination of the cherry-pick menu item\nand git commit --amend.  Maybe something to make that simpler for\nusers would be good, i.e. automate it a bit but still have it be a\nstep-by-step process if necessary.  Part of the problem of course is\nthat neither gitk nor git gui are really designed to be an editing\nenvironment.  In fact you really want an edit/compile/test environment\nso you don't introduce new bugs.\n\nPaul.\n"},{"id":"175435","messageId":"87litsf36r.fsf@steelpick.2x.cz","threadId":"28163","inReplyTo":"20110908205945.GB8091@bloggs.ozlabs.ibm.com","subject":"Re: [PATCH v4] gitk: Allow commit editing","fromName":"Michal Sojka","fromEmail":"sojka@os.inf.tu-dresden.de","sentAt":"2011-09-13T23:11:24Z","receivedAt":"2011-09-13T23:11:24Z","isPatch":true,"sender":{"key":"sojka@os.inf.tu-dresden.de","avatar":"https://gravatar.com/avatar/7aba1f13809fab0717a8c4299caa3ea599413c2028de580d1b736fc82bf10bfa?d=mp&s=160"},"body":"On Thu, 08 Sep 2011, Paul Mackerras wrote:\n> On Sat, Aug 27, 2011 at 02:31:02PM +0200, Michal Sojka wrote:\n> > Here is a new version with the micro-optimization.\n> > \n> > Another minor change is that this patch now applies to gitk repo\n> > (http://git.kernel.org/pub/scm/gitk/gitk.git) instead of the main git\n> > repo.\n> > \n> > -Michal\n> > \n> > --8<---------------cut here---------------start------------->8---\n> > I often use gitk to review patches before pushing them out and I would\n> > like to have an easy way to edit commits. The current approach with\n> > copying the commitid, switching to terminal, invoking git rebase -i,\n> > editing the commit and switching back to gitk is a way too complicated\n> > just for changing a single letter in the commit message or removing a\n> > debug printf().\n> > \n> > This patch adds \"Edit this commit\" item to gitk's context menu which\n> > invokes interactive rebase in a non-interactive way :-). git gui is\n> > used to actually edit the commit.\n> > \n> > Besides editing the commit message, splitting of commits, as described\n> > in git-rebase(1), is also supported.\n> > \n> > The user is warned if the commit to be edited is contained in another\n> > ref besides the current branch and the stash (e.g. in a remote\n> > branch). Additionally, error box is displayed if the user attempts to\n> > edit a commit not contained in the current branch.\n> \n> I have to say that this patch makes me pretty nervous.  I can see the\n> attractiveness of the feature, but I don't like making gitk\n> unresponsive for a potentially long time, i.e. until git gui exits.\n> It may not be clear to users that the reason gitk isn't responding is\n> because some other git gui window is still running.\n\nI understand this. See below for a possible solution.\n \n> Also, if some subsequent commit no longer applies because of the\n> changes you make to a commit, it's going to abort the rebase\n> completely and thus lose the changes you made.  That could be\n> annoying.\n\nAgreed. A solution could be to create a ref called for example\nrefs/gitk/failed-rebase and then abort the rebase together with\ndisplaying the error message explaining what happened. The edited commit\nwould remain visible and a user can manually cherry pick remaining\ncommits and then reset the original branch to this failed-rebase ref.\n\n> I usually do this by starting a new branch just before the commit I\n> want to change and then use a combination of the cherry-pick menu item\n> and git commit --amend.  Maybe something to make that simpler for\n> users would be good, i.e. automate it a bit but still have it be a\n> step-by-step process if necessary.  Part of the problem of course is\n> that neither gitk nor git gui are really designed to be an editing\n> environment.\n\nSo what about the following:\n\n1) When user selects \"Edit commit\", git rebase -i is called, git citool\n   is started on background and things will be set up (I do not know yet\n   how exactly) so that same callback is called when git citool exits.\n\n2) gitk updates the list of commits and marks the current (detached)\n   head by a tag (colored box) saying that there is rebase in progress.\n   The same tag would appear if gitk is executed manually during\n   interactive rebase.\n\n3) When git citool exits (or when you explicitly select something from\n   rebase tag's context menu), rebase would continue.\n\n> In fact you really want an edit/compile/test environment so you don't\n> introduce new bugs.\n\nAnother possibility would be to allow only editing of the commit\nmessage. Then you don't need compile/test steps and the rebase should\nnot fail due to conflicts.\n\nI do not think I would like this limitation, but it would certainly be\nbetter then nothing.\n\n-Michal\n"}]}