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